{"thread":{"id":"59317","subject":"Change branch in the middle of a merge","startedAt":"2023-02-27T12:13:21Z","lastAt":"2023-05-01T17:16:09Z","messageCount":4,"participants":["Tao Klerks","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"472778","messageId":"CAPMMpohyoaH92B9f46hwdX2S5WOYeRB2yMSKW-4UCrHe29SAZQ@mail.gmail.com","threadId":"59317","inReplyTo":null,"subject":"Change branch in the middle of a merge","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-02-27T12:12:55Z","receivedAt":"2023-02-27T12:13:21Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Hi folks,\n\nIn the past couple of years I've had a few instances of myself or\nothers starting a merge on a particular branch, resolving a bunch of\nconflicts, realizing \"this isn't ready for this branch yet\", and\nwanting to commit the resulting resolved index state *and merge\nmetadata* to a new local branch.\n\nThe \"obvious\" thing to do, like any other time you find you have some\nwork you want to commit to a new branch instead of the one you were\nworking on, is something like \"git checkout -b lets-test-this-thing\",\nfollowed by \"git commit\".\n\nIf you do that, you will probably *eventually* realize you didn't\nachieve your objective; the merge metadata disappeared when you\ncreated a new branch. The index is as you wanted it, and after you\ncommit the committed tree is as you wanted it (so your testing can\nproceed), but in the commit dialog you don't get the expected merge\nmessage prepopulated, and after you commit you don't see the expected\nsecond parent in the commit metadata - it's not a merge.\n\nAs as an interested adventurous user, you might then search for\nsomething like \"change branch in the middle of a merge\" in google, and\ndiscover torek's great stackoverflow answer\nhttps://stackoverflow.com/a/45341429/74296 where he explains one thing\nyou could/should do differently, once you understand this git\nbehavior. Basically, *don't* switch to a new branch at that point.\nInstead, you could/should commit to the \"wrong\" branch locally, then\ncreate your testing branch from there, then reset your original\nbranch, and thus you get where you wanted to be, with the original\nbranch unchanged and a new branch containing the merge.\n\nIf you're even more adventurous you can find other ways to do this, of\ncourse, like switching to *another* new branch, committing there, then\nswitching to the intended merge-testing branch, preparing a merge\nstate with \"-s ours --no-commit\", and restoring the desired tree with\n\"git restore\" (to then commit normally in the branch where you wanted\nto).\n\nIf you are trying to provide *simple*, reliable instructions that\ndon't require any depth of understanding of git nor any\npotentially-risky commands/habits, then you might instead just say\n\"always *start* your (risky) merges on new temporary branches, instead\nof doing them on the branch where you eventually want them after the\ntesting\".\n\nAll three of these strategies are quite awful things to have to teach users.\n\nAt first I hoped that \"git switch\" might have fixed this - and in a\nlimited sense it has, in that instead of *silently discarding* the\nmerge state/metadata, it refuses with \"fatal: cannot switch branch\nwhile merging\".\n\nI've briefly searched for discussion of this topic on this list in the\nlast couple of years, but not found any.\n\nI'd like to understand whether this behavior, the fact that you can't\neasily/straightforwardly/intuitively complete a merge onto a new\nbranch (with the exact same \"first parent\" commit of course), is\nintentional for some reason that I am failing to grasp, or just a\nmissing feature.\n\nIt makes complete sense, of course, that switching to a *different\ncommit* than the one you were on when you started the merge, should be\nillegal... but why would switching to the *same* commit be either\nillegal (git switch behavior) or destructive (git checkout behavior),\nas opposed to just letting you get on with it, as it does with other\nin-progress local working tree states?\n\nAssuming that I'm not missing anything, I'd like to propose a path\nforward something like this:\n\n* Change \"git switch\" to just allow switching in the middle of a\nmerge, when the destination commit is the same as the previous commit,\nwithout affecting the merge state/metadata. It is still experimental,\nafter all, so as long as we agree this is sane and helpful behavior,\nthe change should be welcome?\n* Add a config option to \"git checkout\" to have it behave the same way\nin this regard\n* Add a hint to \"git checkout\", which kicks in when you are destroying\na merge state in this way, helping you see:\n** That the merge state just disappeared\n** That using \"git switch\" instead would have prevented this issue\n** That there is also a config option to \"git checkout\" that would\nhave prevented this\n** How to recover the lost merge state in this instance\n* (update corresponding documentation)\n\nThe \"How to recover the lost merge state\" bit is non-trivial, the best\nI can come up with is... nasty, but at least looks like it should work\nacross all the common shells I know of:\n```\ngit switch -c temporary-merge-branch-<INITIAL-SHA>\ngit commit -m \"temporary commit of merge result\"\ngit checkout <THE-BRANCH-YOU-CHECKED-OUT>\ngit merge -s ours <THE-BRANCH-THAT-HAD-BEEN-MERGED> --no-commit\ngit restore --worktree --staged --source\ntemporary-merge-branch-<INITIAL-SHA> -- .\n```\n\nIf the behavior change to \"git switch\" is not acceptable, then a\nparameter, hint, and config option could be added so you can move\nforward from the current switch failure with the new behavior.\n\nIf the proposed behavior is for some reason utterly impossible to\nachieve, then I suppose a hint providing the above \"here is now you\nget to where you probably wanted to be\" procedure would be an\nabsolute-no-brainer change that I could make.\n\nPlease let me know if I'm missing any history, concerns, etc. I\nhaven't looked at the code to determine whether this is a direction\nI'd be comfortable trying to deliver on myself, but I'd like some\nunderstanding of whether it's a reasonable direction to be pursuing at\nall, before I try.\n\nThanks,\nTao\n"},{"id":"476326","messageId":"CAPMMpognK_g5GL4EDamUmA5bzWiy9_VPf=a+0D5AnFtngUNa+A@mail.gmail.com","threadId":"59317","inReplyTo":"CAPx1GveK2J8Wu4cBYzO535c4bQ7P_t2fd1KFJqqphrm2HGQPSA@mail.gmail.com","subject":"Re: Change branch in the middle of a merge","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-01T05:51:14Z","receivedAt":"2023-05-01T05:51:32Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"(background note: Chris accidentally sent a non-plaintext response,\nand I failed to respond for some time, but I still believe this is\nsomething worth fixing)\n\nOn Mon, Feb 27, 2023 at 10:16 PM Chris Torek <chris.torek@gmail.com> wrote:\n>\n> On Mon, Feb 27, 2023 at 4:18 AM Tao Klerks <tao@klerks.biz> wrote:\n>>\n>>\n>> All three of these strategies are quite awful things to have to teach users.\n>\n>\n> I agree.\n>\n\nThanks for confirming, this helped motivate my at least attempting a patch.\n\nIt's not ready yet, but what I have so far seems to work.\n\n>>\n>> I'd like to understand whether this behavior, the fact that you can't\n>> easily/straightforwardly/intuitively complete a merge onto a new\n>> branch (with the exact same \"first parent\" commit of course), is\n>> intentional for some reason that I am failing to grasp, or just a\n>> missing feature.\n>\n>\n> I don't know the answer to this.\n>\n> Internally, it seems trivial to fix: for the \"create new branch\" cases,\n> simply *don't* remove the `MERGE_HEAD` ref.  This has an\n> annoying side effect: you can now create new branch names but\n> not switch among existing branches.  But that's kind of inherent to\n> this state.\n\nI don't understand this bit, but I'm probably missing something.\n\nThe approach I've taken here is to, when the original commit and the\nnew commit are the same (you're switching to a same-head branch, or\ncreating a new branch at the current head), change the\n\"remove_branch_state()\" call at\nhttps://github.com/git/git/blob/2807bd2c10606e590886543afe4e4f208dddd489/builtin/checkout.c#L1012\n/ implemented at\nhttps://github.com/git/git/blob/2807bd2c10606e590886543afe4e4f208dddd489/branch.c#L818,\nto not do the \"remove_merge_branch_state()\" bit - that is, avoid all\nthe removals at\nhttps://github.com/git/git/blob/2807bd2c10606e590886543afe4e4f208dddd489/branch.c#L808.\n\nThis behaves as expected for switching to new and expected branches.\nTo change the \"git checkout\" behavior this change is basically all\nthat's needed (plus tests etc), but for \"git switch\" we also need to\nchange some validation which would refuse to do the switch in the\nmiddle of a merge.\n\nWith these changes things behave in what I think is a very reasonable\nway, with one potentially-surprising aspect: the merge message\nproposed when you go to commit still refers to the branch you were on\nwhen you prepared the merge, rather than the one you're on now. I\npersonally believe this is quite reasonable (you'd still see this if\nyou followed any other process involving fast-forwarding for example),\nbut I can see how it's not perfect.\n\nThe bit I'm having more conceptual difficulty with is considering what\nto \"say\" when a checkout/switch completes with a merge-in-progress.\nHistorically, this happened \"as normal\", with no special messaging.\nNow, I think there should be a message depending on whether the merge\nstate was discarded (head commit changed) or not:\n * If the commit remained the same and so the merge state was\nretained, state as much and warn about the commit message referring to\nthe previous branch name.\n * If the commit changed and so the merge state was discarded (\"git\ncheckout\" only - not possible with \"git switch\"), state as much and\nsuggest that using \"git switch\" would prevent this happening\naccidentally in future.\n\nDoes this kind of messaging make sense to you?\n\n>\n> (Whether this is generally acceptable to others, I don't know. It would\n> break backwards compatibility with `git checkout -b`, obviously, and\n> making `git checkout -b` and `git switch -c` behave *differently* here\n> feels wrong to me, but that's a different thing.)\n>\n\nAfter thinking about this a bit more, I agree that having divergent\ndifferent behavior for checkout and switch would suck. The current\nexisting divergences, as far as I can tell, are around\nvalidation/safety, not behavior when actions are completed.\n\nI think the best thing is to propose a patch that incorporates a\nchange in checkout behavior (stop throwing away merge metadata when\nyou do a checkout/switch in the middle of a merge), with new messaging\nas outlined above, and see how people feel about it. I'm hopeful that\na patch will see some more reactions than this thread did in its first\nround.\n"},{"id":"476338","messageId":"xmqqpm7k6ojz.fsf@gitster.g","threadId":"59317","inReplyTo":"CAPMMpohyoaH92B9f46hwdX2S5WOYeRB2yMSKW-4UCrHe29SAZQ@mail.gmail.com","subject":"Re: Change branch in the middle of a merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T15:48:00Z","receivedAt":"2023-05-01T15:48:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tao Klerks <tao@klerks.biz> writes:\n\n> At first I hoped that \"git switch\" might have fixed this - and in a\n> limited sense it has, in that instead of *silently discarding* the\n> merge state/metadata, it refuses with \"fatal: cannot switch branch\n> while merging\".\n\nI think this is in generally seen as a vast improvement over the\n\"silently discarding\" to have the safety valve.\n\n> I'd like to understand whether this behavior, the fact that you can't\n> easily/straightforwardly/intuitively complete a merge onto a new\n> branch (with the exact same \"first parent\" commit of course), is\n> intentional for some reason that I am failing to grasp, or just a\n> missing feature.\n>\n> It makes complete sense, of course, that switching to a *different\n> commit* than the one you were on when you started the merge, should be\n> illegal... but why would switching to the *same* commit be either\n> illegal (git switch behavior) or destructive (git checkout behavior),\n> as opposed to just letting you get on with it, as it does with other\n> in-progress local working tree states?\n\nI think this is mostly that nobody spent enough brain cycles to\nponder on things like:\n\n - Is it always safe to switch from a merge in progress to a newly\n   created branch that point at the same commit as HEAD?  Why?  Is\n   the story the same if the switched-to branch is an existing one\n   that happens to point at the same commit as HEAD?\n\n - Maybe the answer to the above question is not \"it is not quite\n   safe to switch during a conflicted merge, but by adjusting X and\n   Y while switching to a new branch, we could make it safe\".\n \n - Even if the answer to the question above is \"yes it is always\n   safe\", if it is not safe for a similar situation that ends up in\n   unmerged index (i.e. the index with any path at non-0 stage), the\n   way we determine if we are in \"a merge in progress\" situation\n   must be reliable.  Is it?\n\n - Conversely, perhaps it is also safe to switch to a different\n   branch that points at the same commit as HEAD from the conflicted\n   state after some (but not necessarily all) of the following\n   operations: \"am -3\", \"cherry-pick\" (single commit), \"revert\"\n   (single commit), etc.  Can we come up with a reliable way to\n   determine if we are in such a situation?\n\n - These \"other mergy operations\" that can leave unmerged index may\n   share the same \"is not safe out of the box, but can be made safe\n   by adjusting some stuff\".\n\nAfter that is done, we could update the codepath in \"git switch\"\nthat says \"ah, your index is unmerged, so I will not let you switch\"\nto instead say \"your index is unmerged; let's first see if you are\nswitching to the same commit as HEAD and then the way your index is\nunmerged was caused by an operation that we decided is safe to\nswitch out of.  It seems to be one of the situations that could be\nmade safe, so let's do adjustment X and Y and let you switch\".\n\nAnd the reason why \"git switch\" punts is because nobody bothered to,\nnot because there fundamentally is some reason why it shouldn't work.\n"},{"id":"476359","messageId":"CAPMMpoht4FWnv-WuSM3+Z2R4HhwFY+pahJ6zirFU-BD5r34B7Q@mail.gmail.com","threadId":"59317","inReplyTo":"xmqqpm7k6ojz.fsf@gitster.g","subject":"Re: Change branch in the middle of a merge","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2023-05-01T17:13:08Z","receivedAt":"2023-05-01T17:16:09Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"On Mon, May 1, 2023 at 5:48 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tao Klerks <tao@klerks.biz> writes:\n>\n> > At first I hoped that \"git switch\" might have fixed this - and in a\n> > limited sense it has, in that instead of *silently discarding* the\n> > merge state/metadata, it refuses with \"fatal: cannot switch branch\n> > while merging\".\n>\n> I think this is in generally seen as a vast improvement over the\n> \"silently discarding\" to have the safety valve.\n>\n\nHeh, that's fair - my apologies for the \"ungrateful\" tone.\n\n>\n> I think this is mostly that nobody spent enough brain cycles to\n> ponder on things like:\n\n(I'm not sure I *have* enough brain cycles, or background knowledge,\nto be sure of these things, but I have some tentative answers)\n\n>\n>  - Is it always safe to switch from a merge in progress to a newly\n>    created branch that point at the same commit as HEAD?  Why?  Is\n>    the story the same if the switched-to branch is an existing one\n>    that happens to point at the same commit as HEAD?\n\nTo the best of my knowledge, the only role that the HEAD *ref* plays\nin any of merge processing (before or after resolution), if there even\nis one, is its use in the commit message.\n\nMy proposal is that keeping the original prepared merge message, but\nwarning about its use, makes more sense / is more straightforward than\nanything else we might choose to do here, like re-generating the\nmessage.\n\nWith respect to the safety of index operations - when conflicts have\nbeen resolved already the index is in a \"normal\" state, same as a\nnon-merge checkout with changes, so there is no reason to believe\nthere would be any \"unsafeness\" to worry about?\n\n>\n>  - Maybe the answer to the above question is not \"it is not quite\n>    safe to switch during a conflicted merge, but by adjusting X and\n>    Y while switching to a new branch, we could make it safe\".\n\nThe sense in which it is potentially *not* safe, is that some part of\ncheckout may mess with the index in some way that does not expect to\nencounter conflicts - given that conflicts explicitly prevent not only\na \"git switch\", but also a \"git checkout\", at this time (due to a\ncheck for unmerged paths in builtin/checkout.c#merge_working_tree()).\n\nAs such, the \"simple answer\" is to address the case where conflicts\nare already resolved, and defer the case of open conflicts to someone\nwho is better able to check or enable the safety of a switch in this\nstate.\n\nI believe the urgency/criticality of opening up a \"switch while you\nstill have unmerged paths\" usecase is much lower than the\nsimpler-to-prove \"switch before committing an already-resolved index\"\nusecase.\n\n>\n>  - Even if the answer to the question above is \"yes it is always\n>    safe\", if it is not safe for a similar situation that ends up in\n>    unmerged index (i.e. the index with any path at non-0 stage), the\n>    way we determine if we are in \"a merge in progress\" situation\n>    must be reliable.  Is it?\n\nI'm not sure what \"a path at 0 stage\" means, but I do get the\n*impression* that the conflict checks that are in place in\nbuiltin/checkout.c#merge_working_tree() look reasonable...? That said,\nI do not intend to modify or circumvent these checks, because I don't\nthink the user flow / use case requires it.\n\n>\n>  - Conversely, perhaps it is also safe to switch to a different\n>    branch that points at the same commit as HEAD from the conflicted\n>    state after some (but not necessarily all) of the following\n>    operations: \"am -3\", \"cherry-pick\" (single commit), \"revert\"\n>    (single commit), etc.  Can we come up with a reliable way to\n>    determine if we are in such a situation?\n\nI suspect it is, but I also suspect this is a far less common case,\nand as such one I'm less inclined to worry about. In merge-based\nworkflows, merges can be *large*, and the case where you have a merge\nready, conflicts resolved, and you realize you'd like to test it on\nanother branch, is more common and impactful, than when you have some\n(far more typically limited in scope) stash or cherry-pick...?\n\n(Or maybe I'm just making excuses for the patch I want to submit :) )\n\nMy proposal at least is to carve out an exception, for now, only for\nmerges, and only with a clean (not-conflicted) index.\n\n>\n>  - These \"other mergy operations\" that can leave unmerged index may\n>    share the same \"is not safe out of the box, but can be made safe\n>    by adjusting some stuff\".\n>\n> After that is done, we could update the codepath in \"git switch\"\n> that says \"ah, your index is unmerged, so I will not let you switch\"\n> to instead say \"your index is unmerged; let's first see if you are\n> switching to the same commit as HEAD and then the way your index is\n> unmerged was caused by an operation that we decided is safe to\n> switch out of.  It seems to be one of the situations that could be\n> made safe, so let's do adjustment X and Y and let you switch\".\n\nThis is what I am proposing, but no adjustment needed if the scope is\na merge only and the index is already unconflicted.\n\n>\n> And the reason why \"git switch\" punts is because nobody bothered to,\n> not because there fundamentally is some reason why it shouldn't work.\n\nYep, that makes sense.\n"}]}