Re: [PATCHv3] rebase: pass --[no-]signoff option to git am

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Apr 15, 2017, 09:36 UTC
Message-ID: <CAOxFTcwDrYvg5Nf1w9SfmM=Nt7XYsJPhKSYkJzMC0123EY94Aw@mail.gmail.com>
In-Reply-To: <xmqqefwum3mh.fsf@gitster.mtv.corp.google.com>

On Sat, Apr 15, 2017 at 11:17 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> This makes it easy to sign off a whole patchset before submission.
>>
>> To make things work, we also fix a design issue in git-am that made it
>> ignore the signoff option during rebase (specifically, signoff was
>> handled in parse_mail(), but not in parse_mail_rebasing()).
>
> I doubt that the above implementation detail in the code is "a
> design issue"; it is a logical consequence of a design whose
> "rebase" never passes "--signoff" down to underlying "am", so it is
> understandable that whoever wants to pass "--signoff" thru during
> the rebase needs to update the implementation, but I do not think it
> is fair to call that "an issue".

Good point. It's an issue now that we want to be able to pass signoff, but when the split was introduced it most definitely wasn't 8-)

Show 7 quoted lines

>>  Documentation/git-rebase.txt | 5 +++++
>>  builtin/am.c                 | 6 +++---
>>  git-rebase.sh                | 3 ++-
>>  3 files changed, 10 insertions(+), 4 deletions(-)
>
> We need new tests for "git rebase --signoff" that makes sure this
> works as expected and only when it should.

Would the norm in this case be to introduce the test in the same commit, or in a previous commit (as in: this is the feature we want to implement, it obviously doesn't work now, but the next commit will fix that), or in a subsequent one?

Show 28 quoted lines

>> diff --git a/builtin/am.c b/builtin/am.c
>> index f7a7a971fb..d072027b5a 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const char *mail)
>>       strbuf_addbuf(&msg, &mi.log_message);
>>       strbuf_stripspace(&msg, 0);
>>
>> -     if (state->signoff)
>> -             am_signoff(&msg);
>> -
>>       assert(!state->author_name);
>>       state->author_name = strbuf_detach(&author_name, NULL);
>>
>> @@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)
>>                       if (skip)
>>                               goto next; /* mail should be skipped */
>>
>> +                     if (state->signoff)
>> +                             am_append_signoff(state);
>> +
>>                       write_author_script(state);
>>                       write_commit_msg(state);
>>               }
>
> This removes the last direct caller to am_signoff().  It may be
> worth considering to remove the function and move its body to its
> only internal caller am_append_signoff().

Good point. It becomes a bit bigger change though, so I'll probably split it off in a separate commit now.

-- 
Giuseppe "Oblomov" Bilotta