Aborting cherry-pick

Aborting cherry-pick

[Piotr Krukowiecki]

Hi,

I'd like to suggest improving cherry-pick messages/documentation
in case of conflicts.

Example of cherry-pick conflict:

   $ git cherry-pick  c64e8caa56fd76577bbaea37592f4a9df10ab1b8
   error: could not apply c64e8ca... <<commit message>>
   hint: after resolving the conflicts, mark the corrected paths
   hint: with 'git add <paths>' or 'git rm <paths>'

   $ git status
   # On branch master
   # Unmerged paths:
   #   (use "git reset HEAD <file>..." to unstage)
   #   (use "git add/rm <file>..." as appropriate to mark resolution)
   #
   #	both modified:      <<file>>
   #
   no changes added to commit (use "git add" and/or "git commit -a")

I see following possibilities:

1. add hint to cherry-pick how to abort merge, for example:
   hint: use 'git reset --merge ORIG_HEAD' to abort merge

2. add hint to status how to abort merge, for example:
   #   (use 'git reset --merge ORIG_HEAD' to abort merge)

3. add "--abort" to cherry-pick (same option as for git-merge)

The ORIG_HEAD is specified in man page example. There is also
a possibility of omitting ORIG_HEAD in which case local changes
are retained. Maybe this is a better suggesting for the user?
Or maybe describe both cases somewhere and point user to this
documentation in status/cherry-pick output?

git-cherry-pick uses word "cancel" in man page (in example) and
git-merge uses word "abort" for (as I understand) the same action.

I would suggest using either same word for both cases, or using both
words in both cases. Otherwise it's confusing if you read about "aborting"
merges and when you want to abort cherry-pick you don't find it - because
it's called "canceling".


What is your opinion on this?


-- 
Piotrek

[PATCH] cherry-pick: when pick fails, explain how to cancel

[Jonathan Nieder]

If you tried a cherry-pick or revert resulting in complex conflicts
and want to start over, "git reset --merge HEAD" might help.

Requested-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
(+cc some relevant people. Johan wrote merge --abort, Jay is adding
a status hint during cherry-pick, Christian wrote reset --merge and
much related cherry-pick functionality)

Piotr Krukowiecki wrote:

> Example of cherry-pick conflict:
>
>    $ git cherry-pick  c64e8caa56fd76577bbaea37592f4a9df10ab1b8
>    error: could not apply c64e8ca... <<commit message>>
>    hint: after resolving the conflicts, mark the corrected paths
>    hint: with 'git add <paths>' or 'git rm <paths>'
>
>    $ git status
>    # On branch master
>    # Unmerged paths:
[...]
> I see following possibilities:
>
> 1. add hint to cherry-pick how to abort merge, for example:
>    hint: use 'git reset --merge ORIG_HEAD' to abort merge

Good idea.  "git cherry-pick" does not set ORIG_HEAD, so I
put "git reset --merge HEAD" instead.

> 2. add hint to status how to abort merge, for example:
>    #   (use 'git reset --merge ORIG_HEAD' to abort merge)

"git reset --merge" will remove local changes marked with "git add",
under the assumption that they were part of the conflict resolution
and thus should be cleared away.

This leaves me afraid of the following scenario:

	git pull
	# conflict?  agh!

	# ... two days later ...
	... hack hack hack ...
	... add add add ...
	git commit;	# fails because of unmerged files

	# whoops, forgot about that merge.
	# Let's do what it says to do:
	git reset --merge ORIG_HEAD

It seems safest to only recommend "reset --merge" immediately
after a mergy operation.

> The ORIG_HEAD is specified in man page example.

I assume you mean on the git-reset page?  It might be nice to add
more examples and to update the current ones with current best
practice (e.g., using reset --keep more often).

The use of ORIG_HEAD there also allows the following:

	git pull
	# success?  But I didn't want that
	git reset --merge ORIG_HEAD

and protects against situations in which ORIG_HEAD is not present
(though merge --abort does that better, I think).

> git-cherry-pick uses word "cancel" in man page (in example) and
> git-merge uses word "abort" for (as I understand) the same action.

Hmm, where is this example in the cherry-pick manpage?

Thanks for the suggestions.  Generally speaking, I like them.

 builtin/revert.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index dc1b702..78baae7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -267,11 +267,13 @@ static void print_advice(void)
 	}
 
 	advise("after resolving the conflicts, mark the corrected paths");
-	advise("with 'git add <paths>' or 'git rm <paths>'");
+	advise("with 'git add <paths>' and 'git rm <paths>'");
 
 	if (action == CHERRY_PICK)
 		advise("and commit the result with 'git commit -c %s'",
 		       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
+
+	advise("or use 'git reset --merge HEAD' to abort");
 }
 
 static void write_message(struct strbuf *msgbuf, const char *filename)
