# rebase -i might leave CHERRY_PICK_HEAD behind 20 messages from 2015-06-16 to 2015-06-18. Participants: SZEDER Gábor, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/39645 ## SZEDER Gábor, 2015-06-16 12:06 Subject: rebase -i might leave CHERRY_PICK_HEAD behind Message-ID: <20150616140612.Horde.a1irZQmh2o42SqDfxAytHg1@webmail.informatik.kit.edu> URL: https://gitlist.dev/e/20150616140612.Horde.a1irZQmh2o42SqDfxAytHg1%40webmail.informatik.kit.edu ``` Hi, When skipping an empty commit with 'git rebase --continue' a CHERRY_PICK_HEAD file might be left behind. What I did boils down to this: echo one >file git add file git commit -m first echo two >file git commit -a -m second echo three >file git commit -a -m third git rebase -i HEAD^^ # change todo list to edit the "second" commit echo three >file git commit -a --amend # this effectively makes the third commit an empty commit # and rebase will ask what to do: git rebase --continue The previous cherry-pick is now empty, possibly due to conflict resolution. If you wish to commit it anyway, use: git commit --allow-empty Otherwise, please use 'git reset' rebase in progress; onto 7335bbe7a5 You are currently rebasing branch 'master' on '7335bbe7a5'. nothing to commit, working directory clean Could not apply d19f82ac6d467247117fd734ed039b03ef923c86... third # I didn't want an empty commit, but didn't read that carefully, so I did: git rebase --continue Successfully rebased and updated refs/heads/master. # and was rewarded for my lack of attention with the following bash prompt: test/rebase-empty-continue (master|CHERRY-PICKING)$ # indeed: ls -l .git/CHERRY_PICK_HEAD -rw-r--r-- 1 szeder szeder 41 Jun 16 13:22 .git/CHERRY_PICK_HEAD On one hand, it's user error: it told me to run 'git reset' to achive what I want but I didn't. Note, however, how it told me about 'git reset': while 'git commit --allow-empty' is greatly emphasized by indentation and empty lines before and after, 'git reset' blends in quite well into the rebase progress. It was late, I was tired, and there was a questionable penalty on Copa América as well ;), so I simply didn't notice. On the other hand, 1. 'git rebase' claimed that "Successfully rebased...", yet it left cruft behind. I think it shouldn't. 2. 'git rebase --continue' didn't complain by the lack of prior 'git reset' and finished doing exacly what I expected from it to do (except leaving CHERRY_PICK_HEAD behind, of course). Perhaps it should complain, like it does when the worktree is dirty. Alternatively, it could just continue to DWIM, as it does already, but then it should remove CHERRY_PICK_HEAD as well. Gábor ``` ## Johannes Schindelin, 2015-06-16 14:56 Subject: Re: rebase -i might leave CHERRY_PICK_HEAD behind Message-ID: URL: https://gitlist.dev/e/a1b485b913b374a1cf7f1bcd1e77532f%40www.dscho.org In-Reply-To: <20150616140612.Horde.a1irZQmh2o42SqDfxAytHg1@webmail.informatik.kit.edu> ``` Hi, On 2015-06-16 14:06, SZEDER Gábor wrote: > When skipping an empty commit with 'git rebase --continue' a > CHERRY_PICK_HEAD file might be left behind. Yeah, I noticed that, too... it even survives the cleanup of the finished rebase under certain circumstances. Maybe something like this? -- snipsnap -- diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index dc3133f..16e0a82 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -849,7 +849,11 @@ continue) # do we have anything to commit? if git diff-index --cached --quiet HEAD -- then - : Nothing to commit -- skip this + : Nothing to commit -- skip this commit + + test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || + rm "$GIT_DIR"/CHERRY_PICK_HEAD || + die "Could not remove CHERRY_PICK_HEAD" else if ! test -f "$author_script" then ``` ## Johannes Schindelin, 2015-06-17 08:15 Subject: [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Message-ID: URL: https://gitlist.dev/e/cover.1434528725.git.johannes.schindelin%40gmx.de In-Reply-To: <20150616140612.Horde.a1irZQmh2o42SqDfxAytHg1@webmail.informatik.kit.edu> ``` Gábor's mail reminded me that this bug bites me often enough when rebasing Git for Windows. The symptom is that .git/CHERRY_PICK_HEAD is left behind after skipping an already-merged patch with `git rebase --continue` instead of `git rebase --skip`. I always prefer the former invocation because the latter would also skip legitimate patches if there were merge conflicts, while the former would not allow that. Johannes Schindelin (2): t3404: demonstrate CHERRY_PICK_HEAD bug rebase -i: do not leave a CHERRY_PICK_HEAD file behind git-rebase--interactive.sh | 6 +++++- t/t3404-rebase-interactive.sh | 20 ++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-17 08:16 Subject: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: URL: https://gitlist.dev/e/f76ca5d0b6687a4d2ee4f587d8ba75a643a33c04.1434528725.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When rev-list's --cherry option does not detect that a patch has already been applied upstream, an interactive rebase would offer to reapply it and consequently stop at that patch with a failure, mentioning that the diff is empty. Traditionally, a `git rebase --continue` simply skips the commit in such a situation. However, as pointed out by Gábor Szeder, this leaves a CHERRY_PICK_HEAD behind, making the Git prompt believe that a cherry pick is still going on. This commit adds a test case demonstrating this bug. Signed-off-by: Johannes Schindelin --- t/t3404-rebase-interactive.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ac429a0..5d52775 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1102,4 +1102,24 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test $(git cat-file commit HEAD | sed -ne \$p) = I ' +test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' + git checkout -b commit-to-skip && + for double in X 3 1 + do + seq 5 | sed "s/$double/&&/" >seq && + git add seq && + test_tick && + git commit -m seq-$double + done && + git tag seq-onto && + git reset --hard HEAD~2 && + git cherry-pick seq-onto && + test_must_fail git rebase -i seq-onto && + test -d .git/rebase-merge && + git rebase --continue && + git diff seq-onto && + test ! -d .git/rebase-merge && + test ! -f .git/CHERRY_PICK_HEAD +' + test_done -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-17 08:16 Subject: [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Message-ID: <2751d249ba6315f0139ba8fd6aa6ff23c32c9e69.1434528725.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/2751d249ba6315f0139ba8fd6aa6ff23c32c9e69.1434528725.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When skipping commits whose changes were already applied via `git rebase --continue`, we need to clean up said file explicitly. The same is not true for `git rebase --skip` because that will execute `git reset --hard` as part of the "skip" handling in git-rebase.sh, even before git-rebase--interactive.sh is called. Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 6 +++++- t/t3404-rebase-interactive.sh | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index dc3133f..16e0a82 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -849,7 +849,11 @@ continue) # do we have anything to commit? if git diff-index --cached --quiet HEAD -- then - : Nothing to commit -- skip this + : Nothing to commit -- skip this commit + + test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || + rm "$GIT_DIR"/CHERRY_PICK_HEAD || + die "Could not remove CHERRY_PICK_HEAD" else if ! test -f "$author_script" then diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 5d52775..241d4d1 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1102,7 +1102,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test $(git cat-file commit HEAD | sed -ne \$p) = I ' -test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' +test_expect_success 'rebase --continue removes CHERRY_PICK_HEAD' ' git checkout -b commit-to-skip && for double in X 3 1 do -- 2.3.1.windows.1.9.g8c01ab4 ``` ## SZEDER Gábor, 2015-06-17 12:58 Subject: Re: [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Message-ID: <20150617145814.Horde.VFmyWwlQfspzMMXX3ZuSlQ4@webmail.informatik.kit.edu> URL: https://gitlist.dev/e/20150617145814.Horde.VFmyWwlQfspzMMXX3ZuSlQ4%40webmail.informatik.kit.edu In-Reply-To: <2751d249ba6315f0139ba8fd6aa6ff23c32c9e69.1434528725.git.johannes.schindelin@gmx.de> ``` Hi, Quoting Johannes Schindelin : > When skipping commits whose changes were already applied via `git rebase > --continue`, we need to clean up said file explicitly. > > The same is not true for `git rebase --skip` because that will execute > `git reset --hard` as part of the "skip" handling in git-rebase.sh, even > before git-rebase--interactive.sh is called. > > Signed-off-by: Johannes Schindelin Nice quick turnaround, thanks. So, with this change the 'git reset' won't be necessary at all, right? > --- > git-rebase--interactive.sh | 6 +++++- > t/t3404-rebase-interactive.sh | 2 +- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index dc3133f..16e0a82 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -849,7 +849,11 @@ continue) > # do we have anything to commit? > if git diff-index --cached --quiet HEAD -- > then > - : Nothing to commit -- skip this > + : Nothing to commit -- skip this commit "While at it", perhaps you could turn this into a proper comment with '#". Now that this if-branch starts to actually do something, there's no reason to continue (ab)using the null command. > + > + test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || > + rm "$GIT_DIR"/CHERRY_PICK_HEAD || > + die "Could not remove CHERRY_PICK_HEAD" > else > if ! test -f "$author_script" > then > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 5d52775..241d4d1 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1102,7 +1102,7 @@ test_expect_success 'rebase -i commits that > overwrite untracked files (no ff)' ' > test $(git cat-file commit HEAD | sed -ne \$p) = I > ' > > -test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' > +test_expect_success 'rebase --continue removes CHERRY_PICK_HEAD' ' > git checkout -b commit-to-skip && > for double in X 3 1 > do > -- > 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-17 15:24 Subject: Re: [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Message-ID: URL: https://gitlist.dev/e/d8b9c3f3376817962207b9c3f16c7d01%40www.dscho.org In-Reply-To: <20150617145814.Horde.VFmyWwlQfspzMMXX3ZuSlQ4@webmail.informatik.kit.edu> ``` Hi, On 2015-06-17 14:58, SZEDER Gábor wrote: > Quoting Johannes Schindelin : >> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh >> index dc3133f..16e0a82 100644 >> --- a/git-rebase--interactive.sh >> +++ b/git-rebase--interactive.sh >> @@ -849,7 +849,11 @@ continue) >> # do we have anything to commit? >> if git diff-index --cached --quiet HEAD -- >> then >> - : Nothing to commit -- skip this >> + : Nothing to commit -- skip this commit > > "While at it", perhaps you could turn this into a proper comment with '#". > Now that this if-branch starts to actually do something, there's no > reason to continue (ab)using the null command. Sure thing. I'll wait if there are more comments and then send out v2. Ciao, Johannes ``` ## Junio C Hamano, 2015-06-17 16:43 Subject: Re: [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Message-ID: URL: https://gitlist.dev/e/xmqqvbem2slg.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > The symptom is that .git/CHERRY_PICK_HEAD is left behind after > skipping an already-merged patch with `git rebase --continue` > instead of `git rebase --skip`. I always prefer the former > invocation because the latter would also skip legitimate patches > if there were merge conflicts, while the former would not allow > that. Makes sense; will queue. Thanks. ``` ## Junio C Hamano, 2015-06-17 17:33 Subject: Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: URL: https://gitlist.dev/e/xmqqtwu66xzg.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > +test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' > + git checkout -b commit-to-skip && > + for double in X 3 1 > + do > + seq 5 | sed "s/$double/&&/" >seq && > + git add seq && > + test_tick && > + git commit -m seq-$double > + done && > + git tag seq-onto && > + git reset --hard HEAD~2 && > + git cherry-pick seq-onto && > + test_must_fail git rebase -i seq-onto && Shouldn't this explicitly specify what fake editor is to be used, instead of relying on whatever the last test happened to have used? I thought this deserved to go to older maintenance track, but the fake editor that was used last are different between branches, so "rebase -i" fails for a wrong reason (i.e. cannot spawn the editor) when queued on say maint-2.2. > + test -d .git/rebase-merge && > + git rebase --continue && > + git diff seq-onto && I am puzzled with this "diff"; what is this about? Is it a remnant from an earlier debugging session, or is it making sure seq-onto is a valid tree-ish? > + test ! -d .git/rebase-merge && > + test ! -f .git/CHERRY_PICK_HEAD > +' > + > test_done Thanks. ``` ## Johannes Schindelin, 2015-06-18 11:22 Subject: Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: URL: https://gitlist.dev/e/c2b427850cc6f298bee017123639c4df%40www.dscho.org In-Reply-To: ``` Hi Junio, On 2015-06-17 19:33, Junio C Hamano wrote: > Johannes Schindelin writes: > >> +test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' >> + git checkout -b commit-to-skip && >> + for double in X 3 1 >> + do >> + seq 5 | sed "s/$double/&&/" >seq && >> + git add seq && >> + test_tick && >> + git commit -m seq-$double >> + done && >> + git tag seq-onto && >> + git reset --hard HEAD~2 && >> + git cherry-pick seq-onto && >> + test_must_fail git rebase -i seq-onto && > > Shouldn't this explicitly specify what fake editor is to be used, > instead of relying on whatever the last test happened to have used? > > I thought this deserved to go to older maintenance track, but the > fake editor that was used last are different between branches, so > "rebase -i" fails for a wrong reason (i.e. cannot spawn the editor) > when queued on say maint-2.2. True. Thanks for pointing that out. Will be fixed in v2. >> + test -d .git/rebase-merge && >> + git rebase --continue && >> + git diff seq-onto && > > I am puzzled with this "diff"; what is this about? Is it a remnant > from an earlier debugging session, or is it making sure seq-onto is > a valid tree-ish? The idea is to verify that we end up with the same tree even if we exchanged the latest two patches. I can remove it if you want as it is not strictly necessary, but I would like to keep it just to make sure that we did not end up with an incomplete rebase. Ciao, Dscho ``` ## Johannes Schindelin, 2015-06-18 11:44 Subject: [PATCH v2 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Message-ID: URL: https://gitlist.dev/e/cover.1434627492.git.johannes.schindelin%40gmx.de In-Reply-To: ``` These patches fix a bug that bites me often enough when rebasing Git for Windows. The symptom is that .git/CHERRY_PICK_HEAD is left behind after skipping an already-merged patch with `git rebase --continue` instead of `git rebase --skip`. I always prefer the former invocation because the latter would also skip legitimate patches if there were merge conflicts, while the former would not allow that. Changes since v1: - no longer uses ':' to make the comment a no-op statement - sets the fake editor correctly in the test (I verified that by rebasing onto v2.2.2 and running the test with and without setting the fake editor) Interdiff below the diffstat. Johannes Schindelin (2): t3404: demonstrate CHERRY_PICK_HEAD bug rebase -i: do not leave a CHERRY_PICK_HEAD file behind git-rebase--interactive.sh | 6 +++++- t/t3404-rebase-interactive.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 16e0a82..5ff0f1c 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -849,7 +849,7 @@ continue) # do we have anything to commit? if git diff-index --cached --quiet HEAD -- then - : Nothing to commit -- skip this commit + # Nothing to commit -- skip this commit test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || rm "$GIT_DIR"/CHERRY_PICK_HEAD || diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 241d4d1..f3337ad 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1114,7 +1114,8 @@ test_expect_success 'rebase --continue removes CHERRY_PICK_HEAD' ' git tag seq-onto && git reset --hard HEAD~2 && git cherry-pick seq-onto && - test_must_fail git rebase -i seq-onto && + set_fake_editor && + FAKE_LINES= test_must_fail git rebase -i seq-onto && test -d .git/rebase-merge && git rebase --continue && git diff seq-onto && -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-18 11:44 Subject: [PATCH v2 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: <930906ab4bb4d629d8963850460c1d15f958af5a.1434627492.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/930906ab4bb4d629d8963850460c1d15f958af5a.1434627492.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When rev-list's --cherry option does not detect that a patch has already been applied upstream, an interactive rebase would offer to reapply it and consequently stop at that patch with a failure, mentioning that the diff is empty. Traditionally, a `git rebase --continue` simply skips the commit in such a situation. However, as pointed out by Gábor Szeder, this leaves a CHERRY_PICK_HEAD behind, making the Git prompt believe that a cherry pick is still going on. This commit adds a test case demonstrating this bug. Signed-off-by: Johannes Schindelin --- t/t3404-rebase-interactive.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ac429a0..e5e7744 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1102,4 +1102,25 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test $(git cat-file commit HEAD | sed -ne \$p) = I ' +test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' + git checkout -b commit-to-skip && + for double in X 3 1 + do + seq 5 | sed "s/$double/&&/" >seq && + git add seq && + test_tick && + git commit -m seq-$double + done && + git tag seq-onto && + git reset --hard HEAD~2 && + git cherry-pick seq-onto && + set_fake_editor && + FAKE_LINES= test_must_fail git rebase -i seq-onto && + test -d .git/rebase-merge && + git rebase --continue && + git diff seq-onto && + test ! -d .git/rebase-merge && + test ! -f .git/CHERRY_PICK_HEAD +' + test_done -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-18 11:44 Subject: [PATCH v2 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Message-ID: <23bb1e151564cdc0249735b3c5f28ea441a5a907.1434627492.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/23bb1e151564cdc0249735b3c5f28ea441a5a907.1434627492.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When skipping commits whose changes were already applied via `git rebase --continue`, we need to clean up said file explicitly. The same is not true for `git rebase --skip` because that will execute `git reset --hard` as part of the "skip" handling in git-rebase.sh, even before git-rebase--interactive.sh is called. Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 6 +++++- t/t3404-rebase-interactive.sh | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index dc3133f..5ff0f1c 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -849,7 +849,11 @@ continue) # do we have anything to commit? if git diff-index --cached --quiet HEAD -- then - : Nothing to commit -- skip this + # Nothing to commit -- skip this commit + + test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || + rm "$GIT_DIR"/CHERRY_PICK_HEAD || + die "Could not remove CHERRY_PICK_HEAD" else if ! test -f "$author_script" then diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index e5e7744..f3337ad 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1102,7 +1102,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test $(git cat-file commit HEAD | sed -ne \$p) = I ' -test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' +test_expect_success 'rebase --continue removes CHERRY_PICK_HEAD' ' git checkout -b commit-to-skip && for double in X 3 1 do -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Junio C Hamano, 2015-06-18 16:00 Subject: Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: URL: https://gitlist.dev/e/xmqqoakdf1lu.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: >>> + git diff seq-onto && >> >> I am puzzled with this "diff"; what is this about? Is it a remnant >> from an earlier debugging session, or is it making sure seq-onto is >> a valid tree-ish? > > The idea is to verify that we end up with the same tree even if we > exchanged the latest two patches. I can remove it if you want as it is > not strictly necessary, but I would like to keep it just to make sure > that we did not end up with an incomplete rebase. I agree that such a verification is a very good thing to have here. But you would need to ask "git diff" to signal that it found no differences with --exit-code or --quiet, I would think. Thanks. ``` ## Johannes Schindelin, 2015-06-18 16:16 Subject: Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: <0f55232f453ab3a75bafe17db2744552@www.dscho.org> URL: https://gitlist.dev/e/0f55232f453ab3a75bafe17db2744552%40www.dscho.org In-Reply-To: ``` Hi Junio, On 2015-06-18 18:00, Junio C Hamano wrote: > Johannes Schindelin writes: > >>>> + git diff seq-onto && >>> >>> I am puzzled with this "diff"; what is this about? Is it a remnant >>> from an earlier debugging session, or is it making sure seq-onto is >>> a valid tree-ish? >> >> The idea is to verify that we end up with the same tree even if we >> exchanged the latest two patches. I can remove it if you want as it is >> not strictly necessary, but I would like to keep it just to make sure >> that we did not end up with an incomplete rebase. > > I agree that such a verification is a very good thing to have here. > But you would need to ask "git diff" to signal that it found no > differences with --exit-code or --quiet, I would think. > > Thanks. Whoops! Of course... You want me to re-roll? Ciao, Dscho ``` ## Johannes Schindelin, 2015-06-18 16:38 Subject: [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Message-ID: URL: https://gitlist.dev/e/cover.1434645436.git.johannes.schindelin%40gmx.de In-Reply-To: ``` These patches fix a bug that bites me often enough when rebasing Git for Windows. The symptom is that .git/CHERRY_PICK_HEAD is left behind after skipping an already-merged patch with `git rebase --continue` instead of `git rebase --skip`. I always prefer the former invocation because the latter would also skip legitimate patches if there were merge conflicts, while the former would not allow that. Changes since v2: - the test uses `--exit-code` to verify that the result of the rebase is correct Interdiff below the diffstat. Johannes Schindelin (2): t3404: demonstrate CHERRY_PICK_HEAD bug rebase -i: do not leave a CHERRY_PICK_HEAD file behind git-rebase--interactive.sh | 6 +++++- t/t3404-rebase-interactive.sh | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f3337ad..6bcf18b 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1118,7 +1118,7 @@ test_expect_success 'rebase --continue removes CHERRY_PICK_HEAD' ' FAKE_LINES= test_must_fail git rebase -i seq-onto && test -d .git/rebase-merge && git rebase --continue && - git diff seq-onto && + git diff --exit-code seq-onto && test ! -d .git/rebase-merge && test ! -f .git/CHERRY_PICK_HEAD ' -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-18 16:38 Subject: [PATCH v3 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Message-ID: <66bd04e531ae5db6a06d370a0f7e4370ef4e1539.1434645436.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/66bd04e531ae5db6a06d370a0f7e4370ef4e1539.1434645436.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When rev-list's --cherry option does not detect that a patch has already been applied upstream, an interactive rebase would offer to reapply it and consequently stop at that patch with a failure, mentioning that the diff is empty. Traditionally, a `git rebase --continue` simply skips the commit in such a situation. However, as pointed out by Gábor Szeder, this leaves a CHERRY_PICK_HEAD behind, making the Git prompt believe that a cherry pick is still going on. This commit adds a test case demonstrating this bug. Signed-off-by: Johannes Schindelin --- t/t3404-rebase-interactive.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ac429a0..6fe6c47 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1102,4 +1102,25 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test $(git cat-file commit HEAD | sed -ne \$p) = I ' +test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' + git checkout -b commit-to-skip && + for double in X 3 1 + do + seq 5 | sed "s/$double/&&/" >seq && + git add seq && + test_tick && + git commit -m seq-$double + done && + git tag seq-onto && + git reset --hard HEAD~2 && + git cherry-pick seq-onto && + set_fake_editor && + FAKE_LINES= test_must_fail git rebase -i seq-onto && + test -d .git/rebase-merge && + git rebase --continue && + git diff --exit-code seq-onto && + test ! -d .git/rebase-merge && + test ! -f .git/CHERRY_PICK_HEAD +' + test_done -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Johannes Schindelin, 2015-06-18 16:38 Subject: [PATCH v3 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Message-ID: URL: https://gitlist.dev/e/bf1954a4447bcf99c27a566a10cb0b311221135f.1434645436.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When skipping commits whose changes were already applied via `git rebase --continue`, we need to clean up said file explicitly. The same is not true for `git rebase --skip` because that will execute `git reset --hard` as part of the "skip" handling in git-rebase.sh, even before git-rebase--interactive.sh is called. Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 6 +++++- t/t3404-rebase-interactive.sh | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index dc3133f..5ff0f1c 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -849,7 +849,11 @@ continue) # do we have anything to commit? if git diff-index --cached --quiet HEAD -- then - : Nothing to commit -- skip this + # Nothing to commit -- skip this commit + + test ! -f "$GIT_DIR"/CHERRY_PICK_HEAD || + rm "$GIT_DIR"/CHERRY_PICK_HEAD || + die "Could not remove CHERRY_PICK_HEAD" else if ! test -f "$author_script" then diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 6fe6c47..6bcf18b 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1102,7 +1102,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test $(git cat-file commit HEAD | sed -ne \$p) = I ' -test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' +test_expect_success 'rebase --continue removes CHERRY_PICK_HEAD' ' git checkout -b commit-to-skip && for double in X 3 1 do -- 2.3.1.windows.1.9.g8c01ab4 ``` ## Junio C Hamano, 2015-06-18 19:45 Subject: Re: [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Message-ID: URL: https://gitlist.dev/e/xmqqwpz0er61.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Thanks. This round looks good, except one trivial nit (below), which I'll locally squash-in a fix for. diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index fb1b571..6938e5e 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1052,7 +1052,7 @@ test_expect_failure 'rebase --continue removes CHERRY_PICK_HEAD' ' git reset --hard HEAD~2 && git cherry-pick seq-onto && set_fake_editor && - FAKE_LINES= test_must_fail git rebase -i seq-onto && + test_must_fail env FAKE_LINES= git rebase -i seq-onto && test -d .git/rebase-merge && git rebase --continue && git diff --exit-code seq-onto && -- 2.4.4-569-gdcc90bb ``` ## Johannes Schindelin, 2015-06-18 20:11 Subject: Re: [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Message-ID: URL: https://gitlist.dev/e/f2eff1416fdae8daf003a9c54b314a6b%40www.dscho.org In-Reply-To: ``` On 2015-06-18 21:45, Junio C Hamano wrote: > This round looks good, except one trivial nit (below), which I'll > locally squash-in a fix for. Thanks, Dscho ```