[PATCH v2 0/5] fetch: Extend --jobs to multiple remotes

[PATCH v2 0/5] fetch: Extend --jobs to multiple remotes

[Palmer Dabbelt]

It seems like the feedback for v1 was fairly positive, so I managed to
find some time to go ahead finish it.  The result is a much cleaner
patch set.  I think this could be merged in its current state, but there
are a few outstanding concerns I have:

* fetch.jobs isn't documented because I couldn't find any
  documentation for submodule.fetchjobs so I didn't know where to start
  writing.
* I took the complicated approach and added --submodule-fetch-jobs and
  --fetch-jobs before converting --jobs over to set both.  At the time I
  thought it wouldn't add too much extra complexity.  I wasn't looking
  closely enough and ended up with a custom parsing function which is a
  bit ugly.

I'm happy to fix either of these, but I wanted to send out the v2 before
going any farther because I wasn't sure if --jobs would be converted
over right away or if there was going to be a deprecation period.  I've
written the patch set such that the final patch can easily be dropped to
avoid changing existing behavior.

I'm also happy to re-spin this to just make --jobs control remotes and
drop the --fetch-jobs and --submodule-fetch-jobs arguments, which would
make the patch set somewhat simpler but preclude a deprecation period.

Changes since v1 <20190717015903.4384-1-palmer@sifive.com>:

* fetch_multiple() has been rewritten to use
  run_processes_parallel_tr2() rather than manage children on its own.
* The --fetch-jobs argument has been added.
* The --jobs arugemnt has been used, instead of the --parallel argument.

[PATCH v2 1/5] fetch: Rename max_children to max_children_for_submodules

[Palmer Dabbelt]

This does not change any functionality, but instead just prepares for
the upcoming "--fetch-jobs=<n>" support.  Essentially the "max_children"
variable is ambiguously named, which would complicate the diff in
squashed into the "--fetch-jobs=<n>" patch.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 builtin/fetch.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 717dd14e8961..8aa6a0caf1ab 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -54,7 +54,7 @@ static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosit
 static int progress = -1;
 static int enable_auto_gc = 1;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
-static int max_children = 1;
+static int max_children_for_submodules = 1;
 static enum transport_family family;
 static const char *depth;
 static const char *deepen_since;
@@ -96,7 +96,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "submodule.fetchjobs")) {
-		max_children = parse_submodule_fetchjobs(k, v);
+		max_children_for_submodules = parse_submodule_fetchjobs(k, v);
 		return 0;
 	} else if (!strcmp(k, "fetch.recursesubmodules")) {
 		recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
@@ -134,7 +134,7 @@ static struct option builtin_fetch_options[] = {
 		    N_("fetch all tags and associated objects"), TAGS_SET),
 	OPT_SET_INT('n', NULL, &tags,
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
-	OPT_INTEGER('j', "jobs", &max_children,
+	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
 		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
@@ -1633,7 +1633,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++)
 		strbuf_addf(&default_rla, " %s", argv[i]);
 
-	fetch_config_from_gitmodules(&max_children, &recurse_submodules);
+	fetch_config_from_gitmodules(&max_children_for_submodules,
+				     &recurse_submodules);
 	git_config(git_fetch_config, NULL);
 
 	argc = parse_options(argc, argv, prefix,
@@ -1716,7 +1717,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 						    recurse_submodules,
 						    recurse_submodules_default,
 						    verbosity < 0,
-						    max_children);
+						    max_children_for_submodules);
 		argv_array_clear(&options);
 	}
 
-- 
2.21.0

[PATCH v2 2/5] fetch: Add the "--fetch-jobs" option

[Palmer Dabbelt]

This argument allows users to select the number of jobs that will run in
parallel when fetching, with each job fetching from a different remote.
This is particularly useful when fetching from many remotes that change
slowly on a high latency link, as the fetch time is dominated by
handshake latency -- probably not the most common use case, but one that
ends up frusturating me with some frequency.