-- 
1.7.4.1

Re: [PATCH] cherry-pick: when pick fails, explain how to cancel

[Piotr Krukowiecki]

W dniu 18.02.2011 02:24, Jonathan Nieder pisze:
> Piotr Krukowiecki wrote:
>> 1. add hint to cherry-pick how to abort merge, for example:
>>    hint: use 'git reset --merge ORIG_HEAD' to abort merge
> 
> Good idea.  "git cherry-pick" does not set ORIG_HEAD, so I
> put "git reset --merge HEAD" instead.

You're right. I was sure I have tested it, but when I try to 
use ORIG_HEAD now it does not work.

I took it from Documentation/git-cherry-pick.txt, which seem
to be wrong:

   $ git cherry-pick topic^             <1>
   $ git diff                           <2>
   $ git reset --merge ORIG_HEAD        <3>
   [...]
   <3> cancel the cherry-pick.  In other words, return to the
   pre-cherry-pick state, preserving any local modifications you had in
   the working tree.


>> 2. add hint to status how to abort merge, for example:
>>    #   (use 'git reset --merge ORIG_HEAD' to abort merge)
> 
> "git reset --merge" will remove local changes marked with "git add",
> under the assumption that they were part of the conflict resolution
> and thus should be cleared away.

Didn't know that (one probably shouldn't merge with uncommitted/local
changes anyway). git-reset.txt mentions it, but there's no explicit
warning.


> This leaves me afraid of the following scenario:
> 
> 	git pull
> 	# conflict?  agh!
> 
> 	# ... two days later ...
> 	... hack hack hack ...
> 	... add add add ...
> 	git commit;	# fails because of unmerged files
> 
> 	# whoops, forgot about that merge.
> 	# Let's do what it says to do:
> 	git reset --merge ORIG_HEAD
> 
> It seems safest to only recommend "reset --merge" immediately
> after a mergy operation.

Is it possible to recognize that you have something more than what
was cause by the merge/cherry-pick?


>> The ORIG_HEAD is specified in man page example.
> 
> I assume you mean on the git-reset page?  It might be nice to add
> more examples and to update the current ones with current best
> practice (e.g., using reset --keep more often).

Yes, it says:

   Running "git reset --hard ORIG_HEAD" will let you go back to where
   you were, but it will discard your local changes, which you do not
   want.  "git reset --merge" keeps your local changes.

 
>> git-cherry-pick uses word "cancel" in man page (in example) and
>> git-merge uses word "abort" for (as I understand) the same action.
> 
> Hmm, where is this example in the cherry-pick manpage?

It's in the Documentation/cherry-pick.txt (in current master). I assume 
man pages are generated from those text files. I have also cited it at 
the beginning of this mail.


-- 
Piotr Krukowiecki

[PATCH] Documentation: cherry-pick does not set ORIG_HEAD

[Jonathan Nieder]

The example that uses "reset --merge" to bail out from a failed
cherry-pick is dangerously wrong.  For example,

	git reset --keep master
	git cherry-pick HEAD@{1};	# conflicts!
	git reset --merge ORIG_HEAD;	# let's try that again
	git cherry-pick --strategy=more-careful HEAD@{1}

would not reset the topic to the pre cherry-pick state (master) but to
whatever branch we were on before then.

The example used ORIG_HEAD by habit from aborting merges.  "git reset
--merge HEAD" is more appropriate here.

Noticed-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Piotr Krukowiecki wrote:

> I took it from Documentation/git-cherry-pick.txt, which seem
> to be wrong:

Good catch.  The manpage I was looking at was stale.

>> "git reset --merge" will remove local changes marked with "git add",
>> under the assumption that they were part of the conflict resolution
>> and thus should be cleared away.
>
> Didn't know that (one probably shouldn't merge with uncommitted/local
> changes anyway). git-reset.txt mentions it, but there's no explicit
> warning.

Care to write a patch?

>> This leaves me afraid of the following scenario:
>>
[...]
>> 	# ... two days later ...
>> 	... hack hack hack ...
>> 	... add add add ...
>> 	git commit;	# fails because of unmerged files
>> 
>> 	# whoops, forgot about that merge.
>> 	# Let's do what it says to do:
>> 	git reset --merge ORIG_HEAD
[...]
> Is it possible to recognize that you have something more than what
> was cause by the merge/cherry-pick?

I suppose it depends what you mean.  I see at least two distinct
problems to solve:

 A. "undo" facility to recover from an ugly cherry-pick

This one is what reset --merge is for.  The idea is that after
spending a little while trying to make something good out of a
mess, you say, "oh, bother, let me get back to where I started".

So in this case it really does make sense to back out any changes
you marked with "git add" after the cherry-pick, since they were
part of the messy resolution process.  If there had been any
changes registered before the cherry-pick, the cherry-pick would
have just failed without making a mess.

