BUG: fetch in certain repo always gives "did not send all necessary objects"

BUG: fetch in certain repo always gives "did not send all necessary objects"

[Elijah Newren]

[I cannot share the local repo and had to modify output of commands
slightly to redact hostnames, branch names, etc.  I think I haven't
messed anything up, but it's possible...]

Two people in the last week have come to me after running into a case
where they could not update their repo because any call to git fetch
or git pull would error out with

  fatal: bad object HEAD
  error: gerrit.server:reponame did not send all necessary objects"

which apparently has been reported before at both of:

https://public-inbox.org/git/CAE5ih78nLL6UhKPObvFEA9xQZUtc1XpPvGJNaYTH9fJ0RyFRvA@mail.gmail.com/
and

https://public-inbox.org/git/CAGZ79kYP0z1G_H3nwfmSHraWHMBOcik5LepUXKj0nveeBrihiw@mail.gmail.com/

but without a resolution and it appears to have been difficult to
reproduce.  I can reproduce reliably with a copy of one of their
repos, and have dug in a little bit and may be able to provide more
info.  The pieces I've found that look relevant:

* Both users were using git-2.16.1 (suggesting they update and try new
things often)
* "git fsck --no-dangling" reports no problem on either client or server.
* The error bisects to commit d0c39a49cc ("revision.c: --all adds HEAD
from all worktrees", 2017-08-23).
* The developer has 3 directories in use corresponding to different
worktrees, but four entries under .git/worktrees, one of which appears
that it was once in use but no longer is
* The unused entry under .git/worktrees has a detached HEAD that
points to an object that doesn't exist; I suspect it was garbage
collected.

More details about the last two items:

$ ls ..
master/  1.0-release/  2.0-release/  3.0-release/

$ for i in ../*/.git; do echo $i $(cat $i/.git 2>&1); done
../master/.git cat: ../master/.git: Is a directory
../1.0-release/.git gitdir: /clone/master/.git/worktrees/1.0-release
../2.0-release/.git gitdir: /clone/master/.git/worktrees/2.0-release
../3.0-release/.git gitdir: /clone/master/.git/worktrees/3.0-release1

$ for i in .git/worktrees/*/gitdir; do echo $i $(cat $i); done
.git/worktrees/1.0-release/gitdir /clone/1.0-release/.git
.git/worktrees/2.0-release/gitdir /clone/2.0-release/.git
.git/worktrees/3.0-release1/gitdir /clone/3.0-release/.git
.git/worktrees/3.0-release/gitdir /clone/3.0-release/.git

$ (cd .git && for i in worktrees/*/HEAD; do echo $i $(git rev-parse
$i^{object}); done)
worktrees/1.0-release/HEAD c483c6806fe19c2fa9bcda892e0ddb470f8e9d65
worktrees/2.0-release/HEAD ab4c699ec9d1eab143c7e8ef51a0a6c451fcd4ea
worktrees/3.0-release1/HEAD 55e52e24b24b2f81484de9c6ea7d4520238c5fd5
fatal: ambiguous argument 'worktrees/3.0-release/HEAD^{object}':
unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
worktrees/3.0-release/HEAD worktrees/3.0-release/HEAD^{object}


So, it appears both worktrees/3.0-release and worktrees/3.0-release1
believe they are used by /clone/3.0-release/, but /clone/3.0-release
uses worktree-3.0-release1.  That leaves worktrees/3.0-release unused,
and in fact its HEAD is some sha1 that does not exist in the repo --
possibly GC'ed.


Also, after the investigation above and based on the hint in one of
the other email threads linked above, we tried 'git worktree prune'
but it didn't remove the unused directory under .git/worktree (perhaps
because it thought something was using it?).  The fetch bug didn't go
away until doing an 'rm -rf .git/worktree/3.0-release'.

I have no experience prior to today with git worktree, and the
developer in question doesn't remember how things got to the current
state.  It was all a while ago.


Does anyone have an idea what may have happened here or how to avoid it?

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Stefan Beller]

On Tue, Feb 6, 2018 at 3:04 PM, Elijah Newren <newren@gmail.com> wrote:

>
> Does anyone have an idea what may have happened here or how to avoid it?

According to Peff this got fixed
https://public-inbox.org/git/20171020031630.44zvzh3d2vlhglv4@sigill.intra.peff.net/
and but you've had a corrupted repo from back when you were using an older
version of Git.

Did that repo exist before d0c39a49cc was rolled out? Then we can keep that
hypothesis of "left-over corruption" as Peff put it.

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Elijah Newren]

On Tue, Feb 6, 2018 at 3:20 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Feb 6, 2018 at 3:04 PM, Elijah Newren <newren@gmail.com> wrote:
>
>>
>> Does anyone have an idea what may have happened here or how to avoid it?
>
> According to Peff this got fixed
> https://public-inbox.org/git/20171020031630.44zvzh3d2vlhglv4@sigill.intra.peff.net/
> and but you've had a corrupted repo from back when you were using an older
> version of Git.
>
> Did that repo exist before d0c39a49cc was rolled out? Then we can keep that
> hypothesis of "left-over corruption" as Peff put it.

I'm somewhat confused by this explanation.  That precise commit is the
one I bisected to that _caused_ the fetch to fail.  Also, there might
be one important difference here -- in the link you provide, it
suggests that you had a corrupted working directory that made use of a
now gc'ed commit.  In the case I was able to dig into, we did not.
(There was a left-over .git/worktree/<something> that had a now gc'ed
commit, but no working directory that used it.)

I suspect you mean that there was another previous bug that induced
corruption, that this commit fixed that other bug, while also
introducing this new bug that makes folks' clones unusable because the
error doesn't provide enough information for users to know how to fix.
It took me hours to figure it out, after users ran out of ideas and
came and asked me for help.  (Maybe if I was familiar with worktree,
and knew they had been using it, then I might have guessed that "HEAD"
meant "not your actual HEAD but the HEAD of the vestige of some other
worktree").

Does anyone have pointers about what might be doable in terms of
providing a more useful error message to allow users to recover?
And/or ideas of what steps could cause corruption so I can send out a
PSA to help users avoid it?

If not, I'll try to dig more, but I thought I'd ask others familiar
with this area.

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Duy Nguyen]

On Wed, Feb 7, 2018 at 7:00 AM, Elijah Newren <newren@gmail.com> wrote:
> and knew they had been using it, then I might have guessed that "HEAD"
> meant "not your actual HEAD but the HEAD of the vestige of some other
> worktree").
>
> Does anyone have pointers about what might be doable in terms of
> providing a more useful error message to allow users to recover?

I noticed this too. I was working on improving this message a bit but
got side tracked and since I figured this did not happen often
anymore, this fix got lower priority than others. I'll resume that
work.

> And/or ideas of what steps could cause corruption so I can send out a
> PSA to help users avoid it?

There is another thing we could do. One bad HEAD should not abort the
entire operation (at least if it's not the current worktree's HEAD).
We could still give a warning and move on, or don't warn at all and
let "git worktree prune" collect it (which I see from your message
that it also fails to do).

I guess that's two more items on my todo list :) Sorry for all the
trouble because of this bug of mine.

> If not, I'll try to dig more, but I thought I'd ask others familiar
> with this area.
-- 
Duy

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Jeff King]

On Wed, Feb 07, 2018 at 06:08:40PM +0700, Duy Nguyen wrote:

> > And/or ideas of what steps could cause corruption so I can send out a
> > PSA to help users avoid it?
> 
> There is another thing we could do. One bad HEAD should not abort the
> entire operation (at least if it's not the current worktree's HEAD).
> We could still give a warning and move on, or don't warn at all and
> let "git worktree prune" collect it (which I see from your message
> that it also fails to do).

Whether that's safe or not depends very much on what the caller intends
to do with the ref. Skipping broken refs in "git prune" is a bad idea,
for instance.

-Peff

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Elijah Newren]

On Wed, Feb 7, 2018 at 3:08 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Feb 7, 2018 at 7:00 AM, Elijah Newren <newren@gmail.com> wrote:
>> and knew they had been using it, then I might have guessed that "HEAD"
>> meant "not your actual HEAD but the HEAD of the vestige of some other
>> worktree").
>>
>> Does anyone have pointers about what might be doable in terms of
>> providing a more useful error message to allow users to recover?
>
> I noticed this too. I was working on improving this message a bit but
> got side tracked and since I figured this did not happen often
> anymore, this fix got lower priority than others. I'll resume that
> work.

Sweet, that'd be great.  Let me know if I can help.

>> And/or ideas of what steps could cause corruption so I can send out a
>> PSA to help users avoid it?
>
> There is another thing we could do. One bad HEAD should not abort the
> entire operation (at least if it's not the current worktree's HEAD).
> We could still give a warning and move on, or don't warn at all and
> let "git worktree prune" collect it (which I see from your message
> that it also fails to do).
>
> I guess that's two more items on my todo list :)

Cool, if you're taking a look at those, I'll take a look at the other
one mentioned by Peff in this thread -- "make fsck pay attention to
other worktree HEADs"  :-)

> Sorry for all the trouble because of this bug of mine.

Let me apologize if my tone came across too harshly in the report; I
sometimes accidentally do that.  Anyway, bugs happen.  People were
able to get back up and running with "just reclone somewhere else"
pretty quickly, and no one lost any important data here, it was just
jarring or perplexing enough that I felt the need to dig and figure
out an improvement before more reports come in.

Besides, you've made git a lot better.  Thanks for that.

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Jeff King]

On Tue, Feb 06, 2018 at 04:00:32PM -0800, Elijah Newren wrote:

> > According to Peff this got fixed
> > https://public-inbox.org/git/20171020031630.44zvzh3d2vlhglv4@sigill.intra.peff.net/
> > and but you've had a corrupted repo from back when you were using an older
> > version of Git.
> >
> > Did that repo exist before d0c39a49cc was rolled out? Then we can keep that
> > hypothesis of "left-over corruption" as Peff put it.
> 
> I'm somewhat confused by this explanation.  That precise commit is the
> one I bisected to that _caused_ the fetch to fail.  Also, there might
> be one important difference here -- in the link you provide, it
> suggests that you had a corrupted working directory that made use of a
> now gc'ed commit.  In the case I was able to dig into, we did not.
> (There was a left-over .git/worktree/<something> that had a now gc'ed
> commit, but no working directory that used it.)

If you had a corrupted .git/worktree/<something>/HEAD, then that does
sound like the same problem. It's true that the commit you bisected to
caused "fetch" to fail, but only because it started looking at more of
your corrupted repository. The corruption happened long before (and I
don't know exactly when it was fixed, but I couldn't replicate it
anymore; it might even still exist).

In your case it sounds like you have the extra twist that the matching
working directory for "<something>" had gone away, but I don't think
that materially changes anything. Until you run "git worktree prune",
that HEAD file is still there and still supposed to be valid.

> I suspect you mean that there was another previous bug that induced
> corruption, that this commit fixed that other bug, while also
> introducing this new bug that makes folks' clones unusable because the
> error doesn't provide enough information for users to know how to fix.

If you want to call that last thing a bug, then I guess so. It's perhaps
a matter for the philosophers whether it is the fault of the new code to
start complaining about an existing on-disk corruption.

> It took me hours to figure it out, after users ran out of ideas and
> came and asked me for help.  (Maybe if I was familiar with worktree,
> and knew they had been using it, then I might have guessed that "HEAD"
> meant "not your actual HEAD but the HEAD of the vestige of some other
> worktree").

Yeah, this is the obvious thing that seems like it ought to be improved.

> Does anyone have pointers about what might be doable in terms of
> providing a more useful error message to allow users to recover?
> And/or ideas of what steps could cause corruption so I can send out a
> PSA to help users avoid it?

Here's a minimal manual reproduction:

  # new repo...
  git init
  git commit --allow-empty -m one

  # with a worktree...
  git worktree add foo
  git -C foo commit --allow-empty -m two
  obj=.git/objects/$(git rev-parse foo | sed 's#..#&/#')

  # now we stop using that worktree
  git -C foo checkout --detach
  git branch -f -D foo
  rm -rf foo

  # and this is the corruption; this might have happened ye olden days
  # because of a bug in the worktree code, but we'll assume that somehow
  # the object went away
  rm -f $obj

And now lots of commands may fail with confusing errors:

  $ git prune
  fatal: unable to parse object: HEAD

Unfortunately fixing that is a little tricky. In this case the stack
looks like:

  parse_object_or_die (oid=0x7fffffffd690, name=0x555555792880 "HEAD") at object.c:239
  add_one_ref (path=0x555555792880 "HEAD", oid=0x7fffffffd690, flag=0, cb_data=0x7fffffffd8e0) at reachable.c:38
  refs_head_ref (refs=0x555555a65430, fn=0x5555556b6ef5 <add_one_ref>, cb_data=0x7fffffffd8e0) at refs.c:1316
  other_head_refs (fn=0x5555556b6ef5 <add_one_ref>, cb_data=0x7fffffffd8e0) at worktree.c:404

So other_head_refs knows that it's looking at the worktrees. And it
passes the alternate ref-store to refs_head_ref(), with "add_one_ref" as
the callback. But the knowledge that we're not talking about the real
"HEAD" is lost as we cross that callback boundary. We'd need to either
add another parameter to the callback, or have some way of talking about
"HEAD in this worktree" as a refname (which AFAIK we don't have).

As for PSAs, my normal go-to in confusing matters like this is git-fsck.
But it seems that it does not check worktree HEADs. :(

  $ git fsck
  Checking object directories: 100% (256/256), done.

So that seems like another bug.

The best PSA for this particular bug may be "try pruning the worktrees":

  $ git worktree prune -v
  Removing worktrees/foo: gitdir file points to non-existent location

  $ git prune; echo $?
  0

-Peff

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Jeff King]

On Wed, Feb 07, 2018 at 08:21:57AM -0500, Jeff King wrote:

> The best PSA for this particular bug may be "try pruning the worktrees":
> 
>   $ git worktree prune -v
>   Removing worktrees/foo: gitdir file points to non-existent location
> 
>   $ git prune; echo $?
>   0

Sorry, I just read Stefan's response and not your original before
responding. I see that you did try that. So IMHO that's a separate bug
that "worktree prune" did not do the right thing for your particular
case.

-Peff

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Elijah Newren]

On Wed, Feb 7, 2018 at 5:21 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 06, 2018 at 04:00:32PM -0800, Elijah Newren wrote:
>
>> It took me hours to figure it out, after users ran out of ideas and
>> came and asked me for help.  (Maybe if I was familiar with worktree,
>> and knew they had been using it, then I might have guessed that "HEAD"
>> meant "not your actual HEAD but the HEAD of the vestige of some other
>> worktree").
>
> Yeah, this is the obvious thing that seems like it ought to be improved.
<snip>
> Unfortunately fixing that is a little tricky. In this case the stack
> looks like:
>
>   parse_object_or_die (oid=0x7fffffffd690, name=0x555555792880 "HEAD") at object.c:239
>   add_one_ref (path=0x555555792880 "HEAD", oid=0x7fffffffd690, flag=0, cb_data=0x7fffffffd8e0) at reachable.c:38
>   refs_head_ref (refs=0x555555a65430, fn=0x5555556b6ef5 <add_one_ref>, cb_data=0x7fffffffd8e0) at refs.c:1316
>   other_head_refs (fn=0x5555556b6ef5 <add_one_ref>, cb_data=0x7fffffffd8e0) at worktree.c:404
>
> So other_head_refs knows that it's looking at the worktrees. And it
> passes the alternate ref-store to refs_head_ref(), with "add_one_ref" as
> the callback. But the knowledge that we're not talking about the real
> "HEAD" is lost as we cross that callback boundary. We'd need to either
> add another parameter to the callback, or have some way of talking about
> "HEAD in this worktree" as a refname (which AFAIK we don't have).

Can we use "worktrees/${WORKTREE}/HEAD"?  It already satisfies all the
necessary rev-parse rules...

(And on a slight tangent...do we want to start disallowing the
creation of branches/tags whose name starts with "worktrees/",
"refs/", "hooks/", or other paths that exists under gitdir?  Making a
branch named "refs/heads/foo" so that it fully-qualifies as
"refs/heads/refs/heads/foo" is always fun)

Re: BUG: fetch in certain repo always gives "did not send all necessary objects"

[Jeff King]

On Wed, Feb 07, 2018 at 09:25:42AM -0800, Elijah Newren wrote:

> > So other_head_refs knows that it's looking at the worktrees. And it
> > passes the alternate ref-store to refs_head_ref(), with "add_one_ref" as
> > the callback. But the knowledge that we're not talking about the real
> > "HEAD" is lost as we cross that callback boundary. We'd need to either
> > add another parameter to the callback, or have some way of talking about
> > "HEAD in this worktree" as a refname (which AFAIK we don't have).
> 
> Can we use "worktrees/${WORKTREE}/HEAD"?  It already satisfies all the
> necessary rev-parse rules...

True, but it's mostly an accident that it works. And once we have ref
backends besides the filesystem, it will probably stop working.

I think there was discussion at some point of embedding worktree refs
into the normal ref namespace, but I don't know what came of it (it's
not a feature I've followed very closely).

> (And on a slight tangent...do we want to start disallowing the
> creation of branches/tags whose name starts with "worktrees/",
> "refs/", "hooks/", or other paths that exists under gitdir?  Making a
> branch named "refs/heads/foo" so that it fully-qualifies as
> "refs/heads/refs/heads/foo" is always fun)

We recently taught the porcelain to disallow a branch named "HEAD".
Though I think there are actually two related problems with different
solutions. One is saying something like:

  git checkout -b HEAD

or:

  git checkout -b refs/heads/foo

both of which will not do what you want, and leave you with a
funnily-named branch in the ref namespace.

But that's separate from the fact that:

  git rev-parse info/refs

will look at a file that is not a ref at all. Long-term I think the
solution is storage formats that don't mingle with other files. But we
could probably teach even the files-backend that any ref at the
top-level is supposed to be either in refs/, or to consist only of
"[A-Z_]".

-Peff