patch, 4 partssubmodule: implement `module_name` as a builtin helper

> The goal of this series being rewriting `git submodule update`,
> we don't want to call out to the shell script for config lookups.
>
> So reimplement the lookup of the submodule name in C.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cb18ddf..dd5635f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)
> +static int collect_module_names(const char *key, const char *value, void *cb)
> +{
> +       size_t len;
> +       struct string_list *sl = cb;
> +
> +       if (starts_with(key, "submodule.")
> +           && strip_suffix(key, ".path", &len)) {
> +               struct strbuf sb = STRBUF_INIT;
> +               strbuf_add(&sb, key + strlen("submodule."),
> +                               len - strlen("submodule."));
> +               string_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);
> +               strbuf_release(&sb);

> +       }
> +
> +       return 0;
> +}
> +
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> +       struct string_list_item *item;
> +       struct git_config_source config_source;
> +       struct string_list values = STRING_LIST_INIT_DUP;
> +
> +       if (!argc)

> +               usage("git submodule--helper module_name <path>\n");
> +
> +       memset(&config_source, 0, sizeof(config_source));
> +       config_source.file = ".gitmodules";
> +
> +       if (git_config_with_options(collect_module_names, &values,
> +                                   &config_source, 1) < 0)
> +               die(_("unknown error occured while reading the git modules file"));
> +
> +       item = string_list_lookup(&values, argv[0]);
> +       if (item)
> +               printf("%s\n", (char*)item->util);
> +       else
> +               die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);
> +       return 0;
> +}
> +
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
>         if (argc < 2)
> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>         if (!strcmp(argv[1], "module_list"))
>                 return module_list(argc - 1, argv + 1, prefix);
>
> +       if (!strcmp(argv[1], "module_name"))
> +               return module_name(argc - 2, argv + 2, prefix);
> +
>  usage:
>         usage("git submodule--helper module_list\n");
>  }

> On Tue, Aug 4, 2015 at 8:04 PM, Stefan Beller <sbeller@google.com> wrote:
>> The goal of this series being rewriting `git submodule update`,
>> we don't want to call out to the shell script for config lookups.
>>
>> So reimplement the lookup of the submodule name in C.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index cb18ddf..dd5635f 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)
>> +static int collect_module_names(const char *key, const char *value, void *cb)
>> +{
>> +       size_t len;
>> +       struct string_list *sl = cb;
>> +
>> +       if (starts_with(key, "submodule.")
>> +           && strip_suffix(key, ".path", &len)) {
>> +               struct strbuf sb = STRBUF_INIT;
>> +               strbuf_add(&sb, key + strlen("submodule."),
>> +                               len - strlen("submodule."));
>> +               string_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);
>> +               strbuf_release(&sb);
>
> Why the complexity and overhead of a strbuf when the same could be
> accomplished more easily and straightforwardly with xstrndup()?

>
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int module_name(int argc, const char **argv, const char *prefix)
>> +{
>> +       struct string_list_item *item;
>> +       struct git_config_source config_source;
>> +       struct string_list values = STRING_LIST_INIT_DUP;
>> +
>> +       if (!argc)
>
> Do you mean?
>
>     if (argc != 1)

>
>> +               usage("git submodule--helper module_name <path>\n");
>> +
>> +       memset(&config_source, 0, sizeof(config_source));
>> +       config_source.file = ".gitmodules";
>> +
>> +       if (git_config_with_options(collect_module_names, &values,
>> +                                   &config_source, 1) < 0)
>> +               die(_("unknown error occured while reading the git modules file"));
>> +
>> +       item = string_list_lookup(&values, argv[0]);
>> +       if (item)
>> +               printf("%s\n", (char*)item->util);
>> +       else
>> +               die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);
>> +       return 0;
>> +}
>> +
>>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>>  {
>>         if (argc < 2)
>> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>>         if (!strcmp(argv[1], "module_list"))
>>                 return module_list(argc - 1, argv + 1, prefix);
>>
>> +       if (!strcmp(argv[1], "module_name"))
>> +               return module_name(argc - 2, argv + 2, prefix);
>> +
>>  usage:
>>         usage("git submodule--helper module_list\n");
>>  }

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>   When I started to implement git submodule add in the helper, I realized
>   the very first thing to be done would be module_name translated to C,
>   so I did that separately. Maybe we need to split this up as well into two
>   separate steps for processing and I/O, such that it can be reused better
>   from a future "git submodule--helper update" function
>
>   builtin/submodule--helper.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
>   git-submodule.sh            | 32 +++++++-----------------------
>   2 files changed, 54 insertions(+), 25 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cb18ddf..dd5635f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -5,6 +5,8 @@
>   #include "pathspec.h"
>   #include "dir.h"
>   #include "utf8.h"
> +#include "run-command.h"
> +#include "string-list.h"
>
>   static char *ps_matched;
>   static const struct cache_entry **ce_entries;
> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)
>   	return 0;
>   }
>
> +
> +static int collect_module_names(const char *key, const char *value, void *cb)
> +{
> +	size_t len;
> +	struct string_list *sl = cb;
> +
> +	if (starts_with(key, "submodule.")
> +	    && strip_suffix(key, ".path", &len)) {
> +		struct strbuf sb = STRBUF_INIT;
> +		strbuf_add(&sb, key + strlen("submodule."),
> +				len - strlen("submodule."));
> +		string_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);
> +		strbuf_release(&sb);
> +	}
> +
> +	return 0;
> +}
> +
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> +	struct string_list_item *item;
> +	struct git_config_source config_source;
> +	struct string_list values = STRING_LIST_INIT_DUP;
> +
> +	if (!argc)
> +		usage("git submodule--helper module_name <path>\n");
> +
> +	memset(&config_source, 0, sizeof(config_source));
> +	config_source.file = ".gitmodules";
> +
> +	if (git_config_with_options(collect_module_names, &values,
> +				    &config_source, 1) < 0)
> +		die(_("unknown error occured while reading the git modules file"));
> +
> +	item = string_list_lookup(&values, argv[0]);
> +	if (item)
> +		printf("%s\n", (char*)item->util);
> +	else
> +		die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);
> +	return 0;
> +}
> +
>   int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>   {
>   	if (argc < 2)
> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>   	if (!strcmp(argv[1], "module_list"))
>   		return module_list(argc - 1, argv + 1, prefix);
>
> +	if (!strcmp(argv[1], "module_name"))
> +		return module_name(argc - 2, argv + 2, prefix);
> +
>   usage:
>   	usage("git submodule--helper module_list\n");
>   }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index af9ecef..e6ff38d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -178,24 +178,6 @@ get_submodule_config () {
>   	printf '%s' "${value:-$default}"
>   }
>
> -
> -#
> -# Map submodule path to submodule name
> -#
> -# $1 = path
> -#
> -module_name()
> -{
> -	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
> -	sm_path="$1"
> -	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
> -	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
> -		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
> -	test -z "$name" &&
> -	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
> -	printf '%s\n' "$name"
> -}
> -
>   #
>   # Clone a submodule
>   #
> @@ -498,7 +480,7 @@ cmd_foreach()
>   		then
>   			displaypath=$(relative_path "$sm_path")
>   			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
> -			name=$(module_name "$sm_path")
> +			name=$(git submodule--helper module_name "$sm_path")
>   			(
>   				prefix="$prefix$sm_path/"
>   				clear_local_git_env
> @@ -554,7 +536,7 @@ cmd_init()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>
>   		displaypath=$(relative_path "$sm_path")
>
> @@ -636,7 +618,7 @@ cmd_deinit()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>
>   		displaypath=$(relative_path "$sm_path")
>
> @@ -758,7 +740,7 @@ cmd_update()
>   			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
>   			continue
>   		fi
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>   		url=$(git config submodule."$name".url)
>   		branch=$(get_submodule_config "$name" branch master)
>   		if ! test -z "$update"
> @@ -1022,7 +1004,7 @@ cmd_summary() {
>   			# Respect the ignore setting for --for-status.
>   			if test -n "$for_status"
>   			then
> -				name=$(module_name "$sm_path")
> +				name=$(git submodule--helper module_name "$sm_path")
>   				ignore_config=$(get_submodule_config "$name" ignore none)
>   				test $status != A && test $ignore_config = all && continue
>   			fi
> @@ -1184,7 +1166,7 @@ cmd_status()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>   		url=$(git config submodule."$name".url)
>   		displaypath=$(relative_path "$prefix$sm_path")
>   		if test "$stage" = U
> @@ -1261,7 +1243,7 @@ cmd_sync()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path")
> +		name=$(git submodule--helper module_name "$sm_path")
>   		url=$(git config -f .gitmodules --get submodule."$name".url)
>
>   		# Possibly a url relative to parent
>

> Am 05.08.2015 um 02:04 schrieb Stefan Beller:
>>
>> The goal of this series being rewriting `git submodule update`,
>> we don't want to call out to the shell script for config lookups.
>>
>> So reimplement the lookup of the submodule name in C.
>
>
> Cool. This brings down the duration of the test suite from 3:13
> to 3:12 for me (best of three).
>
> You might wanna have a look into submodule.c: after initially
> calling gitmodules_config() one can lookup the submodule name
> in the static "config_name_for_path" string_list. If you'd add
> a public method to submodule.c which accesses that string_list
> and returns the name for the given path, you won't need your
> two new functions ... or am I missing something?

> This implements the helper `module_name` in C instead of shell,
> yielding a nice performance boost.
>
> Before this patch, I measured a time (best out of three):
>
>    $ time ./t7400-submodule-basic.sh  >/dev/null
>      real	0m11.066s
>      user	0m3.348s
>      sys	0m8.534s
>
> With this patch applied I measured (also best out of three)
>
>    $ time ./t7400-submodule-basic.sh  >/dev/null
>      real	0m10.063s
>      user	0m3.044s
>      sys	0m7.487s
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Is this what you had in mind, Jens?

>   builtin/submodule--helper.c | 23 +++++++++++++++++++++++
>   git-submodule.sh            | 32 +++++++-------------------------
>   submodule.c                 | 18 +++++++++++++-----
>   submodule.h                 |  1 +
>   4 files changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cb18ddf..3713c4c 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -5,6 +5,8 @@
>   #include "pathspec.h"
>   #include "dir.h"
>   #include "utf8.h"
> +#include "submodule.h"
> +#include "string-list.h"
>
>   static char *ps_matched;
>   static const struct cache_entry **ce_entries;
> @@ -98,6 +100,24 @@ static int module_list(int argc, const char **argv, const char *prefix)
>   	return 0;
>   }
>
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> +	const char *name;
> +
> +	if (argc != 1)
> +		usage("git submodule--helper module_name <path>\n");
> +
> +	gitmodules_config();
> +	name = submodule_name_for_path(argv[0]);
> +
> +	if (name)
> +		printf("%s\n", name);
> +	else
> +		die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);

