[Bug] Stashing during merge loses MERGING state

[Bug] Stashing during merge loses MERGING state

[Tassilo Horn]

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]

Re: [Bug] Stashing during merge loses MERGING state

[Jeff King]

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

Re: [Bug] Stashing during merge loses MERGING state

[Tassilo Horn]

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/

Re: [Bug] Stashing during merge loses MERGING state

[Phil Hord]

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

Re: [Bug] Stashing during merge loses MERGING state

[Junio C Hamano]

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.

Re: [Bug] Stashing during merge loses MERGING state

[Elijah Newren]

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.

Re: [Bug] Stashing during merge loses MERGING state

[Chris Torek]

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

Re: [Bug] Stashing during merge loses MERGING state

[Elijah Newren]

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.

Re: [Bug] Stashing during merge loses MERGING state

[Elijah Newren]

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.