patch, 4 partsMake the "promisor-remote" capability support extra fields

The "promisor-remote" capability can only be used to pass the names and URLs of the promisor remotes from the server to the client. After that the client can use this information to decide if it accepts the remotes or not.

It would be nice if the server could pass more fields about its remotes and if the client could use that extra information to decide about the remotes by comparing it with its local information about the remotes.

This patch series implements this by adding the "promisor.sendExtraFields" on the server side and the "promisor.checkExtraFields" on the client side.

For example, if "promisor.sendExtraFields" is set to "partialCloneFilter", and the server has the remote "foo" configured like this:

[remote "foo"]
        url = file:///tmp/foo.git
	partialCloneFilter = blob:none

then "name=foo,url=file:///tmp/foo.git,partialCloneFilter=blob:none" will be sent by the server for this remote.

All the information passed through the "promisor-remote" capability is still only used to decide if the remotes are accepted or not. The client doesn't store it and doesn't use it for any other purpose.

On the technical side, we get rid of 'struct strvec' and we use mostly 'struct string_list' instead, which is more flexible. This matches some suggestions made when the series that introduced the "promisor-remote" capability was reviewed.

Right now in this series, the fields names (like "partialCloneFilter" in the example) that are passed to the client are compared case sensitively to the local config keys. I think this is a bug and they should be compared case insensitively, (while values should still be compared case sensitively). I am planning to fix this in the next iteration of this series. I am also planning to add more tests then.

CI tests are not finished but look fine so far:

https://github.com/chriscool/git/actions/runs/14449949491

Christian Couder (4):
  config: move is_config_key_char() to "config.h"
  promisor-remote: refactor to get rid of 'struct strvec'
  promisor-remote: allow a server to advertise extra fields
  promisor-remote: allow a client to check extra fields

 Documentation/config/promisor.adoc    |  32 +++
 Documentation/gitprotocol-v2.adoc     |  42 ++--
 config.c                              |  11 +-
 config.h                              |   9 +
 promisor-remote.c                     | 287 +++++++++++++++++++++-----
 t/t5710-promisor-remote-capability.sh |  67 ++++++
 6 files changed, 374 insertions(+), 74 deletions(-)

The iskeychar() function in "config.c" checks if a character is valid for the section or variable name part of a config key.

In a follow up commit we will want to check outside "config.c" if a string can be a valid variable name of a config key, so will will want to reuse that fonction.

Let's then move it from "config.c" to "config.h", and, while at it, let's rename it to is_config_key_char().

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 config.c | 11 +++--------
 config.h |  9 +++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url.

For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case.

Let's refactor this and introduce a new 'struct promisor_info' linked list that contains a 'struct string_list fields'. This string_list stores the field names, like "name" and "url", in the 'string' member of its items, and the field values in the 'util' member of its items.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 124 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 37 deletions(-)

On Mon, Apr 14, 2025 at 06:03:41PM +0200, Christian Couder wrote:

Nit: nowadays we would call this something like
`promisor_info_list_free()`, with the name of the subsystem coming
first.

I don't quite follow the usage pattern of `info_list` here. My expectation is that we'd:

  1. Acquire the promisor info list.

  2. Iterate through each of its items.

  3. Free the complete list.

But why do we free `p` here? Shouldn't we free `info_list`? And if we did so, can't we drop `p` completely and just iterate through the list via `r`?

Why do we add this assert now? And if we want to keep it, shouldn't it rather be `BUG()`?

It feels somewhat fragile to assume hardcoded locations of each of the keys in `fields.items`. Would it be preferable to instead have a function that looks up the index by key?

Patrick

On Tue, Apr 22, 2025 at 12:13 PM Patrick Steinhardt <ps@pks.im> wrote:

Fine, I have changed it to `promisor_info_list_free()` in the next version.

Yeah, it's a bug that is fixed in the next version.

Yeah, it's now BUG() in the next version.

On the other hand we always require a 'name' and a 'url' fields, so putting them first can simplify and speed things up.

If the checks look too ugly maybe they can be abstracted out in functions dedicated to get the name or the url?

For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=<remote_name>,url=<remote_url>".

Let's make it possible to pass more information by introducing a new "promisor.sendExtraFields" configuration variable. This variable should contain a comma or space separated list of fields that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client.

For example if "promisor.sendExtraFields" is set to "partialCloneFilter", and the server has the "remote.<name>.partialCloneFilter" config variable set to a value for a remote, then that value will be passed in the form "partialCloneFilter=<value>" after the "name" and "url" fields.

A following commit will allow the client to use the extra information to decide if it accepts the remote or not. For now the client doesn't do anything with the extra information it receives.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    | 15 ++++++
 Documentation/gitprotocol-v2.adoc     | 42 +++++++++------
 promisor-remote.c                     | 75 ++++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh | 32 ++++++++++++
 4 files changed, 139 insertions(+), 25 deletions(-)

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

As a description of overall syntax of the protocol, this and ...

... this may work, but as we are defining protocol between two parties, in order to ensure interoperable reimplementations, we need to also specify semantics, what are the defined "fields", and what each of them mean. Proposing nebulous "with this framework your imagination is the limit, you can invent anything" may work for other parts of the system, but not for the part that is about communication between two repositories.

IOW, we shouldn't be internally calling these "extra". From the point of view of "core" they may be "extra", but to the developers and certainly to the end-users, they shouldn't be "extra" at all. They are all supported parts of the system with defined semantics, right?

Another reason why I hate seeing this nebulous "with this, we can send anything extra" is because such a thing will have a wrong security posture. If we truly *need* to be able to carry *anything*, we need to make sure how values are quoted/escaped, and the code for dequoting/unescaping are robustly written to avoid passing malformed input and misinterpreting it as something else, which would give a new attack vector. If we can enumerate supported fields, their syntax and their possible values, we can make the attack surface a lot smaller.

Thanks.

On Mon, Apr 14, 2025 at 03:04:07PM -0700, Junio C Hamano wrote:

I agree that we should properly specify which fields are accepted and what their respective format as well as semantics would be. Otherwise, without such a definition, hosting sites may eventually end up with slightly incompatible semantics. I also don't expect that there should be all that many fields.

This raises a question though: what would happen if a field was advertised that the client doesn't understand? Should the client simply ignore such a field? Should they bail out? I think we need to also think about this edge case and specify client-side behaviour. I think in the end, both ways would be rather limiting:

  - If we simply ignored all unknown fields our hands might be bound if
    we ever had to introduce changes that aren't backwards compatible.

  - If we always bail out on an unknown field our hands would be bound
    equally, as we cannot ever introduce a new field.

Which raises the question whether we need to be able to dynamically figure out fields. This could be in the form of capability negotiation or protocol versions. But in any case, I think we need to have something ready so that we can change behaviour depending on which features are supported by a client.

Patrick

On Tue, Apr 22, 2025 at 12:13 PM Patrick Steinhardt <ps@pks.im> wrote:

Yeah, I have done that in the next version where only "partialCloneFilter" and "token" are accepted and are documented.

Yes, for backward compatibility we need them to ignore fields they don't understand.

No, this would prevent clients that are a bit old but could work with a server to just not be able to work anymore as soon as the server tries new features.

Yeah, but that still leaves backward compatible changes as OK.

Yeah, that's worse.

There is no real negotiation. The server advertises some remotes and associated fields, and the client just accepts some of these remotes or not. We would need a separate protocol capability if we wanted a real capability negotiation.

If the client ignores fields it doesn't know, a server can provide both some fields for old clients and new fields for new clients. Then the client itself will be able to use what looks best for itself.

On Tue, Apr 15, 2025 at 12:04 AM Junio C Hamano <gitster@pobox.com> wrote:

[...]

Yeah, we can specify a set of accepted fields and their semantics, and that's what I have done in the next version where only "partialCloneFilter" and "token" are accepted so far.

One issue with this is that we might not know how user systems will work and what they will need. For example if they use a special remote helper they created to access a special remote, we might not know what kind of credentials the underlying system on that remote will need, so they might be tempted to reuse some existing fields that weren't designed for that purpose if we don't make it easy enough to add what they need.

Anyway maybe we can deal with that problem later if it ever happens. The "token" field might be enough or maybe there aren't so many kinds of credentials so adding a few more will be enough. And yeah, it's easier to be more lenient on this later than to restrict things later.

I used "extra" to mean "additional" and "optional", compared to the "name" and "url" fields which are mandatory.

But yeah I have removed "extra" from all the names in the next version to avoid possible confusion.

Yeah, maybe it will help a bit security wise.

A previous commit allowed a server to pass extra fields through the "promisor-remote" protocol capability after the "name" and "url" fields.

Let's make it possible for a client to check if these extra fields match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkExtraFields" configuration variable. It should contain a comma or space separated list of fields that will be checked.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  17 +++++
 promisor-remote.c                     | 102 +++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh |  35 +++++++++
 3 files changed, 143 insertions(+), 11 deletions(-)

The "promisor-remote" capability can only be used to pass the names and URLs of the promisor remotes from the server to the client. After that the client can use this information to decide if it accepts the remotes or not.

It would be nice if the server could pass more fields about its remotes and if the client could use that additional information to decide about the remotes by comparing it with its local information about the remotes.

This patch series implements this by adding the "promisor.sendFields" on the server side and the "promisor.checkFields" on the client side.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the remote "foo" configured like this:

[remote "foo"]
        url = file:///tmp/foo.git
	partialCloneFilter = blob:none

then "name=foo,url=file:///tmp/foo.git,partialCloneFilter=blob:none" will be sent by the server for this remote.

All the information passed through the "promisor-remote" capability is still only used to decide if the remotes are accepted or not. The client doesn't store it and doesn't use it for any other purpose.

The fields that can be passed are limited to "partialCloneFilter" and "token".

On the technical side, we get rid of 'struct strvec' and we use mostly 'struct string_list' instead, which is more flexible. This matches some suggestions made when the series that introduced the "promisor-remote" capability was reviewed.

Right now in this series, the fields names (like "partialCloneFilter" in the example) that are passed to the client are compared case sensitively to the local config keys. I think this is a bug and they should be compared case insensitively, (while values should still be compared case sensitively). I am planning to fix this in the next iteration of this series. I am also planning to add more tests then.

Changes since v1 ----------------

Thanks to Patrick and Junio for their comments on the v1.

* Patch 1/4 in v1 has been removed. It isn't necessary anymore because
  we limit the possible field names to some harcoded values. So we
  don't need to check if the field names contain only valid
  characters.

* In patch 1/3, the commit message has been improved to explain why we
  use "field" and to tell that they should correspond to config
  variables.

* In patch 1/3, the following improvements have been made to the code:

  - the comment before the definition of 'struct promisor_info' has
    been improved to better describe the fields and their possible
    names

  - free_info_list() has been renamed promisor_info_list_free()

  - field names are now compared case insensitively by setting the
    'cmp' member of 'struct string_list' to 'strcasecmp'

  - a memory leak in promisor_info_vecs() has been fixed and the code
    simplified, as we used to free only a single 'struct
    promisor_info' instance instead of all the instances

  - an assert() has been replaced by a BUG()

  - a BUG() message has been improved

* In patch 2/3, there are a number of general changes:

  - the name of the "promisor.sendExtraFields" config variable has
    been changed to "promisor.sendFields"

  - in general we just talk about "fields" and not "extra fields"

  - only the "partialCloneFilter" and "token" fields are allowed on
    top of the pre-existing "name" and "url" fields

* In patch 2/3, in "Documentation/gitprotocol-v2.adoc", the
  documentation of the "promisor-remote" capability has been improved
  in a few ways:

  - `fld-key` has been renamed `fld-name` to be consistent with
    everywhere else where we talk about field names

  - it's stated that "Clients SHOULD ignore fields they don't
    recognize to allow for future protocol extensions"

* In patch 2/3, the following improvements have been made to the code:

  - a `static const char *allowed_fields[]` is used to store the
    allowed field names in an extensible way

  - a new is_allowed_field() function is introduced and used in
    valid_field() to check if a field name is valid

  - "extra" has been removed from all function and variable names

* In patch 2/3, the tests have been improved to use "token" instead of
  "id" as "token" is among the list of allowed field names.

* In patch 3/3, there are the same general changes as in patch 2/3.

* In patch 3/3, the commit message says that limiting the fields
  allows both server and client have a shared understanding of field
  semantics and usage.

* In patch 3/3, the following improvements have been made to the code:

  - "extra" has been removed from all function and variable names

  - field names are compared case insensitively by setting the 'cmp'
    member of 'struct string_list' to 'strcasecmp'
  
* In patch 3/3, the tests have been improved to use "token" instead of
  "id" as "token" is among the list of allowed field names.

CI tests --------

Here they are:

https://github.com/chriscool/git/actions/runs/14729278162

They have all passed except the "pedantic(fedora:latest)" one which failed with the following error:

  t/unit-tests/generate-clar-suites.sh: line 63: awk: command not found

Range diff compared to v1 -------------------------