> +	return 0;
> +}
> +
>   int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>   {
>   	if (argc < 2)
> @@ -106,6 +126,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>   	if (!strcmp(argv[1], "module_list"))
>   		return module_list(argc - 1, argv + 1, prefix);
>
> +	if (!strcmp(argv[1], "module_name"))
> +		return module_name(argc - 2, argv + 2, prefix);
> +
>   usage:
>   	usage("git submodule--helper module_list\n");
>   }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index af9ecef..e6ff38d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -178,24 +178,6 @@ get_submodule_config () {
>   	printf '%s' "${value:-$default}"
>   }
>
> -
> -#
> -# Map submodule path to submodule name
> -#
> -# $1 = path
> -#
> -module_name()
> -{
> -	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
> -	sm_path="$1"
> -	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
> -	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
> -		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
> -	test -z "$name" &&
> -	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
> -	printf '%s\n' "$name"
> -}
> -
>   #
>   # Clone a submodule
>   #
> @@ -498,7 +480,7 @@ cmd_foreach()
>   		then
>   			displaypath=$(relative_path "$sm_path")
>   			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
> -			name=$(module_name "$sm_path")
> +			name=$(git submodule--helper module_name "$sm_path")
>   			(
>   				prefix="$prefix$sm_path/"
>   				clear_local_git_env
> @@ -554,7 +536,7 @@ cmd_init()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>
>   		displaypath=$(relative_path "$sm_path")
>
> @@ -636,7 +618,7 @@ cmd_deinit()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>
>   		displaypath=$(relative_path "$sm_path")
>
> @@ -758,7 +740,7 @@ cmd_update()
>   			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
>   			continue
>   		fi
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>   		url=$(git config submodule."$name".url)
>   		branch=$(get_submodule_config "$name" branch master)
>   		if ! test -z "$update"
> @@ -1022,7 +1004,7 @@ cmd_summary() {
>   			# Respect the ignore setting for --for-status.
>   			if test -n "$for_status"
>   			then
> -				name=$(module_name "$sm_path")
> +				name=$(git submodule--helper module_name "$sm_path")
>   				ignore_config=$(get_submodule_config "$name" ignore none)
>   				test $status != A && test $ignore_config = all && continue
>   			fi
> @@ -1184,7 +1166,7 @@ cmd_status()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>   		url=$(git config submodule."$name".url)
>   		displaypath=$(relative_path "$prefix$sm_path")
>   		if test "$stage" = U
> @@ -1261,7 +1243,7 @@ cmd_sync()
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path")
> +		name=$(git submodule--helper module_name "$sm_path")
>   		url=$(git config -f .gitmodules --get submodule."$name".url)
>
>   		# Possibly a url relative to parent
> diff --git a/submodule.c b/submodule.c
> index 700bbf4..dd8dcb0 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)
>   	initialized_fetch_ref_tips = 0;
>   }
>
> +const char* submodule_name_for_path(const char* path)

