{"thread":{"id":"40232","subject":"[PATCHv2 0/3] submodule--helper: Have some refactoring only patches first","startedAt":"2015-08-31T19:19:24Z","lastAt":"2015-09-01T18:35:17Z","messageCount":21,"participants":["Stefan Beller","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269029","messageId":"1441048767-29729-1-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":null,"subject":"[PATCHv2 0/3] submodule--helper: Have some refactoring only patches first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-31T19:19:24Z","receivedAt":"2015-08-31T19:19:24Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch series is the first part of the larger series I sent daily last week\nand contains only those patches which have had good review by Jeff, Dscho\nand Junio doing only internal refactoring, not introducing any parallelism yet.\nIt replaces all patches on top of (5a1ba6b48a62b,\nMerge 'hv/submodule-config' to 'sb/submodule-helper')\n\nChanges are only small nits:\n * whitespace correction in free((char *)pointer);\n * break a long line,\n * git submodule--helper -h adheres to the standard help output\n \nOne of the mistakes I made last week was mixing these quite stable patches\nwith highly RFC/WIP patches and not pointing out loud which is which.\n\nThanks,\nStefan\n\nStefan Beller (3):\n submodule: implement `module_list` as a builtin helper\n submodule: implement `module_name` as a builtin helper\n submodule: implement `module_clone` as a builtin helper\n\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/submodule--helper.c | 286 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 166 +++----------------------\n git.c | 1 +\n 6 files changed, 306 insertions(+), 150 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\n-- \n2.5.0.264.geed6e44.dirty\n"},{"id":"269030","messageId":"1441048767-29729-2-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"1441048767-29729-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/3] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-31T19:19:25Z","receivedAt":"2015-08-31T19:19:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Most of the submodule operations work on a set of submodules.\nCalculating and using this set is usually done via:\n\n module_list \"$@\" | {\n while read mode sha1 stage sm_path\n do\n # the actual operation\n done\n }\n\nCurrently the function `module_list` is implemented in the\ngit-submodule.sh as a shell script wrapping a perl script.\nThe rewrite is in C, such that it is faster and can later be\neasily adapted when other functions are rewritten in C.\n\ngit-submodule.sh similar to the builtin commands will navigate\nto the top most directory of the repository and keeping the\nsubdirectories as a variable. As the helper is called from\nwithin the git-submodule.sh script, we are already navigated\nto the root level, but the path arguments are stil relative\nto the subdirectory we were in when calling git-submodule.sh.\nThat's why there is a `--prefix` option pointing to an alternative\npath where to anchor relative path arguments.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/submodule--helper.c | 114 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 +++------------------\n git.c | 1 +\n 6 files changed, 124 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..beaab7d\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,114 @@\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+static const char *alternative_path;\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;\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\treturn -1;\n+\n+\treturn 0;\n+}\n+\n+static int module_list(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstatic struct pathspec pathspec;\n+\n+\tstruct option module_list_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &alternative_path,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_END()\n+\t};\n+\n+\tstatic const char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper module_list [--prefix=] [...]\"),\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\t} else {\n+\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\t\t}\n+\n+\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n+\t}\n+\treturn 0;\n+}\n+\n+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc < 2)\n+\t\tgoto usage;\n+\n+\tif (!strcmp(argv[1], \"module_list\"))\n+\t\treturn module_list(argc - 1, argv + 1, prefix);\n+\n+usage:\n+\tusage(\"git submodule--helper module_list\\n\");\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..af9ecef 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -145,48 +145,6 @@ relative_path ()\n \techo \"$result$target\"\n }\n \n-#\n-# Get submodule info for registered submodules\n-# $@ = path to limit submodule list\n-#\n-module_list()\n-{\n-\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n-\t(\n-\t\tgit ls-files -z --error-unmatch --stage -- \"$@\" ||\n-\t\techo \"unmatched pathspec exists\"\n-\t) |\n-\t@@PERL@@ -e '\n-\tmy %unmerged = ();\n-\tmy ($null_sha1) = (\"0\" x 40);\n-\tmy @out = ();\n-\tmy $unmatched = 0;\n-\t$/ = \"\\0\";\n-\twhile () {\n-\t\tif (/^unmatched pathspec/) {\n-\t\t\t$unmatched = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tchomp;\n-\t\tmy ($mode, $sha1, $stage, $path) =\n-\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n-\t\tnext unless $mode eq \"160000\";\n-\t\tif ($stage ne \"0\") {\n-\t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n-\t\t\t}\n-\t\t\tnext;\n-\t\t}\n-\t\tpush @out, \"$_\\n\";\n-\t}\n-\tif ($unmatched) {\n-\t\tprint \"#unmatched\\n\";\n-\t} else {\n-\t\tprint for (@out);\n-\t}\n-\t'\n-}\n-\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n@@ -532,7 +490,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tmodule_list |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\"|\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -592,7 +550,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -674,7 +632,7 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -790,7 +748,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tmodule_list \"$@\" | {\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -1222,7 +1180,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -1299,7 +1257,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\ndiff --git a/git.c b/git.c\nindex 55c327c..deecba0 100644\n--- a/git.c\n+++ b/git.c\n@@ -469,6 +469,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n+\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n \t{ \"tag\", cmd_tag, RUN_SETUP },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n-- \n2.5.0.264.geed6e44.dirty\n"},{"id":"269031","messageId":"1441048767-29729-3-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"1441048767-29729-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/3] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-31T19:19:26Z","receivedAt":"2015-08-31T19:19:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This implements the helper `module_name` in C instead of shell,\nyielding a nice performance boost.\n\nBefore this patch, I measured a time (best out of three):\n\n $ time ./t7400-submodule-basic.sh >/dev/null\n real\t0m11.066s\n user\t0m3.348s\n sys\t0m8.534s\n\nWith this patch applied I measured (also best out of three)\n\n $ time ./t7400-submodule-basic.sh >/dev/null\n real\t0m10.063s\n user\t0m3.044s\n sys\t0m7.487s\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 28 +++++++++++++++++++++++++++-\n git-submodule.sh | 32 +++++++-------------------------\n 2 files changed, 34 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex beaab7d..c8f7e0c 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@@ -101,6 +104,26 @@ static int module_list(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int module_name(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *name;\n+\tconst struct submodule *sub;\n+\n+\tif (argc != 1)\n+\t\tusage(\"git submodule--helper module_name \\n\");\n+\n+\tgitmodules_config();\n+\tsub = submodule_from_path(null_sha1, argv[0]);\n+\n+\tif (!sub)\n+\t\tdie(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n+\n+\tname = sub->name;\n+\tprintf(\"%s\\n\", name);\n+\n+\treturn 0;\n+}\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n \tif (argc < 2)\n@@ -109,6 +132,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"module_list\"))\n \t\treturn module_list(argc - 1, argv + 1, prefix);\n \n+\tif (!strcmp(argv[1], \"module_name\"))\n+\t\treturn module_name(argc - 2, argv + 2, prefix);\n+\n usage:\n-\tusage(\"git submodule--helper module_list\\n\");\n+\tusage(\"git submodule--helper [module_list | module_name]\\n\");\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex af9ecef..e6ff38d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -178,24 +178,6 @@ get_submodule_config () {\n \tprintf '%s' \"${value:-$default}\"\n }\n \n-\n-#\n-# Map submodule path to submodule name\n-#\n-# $1 = path\n-#\n-module_name()\n-{\n-\t# Do we have \"submodule..path = $1\" defined in .gitmodules file?\n-\tsm_path=\"$1\"\n-\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n-\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n-\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n-\ttest -z \"$name\" &&\n-\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n-\tprintf '%s\\n' \"$name\"\n-}\n-\n #\n # Clone a submodule\n #\n@@ -498,7 +480,7 @@ cmd_foreach()\n \t\tthen\n \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n-\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tclear_local_git_env\n@@ -554,7 +536,7 @@ cmd_init()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -636,7 +618,7 @@ cmd_deinit()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -758,7 +740,7 @@ cmd_update()\n \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n \t\t\tcontinue\n \t\tfi\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n@@ -1022,7 +1004,7 @@ cmd_summary() {\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n-\t\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n \t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n@@ -1184,7 +1166,7 @@ cmd_status()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module_name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \t\tif test \"$stage\" = U\n@@ -1261,7 +1243,7 @@ cmd_sync()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\")\n+\t\tname=$(git submodule--helper module_name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \n \t\t# Possibly a url relative to parent\n-- \n2.5.0.264.geed6e44.dirty\n"},{"id":"269032","messageId":"1441048767-29729-4-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"1441048767-29729-1-git-send-email-sbeller@google.com","subject":"[PATCH 3/3] submodule: implement `module_clone` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-31T19:19:27Z","receivedAt":"2015-08-31T19:19:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`module_clone` is part of the update command,\nwhich I want to convert to C next.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 148 +++++++++++++++++++++++++++++++++++++++++++-\n git-submodule.sh | 80 +-----------------------\n 2 files changed, 150 insertions(+), 78 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c8f7e0c..d29499c 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@@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix)\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+\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 && strcmp(depth, \"\")) {\n+\t\targv_array_push(&cp.args, \"--depth\");\n+\t\targv_array_push(&cp.args, depth);\n+\t}\n+\tif (reference && strcmp(reference, \"\")) {\n+\t\targv_array_push(&cp.args, \"--reference\");\n+\t\targv_array_push(&cp.args, reference);\n+\t}\n+\tif (gitdir) {\n+\t\targv_array_push(&cp.args, \"--separate-git-dir\");\n+\t\targv_array_push(&cp.args, gitdir);\n+\t}\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+\tconst char *sm_gitdir, *p;\n+\tstruct strbuf rel_path = STRBUF_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstruct option module_update_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_END()\n+\t};\n+\n+\tstatic const char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper update [--prefix=] [--quiet] [--remote] [-N|--no-fetch]\"\n+\t\t \"[-f|--force] [--rebase|--merge] [--reference ]\"\n+\t\t \"[--depth ] [--recursive] [--] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_update_options,\n+\t\t\t git_submodule_helper_usage, 0);\n+\n+\tif (getenv(\"GIT_QUIET\"))\n+\t\tquiet = 1;\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 (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\tsafe_create_leading_directories_const(path);\n+\t\tunlink(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+\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 superprojects config */\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\tstrbuf_addf(&sb, \"/%s\", 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+\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((char *)sm_gitdir);\n+\treturn 0;\n+}\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n \tif (argc < 2)\n@@ -135,6 +277,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"module_name\"))\n \t\treturn module_name(argc - 2, argv + 2, prefix);\n \n+\tif (!strcmp(argv[1], \"module_clone\"))\n+\t\treturn module_clone(argc - 1, argv + 1, prefix);\n+\n usage:\n-\tusage(\"git submodule--helper [module_list | module_name]\\n\");\n+\tusage(\"git submodule--helper [module_list | module_name | \"\n+\t \"module_clone]\\n\");\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex e6ff38d..fb5155e 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 module_clone --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 module_clone --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.264.geed6e44.dirty\n"},{"id":"269046","messageId":"xmqq1tejuscs.fsf@gitster.mtv.corp.google.com","threadId":"40232","inReplyTo":"1441048767-29729-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv2 0/3] submodule--helper: Have some refactoring only patches first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T20:15:47Z","receivedAt":"2015-08-31T20:15:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> One of the mistakes I made last week was mixing these quite stable patches\n> with highly RFC/WIP patches and not pointing out loud which is which.\n\nI think it was very clear which is which to people who have been\nfollowing (which are the only people you would care about anyway),\nbut I agree that it would be easier to follow by others if you made\ntwo series, i.e. solid bits like this one, marked with \"PATCH v$n\",\nand the WIP part as a separate series, described as building on the\nv$n reroll of the other series in its own cover letter.\n\nThanks, will replace. Let's move this part to 'next' and start\ncooking.\n"},{"id":"269047","messageId":"xmqqwpwbtcw2.fsf@gitster.mtv.corp.google.com","threadId":"40232","inReplyTo":"1441048767-29729-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-31T20:35:09Z","receivedAt":"2015-08-31T20:35:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> usage:\n> -\tusage(\"git submodule--helper module_list\\n\");\n> +\tusage(\"git submodule--helper [module_list | module_name]\\n\");\n\nTo me, the above reads as if saying:\n\n The command takes one of the two subcommands at this stage,\n module_list that does not take any parameter, and module_name\n that does not take any parameter.\n\nwhich is not what you intended.\n\nI think that the help for individual options and arguments are\nsufficiently given in the implementation of each subcommand\n(e.g. module_list does its own parse_options() thing), so there is\nno need to duplicate them here. The only purpose of this usage serves\nis to tell the user that the subcommand name was not understood, and\ngive the list of available subcommands. For that, I wonder if the\nusual single-liner \"usage\" is the best way to do so.\n\n $ git submodule--helper frotz\n fatal: 'frotz' is not a valid submodule--helper subcommand, which are\n module_list, module_name.\n\nor something along that line, perhaps, may be more appropriate?\n"},{"id":"269053","messageId":"CAGZ79kb6Nf8Vow=ehv7zib8erOeTa5NX2Cj=b4jcdhSiDEEusQ@mail.gmail.com","threadId":"40232","inReplyTo":"xmqqwpwbtcw2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-31T20:51:48Z","receivedAt":"2015-08-31T20:51:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 31, 2015 at 1:35 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> usage:\n>> - usage(\"git submodule--helper module_list\\n\");\n>> + usage(\"git submodule--helper [module_list | module_name]\\n\");\n>\n> To me, the above reads as if saying:\n>\n> The command takes one of the two subcommands at this stage,\n> module_list that does not take any parameter, and module_name\n> that does not take any parameter.\n>\n> which is not what you intended.\n>\n> I think that the help for individual options and arguments are\n> sufficiently given in the implementation of each subcommand\n> (e.g. module_list does its own parse_options() thing), so there is\n> no need to duplicate them here. The only purpose of this usage serves\n> is to tell the user that the subcommand name was not understood, and\n> give the list of available subcommands. For that, I wonder if the\n> usual single-liner \"usage\" is the best way to do so.\n>\n> $ git submodule--helper frotz\n> fatal: 'frotz' is not a valid submodule--helper subcommand, which are\n> module_list, module_name.\n>\n> or something along that line, perhaps, may be more appropriate?\n\nAs this is something that *should* not happen in the wild, (but it\nwill of course),\nit sounds like a good idea to have a clear error message here. I'll send a patch\nfor that. (either one on top of 3/3 to improve the message, or a\nreroll of the series,\nas you'd like)\n"},{"id":"269059","messageId":"CAPig+cSOFPAca=m4TaiY6xeeB3eYPC+Rtj15JQ7WZ=CK_gmpFw@mail.gmail.com","threadId":"40232","inReplyTo":"1441048767-29729-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/3] submodule: implement `module_list` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T21:55:07Z","receivedAt":"2015-08-31T21:55:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller 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> git-submodule.sh similar to the builtin commands will navigate\n> to the top most directory of the repository and keeping the\n> subdirectories as a variable.\n\nECANNOTPARSE\n\nDid you mean\n s/git-submodule.sh/&,/\n s/commands/&,/\n s/top most/top-most/\n s/keeping/keep/\n s/subdirectories/subdirectory/\n?\n\n> As the helper is called from\n> within the git-submodule.sh script, we are already navigated\n> to the root level, but the path arguments are stil relative\n\ns/stil/still/\n\n> to the subdirectory we were in when calling git-submodule.sh.\n> That's why there is a `--prefix` option pointing to an alternative\n> path where to anchor relative path arguments.\n\ns/where to/at which to/\n\nMore below.\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> new file mode 100644\n> index 0000000..beaab7d\n> --- /dev/null\n> +++ b/builtin/submodule--helper.c\n> @@ -0,0 +1,114 @@\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> +static const char *alternative_path;\n\nWhy is 'alternative_path' declared at file scope?\n\n> +static int module_list_compute(int argc, const char **argv,\n> + const char *prefix,\n> + struct pathspec *pathspec)\n> +{\n> + int i;\n> + char *max_prefix, *ps_matched = NULL;\n> + int max_prefix_len;\n> + parse_pathspec(pathspec, 0,\n> + PATHSPEC_PREFER_FULL |\n> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n> + prefix, argv);\n> +\n> + /* Find common prefix for all pathspec's */\n> + max_prefix = common_prefix(pathspec);\n> + max_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> +\n> + if (pathspec->nr)\n> + ps_matched = xcalloc(pathspec->nr, 1);\n> +\n> + if (read_cache() < 0)\n> + die(\"index file corrupt\");\n> +\n> + for (i = 0; i < active_nr; i++) {\n> + const struct cache_entry *ce = active_cache[i];\n> +\n> + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n> + max_prefix_len, ps_matched,\n> + S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))\n> + continue;\n> +\n> + if (S_ISGITLINK(ce->ce_mode)) {\n> + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n> + ce_entries[ce_used++] = ce;\n> + }\n> +\n> + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))\n> + /*\n> + * Skip entries with the same name in different stages\n> + * to make sure an entry is returned only once.\n> + */\n> + i++;\n> + }\n> + free(max_prefix);\n> +\n> + if (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n> + return -1;\n> +\n> + return 0;\n\nDoes 'ps_matched' need to be freed before these two 'return's?\n\n> +}\n> +\n> +static int module_list(int argc, const char **argv, const char *prefix)\n> +{\n> + int i;\n> + static struct pathspec pathspec;\n\nDrop 'static'.\n\n> +\n> + struct option module_list_options[] = {\n> + OPT_STRING(0, \"prefix\", &alternative_path,\n> + N_(\"path\"),\n> + N_(\"alternative anchor for relative paths\")),\n> + OPT_END()\n> + };\n> +\n> + static const char * const git_submodule_helper_usage[] = {\n\nYou can drop this 'static' too.\n\nStyle: *const\n\n> + N_(\"git submodule--helper module_list [--prefix=] [...]\"),\n> + NULL\n> + };\n> +\n> + argc = parse_options(argc, argv, prefix, module_list_options,\n> + git_submodule_helper_usage, 0);\n> +\n> + if (module_list_compute(argc, argv, alternative_path\n> + ? alternative_path\n> + : prefix, &pathspec) < 0) {\n> + printf(\"#unmatched\\n\");\n> + return 1;\n> + }\n> +\n> + for (i = 0; i < ce_used; i++) {\n> + const struct cache_entry *ce = ce_entries[i];\n> +\n> + if (ce_stage(ce)) {\n> + printf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n> + } else {\n> + printf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n> + }\n\nStyle: drop unnecessary braces.\n\n> + utf8_fprintf(stdout, \"%s\\n\", ce->name);\n> + }\n> + return 0;\n> +}\n> +\n> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> +{\n> + if (argc < 2)\n> + goto usage;\n> +\n> + if (!strcmp(argv[1], \"module_list\"))\n> + return module_list(argc - 1, argv + 1, prefix);\n> +\n> +usage:\n> + usage(\"git submodule--helper module_list\\n\");\n\nAside:\n[bikeshedding on]\n\nI agree with Dscho that these subcommands would be better spelled with\na hyphen than an underscore. If I recall correctly, the arguments for\nusing underscore were (1) a less noisy diff, (2) these are internal\ncommands nobody will be typing anyhow. However, (1) the diff noise\nwill be the same with hyphens, and (2) people will want to test these\ncommands manually anyhow, and its easier to type hyphens and easier to\nremember them since the precedent for hyphens in command-names is\nalready well established.\n\nAlso, the reason that the original shell code used underscores was\nbecause hyphens are not valid characters in shell function names, but\nthat's an implementation detail which shouldn't be allowed to bleed\nover to user-facing interface design (and these subcommands are\nuser-facing).\n\n[bikeshedding off]\n"},{"id":"269060","messageId":"CAPig+cT5Z0CJVok8Za=fBarQLu4WQD3TbA7Wr1aASP4neLLXRA@mail.gmail.com","threadId":"40232","inReplyTo":"1441048767-29729-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/3] submodule: implement `module_name` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T22:01:40Z","receivedAt":"2015-08-31T22:01:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller wrote:\n> This implements the helper `module_name` in C instead of shell,\n> yielding a nice performance boost.\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index beaab7d..c8f7e0c 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -101,6 +104,26 @@ 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 char *name;\n> + const struct submodule *sub;\n> +\n> + if (argc != 1)\n> + usage(\"git submodule--helper module_name \\n\");\n> +\n> + gitmodules_config();\n> + sub = submodule_from_path(null_sha1, argv[0]);\n> +\n> + if (!sub)\n> + die(\"No submodule mapping found in .gitmodules for path '%s'\", argv[0]);\n\nIn the original shell code, this error message went through\neval_gettext(), so don't you want:\n\n die(_(\"No ...\"), ...);\n\n?\n\n> + name = sub->name;\n> + printf(\"%s\\n\", name);\n\nWhy the useless assignment to 'name'? Instead:\n\n printf(\"%s\\n\", sub->name);\n\n> + return 0;\n> +}\n> +\n> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> {\n> if (argc < 2)\n> @@ -109,6 +132,9 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> if (!strcmp(argv[1], \"module_list\"))\n> return module_list(argc - 1, argv + 1, prefix);\n>\n> + if (!strcmp(argv[1], \"module_name\"))\n> + return module_name(argc - 2, argv + 2, prefix);\n> +\n> usage:\n> - usage(\"git submodule--helper module_list\\n\");\n> + usage(\"git submodule--helper [module_list | module_name]\\n\");\n> }\n"},{"id":"269064","messageId":"CAPig+cQVe+qmTsqqdnTXDABxNkQoeEqz07pP2pYbY_n11XMDWA@mail.gmail.com","threadId":"40232","inReplyTo":"1441048767-29729-4-git-send-email-sbeller@google.com","subject":"Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-31T22:35:57Z","receivedAt":"2015-08-31T22:35:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 3:19 PM, Stefan Beller wrote:\n> `module_clone` is part of the update command,\n> which I want to convert to C next.\n\nHmm, place commentary below \"---\".\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index c8f7e0c..d29499c 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -124,6 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix)\n> return 0;\n> }\n>\n> +static int clone_submodule(const char *path, const char *gitdir, const char *url,\n> + const char *depth, const char *reference, int quiet)\n> +{\n> + struct child_process cp;\n> + child_process_init(&cp);\n> +\n> + argv_array_push(&cp.args, \"clone\");\n> + argv_array_push(&cp.args, \"--no-checkout\");\n> + if (quiet)\n> + argv_array_push(&cp.args, \"--quiet\");\n> + if (depth && strcmp(depth, \"\")) {\n\nMore idiomatic:\n\n if (depth && *depth) {\n\n> + argv_array_push(&cp.args, \"--depth\");\n> + argv_array_push(&cp.args, depth);\n\nThese would be easier to read if you consolidate them with argv_array_pushl():\n\n argv_array_pushl(&cp.args, \"--depth\", depth, NULL);\n\nand it would allow you to drop the noisy braces.\n\n> + }\n> + if (reference && strcmp(reference, \"\")) {\n\n if (reference && *reference) {\n\n> + argv_array_push(&cp.args, \"--reference\");\n> + argv_array_push(&cp.args, reference);\n\n argv_array_pushl(&cp.args, \"--reference\", reference, NULL);\n\n> + }\n> + if (gitdir) {\n\nWhy is this case different than the others, in that it doesn't check\nfor the zero-length string \"\"?\n\n> + argv_array_push(&cp.args, \"--separate-git-dir\");\n> + argv_array_push(&cp.args, gitdir);\n\n argv_array_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n\n> + }\n> + argv_array_push(&cp.args, url);\n> + argv_array_push(&cp.args, path);\n> +\n> + cp.git_cmd = 1;\n> + cp.env = local_repo_env;\n> +\n> + cp.no_stdin = 1;\n> + cp.no_stdout = 1;\n> + cp.no_stderr = 1;\n> +\n> + return run_command(&cp);\n> +}\n> +\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> + int quiet = 0;\n> + FILE *submodule_dot_git;\n> + const char *sm_gitdir, *p;\n\nWhy is 'sm_gitdir' declared const even though it's always pointing at\nmemory which this function owns and needs to free? If you drop the\n'const', then you won't have to cast it to non-const in a couple\nplaces below.\n\n> + struct strbuf rel_path = STRBUF_INIT;\n> + struct strbuf sb = STRBUF_INIT;\n> +\n> + struct option module_update_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_END()\n> + };\n> +\n> + static const char * const git_submodule_helper_usage[] = {\n\nYou can drop this 'static'.\n\n> + N_(\"git submodule--helper update [--prefix=] [--quiet] [--remote] [-N|--no-fetch]\"\n> + \"[-f|--force] [--rebase|--merge] [--reference ]\"\n> + \"[--depth ] [--recursive] [--] [...]\"),\n> + NULL\n> + };\n> +\n> + argc = parse_options(argc, argv, prefix, module_update_options,\n> + git_submodule_helper_usage, 0);\n> +\n> + if (getenv(\"GIT_QUIET\"))\n> + quiet = 1;\n\nI understand that you're simply replicating the behavior of the shell\ncode, but this is \"yuck\".\n\n'module_clone' is only called from two places in git-submodule.sh, so\nhow about a cleanup patch which makes 'module_clone' accept a --quiet\nflag and have the two callers pass it explicitly? Finally, have this C\nreplacement accept --quiet as a proper command-line option rather than\nvia this hidden backdoor method.\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> + safe_create_leading_directories_const(sm_gitdir);\n\nWhy is it that the return value of\nsafe_create_leading_directories_const() is sometimes checked in this\nfunction and sometimes not?\n\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> + } else {\n> + safe_create_leading_directories_const(path);\n> + unlink(sm_gitdir);\n\nCheck the return value of unlink()?\n\n> + }\n> +\n> + /* Write a .git file in the submodule to redirect to the superproject. */\n> + if (alternative_path && !strcmp(alternative_path, \"\")) {\n\nMaybe?\n\n if (alternative_path && !*alternative_path) {\n\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\nWhy the mix of capitalized and lowercase-only error messages in this function?\n\nAlso, the original shell code consistently uses eval_gettext(), so\n_(\"...\") seems warranted.\n\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> +\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 superprojects config */\n\ns/superprojects/superproject's/\n\n> + if (!is_absolute_path(sm_gitdir)) {\n> + char *s = (char*)sm_gitdir;\n> + if (strbuf_getcwd(&sb))\n> + die_errno(\"unable to get current working directory\");\n> + strbuf_addf(&sb, \"/%s\", sm_gitdir);\n> + sm_gitdir = strbuf_detach(&sb, NULL);\n> + free(s);\n\nWhy squirrel-away 'sm_gitdir' in 's' unnecessarily? Equivalent:\n\n if (strbuf_getcwd(&sb))\n die_errno(...);\n strbuf_addf(&sb, \"/%s, sm_gitdir);\n free(sm_gitdir);\n sm_gitdir = strbuf_detach(&sb, NULL);\n\n> + }\n> +\n> + if (strbuf_getcwd(&sb))\n> + die_errno(\"unable to get current working directory\");\n\nThe conditional block just above here also gets 'cwd'. If you move\nthis code above the !is_absolute_path(sm_gitdir) conditional block,\nthen you can avoid the duplication.\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> + free((char *)sm_gitdir);\n> + return 0;\n> +}\n> +\n> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> {\n> if (argc < 2)\n> @@ -135,6 +277,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> if (!strcmp(argv[1], \"module_name\"))\n> return module_name(argc - 2, argv + 2, prefix);\n>\n> + if (!strcmp(argv[1], \"module_clone\"))\n> + return module_clone(argc - 1, argv + 1, prefix);\n> +\n> usage:\n> - usage(\"git submodule--helper [module_list | module_name]\\n\");\n> + usage(\"git submodule--helper [module_list | module_name | \"\n> + \"module_clone]\\n\");\n> }\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index e6ff38d..fb5155e 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -178,80 +178,6 @@ get_submodule_config () {\n> printf '%s' \"${value:-$default}\"\n> }\n"},{"id":"269067","messageId":"1441068029-19158-1-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"xmqq1tejuscs.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv3 0/3] submodule--helper: Have some refactoring only patches first","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T00:40:26Z","receivedAt":"2015-09-01T00:40:26Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"* Implemented different error messages as suggested by Junio\n* Implemented all of Erics suggestions,\n including renaming to module-with-dash-now\n\nStefan Beller (3):\n submodule: implement `module-list` as a builtin helper\n submodule: implement `module-name` as a builtin helper\n submodule: implement `module-clone` as a builtin helper\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\n-- \n2.5.0.256.gb62b30d.dirty\n"},{"id":"269068","messageId":"1441068029-19158-2-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"1441068029-19158-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 1/3] submodule: implement `module-list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T00:40:27Z","receivedAt":"2015-09-01T00:40: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 \n---\n .gitignore | 1 +\n Makefile | 1 +\n builtin.h | 1 +\n builtin/submodule--helper.c | 117 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 54 +++-----------------\n git.c | 1 +\n 6 files changed, 127 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..44310f5\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,117 @@\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 module_list [--prefix=] [...]\"),\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+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc < 2)\n+\t\tdie(N_(\"fatal: submodule--helper subcommand must be called with \"\n+\t\t \"a subcommand, which is module-list\\n\"));\n+\n+\tif (!strcmp(argv[1], \"module-list\"))\n+\t\treturn module_list(argc - 1, argv + 1, prefix);\n+\n+\tdie(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n+\t \"which is module-list\\n\"),\n+\t argv[1]);\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..d4d710c 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 () {\n-\t\tif (/^unmatched pathspec/) {\n-\t\t\t$unmatched = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tchomp;\n-\t\tmy ($mode, $sha1, $stage, $path) =\n-\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n-\t\tnext unless $mode eq \"160000\";\n-\t\tif ($stage ne \"0\") {\n-\t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n-\t\t\t}\n-\t\t\tnext;\n-\t\t}\n-\t\tpush @out, \"$_\\n\";\n-\t}\n-\tif ($unmatched) {\n-\t\tprint \"#unmatched\\n\";\n-\t} else {\n-\t\tprint for (@out);\n-\t}\n-\t'\n-}\n-\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n@@ -532,7 +490,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tmodule_list |\n+\tgit submodule--helper module-list --prefix \"$wt_prefix\"|\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -592,7 +550,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -674,7 +632,7 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -790,7 +748,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tmodule_list \"$@\" | {\n+\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -1222,7 +1180,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -1299,7 +1257,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module-list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\ndiff --git a/git.c b/git.c\nindex 55c327c..deecba0 100644\n--- a/git.c\n+++ b/git.c\n@@ -469,6 +469,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n+\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n \t{ \"tag\", cmd_tag, RUN_SETUP },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n-- \n2.5.0.256.gb62b30d.dirty\n"},{"id":"269069","messageId":"1441068029-19158-3-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"1441068029-19158-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 2/3] submodule: implement `module-name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T00:40:28Z","receivedAt":"2015-09-01T00:40: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 \n---\n builtin/submodule--helper.c | 29 +++++++++++++++++++++++++++--\n git-submodule.sh | 32 +++++++-------------------------\n 2 files changed, 34 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 44310f5..91bd420 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,16 +105,38 @@ 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 != 1)\n+\t\tusage(\"git submodule--helper module_name \\n\");\n+\n+\tgitmodules_config();\n+\tsub = submodule_from_path(null_sha1, argv[0]);\n+\n+\tif (!sub)\n+\t\tdie(N_(\"No submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t argv[0]);\n+\n+\tprintf(\"%s\\n\", sub->name);\n+\n+\treturn 0;\n+}\n+\n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\n \tif (argc < 2)\n \t\tdie(N_(\"fatal: submodule--helper subcommand must be called with \"\n-\t\t \"a subcommand, which is module-list\\n\"));\n+\t\t \"a subcommand, which are module-list, module-name\\n\"));\n \n \tif (!strcmp(argv[1], \"module-list\"))\n \t\treturn module_list(argc - 1, argv + 1, prefix);\n \n+\tif (!strcmp(argv[1], \"module-name\"))\n+\t\treturn module_name(argc - 2, argv + 2, prefix);\n+\n \tdie(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n-\t \"which is module-list\\n\"),\n+\t \"which are module-list, module-name\\n\"),\n \t argv[1]);\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex d4d710c..5f3dfc5 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..path = $1\" defined in .gitmodules file?\n-\tsm_path=\"$1\"\n-\tre=$(printf '%s\\n' \"$1\" | sed -e 's/[].[^$\\\\*]/\\\\&/g')\n-\tname=$( git config -f .gitmodules --get-regexp '^submodule\\..*\\.path$' |\n-\t\tsed -n -e 's|^submodule\\.\\(.*\\)\\.path '\"$re\"'$|\\1|p' )\n-\ttest -z \"$name\" &&\n-\tdie \"$(eval_gettext \"No submodule mapping found in .gitmodules for path '\\$sm_path'\")\"\n-\tprintf '%s\\n' \"$name\"\n-}\n-\n #\n # Clone a submodule\n #\n@@ -498,7 +480,7 @@ cmd_foreach()\n \t\tthen\n \t\t\tdisplaypath=$(relative_path \"$sm_path\")\n \t\t\tsay \"$(eval_gettext \"Entering '\\$prefix\\$displaypath'\")\"\n-\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\tname=$(git submodule--helper module-name \"$sm_path\")\n \t\t\t(\n \t\t\t\tprefix=\"$prefix$sm_path/\"\n \t\t\t\tclear_local_git_env\n@@ -554,7 +536,7 @@ cmd_init()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -636,7 +618,7 @@ cmd_deinit()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(relative_path \"$sm_path\")\n \n@@ -758,7 +740,7 @@ cmd_update()\n \t\t\techo >&2 \"Skipping unmerged submodule $prefix$sm_path\"\n \t\t\tcontinue\n \t\tfi\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tbranch=$(get_submodule_config \"$name\" branch master)\n \t\tif ! test -z \"$update\"\n@@ -1022,7 +1004,7 @@ cmd_summary() {\n \t\t\t# Respect the ignore setting for --for-status.\n \t\t\tif test -n \"$for_status\"\n \t\t\tthen\n-\t\t\t\tname=$(module_name \"$sm_path\")\n+\t\t\t\tname=$(git submodule--helper module-name \"$sm_path\")\n \t\t\t\tignore_config=$(get_submodule_config \"$name\" ignore none)\n \t\t\t\ttest $status != A && test $ignore_config = all && continue\n \t\t\tfi\n@@ -1184,7 +1166,7 @@ cmd_status()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\") || exit\n+\t\tname=$(git submodule--helper module-name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=$(relative_path \"$prefix$sm_path\")\n \t\tif test \"$stage\" = U\n@@ -1261,7 +1243,7 @@ cmd_sync()\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n-\t\tname=$(module_name \"$sm_path\")\n+\t\tname=$(git submodule--helper module-name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \n \t\t# Possibly a url relative to parent\n-- \n2.5.0.256.gb62b30d.dirty\n"},{"id":"269070","messageId":"1441068029-19158-4-git-send-email-sbeller@google.com","threadId":"40232","inReplyTo":"1441068029-19158-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T00:40:29Z","receivedAt":"2015-09-01T00:40: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 \n---\n builtin/submodule--helper.c | 144 +++++++++++++++++++++++++++++++++++++++++++-\n git-submodule.sh | 80 +-----------------------\n 2 files changed, 145 insertions(+), 79 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 91bd420..0669641 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@@ -124,11 +125,147 @@ static int module_name(int argc, const char **argv, const char *prefix)\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+\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_update_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+\tstatic const char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper update [--prefix=] [--quiet] [--remote] [-N|--no-fetch]\"\n+\t\t \"[-f|--force] [--rebase|--merge] [--reference ]\"\n+\t\t \"[--depth ] [--recursive] [--] [...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_update_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 (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\tsafe_create_leading_directories_const(path);\n+\t\tunlink(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+\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 superprojects config */\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\tstrbuf_addf(&sb, \"/%s\", 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+\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 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n {\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\"));\n+\t\t \"a subcommand, which are module-list, module-name, \"\n+\t\t \"module-clone\\n\"));\n \n \tif (!strcmp(argv[1], \"module-list\"))\n \t\treturn module_list(argc - 1, argv + 1, prefix);\n@@ -136,7 +273,10 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[1], \"module-name\"))\n \t\treturn module_name(argc - 2, argv + 2, prefix);\n \n+\tif (!strcmp(argv[1], \"module-clone\"))\n+\t\treturn module_clone(argc - 1, argv + 1, prefix);\n+\n \tdie(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n-\t \"which are module-list, module-name\\n\"),\n+\t \"which are module-list, module-name, module-clone\\n\"),\n \t argv[1]);\n }\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 5f3dfc5..d1523ea 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 module-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 module-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.gb62b30d.dirty\n"},{"id":"269071","messageId":"CAPig+cQ33MXQAVbGb2c5ceQMe=CTtMZQmNWdvZfctE3QHuoEeA@mail.gmail.com","threadId":"40232","inReplyTo":"1441068029-19158-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 0/3] submodule--helper: Have some refactoring only patches first","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T02:09:49Z","receivedAt":"2015-09-01T02:09:49Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:\n> * Implemented different error messages as suggested by Junio\n> * Implemented all of Erics suggestions,\n> including renaming to module-with-dash-now\n\nIn the future, please include an interdiff showing changes between\nversions. Thanks.\n\n> Stefan Beller (3):\n> submodule: implement `module-list` as a builtin helper\n> submodule: implement `module-name` as a builtin helper\n> submodule: implement `module-clone` as a builtin helper\n"},{"id":"269076","messageId":"CAPig+cSF=xM5YYh_pzGFtowpSTjkX9A-ENbmsBMX7gkB8nMU4g@mail.gmail.com","threadId":"40232","inReplyTo":"1441068029-19158-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 1/3] submodule: implement `module-list` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T02:22:40Z","receivedAt":"2015-09-01T02:22:40Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller 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> git-submodule.sh, similar to the builtin commands, will navigate\n> to the top-most directory of the repository and keep the\n> subdirectory as a variable. As the helper is called from\n> within the git-submodule.sh script, we are already navigated\n> to the root level, but the path arguments are still relative\n> to the subdirectory we were in when calling git-submodule.sh.\n> That's why there is a `--prefix` option pointing to an alternative\n> path which to anchor relative path arguments.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> new file mode 100644\n> index 0000000..44310f5\n> --- /dev/null\n> +++ b/builtin/submodule--helper.c\n> +static int module_list_compute(int argc, const char **argv,\n> + const char *prefix,\n> + struct pathspec *pathspec)\n> +{\n> + int i, result = 0;\n> + char *max_prefix, *ps_matched = NULL;\n> + int max_prefix_len;\n> + parse_pathspec(pathspec, 0,\n> + PATHSPEC_PREFER_FULL |\n> + PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n> + prefix, argv);\n> +\n> + /* Find common prefix for all pathspec's */\n> + max_prefix = common_prefix(pathspec);\n> + max_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> +\n> + if (pathspec->nr)\n> + ps_matched = xcalloc(pathspec->nr, 1);\n> +\n> + if (read_cache() < 0)\n> + die(\"index file corrupt\");\n\ndie(_(\"...\"));\n\n> + for (i = 0; i < active_nr; i++) {\n> + const struct cache_entry *ce = active_cache[i];\n> +\n> + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n> + max_prefix_len, ps_matched,\n> + S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))\n> + continue;\n> +\n> + if (S_ISGITLINK(ce->ce_mode)) {\n> + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n> + ce_entries[ce_used++] = ce;\n> + }\n> +\n> + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))\n> + /*\n> + * Skip entries with the same name in different stages\n> + * to make sure an entry is returned only once.\n> + */\n> + i++;\n> + }\n> + free(max_prefix);\n> +\n> + if (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n> + result = -1;\n> +\n> + free(ps_matched);\n> +\n> + return result;\n> +}\n> +\n> +static int module_list(int argc, const char **argv, const char *prefix)\n> +{\n> + int i;\n> + struct pathspec pathspec;\n> + const char *alternative_path;\n> +\n> + struct option module_list_options[] = {\n> + OPT_STRING(0, \"prefix\", &alternative_path,\n> + N_(\"path\"),\n> + N_(\"alternative anchor for relative paths\")),\n> + OPT_END()\n> + };\n> +\n> + const char *const git_submodule_helper_usage[] = {\n> + N_(\"git submodule--helper module_list [--prefix=] [...]\"),\n\ns/module_list/module-list/\n\n> + NULL\n> + };\n> +\n> + argc = parse_options(argc, argv, prefix, module_list_options,\n> + git_submodule_helper_usage, 0);\n> +\n> + if (module_list_compute(argc, argv, alternative_path\n> + ? alternative_path\n> + : prefix, &pathspec) < 0) {\n> + printf(\"#unmatched\\n\");\n> + return 1;\n> + }\n> +\n> + for (i = 0; i < ce_used; i++) {\n> + const struct cache_entry *ce = ce_entries[i];\n> +\n> + if (ce_stage(ce))\n> + printf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n> + else\n> + printf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n> +\n> + utf8_fprintf(stdout, \"%s\\n\", ce->name);\n> + }\n> + return 0;\n> +}\n> +\n> +int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> +{\n> + if (argc < 2)\n> + die(N_(\"fatal: submodule--helper subcommand must be called with \"\n> + \"a subcommand, which is module-list\\n\"));\n> +\n> + if (!strcmp(argv[1], \"module-list\"))\n\nCan we drop the \"module-\" prefix altogether from these subcommands,\nplease? Considering that the parent name is already\n\"submodule--helper\", the \"module-\" prefix is probably pure redundancy.\nInstead:\n\n submodule--helper list\n submodule--helper name\n submodule--helper clone\n\n> + return module_list(argc - 1, argv + 1, prefix);\n> +\n> + die(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n> + \"which is module-list\\n\"),\n> + argv[1]);\n> +}\n"},{"id":"269077","messageId":"CAPig+cTTiW_tZBNoz-kj6CkViR+MFegv1Tz5xmVvbGxZ0pbqwQ@mail.gmail.com","threadId":"40232","inReplyTo":"1441068029-19158-3-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T02:31:15Z","receivedAt":"2015-09-01T02:31:15Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:\n> This implements the helper `module_name` in C instead of shell,\n> yielding a nice performance boost.\n>\n> Before this patch, I measured a time (best out of three):\n>\n> $ time ./t7400-submodule-basic.sh >/dev/null\n> real 0m11.066s\n> user 0m3.348s\n> sys 0m8.534s\n>\n> With this patch applied I measured (also best out of three)\n>\n> $ time ./t7400-submodule-basic.sh >/dev/null\n> real 0m10.063s\n> user 0m3.044s\n> sys 0m7.487s\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 44310f5..91bd420 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -102,16 +105,38 @@ 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 != 1)\n> + usage(\"git submodule--helper module_name \\n\");\n\nusage(_(\"...\"));\n\ns/module_name/module-name/\n\nI think you also need to drop the newline from the usage() argument.\n\n> + gitmodules_config();\n> + sub = submodule_from_path(null_sha1, argv[0]);\n> +\n> + if (!sub)\n> + die(N_(\"No submodule mapping found in .gitmodules for path '%s'\"),\n> + argv[0]);\n> +\n> + printf(\"%s\\n\", sub->name);\n> +\n> + return 0;\n> +}\n> +\n> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n> {\n> if (argc < 2)\n> die(N_(\"fatal: submodule--helper subcommand must be called with \"\n> - \"a subcommand, which is module-list\\n\"));\n> + \"a subcommand, which are module-list, module-name\\n\"));\n\nManually maintaining this list is likely to become a maintenance issue\npretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?\n\nAlso, it would like be more readable if the possible commands were\nprinted one per line rather than all squashed together.\n\n> if (!strcmp(argv[1], \"module-list\"))\n> return module_list(argc - 1, argv + 1, prefix);\n>\n> + if (!strcmp(argv[1], \"module-name\"))\n> + return module_name(argc - 2, argv + 2, prefix);\n> +\n> die(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n> - \"which is module-list\\n\"),\n> + \"which are module-list, module-name\\n\"),\n\nAnd, it's duplicated here, making it even more of a maintenance problem.\n\n> argv[1]);\n> }\n"},{"id":"269078","messageId":"CAPig+cRgSb5bD-pOH1rQ-ES61-aPKktXz8-LX-sB9kCqLrCWaw@mail.gmail.com","threadId":"40232","inReplyTo":"1441068029-19158-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 3/3] submodule: implement `module-clone` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T02:41:21Z","receivedAt":"2015-09-01T02:41:21Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:\n> This converts implements the helper `module_clone`. This functionality is\n\n\"converts implements\"?\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 \n> ---\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_update_options[] = {\n\ns/update/clone/ maybe?\n\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> + static const char * const git_submodule_helper_usage[] = {\n\nYou can still drop 'static'.\n\n> + N_(\"git submodule--helper update [--prefix=] [--quiet] [--remote] [-N|--no-fetch]\"\n\n\"update\"?\n\n> + \"[-f|--force] [--rebase|--merge] [--reference ]\"\n> + \"[--depth ] [--recursive] [--] [...]\"),\n\nAlso, what's all this \"--force\", \"--rebase\", \"--remote\", \"--recursive\" stuff?\n\n> + NULL\n> + };\n> +\n> + argc = parse_options(argc, argv, prefix, module_update_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> + safe_create_leading_directories_const(sm_gitdir);\n\nStill inconsistent checking of return value of\nsafe_create_leading_directories_const() in this function.\n\nIn fact, it looks like pretty much all of my v2 review comments are\nstill relevant to the remainder of this function. Rather than\nrepeating them, I'll just give a reference[1].\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/276952\n\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> + } else {\n> + safe_create_leading_directories_const(path);\n> + unlink(sm_gitdir);\n> + }\n> +\n> + /* Write a .git file in the submodule to redirect to the superproject. */\n> + if (alternative_path && !strcmp(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> +\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 superprojects config */\n> + if (!is_absolute_path(sm_gitdir)) {\n> + char *s = (char*)sm_gitdir;\n> + if (strbuf_getcwd(&sb))\n> + die_errno(\"unable to get current working directory\");\n> + strbuf_addf(&sb, \"/%s\", sm_gitdir);\n> + sm_gitdir = strbuf_detach(&sb, NULL);\n> + free(s);\n> + }\n> +\n> + if (strbuf_getcwd(&sb))\n> + die_errno(\"unable to get current working directory\");\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> + free(sm_gitdir);\n> + return 0;\n> +}\n"},{"id":"269099","messageId":"CAGZ79kYAEic2kNmK10xB5g2JArpyqcqxNWjTBUkx6DfG3a_zpg@mail.gmail.com","threadId":"40232","inReplyTo":"CAPig+cTTiW_tZBNoz-kj6CkViR+MFegv1Tz5xmVvbGxZ0pbqwQ@mail.gmail.com","subject":"Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T16:18:53Z","receivedAt":"2015-09-01T16:18:53Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 31, 2015 at 7:31 PM, Eric Sunshine wrote:\n> On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:\n>> This implements the helper `module_name` in C instead of shell,\n>> yielding a nice performance boost.\n>>\n>> Before this patch, I measured a time (best out of three):\n>>\n>> $ time ./t7400-submodule-basic.sh >/dev/null\n>> real 0m11.066s\n>> user 0m3.348s\n>> sys 0m8.534s\n>>\n>> With this patch applied I measured (also best out of three)\n>>\n>> $ time ./t7400-submodule-basic.sh >/dev/null\n>> real 0m10.063s\n>> user 0m3.044s\n>> sys 0m7.487s\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 44310f5..91bd420 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -102,16 +105,38 @@ 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 != 1)\n>> + usage(\"git submodule--helper module_name \\n\");\n>\n> usage(_(\"...\"));\n>\n> s/module_name/module-name/\n>\n> I think you also need to drop the newline from the usage() argument.\n>\n>> + gitmodules_config();\n>> + sub = submodule_from_path(null_sha1, argv[0]);\n>> +\n>> + if (!sub)\n>> + die(N_(\"No submodule mapping found in .gitmodules for path '%s'\"),\n>> + argv[0]);\n>> +\n>> + printf(\"%s\\n\", sub->name);\n>> +\n>> + return 0;\n>> +}\n>> +\n>> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n>> {\n>> if (argc < 2)\n>> die(N_(\"fatal: submodule--helper subcommand must be called with \"\n>> - \"a subcommand, which is module-list\\n\"));\n>> + \"a subcommand, which are module-list, module-name\\n\"));\n>\n> Manually maintaining this list is likely to become a maintenance issue\n> pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?\n\nIf we were using OPT_CMDMODE, we'd need to have all the options in\none array, passing it to the option parser. I don't think we could have\n2 layers of option parsing here. If the first layer only contains the\ncheck for argv[1] (list, name, clone), then it would fail not knowing the\narguments for each of these sub commands.\n\nIf we parse all the options together, then we would need to have lots of\n\n if (cmd == name)\n die (\"it makes no sense to give --reference or --depth\");\n\nalso the variables where the options are passed into would not be\nfunction specific, but file global.\n\n>\n> Also, it would like be more readable if the possible commands were\n> printed one per line rather than all squashed together.\n>\n>> if (!strcmp(argv[1], \"module-list\"))\n>> return module_list(argc - 1, argv + 1, prefix);\n>>\n>> + if (!strcmp(argv[1], \"module-name\"))\n>> + return module_name(argc - 2, argv + 2, prefix);\n>> +\n>> die(N_(\"fatal: '%s' is not a valid submodule--helper subcommand, \"\n>> - \"which is module-list\\n\"),\n>> + \"which are module-list, module-name\\n\"),\n>\n> And, it's duplicated here, making it even more of a maintenance problem.\n>\n>> argv[1]);\n>> }\n\nMaybe we can do it similar to git.c to have our own array with name\nand function pointers,\nwhich can be used both in the help text as well as the actual function call.\n"},{"id":"269114","messageId":"CAGZ79kYDYO-q6QEmRQ4YW4ED2HAmMjZNyRaY52ix6YU1=WVNHw@mail.gmail.com","threadId":"40232","inReplyTo":"CAPig+cQVe+qmTsqqdnTXDABxNkQoeEqz07pP2pYbY_n11XMDWA@mail.gmail.com","subject":"Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-09-01T17:49:41Z","receivedAt":"2015-09-01T17:49:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"took all suggestions except the one below.\n\n>\n> if (strbuf_getcwd(&sb))\n> die_errno(...);\n> strbuf_addf(&sb, \"/%s, sm_gitdir);\n> free(sm_gitdir);\n> sm_gitdir = strbuf_detach(&sb, NULL);\n>\n>> + }\n>> +\n>> + if (strbuf_getcwd(&sb))\n>> + die_errno(\"unable to get current working directory\");\n>\n> The conditional block just above here also gets 'cwd'. If you move\n> this code above the !is_absolute_path(sm_gitdir) conditional block,\n> then you can avoid the duplication.\n\nI don't get it. We need to have strbuf_getcwd twice no matter how we\narrange the code\nas we strbuf_detach will empty the strbuf.\n\nDo you mean to call strbuf_getcwd just once and then xstrdup the value,\nthen reset the strbuf to just contain the cwd and append the other string ?\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>> + free((char *)sm_gitdir);\n>> + return 0;\n>> +}\n"},{"id":"269137","messageId":"CAPig+cSZ9et=7DNF15MJo-Ef5o1bjbLxAoTgxJV6q1BEUiBGcA@mail.gmail.com","threadId":"40232","inReplyTo":"CAGZ79kYDYO-q6QEmRQ4YW4ED2HAmMjZNyRaY52ix6YU1=WVNHw@mail.gmail.com","subject":"Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-01T18:35:17Z","receivedAt":"2015-09-01T18:35:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 1, 2015 at 1:49 PM, Stefan Beller wrote:\n> took all suggestions except the one below.\n>\n>>\n>> if (strbuf_getcwd(&sb))\n>> die_errno(...);\n>> strbuf_addf(&sb, \"/%s, sm_gitdir);\n>> free(sm_gitdir);\n>> sm_gitdir = strbuf_detach(&sb, NULL);\n>>\n>>> + }\n>>> +\n>>> + if (strbuf_getcwd(&sb))\n>>> + die_errno(\"unable to get current working directory\");\n>>\n>> The conditional block just above here also gets 'cwd'. If you move\n>> this code above the !is_absolute_path(sm_gitdir) conditional block,\n>> then you can avoid the duplication.\n>\n> I don't get it. We need to have strbuf_getcwd twice no matter how we\n> arrange the code\n> as we strbuf_detach will empty the strbuf.\n>\n> Do you mean to call strbuf_getcwd just once and then xstrdup the value,\n> then reset the strbuf to just contain the cwd and append the other string ?\n\nSorry, I have no idea what you are asking.\n\nAll I am saying is that you're unnecessarily invoking getcwd() twice.\nIt's value doesn't change between invocations, so the second call is\nentirely redundant and wasteful. You can instead call it just once and\nuse the result in both places.\n"}]}