[PATCHv8 0/9] Expose submodule parallelism to the user

[Stefan Beller <sbeller@google.com>]

This replaces sb/submodule-parallel-update.
(which is based on sb/submodule-parallel-fetch,
but this series also applies cleanly on master)

* using a "more" correct version of parsing the section title
* fixed the memleaks, free-after-use in
  "git submodule update: have a dedicated helper for cloning"
* reworded documentation.
* another additional patch snuck in, which makes code a bit more readable
  (0004-submodule-config-slightly-simplify-lookup_or_create_.patch)

Thanks,
Stefan

Interdiff to v7:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index eda3276..3b02732 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2647,11 +2647,10 @@ submodule.<name>.ignore::
 	affected by this setting.
 
 submodule.fetchJobs::
-	This is used to determine how many submodules will be
-	fetched/cloned at the same time. Specifying a positive integer
-	allows up to that number of submodules being fetched in parallel.
-	This is used in fetch and clone operations only. A value of 0 will
-	give some reasonable configuration. It defaults to 1.
+	Specifies how many submodules are fetched/cloned at the same time.
+	A positive integer allows up to that number of submodules fetched
+	in parallel. A value of 0 will give some reasonable default.
+	If unset, it defaults to 1.
 
 tag.sort::
 	This variable controls the sort ordering of tags when displayed by
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8002187..7e01b85 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -312,33 +312,31 @@ static int update_clone_get_next_task(struct child_process *cp,
 
 	for (; pp->count < pp->list.nr; pp->count++) {
 		const struct submodule *sub = NULL;
-		const char *displaypath = NULL;
 		const struct cache_entry *ce = pp->list.entries[pp->count];
+		struct strbuf displaypath_sb = STRBUF_INIT;
 		struct strbuf sb = STRBUF_INIT;
+		const char *displaypath = NULL;
 		const char *update_module = NULL;
 		char *url = NULL;
 		int needs_cloning = 0;
 
 		if (ce_stage(ce)) {
 			if (pp->recursive_prefix)
-				strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
+				strbuf_addf(err,
+					"Skipping unmerged submodule %s/%s\n",
 					pp->recursive_prefix, ce->name);
 			else
-				strbuf_addf(err, "Skipping unmerged submodule %s\n",
+				strbuf_addf(err,
+					"Skipping unmerged submodule %s\n",
 					ce->name);
-			continue;
+			goto cleanup_and_continue;
 		}
 
 		sub = submodule_from_path(null_sha1, ce->name);
-		if (!sub) {
-			strbuf_addf(err, "BUG: internal error managing submodules. "
-				    "The cache could not locate '%s'", ce->name);
-			pp->print_unmatched = 1;
-			continue;
-		}
 
 		if (pp->recursive_prefix)
-			displaypath = relative_path(pp->recursive_prefix, ce->name, &sb);
+			displaypath = relative_path(pp->recursive_prefix,
+						    ce->name, &displaypath_sb);
 		else
 			displaypath = ce->name;
 
@@ -349,14 +347,15 @@ static int update_clone_get_next_task(struct child_process *cp,
 		if (!update_module)
 			update_module = "checkout";
 		if (!strcmp(update_module, "none")) {
-			strbuf_addf(err, "Skipping submodule '%s'\n", displaypath);
-			continue;
+			strbuf_addf(err, "Skipping submodule '%s'\n",
+				    displaypath);
+			goto cleanup_and_continue;
 		}
 
 		/*
 		 * Looking up the url in .git/config.
-		 * We must not fall back to .gitmodules as we only want to process
-		 * configured submodules.
+		 * We must not fall back to .gitmodules as we only want
+		 * to process configured submodules.
 		 */
 		strbuf_reset(&sb);
 		strbuf_addf(&sb, "submodule.%s.url", sub->name);
@@ -367,9 +366,11 @@ static int update_clone_get_next_task(struct child_process *cp,
 			 * path have been specified
 			 */
 			if (pp->pathspec.nr)
-				strbuf_addf(err, _("Submodule path '%s' not initialized\n"
-					"Maybe you want to use 'update --init'?"), displaypath);
-			continue;
+				strbuf_addf(err,
+					    _("Submodule path '%s' not initialized\n"
+					    "Maybe you want to use 'update --init'?"),
+					    displaypath);
+			goto cleanup_and_continue;
 		}
 
 		strbuf_reset(&sb);
@@ -383,13 +384,19 @@ static int update_clone_get_next_task(struct child_process *cp,
 		string_list_append(&pp->projectlines, sb.buf);
 
 		if (needs_cloning) {
-			fill_clone_command(cp, pp->quiet, pp->prefix, ce->name,
-					   sub->name, url, pp->reference, pp->depth);
+			fill_clone_command(cp, pp->quiet, pp->prefix,
+					   ce->name, sub->name, strdup(url),
+					   pp->reference, pp->depth);
 			pp->count++;
-			free(url);
+		}
+
+cleanup_and_continue:
+		free(url);
+		strbuf_reset(&displaypath_sb);
+		strbuf_reset(&sb);
+
+		if (needs_cloning)
 			return 1;
-		} else
-			free(url);
 	}
 	return 0;
 }
diff --git a/submodule-config.c b/submodule-config.c
index a32259e..339b59d 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,7 +32,7 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
-static int parallel_jobs = -1;
+static unsigned long parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
 			   const struct submodule_entry *b,
@@ -163,22 +163,17 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
 }
 
 static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
-						  const unsigned char *gitmodules_sha1,
-						  const char *name_ptr, int name_len)
+		const unsigned char *gitmodules_sha1, const char *name)
 {
 	struct submodule *submodule;
-	struct strbuf name_buf = STRBUF_INIT;
-	char *name = xmemdupz(name_ptr, name_len);
 
 	submodule = cache_lookup_name(cache, gitmodules_sha1, name);
 	if (submodule)
-		goto out;
+		return submodule;
 
 	submodule = xmalloc(sizeof(*submodule));
 
-	strbuf_addstr(&name_buf, name);
-	submodule->name = strbuf_detach(&name_buf, NULL);
-
+	submodule->name = xstrdup(name);
 	submodule->path = NULL;
 	submodule->url = NULL;
 	submodule->update = NULL;
@@ -188,8 +183,7 @@ static struct submodule *lookup_or_create_by_name(struct submodule_cache *cache,
 	hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
 	cache_add(cache, submodule);
-out:
-	free(name);
+
 	return submodule;
 }
 
@@ -241,18 +235,16 @@ static int parse_generic_submodule_config(const char *key,
 					  struct parse_config_parameter *me)
 {
 	if (!strcmp(key, "fetchjobs")) {
-		parallel_jobs = strtol(value, NULL, 10);
-		if (parallel_jobs < 0) {
-			warning("submodule.fetchJobs not allowed to be negative.");
+		if (!git_parse_ulong(value, &parallel_jobs)) {
+			warning(_("Error parsing submodule.fetchJobs; Defaulting to 1."));
 			parallel_jobs = 1;
-			return 1;
 		}
 	}
 
 	return 0;
 }
 
-static int parse_specific_submodule_config(const char *subsection, int subsection_len,
+static int parse_specific_submodule_config(const char *subsection,
 					   const char *key,
 					   const char *var,
 					   const char *value,
@@ -262,7 +254,7 @@ static int parse_specific_submodule_config(const char *subsection, int subsectio
 	struct submodule *submodule;
 	submodule = lookup_or_create_by_name(me->cache,
 					     me->gitmodules_sha1,
-					     subsection, subsection_len);
+					     subsection);
 
 	if (!strcmp(key, "path")) {
 		if (!value)
@@ -332,19 +324,22 @@ static int parse_specific_submodule_config(const char *subsection, int subsectio
 static int parse_config(const char *var, const char *value, void *data)
 {
 	struct parse_config_parameter *me = data;
-	int subsection_len;
+	int subsection_len, ret;
 	const char *subsection, *key;
+	char *sub;
 
 	if (parse_config_key(var, "submodule", &subsection,
 			     &subsection_len, &key) < 0)
 		return 0;
 
-	if (!subsection_len)
+	if (!subsection)
 		return parse_generic_submodule_config(key, var, value, me);
-	else
-		return parse_specific_submodule_config(subsection,
-						       subsection_len, key,
-						       var, value, me);
+
+	sub = xmemdupz(subsection, subsection_len);
+	ret = parse_specific_submodule_config(sub, key,
+					      var, value, me);
+	free(sub);
+	return ret;
 }
 
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
@@ -493,7 +488,7 @@ void submodule_free(void)
 	is_cache_init = 0;
 }
 
-int config_parallel_submodules(void)
+unsigned long config_parallel_submodules(void)
 {
 	return parallel_jobs;
 }
diff --git a/submodule-config.h b/submodule-config.h
index d9bbf9a..bab1e8d 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -27,6 +27,6 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
 		const char *path);
 void submodule_free(void);
 
-int config_parallel_submodules(void);
+unsigned long config_parallel_submodules(void);
 
 #endif /* SUBMODULE_CONFIG_H */


Stefan Beller (9):
  submodule-config: keep update strategy around
  submodule-config: drop check against NULL
  submodule-config: remove name_and_item_from_var
  submodule-config: slightly simplify lookup_or_create_by_name
  submodule-config: introduce parse_generic_submodule_config
  fetching submodules: respect `submodule.fetchJobs` config option
  git submodule update: have a dedicated helper for cloning
  submodule update: expose parallelism to the user
  clone: allow an explicit argument for parallel submodule clones

 Documentation/config.txt        |   6 +
 Documentation/git-clone.txt     |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c                 |  19 +++-
 builtin/fetch.c                 |   2 +-
 builtin/submodule--helper.c     | 246 ++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh                |  54 ++++-----
 submodule-config.c              | 104 ++++++++++-------
 submodule-config.h              |   3 +
 submodule.c                     |   5 +
 t/t5526-fetch-submodules.sh     |  14 +++
 t/t7400-submodule-basic.sh      |   4 +-
 t/t7406-submodule-update.sh     |  27 +++++
 13 files changed, 414 insertions(+), 83 deletions(-)

-- 
2.7.0.rc0.41.gd102984.dirty