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

Message ID xmqq36roz7ve.fsf_-_@gitster-ct.c.googlers.com
State New
Headers show
Series
  • rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Related show

Commit Message

Junio C Hamano Nov. 26, 2018, 6:10 a.m. UTC
Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>  * "git rebase" and "git rebase -i" have been reimplemented in C.
>>
>> Here's another regression in the C version (and rc1),...
>> I wasn't trying to stress test rebase. I was just wanting to rebase a
>> history I was about to force-push after cleaning it up, hardly an
>> obscure use-case. So [repeat last transmission in
>> https://public-inbox.org/git/87y39w1wc2.fsf@evledraar.gmail.com/ ]
>
> which, to those who are reading from sidelines:
>
>     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).
>
> I am fine with the proposed flip, but I'll have to see the extent of
> damage this late in the game so that I won't miss anything.  In
> addition to the one-liner below, we'd need to update the quoted
> release notes entry, and possibly adjust some tests (even though the
> "reimplementation" ought to be bug-to-bug compatible, it may not be).

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

Comments

Jonathan Nieder Nov. 28, 2018, 4:31 a.m. UTC | #1
Hi,

Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@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
Johannes Schindelin Nov. 28, 2018, 9:23 a.m. UTC | #2
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
Ævar Arnfjörð Bjarmason Nov. 28, 2018, 12:21 p.m. UTC | #3
On Wed, Nov 28 2018, Johannes Schindelin wrote:

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

Since I raised this 'should we hold off?' I thought I'd chime in and say
that I'm fine with going along with what you suggest and having the
builtin as the default in the final. IOW not merge
jc/postpone-rebase-in-c down.
Junio C Hamano Nov. 29, 2018, 4:58 a.m. UTC | #4
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Since I raised this 'should we hold off?' I thought I'd chime in and say
> that I'm fine with going along with what you suggest and having the
> builtin as the default in the final. IOW not merge
> jc/postpone-rebase-in-c down.

OK.
Johannes Schindelin Nov. 29, 2018, 2:17 p.m. UTC | #5
Hi Jonathan,

if you could pry more information (or better information) out of that bug
reporter, that would be nice. Apparently my email address is blacklisted
by his mail provider, so he is unlikely to have received my previous mail
(nor will he receive this one, I am sure).

Thanks,
Dscho

On Wed, 28 Nov 2018, Johannes Schindelin wrote:

> 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
Ian Jackson Nov. 29, 2018, 2:30 p.m. UTC | #6
Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> if you could pry more information (or better information) out of that bug
> reporter, that would be nice. Apparently my email address is blacklisted
> by his mail provider, so he is unlikely to have received my previous mail
> (nor will he receive this one, I am sure).

(I did receive this mail.  Sorry for the inconvenience, which sadly is
inevitable occasionally in the modern email world.  FTR in future feel
free to send the bounce to postmaster@chiark and I will make a
you-shaped hole in my spamfilter.  Also with Debian bugs you can
launder your messages by, eg, emailing 914695-submitter@bugs.)

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

As I wrote in the bug report last night:

 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914695#15

 I have investigated and the bug seems to be that git-rebase --onto now
 fails to honour GIT_REFLOG_ACTION for the initial checkout.

 In a successful run with older git I get a reflog like this:

   4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
   4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
   cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
   0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
   29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
   85e0c46 HEAD@{5}: debrebase: launder for new upstream

 With a newer git I get this:

   6d3fb91 HEAD@{0}: rebase finished: returning to refs/heads/master
   6d3fb91 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
   86c0721 HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
   50ba56c HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
   8272825 HEAD@{4}: rebase: checkout 8272825bb4ff6eba89afa936e32b6460f963a36a
   c78db55 HEAD@{5}: debrebase: launder for new upstream

 This breaks the test because my test suite is checking that I set
 GIT_REFLOG_ACTION appropriately.

 If you want I can provide a minimal test case but this should suffice
 to see the bug I hope...

Regards
Ian.
Johannes Schindelin Nov. 29, 2018, 3:39 p.m. UTC | #7
Hi Ian,

On Thu, 29 Nov 2018, Ian Jackson wrote:

> Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> > if you could pry more information (or better information) out of that bug
> > reporter, that would be nice. Apparently my email address is blacklisted
> > by his mail provider, so he is unlikely to have received my previous mail
> > (nor will he receive this one, I am sure).
> 
> (I did receive this mail.  Sorry for the inconvenience, which sadly is
> inevitable occasionally in the modern email world.  FTR in future feel
> free to send the bounce to postmaster@chiark and I will make a
> you-shaped hole in my spamfilter.  Also with Debian bugs you can
> launder your messages by, eg, emailing 914695-submitter@bugs.)

Right. I myself have plenty of email-related problems that seem to crop up
this year in particular.

> > > > 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.
> 
> As I wrote in the bug report last night:
> 
>  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914695#15
> 
>  I have investigated and the bug seems to be that git-rebase --onto now
>  fails to honour GIT_REFLOG_ACTION for the initial checkout.
> 
>  In a successful run with older git I get a reflog like this:
> 
>    4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
>    4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
>    cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
>    0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
>    29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
>    85e0c46 HEAD@{5}: debrebase: launder for new upstream
> 
>  With a newer git I get this:
> 
>    6d3fb91 HEAD@{0}: rebase finished: returning to refs/heads/master
>    6d3fb91 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
>    86c0721 HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
>    50ba56c HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
>    8272825 HEAD@{4}: rebase: checkout 8272825bb4ff6eba89afa936e32b6460f963a36a
>    c78db55 HEAD@{5}: debrebase: launder for new upstream
> 
>  This breaks the test because my test suite is checking that I set
>  GIT_REFLOG_ACTION appropriately.
> 
>  If you want I can provide a minimal test case but this should suffice
>  to see the bug I hope...

This should be plenty for me to get going. Thank you!

Ciao,
Johannes

> 
> Regards
> Ian.
> 
> -- 
> Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.
> 
> If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
> a private address which bypasses my fierce spamfilter.
>
Ian Jackson Nov. 29, 2018, 3:50 p.m. UTC | #8
Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> >  In a successful run with older git I get a reflog like this:
> > 
> >    4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
> >    4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
> >    cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
> >    0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
> >    29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
> >    85e0c46 HEAD@{5}: debrebase: launder for new upstream
...
> >  This breaks the test because my test suite is checking that I set
> >  GIT_REFLOG_ACTION appropriately.
> > 
> >  If you want I can provide a minimal test case but this should suffice
> >  to see the bug I hope...
> 
> This should be plenty for me to get going. Thank you!

Happy hunting.

While you're looking at this, I observe that the fact that the `rebase
finished' message also does not honour GIT_REFLOG_ACTION appears to be
a pre-existing bug.

