Re: [PATCH v2 2/3] promisor-remote: allow a server to advertise more fields

[Patrick Steinhardt <ps@pks.im>]

On Mon, May 19, 2025 at 04:11:18PM +0200, Christian Couder wrote:
> On Wed, May 7, 2025 at 10:25 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Tue, Apr 29, 2025 at 04:52:42PM +0200, Christian Couder wrote:
> > > diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
> > > index 2638b01f83..71311b70c8 100644
> > > --- a/Documentation/config/promisor.adoc
> > > +++ b/Documentation/config/promisor.adoc
> > > @@ -9,6 +9,24 @@ promisor.advertise::
> > >       "false", which means the "promisor-remote" capability is not
> > >       advertised.
> > >
> > > +promisor.sendFields::
> > > +     A comma or space separated list of additional remote related
> > > +     fields that a server will send while advertising its promisor
> > > +     remotes using the "promisor-remote" capability, see
> > > +     linkgit:gitprotocol-v2[5]. Currently, only the
> > > +     "partialCloneFilter" and "token" fields are supported. The
> > > +     "partialCloneFilter" field contains the partial clone filter
> > > +     used for the remote, and the "token" field contains an
> > > +     authentication token for the remote.
> > > ++
> >
> > Should we maybe convert this into a list of accepted fields? Makes it
> > easier to extend going forward.
> 
> I am not sure I understand what you mean. This promisor.sendFields
> config variable is for the server side which advertises remotes. The
> server advertises its remotes (if it wants to) before receiving
> information from the client, so it cannot know what the client
> accepts.

In the current form you need to reflow this whole paragraph every time a
new field is supported, and it's easy to miss the exact supported
fields. So my idea was to maybe move the supported fields into a
bulleted list. E.g.:

    promisor.sendFields::
         A comma or space separated list of additional remote related
         fields that a server will send while advertising its promisor
         remotes using the "promisor-remote" capability, see
         linkgit:gitprotocol-v2[5]. The following fields are supported:
    +
    * "partialCloneFilter": contains the partial clone filter used for
      the remote.
    * "token": contains the authentication token for the remote.

> > Furthermore, should we maybe refactor this to match the restrictive
> > design where valid fields are explicitly specified? In other words,
> > should we have separate config keys for each of the accepted fields now?
> 
> Maybe I don't understand what you mean with "accepted fields".

I think I had a misunderstanding on my side. I didn't get that this is
only configuring field _names_ that we'll end up sending to the remote
side. So I thought that the user is expected to configure name-value
pairs here that are then sent to the client, not only the name.

I guess this is mostly because the config documentation talks about
"fields", but that term is used elsewhere to indicate a name-value pair.

> > Also, shouldn't this setting be per promisor remote that we want to
> > advertise? I expect that servers will want to send different partial
> > clone filters for each of the advertised remotes, and they may also want
> > to send different tokens. So it seems a bit too inflexible to only have
> > a single, global "sendFields" configuration that covers all promisors.
> 
> First this setting already allows servers to send different partial
> clone filters for each of the advertised remotes. For each remote it
> advertises, a server would send the partial clone filter that is
> already configured for this remote on the server. Same for tokens.
> 
> Also we can extend this setting to be per promisor remote later if
> there is a need for it. I don't think it would be difficult to do. And
> I don't think it's necessary right now, because it's likely that for
> simplicity most servers will manage all their promisor remotes in the
> same way (at least until usage of promisor remotes grows a lot).

Yeah, with my fixed understanding I agree that it's not necessary to
configure this per remote as of now.

[snip]
> > Does
> > it mean that this promisor remote should only be used in case we do have
> > the exact same filter passed to git-clone(1)?
> 
> It's up to the client to decide, but yeah it will likely work better
> if the same filter is used. It should still work if a different filter
> is used though. In case the promisor remote doesn't have an object,
> there should be a fallback to ask the main server for that object.
> 
> Also the filter mechanism already exists for a long time and this
> series doesn't change how it works. It's already possible to have
> different repos using the same promisor remote with different filters.
> So documentation about what happens when they do that should not be
> specific to this patch series.

That's fair enough, but spelling this out somewhere and drawing the
bigger picture helps the reviewer understand the vision that you've got
here.

Patrick