Re: Issue with git log and reference repositories using --dissociate and --filter=blob:none

[+cc Jonathan Tan for his commit mentioned below but also as the general promisor repacking guru]

On Fri, May 17, 2024 at 02:52:45PM +0100, Matt Cree wrote:

Thanks for providing a thorough example, which let me reproduce the problem. I think the stuff about remote-https is a red herring. The fundamental issue is some weirdness in the interaction between the promisor and alternates features when repacking (a "promisor" pack is one we got from a filtered fetch, where the other side "promises" that it can give us the rest of the objects later). Plus Git being a little too hesitant to lazy-fetch the missing commit.

Here's an even simpler from-scratch reproduction:

-- >8 -- # start with a clean slate rm -rf server client reference

# we have a server that allows partial clones git init server git -C server config uploadpack.allowFilter true

# we also have a reference repo which has its first commit git -C server commit --allow-empty -m one git clone --bare server reference

# but importantly the server also has a second commit which is not # in the reference repo git -C server commit --allow-empty -m two

# now we do a partial reference clone. This is going to get a new copy # of commit "two" in a pack marked as a promisor (since we know the # other side can give us anything it points to later if we ask). But it # won't get a copy of "one", because that's in the reference repo git clone --no-local --reference reference --filter=blob:none server client

# At this point we're good, and can access all objects. But if we had # specified --dissociate, it would do the equivalent of these commands: cd client git repack -ad rm -f .git/objects/info/alternates

# And now we find that when we run git-log, we are missing commit "one"! git --no-pager log -- >8 --

During that repack we actually make two packs:

  1. We repack all of the promisor objects we have into their own new
     pack (so just "two" in this case).

  2. And then we follow up by making a new pack with all of the
     non-promisor objects (both local and non-local). Which in this case
     would usually be "one". But we tell it to exclude any promisor
     objects, which causes our traversal to mark "two" with the
     UNINTERESTING flag. And then as we traverse, that flag transitively
     applies to things we can reach, including "one", and we exclude it
     from this pack.

     So we realize that "one" is something we _could_ ask the server
     for, even though it's not in a promisor pack itself. But it's sort
     of both a promisor (reachable from a promisor object) and sort of
     not (we don't have it in a promisor pack). And so it's included in
     neither of the new packs.

I think this is sub-optimal, in that we'd usually try to keep promisor objects we have locally (without alternates, they'd have come from the original filtered fetch and would be in the promisor pack). But we fail to migrate them to the new pack in this case.

However, this isn't actually a corruption, because you really can get "one" from the server. But git-log doesn't ask for it! It used to, but stopped due to 7e2ad1cda2 (commit: don't lazy-fetch commits, 2022-12-14), where parse_commit(), etc, won't do the lazy fetch. You can still grab it with something like "git cat-file commit HEAD^", which lazy-fetches via read_object_file(). After which the repo is repaired (though of course in a bigger repository there may be many such commits).

The logic in 7e2ad1cda2 is that we don't have any filters which exclude commits, so there's no need to lazy-fetch them. But obviously we can still be missing them due to the promisor repacking scheme, as above. So I'm not sure if that commit is wrong, or if the repacking should be more careful about holding on to objects from the reference repo (which would fix the bug, but also do the more efficient thing the user was trying in the first place).

-Peff