> +{
> +	struct string_list_item *item;
> +	item = unsorted_string_list_lookup(&config_name_for_path, path);
> +	if (item)
> +		return item->util;
> +	else
> +		return NULL;

> +}
> +
>   int fetch_populated_submodules(const struct argv_array *options,
>   			       const char *prefix, int command_line_option,
>   			       int quiet)
> @@ -693,7 +703,7 @@ int fetch_populated_submodules(const struct argv_array *options,
>   	int i, result = 0;
>   	struct child_process cp = CHILD_PROCESS_INIT;
>   	struct argv_array argv = ARGV_ARRAY_INIT;
> -	struct string_list_item *name_for_path;
> +	const char *name_for_path;
>   	const char *work_tree = get_git_work_tree();
>   	if (!work_tree)
>   		goto out;
> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,
>   		if (!S_ISGITLINK(ce->ce_mode))
>   			continue;
>
> -		name = ce->name;
> -		name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
> -		if (name_for_path)
> -			name = name_for_path->util;
> +		name_for_path = submodule_name_for_path(ce->name);
> +		name =  name_for_path ? name_for_path : ce->name;

>   		default_argv = "yes";
>   		if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> diff --git a/submodule.h b/submodule.h
> index 7beec48..e3dd854 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>   		struct string_list *needs_pushing);
>   int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
>   void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
> +const char* submodule_name_for_path(const char* path);

