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
