[PATCH 1/2] rebase: support --signoff with implicit rebase

[PATCH 1/2] rebase: support --signoff with implicit rebase

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This allows one to run 'git rebase --exec "make check" --signoff'
which is useful when preparing a patch series for publication and is
more convenient than doing the signoff with another --exec command.
This change also allows --root without --onto to work with --signoff
as well (--root with --onto was already supported). Note that the
failing test is due to a bug in 'rebase --root' when the root commit
is empty which will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase--interactive.sh |  6 +++---
 git-rebase.sh              | 26 +++++++++++++++++++++++++-
 sequencer.c                |  6 ++++++
 t/t3428-rebase-signoff.sh  | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 81c5b42875..4ea54fc1c4 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -283,7 +283,7 @@ pick_one () {
 		pick_one_preserving_merges "$@" && return
 	output eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-			"$strategy_args" $empty_args $ff "$@"
+			$signoff "$strategy_args" $empty_args $ff "$@"
 
 	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
 	# previous task so this commit is not lost.
@@ -525,10 +525,10 @@ do_pick () {
 		# resolve before manually running git commit --amend then git
 		# rebase --continue.
 		git commit --allow-empty --allow-empty-message --amend \
-			   --no-post-rewrite -n -q -C $sha1 &&
+			   --no-post-rewrite -n -q -C $sha1 $signoff &&
 			pick_one -n $sha1 &&
 			git commit --allow-empty --allow-empty-message \
-				   --amend --no-post-rewrite -n -q -C $sha1 \
+				   --amend --no-post-rewrite -n -q -C $sha1 $signoff \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				   die_with_patch $sha1 "$(eval_gettext "Could not apply \$sha1... \$rest")"
 	else
diff --git a/git-rebase.sh b/git-rebase.sh
index b353c33d41..40301756be 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -91,6 +91,7 @@ preserve_merges=
 autosquash=
 keep_empty=
 allow_empty_message=
+signoff=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
@@ -120,6 +121,10 @@ read_basic_state () {
 		allow_rerere_autoupdate="$(cat "$state_dir"/allow_rerere_autoupdate)"
 	test -f "$state_dir"/gpg_sign_opt &&
 		gpg_sign_opt="$(cat "$state_dir"/gpg_sign_opt)"
+	test -f "$state_dir"/signoff && {
+		signoff="$(cat "$state_dir"/signoff)"
+		force_rebase=t
+	}
 }
 
 write_basic_state () {
@@ -134,6 +139,7 @@ write_basic_state () {
 	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
 		"$state_dir"/allow_rerere_autoupdate
 	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
 }
 
 output () {
@@ -329,7 +335,13 @@ do
 	--ignore-whitespace)
 		git_am_opt="$git_am_opt $1"
 		;;
-	--committer-date-is-author-date|--ignore-date|--signoff|--no-signoff)
+	--signoff)
+		signoff=--signoff
+		;;
+	--no-signoff)
+		signoff=
+		;;
+	--committer-date-is-author-date|--ignore-date)
 		git_am_opt="$git_am_opt $1"
 		force_rebase=t
 		;;
@@ -459,6 +471,18 @@ then
 	git_format_patch_opt="$git_format_patch_opt --progress"
 fi
 
+if test -n "$signoff"
+then
+	test "$interactive_rebase" = explicit &&
+		die "$(gettext "error: interactive rebase does not support --signoff")"
+	test "$type" = merge &&
+		die "$(gettext "error: merge rebase does not support --signoff")"
+	test -n "$preserve_merges" &&
+		die "$(gettext "error: cannot combine '--signoff' with '--preserve-merges'")"
+	git_am_opt="$git_am_opt $signoff"
+	force_rebase=t
+fi
+
 if test -z "$rebase_root"
 then
 	case "$#" in
diff --git a/sequencer.c b/sequencer.c
index e9baaf59bd..311569e37d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -127,6 +127,7 @@ static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
 static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
 static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
 static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
@@ -2035,6 +2036,11 @@ static int read_populate_opts(struct replay_opts *opts)
 		if (file_exists(rebase_path_verbose()))
 			opts->verbose = 1;
 
