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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 28, 2018, 04:31 UTC
Message-ID: <20181128043154.GA34163@google.com>
In-Reply-To: <xmqq36roz7ve.fsf_-_@gitster-ct.c.googlers.com>

Hi,

Junio C Hamano wrote:

>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 8 quoted lines

>>> Given that we're still finding regressions bugs in the rebase-in-C
>>> version should we be considering reverting 5541bd5b8f ("rebase: default
>>> to using the builtin rebase", 2018-08-08)?
>>>
>>> I love the feature, but fear that the current list of known regressions
>>> serve as a canary for a larger list which we'd discover if we held off
>>> for another major release (and would re-enable rebase.useBuiltin=true in
>>> master right after 2.20 is out the door).

[...]

Show 16 quoted lines

> So, in a more concrete form, what you want to see is something like
> this in -rc2 and later?
>
> -- >8 --
> Subject: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature
>
> It turns out to be a bit too early to unleash the reimplementation
> to the general public.  Let's rewrite some documentation and make it
> an opt-in feature.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/config/rebase.txt | 16 ++++++----------
>  builtin/rebase.c                |  2 +-
>  t/README                        |  4 ++--
>  3 files changed, 9 insertions(+), 13 deletions(-)

I thought I should weigh in on how this would affect Debian's and Google's deployments.

First of all, I've looked over the revert patch carefully and it is well written and does what it says on the tin.

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

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.

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.

Thanks and hope that helps, Jonathan