Hi,
Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>> 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).
[...]
> 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@xxxxxxxxx>
> ---
> 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
