rebase-in-C stability for 2.20

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Pratik Karki <predatoramigo@gmail.com>, Jeff King <peff@peff.net>,
	Stefan Beller <sbeller@google.com>
Subject: rebase-in-C stability for 2.20
Date: Tue, 13 Nov 2018 22:50:05 +0100	[thread overview]
Message-ID: <87y39w1wc2.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <1b8461d1-6cb7-6622-94d2-44c27623236d@talktalk.net>

On Tue, Nov 13 2018, Phillip Wood wrote:

> Hi Johannes
>
> On 13/11/2018 19:21, Johannes Schindelin wrote:
>> Hi Phillip,
>>
>> On Tue, 13 Nov 2018, Phillip Wood wrote:
>>
>>> Thanks for looking at this. Unfortunately using OPT_PASSTHRU_ARGV seems to
>>> break the error reporting
>>>
>>> Running
>>>    bin/wrappers/git rebase --onto @^^^^ @^^ -Cbad
>>>
>>> Gives
>>>    git encountered an error while preparing the patches to replay
>>>    these revisions:
>>>
>>>
>>> 67f673aa4a580b9e407b1ca505abf1f50510ec47...7c3e01a708856885e60bf4051586970e65dd326c
>>>
>>>    As a result, git cannot rebase them.
>>>
>
> git 2.19.1 gives
>
> First, rewinding head to replay your work on top of it...
> Applying: Ninth batch for 2.20
> error: switch `C' expects a numerical value
>
> So it has a clear message as to what the error is, this patch
> regresses that. It would be better if rebase detected the error before
> starting though.
>
>>> If I do
>>>
>>>    bin/wrappers/git rebase @^^ -Cbad
>>>
>>> I get no error, it just tells me that it does not need to rebase (which is
>>> true)
>>
>> Hmm. Isn't this the same behavior as with the scripted version?
>
> Ah you're right the script does not check if the option argument is valid.
>
>> Also: are
>> we sure that we want to allow options to come *after* the `<upstream>`
>> argument?
>
> Maybe not but the scripted version does. I'm not sure if that is a
> good idea or not.

According to Junio's calendar we're now 2 days from 2.20-rc0. We have
the js/rebase-autostash-detach-fix bug I reported sitting in "pu" still,
and then this.

I see we still have rebase.useBuiltin in the code as an escape hatch,
but it's undocumented.

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

But maybe I'm being overly paranoid. What do those more familiar with
this think?