Re: rebase-in-C stability for 2.20

2018-11-13 Thread Junio C Hamano
Ævar Arnfjörð Bjarmason  writes:

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

I was hoping that having it early in GfW 2.19 would have smoked out
all the remaining issues, but it seems that those building from the
source and testing have usage patterns different enough to find more
issues.  This is a normal part of the development process, and
hopefully the remaining bugs are minor and can be flushed out in the
-rc testing period---this kind of thing is the whole reason why we
code-freeze and test rcs.

It unfortunately is too late to depend on the rebase.useBuiltin as
an escape hatch, as 'master', 'next', or anywhere else had the
"rebase and rebase -i in C" series merged without it set to false
and used in any seriousness.  Quite honestly, I think we can trust
the rest of the "rebase and rebase -i in C" series much more than
its "use the scripted one even though we have built-in one" part.

So if we have to seriously consider holding back, we may be better
off ripping the whole thing out.  I do not offhand know how involved
such a reversion would be, though, and I am *NOT* looking forward to
having do such a major surgery right before the release.


Re: rebase-in-C stability for 2.20

2018-11-13 Thread Elijah Newren
On Tue, Nov 13, 2018 at 1:52 PM Ævar Arnfjörð Bjarmason
 wrote:
> 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)?

That date feels slightly misleading; it has a commit date of Oct 11,
and only merged to master less than two weeks ago.  Given how big the
two different rebase in C series were, I'd expect a couple small
things to fall out after it hit master, which is what appears to be
happening.

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

I probably don't qualify as more familar, but giving my $0.02
anyway...  I'm happy that setting rebase.useBuiltin=false by default
exists an escape hatch if things don't settle down as we get closer to
the release, but I'd rather wait until further into the RC cycle
before going that route, personally.


Re: rebase-in-C stability for 2.20

2018-11-13 Thread Stefan Beller
> But maybe I'm being overly paranoid. What do those more familiar with
> this think?

I am not too worried,
* as rebase is a main porcelain, that is even hard to use in a script.
  so any failures are not deep down in some automation,
  but when found exposed quickly (and hopefully reported).
* 5541bd5b8f was merged to next a month ago; internally we
   distribute the next branch to Googlers (on a weekly basis)
   and we have not had any bug reports regarding rebase.
   (Maybe our environment is too strict for the wide range
of bugs reported)
* Johannes reported that the rebase is used in GfW
   
https://public-inbox.org/git/nycvar.qro.7.76.6.1808241320540...@tvgsbejvaqbjf.bet/
   https://github.com/git-for-windows/git/pull/1800
   and from my cursory reading it is part of
   2.19.windows, which has a large user base.

> (and would re-enable rebase.useBuiltin=true in
> master right after 2.20 is out the door).

That would be fine with me as well, but I'd rather
document rebase.useBuiltin instead of flip-flopping
the switch around the release.

Have there been any fixes that are only in
the C version (has the shell version already bitrotted)?

Stefan