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

Re: [PATCH v3 0/5] First steps towards partial clone submodules

From
Elijah Newren <newren@gmail.com>
Date
Jun 10, 2021, 21:29 UTC
Message-ID
<CABPp-BEEiPP7AEk4Wexw4_MDHcin2n8xkMowO=OXTn9pNPaG0A@mail.gmail.com>
In-Reply-To
<cover.1623345496.git.jonathantanmy@google.com>
On Thu, Jun 10, 2021 at 10:35 AM Jonathan Tan <jonathantanmy@google.com> wrote:
Show 12 quoted lines
>
> I think I've addressed all review comments. As for Junio's suggestion
> about also printing the type in the former patch 4 (now patch 5) [1], I
> decided to just leave the code as-is and not also print the type.
>
> The main changes are that patch 1 is somewhat rewritten - we still
> remove the global variable, but we no longer read the
> extensions.partialClone config directly from promisor-remote.c. Instead,
> we store it in struct repository when the format of a repository is
> being verified, and promisor-remote.c merely reads it from there. Patch
> 3 is a new patch that updates the environment variable preparation
> before it is moved in patch 4 (formerly patch 3).

I've read through all the patches. 2 & 5 look good to me, I had small nitpicks on 1 & 4, and I'm totally lost on patch 3. Patch 3 is just a one-liner and it might be fine, but for some reason I can't figure out the code before or after the patch even after digging around into other commits and other files to try to get my bearings. Hopefully someone else can comment on that one.