1:  fe49a861b9 < -:  ---------- config: move is_config_key_char() to "config.h"
2:  6ebff6d784 ! 1:  d4e12afbc1 promisor-remote: refactor to get rid of 'struct strvec'
    @@ Commit message
         stores the field names, like "name" and "url", in the 'string' member
         of its items, and the field values in the 'util' member of its items.
     
    +    Except for "name", each "<field_name>/<field_value>" pair should
    +    correspond to a "remote.<name>.<field_name>" config variable set to
    +    <field_value> where "<name>" is a promisor remote name.
    +
    +    Previously in Git, the part after the last dot in a configuration
    +    variable key, for example "c" for "a.b.c", was called the "variable
    +    name part" of a configuration key. It would be very confusing to use
    +    "variable name part" or some similar terms in the context of the
    +    'promisor-remote' protocol though, so let's forget about it, and just
    +    use "field", "field name" and "field value" instead.
    +
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
      ## promisor-remote.c ##
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     -			       struct strvec *names,
     -			       struct strvec *urls)
     +/*
    -+ * Linked list for promisor remotes.
    ++ * Linked list for promisor remotes involved in the "promisor-remote"
    ++ * protocol capability.
    ++ *
    ++ * 'fields' contains a defined set of field name/value pairs for
    ++ * each promisor remote. Field names are stored in the 'string'
    ++ * member, and values in the 'util' member.
    ++ *
    ++ * Currently supported field names:
    ++ * - "name": The name of the promisor remote.
    ++ * - "url": The URL of the promisor remote.
    ++ *
    ++ * Except for "name", each "<field_name>/<field_value>" pair should
    ++ * correspond to a "remote.<name>.<field_name>" config variable set to
    ++ * <field_value> where "<name>" is a promisor remote name.
     + *
     + * 'fields' should not be sorted, as we will rely on the order we put
     + * things into it. So, for example, 'string_list_append()' should be
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +	struct string_list fields;
     +};
     +
    -+static void free_info_list(struct promisor_info *p)
    ++static void promisor_info_list_free(struct promisor_info *p)
     +{
     +	struct promisor_info *next;
     +
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     +			struct promisor_info *new_info = xcalloc(1, sizeof(*new_info));
     +
     +			string_list_init_dup(&new_info->fields);
    ++			new_info->fields.cmp = strcasecmp;
     +
     +			string_list_append(&new_info->fields, "name")->util = (char *)r->name;
     +			string_list_append(&new_info->fields, "url")->util = (char *)url;
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     -	struct strvec names = STRVEC_INIT;
     -	struct strvec urls = STRVEC_INIT;
     +	struct promisor_info *info_list;
    -+	struct promisor_info *r, *p;
    ++	struct promisor_info *r;
      
      	git_config_get_bool("promisor.advertise", &advertise_promisors);
      
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      
     -	for (size_t i = 0; i < names.nr; i++) {
     -		if (i)
    -+	for (p = NULL, r = info_list; r; p = r, r = r->next) {
    ++	for (r = info_list; r; r = r->next) {
     +		struct string_list_item *item;
     +		int first = 1;
     +
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      
     -	strvec_clear(&names);
     -	strvec_clear(&urls);
    -+	free_info_list(p);
    ++	promisor_info_list_free(info_list);
      
      	return strbuf_detach(&sb, NULL);
      }
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     -			return i;
     -	return nicks->nr;
     +	for (; p; p = p->next) {
    -+		assert(!strcmp(p->fields.items[0].string, "name"));
    ++		if (strcmp(p->fields.items[0].string, "name"))
    ++			BUG("First field of promisor info should be 'name', but was '%s'.",
    ++			    p->fields.items[0].string);
     +		if (!strcmp(p->fields.items[0].util, nick))
     +			return p;
     +	}
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
     -	if (!strcmp(urls->v[i], remote_url))
     +	if (strcmp(p->fields.items[1].string, "url"))
    -+		BUG("Bad info_list for remote '%s'", remote_name);
    ++		BUG("Bad info_list for remote '%s'.\n"
    ++		    "Second field of promisor info should be 'url', but was '%s'.",
    ++		    remote_name, p->fields.items[1].string);
     +
     +	local_url = p->fields.items[1].util;
     +
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      
     -	strvec_clear(&names);
     -	strvec_clear(&urls);
    -+	free_info_list(info_list);
    ++	promisor_info_list_free(info_list);
      	strbuf_list_free(remotes);
      }
      
3:  f8b875d1a7 ! 2:  d6d4f99768 promisor-remote: allow a server to advertise extra fields
    @@ Metadata
     Author: Christian Couder <chriscool@tuxfamily.org>
     
      ## Commit message ##
    -    promisor-remote: allow a server to advertise extra fields
    +    promisor-remote: allow a server to advertise more fields
     
         For now the "promisor-remote" protocol capability can only pass "name"
         and "url" information from a server to a client in the form
         "name=<remote_name>,url=<remote_url>".
     
         Let's make it possible to pass more information by introducing a new
    -    "promisor.sendExtraFields" configuration variable. This variable
    -    should contain a comma or space separated list of fields that will be
    -    looked up in the configuration of the remote on the server to find the
    -    values that will be passed to the client.
    +    "promisor.sendFields" configuration variable. This variable should
    +    contain a comma or space separated list of fields that will be looked
    +    up in the configuration of the remote on the server to find the values
    +    that will be passed to the client.
     
    -    For example if "promisor.sendExtraFields" is set to "partialCloneFilter",
    +    Only a set of predefined fields are allowed. The only fields in this
    +    set are "partialCloneFilter" and "token".
    +
    +    For example if "promisor.sendFields" is set to "partialCloneFilter",
         and the server has the "remote.<name>.partialCloneFilter" config
         variable set to a value for a remote, then that value will be passed
         in the form "partialCloneFilter=<value>" after the "name" and "url"
         fields.
     
    -    A following commit will allow the client to use the extra information
    -    to decide if it accepts the remote or not. For now the client doesn't
    -    do anything with the extra information it receives.
    +    A following commit will allow the client to use the information to
    +    decide if it accepts the remote or not. For now the client doesn't do
    +    anything with the additional information it receives.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ Documentation/config/promisor.adoc: promisor.advertise::
      	"false", which means the "promisor-remote" capability is not
      	advertised.
      
    -+promisor.sendExtraFields::
    ++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]. When a field named "bar" is part of
    -+	this list and a corresponding "remote.foo.bar" config variable
    -+	is set on the server to a non empty value, for example "baz",
    -+	then the field and its value, so "bar=baz", will be sent when
    -+	advertising the promisor remote "foo". This list has no effect
    -+	unless the "promisor.advertise" config variable, see above, is
    -+	set to "true", and if that's the case, then whatever this list
    -+	contains, the "name" and "url" fields are advertised anyway
    -+	and contain the remote name and URL respectively, so there is
    -+	no need to add "name" or "url" to this list.
    ++	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.
    +++
    ++When a field is part of this list and a corresponding
    ++"remote.foo.<field>" config variable is set on the server to a
    ++non-empty value, then the field and its value will be sent when
    ++advertising the promisor remote "foo". This list has no effect unless
    ++the "promisor.advertise" config variable is set to "true", and the
    ++"name" and "url" fields are always advertised regardless of this
    ++setting.
     +
      promisor.acceptFromServer::
      	If set to "all", a client will accept all the promisor remotes
    @@ Documentation/gitprotocol-v2.adoc: retrieving the header from a bundle at the in
     +	pr-info = pr-fields | pr-info ";" pr-info
      
     -	pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
    -+	pr-fields = fld-key "=" fld-value | pr-fields "," pr-fields
    ++	pr-fields = fld-name "=" fld-value | pr-fields "," pr-fields
      
     -where `pr-name` is the urlencoded name of a promisor remote, and
     -`pr-url` the urlencoded URL of that promisor remote.
    -+where all the `fld-key` and `fld-value` in a given `pr-fields` are
    -+field keys and values related to a single promisor remote.
    ++where all the `fld-name` and `fld-value` in a given `pr-fields` are
    ++field names and values related to a single promisor remote.
      
     -In this case, if the client decides to use one or more promisor
     -remotes the server advertised, it can reply with
     -"promisor-remote=<pr-names>" where <pr-names> should be of the form:
    -+The server MUST advertise at least the "name" and "url" field keys
    ++The server MUST advertise at least the "name" and "url" field names
     +along with the associated field values, which are the name of a valid
     +remote and its URL, in each `pr-fields`.
      
     -	pr-names = pr-name | pr-names ";" pr-name
    -+`fld-key` MUST start with an alphabetic character and contain only
    -+alphanumeric or '-' characters, and `fld-value` MUST be urlencoded.
    ++The server MAY advertise the following optional fields:
    ++
    ++- "partialCloneFilter": Filter used for partial clone, corresponding
    ++  to the "remote.<name>.partialCloneFilter" config setting.
    ++- "token": Authentication token for the remote, corresponding
    ++  to the "remote.<name>.token" config setting.
    ++
    ++No other fields are defined by the protocol at this time. Clients SHOULD
    ++ignore fields they don't recognize to allow for future protocol extensions.
    ++
    ++For now, the client can only use information transmitted through these
    ++fields to decide if it accepts the advertised promisor remote. In the
    ++future that information might be used for other purposes though.
    ++
    ++Field values MUST be urlencoded.
     +
     +If the client decides to use one or more promisor remotes the server
     +advertised, it can reply with "promisor-remote=<pr-names>" where
    @@ Documentation/gitprotocol-v2.adoc: In this case, or if the client doesn't want t
     -The "promisor.advertise" and "promisor.acceptFromServer" configuration
     -options can be used on the server and client side to control what they
     -advertise or accept respectively. See the documentation of these
    --configuration options for more information.
    -+On the server side, the "promisor.advertise" and
    -+"promisor.sendExtraFields" configuration options can be used to
    -+control what it advertises. On the client side, the
    -+"promisor.acceptFromServer" configuration option can be used to
    -+control what it accepts. See the documentation of these configuration
    -+options for more information.
    ++On the server side, the "promisor.advertise" and "promisor.sendFields"
    ++configuration options can be used to control what it advertises. On
    ++the client side, the "promisor.acceptFromServer" configuration option
    ++can be used to control what it accepts. See the documentation of these
    + configuration options for more information.
      
      Note that in the future it would be nice if the "promisor-remote"
    - protocol capability could be used by the server, when responding to
     
      ## promisor-remote.c ##
     @@ promisor-remote.c: static int allow_unsanitized(char ch)
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
      }
      
     +/*
    -+ * A valid extra field "foo" should correspond to a
    -+ * "remote.<name>.foo" config variable, so, like config variables
    -+ * keys, it should start with an alphabetic character and otherwise
    -+ * each character should satisfy is_config_key_char().
    ++ * List of field names allowed to be used in the "promisor-remote"
    ++ * protocol capability. Each field should correspond to a configurable
    ++ * property of a remote that can be relevant for the client.
    ++ */
    ++static const char *allowed_fields[] = {
    ++	"partialCloneFilter", /* Filter used for partial clone */
    ++	"token",              /* Authentication token for the remote */
    ++	NULL
    ++};
    ++
    ++/*
    ++ * Check if 'field' is in the list of allowed field names for the
    ++ * "promisor-remote" protocol capability.
     + */
    -+static int valid_extra_field(struct string_list_item *item, void *cb_data)
    ++static int is_allowed_field(const char *field)
    ++{
    ++	const char **p;
    ++
    ++	for (p = allowed_fields; *p; p++)
    ++		if (!strcasecmp(*p, field))
    ++			return 1;
    ++	return 0;
    ++}
    ++
    ++static int valid_field(struct string_list_item *item, void *cb_data)
     +{
     +	const char *field = item->string;
     +	const char *config_key = (const char *)cb_data;
     +
    -+	for (size_t i = 0; field[i]; i++)
    -+		if (i ? !is_config_key_char(field[i]) : !isalpha(field[i])) {
    -+			warning(_("invalid field '%s' in '%s' config"), field, config_key);
    -+			return 0;
    -+		}
    ++	if (!is_allowed_field(field)) {
    ++		warning(_("unsupported field '%s' in '%s' config"), field, config_key);
    ++		return 0;
    ++	}
     +	return 1;
     +}
     +
     +static char *fields_from_config(struct string_list *fields_list, const char *config_key)
     +{
    -+	char *extras = NULL;
    ++	char *fields = NULL;
     +
    -+	if (!git_config_get_string(config_key, &extras) && *extras) {
    -+		string_list_split_in_place(fields_list, extras, ", ", -1);
    -+		filter_string_list(fields_list, 0, valid_extra_field, (void *)config_key);
    ++	if (!git_config_get_string(config_key, &fields) && *fields) {
    ++		string_list_split_in_place(fields_list, fields, ", ", -1);
    ++		filter_string_list(fields_list, 0, valid_field, (void *)config_key);
     +	}
     +
    -+	return extras;
    ++	return fields;
     +}
     +
    -+static struct string_list *extra_fields_sent(void)
    ++static struct string_list *fields_sent(void)
     +{
     +	static struct string_list fields_list = STRING_LIST_INIT_NODUP;
     +	static int initialized = 0;
     +
     +	if (!initialized) {
    -+		fields_from_config(&fields_list, "promisor.sendExtraFields");
    ++		fields_list.cmp = strcasecmp;
    ++		fields_from_config(&fields_list, "promisor.sendFields");
     +		initialized = 1;
     +	}
     +
     +	return &fields_list;
     +}
     +
    -+static void append_extra_fields(struct string_list *fields,
    -+				struct string_list *extra_fields,
    -+				const char *name)
    ++static void append_fields(struct string_list *fields,
    ++			  struct string_list *field_names,
    ++			  const char *name)
     +{
     +	struct string_list_item *item;
     +
    -+	for_each_string_list_item(item, extra_fields) {
    ++	for_each_string_list_item(item, field_names) {
     +		char *key = xstrfmt("remote.%s.%s", name, item->string);
     +		const char *val;
     +		if (!git_config_get_string_tmp(key, &val) && *val)
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +}
     +
      /*
    -  * Linked list for promisor remotes.
    +  * Linked list for promisor remotes involved in the "promisor-remote"
    +  * protocol capability.
    +@@ promisor-remote.c: static int allow_unsanitized(char ch)
    +  * member, and values in the 'util' member.
    +  *
    +  * Currently supported field names:
    +- * - "name": The name of the promisor remote.
    +- * - "url": The URL of the promisor remote.
    ++ * - "name": The name of the promisor remote,
    ++ * - "url": The URL of the promisor remote,
    ++ * - the fields in 'allowed_fields[]' above.
       *
    -@@ promisor-remote.c: static void free_info_list(struct promisor_info *p)
    +  * Except for "name", each "<field_name>/<field_value>" pair should
    +  * correspond to a "remote.<name>.<field_name>" config variable set to
    +@@ promisor-remote.c: static void promisor_info_list_free(struct promisor_info *p)
       * remotes. For each promisor remote, some of its fields, starting
       * with "name" and "url", are put in the 'fields' string_list.
       */
     -static struct promisor_info *promisor_info_list(struct repository *repo)
     +static struct promisor_info *promisor_info_list(struct repository *repo,
    -+						struct string_list *extra_fields)
    ++						struct string_list *field_names)
      {
      	struct promisor_info *infos = NULL;
      	struct promisor_info **last_info = &infos;
    @@ promisor-remote.c: static struct promisor_info *promisor_info_list(struct reposi
      			string_list_append(&new_info->fields, "name")->util = (char *)r->name;
      			string_list_append(&new_info->fields, "url")->util = (char *)url;
      
    -+			if (extra_fields)
    -+				append_extra_fields(&new_info->fields, extra_fields, r->name);
    ++			if (field_names)
    ++				append_fields(&new_info->fields, field_names, r->name);
     +
      			*last_info = new_info;
      			last_info = &new_info->next;
    @@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
      		return NULL;
      
     -	info_list = promisor_info_list(repo);
    -+	info_list = promisor_info_list(repo, extra_fields_sent());
    ++	info_list = promisor_info_list(repo, fields_sent());
      
      	if (!info_list)
      		return NULL;
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
      	check_missing_objects server 1 "$oid"
      '
      
    -+test_expect_success "clone with promisor.sendExtraFields" '
    ++test_expect_success "clone with promisor.sendFields" '
     +	git -C server config promisor.advertise true &&
     +	test_when_finished "rm -rf client" &&
     +
     +	git -C server remote add otherLop "https://invalid.invalid"  &&
    -+	git -C server config remote.otherLop.id "fooBar" &&
    ++	git -C server config remote.otherLop.token "fooBar" &&
     +	git -C server config remote.otherLop.stuff "baz" &&
     +	git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
     +	test_when_finished "git -C server remote remove otherLop" &&
    -+	git -C server config promisor.sendExtraFields "id, partialCloneFilter" &&
    -+	test_when_finished "git -C server config unset promisor.sendExtraFields" &&
    ++	git -C server config promisor.sendFields "token, partialCloneFilter" &&
    ++	test_when_finished "git -C server config unset promisor.sendFields" &&
     +	test_when_finished "rm trace" &&
     +
     +	# Clone from server to create a client
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
     +		-c promisor.acceptfromserver=All \
     +		--no-local --filter="blob:limit=5k" server client &&
     +
    -+	# Check that extra fields are properly transmitted
    ++	# Check that fields are properly transmitted
     +	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
     +	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
    -+	PR2="name=otherLop,url=https://invalid.invalid,id=fooBar,partialCloneFilter=blob:limit=10k" &&
    ++	PR2="name=otherLop,url=https://invalid.invalid,token=fooBar,partialCloneFilter=blob:limit=10k" &&
     +	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
     +	test_grep "clone> promisor-remote=lop;otherLop" trace &&
     +
4:  d3b09c1afe ! 3:  09af0369a6 promisor-remote: allow a client to check extra fields
    @@ Metadata
     Author: Christian Couder <chriscool@tuxfamily.org>
     
      ## Commit message ##
    -    promisor-remote: allow a client to check extra fields
    +    promisor-remote: allow a client to check fields
     
    -    A previous commit allowed a server to pass extra fields through the
    -    "promisor-remote" protocol capability after the "name" and "url"
    -    fields.
    +    A previous commit allowed a server to pass additional fields through
    +    the "promisor-remote" protocol capability after the "name" and "url"
    +    fields, specifically the "partialCloneFilter" and "token" fields.
     
    -    Let's make it possible for a client to check if these extra fields
    -    match what it expects before accepting a promisor remote.
    +    Let's make it possible for a client to check if these fields match
    +    what it expects before accepting a promisor remote.
     
    -    We allow this by introducing a new "promisor.checkExtraFields"
    -    configuration variable. It should contain a comma or space
    -    separated list of fields that will be checked.
    +    We allow this by introducing a new "promisor.checkFields"
    +    configuration variable. It should contain a comma or space separated
    +    list of fields that will be checked.
    +
    +    By limiting the protocol to specific well-defined fields, we ensure
    +    both server and client have a shared understanding of field
    +    semantics and usage.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
      	to "fetch" and "clone" requests from the client. Name and URL
      	comparisons are case sensitive. See linkgit:gitprotocol-v2[5].
     +
    -+promisor.checkExtraFields::
    ++promisor.checkFields::
     +	A comma or space separated list of additional remote related
     +	fields that a client will check before accepting a promisor
    -+	remote. When a field named "bar" is part of this list and a
    -+	corresponding "remote.foo.bar" config variable is set for
    -+	locally for remote "foo", then the value of the
    -+	"remote.foo.bar" config variable will be checked against the
    -+	value of the "bar" field passed by the server through the
    -+	"promisor-remote" capability for the remote "foo". The remote
    -+	"foo" will be rejected if the values don't match. The fields
    -+	should be passed by the server through the "promisor-remote"
    -+	capability by using the `promisor.sendExtraFields` config
    -+	variable, see above. The extra fields will be checked only if
    -+	the `promisor.acceptFromServer` config variable, see above, is
    -+	not set to "None". If is set to "None", this config variable
    -+	will have no effect. See linkgit:gitprotocol-v2[5].
    ++	remote. Currently, only the "partialCloneFilter" and "token"
    ++	fields are supported.
    +++
    ++When a field is part of this list and a corresponding
    ++"remote.foo.<field>" config variable is set locally for remote "foo",
    ++then the value of this config variable will be checked against the
    ++value of the same field passed by the server for the remote "foo". The
    ++remote "foo" will be rejected if the values don't match.
    +++
    ++For the "partialCloneFilter" field, this allows the client to ensure
    ++that the server's filter matches what it expects locally, preventing
    ++inconsistencies in filtering behavior. For the "token" field, this can
    ++be used to verify that authentication credentials match expected
    ++values.
    +++
    ++The fields should be passed by the server through the
    ++"promisor-remote" capability by using the `promisor.sendFields` config
    ++variable. The fields will be checked only if the
    ++`promisor.acceptFromServer` config variable is not set to "None".  If
    ++set to "None", this config variable will have no effect.  See
    ++linkgit:gitprotocol-v2[5].
     
      ## promisor-remote.c ##
    -@@ promisor-remote.c: static struct string_list *extra_fields_sent(void)
    +@@ promisor-remote.c: static struct string_list *fields_sent(void)
      	return &fields_list;
      }
      
    -+static struct string_list *extra_fields_checked(void)
    ++static struct string_list *fields_checked(void)
     +{
     +	static struct string_list fields_list = STRING_LIST_INIT_NODUP;
     +	static int initialized = 0;
     +
     +	if (!initialized) {
    -+		fields_from_config(&fields_list, "promisor.checkExtraFields");
    ++		fields_list.cmp = strcasecmp;
    ++		fields_from_config(&fields_list, "promisor.checkFields");
     +		initialized = 1;
     +	}
     +
     +	return &fields_list;
     +}
     +
    - static void append_extra_fields(struct string_list *fields,
    - 				struct string_list *extra_fields,
    - 				const char *name)
    + static void append_fields(struct string_list *fields,
    + 			  struct string_list *field_names,
    + 			  const char *name)
     @@ promisor-remote.c: enum accept_promisor {
      	ACCEPT_ALL
      };
      
    -+static int check_extra_field_one(struct string_list_item *item_value,
    -+				 struct promisor_info *p)
    ++static int check_field_one(struct string_list_item *item_value,
    ++			   struct promisor_info *p)
     +{
     +	struct string_list_item *item;
     +
    @@ promisor-remote.c: enum accept_promisor {
     +}
     +
     +
    -+static int check_extra_field(struct string_list_item *item_value,
    -+			     struct promisor_info *p, int in_list)
    ++static int check_field(struct string_list_item *item_value,
    ++		       struct promisor_info *p, int in_list)
     +{
     +	if (!in_list)
    -+		return check_extra_field_one(item_value, p);
    ++		return check_field_one(item_value, p);
     +
     +	for (; p; p = p->next)
    -+		if (check_extra_field_one(item_value, p))
    ++		if (check_field_one(item_value, p))
     +			return 1;
     +
     +	return 0;
     +}
     +
    -+static int check_all_extra_fields(struct string_list* extra_values,
    -+				  struct promisor_info *p,
    -+				  int in_list)
    ++static int check_all_fields(struct string_list* values,
    ++			    struct promisor_info *p,
    ++			    int in_list)
     +{
    -+	struct string_list* fields_checked = extra_fields_checked();
    ++	struct string_list* fields = fields_checked();
     +	struct string_list_item *item_checked;
     +
    -+	string_list_sort(extra_values);
    ++	string_list_sort(values);
     +
    -+	for_each_string_list_item(item_checked, fields_checked) {
    ++	for_each_string_list_item(item_checked, fields) {
     +		struct string_list_item *item_value;
     +
    -+		item_value = string_list_lookup(extra_values, item_checked->string);
    ++		item_value = string_list_lookup(values, item_checked->string);
     +		if (!item_value)
     +			return 0;
    -+		if (!check_extra_field(item_value, p, in_list))
    ++		if (!check_field(item_value, p, in_list))
     +			return 0;
     +	}
     +
    @@ promisor-remote.c: enum accept_promisor {
     -				const char *remote_name, const char *remote_url,
     +				const char *remote_name,
     +				const char *remote_url,
    -+				struct string_list* extras,
    ++				struct string_list* values,
      				struct promisor_info *info_list)
      {
      	struct promisor_info *p;
    @@ promisor-remote.c: enum accept_promisor {
      
      	if (accept == ACCEPT_ALL)
     -		return 1;
    -+		return check_all_extra_fields(extras, info_list, 1);
    ++		return check_all_fields(values, info_list, 1);
      
      	p = remote_nick_find(info_list, remote_name);
      
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
      	if (accept == ACCEPT_KNOWN_NAME)
     -		return 1;
    -+		return check_all_extra_fields(extras, p, 0);
    ++		return check_all_fields(values, p, 0);
      
      	if (accept != ACCEPT_KNOWN_URL)
      		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
      	if (!strcmp(local_url, remote_url))
     -		return 1;
    -+		return check_all_extra_fields(extras, p, 0);
    ++		return check_all_fields(values, p, 0);
      
      	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
      		remote_name, local_url, remote_url);
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      		const char *remote_url = NULL;
      		char *decoded_name = NULL;
      		char *decoded_url = NULL;
    -+		struct string_list extras = STRING_LIST_INIT_NODUP;
    ++		struct string_list field_values = STRING_LIST_INIT_NODUP;
    ++
    ++		field_values.cmp = strcasecmp;
      
      		strbuf_strip_suffix(remotes[i], ";");
      		elems = strbuf_split(remotes[i], ',');
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
     +			p = strchr(elems[j]->buf, '=');
     +			if (p) {
     +				*p = '\0';
    -+				string_list_append(&extras, elems[j]->buf)->util = p + 1;
    ++				string_list_append(&field_values, elems[j]->buf)->util = p + 1;
     +			} else {
     +				warning(_("invalid element '%s' from remote info"),
     +					elems[j]->buf);
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
     -			strvec_push(accepted, decoded_name);
     +		if (decoded_name) {
     +			if (!info_list)
    -+				info_list = promisor_info_list(repo, extra_fields_checked());
    ++				info_list = promisor_info_list(repo, fields_checked());
     +
     +			if (should_accept_remote(accept, decoded_name, decoded_url,
    -+						 &extras, info_list))
    ++						 &field_values, info_list))
     +				strvec_push(accepted, decoded_name);
     +		}
      
    -+		string_list_clear(&extras, 0);
    ++		string_list_clear(&field_values, 0);
      		strbuf_list_free(elems);
      		free(decoded_name);
      		free(decoded_url);
     
      ## t/t5710-promisor-remote-capability.sh ##
    -@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.sendExtraFields" '
    +@@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.sendFields" '
      	check_missing_objects server 1 "$oid"
      '
      
    -+test_expect_success "clone with promisor.checkExtraFields" '
    ++test_expect_success "clone with promisor.checkFields" '
     +	git -C server config promisor.advertise true &&
     +	test_when_finished "rm -rf client" &&
     +
     +	git -C server remote add otherLop "https://invalid.invalid"  &&
    -+	git -C server config remote.otherLop.id "fooBar" &&
    ++	git -C server config remote.otherLop.token "fooBar" &&
     +	git -C server config remote.otherLop.stuff "baz" &&
     +	git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
     +	test_when_finished "git -C server remote remove otherLop" &&
    -+	git -C server config promisor.sendExtraFields "id, partialCloneFilter" &&
    -+	test_when_finished "git -C server config unset promisor.sendExtraFields" &&
    ++	git -C server config promisor.sendFields "token, partialCloneFilter" &&
    ++	test_when_finished "git -C server config unset promisor.sendFields" &&
     +	test_when_finished "rm trace" &&
     +
     +	# Clone from server to create a client
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
     +		-c remote.lop.url="file://$(pwd)/lop" \
     +		-c remote.lop.partialCloneFilter="blob:none" \
     +		-c promisor.acceptfromserver=All \
    -+		-c promisor.checkExtraFields=partialCloneFilter \
    ++		-c promisor.checkFields=partialcloneFilter \
     +		--no-local --filter="blob:limit=5k" server client &&
     +
    -+	# Check that extra fields are properly transmitted
    ++	# Check that fields are properly transmitted
     +	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
     +	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
    -+	PR2="name=otherLop,url=https://invalid.invalid,id=fooBar,partialCloneFilter=blob:limit=10k" &&
    ++	PR2="name=otherLop,url=https://invalid.invalid,token=fooBar,partialCloneFilter=blob:limit=10k" &&
     +	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
     +	test_grep "clone> promisor-remote=lop" trace &&
     +	test_grep ! "clone> promisor-remote=lop;otherLop" trace &&

Christian Couder (3):
  promisor-remote: refactor to get rid of 'struct strvec'
  promisor-remote: allow a server to advertise more fields
  promisor-remote: allow a client to check fields

 Documentation/config/promisor.adoc    |  43 ++++
 Documentation/gitprotocol-v2.adoc     |  52 ++--
 promisor-remote.c                     | 328 ++++++++++++++++++++++----
 t/t5710-promisor-remote-capability.sh |  67 ++++++
 4 files changed, 425 insertions(+), 65 deletions(-)

In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url.

For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case.

Let's refactor this and introduce a new 'struct promisor_info' linked list that contains a 'struct string_list fields'. This string_list stores the field names, like "name" and "url", in the 'string' member of its items, and the field values in the 'util' member of its items.

Except for "name", each "<field_name>/<field_value>" pair should correspond to a "remote.<name>.<field_name>" config variable set to <field_value> where "<name>" is a promisor remote name.

Previously in Git, the part after the last dot in a configuration variable key, for example "c" for "a.b.c", was called the "variable name part" of a configuration key. It would be very confusing to use "variable name part" or some similar terms in the context of the 'promisor-remote' protocol though, so let's forget about it, and just use "field", "field name" and "field value" instead.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 142 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 105 insertions(+), 37 deletions(-)

On Tue, Apr 29, 2025 at 04:52:41PM +0200, Christian Couder wrote:

Now that we have a restricted set of accepted fields, wouldn't it be easier to store those as individual members of this struct directly?

[snip]

If so, we could also simplify code like this because we wouldn't have to assume any indices anymore.

Patrick

On Wed, May 7, 2025 at 10:25 AM Patrick Steinhardt <ps@pks.im> wrote:

I have done so in the v3. The struct now looks like this at the end of the series:

struct promisor_info {
    struct promisor_info *next;
    const char *name;
    const char *url;
    const char *filter;
    const char *token;
};

Yeah, it simplifies things, but on the other hand in some places the fields cannot be handled in a generic way, so we have things like:

        if (!strcmp(elem, promisor_field_name))
            info->name = value;
        else if (!strcmp(elem, promisor_field_url))
            info->url = value;
        else if (!strcasecmp(elem, promisor_field_filter))
            info->filter = value;
        else if (!strcasecmp(elem, promisor_field_token))
            info->token = value;

Anyway I think that it's not too bad.

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

[snip]

Dumb question: As I read through the patch, I realized we really care about the order of the fields and it is mentioned here too. Why is the order important? Shouldn't the client be satisfied as long as the required fields are present? Or is this merely an implementation issue where we simply parse the information in a specific order?

On Wed, May 7, 2025 at 2:27 PM Karthik Nayak <karthik.188@gmail.com> wrote:

We used to only care about the order for the first 2 mandatory fields ("name" and "url"), but this is relaxed in the v3.

Anyway I think it's better if the 2 mandatory fields appear first as in many cases it might theoretically enable the client to reject an advertised remote as soon as it has parsed the first 2 fields. So the client could stop parsing the rest of the remote information in some cases which might reduce its workload a bit and speed things up.

Yeah, I don't think it matters now, and we can take care of this if it ever matters in the future. But if it ever matters, we might be happy to have specified for a long time that "name" and "url" should appear first.

For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=<remote_name>,url=<remote_url>".

Let's make it possible to pass more information by introducing a new "promisor.sendFields" configuration variable. This variable should contain a comma or space separated list of fields that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client.

Only a set of predefined fields are allowed. The only fields in this set are "partialCloneFilter" and "token".

For example if "promisor.sendFields" is set to "partialCloneFilter", and the server has the "remote.<name>.partialCloneFilter" config variable set to a value for a remote, then that value will be passed in the form "partialCloneFilter=<value>" after the "name" and "url" fields.

A following commit will allow the client to use the information to decide if it accepts the remote or not. For now the client doesn't do anything with the additional information it receives.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    | 18 +++++
 Documentation/gitprotocol-v2.adoc     | 52 +++++++++-----
 promisor-remote.c                     | 99 ++++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh | 32 +++++++++
 4 files changed, 175 insertions(+), 26 deletions(-)

On Tue, Apr 29, 2025 at 04:52:42PM +0200, Christian Couder wrote:

Should we maybe convert this into a list of accepted fields? Makes it easier to extend going forward.

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?

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.

Tiny nit, but can we maybe spell out "fld" fully? It doesn't buy us that much to abbreviate "field", and it did cause my reading to trip.

I think we should define semantics of these fields more closely. What exactly is the consequence of a partial clone filter being defined? 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)? Does it mean that the remote only contains objects that would've been filtered _out_ by such a filter?

Furthermore, we should specify how the token is supposed to be passed to the remote.

Shouldn't we require clients to ignore unknown fields? Otherwise, if it's only optional to ignore them, we still can't introduce new fields in the future without breaking existing clients that chose to ignore this guidance.

Nit: it is a bit funny that we talk about allowed fields here, but
the recommendation is to just ignore unknown fields. So maybe this
should instead be called "known_fields".

Patrick

On Wed, May 7, 2025 at 10:25 AM Patrick Steinhardt <ps@pks.im> wrote:

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.

Maybe I don't understand what you mean with "accepted fields".

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).

Fine, I have done this in v3.

It's just that the client will know what filter the server uses to access the promisor remote. In a later patch in this series the client will be able to decide to accept the remote or not based on that information (depending on whether the filter matches the filter already configured on the client side). The client will still not use the information for other things than deciding to accept or not the remote after this patch series.

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.

The filter specification in general is always about what is filtered out from the repo accessing the promisor remote (not about what is filtered out from the promisor remote). Again this is not specific to this patch series. This is general partial clone information since partial clone has been developed a long time ago.

First for now the token is not used for other things than deciding if the client accepts the remote or not. (Same thing as for the filter, url, name...) I think it can already be useful in some cases.

Now, if we want to talk about a future patch series when the token is possibly used, fine, let's do it. Please consider the following:

  - If the token is passed to the remote through a remote helper, then
it's up to the remote helper to pass and use it, and it could do so in
many different ways that we can't anticipate and maybe we will not
necessarily know about them (for privately developed and used remote
helpers for example).

  - If we develop a standard way in Git to use such a token, then we
will know about that standard way's use of the token at that time (not
necessarily about how remote helpers in the wild might use it though)
. But to develop such a standard way, it's better to already have such
a token passed.

  - Suppose we require this standard way to be developed alongside a
patch series such as this one which passes a token. The main issue is
that this would prevent remote helpers from using such a token until
that standard way is developed. And for now we don't really know if
such a standard way is actually needed or if people will only or
mostly use remote helpers.

  - What if this standard way to use a token is actually not needed
because people use very different backends for large objects with very
different remote helpers developed by different people?

  - There are not many technical similarities between code that passes
a token and code that possibly creates it and then uses it. So I don't
think there is anything technically that requires us to do these
things in the same patch series.

  - There might be a catch 22 between a patch series such as this one
and a patch series to develop a standard way to use a token. We might
not accept patch series adding a token like this one because the token
is not used yet, while a patch series to develop a standard way to
access remote helpers using a token cannot work or even be developed
because no token is passed yet.

So I strongly suggest accepting that we pass a token right now, and not wait for possible ways to use it (other than deciding if the remote is accepted or not).

If 'token' is not put into the promisor.sendFields config variable by servers, the only cost is that clients might check if 'token' is passed when servers advertise their promisor remotes. It's a very small cost to enable something likely very much needed and important for security reasons.

In v3 this is now:

"No other fields are defined by the protocol at this time. Clients MUST ignore fields they don't recognize to allow for future protocol extensions."

With "MUST" instead of "SHOULD", we now require clients to ignore unknown fields, so they shouldn't break if we introduce new fields.

Fine, it's now 'known_fields' in v3.

Thanks for the review!

On Mon, May 19, 2025 at 04:11:18PM +0200, Christian Couder wrote:

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.

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.

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

[snip]

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

Patrick Steinhardt <ps@pks.im> writes:

Excellent.

... which suggests the use of that word needs to be tightened a bit to save the next person from getting confused just like you did, perhaps?

As long as we can extend later, starting small is always good. We can do "promisor.<name>.sendFields" later, for example.

On Tue, May 27, 2025 at 9:51 AM Patrick Steinhardt <ps@pks.im> wrote:

Yeah, the doc uses a bulleted list in v4.

I see. I have tried to use "field names" instead of "fields" everywhere it makes sense in the v4.

Ok, I have added the following to the cover letter of the v4 about this:

"Note that the filter mechanism already exists for a long time and this series doesn't change how it works. For example, it has already been possible for a long time to have different repos using the same promisor remote with different filters. See the existing partial clone documentation (like "Documentation/technical/partial-clone.adoc") for more information on partial clone."

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

[snip]

From this, it seems like the order of the fields shouldn't matter, but this is not the case. wouldn't it be better to say:

  pr-infos = pr-info | pr-infos ";" pr-info

  pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
  pr-info = pr-info | pr-info "," fld-name "=" fld-value

[snip]

Nit: Shouldn't this be `is_valid_field` similar to `is_allowed_field`?

Nit: Can't we just inline this?

Nit: Here too, can't this be inlined? While the modularity is nice, I'm
not sure the redirection is warranted for such small functions with very
specific usecases.

[snip]

Apart from the nits, the patch looks good :)

On Wed, May 7, 2025 at 2:45 PM Karthik Nayak <karthik.188@gmail.com> wrote:

I would prefer to keep the simpler version as I find these grammar notations not very easy to understand.

In v3 there is:

=> The "name" and "url" fields MUST appear first in each pr-fields, in that order.

And I think it's enough, especially since the parsing is relaxed so it will work if they are not in this order.

Could this be interpreted to mean that it is Ok to have "name=" pr-name "," "url=" pr-url several times in a single pr-info?

This seems to me really more complex than needed, and I think it's better to keep the simple version and then clarify things with the above sentence.

Yeah, I have renamed it `is_valid_field`

We could, but I think the is_allowed_field() makes sense on its own too, while the current one is very specific to be used in filter_string_list(). So I prefer keeping is_allowed_field() separate.

In a follow up patch in this series we reuse fields_from_config() so I think it's better to keep it separate.

Thanks for the review!

A previous commit allowed a server to pass additional fields through the "promisor-remote" protocol capability after the "name" and "url" fields, specifically the "partialCloneFilter" and "token" fields.

Let's make it possible for a client to check if these fields match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkFields" configuration variable. It should contain a comma or space separated list of fields that will be checked.

By limiting the protocol to specific well-defined fields, we ensure both server and client have a shared understanding of field semantics and usage.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  25 ++++++
 promisor-remote.c                     | 105 +++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh |  35 +++++++++
 3 files changed, 154 insertions(+), 11 deletions(-)

On Tue, Apr 29, 2025 at 04:52:43PM +0200, Christian Couder wrote:

One thought that came to my mind is that inevitably, users will eventually want to specify different conditions and combinations. E.g. "accept a promisor remote if it's announced by GitLab and if the partial filter strips blobs, but not if it requires additional authentication". I don't think that "checkFields" would be able to implement such a use case.

What is the vision where we want to end up here? Should we maybe provide some more flexibility now already so that we don't have to retrofit such a mechanism in the future?

Patrick

On Wed, May 7, 2025 at 1:14 PM Patrick Steinhardt <ps@pks.im> wrote:

I'd prefer to wait until some concrete cases appear. It's possible that the current very simple mechanism will be enough for some time, or it's possible that some users will come soon with a complex use case where it might be necessary to have a hook or some kind of external script called. In those cases it would be sad if we added some flexibility in advance but it appears to be too much or not enough.

On Tue, Apr 29, 2025 at 4:53 PM Christian Couder <christian.couder@gmail.com> wrote:

Sorry, I should have removed the above paragraph as now the field names are compared case insensitively.

The "promisor-remote" capability can only be used to pass the names and URLs of the promisor remotes from the server to the client. After that the client can use this information to decide if it accepts the remotes or not.

It would be nice if the server could pass more fields about its remotes and if the client could use that additional information to decide about the remotes by comparing it with its local information about the remotes.

This patch series implements this by adding the "promisor.sendFields" on the server side and the "promisor.checkFields" on the client side.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the remote "foo" configured like this:

[remote "foo"]
        url = file:///tmp/foo.git
	partialCloneFilter = blob:none

then "name=foo,url=file:///tmp/foo.git,partialCloneFilter=blob:none" will be sent by the server for this remote.

All the information passed through the "promisor-remote" capability is still only used to decide if the remotes are accepted or not. The client doesn't store it and doesn't use it for any other purpose.

The fields that can be passed are limited to "partialCloneFilter" and "token".

On the technical side, we get rid of 'struct strvec' and we use a 'struct promisor_info' linked list and some 'struct string_list' instead. This matches some suggestions made when the series that introduced the "promisor-remote" capability was reviewed.

Changes since v2 ----------------

Thanks to Patrick, Junio and Karthik for their comments on the previous versions.

- A big change affecting all the patches is that instead of a linked
  list, the 'struct promisor_info' now contains a `const char *`
  member for each field (name, url, token, ...) that can be passed
  throught the "promisor-remote" capability.

- In patch 1/5, the commit message has been simplified a bit as
  'struct promisor_info' doesn't contain a 'struct string_list fields'
  anymore, and to void repeating information that is already in some
  code comments.

- In patch 1/5, the promisor_info_list() function has been renamed
  promisor_config_info_list() to make it clear that it populates the
  'struct promisor_info' it returns with information from the
  config. For the same purpose, the `info_list` variable used to point
  to the list returned by that function has been renamed
  `config_info`.

- In patch 2/5, there are a few documentation changes:

  - "fld-name" and "fld-value" have been renamed "field-name" and
    "field-value" respectively for clarity,

  - some information about what the "partialCloneFilter" and "token"
    fields should be used for has been provided,

  - it's stated that the "name" and "url" fields MUST appear first in
    each pr-fields, in that order,

  - it's stated that clients MUST ignore fields they don't recognize.

- In patch 2/5, in the code, there are the following changes:

  - string constants are used instead of repeating the
    "partialCloneFilter" and "token" literal strings,

  - `allowed_fields` and `is_allowed_field()` have been renamed
    `known_fields` and `is_known_field()` respectively,

  - `valid_field()` has been renamed `is_valid_field()`

  - `append_fields()` has been replaced by `set_one_field()` and
    `set_fields()`.

- In patch 2/5, the test has been adjusted to account for the fact
  that fields are now processed in a different order due to the
  changes in 'struct promisor_info'.

- Patch 3/5 ("promisor-remote: refactor how we parse advertised
  fields") is a new refactoring patch that prepares for the next
  patch. It refactors into a new parse_one_advertised_remote()
  function how remote information advertised by the server is
  parsed. Instead of parsing it into variables, we now parse it into a
  `struct promisor_info`.

- Patch 4/5 used to be patch 3/3 in v2. In the documentation we now
  cleary state under which condition having fields listed in
  "promisor.checkFields" results in an accepted remote.

- In patch 4/5, in the code, the main change is that the
  `check_field_one()`, `check_field()` and `check_all_fields()`
  functions have been replaced by the `match_field_against_config()`
  and `all_fields_match()` functions.

- In patch 4/5, the test has also been adjusted to account for the
  fact that fields are now processed in a different order due to the
  changes in 'struct promisor_info'.

- Patch 5/5 ("promisor-remote: use string constants for 'name' and
  'url' too") is a new clean up patch. It uses string constants for
  "name" and "url" to match the fact that they are used for
  "partialCloneFilter" and "token" since patch 2/5.

CI tests --------

Here they are:

https://github.com/chriscool/git/actions/runs/15108739921

They all seem to have passed.

Range diff compared to v2 -------------------------

This range diff doesn't detect that patch 4/5 in v3 was patch 3/3 in v2 as there were a lot of changes. It might be useful for the first 2 patches (1/5 and 2/5) though.

1:  d4e12afbc1 ! 1:  12e753a358 promisor-remote: refactor to get rid of 'struct strvec'
    @@ Commit message
         scale easily in that case.
     
         Let's refactor this and introduce a new 'struct promisor_info' linked
    -    list that contains a 'struct string_list fields'. This string_list
    -    stores the field names, like "name" and "url", in the 'string' member
    -    of its items, and the field values in the 'util' member of its items.
    +    list which for now only contains a 'next' pointer, a 'name' member for
    +    the promisor remote name and an 'url' member for its URL.
     
    -    Except for "name", each "<field_name>/<field_value>" pair should
    -    correspond to a "remote.<name>.<field_name>" config variable set to
    -    <field_value> where "<name>" is a promisor remote name.
    -
    -    Previously in Git, the part after the last dot in a configuration
    -    variable key, for example "c" for "a.b.c", was called the "variable
    -    name part" of a configuration key. It would be very confusing to use
    -    "variable name part" or some similar terms in the context of the
    -    'promisor-remote' protocol though, so let's forget about it, and just
    -    use "field", "field name" and "field value" instead.
    +    Explicit members are used within 'struct promisor_info' for type
    +    safety and clarity regarding the specific information being handled,
    +    rather than a generic key-value store. We want to specify and document
    +    each field and its content, so adding new members to the struct as
    +    more fields are supported is fine.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     + * Linked list for promisor remotes involved in the "promisor-remote"
     + * protocol capability.
     + *
    -+ * 'fields' contains a defined set of field name/value pairs for
    -+ * each promisor remote. Field names are stored in the 'string'
    -+ * member, and values in the 'util' member.
    -+ *
    -+ * Currently supported field names:
    -+ * - "name": The name of the promisor remote.
    -+ * - "url": The URL of the promisor remote.
    -+ *
    -+ * Except for "name", each "<field_name>/<field_value>" pair should
    -+ * correspond to a "remote.<name>.<field_name>" config variable set to
    -+ * <field_value> where "<name>" is a promisor remote name.
    -+ *
    -+ * 'fields' should not be sorted, as we will rely on the order we put
    -+ * things into it. So, for example, 'string_list_append()' should be
    -+ * used instead of 'string_list_insert()'.
    ++ * Except for "next" and "name", each <member> in this struct and its
    ++ * <value> should correspond to a "remote.<name>.<member>" config
    ++ * variable set to <value> where "<name>" is a promisor remote name.
     + */
     +struct promisor_info {
     +	struct promisor_info *next;
    -+	struct string_list fields;
    ++	const char *name;
    ++	const char *url;
     +};
     +
     +static void promisor_info_list_free(struct promisor_info *p)
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +
     +	for (; p; p = next) {
     +		next = p->next;
    -+		string_list_clear(&p->fields, 0);
    ++		free((char *)p->name);
    ++		free((char *)p->url);
     +		free(p);
     +	}
     +}
     +
    -+/*
    -+ * Prepare a 'struct promisor_info' linked list of promisor
    -+ * remotes. For each promisor remote, some of its fields, starting
    -+ * with "name" and "url", are put in the 'fields' string_list.
    -+ */
    -+static struct promisor_info *promisor_info_list(struct repository *repo)
    ++/* Prepare a 'struct promisor_info' linked list with config information. */
    ++static struct promisor_info *promisor_config_info_list(struct repository *repo)
      {
     +	struct promisor_info *infos = NULL;
     +	struct promisor_info **last_info = &infos;
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     -			strvec_push(urls, url);
     +			struct promisor_info *new_info = xcalloc(1, sizeof(*new_info));
     +
    -+			string_list_init_dup(&new_info->fields);
    -+			new_info->fields.cmp = strcasecmp;
    -+
    -+			string_list_append(&new_info->fields, "name")->util = (char *)r->name;
    -+			string_list_append(&new_info->fields, "url")->util = (char *)url;
    ++			new_info->name = xstrdup(r->name);
    ++			new_info->url = xstrdup(url);
     +
     +			*last_info = new_info;
     +			last_info = &new_info->next;
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      	int advertise_promisors = 0;
     -	struct strvec names = STRVEC_INIT;
     -	struct strvec urls = STRVEC_INIT;
    -+	struct promisor_info *info_list;
    -+	struct promisor_info *r;
    ++	struct promisor_info *config_info;
    ++	struct promisor_info *p;
      
      	git_config_get_bool("promisor.advertise", &advertise_promisors);
      
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      		return NULL;
      
     -	promisor_info_vecs(repo, &names, &urls);
    -+	info_list = promisor_info_list(repo);
    ++	config_info = promisor_config_info_list(repo);
      
     -	if (!names.nr)
    -+	if (!info_list)
    ++	if (!config_info)
      		return NULL;
      
     -	for (size_t i = 0; i < names.nr; i++) {
     -		if (i)
    -+	for (r = info_list; r; r = r->next) {
    -+		struct string_list_item *item;
    -+		int first = 1;
    -+
    -+		if (r != info_list)
    ++	for (p = config_info; p; p = p->next) {
    ++		if (p != config_info)
      			strbuf_addch(&sb, ';');
    --		strbuf_addstr(&sb, "name=");
    ++
    + 		strbuf_addstr(&sb, "name=");
     -		strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
    --		strbuf_addstr(&sb, ",url=");
    ++		strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized);
    + 		strbuf_addstr(&sb, ",url=");
     -		strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized);
    -+
    -+		for_each_string_list_item(item, &r->fields) {
    -+			if (first)
    -+				first = 0;
    -+			else
    -+				strbuf_addch(&sb, ',');
    -+			strbuf_addf(&sb, "%s=", item->string);
    -+			strbuf_addstr_urlencode(&sb, (char *)item->util, allow_unsanitized);
    -+		}
    ++		strbuf_addstr_urlencode(&sb, p->url, allow_unsanitized);
      	}
      
     -	strvec_clear(&names);
     -	strvec_clear(&urls);
    -+	promisor_info_list_free(info_list);
    ++	promisor_info_list_free(config_info);
      
      	return strbuf_detach(&sb, NULL);
      }
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     - * Find first index of 'nicks' where there is 'nick'. 'nick' is
     - * compared case sensitively to the strings in 'nicks'. If not found
     - * 'nicks->nr' is returned.
    -+ * Find first element of 'p' where the 'name' field is 'nick'. 'nick'
    ++ * Find first element of 'p' where the 'name' member is 'nick'. 'nick'
     + * is compared case sensitively to the strings in 'p'. If not found
     + * NULL is returned.
       */
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     -			return i;
     -	return nicks->nr;
     +	for (; p; p = p->next) {
    -+		if (strcmp(p->fields.items[0].string, "name"))
    -+			BUG("First field of promisor info should be 'name', but was '%s'.",
    -+			    p->fields.items[0].string);
    -+		if (!strcmp(p->fields.items[0].util, nick))
    ++		if (!strcmp(p->name, nick))
     +			return p;
     +	}
     +	return NULL;
    @@ promisor-remote.c: enum accept_promisor {
      static int should_accept_remote(enum accept_promisor accept,
      				const char *remote_name, const char *remote_url,
     -				struct strvec *names, struct strvec *urls)
    -+				struct promisor_info *info_list)
    ++				struct promisor_info *config_info)
      {
     -	size_t i;
     +	struct promisor_info *p;
    -+	const char *local_url;
      
      	if (accept == ACCEPT_ALL)
      		return 1;
      
     -	i = remote_nick_find(names, remote_name);
    -+	p = remote_nick_find(info_list, remote_name);
    ++	p = remote_nick_find(config_info, remote_name);
      
     -	if (i >= names->nr)
     +	if (!p)
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	}
      
     -	if (!strcmp(urls->v[i], remote_url))
    -+	if (strcmp(p->fields.items[1].string, "url"))
    -+		BUG("Bad info_list for remote '%s'.\n"
    -+		    "Second field of promisor info should be 'url', but was '%s'.",
    -+		    remote_name, p->fields.items[1].string);
    -+
    -+	local_url = p->fields.items[1].util;
    ++	if (!p->url)
    ++		BUG("Bad config_info (invalid URL) for remote '%s'.\n",
    ++		    remote_name);
     +
    -+	if (!strcmp(local_url, remote_url))
    ++	if (!strcmp(p->url, remote_url))
      		return 1;
      
      	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
     -		remote_name, urls->v[i], remote_url);
    -+		remote_name, local_url, remote_url);
    ++		remote_name, p->url, remote_url);
      
      	return 0;
      }
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      	enum accept_promisor accept = ACCEPT_NONE;
     -	struct strvec names = STRVEC_INIT;
     -	struct strvec urls = STRVEC_INIT;
    -+	struct promisor_info *info_list = NULL;
    ++	struct promisor_info *config_info = NULL;
      
      	if (!git_config_get_string_tmp("promisor.acceptfromserver", &accept_str)) {
      		if (!*accept_str || !strcasecmp("None", accept_str))
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      
      	if (accept != ACCEPT_ALL)
     -		promisor_info_vecs(repo, &names, &urls);
    -+		info_list = promisor_info_list(repo);
    ++		config_info = promisor_config_info_list(repo);
      
      	/* Parse remote info received */
      
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      			decoded_url = url_percent_decode(remote_url);
      
     -		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &names, &urls))
    -+		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, info_list))
    ++		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, config_info))
      			strvec_push(accepted, decoded_name);
      
      		strbuf_list_free(elems);
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      
     -	strvec_clear(&names);
     -	strvec_clear(&urls);
    -+	promisor_info_list_free(info_list);
    ++	promisor_info_list_free(config_info);
      	strbuf_list_free(remotes);
      }
      
