{"thread":{"id":"40253","subject":"[PATCHv5 0/3] submodule--helper: Have some refactoring only patches first","startedAt":"2015-09-02T21:42:23Z","lastAt":"2015-09-08T22:46:23Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269240","messageId":"1441230146-26921-1-git-send-email-sbeller@google.com","threadId":"40253","inReplyTo":null,"subject":"[PATCHv5 0/3] submodule--helper: Have some refactoring only patches first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-02T21:42:23Z","receivedAt":"2015-09-02T21:42:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I finished the refactoring and picked up suggestions from Eric and Junio.\n\nStefan Beller (3):\n  submodule: Reimplement `module_list` shell function in C\n  submodule: Reimplement `module_name` shell function in C\n  submodule: Reimplement `module_clone` shell function in C\n\n .gitignore                  |   1 +\n Makefile                    |   1 +\n builtin.h                   |   1 +\n builtin/submodule--helper.c | 282 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 166 +++-----------------------\n git.c                       |   1 +\n 6 files changed, 302 insertions(+), 150 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 63f535a..4e30d8e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -38,17 +38,16 @@ static int module_list_compute(int argc, const char **argv,\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\tif (!S_ISGITLINK(ce->ce_mode) ||\n+\t\t    !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\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@@ -69,10 +68,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@@ -86,9 +84,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@@ -111,13 +107,13 @@ static int module_name(int argc, const char **argv, const char *prefix)\n \tconst struct submodule *sub;\n \n \tif (argc != 2)\n-\t\tusage(\"git submodule--helper module_name <path>\\n\");\n+\t\tusage(_(\"git submodule--helper name <path>\"));\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\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n \t\t    argv[1]);\n \n \tprintf(\"%s\\n\", sub->name);\n@@ -158,15 +154,14 @@ 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+\tchar *sm_gitdir, *cwd, *p;\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\tOPT_STRING(0, \"prefix\", &prefix,\n \t\t\t   N_(\"path\"),\n \t\t\t   N_(\"alternative anchor for relative paths\")),\n \t\tOPT_STRING(0, \"path\", &path,\n@@ -188,7 +183,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tconst char * const git_submodule_helper_usage[] = {\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@@ -205,67 +200,64 @@ static int module_clone(int argc, const char **argv, const char *prefix)\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(_(\"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\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n+\t\tif (unlink(sb.buf) < 0)\n \t\t\tdie_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n+\t\tstrbuf_reset(&sb);\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+\tif (safe_create_leading_directories_const(path) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), 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+\tif (path && *path)\n+\t\tstrbuf_addf(&sb, \"%s/.git\", path);\n+\telse\n+\t\tstrbuf_addf(&sb, \".git\");\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+\t\tdie_errno(_(\"cannot open file '%s'\"), sb.buf);\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+\tstrbuf_reset(&rel_path);\n \n+\tcwd = xgetcwd();\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\tstrbuf_addf(&sb, \"%s/%s\", cwd, 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+\tstrbuf_addf(&sb, \"%s/%s\", cwd, path);\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+\tstrbuf_release(&rel_path);\n \tfree(sm_gitdir);\n+\tfree(cwd);\n+\tfree(p);\n \treturn 0;\n }\n \n struct cmd_struct {\n \tconst char *cmd;\n-\tint (*fct)(int, const char **, const char *);\n+\tint (*fn)(int, const char **, const char *);\n };\n \n static struct cmd_struct commands[] = {\n@@ -278,22 +270,13 @@ 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+\t\tdie(_(\"fatal: submodule--helper subcommand must be \"\n+\t\t      \"called with a subcommand\"));\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+\t\t\treturn commands[i].fn(argc - 1, argv + 1, prefix);\n \n-\texit(129);\n+\tdie(_(\"fatal: '%s' is not a valid submodule--helper \"\n+\t      \"subcommand\"), argv[1]);\n }\n\n-- \n2.5.0.256.g89f8063.dirty\n"},{"id":"269241","messageId":"1441230146-26921-2-git-send-email-sbeller@google.com","threadId":"40253","inReplyTo":"1441230146-26921-1-git-send-email-sbeller@google.com","subject":"[PATCHv5 1/3] submodule: Reimplement `module_list` shell function in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-02T21:42:24Z","receivedAt":"2015-09-02T21:42:24Z","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 | 124 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  54 +++----------------\n git.c                       |   1 +\n 6 files changed, 134 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..e97403e\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,124 @@\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 (!S_ISGITLINK(ce->ce_mode) ||\n+\t\t    !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\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+\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+\n+\tstruct option module_list_options[] = {\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+\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, 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 (*fn)(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\tdie(_(\"fatal: submodule--helper subcommand must be \"\n+\t\t      \"called with a subcommand\"));\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].fn(argc - 1, argv + 1, prefix);\n+\n+\tdie(_(\"fatal: '%s' is not a valid submodule--helper \"\n+\t      \"subcommand\"), argv[1]);\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":"269243","messageId":"1441230146-26921-3-git-send-email-sbeller@google.com","threadId":"40253","inReplyTo":"1441230146-26921-1-git-send-email-sbeller@google.com","subject":"[PATCHv5 2/3] submodule: Reimplement `module_name` shell function in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-02T21:42:25Z","receivedAt":"2015-09-02T21:42:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This implements the helper `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 e97403e..f989319 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@@ -98,6 +101,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 name <path>\"));\n+\n+\tgitmodules_config();\n+\tsub = submodule_from_path(null_sha1, argv[1]);\n+\n+\tif (!sub)\n+\t\tdie(_(\"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@@ -106,6 +127,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":"269242","messageId":"1441230146-26921-4-git-send-email-sbeller@google.com","threadId":"40253","inReplyTo":"1441230146-26921-1-git-send-email-sbeller@google.com","subject":"[PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-02T21:42:26Z","receivedAt":"2015-09-02T21:42:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This reimplements the helper function `module_clone` in shell\nin C as `clone`. This functionality is needed for converting\n`git submodule update` later on, which we want to add threading\nto.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/submodule--helper.c | 136 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  80 +-------------------------\n 2 files changed, 139 insertions(+), 77 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f989319..4e30d8e 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@@ -119,6 +120,140 @@ 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+\tint quiet = 0;\n+\tFILE *submodule_dot_git;\n+\tchar *sm_gitdir, *cwd, *p;\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\", &prefix,\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(_(\"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\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n+\t\tif (unlink(sb.buf) < 0)\n+\t\t\tdie_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n+\t\tstrbuf_reset(&sb);\n+\t}\n+\n+\t/* Write a .git file in the submodule to redirect to the superproject. */\n+\tif (safe_create_leading_directories_const(path) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), path);\n+\n+\tif (path && *path)\n+\t\tstrbuf_addf(&sb, \"%s/.git\", path);\n+\telse\n+\t\tstrbuf_addf(&sb, \".git\");\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_errno(_(\"cannot open file '%s'\"), sb.buf);\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+\tstrbuf_reset(&rel_path);\n+\n+\tcwd = xgetcwd();\n+\t/* Redirect the worktree of the submodule in the superproject's config */\n+\tif (!is_absolute_path(sm_gitdir)) {\n+\t\tstrbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n+\t\tfree(sm_gitdir);\n+\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n+\t}\n+\n+\tstrbuf_addf(&sb, \"%s/%s\", cwd, path);\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+\tstrbuf_release(&rel_path);\n+\tfree(sm_gitdir);\n+\tfree(cwd);\n+\tfree(p);\n+\treturn 0;\n+}\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -128,6 +263,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":"269349","messageId":"xmqqio7rjppv.fsf@gitster.mtv.corp.google.com","threadId":"40253","inReplyTo":"1441230146-26921-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv5 1/3] submodule: Reimplement `module_list` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-03T18:57:16Z","receivedAt":"2015-09-03T18:57:16Z","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> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> new file mode 100644\n> index 0000000..e97403e\n> --- /dev/null\n> +++ b/builtin/submodule--helper.c\n> @@ -0,0 +1,124 @@\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\nIt is customary to use X_alloc, X_nr for an array X_something that\nis managed by ALLOC_GROW(), I think.  I'd also suggest wrapping\nthese in a struct and passing it between module_list_compute() and\nits callers.\n\n> +\tfor (i = 0; i < active_nr; i++) {\n> +\t\tconst struct cache_entry *ce = active_cache[i];\n> +\n> +\t\tif (!S_ISGITLINK(ce->ce_mode) ||\n> +\t\t    !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\nI may have said this already, but unlike tree entries, the index\nentries will never be a directory.  S_ISDIR() check here is\nmeaningless [*1*].\n\nMore importantly when you make this call, you already know that\nS_ISGITLINK(ce->ce_mode) is true.  Otherwise you wouldn't be calling\nmatch_pathspec() in the first place.\n\nSo this should be:\n\n\t\tif (!S_ISGITLINK(ce->ce_mode) ||\n\t\t    !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n\t\t\t\t    max_prefix_len, ps_matched, 1))\n\nI think.\n\nI'll attach a suggested changes on top to be squashed in.\n\n> +struct cmd_struct {\n> +\tconst char *cmd;\n> +\tint (*fn)(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\tdie(_(\"fatal: submodule--helper subcommand must be \"\n> +\t\t      \"called with a subcommand\"));\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].fn(argc - 1, argv + 1, prefix);\n> +\n> +\tdie(_(\"fatal: '%s' is not a valid submodule--helper \"\n> +\t      \"subcommand\"), argv[1]);\n> +}\n\nNice and clean code structure.  I like it ;-).\n\n[Footnote]\n\n*1* It is a benign bug, as a ce that is S_ISGITLINK() always\nsatisfies S_ISDIR() by chance.  But it is a bug nevertheless.\n\nbuiltin/ls-files.c and dir.h, the former of which you copied\nthis code from, may want be corrected.  These mistakes were\nintroduced by ae8d0824 (pathspec: pass directory indicator to\nmatch_pathspec_item(), 2014-01-24).  Further, I suspect these\nmistakes were copied from ce_to_dtype() in cache.h, whose mistake\nwas introduced by d6b8fc30 (gitignore(5): Allow \"foo/\" in ignore\nlist to match directory \"foo\", 2008-01-31).\n\nThat commit mistakenly copied S_ISDIR() check from create_ce_mode(),\nbut create_ce_mode() is fed st.st_mode and checking for S_ISDIR() is\na correct thing to use there.\n\nIn short, there are three hits from \"git grep 'S_ISDIR(ce'\" that\nchecks if ce->ce_mode is S_ISDIR() || S_ISGITLINK(); they all should\ncheck only for S_ISGITLINK().\n\n\n builtin/submodule--helper.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex e97403e..10db4e6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -6,12 +6,16 @@\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+struct module_list {\n+\tconst struct cache_entry **entries;\n+\tint alloc, nr;\n+};\n+#define MODULE_LIST_INIT { NULL, 0, 0 }\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+\t\t\t       const char *prefix,\n+\t\t\t       struct pathspec *pathspec,\n+\t\t\t       struct module_list *list)\n {\n \tint i, result = 0;\n \tchar *max_prefix, *ps_matched = NULL;\n@@ -36,12 +40,11 @@ static int module_list_compute(int argc, const char **argv,\n \n \t\tif (!S_ISGITLINK(ce->ce_mode) ||\n \t\t    !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\t\t    max_prefix_len, ps_matched, 1))\n \t\t\tcontinue;\n \n-\t\tALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n-\t\tce_entries[ce_used++] = ce;\n+\t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n+\t\tlist->entries[list->nr++] = ce;\n \t\twhile (i + 1 < active_nr &&\n \t\t       !strcmp(ce->name, active_cache[i + 1]->name))\n \t\t\t/*\n@@ -64,6 +67,7 @@ static int module_list(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n \tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n \n \tstruct option module_list_options[] = {\n \t\tOPT_STRING(0, \"prefix\", &prefix,\n@@ -80,13 +84,13 @@ 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, prefix, &pathspec) < 0) {\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 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+\tfor (i = 0; i < list.nr; i++) {\n+\t\tconst struct cache_entry *ce = list.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"},{"id":"269352","messageId":"CAGZ79kb5_VJwJoBpvUdVMC43qvJThKa1utQFO68HjM1sciZ6-w@mail.gmail.com","threadId":"40253","inReplyTo":"xmqqio7rjppv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 1/3] submodule: Reimplement `module_list` shell function in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-03T19:18:51Z","receivedAt":"2015-09-03T19:18:51Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 3, 2015 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> It is customary to use X_alloc, X_nr for an array X_something that\n> is managed by ALLOC_GROW(), I think.  I'd also suggest wrapping\n> these in a struct and passing it between module_list_compute() and\n> its callers.\n\nI did not take the suggestion as a strong suggestion at the time, but the\nlooking at resulting squash proposal it looks way better.\n\n> I may have said this already, but unlike tree entries, the index\n> entries will never be a directory.  S_ISDIR() check here is\n> meaningless [*1*].\n\nRight. I was too focused on the other bug, of checking S_ISGITLINK after\nthe pathspec matching, that I overlooked the ISDIR again. :(\n\n>> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>> +{\n>> +     int i;\n>> +     if (argc < 2)\n>> +             die(_(\"fatal: submodule--helper subcommand must be \"\n>> +                   \"called with a subcommand\"));\n>> +\n>> +     for (i = 0; i < ARRAY_SIZE(commands); i++)\n>> +             if (!strcmp(argv[1], commands[i].cmd))\n>> +                     return commands[i].fn(argc - 1, argv + 1, prefix);\n>> +\n>> +     die(_(\"fatal: '%s' is not a valid submodule--helper \"\n>> +           \"subcommand\"), argv[1]);\n>> +}\n>\n> Nice and clean code structure.  I like it ;-).\n\nIt took a good while of discussion and reviews to arrive at\nthat structure eventually.\n\nThe squash proposal looks good to me.\n"},{"id":"269384","messageId":"xmqq8u8ni2bv.fsf@gitster.mtv.corp.google.com","threadId":"40253","inReplyTo":"1441230146-26921-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-03T22:07:48Z","receivedAt":"2015-09-03T22:07:48Z","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> @@ -119,6 +120,140 @@ 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\nOutput from \"git clone\" is not shown, regardless of --quiet option?\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> +\tint quiet = 0;\n> +\tFILE *submodule_dot_git;\n> +\tchar *sm_gitdir, *cwd, *p;\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\", &prefix,\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\nThe original says\n\n\tbase_name=$(dirname \"$name\")\n\nbefore doing this %s/modules/%s concatenation.  I do not think we\nintended to allow a slash in submodule name, so this difference may\nbe showing that the original was doing an unnecessary thing.\n\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(_(\"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\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n> +\t\tif (unlink(sb.buf) < 0)\n> +\t\t\tdie_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n\nThe original says \"we do not care if it failed\" with\n\n\trm -f \"$gitdir/index\"\n\nI think the intention of the original is \"we do not care if it\nfailed because it did not exist.\" in which case unconditional\ndie_errno() here may be something we do not want?\n\n> +\t\tstrbuf_reset(&sb);\n> +\t}\n> +\n> +\t/* Write a .git file in the submodule to redirect to the superproject. */\n> +\tif (safe_create_leading_directories_const(path) < 0)\n> +\t\tdie(_(\"could not create directory '%s'\"), path);\n> +\n> +\tif (path && *path)\n> +\t\tstrbuf_addf(&sb, \"%s/.git\", path);\n> +\telse\n> +\t\tstrbuf_addf(&sb, \".git\");\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_errno(_(\"cannot open file '%s'\"), sb.buf);\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> +\tstrbuf_reset(&rel_path);\n\nThe original seems to go quite a length to make sure symbolic links\ndo not fool the comparison between $gitdir and $sm_path, and also it\nmakes sure one is not a subpath of the other.  Do we need the same\nlevel of carefulness, or is textual relative_path() enough?\n\n> +\tcwd = xgetcwd();\n> +\t/* Redirect the worktree of the submodule in the superproject's config */\n> +\tif (!is_absolute_path(sm_gitdir)) {\n> +\t\tstrbuf_addf(&sb, \"%s/%s\", cwd, sm_gitdir);\n> +\t\tfree(sm_gitdir);\n> +\t\tsm_gitdir = strbuf_detach(&sb, NULL);\n> +\t}\n> +\n> +\tstrbuf_addf(&sb, \"%s/%s\", cwd, path);\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> +\tstrbuf_release(&rel_path);\n> +\tfree(sm_gitdir);\n> +\tfree(cwd);\n> +\tfree(p);\n> +\treturn 0;\n> +}\n>  \n>  struct cmd_struct {\n>  \tconst char *cmd;\n\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 2be8da2..7cfdc2c 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\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\nThis seems to be an unrelated change.\n\nOtherwise looks quite straight-forward.\n\nThanks.\n"},{"id":"269392","messageId":"CAPig+cTjXxo+iD7wkwrmHNQVDDYnqqHPEpVHNdVKCrYohb6adw@mail.gmail.com","threadId":"40253","inReplyTo":"1441230146-26921-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-03T23:17:50Z","receivedAt":"2015-09-03T23:17:50Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 2, 2015 at 5:42 PM, Stefan Beller <sbeller@google.com> wrote:\n> This reimplements the helper function `module_clone` in shell\n> in C as `clone`. This functionality is needed for converting\n> `git submodule update` later on, which we want to add threading\n> to.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> +static int module_clone(int argc, const char **argv, const char *prefix)\n> +{\n> +       /* Write a .git file in the submodule to redirect to the superproject. */\n> +       if (safe_create_leading_directories_const(path) < 0)\n> +               die(_(\"could not create directory '%s'\"), path);\n> +\n> +       if (path && *path)\n> +               strbuf_addf(&sb, \"%s/.git\", path);\n> +       else\n> +               strbuf_addf(&sb, \".git\");\n\nMinor: strbuf_addstr(...);\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_errno(_(\"cannot open file '%s'\"), sb.buf);\n> +}\n"},{"id":"269620","messageId":"CAGZ79kYAsNZ1huLrYOvyPtYHKoN4paBGXbY3OMX3SQNMwqCiKA@mail.gmail.com","threadId":"40253","inReplyTo":"xmqq8u8ni2bv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-08T18:31:17Z","receivedAt":"2015-09-08T18:31:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 3, 2015 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +\n>> +     cp.no_stdin = 1;\n>> +     cp.no_stdout = 1;\n>> +     cp.no_stderr = 1;\n>\n> Output from \"git clone\" is not shown, regardless of --quiet option?\n\nRemoved that.\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>\n> The original says\n>\n>         base_name=$(dirname \"$name\")\n>\n> before doing this %s/modules/%s concatenation.  I do not think we\n> intended to allow a slash in submodule name, so this difference may\n> be showing that the original was doing an unnecessary thing.\n\nThe way I understood the code, this was a workaround of now having\nsafe_create_leading_directories, which takes the base directory of\na given path and creates that directory. (base_name is only used as an\nargument for mkdir -p).\n\nSlashes are already in use for submodule names as the name defaults\nto the path if no explicit name is given. And the path may contain slashes\nas it may be nested. In Gerrit we have a .gitmodules:\n[submodule \"plugins/commit-message-length-validator\"]\n    path = plugins/commit-message-length-validator\n    url = ../plugins/commit-message-length-validator\n[...\n\n>\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(_(\"clone of '%s' into submodule path '%s' failed\"),\n>> +                         url, path);\n>> +     } else {\n>> +             if (safe_create_leading_directories_const(path) < 0)\n>> +                     die(_(\"could not create directory '%s'\"), path);\n>> +             strbuf_addf(&sb, \"%s/index\", sm_gitdir);\n>> +             if (unlink(sb.buf) < 0)\n>> +                     die_errno(_(\"failed to delete '%s'\"), sm_gitdir);\n>\n> The original says \"we do not care if it failed\" with\n>\n>         rm -f \"$gitdir/index\"\n>\n> I think the intention of the original is \"we do not care if it\n> failed because it did not exist.\" in which case unconditional\n> die_errno() here may be something we do not want?\n\nRight, this was a short-circuit reaction from me on Erics comment\nto check for the return value of unlink. I think we can use\nunlink_or_warn here as that only warns if we cannot remove\nan existing file. non existent files are not warned about.\n\n>\n>> +             strbuf_reset(&sb);\n>> +     }\n>> +\n>> +     /* Write a .git file in the submodule to redirect to the superproject. */\n>> +     if (safe_create_leading_directories_const(path) < 0)\n>> +             die(_(\"could not create directory '%s'\"), path);\n>> +\n>> +     if (path && *path)\n>> +             strbuf_addf(&sb, \"%s/.git\", path);\n>> +     else\n>> +             strbuf_addf(&sb, \".git\");\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_errno(_(\"cannot open file '%s'\"), sb.buf);\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>> +     strbuf_reset(&rel_path);\n>\n> The original seems to go quite a length to make sure symbolic links\n> do not fool the comparison between $gitdir and $sm_path, and also it\n> makes sure one is not a subpath of the other.  Do we need the same\n> level of carefulness, or is textual relative_path() enough?\n\nI think the original was doing an \"optimized\" version of relative_path()\nas we know that they have an anchor at the superproject root directory.\n\nrelative_path() seems to deal with the symbolic links just fine, as all\ntests pass. (6eafa6d096ce, \"submodules: don't stumble over symbolic\nlinks when cloning recursively\" did add code as well as testing for symbolic\nlink problems, Thanks Jens!)\n\n>>               --depth=*)\n>> -                     depth=$1\n>> +                     depth=\"$1\"\n>\n> This seems to be an unrelated change.\n\nA leftover from earlier, when I mucked around with --depth and\n--reference in the shell scipt.\n\n\nOn Thu, Sep 3, 2015 at 4:17 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> +       if (path && *path)\n>> +               strbuf_addf(&sb, \"%s/.git\", path);\n>> +       else\n>> +               strbuf_addf(&sb, \".git\");\n>\n> Minor: strbuf_addstr(...);\n\ndone\n"},{"id":"269633","messageId":"xmqqr3m87cn4.fsf@gitster.mtv.corp.google.com","threadId":"40253","inReplyTo":"CAGZ79kYAsNZ1huLrYOvyPtYHKoN4paBGXbY3OMX3SQNMwqCiKA@mail.gmail.com","subject":"Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-08T22:46:23Z","receivedAt":"2015-09-08T22:46:23Z","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> On Thu, Sep 3, 2015 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> +\n>>> +     cp.no_stdin = 1;\n>>> +     cp.no_stdout = 1;\n>>> +     cp.no_stderr = 1;\n>>\n>> Output from \"git clone\" is not shown, regardless of --quiet option?\n>\n> Removed that.\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>>\n>> The original says\n>>\n>>         base_name=$(dirname \"$name\")\n>> ...\n> Slashes are already in use for submodule names as the name defaults\n> to the path if no explicit name is given.\n\nAhh, OK, that base_name thing is so that \"mkdir -p\" can create a\nsurrounding directory without creating the final level, which is\nleft for 'git clone\" to prepare.  I misread the code.  Thanks.\n"}]}