{"thread":{"id":"34546","subject":"[PATCH] commit: correct advice about aborting a cherry-pick","startedAt":"2013-07-26T18:12:00Z","lastAt":"2013-07-29T15:48:10Z","messageCount":18,"participants":["Ramkumar Ramachandra","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"224134","messageId":"1374862320-22637-1-git-send-email-artagnon@gmail.com","threadId":"34546","inReplyTo":null,"subject":"[PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-26T18:12:00Z","receivedAt":"2013-07-26T18:12:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"When a cherry-pick results in an empty commit, git prints:\n\n The previous cherry-pick is now empty, possibly due to conflict resolution.\n If you wish to commit it anyway, use:\n\n git commit --allow-empty\n\n Otherwise, please use 'git reset'\n\nThe last line is plain wrong in the case of a ranged pick, as a 'git\nreset' will fail to remove $GIT_DIR/sequencer, failing a subsequent\ncherry-pick or revert. Change the advice to:\n\n git cherry-pick --abort\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Another candidate for maint?\n\n I'd also really like to squelch this with an advice.* variable; any\n suggestions?\n\n builtin/commit.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 003bd7d..1b213f7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -64,7 +64,10 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \"\\n\"\n \" git commit --allow-empty\\n\"\n \"\\n\"\n-\"Otherwise, please use 'git reset'\\n\");\n+\"Otherwise, use:\\n\"\n+\"\\n\"\n+\" git cherry-pick --abort\\n\"\n+\"\\n\");\n \n static const char *use_message_buffer;\n static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n-- \n1.8.4.rc0.1.g8f6a3e5.dirty\n"},{"id":"224139","messageId":"20130726191631.GD29799@sigill.intra.peff.net","threadId":"34546","inReplyTo":"1374862320-22637-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T19:16:31Z","receivedAt":"2013-07-26T19:16:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2013 at 11:42:00PM +0530, Ramkumar Ramachandra wrote:\n\n> When a cherry-pick results in an empty commit, git prints:\n> \n> The previous cherry-pick is now empty, possibly due to conflict resolution.\n> If you wish to commit it anyway, use:\n> \n> git commit --allow-empty\n> \n> Otherwise, please use 'git reset'\n> \n> The last line is plain wrong in the case of a ranged pick, as a 'git\n> reset' will fail to remove $GIT_DIR/sequencer, failing a subsequent\n> cherry-pick or revert. Change the advice to:\n> \n> git cherry-pick --abort\n\nHmm. I don't think I've run across this message myself, so perhaps I do\nnot understand the situation. But it seems like you would want to do one\nof:\n\n 1. Make an empty commit.\n\n 2. Skip this commit and continue the rest of the cherry-pick sequence.\n\n 3. Abort the cherry pick sequence.\n\nThose are the options presented when rebase runs into an empty commit,\nwhere (2) is presented as \"rebase --skip\". I'm not sure how to do that\nhere; is it just \"cherry-pick --continue\"?\n\n> I'd also really like to squelch this with an advice.* variable; any\n> suggestions?\n\nThis seems like a good candidate for squelching, but you would probably\nwant to split it. The two parts of the message are:\n\n 1. What happened (the cherry-pick is empty).\n\n 2. How to proceed from here (allow-empty, abort, etc).\n\nYou still want to say (1), but (2) is useless to old-timers. Probably\nsomething like advice.cherryPickInstructions would be a good name for an\noption to squelch (2), and it should apply wherever we tell the user how\nto proceed. Potentially it should even be advice.sequenceInstructions,\nand apply to rebase and am as well.\n\n-Peff\n"},{"id":"224145","messageId":"CALkWK0=qYF=r+Ocb1Z1E=Oteau=AAXR7wnKakt-8Cejwz6Usrg@mail.gmail.com","threadId":"34546","inReplyTo":"20130726191631.GD29799@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-26T21:17:47Z","receivedAt":"2013-07-26T21:17:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> Hmm. I don't think I've run across this message myself, so perhaps I do\n> not understand the situation.\n\nIt's very simple.\n\n # on branch master\n $ git checkout -b test\n $ git cherry-pick master\n $ ls .git/sequencer\n # missing\n\nIn the pseudo multi-pick case (I say \"pseudo\" because there's really\njust one commit to pick):\n\n # on branch master\n $ git checkout -b test\n $ git cherry-pick master~..\n $ ls .git/sequencer\n\ncat .git/sequencer/todo if you like.\n\n> 2. Skip this commit and continue the rest of the cherry-pick sequence.\n\nNope, this is unsupported afaik.\n\n> Those are the options presented when rebase runs into an empty commit,\n> where (2) is presented as \"rebase --skip\". I'm not sure how to do that\n> here; is it just \"cherry-pick --continue\"?\n\nNo, --continue will just print the same message over and over again.\n\nYes, the whole ranged cherry-pick thing can use a lot more polish.\n\n> 1. What happened (the cherry-pick is empty).\n>\n> 2. How to proceed from here (allow-empty, abort, etc).\n>\n> You still want to say (1), but (2) is useless to old-timers. Probably\n> something like advice.cherryPickInstructions would be a good name for an\n> option to squelch (2), and it should apply wherever we tell the user how\n> to proceed. Potentially it should even be advice.sequenceInstructions,\n> and apply to rebase and am as well.\n\nGood suggestion. I'll pick advice.cherryPickInstructions when I\ndecide to polish sequencer.c a bit.\n\nThanks.\n"},{"id":"224146","messageId":"20130726212438.GA1388@sigill.intra.peff.net","threadId":"34546","inReplyTo":"CALkWK0=qYF=r+Ocb1Z1E=Oteau=AAXR7wnKakt-8Cejwz6Usrg@mail.gmail.com","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T21:24:38Z","receivedAt":"2013-07-26T21:24:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 27, 2013 at 02:47:47AM +0530, Ramkumar Ramachandra wrote:\n\n> > 2. Skip this commit and continue the rest of the cherry-pick sequence.\n> \n> Nope, this is unsupported afaik.\n\nAh. I don't mind improving the message in the meantime, but it sounds\nlike this is a deficiency in sequenced cherry-pick that needs addressed\neventually.\n\nYour patch is just swapping out \"git reset\" for \"cherry-pick --abort\",\nso I think that is a good improvement in the meantime.\n\n-Peff\n"},{"id":"224147","messageId":"CALkWK0mC0Zcxnq9c-SQc0zURs1Lu1PfL988vdw=Crvb4C7+Ctw@mail.gmail.com","threadId":"34546","inReplyTo":"20130726212438.GA1388@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-26T21:27:00Z","receivedAt":"2013-07-26T21:27:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> Ah. I don't mind improving the message in the meantime, but it sounds\n> like this is a deficiency in sequenced cherry-pick that needs addressed\n> eventually.\n\nI'm especially irked by how slow rebase--am is, and want to replace it.\n"},{"id":"224148","messageId":"20130726213705.GJ14690@google.com","threadId":"34546","inReplyTo":"20130726212438.GA1388@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-26T21:37:05Z","receivedAt":"2013-07-26T21:37:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Your patch is just swapping out \"git reset\" for \"cherry-pick --abort\",\n> so I think that is a good improvement in the meantime.\n\nUm, wasn't the idea of the original message that you can run \"git\nreset\" and then \"git cherry-pick --continue\"?\n\nConfused,\nJonathan\n"},{"id":"224149","messageId":"20130726214036.GB1388@sigill.intra.peff.net","threadId":"34546","inReplyTo":"20130726213705.GJ14690@google.com","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T21:40:36Z","receivedAt":"2013-07-26T21:40:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2013 at 02:37:05PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Your patch is just swapping out \"git reset\" for \"cherry-pick --abort\",\n> > so I think that is a good improvement in the meantime.\n> \n> Um, wasn't the idea of the original message that you can run \"git\n> reset\" and then \"git cherry-pick --continue\"?\n\nMaybe. :)\n\nI missed that subtlety. Of my \"three things you would want to do\", that\nmeans it was _trying_ say number 2, how to skip, rather than 3, how to\nabort. If that is the case, then it should probably explain the sequence\nof steps as \"reset and then --continue\" to make it more clear.\n\nI.e., a patch is needed, but Ram's is going in the opposite direction.\n\n-Peff\n"},{"id":"224153","messageId":"20130726224359.GA3928@sigill.intra.peff.net","threadId":"34546","inReplyTo":"20130726214036.GB1388@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T22:43:59Z","receivedAt":"2013-07-26T22:43:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2013 at 05:40:36PM -0400, Jeff King wrote:\n\n> > Jeff King wrote:\n> > \n> > > Your patch is just swapping out \"git reset\" for \"cherry-pick --abort\",\n> > > so I think that is a good improvement in the meantime.\n> > \n> > Um, wasn't the idea of the original message that you can run \"git\n> > reset\" and then \"git cherry-pick --continue\"?\n> \n> Maybe. :)\n> \n> I missed that subtlety. Of my \"three things you would want to do\", that\n> means it was _trying_ say number 2, how to skip, rather than 3, how to\n> abort. If that is the case, then it should probably explain the sequence\n> of steps as \"reset and then --continue\" to make it more clear.\n> \n> I.e., a patch is needed, but Ram's is going in the opposite direction.\n\nI played around a bit with the test cases that Ram showed. It seems like\nthe advice needed is different depending on whether you are in a single\nor multi-commit cherry-pick.\n\nSo if we hit an empty commit and you want to:\n\n 1. Make an empty commit, then always run \"git commit --allow-empty\".\n\n 2. Skip this commit, then if:\n\n a. this is a single commit cherry-pick, you run \"git reset\" (and\n nothing more, the cherry pick is finished; running \"cherry-pick\n --continue\" will yield an error).\n\n b. this is a multi-commit cherry-pick, you run \"git reset\",\n followed by \"git cherry-pick --continue\"\n\n 3. Abort the commit, run \"git cherry-pick --abort\"\n\nLet's assume that the instructions we want to give the user are how to\ndo options 1 and 2. I do not mind omitting 3, as it should be reasonably\nobvious that \"cherry-pick --abort\" is always good way to abort.\n\nSo we give good instructions for the single-commit case, but bad\ninstructions for the multi-commit case. Ram's patch suggests --abort\ninstead of reset, which is the same for the single-commit case, but\nsuggests 3 instead of 2 for the multi-patch.\n\nI think instead we would want to leave the single-commit case alone, and\nfor the multi-commit case add \"...and then cherry-pick --continue\". That\nmessage is generated from within git-commit, though; I guess it would\nneed to learn about the difference between single/multi cherry-picks.\n\n-Peff\n"},{"id":"224155","messageId":"20130726230527.GA12968@sigill.intra.peff.net","threadId":"34546","inReplyTo":"20130726224359.GA3928@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T23:05:27Z","receivedAt":"2013-07-26T23:05:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2013 at 06:43:59PM -0400, Jeff King wrote:\n\n> I think instead we would want to leave the single-commit case alone, and\n> for the multi-commit case add \"...and then cherry-pick --continue\". That\n> message is generated from within git-commit, though; I guess it would\n> need to learn about the difference between single/multi cherry-picks.\n\nLike this?\n\n-- >8 --\nSubject: [PATCH] commit: tweak empty cherry pick advice for sequencer\n\nWhen we refuse to make an empty commit, we check whether we\nare in a cherry-pick in order to give better advice on how\nto proceed. We instruct the user to repeat the commit with\n\"--allow-empty\" to force the commit, or to use \"git reset\"\nto skip it and abort the cherry-pick.\n\nThis works fine when we are cherry-picking a single commit.\nWhen we are using the sequencer to cherry-pick multiple\nitems, though, using \"git reset\" is not good advice. It does\nnot skip the commit (you must run \"cherry-pick --continue\"\nto keep going), but nor does it abort the cherry-pick (the\n.sequencer directory still exists).\n\nLet's teach commit to tell when the sequencer is in use, and\nto mention \"cherry-pick --continue\" in that case.\n\nSigned-off-by: Jeff King \n---\nThe advice in the multi case could eventually change to \"cherry-pick\n--skip\" if and when it exists.\n\n builtin/commit.c | 23 ++++++++++++++++++++---\n 1 file changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e47f100..45a98d7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -63,8 +63,14 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \"If you wish to commit it anyway, use:\\n\"\n \"\\n\"\n \" git commit --allow-empty\\n\"\n+\"\\n\");\n+static const char empty_cherry_pick_advice_skip_single[] =\n+N_(\"Otherwise, please use 'git reset'\\n\");\n+static const char empty_cherry_pick_advice_skip_multi[] =\n+N_(\"If you wish to skip this commit, use:\\n\"\n \"\\n\"\n-\"Otherwise, please use 'git reset'\\n\");\n+\" git reset && git cherry-pick --continue\\n\"\n+\"\\n\");\n \n static const char *use_message_buffer;\n static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n@@ -107,6 +113,7 @@ static enum commit_whence whence;\n static const char *cleanup_arg;\n \n static enum commit_whence whence;\n+static int sequencer_in_use;\n static int use_editor = 1, include_status = 1;\n static int show_ignored_in_status, have_option_m;\n static const char *only_include_assumed;\n@@ -141,8 +148,11 @@ static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path(\"MERGE_HEAD\")))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\")))\n+\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n \t\twhence = FROM_CHERRY_PICK;\n+\t\tif (file_exists(git_path(\"sequencer\")))\n+\t\t\tsequencer_in_use = 1;\n+\t}\n \telse\n \t\twhence = FROM_COMMIT;\n \tif (s)\n@@ -808,8 +818,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n-\t\telse if (whence == FROM_CHERRY_PICK)\n+\t\telse if (whence == FROM_CHERRY_PICK) {\n \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n+\t\t\tif (!sequencer_in_use)\n+\t\t\t\tfputs(_(empty_cherry_pick_advice_skip_single),\n+\t\t\t\t stderr);\n+\t\t\telse\n+\t\t\t\tfputs(_(empty_cherry_pick_advice_skip_multi),\n+\t\t\t\t stderr);\n+\t\t}\n \t\treturn 0;\n \t}\n \n-- \n1.8.3.rc1.30.gff0fb75\n"},{"id":"224156","messageId":"20130726230857.GK14690@google.com","threadId":"34546","inReplyTo":"20130726230527.GA12968@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-26T23:08:57Z","receivedAt":"2013-07-26T23:08:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -63,8 +63,14 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n> \"If you wish to commit it anyway, use:\\n\"\n> \"\\n\"\n> \" git commit --allow-empty\\n\"\n> +\"\\n\");\n> +static const char empty_cherry_pick_advice_skip_single[] =\n> +N_(\"Otherwise, please use 'git reset'\\n\");\n> +static const char empty_cherry_pick_advice_skip_multi[] =\n> +N_(\"If you wish to skip this commit, use:\\n\"\n> \"\\n\"\n> -\"Otherwise, please use 'git reset'\\n\");\n> +\" git reset && git cherry-pick --continue\\n\"\n> +\"\\n\");\n\nHmm, wouldn't it be more consistent to either say\n\n\tIf you wish to commit it anyway, use\n\n\t\tgit commit --allow-empty && git cherry-pick --continue\n\n\tIf you wish to skip this commit, use\n\n\t\tgit reset && git cherry-pick --continue\n\nOr\n\n\tIf you wish to commit it anyway, use\n\n\t\tgit commit --allow-empty\n\t\n\tIf you wish to skip this commit, use\n\n\t\tgit reset\n\n\tThen \"git cherry-pick --continue\" will resume cherry-picking\n\tthe remaining commits.\n\n?\n"},{"id":"224158","messageId":"20130726231902.GA1674@sigill.intra.peff.net","threadId":"34546","inReplyTo":"20130726230857.GK14690@google.com","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T23:19:02Z","receivedAt":"2013-07-26T23:19:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2013 at 04:08:57PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -63,8 +63,14 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n> > \"If you wish to commit it anyway, use:\\n\"\n> > \"\\n\"\n> > \" git commit --allow-empty\\n\"\n> > +\"\\n\");\n> > +static const char empty_cherry_pick_advice_skip_single[] =\n> > +N_(\"Otherwise, please use 'git reset'\\n\");\n> > +static const char empty_cherry_pick_advice_skip_multi[] =\n> > +N_(\"If you wish to skip this commit, use:\\n\"\n> > \"\\n\"\n> > -\"Otherwise, please use 'git reset'\\n\");\n> > +\" git reset && git cherry-pick --continue\\n\"\n> > +\"\\n\");\n> \n> Hmm, wouldn't it be more consistent to either say\n> \n> \tIf you wish to commit it anyway, use\n> \n> \t\tgit commit --allow-empty && git cherry-pick --continue\n> \n> \tIf you wish to skip this commit, use\n> \n> \t\tgit reset && git cherry-pick --continue\n\nGood point. Clearly the original assumed that you knew to \"cherry-pick\n--continue\", since it is needed (and omitted) in both cases. And perhaps\nmost people do, but certainly the lack of mentioning it confused both me\nand Ram about whether the \"git reset\" advice was meant to skip or abort.\n\n> Or\n> \n> \tIf you wish to commit it anyway, use\n> \n> \t\tgit commit --allow-empty\n> \t\n> \tIf you wish to skip this commit, use\n> \n> \t\tgit reset\n> \n> \tThen \"git cherry-pick --continue\" will resume cherry-picking\n> \tthe remaining commits.\n\nI like this one better.\n\nYou could _almost_ just use the top bit for the single-commit case, but\nI hesitate to use the word \"skip\" in that case. Right now the\nsingle-commit case does not need to make the distinction between \"skip\nthis, and there is nothing else to do\" and \"abort the operation\",\nbecause they are the same thing. Whichever way the user thinks about it\nis OK.\n\n-Peff\n"},{"id":"224160","messageId":"20130726233928.GD12968@sigill.intra.peff.net","threadId":"34546","inReplyTo":"20130726231902.GA1674@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-26T23:39:28Z","receivedAt":"2013-07-26T23:39:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 26, 2013 at 07:19:02PM -0400, Jeff King wrote:\n\n> > Or\n> > \n> > \tIf you wish to commit it anyway, use\n> > \n> > \t\tgit commit --allow-empty\n> > \t\n> > \tIf you wish to skip this commit, use\n> > \n> > \t\tgit reset\n> > \n> > \tThen \"git cherry-pick --continue\" will resume cherry-picking\n> > \tthe remaining commits.\n> \n> I like this one better.\n\nHere it is in patch form, with an updated commit message that hopefully\nexplains the rationale a bit better.\n\n-- >8 --\nSubject: [PATCH] commit: tweak empty cherry pick advice for sequencer\n\nWhen we refuse to make an empty commit, we check whether we\nare in a cherry-pick in order to give better advice on how\nto proceed. We instruct the user to repeat the commit with\n\"--allow-empty\" to force the commit, or to use \"git reset\"\nto skip it and abort the cherry-pick.\n\nIn the case of a single cherry-pick, the distinction between\nskipping and aborting is not important, as there is no more\nwork to be done afterwards. When we are using the sequencer\nto cherry pick a series of commits, though, the instruction\nis confusing: does it skip this commit, or does it abort the\nrest of the cherry-pick?\n\nIt does skip, after which the user can continue the\ncherry-pick. This is the right thing to be advising the user\nto do, but let's make it more clear what will happen, both\nby using the word \"skip\", and by mentioning that the rest of\nthe sequence can be continued via \"cherry-pick --continue\"\n(whether we skip or take the commit).\n\nNoticed-by: Ramkumar Ramachandra \nHelped-by: Jonathan Nieder \nSigned-off-by: Jeff King \n---\n builtin/commit.c | 25 ++++++++++++++++++++++---\n 1 file changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e47f100..73fafe2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -63,8 +63,18 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n \"If you wish to commit it anyway, use:\\n\"\n \"\\n\"\n \" git commit --allow-empty\\n\"\n+\"\\n\");\n+\n+static const char empty_cherry_pick_advice_single[] =\n+N_(\"Otherwise, please use 'git reset'\\n\");\n+\n+static const char empty_cherry_pick_advice_multi[] =\n+N_(\"If you wish to skip this commit, use:\\n\"\n \"\\n\"\n-\"Otherwise, please use 'git reset'\\n\");\n+\" git reset\\n\"\n+\"\\n\"\n+\"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n+\"the remaining commits.\\n\");\n \n static const char *use_message_buffer;\n static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n@@ -107,6 +117,7 @@ static enum commit_whence whence;\n static const char *cleanup_arg;\n \n static enum commit_whence whence;\n+static int sequencer_in_use;\n static int use_editor = 1, include_status = 1;\n static int show_ignored_in_status, have_option_m;\n static const char *only_include_assumed;\n@@ -141,8 +152,11 @@ static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path(\"MERGE_HEAD\")))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\")))\n+\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n \t\twhence = FROM_CHERRY_PICK;\n+\t\tif (file_exists(git_path(\"sequencer\")))\n+\t\t\tsequencer_in_use = 1;\n+\t}\n \telse\n \t\twhence = FROM_COMMIT;\n \tif (s)\n@@ -808,8 +822,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\trun_status(stdout, index_file, prefix, 0, s);\n \t\tif (amend)\n \t\t\tfputs(_(empty_amend_advice), stderr);\n-\t\telse if (whence == FROM_CHERRY_PICK)\n+\t\telse if (whence == FROM_CHERRY_PICK) {\n \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n+\t\t\tif (!sequencer_in_use)\n+\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n+\t\t\telse\n+\t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n+\t\t}\n \t\treturn 0;\n \t}\n \n-- \n1.8.3.rc1.30.gff0fb75\n"},{"id":"224168","messageId":"CALkWK0niBpmD6S78rTvQcQVjdQ0yPE7K1kqt7gmQhEo7CvY=gA@mail.gmail.com","threadId":"34546","inReplyTo":"20130726233928.GD12968@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-27T08:07:01Z","receivedAt":"2013-07-27T08:07:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> builtin/commit.c | 25 ++++++++++++++++++++++---\n> 1 file changed, 22 insertions(+), 3 deletions(-)\n\nOverall, highly inelegant. The single-commit pick has been special\ncased only because we needed to preserve backward compatibility: I\nwould hate for the detail to be user-visible. I'd have expected a\nseries that polishes sequencer.c instead.\n\nBut whatever. I'm going to squelch them anyway.\n"},{"id":"224169","messageId":"CALkWK0m7iypFRHaW-Wt4GQBGh0Kypss7FE8MxPGYU-sG86Dfsg@mail.gmail.com","threadId":"34546","inReplyTo":"20130726213705.GJ14690@google.com","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-27T08:19:42Z","receivedAt":"2013-07-27T08:19:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jonathan Nieder wrote:\n>> Your patch is just swapping out \"git reset\" for \"cherry-pick --abort\",\n>> so I think that is a good improvement in the meantime.\n>\n> Um, wasn't the idea of the original message that you can run \"git\n> reset\" and then \"git cherry-pick --continue\"?\n\nNo, and I don't know where you got that idea. Certainly not by\nreading the history.\n\n37f7a85 (Teach commit about CHERRY_PICK_HEAD, 2011-02-19) introduced\nthat string. This happened before I introduced cherry-pick --continue\nin 5a5d80f (revert: Introduce --continue to continue the operation,\n2011-08-04).\n\nA proper solution to the problem would involve polishing sequencer.c,\nand probably getting --skip-empty so it behaves more like rebase. In\ncase you missed it, one person already did that work and posted the\ncode to the list [1].\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/226982\n"},{"id":"224222","messageId":"7vy58p1joy.fsf@alter.siamese.dyndns.org","threadId":"34546","inReplyTo":"20130726224359.GA3928@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-29T15:15:25Z","receivedAt":"2013-07-29T15:15:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jul 26, 2013 at 05:40:36PM -0400, Jeff King wrote:\n>\n>> > Jeff King wrote:\n>> > \n>> > > Your patch is just swapping out \"git reset\" for \"cherry-pick --abort\",\n>> > > so I think that is a good improvement in the meantime.\n>> > \n>> > Um, wasn't the idea of the original message that you can run \"git\n>> > reset\" and then \"git cherry-pick --continue\"?\n>> \n>> Maybe. :)\n>> \n>> I missed that subtlety. Of my \"three things you would want to do\", that\n>> means it was _trying_ say number 2, how to skip, rather than 3, how to\n>> abort. If that is the case, then it should probably explain the sequence\n>> of steps as \"reset and then --continue\" to make it more clear.\n>> \n>> I.e., a patch is needed, but Ram's is going in the opposite direction.\n>\n> I played around a bit with the test cases that Ram showed. It seems like\n> the advice needed is different depending on whether you are in a single\n> or multi-commit cherry-pick.\n>\n> So if we hit an empty commit and you want to:\n>\n> 1. Make an empty commit, then always run \"git commit --allow-empty\".\n>\n> 2. Skip this commit, then if:\n>\n> a. this is a single commit cherry-pick, you run \"git reset\" (and\n> nothing more, the cherry pick is finished; running \"cherry-pick\n> --continue\" will yield an error).\n\nYes, the single-replay mode never required \"cherry-pick --continue\"\nto clean sequencer cruft when discarding a failed cherry-pick, so it\nis a natural consequence of a conscious design decision that\n\"cherry-pick --continue\" will say \"you are not running a\ncherry-pick\", exactly because you no longer are.\n\n> b. this is a multi-commit cherry-pick, you run \"git reset\",\n> followed by \"git cherry-pick --continue\"\n\nTrue.\n\n> 3. Abort the commit, run \"git cherry-pick --abort\"\n>\n> Let's assume that the instructions we want to give the user are how to\n> do options 1 and 2. I do not mind omitting 3, as it should be reasonably\n> obvious that \"cherry-pick --abort\" is always good way to abort.\n>\n> So we give good instructions for the single-commit case, but bad\n> instructions for the multi-commit case.\n\nYeah, that matches what I thought. It appears that when we did a\nshoddy job when teaching commit to give this advice-message and only\nconsidered a single-pick mode, perhaps because multi-replay mode was\nrelatively new back then.\n\n> I think instead we would want to leave the single-commit case alone, and\n> for the multi-commit case add \"...and then cherry-pick --continue\". That\n> message is generated from within git-commit, though; I guess it would\n> need to learn about the difference between single/multi cherry-picks.\n\nSounds very sensible.\n"},{"id":"224223","messageId":"7vtxjd1jje.fsf@alter.siamese.dyndns.org","threadId":"34546","inReplyTo":"20130726233928.GD12968@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-29T15:18:45Z","receivedAt":"2013-07-29T15:18:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here it is in patch form, with an updated commit message that hopefully\n> explains the rationale a bit better.\n>\n> -- >8 --\n> Subject: [PATCH] commit: tweak empty cherry pick advice for sequencer\n>\n> When we refuse to make an empty commit, we check whether we\n> are in a cherry-pick in order to give better advice on how\n> to proceed. We instruct the user to repeat the commit with\n> \"--allow-empty\" to force the commit, or to use \"git reset\"\n> to skip it and abort the cherry-pick.\n>\n> In the case of a single cherry-pick, the distinction between\n> skipping and aborting is not important, as there is no more\n> work to be done afterwards. When we are using the sequencer\n> to cherry pick a series of commits, though, the instruction\n> is confusing: does it skip this commit, or does it abort the\n> rest of the cherry-pick?\n>\n> It does skip, after which the user can continue the\n> cherry-pick. This is the right thing to be advising the user\n> to do, but let's make it more clear what will happen, both\n> by using the word \"skip\", and by mentioning that the rest of\n> the sequence can be continued via \"cherry-pick --continue\"\n> (whether we skip or take the commit).\n>\n> Noticed-by: Ramkumar Ramachandra \n> Helped-by: Jonathan Nieder \n> Signed-off-by: Jeff King \n> ---\n> builtin/commit.c | 25 ++++++++++++++++++++++---\n> 1 file changed, 22 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index e47f100..73fafe2 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -63,8 +63,18 @@ N_(\"The previous cherry-pick is now empty, possibly due to conflict resolution.\\\n> \"If you wish to commit it anyway, use:\\n\"\n> \"\\n\"\n> \" git commit --allow-empty\\n\"\n> +\"\\n\");\n> +\n> +static const char empty_cherry_pick_advice_single[] =\n> +N_(\"Otherwise, please use 'git reset'\\n\");\n> +\n> +static const char empty_cherry_pick_advice_multi[] =\n> +N_(\"If you wish to skip this commit, use:\\n\"\n> \"\\n\"\n> -\"Otherwise, please use 'git reset'\\n\");\n> +\" git reset\\n\"\n> +\"\\n\"\n> +\"Then \\\"git cherry-pick --continue\\\" will resume cherry-picking\\n\"\n> +\"the remaining commits.\\n\");\n> \n> static const char *use_message_buffer;\n> static const char commit_editmsg[] = \"COMMIT_EDITMSG\";\n> @@ -107,6 +117,7 @@ static enum commit_whence whence;\n> static const char *cleanup_arg;\n> \n> static enum commit_whence whence;\n> +static int sequencer_in_use;\n> static int use_editor = 1, include_status = 1;\n> static int show_ignored_in_status, have_option_m;\n> static const char *only_include_assumed;\n> @@ -141,8 +152,11 @@ static void determine_whence(struct wt_status *s)\n> {\n> \tif (file_exists(git_path(\"MERGE_HEAD\")))\n> \t\twhence = FROM_MERGE;\n> -\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\")))\n> +\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n> \t\twhence = FROM_CHERRY_PICK;\n> +\t\tif (file_exists(git_path(\"sequencer\")))\n> +\t\t\tsequencer_in_use = 1;\n\nShould this be\n\n\tsequencer_in_use = file_exists(...)\n\nso readers do not have to wonder what the variable is initialized\nto?\n\nOther than that, looks reasonable to me. Thanks.\n\n\n\n> +\t}\n> \telse\n> \t\twhence = FROM_COMMIT;\n> \tif (s)\n> @@ -808,8 +822,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n> \t\trun_status(stdout, index_file, prefix, 0, s);\n> \t\tif (amend)\n> \t\t\tfputs(_(empty_amend_advice), stderr);\n> -\t\telse if (whence == FROM_CHERRY_PICK)\n> +\t\telse if (whence == FROM_CHERRY_PICK) {\n> \t\t\tfputs(_(empty_cherry_pick_advice), stderr);\n> +\t\t\tif (!sequencer_in_use)\n> +\t\t\t\tfputs(_(empty_cherry_pick_advice_single), stderr);\n> +\t\t\telse\n> +\t\t\t\tfputs(_(empty_cherry_pick_advice_multi), stderr);\n> +\t\t}\n> \t\treturn 0;\n> \t}\n"},{"id":"224224","messageId":"20130729152345.GA10393@sigill.intra.peff.net","threadId":"34546","inReplyTo":"7vtxjd1jje.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-29T15:23:45Z","receivedAt":"2013-07-29T15:23:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 29, 2013 at 08:18:45AM -0700, Junio C Hamano wrote:\n\n> > \tif (file_exists(git_path(\"MERGE_HEAD\")))\n> > \t\twhence = FROM_MERGE;\n> > -\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\")))\n> > +\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n> > \t\twhence = FROM_CHERRY_PICK;\n> > +\t\tif (file_exists(git_path(\"sequencer\")))\n> > +\t\t\tsequencer_in_use = 1;\n> \n> Should this be\n> \n> \tsequencer_in_use = file_exists(...)\n> \n> so readers do not have to wonder what the variable is initialized\n> to?\n\nYeah, I think that is a readability improvement. I suppose the use-site\ncould also just run \"file_exists\" itself, but I wanted to keep the\nfilesystem-reading \"magic name\" bits together.\n\nI had also originally considered adding new states to \"whence\" to cover\nthese cases (i.e., FROM_CHERRY_PICK_MULTI), but there are fallouts all\nover the place for call sites that do not care whether we are in the\nsingle- or multi-commit mode.\n\n-Peff\n"},{"id":"224229","messageId":"7vli4p1i6d.fsf@alter.siamese.dyndns.org","threadId":"34546","inReplyTo":"20130729152345.GA10393@sigill.intra.peff.net","subject":"Re: [PATCH] commit: correct advice about aborting a cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-29T15:48:10Z","receivedAt":"2013-07-29T15:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 29, 2013 at 08:18:45AM -0700, Junio C Hamano wrote:\n>\n>> > \tif (file_exists(git_path(\"MERGE_HEAD\")))\n>> > \t\twhence = FROM_MERGE;\n>> > -\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\")))\n>> > +\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n>> > \t\twhence = FROM_CHERRY_PICK;\n>> > +\t\tif (file_exists(git_path(\"sequencer\")))\n>> > +\t\t\tsequencer_in_use = 1;\n>> \n>> Should this be\n>> \n>> \tsequencer_in_use = file_exists(...)\n>> \n>> so readers do not have to wonder what the variable is initialized\n>> to?\n>\n> Yeah, I think that is a readability improvement. I suppose the use-site\n> could also just run \"file_exists\" itself, but I wanted to keep the\n> filesystem-reading \"magic name\" bits together.\n\nI take that one back. The use-site is sufficiently far from this\nassignment that is protected with a cascading if that the reader\nneeds to be aware that sequencer_in_use is initialized to zero\nanyway. The code you posted is just as readable, if not more.\n\n> I had also originally considered adding new states to \"whence\" to cover\n> these cases (i.e., FROM_CHERRY_PICK_MULTI), but there are fallouts all\n> over the place for call sites that do not care whether we are in the\n> single- or multi-commit mode.\n\n;-)\n"}]}