# git rebase bug? 6 messages from 2010-07-07 to 2010-07-08. Participants: Mike Hommey, Björn Steinbrink, Jakub Narebski, Jeff King. Thread: https://gitlist.dev/t/24321 ## Mike Hommey, 2010-07-07 15:05 Subject: git rebase bug? Message-ID: <20100707150545.GA24814@glandium.org> URL: https://gitlist.dev/e/20100707150545.GA24814%40glandium.org ``` 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, 2010-07-07 18:00 Subject: Re: git rebase bug? Message-ID: <20100707180004.GA3165@atjola.homenet> URL: https://gitlist.dev/e/20100707180004.GA3165%40atjola.homenet In-Reply-To: <20100707150545.GA24814@glandium.org> ``` 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, 2010-07-07 20:51 Subject: Re: git rebase bug? Message-ID: <20100707205126.GA11240@glandium.org> URL: https://gitlist.dev/e/20100707205126.GA11240%40glandium.org In-Reply-To: <20100707180004.GA3165@atjola.homenet> ``` On Wed, Jul 07, 2010 at 08:00:04PM +0200, Björn Steinbrink 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... Mike ``` ## Jakub Narebski, 2010-07-07 21:44 Subject: Re: git rebase bug? Message-ID: URL: https://gitlist.dev/e/m3bpajm0gw.fsf%40localhost.localdomain In-Reply-To: <20100707205126.GA11240@glandium.org> ``` Mike Hommey writes: > On Wed, Jul 07, 2010 at 08:00:04PM +0200, Björn Steinbrink 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. -- Jakub Narebski Poland ShadeHawk on #git ``` ## Mike Hommey, 2010-07-08 09:47 Subject: Re: git rebase bug? Message-ID: <20100708094720.GB3720@glandium.org> URL: https://gitlist.dev/e/20100708094720.GB3720%40glandium.org In-Reply-To: ``` On Wed, Jul 07, 2010 at 02:44:50PM -0700, Jakub Narebski wrote: > Mike Hommey writes: > > On Wed, Jul 07, 2010 at 08:00:04PM +0200, Björn Steinbrink 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, 2010-07-08 11:37 Subject: Re: git rebase bug? Message-ID: <20100708113731.GC2294@sigill.intra.peff.net> URL: https://gitlist.dev/e/20100708113731.GC2294%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Jul 07, 2010 at 02:44:50PM -0700, Jakub Narebski wrote: > > 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 ```