{"thread":{"id":"29685","subject":"[PATCH] cherry-pick: No advice to commit if --no-commit","startedAt":"2012-02-21T21:05:17Z","lastAt":"2012-02-22T00:18:16Z","messageCount":4,"participants":["Phil Hord","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"185103","messageId":"1329858317-3066-1-git-send-email-hordp@cisco.com","threadId":"29685","inReplyTo":null,"subject":"[PATCH] cherry-pick: No advice to commit if --no-commit","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-02-21T21:05:17Z","receivedAt":"2012-02-21T21:05:17Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"When cherry-pick fails it offers a helpful hint about how to\nproceed. The hint tells the user how to do the cleanup\nneeded by the conflicted cherry-pick and finish the job after\nconflict resolution. In case of cherry-pick --no-commit, the\nhint goes too far. It tells the user to finish up with\n'git commit'. That is not what this git-cherry-pick was\ntrying to do in the first place.\n\nRestrict the hint in case of --no-commit to avoid giving this\nextra advice.\n\nAlso, add a test verifying the reduced hint for the --no-commit\nversion of cherry-pick.\n\nSigned-off-by: Phil Hord \n---\n sequencer.c | 12 +++++++-----\n t/t3507-cherry-pick-conflict.sh | 14 ++++++++++++++\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 5fcbcb8..b7965e8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -123,7 +123,7 @@ static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)\n \tstrbuf_release(&buf);\n }\n \n-static void print_advice(int show_hint)\n+static void print_advice(int show_hint, struct replay_opts *opts)\n {\n \tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n \n@@ -138,10 +138,12 @@ static void print_advice(int show_hint)\n \t\treturn;\n \t}\n \n-\tif (show_hint)\n+\tif (show_hint) {\n \t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n-\t\t\t \"with 'git add ' or 'git rm '\\n\"\n-\t\t\t \"and commit the result with 'git commit'\"));\n+\t\t\t \"with 'git add ' or 'git rm '\"));\n+\t\tif (!opts->no_commit)\n+\t\t\tadvise(_( \"and commit the result with 'git commit'\"));\n+ }\n }\n \n static void write_message(struct strbuf *msgbuf, const char *filename)\n@@ -423,7 +425,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t : _(\"could not apply %s... %s\"),\n \t\t find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n \t\t msg.subject);\n-\t\tprint_advice(res == 1);\n+\t\tprint_advice(res == 1, opts);\n \t\trerere(opts->allow_rerere_auto);\n \t} else {\n \t\tif (!opts->no_commit)\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex ee1659c..0c81b3c 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -59,6 +59,20 @@ test_expect_success 'advice from failed cherry-pick' \"\n \ttest_i18ncmp expected actual\n \"\n \n+test_expect_success 'advice from failed cherry-pick --no-commit' \"\n+\tpristine_detach initial &&\n+\n+\tpicked=\\$(git rev-parse --short picked) &&\n+\tcat <<-EOF >expected &&\n+\terror: could not apply \\$picked... picked\n+\thint: after resolving the conflicts, mark the corrected paths\n+\thint: with 'git add ' or 'git rm '\n+\tEOF\n+\ttest_must_fail git cherry-pick --no-commit picked 2>actual &&\n+\n+\ttest_i18ncmp expected actual\n+\"\n+\n test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick picked &&\n-- \n1.7.9.267.gda172.dirty\n"},{"id":"185109","messageId":"20120221222049.GA31934@burratino","threadId":"29685","inReplyTo":"1329858317-3066-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] cherry-pick: No advice to commit if --no-commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-21T22:23:22Z","receivedAt":"2012-02-21T22:23:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Phil,\n\nPhil Hord wrote:\n\n> Signed-off-by: Phil Hord \n\nThanks.\n\n> +++ b/sequencer.c\n[...]\n> @@ -138,10 +138,12 @@ static void print_advice(int show_hint)\n> \t\treturn;\n> \t}\n> \n> -\tif (show_hint)\n> +\tif (show_hint) {\n> \t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n> -\t\t\t \"with 'git add ' or 'git rm '\\n\"\n> -\t\t\t \"and commit the result with 'git commit'\"));\n> +\t\t\t \"with 'git add ' or 'git rm '\"));\n> +\t\tif (!opts->no_commit)\n> +\t\t\tadvise(_( \"and commit the result with 'git commit'\"));\n\n\"cherry-pick --no-commit\" was not about to commit, but the user might\nhave been. I think the hint is intended to convey that authorship\nwill be correctly preserved if the user continues with \"git commit\"\nand no special -c option is necessary.\n\nCould you say a little more about the motivation for this patch? For\nexample, did the existing message confuse someone, or was it grating\nin the context of some particular workflow?\n\nA smaller detail: splitting the message into two like this gives\ntranslators less control over how to phrase the message and where to\nwrap lines. Luckily that is easy to fix with\n\n\tif (opts->no_commit)\n\t\tadvise(...);\n\telse\n\t\tadvise(...);\n\nwhich means more flexibility in phrasing the message with pertinent\nadvice for each case. ;-)\n\nCiao,\nJonathan\n"},{"id":"185110","messageId":"7v1upng6n7.fsf@alter.siamese.dyndns.org","threadId":"29685","inReplyTo":"20120221222049.GA31934@burratino","subject":"Re: [PATCH] cherry-pick: No advice to commit if --no-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-21T22:31:56Z","receivedAt":"2012-02-21T22:31:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> +\tif (show_hint) {\n>> \t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n>> +\t\t\t \"with 'git add ' or 'git rm '\"));\n>> +\t\tif (!opts->no_commit)\n>> +\t\t\tadvise(_( \"and commit the result with 'git commit'\"));\n>\n> \"cherry-pick --no-commit\" was not about to commit, but the user might\n> have been. I think the hint is intended to convey that authorship\n> will be correctly preserved if the user continues with \"git commit\"\n> and no special -c option is necessary.\n\nActually, an often used but perhaps neglected use case of \"cherry-pick --no-commit\"\nis to use it as a substitute for\n\n\tgit show $that_commit | git apply [--index]\n\nto lift the change from $that_commit and incorporate it into your\nunrelated (from the point of view of the author of $that_commit) work.\nThe user would be doing \"edit $this_file\" and \"edit $that_file\" before or\nafter the \"show | apply\" aka \"cherry-pick --no-commit\", and it is merely\na type-saver, not having to re-type the change $that_commit introduced\nrelative to its parent.\n\nSo in that context, I can understand the suggestion to commit is at best\nannoying (\"I am still working on the current change. Why do you talk about\ncommitting?\") and at worst alarming and misleading (\"Oh, am I to lose some\nchange if I do not commit right now?\").\n\n> A smaller detail: splitting the message into two like this gives\n> translators less control over how to phrase the message and where to\n> wrap lines. Luckily that is easy to fix with\n>\n> \tif (opts->no_commit)\n> \t\tadvise(...);\n> \telse\n> \t\tadvise(...);\n\nYes, this pattern must be followed in the reroll.\n\nThanks for a review and comment.\n"},{"id":"185121","messageId":"4F443448.7010705@cisco.com","threadId":"29685","inReplyTo":"20120221222049.GA31934@burratino","subject":"Re: [PATCH] cherry-pick: No advice to commit if --no-commit","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-02-22T00:18:16Z","receivedAt":"2012-02-22T00:18:16Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Jonathan Nieder wrote:\n\n>> -\tif (show_hint)\n>> +\tif (show_hint) {\n>> \t\tadvise(_(\"after resolving the conflicts, mark the corrected paths\\n\"\n>> -\t\t\t \"with 'git add ' or 'git rm '\\n\"\n>> -\t\t\t \"and commit the result with 'git commit'\"));\n>> +\t\t\t \"with 'git add ' or 'git rm '\"));\n>> +\t\tif (!opts->no_commit)\n>> +\t\t\tadvise(_( \"and commit the result with 'git commit'\"));\n> \n> \"cherry-pick --no-commit\" was not about to commit, but the user might\n> have been. I think the hint is intended to convey that authorship\n> will be correctly preserved if the user continues with \"git commit\"\n> and no special -c option is necessary.\n\nIf that were the case, the hint would also appear when there is no conflict.\n\n> Could you say a little more about the motivation for this patch? For\n> example, did the existing message confuse someone, or was it grating\n> in the context of some particular workflow?\n\nI found it mildly confusing myself. I cherry-picked a commit with\n--no-commit with no intention of committing it. I was testing how the\nchanges would build, but I do not need them on my branch yet. After I\nresolved the conflict and tested them, I wanted to make sure there was\nno lingering effects, leaving git thinking a CP was still in progress.\n\n $ git cherry-pick --abort\n error: no cherry-pick or revert in progress\n\nOk, so the sequencer was smart enough to leave me on my own. But just\nin case, I wondered what the hint was. And I found it was not telling me\nhow to clean up at all, but instead telling me how to commit. It seemed\nincongruous, and I assumed it was only someone's forgetting to consider\nthe --no-commit case.\n\nIt smelled like a bug. I started to ask about it, but it seemed easier\nto just correct it and lower the list noise.\n\n> A smaller detail: splitting the message into two like this gives\n> translators less control over how to phrase the message and where to\n> wrap lines. Luckily that is easy to fix with\n> \n> \tif (opts->no_commit)\n> \t\tadvise(...);\n> \telse\n> \t\tadvise(...);\n>\n> which means more flexibility in phrasing the message with pertinent\n> advice for each case. ;-)\n\nI did this at first and didn't like it. I started to ask, but -- you\nknow, list noise.\n\nI'll fix it in v2.\n\nThanks,\nPhil\n"}]}