This patch implements --fetch-jobs via run_processes_parallel_tr2(),
which allows the user to restrict the amount of parallelism.  While this
adds a lot of boilerplate to the implementation, the pattern is fairly
straight-forward and matches the existing submodule parallel fetch code.
The submodule version of this is a lot more complicated and does things
like retrying and scheduling I/O, but I don't get bit by those issues so
it didn't seem worth the effort.

The speedup is quite noticable for me, even when I'm on a fast link --
my Linux fetches go from 6 seconds to 2 seconds, for example.

I've given this a quick test locally, but haven't done anything
exhaustive like attempting to fetch against broken remotes (where the
intent is that the other remotes are fetched and the result is an
error).

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 Documentation/fetch-options.txt |   5 ++
 builtin/fetch.c                 | 141 +++++++++++++++++++++++++++-----
 2 files changed, 126 insertions(+), 20 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 3c9b4f9e0951..dbd2add686dd 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -165,6 +165,11 @@ ifndef::git-pull[]
 	submodules will be faster. By default submodules will be fetched
 	one at a time.
 
+--fetch-jobs=<n>::
+	Number of parallel children to be used for fetching.  Each will fetch
+	from different remotes, such that fetching from many remotes will be
+	faster.  By default remotes will be fetched from one at a time.
+
 --no-recurse-submodules::
 	Disable recursive fetching of submodules (this has the same effect as
 	using the `--recurse-submodules=no` option).
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8aa6a0caf1ab..fa12ad44e7d9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -55,6 +55,7 @@ static int progress = -1;
 static int enable_auto_gc = 1;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children_for_submodules = 1;
+static int max_children_for_fetch = 1;
 static enum transport_family family;
 static const char *depth;
 static const char *deepen_since;
@@ -136,6 +137,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
 	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
 		    N_("number of submodules fetched in parallel")),
+	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
+		    N_("number of remotes fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,
@@ -1463,10 +1466,100 @@ static void add_options_to_argv(struct argv_array *argv)
 
 }
 