+		if (file_exists(rebase_path_signoff())) {
+			opts->allow_ff = 0;
+			opts->signoff = 1;
+		}
+
 		read_strategy_opts(opts, &buf);
 		strbuf_release(&buf);
 
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 2afb564701..2ff7f534e3 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -12,6 +12,13 @@ cat >file <<EOF
 a
 EOF
 
+# Expected commit message for initial commit after rebase --signoff
+cat >expected-initial-signed <<EOF
+Initial empty commit
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+
 # Expected commit message after rebase --signoff
 cat >expected-signed <<EOF
 first
@@ -43,4 +50,29 @@ test_expect_success 'rebase --no-signoff does not add a sign-off line' '
 	test_cmp expected-unsigned actual
 '
 
+test_expect_success 'rebase --exec --signoff adds a sign-off line' '
+	test_when_finished "rm exec" &&
+	git commit --amend -m "first" &&
+	git rebase --exec "touch exec" --signoff HEAD^ &&
+	test_path_is_file exec &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_failure 'rebase --root --signoff adds a sign-off line' '
+	git commit --amend -m "first" &&
+	git rebase --root --keep-empty --signoff &&
+	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-initial-signed actual &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_success 'rebase -i --signoff fails' '
+	test_must_fail git rebase -i --signoff HEAD^
+'
+
+test_expect_success 'rebase -m --signoff fails' '
+	test_must_fail git rebase -m --signoff HEAD^
+'
 test_done
-- 
2.16.2

[PATCH 2/2] rebase --root -k: fix when root commit is empty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When the root commit was empty it was being pruned by the
--cherry-pick option passed to rev-parse. This is because when --onto
is omitted rebase creates an empty commit (which it later amends) for
the new root commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase--interactive.sh | 7 ++++++-
 git-rebase.sh              | 1 +
 t/t3428-rebase-signoff.sh  | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4ea54fc1c4..3ad74fc57c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -894,7 +894,12 @@ then
 	revisions=$upstream...$orig_head
 	shortrevisions=$shortupstream..$shorthead
 else
-	revisions=$onto...$orig_head
+	if test -n "$squash_onto"
+	then
+		revisions=$orig_head
+	else
+		revisions=$onto...$orig_head
+	fi
 	shortrevisions=$shorthead
 fi
 if test t != "$preserve_merges"
diff --git a/git-rebase.sh b/git-rebase.sh
index 40301756be..30b8eaf489 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -61,6 +61,7 @@ $(gettext 'Resolve all conflicts manually, mark them as resolved with
 You can instead skip this commit: run "git rebase --skip".
 To abort and get back to the state before "git rebase", run "git rebase --abort".')
 "
+squash_onto=
 unset onto
 unset restrict_revision
 cmd=
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 2ff7f534e3..90ca6636d5 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -59,7 +59,7 @@ test_expect_success 'rebase --exec --signoff adds a sign-off line' '
 	test_cmp expected-signed actual
 '
 
-test_expect_failure 'rebase --root --signoff adds a sign-off line' '
+test_expect_success 'rebase --root --signoff adds a sign-off line' '
 	git commit --amend -m "first" &&
 	git rebase --root --keep-empty --signoff &&
 	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&
-- 
2.16.2

Re: [PATCH 1/2] rebase: support --signoff with implicit rebase

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This allows one to run 'git rebase --exec "make check" --signoff'
> which is useful when preparing a patch series for publication and is
> more convenient than doing the signoff with another --exec command.
> This change also allows --root without --onto to work with --signoff
> as well (--root with --onto was already supported). Note that the
> failing test is due to a bug in 'rebase --root' when the root commit
> is empty which will be fixed in the next commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

How important is the word "implicit" in the title?  Is it your
intention to actively ignore --signoff when we fall into the
rebase--interactive codepath explicitly?

I offhand do not think of a strong reason why it is a bad idea to
run "git rebase -i --signoff", turn a few "pick" to either "reword"
or "edit", and then expect that the editor to edit log messages for
these commits to add your sign-off when you start editing them.
The "pick"s that are left as-is would also turn into doing an
otherwise no-op "commit --amend -s", I guess.

If you are teaching --signoff to the whole of "rebase--interactive",
then "git rebase --help" needs a bit of update.

    --signoff::
            This flag is passed to 'git am' to sign off all the rebased
            commits (see linkgit:git-am[1]). Incompatible with the
            --interactive option.

Re: [PATCH 1/2] rebase: support --signoff with implicit rebase

[Johannes Schindelin]

Hi Junio,

On Wed, 14 Mar 2018, Junio C Hamano wrote:

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > This allows one to run 'git rebase --exec "make check" --signoff'
> > which is useful when preparing a patch series for publication and is
> > more convenient than doing the signoff with another --exec command.
> > This change also allows --root without --onto to work with --signoff
> > as well (--root with --onto was already supported). Note that the
> > failing test is due to a bug in 'rebase --root' when the root commit
> > is empty which will be fixed in the next commit.
> >
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> 
> How important is the word "implicit" in the title?  Is it your
> intention to actively ignore --signoff when we fall into the
> rebase--interactive codepath explicitly?

The patch makes the explicit case fail ;-)