>  builtin/submodule--helper.c | 22 ++++++++++++++++++++++
>  git-submodule.sh            | 32 +++++++-------------------------
>  submodule.c                 | 19 ++++++++++++++-----
>  submodule.h                 |  1 +
>  4 files changed, 44 insertions(+), 30 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cb18ddf..bc37b74 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -5,6 +5,8 @@
>  #include "pathspec.h"
>  #include "dir.h"
>  #include "utf8.h"
> +#include "submodule.h"
> +#include "string-list.h"
>  
>  static char *ps_matched;
>  static const struct cache_entry **ce_entries;
> @@ -98,6 +100,23 @@ static int module_list(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> +	const char *name;
> +
> +	if (argc != 1)
> +		usage("git submodule--helper module_name <path>\n");
> +
> +	gitmodules_config();
> +	name = submodule_name_for_path(argv[0]);
> +
> +	if (!name)
> +		die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);
> +
> +	printf("%s\n", name);
> +	return 0;
> +}
> +
>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  {
>  	if (argc < 2)
> @@ -106,6 +125,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>  	if (!strcmp(argv[1], "module_list"))
>  		return module_list(argc - 1, argv + 1, prefix);
>  
> +	if (!strcmp(argv[1], "module_name"))
> +		return module_name(argc - 2, argv + 2, prefix);
> +
>  usage:
>  	usage("git submodule--helper module_list\n");
>  }
> diff --git a/git-submodule.sh b/git-submodule.sh
> index af9ecef..e6ff38d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -178,24 +178,6 @@ get_submodule_config () {
>  	printf '%s' "${value:-$default}"
>  }
>  
> -
> -#
> -# Map submodule path to submodule name
> -#
> -# $1 = path
> -#
> -module_name()
> -{
> -	# Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
> -	sm_path="$1"
> -	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
> -	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
> -		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
> -	test -z "$name" &&
> -	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
> -	printf '%s\n' "$name"
> -}
> -
>  #
>  # Clone a submodule
>  #
> @@ -498,7 +480,7 @@ cmd_foreach()
>  		then
>  			displaypath=$(relative_path "$sm_path")
>  			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
> -			name=$(module_name "$sm_path")
> +			name=$(git submodule--helper module_name "$sm_path")
>  			(
>  				prefix="$prefix$sm_path/"
>  				clear_local_git_env
> @@ -554,7 +536,7 @@ cmd_init()
>  	while read mode sha1 stage sm_path
>  	do
>  		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>  
>  		displaypath=$(relative_path "$sm_path")
>  
> @@ -636,7 +618,7 @@ cmd_deinit()
>  	while read mode sha1 stage sm_path
>  	do
>  		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>  
>  		displaypath=$(relative_path "$sm_path")
>  
> @@ -758,7 +740,7 @@ cmd_update()
>  			echo >&2 "Skipping unmerged submodule $prefix$sm_path"
>  			continue
>  		fi
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>  		url=$(git config submodule."$name".url)
>  		branch=$(get_submodule_config "$name" branch master)
>  		if ! test -z "$update"
> @@ -1022,7 +1004,7 @@ cmd_summary() {
>  			# Respect the ignore setting for --for-status.
>  			if test -n "$for_status"
>  			then
> -				name=$(module_name "$sm_path")
> +				name=$(git submodule--helper module_name "$sm_path")
>  				ignore_config=$(get_submodule_config "$name" ignore none)
>  				test $status != A && test $ignore_config = all && continue
>  			fi
> @@ -1184,7 +1166,7 @@ cmd_status()
>  	while read mode sha1 stage sm_path
>  	do
>  		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path") || exit
> +		name=$(git submodule--helper module_name "$sm_path") || exit
>  		url=$(git config submodule."$name".url)
>  		displaypath=$(relative_path "$prefix$sm_path")
>  		if test "$stage" = U
> @@ -1261,7 +1243,7 @@ cmd_sync()
>  	while read mode sha1 stage sm_path
>  	do
>  		die_if_unmatched "$mode"
> -		name=$(module_name "$sm_path")
> +		name=$(git submodule--helper module_name "$sm_path")
>  		url=$(git config -f .gitmodules --get submodule."$name".url)
>  
>  		# Possibly a url relative to parent
> diff --git a/submodule.c b/submodule.c
> index 15e90d1..78d7616 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)
>  	initialized_fetch_ref_tips = 0;
>  }
>  
> +const char *submodule_name_for_path(const char *path)
> +{
> +	struct string_list_item *item;
> +	item = unsorted_string_list_lookup(&config_name_for_path, path);
> +	if (!item)
> +		return NULL;
> +
> +	return item->util;
> +}
> +
>  int fetch_populated_submodules(const struct argv_array *options,
>  			       const char *prefix, int command_line_option,
>  			       int quiet)
> @@ -693,7 +703,6 @@ int fetch_populated_submodules(const struct argv_array *options,
>  	int i, result = 0;
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct argv_array argv = ARGV_ARRAY_INIT;
> -	struct string_list_item *name_for_path;
>  	const char *work_tree = get_git_work_tree();
>  	if (!work_tree)
>  		goto out;
> @@ -723,10 +732,10 @@ int fetch_populated_submodules(const struct argv_array *options,
>  		if (!S_ISGITLINK(ce->ce_mode))
>  			continue;
>  
> -		name = ce->name;
> -		name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
> -		if (name_for_path)
> -			name = name_for_path->util;
> +		name = submodule_name_for_path(ce->name);
> +		if (!name)
> +			/* Not in .gitmodules, try the default name == path */
> +			name = ce->name;
>  
>  		default_argv = "yes";
>  		if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {
> diff --git a/submodule.h b/submodule.h
> index 7beec48..fc7f8a6 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>  		struct string_list *needs_pushing);
>  int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
>  void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
> +const char *submodule_name_for_path(const char *path);
>  
>  #endif

> Stefan Beller <sbeller@google.com> writes:
>
>> This incorporates the changes from Jens fixup! commit
>> (which addresses all issues he pointed out).
>>
>> I agree this looks much cleaner. :)
>
> The only thing I found somewhat questionable is where to call
> gitmodules_config() from.  I think it is OK to do this at the
> beginning of module_name(), at least for now, simply because the
> other function module_list() does not need to.
>
> When you rewrite sufficiently large parts of the scripted Porcelain
> into C so that different pieces translated from the shell functions
> directly call into each other, that may have to change, though.  I
> do not think gitmodules_config() is designed to be called more than
> once, and I expect module_name() would be called many times inside a
> loop.

>
> Other than that, this is a trivial refactoring (i.e. a new helper
> function added to submodule.c can be used from an existing
> open-coded logic in the same file, and then the same helper function
> gains a new callsite in submodule--helper.c) that makes things
> easier to read.
>
> Thanks.
>
>>  builtin/submodule--helper.c | 22 ++++++++++++++++++++++
>>  git-submodule.sh            | 32 +++++++-------------------------
>>  submodule.c                 | 19 ++++++++++++++-----
>>  submodule.h                 |  1 +
>>  4 files changed, 44 insertions(+), 30 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index cb18ddf..bc37b74 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -5,6 +5,8 @@
>>  #include "pathspec.h"
>>  #include "dir.h"
>>  #include "utf8.h"
>> +#include "submodule.h"
>> +#include "string-list.h"
>>
>>  static char *ps_matched;
>>  static const struct cache_entry **ce_entries;
>> @@ -98,6 +100,23 @@ static int module_list(int argc, const char **argv, const char *prefix)
>>       return 0;
>>  }
>>
>> +static int module_name(int argc, const char **argv, const char *prefix)
>> +{
>> +     const char *name;
>> +
>> +     if (argc != 1)
>> +             usage("git submodule--helper module_name <path>\n");
>> +
>> +     gitmodules_config();
>> +     name = submodule_name_for_path(argv[0]);
>> +
>> +     if (!name)
>> +             die("No submodule mapping found in .gitmodules for path '%s'", argv[0]);
>> +
>> +     printf("%s\n", name);
>> +     return 0;
>> +}
>> +
>>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>>  {
>>       if (argc < 2)
>> @@ -106,6 +125,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>>       if (!strcmp(argv[1], "module_list"))
>>               return module_list(argc - 1, argv + 1, prefix);
>>
>> +     if (!strcmp(argv[1], "module_name"))
>> +             return module_name(argc - 2, argv + 2, prefix);
>> +
>>  usage:
>>       usage("git submodule--helper module_list\n");
>>  }
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index af9ecef..e6ff38d 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -178,24 +178,6 @@ get_submodule_config () {
>>       printf '%s' "${value:-$default}"
>>  }
>>
>> -
>> -#
>> -# Map submodule path to submodule name
>> -#
>> -# $1 = path
>> -#
>> -module_name()
>> -{
>> -     # Do we have "submodule.<something>.path = $1" defined in .gitmodules file?
>> -     sm_path="$1"
>> -     re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
>> -     name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
>> -             sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
>> -     test -z "$name" &&
>> -     die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
>> -     printf '%s\n' "$name"
>> -}
>> -
>>  #
>>  # Clone a submodule
>>  #
>> @@ -498,7 +480,7 @@ cmd_foreach()
>>               then
>>                       displaypath=$(relative_path "$sm_path")
>>                       say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
>> -                     name=$(module_name "$sm_path")
>> +                     name=$(git submodule--helper module_name "$sm_path")
>>                       (
>>                               prefix="$prefix$sm_path/"
>>                               clear_local_git_env
>> @@ -554,7 +536,7 @@ cmd_init()
>>       while read mode sha1 stage sm_path
>>       do
>>               die_if_unmatched "$mode"
>> -             name=$(module_name "$sm_path") || exit
>> +             name=$(git submodule--helper module_name "$sm_path") || exit
>>
>>               displaypath=$(relative_path "$sm_path")
>>
>> @@ -636,7 +618,7 @@ cmd_deinit()
>>       while read mode sha1 stage sm_path
>>       do
>>               die_if_unmatched "$mode"
>> -             name=$(module_name "$sm_path") || exit
>> +             name=$(git submodule--helper module_name "$sm_path") || exit
>>
>>               displaypath=$(relative_path "$sm_path")
>>
>> @@ -758,7 +740,7 @@ cmd_update()
>>                       echo >&2 "Skipping unmerged submodule $prefix$sm_path"
>>                       continue
>>               fi
>> -             name=$(module_name "$sm_path") || exit
>> +             name=$(git submodule--helper module_name "$sm_path") || exit
>>               url=$(git config submodule."$name".url)
>>               branch=$(get_submodule_config "$name" branch master)
>>               if ! test -z "$update"
>> @@ -1022,7 +1004,7 @@ cmd_summary() {
>>                       # Respect the ignore setting for --for-status.
>>                       if test -n "$for_status"
>>                       then
>> -                             name=$(module_name "$sm_path")
>> +                             name=$(git submodule--helper module_name "$sm_path")
>>                               ignore_config=$(get_submodule_config "$name" ignore none)
>>                               test $status != A && test $ignore_config = all && continue
>>                       fi
>> @@ -1184,7 +1166,7 @@ cmd_status()
>>       while read mode sha1 stage sm_path
>>       do
>>               die_if_unmatched "$mode"
>> -             name=$(module_name "$sm_path") || exit
>> +             name=$(git submodule--helper module_name "$sm_path") || exit
>>               url=$(git config submodule."$name".url)
>>               displaypath=$(relative_path "$prefix$sm_path")
>>               if test "$stage" = U
>> @@ -1261,7 +1243,7 @@ cmd_sync()
>>       while read mode sha1 stage sm_path
>>       do
>>               die_if_unmatched "$mode"
>> -             name=$(module_name "$sm_path")
>> +             name=$(git submodule--helper module_name "$sm_path")
>>               url=$(git config -f .gitmodules --get submodule."$name".url)
>>
>>               # Possibly a url relative to parent
>> diff --git a/submodule.c b/submodule.c
>> index 15e90d1..78d7616 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)
>>       initialized_fetch_ref_tips = 0;
>>  }
>>
>> +const char *submodule_name_for_path(const char *path)
>> +{
>> +     struct string_list_item *item;
>> +     item = unsorted_string_list_lookup(&config_name_for_path, path);
>> +     if (!item)
>> +             return NULL;
>> +
>> +     return item->util;
>> +}
>> +
>>  int fetch_populated_submodules(const struct argv_array *options,
>>                              const char *prefix, int command_line_option,
>>                              int quiet)
>> @@ -693,7 +703,6 @@ int fetch_populated_submodules(const struct argv_array *options,
>>       int i, result = 0;
>>       struct child_process cp = CHILD_PROCESS_INIT;
>>       struct argv_array argv = ARGV_ARRAY_INIT;
>> -     struct string_list_item *name_for_path;
>>       const char *work_tree = get_git_work_tree();
>>       if (!work_tree)
>>               goto out;
>> @@ -723,10 +732,10 @@ int fetch_populated_submodules(const struct argv_array *options,
>>               if (!S_ISGITLINK(ce->ce_mode))
>>                       continue;
>>
>> -             name = ce->name;
>> -             name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
>> -             if (name_for_path)
>> -                     name = name_for_path->util;
>> +             name = submodule_name_for_path(ce->name);
>> +             if (!name)
>> +                     /* Not in .gitmodules, try the default name == path */
>> +                     name = ce->name;
>>
>>               default_argv = "yes";
>>               if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {
>> diff --git a/submodule.h b/submodule.h
>> index 7beec48..fc7f8a6 100644
>> --- a/submodule.h
>> +++ b/submodule.h
>> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
>>               struct string_list *needs_pushing);
>>  int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);
>>  void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
>> +const char *submodule_name_for_path(const char *path);
>>
>>  #endif

>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,
>>   		if (!S_ISGITLINK(ce->ce_mode))
>>   			continue;
>>
>> -		name = ce->name;
>> -		name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
>> -		if (name_for_path)
>> -			name = name_for_path->util;
>> +		name_for_path = submodule_name_for_path(ce->name);
>> +		name =  name_for_path ? name_for_path : ce->name;

> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
> This change...
>
>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,
>>>              if (!S_ISGITLINK(ce->ce_mode))
>>>                      continue;
>>>
>>> -            name = ce->name;
>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
>>> -            if (name_for_path)
>>> -                    name = name_for_path->util;
>>> +            name_for_path = submodule_name_for_path(ce->name);
>>> +            name =  name_for_path ? name_for_path : ce->name;
>
> ... interacts with Heiko's cached submodule config work that seems
> to have stalled.

>
> I can discard the stalled topic and queue this one instead, asking
> Heiko to reroll his on top once this has stabilized, or if Stefan is
> really into revamping submodule now (which I hope is the case),
> perhaps Heiko's work can be rerolled by Stefan (with help from
> others, of course) as a prerequisite and then these changes can be
> built on top of it?

