[PATCH v2] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so

[PATCH v2] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so

[Jay Soffian]

do_pick_commit() writes out CHERRY_PICK_HEAD before invoking merge (either
via do_recursive_merge() or try_merge_command()) on the assumption that if
the merge fails it is due to conflict. However, if the tree is dirty, the
merge may not even start, aborting before do_pick_commit() can remove
CHERRY_PICK_HEAD.

Instead, defer writing CHERRY_PICK_HEAD till after merge has returned.
At this point we know the merge has either succeeded or failed due
to conflict. In either case, we want CHERRY_PICK_HEAD to be written
so that it may be picked up by the subsequent invocation of commit.

Note that do_recursive_merge() aborts if the merge cannot start, while
try_merge_command() returns a non-zero value other than 1.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/revert.c                |   10 ++++++++--
 t/t3507-cherry-pick-conflict.sh |   15 +++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3117776c2c..a95b255c86 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -476,8 +476,6 @@ static int do_pick_commit(void)
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
-		if (!no_commit)
-			write_cherry_pick_head();
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -498,6 +496,14 @@ static int do_pick_commit(void)
 		free_commit_list(remotes);
 	}
 
+	/* If the merge was clean or if it failed due to conflict, we write
+	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
+	 * However, if the merge did not even start, then we don't want to
+	 * write it at all.
+	*/
+	if (action == CHERRY_PICK && !no_commit && (res == 0 || res == 1))
+		write_cherry_pick_head();
+
 	if (res) {
 		error(action == REVERT
 		      ? _("could not revert %s... %s")
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 212ec54aaf..7601d0b0d6 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -77,6 +77,21 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
 
+test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '
+	pristine_detach initial &&
+	echo foo > foo &&
+	test_must_fail git cherry-pick base &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+'
+
+test_expect_success \
+'cherry-pick --strategy=resolve w/dirty tree does not set CHERRY_PICK_HEAD' '
+	pristine_detach initial &&
+	echo foo > foo &&
+	test_must_fail git cherry-pick --strategy=resolve base &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+'
+
 test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	(
-- 
1.7.7.6.g25c34

Re: [PATCH v2] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so

[Jay Soffian]

On Thu, Oct 6, 2011 at 1:48 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Note that do_recursive_merge() aborts if the merge cannot start, while
> try_merge_command() returns a non-zero value other than 1.

Maybe you want this on-top:

diff --git i/builtin/revert.c w/builtin/revert.c
index a95b255c86..7e4857530b 100644
--- i/builtin/revert.c
+++ w/builtin/revert.c
@@ -223,7 +223,7 @@ static void advise(const char *advice, ...)
 	va_end(params);
 }

-static void print_advice(void)
+static void print_advice(int show_hint)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");

@@ -238,9 +238,11 @@ static void print_advice(void)
 		return;
 	}

-	advise("after resolving the conflicts, mark the corrected paths");
-	advise("with 'git add <paths>' or 'git rm <paths>'");
-	advise("and commit the result with 'git commit'");
+	if (show_hint) {
+		advise("after resolving the conflicts, mark the corrected paths");
+		advise("with 'git add <paths>' or 'git rm <paths>'");
+		advise("and commit the result with 'git commit'");
+	}
 }

 static void write_message(struct strbuf *msgbuf, const char *filename)
@@ -510,7 +512,7 @@ static int do_pick_commit(void)
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
-		print_advice();
+		print_advice(res == 1);
 		rerere(allow_rerere_auto);
 	} else {
 		if (!no_commit)


j.

Re: [PATCH v2] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Thu, Oct 6, 2011 at 1:48 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
>> Note that do_recursive_merge() aborts if the merge cannot start, while
>> try_merge_command() returns a non-zero value other than 1.
>
> Maybe you want this on-top:

Good thinking.

commit 4ed15ff067b548011b1eda8b12d46d887c4f056c
Author: Jay Soffian <jaysoffian@gmail.com>
Date:   Thu Oct 6 13:58:01 2011 -0400

    cherry-pick: do not give irrelevant advice when cherry-pick punted
    
    If a cherry-pick did not even start because the working tree had local
    changes that would overlap with the operation, we shouldn't be advising
    the users to resolve conflicts nor to conclude it with "git commit".
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks. Care to sign-off?

Re: [PATCH v2] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so

[Jay Soffian]

On Thu, Oct 6, 2011 at 5:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> On Thu, Oct 6, 2011 at 1:48 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
>>> Note that do_recursive_merge() aborts if the merge cannot start, while
>>> try_merge_command() returns a non-zero value other than 1.
>>
>> Maybe you want this on-top:
>
> Good thinking.
>
> commit 4ed15ff067b548011b1eda8b12d46d887c4f056c
> Author: Jay Soffian <jaysoffian@gmail.com>
> Date:   Thu Oct 6 13:58:01 2011 -0400
>
>    cherry-pick: do not give irrelevant advice when cherry-pick punted
>
>    If a cherry-pick did not even start because the working tree had local
>    changes that would overlap with the operation, we shouldn't be advising
>    the users to resolve conflicts nor to conclude it with "git commit".
>
>    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Thanks. Care to sign-off?

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>

Thank you for making it a proper commit. :-)

j.