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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2015, 17:24 UTC
Message-ID: <xmqqd1xvbgw3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACRoPnTUzeF=RUVv_0+2Ej422Vh-NH+KpYaKH0VKNmGbuVQ_8g@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

> s/reimplementated/reimplemented/ ?
> s/resulting an/resulting in an/ ?
> s/extra blank/extra blank line/ ?

Thanks.

Show 8 quoted lines

>> +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.

Show 7 quoted lines

>> +       /* 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.