> On Fri, Aug 7, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>> This change...
>>
>>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,
>>>>              if (!S_ISGITLINK(ce->ce_mode))
>>>>                      continue;
>>>>
>>>> -            name = ce->name;
>>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
>>>> -            if (name_for_path)
>>>> -                    name = name_for_path->util;
>>>> +            name_for_path = submodule_name_for_path(ce->name);
>>>> +            name =  name_for_path ? name_for_path : ce->name;
>>
>> ... interacts with Heiko's cached submodule config work that seems
>> to have stalled.
>
> We can drop that hunk as it only uses the new method
> `submodule_name_for_path` but doesn't change functionality.
> So if you want to keep Heikos work, I'll just resend the patch
> without that hunk.

> 2) Come up with a good thread pool abstraction
>    (Started as "[RFC/PATCH 0/4] parallel fetch for submodules" )
>    This abstraction (if done right) will allow us to use it in different places
>    easily. I started it as part of "git fetch --recurse-submodules" because
>    it is submodule related and reasonably sized

> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Aug 7, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>>
>>> This change...
>>>
>>>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,
>>>>>              if (!S_ISGITLINK(ce->ce_mode))
>>>>>                      continue;
>>>>>
>>>>> -            name = ce->name;
>>>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);
>>>>> -            if (name_for_path)
>>>>> -                    name = name_for_path->util;
>>>>> +            name_for_path = submodule_name_for_path(ce->name);
>>>>> +            name =  name_for_path ? name_for_path : ce->name;
>>>
>>> ... interacts with Heiko's cached submodule config work that seems
>>> to have stalled.
>>
>> We can drop that hunk as it only uses the new method
>> `submodule_name_for_path` but doesn't change functionality.
>> So if you want to keep Heikos work, I'll just resend the patch
>> without that hunk.
>
> Does such a result even make sense?  Note that I wasn't talking
> about textual conflict.
>
> If we followed what you just said, that patch will try to directly
> read the data in config_name_for_path string list, which is removed
> by Heiko's series, if I am reading it right.
>
> In the new world order with Heiko's series, the way you grab
> submodule configuration data is to call submodule_from_path() or
> submodule_from_name() and they allow you to read from .gitmodules
> either in a commit (for historical state), the index, or from the
> working tree.  Which should be much cleaner and goes in the right
> direction in the longer term.
>
> And the best part of the story is that your module_name would be
> just calling submodule_from_path() and peeking into a field.
>
>> 2) Come up with a good thread pool abstraction
>>    (Started as "[RFC/PATCH 0/4] parallel fetch for submodules" )
>>    This abstraction (if done right) will allow us to use it in different places
>>    easily. I started it as part of "git fetch --recurse-submodules" because
>>    it is submodule related and reasonably sized
>
> I personally think this gives the most bang-for-buck.  Write that
> and expose it as "git submodule for-each-parallel", which takes the
> shell scriptlet that currently is the loop body of "while read mode
> sha1 stage sm_path" in update and clone.  You will have immediate
> and large payback.

>
> And you do not even need module_list and module_name written in C in
> order to do so.  Not that these two are not trivial to implement, but
> the payoff from them is not as large as from for-each-parallel ;-)
>

