On 5/14/2021 3:27 AM, Jeff King wrote:
> [+cc Jonathan Tan for partial clone wisdom]
>
> On Thu, May 13, 2021 at 06:53:36AM -0400, Jeff King wrote:
>
>>> So I think it will require some digging. My _guess_ is that it has to do
>>> with the "never filter out an object that was explicitly requested" rule
>>> not being consistently followed. But we'll see.
>>
>> OK, I think I've unraveled the mysteries.
>
> Nope. This part is wrong:
>
>> It is indeed a problem with the "never filter out an object that was
>> explicitly requested" rule. But really, that rule is stronger: it is
>> "always include an explicitly requested object". I.e., it must override
>> even the usual "you said commit X was uninteresting, so we did not
>> include objects reachable from X" logic.
>
> The rule really is the softer "don't filter out explicitly-requested
> objects". It's just that the non-bitmap traversal code is way less
> careful about finding uninteresting objects.
>
> Here's the same scenario failing without using bitmaps at all:
>
...
> # ...and then we fetch both the object we need _and_ that second
> # commit. That causes pack-objects to traverse from base..more.
> # The boundary is at "base", so we mark its tree and blob as
> # UNINTERESTING, and thus we _don't_ send them.
> cd clone
> git fetch origin $(git rev-parse HEAD:file) HEAD
This is the critical reason why this isn't failing in practice: it
is very rare to ask for a commit and a blob at the same time. Usually,
a blob request comes when something like 'git checkout' is actually
navigating to a commit, and then it only asks for missing blobs.
> So I guess the first question is: is this supposed to work? Without
> bitmaps, it often will. Because we walk commits first, and then only
> mark trees uninteresting at the boundary; so if there were more commits
> here, and we were asking to get a blob from one of the middle ones, it
> would probably work. But fundamentally the client is lying to the server
> here (as all partial clones must); it is saying "I have that first
> commit", but of course we don't have all of the reachable objects.
It _should_ work. We should be specifying the blob:none filter, so when
we say "we have these commits" it should apply that filter to those
commits for the "haves".
If there was room to adjust the protocol more, it would perhaps be
helpful to annotate the haves in this way ("have-filtered" or something,
similar to how the "shallow" advertisement works).
> If this is supposed to work, I think we need to teach the traversal code
> to "add back" all of the objects that were explicitly given when a
> filter is in use (either explicitly, or perhaps just clearing or
> avoiding the UNINTERESTING flag on user-given objects in the first
> place). And my earlier patch does that for the bitmap side, but not the
> regular traversal.
This gets more complicated if we were in a treeless clone, for example.
We could explicitly ask for a tree and really need all of its reachable
trees and blobs. It's not enough to just isolate that single object.
I wouldn't spend too much time optimizing for the treeless clone case,
as I believe the client will self-recover by asking for those reachable
trees when it walks to them.
> Alternatively, part of that fundamental "partial clones are lying about
> their reachability" property is that we assume they can fetch the
> objects they need on-demand. And indeed, the on-demand fetch we'd do
> will use the noop negotiation algorithm, and will succeed. So should we,
> after receiving an empty pack from the other side (because we claimed to
> have objects reachable from the commit), then do a follow-up on-demand
> fetch? If so, then why isn't that kicking in (I can guess there might be
> some limits to on-demand fetching during a fetch itself, in order to
> avoid infinite recursion)?
The client has an opportunity to be more robust here, especially in
the case of a promisor remote. A simple retry would be a good band-aid
for now.
A more involved case might be to isolate objects that we have
identified as reachable from a local object, and set those aside
for a separate request. Here, we could ask for the blob in a
different request than for the remote's HEAD reference.
Thanks,
-Stolee