2:  d6d4f99768 ! 2:  29c6104827 promisor-remote: allow a server to advertise more fields
    @@ Commit message
         that will be passed to the client.
     
         Only a set of predefined fields are allowed. The only fields in this
    -    set are "partialCloneFilter" and "token".
    +    set are "partialCloneFilter" and "token". The "partialCloneFilter"
    +    field specifies the filter definition used by the promisor remote,
    +    and the "token" field can provide an authentication credential for
    +    accessing it.
     
         For example if "promisor.sendFields" is set to "partialCloneFilter",
         and the server has the "remote.<name>.partialCloneFilter" config
    @@ Documentation/gitprotocol-v2.adoc: retrieving the header from a bundle at the in
     +	pr-info = pr-fields | pr-info ";" pr-info
      
     -	pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
    -+	pr-fields = fld-name "=" fld-value | pr-fields "," pr-fields
    ++	pr-fields = field-name "=" field-value | pr-fields "," pr-fields
      
     -where `pr-name` is the urlencoded name of a promisor remote, and
     -`pr-url` the urlencoded URL of that promisor remote.
    -+where all the `fld-name` and `fld-value` in a given `pr-fields` are
    -+field names and values related to a single promisor remote.
    ++where all the `field-name` and `field-value` in a given `pr-fields`
    ++are field names and values related to a single promisor remote.
      
     -In this case, if the client decides to use one or more promisor
     -remotes the server advertised, it can reply with
     -"promisor-remote=<pr-names>" where <pr-names> should be of the form:
     +The server MUST advertise at least the "name" and "url" field names
     +along with the associated field values, which are the name of a valid
    -+remote and its URL, in each `pr-fields`.
    ++remote and its URL, in each `pr-fields`. The "name" and "url" fields
    ++MUST appear first in each pr-fields, in that order.
      
     -	pr-names = pr-name | pr-names ";" pr-name
    -+The server MAY advertise the following optional fields:
    ++After these mandatory fields, the server MAY advertise the following
    ++optional fields in any order:
     +
    -+- "partialCloneFilter": Filter used for partial clone, corresponding
    -+  to the "remote.<name>.partialCloneFilter" config setting.
    -+- "token": Authentication token for the remote, corresponding
    -+  to the "remote.<name>.token" config setting.
    ++- "partialCloneFilter": The filter specification used by the remote.
    ++Clients can use this to determine if the remote's filtering strategy
    ++is compatible with their needs (e.g., checking if both use "blob:none").
    ++It corresponds to the "remote.<name>.partialCloneFilter" config setting.
     +
    -+No other fields are defined by the protocol at this time. Clients SHOULD
    -+ignore fields they don't recognize to allow for future protocol extensions.
    ++- "token": An authentication token that clients can use when
    ++connecting to the remote. It corresponds to the "remote.<name>.token"
    ++config setting.
    ++
    ++No other fields are defined by the protocol at this time. Clients MUST
    ++ignore fields they don't recognize to allow for future protocol
    ++extensions.
     +
     +For now, the client can only use information transmitted through these
     +fields to decide if it accepts the advertised promisor remote. In the
    @@ Documentation/gitprotocol-v2.adoc: retrieving the header from a bundle at the in
     -remote name, and the ';' and ',' characters MUST be encoded if they
     -appear in `pr-name` or `pr-url`.
     +Note that, everywhere in this document, the ';' and ',' characters
    -+MUST be encoded if they appear in `pr-name` or `fld-value`.
    ++MUST be encoded if they appear in `pr-name` or `field-value`.
      
      If the server doesn't know any promisor remote that could be good for
      a client to use, or prefers a client not to use any promisor remote it
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
      	return ch > 32 && ch < 127;
      }
      
    ++static const char promisor_field_filter[] = "partialCloneFilter";
    ++static const char promisor_field_token[] = "token";
    ++
     +/*
    -+ * List of field names allowed to be used in the "promisor-remote"
    -+ * protocol capability. Each field should correspond to a configurable
    -+ * property of a remote that can be relevant for the client.
    ++ * List of optional field names that can be used in the
    ++ * "promisor-remote" protocol capability (others must be
    ++ * ignored). Each field should correspond to a configurable property
    ++ * of a remote that can be relevant for the client.
     + */
    -+static const char *allowed_fields[] = {
    -+	"partialCloneFilter", /* Filter used for partial clone */
    -+	"token",              /* Authentication token for the remote */
    ++static const char *known_fields[] = {
    ++	promisor_field_filter, /* Filter used for partial clone */
    ++	promisor_field_token,  /* Authentication token for the remote */
     +	NULL
     +};
     +
     +/*
    -+ * Check if 'field' is in the list of allowed field names for the
    ++ * Check if 'field' is in the list of the known field names for the
     + * "promisor-remote" protocol capability.
     + */
    -+static int is_allowed_field(const char *field)
    ++static int is_known_field(const char *field)
     +{
     +	const char **p;
     +
    -+	for (p = allowed_fields; *p; p++)
    ++	for (p = known_fields; *p; p++)
     +		if (!strcasecmp(*p, field))
     +			return 1;
     +	return 0;
     +}
     +
    -+static int valid_field(struct string_list_item *item, void *cb_data)
    ++static int is_valid_field(struct string_list_item *item, void *cb_data)
     +{
     +	const char *field = item->string;
     +	const char *config_key = (const char *)cb_data;
     +
    -+	if (!is_allowed_field(field)) {
    ++	if (!is_known_field(field)) {
     +		warning(_("unsupported field '%s' in '%s' config"), field, config_key);
     +		return 0;
     +	}
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +
     +	if (!git_config_get_string(config_key, &fields) && *fields) {
     +		string_list_split_in_place(fields_list, fields, ", ", -1);
    -+		filter_string_list(fields_list, 0, valid_field, (void *)config_key);
    ++		filter_string_list(fields_list, 0, is_valid_field, (void *)config_key);
     +	}
     +
     +	return fields;
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +	return &fields_list;
     +}
     +
    -+static void append_fields(struct string_list *fields,
    -+			  struct string_list *field_names,
    -+			  const char *name)
    + /*
    +  * Linked list for promisor remotes involved in the "promisor-remote"
    +  * protocol capability.
    +@@ promisor-remote.c: struct promisor_info {
    + 	struct promisor_info *next;
    + 	const char *name;
    + 	const char *url;
    ++	const char *filter;
    ++	const char *token;
    + };
    + 
    + static void promisor_info_list_free(struct promisor_info *p)
    +@@ promisor-remote.c: static void promisor_info_list_free(struct promisor_info *p)
    + 		next = p->next;
    + 		free((char *)p->name);
    + 		free((char *)p->url);
    ++		free((char *)p->filter);
    ++		free((char *)p->token);
    + 		free(p);
    + 	}
    + }
    + 
    +-/* Prepare a 'struct promisor_info' linked list with config information. */
    +-static struct promisor_info *promisor_config_info_list(struct repository *repo)
    ++static void set_one_field(struct promisor_info *p,
    ++			  const char *field, const char *value)
    ++{
    ++	if (!strcasecmp(field, promisor_field_filter))
    ++		p->filter = xstrdup(value);
    ++	else if (!strcasecmp(field, promisor_field_token))
    ++		p->token = xstrdup(value);
    ++	else
    ++		BUG("Invalid field '%s'", field);
    ++}
    ++
    ++static void set_fields(struct promisor_info *p,
    ++		       struct string_list *field_names)
     +{
     +	struct string_list_item *item;
     +
     +	for_each_string_list_item(item, field_names) {
    -+		char *key = xstrfmt("remote.%s.%s", name, item->string);
    ++		char *key = xstrfmt("remote.%s.%s", p->name, item->string);
     +		const char *val;
     +		if (!git_config_get_string_tmp(key, &val) && *val)
    -+			string_list_append(fields, item->string)->util = (char *)val;
    ++			set_one_field(p, item->string, val);
     +		free(key);
     +	}
     +}
     +
    - /*
    -  * Linked list for promisor remotes involved in the "promisor-remote"
    -  * protocol capability.
    -@@ promisor-remote.c: static int allow_unsanitized(char ch)
    -  * member, and values in the 'util' member.
    -  *
    -  * Currently supported field names:
    -- * - "name": The name of the promisor remote.
    -- * - "url": The URL of the promisor remote.
    -+ * - "name": The name of the promisor remote,
    -+ * - "url": The URL of the promisor remote,
    -+ * - the fields in 'allowed_fields[]' above.
    -  *
    -  * Except for "name", each "<field_name>/<field_value>" pair should
    -  * correspond to a "remote.<name>.<field_name>" config variable set to
    -@@ promisor-remote.c: static void promisor_info_list_free(struct promisor_info *p)
    -  * remotes. For each promisor remote, some of its fields, starting
    -  * with "name" and "url", are put in the 'fields' string_list.
    -  */
    --static struct promisor_info *promisor_info_list(struct repository *repo)
    -+static struct promisor_info *promisor_info_list(struct repository *repo,
    -+						struct string_list *field_names)
    ++/*
    ++ * Prepare a 'struct promisor_info' linked list of promisor remotes
    ++ * with config information. Only members of that struct specified by
    ++ * the 'field_names' linked list are set (using values from the
    ++ * configuration).
    ++ */
    ++static struct promisor_info *promisor_config_info_list(struct repository *repo,
    ++						       struct string_list *field_names)
      {
      	struct promisor_info *infos = NULL;
      	struct promisor_info **last_info = &infos;
    -@@ promisor-remote.c: static struct promisor_info *promisor_info_list(struct repository *repo)
    - 			string_list_append(&new_info->fields, "name")->util = (char *)r->name;
    - 			string_list_append(&new_info->fields, "url")->util = (char *)url;
    +@@ promisor-remote.c: static struct promisor_info *promisor_config_info_list(struct repository *repo)
    + 			new_info->name = xstrdup(r->name);
    + 			new_info->url = xstrdup(url);
      
     +			if (field_names)
    -+				append_fields(&new_info->fields, field_names, r->name);
    ++				set_fields(new_info, field_names);
     +
      			*last_info = new_info;
      			last_info = &new_info->next;
    @@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
      	if (!advertise_promisors)
      		return NULL;
      
    --	info_list = promisor_info_list(repo);
    -+	info_list = promisor_info_list(repo, fields_sent());
    +-	config_info = promisor_config_info_list(repo);
    ++	config_info = promisor_config_info_list(repo, fields_sent());
      
    - 	if (!info_list)
    + 	if (!config_info)
      		return NULL;
    +@@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
    + 		strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized);
    + 		strbuf_addstr(&sb, ",url=");
    + 		strbuf_addstr_urlencode(&sb, p->url, allow_unsanitized);
    ++
    ++		if (p->filter) {
    ++			strbuf_addf(&sb, ",%s=", promisor_field_filter);
    ++			strbuf_addstr_urlencode(&sb, p->filter, allow_unsanitized);
    ++		}
    ++		if (p->token) {
    ++			strbuf_addf(&sb, ",%s=", promisor_field_token);
    ++			strbuf_addstr_urlencode(&sb, p->token, allow_unsanitized);
    ++		}
    + 	}
    + 
    + 	promisor_info_list_free(config_info);
     @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      		return;
      
      	if (accept != ACCEPT_ALL)
    --		info_list = promisor_info_list(repo);
    -+		info_list = promisor_info_list(repo, NULL);
    +-		config_info = promisor_config_info_list(repo);
    ++		config_info = promisor_config_info_list(repo, NULL);
      
      	/* Parse remote info received */
      
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
     +	git -C server config remote.otherLop.stuff "baz" &&
     +	git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
     +	test_when_finished "git -C server remote remove otherLop" &&
    -+	git -C server config promisor.sendFields "token, partialCloneFilter" &&
    ++	git -C server config promisor.sendFields "partialCloneFilter, token" &&
     +	test_when_finished "git -C server config unset promisor.sendFields" &&
     +	test_when_finished "rm trace" &&
     +
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
     +	# Check that fields are properly transmitted
     +	ENCODED_URL=$(echo "file://$(pwd)/lop" | sed -e "s/ /%20/g") &&
     +	PR1="name=lop,url=$ENCODED_URL,partialCloneFilter=blob:none" &&
    -+	PR2="name=otherLop,url=https://invalid.invalid,token=fooBar,partialCloneFilter=blob:limit=10k" &&
    ++	PR2="name=otherLop,url=https://invalid.invalid,partialCloneFilter=blob:limit=10k,token=fooBar" &&
     +	test_grep "clone< promisor-remote=$PR1;$PR2" trace &&
     +	test_grep "clone> promisor-remote=lop;otherLop" trace &&
     +
3:  09af0369a6 < -:  ---------- promisor-remote: allow a client to check fields
-:  ---------- > 3:  6d8236bf70 promisor-remote: refactor how we parse advertised fields
-:  ---------- > 4:  dab5823356 promisor-remote: allow a client to check fields
-:  ---------- > 5:  9bd612cdb8 promisor-remote: use string constants for 'name' and 'url' too

Christian Couder (5):
  promisor-remote: refactor to get rid of 'struct strvec'
  promisor-remote: allow a server to advertise more fields
  promisor-remote: refactor how we parse advertised fields
  promisor-remote: allow a client to check fields
  promisor-remote: use string constants for 'name' and 'url' too

 Documentation/config/promisor.adoc    |  53 ++++
 Documentation/gitprotocol-v2.adoc     |  59 ++--
 promisor-remote.c                     | 383 +++++++++++++++++++++-----
 t/t5710-promisor-remote-capability.sh |  67 +++++
 4 files changed, 479 insertions(+), 83 deletions(-)

In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url.

For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case.

Let's refactor this and introduce a new 'struct promisor_info' linked list which for now only contains a 'next' pointer, a 'name' member for the promisor remote name and an 'url' member for its URL.

Explicit members are used within 'struct promisor_info' for type safety and clarity regarding the specific information being handled, rather than a generic key-value store. We want to specify and document each field and its content, so adding new members to the struct as more fields are supported is fine.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 107 +++++++++++++++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 35 deletions(-)

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

[snip]

Nit: while we're here wouldn't be nicer to rename this to
`promiser_info_list_find_name` or similar?

[snip]

On Tue, May 20, 2025 at 11:37 AM Karthik Nayak <karthik.188@gmail.com> wrote:

Junio suggested this name in a discussion of a previous patch series:

https://lore.kernel.org/git/xmqqa5bbq0nb.fsf@gitster.g/

I am not sure that changing its first argument from a `struct strvec *` to a `struct promisor_info *` makes it worth renaming. If that's the case, then I would be fine with renaming it like you suggest though.

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

Don't blame me for that name ;-)

The name was for a hypothetical variant that took "struct strvec *" as its first parameter, and the name was given only because it did not make much sense to call the helper after "strvec". A function name that signals that we are finding (something) using "nick"-name was a much better choice. Since your final one finds in "struct promisor_info *", not a generic "struct strvec *", I wouldn't be surprised if a name that is about promisor-info (whatever it is) more clearly describes what it does. If this is a file-local helper, promisor_info_find() or find_promisor_info() may be sufficient, if "by nickname" is the primary and only way for the application to find an instance of promisor_info.

Is promisor_info primarily be found by their names before its other members are accessed? If so, I wonder if a strmap or string_list that uses the nickname as the key with a pointer to a promisor_info structure as the data more appropriate than a hand-crafted linear linked list.

Thanks.

On Tue, May 20, 2025 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:

OK, I think I will use promisor_info_find() or promisor_info_find_by_name() in the next reroll, as it goes well with how other functions related to "struct promisor_info *" are named.

No, there are different ways the promisor_info instances are iterated on or searched, see promisor_remote_info() and all_fields_match() where we iterate on all the instances and then either use the instance members to generate an advertisement string, or check if some of the members match advertised values.

I don't think it would bring a lot of benefits. Using an strmap or a sorted string list might make things faster if there are a lot of promisor remotes configured on the clients, but I don't think we are at a point where such an optimisation is worth it.

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

What I was getting at using common collection types instead of rolling your own linked list was not primarily about performance. They are more battle-tested and much easier to readers who are familiar with these existing types.

On Wed, May 21, 2025 at 5:00 PM Junio C Hamano <gitster@pobox.com> wrote:

OK, I have implemented the string_list approach you suggested that uses the nickname as the key (item->string) with a pointer to a promisor_info structure as the data (item->util) in the v4. The string_list is sorted when it can be useful.

Also remote_nick_find() has been renamed promisor_info_find().

For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=<remote_name>,url=<remote_url>".

Let's make it possible to pass more information by introducing a new "promisor.sendFields" configuration variable. This variable should contain a comma or space separated list of fields that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client.

Only a set of predefined fields are allowed. The only fields in this set are "partialCloneFilter" and "token". The "partialCloneFilter" field specifies the filter definition used by the promisor remote, and the "token" field can provide an authentication credential for accessing it.

For example if "promisor.sendFields" is set to "partialCloneFilter", and the server has the "remote.<name>.partialCloneFilter" config variable set to a value for a remote, then that value will be passed in the form "partialCloneFilter=<value>" after the "name" and "url" fields.

A following commit will allow the client to use the information to decide if it accepts the remote or not. For now the client doesn't do anything with the additional information it receives.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  18 ++++
 Documentation/gitprotocol-v2.adoc     |  59 ++++++++----
 promisor-remote.c                     | 128 ++++++++++++++++++++++++--
 t/t5710-promisor-remote-capability.sh |  32 +++++++
 4 files changed, 212 insertions(+), 25 deletions(-)

On 25/05/19 04:12PM, Christian Couder wrote:

Ok, so if the server has an unknown sendField value configured, the value is ignored by the server and a warning printed.

We could use `repo_config_get_string()` here instead and wire the nearby `struct repository`, but as the rest of the file is not doing so it's not really a big deal.

Are there scenarios where `fields_sent()` is getting invoked more than once? My understanding is that this is invoked only when the capability is being advertised. Regardless, I wonder if we really need the static initialization here.

This configuration results in the unsupported field warning message being printed. This is because of the SP following the comma resulting in an empty value in the middle after the string split.

I think we could use `test_config` which would automatically unset the configuration after the test.

On Wed, May 21, 2025 at 10:36 PM Justin Tobler <jltobler@gmail.com> wrote:

Yes.

Yeah, removing use of the_repository in this file is left for another future patch series.

I am not sure if `fields_sent()` could be invoked more than once (by the same process), but I think it's very risky to rely on it being called just once, and I think it doesn't cost much to be safe.

Yeah, right, in the v4 I have added a call to string_list_remove_empty_items() to avoid spurious empty values and related warnings.

Yeah, `test_config` is used in v4 to avoid this test_when_finished.

Thanks for your review!

On Mon, May 19, 2025 at 04:12:56PM +0200, Christian Couder wrote:

I think this documentation should be clarified to explicitly talk about "field names". In v2 I misread these paragraphs to mean that the admin is expected to configure name-value pairs because you say "fields" here, and that term is specified elsewhere to be such a pair.

Curious. Why aren't these declared as mere string constants (static const char *)? It might be a bit more idiomatic to have these as all-uppercase defines to make it obvious that those aren't a local variable.

    #define PROMISOR_FIELD_FILTER "partialCloneFilter"
    #define PROMISOR_FIELD_TOKEN  "token"

Yup, this now follows my suggestion.

s/Invalid/invalid/

Patrick

On Tue, May 27, 2025 at 9:51 AM Patrick Steinhardt <ps@pks.im> wrote:

Yeah, right, in the v4, I have clarified the doc here and in other places to talk about "field names" when relevant.

Using "static const char *" is a bit less efficient, as it uses an additional pointer to point to the literal string.

This has been discussed a few times in the past on the mailing list but I can't find any references to such discussions now.

As far as I recall the first discussion I had about this was with Dscho who was in favor of using "static const char []" because it's more type safe than "#define ..." and more efficient than "static const char *".

I have made this change in the v4.

Thanks for your review!

In a follow up commit we are going to parse more fields, like a filter and a token, coming from the server when it advertises promisor remotes using the "promisor-remote" capability.

To prepare for this, let's refactor the code that parses the advertised fields coming from the server into a new parse_one_advertised_remote() function that will populate a `struct promisor_info` with the content of the fields it parsed.

While at it, let's also pass this `struct promisor_info` to the should_accept_remote() function, instead of passing it the parsed name and url.

These changes will make it simpler to both parse more fields and access the content of these parsed fields in follow up commits.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 71 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 21 deletions(-)

A previous commit allowed a server to pass additional fields through the "promisor-remote" protocol capability after the "name" and "url" fields, specifically the "partialCloneFilter" and "token" fields.

Let's make it possible for a client to check if these fields match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkFields" configuration variable. It should contain a comma or space separated list of fields that will be checked.

By limiting the protocol to specific well-defined fields, we ensure both server and client have a shared understanding of field semantics and usage.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    | 35 ++++++++++++
 promisor-remote.c                     | 79 +++++++++++++++++++++++++--
 t/t5710-promisor-remote-capability.sh | 35 ++++++++++++
 3 files changed, 143 insertions(+), 6 deletions(-)

A previous commit started to define `promisor_field_filter` and `promisor_field_token`, and used them instead of the "partialCloneFilter" and "token" string literals.

Let's do the same for "name" and "url" to avoid repeating them several times and for consistency with the other fields.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

The "promisor-remote" capability can only be used to pass the names and URLs of the promisor remotes from the server to the client. After that the client can use this information to decide if it accepts the remotes or not.

It would be nice if the server could pass more fields about its remotes and if the client could use that additional information to decide about the remotes by comparing it with its local information about the remotes.

This patch series implements this by adding the "promisor.sendFields" on the server side and the "promisor.checkFields" on the client side.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the remote "foo" configured like this:

[remote "foo"]
        url = file:///tmp/foo.git
	partialCloneFilter = blob:none

then "name=foo,url=file:///tmp/foo.git,partialCloneFilter=blob:none" will be sent by the server for this remote.

All the information passed through the "promisor-remote" capability is still only used to decide if the remotes are accepted or not. The client doesn't store it and doesn't use it for any other purpose.

Note that the filter mechanism already exists for a long time and this series doesn't change how it works. For example, it has already been possible for a long time to have different repos using the same promisor remote with different filters. See the existing partial clone documentation (like "Documentation/technical/partial-clone.adoc") for more information on partial clone.

The fields that can be passed are limited to "partialCloneFilter" and "token".

On the technical side, we get rid of 'struct strvec' and we use 'struct promisor_info' to store the data and 'struct string_list' to store the 'struct promisor_info' instances instead. This matches the latest suggestion from Junio.

This work is part of the "LOP" effort documented in:

  Documentation/technical/large-object-promisors.adoc

See that doc for more information on the broader context.

Changes since v3 ----------------

Thanks to Patrick, Junio, Karthik and Justin for their comments on the previous versions.

- A big change affecting all the patches is that 'struct string_list'
  is used to store the 'struct promisor_info' instances which don't
  contain a 'next' pointer anymore. The items in 'struct string_list'
  have their 'string' pointer point to the remote name and their
  'util' pointer point to a 'struct promisor_info' instance. The
  'struct string_list' is sorted when useful.

- Another global change is that "field name" is used instead of
  "fields" where it makes sense.

- In patch 1/5, the error message in a BUG() has been improved a bit.

- In patch 2/5, bullet points are used to improve the doc of the new
  "promisor.sendFields" config variable.

- In patch 2/5, a call to string_list_remove_empty_items() has been
  added to avoid issues with empty fields.

- In patch 2/5, test_config is used instead of removing some config
  with `test_when_finished`.

- In patch 4/5, test_config is also used instead of removing some
  config with `test_when_finished`.

CI tests --------

Here they are:

https://github.com/chriscool/git/actions/runs/15585655648

They have all passed.

Range diff compared to v3 -------------------------

1:  12e753a358 ! 1:  8a4df71d2c promisor-remote: refactor to get rid of 'struct strvec'
    @@ Commit message
         with one 'struct strvec' for each field like "name" or "url" does not
         scale easily in that case.
     
    -    Let's refactor this and introduce a new 'struct promisor_info' linked
    -    list which for now only contains a 'next' pointer, a 'name' member for
    -    the promisor remote name and an 'url' member for its URL.
    +    Let's refactor this and introduce a new 'struct promisor_info'.
    +
    +    It will only store promisor remote information in its members. For now
    +    it has only a 'name' member for the promisor remote name and an 'url'
    +    member for its URL. We will use use a 'struct string_list' to store
    +    the instances of 'struct promisor_info'. For each 'item' in the
    +    string_list, 'item->string' will point to the promisor remote name and
    +    'item->util' will point to the corresponding 'struct promisor_info'
    +    instance.
     
         Explicit members are used within 'struct promisor_info' for type
         safety and clarity regarding the specific information being handled,
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     -			       struct strvec *names,
     -			       struct strvec *urls)
     +/*
    -+ * Linked list for promisor remotes involved in the "promisor-remote"
    ++ * Struct for promisor remotes involved in the "promisor-remote"
     + * protocol capability.
     + *
    -+ * Except for "next" and "name", each <member> in this struct and its
    -+ * <value> should correspond to a "remote.<name>.<member>" config
    -+ * variable set to <value> where "<name>" is a promisor remote name.
    ++ * Except for "name", each <member> in this struct and its <value>
    ++ * should correspond (either on the client side or on the server side)
    ++ * to a "remote.<name>.<member>" config variable set to <value> where
    ++ * "<name>" is a promisor remote name.
     + */
     +struct promisor_info {
    -+	struct promisor_info *next;
     +	const char *name;
     +	const char *url;
     +};
     +
    -+static void promisor_info_list_free(struct promisor_info *p)
    ++static void promisor_info_list_clear(struct string_list *list)
     +{
    -+	struct promisor_info *next;
    -+
    -+	for (; p; p = next) {
    -+		next = p->next;
    ++	for (size_t i = 0; i < list->nr; i++) {
    ++		struct promisor_info *p = list->items[i].util;
     +		free((char *)p->name);
     +		free((char *)p->url);
    -+		free(p);
     +	}
    ++	string_list_clear(list, 1);
     +}
     +
    -+/* Prepare a 'struct promisor_info' linked list with config information. */
    -+static struct promisor_info *promisor_config_info_list(struct repository *repo)
    ++/*
    ++ * Populate 'list' with promisor remote information from the config.
    ++ * The 'util' pointer of each list item will hold a 'struct promisor_info'.
    ++ */
    ++static void promisor_config_info_list(struct repository *repo, struct string_list *list)
      {
    -+	struct promisor_info *infos = NULL;
    -+	struct promisor_info **last_info = &infos;
      	struct promisor_remote *r;
      
    - 	promisor_remote_init(repo);
     @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      
      		/* Only add remotes with a non empty URL */
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
     -			strvec_push(names, r->name);
     -			strvec_push(urls, url);
     +			struct promisor_info *new_info = xcalloc(1, sizeof(*new_info));
    ++			struct string_list_item *item;
     +
     +			new_info->name = xstrdup(r->name);
     +			new_info->url = xstrdup(url);
     +
    -+			*last_info = new_info;
    -+			last_info = &new_info->next;
    ++			item = string_list_append(list, new_info->name);
    ++			item->util = new_info;
      		}
      
      		free(url_key);
    - 	}
    -+
    -+	return infos;
    - }
    - 
    - char *promisor_remote_info(struct repository *repo)
    +@@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
      {
      	struct strbuf sb = STRBUF_INIT;
      	int advertise_promisors = 0;
     -	struct strvec names = STRVEC_INIT;
     -	struct strvec urls = STRVEC_INIT;
    -+	struct promisor_info *config_info;
    -+	struct promisor_info *p;
    ++	struct string_list config_info = STRING_LIST_INIT_NODUP;
    ++	struct string_list_item *item;
      
      	git_config_get_bool("promisor.advertise", &advertise_promisors);
      
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      		return NULL;
      
     -	promisor_info_vecs(repo, &names, &urls);
    -+	config_info = promisor_config_info_list(repo);
    ++	promisor_config_info_list(repo, &config_info);
      
     -	if (!names.nr)
    -+	if (!config_info)
    ++	if (!config_info.nr)
      		return NULL;
      
     -	for (size_t i = 0; i < names.nr; i++) {
     -		if (i)
    -+	for (p = config_info; p; p = p->next) {
    -+		if (p != config_info)
    ++	for_each_string_list_item(item, &config_info) {
    ++		struct promisor_info *p = item->util;
    ++
    ++		if (item != config_info.items)
      			strbuf_addch(&sb, ';');
     +
      		strbuf_addstr(&sb, "name=");
    @@ promisor-remote.c: static void promisor_info_vecs(struct repository *repo,
      
     -	strvec_clear(&names);
     -	strvec_clear(&urls);
    -+	promisor_info_list_free(config_info);
    ++	promisor_info_list_clear(&config_info);
      
      	return strbuf_detach(&sb, NULL);
      }
      
    - /*
    +-/*
     - * Find first index of 'nicks' where there is 'nick'. 'nick' is
     - * compared case sensitively to the strings in 'nicks'. If not found
     - * 'nicks->nr' is returned.
    -+ * Find first element of 'p' where the 'name' member is 'nick'. 'nick'
    -+ * is compared case sensitively to the strings in 'p'. If not found
    -+ * NULL is returned.
    -  */
    +- */
     -static size_t remote_nick_find(struct strvec *nicks, const char *nick)
    -+static struct promisor_info *remote_nick_find(struct promisor_info *p, const char *nick)
    - {
    +-{
     -	for (size_t i = 0; i < nicks->nr; i++)
     -		if (!strcmp(nicks->v[i], nick))
     -			return i;
     -	return nicks->nr;
    -+	for (; p; p = p->next) {
    -+		if (!strcmp(p->name, nick))
    -+			return p;
    -+	}
    -+	return NULL;
    - }
    - 
    +-}
    +-
      enum accept_promisor {
    + 	ACCEPT_NONE = 0,
    + 	ACCEPT_KNOWN_URL,
     @@ promisor-remote.c: enum accept_promisor {
      
      static int should_accept_remote(enum accept_promisor accept,
      				const char *remote_name, const char *remote_url,
     -				struct strvec *names, struct strvec *urls)
    -+				struct promisor_info *config_info)
    ++				struct string_list *config_info)
      {
     -	size_t i;
     +	struct promisor_info *p;
    ++	struct string_list_item *item;
      
      	if (accept == ACCEPT_ALL)
      		return 1;
      
     -	i = remote_nick_find(names, remote_name);
    -+	p = remote_nick_find(config_info, remote_name);
    ++	/* Get config info for that promisor remote */
    ++	item = string_list_lookup(config_info, remote_name);
      
     -	if (i >= names->nr)
    -+	if (!p)
    ++	if (!item)
      		/* We don't know about that remote */
      		return 0;
      
    ++	p = item->util;
    ++
    + 	if (accept == ACCEPT_KNOWN_NAME)
    + 		return 1;
    + 
     @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      		return 0;
      	}
      
     -	if (!strcmp(urls->v[i], remote_url))
     +	if (!p->url)
    -+		BUG("Bad config_info (invalid URL) for remote '%s'.\n",
    ++		BUG("bad config_info (invalid URL) for remote '%s'",
     +		    remote_name);
     +
     +	if (!strcmp(p->url, remote_url))
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      	enum accept_promisor accept = ACCEPT_NONE;
     -	struct strvec names = STRVEC_INIT;
     -	struct strvec urls = STRVEC_INIT;
    -+	struct promisor_info *config_info = NULL;
    ++	struct string_list config_info = STRING_LIST_INIT_NODUP;
      
      	if (!git_config_get_string_tmp("promisor.acceptfromserver", &accept_str)) {
      		if (!*accept_str || !strcasecmp("None", accept_str))
     @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
    + 	if (accept == ACCEPT_NONE)
      		return;
      
    - 	if (accept != ACCEPT_ALL)
    +-	if (accept != ACCEPT_ALL)
     -		promisor_info_vecs(repo, &names, &urls);
    -+		config_info = promisor_config_info_list(repo);
    ++	if (accept != ACCEPT_ALL) {
    ++		promisor_config_info_list(repo, &config_info);
    ++		string_list_sort(&config_info);
    ++	}
      
      	/* Parse remote info received */
      
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      			decoded_url = url_percent_decode(remote_url);
      
     -		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &names, &urls))
    -+		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, config_info))
    ++		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &config_info))
      			strvec_push(accepted, decoded_name);
      
      		strbuf_list_free(elems);
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      
     -	strvec_clear(&names);
     -	strvec_clear(&urls);
    -+	promisor_info_list_free(config_info);
    ++	promisor_info_list_clear(&config_info);
      	strbuf_list_free(remotes);
      }
      