> On Fri, Aug 7, 2015 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>> ...
>>> We can drop that hunk as it only uses the new method
>>> `submodule_name_for_path` but doesn't change functionality.
>>> So if you want to keep Heikos work, I'll just resend the patch
>>> without that hunk.
>>
>> Does such a result even make sense?  Note that I wasn't talking
>> about textual conflict.
>>
>> If we followed what you just said, that patch will try to directly
>> read the data in config_name_for_path string list, which is removed
>> by Heiko's series, if I am reading it right.

>>> 2) Come up with a good thread pool abstraction
>>>    (Started as "[RFC/PATCH 0/4] parallel fetch for submodules" )
>>>    This abstraction (if done right) will allow us to use it in different places
>>>    easily. I started it as part of "git fetch --recurse-submodules" because
>>>    it is submodule related and reasonably sized
>>
>> I personally think this gives the most bang-for-buck.  Write that
>> and expose it as "git submodule for-each-parallel", which takes the
>> shell scriptlet that currently is the loop body of "while read mode
>> sha1 stage sm_path" in update and clone.  You will have immediate
>> and large payback.
>
> You said that before. I feel like this is a bit to narrow.

> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Aug 7, 2015 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>> ...
>>>> We can drop that hunk as it only uses the new method
>>>> `submodule_name_for_path` but doesn't change functionality.
>>>> So if you want to keep Heikos work, I'll just resend the patch
>>>> without that hunk.
>>>
>>> Does such a result even make sense?  Note that I wasn't talking
>>> about textual conflict.
>>>
>>> If we followed what you just said, that patch will try to directly
>>> read the data in config_name_for_path string list, which is removed
>>> by Heiko's series, if I am reading it right.
>
> By the way, the above is more important part of the message you are
> responding to.  The result does not simply link, because your
> unsorted_string_list_lookup() will no longer have the string list in
> the first place X-<.

>
>>>> 2) Come up with a good thread pool abstraction
>>>>    (Started as "[RFC/PATCH 0/4] parallel fetch for submodules" )
>>>>    This abstraction (if done right) will allow us to use it in different places
>>>>    easily. I started it as part of "git fetch --recurse-submodules" because
>>>>    it is submodule related and reasonably sized
>>>
>>> I personally think this gives the most bang-for-buck.  Write that
>>> and expose it as "git submodule for-each-parallel", which takes the
>>> shell scriptlet that currently is the loop body of "while read mode
>>> sha1 stage sm_path" in update and clone.  You will have immediate
>>> and large payback.
>>
>> You said that before. I feel like this is a bit to narrow.
>
> That depends on how good a design you make for the internal
> "parallel execution" engine.  If it is made to take an arbitrary C
> function with list of arguments to call it with, for-each-parallel
> would be just a degenerate and narrow case where that arbitrary C
> function happens to be exec'ing a shell and feed a shell scriptlet
> to it.
>
> The internal parallel execution engine would be reusable without any
> change to call C native functions, allowing you to do everything
> inside the process in the future.  And the "narrow" case is a good
> first step to validate your design actually _works_.
>

> On Fri, Aug 7, 2015 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>>> If we followed what you just said, that patch will try to directly
>>>> read the data in config_name_for_path string list, which is removed
>>>> by Heiko's series, if I am reading it right.
>>
>> By the way, the above is more important part of the message you are
>> responding to.  The result does not simply link, because your
>> unsorted_string_list_lookup() will no longer have the string list in
>> the first place X-<.
>
> I looked through Heikos series and think it is an improvement. I mean I
> can redo my patches on top of his. Specially this patch will be easy,
> as patch 2/4 (extract functions for submodule config set and lookup)
> implements get_name_for_path. All I would need to do then is expose it
> to the shell via the helper.

> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Aug 7, 2015 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>>>> If we followed what you just said, that patch will try to directly
>>>>> read the data in config_name_for_path string list, which is removed
>>>>> by Heiko's series, if I am reading it right.
>>>
>>> By the way, the above is more important part of the message you are
>>> responding to.  The result does not simply link, because your
>>> unsorted_string_list_lookup() will no longer have the string list in
>>> the first place X-<.
>>
>> I looked through Heikos series and think it is an improvement. I mean I
>> can redo my patches on top of his. Specially this patch will be easy,
>> as patch 2/4 (extract functions for submodule config set and lookup)
>> implements get_name_for_path. All I would need to do then is expose it
>> to the shell via the helper.
>
> Yes, that is exactly what I said a few messages ago, wasn't it?

>
> But that would require that you read and understand Heiko's work and
> that you understand what its future direction should be, communicate
> that vision to others to share, before building on top of it.  And
> with that effort, you would already be in a good position to polish
> Heiko's stalled work and move it forward.  After all, you cannot
> just build on a stalled work, declare "my part is done; the result
> is not mergeable because the foundation is not cooked, but that is
> not my problem" ;-).

>
> IIRC, the issues around the topic were nothing show-stopping, but
> the primary reason it stalled was that Heiko got busy with his other
> obligations, so I'd appreciate others like you to help the topic
> move forward.

> Stefan Beller <sbeller@google.com> writes:
>
>> That's why I want to be a bit more generic and have this thread pool API
>> done in C, such that "any for loop" in git can be easily replaced by using
>> the thread pool. I think of "git fetch --all" specially.
>
> One more thing, as I didn't notice that you kept repeating "thread"
> pool API.

>
> While I doubt that you would gain much by using threads in place of
> processes to perform parallel "submodule update", "submodule clone",
> "fetch all", etc., all of which are fairly well isolated and heavy
> weight operations themselves, and I suspect that the implementation
> simplicity of using separate processes would probably be huge plus
> compared to any possible upside you may gain from using threads, if
> you really want to go the "thread" route, the first thing to try
> would be to see if a few places we already use threads for
> parallelism (namely, "grep", "pack-objects", "preload-index" and
> "index-pack") can be factored out and model your new API around the
> commonality among them.

>> One more thing, as I didn't notice that you kept repeating "thread"
>> pool API.
>
> Yeah I intended to use both threads and processes for the heavy submodule
> operations.

>> ... if
>> you really want to go the "thread" route, the first thing to try
>> would be to see if a few places we already use threads for
>> parallelism (namely, "grep", "pack-objects", "preload-index" and
>> "index-pack") can be factored out and model your new API around the
>> commonality among them.

>>> ... if
>>> you really want to go the "thread" route, the first thing to try
>>> would be to see if a few places we already use threads for
>>> parallelism (namely, "grep", "pack-objects", "preload-index" and
>>> "index-pack") can be factored out and model your new API around the
>>> commonality among them.
>
> And obviously, doing your pool API around threads will allow you to
> throw future per-thread function that do not involve run_command()
> at all at your API, and it will make it easy to adapt the current
> threaded parts of the system to the API.

