threads / bug / 24321

git rebase bug?

Subject: git rebase bug?

## tl;dr

6 messages between Jul 7, 2010 and Jul 8, 2010.

replies: 5people: 4as markdown or json

Mike Hommey· Jul 7, 2010, 15:05 UTC · lore

Hi,

I got a really weird result from a rebase today, and I'm wondering if that's a corner case or if that could be considered a bug in rebase.

This is reproducible with the following setup: $ git clone git://git.debian.org/pkg-mozilla/xulrunner/experimental xulrunner-1.9.2 $ cd xulrunner-1.9.2 $ git remote add upstream git://git.debian.org/pkg-mozilla/upstream $ git fetch upstream xulrunner/2.0:xulrunner/2.0 $ git checkout -b test upstream/1.9.2.4 $ git cherry-pick 67e469ef725ac3f4cdf043809066c353e6843db4

The patch that is cherry picked here has the following stats:
 security/manager/ssl/public/Makefile.in            |    1 +
 security/manager/ssl/public/nsIBadCertListener.idl |  155 ++++++++++++++++++++
 security/manager/ssl/src/nsNSSIOLayer.cpp          |  103 +++++++++++++-
 security/manager/ssl/src/nsNSSIOLayer.h            |    8 +

$ git rebase --onto xulrunner/2.0 upstream/1.9.2.4 test

The resulting commit has the following stats:
 security/manager/ssl/public/Makefile.in        |    1 +
 security/manager/ssl/src/nsNSSIOLayer.cpp      |  103 ++++++++++++++++-
 security/manager/ssl/src/nsNSSIOLayer.h        |    8 ++
 xulrunner/examples/simple/content/contents.rdf |  155 ++++++++++++++++++++++++

See how the security/manager/ssl/public/nsIBadCertListener.idl file that was created by the original patch is created as xulrunner/examples/simple/content/contents.rdf.

Please note that as the xulrunner/2.0 commit has no parent, I also tried grafting it on top of upstream/1.9.2.4, which didn't change anything.

So, corner case or definite bug?

Cheers,

Mike

PS: this all is with current master

Björn Steinbrink· Jul 7, 2010, 18:00 UTC · re: Mike Hommey · lore

Re: git rebase bug?

On 2010.07.07 17:05:45 +0200, Mike Hommey wrote:

> See how the security/manager/ssl/public/nsIBadCertListener.idl file that
> was created by the original patch is created as
> xulrunner/examples/simple/content/contents.rdf.

The "problem" is that nsIBadCertListener.idl wasn't actually created by the cherry-picked commit, but was modified. It was an empty file before, created in 4292283190983fa91b875e22664a79a3aa9ea45d.

And as nsIBadCertListener.idl is missing from the xulrunner/2.0 branch, git does the usual rename detection, finding another empty file and ends up patching that one instead.

Björn

Mike Hommey· Jul 7, 2010, 20:51 UTC · re: Björn Steinbrink · lore

Re: git rebase bug?

On Wed, Jul 07, 2010 at 08:00:04PM +0200, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:

Show 12 quoted lines

> On 2010.07.07 17:05:45 +0200, Mike Hommey wrote:
> > See how the security/manager/ssl/public/nsIBadCertListener.idl file that
> > was created by the original patch is created as
> > xulrunner/examples/simple/content/contents.rdf.
> 
> The "problem" is that nsIBadCertListener.idl wasn't actually created by
> the cherry-picked commit, but was modified. It was an empty file before,
> created in 4292283190983fa91b875e22664a79a3aa9ea45d.
> 
> And as nsIBadCertListener.idl is missing from the xulrunner/2.0 branch,
> git does the usual rename detection, finding another empty file and ends
> up patching that one instead.

Oh, makes sense. Thanks. So that's a quite troubling corner case... I wonder if empty files shouldn't be special cased...

Mike

Jakub Narebski· Jul 7, 2010, 21:44 UTC · re: Mike Hommey · lore

Re: git rebase bug?

Mike Hommey <mh@glandium.org> writes:

> On Wed, Jul 07, 2010 at 08:00:04PM +0200, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> > On 2010.07.07 17:05:45 +0200, Mike Hommey wrote:

Show 14 quoted lines

> > > See how the security/manager/ssl/public/nsIBadCertListener.idl file that
> > > was created by the original patch is created as
> > > xulrunner/examples/simple/content/contents.rdf.
> > 
> > The "problem" is that nsIBadCertListener.idl wasn't actually created by
> > the cherry-picked commit, but was modified. It was an empty file before,
> > created in 4292283190983fa91b875e22664a79a3aa9ea45d.
> > 
> > And as nsIBadCertListener.idl is missing from the xulrunner/2.0 branch,
> > git does the usual rename detection, finding another empty file and ends
> > up patching that one instead.
> 
> Oh, makes sense. Thanks. So that's a quite troubling corner case...
> I wonder if empty files shouldn't be special cased...

Well, similarity score (of contents and of filename) is weighted by contents length, but perhaps empty files / zero length somehow fall out as an edge case...

I agree that empty files should be special cased... unless filename is _very_ similar.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Mike Hommey· Jul 8, 2010, 09:47 UTC · re: Jakub Narebski · lore

Re: git rebase bug?

On Wed, Jul 07, 2010 at 02:44:50PM -0700, Jakub Narebski wrote:

Show 25 quoted lines

> Mike Hommey <mh@glandium.org> writes:
> > On Wed, Jul 07, 2010 at 08:00:04PM +0200, Björn Steinbrink <B.Steinbrink@gmx.de> wrote:
> > > On 2010.07.07 17:05:45 +0200, Mike Hommey wrote:
> 
> > > > See how the security/manager/ssl/public/nsIBadCertListener.idl file that
> > > > was created by the original patch is created as
> > > > xulrunner/examples/simple/content/contents.rdf.
> > > 
> > > The "problem" is that nsIBadCertListener.idl wasn't actually created by
> > > the cherry-picked commit, but was modified. It was an empty file before,
> > > created in 4292283190983fa91b875e22664a79a3aa9ea45d.
> > > 
> > > And as nsIBadCertListener.idl is missing from the xulrunner/2.0 branch,
> > > git does the usual rename detection, finding another empty file and ends
> > > up patching that one instead.
> > 
> > Oh, makes sense. Thanks. So that's a quite troubling corner case...
> > I wonder if empty files shouldn't be special cased...
> 
> Well, similarity score (of contents and of filename) is weighted by
> contents length, but perhaps empty files / zero length somehow fall
> out as an edge case...
> 
> I agree that empty files should be special cased... unless filename is
> _very_ similar.

Question is, in a case like mine, what should the result be?

Mike

Jeff King· Jul 8, 2010, 11:37 UTC · re: Jakub Narebski · lore

Re: git rebase bug?

On Wed, Jul 07, 2010 at 02:44:50PM -0700, Jakub Narebski wrote:

Show 6 quoted lines

> > Oh, makes sense. Thanks. So that's a quite troubling corner case...
> > I wonder if empty files shouldn't be special cased...
> 
> Well, similarity score (of contents and of filename) is weighted by
> contents length, but perhaps empty files / zero length somehow fall
> out as an edge case...

Exact rename detection is handled before inexact detection, so the contents length are irrelevant. So it is not about empty files, but about exact matches. I'm not sure if the basename-matching code is used for exact matches, though. But ideally we would break exact-match ties based on filename. I'd have to read through the code and/or perform some tests to be sure, though.

-Peff