-static int fetch_multiple(struct string_list *list)
+/*
+ * Support for fetching multiple remotes in parallel.  This is spread over
+ * multiple functions and structures, all of which are helpers for
+ * fench_multiple().  The general idea is that there is a single struct
+ * fetch_remote, which contains the entire state for a fetch_multiple()
+ * instance.
+ */
+struct fetch_remote_task {
+	int in_use;
+	struct argv_array argv;
+};
+
+struct fetch_remote {
+	struct string_list *all_remotes;
+	int next_remote_index;
+	struct fetch_remote_task *all_tasks;
+	int task_count;
+	int result;
+};
+
+static int next_remote_to_fetch(struct child_process *cp,
+				struct strbuf *out,
+				void *state_uncast,
+				void **task_state_out)
+{
+	int i;
+	struct fetch_remote *state = state_uncast;
+	struct fetch_remote_task *task_state = NULL;
+	const char *remote_name;
+
+	if (state->next_remote_index >= state->all_remotes->nr)
+		return 0;
+
+	remote_name = state->all_remotes->items[state->next_remote_index].string;
+	state->next_remote_index++;
+
+	/*
+	 * Finds somewhere to store the state for a task.  This is guarnteed to
+	 * succeed because there are always enough tasks allocated to cover the
+	 * number that have been requested to run in parallel.  Rather than
+	 * bothering with some sort of free list, this just brute force
+	 * searches for a free task.  The assumption is that there aren't that
+	 * many tasks to look through.
+	 */
+	for (i = 0; i < state->task_count; ++i) {
+		if (!state->all_tasks[i].in_use) {
+			task_state = state->all_tasks + i;
+			break;
+		}
+	}
+	assert(task_state != NULL);
+	task_state->in_use = 1;
+
+	*task_state_out = task_state;
+	argv_array_push(&task_state->argv, remote_name);
+	cp->argv = task_state->argv.argv;
+	cp->git_cmd = 1;
+
+	printf(_("Fetching %s\n"), remote_name);
+
+	return 1;
+}
+
+static int remote_fetch_failed_to_start(struct strbuf *out,
+					void *state_uncast,
+					void *task_state_uncast)
+{
+	struct fetch_remote *state = state_uncast;
+	struct fetch_remote_task *task_state = task_state_uncast;
+	assert(task_state != NULL);
+	argv_array_pop(&task_state->argv);
+	task_state->in_use = 0;
+	state->result |= -1;
+	return 0;
+}
+
+static int remote_fetch_finished(int result,
+				 struct strbuf *out,
+				 void *state_uncast,
+				 void *task_state_uncast)
+{
+	struct fetch_remote *state = state_uncast;
+	struct fetch_remote_task *task_state = task_state_uncast;
+	assert(task_state != NULL);
+	argv_array_pop(&task_state->argv);
+	task_state->in_use = 0;
+	state->result |= result;
+	return 0;
+}
+
+static int fetch_multiple(struct string_list *all_remotes, int max_children)
 {
 	int i, result = 0;
-	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct fetch_remote state;
 
 	if (!append && !dry_run) {
 		int errcode = truncate_fetch_head();
@@ -1474,23 +1567,31 @@ static int fetch_multiple(struct string_list *list)
 			return errcode;
 	}
 
-	argv_array_pushl(&argv, "fetch", "--append", "--no-auto-gc", NULL);
-	add_options_to_argv(&argv);
-
-	for (i = 0; i < list->nr; i++) {
-		const char *name = list->items[i].string;
-		argv_array_push(&argv, name);
-		if (verbosity >= 0)
-			printf(_("Fetching %s\n"), name);
-		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
-			error(_("Could not fetch %s"), name);
-			result = 1;
-		}
-		argv_array_pop(&argv);
-	}
-
-	argv_array_clear(&argv);
-	return result;
+	state.all_remotes = all_remotes;
+	state.next_remote_index = 0;
+	state.all_tasks = xcalloc(sizeof(*state.all_tasks), max_children);
+	state.task_count = max_children;
+	state.result = 0;
+
+	for (i = 0; i < max_children; ++i) {
+		struct argv_array *argv = &state.all_tasks[i].argv;
+		state.all_tasks[i].in_use = 0;
+		argv_array_init(argv);
+		argv_array_pushl(argv, "fetch", "--append", "--no-auto-gc", NULL);
+		add_options_to_argv(argv);
+	};
+
+	result = run_processes_parallel_tr2(max_children,
+					    &next_remote_to_fetch,
+					    &remote_fetch_failed_to_start,
+					    &remote_fetch_finished,
+					    &state,
+					    "fetch", "parallel");
+
+	for (i = 0; i < max_children; ++i)
+		argv_array_clear(&state.all_tasks[i].argv);
+	free(state.all_tasks);
+	return result | state.result;
 }
 
 /*
@@ -1704,7 +1805,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			die(_("--filter can only be used with the remote "
 			      "configured in extensions.partialclone"));
 		/* TODO should this also die if we have a previous partial-clone? */
-		result = fetch_multiple(&list);
+		result = fetch_multiple(&list, max_children_for_fetch);
 	}
 
 	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
-- 
2.21.0

Re: [PATCH v2 2/5] fetch: Add the "--fetch-jobs" option

[Eric Wong]

Palmer Dabbelt <palmer@sifive.com> wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8aa6a0caf1ab..fa12ad44e7d9 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c

<snip>

