Re: A rebase regression in Git 2.18.0

[Elijah Newren <newren@gmail.com>]

On Tue, Aug 28, 2018 at 6:33 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 28 Aug 2018, Ævar Arnfjörð Bjarmason wrote:
> > On Tue, Aug 28 2018, Nikolay Kasyanov wrote:
> >
> > > I’ve found something that may be a regression in git rebase implementation in 2.18.0.
> > > First I spotted it on macOS but I can also confirm it happening on Linux.
> > > Git 2.19.0.rc0.48.gb9dfa238d is affected too.
> > >
> > > In order to trigger it, a repo layout similar to the following is required:
> > >
> > > files/
> > >     file1
> > >     file2
> > >     file3
> > >     file4
> > >     file5
> > > project
> > >
> > > Let’s call this state baseline. Then, in a branch, let’s edit project file and move file3 to nested/files subdirectory, here’s the final layout:
> > >
> > > files/
> > >     file1
> > >     file2
> > >     file4
> > >     file5
> > > nested/
> > >     files/
> > >             file3
> > > project
> > >
> > > Let’s get back to master and also edit project file to cause a conflict. After that trying to rebase the branch upon master will cause the following git status output:
> > >
> > > rebase in progress; onto baf8d2a
> > > You are currently rebasing branch 'branch' on 'baf8d2a'.
> > >   (fix conflicts and then run "git rebase --continue")
> > >   (use "git rebase --skip" to skip this patch)
> > >   (use "git rebase --abort" to check out the original branch)
> > >
> > > Changes to be committed:
> > >   (use "git reset HEAD <file>..." to unstage)
> > >
> > >     renamed:    files/file1 -> nested/files/file1
> > >     renamed:    files/file2 -> nested/files/file2
> > >     renamed:    files/file3 -> nested/files/file3
> > >     renamed:    files/file4 -> nested/files/file4
> > >     renamed:    files/file5 -> nested/files/file5
> > >
> > > Unmerged paths:
> > >   (use "git reset HEAD <file>..." to unstage)
> > >   (use "git add <file>..." to mark resolution)
> > >
> > >     both modified:   project
...
> > >
> > > Here’s a ready-to-use repository: https://github.com/nikolaykasyanov/git-rebase-bug.
> >
> > Thanks for the test case. This bisects down to 9c0743fe1e
> > ("merge-recursive: apply necessary modifications for directory renames",
> > 2018-04-19) first released as part of 2.18.0.
> >
> > I have not dug to see if the behavior change is desired or not, that
> > commit changed the results of a bunch of test cases, maybe it was
> > intended. Elijah?
>
> I think this was already mentioned before, in a different mail thread:
> have you tried whether `git rebase -m` fixes that behavior?

I'm not aware of a previous mention, but yes, using a rebase type
other than the default am one (either -m or -i) will fix this.  (I did
previously bring up that am-based rebase would fail to detect
directory renames, due to not even calling in to the recursive merge
machinery in many cases[1].  But this is an example of am-based rebase
doing the opposite -- detecting a directory rename where there is
none, which had never occurred to me until seeing this report.)


I'm pretty sure this is a bad interaction between the
build_fake_ancestor() stuff and directory rename detection.  You see,
you *think* the following three commits are being merged:

Base: files/{file1,file2,file3,file4,file5}, project_v1
branch: files/{file1,file2,file4,file5}, nested/files/file3, project_v2
master: files/{file1,file2,file3,file4,file5}, project_v3

But the default rebase (via builtin/am) does NOT do that.  It instead
merges the following trees:

Base: files/file3, project_v1
branch: nested/files/file3, project_v2
master: files/{file1,file2,file3,file4,file5}, project_v3


To the recursive machinery, that looks an awful lot like "branch"
renamed files/ -> nested/files/, and that master just added a bunch of
paths (file[1245]) into the files/ directory.  From this view, what
merge-recursive did was correct, it's just that rebase/am fed it
information that doesn't quite match what should really be merged.

Possible fixes:
  - Change builtin/am.c:fall_back_threeway() to use actual commit
trees when available instead of building fake minimal ones.  (One of
the problems with am, is that the "base" commit may not exist in the
current repo, so there's an issue here with threading information from
rebase down to am.)
  - Add a flag to turn off directory rename detection, and set the
flag for every call from am.c in order to avoid problems like this.

The first option might be a bit nicer for the end-user, but would only
help when am is called from rebase; when running `git am` directly,
things would get pretty messy.  So we might need the second option
anyway.  Since we're in -rc for 2.19, we should probably just go for
the second option.  I'll try to put together some patches this
evening.


[1] https://public-inbox.org/git/20180607171344.23331-4-newren@gmail.com/