Re: Multiple fetches when unshallowing a shallow clone

From: Jeff King <peff@peff.net>
Date: Dec 6, 2015, 06:37 UTC
Message-ID: <20151206063718.GA549@sigill.intra.peff.net>
In-Reply-To: <xmqqmvtons4j.fsf@gitster.mtv.corp.google.com>

On Sat, Dec 05, 2015 at 08:00:28PM -0800, Junio C Hamano wrote:

Show 5 quoted lines

> > I suppose followtags feature has been around long enough that we can
> > simply trust that and skip the second fetch?
> 
> Hmmmm, I wonder why the code needs the backfill fetch while talking
> to a server that has the include-tag capability, then?

The logic in find_non_local_tags makes no sense to me.

After we do the first fetch, we call this function to find out if there are any tags we need to pick up. We iterate through the list of refs given to us by the remote (including peeled lines), and do:

        if (ends_with(ref->name, "^{}")) {
                if (item && !has_sha1_file(ref->old_sha1) &&
                    !will_fetch(head, ref->old_sha1) &&
                    !has_sha1_file(item->util) &&
                    !will_fetch(head, item->util))
                        item->util = NULL;
                ...
        }

where "ref" is the peeled line (i.e., the pointed-to commit), and "item" is the preceding line (i.e., the tag itself) with util set to its sha1. Any such item whose util survives as non-NULL is fetched in the backfill step.

So I'd think the logic for backfilling a given tag should be:

  1. We have the peeled object, and...

  2. We don't currently have the tag pointing to it, and...

  3. We are not already going to fetch it.

You could write that as:

  if (has_sha1_file(ref->old_sha1) &&
      !has_sha1_file(item->util) &&
      !will_fetch(head, item->util))

Of course the conditional in the code is "should we skip the backfill", the negation. So using De Morgan's laws, we'd write:

  if (!has_sha1_file(ref->old_sha1) ||
      has_sha1_file(item->util) ||
      will_fetch(head, item->util))

Which is quite a bit different than what is there. I'm not sure at all what the "will_fetch(head, ref->old_sha1)" is doing. In fact, for the backfill step, it looks like we feed an empty "head" list, so the !will_fetch() is always true.

And indeed, replacing the logic with what I wrote does make the backfill go away in my test case. But it's so far from what is there that I feel like I must be missing something.

-Peff