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

Re: [PATCH 6/9] clone: allow an explicit argument for parallel submodule clones

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 27, 2015, 20:57 UTC
Message-ID
<xmqqlhaoni6q.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1445969753-418-7-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 29 quoted lines
> Just pass it along to "git submodule update", which may pick reasonable
> defaults if you don't specify an explicit number.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/git-clone.txt |  5 ++++-
>  builtin/clone.c             | 26 ++++++++++++++++++++------
>  t/t7406-submodule-update.sh | 15 +++++++++++++++
>  3 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index f1f2a3f..affa52e 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -14,7 +14,7 @@ SYNOPSIS
>  	  [-o <name>] [-b <name>] [-u <upload-pack>] [--reference <repository>]
>  	  [--dissociate] [--separate-git-dir <git dir>]
>  	  [--depth <depth>] [--[no-]single-branch]
> -	  [--recursive | --recurse-submodules] [--] <repository>
> +	  [--recursive | --recurse-submodules] [--jobs <n>] [--] <repository>
>  	  [<directory>]
>  
>  DESCRIPTION
> @@ -216,6 +216,9 @@ objects from the source repository into a pack in the cloned repository.
>  	The result is Git repository can be separated from working
>  	tree.
>  
> +-j::
> +--jobs::

Judging from the way how "--depth <depth>" and other options with parameter are described, I think this should be:

          -j <n>::
          --jobs <n>::
