threads / discuss / 45698

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

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

## tl;dr

4 messages between Apr 14, 2017 and Apr 15, 2017.

replies: 3people: 2as markdown or json

Giuseppe Bilotta· Apr 14, 2017, 22:57 UTC · lore
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
Junio C Hamano· Apr 15, 2017, 09:17 UTC · re: Giuseppe Bilotta · lore

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

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

Giuseppe Bilotta· Apr 15, 2017, 09:36 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Apr 15, 2017, 10:03 UTC · re: Giuseppe Bilotta · lore

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

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
Show 7 quoted lines
>> 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.

← back to recent threads