Re: Regression in 'git pull --rebase --autostash' since v2.32.0

[Felipe Contreras <felipe.contreras@gmail.com>]

Philippe Blain wrote:
> Le 2021-07-17 à 15:34, Felipe Contreras a écrit :
> > Hello,
> > 
> > Philippe Blain wrote:
> >> Your recent clean-up of 'git pull --autostash' seems to unfortunately have made things
> >> worse if the pull brings new files that conflict with existing untracked files,
> >> which makes the pull abort,
> >> and there are tracked files with modifications (so --autostash comes into play).
> >>
> >> Before your change, 'git pull --no-rebase --autostash' did *not* apply the autostash
> >> after the pull failed, thus loosing modifications to tracked files (and it did not save the
> >> stash entry !). 'git pull --rebase --autostash' correctly applied the autostash, but ended with
> >> a strange "error: could not detach HEAD".
> >>
> >> After your change, both 'git pull --no-rebase --autostash' and 'git pull --rebase --autostash'
> >> have the same buggy behaviour: they do not apply the autostash and do not save it in the stash list.
> >>
> >> I had already documented the old behaviour at [1]. Here, I copy my reproducer script
> >> (save it as "script"):
> > 
> > I cannot reproduce this. In my case the reproducer script never puts
> > anything in the stash list.
> 
> I tried again with GIT_CONFIG_GLOBAL=/dev/null on my end (and manually
> setting GIT_AUTHOR_{NAME,EMAIL}) to be sure that my config was not playing
> a role, and I still reproduce. Copying the output from my 1st email
> (for --rebase, but it's the same thing for --no-rebase):
> 
> 
> > ** ./script --rebase --autostash **
> > 
> > ~~~
> > + git status
> > On branch master
> > Your branch is up to date with 'origin/master'.
> > 
> > Changes not staged for commit:
> >   (use "git add <file>..." to update what will be committed)
> >   (use "git restore <file>..." to discard changes in working directory)
> >     modified:   file
> > 
> > Untracked files:
> >   (use "git add <file>..." to include in what will be committed)
> >     other
> > 
> > no changes added to commit (use "git add" and/or "git commit -a")
> > + git pull --rebase --autostash
> > remote: Enumerating objects: 4, done.
> > remote: Counting objects: 100% (4/4), done.
> > remote: Compressing objects: 100% (2/2), done.
> > remote: Total 3 (delta 0), reused 0 (delta 0), pack-reused 0
> > Unpacking objects: 100% (3/3), 283 bytes | 94.00 KiB/s, done.
> > From /Users/Philippe/Code/GIT-devel/BUGS/ggg-759-pull-autotash-untracked/test
> >    4ebab2f..fc7a169  master     -> origin/master
> > Updating 4ebab2f..fc7a169
> > Created autostash: cfd51b5
> > error: The following untracked working tree files would be overwritten by merge:
> >     other
> > Please move or remove them before you merge.
> > Aborting
> 
> Here is the bug: we should see "Applied autostash" after "Aborting". The 'git status'
> below is to verify that the autostash was indeed not applied, and the
> 'git stash list' was to check if the autostash was at least available
> in the stash list (it's not).

Ahh, I thought you expected `git stash list` to show something. If this
is the bug then yeah, I can reproduce.

> > + git status
> > On branch master
> > Your branch is behind 'origin/master' by 1 commit, and can be fast-forwarded.
> >   (use "git pull" to update your local branch)
> > 
> > Untracked files:
> >   (use "git add <file>..." to include in what will be committed)
> >     other
> > 
> > nothing added to commit but untracked files present (use "git add" to track)
> > + git stash list
> > # empty!
> > ~~~ 
> 
> Back to your reply:
> 
> > 
> > Moreover, this is not an issue of `git pull`, but `git merge`.
> > 
> > I can reproduce the problem that the modifications are lost like this:
> > 
> >    git init test
> >    (
> >      cd test
> >      date >> file
> >      git add file
> >      git commit -m 'add file'
> >      date >> other
> >      git add other
> >      git commit -m 'add other'
> >      git checkout -b topic @~
> >      date >> other
> >      date >> file
> >      git status
> >      git "$@" master
> >      git status
> >      git stash list
> >    )
> > 
> > Running this with 'rebase --autostash' fails and nothing is put in the
> > stash list, but the modifications to 'file' remain. I think this is the
> > correct behavior.
> 
> Yes. We see the following output:
> 
>      Created autostash: 69775ee
>      Applied autostash.
>      Successfully rebased and updated refs/heads/topic.
> 
> The code correctly applied the autostash after the rebase, so it's
> normal it's not kept in the stash list. I agree it's the correct behaviour.

Good.

> > But with 'merge --autostash' the modifications to 'file' are lost. That
> > is a bug, and it's already present in v2.32.
> 
> That's also true. From what I understand it dates back to the introduction
> of 'git merge --autostash'.

Yes, that's probably true.

> > Do you agree?
> 
> I agree that the bug with 'merge --autostash' was already present in v2.32.0.
> But since your 340062243a (pull: cleanup autostash check, 2021-06-17),
> 'git pull --rebase --autostash' calls 'git merge --autostash' in the fast-forward
> case, and so we hit the bug, which was not the case before since the fast-forward
> merge shortcut was skipped if '--autostash' was given for 'git pull --rebase'.

Yes we hit the bug with `git pull`, but the bug was there already when
people did `git merge`.

> The fix I suggested in [1] seems to fix both cases, but as I wrote there
> it still leaves two code paths that might trigger the bug. I'm not familiar
> at all with the code for these two code paths, so I'm not ready to send
> a formal patch; I thought I'd send the diff anyway if you or Denton (or anyone
> else) wants to start from there.

Unfortunately I'm not familiar with the `git merge` code either, I've
only been modifying the `git pull` code, so I would also be starting
from scratch if I tried to fix that myself.

But I think that's where it should be fixed.

Cheers.

-- 
Felipe Contreras