git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: "Ævar Arnfjörð" <avarab@gmail.com>,
	corrmage@gmail.com, "Git Mailing List" <git@vger.kernel.org>,
	"Stefan Beller" <sbeller@google.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: A rebase regression in Git 2.18.0
Date: Tue, 28 Aug 2018 08:35:03 -0700	[thread overview]
Message-ID: <CABPp-BENB=mqfFU4FGb2OS9VDV=9VdT71HhFLZwtyxD8MpdTMQ@mail.gmail.com> (raw)
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:
> 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/

  parent reply	other threads:[~2018-08-28 15:35 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-28 12:27 Nikolay Kasyanov
2018-08-28 13:17 ` Ævar Arnfjörð Bjarmason
2018-08-28 13:33   ` Johannes Schindelin
2018-08-28 13:46     ` Nikolay Kasyanov
2018-08-28 15:35     ` Elijah Newren [this message]
2018-08-28 16:58       ` Junio C Hamano
2018-08-29  7:06         ` [PATCH 0/3] Turn off directory rename detection in am -3 Elijah Newren
2018-08-29  7:06           ` [PATCH 1/3] t3401: add another directory rename testcase for rebase and am Elijah Newren
2018-08-29 22:12             ` Junio C Hamano
2018-08-29 23:47               ` Elijah Newren
2018-08-30 16:01                 ` Junio C Hamano
2018-08-30 16:26                   ` Elijah Newren
2018-08-29  7:06           ` [PATCH 2/3] merge-recursive: add ability to turn off directory rename detection Elijah Newren
2018-08-29 12:54             ` Johannes Schindelin
2018-08-29 23:00               ` Elijah Newren
2018-08-29  7:06           ` [PATCH 3/3] am: avoid directory rename detection when calling recursive merge machinery Elijah Newren
2018-08-29 12:51             ` Johannes Schindelin
2018-08-30 16:41         ` A rebase regression in Git 2.18.0 Elijah Newren
2018-08-31 10:11           ` Johannes Schindelin
2018-08-31 19:37             ` Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BENB=mqfFU4FGb2OS9VDV=9VdT71HhFLZwtyxD8MpdTMQ@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=corrmage@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.com \
    --subject='Re: A rebase regression in Git 2.18.0' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git