threads / patch / 28603

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

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

## tl;dr

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

replies: 6people: 2as markdown or json

Jay Soffian· Oct 6, 2011, 14:34 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.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 builtin/revert.c                |    6 +++++-
 t/t3507-cherry-pick-conflict.sh |    7 +++++++
 2 files changed, 12 insertions(+), 1 deletions(-)
Show changes to 2 files +12 −1

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 3117776c2c..48526e1ef1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -384,6 +384,7 @@ static int do_pick_commit(void)
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
+	int record_cherry_pick_head = 0;
 
 	if (no_commit) {
 		/*
@@ -477,7 +478,7 @@ static int do_pick_commit(void)
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			record_cherry_pick_head = 1;
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -498,6 +499,9 @@ static int do_pick_commit(void)
 		free_commit_list(remotes);
 	}
 
+	if (record_cherry_pick_head)
+		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..29890bf5ac 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -77,6 +77,13 @@ 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 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	(

-- 
1.7.7.6.g25c34
Junio C Hamano· Oct 6, 2011, 17:02 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
Show 10 quoted lines
> 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.

I think do_recursive_merge() would die() when the merge cannot even start (i.e. the local changes after the cherry-pick exits are the ones from the time before such a failed cherry-pick started), but I suspect that the other codepath uses try_merge_command() to drive strategies other than recursive and it does not die() there in such a case. Can you make sure this patch is sufficient in such a case as well?

Other than that I think this is better than the previous rounds.
Thanks.
Show 55 quoted lines
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
>  builtin/revert.c                |    6 +++++-
>  t/t3507-cherry-pick-conflict.sh |    7 +++++++
>  2 files changed, 12 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 3117776c2c..48526e1ef1 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -384,6 +384,7 @@ static int do_pick_commit(void)
>  	char *defmsg = NULL;
>  	struct strbuf msgbuf = STRBUF_INIT;
>  	int res;
> +	int record_cherry_pick_head = 0;
>  
>  	if (no_commit) {
>  		/*
> @@ -477,7 +478,7 @@ static int do_pick_commit(void)
>  			strbuf_addstr(&msgbuf, ")\n");
>  		}
>  		if (!no_commit)
> -			write_cherry_pick_head();
> +			record_cherry_pick_head = 1;
>  	}
>  
>  	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
> @@ -498,6 +499,9 @@ static int do_pick_commit(void)
>  		free_commit_list(remotes);
>  	}
>  
> +	if (record_cherry_pick_head)
> +		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..29890bf5ac 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -77,6 +77,13 @@ 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 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
>  	pristine_detach initial &&
>  	(
Jay Soffian· Oct 6, 2011, 17:21 UTC · re: Junio C Hamano · lore

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

On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> I think do_recursive_merge() would die() when the merge cannot even start
> (i.e. the local changes after the cherry-pick exits are the ones from the
> time before such a failed cherry-pick started), but I suspect that the
> other codepath uses try_merge_command() to drive strategies other than
> recursive and it does not die() there in such a case. Can you make sure
> this patch is sufficient in such a case as well?
Why does recursive die with a dirty tree but not other strategies?

For that matter, why does revert.c have it's own implementation of recursive instead of just calling try_merge_command("recursive", ...)?

j.
Junio C Hamano· Oct 6, 2011, 17:28 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
> For that matter, why does revert.c have it's own implementation of
> recursive instead of just calling try_merge_command("recursive", ...)?
I think the people who did this part wanted not to fork.
Jay Soffian· Oct 6, 2011, 17:28 UTC · re: Junio C Hamano · lore

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

On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> I think do_recursive_merge() would die() when the merge cannot even start
> (i.e. the local changes after the cherry-pick exits are the ones from the
> time before such a failed cherry-pick started), but I suspect that the
> other codepath uses try_merge_command() to drive strategies other than
> recursive and it does not die() there in such a case. Can you make sure
> this patch is sufficient in such a case as well?

It's wrong to write out CHERRY_PICK_HEAD if we couldn't start the merge, but using cherry-pick with a strategy other than recursive seems broken in that case in other ways as well:

$ git cherry-pick --strategy=resolve side error: Untracked working tree file 'bar' would be overwritten by merge. error: could not apply a77535c9ac... bar hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

The "hint" advice is completely wrong.
j.
Jay Soffian· Oct 6, 2011, 17:33 UTC · re: Jay Soffian · lore

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

On Thu, Oct 6, 2011 at 1:28 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 20 quoted lines
> On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think do_recursive_merge() would die() when the merge cannot even start
>> (i.e. the local changes after the cherry-pick exits are the ones from the
>> time before such a failed cherry-pick started), but I suspect that the
>> other codepath uses try_merge_command() to drive strategies other than
>> recursive and it does not die() there in such a case. Can you make sure
>> this patch is sufficient in such a case as well?
>
> It's wrong to write out CHERRY_PICK_HEAD if we couldn't start the
> merge, but using cherry-pick with a strategy other than recursive
> seems broken in that case in other ways as well:
>
> $ git cherry-pick --strategy=resolve side
> error: Untracked working tree file 'bar' would be overwritten by merge.
> error: could not apply a77535c9ac... bar
> hint: after resolving the conflicts, mark the corrected paths
> hint: with 'git add <paths>' or 'git rm <paths>'
> hint: and commit the result with 'git commit'
>
> The "hint" advice is completely wrong.

For that matter, I don't see how to distinguish "the merge did not even start" from "the merge had conflicts" when using try_merge_command(). In the former case we do NOT want CHERRY_PICK_HEAD left behind (nor to print the wrong advice above), while in the latter case we do.

j.
Junio C Hamano· Oct 6, 2011, 21:49 UTC · re: Jay Soffian · lore

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

Jay Soffian <jaysoffian@gmail.com> writes:
> For that matter, I don't see how to distinguish "the merge did not
> even start" from "the merge had conflicts" when using
> try_merge_command().

I suspect you figured it out by now (sorry I have been tending other topics and general project maintenance issues), but in case you haven't, the exit values from the strategy backends are returned straight back to the caller. The strategy backends are supposed to exit with 1 when they left the conflicts, and 2 when they punted and did not touch the index nor the working tree.

← back to recent threads