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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2017, 09:17 UTC
Message-ID: <xmqqefwum3mh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170414225713.29710-1-giuseppe.bilotta@gmail.com>

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Show 5 quoted lines

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

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

Show 24 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().