{"thread":{"id":"40243","subject":"[PATCHv4 0/3] submodule--helper: Have some refactoring only patches first","startedAt":"2015-09-01T18:24:26Z","lastAt":"2015-09-02T07:20:56Z","messageCount":12,"participants":["Stefan Beller","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269119","messageId":"1441131869-29474-1-git-send-email-sbeller@google.com","threadId":"40243","inReplyTo":null,"subject":"[PATCHv4 0/3] submodule--helper: Have some refactoring only patches first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T18:24:26Z","receivedAt":"2015-09-01T18:24:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I added all suggestions from Eric and rewrote the main function\nto not have hardcoded all the commands we're introducing.\n\ndiff to patch series 3 below.\n\nStefan Beller (3):\n  submodule: implement `list` as a builtin helper\n  submodule: implement `name` as a builtin helper\n  submodule: implement `clone` as a builtin helper\n\n .gitignore                  |   1 +\n Makefile                    |   1 +\n builtin.h                   |   1 +\n builtin/submodule--helper.c | 299 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 166 +++---------------------\n git.c                       |   1 +\n 6 files changed, 319 insertions(+), 150 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0669641..63f535a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -33,7 +33,7 @@ static int module_list_compute(int argc, const char **argv,\n \t\tps_matched = xcalloc(pathspec->nr, 1);\n \n \tif (read_cache() < 0)\n-\t\tdie(\"index file corrupt\");\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@@ -79,7 +79,7 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \t};\n \n \tconst char *const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper module_list [--prefix=<path>] [<path>...]\"),\n+\t\tN_(\"git submodule--helper list [--prefix=<path>] [<path>...]\"),\n \t\tNULL\n \t};\n \n@@ -110,21 +110,20 @@ static int module_name(int argc, const char **argv, const char *prefix)\n {\n \tconst struct submodule *sub;\n \n-\tif (argc != 1)\n+\tif (argc != 2)\n \t\tusage(\"git submodule--helper module_name <path>\\n\");\n \n \tgitmodules_config();\n-\tsub = submodule_from_path(null_sha1, argv[0]);\n+\tsub = submodule_from_path(null_sha1, argv[1]);\n \n \tif (!sub)\n \t\tdie(N_(\"No submodule mapping found in .gitmodules for path '%s'\"),\n-\t\t    argv[0]);\n+\t\t    argv[1]);\n \n \tprintf(\"%s\\n\", sub->name);\n \n \treturn 0;\n }\n-\n static int clone_submodule(const char *path, const char *gitdir, const char *url,\n \t\t\t   const char *depth, const char *reference, int quiet)\n {\n@@ -166,7 +165,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \tstruct strbuf rel_path = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tstruct option module_update_options[] = {\n+\tstruct option module_clone_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@@ -189,38 +188,41 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tstatic const char * const git_submodule_helper_usage[] = {\n-\t\tN_(\"git submodule--helper update [--prefix=<path>] [--quiet] [--remote] [-N|--no-fetch]\"\n-\t\t   \"[-f|--force] [--rebase|--merge] [--reference <repository>]\"\n-\t\t   \"[--depth <depth>] [--recursive] [--] [<path>...]\"),\n+\tconst char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper clone [--prefix=<path>] [--quiet] \"\n+\t\t   \"[--reference <repository>] [--name <name>] [--url <url>]\"\n+\t\t   \"[--depth <depth>] [--] [<path>...]\"),\n \t\tNULL\n \t};\n \n-\targc = parse_options(argc, argv, prefix, module_update_options,\n+\targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \n \tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n \tsm_gitdir = strbuf_detach(&sb, NULL);\n \n \tif (!file_exists(sm_gitdir)) {\n-\t\tsafe_create_leading_directories_const(sm_gitdir);\n+\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n \t\tif (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))\n-\t\t\tdie(N_(\"Clone of '%s' into submodule path '%s' failed\"),\n+\t\t\tdie(N_(\"clone of '%s' into submodule path '%s' failed\"),\n \t\t\t    url, path);\n \t} else {\n-\t\tsafe_create_leading_directories_const(path);\n-\t\tunlink(sm_gitdir);\n+\t\tif (safe_create_leading_directories_const(path) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), path);\n+\t\tif (unlink(sm_gitdir) < 0)\n+\t\t\tdie_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n \t}\n \n \t/* Write a .git file in the submodule to redirect to the superproject. */\n-\tif (alternative_path && !strcmp(alternative_path, \"\")) {\n+\tif (alternative_path && *alternative_path)) {\n \t\tp = relative_path(path, alternative_path, &sb);\n \t\tstrbuf_reset(&sb);\n \t} else\n \t\tp = path;\n \n \tif (safe_create_leading_directories_const(p) < 0)\n-\t\tdie(\"Could not create directory '%s'\", p);\n+\t\tdie(_(\"could not create directory '%s'\"), p);\n \n \tstrbuf_addf(&sb, \"%s/.git\", p);\n \n@@ -228,31 +230,32 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"could not create leading directories of '%s'\"), sb.buf);\n \tsubmodule_dot_git = fopen(sb.buf, \"w\");\n \tif (!submodule_dot_git)\n-\t\tdie (\"Cannot open file '%s': %s\", sb.buf, strerror(errno));\n+\t\tdie (_(\"cannot open file '%s': %s\"), sb.buf, strerror(errno));\n \n \tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n \t\trelative_path(sm_gitdir, path, &rel_path));\n \tif (fclose(submodule_dot_git))\n-\t\tdie(\"Could not close file %s\", sb.buf);\n+\t\tdie(_(\"could not close file %s\"), sb.buf);\n \tstrbuf_reset(&sb);\n \n-\t/* Redirect the worktree of the submodule in the superprojects config */\n+\t/* Redirect the worktree of the submodule in the superproject's config */\n+\tif (strbuf_getcwd(&sb))\n+\t\tdie_errno(_(\"unable to get current working directory\"));\n+\n \tif (!is_absolute_path(sm_gitdir)) {\n-\t\tchar *s = (char*)sm_gitdir;\n \t\tif (strbuf_getcwd(&sb))\n-\t\t\tdie_errno(\"unable to get current working directory\");\n+\t\t\tdie_errno(_(\"unable to get current working directory\"));\n \t\tstrbuf_addf(&sb, \"/%s\", sm_gitdir);\n+\t\tfree(sm_gitdir);\n \t\tsm_gitdir = strbuf_detach(&sb, NULL);\n-\t\tfree(s);\n \t}\n \n-\tif (strbuf_getcwd(&sb))\n-\t\tdie_errno(\"unable to get current working directory\");\n+\n \tstrbuf_addf(&sb, \"/%s\", path);\n \n \tp = git_pathdup_submodule(path, \"config\");\n \tif (!p)\n-\t\tdie(\"Could not get submodule directory for '%s'\", path);\n+\t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n \tgit_config_set_in_file(p, \"core.worktree\",\n \t\t\t       relative_path(sb.buf, sm_gitdir, &rel_path));\n \tstrbuf_release(&sb);\n@@ -260,23 +263,37 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+struct cmd_struct {\n+\tconst char *cmd;\n+\tint (*fct)(int, const char **, const char *);\n+};\n+\n+static struct cmd_struct commands[] = {\n+\t{\"list\", module_list},\n+\t{\"name\", module_name},\n+\t{\"clone\", module_clone},\n+};\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n+\tint i;\n \tif (argc < 2)\n-\t\tdie(N_(\"fatal: submodule--helper subcommand must be called with \"\n-\t\t       \"a subcommand, which are module-list, module-name, \"\n-\t\t       \"module-clone\\n\"));\n+\t\tgoto out;\n \n-\tif (!strcmp(argv[1], \"module-list\"))\n-\t\treturn module_list(argc - 1, argv + 1, prefix);\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n+\t\tif (!strcmp(argv[1], commands[i].cmd))\n+\t\t\treturn commands[i].fct(argc - 1, argv + 1, prefix);\n \n-\tif (!strcmp(argv[1], \"module-name\"))\n-\t\treturn module_name(argc - 2, argv + 2, prefix);\n+out:\n+\tif (argc > 1)\n+\t\tfprintf(stderr, _(\"fatal: '%s' is not a valid submodule--helper \"\n+\t\t\t\t  \"subcommand, which are:\\n\"), argv[1]);\n+\telse\n+\t\tfprintf(stderr, _(\"fatal: submodule--helper subcommand must be \"\n+\t\t\t\t  \"called with a subcommand, which are:\\n\"));\n \n-\tif (!strcmp(argv[1], \"module-clone\"))\n-\t\treturn module_clone(argc - 1, argv + 1, prefix);\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n+\t\tfprintf(stderr, \"\\t%s\\n\", commands[i].cmd);\n \n-\tdie(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n-\t       \"which are module-list, module-name, module-clone\\n\"),\n-\t    argv[1]);\n+\texit(129);\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d1523ea..7cfdc2c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -338,7 +338,7 @@ Use -f if you really want to add it.\" >&2\n \t\t\t\techo \"$(eval_gettext \"Reactivating local git directory for submodule '\\$sm_name'.\")\"\n \t\t\tfi\n \t\tfi\n-\t\tgit submodule--helper module-clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n+\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n \t\t(\n \t\t\tclear_local_git_env\n \t\t\tcd \"$sm_path\" &&\n@@ -398,7 +398,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tgit submodule--helper module-list --prefix \"$wt_prefix\"|\n+\tgit submodule--helper list --prefix \"$wt_prefix\"|\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -406,7 +406,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=$(git submodule--helper module-name \"$sm_path\")\n+\t\t\tname=$(git submodule--helper name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tclear_local_git_env\n@@ -458,11 +458,11 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -540,11 +540,11 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -656,7 +656,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" | {\n+\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -666,7 +666,7 @@ cmd_update()\n \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n \t\t\tcontinue\n \t\tfi\n-\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper 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@@ -700,7 +700,7 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tgit submodule--helper module-clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n+\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n \t\telse\n@@ -930,7 +930,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=$(git submodule--helper module-name \"$sm_path\")\n+\t\t\t\tname=$(git submodule--helper 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@@ -1088,11 +1088,11 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper 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@@ -1165,11 +1165,11 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n+\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(git submodule--helper module-name \"$sm_path\")\n+\t\tname=$(git submodule--helper 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\n-- \n2.5.0.256.g89f8063.dirty\n"},{"id":"269122","messageId":"1441131869-29474-2-git-send-email-sbeller@google.com","threadId":"40243","inReplyTo":"1441131869-29474-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 1/3] submodule: implement `list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T18:24:27Z","receivedAt":"2015-09-01T18:24:27Z","isPatch":false,"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 keep the\nsubdirectory 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 still 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 which to anchor relative path arguments.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n .gitignore                  |   1 +\n Makefile                    |   1 +\n builtin.h                   |   1 +\n builtin/submodule--helper.c | 137 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  54 ++---------------\n git.c                       |   1 +\n 6 files changed, 147 insertions(+), 48 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 4fd81ba..1c2f832 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 24b636d..d434e73 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -901,6 +901,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..16d9abe\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,137 @@\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 const struct cache_entry **ce_entries;\n+static int ce_alloc, ce_used;\n+\n+static int 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, result = 0;\n+\tchar *max_prefix, *ps_matched = NULL;\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(pathspec->nr, 1);\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+\n+\t\twhile (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))\n+\t\t\t/*\n+\t\t\t * Skip entries with the same name in different stages\n+\t\t\t * to make sure an entry is returned only once.\n+\t\t\t */\n+\t\t\ti++;\n+\t}\n+\tfree(max_prefix);\n+\n+\tif (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n+\t\tresult = -1;\n+\n+\tfree(ps_matched);\n+\n+\treturn result;\n+}\n+\n+static int module_list(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstruct pathspec pathspec;\n+\tconst char *alternative_path;\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+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper 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+\tif (module_list_compute(argc, argv, alternative_path\n+\t\t\t\t\t    ? alternative_path\n+\t\t\t\t\t    : prefix, &pathspec) < 0) {\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 (ce_stage(ce))\n+\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n+\t\telse\n+\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\n+\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n+\t}\n+\treturn 0;\n+}\n+\n+\n+struct cmd_struct {\n+\tconst char *cmd;\n+\tint (*fct)(int, const char **, const char *);\n+};\n+\n+static struct cmd_struct commands[] = {\n+\t{\"list\", module_list},\n+};\n+\n+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tif (argc < 2)\n+\t\tgoto out;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n+\t\tif (!strcmp(argv[1], commands[i].cmd))\n+\t\t\treturn commands[i].fct(argc - 1, argv + 1, prefix);\n+\n+out:\n+\tif (argc > 1)\n+\t\tfprintf(stderr, _(\"fatal: '%s' is not a valid submodule--helper \"\n+\t\t\t\t  \"subcommand, which are:\\n\"), argv[1]);\n+\telse\n+\t\tfprintf(stderr, _(\"fatal: submodule--helper subcommand must be \"\n+\t\t\t\t  \"called with a subcommand, which are:\\n\"));\n+\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n+\t\tfprintf(stderr, \"\\t%s\\n\", commands[i].cmd);\n+\n+\texit(129);\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..95c04fc 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 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 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 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 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 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 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.256.g89f8063.dirty\n"},{"id":"269121","messageId":"1441131869-29474-3-git-send-email-sbeller@google.com","threadId":"40243","inReplyTo":"1441131869-29474-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 2/3] submodule: implement `name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T18:24:28Z","receivedAt":"2015-09-01T18:24:28Z","isPatch":false,"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 builtin/submodule--helper.c | 22 ++++++++++++++++++++++\n git-submodule.sh            | 32 +++++++-------------------------\n 2 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 16d9abe..f5e408a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -5,6 +5,9 @@\n #include \"pathspec.h\"\n #include \"dir.h\"\n #include \"utf8.h\"\n+#include \"submodule.h\"\n+#include \"submodule-config.h\"\n+#include \"string-list.h\"\n \n static const struct cache_entry **ce_entries;\n static int ce_alloc, ce_used;\n@@ -102,6 +105,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 struct submodule *sub;\n+\n+\tif (argc != 2)\n+\t\tusage(\"git submodule--helper module_name <path>\\n\");\n+\n+\tgitmodules_config();\n+\tsub = submodule_from_path(null_sha1, argv[1]);\n+\n+\tif (!sub)\n+\t\tdie(N_(\"No submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t    argv[1]);\n+\n+\tprintf(\"%s\\n\", sub->name);\n+\n+\treturn 0;\n+}\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -110,6 +131,7 @@ struct cmd_struct {\n \n static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n+\t{\"name\", module_name},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 95c04fc..2be8da2 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 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 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 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 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 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 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 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.256.g89f8063.dirty\n"},{"id":"269120","messageId":"1441131869-29474-4-git-send-email-sbeller@google.com","threadId":"40243","inReplyTo":"1441131869-29474-1-git-send-email-sbeller@google.com","subject":"[PATCHv4 3/3] submodule: implement `clone` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T18:24:29Z","receivedAt":"2015-09-01T18:24:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This converts implements the helper `module_clone`. This functionality is\nneeded for converting `git submodule update` later on, which we want to\nadd threading to.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 140 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  80 +------------------------\n 2 files changed, 143 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f5e408a..63f535a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -8,6 +8,7 @@\n #include \"submodule.h\"\n #include \"submodule-config.h\"\n #include \"string-list.h\"\n+#include \"run-command.h\"\n \n static const struct cache_entry **ce_entries;\n static int ce_alloc, ce_used;\n@@ -123,6 +124,144 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \n \treturn 0;\n }\n+static int clone_submodule(const char *path, const char *gitdir, const char *url,\n+\t\t\t   const char *depth, const char *reference, int quiet)\n+{\n+\tstruct child_process cp;\n+\tchild_process_init(&cp);\n+\n+\targv_array_push(&cp.args, \"clone\");\n+\targv_array_push(&cp.args, \"--no-checkout\");\n+\tif (quiet)\n+\t\targv_array_push(&cp.args, \"--quiet\");\n+\tif (depth && *depth)\n+\t\targv_array_pushl(&cp.args, \"--depth\", depth, NULL);\n+\tif (reference && *reference)\n+\t\targv_array_pushl(&cp.args, \"--reference\", reference, NULL);\n+\tif (gitdir && *gitdir)\n+\t\targv_array_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n+\n+\targv_array_push(&cp.args, url);\n+\targv_array_push(&cp.args, path);\n+\n+\tcp.git_cmd = 1;\n+\tcp.env = local_repo_env;\n+\n+\tcp.no_stdin = 1;\n+\tcp.no_stdout = 1;\n+\tcp.no_stderr = 1;\n+\n+\treturn run_command(&cp);\n+}\n+\n+static int module_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *path = NULL, *name = NULL, *url = NULL;\n+\tconst char *reference = NULL, *depth = NULL;\n+\tconst char *alternative_path = NULL, *p;\n+\tint quiet = 0;\n+\tFILE *submodule_dot_git;\n+\tchar *sm_gitdir;\n+\tstruct strbuf rel_path = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstruct option module_clone_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_STRING(0, \"path\", &path,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"where the new submodule will be cloned to\")),\n+\t\tOPT_STRING(0, \"name\", &name,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"name of the new submodule\")),\n+\t\tOPT_STRING(0, \"url\", &url,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"url where to clone the submodule from\")),\n+\t\tOPT_STRING(0, \"reference\", &reference,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"reference repository\")),\n+\t\tOPT_STRING(0, \"depth\", &depth,\n+\t\t\t   N_(\"string\"),\n+\t\t\t   N_(\"depth for shallow clones\")),\n+\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper clone [--prefix=<path>] [--quiet] \"\n+\t\t   \"[--reference <repository>] [--name <name>] [--url <url>]\"\n+\t\t   \"[--depth <depth>] [--] [<path>...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_clone_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n+\tsm_gitdir = strbuf_detach(&sb, NULL);\n+\n+\tif (!file_exists(sm_gitdir)) {\n+\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n+\t\tif (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))\n+\t\t\tdie(N_(\"clone of '%s' into submodule path '%s' failed\"),\n+\t\t\t    url, path);\n+\t} else {\n+\t\tif (safe_create_leading_directories_const(path) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), path);\n+\t\tif (unlink(sm_gitdir) < 0)\n+\t\t\tdie_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n+\t}\n+\n+\t/* Write a .git file in the submodule to redirect to the superproject. */\n+\tif (alternative_path && *alternative_path)) {\n+\t\tp = relative_path(path, alternative_path, &sb);\n+\t\tstrbuf_reset(&sb);\n+\t} else\n+\t\tp = path;\n+\n+\tif (safe_create_leading_directories_const(p) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), p);\n+\n+\tstrbuf_addf(&sb, \"%s/.git\", p);\n+\n+\tif (safe_create_leading_directories_const(sb.buf) < 0)\n+\t\tdie(_(\"could not create leading directories of '%s'\"), sb.buf);\n+\tsubmodule_dot_git = fopen(sb.buf, \"w\");\n+\tif (!submodule_dot_git)\n+\t\tdie (_(\"cannot open file '%s': %s\"), sb.buf, strerror(errno));\n+\n+\tfprintf(submodule_dot_git, \"gitdir: %s\\n\",\n+\t\trelative_path(sm_gitdir, path, &rel_path));\n+\tif (fclose(submodule_dot_git))\n+\t\tdie(_(\"could not close file %s\"), sb.buf);\n+\tstrbuf_reset(&sb);\n+\n+\t/* Redirect the worktree of the submodule in the superproject's config */\n+\tif (strbuf_getcwd(&sb))\n+\t\tdie_errno(_(\"unable to get current working directory\"));\n+\n+\tif (!is_absolute_path(sm_gitdir)) {\n+\t\tif (strbuf_getcwd(&sb))\n+\t\t\tdie_errno(_(\"unable to get current working directory\"));\n+\t\tstrbuf_addf(&sb, \"/%s\", sm_gitdir);\n+\t\tfree(sm_gitdir);\n+\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n+\t}\n+\n+\n+\tstrbuf_addf(&sb, \"/%s\", path);\n+\n+\tp = git_pathdup_submodule(path, \"config\");\n+\tif (!p)\n+\t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n+\tgit_config_set_in_file(p, \"core.worktree\",\n+\t\t\t       relative_path(sb.buf, sm_gitdir, &rel_path));\n+\tstrbuf_release(&sb);\n+\tfree(sm_gitdir);\n+\treturn 0;\n+}\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -132,6 +271,7 @@ struct cmd_struct {\n static struct cmd_struct commands[] = {\n \t{\"list\", module_list},\n \t{\"name\", module_name},\n+\t{\"clone\", module_clone},\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 2be8da2..7cfdc2c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -178,80 +178,6 @@ get_submodule_config () {\n \tprintf '%s' \"${value:-$default}\"\n }\n \n-#\n-# Clone a submodule\n-#\n-# $1 = submodule path\n-# $2 = submodule name\n-# $3 = URL to clone\n-# $4 = reference repository to reuse (empty for independent)\n-# $5 = depth argument for shallow clones (empty for deep)\n-#\n-# Prior to calling, cmd_update checks that a possibly existing\n-# path is not a git repository.\n-# Likewise, cmd_add checks that path does not exist at all,\n-# since it is the location of a new submodule.\n-#\n-module_clone()\n-{\n-\tsm_path=$1\n-\tname=$2\n-\turl=$3\n-\treference=\"$4\"\n-\tdepth=\"$5\"\n-\tquiet=\n-\tif test -n \"$GIT_QUIET\"\n-\tthen\n-\t\tquiet=-q\n-\tfi\n-\n-\tgitdir=\n-\tgitdir_base=\n-\tbase_name=$(dirname \"$name\")\n-\n-\tgitdir=$(git rev-parse --git-dir)\n-\tgitdir_base=\"$gitdir/modules/$base_name\"\n-\tgitdir=\"$gitdir/modules/$name\"\n-\n-\tif test -d \"$gitdir\"\n-\tthen\n-\t\tmkdir -p \"$sm_path\"\n-\t\trm -f \"$gitdir/index\"\n-\telse\n-\t\tmkdir -p \"$gitdir_base\"\n-\t\t(\n-\t\t\tclear_local_git_env\n-\t\t\tgit clone $quiet ${depth:+\"$depth\"} -n ${reference:+\"$reference\"} \\\n-\t\t\t\t--separate-git-dir \"$gitdir\" \"$url\" \"$sm_path\"\n-\t\t) ||\n-\t\tdie \"$(eval_gettext \"Clone of '\\$url' into submodule path '\\$sm_path' failed\")\"\n-\tfi\n-\n-\t# We already are at the root of the work tree but cd_to_toplevel will\n-\t# resolve any symlinks that might be present in $PWD\n-\ta=$(cd_to_toplevel && cd \"$gitdir\" && pwd)/\n-\tb=$(cd_to_toplevel && cd \"$sm_path\" && pwd)/\n-\t# Remove all common leading directories after a sanity check\n-\tif test \"${a#$b}\" != \"$a\" || test \"${b#$a}\" != \"$b\"; then\n-\t\tdie \"$(eval_gettext \"Gitdir '\\$a' is part of the submodule path '\\$b' or vice versa\")\"\n-\tfi\n-\twhile test \"${a%%/*}\" = \"${b%%/*}\"\n-\tdo\n-\t\ta=${a#*/}\n-\t\tb=${b#*/}\n-\tdone\n-\t# Now chop off the trailing '/'s that were added in the beginning\n-\ta=${a%/}\n-\tb=${b%/}\n-\n-\t# Turn each leading \"*/\" component into \"../\"\n-\trel=$(printf '%s\\n' \"$b\" | sed -e 's|[^/][^/]*|..|g')\n-\tprintf '%s\\n' \"gitdir: $rel/$a\" >\"$sm_path/.git\"\n-\n-\trel=$(printf '%s\\n' \"$a\" | sed -e 's|[^/][^/]*|..|g')\n-\t(clear_local_git_env; cd \"$sm_path\" && GIT_WORK_TREE=. git config core.worktree \"$rel/$b\")\n-}\n-\n isnumber()\n {\n \tn=$(($1 + 0)) 2>/dev/null && test \"$n\" = \"$1\"\n@@ -301,7 +227,7 @@ cmd_add()\n \t\t\tshift\n \t\t\t;;\n \t\t--depth=*)\n-\t\t\tdepth=$1\n+\t\t\tdepth=\"$1\"\n \t\t\t;;\n \t\t--)\n \t\t\tshift\n@@ -412,7 +338,7 @@ Use -f if you really want to add it.\" >&2\n \t\t\t\techo \"$(eval_gettext \"Reactivating local git directory for submodule '\\$sm_name'.\")\"\n \t\t\tfi\n \t\tfi\n-\t\tmodule_clone \"$sm_path\" \"$sm_name\" \"$realrepo\" \"$reference\" \"$depth\" || exit\n+\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$wt_prefix\" --path \"$sm_path\" --name \"$sm_name\" --url \"$realrepo\" \"$reference\" \"$depth\" || exit\n \t\t(\n \t\t\tclear_local_git_env\n \t\t\tcd \"$sm_path\" &&\n@@ -774,7 +700,7 @@ Maybe you want to use 'update --init'?\")\"\n \n \t\tif ! test -d \"$sm_path\"/.git && ! test -f \"$sm_path\"/.git\n \t\tthen\n-\t\t\tmodule_clone \"$sm_path\" \"$name\" \"$url\" \"$reference\" \"$depth\" || exit\n+\t\t\tgit submodule--helper clone ${GIT_QUIET:+--quiet} --prefix \"$prefix\" --path \"$sm_path\" --name \"$name\" --url \"$url\" \"$reference\" \"$depth\" || exit\n \t\t\tcloned_modules=\"$cloned_modules;$name\"\n \t\t\tsubsha1=\n \t\telse\n-- \n2.5.0.256.g89f8063.dirty\n"},{"id":"269139","messageId":"CAPig+cRxQdquKZXdmpiZjj8JFB2wdCE4UcbW6qYUwvtZsVw=ug@mail.gmail.com","threadId":"40243","inReplyTo":"1441131869-29474-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T18:52:54Z","receivedAt":"2015-09-01T18:52:54Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller <sbeller@google.com> wrote:\n> This converts implements the helper `module_clone`. This functionality is\n\nMentioned previously[1]: \"converts implements\"?\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/276966\n\n> needed for converting `git submodule update` later on, which we want to\n> add threading to.\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 f5e408a..63f535a 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> +static int module_clone(int argc, const char **argv, const char *prefix)\n> +{\n> +       const char *path = NULL, *name = NULL, *url = NULL;\n> +       const char *reference = NULL, *depth = NULL;\n> +       const char *alternative_path = NULL, *p;\n> +       int quiet = 0;\n> +       FILE *submodule_dot_git;\n> +       char *sm_gitdir;\n> +       struct strbuf rel_path = STRBUF_INIT;\n> +       struct strbuf sb = STRBUF_INIT;\n> +\n> +       struct option module_clone_options[] = {\n> +               OPT_STRING(0, \"prefix\", &alternative_path,\n> +                          N_(\"path\"),\n> +                          N_(\"alternative anchor for relative paths\")),\n> +               OPT_STRING(0, \"path\", &path,\n> +                          N_(\"path\"),\n> +                          N_(\"where the new submodule will be cloned to\")),\n> +               OPT_STRING(0, \"name\", &name,\n> +                          N_(\"string\"),\n> +                          N_(\"name of the new submodule\")),\n> +               OPT_STRING(0, \"url\", &url,\n> +                          N_(\"string\"),\n> +                          N_(\"url where to clone the submodule from\")),\n> +               OPT_STRING(0, \"reference\", &reference,\n> +                          N_(\"string\"),\n> +                          N_(\"reference repository\")),\n> +               OPT_STRING(0, \"depth\", &depth,\n> +                          N_(\"string\"),\n> +                          N_(\"depth for shallow clones\")),\n> +               OPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n> +               OPT_END()\n> +       };\n> +\n> +       const char * const git_submodule_helper_usage[] = {\n\nStyle: *const\n\n> +               N_(\"git submodule--helper clone [--prefix=<path>] [--quiet] \"\n> +                  \"[--reference <repository>] [--name <name>] [--url <url>]\"\n> +                  \"[--depth <depth>] [--] [<path>...]\"),\n> +               NULL\n> +       };\n> +\n> +       argc = parse_options(argc, argv, prefix, module_clone_options,\n> +                            git_submodule_helper_usage, 0);\n> +\n> +       strbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n> +       sm_gitdir = strbuf_detach(&sb, NULL);\n> +\n> +       if (!file_exists(sm_gitdir)) {\n> +               if (safe_create_leading_directories_const(sm_gitdir) < 0)\n> +                       die(_(\"could not create directory '%s'\"), sm_gitdir);\n> +               if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))\n> +                       die(N_(\"clone of '%s' into submodule path '%s' failed\"),\n> +                           url, path);\n\ns/N_/_/\n\n> +       } else {\n> +               if (safe_create_leading_directories_const(path) < 0)\n> +                       die(_(\"could not create directory '%s'\"), path);\n> +               if (unlink(sm_gitdir) < 0)\n> +                       die_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n> +       }\n> +\n> +       /* Write a .git file in the submodule to redirect to the superproject. */\n> +       if (alternative_path && *alternative_path)) {\n> +               p = relative_path(path, alternative_path, &sb);\n> +               strbuf_reset(&sb);\n> +       } else\n> +               p = path;\n> +\n> +       if (safe_create_leading_directories_const(p) < 0)\n> +               die(_(\"could not create directory '%s'\"), p);\n> +\n> +       strbuf_addf(&sb, \"%s/.git\", p);\n> +\n> +       if (safe_create_leading_directories_const(sb.buf) < 0)\n> +               die(_(\"could not create leading directories of '%s'\"), sb.buf);\n> +       submodule_dot_git = fopen(sb.buf, \"w\");\n> +       if (!submodule_dot_git)\n> +               die (_(\"cannot open file '%s': %s\"), sb.buf, strerror(errno));\n\nStyle: drop space before '('\n\nAlso, can't you use die_errno() here?\n\n> +       fprintf(submodule_dot_git, \"gitdir: %s\\n\",\n> +               relative_path(sm_gitdir, path, &rel_path));\n> +       if (fclose(submodule_dot_git))\n> +               die(_(\"could not close file %s\"), sb.buf);\n> +       strbuf_reset(&sb);\n> +\n> +       /* Redirect the worktree of the submodule in the superproject's config */\n> +       if (strbuf_getcwd(&sb))\n> +               die_errno(_(\"unable to get current working directory\"));\n> +\n> +       if (!is_absolute_path(sm_gitdir)) {\n> +               if (strbuf_getcwd(&sb))\n> +                       die_errno(_(\"unable to get current working directory\"));\n\nWhy does this need to call getcwd() on 'sb' when it was called\nimmediately above the conditional and its value hasn't changed?\n\n> +               strbuf_addf(&sb, \"/%s\", sm_gitdir);\n> +               free(sm_gitdir);\n> +               sm_gitdir = strbuf_detach(&sb, NULL);\n> +       }\n> +\n> +\n> +       strbuf_addf(&sb, \"/%s\", path);\n> +\n> +       p = git_pathdup_submodule(path, \"config\");\n> +       if (!p)\n> +               die(_(\"could not get submodule directory for '%s'\"), path);\n> +       git_config_set_in_file(p, \"core.worktree\",\n> +                              relative_path(sb.buf, sm_gitdir, &rel_path));\n> +       strbuf_release(&sb);\n\nIs 'rel_path' being leaked here?\n\n> +       free(sm_gitdir);\n> +       return 0;\n> +}\n>\n>  struct cmd_struct {\n>         const char *cmd;\n> @@ -132,6 +271,7 @@ struct cmd_struct {\n>  static struct cmd_struct commands[] = {\n>         {\"list\", module_list},\n>         {\"name\", module_name},\n> +       {\"clone\", module_clone},\n>  };\n"},{"id":"269141","messageId":"CAPig+cTqJwx27s7bNdn7g2m5UOK28JdnXcWWvkduQiuMyDZo1g@mail.gmail.com","threadId":"40243","inReplyTo":"1441131869-29474-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 2/3] submodule: implement `name` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T19:01:16Z","receivedAt":"2015-09-01T19:01:16Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller <sbeller@google.com> wrote:\n> This implements the helper `module_name` in C instead of shell,\n\nYou probably want s/module_name/name/ or state more explicitly:\n\n    Reimplement `module_name` shell function in C as `name`.\n\nor something.\n\n> yielding a nice performance boost.\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 16d9abe..f5e408a 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -102,6 +105,24 @@ static int module_list(int argc, const char **argv, const char *prefix)\n> +static int module_name(int argc, const char **argv, const char *prefix)\n> +{\n> +       const struct submodule *sub;\n> +\n> +       if (argc != 2)\n> +               usage(\"git submodule--helper module_name <path>\\n\");\n\nMentioned previously[1]:\n\nusage(_(\"...\"));\ns/module_name/name/\ns/\\\\n//\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/276965\n\n> +       gitmodules_config();\n> +       sub = submodule_from_path(null_sha1, argv[1]);\n> +\n> +       if (!sub)\n> +               die(N_(\"No submodule mapping found in .gitmodules for path '%s'\"),\n> +                   argv[1]);\n\ns/N_/_/\ns/No/no/\n\n> +       printf(\"%s\\n\", sub->name);\n> +\n> +       return 0;\n> +}\n>\n>  struct cmd_struct {\n>         const char *cmd;\n> @@ -110,6 +131,7 @@ struct cmd_struct {\n>\n>  static struct cmd_struct commands[] = {\n>         {\"list\", module_list},\n> +       {\"name\", module_name},\n>  };\n"},{"id":"269142","messageId":"20150901190506.GA8254@sigill.intra.peff.net","threadId":"40243","inReplyTo":"CAPig+cRxQdquKZXdmpiZjj8JFB2wdCE4UcbW6qYUwvtZsVw=ug@mail.gmail.com","subject":"Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-01T19:05:06Z","receivedAt":"2015-09-01T19:05:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:\n\n> > +       /* Redirect the worktree of the submodule in the superproject's config */\n> > +       if (strbuf_getcwd(&sb))\n> > +               die_errno(_(\"unable to get current working directory\"));\n> > +\n> > +       if (!is_absolute_path(sm_gitdir)) {\n> > +               if (strbuf_getcwd(&sb))\n> > +                       die_errno(_(\"unable to get current working directory\"));\n> \n> Why does this need to call getcwd() on 'sb' when it was called\n> immediately above the conditional and its value hasn't changed?\n\nEven weirder, the next code is:\n\n> \n> > +               strbuf_addf(&sb, \"/%s\", sm_gitdir);\n> > +               free(sm_gitdir);\n> > +               sm_gitdir = strbuf_detach(&sb, NULL);\n> > +       }\n> > +\n> > +\n> > +       strbuf_addf(&sb, \"/%s\", path);\n\nSo if it _was_ an absolute path, we are adding \"/$path\" to nothing\n(having just detached the strbuf in the conditional above). That seems\nweird.\n\n-Peff\n"},{"id":"269145","messageId":"xmqqy4gqq7fw.fsf@gitster.mtv.corp.google.com","threadId":"40243","inReplyTo":"CAPig+cRxQdquKZXdmpiZjj8JFB2wdCE4UcbW6qYUwvtZsVw=ug@mail.gmail.com","subject":"Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-01T19:13:23Z","receivedAt":"2015-09-01T19:13:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller <sbeller@google.com> wrote:\n>> This converts implements the helper `module_clone`. This functionality is\n>\n> Mentioned previously[1]: \"converts implements\"?\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/276966\n\nStefan, perhaps another round of proofreading before sending would\nhave helped.\n\nI am neutral with asterisk around const myself.  I think I saw this\noften spelled with SP on both sides, i.e.\n\n\t$ git grep ' \\* const ' \\*.c | wc -l\n        121\n        $ git grep ' \\*const ' \\*.c | wc -l\n        45\n\nMajority of the latter, when viewed without pipe to wc, tells me\nthey are mostly borrowed code.\n\nAll the other comments from you on this patch makes sense to me.\n\nThanks.\n"},{"id":"269146","messageId":"CAPig+cQUKVSmt6KNpo01wo8Wzq4YxoX130ROWEri7pVXv1YLqg@mail.gmail.com","threadId":"40243","inReplyTo":"20150901190506.GA8254@sigill.intra.peff.net","subject":"Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T19:15:10Z","receivedAt":"2015-09-01T19:15:10Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 1, 2015 at 3:05 PM, Jeff King <peff@peff.net> wrote:\n> On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:\n>> > +       /* Redirect the worktree of the submodule in the superproject's config */\n>> > +       if (strbuf_getcwd(&sb))\n>> > +               die_errno(_(\"unable to get current working directory\"));\n>> > +\n>> > +       if (!is_absolute_path(sm_gitdir)) {\n>> > +               if (strbuf_getcwd(&sb))\n>> > +                       die_errno(_(\"unable to get current working directory\"));\n>>\n>> Why does this need to call getcwd() on 'sb' when it was called\n>> immediately above the conditional and its value hasn't changed?\n>\n> Even weirder, the next code is:\n>\n>> > +               strbuf_addf(&sb, \"/%s\", sm_gitdir);\n>> > +               free(sm_gitdir);\n>> > +               sm_gitdir = strbuf_detach(&sb, NULL);\n>> > +       }\n>> > +\n>> > +\n>> > +       strbuf_addf(&sb, \"/%s\", path);\n>\n> So if it _was_ an absolute path, we are adding \"/$path\" to nothing\n> (having just detached the strbuf in the conditional above). That seems\n> weird.\n\nIndeed, I saw that too, but didn't mention it since this appears to be\nan incomplete refactoring from the previous round, and my hope was\nthat mentioning the getcwd() oddity would be enough to trigger a\nthorough check of the code before sending the next version.\n"},{"id":"269147","messageId":"xmqqtwrdrk1k.fsf@gitster.mtv.corp.google.com","threadId":"40243","inReplyTo":"1441131869-29474-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-01T19:55:51Z","receivedAt":"2015-09-01T19:55:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +static int 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, result = 0;\n> +\tchar *max_prefix, *ps_matched = NULL;\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(pathspec->nr, 1);\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> +\n> +\t\twhile (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))\n> +\t\t\t/*\n> +\t\t\t * Skip entries with the same name in different stages\n> +\t\t\t * to make sure an entry is returned only once.\n> +\t\t\t */\n> +\t\t\ti++;\n> +\t}\n\nI hate myself not catching this earlier, but I suspect that this is\nnot quite a faithful rewrite of the original.  It changes behaviour\nwhen a conflicted path records a non submodule in stage #1 and a\nsubmodule in stage #2 (or stage #3), doesn't it?\n\nThe original scripted version will see the stage #1 entry and skips\nit because it is not a submodule, then will see the stage #2 entry\nand because the path is not yet in the %unmerged hash, and it will\npush it to @out.\n\nThis loop instead sees a non-submodule entry at stage #1, skips it\n(because it is not a submodule), then goes on to skip the entries\nwith the same name, including the stage #2 entry that _is_ a\nsubmodule.\n\nI think the clever \"we are done with this entry, so let's skip all\nothers that have the same name\" optimization should be done only\nwhen we did handle an entry with the same name.  I.e. something like\nthis squashed in.\n\t\n--------------------------------------------------\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 16d9abe..c4aff0c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -39,12 +39,13 @@ static int module_list_compute(int argc, const char **argv,\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\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n \n-\t\twhile (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))\n+\t\tALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n+\t\tce_entries[ce_used++] = ce;\n+\t\twhile (i + 1 < active_nr &&\n+\t\t       !strcmp(ce->name, active_cache[i + 1]->name))\n \t\t\t/*\n \t\t\t * Skip entries with the same name in different stages\n \t\t\t * to make sure an entry is returned only once.\n\n--------------------------------------------------\n\n> +static int module_list(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint i;\n> +\tstruct pathspec pathspec;\n> +\tconst char *alternative_path;\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\nDo we really need this alternative_path variable?  The \"--prefix\"\noption that overrides the passed-in variable prefix would be easier\nto understand if it touched the same variable, i.e.\n\n--------------------------------------------------\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 16d9abe..387539f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -65,10 +66,9 @@ static int module_list(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n \tstruct pathspec pathspec;\n-\tconst char *alternative_path;\n \n \tstruct option module_list_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &alternative_path,\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n \t\t\t   N_(\"path\"),\n \t\t\t   N_(\"alternative anchor for relative paths\")),\n \t\tOPT_END()\n@@ -82,9 +82,7 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_list_options,\n \t\t\t     git_submodule_helper_usage, 0);\n \n-\tif (module_list_compute(argc, argv, alternative_path\n-\t\t\t\t\t    ? alternative_path\n-\t\t\t\t\t    : prefix, &pathspec) < 0) {\n+\tif (module_list_compute(argc, argv, prefix, &pathspec) < 0) {\n \t\tprintf(\"#unmatched\\n\");\n \t\treturn 1;\n \t}\n\n--------------------------------------------------\n"},{"id":"269184","messageId":"CAPig+cTBK0Zw6ECJY4cDeP-eYOoGrCGAPXAvj1Xk-HABeBv-_A@mail.gmail.com","threadId":"40243","inReplyTo":"1441131869-29474-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T23:17:28Z","receivedAt":"2015-09-01T23:17:28Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller <sbeller@google.com> wrote:\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> [...]\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> +struct cmd_struct {\n> +       const char *cmd;\n> +       int (*fct)(int, const char **, const char *);\n\nIt would be better to stick with a more idiomatic name such as 'f' or\n'func' than invent an entirely new one ('fct').\n\n> +};\n> +\n> +static struct cmd_struct commands[] = {\n> +       {\"list\", module_list},\n> +};\n> +\n> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +       int i;\n> +       if (argc < 2)\n> +               goto out;\n> +\n> +       for (i = 0; i < ARRAY_SIZE(commands); i++)\n> +               if (!strcmp(argv[1], commands[i].cmd))\n> +                       return commands[i].fct(argc - 1, argv + 1, prefix);\n> +\n> +out:\n> +       if (argc > 1)\n> +               fprintf(stderr, _(\"fatal: '%s' is not a valid submodule--helper \"\n> +                                 \"subcommand, which are:\\n\"), argv[1]);\n> +       else\n> +               fprintf(stderr, _(\"fatal: submodule--helper subcommand must be \"\n> +                                 \"called with a subcommand, which are:\\n\"));\n> +\n> +       for (i = 0; i < ARRAY_SIZE(commands); i++)\n> +               fprintf(stderr, \"\\t%s\\n\", commands[i].cmd);\n\nA couple observations:\n\n1. git-submodule--helper isn't the only Git command featuring\nsubcommands which could benefit from this dispatching and error\nreporting code, so making it re-usable would be sensible rather than\nhiding it away inside this one (very low-level, not user-facing)\ncommand. If you go that route, it would deserve its own patch series,\nand well thought out design and interface. However...\n\n2. I realize that the suggestion of listing available subcommands was\nput forth tentatively by Junio[1], but it seems overkill for a command\nlike this which is not user-facing, and is inconsistent with other\nsubcommand-bearing commands. At this stage, it should be sufficient\nmerely to emit a plain error message (without any usage information):\n\n    unrecognized subcommand: %s\n\n    missing subcommand\n\nat which point the user can consult the man page or \"git\nsubcommand--helper -h\" to find out what went wrong.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/276935\n\n> +\n> +       exit(129);\n> +}\n"},{"id":"269188","messageId":"xmqqk2s9p9rb.fsf@gitster.mtv.corp.google.com","threadId":"40243","inReplyTo":"xmqqtwrdrk1k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-02T07:20:56Z","receivedAt":"2015-09-02T07:20:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +static int module_list_compute(int argc, const char **argv,\n>> +\t\t\t\tconst char *prefix,\n>> +\t\t\t\tstruct pathspec *pathspec)\n>> +{\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\nAnother minor thing I noticed here is that ce->ce_mode would never\nbe S_ISDIR().\n\nIt is not immediately clear if it is necessary to pass is_dir=true\nupon S_ISGITLINK(ce->ce_mode) to match_pathspec(), but I think that\nis probably a right thing to do.  The only difference this makes, I\nthink, is when a pathspec ends with a slash.  E.g. when you have a\nsubmodule at path ce->ce_name == \"dir\" and the caller says \"dir/\".\nThen DO_MATCH_DIRECTORY logic would say \"dir/\" does match \"dir\".\n\nSo taken together with the remainder of the loop, perhaps\n\n        for (i = 0; i < active_nr; i++) {\n                ce = active_cache[i];\n\n                if (!S_ISGITLINK(ce->ce_mode) ||\n                    !match_pathspec(..., is_dir=1))\n                        continue;\n\n                ALLOC_GROW(ce_entries, ce_nr + 1, ce_alloc);\n                ce_entries[ce_nr++] = ce;\n                while (...)\n                        skip the entries with the same name;\n        }\n\nwould be what we want.  Pathspec matching is much more expensive\nthan ce_mode check, and after passing that check, you know the last\nparameter to the match_pathspec() at that point, so the above\nstructure would also make sense from performance point of view, I\nthink.  And of course, the structure makes it clear that we only\ncare about GITLINK entries and nothing else in this loop, so it is\ngood from readability point of view, too.\n"}]}