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

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

[Giuseppe Bilotta]

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

This is trivially fixed by moving the conditional addition of the
signoff from parse_mail() to the caller (am_run()), after either of the
parse_mail*() functions were called.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 Documentation/git-rebase.txt | 5 +++++
 builtin/am.c                 | 6 +++---
 git-rebase.sh                | 3 ++-
 3 files changed, 10 insertions(+), 4 deletions(-)

As suggested by Ævar, it's [no-]signoff, not [no]-signoff.

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e6883..e6f0b93337 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -385,6 +385,11 @@ have the long commit hash prepended to the format.
 	Recreate merge commits instead of flattening the history by replaying
 	commits a merge commit introduces. Merge conflict resolutions or manual
 	amendments to merge commits are not preserved.
+
+--signoff::
+	This flag is passed to 'git am' to sign off all the rebased
+	commits (see linkgit:git-am[1]).
+
 +
 This uses the `--interactive` machinery internally, but combining it
 with the `--interactive` option explicitly is generally not a good
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);
 		}
diff --git a/git-rebase.sh b/git-rebase.sh
index 48d7c5ded4..cce72d8494 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -34,6 +34,7 @@ root!              rebase all reachable commits up to the root(s)
 autosquash         move commits that begin with squash!/fixup! under -i
 committer-date-is-author-date! passed to 'git am'
 ignore-date!       passed to 'git am'
+[no-]signoff!      passed to 'git am'
 whitespace=!       passed to 'git apply'
 ignore-whitespace! passed to 'git apply'
 C=!                passed to 'git apply'
@@ -321,7 +322,7 @@ do
 	--ignore-whitespace)
 		git_am_opt="$git_am_opt $1"
 		;;
-	--committer-date-is-author-date|--ignore-date)
+	--committer-date-is-author-date|--ignore-date|--signoff|--no-signoff)
 		git_am_opt="$git_am_opt $1"
 		force_rebase=t
 		;;
-- 
2.12.2.765.g845dc5dc05

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

[Junio C Hamano]

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

>  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);
> -
>  	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().

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

[Giuseppe Bilotta]

On Sat, Apr 15, 2017 at 11:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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-)

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

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

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

[Junio C Hamano]

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

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

For a new feature (especially with this small implementation), it is
best to have the test in the same commit.  

We often use the "start with expect_failure, update the code while
flipping _failure to _success" pattern but that is primarily
suitable for bugfixes.