Re: [PATCH 2/2] rebase --root -k: fix when root commit is empty
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 15, 2018, 10:28 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1803151118490.20700@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
- In-Reply-To
- <20180314111127.14217-2-phillip.wood@talktalk.net>
Hi Phillip,
On Wed, 14 Mar 2018, Phillip Wood wrote:
Show 6 quoted lines
> 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
Show 15 quoted lines
> 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_headAs 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.
Show 12 quoted lines
> 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`.
Show 13 quoted lines
> 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