> I offhand do not think of a strong reason why it is a bad idea to
> run "git rebase -i --signoff", turn a few "pick" to either "reword"
> or "edit", and then expect that the editor to edit log messages for
> these commits to add your sign-off when you start editing them.
> The "pick"s that are left as-is would also turn into doing an
> otherwise no-op "commit --amend -s", I guess.
> 
> If you are teaching --signoff to the whole of "rebase--interactive",
> then "git rebase --help" needs a bit of update.
> 
>     --signoff::
>             This flag is passed to 'git am' to sign off all the rebased
>             commits (see linkgit:git-am[1]). Incompatible with the
>             --interactive option.

Good point. I'd actually be in favor of having all rebase modes support
--signoff, but I do not want to pile onto Phillip's workload.

Ciao,
Dscho

Re: [PATCH 1/2] rebase: support --signoff with implicit rebase

[Johannes Schindelin]

Hi Phillip,

On Wed, 14 Mar 2018, Phillip Wood wrote:

> diff --git a/git-rebase.sh b/git-rebase.sh
> index b353c33d41..40301756be 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -459,6 +471,18 @@ then
>  	git_format_patch_opt="$git_format_patch_opt --progress"
>  fi
>  
> +if test -n "$signoff"
> +then
> +	test "$interactive_rebase" = explicit &&
> +		die "$(gettext "error: interactive rebase does not support --signoff")"
> +	test "$type" = merge &&
> +		die "$(gettext "error: merge rebase does not support --signoff")"
> +	test -n "$preserve_merges" &&
> +		die "$(gettext "error: cannot combine '--signoff' with '--preserve-merges'")"
> +	git_am_opt="$git_am_opt $signoff"
> +	force_rebase=t
> +fi

I wonder whether we can have this change as a separate commit? Otherwise
we would lump that change (--interactive --signoff was previously allowed
but the --signoff was simply ignored) with the other changes...

As I mentioned in my reply to Junio's comment, it'd be awesome if
--interactive --signoff was supported (and likewise --merge --signoff),
but it feels like an undue feature request to be dumped on you, so I'm
fine with the patch series simply erroring out on those combinations.

