{"thread":{"id":"40015","subject":"[PATCH 2/4] submodule: implement `module_name` as a builtin helper","startedAt":"2015-08-05T00:04:29Z","lastAt":"2015-08-10T17:03:16Z","messageCount":27,"participants":["Stefan Beller","Eric Sunshine","Jens Lehmann","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"267491","messageId":"1438733070-15805-1-git-send-email-sbeller@google.com","threadId":"40015","inReplyTo":null,"subject":"[PATCH 1/4] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-05T00:04:29Z","receivedAt":"2015-08-05T00:04:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Most of the submodule operations work on a set of submodules.\nCalculating and using this set is usually done via:\n\n       module_list \"$@\" | {\n           while read mode sha1 stage sm_path\n           do\n                # the actual operation\n           done\n       }\n\nCurrently the function `module_list` is implemented in the\ngit-submodule.sh as a shell script wrapping a perl script.\nThe rewrite is in C, such that it is faster and can later be\neasily adapted when other functions are rewritten in C.\n\ngit-submodule.sh similar to the builtin commands will navigate\nto the top most directory of the repository and keeping the\nsubdirectories as a variable. As the helper is called from\nwithin the git-submodule.sh script, we are already navigated\nto the root level, but the path arguments are stil relative\nto the subdirectory we were in when calling git-submodule.sh.\nThat's why there is a `--prefix` option pointing to an alternative\npath where to anchor relative path arguments.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThe same as yesterday evening, just an entry added to .gitignore.\n\nSo we'll have a \"git submodule--helper module_list\" here.\n\n .gitignore                  |   1 +\n Makefile                    |   1 +\n builtin.h                   |   1 +\n builtin/submodule--helper.c | 111 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  54 +++------------------\n git.c                       |   1 +\n 6 files changed, 121 insertions(+), 48 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\ndiff --git a/.gitignore b/.gitignore\nindex a685ec1..2a69ba0 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -155,6 +155,7 @@\n /git-status\n /git-stripspace\n /git-submodule\n+/git-submodule--helper\n /git-svn\n /git-symbolic-ref\n /git-tag\ndiff --git a/Makefile b/Makefile\nindex 7efedbe..460d17a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -899,6 +899,7 @@ BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-ref.o\n BUILTIN_OBJS += builtin/stripspace.o\n+BUILTIN_OBJS += builtin/submodule--helper.o\n BUILTIN_OBJS += builtin/symbolic-ref.o\n BUILTIN_OBJS += builtin/tag.o\n BUILTIN_OBJS += builtin/unpack-file.o\ndiff --git a/builtin.h b/builtin.h\nindex 839483d..924e6c4 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);\n extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\n extern int cmd_status(int argc, const char **argv, const char *prefix);\n extern int cmd_stripspace(int argc, const char **argv, const char *prefix);\n+extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_tag(int argc, const char **argv, const char *prefix);\n extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nnew file mode 100644\nindex 0000000..cb18ddf\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,111 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"parse-options.h\"\n+#include \"quote.h\"\n+#include \"pathspec.h\"\n+#include \"dir.h\"\n+#include \"utf8.h\"\n+\n+static char *ps_matched;\n+static const struct cache_entry **ce_entries;\n+static int ce_alloc, ce_used;\n+static struct pathspec pathspec;\n+static const char *alternative_path;\n+\n+static void module_list_compute(int argc, const char **argv,\n+\t\t\t\tconst char *prefix,\n+\t\t\t\tstruct pathspec *pathspec)\n+{\n+\tint i;\n+\tchar *max_prefix;\n+\tint max_prefix_len;\n+\tparse_pathspec(pathspec, 0,\n+\t\t       PATHSPEC_PREFER_FULL |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n+\t\t       prefix, argv);\n+\n+\t/* Find common prefix for all pathspec's */\n+\tmax_prefix = common_prefix(pathspec);\n+\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n+\n+\tif (pathspec->nr)\n+\t\tps_matched = xcalloc(1, pathspec->nr);\n+\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"index file corrupt\");\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tconst struct cache_entry *ce = active_cache[i];\n+\n+\t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n+\t\t\t\t    max_prefix_len, ps_matched,\n+\t\t\t\t    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))\n+\t\t\tcontinue;\n+\n+\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\tALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n+\t\t\tce_entries[ce_used++] = ce;\n+\t\t}\n+\t}\n+}\n+\n+static int module_list(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstruct string_list already_printed = STRING_LIST_INIT_NODUP;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &alternative_path,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tstatic const char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper module_list [--prefix=<path>] [<path>...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tmodule_list_compute(argc, argv, alternative_path\n+\t\t\t\t\t? alternative_path\n+\t\t\t\t\t: prefix, &pathspec);\n+\n+\tif (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor (i = 0; i < ce_used; i++) {\n+\t\tconst struct cache_entry *ce = ce_entries[i];\n+\n+\t\tif (string_list_has_string(&already_printed, ce->name))\n+\t\t\tcontinue;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n+\t\t} else {\n+\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\t\t}\n+\n+\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n+\n+\t\tstring_list_insert(&already_printed, ce->name);\n+\t}\n+\treturn 0;\n+}\n+\n+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc < 2)\n+\t\tgoto usage;\n+\n+\tif (!strcmp(argv[1], \"module_list\"))\n+\t\treturn module_list(argc - 1, argv + 1, prefix);\n+\n+usage:\n+\tusage(\"git submodule--helper module_list\\n\");\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..af9ecef 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -145,48 +145,6 @@ relative_path ()\n \techo \"$result$target\"\n }\n \n-#\n-# Get submodule info for registered submodules\n-# $@ = path to limit submodule list\n-#\n-module_list()\n-{\n-\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n-\t(\n-\t\tgit ls-files -z --error-unmatch --stage -- \"$@\" ||\n-\t\techo \"unmatched pathspec exists\"\n-\t) |\n-\t@@PERL@@ -e '\n-\tmy %unmerged = ();\n-\tmy ($null_sha1) = (\"0\" x 40);\n-\tmy @out = ();\n-\tmy $unmatched = 0;\n-\t$/ = \"\\0\";\n-\twhile (<STDIN>) {\n-\t\tif (/^unmatched pathspec/) {\n-\t\t\t$unmatched = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tchomp;\n-\t\tmy ($mode, $sha1, $stage, $path) =\n-\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n-\t\tnext unless $mode eq \"160000\";\n-\t\tif ($stage ne \"0\") {\n-\t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n-\t\t\t}\n-\t\t\tnext;\n-\t\t}\n-\t\tpush @out, \"$_\\n\";\n-\t}\n-\tif ($unmatched) {\n-\t\tprint \"#unmatched\\n\";\n-\t} else {\n-\t\tprint for (@out);\n-\t}\n-\t'\n-}\n-\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n@@ -532,7 +490,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tmodule_list |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\"|\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -592,7 +550,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -674,7 +632,7 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -790,7 +748,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tmodule_list \"$@\" | {\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -1222,7 +1180,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -1299,7 +1257,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\ndiff --git a/git.c b/git.c\nindex 55c327c..deecba0 100644\n--- a/git.c\n+++ b/git.c\n@@ -469,6 +469,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n+\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n \t{ \"tag\", cmd_tag, RUN_SETUP },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n-- \n2.5.0.2.gbb9888f.dirty\n"},{"id":"267490","messageId":"1438733070-15805-2-git-send-email-sbeller@google.com","threadId":"40015","inReplyTo":"1438733070-15805-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/4] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-05T00:04:30Z","receivedAt":"2015-08-05T00:04:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The goal of this series being rewriting `git submodule update`,\nwe don't want to call out to the shell script for config lookups.\n\nSo reimplement the lookup of the submodule name in C.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n When I started to implement git submodule add in the helper, I realized\n the very first thing to be done would be module_name translated to C,\n so I did that separately. Maybe we need to split this up as well into two\n separate steps for processing and I/O, such that it can be reused better\n from a future \"git submodule--helper update\" function\n\n builtin/submodule--helper.c | 47 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 32 +++++++-----------------------\n 2 files changed, 54 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex cb18ddf..dd5635f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -5,6 +5,8 @@\n #include \"pathspec.h\"\n #include \"dir.h\"\n #include \"utf8.h\"\n+#include \"run-command.h\"\n+#include \"string-list.h\"\n \n static char *ps_matched;\n static const struct cache_entry **ce_entries;\n@@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+\n+static int collect_module_names(const char *key, const char *value, void *cb)\n+{\n+\tsize_t len;\n+\tstruct string_list *sl = cb;\n+\n+\tif (starts_with(key, \"submodule.\")\n+\t    && strip_suffix(key, \".path\", &len)) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_add(&sb, key + strlen(\"submodule.\"),\n+\t\t\t\tlen - strlen(\"submodule.\"));\n+\t\tstring_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static int module_name(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct string_list_item *item;\n+\tstruct git_config_source config_source;\n+\tstruct string_list values = STRING_LIST_INIT_DUP;\n+\n+\tif (!argc)\n+\t\tusage(\"git submodule--helper module_name <path>\\n\");\n+\n+\tmemset(&config_source, 0, sizeof(config_source));\n+\tconfig_source.file = \".gitmodules\";\n+\n+\tif (git_config_with_options(collect_module_names, &values,\n+\t\t\t\t    &config_source, 1) < 0)\n+\t\tdie(_(\"unknown error occured while reading the git modules file\"));\n+\n+\titem = string_list_lookup(&values, argv[0]);\n+\tif (item)\n+\t\tprintf(\"%s\\n\", (char*)item->util);\n+\telse\n+\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n+\treturn 0;\n+}\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n \tif (argc < 2)\n@@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"module_list\"))\n \t\treturn module_list(argc - 1, argv + 1, prefix);\n \n+\tif (!strcmp(argv[1], \"module_name\"))\n+\t\treturn module_name(argc - 2, argv + 2, prefix);\n+\n usage:\n \tusage(\"git submodule--helper module_list\\n\");\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex af9ecef..e6ff38d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -178,24 +178,6 @@ get_submodule_config () {\n \tprintf '%s' \"${value:-$default}\"\n }\n \n-\n-#\n-# Map submodule path to submodule name\n-#\n-# $1 = path\n-#\n-module_name()\n-{\n-\t# Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n-\tsm_path=\"$1\"\n-\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n-\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n-\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n-\ttest -z \"$name\" &&\n-\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n-\tprintf '%s\\n' \"$name\"\n-}\n-\n #\n # Clone a submodule\n #\n@@ -498,7 +480,7 @@ cmd_foreach()\n \t\tthen\n \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n-\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tclear_local_git_env\n@@ -554,7 +536,7 @@ cmd_init()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -636,7 +618,7 @@ cmd_deinit()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -758,7 +740,7 @@ cmd_update()\n \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n \t\t\tcontinue\n \t\tfi\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n@@ -1022,7 +1004,7 @@ cmd_summary() {\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n-\t\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n \t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n@@ -1184,7 +1166,7 @@ cmd_status()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \t\tif test \"$stage\" = U\n@@ -1261,7 +1243,7 @@ cmd_sync()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\")\n+\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \n \t\t# Possibly a url relative to parent\n-- \n2.5.0.2.gbb9888f.dirty\n"},{"id":"267492","messageId":"CAGZ79kZzqT7pJg_hr0z6nipTQYSNeCZf9sRZjO5pyH1JWe2LZw@mail.gmail.com","threadId":"40015","inReplyTo":"1438733070-15805-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-05T00:05:37Z","receivedAt":"2015-08-05T00:05:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The series consists of 2 patches only actually. The next patches have\nnot been sent as they are heavy WIP.\n"},{"id":"267493","messageId":"CAPig+cQtoWF_9Km4H7MXDf7ySQNUP9gncxkrV3NFTrULA8fbbA@mail.gmail.com","threadId":"40015","inReplyTo":"1438733070-15805-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: implement `module_name` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-05T00:58:41Z","receivedAt":"2015-08-05T00:58:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 4, 2015 at 8:04 PM, Stefan Beller <sbeller@google.com> wrote:\n> The goal of this series being rewriting `git submodule update`,\n> we don't want to call out to the shell script for config lookups.\n>\n> So reimplement the lookup of the submodule name in C.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index cb18ddf..dd5635f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> +static int collect_module_names(const char *key, const char *value, void *cb)\n> +{\n> +       size_t len;\n> +       struct string_list *sl = cb;\n> +\n> +       if (starts_with(key, \"submodule.\")\n> +           && strip_suffix(key, \".path\", &len)) {\n> +               struct strbuf sb = STRBUF_INIT;\n> +               strbuf_add(&sb, key + strlen(\"submodule.\"),\n> +                               len - strlen(\"submodule.\"));\n> +               string_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);\n> +               strbuf_release(&sb);\n\nWhy the complexity and overhead of a strbuf when the same could be\naccomplished more easily and straightforwardly with xstrndup()?\n\n> +       }\n> +\n> +       return 0;\n> +}\n> +\n> +static int module_name(int argc, const char **argv, const char *prefix)\n> +{\n> +       struct string_list_item *item;\n> +       struct git_config_source config_source;\n> +       struct string_list values = STRING_LIST_INIT_DUP;\n> +\n> +       if (!argc)\n\nDo you mean?\n\n    if (argc != 1)\n\n> +               usage(\"git submodule--helper module_name <path>\\n\");\n> +\n> +       memset(&config_source, 0, sizeof(config_source));\n> +       config_source.file = \".gitmodules\";\n> +\n> +       if (git_config_with_options(collect_module_names, &values,\n> +                                   &config_source, 1) < 0)\n> +               die(_(\"unknown error occured while reading the git modules file\"));\n> +\n> +       item = string_list_lookup(&values, argv[0]);\n> +       if (item)\n> +               printf(\"%s\\n\", (char*)item->util);\n> +       else\n> +               die(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n> +       return 0;\n> +}\n> +\n>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>  {\n>         if (argc < 2)\n> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>         if (!strcmp(argv[1], \"module_list\"))\n>                 return module_list(argc - 1, argv + 1, prefix);\n>\n> +       if (!strcmp(argv[1], \"module_name\"))\n> +               return module_name(argc - 2, argv + 2, prefix);\n> +\n>  usage:\n>         usage(\"git submodule--helper module_list\\n\");\n>  }\n"},{"id":"267518","messageId":"CAGZ79kb8q_OzdMTxeNQo6t0T9Ay8TJkMPPqZU-G0CkOZ9rA_hA@mail.gmail.com","threadId":"40015","inReplyTo":"CAPig+cQtoWF_9Km4H7MXDf7ySQNUP9gncxkrV3NFTrULA8fbbA@mail.gmail.com","subject":"Re: [PATCH 2/4] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-05T16:29:37Z","receivedAt":"2015-08-05T16:29:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 4, 2015 at 5:58 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Aug 4, 2015 at 8:04 PM, Stefan Beller <sbeller@google.com> wrote:\n>> The goal of this series being rewriting `git submodule update`,\n>> we don't want to call out to the shell script for config lookups.\n>>\n>> So reimplement the lookup of the submodule name in C.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index cb18ddf..dd5635f 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>> +static int collect_module_names(const char *key, const char *value, void *cb)\n>> +{\n>> +       size_t len;\n>> +       struct string_list *sl = cb;\n>> +\n>> +       if (starts_with(key, \"submodule.\")\n>> +           && strip_suffix(key, \".path\", &len)) {\n>> +               struct strbuf sb = STRBUF_INIT;\n>> +               strbuf_add(&sb, key + strlen(\"submodule.\"),\n>> +                               len - strlen(\"submodule.\"));\n>> +               string_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);\n>> +               strbuf_release(&sb);\n>\n> Why the complexity and overhead of a strbuf when the same could be\n> accomplished more easily and straightforwardly with xstrndup()?\n\nfixed.\n\n>\n>> +       }\n>> +\n>> +       return 0;\n>> +}\n>> +\n>> +static int module_name(int argc, const char **argv, const char *prefix)\n>> +{\n>> +       struct string_list_item *item;\n>> +       struct git_config_source config_source;\n>> +       struct string_list values = STRING_LIST_INIT_DUP;\n>> +\n>> +       if (!argc)\n>\n> Do you mean?\n>\n>     if (argc != 1)\n\ndoh! Yes I meant that.\n\n>\n>> +               usage(\"git submodule--helper module_name <path>\\n\");\n>> +\n>> +       memset(&config_source, 0, sizeof(config_source));\n>> +       config_source.file = \".gitmodules\";\n>> +\n>> +       if (git_config_with_options(collect_module_names, &values,\n>> +                                   &config_source, 1) < 0)\n>> +               die(_(\"unknown error occured while reading the git modules file\"));\n>> +\n>> +       item = string_list_lookup(&values, argv[0]);\n>> +       if (item)\n>> +               printf(\"%s\\n\", (char*)item->util);\n>> +       else\n>> +               die(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n>> +       return 0;\n>> +}\n>> +\n>>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>>  {\n>>         if (argc < 2)\n>> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>>         if (!strcmp(argv[1], \"module_list\"))\n>>                 return module_list(argc - 1, argv + 1, prefix);\n>>\n>> +       if (!strcmp(argv[1], \"module_name\"))\n>> +               return module_name(argc - 2, argv + 2, prefix);\n>> +\n>>  usage:\n>>         usage(\"git submodule--helper module_list\\n\");\n>>  }\n"},{"id":"267529","messageId":"55C25681.3010208@web.de","threadId":"40015","inReplyTo":"1438733070-15805-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/4] submodule: implement `module_list` as a builtin helper","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-08-05T18:31:29Z","receivedAt":"2015-08-05T18:31:29Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 05.08.2015 um 02:04 schrieb Stefan Beller:\n> Most of the submodule operations work on a set of submodules.\n> Calculating and using this set is usually done via:\n>\n>         module_list \"$@\" | {\n>             while read mode sha1 stage sm_path\n>             do\n>                  # the actual operation\n>             done\n>         }\n>\n> Currently the function `module_list` is implemented in the\n> git-submodule.sh as a shell script wrapping a perl script.\n> The rewrite is in C, such that it is faster and can later be\n> easily adapted when other functions are rewritten in C.\n>\n> git-submodule.sh similar to the builtin commands will navigate\n> to the top most directory of the repository and keeping the\n> subdirectories as a variable. As the helper is called from\n> within the git-submodule.sh script, we are already navigated\n> to the root level, but the path arguments are stil relative\n> to the subdirectory we were in when calling git-submodule.sh.\n> That's why there is a `--prefix` option pointing to an alternative\n> path where to anchor relative path arguments.\n\nGreat you are working on this! I'll try to help, but you might\nsee some latency as my Git time budget is currently very limited.\n\nI think this patch is definitely going into the right direction.\nThe whole test suite runs 3 seconds faster for me with this\napplied: best of three is 3:16 without and 3:13 with this patch.\nThat's quite an improvement, especially as only parts of the test\nsuite deal with submodules! (And I expect Windows users to profit\neven more, considered how expensive forking is there)\n\nAcked-by: Jens Lehmann <Jens.Lehmann@web.de>\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> The same as yesterday evening, just an entry added to .gitignore.\n>\n> So we'll have a \"git submodule--helper module_list\" here.\n>\n>   .gitignore                  |   1 +\n>   Makefile                    |   1 +\n>   builtin.h                   |   1 +\n>   builtin/submodule--helper.c | 111 ++++++++++++++++++++++++++++++++++++++++++++\n>   git-submodule.sh            |  54 +++------------------\n>   git.c                       |   1 +\n>   6 files changed, 121 insertions(+), 48 deletions(-)\n>   create mode 100644 builtin/submodule--helper.c\n>\n> diff --git a/.gitignore b/.gitignore\n> index a685ec1..2a69ba0 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -155,6 +155,7 @@\n>   /git-status\n>   /git-stripspace\n>   /git-submodule\n> +/git-submodule--helper\n>   /git-svn\n>   /git-symbolic-ref\n>   /git-tag\n> diff --git a/Makefile b/Makefile\n> index 7efedbe..460d17a 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -899,6 +899,7 @@ BUILTIN_OBJS += builtin/shortlog.o\n>   BUILTIN_OBJS += builtin/show-branch.o\n>   BUILTIN_OBJS += builtin/show-ref.o\n>   BUILTIN_OBJS += builtin/stripspace.o\n> +BUILTIN_OBJS += builtin/submodule--helper.o\n>   BUILTIN_OBJS += builtin/symbolic-ref.o\n>   BUILTIN_OBJS += builtin/tag.o\n>   BUILTIN_OBJS += builtin/unpack-file.o\n> diff --git a/builtin.h b/builtin.h\n> index 839483d..924e6c4 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);\n>   extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\n>   extern int cmd_status(int argc, const char **argv, const char *prefix);\n>   extern int cmd_stripspace(int argc, const char **argv, const char *prefix);\n> +extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n>   extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n>   extern int cmd_tag(int argc, const char **argv, const char *prefix);\n>   extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> new file mode 100644\n> index 0000000..cb18ddf\n> --- /dev/null\n> +++ b/builtin/submodule--helper.c\n> @@ -0,0 +1,111 @@\n> +#include \"builtin.h\"\n> +#include \"cache.h\"\n> +#include \"parse-options.h\"\n> +#include \"quote.h\"\n> +#include \"pathspec.h\"\n> +#include \"dir.h\"\n> +#include \"utf8.h\"\n> +\n> +static char *ps_matched;\n> +static const struct cache_entry **ce_entries;\n> +static int ce_alloc, ce_used;\n> +static struct pathspec pathspec;\n> +static const char *alternative_path;\n> +\n> +static void module_list_compute(int argc, const char **argv,\n> +\t\t\t\tconst char *prefix,\n> +\t\t\t\tstruct pathspec *pathspec)\n> +{\n> +\tint i;\n> +\tchar *max_prefix;\n> +\tint max_prefix_len;\n> +\tparse_pathspec(pathspec, 0,\n> +\t\t       PATHSPEC_PREFER_FULL |\n> +\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n> +\t\t       prefix, argv);\n> +\n> +\t/* Find common prefix for all pathspec's */\n> +\tmax_prefix = common_prefix(pathspec);\n> +\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> +\n> +\tif (pathspec->nr)\n> +\t\tps_matched = xcalloc(1, pathspec->nr);\n> +\n> +\n> +\tif (read_cache() < 0)\n> +\t\tdie(\"index file corrupt\");\n> +\n> +\tfor (i = 0; i < active_nr; i++) {\n> +\t\tconst struct cache_entry *ce = active_cache[i];\n> +\n> +\t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n> +\t\t\t\t    max_prefix_len, ps_matched,\n> +\t\t\t\t    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (S_ISGITLINK(ce->ce_mode)) {\n> +\t\t\tALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n> +\t\t\tce_entries[ce_used++] = ce;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +static int module_list(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint i;\n> +\tstruct string_list already_printed = STRING_LIST_INIT_NODUP;\n> +\n> +\tstruct option module_list_options[] = {\n> +\t\tOPT_STRING(0, \"prefix\", &alternative_path,\n> +\t\t\t   N_(\"path\"),\n> +\t\t\t   N_(\"alternative anchor for relative paths\")),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tstatic const char * const git_submodule_helper_usage[] = {\n> +\t\tN_(\"git submodule--helper module_list [--prefix=<path>] [<path>...]\"),\n> +\t\tNULL\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, module_list_options,\n> +\t\t\t     git_submodule_helper_usage, 0);\n> +\n> +\tmodule_list_compute(argc, argv, alternative_path\n> +\t\t\t\t\t? alternative_path\n> +\t\t\t\t\t: prefix, &pathspec);\n> +\n> +\tif (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n> +\t\tprintf(\"#unmatched\\n\");\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tfor (i = 0; i < ce_used; i++) {\n> +\t\tconst struct cache_entry *ce = ce_entries[i];\n> +\n> +\t\tif (string_list_has_string(&already_printed, ce->name))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (ce_stage(ce)) {\n> +\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n> +\t\t} else {\n> +\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n> +\t\t}\n> +\n> +\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n> +\n> +\t\tstring_list_insert(&already_printed, ce->name);\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +\tif (argc < 2)\n> +\t\tgoto usage;\n> +\n> +\tif (!strcmp(argv[1], \"module_list\"))\n> +\t\treturn module_list(argc - 1, argv + 1, prefix);\n> +\n> +usage:\n> +\tusage(\"git submodule--helper module_list\\n\");\n> +}\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 36797c3..af9ecef 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -145,48 +145,6 @@ relative_path ()\n>   \techo \"$result$target\"\n>   }\n>\n> -#\n> -# Get submodule info for registered submodules\n> -# $@ = path to limit submodule list\n> -#\n> -module_list()\n> -{\n> -\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n> -\t(\n> -\t\tgit ls-files -z --error-unmatch --stage -- \"$@\" ||\n> -\t\techo \"unmatched pathspec exists\"\n> -\t) |\n> -\t@@PERL@@ -e '\n> -\tmy %unmerged = ();\n> -\tmy ($null_sha1) = (\"0\" x 40);\n> -\tmy @out = ();\n> -\tmy $unmatched = 0;\n> -\t$/ = \"\\0\";\n> -\twhile (<STDIN>) {\n> -\t\tif (/^unmatched pathspec/) {\n> -\t\t\t$unmatched = 1;\n> -\t\t\tnext;\n> -\t\t}\n> -\t\tchomp;\n> -\t\tmy ($mode, $sha1, $stage, $path) =\n> -\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n> -\t\tnext unless $mode eq \"160000\";\n> -\t\tif ($stage ne \"0\") {\n> -\t\t\tif (!$unmerged{$path}++) {\n> -\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n> -\t\t\t}\n> -\t\t\tnext;\n> -\t\t}\n> -\t\tpush @out, \"$_\\n\";\n> -\t}\n> -\tif ($unmatched) {\n> -\t\tprint \"#unmatched\\n\";\n> -\t} else {\n> -\t\tprint for (@out);\n> -\t}\n> -\t'\n> -}\n> -\n>   die_if_unmatched ()\n>   {\n>   \tif test \"$1\" = \"#unmatched\"\n> @@ -532,7 +490,7 @@ cmd_foreach()\n>   \t# command in the subshell (and a recursive call to this function)\n>   \texec 3<&0\n>\n> -\tmodule_list |\n> +\tgit submodule--helper module_list --prefix \"$wt_prefix\"|\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> @@ -592,7 +550,7 @@ cmd_init()\n>   \t\tshift\n>   \tdone\n>\n> -\tmodule_list \"$@\" |\n> +\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> @@ -674,7 +632,7 @@ cmd_deinit()\n>   \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n>   \tfi\n>\n> -\tmodule_list \"$@\" |\n> +\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> @@ -790,7 +748,7 @@ cmd_update()\n>   \tfi\n>\n>   \tcloned_modules=\n> -\tmodule_list \"$@\" | {\n> +\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" | {\n>   \terr=\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n> @@ -1222,7 +1180,7 @@ cmd_status()\n>   \t\tshift\n>   \tdone\n>\n> -\tmodule_list \"$@\" |\n> +\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> @@ -1299,7 +1257,7 @@ cmd_sync()\n>   \t\tesac\n>   \tdone\n>   \tcd_to_toplevel\n> -\tmodule_list \"$@\" |\n> +\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> diff --git a/git.c b/git.c\n> index 55c327c..deecba0 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -469,6 +469,7 @@ static struct cmd_struct commands[] = {\n>   \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n>   \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n>   \t{ \"stripspace\", cmd_stripspace },\n> +\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\n>   \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n>   \t{ \"tag\", cmd_tag, RUN_SETUP },\n>   \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n>\n"},{"id":"267531","messageId":"55C25EBB.2000304@web.de","threadId":"40015","inReplyTo":"1438733070-15805-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: implement `module_name` as a builtin helper","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-08-05T19:06:35Z","receivedAt":"2015-08-05T19:06:35Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 05.08.2015 um 02:04 schrieb Stefan Beller:\n> The goal of this series being rewriting `git submodule update`,\n> we don't want to call out to the shell script for config lookups.\n>\n> So reimplement the lookup of the submodule name in C.\n\nCool. This brings down the duration of the test suite from 3:13\nto 3:12 for me (best of three).\n\nYou might wanna have a look into submodule.c: after initially\ncalling gitmodules_config() one can lookup the submodule name\nin the static \"config_name_for_path\" string_list. If you'd add\na public method to submodule.c which accesses that string_list\nand returns the name for the given path, you won't need your\ntwo new functions ... or am I missing something?\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n>   When I started to implement git submodule add in the helper, I realized\n>   the very first thing to be done would be module_name translated to C,\n>   so I did that separately. Maybe we need to split this up as well into two\n>   separate steps for processing and I/O, such that it can be reused better\n>   from a future \"git submodule--helper update\" function\n>\n>   builtin/submodule--helper.c | 47 +++++++++++++++++++++++++++++++++++++++++++++\n>   git-submodule.sh            | 32 +++++++-----------------------\n>   2 files changed, 54 insertions(+), 25 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index cb18ddf..dd5635f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -5,6 +5,8 @@\n>   #include \"pathspec.h\"\n>   #include \"dir.h\"\n>   #include \"utf8.h\"\n> +#include \"run-command.h\"\n> +#include \"string-list.h\"\n>\n>   static char *ps_matched;\n>   static const struct cache_entry **ce_entries;\n> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>   \treturn 0;\n>   }\n>\n> +\n> +static int collect_module_names(const char *key, const char *value, void *cb)\n> +{\n> +\tsize_t len;\n> +\tstruct string_list *sl = cb;\n> +\n> +\tif (starts_with(key, \"submodule.\")\n> +\t    && strip_suffix(key, \".path\", &len)) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstrbuf_add(&sb, key + strlen(\"submodule.\"),\n> +\t\t\t\tlen - strlen(\"submodule.\"));\n> +\t\tstring_list_insert(sl, value)->util = strbuf_detach(&sb, NULL);\n> +\t\tstrbuf_release(&sb);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int module_name(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct string_list_item *item;\n> +\tstruct git_config_source config_source;\n> +\tstruct string_list values = STRING_LIST_INIT_DUP;\n> +\n> +\tif (!argc)\n> +\t\tusage(\"git submodule--helper module_name <path>\\n\");\n> +\n> +\tmemset(&config_source, 0, sizeof(config_source));\n> +\tconfig_source.file = \".gitmodules\";\n> +\n> +\tif (git_config_with_options(collect_module_names, &values,\n> +\t\t\t\t    &config_source, 1) < 0)\n> +\t\tdie(_(\"unknown error occured while reading the git modules file\"));\n> +\n> +\titem = string_list_lookup(&values, argv[0]);\n> +\tif (item)\n> +\t\tprintf(\"%s\\n\", (char*)item->util);\n> +\telse\n> +\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n> +\treturn 0;\n> +}\n> +\n>   int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>   {\n>   \tif (argc < 2)\n> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>   \tif (!strcmp(argv[1], \"module_list\"))\n>   \t\treturn module_list(argc - 1, argv + 1, prefix);\n>\n> +\tif (!strcmp(argv[1], \"module_name\"))\n> +\t\treturn module_name(argc - 2, argv + 2, prefix);\n> +\n>   usage:\n>   \tusage(\"git submodule--helper module_list\\n\");\n>   }\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index af9ecef..e6ff38d 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -178,24 +178,6 @@ get_submodule_config () {\n>   \tprintf '%s' \"${value:-$default}\"\n>   }\n>\n> -\n> -#\n> -# Map submodule path to submodule name\n> -#\n> -# $1 = path\n> -#\n> -module_name()\n> -{\n> -\t# Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n> -\tsm_path=\"$1\"\n> -\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n> -\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n> -\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n> -\ttest -z \"$name\" &&\n> -\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n> -\tprintf '%s\\n' \"$name\"\n> -}\n> -\n>   #\n>   # Clone a submodule\n>   #\n> @@ -498,7 +480,7 @@ cmd_foreach()\n>   \t\tthen\n>   \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n>   \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n> -\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>   \t\t\t(\n>   \t\t\t\tprefix=\"$prefix$sm_path/\"\n>   \t\t\t\tclear_local_git_env\n> @@ -554,7 +536,7 @@ cmd_init()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>\n>   \t\tdisplaypath=$(relative_path \"$sm_path\")\n>\n> @@ -636,7 +618,7 @@ cmd_deinit()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>\n>   \t\tdisplaypath=$(relative_path \"$sm_path\")\n>\n> @@ -758,7 +740,7 @@ cmd_update()\n>   \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n>   \t\t\tcontinue\n>   \t\tfi\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>   \t\turl=$(git config submodule.\"$name\".url)\n>   \t\tbranch=$(get_submodule_config \"$name\" branch master)\n>   \t\tif ! test -z \"$update\"\n> @@ -1022,7 +1004,7 @@ cmd_summary() {\n>   \t\t\t# Respect the ignore setting for --for-status.\n>   \t\t\tif test -n \"$for_status\"\n>   \t\t\tthen\n> -\t\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>   \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n>   \t\t\t\ttest $status != A && test $ignore_config = all && continue\n>   \t\t\tfi\n> @@ -1184,7 +1166,7 @@ cmd_status()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>   \t\turl=$(git config submodule.\"$name\".url)\n>   \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n>   \t\tif test \"$stage\" = U\n> @@ -1261,7 +1243,7 @@ cmd_sync()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\")\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>   \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n>\n>   \t\t# Possibly a url relative to parent\n>\n"},{"id":"267534","messageId":"CAGZ79kY=jQSjJUxkznkwwupo527-nT05P_bKXy=GO=E4QjC8tQ@mail.gmail.com","threadId":"40015","inReplyTo":"55C25EBB.2000304@web.de","subject":"Re: [PATCH 2/4] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-05T19:55:30Z","receivedAt":"2015-08-05T19:55:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Aug 5, 2015 at 12:06 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:\n> Am 05.08.2015 um 02:04 schrieb Stefan Beller:\n>>\n>> The goal of this series being rewriting `git submodule update`,\n>> we don't want to call out to the shell script for config lookups.\n>>\n>> So reimplement the lookup of the submodule name in C.\n>\n>\n> Cool. This brings down the duration of the test suite from 3:13\n> to 3:12 for me (best of three).\n>\n> You might wanna have a look into submodule.c: after initially\n> calling gitmodules_config() one can lookup the submodule name\n> in the static \"config_name_for_path\" string_list. If you'd add\n> a public method to submodule.c which accesses that string_list\n> and returns the name for the given path, you won't need your\n> two new functions ... or am I missing something?\n\nYes I just realized there is already lots of submodule related code\nwritten in C, so I wanted to look at that as the next step and see what\ncan be reused and maybe redo the patches reusing code.\n"},{"id":"267541","messageId":"1438808880-9080-1-git-send-email-sbeller@google.com","threadId":"40015","inReplyTo":"CAGZ79kY=jQSjJUxkznkwwupo527-nT05P_bKXy=GO=E4QjC8tQ@mail.gmail.com","subject":"[PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-05T21:08:00Z","receivedAt":"2015-08-05T21:08:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This implements the helper `module_name` in C instead of shell,\nyielding a nice performance boost.\n\nBefore this patch, I measured a time (best out of three):\n\n  $ time ./t7400-submodule-basic.sh  >/dev/null\n    real\t0m11.066s\n    user\t0m3.348s\n    sys\t0m8.534s\n\nWith this patch applied I measured (also best out of three)\n\n  $ time ./t7400-submodule-basic.sh  >/dev/null\n    real\t0m10.063s\n    user\t0m3.044s\n    sys\t0m7.487s\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nIs this what you had in mind, Jens?\n\nJonathan pointed me to https://github.com/jlehmann/git-submod-enhancements/wiki\nDoes it reflect reality (i.e. as time passes code changes)?\n\nI also noticed that you have made quite some changes to submodules on different\nbranches which are not upstream. Soem changes look familiar (as in \"I believe\nthis is upstream alreaday?\" while others look new and exciting to me).\nI could not quite get the order yet, though.\n\n builtin/submodule--helper.c | 23 +++++++++++++++++++++++\n git-submodule.sh            | 32 +++++++-------------------------\n submodule.c                 | 18 +++++++++++++-----\n submodule.h                 |  1 +\n 4 files changed, 44 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex cb18ddf..3713c4c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -5,6 +5,8 @@\n #include \"pathspec.h\"\n #include \"dir.h\"\n #include \"utf8.h\"\n+#include \"submodule.h\"\n+#include \"string-list.h\"\n \n static char *ps_matched;\n static const struct cache_entry **ce_entries;\n@@ -98,6 +100,24 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_name(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *name;\n+\n+\tif (argc != 1)\n+\t\tusage(\"git submodule--helper module_name <path>\\n\");\n+\n+\tgitmodules_config();\n+\tname = submodule_name_for_path(argv[0]);\n+\n+\tif (name)\n+\t\tprintf(\"%s\\n\", name);\n+\telse\n+\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n+\n+\treturn 0;\n+}\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n \tif (argc < 2)\n@@ -106,6 +126,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"module_list\"))\n \t\treturn module_list(argc - 1, argv + 1, prefix);\n \n+\tif (!strcmp(argv[1], \"module_name\"))\n+\t\treturn module_name(argc - 2, argv + 2, prefix);\n+\n usage:\n \tusage(\"git submodule--helper module_list\\n\");\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex af9ecef..e6ff38d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -178,24 +178,6 @@ get_submodule_config () {\n \tprintf '%s' \"${value:-$default}\"\n }\n \n-\n-#\n-# Map submodule path to submodule name\n-#\n-# $1 = path\n-#\n-module_name()\n-{\n-\t# Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n-\tsm_path=\"$1\"\n-\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n-\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n-\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n-\ttest -z \"$name\" &&\n-\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n-\tprintf '%s\\n' \"$name\"\n-}\n-\n #\n # Clone a submodule\n #\n@@ -498,7 +480,7 @@ cmd_foreach()\n \t\tthen\n \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n-\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tclear_local_git_env\n@@ -554,7 +536,7 @@ cmd_init()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -636,7 +618,7 @@ cmd_deinit()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -758,7 +740,7 @@ cmd_update()\n \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n \t\t\tcontinue\n \t\tfi\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n@@ -1022,7 +1004,7 @@ cmd_summary() {\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n-\t\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n \t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n@@ -1184,7 +1166,7 @@ cmd_status()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \t\tif test \"$stage\" = U\n@@ -1261,7 +1243,7 @@ cmd_sync()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\")\n+\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \n \t\t# Possibly a url relative to parent\ndiff --git a/submodule.c b/submodule.c\nindex 700bbf4..dd8dcb0 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n+const char* submodule_name_for_path(const char* path)\n+{\n+\tstruct string_list_item *item;\n+\titem = unsorted_string_list_lookup(&config_name_for_path, path);\n+\tif (item)\n+\t\treturn item->util;\n+\telse\n+\t\treturn NULL;\n+}\n+\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t       const char *prefix, int command_line_option,\n \t\t\t       int quiet)\n@@ -693,7 +703,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n \tint i, result = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tstruct string_list_item *name_for_path;\n+\tconst char *name_for_path;\n \tconst char *work_tree = get_git_work_tree();\n \tif (!work_tree)\n \t\tgoto out;\n@@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n-\t\tname = ce->name;\n-\t\tname_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n-\t\tif (name_for_path)\n-\t\t\tname = name_for_path->util;\n+\t\tname_for_path = submodule_name_for_path(ce->name);\n+\t\tname =  name_for_path ? name_for_path : ce->name;\n \n \t\tdefault_argv = \"yes\";\n \t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\ndiff --git a/submodule.h b/submodule.h\nindex 7beec48..e3dd854 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+const char* submodule_name_for_path(const char* path);\n \n #endif\n-- \n2.5.0.236.g32a3769\n"},{"id":"267584","messageId":"55C3BA5C.3030404@web.de","threadId":"40015","inReplyTo":"1438808880-9080-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2015-08-06T19:49:48Z","receivedAt":"2015-08-06T19:49:48Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 05.08.2015 um 23:08 schrieb Stefan Beller:\n> This implements the helper `module_name` in C instead of shell,\n> yielding a nice performance boost.\n>\n> Before this patch, I measured a time (best out of three):\n>\n>    $ time ./t7400-submodule-basic.sh  >/dev/null\n>      real\t0m11.066s\n>      user\t0m3.348s\n>      sys\t0m8.534s\n>\n> With this patch applied I measured (also best out of three)\n>\n>    $ time ./t7400-submodule-basic.sh  >/dev/null\n>      real\t0m10.063s\n>      user\t0m3.044s\n>      sys\t0m7.487s\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> Is this what you had in mind, Jens?\n\nYup, thanks!\n\nJust some small nits, please see below (and also in the fixup! commit\nI pushed on the \"submodule--helper\" branch in my Github repo).\n\n> Jonathan pointed me to https://github.com/jlehmann/git-submod-enhancements/wiki\n> Does it reflect reality (i.e. as time passes code changes)?\n >\n> I also noticed that you have made quite some changes to submodules on different\n> branches which are not upstream. Soem changes look familiar (as in \"I believe\n> this is upstream alreaday?\" while others look new and exciting to me).\n> I could not quite get the order yet, though.\n\nI think the Wiki should be pretty much up to date, but I'll try to check\nthat and the state of the branches on the weekend to see if it needs an\nupdate. If you see some branches you believe are already upstream, it'd\nbe great if you could mention them so I can double check.\n\n>   builtin/submodule--helper.c | 23 +++++++++++++++++++++++\n>   git-submodule.sh            | 32 +++++++-------------------------\n>   submodule.c                 | 18 +++++++++++++-----\n>   submodule.h                 |  1 +\n>   4 files changed, 44 insertions(+), 30 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index cb18ddf..3713c4c 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -5,6 +5,8 @@\n>   #include \"pathspec.h\"\n>   #include \"dir.h\"\n>   #include \"utf8.h\"\n> +#include \"submodule.h\"\n> +#include \"string-list.h\"\n>\n>   static char *ps_matched;\n>   static const struct cache_entry **ce_entries;\n> @@ -98,6 +100,24 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>   \treturn 0;\n>   }\n>\n> +static int module_name(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *name;\n> +\n> +\tif (argc != 1)\n> +\t\tusage(\"git submodule--helper module_name <path>\\n\");\n> +\n> +\tgitmodules_config();\n> +\tname = submodule_name_for_path(argv[0]);\n> +\n> +\tif (name)\n> +\t\tprintf(\"%s\\n\", name);\n> +\telse\n> +\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n\nHmm, I prefer the pattern to bail out inside if() and continue with the\nexpected case without else:\n\n+\tif (!name)\n+\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n+\n+\tprintf(\"%s\\n\", name);\n\nBut maybe that's just me.\n\n> +\treturn 0;\n> +}\n> +\n>   int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>   {\n>   \tif (argc < 2)\n> @@ -106,6 +126,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>   \tif (!strcmp(argv[1], \"module_list\"))\n>   \t\treturn module_list(argc - 1, argv + 1, prefix);\n>\n> +\tif (!strcmp(argv[1], \"module_name\"))\n> +\t\treturn module_name(argc - 2, argv + 2, prefix);\n> +\n>   usage:\n>   \tusage(\"git submodule--helper module_list\\n\");\n>   }\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index af9ecef..e6ff38d 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -178,24 +178,6 @@ get_submodule_config () {\n>   \tprintf '%s' \"${value:-$default}\"\n>   }\n>\n> -\n> -#\n> -# Map submodule path to submodule name\n> -#\n> -# $1 = path\n> -#\n> -module_name()\n> -{\n> -\t# Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n> -\tsm_path=\"$1\"\n> -\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n> -\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n> -\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n> -\ttest -z \"$name\" &&\n> -\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n> -\tprintf '%s\\n' \"$name\"\n> -}\n> -\n>   #\n>   # Clone a submodule\n>   #\n> @@ -498,7 +480,7 @@ cmd_foreach()\n>   \t\tthen\n>   \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n>   \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n> -\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>   \t\t\t(\n>   \t\t\t\tprefix=\"$prefix$sm_path/\"\n>   \t\t\t\tclear_local_git_env\n> @@ -554,7 +536,7 @@ cmd_init()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>\n>   \t\tdisplaypath=$(relative_path \"$sm_path\")\n>\n> @@ -636,7 +618,7 @@ cmd_deinit()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>\n>   \t\tdisplaypath=$(relative_path \"$sm_path\")\n>\n> @@ -758,7 +740,7 @@ cmd_update()\n>   \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n>   \t\t\tcontinue\n>   \t\tfi\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>   \t\turl=$(git config submodule.\"$name\".url)\n>   \t\tbranch=$(get_submodule_config \"$name\" branch master)\n>   \t\tif ! test -z \"$update\"\n> @@ -1022,7 +1004,7 @@ cmd_summary() {\n>   \t\t\t# Respect the ignore setting for --for-status.\n>   \t\t\tif test -n \"$for_status\"\n>   \t\t\tthen\n> -\t\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>   \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n>   \t\t\t\ttest $status != A && test $ignore_config = all && continue\n>   \t\t\tfi\n> @@ -1184,7 +1166,7 @@ cmd_status()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>   \t\turl=$(git config submodule.\"$name\".url)\n>   \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n>   \t\tif test \"$stage\" = U\n> @@ -1261,7 +1243,7 @@ cmd_sync()\n>   \twhile read mode sha1 stage sm_path\n>   \tdo\n>   \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\")\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>   \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n>\n>   \t\t# Possibly a url relative to parent\n> diff --git a/submodule.c b/submodule.c\n> index 700bbf4..dd8dcb0 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)\n>   \tinitialized_fetch_ref_tips = 0;\n>   }\n>\n> +const char* submodule_name_for_path(const char* path)\n\nAsterisk belongs to the name, not the type:\n\n+const char *submodule_name_for_path(const char *path)\n\n> +{\n> +\tstruct string_list_item *item;\n> +\titem = unsorted_string_list_lookup(&config_name_for_path, path);\n> +\tif (item)\n> +\t\treturn item->util;\n> +\telse\n> +\t\treturn NULL;\n\nMaybe we could use the \"if() bailout;\" pattern here too:\n\n+\tif (!item)\n+\t\treturn NULL;\n+\n+\treturn item->util;\n\n(\"else\" after \"return\" looks strange ;-)\n\n> +}\n> +\n>   int fetch_populated_submodules(const struct argv_array *options,\n>   \t\t\t       const char *prefix, int command_line_option,\n>   \t\t\t       int quiet)\n> @@ -693,7 +703,7 @@ int fetch_populated_submodules(const struct argv_array *options,\n>   \tint i, result = 0;\n>   \tstruct child_process cp = CHILD_PROCESS_INIT;\n>   \tstruct argv_array argv = ARGV_ARRAY_INIT;\n> -\tstruct string_list_item *name_for_path;\n> +\tconst char *name_for_path;\n>   \tconst char *work_tree = get_git_work_tree();\n>   \tif (!work_tree)\n>   \t\tgoto out;\n> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,\n>   \t\tif (!S_ISGITLINK(ce->ce_mode))\n>   \t\t\tcontinue;\n>\n> -\t\tname = ce->name;\n> -\t\tname_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n> -\t\tif (name_for_path)\n> -\t\t\tname = name_for_path->util;\n> +\t\tname_for_path = submodule_name_for_path(ce->name);\n> +\t\tname =  name_for_path ? name_for_path : ce->name;\n\nI think we can get rid of name_for_path auto variable altogether here.\n(And while at it why not add the previously missing comment why we do\nfall back to the path here?):\n\n+\t\tname = submodule_name_for_path(ce->name);\n+\t\tif (!name)\n+\t\t\t/* Not in .gitmodules, try the default name == path */\n+\t\t\tname = ce->name;\n\n>   \t\tdefault_argv = \"yes\";\n>   \t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> diff --git a/submodule.h b/submodule.h\n> index 7beec48..e3dd854 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n>   \t\tstruct string_list *needs_pushing);\n>   int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n>   void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n> +const char* submodule_name_for_path(const char* path);\n\nAsterisk belongs to the names, not the type here too.\n\n+const char *submodule_name_for_path(const char *path);\n\n>\n>   #endif\n>\n"},{"id":"267590","messageId":"1438897123-16997-1-git-send-email-sbeller@google.com","threadId":"40015","inReplyTo":"55C3BA5C.3030404@web.de","subject":"[PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-06T21:38:43Z","receivedAt":"2015-08-06T21:38:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This implements the helper `module_name` in C instead of shell,\nyielding a nice performance boost.\n\nBefore this patch, I measured a time (best out of three):\n\n  $ time ./t7400-submodule-basic.sh  >/dev/null\n    real\t0m11.066s\n    user\t0m3.348s\n    sys\t0m8.534s\n\nWith this patch applied I measured (also best out of three)\n\n  $ time ./t7400-submodule-basic.sh  >/dev/null\n    real\t0m10.063s\n    user\t0m3.044s\n    sys\t0m7.487s\n\nHelped-by: Jens Lehmann <Jens.Lehmann@web.de>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis incorporates the changes from Jens fixup! commit\n(which addresses all issues he pointed out).\n\nI agree this looks much cleaner. :)\n\nThis patch advances origin/sb/submodule-helper (d2c6c09ac8,\nsubmodule: implement `module_list` as a builtin helper)\n\n builtin/submodule--helper.c | 22 ++++++++++++++++++++++\n git-submodule.sh            | 32 +++++++-------------------------\n submodule.c                 | 19 ++++++++++++++-----\n submodule.h                 |  1 +\n 4 files changed, 44 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex cb18ddf..bc37b74 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -5,6 +5,8 @@\n #include \"pathspec.h\"\n #include \"dir.h\"\n #include \"utf8.h\"\n+#include \"submodule.h\"\n+#include \"string-list.h\"\n \n static char *ps_matched;\n static const struct cache_entry **ce_entries;\n@@ -98,6 +100,23 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_name(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *name;\n+\n+\tif (argc != 1)\n+\t\tusage(\"git submodule--helper module_name <path>\\n\");\n+\n+\tgitmodules_config();\n+\tname = submodule_name_for_path(argv[0]);\n+\n+\tif (!name)\n+\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n+\n+\tprintf(\"%s\\n\", name);\n+\treturn 0;\n+}\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n \tif (argc < 2)\n@@ -106,6 +125,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"module_list\"))\n \t\treturn module_list(argc - 1, argv + 1, prefix);\n \n+\tif (!strcmp(argv[1], \"module_name\"))\n+\t\treturn module_name(argc - 2, argv + 2, prefix);\n+\n usage:\n \tusage(\"git submodule--helper module_list\\n\");\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex af9ecef..e6ff38d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -178,24 +178,6 @@ get_submodule_config () {\n \tprintf '%s' \"${value:-$default}\"\n }\n \n-\n-#\n-# Map submodule path to submodule name\n-#\n-# $1 = path\n-#\n-module_name()\n-{\n-\t# Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n-\tsm_path=\"$1\"\n-\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n-\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n-\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n-\ttest -z \"$name\" &&\n-\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n-\tprintf '%s\\n' \"$name\"\n-}\n-\n #\n # Clone a submodule\n #\n@@ -498,7 +480,7 @@ cmd_foreach()\n \t\tthen\n \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n-\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tclear_local_git_env\n@@ -554,7 +536,7 @@ cmd_init()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -636,7 +618,7 @@ cmd_deinit()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -758,7 +740,7 @@ cmd_update()\n \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n \t\t\tcontinue\n \t\tfi\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n@@ -1022,7 +1004,7 @@ cmd_summary() {\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n-\t\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n \t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n@@ -1184,7 +1166,7 @@ cmd_status()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \t\tif test \"$stage\" = U\n@@ -1261,7 +1243,7 @@ cmd_sync()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\")\n+\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \n \t\t# Possibly a url relative to parent\ndiff --git a/submodule.c b/submodule.c\nindex 15e90d1..78d7616 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)\n \tinitialized_fetch_ref_tips = 0;\n }\n \n+const char *submodule_name_for_path(const char *path)\n+{\n+\tstruct string_list_item *item;\n+\titem = unsorted_string_list_lookup(&config_name_for_path, path);\n+\tif (!item)\n+\t\treturn NULL;\n+\n+\treturn item->util;\n+}\n+\n int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t       const char *prefix, int command_line_option,\n \t\t\t       int quiet)\n@@ -693,7 +703,6 @@ int fetch_populated_submodules(const struct argv_array *options,\n \tint i, result = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tstruct string_list_item *name_for_path;\n \tconst char *work_tree = get_git_work_tree();\n \tif (!work_tree)\n \t\tgoto out;\n@@ -723,10 +732,10 @@ int fetch_populated_submodules(const struct argv_array *options,\n \t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\tcontinue;\n \n-\t\tname = ce->name;\n-\t\tname_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n-\t\tif (name_for_path)\n-\t\t\tname = name_for_path->util;\n+\t\tname = submodule_name_for_path(ce->name);\n+\t\tif (!name)\n+\t\t\t/* Not in .gitmodules, try the default name == path */\n+\t\t\tname = ce->name;\n \n \t\tdefault_argv = \"yes\";\n \t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\ndiff --git a/submodule.h b/submodule.h\nindex 7beec48..fc7f8a6 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+const char *submodule_name_for_path(const char *path);\n \n #endif\n-- \n2.5.0.239.g9728e1d.dirty\n"},{"id":"267626","messageId":"xmqqwpx6yixf.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"1438733070-15805-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/4] submodule: implement `module_list` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T19:53:16Z","receivedAt":"2015-08-07T19:53:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> new file mode 100644\n> index 0000000..cb18ddf\n> --- /dev/null\n> +++ b/builtin/submodule--helper.c\n> @@ -0,0 +1,111 @@\n> + ...\n> +static char *ps_matched;\n> +static const struct cache_entry **ce_entries;\n> +static int ce_alloc, ce_used;\n> +static struct pathspec pathspec;\n> +static const char *alternative_path;\n\nThese are OK for now to be global variables, but in the longer run,\nI think you would need to introduce a struct or two that group the\nrelevant pieces and passed around the callchain.  For example, a\ncaller calling into module_list_compute() would want to pass a\npointer to a struct that has ce_entries[] and ps_matched to receive\nthe result.  pathspec and alternative_path would want to be a\nfunction-scope auto variable in module_list, I would think.\n\n> +static void module_list_compute(int argc, const char **argv,\n> +\t\t\t\tconst char *prefix,\n> +\t\t\t\tstruct pathspec *pathspec)\n> +{\n> +\tint i;\n> +\tchar *max_prefix;\n> +\tint max_prefix_len;\n> +\tparse_pathspec(pathspec, 0,\n> +\t\t       PATHSPEC_PREFER_FULL |\n> +\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n> +\t\t       prefix, argv);\n> +\n> +\t/* Find common prefix for all pathspec's */\n> +\tmax_prefix = common_prefix(pathspec);\n> +\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> +\n> +\tif (pathspec->nr)\n> +\t\tps_matched = xcalloc(1, pathspec->nr);\n> +\n> +\n> +\tif (read_cache() < 0)\n> +\t\tdie(\"index file corrupt\");\n\nAgain, this is OK for now, but I suspect you would eventually want\nto return an error and have the caller react to it.\n\n> +static int module_list(int argc, const char **argv, const char *prefix)\n> +{\n> +...\n> +\tfor (i = 0; i < ce_used; i++) {\n> +\t\tconst struct cache_entry *ce = ce_entries[i];\n> +\n> +\t\tif (string_list_has_string(&already_printed, ce->name))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (ce_stage(ce)) {\n> +\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n> +\t\t} else {\n> +\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n> +\t\t}\n> +\n> +\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n> +\n> +\t\tstring_list_insert(&already_printed, ce->name);\n\nThis looks a wasteful use of string-list.\n\nWhen we iterate over the in-core index, or a subset obtained from\nthe in-core index without reordering, the standard technique to\nhandle entries for the same path only once is to handle one (while\nremembering what it is) and then skip the ones that follow with the\nsame name, with a loop like this:\n\n\ti = 0;\n        while (i < ce_used) {\n        \tce = ce_entries[i++];\n\t\tuse that ce;\n                while (i < ce_used && !strcmp(ce->ce_name, ce_entries[i]->ce_name))\n\t\t\ti++; /* skip entries with the same name */\n\t}\n\nTake advantage of the fact that the entries are still sorted.\n"},{"id":"267627","messageId":"xmqqsi7uyigm.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"1438897123-16997-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T20:03:21Z","receivedAt":"2015-08-07T20:03:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This incorporates the changes from Jens fixup! commit\n> (which addresses all issues he pointed out).\n>\n> I agree this looks much cleaner. :)\n\nThe only thing I found somewhat questionable is where to call\ngitmodules_config() from.  I think it is OK to do this at the\nbeginning of module_name(), at least for now, simply because the\nother function module_list() does not need to.\n\nWhen you rewrite sufficiently large parts of the scripted Porcelain\ninto C so that different pieces translated from the shell functions\ndirectly call into each other, that may have to change, though.  I\ndo not think gitmodules_config() is designed to be called more than\nonce, and I expect module_name() would be called many times inside a\nloop.\n\nOther than that, this is a trivial refactoring (i.e. a new helper\nfunction added to submodule.c can be used from an existing\nopen-coded logic in the same file, and then the same helper function\ngains a new callsite in submodule--helper.c) that makes things\neasier to read.\n\nThanks.\n\n>  builtin/submodule--helper.c | 22 ++++++++++++++++++++++\n>  git-submodule.sh            | 32 +++++++-------------------------\n>  submodule.c                 | 19 ++++++++++++++-----\n>  submodule.h                 |  1 +\n>  4 files changed, 44 insertions(+), 30 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index cb18ddf..bc37b74 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -5,6 +5,8 @@\n>  #include \"pathspec.h\"\n>  #include \"dir.h\"\n>  #include \"utf8.h\"\n> +#include \"submodule.h\"\n> +#include \"string-list.h\"\n>  \n>  static char *ps_matched;\n>  static const struct cache_entry **ce_entries;\n> @@ -98,6 +100,23 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>  \treturn 0;\n>  }\n>  \n> +static int module_name(int argc, const char **argv, const char *prefix)\n> +{\n> +\tconst char *name;\n> +\n> +\tif (argc != 1)\n> +\t\tusage(\"git submodule--helper module_name <path>\\n\");\n> +\n> +\tgitmodules_config();\n> +\tname = submodule_name_for_path(argv[0]);\n> +\n> +\tif (!name)\n> +\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n> +\n> +\tprintf(\"%s\\n\", name);\n> +\treturn 0;\n> +}\n> +\n>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>  {\n>  \tif (argc < 2)\n> @@ -106,6 +125,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>  \tif (!strcmp(argv[1], \"module_list\"))\n>  \t\treturn module_list(argc - 1, argv + 1, prefix);\n>  \n> +\tif (!strcmp(argv[1], \"module_name\"))\n> +\t\treturn module_name(argc - 2, argv + 2, prefix);\n> +\n>  usage:\n>  \tusage(\"git submodule--helper module_list\\n\");\n>  }\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index af9ecef..e6ff38d 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -178,24 +178,6 @@ get_submodule_config () {\n>  \tprintf '%s' \"${value:-$default}\"\n>  }\n>  \n> -\n> -#\n> -# Map submodule path to submodule name\n> -#\n> -# $1 = path\n> -#\n> -module_name()\n> -{\n> -\t# Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n> -\tsm_path=\"$1\"\n> -\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n> -\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n> -\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n> -\ttest -z \"$name\" &&\n> -\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n> -\tprintf '%s\\n' \"$name\"\n> -}\n> -\n>  #\n>  # Clone a submodule\n>  #\n> @@ -498,7 +480,7 @@ cmd_foreach()\n>  \t\tthen\n>  \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n>  \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n> -\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>  \t\t\t(\n>  \t\t\t\tprefix=\"$prefix$sm_path/\"\n>  \t\t\t\tclear_local_git_env\n> @@ -554,7 +536,7 @@ cmd_init()\n>  \twhile read mode sha1 stage sm_path\n>  \tdo\n>  \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>  \n>  \t\tdisplaypath=$(relative_path \"$sm_path\")\n>  \n> @@ -636,7 +618,7 @@ cmd_deinit()\n>  \twhile read mode sha1 stage sm_path\n>  \tdo\n>  \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>  \n>  \t\tdisplaypath=$(relative_path \"$sm_path\")\n>  \n> @@ -758,7 +740,7 @@ cmd_update()\n>  \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n>  \t\t\tcontinue\n>  \t\tfi\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>  \t\turl=$(git config submodule.\"$name\".url)\n>  \t\tbranch=$(get_submodule_config \"$name\" branch master)\n>  \t\tif ! test -z \"$update\"\n> @@ -1022,7 +1004,7 @@ cmd_summary() {\n>  \t\t\t# Respect the ignore setting for --for-status.\n>  \t\t\tif test -n \"$for_status\"\n>  \t\t\tthen\n> -\t\t\t\tname=$(module_name \"$sm_path\")\n> +\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>  \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n>  \t\t\t\ttest $status != A && test $ignore_config = all && continue\n>  \t\t\tfi\n> @@ -1184,7 +1166,7 @@ cmd_status()\n>  \twhile read mode sha1 stage sm_path\n>  \tdo\n>  \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\") || exit\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n>  \t\turl=$(git config submodule.\"$name\".url)\n>  \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n>  \t\tif test \"$stage\" = U\n> @@ -1261,7 +1243,7 @@ cmd_sync()\n>  \twhile read mode sha1 stage sm_path\n>  \tdo\n>  \t\tdie_if_unmatched \"$mode\"\n> -\t\tname=$(module_name \"$sm_path\")\n> +\t\tname=$(git submodule--helper module_name \"$sm_path\")\n>  \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n>  \n>  \t\t# Possibly a url relative to parent\n> diff --git a/submodule.c b/submodule.c\n> index 15e90d1..78d7616 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)\n>  \tinitialized_fetch_ref_tips = 0;\n>  }\n>  \n> +const char *submodule_name_for_path(const char *path)\n> +{\n> +\tstruct string_list_item *item;\n> +\titem = unsorted_string_list_lookup(&config_name_for_path, path);\n> +\tif (!item)\n> +\t\treturn NULL;\n> +\n> +\treturn item->util;\n> +}\n> +\n>  int fetch_populated_submodules(const struct argv_array *options,\n>  \t\t\t       const char *prefix, int command_line_option,\n>  \t\t\t       int quiet)\n> @@ -693,7 +703,6 @@ int fetch_populated_submodules(const struct argv_array *options,\n>  \tint i, result = 0;\n>  \tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \tstruct argv_array argv = ARGV_ARRAY_INIT;\n> -\tstruct string_list_item *name_for_path;\n>  \tconst char *work_tree = get_git_work_tree();\n>  \tif (!work_tree)\n>  \t\tgoto out;\n> @@ -723,10 +732,10 @@ int fetch_populated_submodules(const struct argv_array *options,\n>  \t\tif (!S_ISGITLINK(ce->ce_mode))\n>  \t\t\tcontinue;\n>  \n> -\t\tname = ce->name;\n> -\t\tname_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n> -\t\tif (name_for_path)\n> -\t\t\tname = name_for_path->util;\n> +\t\tname = submodule_name_for_path(ce->name);\n> +\t\tif (!name)\n> +\t\t\t/* Not in .gitmodules, try the default name == path */\n> +\t\t\tname = ce->name;\n>  \n>  \t\tdefault_argv = \"yes\";\n>  \t\tif (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n> diff --git a/submodule.h b/submodule.h\n> index 7beec48..fc7f8a6 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n>  \t\tstruct string_list *needs_pushing);\n>  int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n>  void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n> +const char *submodule_name_for_path(const char *path);\n>  \n>  #endif\n"},{"id":"267628","messageId":"xmqqlhdmyhtt.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"55C3BA5C.3030404@web.de","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T20:17:02Z","receivedAt":"2015-08-07T20:17:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\nThis change...\n\n>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,\n>>   \t\tif (!S_ISGITLINK(ce->ce_mode))\n>>   \t\t\tcontinue;\n>>\n>> -\t\tname = ce->name;\n>> -\t\tname_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n>> -\t\tif (name_for_path)\n>> -\t\t\tname = name_for_path->util;\n>> +\t\tname_for_path = submodule_name_for_path(ce->name);\n>> +\t\tname =  name_for_path ? name_for_path : ce->name;\n\n... interacts with Heiko's cached submodule config work that seems\nto have stalled.\n\nI can discard the stalled topic and queue this one instead, asking\nHeiko to reroll his on top once this has stabilized, or if Stefan is\nreally into revamping submodule now (which I hope is the case),\nperhaps Heiko's work can be rerolled by Stefan (with help from\nothers, of course) as a prerequisite and then these changes can be\nbuilt on top of it?\n"},{"id":"267631","messageId":"CAGZ79kYtCgYRHuMcxNoi6f9+GYYYCq6aRTdvx4ZKELSuQErkVQ@mail.gmail.com","threadId":"40015","inReplyTo":"xmqqlhdmyhtt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-07T20:49:30Z","receivedAt":"2015-08-07T20:49:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>\n> This change...\n>\n>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,\n>>>              if (!S_ISGITLINK(ce->ce_mode))\n>>>                      continue;\n>>>\n>>> -            name = ce->name;\n>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n>>> -            if (name_for_path)\n>>> -                    name = name_for_path->util;\n>>> +            name_for_path = submodule_name_for_path(ce->name);\n>>> +            name =  name_for_path ? name_for_path : ce->name;\n>\n> ... interacts with Heiko's cached submodule config work that seems\n> to have stalled.\n\nWe can drop that hunk as it only uses the new method\n`submodule_name_for_path` but doesn't change functionality.\nSo if you want to keep Heikos work, I'll just resend the patch\nwithout that hunk.\n\n>\n> I can discard the stalled topic and queue this one instead, asking\n> Heiko to reroll his on top once this has stabilized, or if Stefan is\n> really into revamping submodule now (which I hope is the case),\n> perhaps Heiko's work can be rerolled by Stefan (with help from\n> others, of course) as a prerequisite and then these changes can be\n> built on top of it?\n\nI am a bit overwhelmed as I am into git submodule for a few days now\nand still have not a full&good understanding of the details.\nSo for now my plan looks like this:\n\n1) rewrite the helpers in C (module_list, module_name and module_clone)\n   These helper functions are useful for themselves as they speed up\n   git submodule operations, but I have them on the plan as they are a\n   pre requisite for rewriting `git submodule update`\n\n2) Come up with a good thread pool abstraction\n   (Started as \"[RFC/PATCH 0/4] parallel fetch for submodules\" )\n   This abstraction (if done right) will allow us to use it in different places\n   easily. I started it as part of \"git fetch --recurse-submodules\" because\n   it is submodule related and reasonably sized\n\n3) Rewrite `git submodule update` in C\n  This will start out as a literal translation and once 2 is coming\nalong nicely,\n  I want to add that thread pool capability to the C rewrite.\n\nThis plan still is huge to me, but a lot smaller than my initial\n\"rewrite the whole\ngit-submodule.sh in C\".\n\nOnce the rewrite and parallelism is done, I may want to start adding new\nfeatures to the submodules, such as different \"classes\". Currently I think\neach submodule would have a set of these classes or features, such that\nyou could clone a superproject with hints on the classes:\n\n  git clone <url-to-super-project> --submodule-flags=basis,amiga-specific,ui\n\neach of the flags would then include a specifc set of submodules, such\nthat after cloning the super project you have everything you need to build\nthe program with ui on amiga.\n\n\n\n\n\n\n>\n"},{"id":"267632","messageId":"CAGZ79kZEXuP_vZpO33X3BxKrNeRU5SjoU9dnk__ZzQR+rn7LUw@mail.gmail.com","threadId":"40015","inReplyTo":"xmqqsi7uyigm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-07T20:54:00Z","receivedAt":"2015-08-07T20:54:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 1:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This incorporates the changes from Jens fixup! commit\n>> (which addresses all issues he pointed out).\n>>\n>> I agree this looks much cleaner. :)\n>\n> The only thing I found somewhat questionable is where to call\n> gitmodules_config() from.  I think it is OK to do this at the\n> beginning of module_name(), at least for now, simply because the\n> other function module_list() does not need to.\n>\n> When you rewrite sufficiently large parts of the scripted Porcelain\n> into C so that different pieces translated from the shell functions\n> directly call into each other, that may have to change, though.  I\n> do not think gitmodules_config() is designed to be called more than\n> once, and I expect module_name() would be called many times inside a\n> loop.\n\nI want to structure the each part rewritten as a reusable core part\nand a wrapper which just sets up the environment (option parsing,\nreading the index, configs).\n\nIn this patch you only see the latter part, the wrapper, because the\ncore part only consists of one line\n\n    name = submodule_name_for_path(<input>);\n\nso I did not want to wrap that into its own function. But when rewriting\npiece of shell code, which originally called into module_name, I'd rather\nuse the one liner instead. (This doesn't quite follow the literal translation\nstrategy, but I think it may be appropriate here).\n\n>\n> Other than that, this is a trivial refactoring (i.e. a new helper\n> function added to submodule.c can be used from an existing\n> open-coded logic in the same file, and then the same helper function\n> gains a new callsite in submodule--helper.c) that makes things\n> easier to read.\n>\n> Thanks.\n>\n>>  builtin/submodule--helper.c | 22 ++++++++++++++++++++++\n>>  git-submodule.sh            | 32 +++++++-------------------------\n>>  submodule.c                 | 19 ++++++++++++++-----\n>>  submodule.h                 |  1 +\n>>  4 files changed, 44 insertions(+), 30 deletions(-)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index cb18ddf..bc37b74 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -5,6 +5,8 @@\n>>  #include \"pathspec.h\"\n>>  #include \"dir.h\"\n>>  #include \"utf8.h\"\n>> +#include \"submodule.h\"\n>> +#include \"string-list.h\"\n>>\n>>  static char *ps_matched;\n>>  static const struct cache_entry **ce_entries;\n>> @@ -98,6 +100,23 @@ static int module_list(int argc, const char **argv, const char *prefix)\n>>       return 0;\n>>  }\n>>\n>> +static int module_name(int argc, const char **argv, const char *prefix)\n>> +{\n>> +     const char *name;\n>> +\n>> +     if (argc != 1)\n>> +             usage(\"git submodule--helper module_name <path>\\n\");\n>> +\n>> +     gitmodules_config();\n>> +     name = submodule_name_for_path(argv[0]);\n>> +\n>> +     if (!name)\n>> +             die(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n>> +\n>> +     printf(\"%s\\n\", name);\n>> +     return 0;\n>> +}\n>> +\n>>  int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>>  {\n>>       if (argc < 2)\n>> @@ -106,6 +125,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>>       if (!strcmp(argv[1], \"module_list\"))\n>>               return module_list(argc - 1, argv + 1, prefix);\n>>\n>> +     if (!strcmp(argv[1], \"module_name\"))\n>> +             return module_name(argc - 2, argv + 2, prefix);\n>> +\n>>  usage:\n>>       usage(\"git submodule--helper module_list\\n\");\n>>  }\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index af9ecef..e6ff38d 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -178,24 +178,6 @@ get_submodule_config () {\n>>       printf '%s' \"${value:-$default}\"\n>>  }\n>>\n>> -\n>> -#\n>> -# Map submodule path to submodule name\n>> -#\n>> -# $1 = path\n>> -#\n>> -module_name()\n>> -{\n>> -     # Do we have \"submodule.<something>.path = $1\" defined in .gitmodules file?\n>> -     sm_path=\"$1\"\n>> -     re=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n>> -     name=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n>> -             sed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n>> -     test -z \"$name\" &&\n>> -     die \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n>> -     printf '%s\\n' \"$name\"\n>> -}\n>> -\n>>  #\n>>  # Clone a submodule\n>>  #\n>> @@ -498,7 +480,7 @@ cmd_foreach()\n>>               then\n>>                       displaypath=$(relative_path \"$sm_path\")\n>>                       say \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n>> -                     name=$(module_name \"$sm_path\")\n>> +                     name=$(git submodule--helper module_name \"$sm_path\")\n>>                       (\n>>                               prefix=\"$prefix$sm_path/\"\n>>                               clear_local_git_env\n>> @@ -554,7 +536,7 @@ cmd_init()\n>>       while read mode sha1 stage sm_path\n>>       do\n>>               die_if_unmatched \"$mode\"\n>> -             name=$(module_name \"$sm_path\") || exit\n>> +             name=$(git submodule--helper module_name \"$sm_path\") || exit\n>>\n>>               displaypath=$(relative_path \"$sm_path\")\n>>\n>> @@ -636,7 +618,7 @@ cmd_deinit()\n>>       while read mode sha1 stage sm_path\n>>       do\n>>               die_if_unmatched \"$mode\"\n>> -             name=$(module_name \"$sm_path\") || exit\n>> +             name=$(git submodule--helper module_name \"$sm_path\") || exit\n>>\n>>               displaypath=$(relative_path \"$sm_path\")\n>>\n>> @@ -758,7 +740,7 @@ cmd_update()\n>>                       echo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n>>                       continue\n>>               fi\n>> -             name=$(module_name \"$sm_path\") || exit\n>> +             name=$(git submodule--helper module_name \"$sm_path\") || exit\n>>               url=$(git config submodule.\"$name\".url)\n>>               branch=$(get_submodule_config \"$name\" branch master)\n>>               if ! test -z \"$update\"\n>> @@ -1022,7 +1004,7 @@ cmd_summary() {\n>>                       # Respect the ignore setting for --for-status.\n>>                       if test -n \"$for_status\"\n>>                       then\n>> -                             name=$(module_name \"$sm_path\")\n>> +                             name=$(git submodule--helper module_name \"$sm_path\")\n>>                               ignore_config=$(get_submodule_config \"$name\" ignore none)\n>>                               test $status != A && test $ignore_config = all && continue\n>>                       fi\n>> @@ -1184,7 +1166,7 @@ cmd_status()\n>>       while read mode sha1 stage sm_path\n>>       do\n>>               die_if_unmatched \"$mode\"\n>> -             name=$(module_name \"$sm_path\") || exit\n>> +             name=$(git submodule--helper module_name \"$sm_path\") || exit\n>>               url=$(git config submodule.\"$name\".url)\n>>               displaypath=$(relative_path \"$prefix$sm_path\")\n>>               if test \"$stage\" = U\n>> @@ -1261,7 +1243,7 @@ cmd_sync()\n>>       while read mode sha1 stage sm_path\n>>       do\n>>               die_if_unmatched \"$mode\"\n>> -             name=$(module_name \"$sm_path\")\n>> +             name=$(git submodule--helper module_name \"$sm_path\")\n>>               url=$(git config -f .gitmodules --get submodule.\"$name\".url)\n>>\n>>               # Possibly a url relative to parent\n>> diff --git a/submodule.c b/submodule.c\n>> index 15e90d1..78d7616 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -686,6 +686,16 @@ static void calculate_changed_submodule_paths(void)\n>>       initialized_fetch_ref_tips = 0;\n>>  }\n>>\n>> +const char *submodule_name_for_path(const char *path)\n>> +{\n>> +     struct string_list_item *item;\n>> +     item = unsorted_string_list_lookup(&config_name_for_path, path);\n>> +     if (!item)\n>> +             return NULL;\n>> +\n>> +     return item->util;\n>> +}\n>> +\n>>  int fetch_populated_submodules(const struct argv_array *options,\n>>                              const char *prefix, int command_line_option,\n>>                              int quiet)\n>> @@ -693,7 +703,6 @@ int fetch_populated_submodules(const struct argv_array *options,\n>>       int i, result = 0;\n>>       struct child_process cp = CHILD_PROCESS_INIT;\n>>       struct argv_array argv = ARGV_ARRAY_INIT;\n>> -     struct string_list_item *name_for_path;\n>>       const char *work_tree = get_git_work_tree();\n>>       if (!work_tree)\n>>               goto out;\n>> @@ -723,10 +732,10 @@ int fetch_populated_submodules(const struct argv_array *options,\n>>               if (!S_ISGITLINK(ce->ce_mode))\n>>                       continue;\n>>\n>> -             name = ce->name;\n>> -             name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n>> -             if (name_for_path)\n>> -                     name = name_for_path->util;\n>> +             name = submodule_name_for_path(ce->name);\n>> +             if (!name)\n>> +                     /* Not in .gitmodules, try the default name == path */\n>> +                     name = ce->name;\n>>\n>>               default_argv = \"yes\";\n>>               if (command_line_option == RECURSE_SUBMODULES_DEFAULT) {\n>> diff --git a/submodule.h b/submodule.h\n>> index 7beec48..fc7f8a6 100644\n>> --- a/submodule.h\n>> +++ b/submodule.h\n>> @@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam\n>>               struct string_list *needs_pushing);\n>>  int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n>>  void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n>> +const char *submodule_name_for_path(const char *path);\n>>\n>>  #endif\n"},{"id":"267634","messageId":"xmqqa8u2yf6e.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"CAGZ79kYtCgYRHuMcxNoi6f9+GYYYCq6aRTdvx4ZKELSuQErkVQ@mail.gmail.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T21:14:17Z","receivedAt":"2015-08-07T21:14:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Aug 7, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>\n>> This change...\n>>\n>>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,\n>>>>              if (!S_ISGITLINK(ce->ce_mode))\n>>>>                      continue;\n>>>>\n>>>> -            name = ce->name;\n>>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n>>>> -            if (name_for_path)\n>>>> -                    name = name_for_path->util;\n>>>> +            name_for_path = submodule_name_for_path(ce->name);\n>>>> +            name =  name_for_path ? name_for_path : ce->name;\n>>\n>> ... interacts with Heiko's cached submodule config work that seems\n>> to have stalled.\n>\n> We can drop that hunk as it only uses the new method\n> `submodule_name_for_path` but doesn't change functionality.\n> So if you want to keep Heikos work, I'll just resend the patch\n> without that hunk.\n\nDoes such a result even make sense?  Note that I wasn't talking\nabout textual conflict.\n\nIf we followed what you just said, that patch will try to directly\nread the data in config_name_for_path string list, which is removed\nby Heiko's series, if I am reading it right.\n\nIn the new world order with Heiko's series, the way you grab\nsubmodule configuration data is to call submodule_from_path() or\nsubmodule_from_name() and they allow you to read from .gitmodules\neither in a commit (for historical state), the index, or from the\nworking tree.  Which should be much cleaner and goes in the right\ndirection in the longer term.\n\nAnd the best part of the story is that your module_name would be\njust calling submodule_from_path() and peeking into a field.\n\n> 2) Come up with a good thread pool abstraction\n>    (Started as \"[RFC/PATCH 0/4] parallel fetch for submodules\" )\n>    This abstraction (if done right) will allow us to use it in different places\n>    easily. I started it as part of \"git fetch --recurse-submodules\" because\n>    it is submodule related and reasonably sized\n\nI personally think this gives the most bang-for-buck.  Write that\nand expose it as \"git submodule for-each-parallel\", which takes the\nshell scriptlet that currently is the loop body of \"while read mode\nsha1 stage sm_path\" in update and clone.  You will have immediate\nand large payback.\n\nAnd you do not even need module_list and module_name written in C in\norder to do so.  Not that these two are not trivial to implement, but\nthe payoff from them is not as large as from for-each-parallel ;-)\n"},{"id":"267635","messageId":"CAGZ79kYjaXtGurWgPk47FauLhC=k-gBjLYhepuz4gJE6Rm_8DA@mail.gmail.com","threadId":"40015","inReplyTo":"xmqqa8u2yf6e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-07T21:21:52Z","receivedAt":"2015-08-07T21:21:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Fri, Aug 7, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>>\n>>> This change...\n>>>\n>>>>> @@ -723,10 +733,8 @@ int fetch_populated_submodules(const struct argv_array *options,\n>>>>>              if (!S_ISGITLINK(ce->ce_mode))\n>>>>>                      continue;\n>>>>>\n>>>>> -            name = ce->name;\n>>>>> -            name_for_path = unsorted_string_list_lookup(&config_name_for_path, ce->name);\n>>>>> -            if (name_for_path)\n>>>>> -                    name = name_for_path->util;\n>>>>> +            name_for_path = submodule_name_for_path(ce->name);\n>>>>> +            name =  name_for_path ? name_for_path : ce->name;\n>>>\n>>> ... interacts with Heiko's cached submodule config work that seems\n>>> to have stalled.\n>>\n>> We can drop that hunk as it only uses the new method\n>> `submodule_name_for_path` but doesn't change functionality.\n>> So if you want to keep Heikos work, I'll just resend the patch\n>> without that hunk.\n>\n> Does such a result even make sense?  Note that I wasn't talking\n> about textual conflict.\n>\n> If we followed what you just said, that patch will try to directly\n> read the data in config_name_for_path string list, which is removed\n> by Heiko's series, if I am reading it right.\n>\n> In the new world order with Heiko's series, the way you grab\n> submodule configuration data is to call submodule_from_path() or\n> submodule_from_name() and they allow you to read from .gitmodules\n> either in a commit (for historical state), the index, or from the\n> working tree.  Which should be much cleaner and goes in the right\n> direction in the longer term.\n>\n> And the best part of the story is that your module_name would be\n> just calling submodule_from_path() and peeking into a field.\n>\n>> 2) Come up with a good thread pool abstraction\n>>    (Started as \"[RFC/PATCH 0/4] parallel fetch for submodules\" )\n>>    This abstraction (if done right) will allow us to use it in different places\n>>    easily. I started it as part of \"git fetch --recurse-submodules\" because\n>>    it is submodule related and reasonably sized\n>\n> I personally think this gives the most bang-for-buck.  Write that\n> and expose it as \"git submodule for-each-parallel\", which takes the\n> shell scriptlet that currently is the loop body of \"while read mode\n> sha1 stage sm_path\" in update and clone.  You will have immediate\n> and large payback.\n\nYou said that before. I feel like this is a bit to narrow. A \"git submodule\nfor-each-parallel\" would be a very specific tool which we would use to\nmake the different submodule operations parallel with ease. But it would\nbe very submodule specifc I guess?\n\nThat's why I want to be a bit more generic and have this thread pool API\ndone in C, such that \"any for loop\" in git can be easily replaced by using\nthe thread pool. I think of \"git fetch --all\" specially.\n\n>\n> And you do not even need module_list and module_name written in C in\n> order to do so.  Not that these two are not trivial to implement, but\n> the payoff from them is not as large as from for-each-parallel ;-)\n>\n\nI think I can do this for-each-parallel once I have the more generic thread\npooling done. All that is left now is a good handling of stdout/stderr, which\nI am not yet convinced how to do it right. Maybe each task accumulates\nmessages in two string buffers and then the thread pool will output the\nstring buffer one a task is done.\n"},{"id":"267636","messageId":"xmqq614qyebl.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"CAGZ79kYjaXtGurWgPk47FauLhC=k-gBjLYhepuz4gJE6Rm_8DA@mail.gmail.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T21:32:46Z","receivedAt":"2015-08-07T21:32:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Aug 7, 2015 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>> ...\n>>> We can drop that hunk as it only uses the new method\n>>> `submodule_name_for_path` but doesn't change functionality.\n>>> So if you want to keep Heikos work, I'll just resend the patch\n>>> without that hunk.\n>>\n>> Does such a result even make sense?  Note that I wasn't talking\n>> about textual conflict.\n>>\n>> If we followed what you just said, that patch will try to directly\n>> read the data in config_name_for_path string list, which is removed\n>> by Heiko's series, if I am reading it right.\n\nBy the way, the above is more important part of the message you are\nresponding to.  The result does not simply link, because your\nunsorted_string_list_lookup() will no longer have the string list in\nthe first place X-<.\n\n>>> 2) Come up with a good thread pool abstraction\n>>>    (Started as \"[RFC/PATCH 0/4] parallel fetch for submodules\" )\n>>>    This abstraction (if done right) will allow us to use it in different places\n>>>    easily. I started it as part of \"git fetch --recurse-submodules\" because\n>>>    it is submodule related and reasonably sized\n>>\n>> I personally think this gives the most bang-for-buck.  Write that\n>> and expose it as \"git submodule for-each-parallel\", which takes the\n>> shell scriptlet that currently is the loop body of \"while read mode\n>> sha1 stage sm_path\" in update and clone.  You will have immediate\n>> and large payback.\n>\n> You said that before. I feel like this is a bit to narrow.\n\nThat depends on how good a design you make for the internal\n\"parallel execution\" engine.  If it is made to take an arbitrary C\nfunction with list of arguments to call it with, for-each-parallel\nwould be just a degenerate and narrow case where that arbitrary C\nfunction happens to be exec'ing a shell and feed a shell scriptlet\nto it.\n\nThe internal parallel execution engine would be reusable without any\nchange to call C native functions, allowing you to do everything\ninside the process in the future.  And the \"narrow\" case is a good\nfirst step to validate your design actually _works_.\n"},{"id":"267638","messageId":"CAGZ79kZx=ML-tT6vvjynZ91x6JYqpgKVaEp-Q43MShz0J2EanQ@mail.gmail.com","threadId":"40015","inReplyTo":"xmqq614qyebl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-07T22:04:40Z","receivedAt":"2015-08-07T22:04:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Fri, Aug 7, 2015 at 2:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>> ...\n>>>> We can drop that hunk as it only uses the new method\n>>>> `submodule_name_for_path` but doesn't change functionality.\n>>>> So if you want to keep Heikos work, I'll just resend the patch\n>>>> without that hunk.\n>>>\n>>> Does such a result even make sense?  Note that I wasn't talking\n>>> about textual conflict.\n>>>\n>>> If we followed what you just said, that patch will try to directly\n>>> read the data in config_name_for_path string list, which is removed\n>>> by Heiko's series, if I am reading it right.\n>\n> By the way, the above is more important part of the message you are\n> responding to.  The result does not simply link, because your\n> unsorted_string_list_lookup() will no longer have the string list in\n> the first place X-<.\n\nI looked through Heikos series and think it is an improvement. I mean I\ncan redo my patches on top of his. Specially this patch will be easy,\nas patch 2/4 (extract functions for submodule config set and lookup)\nimplements get_name_for_path. All I would need to do then is expose it\nto the shell via the helper.\n\n>\n>>>> 2) Come up with a good thread pool abstraction\n>>>>    (Started as \"[RFC/PATCH 0/4] parallel fetch for submodules\" )\n>>>>    This abstraction (if done right) will allow us to use it in different places\n>>>>    easily. I started it as part of \"git fetch --recurse-submodules\" because\n>>>>    it is submodule related and reasonably sized\n>>>\n>>> I personally think this gives the most bang-for-buck.  Write that\n>>> and expose it as \"git submodule for-each-parallel\", which takes the\n>>> shell scriptlet that currently is the loop body of \"while read mode\n>>> sha1 stage sm_path\" in update and clone.  You will have immediate\n>>> and large payback.\n>>\n>> You said that before. I feel like this is a bit to narrow.\n>\n> That depends on how good a design you make for the internal\n> \"parallel execution\" engine.  If it is made to take an arbitrary C\n> function with list of arguments to call it with, for-each-parallel\n> would be just a degenerate and narrow case where that arbitrary C\n> function happens to be exec'ing a shell and feed a shell scriptlet\n> to it.\n>\n> The internal parallel execution engine would be reusable without any\n> change to call C native functions, allowing you to do everything\n> inside the process in the future.  And the \"narrow\" case is a good\n> first step to validate your design actually _works_.\n>\n"},{"id":"267639","messageId":"xmqq1tfeyc6p.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"CAGZ79kZx=ML-tT6vvjynZ91x6JYqpgKVaEp-Q43MShz0J2EanQ@mail.gmail.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T22:18:54Z","receivedAt":"2015-08-07T22:18:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Aug 7, 2015 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>>>> If we followed what you just said, that patch will try to directly\n>>>> read the data in config_name_for_path string list, which is removed\n>>>> by Heiko's series, if I am reading it right.\n>>\n>> By the way, the above is more important part of the message you are\n>> responding to.  The result does not simply link, because your\n>> unsorted_string_list_lookup() will no longer have the string list in\n>> the first place X-<.\n>\n> I looked through Heikos series and think it is an improvement. I mean I\n> can redo my patches on top of his. Specially this patch will be easy,\n> as patch 2/4 (extract functions for submodule config set and lookup)\n> implements get_name_for_path. All I would need to do then is expose it\n> to the shell via the helper.\n\nYes, that is exactly what I said a few messages ago, wasn't it?\n\nBut that would require that you read and understand Heiko's work and\nthat you understand what its future direction should be, communicate\nthat vision to others to share, before building on top of it.  And\nwith that effort, you would already be in a good position to polish\nHeiko's stalled work and move it forward.  After all, you cannot\njust build on a stalled work, declare \"my part is done; the result\nis not mergeable because the foundation is not cooked, but that is\nnot my problem\" ;-).\n\nIIRC, the issues around the topic were nothing show-stopping, but\nthe primary reason it stalled was that Heiko got busy with his other\nobligations, so I'd appreciate others like you to help the topic\nmove forward.\n\nThanks.\n"},{"id":"267641","messageId":"xmqqpp2ywwj1.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"CAGZ79kYjaXtGurWgPk47FauLhC=k-gBjLYhepuz4gJE6Rm_8DA@mail.gmail.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-07T22:42:26Z","receivedAt":"2015-08-07T22:42:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> That's why I want to be a bit more generic and have this thread pool API\n> done in C, such that \"any for loop\" in git can be easily replaced by using\n> the thread pool. I think of \"git fetch --all\" specially.\n\nOne more thing, as I didn't notice that you kept repeating \"thread\"\npool API.\n\nWhile I doubt that you would gain much by using threads in place of\nprocesses to perform parallel \"submodule update\", \"submodule clone\",\n\"fetch all\", etc., all of which are fairly well isolated and heavy\nweight operations themselves, and I suspect that the implementation\nsimplicity of using separate processes would probably be huge plus\ncompared to any possible upside you may gain from using threads, if\nyou really want to go the \"thread\" route, the first thing to try\nwould be to see if a few places we already use threads for\nparallelism (namely, \"grep\", \"pack-objects\", \"preload-index\" and\n\"index-pack\") can be factored out and model your new API around the\ncommonality among them.\n"},{"id":"267643","messageId":"CAGZ79ka8nr54P3100roRt8aGScx2a=xQM=c2P3aKkntMc+6ihQ@mail.gmail.com","threadId":"40015","inReplyTo":"xmqq1tfeyc6p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-07T23:12:02Z","receivedAt":"2015-08-07T23:12:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 3:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Fri, Aug 7, 2015 at 2:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>>>> If we followed what you just said, that patch will try to directly\n>>>>> read the data in config_name_for_path string list, which is removed\n>>>>> by Heiko's series, if I am reading it right.\n>>>\n>>> By the way, the above is more important part of the message you are\n>>> responding to.  The result does not simply link, because your\n>>> unsorted_string_list_lookup() will no longer have the string list in\n>>> the first place X-<.\n>>\n>> I looked through Heikos series and think it is an improvement. I mean I\n>> can redo my patches on top of his. Specially this patch will be easy,\n>> as patch 2/4 (extract functions for submodule config set and lookup)\n>> implements get_name_for_path. All I would need to do then is expose it\n>> to the shell via the helper.\n>\n> Yes, that is exactly what I said a few messages ago, wasn't it?\n\nYes, you did. I needed to repeat it to actually mentally process it.\n\n>\n> But that would require that you read and understand Heiko's work and\n> that you understand what its future direction should be, communicate\n> that vision to others to share, before building on top of it.  And\n> with that effort, you would already be in a good position to polish\n> Heiko's stalled work and move it forward.  After all, you cannot\n> just build on a stalled work, declare \"my part is done; the result\n> is not mergeable because the foundation is not cooked, but that is\n> not my problem\" ;-).\n\nThat would be an easy world to live in, but I agree that I should get the\nfundamentals right.\n\n>\n> IIRC, the issues around the topic were nothing show-stopping, but\n> the primary reason it stalled was that Heiko got busy with his other\n> obligations, so I'd appreciate others like you to help the topic\n> move forward.\n\nOk, I'll dig up the old reviews and review it myself before continuing\non my patches.\n\n>\n> Thanks.\n>\n"},{"id":"267644","messageId":"CAGZ79kaGe59=z4Eeuno8DRuWWRD12OyLETi7NA+utfoLsJ4+=A@mail.gmail.com","threadId":"40015","inReplyTo":"xmqqpp2ywwj1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-07T23:19:25Z","receivedAt":"2015-08-07T23:19:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> That's why I want to be a bit more generic and have this thread pool API\n>> done in C, such that \"any for loop\" in git can be easily replaced by using\n>> the thread pool. I think of \"git fetch --all\" specially.\n>\n> One more thing, as I didn't notice that you kept repeating \"thread\"\n> pool API.\n\nYeah I intended to use both threads and processes for the heavy submodule\noperations.\n\nEach thread in the thread pool would setup the right argument lists\nand environment\nand then spawn a process for heavy weight operations, wait for the process,\nmaybe process its output.\n\nMaybe I should omit the whole thread pool and only use select from a single\nthreaded main program.\n\n>\n> While I doubt that you would gain much by using threads in place of\n> processes to perform parallel \"submodule update\", \"submodule clone\",\n> \"fetch all\", etc., all of which are fairly well isolated and heavy\n> weight operations themselves, and I suspect that the implementation\n> simplicity of using separate processes would probably be huge plus\n> compared to any possible upside you may gain from using threads, if\n> you really want to go the \"thread\" route, the first thing to try\n> would be to see if a few places we already use threads for\n> parallelism (namely, \"grep\", \"pack-objects\", \"preload-index\" and\n> \"index-pack\") can be factored out and model your new API around the\n> commonality among them.\n"},{"id":"267645","messageId":"xmqqlhdmwrxx.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"CAGZ79kaGe59=z4Eeuno8DRuWWRD12OyLETi7NA+utfoLsJ4+=A@mail.gmail.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-08T00:21:30Z","receivedAt":"2015-08-08T00:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> One more thing, as I didn't notice that you kept repeating \"thread\"\n>> pool API.\n>\n> Yeah I intended to use both threads and processes for the heavy submodule\n> operations.\n\nOK.  I somehow had an impression that it might be more tricky than\nit is worth to spawn/run_command out of a thread/run_async, but if\nit makes it easier and more generic to correctly arrange the\nthread-pool API to allow the per-thread functions to run_command(),\nI wouldn't object to that approach at all.\n\nThen for-each-parallel would truly become a trivial application of\nthat API.  Your per-thread function happens to be a one that\nprepares appropriate \"struct child_process\" and calls run_command()\nto interact with that single child, receiving its output and culling\nit when it is done.\n\n>> ... if\n>> you really want to go the \"thread\" route, the first thing to try\n>> would be to see if a few places we already use threads for\n>> parallelism (namely, \"grep\", \"pack-objects\", \"preload-index\" and\n>> \"index-pack\") can be factored out and model your new API around the\n>> commonality among them.\n\nAnd obviously, doing your pool API around threads will allow you to\nthrow future per-thread function that do not involve run_command()\nat all at your API, and it will make it easy to adapt the current\nthreaded parts of the system to the API.\n\nPerfect.\n\n;-)\n"},{"id":"267649","messageId":"xmqq4mkawbc8.fsf@gitster.dls.corp.google.com","threadId":"40015","inReplyTo":"xmqqlhdmwrxx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-08T06:20:07Z","receivedAt":"2015-08-08T06:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>> ... if\n>>> you really want to go the \"thread\" route, the first thing to try\n>>> would be to see if a few places we already use threads for\n>>> parallelism (namely, \"grep\", \"pack-objects\", \"preload-index\" and\n>>> \"index-pack\") can be factored out and model your new API around the\n>>> commonality among them.\n>\n> And obviously, doing your pool API around threads will allow you to\n> throw future per-thread function that do not involve run_command()\n> at all at your API, and it will make it easy to adapt the current\n> threaded parts of the system to the API.\n\nJust a few random thoughts before going to bed and going offline for\nthe weekend...\n\nEventually, we would want to do \"submodule update\" of a top-level\nproject that has 500 submodules underneath, but obviously we would\nnot want to blindly spawn 500 threads, each of which runs \"fetch\",\nall at the same time.  We'd want to limit the parallelism to a sane\nlimit (say, 16 or 32), stuff 500 work units to a queue, from which\nthat many number of worker bees grab work unit one by one to process\nand then come back to ask for more work.\n\nAnd we would eventually want to be able to do this even when these\n500 submodules are spread across multiple levels of nested\nsubmodules (e.g. top-level may have 8 submodules, and they have 16\nnested subsubmodules each on average, each of which may have 4\nnested subsubsubmodules on average).  Specifying -j16 at the top\nlevel and apportioning the parallelism to recursive invoation of\n\"submodule update\" in such a way that the overall process is\nefficient and without waste would be a bit tricky.\n\nIn such a nested submodule case, we may want to instead try to\nenumerate these 500 submodules upfront with unbounded parallelism\n(e.g. the top-level will ask 4 worker bees to process immediate 8\nsubmodules, and they each spawn 4 worker bees to process their\nimmediate 16 submodules, and so on---it is unbounded because we do\nnot know upfront how deep the nesting is).\n\nLet's call that a recursive module_list.  You would want out of a\nrecursive module_list:\n\n - the path to the submodule (or \".\" for the top-level) to indicate\n   where in the nested hierarchy the information came from;\n\n - the information the flat module_list gives for that location.\n\nSince you already have module_list() function natively callable from\nC and also it is available via \"git submodule--helper module_list\",\nimplementing a recursive module_list would be a good first proof of\nconcept exercise for your \"thread pool\" engine.  You can employ the\n\"dual implementation\" trick to call\n\n - a version that tells the thread to run the native C version of\n   module_list(),\n\n - another version that tells the thread to run_command()\n   \"submodule--helper module_list\" in the top-level and nested\n   submodules.\n\nand collect and compare their results and performance.\n\nThat will not just be a good proof of concept for the pool\nimplementation.\n\nOnce you have such a recursive module_list, you can use it as a way\nto easily obtain such a \"unified view\" list of all submodules.  That\ncan be used to stuff a flat work unit queue to implement reasonably\nbounded parallelism.\n\nYour recursive \"submoule update\" implementation could be\n\n - Run recursive module_list to stuff the work queue with these 500\n   submodules (possibly spread across in top-level and in nested\n   submodules, or all 500 in the flat top-level);\n\n - Start N worker bees, and tell them to pick from that work queue,\n   each element of which tells them to process which submodule that\n   resides in where (either in the top-level project or in a\n   submodule).\n\nAnd each work element would essentially be to run \"git fetch\" in\nthat submodule directory.\n\nHmm...\n"},{"id":"267774","messageId":"CAGZ79kaKdzvyU0D9XectR+3BQE+7FZ+x=X=HUYuNcLvoHDdZKA@mail.gmail.com","threadId":"40015","inReplyTo":"xmqq4mkawbc8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-10T17:03:16Z","receivedAt":"2015-08-10T17:03:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 7, 2015 at 11:20 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>>>> ... if\n>>>> you really want to go the \"thread\" route, the first thing to try\n>>>> would be to see if a few places we already use threads for\n>>>> parallelism (namely, \"grep\", \"pack-objects\", \"preload-index\" and\n>>>> \"index-pack\") can be factored out and model your new API around the\n>>>> commonality among them.\n>>\n>> And obviously, doing your pool API around threads will allow you to\n>> throw future per-thread function that do not involve run_command()\n>> at all at your API, and it will make it easy to adapt the current\n>> threaded parts of the system to the API.\n>\n> Just a few random thoughts before going to bed and going offline for\n> the weekend...\n>\n> Eventually, we would want to do \"submodule update\" of a top-level\n> project that has 500 submodules underneath, but obviously we would\n> not want to blindly spawn 500 threads, each of which runs \"fetch\",\n> all at the same time.  We'd want to limit the parallelism to a sane\n> limit (say, 16 or 32), stuff 500 work units to a queue, from which\n> that many number of worker bees grab work unit one by one to process\n> and then come back to ask for more work.\n\nThis is exactly what is currently implemented and works in patch\n\"[RFC PATCH 2/4] Add a workdispatcher to get work done in parallel\"\n\n>\n> And we would eventually want to be able to do this even when these\n> 500 submodules are spread across multiple levels of nested\n> submodules (e.g. top-level may have 8 submodules, and they have 16\n> nested subsubmodules each on average, each of which may have 4\n> nested subsubsubmodules on average).  Specifying -j16 at the top\n> level and apportioning the parallelism to recursive invoation of\n> \"submodule update\" in such a way that the overall process is\n> efficient and without waste would be a bit tricky.\n>\n> In such a nested submodule case, we may want to instead try to\n> enumerate these 500 submodules upfront with unbounded parallelism\n\nThe problem here is we need to have finished cloning at least one top level\nsubmodule before we can add any further sub-submodules into the work\nqueue. So if there are only 4 top level submodules we'd have a slow start.\n\nIf we have only one top level work queue, the deeper nesting levels\nwill not explode unbound, but eventually we will reach the 16 threads\nand keep working with them, and once git submodule is ported to C\nwe don't need to have process invocations, but can rely on the just the\nthreading done right. And then we it should be rather easy to only use\none top level task queue.\n\n> (e.g. the top-level will ask 4 worker bees to process immediate 8\n> submodules, and they each spawn 4 worker bees to process their\n> immediate 16 submodules, and so on---it is unbounded because we do\n> not know upfront how deep the nesting is).\n>\n> Let's call that a recursive module_list.  You would want out of a\n> recursive module_list:\n>\n>  - the path to the submodule (or \".\" for the top-level) to indicate\n>    where in the nested hierarchy the information came from;\n>\n>  - the information the flat module_list gives for that location.\n\nI only see value in this recursive module_list approach for updating\nthe work tree (fetch --recurse-submodules=yes, instead of cloning)\nas you already have most of the submodules there (there may be new\nsubmodules after fetching).\n\nAlso collecting 500 submodule information is in the order of reading\n500 files. But then we need to do 500 fetches. And doing a fetch takes\nsome orders of magnitude longer than reading a file, so I am not convinced\na parallel collection of work to be done is a good first step?\n\n>\n> Since you already have module_list() function natively callable from\n> C and also it is available via \"git submodule--helper module_list\",\n> implementing a recursive module_list would be a good first proof of\n> concept exercise for your \"thread pool\" engine.  You can employ the\n> \"dual implementation\" trick to call\n>\n>  - a version that tells the thread to run the native C version of\n>    module_list(),\n>\n>  - another version that tells the thread to run_command()\n>    \"submodule--helper module_list\" in the top-level and nested\n>    submodules.\n>\n> and collect and compare their results and performance.\n>\n> That will not just be a good proof of concept for the pool\n> implementation.\n>\n> Once you have such a recursive module_list, you can use it as a way\n> to easily obtain such a \"unified view\" list of all submodules.  That\n> can be used to stuff a flat work unit queue to implement reasonably\n> bounded parallelism.\n>\n> Your recursive \"submoule update\" implementation could be\n>\n>  - Run recursive module_list to stuff the work queue with these 500\n>    submodules (possibly spread across in top-level and in nested\n>    submodules, or all 500 in the flat top-level);\n>\n>  - Start N worker bees, and tell them to pick from that work queue,\n>    each element of which tells them to process which submodule that\n>    resides in where (either in the top-level project or in a\n>    submodule).\n>\n> And each work element would essentially be to run \"git fetch\" in\n> that submodule directory.\n\nAs said before the 500 fetches are many orders of magnitude slower than\ncollecting the information what to do.\n\n>\n> Hmm...\n>\n>\n"}]}