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

Message ID	d8f5fa9b9fab73c2e0923ccf38d5bdb15f7b7a70.1622580781.git.jonathantanmy@google.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <git-owner@kernel.org> Date: Tue, 1 Jun 2021 14:34:17 -0700 In-Reply-To: <cover.1622580781.git.jonathantanmy@google.com> Message-Id: <d8f5fa9b9fab73c2e0923ccf38d5bdb15f7b7a70.1622580781.git.jonathantanmy@google.com> Mime-Version: 1.0 References: <cover.1622580781.git.jonathantanmy@google.com> Subject: [PATCH 2/4] promisor-remote: support per-repository config From: Jonathan Tan <jonathantanmy@google.com> To: git@vger.kernel.org Cc: Jonathan Tan <jonathantanmy@google.com> Content-Type: text/plain; charset="UTF-8" Precedence: bulk
Series	First steps towards partial clone submodules \| expand [0/4] First steps towards partial clone submodules [1/4] promisor-remote: read partialClone config here [2/4] promisor-remote: support per-repository config [3/4] run-command: move envvar-resetting function [4/4] promisor-remote: teach lazy-fetch in any repo

Jonathan Tan June 1, 2021, 9:34 p.m. UTC

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.

The actual lazy-fetching of missing objects currently does not work on
repositories other than the_repository, and will still not work after
this commit, so add a BUG message explaining this. A subsequent commit
will remove this limitation.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 promisor-remote.c | 101 +++++++++++++++++++++++++---------------------
 promisor-remote.h |  20 +++++++--
 repository.h      |   4 ++
 3 files changed, 77 insertions(+), 48 deletions(-)

Taylor Blau June 4, 2021, 8:09 p.m. UTC | #1

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.
>
> The actual lazy-fetching of missing objects currently does not work on
> repositories other than the_repository, and will still not work after
> this commit, so add a BUG message explaining this. A subsequent commit
> will remove this limitation.

Makes sense to me. I found my answer to the question that I raised
during my review of the previous patch, and I think it would make sense
to address in an amended version of this patch.

Other than that, the translation all looked very faithful to me.