A patch in flight makes "git cherry-pick" print advice for this case
when it encounters conflicts (thanks!).

 B. clearing away a forgotten merge from long ago

If you had been doing work without remembering that there was
a merge in progress, the best way to recover is probably a plain
"git reset HEAD -- .".

This is a case people might be looking for advice about when reading
"git status" ("where was I?") output.

Thanks.

 Documentation/git-cherry-pick.txt |    6 +-
 t/t3404-rebase-interactive.sh     |    6 --
 t/t3510-cherry-pick-doc.sh        |  137 +++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                     |    9 +++
 4 files changed, 149 insertions(+), 9 deletions(-)
 create mode 100755 t/t3510-cherry-pick-doc.sh

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 749d68a..abedcb7 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -137,7 +137,7 @@ again, this time exercising more care about matching up context lines.
 ------------
 $ git cherry-pick topic^             <1>
 $ git diff                           <2>
-$ git reset --merge ORIG_HEAD        <3>
+$ git reset --merge HEAD             <3>
 $ git cherry-pick -Xpatience topic^  <4>
 ------------
 <1> apply the change that would be shown by `git show topic^`.
@@ -146,8 +146,8 @@ information about the conflict is written to the index and
 working tree and no new commit results.
 <2> summarize changes to be reconciled
 <3> cancel the cherry-pick.  In other words, return to the
-pre-cherry-pick state, preserving any local modifications you had in
-the working tree.
+pre-cherry-pick state, preserving any local modifications you have
+in the working tree.
 <4> try to apply the change introduced by `topic^` again,
 spending extra time to avoid mistakes based on incorrectly matching
 context lines.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 7d8147b..88f1192 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -29,12 +29,6 @@ Initial setup:
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
-test_cmp_rev () {
-	git rev-parse --verify "$1" >expect.rev &&
-	git rev-parse --verify "$2" >actual.rev &&
-	test_cmp expect.rev actual.rev
-}
-
 set_fake_editor
 
 # WARNING: Modifications to the initial repository can change the SHA ID used
diff --git a/t/t3510-cherry-pick-doc.sh b/t/t3510-cherry-pick-doc.sh
new file mode 100755
index 0000000..e645caa
--- /dev/null
+++ b/t/t3510-cherry-pick-doc.sh
@@ -0,0 +1,137 @@
+#!/bin/sh
+
+test_description='examples from git-cherry-pick(1)
+
+Check that what the manual claims is still true.
+
+  -
+  + seven [next]
+  + six
+  + five [master]
+  + four
+  + three
+  + two (modifies one.t)
+  + one
+  + initial
+'
+. ./test-lib.sh
+
+check_log () {
+	git rev-list "$2" |
+	git diff-tree -r --root --stdin |
+	sed -e "s/$_x40/OBJID/g" >actual.log &&
+	test_cmp "$1" actual.log
+}
+
+test_expect_success setup '
+	GIT_EDITOR=false &&
+	export GIT_EDITOR &&
+
+	test_commit initial &&
+	test_commit one &&
+	test_commit two one.t &&
+	test_commit three &&
+	test_commit four &&
+	test_commit five &&
+	git checkout -b next &&
+	test_commit six &&
+	test_commit seven &&
+	git checkout -b topic initial
+'
+
+test_expect_success 'cherry-pick tip of branch' '
+	cat >expect.log <<-\EOF &&
+	OBJID
+	:000000 100644 OBJID OBJID A	five.t
+	EOF
+	git reset --hard initial &&
+	git cherry-pick master &&
+	check_log expect.log initial..topic
+'
+
+test_expect_success 'cherry-pick to catch up' '
+	cat >expect.log <<-\EOF &&
+	OBJID
+	:000000 100644 OBJID OBJID A	five.t
+	OBJID
+	:000000 100644 OBJID OBJID A	four.t
+	OBJID
+	:000000 100644 OBJID OBJID A	three.t
+	OBJID
+	:100644 100644 OBJID OBJID M	one.t
+	OBJID
+	:000000 100644 OBJID OBJID A	one.t
+	EOF
+	git reset --hard initial &&
+	git cherry-pick ..master &&
+	check_log expect.log initial..topic &&
+
+	git reset --hard initial &&
+	git cherry-pick ^HEAD master &&
+	check_log expect.log initial..topic
+'
+
+test_expect_success 'cherry-pick selected changes' '
+	cat >expect.log <<-\EOF &&
+	OBJID
+	:000000 100644 OBJID OBJID A	three.t
+	OBJID
+	:000000 100644 OBJID OBJID A	one.t
+	EOF
+	git reset --hard initial &&
+	git cherry-pick master~4 master~2 &&
+	check_log expect.log initial..topic
+'
+
+test_expect_success 'cherry-pick --no-commit' '
+	>expect.log &&
+	cat >expect.after-commit <<-\EOF &&
+	OBJID
+	:000000 100644 OBJID OBJID A	four.t
+	:000000 100644 OBJID OBJID A	seven.t
+	EOF
+	git reset --hard initial &&
+	git cherry-pick -n master~1 next &&
+	check_log expect.log initial..topic &&
+	test_must_fail git commit &&
+	git commit -m "squashed patches" &&
+	check_log expect.after-commit initial..topic
+'
+
+test_expect_success 'cherry-pick --ff' '
+	git reset --hard initial &&
+	git cherry-pick --ff ..next &&
+	test_cmp_rev next topic
+'
+
+test_expect_success 'cherry-pick --no-commit --stdin' '
+	git reset --hard initial &&
+	git rev-list --reverse master -- one.t |
+	git cherry-pick -n --stdin &&
+
+	test_cmp_rev initial topic &&
+	git diff-index --cached --exit-code master -- one.t &&
+	git rm -f one.t &&
+	git diff-index --cached --exit-code initial
+'
+
+test_expect_success 'cherry-pick -Xignore-all-space' '
+	git reset --hard one &&
+	test_commit one-with-whitespace one.t "   one   " &&
+
+	test_must_fail git cherry-pick three^ &&
+	git diff >conflict-desc &&
+	grep "[+][+]<<<" conflict-desc &&
+
+	echo modified >initial.t &&
+	git reset --merge HEAD &&
+	test_cmp_rev one-with-whitespace topic &&
+	test_must_fail git diff --exit-code HEAD &&
+	echo initial >initial.t &&
+	git diff --exit-code HEAD &&
+
+	git cherry-pick -Xignore-all-space three^ &&
+	git diff --exit-code two topic
+'
+
+test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0fdc541..0c053b3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -673,6 +673,15 @@ test_line_count () {
 	fi
 }
 
