Re: [PATCH 3/4] Add 'promisor-remote' capability to protocol v2

From: Christian Couder <christian.couder@gmail.com>
Date: Sep 10, 2024, 16:31 UTC
Message-ID: <CAP8UFD34FP2CCaeeYU5maUiNB5m5pD3_neyEYFtf8GQ4xCZmvA@mail.gmail.com>
In-Reply-To: <ZrDYIFolRlERFdUT@tanuki>

On Mon, Aug 5, 2024 at 8:01 PM Patrick Steinhardt <ps@pks.im> wrote:

>
> On Wed, Jul 31, 2024 at 03:40:13PM +0200, Christian Couder wrote:

Show 31 quoted lines

> > +The server may advertise some promisor remotes it is using, if it's OK
> > +for the server that a client uses them too. In this case <pr-infos>
> > +should be of the form:
> > +
> > +     pr-infos = pr-info | pr-infos ";" pr-info
> > +
> > +     pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
> > +
> > +where `pr-name` is the name of a promisor remote, and `pr-url` the
> > +urlencoded URL of that promisor remote.
> > +
> > +In this case a client wanting to use one or more promisor remotes the
> > +server advertised should reply with "promisor-remote=<pr-names>" where
> > +<pr-names> should be of the form:
> > +
> > +     pr-names = pr-name | pr-names ";" pr-name
> > +
> > +where `pr-name` is the name of a promisor remote the server
> > +advertised.
> > +
> > +If the server prefers a client not to use any promisor remote the
> > +server uses, or if the server doesn't use any promisor remote, it
> > +should only advertise "promisor-remote" without any value or "=" sign
> > +after it.
> > +
> > +In this case, or if the client doesn't want to use any promisor remote
> > +the server advertised, the client should reply only "promisor-remote"
> > +without any value or "=" sign after it.
>
> Why does the client have to advertise anything if they don't want to use
> any of the promisor remotes?

I have tried to explain it in a reply to Taylor, but as you, Junio and others seem to prefer the capability not to be advertised at all when not used, I have changed this in version 2.

Show 9 quoted lines

> > +The "promisor.advertise" and "promisor.acceptFromServer" configuration
> > +options can be used on the server and client side respectively to
> > +control what they advertise or accept respectively. See the
> > +documentation of these configuration options for more information.
>
> One thing I'm not totally clear on is the consequence of this
> capability. What is the expected consequence if the client accepts one
> of the promisor remotes? What is the consequence if the client accepts
> none?

I have tried to improve the documentation significatively, especially according to Junio's suggestion, in version 2.

> In the former case I'd expect that the server is free to omit objects,
> but that isn't made explicit anywhere, I think.

Junio also suggested making it explicit so I have done that in version 2.

> Also, is there any
> mechanism that tells the client exactly which objects have been omitted?

I don't think it's necessary. Agreeing on which promisor remote (name and URL) to use should be enough security wise. When using bundle-uri, for example, the server is not telling the client exactly which objects are in the bundle.

> In the latter case I assume that the result will be a full clone, that
> is the server fetched any objects it didn't have from the promisor?

Yeah, the server should fetch any objects it doesn't have from the promisor, so it can send everything to the client.

> Or does the server side continue to only honor whatever the client has
> provided as object filters, but signals to the client that it shall
> please contact somebody else when backfilling those promised objects?

No. Options to enable things like this could be built on top later if needed though.

Thanks for the review.