Hi Jonathan,

if you could pry more information (or better information) out of that bug
reporter, that would be nice. Apparently my email address is blacklisted
by his mail provider, so he is unlikely to have received my previous mail
(nor will he receive this one, I am sure).

Thanks,
Dscho

On Wed, 28 Nov 2018, Johannes Schindelin wrote:

> Hi Jonathan,
> 
> On Tue, 27 Nov 2018, Jonathan Nieder wrote:
> 
> > At https://bugs.debian.org/914695 is a report of a test regression in
> > an outside project that is very likely to have been triggered by the
> > new faster rebase code.
> 
> From looking through that log.gz (without having a clue where the test
> code lives, so I cannot say what it is supposed to do, and also: this is
> the first time I hear about dgit...), it would appear that this must be a
> regression in the reflog messages produced by `git rebase`.
> 
> > The issue has not been triaged, so I don't know yet whether it's a
> > problem in rebase-in-c or a manifestation of a bug in the test.
> 
> It ends thusly:
> 
> -- snip --
> [...]
> + git reflog
> + egrep 'debrebase new-upstream.*checkout'
> + test 1 = 0
> + t-report-failure
> + set +x
> TEST FAILED
> -- snap --
> 
> Which makes me think that the reflog we produce in *some* code path that
> originally called `git checkout` differs from the scripted rebase's
> generated reflog.
> 
> > That said, Google has been running with the new rebase since ~1 month
> > ago when it became the default, with no issues reported by users.  As a
> > result, I am confident that it can cope with what most users of "next"
> > throw at it, which means that if we are to find more issues to polish it
> > better, it will need all the exposure it can get.
> 
> Right. And there are a few weeks before the holidays, which should give me
> time to fix whatever bugs are discovered (I only half mind being the only
> one who fixes these bugs).
> 
> > In the Google deployment, we will keep using rebase-in-c even if it
> > gets disabled by default, in order to help with that.
> > 
> > From the Debian point of view, it's only a matter of time before
> > rebase-in-c becomes the default: even if it's not the default in 2.20,
> > it would presumably be so in 2.21 or 2.22.  That means the community's
> > attention when resolving security and reliability bugs would be on the
> > rebase-in-c implementation.  As a result, the Debian package will most
> > likely enable rebase-in-c by default even if upstream disables it, in
> > order to increase the package's shelf life (i.e. to ease the
> > maintenance burden of supporting whichever version of the package ends
> > up in the next Debian stable).
> > 
> > So with either hat on, it doesn't matter whether you apply this patch
> > upstream.
> > 
> > Having two pretty different deployments end up with the same
> > conclusion leads me to suspect that it's best for upstream not to
> > apply the revert patch, unless either
> > 
> >   (a) we have a concrete regression to address and then try again, or
> >   (b) we have a test or other plan to follow before trying again.
> 
> In this instance, I am more a fan of the "let's move fast and break
> things, then move even faster fixing them" approach.
> 
> Besides, the bug that Ævar discovered was a bug already in the scripted
> rebase, but hidden by yet another bug (the missing error checking).
> 
> I get the pretty firm impression that the common code paths are now pretty
> robust, and only lesser-exercised features may expose a bug (or
> regression, as in the case of the reflogs, where one could argue that the
> exact reflog message is not something we promise not to fiddle with).
> 
> Ciao,
> Dscho