git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren <newren@gmail.com>
Date
Jun 4, 2021, 21:21 UTC
Message-ID
<CABPp-BHZ0OrADfP3V-hZ1YYf3fQYW3fU-tCJWgs976amG-T1Dg@mail.gmail.com>
In-Reply-To
<d8f5fa9b9fab73c2e0923ccf38d5bdb15f7b7a70.1622580781.git.jonathantanmy@google.com>
On Tue, Jun 1, 2021 at 2:38 PM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 262 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.
>
> 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.

Show 27 quoted lines
>
>  /*
>   * 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.

Previous: Jonathan TanNext: Jonathan Tan
Message 9 of 77 in “First steps towards partial clone submodules”
  1. 0/4 First steps towards partial clone submodulesJonathan Tan, Jun 1, 2021
  2. 1/4 promisor-remote: read partialClone config hereJonathan Tan, Jun 1, 2021
  3. Taylor BlauJun 4, 2021
  4. Jonathan TanJun 5, 2021
  5. Emily ShafferJun 7, 2021
  6. 2/4 promisor-remote: support per-repository configJonathan Tan, Jun 1, 2021
  7. Taylor BlauJun 4, 2021
  8. Jonathan TanJun 5, 2021
  9. Elijah NewrenJun 4, 2021
  10. Jonathan TanJun 5, 2021
  11. Emily ShafferJun 8, 2021
  12. 3/4 run-command: move envvar-resetting functionJonathan Tan, Jun 1, 2021
  13. Taylor BlauJun 4, 2021
  14. Jonathan TanJun 5, 2021
  15. Emily ShafferJun 8, 2021
  16. 4/4 promisor-remote: teach lazy-fetch in any repoJonathan Tan, Jun 1, 2021
  17. Taylor BlauJun 4, 2021
  18. Jonathan TanJun 5, 2021
  19. Elijah NewrenJun 4, 2021
  20. Jonathan TanJun 5, 2021
  21. Elijah NewrenJun 5, 2021
  22. Elijah NewrenJun 5, 2021
  23. Jonathan TanJun 5, 2021
  24. Emily ShafferJun 8, 2021
  25. Jonathan TanJun 9, 2021
  26. 0/4 First steps towards partial clone submodulesJonathan Tan, Jun 8, 2021
  27. 3/4 run-command: move envvar-resetting functionJonathan Tan, Jun 8, 2021
  28. Junio C HamanoJun 8, 2021
  29. Jonathan TanJun 9, 2021
  30. Junio C HamanoJun 9, 2021
  31. Jonathan TanJun 9, 2021
  32. 2/4 promisor-remote: support per-repository configJonathan Tan, Jun 8, 2021
  33. Junio C HamanoJun 8, 2021
  34. Jonathan TanJun 9, 2021
  35. 1/4 promisor-remote: read partialClone config hereJonathan Tan, Jun 8, 2021
  36. Junio C HamanoJun 8, 2021
  37. Jonathan TanJun 9, 2021
  38. Junio C HamanoJun 9, 2021
  39. Jonathan TanJun 9, 2021
  40. Elijah NewrenJun 8, 2021
  41. Jonathan TanJun 9, 2021
  42. Elijah NewrenJun 9, 2021
  43. Jonathan TanJun 10, 2021
  44. 4/4 promisor-remote: teach lazy-fetch in any repoJonathan Tan, Jun 8, 2021
  45. Junio C HamanoJun 8, 2021
  46. Jonathan TanJun 9, 2021
  47. Junio C HamanoJun 9, 2021
  48. Jonathan TanJun 9, 2021
  49. Junio C HamanoJun 10, 2021
  50. Elijah NewrenJun 8, 2021
  51. Jonathan TanJun 9, 2021
  52. Elijah NewrenJun 8, 2021
  53. Junio C HamanoJun 8, 2021
  54. Elijah NewrenJun 9, 2021
  55. Junio C HamanoJun 9, 2021
  56. Jonathan TanJun 9, 2021
  57. Emily ShafferJun 8, 2021
  58. 0/5 First steps towards partial clone submodulesJonathan Tan, Jun 10, 2021
  59. 2/5 promisor-remote: support per-repository configJonathan Tan, Jun 10, 2021
  60. 5/5 promisor-remote: teach lazy-fetch in any repoJonathan Tan, Jun 10, 2021
  61. 1/5 repository: move global r_f_p_c to repo structJonathan Tan, Jun 10, 2021
  62. Elijah NewrenJun 10, 2021
  63. 3/5 submodule: refrain from filtering GIT_CONFIG_COUNTJonathan Tan, Jun 10, 2021
  64. Elijah NewrenJun 10, 2021
  65. Jeff KingJun 10, 2021
  66. Jonathan TanJun 11, 2021
  67. 4/5 run-command: refactor subprocess env preparationJonathan Tan, Jun 10, 2021
  68. Elijah NewrenJun 10, 2021
  69. Elijah NewrenJun 10, 2021
  70. Elijah NewrenJun 15, 2021
  71. 0/5 First steps towards partial clone submodulesJonathan Tan, Jun 17, 2021
  72. 1/5 repository: move global r_f_p_c to repo structJonathan Tan, Jun 17, 2021
  73. 2/5 promisor-remote: support per-repository configJonathan Tan, Jun 17, 2021
  74. 3/5 submodule: refrain from filtering GIT_CONFIG_COUNTJonathan Tan, Jun 17, 2021
  75. 4/5 run-command: refactor subprocess env preparationJonathan Tan, Jun 17, 2021
  76. 5/5 promisor-remote: teach lazy-fetch in any repoJonathan Tan, Jun 17, 2021
  77. Elijah NewrenJun 19, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.