> -void promisor_remote_reinit(void)
> +void repo_promisor_remote_reinit(struct repository *r)
>  {
> -	initialized = 0;
> -	promisor_remote_clear();
> -	promisor_remote_init();
> +	promisor_remote_clear(r->promisor_remote_config);

Ah, this is probably where I would have expected to see
r->promisor_remote_config->repository_format_partial_clone freed as
well.

I wondered whether or not that should have been freed, since on first
read it seemed that this function was mostly concerned with the list of
promisor remotes rather than the structure containing them. But on a
closer look, we are re-initializing the whole structure with
promisor_remote_init(), which runs the whole promisor_remote_config
callback again.

So I do think we want to free that part of the structure, too, before
reinitializing it. I would probably do it in promisor_remote_clear().

> @@ -235,9 +244,11 @@ int promisor_remote_get_direct(struct repository *repo,
>  	if (oid_nr == 0)
>  		return 0;
>
> -	promisor_remote_init();
> +	promisor_remote_init(repo);
>
> -	for (r = promisors; r; r = r->next) {
> +	if (repo != the_repository)
> +		BUG("only the_repository is supported for now");

I could go either way on whether this is worthy of a BUG() or not, but I
don't really have much of a strong feeling about it.

Thanks,
Taylor

Elijah Newren June 4, 2021, 9:21 p.m. UTC | #2

On Tue, Jun 1, 2021 at 2:38 PM Jonathan Tan <jonathantanmy@google.com> 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.
>
> The actual lazy-fetching of missing objects currently does not work on
> repositories other than the_repository, and will still not work after
> this commit, so add a BUG message explaining this. A subsequent commit
> will remove this limitation.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  promisor-remote.c | 101 +++++++++++++++++++++++++---------------------
>  promisor-remote.h |  20 +++++++--
>  repository.h      |   4 ++
>  3 files changed, 77 insertions(+), 48 deletions(-)
>
> 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;
> +};
>
>  static int fetch_objects(const char *remote_name,
>                          const struct object_id *oids,
> @@ -37,10 +41,8 @@ static int fetch_objects(const char *remote_name,
>         return finish_command(&child) ? -1 : 0;
>  }
>
> -static struct promisor_remote *promisors;
> -static struct promisor_remote **promisors_tail = &promisors;
> -
> -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)
>  {
>         struct promisor_remote *r;
>
> @@ -52,18 +54,19 @@ static struct promisor_remote *promisor_remote_new(const char *remote_name)
>
>         FLEX_ALLOC_STR(r, name, remote_name);
>
> -       *promisors_tail = r;
> -       promisors_tail = &r->next;
> +       *config->promisors_tail = r;
> +       config->promisors_tail = &r->next;
>
>         return r;
>  }
>
> -static struct promisor_remote *promisor_remote_lookup(const char *remote_name,
> +static struct promisor_remote *promisor_remote_lookup(struct promisor_remote_config *config,
> +                                                     const char *remote_name,
>                                                       struct promisor_remote **previous)
>  {
>         struct promisor_remote *r, *p;
>
> -       for (p = NULL, r = promisors; r; p = r, r = r->next)
> +       for (p = NULL, r = config->promisors; r; p = r, r = r->next)
>                 if (!strcmp(r->name, remote_name)) {
>                         if (previous)
>                                 *previous = p;
> @@ -73,7 +76,8 @@ static struct promisor_remote *promisor_remote_lookup(const char *remote_name,
>         return NULL;
>  }
>
> -static void promisor_remote_move_to_tail(struct promisor_remote *r,
> +static void promisor_remote_move_to_tail(struct promisor_remote_config *config,
> +                                        struct promisor_remote *r,
>                                          struct promisor_remote *previous)
>  {
>         if (r->next == NULL)
> @@ -82,20 +86,21 @@ static void promisor_remote_move_to_tail(struct promisor_remote *r,
>         if (previous)
>                 previous->next = r->next;
>         else
> -               promisors = r->next ? r->next : r;
> +               config->promisors = r->next ? r->next : r;
>         r->next = NULL;
> -       *promisors_tail = r;
> -       promisors_tail = &r->next;
> +       *config->promisors_tail = r;
> +       config->promisors_tail = &r->next;
>  }
>
>  static int promisor_remote_config(const char *var, const char *value, void *data)
>  {
> +       struct promisor_remote_config *config = data;
>         const char *name;
>         size_t namelen;
>         const char *subkey;
>
>         if (!strcmp(var, "extensions.partialclone")) {
> -               repository_format_partial_clone = xstrdup(value);
> +               config->repository_format_partial_clone = xstrdup(value);
>                 return 0;
>         }
>
> @@ -110,8 +115,8 @@ static int promisor_remote_config(const char *var, const char *value, void *data
>
>                 remote_name = xmemdupz(name, namelen);
>
> -               if (!promisor_remote_lookup(remote_name, NULL))
> -                       promisor_remote_new(remote_name);
> +               if (!promisor_remote_lookup(config, remote_name, NULL))
> +                       promisor_remote_new(config, remote_name);
>
>                 free(remote_name);
>                 return 0;
> @@ -120,9 +125,9 @@ static int promisor_remote_config(const char *var, const char *value, void *data
>                 struct promisor_remote *r;
>                 char *remote_name = xmemdupz(name, namelen);
>
> -               r = promisor_remote_lookup(remote_name, NULL);
> +               r = promisor_remote_lookup(config, remote_name, NULL);
>                 if (!r)
> -                       r = promisor_remote_new(remote_name);
> +                       r = promisor_remote_new(config, remote_name);
>
>                 free(remote_name);
>
> @@ -135,59 +140,63 @@ static int promisor_remote_config(const char *var, const char *value, void *data
>         return 0;
>  }
>
> -static int initialized;
> -
> -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;
> +       config = r->promisor_remote_config =
> +               xcalloc(sizeof(*r->promisor_remote_config), 1);
> +       config->promisors_tail = &config->promisors;
>
> -       git_config(promisor_remote_config, NULL);
> +       git_config(promisor_remote_config, config);
>
> -       if (repository_format_partial_clone) {
> +       if (config->repository_format_partial_clone) {
>                 struct promisor_remote *o, *previous;
>
> -               o = promisor_remote_lookup(repository_format_partial_clone,
> +               o = promisor_remote_lookup(config,
> +                                          config->repository_format_partial_clone,
>                                            &previous);
>                 if (o)
> -                       promisor_remote_move_to_tail(o, previous);
> +                       promisor_remote_move_to_tail(config, o, previous);
>                 else
> -                       promisor_remote_new(repository_format_partial_clone);
> +                       promisor_remote_new(config, config->repository_format_partial_clone);
>         }
>  }
>
> -static void promisor_remote_clear(void)
> +static void promisor_remote_clear(struct promisor_remote_config *config)
>  {
> -       while (promisors) {
> -               struct promisor_remote *r = promisors;
> -               promisors = promisors->next;
> +       while (config->promisors) {
> +               struct promisor_remote *r = config->promisors;
> +               config->promisors = config->promisors->next;
>                 free(r);
>         }
>
> -       promisors_tail = &promisors;
> +       config->promisors_tail = &config->promisors;
>  }
>
> -void promisor_remote_reinit(void)
> +void repo_promisor_remote_reinit(struct repository *r)
>  {
> -       initialized = 0;
> -       promisor_remote_clear();
> -       promisor_remote_init();
> +       promisor_remote_clear(r->promisor_remote_config);
> +       FREE_AND_NULL(r->promisor_remote_config);
> +       promisor_remote_init(r);
>  }
>
> -struct promisor_remote *promisor_remote_find(const char *remote_name)
> +struct promisor_remote *repo_promisor_remote_find(struct repository *r,
> +                                                 const char *remote_name)
>  {
> -       promisor_remote_init();
> +       promisor_remote_init(r);
>
>         if (!remote_name)
> -               return promisors;
> +               return r->promisor_remote_config->promisors;
>
> -       return promisor_remote_lookup(remote_name, NULL);
> +       return promisor_remote_lookup(r->promisor_remote_config, remote_name, NULL);
>  }
>
> -int has_promisor_remote(void)
> +int repo_has_promisor_remote(struct repository *r)
>  {
> -       return !!promisor_remote_find(NULL);
> +       return !!repo_promisor_remote_find(r, NULL);
>  }
>
>  static int remove_fetched_oids(struct repository *repo,
> @@ -235,9 +244,11 @@ int promisor_remote_get_direct(struct repository *repo,
>         if (oid_nr == 0)
>                 return 0;
>
> -       promisor_remote_init();
> +       promisor_remote_init(repo);
>
> -       for (r = promisors; r; r = r->next) {
> +       if (repo != the_repository)
> +               BUG("only the_repository is supported for now");
> +       for (r = repo->promisor_remote_config->promisors; r; r = r->next) {
>                 if (fetch_objects(r->name, remaining_oids, remaining_nr) < 0) {
>                         if (remaining_nr == 1)
>                                 continue;
> diff --git a/promisor-remote.h b/promisor-remote.h
> index 687210ab87..5390d3e7bf 100644
> --- a/promisor-remote.h
> +++ b/promisor-remote.h
> @@ -17,9 +17,23 @@ struct promisor_remote {
>         const char name[FLEX_ARRAY];
>  };
>
> -void promisor_remote_reinit(void);
> -struct promisor_remote *promisor_remote_find(const char *remote_name);
> -int has_promisor_remote(void);
> +void repo_promisor_remote_reinit(struct repository *r);
> +static inline void promisor_remote_reinit(void)
> +{
> +       repo_promisor_remote_reinit(the_repository);
> +}
> +
> +struct promisor_remote *repo_promisor_remote_find(struct repository *r, const char *remote_name);
> +static inline struct promisor_remote *promisor_remote_find(const char *remote_name)
> +{
> +       return repo_promisor_remote_find(the_repository, remote_name);
> +}
> +
> +int repo_has_promisor_remote(struct repository *r);
> +static inline int has_promisor_remote(void)
> +{
> +       return repo_has_promisor_remote(the_repository);
> +}

Is part of the plan for supporting partial clones within submodules to
audit the code for use of these inline wrappers and convert them over
to the repo_* variants?  I'm particularly interested in the
has_promisor_remote() function, since there are calls in
diffcore-rename at least that protect that call with a check against r
== the_repository.

>
>  /*
>   * Fetches all requested objects from all promisor remotes, trying them one at
> diff --git a/repository.h b/repository.h
> index a45f7520fd..fc06c154e2 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -10,6 +10,7 @@ struct lock_file;
>  struct pathspec;
>  struct raw_object_store;
>  struct submodule_cache;
> +struct promisor_remote_config;
>
>  enum untracked_cache_setting {
>         UNTRACKED_CACHE_UNSET = -1,
> @@ -139,6 +140,9 @@ struct repository {
>         /* True if commit-graph has been disabled within this process. */
>         int commit_graph_disabled;
>
> +       /* Configurations related to promisor remotes. */
> +       struct promisor_remote_config *promisor_remote_config;
> +
>         /* Configurations */
>
>         /* Indicate if a repository has a different 'commondir' from 'gitdir' */
> --
> 2.32.0.rc0.204.g9fa02ecfa5-goog

Looks like a reasonable step in moving away from globals and have
repository-specific variants of these functions; I didn't spot any
problems, just one question about additional plans.

Jonathan Tan June 5, 2021, 1:43 a.m. UTC | #3

> 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.
> >
> > The actual lazy-fetching of missing objects currently does not work on
> > repositories other than the_repository, and will still not work after
> > this commit, so add a BUG message explaining this. A subsequent commit
> > will remove this limitation.
> 
> Makes sense to me. I found my answer to the question that I raised
> during my review of the previous patch, and I think it would make sense
> to address in an amended version of this patch.

Just to clarify, what is the question and what is the answer?

> Other than that, the translation all looked very faithful to me.
> 
> > -void promisor_remote_reinit(void)
> > +void repo_promisor_remote_reinit(struct repository *r)
> >  {
> > -	initialized = 0;
> > -	promisor_remote_clear();
> > -	promisor_remote_init();
> > +	promisor_remote_clear(r->promisor_remote_config);
> 
> Ah, this is probably where I would have expected to see
> r->promisor_remote_config->repository_format_partial_clone freed as
> well.
> 
> I wondered whether or not that should have been freed, since on first
> read it seemed that this function was mostly concerned with the list of
> promisor remotes rather than the structure containing them. But on a
> closer look, we are re-initializing the whole structure with
> promisor_remote_init(), which runs the whole promisor_remote_config
> callback again.
> 
> So I do think we want to free that part of the structure, too, before
> reinitializing it. I would probably do it in promisor_remote_clear().

I'll add that in the next version.

> > @@ -235,9 +244,11 @@ int promisor_remote_get_direct(struct repository *repo,
> >  	if (oid_nr == 0)
> >  		return 0;
> >
> > -	promisor_remote_init();
> > +	promisor_remote_init(repo);
> >
> > -	for (r = promisors; r; r = r->next) {
> > +	if (repo != the_repository)
> > +		BUG("only the_repository is supported for now");
> 
> I could go either way on whether this is worthy of a BUG() or not, but I
> don't really have much of a strong feeling about it.

This BUG() will be removed in patch 4. I'm OK either way (adding BUG()
here and removing it in patch 4, or never adding BUG() in the first
place).

Jonathan Tan June 5, 2021, 1:54 a.m. UTC | #4

> > +int repo_has_promisor_remote(struct repository *r);
> > +static inline int has_promisor_remote(void)
> > +{
> > +       return repo_has_promisor_remote(the_repository);
> > +}
> 
> Is part of the plan for supporting partial clones within submodules to
> audit the code for use of these inline wrappers and convert them over
> to the repo_* variants?  I'm particularly interested in the
> has_promisor_remote() function, since there are calls in
> diffcore-rename at least that protect that call with a check against r
> == the_repository.

Good point. Yes, that should be part of the plan - at least, we should
see what those invocations are for.

> > @@ -139,6 +140,9 @@ struct repository {
> >         /* True if commit-graph has been disabled within this process. */
> >         int commit_graph_disabled;
> >
> > +       /* Configurations related to promisor remotes. */
> > +       struct promisor_remote_config *promisor_remote_config;
> > +
> >         /* Configurations */
> >
> >         /* Indicate if a repository has a different 'commondir' from 'gitdir' */
> > --
> > 2.32.0.rc0.204.g9fa02ecfa5-goog
> 
> Looks like a reasonable step in moving away from globals and have
> repository-specific variants of these functions; I didn't spot any
> problems, just one question about additional plans.

Thanks for taking a look.

Emily Shaffer June 8, 2021, 12:48 a.m. UTC | #5

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

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

Commit Message

Comments

Patch