* rebase -i might leave CHERRY_PICK_HEAD behind
@ 2015-06-16 12:06 SZEDER Gábor
2015-06-16 14:56 ` Johannes Schindelin
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
0 siblings, 2 replies; 20+ messages in thread
From: SZEDER Gábor @ 2015-06-16 12:06 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: rebase -i might leave CHERRY_PICK_HEAD behind
2015-06-16 12:06 rebase -i might leave CHERRY_PICK_HEAD behind SZEDER Gábor
@ 2015-06-16 14:56 ` Johannes Schindelin
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-16 14:56 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD
2015-06-16 12:06 rebase -i might leave CHERRY_PICK_HEAD behind SZEDER Gábor
2015-06-16 14:56 ` Johannes Schindelin
@ 2015-06-17 8:15 ` Johannes Schindelin
2015-06-17 8:16 ` [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
` (3 more replies)
1 sibling, 4 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-17 8:15 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
@ 2015-06-17 8:16 ` Johannes Schindelin
2015-06-17 17:33 ` Junio C Hamano
2015-06-17 8:16 ` [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-17 8:16 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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 <johannes.schindelin@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2015-06-17 8:16 ` [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
@ 2015-06-17 8:16 ` Johannes Schindelin
2015-06-17 12:58 ` SZEDER Gábor
2015-06-17 16:43 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
2015-06-18 11:44 ` [PATCH v2 " Johannes Schindelin
3 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-17 8:16 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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 <johannes.schindelin@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind
2015-06-17 8:16 ` [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
@ 2015-06-17 12:58 ` SZEDER Gábor
2015-06-17 15:24 ` Johannes Schindelin
0 siblings, 1 reply; 20+ messages in thread
From: SZEDER Gábor @ 2015-06-17 12:58 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: gitster, git
Hi,
Quoting Johannes Schindelin <johannes.schindelin@gmx.de>:
> 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 <johannes.schindelin@gmx.de>
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind
2015-06-17 12:58 ` SZEDER Gábor
@ 2015-06-17 15:24 ` Johannes Schindelin
0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-17 15:24 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: gitster, git
Hi,
On 2015-06-17 14:58, SZEDER Gábor wrote:
> Quoting Johannes Schindelin <johannes.schindelin@gmx.de>:
>> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2015-06-17 8:16 ` [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
2015-06-17 8:16 ` [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
@ 2015-06-17 16:43 ` Junio C Hamano
2015-06-18 11:44 ` [PATCH v2 " Johannes Schindelin
3 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2015-06-17 16:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, szeder
Johannes Schindelin <johannes.schindelin@gmx.de> 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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-17 8:16 ` [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
@ 2015-06-17 17:33 ` Junio C Hamano
2015-06-18 11:22 ` Johannes Schindelin
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2015-06-17 17:33 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, szeder
Johannes Schindelin <johannes.schindelin@gmx.de> 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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-17 17:33 ` Junio C Hamano
@ 2015-06-18 11:22 ` Johannes Schindelin
2015-06-18 16:00 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 11:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, szeder
Hi Junio,
On 2015-06-17 19:33, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
` (2 preceding siblings ...)
2015-06-17 16:43 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
@ 2015-06-18 11:44 ` Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
` (2 more replies)
3 siblings, 3 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 11:44 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-18 11:44 ` [PATCH v2 " Johannes Schindelin
@ 2015-06-18 11:44 ` Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 11:44 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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 <johannes.schindelin@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind
2015-06-18 11:44 ` [PATCH v2 " Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
@ 2015-06-18 11:44 ` Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 11:44 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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 <johannes.schindelin@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-18 11:22 ` Johannes Schindelin
@ 2015-06-18 16:00 ` Junio C Hamano
2015-06-18 16:16 ` Johannes Schindelin
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2015-06-18 16:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, szeder
Johannes Schindelin <johannes.schindelin@gmx.de> 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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-18 16:00 ` Junio C Hamano
@ 2015-06-18 16:16 ` Johannes Schindelin
0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 16:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, szeder
Hi Junio,
On 2015-06-18 18:00, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> 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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD
2015-06-18 11:44 ` [PATCH v2 " Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
@ 2015-06-18 16:38 ` Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
` (2 more replies)
2 siblings, 3 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 16:38 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug
2015-06-18 16:38 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
@ 2015-06-18 16:38 ` Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
2015-06-18 19:45 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
2 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 16:38 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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 <johannes.schindelin@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind
2015-06-18 16:38 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
@ 2015-06-18 16:38 ` Johannes Schindelin
2015-06-18 19:45 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
2 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 16:38 UTC (permalink / raw)
To: gitster; +Cc: git, szeder
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 <johannes.schindelin@gmx.de>
---
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD
2015-06-18 16:38 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
@ 2015-06-18 19:45 ` Junio C Hamano
2015-06-18 20:11 ` Johannes Schindelin
2 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2015-06-18 19:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, szeder
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD
2015-06-18 19:45 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
@ 2015-06-18 20:11 ` Johannes Schindelin
0 siblings, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2015-06-18 20:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, szeder
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2015-06-18 20:11 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-16 12:06 rebase -i might leave CHERRY_PICK_HEAD behind SZEDER Gábor
2015-06-16 14:56 ` Johannes Schindelin
2015-06-17 8:15 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2015-06-17 8:16 ` [PATCH 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
2015-06-17 17:33 ` Junio C Hamano
2015-06-18 11:22 ` Johannes Schindelin
2015-06-18 16:00 ` Junio C Hamano
2015-06-18 16:16 ` Johannes Schindelin
2015-06-17 8:16 ` [PATCH 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
2015-06-17 12:58 ` SZEDER Gábor
2015-06-17 15:24 ` Johannes Schindelin
2015-06-17 16:43 ` [PATCH 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
2015-06-18 11:44 ` [PATCH v2 " Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
2015-06-18 11:44 ` [PATCH v2 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 1/2] t3404: demonstrate CHERRY_PICK_HEAD bug Johannes Schindelin
2015-06-18 16:38 ` [PATCH v3 2/2] rebase -i: do not leave a CHERRY_PICK_HEAD file behind Johannes Schindelin
2015-06-18 19:45 ` [PATCH v3 0/2] rebase -i: Fix left-behind CHERRY_PICK_HEAD Junio C Hamano
2015-06-18 20:11 ` Johannes Schindelin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.