2:  29c6104827 ! 2:  8f3111b4f2 promisor-remote: allow a server to advertise more fields
    @@ Commit message
     
         Let's make it possible to pass more information by introducing a new
         "promisor.sendFields" configuration variable. This variable should
    -    contain a comma or space separated list of fields that will be looked
    -    up in the configuration of the remote on the server to find the values
    -    that will be passed to the client.
    +    contain a comma or space separated list of field names that will be
    +    looked up in the configuration of the remote on the server to find the
    +    values that will be passed to the client.
     
         Only a set of predefined fields are allowed. The only fields in this
         set are "partialCloneFilter" and "token". The "partialCloneFilter"
    @@ Commit message
         and the "token" field can provide an authentication credential for
         accessing it.
     
    -    For example if "promisor.sendFields" is set to "partialCloneFilter",
    +    For example, if "promisor.sendFields" is set to "partialCloneFilter",
         and the server has the "remote.<name>.partialCloneFilter" config
         variable set to a value for a remote, then that value will be passed
         in the form "partialCloneFilter=<value>" after the "name" and "url"
    @@ Documentation/config/promisor.adoc: promisor.advertise::
      
     +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.
    ++	field names. A server will send these field names and the
    ++	associated field values from its configuration when
    ++	advertising its promisor remotes using the "promisor-remote"
    ++	capability, see linkgit:gitprotocol-v2[5]. Currently, only the
    ++	"partialCloneFilter" and "token" field names are supported.
     ++
    -+When a field is part of this list and a corresponding
    -+"remote.foo.<field>" config variable is set on the server to a
    -+non-empty value, then the field and its value will be sent when
    -+advertising the promisor remote "foo". This list has no effect unless
    -+the "promisor.advertise" config variable is set to "true", and the
    -+"name" and "url" fields are always advertised regardless of this
    -+setting.
    ++* "partialCloneFilter": contains the partial clone filter
    ++  used for the remote.
    +++
    ++* "token": contains an authentication token for the remote.
    +++
    ++When a field name is part of this list and a corresponding
    ++"remote.foo.<field name>" config variable is set on the server to a
    ++non-empty value, then the field name and value will be sent when
    ++advertising the promisor remote "foo".
    +++
    ++This list has no effect unless the "promisor.advertise" config
    ++variable is set to "true", and the "name" and "url" fields are always
    ++advertised regardless of this setting.
     +
      promisor.acceptFromServer::
      	If set to "all", a client will accept all the promisor remotes
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +
     +	if (!git_config_get_string(config_key, &fields) && *fields) {
     +		string_list_split_in_place(fields_list, fields, ", ", -1);
    ++		string_list_remove_empty_items(fields_list, 0);
     +		filter_string_list(fields_list, 0, is_valid_field, (void *)config_key);
     +	}
     +
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +}
     +
      /*
    -  * Linked list for promisor remotes involved in the "promisor-remote"
    +  * Struct for promisor remotes involved in the "promisor-remote"
       * protocol capability.
    -@@ promisor-remote.c: struct promisor_info {
    - 	struct promisor_info *next;
    +@@ promisor-remote.c: static int allow_unsanitized(char ch)
    + struct promisor_info {
      	const char *name;
      	const char *url;
     +	const char *filter;
     +	const char *token;
      };
      
    - static void promisor_info_list_free(struct promisor_info *p)
    -@@ promisor-remote.c: static void promisor_info_list_free(struct promisor_info *p)
    - 		next = p->next;
    + static void promisor_info_list_clear(struct string_list *list)
    +@@ promisor-remote.c: static void promisor_info_list_clear(struct string_list *list)
    + 		struct promisor_info *p = list->items[i].util;
      		free((char *)p->name);
      		free((char *)p->url);
     +		free((char *)p->filter);
     +		free((char *)p->token);
    - 		free(p);
      	}
    + 	string_list_clear(list, 1);
      }
      
    --/* Prepare a 'struct promisor_info' linked list with config information. */
    --static struct promisor_info *promisor_config_info_list(struct repository *repo)
     +static void set_one_field(struct promisor_info *p,
     +			  const char *field, const char *value)
     +{
    @@ promisor-remote.c: static void promisor_info_list_free(struct promisor_info *p)
     +	else if (!strcasecmp(field, promisor_field_token))
     +		p->token = xstrdup(value);
     +	else
    -+		BUG("Invalid field '%s'", field);
    ++		BUG("invalid field '%s'", field);
     +}
     +
     +static void set_fields(struct promisor_info *p,
    @@ promisor-remote.c: static void promisor_info_list_free(struct promisor_info *p)
     +	}
     +}
     +
    -+/*
    -+ * Prepare a 'struct promisor_info' linked list of promisor remotes
    -+ * with config information. Only members of that struct specified by
    -+ * the 'field_names' linked list are set (using values from the
    -+ * configuration).
    -+ */
    -+static struct promisor_info *promisor_config_info_list(struct repository *repo,
    -+						       struct string_list *field_names)
    + /*
    +  * Populate 'list' with promisor remote information from the config.
    +- * The 'util' pointer of each list item will hold a 'struct promisor_info'.
    ++ * The 'util' pointer of each list item will hold a 'struct
    ++ * promisor_info'. Except "name" and "url", only members of that
    ++ * struct specified by the 'field_names' list are set (using values
    ++ * from the configuration).
    +  */
    +-static void promisor_config_info_list(struct repository *repo, struct string_list *list)
    ++static void promisor_config_info_list(struct repository *repo,
    ++				      struct string_list *list,
    ++				      struct string_list *field_names)
      {
    - 	struct promisor_info *infos = NULL;
    - 	struct promisor_info **last_info = &infos;
    -@@ promisor-remote.c: static struct promisor_info *promisor_config_info_list(struct repository *repo)
    + 	struct promisor_remote *r;
    + 
    +@@ promisor-remote.c: static void promisor_config_info_list(struct repository *repo, struct string_lis
      			new_info->name = xstrdup(r->name);
      			new_info->url = xstrdup(url);
      
     +			if (field_names)
     +				set_fields(new_info, field_names);
     +
    - 			*last_info = new_info;
    - 			last_info = &new_info->next;
    + 			item = string_list_append(list, new_info->name);
    + 			item->util = new_info;
      		}
     @@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
      	if (!advertise_promisors)
      		return NULL;
      
    --	config_info = promisor_config_info_list(repo);
    -+	config_info = promisor_config_info_list(repo, fields_sent());
    +-	promisor_config_info_list(repo, &config_info);
    ++	promisor_config_info_list(repo, &config_info, fields_sent());
      
    - 	if (!config_info)
    + 	if (!config_info.nr)
      		return NULL;
     @@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
      		strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized);
    @@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
     +		}
      	}
      
    - 	promisor_info_list_free(config_info);
    + 	promisor_info_list_clear(&config_info);
     @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      		return;
      
    - 	if (accept != ACCEPT_ALL)
    --		config_info = promisor_config_info_list(repo);
    -+		config_info = promisor_config_info_list(repo, NULL);
    - 
    - 	/* Parse remote info received */
    + 	if (accept != ACCEPT_ALL) {
    +-		promisor_config_info_list(repo, &config_info);
    ++		promisor_config_info_list(repo, &config_info, NULL);
    + 		string_list_sort(&config_info);
    + 	}
      
     @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      		elems = strbuf_split(remotes[i], ',');
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with 'KnownUrl
     +	git -C server config remote.otherLop.stuff "baz" &&
     +	git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
     +	test_when_finished "git -C server remote remove otherLop" &&
    -+	git -C server config promisor.sendFields "partialCloneFilter, token" &&
    -+	test_when_finished "git -C server config unset promisor.sendFields" &&
    ++	test_config -C server promisor.sendFields "partialCloneFilter, token" &&
     +	test_when_finished "rm trace" &&
     +
     +	# Clone from server to create a client
