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

[PATCH] 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.

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 &&
 	(
-- 
1.7.7.6.g25c34

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

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

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

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

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

[Jay Soffian]

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?

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.

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

[Junio C Hamano]

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.

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

[Jay Soffian]

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.

j.

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

[Jay Soffian]

On Thu, Oct 6, 2011 at 1:28 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> 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.

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

[Junio C Hamano]

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.