> +static int next_remote_to_fetch(struct child_process *cp,
> +				struct strbuf *out,
> +				void *state_uncast,
> +				void **task_state_out)
> +{
> +	int i;
> +	struct fetch_remote *state = state_uncast;
> +	struct fetch_remote_task *task_state = NULL;
> +	const char *remote_name;
> +
> +	if (state->next_remote_index >= state->all_remotes->nr)
> +		return 0;
> +
> +	remote_name = state->all_remotes->items[state->next_remote_index].string;
> +	state->next_remote_index++;
> +
> +	/*
> +	 * Finds somewhere to store the state for a task.  This is guarnteed to
> +	 * succeed because there are always enough tasks allocated to cover the
> +	 * number that have been requested to run in parallel.  Rather than
> +	 * bothering with some sort of free list, this just brute force
> +	 * searches for a free task.  The assumption is that there aren't that
> +	 * many tasks to look through.
> +	 */
> +	for (i = 0; i < state->task_count; ++i) {
> +		if (!state->all_tasks[i].in_use) {
> +			task_state = state->all_tasks + i;
> +			break;
> +		}
> +	}

Fwiw, I added list.h, the linked-list derived from the Linux
kernel to simplify usage of free lists, queues, etc...

I think it could improve readability, too; but I'm not really
a C programmer and prefer high-level scripting languages.

[PATCH v2 3/5] fetch: Add the fetch.jobs config key

[Palmer Dabbelt]

This allows users to default to parallel fetches, so they don't have to
pass --fetch-jobs=N on the command line every time.  The implementation
matches submodule.fetchjobs by die()ing on invalid job counts.

I couldn't find any documentation for submodule.fetchjobs, so I didn't
write any for this one.  I've tested this in my config and in
conjunction with the command-line argument.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 builtin/fetch.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index fa12ad44e7d9..4c5f2ea3a931 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -104,6 +104,13 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "fetch.jobs")) {
+		max_children_for_fetch = git_config_int(k, v);
+		if (max_children_for_fetch < 0)
+			die(_("negative values not allowed for fetch.jobs"));
+		return 0;
+	}
+
 	return git_default_config(k, v, cb);
 }
 
-- 
2.21.0

[PATCH v2 4/5] fetch: Add the --submodule-fetch-jobs option

[Palmer Dabbelt]

This is exactly the same as --jobs, but the more explicit name will
allow us to later change the behavior of --jobs to control both the
number of jobs for remotes as well as submodules.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 Documentation/fetch-options.txt | 1 +
 builtin/fetch.c                 | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index dbd2add686dd..5836024f1934 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -160,6 +160,7 @@ ifndef::git-pull[]
 
 -j::
 --jobs=<n>::
+--submodule-fetch-jobs=<n>::
 	Number of parallel children to be used for fetching submodules.
 	Each will fetch from different submodules, such that fetching many
 	submodules will be faster. By default submodules will be fetched
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4c5f2ea3a931..67d001f3f78b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -146,6 +146,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("number of submodules fetched in parallel")),
 	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
 		    N_("number of remotes fetched in parallel")),
+	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
+		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,
-- 
2.21.0

[PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Palmer Dabbelt]

The existing --jobs argument was defined to control the number of jobs
used for submodule fetching, but it makes more sense to have this
argument control the number of jobs to be used when fetching from
multiple remotes as well.

This patch simply changes the --jobs argument parsing code to set both
max_children_for_{submodules,fetch}, as well as noting this new behavior
in the documentation.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 Documentation/fetch-options.txt |  4 ++++
 builtin/fetch.c                 | 21 ++++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 5836024f1934..0915fd4ed6d5 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -160,6 +160,10 @@ ifndef::git-pull[]
 
 -j::
 --jobs=<n>::
+	Number of parallel children to be used for all forms of fetching.
+	This is the same as passing `--submodule-fetch-jobs=<n>` and
+	`--fetch-jobs=<n>`.
+
 --submodule-fetch-jobs=<n>::
 	Number of parallel children to be used for fetching submodules.
 	Each will fetch from different submodules, such that fetching many
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67d001f3f78b..41498e9efb3b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	return git_default_config(k, v, cb);
 }
 