3:  6d8236bf70 ! 3:  c91a1ba04a promisor-remote: refactor how we parse advertised fields
    @@ Commit message
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
      ## promisor-remote.c ##
    +@@ promisor-remote.c: struct promisor_info {
    + 	const char *token;
    + };
    + 
    ++static void promisor_info_free(struct promisor_info *p)
    ++{
    ++	free((char *)p->name);
    ++	free((char *)p->url);
    ++	free((char *)p->filter);
    ++	free((char *)p->token);
    ++	free(p);
    ++}
    ++
    + static void promisor_info_list_clear(struct string_list *list)
    + {
    +-	for (size_t i = 0; i < list->nr; i++) {
    +-		struct promisor_info *p = list->items[i].util;
    +-		free((char *)p->name);
    +-		free((char *)p->url);
    +-		free((char *)p->filter);
    +-		free((char *)p->token);
    +-	}
    +-	string_list_clear(list, 1);
    ++	for (size_t i = 0; i < list->nr; i++)
    ++		promisor_info_free(list->items[i].util);
    ++	string_list_clear(list, 0);
    + }
    + 
    + static void set_one_field(struct promisor_info *p,
     @@ promisor-remote.c: enum accept_promisor {
      };
      
      static int should_accept_remote(enum accept_promisor accept,
     -				const char *remote_name, const char *remote_url,
     +				struct promisor_info *advertised,
    - 				struct promisor_info *config_info)
    + 				struct string_list *config_info)
      {
      	struct promisor_info *p;
    + 	struct string_list_item *item;
     +	const char *remote_name = advertised->name;
     +	const char *remote_url = advertised->url;
      
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
     +	if (!info->name || !info->url) {
     +		warning(_("server advertised a promisor remote without a name or URL: %s"),
     +			remote_info->buf);
    -+		promisor_info_list_free(info);
    ++		promisor_info_free(info);
     +		return NULL;
     +	}
     +
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
     +		if (!advertised)
     +			continue;
      
    --		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, config_info))
    +-		if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &config_info))
     -			strvec_push(accepted, decoded_name);
    -+		if (should_accept_remote(accept, advertised, config_info))
    ++		if (should_accept_remote(accept, advertised, &config_info))
     +			strvec_push(accepted, advertised->name);
      
     -		strbuf_list_free(elems);
     -		free(decoded_name);
     -		free(decoded_url);
    -+		promisor_info_list_free(advertised);
    ++		promisor_info_free(advertised);
      	}
      
    - 	promisor_info_list_free(config_info);
    + 	promisor_info_list_clear(&config_info);