> Junio C Hamano <gitster@pobox.com> writes:
>
>>>> ... if
>>>> you really want to go the "thread" route, the first thing to try
>>>> would be to see if a few places we already use threads for
>>>> parallelism (namely, "grep", "pack-objects", "preload-index" and
>>>> "index-pack") can be factored out and model your new API around the
>>>> commonality among them.
>>
>> And obviously, doing your pool API around threads will allow you to
>> throw future per-thread function that do not involve run_command()
>> at all at your API, and it will make it easy to adapt the current
>> threaded parts of the system to the API.
>
> Just a few random thoughts before going to bed and going offline for
> the weekend...
>
> Eventually, we would want to do "submodule update" of a top-level
> project that has 500 submodules underneath, but obviously we would
> not want to blindly spawn 500 threads, each of which runs "fetch",
> all at the same time.  We'd want to limit the parallelism to a sane
> limit (say, 16 or 32), stuff 500 work units to a queue, from which
> that many number of worker bees grab work unit one by one to process
> and then come back to ask for more work.

>
> And we would eventually want to be able to do this even when these
> 500 submodules are spread across multiple levels of nested
> submodules (e.g. top-level may have 8 submodules, and they have 16
> nested subsubmodules each on average, each of which may have 4
> nested subsubsubmodules on average).  Specifying -j16 at the top
> level and apportioning the parallelism to recursive invoation of
> "submodule update" in such a way that the overall process is
> efficient and without waste would be a bit tricky.
>
> In such a nested submodule case, we may want to instead try to
> enumerate these 500 submodules upfront with unbounded parallelism

> (e.g. the top-level will ask 4 worker bees to process immediate 8
> submodules, and they each spawn 4 worker bees to process their
> immediate 16 submodules, and so on---it is unbounded because we do
> not know upfront how deep the nesting is).
>
> Let's call that a recursive module_list.  You would want out of a
> recursive module_list:
>
>  - the path to the submodule (or "." for the top-level) to indicate
>    where in the nested hierarchy the information came from;
>
>  - the information the flat module_list gives for that location.

>
> Since you already have module_list() function natively callable from
> C and also it is available via "git submodule--helper module_list",
> implementing a recursive module_list would be a good first proof of
> concept exercise for your "thread pool" engine.  You can employ the
> "dual implementation" trick to call
>
>  - a version that tells the thread to run the native C version of
>    module_list(),
>
>  - another version that tells the thread to run_command()
>    "submodule--helper module_list" in the top-level and nested
>    submodules.
>
> and collect and compare their results and performance.
>
> That will not just be a good proof of concept for the pool
> implementation.
>
> Once you have such a recursive module_list, you can use it as a way
> to easily obtain such a "unified view" list of all submodules.  That
> can be used to stuff a flat work unit queue to implement reasonably
> bounded parallelism.
>
> Your recursive "submoule update" implementation could be
>
>  - Run recursive module_list to stuff the work queue with these 500
>    submodules (possibly spread across in top-level and in nested
>    submodules, or all 500 in the flat top-level);
>
>  - Start N worker bees, and tell them to pick from that work queue,
>    each element of which tells them to process which submodule that
>    resides in where (either in the top-level project or in a
>    submodule).
>
> And each work element would essentially be to run "git fetch" in
> that submodule directory.

