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

Re: [PATCHv2 6/8] git submodule update: have a dedicated helper for cloning

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 29, 2015, 22:34 UTC
Message-ID
<xmqqfv0tqp6u.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1446074504-6014-7-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 13 quoted lines
> +struct submodule_update_clone {
> +	int count;
> +	int quiet;
> +	int print_unmatched;
> +	char *reference;
> +	char *depth;
> +	char *update;
> +	const char *recursive_prefix;
> +	const char *prefix;
> +	struct module_list list;
> +	struct string_list projectlines;
> +	struct pathspec pathspec;
> +};

These fields should be split into at least two classes, the ones that are primarily the "configuration", and the others that are "states". I am guessing 'quiet' is what the caller prepares and tells the pp callbacks that they must work with reduced verbosity, and 'print_unmatched' is also in the same boat. From the above structure definition, nobody can guess what 'count' represents. Is that the number of modules you have in the top-level superproject? Is that the number of modules updated so far? Some other number?

We can guess "list" is probably the list of modules to be cloned or updated, but we have no idea what "projectlines" mean and what it will be used for. The only word with 'project' we would use in the context of discussing submodules is the "top level superproject", but then that will not need a "list", so that is not it. Perhaps this refers to a list of projects bound to our tree as submodules, and perhaps each such submodule gives some kind of "lines", but it is totally unclear what kind of lines they use.

Show 20 quoted lines
> +static void fill_clone_command(struct child_process *cp, int quiet,
> +			       const char *prefix, const char *path,
> +			       const char *name, const char *url,
> +			       const char *reference, const char *depth)
> +{
> +	cp->git_cmd = 1;
> +	cp->no_stdin = 1;
> +	cp->stdout_to_stderr = 1;
> +	cp->err = -1;
> +	argv_array_push(&cp->args, "submodule--helper");
> +	argv_array_push(&cp->args, "clone");
> +	if (quiet)
> +		argv_array_push(&cp->args, "--quiet");
> +
> +	if (prefix) {
> +		argv_array_push(&cp->args, "--prefix");
> +		argv_array_push(&cp->args, prefix);
> +	}
> +	argv_array_push(&cp->args, "--path");
> +	argv_array_push(&cp->args, path);

The pattern makes readers wish if there were a way to make these pair of pushes easier to read. The best I can come up with is

    argv_array_pushl(&cp->args, "--path", path, NULL);

While that would be already a vast improvement, when we know there are many "I want to push two", it makes me wonder if I am entitled to find the repeated ", NULL" irritating.

    argv_array_push2(&cp->args, "--path", path);
on the hand feels slightly too specific.  I dunno.
Show 32 quoted lines
> +static int update_clone_get_next_task(void **pp_task_cb,
> +				      struct child_process *cp,
> +				      struct strbuf *err,
> +				      void *pp_cb)
> +{
> +	struct submodule_update_clone *pp = pp_cb;
> +
> +	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 sb = STRBUF_INIT;
> +		const char *update_module = NULL;
> +		char *url = NULL;
> +		int just_cloned = 0;
> +
> +		if (ce_stage(ce)) {
> +			if (pp->recursive_prefix)
> +				strbuf_addf(err, "Skipping unmerged submodule %s/%s\n",
> +					pp->recursive_prefix, ce->name);
> +			else
> +				strbuf_addf(err, "Skipping unmerged submodule %s\n",
> +					ce->name);
> +			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;
> +			return 0;

This feels a bit inconsistent. When the pp->count'th submodule is set not to update (i.e. "none" below), you let this loop to ignore that submodule and continue on to process pp->count+1'th one without returning to the caller. Is there a reason why this case should be processed differently? If the rest of the code treats this condition as a "grave error" that tells the caller to never call get-next again (i.e. the "emergency abort" condition), that sort of makes sense, but I cannot offhand see if that is being done in this patch.

Show 21 quoted lines
> +		}
> +
> +		if (pp->recursive_prefix)
> +			displaypath = relative_path(pp->recursive_prefix, ce->name, &sb);
> +		else
> +			displaypath = ce->name;
> +
> +		if (pp->update)
> +			update_module = pp->update;
> +		if (!update_module)
> +			update_module = sub->update;
> +		if (!update_module)
> +			update_module = "checkout";
> +		if (!strcmp(update_module, "none")) {
> +			strbuf_addf(err, "Skipping submodule '%s'\n", displaypath);
> +			continue;
> +		}
> +
> +		/*
> +		 * Looking up the url in .git/config.
> +		 * We cannot fall back to .gitmodules as we only want to process
s/cannot/must not/, right?
> +		 * configured submodules. This renders the submodule lookup API
> +		 * useless, as it cannot lookup without fallback.
> +		 */

I doubt the value of the last sentence, especially the "useless" part.

Either "We do not want to read .gitmodules and that is why we do not use submodule config API, period" (which does not make it "useless", it is just not meant to be used here at all), or "We do not want to read .gitmodules in this codepath, and submodule config API cannot be used here before we teach it an option to only check the config without falling back" (which does not make it "useless", it is just that you haven't made it ready to be used here yet).

Show 17 quoted lines
> +		strbuf_reset(&sb);
> +		strbuf_addf(&sb, "submodule.%s.url", sub->name);
> +		git_config_get_string(sb.buf, &url);
> +		if (!url) {
> +			/*
> +			 * Only mention uninitialized submodules when its
> +			 * 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_reset(&sb);
> +		strbuf_addf(&sb, "%s/.git", ce->name);
> +		just_cloned = !file_exists(sb.buf);

That name was misleading and had me scratch my head for a while. This module is in the "needs cloning" state, and you haven't even started cloning it yet.

Show 17 quoted lines
> +		strbuf_reset(&sb);
> +		strbuf_addf(&sb, "%06o %s %d %d\t%s\n", ce->ce_mode,
> +				sha1_to_hex(ce->sha1), ce_stage(ce),
> +				just_cloned, ce->name);
> +		string_list_append(&pp->projectlines, sb.buf);
> +
> +		if (just_cloned) {
> +			fill_clone_command(cp, pp->quiet, pp->prefix, ce->name,
> +					   sub->name, url, pp->reference, pp->depth);
> +			pp->count++;
> +			free(url);
> +			return 1;
> +		} else
> +			free(url);
> +	}
> +	return 0;
> +}
That's it for today.  I'll take a look at the remainder another day.
Thanks.
Previous: Stefan BellerNext: Stefan Beller
Message 38 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.