{"thread":{"id":"61653","subject":"Thoughts about the -m option of cherry-pick and revert","startedAt":"2024-06-20T10:15:17Z","lastAt":"2024-06-24T18:43:51Z","messageCount":9,"participants":["Stefan Haller","Junio C Hamano","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"497395","messageId":"e60a8b1a-98c8-4ac7-b966-ff9635bb781d@haller-berlin.de","threadId":"61653","inReplyTo":null,"subject":"Thoughts about the -m option of cherry-pick and revert","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2024-06-20T10:05:45Z","receivedAt":"2024-06-20T10:15:17Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"There are plenty of StackOverflow questions and blog posts about the\nerror message that you get when you use git cherry-pick or git revert on\na merge commit without specifying the -m option. Many people don't seem\nto understand what the error message means, or why they even get an\nerror in the first place.\n\nThe answers to these questions patiently explain what the error means\nand why the -m option is necessary. Many of them contain example\nscenarios; but I haven't seen a single one that doesn't use -m1 to\nillustrate the usage.\n\nI have two questions:\n\n- What are real-world scenarios where you would use a mainline number\n other than 1? I could only come up with a single example myself, which\n is that you have a topic branch, and right before merging it back to\n main, you merge main into the topic branch; and then you merge it to\n main with a fast-forward merge. If you then want to cherry-pick or\n revert that topic, you'd have to use -m2 on that last merge from main.\n Any other examples?\n- Wouldn't it make sense to default to -m1 when no -m option is given?\n It seems that this would do the expected thing in the vast majority of\n cases.\n\nFor the GUI client that I'm co-maintaining (lazygit), I'm actually\nconsidering going so far as to not providing a choice at all, and always\nusing -m1. I'm not fully decided yet if that's a good idea, but it seems\nthat most people expect this, most of the time.\n\nI'm probably missing something though, but what?\n\n-Stefan\n"},{"id":"497444","messageId":"xmqqa5jfoxvh.fsf@gitster.g","threadId":"61653","inReplyTo":"e60a8b1a-98c8-4ac7-b966-ff9635bb781d@haller-berlin.de","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T02:03:46Z","receivedAt":"2024-06-21T02:03:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Haller writes:\n\n> I have two questions:\n>\n> - What are real-world scenarios where you would use a mainline number\n> other than 1? I could only come up with a single example myself, which\n> is that you have a topic branch, and right before merging it back to\n> main, you merge main into the topic branch; and then you merge it to\n> main with a fast-forward merge. If you then want to cherry-pick or\n> revert that topic, you'd have to use -m2 on that last merge from main.\n> Any other examples?\n\nI do think your example is a real issue that is helped by using -m2;\nI do not think of any other cases offhand myself.\n\n> - Wouldn't it make sense to default to -m1 when no -m option is given?\n> It seems that this would do the expected thing in the vast majority of\n> cases.\n\nI do agree -m2 or higher would be rare when doing \"git revert\". \n\nGiven that the current behaviour was chosen to make sure that the\nuser is aware that the commit being reverted/cherry-picked is a\nmerge and has a chance to choose the right parent (as opposed to\nblindly picking the first parent that happened to be the right one\nby accident), I am not sure if it is prudent to change the\nbehaviour.\n\nIf I were simplifying this, I would probably\n\n (1) disallow cherry-picking a merge (and suggest redoing the same\n merge, possibly after rebasing the copy of the merged history\n to an appropriate base as needed), and\n (2) allowing reverting a merge only wrt the first parent,\n\nbut that is a different story.\n"},{"id":"497465","messageId":"dd58a60d-a551-4726-85a7-f47b851914be@haller-berlin.de","threadId":"61653","inReplyTo":"xmqqa5jfoxvh.fsf@gitster.g","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2024-06-21T06:33:22Z","receivedAt":"2024-06-21T06:33:24Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"On 21.06.24 04:03, Junio C Hamano wrote:\n> Stefan Haller writes:\n> \n>> - Wouldn't it make sense to default to -m1 when no -m option is given?\n> \n> Given that the current behaviour was chosen to make sure that the\n> user is aware that the commit being reverted/cherry-picked is a\n> merge and has a chance to choose the right parent (as opposed to\n> blindly picking the first parent that happened to be the right one\n> by accident), I am not sure if it is prudent to change the\n> behaviour.\n\nHm, in all example scenarios I experimented with, picking the wrong\nparent would result in an empty diff, and consequently an error message\nlike this:\n\n nothing to commit, working tree clean\n The previous cherry-pick is now empty, possibly due to conflict\n resolution.\n If you wish to commit it anyway, use:\n\n git commit --allow-empty\n\n Otherwise, please use 'git cherry-pick --skip'\n\nI'm not sure if this error is easier or harder to understand than the\none you get today when omitting -m, but we could probably improve it by\nmentioning the -m option if the cherry-picked commit was a merge.\n\nI'd be interested in example scenarios where both sides of the merge\nhave non-empty diffs. Won't this only happen for evil merges?\n\n> If I were simplifying this, I would probably\n> \n> (1) disallow cherry-picking a merge (and suggest redoing the same\n> merge, possibly after rebasing the copy of the merged history\n> to an appropriate base as needed), and\n\nThis seems unnecessarily restrictive to me. Cherry-picking merge commits\nusing -m1 is useful, it's an important part of our release workflow at\nmy day job.\n\n> (2) allowing reverting a merge only wrt the first parent,\n\nInteresting, that's what I'm considering doing in lazygit (except for\nboth revert and cherry-pick), but I kind of didn't expect much support\nfor that idea. :-)\n\n-Stefan\n"},{"id":"497475","messageId":"919b40c6-9497-4646-b7ba-62c2236a4c79@gmail.com","threadId":"61653","inReplyTo":"xmqqa5jfoxvh.fsf@gitster.g","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-21T10:12:22Z","receivedAt":"2024-06-21T10:12:24Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/06/2024 03:03, Junio C Hamano wrote:\n> Stefan Haller writes:\n>\n> Given that the current behaviour was chosen to make sure that the\n> user is aware that the commit being reverted/cherry-picked is a\n> merge and has a chance to choose the right parent (as opposed to\n> blindly picking the first parent that happened to be the right one\n> by accident), I am not sure if it is prudent to change the\n> behaviour.\n\n\nFWIW I agree with this, for me the main benefit of the current behavior \nis stopping when I'm not expecting to cherry-pick a merge.\n\nBest Wishes\n\nPhillip\n\n> If I were simplifying this, I would probably\n> \n> (1) disallow cherry-picking a merge (and suggest redoing the same\n> merge, possibly after rebasing the copy of the merged history\n> to an appropriate base as needed), and\n> (2) allowing reverting a merge only wrt the first parent,\n> \n> but that is a different story.\n> \n\n"},{"id":"497476","messageId":"6e71b1f3-599f-49c3-be37-e499f28983cf@gmail.com","threadId":"61653","inReplyTo":"dd58a60d-a551-4726-85a7-f47b851914be@haller-berlin.de","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-21T10:19:31Z","receivedAt":"2024-06-21T10:19:33Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/06/2024 07:33, Stefan Haller wrote:\n> On 21.06.24 04:03, Junio C Hamano wrote:\n>> Stefan Haller writes:\n >\n> Hm, in all example scenarios I experimented with, picking the wrong\n> parent would result in an empty diff, and consequently an error message\n> like this:\n> \n> nothing to commit, working tree clean\n> The previous cherry-pick is now empty, possibly due to conflict\n> resolution.\n> If you wish to commit it anyway, use:\n> \n> git commit --allow-empty\n> \n> Otherwise, please use 'git cherry-pick --skip'\n> \n> I'm not sure if this error is easier or harder to understand than the\n> one you get today when omitting -m, but we could probably improve it by\n> mentioning the -m option if the cherry-picked commit was a merge.\n\nThat might be helpful - if we do that we'd want to make sure that the \nuser can retry this pick with \"-m\" without restarting the whole cherry-pick.\n\n> I'd be interested in example scenarios where both sides of the merge\n> have non-empty diffs. Won't this only happen for evil merges?\n\nI think you'd need a conflicting merge that is resolved in a way that \nthe resolution of the conflicting lines doesn't match either parent. (I \nassume that's what you mean by evil but I thought it best to check)\n\n>> If I were simplifying this, I would probably\n>>\n>> (1) disallow cherry-picking a merge (and suggest redoing the same\n>> merge, possibly after rebasing the copy of the merged history\n>> to an appropriate base as needed), and\n> \n> This seems unnecessarily restrictive to me. Cherry-picking merge commits\n> using -m1 is useful, it's an important part of our release workflow at\n> my day job.\n\nI can see why people want to revert merges but cherry-picking them \nalways feels strange to me - what is the advantage over actually merging \nthe branch and seeing the full history of that commit?\n\n>> (2) allowing reverting a merge only wrt the first parent,\n> \n> Interesting, that's what I'm considering doing in lazygit (except for\n> both revert and cherry-pick), but I kind of didn't expect much support\n> for that idea. :-)\n\nFor lazygit I would think it would be fine to be a bit more restrictive \nthat git as anyone with an unusual requirement can always fall back to \nusing git for that.\n\nBest Wishes\n\nPhillip\n\n\n> -Stefan\n> \n\n"},{"id":"497479","messageId":"45c76e96-9f05-4b35-9337-faa26980519a@haller-berlin.de","threadId":"61653","inReplyTo":"6e71b1f3-599f-49c3-be37-e499f28983cf@gmail.com","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2024-06-21T11:48:49Z","receivedAt":"2024-06-21T11:48:57Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"On 21.06.24 12:19, Phillip Wood wrote:\n> On 21/06/2024 07:33, Stefan Haller wrote:\n> \n>> I'd be interested in example scenarios where both sides of the merge\n>> have non-empty diffs. Won't this only happen for evil merges?\n> \n> I think you'd need a conflicting merge that is resolved in a way that\n> the resolution of the conflicting lines doesn't match either parent. (I\n> assume that's what you mean by evil but I thought it best to check)\n\nAh yes, that's another example, but it's not an evil merge. An evil\nmerge is one that has additional changes that don't come from either\nside, and don't come from conflict resolution (e.g. they were amended\ninto the merge). I thought that was commonly understood terminology.\n\n>> This seems unnecessarily restrictive to me. Cherry-picking merge commits\n>> using -m1 is useful, it's an important part of our release workflow at\n>> my day job.\n> \n> I can see why people want to revert merges but cherry-picking them\n> always feels strange to me - what is the advantage over actually merging\n> the branch and seeing the full history of that commit?\n\nIt's less work (if you otherwise insist on rebasing the branch to the\ndestination before merging), and results in a simpler graph that's\neasier to understand (if you don't).\n\nAnd I suppose you could ask the same question about the --squash option\nof git-merge.\n\n-Stefan\n"},{"id":"497501","messageId":"xmqqv822ntkh.fsf@gitster.g","threadId":"61653","inReplyTo":"6e71b1f3-599f-49c3-be37-e499f28983cf@gmail.com","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T16:34:22Z","receivedAt":"2024-06-21T16:34:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I can see why people want to revert merges but cherry-picking them\n> always feels strange to me - what is the advantage over actually\n> merging the branch and seeing the full history of that commit?\n\nOne case that comes to my mind is when you failed to plan ahead and\nused a wrong base when building a series to \"fix\" an old bug. You\nbuilt a 7-patch series to fix a bug that you introduced in release\n1.0, but instead of basing the fix on maint-1.0 maintenance track,\nyou forked from the tip of master that is preparing for your next\nfeature release that is release 1.4.\n\nEven if you realized that the fix is important enough to warrant\napplying to the maint-1.0 maintenance track, you cannot merge the\ntopic that houses 7-patch series down to the old maintenance track\nwithout bringing all the new features that happened since 1.0 on the\nmaster track.\n\nA kosher way may be to rebase the 7-patch series to maint-1.0 and\nmerge the result into the maint-1.0 track (and upwards if needed).\nBut cherry-picking the commit that merged the original \"fix\" topic\ninto master _may_ be simpler, as you need to resolve a larger\nconflict but (hopefully) only once, instead of up to 7 times, once\nper each commit on the \"fix\" topic while rebasing.\n\nBut of course if something goes wrong, it makes the result\nimpossible to bisect---exactly the same reason why you should think\ntwice before doing a \"merge --squash\". In addition, if you somehow\nfigured out why the cherry-picked fix was inadequate, you'd now need\nto forward-port the fix for the fix to the master track or whereever\nthe cherry-picked-merge was taken from.\n\nOn the other hand, if the original \"fix\" branch was rebased on\nmaint-1.0 and then further fixed, the result can be merged to\nmaint-1.0 as well as all the way to the master track.\n\nSo, I can understand why people may want to cherry-pick a merge,\nI suspect it is a false economy. Optimizing for picking, paying\nhigher price when the result of (incorrect) picking has to be\ncorrected later.\n"},{"id":"497556","messageId":"0a948acf-ebe9-407e-8899-d714b6fcb528@haller-berlin.de","threadId":"61653","inReplyTo":"xmqqv822ntkh.fsf@gitster.g","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2024-06-24T07:33:24Z","receivedAt":"2024-06-24T07:33:33Z","isPatch":false,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"On 21.06.24 18:34, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> I can see why people want to revert merges but cherry-picking them\n>> always feels strange to me - what is the advantage over actually\n>> merging the branch and seeing the full history of that commit?\n> \n> One case that comes to my mind is when you failed to plan ahead and\n> used a wrong base when building a series to \"fix\" an old bug. You\n> built a 7-patch series to fix a bug that you introduced in release\n> 1.0, but instead of basing the fix on maint-1.0 maintenance track,\n> you forked from the tip of master that is preparing for your next\n> feature release that is release 1.4.\n> \n> Even if you realized that the fix is important enough to warrant\n> applying to the maint-1.0 maintenance track, you cannot merge the\n> topic that houses 7-patch series down to the old maintenance track\n> without bringing all the new features that happened since 1.0 on the\n> master track.\n> \n> A kosher way may be to rebase the 7-patch series to maint-1.0 and\n> merge the result into the maint-1.0 track (and upwards if needed).\n> But cherry-picking the commit that merged the original \"fix\" topic\n> into master _may_ be simpler, as you need to resolve a larger\n> conflict but (hopefully) only once, instead of up to 7 times, once\n> per each commit on the \"fix\" topic while rebasing.\n> \n> But of course if something goes wrong, it makes the result\n> impossible to bisect---exactly the same reason why you should think\n> twice before doing a \"merge --squash\". In addition, if you somehow\n> figured out why the cherry-picked fix was inadequate, you'd now need\n> to forward-port the fix for the fix to the master track or whereever\n> the cherry-picked-merge was taken from.\n> \n> On the other hand, if the original \"fix\" branch was rebased on\n> maint-1.0 and then further fixed, the result can be merged to\n> maint-1.0 as well as all the way to the master track.\n> \n> So, I can understand why people may want to cherry-pick a merge,\n> I suspect it is a false economy. Optimizing for picking, paying\n> higher price when the result of (incorrect) picking has to be\n> corrected later.\n\nYou may call this \"failed to plan ahead\", but for us it's a deliberate\ndecision to work this way. Developers work exclusively on main, and\nmerge their branches to main, always. Release management decides later\n(sometimes much later) which of these branches are cherry-picked to\nwhich release branches. We never merge back from a release branch to main.\n\nAnd we prefer single-commit cherry-picks of the merge commits because it\nmakes the history of the release branches easier to read. Bisectability\nis not an issue; developers bisect failures on the main branch. (Yes,\nI'm aware that there may be cases where a defect manifests itself\ndifferently (or not at all) on main than on the release branch, but\nthese are so rare that it hasn't been an issue for us so far.)\n\nI'm not saying I'm very happy with this workflow, it wasn't my decision.\nAnd in particular I'm not trying to argue which workflow is better than\nthe other; all I'm saying is that there are teams who decide they want\nto cherry-pick merge commits, so git should continue to allow it. This\nis only in response to your earlier \"If I were simplifying this, I would\nprobably [...] disallow cherry-picking a merge\".\n\n(Side note: my main gripe about cherry-picking in general is, of course,\nthat it makes it impossible to use \"git branch --contains\" or \"git tag\n--contains\" to find out which releases contain a given bug fix; but\nthat's a problem no matter whether you cherry-pick the merge commit, or\nreplay the branch on maint and merge it there again.)\n\n-Stefan\n"},{"id":"497583","messageId":"xmqqmsnab2ql.fsf@gitster.g","threadId":"61653","inReplyTo":"0a948acf-ebe9-407e-8899-d714b6fcb528@haller-berlin.de","subject":"Re: Thoughts about the -m option of cherry-pick and revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-24T18:43:46Z","receivedAt":"2024-06-24T18:43:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Haller writes:\n\n> And we prefer single-commit cherry-picks of the merge commits because it\n> makes the history of the release branches easier to read.\n\nI would have expected that \"git log --first-parent\" would give you\nthe same \"easy-to-read\" history as a run of squached cherry-picks.\n\nIt of course takes some discipline to ensure that the first-parent\nchain is kept meaningful, but \"never merge, always cherry-pick the\nmerges of topics\" also already takes some discipline, so I do not\nsee either workflow has much upside with respect to this point.\n\n> I'm not saying I'm very happy with this workflow, it wasn't my decision.\n> And in particular I'm not trying to argue which workflow is better than\n> the other; all I'm saying is that there are teams who decide they want\n> to cherry-pick merge commits, so git should continue to allow it. This\n> is only in response to your earlier \"If I were simplifying this, I would\n> probably [...] disallow cherry-picking a merge\".\n\nSure. I thought it was fairly obvious to everybody that I was not\n\"simplifying this\", at least unilaterally, so raising a concern like\nyou did was the right thing ;-).\n\n> (Side note: my main gripe about cherry-picking in general is, of course,\n> that it makes it impossible to use \"git branch --contains\" or \"git tag\n> --contains\" to find out which releases contain a given bug fix; but\n> that's a problem no matter whether you cherry-pick the merge commit, or\n> replay the branch on maint and merge it there again.)\n\nCorrect for the \"cherry-picking\", but not necessarily for a \"rebase\nand merge\".\n\nYou can have (1) a \"fix\" based on the \"main\", and (2) the backport\nof the same \"fix\" rebased on the \"maint\", the latter of which has\nlikely been spawned after the former was merged to \"main\". You can\nmerge the \"rebased fix\" to the \"maint\" *as well as* to the \"main\".\n\nIf we think about it, that is a natural thing to do. By rebasing\n\"fix\" to \"maint\" to create \"rebased fix\", we are \"correcting\" an\nearlier mistake of basing the fix on a wrong (iow, too recent)\npoint, so after such a rebase, we treat the rebased result as the\nprimary thing.\n\nAfter that, \"contains\" will do the right thing for the \"rebased\nfix\", which is now the primary fix, and shows both \"main\" and\n\"maint\" has it.\n\nOf course, a project can choose to adopt a workflow that refuses to\nobtain such a benefit (presumably for other reasons that gives\nbenefits that outweigh the \"sigh, we cannot merge but have to\ncherry-pick, with all the inconveniences\").\n"}]}