| Message ID | xmqq36roz7ve.fsf_-_@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1) | expand |
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
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
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.
Æ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.
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
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.
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. >
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.
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. >
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.
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