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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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