rebase -i might leave CHERRY_PICK_HEAD behind

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

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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

Hi,

Quoting Johannes Schindelin [off-list ref]:

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 <redacted>

Nice quick turnaround, thanks.

So, with this change the 'git reset' won't be necessary at all, right?

"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.

Hi,

On 2015-06-17 14:58, SZEDER Gábor wrote:

Quoting Johannes Schindelin [off-list ref]:

quoted

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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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

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 <redacted>
---
 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

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 <redacted>
---
 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