{"thread":{"id":"29494","subject":"Rebase regression in v1.7.9?","startedAt":"2012-01-31T22:56:29Z","lastAt":"2012-04-04T20:20:05Z","messageCount":22,"participants":["Felipe Contreras","Andrew Wong","Junio C Hamano","Ramkumar Ramachandra","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"183441","messageId":"CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com","threadId":"29494","inReplyTo":null,"subject":"Rebase regression in v1.7.9?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-01-31T22:56:29Z","receivedAt":"2012-01-31T22:56:29Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nTry the following steps:\n\n% git checkout -b tmp v1.7.9\n% git cherry-pick 6e1c9bb^2^\n% git rebase -i --onto 6e1c9bb HEAD^\n% git rebase --continue\n\nThe rebase will finish, but there will be a .git/CHERRY_PICK_HEAD file.\n\nThis doesn't happen if you run rebase without -i, or do rebase --skip instead.\n\nIIRC correctly rebase --continue used to work fine.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"183486","messageId":"4F29761E.1030605@sohovfx.com","threadId":"29494","inReplyTo":"CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com","subject":"Re: Rebase regression in v1.7.9?","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2012-02-01T17:27:58Z","receivedAt":"2012-02-01T17:27:58Z","isPatch":false,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 01/31/2012 05:56 PM, Felipe Contreras wrote:\n> The rebase will finish, but there will be a .git/CHERRY_PICK_HEAD file.\n> \nAh, good catch. I can reproduce the issue. This is only happening in\n\"rebase -i\" because interactive rebase relies on cherry-pick, but not\nregular rebase. And now cherry-pick creates a state when there's a\nconflict (since 1.7.5?), which \"rebase -i\" didn't expect before. We\nprobably just need to do a manual clean up before \"rebase -i\" continues.\n\nI'll try to come up with a patch for this. In the mean time, doing a\n\"git reset\" will remove that dangling file. Of course, you could always\nmanually remove it. Does the dangling file cause a subsequent git\ncommand to fail?\n"},{"id":"183491","messageId":"CAMP44s12Q3BdXzgr_m2Z0EpApiexngYoWgLi6NRONfCtr1zHKQ@mail.gmail.com","threadId":"29494","inReplyTo":"4F29761E.1030605@sohovfx.com","subject":"Re: Rebase regression in v1.7.9?","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-02-01T19:30:41Z","receivedAt":"2012-02-01T19:30:41Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Feb 1, 2012 at 7:27 PM, Andrew Wong wrote:\n> On 01/31/2012 05:56 PM, Felipe Contreras wrote:\n>> The rebase will finish, but there will be a .git/CHERRY_PICK_HEAD file.\n>>\n> Ah, good catch. I can reproduce the issue. This is only happening in\n> \"rebase -i\" because interactive rebase relies on cherry-pick, but not\n> regular rebase. And now cherry-pick creates a state when there's a\n> conflict (since 1.7.5?), which \"rebase -i\" didn't expect before. We\n> probably just need to do a manual clean up before \"rebase -i\" continues.\n>\n> I'll try to come up with a patch for this. In the mean time, doing a\n> \"git reset\" will remove that dangling file. Of course, you could always\n> manually remove it. Does the dangling file cause a subsequent git\n> command to fail?\n\nNo, it's just annoying with the __git_ps1 prompt stuff. Yeah, 'git\nreset' solves the problem, but it's much better to type 'git skip'\ninstead... for now.\n\n-- \nFelipe Contreras\n"},{"id":"187218","messageId":"1332106632-31882-1-git-send-email-andrew.kw.w@gmail.com","threadId":"29494","inReplyTo":"CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com","subject":"[PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2012-03-18T21:37:12Z","receivedAt":"2012-03-18T21:37:12Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"Instead of having the sequencer catch errors and remove CHERRY_PICK_HEAD\nfor its caller's sake, let its caller do the work. This way, the\nsequencer doesn't have to check all points of failures where its caller\ndoesn't want CHERRY_PICK_HEAD.\n\nFor example, the sequencer current doesn't clean up CHERRY_PICK_HEAD if\n'commit' failed due to an empty commit. Letting 'rebase -i' deal with\nremoving CHERRY_PICK_HEAD keeps the sequencer's logic a bit cleaner.\n\nSigned-off-by: Andrew Wong \n---\n git-rebase--interactive.sh | 10 +++++++++-\n sequencer.c | 6 ------\n 2 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 5812222..061248c 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -196,7 +196,12 @@ pick_one () {\n \toutput git rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $ff \"$@\"\n+\toutput git cherry-pick $ff \"$@\" ||\n+\t{\n+\t\tstatus=$?\n+\t\trm -f \"$GIT_DIR\"/CHERRY_PICK_HEAD\n+\t\treturn $status\n+\t}\n }\n \n pick_one_preserving_merges () {\n@@ -308,7 +313,10 @@ pick_one_preserving_merges () {\n \t\t\t;;\n \t\t*)\n \t\t\toutput git cherry-pick \"$@\" ||\n+\t\t\t{\n+\t\t\t\trm -f \"$GIT_DIR\"/CHERRY_PICK_HEAD\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n+\t\t\t}\n \t\t\t;;\n \t\tesac\n \t\t;;\ndiff --git a/sequencer.c b/sequencer.c\nindex a37846a..c2eceb5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -129,12 +129,6 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n \n \tif (msg) {\n \t\tfprintf(stderr, \"%s\\n\", msg);\n-\t\t/*\n-\t\t * A conflict has occured but the porcelain\n-\t\t * (typically rebase --interactive) wants to take care\n-\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n-\t\t */\n-\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n \t\treturn;\n \t}\n \n-- \n1.7.10.rc1.22.gf5241\n"},{"id":"187260","messageId":"7vk42gbkl1.fsf@alter.siamese.dyndns.org","threadId":"29494","inReplyTo":"1332106632-31882-1-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-19T16:51:22Z","receivedAt":"2012-03-19T16:51:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> For example, the sequencer current doesn't clean up CHERRY_PICK_HEAD if\n> 'commit' failed due to an empty commit. Letting 'rebase -i' deal with\n> removing CHERRY_PICK_HEAD keeps the sequencer's logic a bit cleaner.\n\nHmmm.\n\nIsn't the real solution *not* to create the CHERRY_PICK_HEAD in the\nsequencer when it is not know if it is needed, instead of the current code\nwhich seems to create first and then selectively try to unlink() it?\n"},{"id":"187281","messageId":"4F679E67.4080708@sohovfx.com","threadId":"29494","inReplyTo":"7vk42gbkl1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2012-03-19T21:00:23Z","receivedAt":"2012-03-19T21:00:23Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 03/19/2012 12:51 PM, Junio C Hamano wrote:\n> Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the\n> sequencer when it is not know if it is needed, instead of the current code\n> which seems to create first and then selectively try to unlink() it?\n> \nI do agree that it's not pretty to create the file, then quickly delete\nit. I did consider putting a condition around the code that creates\nCHERRY_PICK_HEAD and REVERT_HEAD.\n\nA possible condition would be checking the env var GIT_CHERRY_PICK_HELP,\nwhich is only set if 'cherry-pick' is called under 'rebase -i'. I never\nliked how we're passing in a help message using an env var, so I don't\nfeel like introducing another dependency on this env var is a good idea.\n\nAnother possible condition would be to add another flag to\n\"cherry-pick\". But a proper implementation would not only involve adding\ncode to parse the flag in 'cherry-pick', but also adding code to\nsave/restore the option in sequencer, even though 'rebase -i' only need\nit for single_pick. It's not that adding these codes are difficult, but\nit seems like we're adding a lot of code just to add a behavior that\nonly 'rebase -i' needs.\n\nThough if the additional flag in \"cherry-pick\" and additional option in\nsequencer could be useful elsewhere, I could do it that way too.\n"},{"id":"187654","messageId":"4F6E289B.4020104@sohovfx.com","threadId":"29494","inReplyTo":"4F679E67.4080708@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w-lists@sohovfx.com","sentAt":"2012-03-24T20:03:39Z","receivedAt":"2012-03-24T20:03:39Z","isPatch":true,"sender":{"key":"andrew.w-lists@sohovfx.com","avatar":null},"body":"On 12-03-19 5:00 PM, Andrew Wong wrote:\n> On 03/19/2012 12:51 PM, Junio C Hamano wrote:\n>> Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the\n>> sequencer when it is not know if it is needed, instead of the current code\n>> which seems to create first and then selectively try to unlink() it?\n>>\n> Though if the additional flag in \"cherry-pick\" and additional option in\n> sequencer could be useful elsewhere, I could do it that way too.\nI looked into adding a \"no-state\" flag in 'cherry-pick' to not create \nthe CHERRY_PICK_HEAD, but 'commit' actually has several dependencies on \nCHERRY_PICK_HEAD, such as recording reflog message, 'prepare-commit-msg' \nhook, and formatting a user message. So if we want to continue to pursue \nthis path, we'd have to preserve those behaviors in 'commit' as well. \nIt's probably not a good idea to make all these changes in \n'cherry-pick' and 'commit' just to avoid a simple cleanup in 'rebase \n-i'. So I still prefer the patch I submitted earlier.\n"},{"id":"188363","messageId":"4F7A2A79.1040900@sohovfx.com","threadId":"29494","inReplyTo":"4F6E289B.4020104@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w-lists@sohovfx.com","sentAt":"2012-04-02T22:38:49Z","receivedAt":"2012-04-02T22:38:49Z","isPatch":true,"sender":{"key":"andrew.w-lists@sohovfx.com","avatar":null},"body":"On 03/24/2012 04:03 PM, Andrew Wong wrote:\n> On 12-03-19 5:00 PM, Andrew Wong wrote:\n>> On 03/19/2012 12:51 PM, Junio C Hamano wrote:\n>>> Isn't the real solution *not* to create the CHERRY_PICK_HEAD in the\n>>> sequencer when it is not know if it is needed, instead of the\n>>> current code\n>>> which seems to create first and then selectively try to unlink() it?\n>>>\n>> Though if the additional flag in \"cherry-pick\" and additional option in\n>> sequencer could be useful elsewhere, I could do it that way too.\n> I looked into adding a \"no-state\" flag in 'cherry-pick' to not create\n> the CHERRY_PICK_HEAD, but 'commit' actually has several dependencies\n> on CHERRY_PICK_HEAD, such as recording reflog message,\n> 'prepare-commit-msg' hook, and formatting a user message. So if we\n> want to continue to pursue this path, we'd have to preserve those\n> behaviors in 'commit' as well. It's probably not a good idea to make\n> all these changes in 'cherry-pick' and 'commit' just to avoid a\n> simple cleanup in 'rebase -i'. So I still prefer the patch I submitted\n> earlier.\nCan we look into queuing this patch? Or does anyone have any thoughts on\nthis?\n"},{"id":"188367","messageId":"7vr4w5d955.fsf@alter.siamese.dyndns.org","threadId":"29494","inReplyTo":"4F7A2A79.1040900@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-02T23:08:38Z","receivedAt":"2012-04-02T23:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> Can we look into queuing this patch? Or does anyone have any thoughts on\n> this?\n\nI do not recall if I convinced myself that the patch was fixing the right\nproblem, or it does not look like it would break other cases; reviews from\ninterested parties are very much appreciated.\n\nThis fell through the crack. Thanks for sending a reminder.\n"},{"id":"188383","messageId":"7vk41xcs5y.fsf@alter.siamese.dyndns.org","threadId":"29494","inReplyTo":"7vr4w5d955.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-03T05:15:21Z","receivedAt":"2012-04-03T05:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Andrew Wong writes:\n>\n>> Can we look into queuing this patch? Or does anyone have any thoughts on\n>> this?\n>\n> I do not recall if I convinced myself that the patch was fixing the right\n> problem, or it does not look like it would break other cases; reviews from\n> interested parties are very much appreciated.\n>\n> This fell through the crack. Thanks for sending a reminder.\n\nAnd I forgot to Cc who is responsible for that CHERRY_PICK_HEAD part...\n"},{"id":"188385","messageId":"CALkWK0nmNWaOKcyGH2N0s3B1AFD-+3vHz1BBc3U=RMEFLNuc7A@mail.gmail.com","threadId":"29494","inReplyTo":"1332106632-31882-1-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-04-03T06:32:29Z","receivedAt":"2012-04-03T06:32:29Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Andrew,\n\n[+CC: Jonathan Nieder]\n\nAndrew Wong wrote:\n> Instead of having the sequencer catch errors and remove CHERRY_PICK_HEAD\n> for its caller's sake, let its caller do the work. This way, the\n> sequencer doesn't have to check all points of failures where its caller\n> doesn't want CHERRY_PICK_HEAD.\n\nThis part makes sense.\n\n> For example, the sequencer current doesn't clean up CHERRY_PICK_HEAD if\n> 'commit' failed due to an empty commit. Letting 'rebase -i' deal with\n> removing CHERRY_PICK_HEAD keeps the sequencer's logic a bit cleaner.\n\nYes, that's because git-commit is spawned. The sequencer has no way\nto tell if the commit was actually successful. Incidentally, what is\nyour motivation for this patch? Did the \"rebase -i\" or the sequencer\nmisbehave in some scenario? Wouldn't it make sense to add a failing\ntest for that scenario first?\n\nAlso, note that in a previous iteration, we considered the possibility\nof making git-commit remove CHERRY_PICK_HEAD, but decided that it\nwould be ugly subsequently.\n\n> A possible condition would be checking the env var GIT_CHERRY_PICK_HELP,\n> which is only set if 'cherry-pick' is called under 'rebase -i'. I never\n> liked how we're passing in a help message using an env var, so I don't\n> feel like introducing another dependency on this env var is a good idea.\n\nTrue; this is ugly. It detracts us from the purpose of the patch\nwhich is to shift the responsibility of cleaning up CHERRY_PICK_HEAD\nto the caller.\n\n> Another possible condition would be to add another flag to\n> \"cherry-pick\". But a proper implementation would not only involve adding\n> code to parse the flag in 'cherry-pick', but also adding code to\n> save/restore the option in sequencer, even though 'rebase -i' only need\n> it for single_pick. It's not that adding these codes are difficult, but\n> it seems like we're adding a lot of code just to add a behavior that\n> only 'rebase -i' needs.\n\nAnother ugly solution.\n\n> Signed-off-by: Andrew Wong \n> [...]\n\nBonus: After this patch, the sequencer code is symmetric in\nCHERY_PICK_HEAD and REVERT_HEAD. How do we convince ourselves that\nwe're not breaking some corner case though? I'd be more comfortable\nwith the patch if you can present a failing test first.\n\nThanks.\n\n Ram\n"},{"id":"188417","messageId":"20120403144505.GE15589@burratino","threadId":"29494","inReplyTo":"CALkWK0nmNWaOKcyGH2N0s3B1AFD-+3vHz1BBc3U=RMEFLNuc7A@mail.gmail.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T14:45:05Z","receivedAt":"2012-04-03T14:45:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Jay, expert on the CHERRY_PICK_HEAD facility)\nHi all,\n\nRamkumar Ramachandra wrote:\n> Andrew Wong wrote:\n\n>> Instead of having the sequencer catch errors and remove CHERRY_PICK_HEAD\n>> for its caller's sake, let its caller do the work. This way, the\n>> sequencer doesn't have to check all points of failures where its caller\n>> doesn't want CHERRY_PICK_HEAD.\n>\n> This part makes sense.\n\nSorry, I think I've missed the point. Can you explain to me what\nproblem this is solving, aside from somehow dividing responsibility\nfor the CHERRY_PICK_HEAD file among different tools?\n\n>From the surrounding thread, it looks like the following sequence of\ncommands is at stake:\n\n\tgit checkout -b tmp v1.7.9\n\tgit cherry-pick 6e1c9bb^2^\n\tgit rebase -i --onto 6e1c9bb HEAD^\n\tgit rebase --continue\n\nThe pick works fine and is just part of the setup. The rebase produces\n\n\tThe previous cherry-pick is now empty, possibly due to conflict resolution.\n\tIf you wish to commit it anyway, use:\n\n\t git commit --allow-empty\n\n\tOtherwise, please use 'git reset'\n\nCHERRY_PICK_HEAD points to \"run-command: optionally kill children on\nexit\" to help the user understand how to resolve the conflict.\nNormally print_advice() would remove it because the caller has set\nGIT_CHERRY_PICK_HELP to indicate that it wants to use some other\nmechanism than \"git commit\" to deal with resolved conflicts.\nUnfortunately the GIT_CHERRY_PICK_HELP facility does not give the\ncaller a way to specify an alternative message for this case, like:\n\n\tThe previous cherry-pick is now empty, possibly due to conflict resolution.\n\n\tWhen you have resolved this problem run \"git rebase --continue\".\n\tIf you would prefer to skip this patch, instead run \"git rebase --skip\".\n\tTo check out the original branch and stop rebasing run \"git rebase --abort\".\n\nIn fact, \"git cherry-pick\" does not handle this case at all. It lets\n\"git commit\" notice the lack of change. \"git commit\" emits a message\nand follows the usual rules for a failed commit, including preserving\nCHERRY_PICK_HEAD to help the operator clean up.\n\nOk. Now the user (sensibly) ignores the message from cherry-pick and\njust runs \"git rebase --continue\". The rebase finishes but nobody\nfeels it's his responsibility to remove the .git/CHERRY_PICK_HEAD file\nand it gets left behind.\n\nFor symptom relief, your patch makes sense, though I haven't checked\nit in detail yet. The description distracted me --- it would be\nbetter to say \"this sequence of commands has this bad consequence;\nthis patch papers over the problem to make people happier until the\nunderlying problem can be addressed\" instead of pretending the design\nwas almost sane and we are just fixing the last detail. ;-)\n\nI suspect a more appropriate long-term fix would involve \"git\ncherry-pick\" noticing when a patch has resolved to nothing instead of\nleaving it to \"git commit\" to detect that.\n\nSensible?\nJonathan\n"},{"id":"188434","messageId":"4F7B650C.9060800@sohovfx.com","threadId":"29494","inReplyTo":"20120403144505.GE15589@burratino","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2012-04-03T21:01:00Z","receivedAt":"2012-04-03T21:01:00Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 04/03/2012 10:45 AM, Jonathan Nieder wrote:\n> Ok. Now the user (sensibly) ignores the message from cherry-pick and\n> just runs \"git rebase --continue\". The rebase finishes but nobody\n> feels it's his responsibility to remove the .git/CHERRY_PICK_HEAD file\n> and it gets left behind.\n> \nYes, that's exactly what's happening. That particular rebase will leave\nbehind CHERRY_PICK_HEAD, which have bad consequences such as:\n1. Confuses __git_ps1 (from git-completion.bash) into thinking a\ncherry-pick is still in progress. (which is what started this discussion)\n2. Cause \"cherry-pick --continue\" to think a cherry-pick is still in\nprogress.\n3. Similarly, if a user then continue on to modifying their files, and\ndo a \"add\" and \"commit\", \"commit\" would reuse the message from the\nCHERRY_PICK_HEAD.\n\n> I suspect a more appropriate long-term fix would involve \"git\n> cherry-pick\" noticing when a patch has resolved to nothing instead of\n> leaving it to \"git commit\" to detect that.\n> \nI actually tried implementing a fix like that too. But then I thought\nthere might be other scenarios where \"commit\" could fail, and it doesn't\nseem to make sense for \"cherry-pick\" to have to detect all possible\n\"commit\" failures. Though it also feels like the question of whether or\nnot \"cherry-pick\" should detects the \"empty commit\" is a separate issue\naltogether.\n\nBesides the \"empty commit\" failure, \"cherry-pick\" can still run into\nvarious errors, such as merge conflict. So it will have to keep a state\nsomehow. And instead of having \"cherry-pick\" make special cases for\n\"rebase -i\" to remove the state, it makes more sense to teach \"rebase\n-i\" that \"cherry-pick\" now keeps a state on failure. So if \"cherry-pick\"\nfails, \"rebase -i\" is responsible for clearing that state. And that's\nwhat this patch is supposed to do.\n\nPerhaps I should rephrase my description to reflect this better?\nSomething along the line of: \"cherry-pick\" now keeps a state on failure.\nInstead of having a special case inside the sequencer to remove the\nstate, we teach \"rebase -i\" that we need to clear the state.\n"},{"id":"188436","messageId":"20120403210815.GB19858@burratino","threadId":"29494","inReplyTo":"4F7B650C.9060800@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T21:08:15Z","receivedAt":"2012-04-03T21:08:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andrew Wong wrote:\n\n> Besides the \"empty commit\" failure, \"cherry-pick\" can still run into\n> various errors, such as merge conflict.\n\nCherry-pick does the merge, so it is what notices the merge conflict.\nIf you search for CHERRY_PICK_HELP in builtin/revert.c, the relevant\ncode should show up.\n"},{"id":"188437","messageId":"20120403211219.GC19858@burratino","threadId":"29494","inReplyTo":"20120403210815.GB19858@burratino","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T21:12:19Z","receivedAt":"2012-04-03T21:12:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Andrew Wong wrote:\n\n>> Besides the \"empty commit\" failure, \"cherry-pick\" can still run into\n>> various errors, such as merge conflict.\n>\n> Cherry-pick does the merge, so it is what notices the merge conflict.\n> If you search for CHERRY_PICK_HELP in builtin/revert.c, the relevant\n> code should show up.\n\nI was looking at an older git version. In newer gits, the code path\nin question lives at print_advice() in sequencer.c.\n\nSorry for the noise.\nJonathan\n"},{"id":"188438","messageId":"4F7B69FE.9010600@sohovfx.com","threadId":"29494","inReplyTo":"20120403211219.GC19858@burratino","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2012-04-03T21:22:06Z","receivedAt":"2012-04-03T21:22:06Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 04/03/2012 05:12 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n> \n>> Cherry-pick does the merge, so it is what notices the merge conflict.\n>> If you search for CHERRY_PICK_HELP in builtin/revert.c, the relevant\n>> code should show up.\n>> \n> I was looking at an older git version. In newer gits, the code path\n> in question lives at print_advice() in sequencer.c.\n> \nYes, the code has been moved into sequencer now.\n\nBut what I meant was, regardless of who's calling \"cherry-pick\", if\n\"cherry-pick\" runs into an error and needs to stop, it needs to save a\nstate so that it can do a \"--continue\". And this behavior should stay\nthe same regardless of who the caller is. And that means its callers\n(e.g. \"rebase -i\") should know about this and do a cleanup when\n\"cherry-pick\" failed.\n"},{"id":"188439","messageId":"20120403212650.GD19858@burratino","threadId":"29494","inReplyTo":"4F7B69FE.9010600@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T21:26:50Z","receivedAt":"2012-04-03T21:26:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andrew Wong wrote:\n\n> But what I meant was, regardless of who's calling \"cherry-pick\", if\n> \"cherry-pick\" runs into an error and needs to stop, it needs to save a\n> state so that it can do a \"--continue\". And this behavior should stay\n> the same regardless of who the caller is. And that means its callers\n> (e.g. \"rebase -i\") should know about this and do a cleanup when\n> \"cherry-pick\" failed.\n\nThe current CHERRY_PICK_HELP codepath removes CHERRY_PICK_HEAD to let\nits caller take care of the appropriate \"commit -c\" magic for\nhistorical reasons. I'd be happy to see \"rebase -i\" stop relying on\nthat.\n\nUnfortunately, outside scripts from before CHERRY_PICK_HEAD existed\nare also allowed to use CHERRY_PICK_HELP, so the incomplete\nimplementation that leaves behind a CHERRY_PICK_HEAD when the commit\nbeing cherry-picked resolves into nothingness is still a bug.\n"},{"id":"188459","messageId":"4F7B839D.2020808@sohovfx.com","threadId":"29494","inReplyTo":"20120403212650.GD19858@burratino","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2012-04-03T23:11:25Z","receivedAt":"2012-04-03T23:11:25Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 04/03/2012 05:26 PM, Jonathan Nieder wrote:\n> The current CHERRY_PICK_HELP codepath removes CHERRY_PICK_HEAD to let\n> its caller take care of the appropriate \"commit -c\" magic for\n> historical reasons. I'd be happy to see \"rebase -i\" stop relying on\n> that.\n> \n\"rebase -i\" doesn't rely on \"commit -c\". It stores the author info\ninside its state dir, so when the user does a \"rebase --continue\", the\nauthor info from the state dir is used.\n\nAlso, \"commit -c\" does override CHERRY_PICK_HEAD for author info and\nmessage. So having CHERRY_PICK_HEAD around shouldn't affect scripts that\nrely on \"commit -c\".\n\n> Unfortunately, outside scripts from before CHERRY_PICK_HEAD existed\n> are also allowed to use CHERRY_PICK_HELP,so the incomplete\n> implementation that leaves behind a CHERRY_PICK_HEAD when the commit\n> being cherry-picked resolves into nothingness is still a bug.\n> \nCHERRY_PICK_HELP was introduced as a hack to allow \"rebase -i\" to\noverride what message \"cherry-pick\" shows, and not as something that\naffects the behavior of \"cherry-pick\". Since it is also not documented,\nit feels more like an internal implementation detail. So I suspect not\nmany people know about this env var and even fewer would actually use it.\n"},{"id":"188506","messageId":"20120404181148.GB16993@burratino","threadId":"29494","inReplyTo":"4F7B839D.2020808@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-04T18:11:48Z","receivedAt":"2012-04-04T18:11:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andrew Wong wrote:\n\n> CHERRY_PICK_HELP was introduced as a hack\n\nDo you mean we should get rid of CHERRY_PICK_HELP?\n\nNote that you don't have to convince me of anything --- I was just\ntrying to help by providing my reactions and some background. If the\nRight Thing to Do as revealed by list consensus or Junio's decree ;-)\ninvolves keeping CHERRY_PICK_HELP difficult to use and fragile, I\nwon't mind much as long as rebase -i works, since I don't have any\nprivate scripts that use CHERRY_PICK_HELP personally.\n\nSorry for the lack of clarity.\nJonathan\n"},{"id":"188511","messageId":"4F7C9FAE.5050806@sohovfx.com","threadId":"29494","inReplyTo":"20120404181148.GB16993@burratino","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Andrew Wong","fromEmail":"andrew.w@sohovfx.com","sentAt":"2012-04-04T19:23:26Z","receivedAt":"2012-04-04T19:23:26Z","isPatch":true,"sender":{"key":"andrew.w@sohovfx.com","avatar":null},"body":"On 04/04/2012 02:11 PM, Jonathan Nieder wrote:\n> Do you mean we should get rid of CHERRY_PICK_HELP?\n> \nNo. I thought you meant that if \"cherry-pick\" runs into an error, it\nshould not leave behind a state (i.e. CHERRY_PICK_HEAD) when\nCHERRY_PICK_HELP is defined. And I was arguing that defining\nCHERRY_PICK_HELP shouldn't affect the behavior of \"cherry-pick\" at all.\nSo it shouldn't be trying to remove the state in the first place. The\ncleanup responsibility should fall into caller of \"cherry-pick\". i.e.\n\"rebase -i\"\n\nThough I now think that my original patch description could be improved\nto better reflect that.\n\nAnd you also mention earlier that the patch is more of a symptom relief,\nand that\n> a more appropriate long-term fix would involve \"git\n> cherry-pick\" noticing when a patch has resolved to nothing instead of\n> leaving it to \"git commit\" to detect that.\nAnd I was arguing that \"cherry-pick\" doesn't have to detect scenarios\nwhere \"commit\" could fail. Since there could be other scenarios where\n\"commit\" could fail and \"cherry-pick\" is already handling \"commit\"\nfailing, I think there's no need for \"cherry-pick\" to handle an empty\ncommit specifically.\n\nSo if the list or Junio thinks that the patch is the right thing to do,\nI should improve on the patch description before we queue it.\n"},{"id":"188523","messageId":"20120404201610.GB17544@burratino","threadId":"29494","inReplyTo":"4F7C9FAE.5050806@sohovfx.com","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-04T20:16:10Z","receivedAt":"2012-04-04T20:16:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Andrew Wong wrote:\n\n> Since there could be other scenarios where\n> \"commit\" could fail\n\nAs far as I can tell, there just aren't any such other scenarios,\nunless you mean like running out of memory or disk space. \"git\ncherry-pick\" disables hooks when running \"git commit\" so the\npre-commit hook can't block the commit.\n\nSo the scenarios fall into three or so categories.\n\n - when \"git cherry-pick\" performs a merge and encounters conflicts,\n it prints a message and exits, writing CHERRY_PICK_HEAD to tell\n the operator what command to use (instead of \"git commit\" or\n \"git cherry-pick --continue\") when the problem is resolved.\n\n If my script sets GIT_CHERRY_PICK_HELP, it will print a different\n message and does not write CHERRY_PICK_HEAD because the operator\n is going to run \"myscript --resume\" and not \"git commit\" or \"git\n cherry-pick --continue\" when the problem is resolved.\n\n - when \"git cherry-pick\" performs a clean merge that produces no\n change, \"git commit\" prints a message about a missing --allow-empty\n argument and exits.\n\n My GIT_CHERRY_PICK_HELP setting is not respected, so the user is\n likely to run \"git commit\" or \"git cherry-pick --continue\" instead\n of the command I wanted.\n\n - when \"git cherry-pick\" performs a clean merge that produces a\n change but \"git commit\" fails to record it due to a stray signal or\n running out of disk space, git does not print any advice for the\n operator.\n\n In particular, my GIT_CHERRY_PICK_HELP setting is not respected.\n Also, CHERRY_PICK_HEAD is written even though my wrapper script\n that sets GIT_CHERRY_PICK_HELP didn't expect that.\n\n The operator can return to a familar state with \"git reset --hard\"\n followed by \"git checkout\" of some familiar branch, except that my\n script may be keeping some state of its own that lingers until the\n operator tries to use it again...\n\nI was focusing on the second category. Using a stock message instead\nof the custom message from GIT_CHERRY_PICK_HELP certainly seems to me\nlike a bug or incomplete feature.\n\nWhen you say that there are other ways for \"git commit\" to fail and\nJunio says that in some cases \"git cherry-pick\" should not write\nCHERRY_PICK_HEAD at all, you are probably thinking of the third\ncategory.\n\nHope that helps,\nJonathan\n"},{"id":"188524","messageId":"20120404202005.GC17544@burratino","threadId":"29494","inReplyTo":"20120404201610.GB17544@burratino","subject":"Re: [PATCH] rebase -i: remove CHERRY_PICK_HEAD when cherry-pick failed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-04T20:20:05Z","receivedAt":"2012-04-04T20:20:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> - when \"git cherry-pick\" performs a merge and encounters conflicts,\n> it prints a message and exits, writing CHERRY_PICK_HEAD to tell\n> the operator what command to use (instead of \"git commit\" or\n> \"git cherry-pick --continue\") when the problem is resolved.\n\nThe above paragraph doesn't make any sense. I meant:\n\n\tWhen cherry-pick encounters conflicts, it prints a message\n\ttelling the operator what command to use (namely \"git commit\"\n\tor \"git cherry-pick --continue\") once the problem is resolved\n\tand writes CHERRY_PICK_HEAD to make that command work.\n\nSorry for the noise.\n\n> If my script sets GIT_CHERRY_PICK_HELP, it will print a different\n> message and cherry-pick does not write CHERRY_PICK_HEAD because\n> the operator is going to run \"myscript --resume\" instead of\n> \"git commit\" or \"git cherry-pick --continue\" when the problem is\n> resolved.\n\nJonathan\n"}]}