threads / patch / 28608

v2revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so

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

## tl;dr

4 messages between Oct 6, 2011 and Oct 6, 2011. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Jay Soffian· Oct 6, 2011, 17:48 UTC · lore

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(-)

Show changes to 2 files +23 −2

builtin/revert.c, t/t3507-cherry-pick-conflict.sh

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

Jay Soffian· Oct 6, 2011, 17:58 UTC · re: Jay Soffian · lore

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

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:

Show changes to diff +7 −5

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.

Junio C Hamano· Oct 6, 2011, 21:55 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:

Show 5 quoted lines

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

Jay Soffian· Oct 6, 2011, 22:02 UTC · re: Junio C Hamano · lore

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

On Thu, Oct 6, 2011 at 5:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

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