Re: [PATCHv3] rebase: pass --[no-]signoff option to git am
- Date: Sat, 15 Apr 2017 02:17:26 -0700
- From: Junio C Hamano <gitster@xxxxxxxxx>
- Subject: Re: [PATCHv3] rebase: pass --[no-]signoff option to git am
Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> 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".
> 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.
> 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);
> 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);
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().