Re: [Outreachy][PATCH v2] promisor-remote: honor promisor.fetchOrder when trying remotes

[Junio C Hamano <gitster@pobox.com>]

Christian, I thought you are the go-to person for promisor remote,
and if I am not mistaken, you worked on letting the server side tell
the clients which promisors to use first?  How would this mesh well
with that?

Thanks.

Yopa Nelly <yopatatchou@gmail.com> writes:

> From: Nelly Yopa <yopatatchou@gmail.com>
>
> When a repository is configured with more than one promisor
> remote, Git tries them one after another until the missing
> objects have been fetched. The order is currently determined
> only by the order in which remote.<name>.promisor entries appear
> in the configuration. There is no way to say "try this remote
> first" without moving config lines around.
>
> Add a promisor.fetchOrder configuration variable that takes a
> comma-separated list of remote names. Remotes named in the list
> are tried first, in the order listed. Remotes not named in the
> list are tried afterwards, in their original configuration
> order. The extensions.partialClone remote is still moved to the
> end of the list by promisor_remote_init() unless it is named in
> fetchOrder, in which case its listed position wins.
>
> The order is applied inside each of the two passes in
> lazy_fetch_objects() first the accepted remotes, then the
> rest so the existing "accepted before configured" behavior is
> unchanged. Within each pass, the fetchOrder order takes
> precedence over the configuration order.
>
> The parsed list is stored in struct promisor_remote_config and
> is re-parsed each time the config is read. Instead of sorting
> the linked list at fetch time, try_promisor_remotes() walks the
> list twice: once in fetchOrder order for the remotes named
> there, and once in the original config order for the rest.
>
> Signed-off-by: Yopa Nelly <yopatatchou@gmail.com>
> ---
>  Documentation/config/promisor.adoc |  11 +++
>  promisor-remote.c                  | 108 +++++++++++++++++++++++++----
>  2 files changed, 107 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/config/promisor.adoc b/Documentation/config/promisor.adoc
> index c1f3d766a6..ccc0925800 100644
> --- a/Documentation/config/promisor.adoc
> +++ b/Documentation/config/promisor.adoc
> @@ -31,6 +31,17 @@ 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.fetchOrder`::
> +	A comma-separated list of promisor remote names. When Git
> +	needs to lazily fetch a missing object, it tries the remotes
> +	in this list first, in the order listed, then the remaining
> +	configured promisor remotes in their configuration order.
> +	The `extensions.partialClone` remote is tried last unless it
> +	is named in this list.
> ++
> +See linkgit:gitprotocol-v2[5] and the "Partial Clone Design
> +Notes" in link:technical/partial-clone.html[Documentation/technical/partial-clone].
> +
>  promisor.acceptFromServer::
>  	Controls which promisor remotes advertised by a server (using the
>  	"promisor-remote" protocol capability) a client will accept. By
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 316d9950aa..c664a48f88 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -19,8 +19,11 @@
>  struct promisor_remote_config {
>  	struct promisor_remote *promisors;
>  	struct promisor_remote **promisors_tail;
> +	struct string_list fetch_order;
> +	int fetch_order_initialized;
>  };
>  
> +
>  static int fetch_objects(struct repository *repo,
>  			 const char *remote_name,
>  			 const struct object_id *oids,
> @@ -120,6 +123,29 @@ static int promisor_remote_config(const char *var, const char *value,
>  	size_t namelen;
>  	const char *subkey;
>  
> +    	if (!strcmp(var, "promisor.fetchorder")) {
> +		char *value_copy;
> +
> +		if (!value)
> +			return config_error_nonbool(var);
> +		/*
> +		 * The list is re-read from scratch every time the config
> +		 * is parsed, so start by clearing any previous entries.
> +		 */
> +		if (config->fetch_order_initialized)
> +			string_list_clear(&config->fetch_order, 0);
> +		else
> +			string_list_init_nodup(&config->fetch_order);
> +
> +		value_copy = xstrdup(value);
> +		string_list_split_in_place_f(&config->fetch_order, value_copy, ",", -1,
> +					     STRING_LIST_SPLIT_TRIM |
> +					     STRING_LIST_SPLIT_NONEMPTY);
> +		free(value_copy);
> +		config->fetch_order_initialized = 1;
> +		return 0;
> +	}
> +
>  	if (parse_config_key(var, "remote", &name, &namelen, &subkey) < 0)
>  		return 0;
>  
> @@ -157,6 +183,8 @@ static int promisor_remote_config(const char *var, const char *value,
>  	return 0;
>  }
>  
> +
> +
>  static void promisor_remote_init(struct repository *r)
>  {
>  	struct promisor_remote_config *config;
> @@ -180,6 +208,7 @@ static void promisor_remote_init(struct repository *r)
>  		else
>  			promisor_remote_new(config, r->repository_format_partial_clone);
>  	}
> +
>  }
>  
>  void promisor_remote_clear(struct promisor_remote_config *config)
> @@ -193,6 +222,9 @@ void promisor_remote_clear(struct promisor_remote_config *config)
>  	}
>  
>  	config->promisors_tail = &config->promisors;
> +	if (config->fetch_order_initialized)
> +		string_list_clear(&config->fetch_order, 0);
> +	config->fetch_order_initialized = 0;
>  }
>  
>  void repo_promisor_remote_reinit(struct repository *r)
> @@ -262,6 +294,8 @@ static int remove_fetched_oids(struct repository *repo,
>  	return remaining_nr;
>  }
>  
> +
> +
>  /*
>   * Fetch the remaining objects (given in '*remaining_oids', which
>   * contains '*remaining_nr' object ids) from the known promisor
> @@ -279,6 +313,24 @@ static int remove_fetched_oids(struct repository *repo,
>   * Return 1 when all the requested objects have been fetched, 0
>   * otherwise.
>   */
> +static int try_one_remote(struct repository *repo, struct promisor_remote *r,
> +			  struct object_id **remaining_oids,
> +			  int *remaining_nr, int *to_free, int depth)
> +{
> +	if (fetch_objects(repo, r->name,
> +			  *remaining_oids, *remaining_nr, depth) < 0) {
> +		if (*remaining_nr == 1)
> +			return 0;
> +		*remaining_nr = remove_fetched_oids(repo, remaining_oids,
> +						    *remaining_nr, *to_free);
> +		if (*remaining_nr) {
> +			*to_free = 1;
> +			return 0;
> +		}
> +	}
> +	return 1; /* all fetched */
> +}
> +
>  static int try_promisor_remotes(struct repository *repo,
>  				struct object_id **remaining_oids,
>  				int *remaining_nr,
> @@ -286,24 +338,56 @@ static int try_promisor_remotes(struct repository *repo,
>  				int depth,
>  				bool accepted_only)
>  {
> -	struct promisor_remote *r = repo->promisor_remote_config->promisors;
> +	struct promisor_remote_config *config = repo->promisor_remote_config;
> +	struct promisor_remote *r;
> +	size_t i;
> +	int fetched;
> +
> +	/*
> +	 * First pass: try the remotes in the order the user listed
> +	 * them in promisor.fetchOrder.
> +	 */
> +	if (config->fetch_order_initialized) {
> +		for (i = 0; i < config->fetch_order.nr; i++) {
> +			r = promisor_remote_lookup(config,
> +						   config->fetch_order.items[i].string,
> +						   NULL);
> +			if (!r || accepted_only != r->accepted)
> +				continue;
>  
> -	for (; r; r = r->next) {
> +			fetched = try_one_remote(repo, r, remaining_oids,
> +						 remaining_nr, to_free, depth);
> +			if (fetched)
> +				return 1;
> +		}
> +	}
> +
> +	/*
> +	 * Second pass: try the remaining remotes in their config order.
> +	 */
> +	for (r = config->promisors; r; r = r->next) {
>  		if (accepted_only != r->accepted)
>  			continue;
> -		if (fetch_objects(repo, r->name,
> -				  *remaining_oids, *remaining_nr, depth) < 0) {
> -			if (*remaining_nr == 1)
> -				continue;
> -			*remaining_nr = remove_fetched_oids(repo, remaining_oids,
> -							    *remaining_nr, *to_free);
> -			if (*remaining_nr) {
> -				*to_free = 1;
> -				continue;
> +
> +		/* Skip ones we already tried in the first pass. */
> +		if (config->fetch_order_initialized) {
> +			bool already_tried = false;
> +			for (i = 0; i < config->fetch_order.nr; i++) {
> +				if (!strcmp(r->name, config->fetch_order.items[i].string)) {
> +					already_tried = true;
> +					break;
> +				}
>  			}
> +			if (already_tried)
> +				continue;
>  		}
> -		return 1; /* all fetched */
> +
> +		fetched = try_one_remote(repo, r, remaining_oids,
> +					 remaining_nr, to_free, depth);
> +		if (fetched)
> +			return 1;
>  	}
> +
>  	return 0;
>  }