Paul Tan <[email protected]> writes:
> s/reimplementated/reimplemented/ ?
> s/resulting an/resulting in an/ ?
> s/extra blank/extra blank line/ ?
Thanks.
>> +static void am_signoff(struct strbuf *sb)
>> +{
>
> Hmm, okay, but now we have two similarly named functions am_signoff()
> and am_append_signoff() which both do nearly similar things, the only
> difference being am_signoff() operates on a strbuf while
> am_append_signoff() operates on the "msg" char* field in the am_state,
> which seems a bit iffy to me.
I do recall advising you not to overuse strbuf in am_state, and
specifically not to use strbuf for a field like author_name, and the
criteria to tell why a field should not be a strbuf in am_state is
if the code used its strbuf-ness only because it is initially read
with strbuf_read_file(), but afterwards it is necessary to use the
field as a strbuf because the field is not modified later and is not
passed to a helper function that takes a strbuf.
I think the final code ended up following that piece of advice a bit
too aggressively. The <msg, msg_len> pair in am_state did need to
be modified with sign-off, and it was done by passing it as a strbuf
to append_signoff() in the code we are fixing here; keeping msg
field that you wrote as a strbuf in the original would have made
am_append_signoff() unnecessary.
But this patch you are commenting on is purely for regression fix,
and I didn't want to change other things like data representation at
the same time.
>> + /* Does it end with our own sign-off? */
>> + strbuf_addf(&mine, "\n%s%s\n",
>> + sign_off_header,
>> + fmt_name(getenv("GIT_COMMITTER_NAME"),
>> + getenv("GIT_COMMITTER_EMAIL")));
>
> Maybe use git_committer_info() here?
Perhaps, but I wanted to make sure I am doing the same thing as the
codepath of sequencer.c::append_signoff(), which the original ended
up calling. git_committer_info() does way more than that, no?
>> + if (mine.len < sb->len &&
>> + !strcmp(mine.buf, sb->buf + sb->len - mine.len))
>
> Perhaps use ends_with()?
This caller already _knows_ how long the sb->buf string is; it is
pointless to have ends_with() run strlen() on it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
