Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)

> 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).