(I don't care about --preserve-merges anymore, as everybody knows by now.)

Ciao,
Dscho

Re: [PATCH 2/2] rebase --root -k: fix when root commit is empty

[Johannes Schindelin]

Hi Phillip,

On Wed, 14 Mar 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When the root commit was empty it was being pruned by the
> --cherry-pick option passed to rev-parse. This is because when --onto
> is omitted rebase creates an empty commit (which it later amends) for
> the new root commit.

This will have ramifications on the upcoming patches on top of my
--recreate-merges patches that introduce support for running -i --root
directly through the sequencer. But from your patch, it looks as if things
should Just Work.

FWIW the patch in question is currently here (will be rebased frequently):
https://github.com/dscho/git/commit/147e8f3c2bf5f07708d76efd9da51cbd0eb5958c

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 4ea54fc1c4..3ad74fc57c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -894,7 +894,12 @@ then
>  	revisions=$upstream...$orig_head
>  	shortrevisions=$shortupstream..$shorthead
>  else
> -	revisions=$onto...$orig_head
> +	if test -n "$squash_onto"
> +	then
> +		revisions=$orig_head
> +	else
> +		revisions=$onto...$orig_head
> +	fi

Before anybody else can post the suggestion: this could be written more
compactly as

	revisions=${squash_onto:+$onto...}$orig_head

As it would not only be more compact, but also a lot less readable, I
would *also* like to point out that I prefer your version.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 40301756be..30b8eaf489 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -61,6 +61,7 @@ $(gettext 'Resolve all conflicts manually, mark them as resolved with
>  You can instead skip this commit: run "git rebase --skip".
>  To abort and get back to the state before "git rebase", run "git rebase --abort".')
>  "
> +squash_onto=
>  unset onto
>  unset restrict_revision
>  cmd=

This is a bug fix. Maybe we can pull it out into its own commit? Commit
message would be something like this:

	rebase --root: stop assuming that squash_onto is unset

	When the user set the environment variable `squash_onto`, the
	`rebase` command would erroneously assume that the user passed the
	option `--root`.

> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
> index 2ff7f534e3..90ca6636d5 100755
> --- a/t/t3428-rebase-signoff.sh
> +++ b/t/t3428-rebase-signoff.sh
> @@ -59,7 +59,7 @@ test_expect_success 'rebase --exec --signoff adds a sign-off line' '
>  	test_cmp expected-signed actual
>  '
>  
> -test_expect_failure 'rebase --root --signoff adds a sign-off line' '
> +test_expect_success 'rebase --root --signoff adds a sign-off line' '
>  	git commit --amend -m "first" &&
>  	git rebase --root --keep-empty --signoff &&
>  	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&

Hehe... I guess this fixes that test case for a surprising reason: it is
not that all of a sudden, rebase --root respects --signoff. The reason it
works now is that an "empty" root commit is now rebased correctly.

The test case is simple enough to pass my "can a regression be debugged
easily, even by somebody else than the original author" test, so I do not
mind at all lumping the two issues (--root respects --signoff, and --root
respects empty root commits) into one test case, as that will make the
test suite run faster.

Please note that I do not feel all that strongly about my suggested
modifications to pull out changes from 1/2 and 2/2 into their own
respective commits. If you have the time, and if you feel that there is
merit to my suggestions, please do it. Otherwise, please feel free not to
do it ;-)

So: ACK from me.

Ciao,
Dscho

Re: [PATCH 1/2] rebase: support --signoff with implicit rebase

[Phillip Wood]

On 15/03/18 10:18, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 14 Mar 2018, Phillip Wood wrote:
> 
>> diff --git a/git-rebase.sh b/git-rebase.sh
>> index b353c33d41..40301756be 100755
>> --- a/git-rebase.sh
>> +++ b/git-rebase.sh
>> @@ -459,6 +471,18 @@ then
>>  	git_format_patch_opt="$git_format_patch_opt --progress"
>>  fi
>>  
>> +if test -n "$signoff"
>> +then
>> +	test "$interactive_rebase" = explicit &&
>> +		die "$(gettext "error: interactive rebase does not support --signoff")"
>> +	test "$type" = merge &&
>> +		die "$(gettext "error: merge rebase does not support --signoff")"
>> +	test -n "$preserve_merges" &&
>> +		die "$(gettext "error: cannot combine '--signoff' with '--preserve-merges'")"
>> +	git_am_opt="$git_am_opt $signoff"
>> +	force_rebase=t
>> +fi
> 
> I wonder whether we can have this change as a separate commit? Otherwise
> we would lump that change (--interactive --signoff was previously allowed
> but the --signoff was simply ignored) with the other changes...
> 
> As I mentioned in my reply to Junio's comment, it'd be awesome if
> --interactive --signoff was supported (and likewise --merge --signoff),
> but it feels like an undue feature request to be dumped on you, so I'm
> fine with the patch series simply erroring out on those combinations.

I'll have a look, if it's not too much work I'll do that, it would be
nice to have --signoff better supported.

> (I don't care about --preserve-merges anymore, as everybody knows by now.)
> 
> Ciao,
> Dscho
> 

Re: [PATCH 2/2] rebase --root -k: fix when root commit is empty

[Phillip Wood]

On 14/03/18 11:11, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When the root commit was empty it was being pruned by the
> --cherry-pick option passed to rev-parse. This is because when --onto
> is omitted rebase creates an empty commit (which it later amends) for
> the new root commit.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  git-rebase--interactive.sh | 7 ++++++-
>  git-rebase.sh              | 1 +
>  t/t3428-rebase-signoff.sh  | 2 +-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 4ea54fc1c4..3ad74fc57c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -894,7 +894,12 @@ then
>  	revisions=$upstream...$orig_head
>  	shortrevisions=$shortupstream..$shorthead
>  else
> -	revisions=$onto...$orig_head
> +	if test -n "$squash_onto"
> +	then
> +		revisions=$orig_head
> +	else
> +		revisions=$onto...$orig_head
> +	fi
>  	shortrevisions=$shorthead
>  fi
>  if test t != "$preserve_merges"

On reflection I'm not sure this is the best way to fix the problem. This
is a specific instance of a wider problem where rebase -k does not
preserve empty commits if there is an empty commit upstream (see
t3421:rebase -i --keep-empty keeps empty even if already in upstream). I
think the way to solve it is to get two lists of revs, the first with
--cherry-pick and the second without and use all the commits from the
first list and the empty ones from the second. I'll have a look and send
a reroll.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 40301756be..30b8eaf489 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -61,6 +61,7 @@ $(gettext 'Resolve all conflicts manually, mark them as resolved with
>  You can instead skip this commit: run "git rebase --skip".
>  To abort and get back to the state before "git rebase", run "git rebase --abort".')
>  "
> +squash_onto=
>  unset onto
>  unset restrict_revision
>  cmd=
> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
> index 2ff7f534e3..90ca6636d5 100755
> --- a/t/t3428-rebase-signoff.sh
> +++ b/t/t3428-rebase-signoff.sh
> @@ -59,7 +59,7 @@ test_expect_success 'rebase --exec --signoff adds a sign-off line' '
>  	test_cmp expected-signed actual
>  '
>  
> -test_expect_failure 'rebase --root --signoff adds a sign-off line' '
> +test_expect_success 'rebase --root --signoff adds a sign-off line' '
>  	git commit --amend -m "first" &&
>  	git rebase --root --keep-empty --signoff &&
>  	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&
> 

Re: [PATCH 1/2] rebase: support --signoff with implicit rebase

[Johannes Schindelin]

Hi Phillip,

On Thu, 15 Mar 2018, Phillip Wood wrote:

> On 15/03/18 10:18, Johannes Schindelin wrote:
>
> > As I mentioned in my reply to Junio's comment, it'd be awesome if
> > --interactive --signoff was supported (and likewise --merge
> > --signoff), but it feels like an undue feature request to be dumped on
> > you, so I'm fine with the patch series simply erroring out on those
> > combinations.
> 
> I'll have a look, if it's not too much work I'll do that, it would be
> nice to have --signoff better supported.

Thank you so much!
Dscho

[PATCH v2 0/3] rebase: extend --signoff support

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Thanks for the feedback on v1, based on that I've added support for
--interacive --signoff and --merge --signoff. I've split this series
so the fixes from patch two in the last verion are now in a separate
series [1]. The tests in this version depends on those fixes. The
third patch is new.

[1] https://public-inbox.org/git/20180320100315.15261-1-phillip.wood@talktalk.net/T/#t

Phillip Wood (3):
  rebase: extend --signoff support
  rebase -p: error out if --signoff is given
  rebase --keep-empty: always use interactive rebase

 Documentation/git-rebase.txt      |  7 ++--
 git-rebase--am.sh                 | 79 ++++++++++++++++-----------------------
 git-rebase--interactive.sh        |  6 +--
 git-rebase--merge.sh              |  2 +-
 git-rebase.sh                     | 27 ++++++++++++-
 sequencer.c                       |  8 +++-
 t/t3421-rebase-topology-linear.sh |  4 +-
 t/t3428-rebase-signoff.sh         | 38 +++++++++++++++++++
 8 files changed, 114 insertions(+), 57 deletions(-)

-- 
2.16.2

[PATCH v2 1/3] rebase: extend --signoff support

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Allow --signoff to be used with --interactive and --merge. In
interactive mode only commits marked to be picked, edited or reworded
will be signed off.

The main motivation for this patch was to allow one to run 'git rebase
--exec "make check" --signoff' which is useful when preparing a patch
series for publication and is more convenient than doing the signoff
with another --exec command.

This change also allows --root without --onto to work with --signoff
as well (--root with --onto was already supported).

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    changes since v1:
     - added support for --signoff with --interactive and --merge
     - split out the --preserve-merges test into the next commit
     - updated the documentation for --signoff
     - reworded commit message to reflect above changes

 Documentation/git-rebase.txt |  7 ++++---
 git-rebase--interactive.sh   |  6 +++---
 git-rebase--merge.sh         |  2 +-
 git-rebase.sh                | 20 +++++++++++++++++++-
 sequencer.c                  |  8 +++++++-
 t/t3428-rebase-signoff.sh    | 38 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3277ca1432..dd852068b1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -364,9 +364,10 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
 	Incompatible with the --interactive option.
 
 --signoff::
-	This flag is passed to 'git am' to sign off all the rebased
-	commits (see linkgit:git-am[1]). Incompatible with the
-	--interactive option.
+	Add a Signed-off-by: trailer to all the rebased commits. Note
+	that if `--interactive` is given then only commits marked to be
+	picked, edited or reworded will have the trailer added. Incompatible
+	with the `--preserve-merges` option.
 
 -i::
 --interactive::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfeac..104de328ee 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -285,7 +285,7 @@ pick_one () {
 		pick_one_preserving_merges "$@" && return
 	output eval git cherry-pick $allow_rerere_autoupdate $allow_empty_message \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-			"$strategy_args" $empty_args $ff "$@"
+			$signoff "$strategy_args" $empty_args $ff "$@"
 
 	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
 	# previous task so this commit is not lost.
@@ -527,10 +527,10 @@ do_pick () {
 		# resolve before manually running git commit --amend then git
 		# rebase --continue.
 		git commit --allow-empty --allow-empty-message --amend \
-			   --no-post-rewrite -n -q -C $sha1 &&
+			   --no-post-rewrite -n -q -C $sha1 $signoff &&
 			pick_one -n $sha1 &&
 			git commit --allow-empty --allow-empty-message \
-				   --amend --no-post-rewrite -n -q -C $sha1 \
+				   --amend --no-post-rewrite -n -q -C $sha1 $signoff \
 				   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
 				   die_with_patch $sha1 "$(eval_gettext "Could not apply \$sha1... \$rest")"
 	else
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index ceb715453c..368b63671d 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -28,7 +28,7 @@ continue_merge () {
 	if ! git diff-index --quiet --ignore-submodules HEAD --
 	then
 		if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} $allow_empty_message \
-			--no-verify -C "$cmt"
+			$signoff --no-verify -C "$cmt"
 		then
 			echo "Commit failed, please do not call \"git commit\""
 			echo "directly, but instead do one of the following: "
diff --git a/git-rebase.sh b/git-rebase.sh
index a1f6e5de6a..5d854657a7 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -92,6 +92,7 @@ preserve_merges=
 autosquash=
 keep_empty=
 allow_empty_message=
+signoff=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)	gpg_sign_opt=-S ;;
@@ -121,6 +122,10 @@ read_basic_state () {
 		allow_rerere_autoupdate="$(cat "$state_dir"/allow_rerere_autoupdate)"
 	test -f "$state_dir"/gpg_sign_opt &&
 		gpg_sign_opt="$(cat "$state_dir"/gpg_sign_opt)"
+	test -f "$state_dir"/signoff && {
+		signoff="$(cat "$state_dir"/signoff)"
+		force_rebase=t
+	}
 }
 
 write_basic_state () {
@@ -135,6 +140,7 @@ write_basic_state () {
 	test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" > \
 		"$state_dir"/allow_rerere_autoupdate
 	test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" > "$state_dir"/gpg_sign_opt
+	test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
 }
 
 output () {
@@ -331,7 +337,13 @@ do
 	--ignore-whitespace)
 		git_am_opt="$git_am_opt $1"
 		;;
-	--committer-date-is-author-date|--ignore-date|--signoff|--no-signoff)
+	--signoff)
+		signoff=--signoff
+		;;
+	--no-signoff)
+		signoff=
+		;;
+	--committer-date-is-author-date|--ignore-date)
 		git_am_opt="$git_am_opt $1"
 		force_rebase=t
 		;;
@@ -465,6 +477,12 @@ then
 	git_format_patch_opt="$git_format_patch_opt --progress"
 fi
 
+if test -n "$signoff"
+then
+	git_am_opt="$git_am_opt $signoff"
+	force_rebase=t
+fi
+
 if test -z "$rebase_root"
 then
 	case "$#" in
diff --git a/sequencer.c b/sequencer.c
index 091bd6bda5..68683edfc1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -127,6 +127,7 @@ static GIT_PATH_FUNC(rebase_path_rewritten_pending,
 static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
 static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
 static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
+static GIT_PATH_FUNC(rebase_path_signoff, "rebase-merge/signoff")
 static GIT_PATH_FUNC(rebase_path_head_name, "rebase-merge/head-name")
 static GIT_PATH_FUNC(rebase_path_onto, "rebase-merge/onto")
 static GIT_PATH_FUNC(rebase_path_autostash, "rebase-merge/autostash")
@@ -1605,7 +1606,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 		}
 	}
 