Show 253 quoted lines
>
> [1] https://lore.kernel.org/git/xmqq7dj2ik7k.fsf@gitster.g/
>
> Jonathan Tan (5):
>   repository: move global r_f_p_c to repo struct
>   promisor-remote: support per-repository config
>   submodule: refrain from filtering GIT_CONFIG_COUNT
>   run-command: refactor subprocess env preparation
>   promisor-remote: teach lazy-fetch in any repo
>
>  Makefile                      |   1 +
>  object-file.c                 |   7 +--
>  promisor-remote.c             | 108 ++++++++++++++++++----------------
>  promisor-remote.h             |  28 ++++++---
>  repository.c                  |   9 +++
>  repository.h                  |   5 ++
>  run-command.c                 |  12 ++++
>  run-command.h                 |  10 ++++
>  setup.c                       |  16 +++--
>  submodule.c                   |  17 +-----
>  t/helper/test-partial-clone.c |  43 ++++++++++++++
>  t/helper/test-tool.c          |   1 +
>  t/helper/test-tool.h          |   1 +
>  t/t0410-partial-clone.sh      |  23 ++++++++
>  14 files changed, 196 insertions(+), 85 deletions(-)
>  create mode 100644 t/helper/test-partial-clone.c
>
> Range-diff against v2:
> 1:  d99598ca50 < -:  ---------- promisor-remote: read partialClone config here
> -:  ---------- > 1:  255d112256 repository: move global r_f_p_c to repo struct
> 2:  5a1ccae335 ! 2:  a52448cff2 promisor-remote: support per-repository config
>     @@ promisor-remote.c
>       #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,
>     +                    int oid_nr)
>      @@ promisor-remote.c: static int fetch_objects(const char *remote_name,
>         return finish_command(&child) ? -1 : 0;
>       }
>     @@ promisor-remote.c: static void promisor_remote_move_to_tail(struct promisor_remo
>         const char *name;
>         size_t namelen;
>         const char *subkey;
>     -@@ promisor-remote.c: static int promisor_remote_config(const char *var, const char *value, void *data
>     -            * NULL value is handled in handle_extension_v0 in setup.c.
>     -            */
>     -           if (value)
>     --                  repository_format_partial_clone = xstrdup(value);
>     -+                  config->repository_format_partial_clone = xstrdup(value);
>     -           return 0;
>     -   }
>     -
>      @@ promisor-remote.c: static int promisor_remote_config(const char *var, const char *value, void *data
>
>                 remote_name = xmemdupz(name, namelen);
>     @@ promisor-remote.c: static int promisor_remote_config(const char *var, const char
>      +  config->promisors_tail = &config->promisors;
>
>      -  git_config(promisor_remote_config, NULL);
>     -+  git_config(promisor_remote_config, config);
>     ++  repo_config(r, promisor_remote_config, config);
>
>     --  if (repository_format_partial_clone) {
>     -+  if (config->repository_format_partial_clone) {
>     +-  if (the_repository->repository_format_partial_clone) {
>     ++  if (r->repository_format_partial_clone) {
>                 struct promisor_remote *o, *previous;
>
>     --          o = promisor_remote_lookup(repository_format_partial_clone,
>     +-          o = promisor_remote_lookup(the_repository->repository_format_partial_clone,
>      +          o = promisor_remote_lookup(config,
>     -+                                     config->repository_format_partial_clone,
>     ++                                     r->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);
>     +-                  promisor_remote_new(the_repository->repository_format_partial_clone);
>     ++                  promisor_remote_new(config, r->repository_format_partial_clone);
>         }
>       }
>
>     @@ promisor-remote.c: static int promisor_remote_config(const char *var, const char
>      -  while (promisors) {
>      -          struct promisor_remote *r = promisors;
>      -          promisors = promisors->next;
>     -+  FREE_AND_NULL(config->repository_format_partial_clone);
>     -+
>      +  while (config->promisors) {
>      +          struct promisor_remote *r = config->promisors;
>      +          config->promisors = config->promisors->next;
>     @@ repository.h: struct lock_file;
>       enum untracked_cache_setting {
>         UNTRACKED_CACHE_UNSET = -1,
>      @@ repository.h: struct repository {
>     -   /* True if commit-graph has been disabled within this process. */
>     -   int commit_graph_disabled;
>
>     -+  /* Configurations related to promisor remotes. */
>     +   /* Configurations related to promisor remotes. */
>     +   char *repository_format_partial_clone;
>      +  struct promisor_remote_config *promisor_remote_config;
>     -+
>     +
>         /* Configurations */
>
>     -   /* Indicate if a repository has a different 'commondir' from 'gitdir' */
> -:  ---------- > 3:  e1a40108f4 submodule: refrain from filtering GIT_CONFIG_COUNT
> 3:  3f7c4e6e67 ! 4:  fd6907822c run-command: move envvar-resetting function
>     @@ Metadata
>      Author: Jonathan Tan <jonathantanmy@google.com>
>
>       ## Commit message ##
>     -    run-command: move envvar-resetting function
>     +    run-command: refactor subprocess env preparation
>
>     -    There is a function that resets environment variables, used when
>     -    invoking a sub-process in a submodule. The lazy-fetching code (used in
>     -    partial clones) will need this function in a subsequent commit, so move
>     -    it to a more central location.
>     +    submodule.c has functionality that prepares the environment for running
>     +    a subprocess in a new repo. The lazy-fetching code (used in partial
>     +    clones) will need this in a subsequent commit, so move it to a more
>     +    central location.
>
>          Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>
>     @@ run-command.c: int run_auto_maintenance(int quiet)
>         return run_command(&maint);
>       }
>      +
>     -+void prepare_other_repo_env(struct strvec *env_array)
>     ++void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>      +{
>      +  const char * const *var;
>      +
>      +  for (var = local_repo_env; *var; var++) {
>     -+          if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>     ++          if (strcmp(*var, CONFIG_DATA_ENVIRONMENT) &&
>     ++              strcmp(*var, CONFIG_COUNT_ENVIRONMENT))
>      +                  strvec_push(env_array, *var);
>      +  }
>     ++  strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
>      +}
>
>       ## run-command.h ##
>     @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai
>                                const char *tr2_category, const char *tr2_label);
>
>      +/**
>     -+ * Convenience function which adds all GIT_* environment variables to env_array
>     -+ * with the exception of GIT_CONFIG_PARAMETERS. When used as the env_array of a
>     -+ * subprocess, these entries cause the corresponding environment variables to
>     -+ * be unset in the subprocess. See local_repo_env in cache.h for more
>     ++ * Convenience function which prepares env_array for a command to be run in a
>     ++ * new repo. This adds all GIT_* environment variables to env_array with the
>     ++ * exception of GIT_CONFIG_PARAMETERS (which cause the corresponding
>     ++ * environment variables to be unset in the subprocess) and adds an environment
>     ++ * variable pointing to new_git_dir. See local_repo_env in cache.h for more
>      + * information.
>      + */
>     -+void prepare_other_repo_env(struct strvec *env_array);
>     ++void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);
>      +
>       #endif
>
>     @@ submodule.c: static void print_submodule_diff_summary(struct repository *r, stru
>      -  const char * const *var;
>      -
>      -  for (var = local_repo_env; *var; var++) {
>     --          if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
>     +-          if (strcmp(*var, CONFIG_DATA_ENVIRONMENT) &&
>     +-              strcmp(*var, CONFIG_COUNT_ENVIRONMENT))
>      -                  strvec_push(out, *var);
>      -  }
>      -}
>     @@ submodule.c: static void print_submodule_diff_summary(struct repository *r, stru
>       void prepare_submodule_repo_env(struct strvec *out)
>       {
>      -  prepare_submodule_repo_env_no_git_dir(out);
>     -+  prepare_other_repo_env(out);
>     -   strvec_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
>     -                DEFAULT_GIT_DIR_ENVIRONMENT);
>     +-  strvec_pushf(out, "%s=%s", GIT_DIR_ENVIRONMENT,
>     +-               DEFAULT_GIT_DIR_ENVIRONMENT);
>     ++  prepare_other_repo_env(out, DEFAULT_GIT_DIR_ENVIRONMENT);
>       }
>
>       static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)
>       {
>      -  prepare_submodule_repo_env_no_git_dir(out);
>     -+  prepare_other_repo_env(out);
>     -   strvec_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
>     +-  strvec_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
>     ++  prepare_other_repo_env(out, ".");
>       }
>
>     + /*
> 4:  655607d575 ! 5:  a6d73662b1 promisor-remote: teach lazy-fetch in any repo
>     @@ Commit message
>          prevents testing of the functionality in this patch by user-facing
>          commands. So for now, test this mechanism using a test helper.
>
>     +    Besides that, there is some code that uses the wrapper functions
>     +    like has_promisor_remote(). Those will need to be checked to see if they
>     +    could support the non-wrapper functions instead (and thus support any
>     +    repository, not just the_repository).
>     +
>          Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>
>       ## Makefile ##
>     @@ promisor-remote.c: static int fetch_objects(const char *remote_name,
>
>         child.git_cmd = 1;
>         child.in = -1;
>     -+  if (repo != the_repository) {
>     -+          prepare_other_repo_env(&child.env_array);
>     -+          strvec_pushf(&child.env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
>     -+                       repo->gitdir);
>     -+  }
>     ++  if (repo != the_repository)
>     ++          prepare_other_repo_env(&child.env_array, repo->gitdir);
>         strvec_pushl(&child.args, "-c", "fetch.negotiationAlgorithm=noop",
>                      "fetch", remote_name, "--no-tags",
>                      "--no-write-fetch-head", "--recurse-submodules=no",
>     -@@ promisor-remote.c: static void promisor_remote_init(struct repository *r)
>     -           xcalloc(sizeof(*r->promisor_remote_config), 1);
>     -   config->promisors_tail = &config->promisors;
>     -
>     --  git_config(promisor_remote_config, config);
>     -+  repo_config(r, promisor_remote_config, config);
>     -
>     -   if (config->repository_format_partial_clone) {
>     -           struct promisor_remote *o, *previous;
>      @@ promisor-remote.c: int promisor_remote_get_direct(struct repository *repo,
>
>         promisor_remote_init(repo);
> --
> 2.32.0.rc1.229.g3e70b5a671-goog
>
Previous: Elijah NewrenNext: Elijah Newren
Message 69 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.