On Wed, Nov 28 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). Since I raised this 'should we hold off?' I thought I'd chime in and say that I'm fine with going along with what you suggest and having the builtin as the default in the final. IOW not merge jc/postpone-rebase-in-c down.
