Re: [PATCH 01/16] promisor-remote: try accepted remotes before others in get_direct()

[Patrick Steinhardt <ps@pks.im>]

On Mon, Mar 23, 2026 at 09:05:04AM +0100, Christian Couder wrote:
> When a server advertises promisor remotes and the client accepts some
> of them, those remotes carry the server's intent: 'fetch missing
> objects preferably from here', and the client agrees with that for the
> remotes it accepts.
> 
> However promisor_remote_get_direct() actually iterates over all
> promisor remotes in list order, which is the order they appear in the
> config files (except perhaps for the one appearing in the
> `extensions.partialClone` config variable which is tried last).
> 
> This means an existing, but not accepted, promisor remote, could be
> tried before the accepted ones, which does not reflect the intent of
> the agreement between client and server.
> 
> If the client doesn't care about what the server suggests, it should
> accept nothing and rely on its remotes as they are already configured.
> 
> To better reflect the agreement between client and server, let's make
> promisor_remote_get_direct() try the accepted promisor remotes before
> the non-accepted ones.

Interesting, and it feels sensible to me. Is it documented anywhere that
the ordering of announced remotes is actually important?

> Concretely, let's extract a try_promisor_remotes() helper and call it
> twice from promisor_remote_get_direct():
> 
> - first with an `accepted_only=true` argument to try only the accepted
>   remotes,
> - then with `accepted_only=false` to fall back to any remaining remote.
> 
> Ensuring that accepted remotes are preferred will be even more
> important if in the future a mechanism is developed to allow the
> client to auto-configure remotes that the server advertises. This will
> in particular avoid fetching from the server (which is already
> configured as a promisor remote) before trying the auto-configured
> remotes, as these new remotes would likely appear at the end of the
> config file, and as the server might not appear in the
> `extensions.partialClone` config variable.

Not quite sure I correctly understand this paragraph. Is the idea that
in the future, we might not even store announced promisors in the config
at all but simply use whatever the server announces on any given fetch?

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 96fa215b06..3f8aeee787 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -268,11 +268,37 @@ static int remove_fetched_oids(struct repository *repo,
>  	return remaining_nr;
>  }
>  
> +static int try_promisor_remotes(struct repository *repo,
> +				struct object_id **remaining_oids,
> +				int *remaining_nr, int *to_free,
> +				bool accepted_only)
> +{
> +	struct promisor_remote *r = repo->promisor_remote_config->promisors;
> +
> +	for (; r; r = r->next) {
> +		if (accepted_only && !r->accepted)
> +			continue;
> +		if (!accepted_only && r->accepted)
> +			continue;

This can be simplified to `if (!accepted_only != !r->accepted)`.

Also, can we maybe add a test for this to verify that we use the correct
ordering now?

Patrick