Re: Tag peeling peculiarities

From: Jeff King <peff@peff.net>
Date: Mar 14, 2013, 13:40 UTC
Message-ID: <20130314134032.GA9222@sigill.intra.peff.net>
In-Reply-To: <5141B475.1000707@alum.mit.edu>

On Thu, Mar 14, 2013 at 12:28:53PM +0100, Michael Haggerty wrote:

> Perhaps if peel_ref() *were* 100% reliable, we might be able to use it
> to avoid object lookups in some other places.

In theory, some of the many uses of deref_tag could be adopted. However, we do not always have the refname handy at that point (and I believe peel_ref's optimization only kicks in during for_each_ref traversals anyway).

It may still be a win to check the packed-refs file before peeling a random sha1, as looking up there should be cheaper than actually loading the object. But right now, the way the optimization is used is always O(1) to just check the last ref loaded. With your recent ref refactoring, I think we should be able to do lookups in O(lg n).

Show 9 quoted lines

> > Another fun fact: upload-pack did not use peel_ref until recently
> > (435c833, in v1.8.1). So while it is tempting to say "well, this was
> > always broken, and nobody cared", it was not really; it is a fairly
> > recent regression in 435c833.
> 
> I didn't realize that; thanks for pointing it out.  Although peel_ref()
> itself has been broken since it was introduced, at least in the sense
> that it gives wrong answers for tags outside of refs/tags, prior to
> 435c833 it appears to only have been used for refs/tags.

Hmph. I coincidentally ran across another problem with 435c833 today. Try this:

  git init repo &&
  cd repo &&
  git commit --allow-empty -m one &&
  git checkout -b other &&
  git commit --allow-empty -m two &&
  git checkout master &&
  git commit --allow-empty -m three &&
  git pack-refs --all &&
  git.compile clone --no-local --depth=1 --no-single-branch . foo

I get:

  Cloning into 'foo'...
  fatal: (null) is unknown object
  remote: Total 0 (delta 0), reused 0 (delta 0)
  fatal: recursion detected in die handler
  remote: aborting due to possible repository corruption on the remote side.
  fatal: error in sideband demultiplexer

This is not due to the same problem you are describing with peel_refs, but simply due to the fact that we do not load and parse objects anymore (which is the point of the optimization). The client feeds these back to us in the "want" list, and we then feed these objects to the revision walker, which expects them to be parsed and have their "type" field actually filled in.

We never noticed before because:

  1. It only happens with --depth, because otherwise we do not do the
     revision walk in-process.

  2. Modern git, when given --depth, will default to --single-branch,
     and so ask only about HEAD, which we do parse. However, older
     versions of git (pre v1.7.10) will ask for much more, and trigger
     the bug.

     I haven't tried yet, but I suspect you may also be able to trigger
     it by asking for "clone --depth=1 -b other".

That's the overtly visible symptom. We also check the type in ok_to_give_up, so I suspect that can produce subtly wrong results in some situations. The solution is to actually parse the objects in question, but I need to figure out when is the optimal time. Obviously any time we read a "want" line would be enough, but we may be able to get by with less. OTOH, it probably doesn't matter that much; the point of the optimization was to avoid touching objects for the ref advertisement. Once we have a "want", the client really is going to fetch objects, and accessing them will probably be lost in the noise.

But that's somewhat off-topic for this discussion. I'll look into it further and try to make a patch later today or tomorrow.

> Your patch looks about right aside from its lack of comments :-).  I was
> going to implement approximately the same thing in a patch series that I
> am working on, but if you prefer then go ahead and submit your patch and
> I will rebase my branch on top of it.

I just meant it as a quick sketch, since you said you were working in the area. I'm not sure what you are working on. If we don't consider it an urgent bugfix, I'm just as happy for you to incorporate the idea into what you are doing.

But if we want to do it as a maint-track fix, I can clean up my patch. Junio?

-Peff