{"thread":{"id":"28603","subject":"[PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","startedAt":"2011-10-06T14:34:01Z","lastAt":"2011-10-06T21:49:45Z","messageCount":7,"participants":["Jay Soffian","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"177031","messageId":"1317911641-47660-1-git-send-email-jaysoffian@gmail.com","threadId":"28603","inReplyTo":null,"subject":"[PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-06T14:34:01Z","receivedAt":"2011-10-06T14:34:01Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"do_pick_commit() writes out CHERRY_PICK_HEAD before invoking merge (either\nvia do_recursive_merge() or try_merge_command()) on the assumption that if\nthe merge fails it is due to conflict. However, if the tree is dirty, the\nmerge may not even start, aborting before do_pick_commit() can remove\nCHERRY_PICK_HEAD.\n\nInstead, defer writing CHERRY_PICK_HEAD till after merge has returned.\nAt this point we know the merge has either succeeded or failed due\nto conflict. In either case, we want CHERRY_PICK_HEAD to be written\nso that it may be picked up by the subsequent invocation of commit.\n\nSigned-off-by: Jay Soffian \n---\n builtin/revert.c | 6 +++++-\n t/t3507-cherry-pick-conflict.sh | 7 +++++++\n 2 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 3117776c2c..48526e1ef1 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -384,6 +384,7 @@ static int do_pick_commit(void)\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res;\n+\tint record_cherry_pick_head = 0;\n \n \tif (no_commit) {\n \t\t/*\n@@ -477,7 +478,7 @@ static int do_pick_commit(void)\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n \t\tif (!no_commit)\n-\t\t\twrite_cherry_pick_head();\n+\t\t\trecord_cherry_pick_head = 1;\n \t}\n \n \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n@@ -498,6 +499,9 @@ static int do_pick_commit(void)\n \t\tfree_commit_list(remotes);\n \t}\n \n+\tif (record_cherry_pick_head)\n+\t\twrite_cherry_pick_head();\n+\n \tif (res) {\n \t\terror(action == REVERT\n \t\t ? _(\"could not revert %s... %s\")\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 212ec54aaf..29890bf5ac 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -77,6 +77,13 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n \n+test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '\n+\tpristine_detach initial &&\n+\techo foo > foo &&\n+\ttest_must_fail git cherry-pick base &&\n+\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n+'\n+\n test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \t(\n-- \n1.7.7.6.g25c34\n"},{"id":"177059","messageId":"7vty7m12b2.fsf@alter.siamese.dyndns.org","threadId":"28603","inReplyTo":"1317911641-47660-1-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-06T17:02:09Z","receivedAt":"2011-10-06T17:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> do_pick_commit() writes out CHERRY_PICK_HEAD before invoking merge (either\n> via do_recursive_merge() or try_merge_command()) on the assumption that if\n> the merge fails it is due to conflict. However, if the tree is dirty, the\n> merge may not even start, aborting before do_pick_commit() can remove\n> CHERRY_PICK_HEAD.\n>\n> Instead, defer writing CHERRY_PICK_HEAD till after merge has returned.\n> At this point we know the merge has either succeeded or failed due\n> to conflict. In either case, we want CHERRY_PICK_HEAD to be written\n> so that it may be picked up by the subsequent invocation of commit.\n\nI think do_recursive_merge() would die() when the merge cannot even start\n(i.e. the local changes after the cherry-pick exits are the ones from the\ntime before such a failed cherry-pick started), but I suspect that the\nother codepath uses try_merge_command() to drive strategies other than\nrecursive and it does not die() there in such a case. Can you make sure\nthis patch is sufficient in such a case as well?\n\nOther than that I think this is better than the previous rounds.\n\nThanks.\n\n> Signed-off-by: Jay Soffian \n> ---\n> builtin/revert.c | 6 +++++-\n> t/t3507-cherry-pick-conflict.sh | 7 +++++++\n> 2 files changed, 12 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin/revert.c b/builtin/revert.c\n> index 3117776c2c..48526e1ef1 100644\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -384,6 +384,7 @@ static int do_pick_commit(void)\n> \tchar *defmsg = NULL;\n> \tstruct strbuf msgbuf = STRBUF_INIT;\n> \tint res;\n> +\tint record_cherry_pick_head = 0;\n> \n> \tif (no_commit) {\n> \t\t/*\n> @@ -477,7 +478,7 @@ static int do_pick_commit(void)\n> \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n> \t\t}\n> \t\tif (!no_commit)\n> -\t\t\twrite_cherry_pick_head();\n> +\t\t\trecord_cherry_pick_head = 1;\n> \t}\n> \n> \tif (!strategy || !strcmp(strategy, \"recursive\") || action == REVERT) {\n> @@ -498,6 +499,9 @@ static int do_pick_commit(void)\n> \t\tfree_commit_list(remotes);\n> \t}\n> \n> +\tif (record_cherry_pick_head)\n> +\t\twrite_cherry_pick_head();\n> +\n> \tif (res) {\n> \t\terror(action == REVERT\n> \t\t ? _(\"could not revert %s... %s\")\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 212ec54aaf..29890bf5ac 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -77,6 +77,13 @@ test_expect_success 'cherry-pick --no-commit does not set CHERRY_PICK_HEAD' '\n> \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> '\n> \n> +test_expect_success 'cherry-pick w/dirty tree does not set CHERRY_PICK_HEAD' '\n> +\tpristine_detach initial &&\n> +\techo foo > foo &&\n> +\ttest_must_fail git cherry-pick base &&\n> +\ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n> +'\n> +\n> test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '\n> \tpristine_detach initial &&\n> \t(\n"},{"id":"177063","messageId":"CAG+J_Dzh2Njjwykt-f4w_YqpftrJEpQfUW2OvSRs_MSPcNFQnw@mail.gmail.com","threadId":"28603","inReplyTo":"7vty7m12b2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-06T17:21:40Z","receivedAt":"2011-10-06T17:21:40Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano wrote:\n> I think do_recursive_merge() would die() when the merge cannot even start\n> (i.e. the local changes after the cherry-pick exits are the ones from the\n> time before such a failed cherry-pick started), but I suspect that the\n> other codepath uses try_merge_command() to drive strategies other than\n> recursive and it does not die() there in such a case. Can you make sure\n> this patch is sufficient in such a case as well?\n\nWhy does recursive die with a dirty tree but not other strategies?\n\nFor that matter, why does revert.c have it's own implementation of\nrecursive instead of just calling try_merge_command(\"recursive\", ...)?\n\nj.\n"},{"id":"177070","messageId":"CAG+J_DxqpDAm-qPR-Udkr_b1gc=Y+LoKbsQdmiCi6ztNKz0_Mg@mail.gmail.com","threadId":"28603","inReplyTo":"7vty7m12b2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-06T17:28:06Z","receivedAt":"2011-10-06T17:28:06Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano wrote:\n> I think do_recursive_merge() would die() when the merge cannot even start\n> (i.e. the local changes after the cherry-pick exits are the ones from the\n> time before such a failed cherry-pick started), but I suspect that the\n> other codepath uses try_merge_command() to drive strategies other than\n> recursive and it does not die() there in such a case. Can you make sure\n> this patch is sufficient in such a case as well?\n\nIt's wrong to write out CHERRY_PICK_HEAD if we couldn't start the\nmerge, but using cherry-pick with a strategy other than recursive\nseems broken in that case in other ways as well:\n\n$ git cherry-pick --strategy=resolve side\nerror: Untracked working tree file 'bar' would be overwritten by merge.\nerror: could not apply a77535c9ac... bar\nhint: after resolving the conflicts, mark the corrected paths\nhint: with 'git add ' or 'git rm '\nhint: and commit the result with 'git commit'\n\nThe \"hint\" advice is completely wrong.\n\nj.\n"},{"id":"177071","messageId":"7vaa9e113d.fsf@alter.siamese.dyndns.org","threadId":"28603","inReplyTo":"CAG+J_Dzh2Njjwykt-f4w_YqpftrJEpQfUW2OvSRs_MSPcNFQnw@mail.gmail.com","subject":"Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-06T17:28:22Z","receivedAt":"2011-10-06T17:28:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> For that matter, why does revert.c have it's own implementation of\n> recursive instead of just calling try_merge_command(\"recursive\", ...)?\n\nI think the people who did this part wanted not to fork.\n"},{"id":"177074","messageId":"CAG+J_Dx0W0oRL-MkoZSGuOfmUNCqmMRUigmgND_0o7kbKppu1Q@mail.gmail.com","threadId":"28603","inReplyTo":"CAG+J_DxqpDAm-qPR-Udkr_b1gc=Y+LoKbsQdmiCi6ztNKz0_Mg@mail.gmail.com","subject":"Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-10-06T17:33:23Z","receivedAt":"2011-10-06T17:33:23Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Oct 6, 2011 at 1:28 PM, Jay Soffian wrote:\n> On Thu, Oct 6, 2011 at 1:02 PM, Junio C Hamano wrote:\n>> I think do_recursive_merge() would die() when the merge cannot even start\n>> (i.e. the local changes after the cherry-pick exits are the ones from the\n>> time before such a failed cherry-pick started), but I suspect that the\n>> other codepath uses try_merge_command() to drive strategies other than\n>> recursive and it does not die() there in such a case. Can you make sure\n>> this patch is sufficient in such a case as well?\n>\n> It's wrong to write out CHERRY_PICK_HEAD if we couldn't start the\n> merge, but using cherry-pick with a strategy other than recursive\n> seems broken in that case in other ways as well:\n>\n> $ git cherry-pick --strategy=resolve side\n> error: Untracked working tree file 'bar' would be overwritten by merge.\n> error: could not apply a77535c9ac... bar\n> hint: after resolving the conflicts, mark the corrected paths\n> hint: with 'git add ' or 'git rm '\n> hint: and commit the result with 'git commit'\n>\n> The \"hint\" advice is completely wrong.\n\nFor that matter, I don't see how to distinguish \"the merge did not\neven start\" from \"the merge had conflicts\" when using\ntry_merge_command(). In the former case we do NOT want\nCHERRY_PICK_HEAD left behind (nor to print the wrong advice above),\nwhile in the latter case we do.\n\nj.\n"},{"id":"177116","messageId":"7v4nzlzt6u.fsf@alter.siamese.dyndns.org","threadId":"28603","inReplyTo":"CAG+J_Dx0W0oRL-MkoZSGuOfmUNCqmMRUigmgND_0o7kbKppu1Q@mail.gmail.com","subject":"Re: [PATCH] revert.c: defer writing CHERRY_PICK_HEAD till it is safe to do so","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-06T21:49:45Z","receivedAt":"2011-10-06T21:49:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian writes:\n\n> For that matter, I don't see how to distinguish \"the merge did not\n> even start\" from \"the merge had conflicts\" when using\n> try_merge_command().\n\nI suspect you figured it out by now (sorry I have been tending other\ntopics and general project maintenance issues), but in case you haven't,\nthe exit values from the strategy backends are returned straight back to\nthe caller. The strategy backends are supposed to exit with 1 when they\nleft the conflicts, and 2 when they punted and did not touch the index nor\nthe working tree.\n"}]}