+static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
+{
+	int jobs;
+
+	jobs = atoi(arg);
+	if (jobs < 1)
+		die(_("There must be a positive number of jobs"));
+
+	max_children_for_submodules = jobs;
+	max_children_for_fetch = jobs;
+
+	return 0;
+}
+
 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
 {
 	BUG_ON_OPT_NEG(unset);
@@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
 		    N_("fetch all tags and associated objects"), TAGS_SET),
 	OPT_SET_INT('n', NULL, &tags,
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
-	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
+	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
+		    N_("number of parallel tasks to run while fetching"),
+		    PARSE_OPT_OPTARG, &parse_jobs_arg },
+	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
 		    N_("number of submodules fetched in parallel")),
 	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
 		    N_("number of remotes fetched in parallel")),
-	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
-		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,
-- 
2.21.0

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Johannes Schindelin]

Hi,


On Mon, 12 Aug 2019, Palmer Dabbelt wrote:

> The existing --jobs argument was defined to control the number of jobs
> used for submodule fetching, but it makes more sense to have this
> argument control the number of jobs to be used when fetching from
> multiple remotes as well.
>
> This patch simply changes the --jobs argument parsing code to set both
> max_children_for_{submodules,fetch}, as well as noting this new behavior
> in the documentation.
>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---

I very much miss in this description a reflection of my analysis in
https://public-inbox.org/git/nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet/

Given that analysis, combined with the fact that the `--jobs` option
tries to control both the `--multiple` and `--recursive-submodules` code
paths in the end, anyway, I do doubt that it makes sense to even
introduce the `--fetch-jobs` and the `--submodule-fetch-jobs` options;
They are probably only confusing and do not add much benefit to the end
user.

Ciao,
Johannes

>  Documentation/fetch-options.txt |  4 ++++
>  builtin/fetch.c                 | 21 ++++++++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 5836024f1934..0915fd4ed6d5 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -160,6 +160,10 @@ ifndef::git-pull[]
>
>  -j::
>  --jobs=<n>::
> +	Number of parallel children to be used for all forms of fetching.
> +	This is the same as passing `--submodule-fetch-jobs=<n>` and
> +	`--fetch-jobs=<n>`.
> +
>  --submodule-fetch-jobs=<n>::
>  	Number of parallel children to be used for fetching submodules.
>  	Each will fetch from different submodules, such that fetching many
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 67d001f3f78b..41498e9efb3b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  	return git_default_config(k, v, cb);
>  }
>
> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
> +{
> +	int jobs;
> +
> +	jobs = atoi(arg);
> +	if (jobs < 1)
> +		die(_("There must be a positive number of jobs"));
> +
> +	max_children_for_submodules = jobs;
> +	max_children_for_fetch = jobs;
> +
> +	return 0;
> +}
> +
>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>  	OPT_SET_INT('n', NULL, &tags,
>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
> +		    N_("number of parallel tasks to run while fetching"),
> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
> +	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
>  		    N_("number of submodules fetched in parallel")),
>  	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
>  		    N_("number of remotes fetched in parallel")),
> -	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
> -		    N_("number of submodules fetched in parallel")),
>  	OPT_BOOL('p', "prune", &prune,
>  		 N_("prune remote-tracking branches no longer on remote")),
>  	OPT_BOOL('P', "prune-tags", &prune_tags,
> --
> 2.21.0
>
>

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Palmer Dabbelt]

On Tue, 13 Aug 2019 13:16:11 PDT (-0700), Johannes.Schindelin@gmx.de wrote:
> Hi,
>
>
> On Mon, 12 Aug 2019, Palmer Dabbelt wrote:
>
>> The existing --jobs argument was defined to control the number of jobs
>> used for submodule fetching, but it makes more sense to have this
>> argument control the number of jobs to be used when fetching from
>> multiple remotes as well.
>>
>> This patch simply changes the --jobs argument parsing code to set both
>> max_children_for_{submodules,fetch}, as well as noting this new behavior
>> in the documentation.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>> ---
>
> I very much miss in this description a reflection of my analysis in
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet/
>
> Given that analysis, combined with the fact that the `--jobs` option
> tries to control both the `--multiple` and `--recursive-submodules` code
> paths in the end, anyway, I do doubt that it makes sense to even
> introduce the `--fetch-jobs` and the `--submodule-fetch-jobs` options;
> They are probably only confusing and do not add much benefit to the end
> user.

