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

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

From
BWBrandon Williams <bmwill@google.com>
Date
May 26, 2017, 16:44 UTC
Message-ID
<20170526164459.GB4592@google.com>
In-Reply-To
<20170526151713.10974-3-pc44800@gmail.com>
On 05/26, Prathamesh Chavan wrote:
Show 83 quoted lines
> This aims to make git-submodule foreach a builtin. This is the very
> first step taken in this direction. Hence, 'foreach' is ported to
> submodule--helper, and submodule--helper is called from git-submodule.sh.
> The code is split up to have one function to obtain all the list of
> submodules. This function acts as the front-end of git-submodule foreach
> subcommand. It calls the function for_each_submodule_list, which basically
> loops through the list and calls function fn, which in this case is
> runcommand_in_submodule. This third function is a calling function that
> takes care of running the command in that submodule, and recursively
> perform the same when --recursive is flagged.
> 
> The first function module_foreach first parses the options present in
> argv, and then with the help of module_list_compute, generates the list of
> submodules present in the current working tree.
> 
> The second function for_each_submodule_list traverses through the
> list, and calls function fn (which in case of submodule subcommand
> foreach is runcommand_in_submodule) is called for each entry.
> 
> The third function runcommand_in_submodule, generates a submodule struct sub
> for $name, value and then later prepends name=sub->name; and other
> value assignment to the env argv_array structure of a child_process.
> Also the <command> of submodule-foreach is push to args argv_array
> structure and finally, using run_command the commands are executed
> using a shell.
> 
> The third function also takes care of the recursive flag, by creating
> a separate child_process structure and prepending "--super-prefix displaypath",
> to the args argv_array structure. Other required arguments and the
> input <command> of submodule-foreach is also appended to this argv_array.
> 
> The commit 1c4fb136db (submodule foreach: skip eval for more than one
> argument, 2013-09-27), which explains that why for the case when argc>1,
> we do not use eval. But since in this patch, we are calling the
> command in a separate shell itself for all values of argc, this case
> is not considered separately.
> 
> Both env variable $path and $sm_path were added since both are used in
> tests in t7407.
> 
> Helped-by: Brandon Williams <bmwill@google.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> These series of patches passes the complete test suite.
> Its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds
> Branch: submodule-foreach
> Build #71
> 
>  builtin/submodule--helper.c | 148 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  39 +-----------
>  2 files changed, 149 insertions(+), 38 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 566a5b6a6..343b6269c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -13,6 +13,8 @@
>  #include "refs.h"
>  #include "connect.h"
>  
> +typedef void (*submodule_list_func_t)(const struct cache_entry *list_item, void *cb_data);
> +
>  static char *get_default_remote(void)
>  {
>  	char *dest = NULL, *ret;
> @@ -219,6 +221,26 @@ static int resolve_relative_url_test(int argc, const char **argv, const char *pr
>  	return 0;
>  }
>  
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> +	const char *super_prefix = get_super_prefix();
> +
> +	if (prefix && super_prefix) {
> +		BUG("cannot have prefix '%s' and superprefix '%s'",
> +		    prefix, super_prefix);
> +	} else if (prefix) {
> +		struct strbuf sb = STRBUF_INIT;
> +		char *displaypath;
> +		displaypath = xstrdup(relative_path(path, prefix, &sb));
These can probably go on the same line:
  
  char *displaypath = xstrdup(relative_path(path, prefix, &sb));
Show 61 quoted lines
> +		strbuf_release(&sb);
> +		return displaypath;
> +	} else if (super_prefix) {
> +		return xstrfmt("%s/%s", super_prefix, path);
> +	} else {
> +		return xstrdup(path);
> +	}
> +}
> +
>  struct module_list {
>  	const struct cache_entry **entries;
>  	int alloc, nr;
> @@ -331,6 +353,14 @@ static int module_list(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +static void for_each_submodule_list(const struct module_list list,
> +				    submodule_list_func_t fn, void *cb_data)
> +{
> +	int i;
> +	for (i = 0; i < list.nr; i++)
> +		fn(list.entries[i], cb_data);
> +}
> +
>  static void init_submodule(const char *path, const char *prefix, int quiet)
>  {
>  	const struct submodule *sub;
> @@ -487,6 +517,123 @@ static int module_name(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct cb_foreach {
> +	int argc;
> +	const char **argv;
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int recursive: 1;
> +};
> +#define CB_FOREACH_INIT { 0, NULL, NULL, 0, 0 }
> +
> +static void runcommand_in_submodule(const struct cache_entry *list_item,
> +				    void *cb_data)
> +{
> +	struct cb_foreach *info = cb_data;
> +	char *toplevel = xgetcwd();
> +	const struct submodule *sub;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	char* displaypath = NULL;
> +	int i;
> +
> +	/* Only loads from .gitmodules, no overlay with .git/config */
> +	gitmodules_config();
> +
> +	sub = submodule_from_path(null_sha1, list_item->name);
> +
> +	if (!sub)
> +		die(_("No url found for submodule path '%s' in .gitmodules"),
> +		      displaypath);
> +
> +	if (!is_submodule_populated_gently(list_item->name, NULL))
> +		return;

I missed one other memory leak from the last round. You should probably call xgetcwd() to fill 'toplevel' here to avoid leaking the memory if you do an early return.

Show 152 quoted lines
> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	prepare_submodule_repo_env(&cp.env_array);
> +	cp.use_shell = 1;
> +	cp.dir = list_item->name;
> +
> +	argv_array_pushf(&cp.env_array, "name=%s", sub->name);
> +	argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
> +	argv_array_pushf(&cp.env_array, "path=%s", list_item->name);
> +	argv_array_pushf(&cp.env_array, "sha1=%s", oid_to_hex(&list_item->oid));
> +	argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> +	for (i = 0; i < info->argc; i++)
> +		argv_array_push(&cp.args, info->argv[i]);
> +
> +	if (!info->quiet)
> +		printf(_("Entering '%s'\n"), displaypath);
> +
> +	if (info->argv[0] && run_command(&cp))
> +		die(_("run_command returned non-zero status for %s\n."),
> +		      displaypath);
> +
> +	if (info->recursive) {
> +		struct child_process cpr = CHILD_PROCESS_INIT;
> +
> +		cpr.git_cmd = 1;
> +		cpr.dir = list_item->name;
> +		prepare_submodule_repo_env(&cpr.env_array);
> +
> +		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
> +				 "submodule--helper", "foreach", "--recursive",
> +				 NULL);
> +
> +		if (info->quiet)
> +			argv_array_push(&cpr.args, "--quiet");
> +
> +		for (i = 0; i < info->argc; i++)
> +			argv_array_push(&cpr.args, info->argv[i]);
> +
> +		if (run_command(&cpr))
> +			die(_("run_command returned non-zero status while"
> +			      "recursing in the nested submodules of %s\n."),
> +			      displaypath);
> +	}
> +
> +	free(displaypath);
> +	free(toplevel);
> +}
> +
> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> +	struct cb_foreach info;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +
> +	struct option module_foreach_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			 N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper foreach [--quiet] [--recursive] <command>"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_foreach_options,
> +			     git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> +	if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> +		BUG("module_list_compute should not choke on empty pathspec");
> +
> +	info.argc = argc;
> +	info.argv = argv;
> +	info.prefix = prefix;
> +	info.quiet = quiet;
> +	info.recursive = recursive;
> +
> +	for_each_submodule_list(list, runcommand_in_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>  			   const char *depth, struct string_list *reference,
>  			   int quiet, int progress)
> @@ -1212,6 +1359,7 @@ static struct cmd_struct commands[] = {
>  	{"relative-path", resolve_relative_path, 0},
>  	{"resolve-relative-url", resolve_relative_url, 0},
>  	{"resolve-relative-url-test", resolve_relative_url_test, 0},
> +	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
>  	{"init", module_init, SUPPORT_SUPER_PREFIX},
>  	{"remote-branch", resolve_remote_submodule_branch, 0},
>  	{"push-check", push_check, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index ea6f56337..032fd2540 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -322,45 +322,8 @@ cmd_foreach()
>  		shift
>  	done
>  
> -	toplevel=$(pwd)
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper foreach ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
>  
> -	# dup stdin so that it can be restored when running the external
> -	# command in the subshell (and a recursive call to this function)
> -	exec 3<&0
> -
> -	{
> -		git submodule--helper list --prefix "$wt_prefix" ||
> -		echo "#unmatched" $?
> -	} |
> -	while read -r mode sha1 stage sm_path
> -	do
> -		die_if_unmatched "$mode" "$sha1"
> -		if test -e "$sm_path"/.git
> -		then
> -			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> -			say "$(eval_gettext "Entering '\$displaypath'")"
> -			name=$(git submodule--helper name "$sm_path")
> -			(
> -				prefix="$prefix$sm_path/"
> -				sanitize_submodule_env
> -				cd "$sm_path" &&
> -				# we make $path available to scripts ...
> -				path=$sm_path &&
> -				sm_path=$displaypath &&
> -				if test $# -eq 1
> -				then
> -					eval "$1"
> -				else
> -					"$@"
> -				fi &&
> -				if test -n "$recursive"
> -				then
> -					cmd_foreach "--recursive" "$@"
> -				fi
> -			) <&3 3<&- ||
> -			die "$(eval_gettext "Stopping at '\$displaypath'; script returned non-zero status.")"
> -		fi
> -	done
>  }
>  
>  #
> -- 
> 2.11.0
> 
Looking good!
-- 
Brandon Williams
Previous: Stefan BellerNext: Johannes Sixt
Message 23 of 48 in “submodule: port subcommand foreach from shell to C”
  1. Prathamesh ChavanApr 19, 2017
  2. Stefan BellerApr 19, 2017
  3. [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to CPrathamesh Chavan, Apr 22, 2017
  4. Junio C HamanoApr 24, 2017
  5. Stefan BellerApr 24, 2017
  6. Ramsay JonesApr 24, 2017
  7. Stefan BellerApr 24, 2017
  8. Ramsay JonesApr 24, 2017
  9. [GSoC][RFC/PATCH v3 1/2] t7407: test "submodule foreach --recursive" from subdirectory addedPrathamesh Chavan, May 12, 2017
  10. [GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to CPrathamesh Chavan, May 12, 2017
  11. Stefan BellerMay 15, 2017
  12. Brandon WilliamsMay 15, 2017
  13. [GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory addedPrathamesh Chavan, May 21, 2017
  14. [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to CPrathamesh Chavan, May 21, 2017
  15. Stefan BellerMay 22, 2017
  16. Brandon WilliamsMay 23, 2017
  17. Brandon WilliamsMay 23, 2017
  18. Stefan BellerMay 23, 2017
  19. Brandon WilliamsMay 23, 2017
  20. [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's valuePrathamesh Chavan, May 26, 2017
  21. [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to CPrathamesh Chavan, May 26, 2017
  22. Stefan BellerMay 26, 2017
  23. Brandon WilliamsMay 26, 2017
  24. Johannes SixtMay 26, 2017
  25. Ramsay JonesMay 27, 2017
  26. Ramsay JonesMay 27, 2017
  27. Johannes SixtMay 27, 2017
  28. Brandon WilliamsMay 26, 2017
  29. [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory addedPrathamesh Chavan, May 26, 2017
  30. Stefan BellerMay 26, 2017
  31. Brandon WilliamsMay 26, 2017
  32. Ramsay JonesMay 26, 2017
  33. Stefan BellerMay 26, 2017
  34. Ramsay JonesMay 27, 2017
  35. Stefan BellerMay 30, 2017
  36. Ramsay JonesMay 30, 2017
  37. Stefan BellerMay 30, 2017
  38. Ramsay JonesMay 31, 2017
  39. Ramsay JonesMay 31, 2017
  40. [GSoC][PATCH v6 1/2] submodule: fix buggy $path and $sm_path variable's valuePrathamesh Chavan, Jun 2, 2017
  41. [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to CPrathamesh Chavan, Jun 2, 2017
  42. Stefan BellerJun 3, 2017
  43. Prathamesh ChavanJun 4, 2017
  44. Brandon WilliamsMay 23, 2017
  45. submodule foreach: correct $sm_path in nested submodules from a dirStefan Beller, Jun 3, 2017
  46. Ramsay JonesJun 3, 2017
  47. Ramsay JonesJun 4, 2017
  48. Jonathan NiederJun 5, 2017

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.