+# test_cmp_rev checks that two revision specifiers refer to the
+# same object.  Uses expect.rev and actual.rev files in the
+# current directory as scratch space.
+test_cmp_rev () {
+	git rev-parse --verify "$1" >expect.rev &&
+	git rev-parse --verify "$2" >actual.rev &&
+	test_cmp expect.rev actual.rev
+}
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
-- 
1.7.4.1

Re: [PATCH] Documentation: cherry-pick does not set ORIG_HEAD

[Piotr Krukowiecki]

W dniu 19.02.2011 00:17, Jonathan Nieder pisze:
>>> "git reset --merge" will remove local changes marked with "git add",
>>> under the assumption that they were part of the conflict resolution
>>> and thus should be cleared away.
>>
>> Didn't know that (one probably shouldn't merge with uncommitted/local
>> changes anyway). git-reset.txt mentions it, but there's no explicit
>> warning.
> 
> Care to write a patch?

I tried but gave up :( 


>> Is it possible to recognize that you have something more than what
>> was cause by the merge/cherry-pick?
> 
> I suppose it depends what you mean.  I see at least two distinct
> problems to solve:
> 
>  A. "undo" facility to recover from an ugly cherry-pick

That's what I meant. There's a 'git merge --abort', the same should be
available for cherry-pick.

If I understand correctly, this requires changes to cherry-pick to 
set ORIG_HEAD?


> This one is what reset --merge is for.  The idea is that after
> spending a little while trying to make something good out of a
> mess, you say, "oh, bother, let me get back to where I started".

Maybe 'git cherry-pick --abort' could do 'reset --merge' internally.
You would not have to remember "ok, for merge I can do --abort, 
but for cherry pick I must do reset --merge".


> So in this case it really does make sense to back out any changes
> you marked with "git add" after the cherry-pick, since they were
> part of the messy resolution process.  If there had been any
> changes registered before the cherry-pick, the cherry-pick would
> have just failed without making a mess.

Indeed, cherry-pick won't start if there are any stashed changes. 
I wasn't aware about this.

Which is a bit 	strange: why cherry-pick behaves differently than merge?
(merge allows staged changes when merging, cherry-pick doesn't).
Shouldn't they work the same, i.e. allow merge or cherry-pick if the
changes are not conflicting?


Anyway, cherry-pick refuses to work if you have staged changes, but still
doesn't mind local unstaged changes. Which makes possible following case:

   do some local hacking, do not stage yet
   cherry-pick commit with merge conflicts
   try to resolve them, adding local changes in meantime
   give up and try to abort with 'git reset --merge'

In result your local changes will be lost. I don't know how real such
situation is, but maybe git could/should prevent it?

For example by saying "you're trying to abort merge, but it will also
revert your local changes from before the merge".
 

> A patch in flight makes "git cherry-pick" print advice for this case
> when it encounters conflicts (thanks!).

Nice :)

 

-- 
Piotr Krukowiecki