-	if (opts->signoff)
+	if (opts->signoff && !is_fixup(command))
 		append_signoff(&msgbuf, 0, 0);
 
 	if (is_rebase_i(opts) && write_author_script(msg.message) < 0)
@@ -2044,6 +2045,11 @@ static int read_populate_opts(struct replay_opts *opts)
 		if (file_exists(rebase_path_verbose()))
 			opts->verbose = 1;
 
+		if (file_exists(rebase_path_signoff())) {
+			opts->allow_ff = 0;
+			opts->signoff = 1;
+		}
+
 		read_strategy_opts(opts, &buf);
 		strbuf_release(&buf);
 
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
index 2afb564701..f6993b7e14 100755
--- a/t/t3428-rebase-signoff.sh
+++ b/t/t3428-rebase-signoff.sh
@@ -12,6 +12,13 @@ cat >file <<EOF
 a
 EOF
 
+# Expected commit message for initial commit after rebase --signoff
+cat >expected-initial-signed <<EOF
+Initial empty commit
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+
 # Expected commit message after rebase --signoff
 cat >expected-signed <<EOF
 first
@@ -43,4 +50,35 @@ test_expect_success 'rebase --no-signoff does not add a sign-off line' '
 	test_cmp expected-unsigned actual
 '
 
+test_expect_success 'rebase --exec --signoff adds a sign-off line' '
+	test_when_finished "rm exec" &&
+	git commit --amend -m "first" &&
+	git rebase --exec "touch exec" --signoff HEAD^ &&
+	test_path_is_file exec &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_success 'rebase --root --signoff adds a sign-off line' '
+	git commit --amend -m "first" &&
+	git rebase --root --keep-empty --signoff &&
+	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-initial-signed actual &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_success 'rebase -i --signoff fails' '
+	git commit --amend -m "first" &&
+	git rebase -i --signoff HEAD^ &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
+
+test_expect_success 'rebase -m --signoff fails' '
+	git commit --amend -m "first" &&
+	git rebase -m --signoff HEAD^ &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed actual
+'
 test_done
