Re: [PATCH v3 0/3] upload-pack: support a missing-action

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2024, 18:25 UTC
Message-ID: <xmqq5xv3kqxh.fsf@gitster.g>
In-Reply-To: <20240524163926.2019648-1-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 6 quoted lines

> The changes since v2 are the following:
> ...
>
> Thanks to Junio for his reviews of v1 and v3, and for suggesting the
> most of the above changes.
> ...

Show 30 quoted lines

>
> Range diff between v2 and v3
> ============================
>
> (Might not be super useful as changes in patch 2/3 are not seen.)
>
> 1:  0a961dd4f5 = 1:  67c761b08a rev-list: refactor --missing=<missing-action>
> 2:  410acc6a39 < -:  ---------- pack-objects: use the missing action API
> -:  ---------- > 2:  7bf04f3096 pack-objects: use the missing action API
> 3:  0f5efb064b ! 3:  bac909a070 upload-pack: allow configuring a missing-action
>     @@ Metadata
>       ## Commit message ##
>          upload-pack: allow configuring a missing-action
>      
>     -    In case some objects are missing from a server, it might still be
>     +    In case some objects are missing from a server, it is sometimes
>          useful to be able to fetch or clone from it if the client already has
>          the missing objects or can get them in some way.
>      
>     -    For example, in case both the server and the client are using a
>     -    separate promisor remote that contain some objects, it can be better
>     -    if the server doesn't try to send such objects back to the client, but
>     -    instead let the client get those objects separately from the promisor
>     -    remote. (The client needs to have the separate promisor remote
>     -    configured, for that to work.)
>     +    Suppose repository S borrows from its "promisor" X, and repository C
>     +    which initially cloned from S borrows from its "promisor" S. If C
>     +    wants an object in order to fill in the gap in its object graph, and
>     +    S does not have it (as S itself has no need for that object), then it
>     +    makes sense to let C go directly to X bypassing S.

Most notably, what is still missing in this iteration, even though I already pointed it out in the earlier reviews, is that the readers would not get a good sense of how much trust they need to place on the other side S, in order to save their repositories from getting corrupted by S sending an incomplete pack, and what mechanism there already is to make sure missing objects after fetching such an incomplete pack from S are all available at X.

In short, I agree with the goal of having "S is borrowing from X, we cloned from S, we can fill our missing objects by lazily fetching directly from X" as a feature. But I want to see it as a safe feature, but from these patches I do not see how the necessary safety is guaranteed.

Thanks.