The cover letter at least attempts to describe this.  I figured I'd have to 
pick one option for a v2, so I went with the more complicated one under the 
assumption it would be easy to re-spin a v3 that drops the extra arguments.  
I'm happy to do so.

>
> Ciao,
> Johannes
>
>>  Documentation/fetch-options.txt |  4 ++++
>>  builtin/fetch.c                 | 21 ++++++++++++++++++---
>>  2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
>> index 5836024f1934..0915fd4ed6d5 100644
>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -160,6 +160,10 @@ ifndef::git-pull[]
>>
>>  -j::
>>  --jobs=<n>::
>> +	Number of parallel children to be used for all forms of fetching.
>> +	This is the same as passing `--submodule-fetch-jobs=<n>` and
>> +	`--fetch-jobs=<n>`.
>> +
>>  --submodule-fetch-jobs=<n>::
>>  	Number of parallel children to be used for fetching submodules.
>>  	Each will fetch from different submodules, such that fetching many
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 67d001f3f78b..41498e9efb3b 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>>  	return git_default_config(k, v, cb);
>>  }
>>
>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
>> +{
>> +	int jobs;
>> +
>> +	jobs = atoi(arg);
>> +	if (jobs < 1)
>> +		die(_("There must be a positive number of jobs"));
>> +
>> +	max_children_for_submodules = jobs;
>> +	max_children_for_fetch = jobs;
>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>  	BUG_ON_OPT_NEG(unset);
>> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>>  	OPT_SET_INT('n', NULL, &tags,
>>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
>> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
>> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
>> +		    N_("number of parallel tasks to run while fetching"),
>> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
>> +	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
>>  		    N_("number of submodules fetched in parallel")),
>>  	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
>>  		    N_("number of remotes fetched in parallel")),
>> -	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
>> -		    N_("number of submodules fetched in parallel")),
>>  	OPT_BOOL('p', "prune", &prune,
>>  		 N_("prune remote-tracking branches no longer on remote")),
>>  	OPT_BOOL('P', "prune-tags", &prune_tags,
>> --
>> 2.21.0
>>
>>

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Junio C Hamano]

Palmer Dabbelt <palmer@sifive.com> writes:

> The existing --jobs argument was defined to control the number of jobs
> used for submodule fetching, but it makes more sense to have this
> argument control the number of jobs to be used when fetching from
> multiple remotes as well.
>
> This patch simply changes the --jobs argument parsing code to set both
> max_children_for_{submodules,fetch}, as well as noting this new behavior
> in the documentation.

That's a sensible, if overly careful, transition plan.  This patch
cannot be queued together with the other four, though, for the plan
to be practical.  It probably needs to come a few releases after the
other four hits a release.

A less involved and much more careless transition plan may be to
just declare that "--jobs that only controls submodule fetches is a
bug and it must also affect how fetches from multiple remote
repositories are done" and come directly to this step, without
necessarily introducing options that control them independently.

I have a suspicion that we can afford to go the careless route for
this particular one, if we wanted to, as not may people would care
about controlling these independently.

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Palmer Dabbelt]

On Tue, 13 Aug 2019 15:00:13 PDT (-0700), gitster@pobox.com wrote:
> Palmer Dabbelt <palmer@sifive.com> writes:
>
>> The existing --jobs argument was defined to control the number of jobs
>> used for submodule fetching, but it makes more sense to have this
>> argument control the number of jobs to be used when fetching from
>> multiple remotes as well.
>>
>> This patch simply changes the --jobs argument parsing code to set both
>> max_children_for_{submodules,fetch}, as well as noting this new behavior
>> in the documentation.
>
> That's a sensible, if overly careful, transition plan.  This patch
> cannot be queued together with the other four, though, for the plan
> to be practical.  It probably needs to come a few releases after the
> other four hits a release.
>
> A less involved and much more careless transition plan may be to
> just declare that "--jobs that only controls submodule fetches is a
> bug and it must also affect how fetches from multiple remote
> repositories are done" and come directly to this step, without
> necessarily introducing options that control them independently.
>
> I have a suspicion that we can afford to go the careless route for
> this particular one, if we wanted to, as not may people would care
> about controlling these independently.