> Most of the submodule operations work on a set of submodules.
> Calculating and using this set is usually done via:
>
>         module_list "$@" | {
>             while read mode sha1 stage sm_path
>             do
>                  # the actual operation
>             done
>         }
>
> Currently the function `module_list` is implemented in the
> git-submodule.sh as a shell script wrapping a perl script.
> The rewrite is in C, such that it is faster and can later be
> easily adapted when other functions are rewritten in C.
>
> git-submodule.sh similar to the builtin commands will navigate
> to the top most directory of the repository and keeping the
> subdirectories as a variable. As the helper is called from
> within the git-submodule.sh script, we are already navigated
> to the root level, but the path arguments are stil relative
> to the subdirectory we were in when calling git-submodule.sh.
> That's why there is a `--prefix` option pointing to an alternative
> path where to anchor relative path arguments.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> The same as yesterday evening, just an entry added to .gitignore.
>
> So we'll have a "git submodule--helper module_list" here.
>
>   .gitignore                  |   1 +
>   Makefile                    |   1 +
>   builtin.h                   |   1 +
>   builtin/submodule--helper.c | 111 ++++++++++++++++++++++++++++++++++++++++++++
>   git-submodule.sh            |  54 +++------------------
>   git.c                       |   1 +
>   6 files changed, 121 insertions(+), 48 deletions(-)
>   create mode 100644 builtin/submodule--helper.c
>
> diff --git a/.gitignore b/.gitignore
> index a685ec1..2a69ba0 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -155,6 +155,7 @@
>   /git-status
>   /git-stripspace
>   /git-submodule
> +/git-submodule--helper
>   /git-svn
>   /git-symbolic-ref
>   /git-tag
> diff --git a/Makefile b/Makefile
> index 7efedbe..460d17a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -899,6 +899,7 @@ BUILTIN_OBJS += builtin/shortlog.o
>   BUILTIN_OBJS += builtin/show-branch.o
>   BUILTIN_OBJS += builtin/show-ref.o
>   BUILTIN_OBJS += builtin/stripspace.o
> +BUILTIN_OBJS += builtin/submodule--helper.o
>   BUILTIN_OBJS += builtin/symbolic-ref.o
>   BUILTIN_OBJS += builtin/tag.o
>   BUILTIN_OBJS += builtin/unpack-file.o
> diff --git a/builtin.h b/builtin.h
> index 839483d..924e6c4 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);
>   extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
>   extern int cmd_status(int argc, const char **argv, const char *prefix);
>   extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
> +extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
>   extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
>   extern int cmd_tag(int argc, const char **argv, const char *prefix);
>   extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> new file mode 100644
> index 0000000..cb18ddf
> --- /dev/null
> +++ b/builtin/submodule--helper.c
> @@ -0,0 +1,111 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "quote.h"
> +#include "pathspec.h"
> +#include "dir.h"
> +#include "utf8.h"
> +
> +static char *ps_matched;
> +static const struct cache_entry **ce_entries;
> +static int ce_alloc, ce_used;
> +static struct pathspec pathspec;
> +static const char *alternative_path;
> +
> +static void module_list_compute(int argc, const char **argv,
> +				const char *prefix,
> +				struct pathspec *pathspec)
> +{
> +	int i;
> +	char *max_prefix;
> +	int max_prefix_len;
> +	parse_pathspec(pathspec, 0,
> +		       PATHSPEC_PREFER_FULL |
> +		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> +		       prefix, argv);
> +
> +	/* Find common prefix for all pathspec's */
> +	max_prefix = common_prefix(pathspec);
> +	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> +
> +	if (pathspec->nr)
> +		ps_matched = xcalloc(1, pathspec->nr);
> +
> +
> +	if (read_cache() < 0)
> +		die("index file corrupt");
> +
> +	for (i = 0; i < active_nr; i++) {
> +		const struct cache_entry *ce = active_cache[i];
> +
> +		if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> +				    max_prefix_len, ps_matched,
> +				    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))
> +			continue;
> +
> +		if (S_ISGITLINK(ce->ce_mode)) {
> +			ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
> +			ce_entries[ce_used++] = ce;
> +		}
> +	}
> +}
> +
> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> +	int i;
> +	struct string_list already_printed = STRING_LIST_INIT_NODUP;
> +
> +	struct option module_list_options[] = {
> +		OPT_STRING(0, "prefix", &alternative_path,
> +			   N_("path"),
> +			   N_("alternative anchor for relative paths")),
> +		OPT_END()
> +	};
> +
> +	static const char * const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper module_list [--prefix=<path>] [<path>...]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_list_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	module_list_compute(argc, argv, alternative_path
> +					? alternative_path
> +					: prefix, &pathspec);
> +
> +	if (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {
> +		printf("#unmatched\n");
> +		return 1;
> +	}
> +
> +	for (i = 0; i < ce_used; i++) {
> +		const struct cache_entry *ce = ce_entries[i];
> +
> +		if (string_list_has_string(&already_printed, ce->name))
> +			continue;
> +
> +		if (ce_stage(ce)) {
> +			printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1));
> +		} else {
> +			printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));
> +		}
> +
> +		utf8_fprintf(stdout, "%s\n", ce->name);
> +
> +		string_list_insert(&already_printed, ce->name);
> +	}
> +	return 0;
> +}
> +
> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> +{
> +	if (argc < 2)
> +		goto usage;
> +
> +	if (!strcmp(argv[1], "module_list"))
> +		return module_list(argc - 1, argv + 1, prefix);
> +
> +usage:
> +	usage("git submodule--helper module_list\n");
> +}
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 36797c3..af9ecef 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -145,48 +145,6 @@ relative_path ()
>   	echo "$result$target"
>   }
>
> -#
> -# Get submodule info for registered submodules
> -# $@ = path to limit submodule list
> -#
> -module_list()
> -{
> -	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> -	(
> -		git ls-files -z --error-unmatch --stage -- "$@" ||
> -		echo "unmatched pathspec exists"
> -	) |
> -	@@PERL@@ -e '
> -	my %unmerged = ();
> -	my ($null_sha1) = ("0" x 40);
> -	my @out = ();
> -	my $unmatched = 0;
> -	$/ = "\0";
> -	while (<STDIN>) {
> -		if (/^unmatched pathspec/) {
> -			$unmatched = 1;
> -			next;
> -		}
> -		chomp;
> -		my ($mode, $sha1, $stage, $path) =
> -			/^([0-7]+) ([0-9a-f]{40}) ([0-3])\t(.*)$/;
> -		next unless $mode eq "160000";
> -		if ($stage ne "0") {
> -			if (!$unmerged{$path}++) {
> -				push @out, "$mode $null_sha1 U\t$path\n";
> -			}
> -			next;
> -		}
> -		push @out, "$_\n";
> -	}
> -	if ($unmatched) {
> -		print "#unmatched\n";
> -	} else {
> -		print for (@out);
> -	}
> -	'
> -}
> -
>   die_if_unmatched ()
>   {
>   	if test "$1" = "#unmatched"
> @@ -532,7 +490,7 @@ cmd_foreach()
>   	# command in the subshell (and a recursive call to this function)
>   	exec 3<&0
>
> -	module_list |
> +	git submodule--helper module_list --prefix "$wt_prefix"|
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> @@ -592,7 +550,7 @@ cmd_init()
>   		shift
>   	done
>
> -	module_list "$@" |
> +	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> @@ -674,7 +632,7 @@ cmd_deinit()
>   		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
>   	fi
>
> -	module_list "$@" |
> +	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> @@ -790,7 +748,7 @@ cmd_update()
>   	fi
>
>   	cloned_modules=
> -	module_list "$@" | {
> +	git submodule--helper module_list --prefix "$wt_prefix" "$@" | {
>   	err=
>   	while read mode sha1 stage sm_path
>   	do
> @@ -1222,7 +1180,7 @@ cmd_status()
>   		shift
>   	done
>
> -	module_list "$@" |
> +	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> @@ -1299,7 +1257,7 @@ cmd_sync()
>   		esac
>   	done
>   	cd_to_toplevel
> -	module_list "$@" |
> +	git submodule--helper module_list --prefix "$wt_prefix" "$@" |
>   	while read mode sha1 stage sm_path
>   	do
>   		die_if_unmatched "$mode"
> diff --git a/git.c b/git.c
> index 55c327c..deecba0 100644
> --- a/git.c
> +++ b/git.c
> @@ -469,6 +469,7 @@ static struct cmd_struct commands[] = {
>   	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
>   	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
>   	{ "stripspace", cmd_stripspace },
> +	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP },
>   	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
>   	{ "tag", cmd_tag, RUN_SETUP },
>   	{ "unpack-file", cmd_unpack_file, RUN_SETUP },
>

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> new file mode 100644
> index 0000000..cb18ddf
> --- /dev/null
> +++ b/builtin/submodule--helper.c
> @@ -0,0 +1,111 @@
> + ...
> +static char *ps_matched;
> +static const struct cache_entry **ce_entries;
> +static int ce_alloc, ce_used;
> +static struct pathspec pathspec;
> +static const char *alternative_path;

> +static void module_list_compute(int argc, const char **argv,
> +				const char *prefix,
> +				struct pathspec *pathspec)
> +{
> +	int i;
> +	char *max_prefix;
> +	int max_prefix_len;
> +	parse_pathspec(pathspec, 0,
> +		       PATHSPEC_PREFER_FULL |
> +		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> +		       prefix, argv);
> +
> +	/* Find common prefix for all pathspec's */
> +	max_prefix = common_prefix(pathspec);
> +	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> +
> +	if (pathspec->nr)
> +		ps_matched = xcalloc(1, pathspec->nr);
> +
> +
> +	if (read_cache() < 0)
> +		die("index file corrupt");

> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> +...
> +	for (i = 0; i < ce_used; i++) {
> +		const struct cache_entry *ce = ce_entries[i];
> +
> +		if (string_list_has_string(&already_printed, ce->name))
> +			continue;
> +
> +		if (ce_stage(ce)) {
> +			printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1));
> +		} else {
> +			printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));
> +		}
> +
> +		utf8_fprintf(stdout, "%s\n", ce->name);
> +
> +		string_list_insert(&already_printed, ce->name);