Re: [PATCH] am: match --signoff to the original scripted version

[Junio C Hamano <gitster@pobox.com>]

Paul Tan <pyokagan@gmail.com> 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.