> +	The number of submodules fetched at the same time.
Do we want to say "Defaults to submodule.jobs" somewhere?
Show 48 quoted lines
>  
>  <repository>::
>  	The (possibly remote) repository to clone from.  See the
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 5864ad1..b8b1d4c 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -50,6 +50,7 @@ static int option_progress = -1;
>  static struct string_list option_config;
>  static struct string_list option_reference;
>  static int option_dissociate;
> +static int max_jobs = -1;
>  
>  static struct option builtin_clone_options[] = {
>  	OPT__VERBOSITY(&option_verbosity),
> @@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
>  		    N_("initialize submodules in the clone")),
>  	OPT_BOOL(0, "recurse-submodules", &option_recursive,
>  		    N_("initialize submodules in the clone")),
> +	OPT_INTEGER('j', "jobs", &max_jobs,
> +		    N_("number of submodules cloned in parallel")),
>  	OPT_STRING(0, "template", &option_template, N_("template-directory"),
>  		   N_("directory from which templates will be used")),
>  	OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
> @@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
>  	OPT_END()
>  };
>  
> -static const char *argv_submodule[] = {
> -	"submodule", "update", "--init", "--recursive", NULL
> -};
> -
>  static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
>  {
>  	static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
> @@ -674,8 +673,23 @@ static int checkout(void)
>  	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
>  			   sha1_to_hex(sha1), "1", NULL);
>  
> -	if (!err && option_recursive)
> -		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
> +	if (!err && option_recursive) {
> +		struct argv_array args = ARGV_ARRAY_INIT;
> +		argv_array_pushl(&args, "submodule", "update", "--init", "--recursive", NULL);
> +
> +		if (max_jobs == -1)
> +			if (git_config_get_int("submodule.jobs", &max_jobs))
> +				max_jobs = 1;

This is somewhat an irregular way to handle a configuration variable. Usually we instead do:

	* initialize a variable to "unspecified" (e.g. -1);
        * let git_config() callback to overwrite the variable;
        * let parse_options() to overwrite the variable.

so that you can just use the variable at the use site like this function, knowing that the variable is already set with the correct precedence order.

Besides, if you really cared what the value of submodule.jobs is, shouldn't you be calling config_parallel_submodules()? I'd also think that you do not want to read that variable here in the first place (see below)...

Show 6 quoted lines
> +		if (max_jobs != 1) {
> +			struct strbuf sb = STRBUF_INIT;
> +			strbuf_addf(&sb, "--jobs=%d", max_jobs);
> +			argv_array_push(&args, sb.buf);
> +			strbuf_release(&sb);
> +		}

I am tempted to suggest that you should not pay attention to "submodule.jobs" in this command at all and just pass through "--jobs=$max_jobs" that was specified from the command line, as the spawned "submodule update --init --recursive" would handle "submodule.jobs" itself.

Once you start allowing "clone.jobs" as a more specific version of "submodule.jobs", then reading max_jobs first from "clone.jobs" and then from the command line starts to make sense. When neither is specified, you would spawn "submodule update --init --recursive" without any explicit "-j N" and let it honor its more generic "submodule.jobs" setting; otherwise, you would run it with "-j N" to override that more generic "submodule.jobs" setting with either the value the command line -j given to "clone" or specified by a more specific "clone.jobs".

> +		err = run_command_v_opt(args.argv, RUN_GIT_CMD);
> +		argv_array_clear(&args);
> +	}
Thanks.
Previous: Stefan BellerNext: Stefan Beller
Message 13 of 48 in “Expose the submodule parallelism to the user”
  1. 0/9 Expose the submodule parallelism to the userStefan Beller, Oct 27, 2015
  2. 1/9 submodule-config: "goto" removal in parse_config()Stefan Beller, Oct 27, 2015
  3. Jonathan NiederOct 27, 2015
  4. Junio C HamanoOct 27, 2015
  5. 2/9 submodule config: keep update strategy aroundStefan Beller, Oct 27, 2015
  6. 3/9 run_processes_parallel: Add output to tracing messagesStefan Beller, Oct 27, 2015
  7. 4/9 git submodule update: have a dedicated helper for cloningStefan Beller, Oct 27, 2015
  8. 5/9 submodule update: expose parallelism to the userStefan Beller, Oct 27, 2015
  9. Junio C HamanoOct 27, 2015
  10. Stefan BellerOct 28, 2015
  11. Junio C HamanoOct 28, 2015
  12. 6/9 clone: allow an explicit argument for parallel submodule clonesStefan Beller, Oct 27, 2015
  13. Junio C HamanoOct 27, 2015
  14. Stefan BellerOct 28, 2015
  15. 7/9 submodule config: remove name_and_item_from_varStefan Beller, Oct 27, 2015
  16. 8/9 submodule-config: parse_configStefan Beller, Oct 27, 2015
  17. 9/9 fetching submodules: Respect `submodule.jobs` config optionStefan Beller, Oct 27, 2015
  18. Junio C HamanoOct 27, 2015
  19. Junio C HamanoOct 27, 2015
  20. 0/8 Expose the submodule parallelism to the userStefan Beller, Oct 28, 2015
  21. 1/8 run_processes_parallel: Add output to tracing messagesStefan Beller, Oct 28, 2015
  22. Eric SunshineOct 30, 2015
  23. Stefan BellerOct 30, 2015
  24. 2/8 submodule config: keep update strategy aroundStefan Beller, Oct 28, 2015
  25. Eric SunshineOct 30, 2015
  26. Stefan BellerOct 30, 2015
  27. Eric SunshineOct 30, 2015
  28. Stefan BellerOct 30, 2015
  29. 3/8 submodule config: remove name_and_item_from_varStefan Beller, Oct 28, 2015
  30. Eric SunshineOct 30, 2015
  31. Stefan BellerOct 30, 2015
  32. 4/8 submodule-config: parse_configStefan Beller, Oct 28, 2015
  33. Eric SunshineOct 30, 2015
  34. Stefan BellerOct 30, 2015
  35. 5/8 fetching submodules: Respect `submodule.jobs` config optionStefan Beller, Oct 28, 2015
  36. Eric SunshineOct 30, 2015
  37. 6/8 git submodule update: have a dedicated helper for cloningStefan Beller, Oct 28, 2015
  38. Junio C HamanoOct 29, 2015
  39. 7/8 submodule update: expose parallelism to the userStefan Beller, Oct 28, 2015
  40. 8/8 clone: allow an explicit argument for parallel submodule clonesStefan Beller, Oct 28, 2015
  41. Eric SunshineNov 1, 2015
  42. Ramsay JonesOct 29, 2015
  43. Stefan BellerOct 29, 2015
  44. Junio C HamanoOct 29, 2015
  45. Stefan BellerOct 29, 2015
  46. Ramsay JonesOct 29, 2015
  47. Stefan BellerNov 3, 2015
  48. Junio C HamanoOct 29, 2015

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.