* [Bug] Stashing during merge loses MERGING state @ 2021-03-11 14:00 Tassilo Horn 2021-03-11 19:25 ` Jeff King 0 siblings, 1 reply; 9+ messages in thread From: Tassilo Horn @ 2021-03-11 14:00 UTC (permalink / raw) To: git Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue. What did you do before the bug happened? (Steps to reproduce your issue) I did a large merge, resolved the conflicts but still had compile errors. In order to check how stuff has worked before the merge, I stashed all changes, and eventually popped the stash. Then the MERGING state was gone and committing created a single-parent commit rather than a merge commit with two parents. I was lucky to see that before pushing, so all is good. Here is a simple recipe with a publicly available repo: ```sh git clone https://github.com/magit/magit.git # Current master is 4735b9254105eb7dd538f979d8b4c6ab96b827b9 cd magit git merge origin/km/reshelve-rewrite # currently 0073bff21c826a57a4b48076074bdbba092d4b67 # Conflict in magit-extras.el git add lisp/magit-extras.el git stash # The MERGING state is gone git stash pop # And it won't come back, so when I commit now, my "merge" has just # one parent. ``` What did you expect to happen? (Expected behavior) I expected that stashing during a merge will keep the MERGING state. Or that popping the stash again would also restore the MERGING state. What happened instead? (Actual behavior) The MERGING state is lost. What's different between what you expected and what actually happened? I've lost the MERGING state, thus committing results in a one-parent commit rather than a 2-parent merge commit. Anything else you want to add: In my original situation, I've copied the working tree to /tmp, resetted to origin/master, re-initiated the merge, copied the modified files from the /tmp backup, staged the changes, committed, and pushed. I wonder if there would have been a better approach to come from the "accidentally having a single-parent commit containing all merged and resolution changes" state to a proper merge commit state. In the end, the tree was correct, just the second parent to the commit on the merged branch was missing. Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.30.2 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 5.11.4-arch1-1 #1 SMP PREEMPT Sun, 07 Mar 2021 18:00:49 +0000 x86_64 compiler info: gnuc: 10.2 libc info: glibc: 2.33 $SHELL (typically, interactive shell): /usr/bin/fish [Enabled Hooks] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-11 14:00 [Bug] Stashing during merge loses MERGING state Tassilo Horn @ 2021-03-11 19:25 ` Jeff King 2021-03-11 20:31 ` Tassilo Horn 0 siblings, 1 reply; 9+ messages in thread From: Jeff King @ 2021-03-11 19:25 UTC (permalink / raw) To: Tassilo Horn; +Cc: git On Thu, Mar 11, 2021 at 03:00:58PM +0100, Tassilo Horn wrote: > Here is a simple recipe with a publicly available repo: > > ```sh > git clone https://github.com/magit/magit.git > # Current master is 4735b9254105eb7dd538f979d8b4c6ab96b827b9 > cd magit > git merge origin/km/reshelve-rewrite # currently 0073bff21c826a57a4b48076074bdbba092d4b67 > # Conflict in magit-extras.el > git add lisp/magit-extras.el > git stash > # The MERGING state is gone > git stash pop > # And it won't come back, so when I commit now, my "merge" has just > # one parent. > ``` > > What did you expect to happen? (Expected behavior) > > I expected that stashing during a merge will keep the MERGING state. Thanks for providing a clear recipe and expectation. However, I think Git is working here as intended. The MERGE_HEAD file (which is how "git status", the prompt, etc figure out that we're in the middle of a merge) is cleaned up when stash runs "git reset --hard" under the hood. However, I don't think we would want to _not_ clear that file. The conflicted merge placed some changes into the index and working tree representing what happened on the branch you're merging in. Then making the stash (and the reset of the working tree) removes those changes. If we were to leave MERGE_HEAD in place and you ran "git commit", then it would create a merge commit that claims to have incorporated everything from the other branch, but has quietly dropped those changes as part of the merge resolution. > Or that popping the stash again would also restore the MERGING state. This would make more sense: the stash records that part of the state, and then we make it available again later when the stash is applied. However, that feature doesn't exist yet. I can't offhand think of a reason it couldn't be implemented. It's possible that it would mess with somebody else's workflow (e.g., they think it's useful to stash some changes independent of the merging state, and then apply it later, perhaps while replaying the same or a similar merge). So it might need to be tied to a command-line option or similar. -Peff ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-11 19:25 ` Jeff King @ 2021-03-11 20:31 ` Tassilo Horn 2021-03-12 5:00 ` Phil Hord 0 siblings, 1 reply; 9+ messages in thread From: Tassilo Horn @ 2021-03-11 20:31 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: Hi Jeff, >> What did you expect to happen? (Expected behavior) >> >> I expected that stashing during a merge will keep the MERGING state. > > Thanks for providing a clear recipe and expectation. However, I think > Git is working here as intended. The MERGE_HEAD file (which is how "git > status", the prompt, etc figure out that we're in the middle of a merge) > is cleaned up when stash runs "git reset --hard" under the hood. > > However, I don't think we would want to _not_ clear that file. The > conflicted merge placed some changes into the index and working tree > representing what happened on the branch you're merging in. Then > making the stash (and the reset of the working tree) removes those > changes. If we were to leave MERGE_HEAD in place and you ran "git > commit", then it would create a merge commit that claims to have > incorporated everything from the other branch, but has quietly dropped > those changes as part of the merge resolution. Yes, that makes sense. >> Or that popping the stash again would also restore the MERGING state. > > This would make more sense: the stash records that part of the state, > and then we make it available again later when the stash is applied. > However, that feature doesn't exist yet. Too bad. > I can't offhand think of a reason it couldn't be implemented. It's > possible that it would mess with somebody else's workflow (e.g., they > think it's useful to stash some changes independent of the merging > state, and then apply it later, perhaps while replaying the same or a > similar merge). So it might need to be tied to a command-line option > or similar. Everything breakes someones workflow [1], so an option would be fine. However, I'd suggest to protect users shooting in their foot with a warning and confirmation query for the time being. I consider myself a quite experienced git user but this stash trouble today came totally unexpected. And I've asked on #git@irc.freenode.net and got no answer which is totally uncommon. So I guess that this stash during merge thing is pretty much a gray area. Bye, Tassilo [1] https://xkcd.com/1172/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-11 20:31 ` Tassilo Horn @ 2021-03-12 5:00 ` Phil Hord 2021-03-12 6:09 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Phil Hord @ 2021-03-12 5:00 UTC (permalink / raw) To: Tassilo Horn; +Cc: Jeff King, Git On Thu, Mar 11, 2021 at 12:45 PM Tassilo Horn <tsdh@gnu.org> wrote: > >> Or that popping the stash again would also restore the MERGING state. > > > > This would make more sense: the stash records that part of the state, > > and then we make it available again later when the stash is applied. > > However, that feature doesn't exist yet. > > Too bad. Consider also what happens when `git stash apply` results in a merge conflict because of differences between your current index and the one you had when you originally saved the stash. This results in the usual merge conflict markers that then need to be cleaned up. Could we sanely deal with this in a world where we also tried to restore .git/MERGE_HEAD when the stash was applied. Something like `git stash apply --continue`, possibly after resolving the stash conflicts? But what if we stashed the merge conflict that resulted from the stash apply? I guess it would still work, but the stash history would be, um, interesting. I wonder if a fix could be as simple as recording the MERGE_HEAD as the third parent commit of the stash ref. I think that would provide all the information needed to put things back, except possibly for things like the rerere state, which is also set up during a conflict, and other incidentals like .git/MERGE_MSG. (And it feels like it might break compatibility with older versions that don't expect a third parent.) I would be a bit concerned about the possibility of silently creating an "evil merge". Suppose you stash this conflict on some branch and then pop it onto a different one. I expect we would then be prepared to store all those changes from a different branch including existing resolved merge conflicts into the new one. That could be surprising and subtle. But maybe I'm overthinking it. Wouldn't the stash apply result in merge conflicts that would catch out all the troubling parts? I think being able to stash during a merge conflict could be very useful. I do sometimes need to get back to a working state momentarily and a merge conflict represents a long pole to doing so. Similarly, it could be useful to stash a conflicted `git rebase` so I could return to it later and pick up where I left off. Now we really would need to store some extra metadata, though, like the todo-list and ORIG_HEAD. And we would definitely need some extra command line switch to tell stash (or rebase) that I want to include all the rebase state and also "pause" the rebase by restoring to my starting point. Thanks for raising the issue, Tassilo. This has obviously given me more ideas for things I forgot were missing. > > I can't offhand think of a reason it couldn't be implemented. It's > > possible that it would mess with somebody else's workflow (e.g., they > > think it's useful to stash some changes independent of the merging > > state, and then apply it later, perhaps while replaying the same or a > > similar merge). So it might need to be tied to a command-line option > > or similar. > > Everything breakes someones workflow [1], so an option would be fine. > > However, I'd suggest to protect users shooting in their foot with a > warning and confirmation query for the time being. I consider myself a > quite experienced git user but this stash trouble today came totally > unexpected. And I've asked on #git@irc.freenode.net and got no answer > which is totally uncommon. So I guess that this stash during merge > thing is pretty much a gray area. I don't think we could easily add the warning when the stash is applied since we have forgotten the merge existed in the first place. So we would have to do it during stash save. "Warning: You are stashing during a merge conflict and your merge state will not be restored by stash apply." Seems reasonable. Phil ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-12 5:00 ` Phil Hord @ 2021-03-12 6:09 ` Junio C Hamano 2021-03-12 7:04 ` Elijah Newren 2021-03-12 7:02 ` Chris Torek 2021-03-12 7:02 ` Elijah Newren 2 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2021-03-12 6:09 UTC (permalink / raw) To: Phil Hord; +Cc: Tassilo Horn, Jeff King, Git Phil Hord <phil.hord@gmail.com> writes: > On Thu, Mar 11, 2021 at 12:45 PM Tassilo Horn <tsdh@gnu.org> wrote: >> >> Or that popping the stash again would also restore the MERGING state. >> > >> > This would make more sense: the stash records that part of the state, >> > and then we make it available again later when the stash is applied. >> > However, that feature doesn't exist yet. >> >> Too bad. > > Consider also what happens when `git stash apply` results in a merge > conflict because of differences between your current index and the one > you had when you originally saved the stash. This results in the > usual merge conflict markers that then need to be cleaned up. I agree with you that allowing a stash while a merge is in progress will introduce many unpleasant corner cases the users wouldn't want to deal with. We certainly do prevent "git stash push" from running when the index is still unmerged (which is a sign that a mergy operation (like pull, rebase, merge, am -3, cherry-pick and revert that stops due to a conflict in the middle) is in progress), but once the end user resolves the conflicts in the index (either manually, or having the rerere.autoupdate feature in effect), such a sign of mergy operation still in progress that "git stash" currently uses will be gone. We should teach "git stash push" to pay attention to other such signs like MERGE_HEAD etc. and stop before creating a stash (and also do the same to "git stash pop/apply"). THanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-12 6:09 ` Junio C Hamano @ 2021-03-12 7:04 ` Elijah Newren 0 siblings, 0 replies; 9+ messages in thread From: Elijah Newren @ 2021-03-12 7:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Hord, Tassilo Horn, Jeff King, Git On Thu, Mar 11, 2021 at 10:15 PM Junio C Hamano <gitster@pobox.com> wrote: > > Phil Hord <phil.hord@gmail.com> writes: > > > On Thu, Mar 11, 2021 at 12:45 PM Tassilo Horn <tsdh@gnu.org> wrote: > >> >> Or that popping the stash again would also restore the MERGING state. > >> > > >> > This would make more sense: the stash records that part of the state, > >> > and then we make it available again later when the stash is applied. > >> > However, that feature doesn't exist yet. > >> > >> Too bad. > > > > Consider also what happens when `git stash apply` results in a merge > > conflict because of differences between your current index and the one > > you had when you originally saved the stash. This results in the > > usual merge conflict markers that then need to be cleaned up. > > I agree with you that allowing a stash while a merge is in progress > will introduce many unpleasant corner cases the users wouldn't want > to deal with. We certainly do prevent "git stash push" from running > when the index is still unmerged (which is a sign that a mergy > operation (like pull, rebase, merge, am -3, cherry-pick and revert > that stops due to a conflict in the middle) is in progress), but > once the end user resolves the conflicts in the index (either > manually, or having the rerere.autoupdate feature in effect), such a > sign of mergy operation still in progress that "git stash" currently > uses will be gone. We should teach "git stash push" to pay > attention to other such signs like MERGE_HEAD etc. and stop before > creating a stash (and also do the same to "git stash pop/apply"). > > THanks. I should have read the rest of the emails before responding. Once again, you manage to say roughly what I was thinking, but do so both more concisely and more eloquently. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-12 5:00 ` Phil Hord 2021-03-12 6:09 ` Junio C Hamano @ 2021-03-12 7:02 ` Chris Torek 2021-03-12 7:17 ` Elijah Newren 2021-03-12 7:02 ` Elijah Newren 2 siblings, 1 reply; 9+ messages in thread From: Chris Torek @ 2021-03-12 7:02 UTC (permalink / raw) To: Phil Hord; +Cc: Tassilo Horn, Jeff King, Git On Thu, Mar 11, 2021 at 9:19 PM Phil Hord <phil.hord@gmail.com> wrote: > I wonder if a fix could be as simple as recording the MERGE_HEAD as > the third parent commit of the stash ref. There is already a third parent, but only when using -u / -a: this third-parent commit holds the untracked files (which are then removed a la `git clean`). A better trick would, I think, be able to save a partial merge state in general, including the unmerged statuses of various files, the ongoing action (merge or one of the other things that use it), and so on, in a form that could be restored later. A plain `git stash` in any partially-merged state should tell you: no, use the fancier merge-state-saver instead. > I think being able to stash during a merge conflict could be very > useful. I do sometimes need to get back to a working state > momentarily and a merge conflict represents a long pole to doing so. > Similarly, it could be useful to stash a conflicted `git rebase` so I > could return to it later and pick up where I left off. Now we really > would need to store some extra metadata, though, like the todo-list > and ORIG_HEAD. And we would definitely need some extra command line > switch to tell stash (or rebase) that I want to include all the rebase > state and also "pause" the rebase by restoring to my starting point. This is the sort of thing I'm thinking of, for the "superstash" (terrible name for it). Note that whatever this becomes, it should be send-able (via push and/or email) so that you can have multiple people work on resolving a particularly hairy merge. Chris ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-12 7:02 ` Chris Torek @ 2021-03-12 7:17 ` Elijah Newren 0 siblings, 0 replies; 9+ messages in thread From: Elijah Newren @ 2021-03-12 7:17 UTC (permalink / raw) To: Chris Torek; +Cc: Phil Hord, Tassilo Horn, Jeff King, Git On Thu, Mar 11, 2021 at 11:07 PM Chris Torek <chris.torek@gmail.com> wrote: > > On Thu, Mar 11, 2021 at 9:19 PM Phil Hord <phil.hord@gmail.com> wrote: > > I wonder if a fix could be as simple as recording the MERGE_HEAD as > > the third parent commit of the stash ref. > > There is already a third parent, but only when using -u / -a: this > third-parent commit holds the untracked files (which are then removed > a la `git clean`). > > A better trick would, I think, be able to save a partial merge state > in general, including the unmerged statuses of various files, the > ongoing action (merge or one of the other things that use it), and > so on, in a form that could be restored later. A plain `git stash` in > any partially-merged state should tell you: no, use the fancier > merge-state-saver instead. > > > I think being able to stash during a merge conflict could be very > > useful. I do sometimes need to get back to a working state > > momentarily and a merge conflict represents a long pole to doing so. > > Similarly, it could be useful to stash a conflicted `git rebase` so I > > could return to it later and pick up where I left off. Now we really > > would need to store some extra metadata, though, like the todo-list > > and ORIG_HEAD. And we would definitely need some extra command line > > switch to tell stash (or rebase) that I want to include all the rebase > > state and also "pause" the rebase by restoring to my starting point. > > This is the sort of thing I'm thinking of, for the "superstash" (terrible > name for it). Note that whatever this becomes, it should be send-able > (via push and/or email) so that you can have multiple people work > on resolving a particularly hairy merge. You can use git-imerge (https://github.com/mhagger/git-imerge) for that...just not once you've already started a merge (unless you're willing to undo and restart). As per my other email, I'm pretty worried about attempting to stash a merge, given that stash allows its contents to be "reapplied" on top of a different commit. That works fine for single-parent commits, but merges have multiple parents and it'd be rather tricky to reapply such a stash on a different base without creating an evil merge. Perhaps if "superstash" forced you to only reapply on the same base then it'd be fine...but it's starting to no longer be a "stash" anymore but something else. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Bug] Stashing during merge loses MERGING state 2021-03-12 5:00 ` Phil Hord 2021-03-12 6:09 ` Junio C Hamano 2021-03-12 7:02 ` Chris Torek @ 2021-03-12 7:02 ` Elijah Newren 2 siblings, 0 replies; 9+ messages in thread From: Elijah Newren @ 2021-03-12 7:02 UTC (permalink / raw) To: Phil Hord; +Cc: Tassilo Horn, Jeff King, Git On Thu, Mar 11, 2021 at 9:21 PM Phil Hord <phil.hord@gmail.com> wrote: > > On Thu, Mar 11, 2021 at 12:45 PM Tassilo Horn <tsdh@gnu.org> wrote: > > >> Or that popping the stash again would also restore the MERGING state. > > > > > > This would make more sense: the stash records that part of the state, > > > and then we make it available again later when the stash is applied. > > > However, that feature doesn't exist yet. > > > > Too bad. > > Consider also what happens when `git stash apply` results in a merge > conflict because of differences between your current index and the one > you had when you originally saved the stash. This results in the > usual merge conflict markers that then need to be cleaned up. > > Could we sanely deal with this in a world where we also tried to > restore .git/MERGE_HEAD when the stash was applied. Something like > `git stash apply --continue`, possibly after resolving the stash > conflicts? But what if we stashed the merge conflict that resulted > from the stash apply? I guess it would still work, but the stash > history would be, um, interesting. > > I wonder if a fix could be as simple as recording the MERGE_HEAD as > the third parent commit of the stash ref. I think that would provide > all the information needed to put things back, except possibly for > things like the rerere state, which is also set up during a conflict, > and other incidentals like .git/MERGE_MSG. (And it feels like it > might break compatibility with older versions that don't expect a > third parent.) Third parent is already reserved by --untracked to store a "parent" commit that has all the untracked files added. So, it'd be a fourth parent. Except when --untracked isn't passed we don't need the third parent, and if it wasn't a merge commit we don't need the fourth...fun times. > I would be a bit concerned about the possibility of silently creating > an "evil merge". Suppose you stash this conflict on some branch and > then pop it onto a different one. I expect we would then be prepared > to store all those changes from a different branch including existing > resolved merge conflicts into the new one. That could be surprising > and subtle. > > But maybe I'm overthinking it. Wouldn't the stash apply result in > merge conflicts that would catch out all the troubling parts? I don't think you're overthinking it; I think this is highly problematic. Let's consider the following history (from the git-rebase manpage): o---o---o---o---o master \ o---o---o---o---o next \ o---o---o topic First, let's consider the case where you don't record MERGE_HEAD when stashing. So, you start out on next and run: $ git merge topic but you hit some conflicts and stash it: $ git stash push -m "in middle of merging topic" Note that this would record applying the changes new to "topic" since "next", with whatever conflicts still exist. Then sometime later you check out master, and decide to apply the stash: $ git stash apply What would you get? Just the changes in topic new relative to next, but nothing in next that preceded it. If you record that as a merge commit with topic as the second parent, you've created an evil merge. Of course, that was saying if you didn't store MERGE_HEAD somewhere. But now let's say you did store MERGE_HEAD (== topic) when you stashed. What then happens when you go to apply the stash? If you really want a merge with MERGE_HEAD, then you need to redo the merge. So you invoke the merge machinery. That gives you conflicts anew. How do you use your stash at this point to resolve the set of conflicts, even if they were the same as what you got in your original merge? Personally, I think we should make stash check for $GIT_DIR/MERGE_HEAD, and error out if it exists. I don't see an easy way to make this behave reasonably. (Which isn't to say I don't see ways, but it'd probably involve being able to do a diff since the point when the merge stopped and recording it, then attempting to reapply _that_ patch, one that removes conflict markers and such, on top of a redone merge, possibly complete with nested conflict markers. But I'm not sure this is sounding like a reasonable level of difficulty for users to deal with.) > I think being able to stash during a merge conflict could be very > useful. I do sometimes need to get back to a working state > momentarily and a merge conflict represents a long pole to doing so. > Similarly, it could be useful to stash a conflicted `git rebase` so I > could return to it later and pick up where I left off. Now we really > would need to store some extra metadata, though, like the todo-list > and ORIG_HEAD. And we would definitely need some extra command line > switch to tell stash (or rebase) that I want to include all the rebase > state and also "pause" the rebase by restoring to my starting point. > > Thanks for raising the issue, Tassilo. This has obviously given me > more ideas for things I forgot were missing. > > > > I can't offhand think of a reason it couldn't be implemented. It's > > > possible that it would mess with somebody else's workflow (e.g., they > > > think it's useful to stash some changes independent of the merging > > > state, and then apply it later, perhaps while replaying the same or a > > > similar merge). So it might need to be tied to a command-line option > > > or similar. > > > > Everything breakes someones workflow [1], so an option would be fine. > > > > However, I'd suggest to protect users shooting in their foot with a > > warning and confirmation query for the time being. I consider myself a > > quite experienced git user but this stash trouble today came totally > > unexpected. And I've asked on #git@irc.freenode.net and got no answer > > which is totally uncommon. So I guess that this stash during merge > > thing is pretty much a gray area. > > I don't think we could easily add the warning when the stash is > applied since we have forgotten the merge existed in the first place. > So we would have to do it during stash save. > > "Warning: You are stashing during a merge conflict and your merge > state will not be restored by stash apply." > > Seems reasonable. I think it should be an error and abort the stash operation; this is way too much of a footgun. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-12 7:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-11 14:00 [Bug] Stashing during merge loses MERGING state Tassilo Horn 2021-03-11 19:25 ` Jeff King 2021-03-11 20:31 ` Tassilo Horn 2021-03-12 5:00 ` Phil Hord 2021-03-12 6:09 ` Junio C Hamano 2021-03-12 7:04 ` Elijah Newren 2021-03-12 7:02 ` Chris Torek 2021-03-12 7:17 ` Elijah Newren 2021-03-12 7:02 ` Elijah Newren
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).