Re: What's cooking in git.git (Jun 2011, #02; Sat, 11)

From: Jeff King <peff@peff.net>
Date: Jun 16, 2011, 21:05 UTC
Message-ID: <20110616210551.GA15505@sigill.intra.peff.net>
In-Reply-To: <BANLkTin-sAXAZ4ZJ9z77SSYx+xYTYeZMww@mail.gmail.com>

On Mon, Jun 13, 2011 at 10:27:50PM -0600, Elijah Newren wrote:

Show 19 quoted lines

> On Sun, Jun 12, 2011 at 12:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Here are the topics that have been cooking.  Commits prefixed with '-' are
> > only in 'pu' while commits prefixed with '+' are in 'next'.
> >
> > --------------------------------------------------
> > [Stalled]
> >
> > * jk/maint-merge-rename-create (2011-03-25) 3 commits
> >  - merge: turn on rewrite detection
> >  - merge: handle renames with replacement content
> >  - t3030: fix accidental success in symlink rename
> >
> > Peff wanted to reroll this. Perhaps will discard to make room for Elijah's
> > merge-recursive series in the meantime.
> 
> Peff: Do you want any help rerolling, updating, reviewing, etc.?  I
> don't want to cause any undue problems with my merge-recursive series,
> and besides, the idea you have in this series looks like it may be
> interesting.

Thanks. The sticking point in my series that there is a weird regression it introduces, and I haven't quite figured out the cause.

I'm cc'ing Jay Soffian, who found it. You can reproduce with this recipe (sorry, the chromium repo is huge, but I don't have a smaller test case yet):

  git clone http://git.chromium.org/git/chromium.git &&
  cd chromium &&
  git config merge.renameLimit 0 &&
  git checkout 0f6d00c &&
  git cherry-pick d7081a74

Looking at d7081a74, you can see that it's a tiny commit, and in fact it should cherry-pick just fine. But due to the 3-way merge that cherry-pick does, we still end up considering a lot of renames between the merge base (d7081a74^) and 0f6d00c. Which is fine and correct, of course, but makes the merge a lot more complex than it would at first appear.

With git v1.7.5.3, it works just fine. With my patches on top, it reliably fails the merge (even though there were no conflicts!), and leaves a ton of crufty temporary files in the working directory.

Bumping up the merge verbosity, it does report a bunch of rename/add conflicts. Which are totally bogus, of course. Here's what I wrote earlier to Jay:

   The final patch turned on break detection. The patch before it fixed
   the "source" case: when the source of a rename comes from something
   besides empty (because the source is a broken pair), we need to merge
   properly and not just delete. But I never did the "dest" case, in
   which merge-recursive assumes that renamed content started as empty.

   And that's why you see the bogus "rename/add" conflict, I think. It's
   assuming the file came from nowhere, but it came from a break pair.
   So the good news is I think I can come up with a simple test case,
   and the fix is probably something like comparing against the ancestor
   sha1 instead of the null sha1.

But then I got sidetracked, and as you probably know from working on merge-recursive, it's some pretty hairy code. Every time I start to look at it I get confused and can't remember what breakthrough I thought I was close to making before. :)

If you have any input, I'd appreciate it.

-Peff