(In general one often can't rely on GIT_REFLOG_ACTION still being set
because the rebase might have been interrupted and restarted, which I
think is why my test case looks for it in the initial `checkout'
message.)

Regards,
Ian.
Johannes Schindelin Nov. 29, 2018, 4:14 p.m. UTC | #9
Hi Ian,

On Thu, 29 Nov 2018, Ian Jackson wrote:

> Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> > >  In a successful run with older git I get a reflog like this:
> > > 
> > >    4833d74 HEAD@{0}: rebase finished: returning to refs/heads/with-preexisting
> > >    4833d74 HEAD@{1}: debrebase new-upstream 2.1-1: rebase: Add another new upstream file
> > >    cabd5ec HEAD@{2}: debrebase new-upstream 2.1-1: rebase: Edit the .c file
> > >    0b362ce HEAD@{3}: debrebase new-upstream 2.1-1: rebase: Add a new upstream file
> > >    29653e5 HEAD@{4}: debrebase new-upstream 2.1-1: rebase: checkout 29653e5a17bee4ac23a68bba3e12bc1f52858ac3
> > >    85e0c46 HEAD@{5}: debrebase: launder for new upstream
> ...
> > >  This breaks the test because my test suite is checking that I set
> > >  GIT_REFLOG_ACTION appropriately.
> > > 
> > >  If you want I can provide a minimal test case but this should suffice
> > >  to see the bug I hope...
> > 
> > This should be plenty for me to get going. Thank you!
> 
> Happy hunting.

I'll have to take a (lengthy) dinner break now, but this is what I have so
far: a regression test that verifies the breakage (see the
`fix-reflog-action` branch at https://github.com/dscho/git). I'll continue
after dinner and am confident that this bug will be fixed within the next
four hours.

> While you're looking at this, I observe that the fact that the `rebase
> finished' message also does not honour GIT_REFLOG_ACTION appears to be
> a pre-existing bug.

I noticed that, too, but at this point I am only fixing regressions. We
can try to fix this long-standing bug in the v2.20 cycle.

Ciao,
Johannes

> (In general one often can't rely on GIT_REFLOG_ACTION still being set
> because the rebase might have been interrupted and restarted, which I
> think is why my test case looks for it in the initial `checkout'
> message.)
> 
> Regards,
> Ian.
> 
> -- 
> Ian Jackson <ijackson@chiark.greenend.org.uk>   These opinions are my own.
> 
> If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
> a private address which bypasses my fierce spamfilter.
>
Ian Jackson Nov. 29, 2018, 4:26 p.m. UTC | #10
Johannes Schindelin writes ("Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)"):
> I'll have to take a (lengthy) dinner break now, but this is what I have so
> far: a regression test that verifies the breakage (see the
> `fix-reflog-action` branch at https://github.com/dscho/git). I'll continue
> after dinner and am confident that this bug will be fixed within the next
> four hours.

That seems super speedy to me!

When you have a fix I will leave it up to the Debian git maintainers
to decide whether they want to cherry pick your fix into their
package, or await an updated upstream branch with rc, or what.

> [Ian:]
> > While you're looking at this, I observe that the fact that the `rebase
> > finished' message also does not honour GIT_REFLOG_ACTION appears to be
> > a pre-existing bug.
> 
> I noticed that, too, but at this point I am only fixing regressions. We
> can try to fix this long-standing bug in the v2.20 cycle.

Right.

Thanks,
Ian.

Patch
diff mbox series

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f079bf6b7e..af12623151 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -1,16 +1,12 @@ 
 rebase.useBuiltin::
-	Set to `false` to use the legacy shellscript implementation of
-	linkgit:git-rebase[1]. Is `true` by default, which means use
-	the built-in rewrite of it in C.
+	Set to `true` to use the experimental reimplementation of
+	linkgit:git-rebase[1] in C.  Defaults to `false`.
 +
 The C rewrite is first included with Git version 2.20. This option
-serves an an escape hatch to re-enable the legacy version in case any
-bugs are found in the rewrite. This option and the shellscript version
-of linkgit:git-rebase[1] will be removed in some future release.
-+
-If you find some reason to set this option to `false` other than
-one-off testing you should report the behavior difference as a bug in
-git.
+allows early adopters to opt into the experimental version to find
+bugs in the rewritten version. This option and the shellscript version
+of linkgit:git-rebase[1] will be removed in some future release once
+the reimplementation becomes more stable.
 
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b3e5baec8..19ad97b177 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -59,7 +59,7 @@  static int use_builtin_rebase(void)
 	cp.git_cmd = 1;
 	if (capture_command(&cp, &out, 6)) {
 		strbuf_release(&out);
-		return 1;
+		return 0;
 	}
 
 	strbuf_trim(&out);
diff --git a/t/README b/t/README
index 28711cc508..7e925e5fea 100644
--- a/t/README
+++ b/t/README
@@ -345,8 +345,8 @@  for the index version specified.  Can be set to any valid version
 GIT_TEST_PRELOAD_INDEX=<boolean> exercises the preload-index code path
 by overriding the minimum number of cache entries required per thread.
 
-GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when false, disables the
-builtin version of git-rebase. See 'rebase.useBuiltin' in
+GIT_TEST_REBASE_USE_BUILTIN=<boolean>, when true, forces the use of
+builtin version of git-rebase in the test. See 'rebase.useBuiltin' in
 git-config(1).
 
 GIT_TEST_INDEX_THREADS=<n> enables exercising the multi-threaded loading