Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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.
--
Ian JacksonThese 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.
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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 JacksonThese 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.
>
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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.
--
Ian JacksonThese 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.
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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 JacksonThese 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.
>
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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.
--
Ian JacksonThese 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.
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Ævar Arnfjörð Bjarmason 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.
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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.
Re: [ANNOUNCE] Git v2.20.0-rc1
Hi Junio, On Wed, 28 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > ... > > In short, even a thorough study of the code (keeping in mind the few > > tidbits of information provided by you) leaves me really wondering which > > code you run, because it sure does not look like current `master` to me. > > > > And if it is not `master`, then I have to ask why you keep suggesting to > > turn off the built-in rebase by default in `master`. > > > > Ciao, > > Johannes > > > > P.S.: Maybe you have a hook you forgot to mention? > > Any response? Or can I retract jc/postpone-rebase-in-c that was > prepared as a reaction to this? I worked with Ævar via IRC and we figured out the root cause and I submitted https://public-inbox.org/git/pull.88.git.gitgitgad...@gmail.com/ to fix it. Given that this is a really obscure edge case (`git rebase --stat -v -i` onto an unrelated commit history, if you take away one of these, the bug does not trigger), and that it was only discovered to be a bug *because* of the built-in rebase (the scripted version had the same bug, but simply forgot to do proper error checking), I would not think that the reported bug is a strong argument in favor of turning off the built-in rebase by defauly. In other words, after understanding the bug I am even more confident than before that the built-in rebase is actually in a pretty good shape. I do not expect any major regressions, and if any happen: we do have that escape hatch for corner cases while I fix those bugs. Ciao, Dscho
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
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
Re: [PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Hi,
Junio C Hamano wrote:
>> Ævar Arnfjörð Bjarmason 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
> ---
> 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
Re: [ANNOUNCE] Git v2.20.0-rc1
Johannes Schindelin writes: > ... > In short, even a thorough study of the code (keeping in mind the few > tidbits of information provided by you) leaves me really wondering which > code you run, because it sure does not look like current `master` to me. > > And if it is not `master`, then I have to ask why you keep suggesting to > turn off the built-in rebase by default in `master`. > > Ciao, > Johannes > > P.S.: Maybe you have a hook you forgot to mention? Any response? Or can I retract jc/postpone-rebase-in-c that was prepared as a reaction to this?
Re: [ANNOUNCE] Git v2.20.0-rc1
Elijah Newren writes: > If we don't set rebase.useBuiltin to false, then there is also a minor > regression in the error message printed by the built-in rebase we may > want to try to address. I have a patch for it at > <20181122044841.20993-2-new...@gmail.com>, but I don't know if that > patch is acceptable as-is this close to a release since that'd not > give translators much time to update. For this particular one, I'd rather ship "rebase in C" with known message glitch, with or without the "mark it experimental and make it opt-in" last-time change. Of course, turning it off by default would let us worry about these message glitches even less, but at this point, I'd be worried more about bugs that can affect the actual operation and outcome recorded in the resulting history.
Re: [ANNOUNCE] Git v2.20.0-rc1
Hi Ævar,
On Mon, 26 Nov 2018, Johannes Schindelin wrote:
> On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
> > On Wed, Nov 21 2018, Junio C Hamano wrote:
> >
> > > * "git rebase" and "git rebase -i" have been reimplemented in C.
> >
> > Here's another regression in the C version (and rc1), note: the
> > sha1collisiondetection is just a stand in for "some repo":
> >
> > (
> > rm -rf /tmp/repo &&
> > git init /tmp/repo &&
> > cd /tmp/repo &&
> > for c in 1 2
> > do
> > touch $c &&
> > git add $c &&
> > git commit -m"add $c"
> > done &&
> > git remote add origin
> > https://github.com/cr-marcstevens/sha1collisiondetection.git &&
> > git fetch &&
> > git branch --set-upstream-to origin/master &&
> > git rebase -i
> > )
> >
> > The C version will die with "fatal: unable to read tree
> > ". Running this with
> > rebase.useBuiltin=false does the right thing and rebases as of the merge
> > base of the two (which here is the root of the history).
>
> Sorry, this bug does not reproduce here:
>
> $ git rebase -i
> Successfully rebased and updated refs/heads/master.
>
> > 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@evledraar.gmail.com/ ]
>
> Maybe you can give me the full details so that I can verify that this is
> indeed a bug in the builtin C and not just a regression caused by some
> random branches being merged together?
>
> In short: please provide me with the exact URL and branch of your git.git
> fork to test. Then please make sure to specify the precise revision of the
> sha1collisiondetection/master rev, just in case that it matters.
>
> Ideally, you would reduce the problem to a proper test case, say, for
> t3412 (it seems that you try to rebase onto an unrelated history, so it is
> *vaguely* related to "rebase-root").
So I was getting spooked enough by your half-complete bug report that I
did more digging (it is really quite a bit frustrating to have so little
hard evidence to go on, a wild goose chase is definitely not what I was
looking forward to after a day of fighting other fires, but you know,
built-in rebase is dear to me).
The error message you copied clearly comes from tree-walk.c, from
`fill_tree_descriptor()` (the other "unable to read tree" messages enclose
the hash in parentheses).
There are exactly 3 calls to said function in the built-in rebase/rebase
-i in the current `master`, a1598010f775 (Merge branch
'nd/per-worktree-ref-iteration', 2018-11-26):
$ git grep fill_tree_descriptor -- builtin/rebase*.c sequencer.[ch]
rebase-interactive.[ch]
builtin/rebase.c: if (!reset_hard && !fill_tree_descriptor([nr++],
_oid)) {
builtin/rebase.c: if (!fill_tree_descriptor([nr++], oid)) {
sequencer.c:if (!fill_tree_descriptor(, )) {
The last one of these is in `do_reset()`, i.e. handling a `reset` command
which you did not ask for, as you passed `-i` to `git rebase`, not `-ir`.
The first two *both* are in `reset_head()`. The first of them uses
`head_oid`, which is read directly via `get_oid("HEAD", _oid)`, so if
this is all zeroes for you, then it's not rebase's fault.
The second one uses the parameter `oid` passed into `reset_head()`. The
only calls to that function that do not pass `NULL` as `oid` (which would
trigger `oid` to be replaced by `_oid`, i.e again not all zeroes
unless your setup is broken) are:
- in the `--abort` code path
- in the `--autostash` code path
- in the fast-forwarding code path
- just after the "First, rewinding head" message in the *non*-interactive
rebase
None of these apply to your script snippet.
Under the assumption that you might have forgotten to talk about
rebase.autostash=true and some dirty file, I tried to augment the script
snippet accordingly, but the built-in rebase as of current `master` still
works for me, plus: reading the autostash code path, it is hard to imagine
that the `lookup_commit_reference()` would return a pointer to a commit
object whose oid is all zeroes.
In short, even a thorough study of the code (keeping in mind the few
tidbits of information provided by you) leaves me really wondering which
code you run, because it sure does not look like current `master` to me.
And if it is not `master`, then I have to ask why you keep suggesting to
turn off the built-in rebase by default in `master`.
Ciao,
Johannes
P.S.: Maybe you have a hook you forgot to mention?
Re: [ANNOUNCE] Git v2.20.0-rc1
Hi Ævar, On Sat, 24 Nov 2018, Ævar Arnfjörð Bjarmason wrote: > On Wed, Nov 21 2018, Junio C Hamano wrote: > > > * "git rebase" and "git rebase -i" have been reimplemented in C. > > Here's another regression in the C version (and rc1), note: the > sha1collisiondetection is just a stand in for "some repo": > > ( > rm -rf /tmp/repo && > git init /tmp/repo && > cd /tmp/repo && > for c in 1 2 > do > touch $c && > git add $c && > git commit -m"add $c" > done && > git remote add origin > https://github.com/cr-marcstevens/sha1collisiondetection.git && > git fetch && > git branch --set-upstream-to origin/master && > git rebase -i > ) > > The C version will die with "fatal: unable to read tree > ". Running this with > rebase.useBuiltin=false does the right thing and rebases as of the merge > base of the two (which here is the root of the history). Sorry, this bug does not reproduce here: $ git rebase -i Successfully rebased and updated refs/heads/master. > 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@evledraar.gmail.com/ ] Maybe you can give me the full details so that I can verify that this is indeed a bug in the builtin C and not just a regression caused by some random branches being merged together? In short: please provide me with the exact URL and branch of your git.git fork to test. Then please make sure to specify the precise revision of the sha1collisiondetection/master rev, just in case that it matters. Ideally, you would reduce the problem to a proper test case, say, for t3412 (it seems that you try to rebase onto an unrelated history, so it is *vaguely* related to "rebase-root"). Ciao, Dscho
Re: [ANNOUNCE] Git v2.20.0-rc1
On Sun, Nov 25, 2018 at 11:37 PM Junio C Hamano wrote: > > Unless I hear otherwise in the next 24 hours, I am planning to > merge the following topics to 'master' before cutting -rc2. Please > stop me on any of these topics. > > - jc/postpone-rebase-in-c > >This may be the most controversial. It demotes the C >reimplementation of "git rebase" to an experimental opt-in >feature that can only be enabled by setting rebase.useBuiltIn >configuration that defaults to false. > >cf. If we don't set rebase.useBuiltin to false, then there is also a minor regression in the error message printed by the built-in rebase we may want to try to address. I have a patch for it at <20181122044841.20993-2-new...@gmail.com>, but I don't know if that patch is acceptable as-is this close to a release since that'd not give translators much time to update.
Re: [ANNOUNCE] Git v2.20.0-rc1
Unless I hear otherwise in the next 24 hours, I am planning to merge the following topics to 'master' before cutting -rc2. Please stop me on any of these topics. - jc/postpone-rebase-in-c This may be the most controversial. It demotes the C reimplementation of "git rebase" to an experimental opt-in feature that can only be enabled by setting rebase.useBuiltIn configuration that defaults to false. cf. - ab/format-patch-rangediff-not-stat The "--rangediff" option recently added to "format-patch" interspersed a bogus and useless "--stat" information by mistake, which is being corrected. cf. The following three topics I do not need help in deciding; they are all good and will be merged before -rc2. - jk/t5562-perl-path-fix This is to invoke a perl scriptlet with "$PERL_PATH" in one of the new tests, instead of relying on (an incorrect) she-bang line in the script file. - tb/clone-case-smashing-warning-test This enables the test to see the behaviour of "git clone" after cloning a project that has paths that are different only in case on MINGW (earlier it wanted CASE_INSENSITIVE_FS prerequisite and in addition not on MINGW). - nd/per-worktree-ref-iteration This fixes a "return function_that_returns_void(...)" in a function that returns void. Thanks.
[PATCH] rebase: mark the C reimplementation as an experimental opt-in feature (was Re: [ANNOUNCE] Git v2.20.0-rc1)
Junio C Hamano writes:
> Ævar Arnfjörð Bjarmason 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@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
---
Documentation/config/rebase.txt | 16 ++--
builtin/rebase.c| 2 +-
t/README| 4 ++--
3 files changed, 9 insertions(+), 13 deletions(-)
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(, , 6)) {
strbuf_release();
- return 1;
+ return 0;
}
strbuf_trim();
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= exercises the preload-index code path
by overriding the minimum number of cache entries required per thread.
-GIT_TEST_REBASE_USE_BUILTIN=, when false, disables the
-builtin version of git-rebase. See 'rebase.useBuiltin' in
+GIT_TEST_REBASE_USE_BUILTIN=, 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= enables exercising the multi-threaded loading
--
2.20.0-rc1
Re: [ANNOUNCE] Git v2.20.0-rc1
Ævar Arnfjörð Bjarmason 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@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).
diff --git b/builtin/rebase.c a/builtin/rebase.c
index 9dc8475cd3..60e357c735 100644
--- b/builtin/rebase.c
+++ a/builtin/rebase.c
@@ -54,7 +54,7 @@ static int use_builtin_rebase(void)
cp.git_cmd = 1;
if (capture_command(, , 6)) {
strbuf_release();
- return 1;
+ return 0;
}
strbuf_trim();
Re: [ANNOUNCE] Git v2.20.0-rc1
On Wed, Nov 21 2018, Junio C Hamano wrote: > * "git rebase" and "git rebase -i" have been reimplemented in C. Here's another regression in the C version (and rc1), note: the sha1collisiondetection is just a stand in for "some repo": ( rm -rf /tmp/repo && git init /tmp/repo && cd /tmp/repo && for c in 1 2 do touch $c && git add $c && git commit -m"add $c" done && git remote add origin https://github.com/cr-marcstevens/sha1collisiondetection.git && git fetch && git branch --set-upstream-to origin/master && git rebase -i ) The C version will die with "fatal: unable to read tree ". Running this with rebase.useBuiltin=false does the right thing and rebases as of the merge base of the two (which here is the root of the history). 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@evledraar.gmail.com/ ]