-- 
2.16.2

[PATCH v2 2/3] rebase -p: error out if --signoff is given

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

rebase --preserve-merges does not support --signoff so error out
rather than just silently ignoring it so that the user knows the
commits will not be signed off.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index 5d854657a7..7d3bb9d443 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -479,6 +479,8 @@ fi
 
 if test -n "$signoff"
 then
+	test -n "$preserve_merges" &&
+		die "$(gettext "error: cannot combine '--signoff' with '--preserve-merges'")"
 	git_am_opt="$git_am_opt $signoff"
 	force_rebase=t
 fi
-- 
2.16.2

[PATCH v2 3/3] rebase --keep-empty: always use interactive rebase

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

rebase --merge accepts --keep-empty but just ignores it, by using an
implicit interactive rebase the user still gets the rename detection
of a merge based rebase but with with --keep-empty support.

If rebase --keep-empty without --interactive or --merge stops for the
user to resolve merge conflicts then 'git rebase --continue' will
fail. This is because it uses a different code path that does not
create $git_dir/rebase-apply. As rebase --keep-empty was implemented
using cherry-pick it has never supported the am options and now that
interactive rebases support --signoff there is no loss of
functionality by using an implicit interactive rebase.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase--am.sh                 | 79 ++++++++++++++++-----------------------
 git-rebase.sh                     |  5 +++
 t/t3421-rebase-topology-linear.sh |  4 +-
 3 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index be3f068922..ae596d3731 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -41,60 +41,47 @@ else
 fi
 
 ret=0