4:  dab5823356 ! 4:  ad68202057 promisor-remote: allow a client to check fields
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
     +
     +promisor.checkFields::
     +	A comma or space separated list of additional remote related
    -+	fields that a client will check before accepting a promisor
    -+	remote. Currently, "partialCloneFilter" and "token" are the only
    -+	supported field names.
    ++	field names. A client will check if the values of these fields
    ++	transmitted by a server correspond to the values of these
    ++	fields in its own configuration before accepting a promisor
    ++	remote. Currently, "partialCloneFilter" and "token" are the
    ++	only supported field names.
     ++
     +If one of these field names (e.g., "token") is being checked for an
     +advertised promisor remote (e.g., "foo"), three conditions must be met
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
     +3. The value of the locally configured `remote.foo.token` must exactly
     +   match the value advertised by the server for the "token" field.
     ++
    -+If any of these conditions are not met for any field name listed in
    ++If any of these conditions is not met for any field name listed in
     +`promisor.checkFields`, the advertised remote "foo" will be rejected.
     ++
     +For the "partialCloneFilter" field, this allows the client to ensure
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
     +be used to verify that authentication credentials match expected
     +values.
     ++
    ++Field names are compared case-insensitively. Field values are compared
    ++case-sensitively.
    +++
     +The "name" and "url" fields are always checked according to the
     +`promisor.acceptFromServer` policy, independently of this setting.
     ++
    -+The fields should be passed by the server through the
    ++The field names and values should be passed by the server through the
     +"promisor-remote" capability by using the `promisor.sendFields` config
     +variable. The fields will be checked only if the
     +`promisor.acceptFromServer` config variable is not set to "None". If
    @@ promisor-remote.c: static struct string_list *fields_sent(void)
     +}
     +
      /*
    -  * Linked list for promisor remotes involved in the "promisor-remote"
    +  * Struct for promisor remotes involved in the "promisor-remote"
       * protocol capability.
     @@ promisor-remote.c: enum accept_promisor {
      	ACCEPT_ALL
    @@ promisor-remote.c: enum accept_promisor {
     +}
     +
     +static int all_fields_match(struct promisor_info *advertised,
    -+			    struct promisor_info *config_info,
    ++			    struct string_list *config_info,
     +			    int in_list)
     +{
     +	struct string_list* fields = fields_checked();
    @@ promisor-remote.c: enum accept_promisor {
     +		int match = 0;
     +		const char *field = item_checked->string;
     +		const char *value = NULL;
    ++		struct string_list_item *item;
     +
     +		if (!strcasecmp(field, promisor_field_filter))
     +			value = advertised->filter;
    @@ promisor-remote.c: enum accept_promisor {
     +			return 0;
     +
     +		if (in_list) {
    -+			for (struct promisor_info *p = config_info; p; p = p->next) {
    ++			for_each_string_list_item(item, config_info) {
    ++				struct promisor_info *p = item->util;
     +				if (match_field_against_config(field, value, p)) {
     +					match = 1;
     +					break;
     +				}
     +			}
     +		} else {
    -+			match = match_field_against_config(field, value, config_info);
    ++			item = string_list_lookup(config_info, advertised->name);
    ++			if (item) {
    ++				struct promisor_info *p = item->util;
    ++				match = match_field_against_config(field, value, p);
    ++			}
     +		}
     +
     +		if (!match)
    @@ promisor-remote.c: enum accept_promisor {
     +
      static int should_accept_remote(enum accept_promisor accept,
      				struct promisor_info *advertised,
    - 				struct promisor_info *config_info)
    + 				struct string_list *config_info)
     @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	const char *remote_url = advertised->url;
      
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
     -		return 1;
     +		return all_fields_match(advertised, config_info, 1);
      
    - 	p = remote_nick_find(config_info, remote_name);
    - 
    + 	/* Get config info for that promisor remote */
    + 	item = string_list_lookup(config_info, remote_name);
     @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
    - 		return 0;
    + 	p = item->util;
      
      	if (accept == ACCEPT_KNOWN_NAME)
     -		return 1;
    -+		return all_fields_match(advertised, p, 0);
    ++		return all_fields_match(advertised, config_info, 0);
      
      	if (accept != ACCEPT_KNOWN_URL)
      		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
      	if (!strcmp(p->url, remote_url))
     -		return 1;
    -+		return all_fields_match(advertised, p, 0);
    ++		return all_fields_match(advertised, config_info, 0);
      
      	warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
      		remote_name, p->url, remote_url);
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      	if (accept == ACCEPT_NONE)
      		return;
      
    --	if (accept != ACCEPT_ALL)
    --		config_info = promisor_config_info_list(repo, NULL);
    +-	if (accept != ACCEPT_ALL) {
    +-		promisor_config_info_list(repo, &config_info, NULL);
    +-		string_list_sort(&config_info);
    +-	}
     -
      	/* Parse remote info received */
      
    @@ promisor-remote.c: static void filter_promisor_remote(struct repository *repo,
      		if (!advertised)
      			continue;
      
    -+		if (!config_info)
    -+			config_info = promisor_config_info_list(repo, fields_checked());
    ++		if (!config_info.nr) {
    ++			promisor_config_info_list(repo, &config_info, fields_checked());
    ++			string_list_sort(&config_info);
    ++		}
     +
    - 		if (should_accept_remote(accept, advertised, config_info))
    + 		if (should_accept_remote(accept, advertised, &config_info))
      			strvec_push(accepted, advertised->name);
      
     
    @@ t/t5710-promisor-remote-capability.sh: test_expect_success "clone with promisor.
     +	git -C server config remote.otherLop.stuff "baz" &&
     +	git -C server config remote.otherLop.partialCloneFilter "blob:limit=10k" &&
     +	test_when_finished "git -C server remote remove otherLop" &&
    -+	git -C server config promisor.sendFields "partialCloneFilter, token" &&
    -+	test_when_finished "git -C server config unset promisor.sendFields" &&
    ++	test_config -C server promisor.sendFields "partialCloneFilter, token" &&
     +	test_when_finished "rm trace" &&
     +
     +	# Clone from server to create a client
5:  9bd612cdb8 ! 5:  e8efe62b7f promisor-remote: use string constants for 'name' and 'url' too
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
      static const char promisor_field_token[] = "token";
      
     @@ promisor-remote.c: char *promisor_remote_info(struct repository *repo)
    - 		if (p != config_info)
    + 		if (item != config_info.items)
      			strbuf_addch(&sb, ';');
      
     -		strbuf_addstr(&sb, "name=");

Christian Couder (5):
  promisor-remote: refactor to get rid of 'struct strvec'
  promisor-remote: allow a server to advertise more fields
  promisor-remote: refactor how we parse advertised fields
  promisor-remote: allow a client to check fields
  promisor-remote: use string constants for 'name' and 'url' too

 Documentation/config/promisor.adoc    |  62 ++++
 Documentation/gitprotocol-v2.adoc     |  59 ++--
 promisor-remote.c                     | 395 +++++++++++++++++++++-----
 t/t5710-promisor-remote-capability.sh |  65 +++++
 4 files changed, 493 insertions(+), 88 deletions(-)

In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url.

For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case.

Let's refactor this and introduce a new 'struct promisor_info'.

It will only store promisor remote information in its members. For now it has only a 'name' member for the promisor remote name and an 'url' member for its URL. We will use use a 'struct string_list' to store the instances of 'struct promisor_info'. For each 'item' in the string_list, 'item->string' will point to the promisor remote name and 'item->util' will point to the corresponding 'struct promisor_info' instance.

Explicit members are used within 'struct promisor_info' for type safety and clarity regarding the specific information being handled, rather than a generic key-value store. We want to specify and document each field and its content, so adding new members to the struct as more fields are supported is fine.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 111 +++++++++++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 41 deletions(-)

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

Nit: It would be nice to mention _why_ it doesn't scale easily here.

The rest of the patch looks good to me.

[snip]

On Thu, Jun 19, 2025 at 1:53 PM Karthik Nayak <karthik.188@gmail.com> wrote:

In v5, I have added the following to better explain it:

"We would need one 'struct strvec' for each new field, and then we would have to pass all these 'struct strvec' around."

[...]

Thanks!

On 25/06/11 03:45PM, Christian Couder wrote:

s/use use/use/

Ok so now all promisor info for a given remote is stored in its own struct. This will enable easier extension in the future to add additional fields.

In this version, each remote is now stored as a member of a `struct string_list` instead of a custom collection type specific to promisor remote info. Nice

nit: Ok in this context, "config_info" is specific to the list of promisor_info not just generic git configuration. Something like "promisor_info_list" would be a bit more explicit, but I don't feel super strongly.

Out of curiousity, is it invalid for the trailing promisor remote entry to end with a ';'? It would be simpler if each entry could just end with a semi-colon.

Ok just to clarify, it is invalid for a promisor remote to not have a URL specified. If so, it might be better to say "empty URL" or something along those lines.

On Mon, Jun 23, 2025 at 9:43 PM Justin Tobler <jltobler@gmail.com> wrote:

Fixed in v5.

[...]

Yeah, it's not generic config, but it's still config, and I think that's important to help understand what the code does when it uses it, so we should keep "config" somehow in this variable name. For now I haven't changed it.

It would work but it's not really valid.

In "Documentation/gitprotocol-v2.adoc" which specifies the protocol, most of the time when different items are transmitted together they are separated by SP like:

item1 SP item2 SP item3 LF

and there is no SP before the LF.

For the "promisor-remote", we need something more complex but for consistency I think it makes sense to not repeat separators at the end, in the same way as SP are not repeated before LF.

Yeah, in v5 it's now "URL is NULL" instead of "invalid URL".

Thanks.

For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=<remote_name>,url=<remote_url>".

Let's make it possible to pass more information by introducing a new "promisor.sendFields" configuration variable. This variable should contain a comma or space separated list of field names that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client.

Only a set of predefined fields are allowed. The only fields in this set are "partialCloneFilter" and "token". The "partialCloneFilter" field specifies the filter definition used by the promisor remote, and the "token" field can provide an authentication credential for accessing it.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the "remote.<name>.partialCloneFilter" config variable set to a value for a remote, then that value will be passed in the form "partialCloneFilter=<value>" after the "name" and "url" fields.

A following commit will allow the client to use the information to decide if it accepts the remote or not. For now the client doesn't do anything with the additional information it receives.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  22 +++++
 Documentation/gitprotocol-v2.adoc     |  59 ++++++++----
 promisor-remote.c                     | 128 ++++++++++++++++++++++++--
 t/t5710-promisor-remote-capability.sh |  31 +++++++
 4 files changed, 215 insertions(+), 25 deletions(-)

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

[snip]

What about testing only 'comma' separated and only 'space' separated fields, since we support those too.

On Thu, Jun 19, 2025 at 2:15 PM Karthik Nayak <karthik.188@gmail.com> wrote:

[...]

I think it would only test the following 2 lines from fields_from_config():

string_list_split_in_place(fields_list, fields, ", ", -1); string_list_remove_empty_items(fields_list, 0);

which should be already tested separately in some string_list C tests.

So I don't think it's worth it.

Thanks.

On 25/06/11 03:45PM, Christian Couder wrote:

[snip]

Ok, in this version we now filter out empty entries from the string_list. Previously if fields were specified with both a comma and SP character (i.e. "partialCloneFilter, token"), an empty entry would be parsed in the middle and lead to a warning message.

This change is good because it would be pretty natural for a user to specify the config with both. It might be nice to leave a comment explaining why we do this though as it may be confusing without context.

-Justin

On Mon, Jun 23, 2025 at 10:04 PM Justin Tobler <jltobler@gmail.com> wrote:

Yeah, I have added some comments, so it looks like this in v5:

        /* Split on any comma or space character */
        string_list_split_in_place(fields_list, fields, ", ", -1);
        /*
         * Remove empty items that might result from trailing
         * commas, or from items being separated by both
         * commas and spaces.
         */
        string_list_remove_empty_items(fields_list, 0);

Thanks.

In a follow up commit we are going to parse more fields, like a filter and a token, coming from the server when it advertises promisor remotes using the "promisor-remote" capability.

To prepare for this, let's refactor the code that parses the advertised fields coming from the server into a new parse_one_advertised_remote() function that will populate a `struct promisor_info` with the content of the fields it parsed.

While at it, let's also pass this `struct promisor_info` to the should_accept_remote() function, instead of passing it the parsed name and url.

These changes will make it simpler to both parse more fields and access the content of these parsed fields in follow up commits.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 91 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 29 deletions(-)

A previous commit allowed a server to pass additional fields through the "promisor-remote" protocol capability after the "name" and "url" fields, specifically the "partialCloneFilter" and "token" fields.

Let's make it possible for a client to check if these fields match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkFields" configuration variable. It should contain a comma or space separated list of fields that will be checked.

By limiting the protocol to specific well-defined fields, we ensure both server and client have a shared understanding of field semantics and usage.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    | 40 ++++++++++++
 promisor-remote.c                     | 89 ++++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh | 34 ++++++++++
 3 files changed, 155 insertions(+), 8 deletions(-)

A previous commit started to define `promisor_field_filter` and `promisor_field_token`, and used them instead of the "partialCloneFilter" and "token" string literals.

Let's do the same for "name" and "url" to avoid repeating them several times and for consistency with the other fields.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

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

I've left some small nits, but mostly this version looks good to me. I don't specifically see a need for re-roll, but will leave it up to you!

[snip]

Thanks,
- Karthik

The "promisor-remote" capability can only be used to pass the names and URLs of the promisor remotes from the server to the client. After that the client can use this information to decide if it accepts the remotes or not.

It would be nice if the server could pass more fields about its remotes and if the client could use that additional information to decide about the remotes by comparing it with its local information about the remotes.

This patch series implements this by adding the "promisor.sendFields" on the server side and the "promisor.checkFields" on the client side.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the remote "foo" configured like this:

[remote "foo"]
        url = file:///tmp/foo.git
	partialCloneFilter = blob:none

then "name=foo,url=file:///tmp/foo.git,partialCloneFilter=blob:none" will be sent by the server for this remote.

All the information passed through the "promisor-remote" capability is still only used to decide if the remotes are accepted or not. The client doesn't store it and doesn't use it for any other purpose.

Note that the filter mechanism already exists for a long time and this series doesn't change how it works. For example, it has already been possible for a long time to have different repos using the same promisor remote with different filters. See the existing partial clone documentation (like "Documentation/technical/partial-clone.adoc") for more information on partial clone.

The fields that can be passed are limited to "partialCloneFilter" and "token".

On the technical side, we get rid of 'struct strvec' and we use 'struct promisor_info' to store the data and 'struct string_list' to store the 'struct promisor_info' instances instead.

This work is part of the "LOP" effort documented in:

  Documentation/technical/large-object-promisors.adoc

See that doc for more information on the broader context.

Changes since v4 ----------------

Thanks to Patrick, Junio, Karthik and Justin for their comments on the previous versions.

There are very few changes compared to v4 and they are quite small.

In patch 1/5, in the commit message:

  - a few sentences were added to explain why using 'struct strvec'
    for the new fields wouldn't scale well,

  - a typo "use use" was fixed.

In patch 1/5, in the code, a BUG() message was improved.

In patch 2/5, in the code, some code comments were added in fields_from_config().

CI tests --------

They all passed:

https://github.com/chriscool/git/actions/runs/15846478103

Range diff compared to v4 -------------------------

1:  8a4df71d2c ! 1:  3700939f67 promisor-remote: refactor to get rid of 'struct strvec'
    @@ Commit message
         for different promisor remotes. Unfortunately using 'struct strvec',
         as we currently do, to store information about the promisor remotes
         with one 'struct strvec' for each field like "name" or "url" does not
    -    scale easily in that case.
    +    scale easily in that case. We would need one 'struct strvec' for each
    +    new field, and then we would have to pass all these 'struct strvec'
    +    around.
     
         Let's refactor this and introduce a new 'struct promisor_info'.
     
         It will only store promisor remote information in its members. For now
         it has only a 'name' member for the promisor remote name and an 'url'
    -    member for its URL. We will use use a 'struct string_list' to store
    -    the instances of 'struct promisor_info'. For each 'item' in the
    +    member for its URL. We will use a 'struct string_list' to store the
    +    instances of 'struct promisor_info'. For each 'item' in the
         string_list, 'item->string' will point to the promisor remote name and
         'item->util' will point to the corresponding 'struct promisor_info'
         instance.
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      
     -  if (!strcmp(urls->v[i], remote_url))
     +  if (!p->url)
    -+          BUG("bad config_info (invalid URL) for remote '%s'",
    ++          BUG("bad config_info (URL is NULL) for remote '%s'",
     +              remote_name);
     +
     +  if (!strcmp(p->url, remote_url))
2:  8f3111b4f2 ! 2:  f546756705 promisor-remote: allow a server to advertise more fields
    @@ promisor-remote.c: static int allow_unsanitized(char ch)
     +  char *fields = NULL;
     +
     +  if (!git_config_get_string(config_key, &fields) && *fields) {
    ++          /* Split on any comma or space character */
     +          string_list_split_in_place(fields_list, fields, ", ", -1);
    ++          /*
    ++           * Remove empty items that might result from trailing
    ++           * commas, or from items being separated by both
    ++           * commas and spaces.
    ++           */
     +          string_list_remove_empty_items(fields_list, 0);
     +          filter_string_list(fields_list, 0, is_valid_field, (void *)config_key);
     +  }
3:  c91a1ba04a = 3:  3ac73b14eb promisor-remote: refactor how we parse advertised fields
4:  ad68202057 = 4:  b75577eff2 promisor-remote: allow a client to check fields
5:  e8efe62b7f = 5:  149b275087 promisor-remote: use string constants for 'name' and 'url' too

Christian Couder (5):
  promisor-remote: refactor to get rid of 'struct strvec'
  promisor-remote: allow a server to advertise more fields
  promisor-remote: refactor how we parse advertised fields
  promisor-remote: allow a client to check fields
  promisor-remote: use string constants for 'name' and 'url' too

 Documentation/config/promisor.adoc    |  62 ++++
 Documentation/gitprotocol-v2.adoc     |  59 +++-
 promisor-remote.c                     | 401 +++++++++++++++++++++-----
 t/t5710-promisor-remote-capability.sh |  65 +++++
 4 files changed, 499 insertions(+), 88 deletions(-)

In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url.

For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case. We would need one 'struct strvec' for each new field, and then we would have to pass all these 'struct strvec' around.

Let's refactor this and introduce a new 'struct promisor_info'.

It will only store promisor remote information in its members. For now it has only a 'name' member for the promisor remote name and an 'url' member for its URL. We will use a 'struct string_list' to store the instances of 'struct promisor_info'. For each 'item' in the string_list, 'item->string' will point to the promisor remote name and 'item->util' will point to the corresponding 'struct promisor_info' instance.

Explicit members are used within 'struct promisor_info' for type safety and clarity regarding the specific information being handled, rather than a generic key-value store. We want to specify and document each field and its content, so adding new members to the struct as more fields are supported is fine.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 111 +++++++++++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 41 deletions(-)

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

The code seems to trust string_list enough that once an earlier call to string_list_lookup() in this code path finds the entry for the remote_name, item->util is assumed to be non-NULL and points at a valid promisor_info instance. But it does not seem to be defensive against p being NULL, but it does so for p->url being NULL, which smells fishy. Once promisor_config_info_list() reads the data from the configuration, do we ever update the contents and the risk of us corrupting p->url while we are running is great enough to warrant being defensive?

My inclination is to suggest removing the check and BUG() here, but I may be missing something obvious (like "p->url gets NULL'ed out in this function, but then such a remote is never checked with this function because such and such logic in that function").

Thanks.

On Wed, Jun 25, 2025 at 7:05 PM Junio C Hamano <gitster@pobox.com> wrote:

Yeah, right, if we trust promisor_config_info_list() to properly populate the `config_info` string list (which is not modified anywhere), let's trust it fully. So I have removed the `if (!p->url)` check and the associated `BUG(...)`, in the v6.

Thanks.

In a follow up commit we are going to parse more fields, like a filter and a token, coming from the server when it advertises promisor remotes using the "promisor-remote" capability.

To prepare for this, let's refactor the code that parses the advertised fields coming from the server into a new parse_one_advertised_remote() function that will populate a `struct promisor_info` with the content of the fields it parsed.

While at it, let's also pass this `struct promisor_info` to the should_accept_remote() function, instead of passing it the parsed name and url.

These changes will make it simpler to both parse more fields and access the content of these parsed fields in follow up commits.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 91 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 29 deletions(-)

For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=<remote_name>,url=<remote_url>".

Let's make it possible to pass more information by introducing a new "promisor.sendFields" configuration variable. This variable should contain a comma or space separated list of field names that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client.

Only a set of predefined fields are allowed. The only fields in this set are "partialCloneFilter" and "token". The "partialCloneFilter" field specifies the filter definition used by the promisor remote, and the "token" field can provide an authentication credential for accessing it.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the "remote.<name>.partialCloneFilter" config variable set to a value for a remote, then that value will be passed in the form "partialCloneFilter=<value>" after the "name" and "url" fields.

A following commit will allow the client to use the information to decide if it accepts the remote or not. For now the client doesn't do anything with the additional information it receives.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  22 +++++
 Documentation/gitprotocol-v2.adoc     |  59 +++++++++---
 promisor-remote.c                     | 134 ++++++++++++++++++++++++--
 t/t5710-promisor-remote-capability.sh |  31 ++++++
 4 files changed, 221 insertions(+), 25 deletions(-)

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

Motivation, like "in order to allow the client more efficient transfer", "in order to give more control on what can be fetched from the server", etc., needs to be given before we say "let's do X". We want to do X not because we can do X; we want to do X as it currently seems to us the best way to achieve Y. What is our Y in this case for the proposed feature?

By making it easier for casual humans who manually write the configuration variable (presumably while testing) and allowing both comma and space as separator, this design decision is forcing one more rule to worry about for those who are writing the parser for the value. There may be some existing configuration variables with such a "leninent" syntax, but I'd rather see us not make the mess even worse.

I know the name "field" was discussed in earlier iterations, but these three lines together with "For example" in a later paragraph, it hints to me that this mechanism is to choose variable-value pairs for which among remotes.<name>.* variables to send after name=<name> and url=<url>; is my understanding correct? If so, can we clarify the paragraphs around here even more so that I do not even have to ask this question?

What do we call the third-level of a variable name in the configuration file? The description on the "--regexp" option in "git config --help" hints one:

    With `get`, interpret the name as a regular expression. Regular
    expression matching is currently case-sensitive and done against
    a canonicalized version of the key in which section and variable
    names are lowercased, but subsection names are not.

So a for remote.origin.partialCloneFilter, "remote" is the section name, "origin" is the subsection name, and "partialCloneFilter" is the variable name.

Armed with that knowledge, perhaps something like:

    <<the motivation comes here, and then ...>> In order to give
    additional information to the client, the server side can set a
    new 'promisor.sendAdditionalVariables' configuration variable,
    whose value is a comma separated list of variable names.  

    The values of the configuration variables specified by this
    variable for the given remote [*] are sent as comma separated
    list of variable=<value>, after name=<name>,url=<url> in the
    promisor-remote capability.

    [*] Concatenating each of these variable names listed as the
    value of promisor.sendAdditionalVariables after remote.<name>.
    results in the configuration variables exposed to the client.

to replace all of the above, and then it ...

... is a good thing to describe that we have these two supported.

And this one ...

... has already been covered. My main point is that we should clarify what a 'field' is and how it relates to the variables in the configuration file of which side (the server, not the client) a lot earlier than we say "these two are the only ones that are supported". Before understanding what a field is, "only these two are valid" does not give readers much useful information.

Typically a name is at most a few words (concatenated with some punctuation), and url would probably be a few hundred bytes at most? can we afford to cram even more var=value on the same line?

The syntax allows you to send more than one name/url pair on the same promisor-remote. Length aside, does this pr-fields mechanism give the values of these fields for multiple remotes?

Suppose you want to advertise promisor-remote "A" and "B", and your sendFields says "token". What does your promisor-remote=<pr-info> look like in such a case?

    name=A,url=https://git.git/A,token=foo;name=B,url=https://git.git/B,token=bar

Do we explicitly say that the set of var=val on promisor-remote are grouped into distinct sets with ';' and each set talks about only one remote? I.e.

    name=A,name=B,url=https://git.git/A,url=https://git.git/B,token=foo,token=bar

may be syntactically allowed in the above BNF, but it needs _some_ rule to match which URL and which token go with which name. Do we also need to say that within a single ';' separated group, a variable can only appear once? IOW, this is invalid?

    name=A,url=https://git.git/A,token=foo,token=bar

With

    pr-fields = pr-fields "," pr-fields

the rule in the last sentence does not make sense. You'd need a distinct BNF non-terminal to name the one you want the rule to apply to. Perhaps something like

	pr-info = pr-one-remote-info | pr-info ';' pr-one-remote-info 
	pr-one-remote-info = pr-fields
        pr-fields = pr-field | pr-fields ',' pr-field
	pr-field = variable '=' value

Then you can safely say 'name' and 'url', must be the first two that appear in each pr-one-remote-info.

This forces us to never add support for a field that MUST be understood for the protocol to operate correctly. Is it sensible?

Just like the index file format defines optional extensions that can be ignored (implication of which is that you MUST die if you do not understand any non-optional ones), shouldn't we have some mechanism to tell "if you do not understand this, do not use the remote, or your repository will be broken in a horrible way"?

I dunno.

On Thu, Jun 26, 2025 at 12:29 AM Junio C Hamano <gitster@pobox.com> wrote:

Ok, I have changed it to the following in v6:

   To allow clients to make more informed decisions about which promisor
   remotes they accept, let's make it possible to pass more information
   by introducing a new "promisor.sendFields" configuration variable.

We indeed have a number of configuration variables accepting lists of items separated by both comma and space. As we cannot fix those easily for backward compatibility and they still make things a bit simpler for users, my opinion is that we'd better bite the bullet and make sure we have a simple and hard-to-misuse standard way to parse such lists. Then we can allow such comma and space separated lists in many places, and use this standard way to parse those lists wherever they are allowed. Users would have a consistent experience when dealing with different configuration variables.

It also seems to me that parsing such lists is not so difficult right now. It basically only requires a call to `string_list_split_in_place()`, followed by a call to `string_list_remove_empty_items()`. Maybe we could refactor that into a dedicated (possibly inline) function (maybe called `string_list_split_config_list()`) and make sure it is used in all the places where such lists are accepted.

I would prefer adding that kind of refactoring in the form of a preparatory patch (or maybe a separate patch altogether) rather than for example allowing only spaces as separators. For now I have made no changes in v6 regarding this.

Yeah, it seems to me that there was previously a sentence about what fields are, but it looks like I removed it by mistake. Anyway, before the paragraph about what the "promisor.sendFields" configuration variable contains, I have added the following in v6:

   On the server side, information about a remote `foo` is stored in
   configuration variables named `remote.foo.<variable-name>`. To make
   it clearer and simpler, we use `field` and `field name` like this:

     * `field name` refers to the <variable-name> part of such a
       configuration variable, and

     * `field` refers to both the `field name` and the value of such a
       configuration variable.

Yeah, I thought that "variable name" could be confusing, so I prefered to use another word, "field", to talk about this.

I'd rather avoid talking about "variable name" other than to explain what "field name" and "field" are. I think it would be too confusing, especially if the name of the new config options are still "promisor.sendFields" and "promisor.checkFields".

Yeah, I hope that the definitions I have added are good enough.

We are adding only 2 relatively short optional "var=value" per remote, so a few hundred bytes at most per remote. This means the whole advertisement for one remote is at most around 500 bytes. As the max pkt-line size is around 16^4, so 65536, this still allows for more than 130 remotes to be advertised which should be enough for now.

Yes, it should look like this, and each "field-name" should appear at most once.

There is already the following:

"where all the `field-name` and `field-value` in a given `pr-fields` are field names and values related to a single promisor remote."

This disallows having fields related to different promisor remotes grouped together by ';'.

This should be invalid, but it wasn't explicitly said that it is invalid. In v6 though I have added "A given `field-name` MUST NOT appear more than once in given `pr-fields`."

I think the following, which I have put in v6, should be enough but maybe I am missing something:

    pr-info = pr-fields | pr-info ";" pr-fields
    pr-fields = pr-field | pr-fields "," pr-field
    pr-field = field-name "=" field-value

Also in the 'object-info' section of the same file, "attrs = attr | attrs SP attrs" has the same problem and might want to be changed to "attrs = attr | attrs SP attr".

It seems to me that this sentence which is already in the doc should be enough:

"The "name" and "url" fields MUST appear first in each pr-fields, in that order."

The protocol for now allows clients to only reply with the names of the promisor remote they accept. Clients can also set "promisor.acceptFromServer" to "All". This means that servers should not trust anyway that the client understood any specific field they sent based on the client's reply.

If the server controls the promisor remotes it advertises, it can send for example a token in the "token" field and then use other means than this protocol to make sure that the client understood the token (and maybe other fields) when the client actually accesses the promisor remote.

If a client wants to make sure that the server passed a specific field with a specific value, they can list this field's name in their "promisor.checkFields" configuration variable which is introduced in patch 4/5 and have their corresponding config option set to the specific value.

If we want to make sure that clients don't access some remotes unless they understand something, we'd better rely at least partially on another mechanism because clients can already be configured to access any remote. They don't need this protocol to be able to access a remote.

In fact the information transmitted by the server is not used to access the remote. It's only used to decide if the remote is accepted. So the remote has to be already configured to access the remote.

Thanks.

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

A meta observation, as I haven't even thought if the above proposal makes sense or not, but a reponse for v5 like the above that comes nearly at the same minite as v6 highly discourages any response to this message that may want to say "oh, that is not a good idea because ...".

The position is understandable (I am not saying agreeable), but if one takes such a position, this series will get even longer, by requiring such a refactoring and new set of helper functions in the front of the series, to avoid making things even worse than they currently are. If you want to punt on that "simple standard way" and leave it outside the series, then please do not add such syntax to the variables in this series.

Sorry, the argument does not make much sense to me.

After all, the end-users who follow the documentation needs to know which *VARIABLES* (in remotes.<name><VARIABLE>) to set to affect the value that this mechanism lets them send out. And the configuration variable to control which of remotes.<name>.<VARIABLE> are sent out is a new thing, and it itself calls it a FIELD. Whatever name that new thing chooses to call itself, the fact is that it is about what the users have long known as configuration variables.

So, what makes this whole thing more confusing than necessary, to me, looks like your use of the word "field" in the first place. The use of word "field" in "sendFields" and "checkFields" are much much more recent than the concept of "configuration variables" that has long been established in users' minds (as far as I know, these "fields" are not even in any released versions), so I do not see why we want to keep it and force users learn yet another word. Just fix the name of these new configuration variables, explain that they are used to name other configuration variables, and be done it without uttering "field" even once, and we would be good and less confusing, wouldn't we? Or am I being too naive and forgetting some already established use of sendFields and checkFields?

On Mon, Jul 21, 2025 at 8:53 PM Junio C Hamano <gitster@pobox.com> wrote:

I am not sure why it discourages reponses. People can still reply to the v6 patch or to the discussion that started from the v5 patch or both. The discussion from the v5 patch is not obsolete if some comments have not been properly addressed.

Also the fact that the replies come at the same time as a new version, means that reviewers don't need to context switch as much as if there were for example some replies at one time, then a few days later replies to the rest of the reviews, then a few days later a new version. So for reviewers who don't like to context switch much it should be better if everything is sent at once.

I'd like it if there was a consensus on doing such a refactoring before starting to work on it. So if you are fine with that refactoring, I am willing to either add it as part of this series, or work on it after this series is merged (like for the recent cc/t9350-cleanup patch), as you prefer.

I couldn't know your and others' opinion on it (and I am still not sure about it) before we started discussing it. Sometimes for example you are also fine with just leaving over those kinds of bits. So for now in v7, I haven't changed this, but let me know and I will make the necessary changes in v8 or later in separate patches.

You mean using "promisor.sendVariables" and "promisor.checkVariables" instead of "promisor.sendFields" and "promisor.checkFields", and using "variable" instead of "field" everywhere in the documentation and the code?

I just think it will be confusing because after being sent over through the protocol, they are no longer variable in the sense that the client can't (and shouldn't) change them. They could also be confused with environment variables, so perhaps "promisor.sendConfigVariables" and "promisor.checkConfigVariables", but the names will start to be long especially if we have to say that they are from the server when processed on the client side.

Also I think it's better to use a different name because they are used differently, serve different purposes and have different rules applied to them in the context of the promisor-remote protocol capability.

Anyway if that's what is prefered, I will make the change. For now in the v7, I haven't changed this.

Le 25/06/2025 à 14:50, Christian Couder a écrit :

This kind of text structure calls a description list instead and you can already use backquotes:

`partialCloneFilter`:: contains the partial clone filter

Please no space in placeholders: <field-name>

More generally, I am a bit annoyed by the usage of the "will" auxiliary when not expressing a true future. For an international audience, this can be misleading. The plain language[1] philosophy mandates to not use auxiliaries other than where they are required (no convoluted sentences).

Would it make sense to start a style guide to help writing consistent documentation that targets people whose first language is not English? Being an non native speaker, I often find our docs too literate, with lengthy sentences.

[1] https://en.wikipedia.org/wiki/Plain_language

Be careful to adjust the length of the underline to the one of the title

 >  about to a client which may want to use them as its promisor
remotes,> -instead of this repository. In this case <pr-infos> should be
of the

This list can be turned into a description list.

Here the syntax used is not compatible with synopsis. Would it make sense to uniformize it, or is BNF ok?

Thanks

Jean-Noël

On Fri, Jun 27, 2025 at 8:48 PM Jean-Noël Avila <jn.avila@free.fr> wrote:

Thanks for the suggestion. I have used that in v6.

I have removed some usage of "will" in the patches in v6. I haven't changed the existing documentation in this file though.

I think it's a separate discussion, and I am not sure I want to participate in it.

I have adjusted it in v6. Thanks.

[...]

Done.

I am not sure what you call the "synopsis". Is it "promisor-remote=<pr-info>"?

If yes, then I think  "promisor-remote=<pr-names>" where

    pr-names = pr-name | pr-names ";" pr-name

(which is what I have currently) should be considered compatible. Or do you have another suggestion?

Thanks for your review, Christian.

A previous commit allowed a server to pass additional fields through the "promisor-remote" protocol capability after the "name" and "url" fields, specifically the "partialCloneFilter" and "token" fields.

Let's make it possible for a client to check if these fields match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkFields" configuration variable. It should contain a comma or space separated list of fields that will be checked.

By limiting the protocol to specific well-defined fields, we ensure both server and client have a shared understanding of field semantics and usage.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    | 40 ++++++++++++
 promisor-remote.c                     | 89 ++++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh | 34 ++++++++++
 3 files changed, 155 insertions(+), 8 deletions(-)

A previous commit started to define `promisor_field_filter` and `promisor_field_token`, and used them instead of the "partialCloneFilter" and "token" string literals.

Let's do the same for "name" and "url" to avoid repeating them several times and for consistency with the other fields.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

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

The topic saw only a few comments in this last iteration. Would we be seeing a hopefully small and final update to tie the loose ends before we declare that the topic is ready for 'next'?

Thanks.

On Tue, Jul 8, 2025 at 12:35 AM Junio C Hamano <gitster@pobox.com> wrote:

Yeah, I am planning to work on a small update soon.

Thanks.

The "promisor-remote" capability can only be used to pass the names and URLs of the promisor remotes from the server to the client. After that the client can use this information to decide if it accepts the remotes or not.

It would be nice if the server could pass more fields about its remotes and if the client could use that additional information to decide about the remotes by comparing it with its local information about the remotes.

This patch series implements this by adding the "promisor.sendFields" on the server side and the "promisor.checkFields" on the client side.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the remote "foo" configured like this:

[remote "foo"]
        url = file:///tmp/foo.git
	partialCloneFilter = blob:none

then "name=foo,url=file:///tmp/foo.git,partialCloneFilter=blob:none" will be sent by the server for this remote.

All the information passed through the "promisor-remote" capability is still only used to decide if the remotes are accepted or not. The client doesn't store it and doesn't use it for any other purpose.

Note that the filter mechanism already exists for a long time and this series doesn't change how it works. For example, it has already been possible for a long time to have different repos using the same promisor remote with different filters. See the existing partial clone documentation (like "Documentation/technical/partial-clone.adoc") for more information on partial clone.

The fields that can be passed are limited to "partialCloneFilter" and "token".

On the technical side, we get rid of 'struct strvec' and we use 'struct promisor_info' to store the data and 'struct string_list' to store the 'struct promisor_info' instances instead.

This work is part of the "LOP" effort documented in:

  Documentation/technical/large-object-promisors.adoc

See that doc for more information on the broader context.

Changes since v5 ----------------

Thanks to Patrick, Junio, Karthik, Jean-Noël and Justin for their comments on the previous versions.

There are a number of changes compared to v5 but they are mostly on the documentation and commit messages.

  - In patch 1/5 ("promisor-remote: refactor to get rid of 'struct
    strvec'"), we don't check p->url and BUG() if it's NULL anymore.

  - In patch 2/5 ("promisor-remote: allow a server to advertise more
    fields") the commit message has been improved. Especially there
    are now explanations about what we call fields and field names.

  - In patch 2/5 there are the following changes to the documentation:

    - use of "will" has been reduced,

    - "`partialCloneFilter`::" and "`token`::" are used to describe
      these field names,

    - "<field-name>" is used instead of "<field name>",

    - the length of the underline to the "promisor-remote=<pr-info>"
      title has been adjusted

    - the BNF definition of "pr-info" has been improved,

    - it's stated that a given field name should not appear more than
      once for each promisor remote,

    - the BNF definition of "pr-names" has been improved.

  - In patch 4/5 ("promisor-remote: allow a client to check fields")
    the use of "will" has alse been reduced.

CI tests --------

They have all passed, see:

https://github.com/chriscool/git/actions/runs/16417265102

Range diff compared to v5 -------------------------

1:  3700939f67 ! 1:  87a6ba5c48 promisor-remote: refactor to get rid of 'struct strvec'
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	}
      
     -	if (!strcmp(urls->v[i], remote_url))
    -+	if (!p->url)
    -+		BUG("bad config_info (URL is NULL) for remote '%s'",
    -+		    remote_name);
    -+
     +	if (!strcmp(p->url, remote_url))
      		return 1;
      
2:  f546756705 ! 2:  0543a42858 promisor-remote: allow a server to advertise more fields
    @@ Commit message
         and "url" information from a server to a client in the form
         "name=<remote_name>,url=<remote_url>".
     
    -    Let's make it possible to pass more information by introducing a new
    -    "promisor.sendFields" configuration variable. This variable should
    -    contain a comma or space separated list of field names that will be
    -    looked up in the configuration of the remote on the server to find the
    -    values that will be passed to the client.
    +    To allow clients to make more informed decisions about which promisor
    +    remotes they accept, let's make it possible to pass more information
    +    by introducing a new "promisor.sendFields" configuration variable.
     
    -    Only a set of predefined fields are allowed. The only fields in this
    -    set are "partialCloneFilter" and "token". The "partialCloneFilter"
    -    field specifies the filter definition used by the promisor remote,
    -    and the "token" field can provide an authentication credential for
    -    accessing it.
    +    On the server side, information about a remote `foo` is stored in
    +    configuration variables named `remote.foo.<variable-name>`. To make
    +    it clearer and simpler, we use `field` and `field name` like this:
    +
    +      * `field name` refers to the <variable-name> part of such a
    +        configuration variable, and
    +
    +      * `field` refers to both the `field name` and the value of such a
    +        configuration variable.
    +
    +    The "promisor.sendFields" configuration variable should contain a
    +    comma or space separated list of field names that will be looked up
    +    in the configuration of the remote on the server to find the values
    +    that will be passed to the client.
    +
    +    Only a set of predefined field names are allowed. The only field
    +    names in this set are "partialCloneFilter" and "token". The
    +    "partialCloneFilter" field name specifies the filter definition used
    +    by the promisor remote, and the "token" field name can provide an
    +    authentication credential for accessing it.
     
         For example, if "promisor.sendFields" is set to "partialCloneFilter",
    -    and the server has the "remote.<name>.partialCloneFilter" config
    -    variable set to a value for a remote, then that value will be passed
    -    in the form "partialCloneFilter=<value>" after the "name" and "url"
    -    fields.
    +    and the server has the "remote.foo.partialCloneFilter" config
    +    variable set to a value, then that value will be passed in the
    +    "partialCloneFilter" field in the form "partialCloneFilter=<value>"
    +    after the "name" and "url" fields.
     
         A following commit will allow the client to use the information to
         decide if it accepts the remote or not. For now the client doesn't do
    @@ Documentation/config/promisor.adoc: promisor.advertise::
      
     +promisor.sendFields::
     +	A comma or space separated list of additional remote related
    -+	field names. A server will send these field names and the
    ++	field names. A server sends these field names and the
     +	associated field values from its configuration when
     +	advertising its promisor remotes using the "promisor-remote"
     +	capability, see linkgit:gitprotocol-v2[5]. Currently, only the
     +	"partialCloneFilter" and "token" field names are supported.
     ++
    -+* "partialCloneFilter": contains the partial clone filter
    -+  used for the remote.
    ++`partialCloneFilter`:: contains the partial clone filter
    ++used for the remote.
     ++
    -+* "token": contains an authentication token for the remote.
    ++`token`:: contains an authentication token for the remote.
     ++
     +When a field name is part of this list and a corresponding
    -+"remote.foo.<field name>" config variable is set on the server to a
    -+non-empty value, then the field name and value will be sent when
    ++"remote.foo.<field-name>" config variable is set on the server to a
    ++non-empty value, then the field name and value are sent when
     +advertising the promisor remote "foo".
     ++
     +This list has no effect unless the "promisor.advertise" config
    @@ Documentation/gitprotocol-v2.adoc: retrieving the header from a bundle at the in
      headers of that bundle or bundles.
      
     -promisor-remote=<pr-infos>
    +-~~~~~~~~~~~~~~~~~~~~~~~~~~
     +promisor-remote=<pr-info>
    - ~~~~~~~~~~~~~~~~~~~~~~~~~~
    ++~~~~~~~~~~~~~~~~~~~~~~~~~
      
      The server may advertise some promisor remotes it is using or knows
      about to a client which may want to use them as its promisor remotes,
    @@ Documentation/gitprotocol-v2.adoc: retrieving the header from a bundle at the in
      form:
      
     -	pr-infos = pr-info | pr-infos ";" pr-info
    -+	pr-info = pr-fields | pr-info ";" pr-info
    ++	pr-info = pr-fields | pr-info ";" pr-fields
      
     -	pr-info = "name=" pr-name | "name=" pr-name "," "url=" pr-url
    -+	pr-fields = field-name "=" field-value | pr-fields "," pr-fields
    ++	pr-fields = pr-field | pr-fields "," pr-field
      
     -where `pr-name` is the urlencoded name of a promisor remote, and
     -`pr-url` the urlencoded URL of that promisor remote.
    -+where all the `field-name` and `field-value` in a given `pr-fields`
    -+are field names and values related to a single promisor remote.
    ++	pr-field = field-name "=" field-value
      
     -In this case, if the client decides to use one or more promisor
     -remotes the server advertised, it can reply with
     -"promisor-remote=<pr-names>" where <pr-names> should be of the form:
    ++where all the `field-name` and `field-value` in a given `pr-fields`
    ++are field names and values related to a single promisor remote. A
    ++given `field-name` MUST NOT appear more than once in given
    ++`pr-fields`.
    ++
     +The server MUST advertise at least the "name" and "url" field names
     +along with the associated field values, which are the name of a valid
     +remote and its URL, in each `pr-fields`. The "name" and "url" fields
     +MUST appear first in each pr-fields, in that order.
    - 
    --	pr-names = pr-name | pr-names ";" pr-name
    ++
     +After these mandatory fields, the server MAY advertise the following
     +optional fields in any order:
     +
    -+- "partialCloneFilter": The filter specification used by the remote.
    ++`partialCloneFilter`:: The filter specification used by the remote.
     +Clients can use this to determine if the remote's filtering strategy
     +is compatible with their needs (e.g., checking if both use "blob:none").
     +It corresponds to the "remote.<name>.partialCloneFilter" config setting.
     +
    -+- "token": An authentication token that clients can use when
    ++`token`:: An authentication token that clients can use when
     +connecting to the remote. It corresponds to the "remote.<name>.token"
     +config setting.
     +
    @@ Documentation/gitprotocol-v2.adoc: retrieving the header from a bundle at the in
     +If the client decides to use one or more promisor remotes the server
     +advertised, it can reply with "promisor-remote=<pr-names>" where
     +<pr-names> should be of the form:
    -+
    -+	pr-names = pr-name | pr-names ";" pr-names
    + 
    + 	pr-names = pr-name | pr-names ";" pr-name
      
      where `pr-name` is the urlencoded name of a promisor remote the server
      advertised and the client accepts.
3:  3ac73b14eb = 3:  d566719807 promisor-remote: refactor how we parse advertised fields
4:  b75577eff2 ! 4:  d2a13eabc0 promisor-remote: allow a client to check fields
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
     +
     +promisor.checkFields::
     +	A comma or space separated list of additional remote related
    -+	field names. A client will check if the values of these fields
    ++	field names. A client checks if the values of these fields
     +	transmitted by a server correspond to the values of these
     +	fields in its own configuration before accepting a promisor
     +	remote. Currently, "partialCloneFilter" and "token" are the
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
     +   match the value advertised by the server for the "token" field.
     ++
     +If any of these conditions is not met for any field name listed in
    -+`promisor.checkFields`, the advertised remote "foo" will be rejected.
    ++`promisor.checkFields`, the advertised remote "foo" is rejected.
     ++
     +For the "partialCloneFilter" field, this allows the client to ensure
     +that the server's filter matches what it expects locally, preventing
    @@ Documentation/config/promisor.adoc: promisor.acceptFromServer::
     ++
     +The field names and values should be passed by the server through the
     +"promisor-remote" capability by using the `promisor.sendFields` config
    -+variable. The fields will be checked only if the
    ++variable. The fields are checked only if the
     +`promisor.acceptFromServer` config variable is not set to "None". If
    -+set to "None", this config variable will have no effect. See
    ++set to "None", this config variable has no effect. See
     +linkgit:gitprotocol-v2[5].
     
      ## promisor-remote.c ##
    @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
      	if (accept != ACCEPT_KNOWN_URL)
      		BUG("Unhandled 'enum accept_promisor' value '%d'", accept);
     @@ promisor-remote.c: static int should_accept_remote(enum accept_promisor accept,
    - 		    remote_name);
    + 	}
      
      	if (!strcmp(p->url, remote_url))
     -		return 1;
5:  149b275087 = 5:  c7d7c83534 promisor-remote: use string constants for 'name' and 'url' too

Christian Couder (5):
  promisor-remote: refactor to get rid of 'struct strvec'
  promisor-remote: allow a server to advertise more fields
  promisor-remote: refactor how we parse advertised fields
  promisor-remote: allow a client to check fields
  promisor-remote: use string constants for 'name' and 'url' too

 Documentation/config/promisor.adoc    |  62 ++++
 Documentation/gitprotocol-v2.adoc     |  63 ++--
 promisor-remote.c                     | 397 +++++++++++++++++++++-----
 t/t5710-promisor-remote-capability.sh |  65 +++++
 4 files changed, 499 insertions(+), 88 deletions(-)

In a following commit, we will use the new 'promisor-remote' protocol capability introduced by d460267613 (Add 'promisor-remote' capability to protocol v2, 2025-02-18) to pass and process more information about promisor remotes than just their name and url.

For that purpose, we will need to store information about other fields, especially information that might or might not be available for different promisor remotes. Unfortunately using 'struct strvec', as we currently do, to store information about the promisor remotes with one 'struct strvec' for each field like "name" or "url" does not scale easily in that case. We would need one 'struct strvec' for each new field, and then we would have to pass all these 'struct strvec' around.

Let's refactor this and introduce a new 'struct promisor_info'.

It will only store promisor remote information in its members. For now it has only a 'name' member for the promisor remote name and an 'url' member for its URL. We will use a 'struct string_list' to store the instances of 'struct promisor_info'. For each 'item' in the string_list, 'item->string' will point to the promisor remote name and 'item->util' will point to the corresponding 'struct promisor_info' instance.

Explicit members are used within 'struct promisor_info' for type safety and clarity regarding the specific information being handled, rather than a generic key-value store. We want to specify and document each field and its content, so adding new members to the struct as more fields are supported is fine.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 107 ++++++++++++++++++++++++++++------------------
 1 file changed, 66 insertions(+), 41 deletions(-)

For now the "promisor-remote" protocol capability can only pass "name" and "url" information from a server to a client in the form "name=<remote_name>,url=<remote_url>".

To allow clients to make more informed decisions about which promisor remotes they accept, let's make it possible to pass more information by introducing a new "promisor.sendFields" configuration variable.

On the server side, information about a remote `foo` is stored in configuration variables named `remote.foo.<variable-name>`. To make it clearer and simpler, we use `field` and `field name` like this:

  * `field name` refers to the <variable-name> part of such a
    configuration variable, and

  * `field` refers to both the `field name` and the value of such a
    configuration variable.

The "promisor.sendFields" configuration variable should contain a comma or space separated list of field names that will be looked up in the configuration of the remote on the server to find the values that will be passed to the client.

Only a set of predefined field names are allowed. The only field names in this set are "partialCloneFilter" and "token". The "partialCloneFilter" field name specifies the filter definition used by the promisor remote, and the "token" field name can provide an authentication credential for accessing it.

For example, if "promisor.sendFields" is set to "partialCloneFilter", and the server has the "remote.foo.partialCloneFilter" config variable set to a value, then that value will be passed in the "partialCloneFilter" field in the form "partialCloneFilter=<value>" after the "name" and "url" fields.

A following commit will allow the client to use the information to decide if it accepts the remote or not. For now the client doesn't do anything with the additional information it receives.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    |  22 +++++
 Documentation/gitprotocol-v2.adoc     |  63 +++++++++---
 promisor-remote.c                     | 134 ++++++++++++++++++++++++--
 t/t5710-promisor-remote-capability.sh |  31 ++++++
 4 files changed, 225 insertions(+), 25 deletions(-)

In a follow up commit we are going to parse more fields, like a filter and a token, coming from the server when it advertises promisor remotes using the "promisor-remote" capability.

To prepare for this, let's refactor the code that parses the advertised fields coming from the server into a new parse_one_advertised_remote() function that will populate a `struct promisor_info` with the content of the fields it parsed.

While at it, let's also pass this `struct promisor_info` to the should_accept_remote() function, instead of passing it the parsed name and url.

These changes will make it simpler to both parse more fields and access the content of these parsed fields in follow up commits.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 91 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 29 deletions(-)

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

Unless the primary use of an array is about passing it around as a whole "set", name such a variable singular, so that element[4] can be naturally read as "fourth element"---"fourth elements" is not as natural.

Also, can't we do this without strbuf_split(), which is a wrong API to use in general [*]? strbuf is a very good data structure to work with when editing string data, but an array of strbuf is not---you would not be editing many pieces of string data in parallel.

    [*] often string_list_split_in_place() is a better alternative,
    especially when you do not have to heavily edit the substrings.

	The pointer elem points at the name, and the pointer p
	points at the beginning of value, which could contain '='.

This does not even count as "editing"; split_in_place() would have removed the trailing comma (and replaced it with NUL to terminate the string).

	elem pointed at "foo" or "foo,"; we may have stripped the
	trailing comma, but we didn't see the equal sign to start
	the value at all.  Bad input.

	Terminate the name by replacing '=' with NUL.

	Can this helper function fail and signal that it saw a
	malformed data?  If not already, shouldn't it be taught to
	do so?

We are inventing the syntax for this data in this series, so if this helper takes garbage data silently, and if we are not willing to fix it, then we can even consider changing the syntax to something with a helper we can use that already has a good error checking.

As url_percent_decode() always allocate a new copy of string even when there is nothing to decode, value will always be an allocated string, and if we are not storing it away, it will leak. The copies we kept in info->{name,url} are ours to own. Makes sense.

And because [elem..p] (name) we only peeked, we can safely release the whole thing. If you used string_list_split_in_place(), you would only free the string_list shell without having to free the underlying string.

Nicely done.

On Mon, Jul 21, 2025 at 10:39 PM Junio C Hamano <gitster@pobox.com> wrote:

Yeah, string_list_split_in_place() is used in v7 and it looks better.

Yeah, right. This is fixed in v7.

I am not sure what the interface would look like. Should a version of url_percent_decode() that returns NULL in case of error would be enough, or should it use `return error("...");`in case of error?

And it seems to me that an error can only happen when the string passed to url_percent_decode() contains a '%' which is not followed by 2 hexadecimal characters, or do you see other possible errors?

For now I haven't changed this in v7, but I am open to suggestions about the best way to implement it in a v8 or maybe a separate series.

Yeah, it's better with string_list_split_in_place().

Thanks.

A previous commit allowed a server to pass additional fields through the "promisor-remote" protocol capability after the "name" and "url" fields, specifically the "partialCloneFilter" and "token" fields.

Let's make it possible for a client to check if these fields match what it expects before accepting a promisor remote.

We allow this by introducing a new "promisor.checkFields" configuration variable. It should contain a comma or space separated list of fields that will be checked.

By limiting the protocol to specific well-defined fields, we ensure both server and client have a shared understanding of field semantics and usage.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/config/promisor.adoc    | 40 ++++++++++++
 promisor-remote.c                     | 89 ++++++++++++++++++++++++---
 t/t5710-promisor-remote-capability.sh | 34 ++++++++++
 3 files changed, 155 insertions(+), 8 deletions(-)