Re: A rebase regression in Git 2.18.0

From: Elijah Newren <newren@gmail.com>
Date: Aug 28, 2018, 15:35 UTC
Message-ID: <CABPp-BENB=mqfFU4FGb2OS9VDV=9VdT71HhFLZwtyxD8MpdTMQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808281532220.16620@tvgsbejvaqbjf.bet>

On Tue, Aug 28, 2018 at 6:33 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 51 quoted lines

> 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

...

Show 13 quoted lines

> > >
> > > 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/