-if test -n "$keep_empty"
-then
-	# we have to do this the hard way.  git format-patch completely squashes
-	# empty commits and even if it didn't the format doesn't really lend
-	# itself well to recording empty patches.  fortunately, cherry-pick
-	# makes this easy
-	git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
-		$allow_rerere_autoupdate --right-only "$revisions" \
-		$allow_empty_message \
-		${restrict_revision+^$restrict_revision}
-	ret=$?
-else
-	rm -f "$GIT_DIR/rebased-patches"
+rm -f "$GIT_DIR/rebased-patches"
 
-	git format-patch -k --stdout --full-index --cherry-pick --right-only \
-		--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
-		--pretty=mboxrd \
-		$git_format_patch_opt \
-		"$revisions" ${restrict_revision+^$restrict_revision} \
-		>"$GIT_DIR/rebased-patches"
-	ret=$?
+git format-patch -k --stdout --full-index --cherry-pick --right-only \
+	--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
+	--pretty=mboxrd \
+	$git_format_patch_opt \
+	"$revisions" ${restrict_revision+^$restrict_revision} \
+	>"$GIT_DIR/rebased-patches"
+ret=$?
 
-	if test 0 != $ret
-	then
-		rm -f "$GIT_DIR/rebased-patches"
-		case "$head_name" in
-		refs/heads/*)
-			git checkout -q "$head_name"
-			;;
-		*)
-			git checkout -q "$orig_head"
-			;;
-		esac
+if test 0 != $ret
+then
+	rm -f "$GIT_DIR/rebased-patches"
+	case "$head_name" in
+	refs/heads/*)
+		git checkout -q "$head_name"
+		;;
+	*)
+		git checkout -q "$orig_head"
+		;;
+	esac
 
-		cat >&2 <<-EOF
+	cat >&2 <<-EOF
 
-		git encountered an error while preparing the patches to replay
-		these revisions:
+	git encountered an error while preparing the patches to replay
+	these revisions:
 
-		    $revisions
+	    $revisions
 
-		As a result, git cannot rebase them.
-		EOF
-		return $ret
-	fi
+	As a result, git cannot rebase them.
+	EOF
+	return $ret
+fi
 
-	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" \
-		--patch-format=mboxrd \
-		$allow_rerere_autoupdate \
-		${gpg_sign_opt:+"$gpg_sign_opt"} <"$GIT_DIR/rebased-patches"
-	ret=$?
+git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" \
+	--patch-format=mboxrd \
+	$allow_rerere_autoupdate \
+	${gpg_sign_opt:+"$gpg_sign_opt"} <"$GIT_DIR/rebased-patches"
+ret=$?
 
-	rm -f "$GIT_DIR/rebased-patches"
-fi
+rm -f "$GIT_DIR/rebased-patches"
 
 if test 0 != $ret
 then
diff --git a/git-rebase.sh b/git-rebase.sh
index 7d3bb9d443..ad1a3bd3ef 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -459,6 +459,11 @@ then
 	test -z "$interactive_rebase" && interactive_rebase=implied
 fi
 
+if test -n "$keep_empty"
+then
+	test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
 if test -n "$interactive_rebase"
 then
 	type=interactive
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index 68fe2003ef..13e5c1a2f1 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -199,7 +199,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase failure -p
 
@@ -214,7 +214,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase failure -i
 test_run_rebase failure -p
 
-- 
2.16.2

Re: [PATCH v2 3/3] rebase --keep-empty: always use interactive rebase

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

>  git-rebase--am.sh                 | 79 ++++++++++++++++-----------------------

Those are a lot of changes, but pretty much all of it is a change in
indentation.

All three patches look good to me.

Thanks,
Dscho

Re: [PATCH v2 3/3] rebase --keep-empty: always use interactive rebase

[Phillip Wood]

On 20/03/18 16:08, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 20 Mar 2018, Phillip Wood wrote:
> 
>>  git-rebase--am.sh                 | 79 ++++++++++++++++-----------------------
> 
> Those are a lot of changes, but pretty much all of it is a change in
> indentation.

Yes it's a big diff for what is really just deleting a few lines.

> All three patches look good to me.

That's good, thanks for looking at these and the other ones

Best Wishes

Phillip