Re: [PATCH 2/4] promisor-remote: support per-repository config

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Jun 01, 2021 at 02:34:17PM -0700, Jonathan Tan wrote:
> 
> Instead of using global variables to store promisor remote information,
> store this config in struct repository instead, and add
> repository-agnostic non-static functions corresponding to the existing
> non-static functions that only work on the_repository.

Nice!

> diff --git a/promisor-remote.c b/promisor-remote.c
> index bfe8eee5f2..5819d2cf28 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -5,7 +5,11 @@
>  #include "transport.h"
>  #include "strvec.h"
>  
> -static char *repository_format_partial_clone;
> +struct promisor_remote_config {
> +	char *repository_format_partial_clone;
> +	struct promisor_remote *promisors;
> +	struct promisor_remote **promisors_tail;
> +};
[snip]
> -static struct promisor_remote *promisors;
> -static struct promisor_remote **promisors_tail = &promisors;

Nice, so all the old globals are contained in a data struct now...

> -static struct promisor_remote *promisor_remote_new(const char *remote_name)
> +static struct promisor_remote *promisor_remote_new(struct promisor_remote_config *config,
> +						   const char *remote_name)

...which we use during promisor_remote creation now.

> @@ -135,59 +140,63 @@ static int promisor_remote_config(const char *var, const char *value, void *data
>  	return 0;
>  }
>  
> -static int initialized;

Very happy to see this pattern vanquished. ;)

> -static void promisor_remote_init(void)
> +static void promisor_remote_init(struct repository *r)
>  {
> -	if (initialized)
> +	struct promisor_remote_config *config;
> +
> +	if (r->promisor_remote_config)
>  		return;
> -	initialized = 1;

So it's not a "call once, set global state" anymore, but we are also
careful not to trample any existing state on the 'struct repository'.
Nice.

> +	config = r->promisor_remote_config =
> +		xcalloc(sizeof(*r->promisor_remote_config), 1);

Hm. Am I the only one who doesn't like assigning from the result of an
assignment like this? ...Based on 'git grep "=[^=]\+=[^=]"' yes, I am the
only one :)


Overall this patch looks OK to me, but I see that there are some changes
suggested for the next version, so I'll hold off on a reviewed-by so I
can have a look at those too.

 - Emily