Re: [PATCH 2/4] promisor-remote: support per-repository config
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Jun 8, 2021, 00:48 UTC
- Message-ID
- <YL6+d3u0I8WC0FS0@google.com>
- In-Reply-To
- <d8f5fa9b9fab73c2e0923ccf38d5bdb15f7b7a70.1622580781.git.jonathantanmy@google.com>
On Tue, Jun 01, 2021 at 02:34:17PM -0700, Jonathan Tan wrote:
Show 5 quoted lines
> > 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!
Show 14 quoted lines
> 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.
Show 5 quoted lines
> @@ -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. ;)
Show 9 quoted lines
> -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