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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

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