This was brought up as part of the v1, and the cover letter lays out a plan to 
do so.  I'm happy to re-spin the patch set to just have --jobs control 
everything.

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[SZEDER Gábor]

On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 67d001f3f78b..41498e9efb3b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  	return git_default_config(k, v, cb);
>  }
>  
> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
> +{
> +	int jobs;
> +
> +	jobs = atoi(arg);
> +	if (jobs < 1)
> +		die(_("There must be a positive number of jobs"));
> +
> +	max_children_for_submodules = jobs;
> +	max_children_for_fetch = jobs;
> +
> +	return 0;
> +}
> +
>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>  	OPT_SET_INT('n', NULL, &tags,
>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
> +		    N_("number of parallel tasks to run while fetching"),
> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },

These changes result segmentation faults in the tests '--quiet
propagates to parallel submodules' and 'fetching submodules respects
parallel settings' in 't5526-fetch-submodules.sh'.

If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an
unstuck argument of the option, as opposed to '-j2' or '--jobs=7',
then 'arg' in the parse_jobs_arg() callback is NULL, which then causes
the segfault somewhere inside that atoi() call.

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 67d001f3f78b..41498e9efb3b 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>>  	return git_default_config(k, v, cb);
>>  }
>>  
>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
>> +{
>> +	int jobs;
>> +
>> +	jobs = atoi(arg);
>> +	if (jobs < 1)
>> +		die(_("There must be a positive number of jobs"));
>> +
>> +	max_children_for_submodules = jobs;
>> +	max_children_for_fetch = jobs;
>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>  	BUG_ON_OPT_NEG(unset);
>> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>>  	OPT_SET_INT('n', NULL, &tags,
>>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
>> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
>> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
>> +		    N_("number of parallel tasks to run while fetching"),
>> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
>
> These changes result segmentation faults in the tests '--quiet
> propagates to parallel submodules' and 'fetching submodules respects
> parallel settings' in 't5526-fetch-submodules.sh'.
>
> If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an
> unstuck argument of the option, as opposed to '-j2' or '--jobs=7',
> then 'arg' in the parse_jobs_arg() callback is NULL, which then causes
> the segfault somewhere inside that atoi() call.

True.  

An easier and more readable way would be to set another "nr-jobs"
variable using plain vanilla OPT_INTEGER() and override the other
two with it when they are not set after parse_options() returns, I
guess.

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

[Palmer Dabbelt]

On Wed, 14 Aug 2019 01:32:45 PDT (-0700), szeder.dev@gmail.com wrote:
> On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 67d001f3f78b..41498e9efb3b 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>>  	return git_default_config(k, v, cb);
>>  }
>>
>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
>> +{
>> +	int jobs;
>> +
>> +	jobs = atoi(arg);
>> +	if (jobs < 1)
>> +		die(_("There must be a positive number of jobs"));
>> +
>> +	max_children_for_submodules = jobs;
>> +	max_children_for_fetch = jobs;
>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>  	BUG_ON_OPT_NEG(unset);
>> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>>  	OPT_SET_INT('n', NULL, &tags,
>>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
>> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
>> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
>> +		    N_("number of parallel tasks to run while fetching"),
>> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
>
> These changes result segmentation faults in the tests '--quiet
> propagates to parallel submodules' and 'fetching submodules respects
> parallel settings' in 't5526-fetch-submodules.sh'.
>
> If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an
> unstuck argument of the option, as opposed to '-j2' or '--jobs=7',
> then 'arg' in the parse_jobs_arg() callback is NULL, which then causes
> the segfault somewhere inside that atoi() call.

Thanks, I'll fix that in the v3.