Re: [ANNOUNCE] Git v2.20.0-rc1

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 4736 bytes --]

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
> > 0000000000000000000000000000000000000000". 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.fsf@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(&desc[nr++], &head_oid)) {
builtin/rebase.c:       if (!fill_tree_descriptor(&desc[nr++], oid)) {
sequencer.c:    if (!fill_tree_descriptor(&desc, &oid)) {

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", &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 `&head_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?