{"thread":{"id":"39975","subject":"[PATCH] add: remove dead code","startedAt":"2015-07-31T00:19:16Z","lastAt":"2015-08-03T22:58:08Z","messageCount":15,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"267182","messageId":"1438301956-1658-1-git-send-email-sbeller@google.com","threadId":"39975","inReplyTo":null,"subject":"[PATCH] add: remove dead code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-07-31T00:19:16Z","receivedAt":"2015-07-31T00:19:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n\nSo I was trying to understand how to implement \"git add .gitmodules\" as \nI intend to rewrite git submodules in C.\n\n builtin/add.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 4bd98b7..b2a5c57 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -375,7 +375,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tif (add_new_files) {\n \t\tint baselen;\n-\t\tstruct pathspec empty_pathspec;\n \n \t\t/* Set up the default git porcelain excludes */\n \t\tmemset(&dir, 0, sizeof(dir));\n@@ -384,7 +383,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\tsetup_standard_excludes(&dir);\n \t\t}\n \n-\t\tmemset(&empty_pathspec, 0, sizeof(empty_pathspec));\n \t\t/* This picks up the paths that are not tracked */\n \t\tbaselen = fill_directory(&dir, &pathspec);\n \t\tif (pathspec.nr)\n-- \n2.5.0.rc1.391.g15b60ce\n"},{"id":"267202","messageId":"xmqqr3no5lfi.fsf@gitster.dls.corp.google.com","threadId":"39975","inReplyTo":"1438301956-1658-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] add: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-31T16:41:37Z","receivedAt":"2015-07-31T16:41:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> So I was trying to understand how to implement \"git add .gitmodules\" as \n> I intend to rewrite git submodules in C.\n\nA request and a suggestion.\n\n - Please keep Jens and Heiko in the loop and pick their brains ;-)\n\n - It may not be the best use of your time to attempt rewriting the\n   whole of \"git submodule\" in C in a single pass (which is the\n   impression I am getting from seeing you say \"git add\n   .gitmodules\").\n\n   The largest pain point a rewrite in C would solve is that \"git\n   submodule init\" and \"update\" have to go sequencially even though\n\n   (1) there is no inherent reason for cloning and fetching of\n       different submodules to happen in some order;\n\n   (2) there is no inherent reason for a failure to clone or fetch\n       of one submodule to abort the entire session without cloning\n       or fetching other submodules; and\n\n   (3) the operation in each submodule takes human-scale time and\n       the users would benefit greatly by parallel operations.\n\n   One approach that may be beneficial would be to introduce \"git\n   submodule--helper\" written in C to have selected primitives used\n   in \"git submodule\" script to speed them up.  Perhaps the first\n   subcommand would be \"\n\n    $ git submodule--helper foreach-parallel --cmd=$cmd $args...\n\n   where it takes the arguments currently fed to module_list as\n   $args... and runs $cmd in parallel.  The initial implementation\n   of \"git submodule update\" then would replace the expensive and\n   sequencial\n\n       module_list \"$@\" | {\n           ...\n           while read mode sha1 stage sm_path\n           do\n           \t...\n\t   done\n       }\n\n   loop with a call to the foreach-parallel subcommand.\n\n   The end-user scripts that currently use \"git submodule foreach\"\n   may or may not depend on the sequencial nature of the current\n   implementation, so adding \"git submodule foreach-parallel\" may be\n   a good way to expose this as a new \"do it in parallel\" feature.\n\n   Once you have a solid infrastructure to implement the helper\n   subcommand \"foreach-parallel\" (I'd expect the interface inside C\n   into that function would be to give it a worker function with the\n   data for the function to consume, and the above --cmd=$cmd form\n   would use a worker function that essentially does run_command();\n   the spawn(2)ing and wait(2)ing would be done on the more generic\n   API side), you can rewrite the $cmd part in C little by little,\n   and eventually you would get a full C implemention while keeping\n   everything working and retaining debuggability during the course\n   of development.\n\nThanks.\n"},{"id":"267237","messageId":"1438384147-3275-1-git-send-email-sbeller@google.com","threadId":"39975","inReplyTo":"1438301956-1658-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 0/2] Submodules: refactoring the `module_list` function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-07-31T23:09:05Z","receivedAt":"2015-07-31T23:09:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi,\n\nso eventually we want to have the whole Android project inside a git repository,\nwhich includes > 500 submodules for the different sub systems. To make that feasable\nwe want to improve git-submodule for a huge set of submodules such as parallelizing\n`git submodule update`. As a first step I was toying around the `module_list` function\nas that is one key component for all the submodule operations. Also that part of the\ncode is where the operations are split up to the different submodules, so that\nwould be the natural place where we'd approach introducing threads later on.\n\nThis is an early RFC as it breaks the test suite. I do not remember ever interacting\nwith pathspecs or cache entries before, so I'd appreciate some guiding comments.\n(Am I holding it wrong? Also the git code base seems large, it's easy to find new\nplaygrounds :)\n\nThanks,\nStefan\n\nStefan Beller (2):\n  submodule: implement `module_list` as a builtin helper\n  Testing the new code\n\n Makefile                    |  1 +\n builtin.h                   |  1 +\n builtin/submodule--helper.c | 99 +++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            | 25 +++++++++++-\n git.c                       |  1 +\n 5 files changed, 126 insertions(+), 1 deletion(-)\n create mode 100644 builtin/submodule--helper.c\n\n-- \n2.5.0.5.gf4cd9ae.dirty\n"},{"id":"267239","messageId":"1438384147-3275-2-git-send-email-sbeller@google.com","threadId":"39975","inReplyTo":"1438384147-3275-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-07-31T23:09:06Z","receivedAt":"2015-07-31T23:09:06Z","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\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Makefile                    |  1 +\n builtin.h                   |  1 +\n builtin/submodule--helper.c | 99 +++++++++++++++++++++++++++++++++++++++++++++\n git.c                       |  1 +\n 4 files changed, 102 insertions(+)\n create mode 100644 builtin/submodule--helper.c\n\ndiff --git a/Makefile b/Makefile\nindex 8c3c724..6fb7484 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -898,6 +898,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 9e04f97..7bf9597 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -118,6 +118,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..2e24fdc\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,99 @@\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+\n+static const char * const git_submodule_helper_usage[] = {\n+\tN_(\"git submodule--helper --module_list [<path>...]\"),\n+\tNULL\n+};\n+\n+int module_list(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstatic struct pathspec pathspec;\n+\tconst struct cache_entry **ce_entries = NULL;\n+\tint alloc = 0, used = 0;\n+\tchar *ps_matched = NULL;\n+\tchar *max_prefix;\n+\tint max_prefix_len;\n+\tstruct string_list already_printed = STRING_LIST_INIT_NODUP;\n+\n+\tparse_pathspec(&pathspec, 0,\n+\t\t       PATHSPEC_PREFER_FULL,\n+\t\t       prefix, argv);\n+\n+\t/* Find common prefix for all pathspec's */\n+\tmax_prefix = common_prefix(&pathspec);\n+\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n+\n+\tif (pathspec.nr)\n+\t\tps_matched = xcalloc(1, pathspec.nr);\n+\n+\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, used + 1, alloc);\n+\t\t\tce_entries[used++] = ce;\n+\t\t}\n+\t}\n+\n+\tif (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor (i = 0; i < used; i++) {\n+\t\tconst struct cache_entry *ce = ce_entries[i];\n+\n+\t\tif (string_list_has_string(&already_printed, ce->name))\n+\t\t\tcontinue;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n+\t\t} else {\n+\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\t\t}\n+\t\twrite_name_quoted(ce->name, stdout, '\\n');\n+\t\tstring_list_insert(&already_printed, ce->name);\n+\t}\n+\treturn 0;\n+}\n+\n+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tenum {\n+\t\tMODE_UNSPECIFIED = 0,\n+\t\tMODE_MODULE_LIST,\n+\t} cmdmode = MODE_UNSPECIFIED;\n+\n+\tstruct option options[] = {\n+\t\tOPT_CMDMODE(0, \"module_list\", &cmdmode,\n+\t\t\t    N_(\"Get the submodule list\"), MODE_MODULE_LIST),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tswitch (cmdmode) {\n+\tcase MODE_MODULE_LIST:\n+\t\treturn module_list(argc, argv, prefix);\n+\t\tbreak;\n+\tcase MODE_UNSPECIFIED:\n+\t\tusage_with_options(git_submodule_helper_usage, options);\n+\t\tbreak;\n+\t}\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex fe94066..721995e 100644\n--- a/git.c\n+++ b/git.c\n@@ -468,6 +468,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.5.gf4cd9ae.dirty\n"},{"id":"267238","messageId":"1438384147-3275-3-git-send-email-sbeller@google.com","threadId":"39975","inReplyTo":"1438384147-3275-1-git-send-email-sbeller@google.com","subject":"[RFC/PATCH 2/2] Testing the new code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-07-31T23:09:07Z","receivedAt":"2015-07-31T23:09:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    The output of the differential testing is below,\n    which points out 2 bugs:\n    * resolving relative paths seems is broken, so we\n      would need to have the equivalent of\n        eval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n      (091a6eb0, 2013-06-16,  submodule: drop the top-level requirement)\n    * The handling of funny characters is broken.\n    \n    ----------------------------------------------\n    ../init\n    160000 9ab41d0d44279924747e3eafa6da77966579aa45 0\tinit\n    ----\n    ----\n    ----\n    fatal: ../init: '../init' is outside repository\n    ----------------------------------------------\n    ../init\n    160000 37b832c3ae4dcc0830b173572ff59be6b089bdcd 0\tinit\n    ----\n    ----\n    ----\n    fatal: ../init: '../init' is outside repository\n    ----------------------------------------------\n    ../init\n    160000 9ab41d0d44279924747e3eafa6da77966579aa45 0\tinit\n    ----\n    ----\n    ----\n    fatal: ../init: '../init' is outside repository\n    ----------------------------------------------\n    \n    160000 37b832c3ae4dcc0830b173572ff59be6b089bdcd 0\texample2\n    160000 9ab41d0d44279924747e3eafa6da77966579aa45 0\tinit\n    160000 01b25b1d44b1049978e053768790100df8b8d9d9 0\tå äö\n    ----\n    160000 37b832c3ae4dcc0830b173572ff59be6b089bdcd 0\texample2\n    160000 9ab41d0d44279924747e3eafa6da77966579aa45 0\tinit\n    160000 01b25b1d44b1049978e053768790100df8b8d9d9 0\t\"\\303\\245 \\303\\244\\303\\266\"\n    ----\n    ----\n\n git-submodule.sh | 25 ++++++++++++++++++++++++-\n 1 file changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..717b74d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -149,7 +149,7 @@ relative_path ()\n # Get submodule info for registered submodules\n # $@ = path to limit submodule list\n #\n-module_list()\n+module_list_shell()\n {\n \teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n \t(\n@@ -187,6 +187,29 @@ module_list()\n \t'\n }\n \n+module_list()\n+{\n+\t# call both the old and new code\n+\tmodule_list_shell $@ >/u/git_submodule_module_list_shell 2>/u/git_submodule_module_list_shell2\n+\tgit submodule--helper --module_list $@ >/u/git_submodule_module_list 2>/u/git_submodule_module_list2\n+\t# if there is a diff record it\n+\tDIFF=$(diff /u/git_submodule_module_list_shell /u/git_submodule_module_list)\n+\tif [ \"$DIFF\" != \"\" ]\n+\tthen\n+\t\techo \"----------------------------------------------\">>/u/git_submodule_module_diffs\n+\t\techo $@ >>/u/git_submodule_module_diffs\n+\t\tcat /u/git_submodule_module_list_shell >>/u/git_submodule_module_diffs\n+\t\techo \"----\" >>/u/git_submodule_module_diffs\n+\t\tcat /u/git_submodule_module_list >>/u/git_submodule_module_diffs\n+\t\techo \"----\" >>/u/git_submodule_module_diffs\n+\t\tcat /u/git_submodule_module_list_shell2 >>/u/git_submodule_module_diffs\n+\t\techo \"----\" >>/u/git_submodule_module_diffs\n+\t\tcat /u/git_submodule_module_list2 >>/u/git_submodule_module_diffs\n+\tfi\n+\t# output to the caller\n+\tcat /u/git_submodule_module_list\n+}\n+\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n-- \n2.5.0.5.gf4cd9ae.dirty\n"},{"id":"267251","messageId":"xmqqmvybakjl.fsf@gitster.dls.corp.google.com","threadId":"39975","inReplyTo":"1438384147-3275-2-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-01T01:01:50Z","receivedAt":"2015-08-01T01:01:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +static const char * const git_submodule_helper_usage[] = {\n> +\tN_(\"git submodule--helper --module_list [<path>...]\"),\n\nYuck.  Please do not force --multi_word_opt upon us, which is simply\ntoo ugly to live around here.  --module-list is perhaps OK, but\nbecause submodule--helper would not have an default action, I'd\nprefer to make these just \"command words\", i.e.\n\n    $ git submodule--helper module_list\n\n> +int module_list(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint i;\n> +\tstatic struct pathspec pathspec;\n> +\tconst struct cache_entry **ce_entries = NULL;\n> +\tint alloc = 0, used = 0;\n> +\tchar *ps_matched = NULL;\n> +\tchar *max_prefix;\n> +\tint max_prefix_len;\n> +\tstruct string_list already_printed = STRING_LIST_INIT_NODUP;\n> +\n> +\tparse_pathspec(&pathspec, 0,\n> +\t\t       PATHSPEC_PREFER_FULL,\n> +\t\t       prefix, argv);\n> +\n> +\t/* Find common prefix for all pathspec's */\n> +\tmax_prefix = common_prefix(&pathspec);\n> +\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> +\n> +\tif (pathspec.nr)\n> +\t\tps_matched = xcalloc(1, pathspec.nr);\n\nUp to this point it interprets its input, and ...\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, used + 1, alloc);\n> +\t\t\tce_entries[used++] = ce;\n> +\t\t}\n> +\t}\n> +\n> +\tif (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n> +\t\tprintf(\"#unmatched\\n\");\n> +\t\treturn 1;\n> +\t}\n\n... does the computation, with diagnosis.\n\nAnd then it does the I/O with formatting.\n\n> +\n> +\tfor (i = 0; i < used; i++) {\n> +\t\tconst struct cache_entry *ce = ce_entries[i];\n...\n> +\treturn 0;\n> +}\n\nWhen you have the implementation of \"foreach-parallel\" to move the\nmost expensive part of \"submodule update\" of a tree with 500\nsubmodules, you would want to receive more or less the same \"args\"\nas this thing takes and pass the ce_entries[] list to the \"spawn and\nrun the user script in them in parallel\" engine.\n\nSo I think it makes more sense to split this function into two (or\nthree).  One that reads from (argc, argv) and allocates and fills\nce_entries[] can become a helper that you can reuse later.  \n\n'int module_list()' (shouldn't it be static?), can make a call to\nthat helper at the begining of it, and the remainder of the function\nwould do the textual I/O.\n"},{"id":"267252","messageId":"xmqqio8zakhv.fsf@gitster.dls.corp.google.com","threadId":"39975","inReplyTo":"1438384147-3275-3-git-send-email-sbeller@google.com","subject":"Re: [RFC/PATCH 2/2] Testing the new code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-01T01:02:52Z","receivedAt":"2015-08-01T01:02:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> -module_list()\n> +module_list_shell()\n>  {\n>  \teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n>  \t(\n> @@ -187,6 +187,29 @@ module_list()\n>  \t'\n>  }\n>  \n> +module_list()\n> +{\n> +\t# call both the old and new code\n> +\tmodule_list_shell $@ >/u/git_submodule_module_list_shell 2>/u/git_submodule_module_list_shell2\n> +\tgit submodule--helper --module_list $@ >/u/git_submodule_module_list 2>/u/git_submodule_module_list2\n\nYou seem to be discarding the double-quote around $@ in both of\nthese two places.  Intended?\n"},{"id":"267316","messageId":"CAGZ79kYBvQ9vfmDNs2_WTLU0OrHShS7_fLnjHgYxX9C29PdDsA@mail.gmail.com","threadId":"39975","inReplyTo":"xmqqio8zakhv.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 2/2] Testing the new code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-03T16:23:11Z","receivedAt":"2015-08-03T16:23:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 31, 2015 at 6:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> -module_list()\n>> +module_list_shell()\n>>  {\n>>       eval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n>>       (\n>> @@ -187,6 +187,29 @@ module_list()\n>>       '\n>>  }\n>>\n>> +module_list()\n>> +{\n>> +     # call both the old and new code\n>> +     module_list_shell $@ >/u/git_submodule_module_list_shell 2>/u/git_submodule_module_list_shell2\n>> +     git submodule--helper --module_list $@ >/u/git_submodule_module_list 2>/u/git_submodule_module_list2\n>\n> You seem to be discarding the double-quote around $@ in both of\n> these two places.  Intended?\n\nNo, not at all. This was a bit sloppy.\nThis patch was rather showing off how I intend to test the previous patch.\nThe idea came from a talk[1] at oscon which presented refactoring code with\nthis differential strategy (except that they used it on a larger code base and\ncollected the diffs between the new and old code for a while in production\ninstead of just relying on the test suite passing).\n\n[1] http://www.oscon.com/open-source-2015/public/schedule/detail/41888\n"},{"id":"267335","messageId":"xmqqoaio888d.fsf@gitster.dls.corp.google.com","threadId":"39975","inReplyTo":"CAGZ79kYBvQ9vfmDNs2_WTLU0OrHShS7_fLnjHgYxX9C29PdDsA@mail.gmail.com","subject":"Re: [RFC/PATCH 2/2] Testing the new code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-03T19:47:30Z","receivedAt":"2015-08-03T19:47:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Jul 31, 2015 at 6:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> -module_list()\n>>> +module_list_shell()\n>>>  {\n>>>       eval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n>>>       (\n>>> @@ -187,6 +187,29 @@ module_list()\n>>>       '\n>>>  }\n>>>\n>>> +module_list()\n>>> +{\n>>> +     # call both the old and new code\n>>> +     module_list_shell $@ >/u/git_submodule_module_list_shell 2>/u/git_submodule_module_list_shell2\n>>> +     git submodule--helper --module_list $@ >/u/git_submodule_module_list 2>/u/git_submodule_module_list2\n>>\n>> You seem to be discarding the double-quote around $@ in both of\n>> these two places.  Intended?\n>\n> No, not at all. This was a bit sloppy.\n\nOK.\n\n> This patch was rather showing off how I intend to test the previous patch.\n\nYeah, I can see what the code is doing, and you already saw that I\ndidn't disagree with the approach ;).  During a reimplementation\nexercise, it often is a good idea, if the code structure allows you\nto, to run both implementations and compare the results---but it can\ngo only so far.  It obviously is tricky to apply the trick to an\noperation that is not idempotent to let two implementations to do it\ntwice in different ways and make sure they produce the same result.\n"},{"id":"267355","messageId":"CAGZ79kY=39j4H=62=VZRm4VOcqzgOAU6tDpJVsqdeqnPtB8hQQ@mail.gmail.com","threadId":"39975","inReplyTo":"xmqqmvybakjl.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-03T21:30:42Z","receivedAt":"2015-08-03T21:30:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 31, 2015 at 6:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +static const char * const git_submodule_helper_usage[] = {\n>> +     N_(\"git submodule--helper --module_list [<path>...]\"),\n>\n> Yuck.  Please do not force --multi_word_opt upon us, which is simply\n> too ugly to live around here.  --module-list is perhaps OK,\n\nI agree there. The way you word it here, it sounds as if the mixture\nof dashes and underscores are a problem.\n\n> but\n> because submodule--helper would not have an default action, I'd\n> prefer to make these just \"command words\", i.e.\n>\n>     $ git submodule--helper module_list\n\nWhy would you use an underscore in here as opposed to a dash?\n     $ git submodule--helper module-list\n\nI went with --module-list for now as I see no reason real to make it\na command word for now as it is not user facing but just a helper.\nI have a patch from my previous attempt to rewrite \"git submodule\"\nas a whole to accept both command words as well as double dashed\nselected modes.\n\n>\n>> +int module_list(int argc, const char **argv, const char *prefix)\n>> +{\n>> +     int i;\n>> +     static struct pathspec pathspec;\n>> +     const struct cache_entry **ce_entries = NULL;\n>> +     int alloc = 0, used = 0;\n>> +     char *ps_matched = NULL;\n>> +     char *max_prefix;\n>> +     int max_prefix_len;\n>> +     struct string_list already_printed = STRING_LIST_INIT_NODUP;\n>> +\n>> +     parse_pathspec(&pathspec, 0,\n>> +                    PATHSPEC_PREFER_FULL,\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(1, pathspec.nr);\n>\n> Up to this point it interprets its input, and ...\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, used + 1, alloc);\n>> +                     ce_entries[used++] = ce;\n>> +             }\n>> +     }\n>> +\n>> +     if (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n>> +             printf(\"#unmatched\\n\");\n>> +             return 1;\n>> +     }\n>\n> ... does the computation, with diagnosis.\n>\n> And then it does the I/O with formatting.\n>\n>> +\n>> +     for (i = 0; i < used; i++) {\n>> +             const struct cache_entry *ce = ce_entries[i];\n> ...\n>> +     return 0;\n>> +}\n>\n> When you have the implementation of \"foreach-parallel\" to move the\n> most expensive part of \"submodule update\" of a tree with 500\n> submodules, you would want to receive more or less the same \"args\"\n> as this thing takes and pass the ce_entries[] list to the \"spawn and\n> run the user script in them in parallel\" engine.\n\nThat's true, I thought about splitting it up later when I actually need it.\n[That seems easier to write, but not easier to review :( ]\nI did split up the function just now.\n\n>\n> So I think it makes more sense to split this function into two (or\n> three).  One that reads from (argc, argv) and allocates and fills\n> ce_entries[] can become a helper that you can reuse later.\n>\n> 'int module_list()' (shouldn't it be static?), can make a call to\n> that helper at the begining of it, and the remainder of the function\n> would do the textual I/O.\n"},{"id":"267356","messageId":"xmqqfv406ojg.fsf@gitster.dls.corp.google.com","threadId":"39975","inReplyTo":"CAGZ79kY=39j4H=62=VZRm4VOcqzgOAU6tDpJVsqdeqnPtB8hQQ@mail.gmail.com","subject":"Re: [RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-03T21:38:11Z","receivedAt":"2015-08-03T21:38:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>     $ git submodule--helper module_list\n>\n> Why would you use an underscore in here as opposed to a dash?\n\nSimply because the diff would be easier to read; the callers used to\ncall module_list shell function, now they call the subcommand with the\nsame name of submodule--helper.\n"},{"id":"267358","messageId":"1438639134-29370-1-git-send-email-sbeller@google.com","threadId":"39975","inReplyTo":"CAGZ79kY=39j4H=62=VZRm4VOcqzgOAU6tDpJVsqdeqnPtB8hQQ@mail.gmail.com","subject":"[PATCH] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-03T21:58:54Z","receivedAt":"2015-08-03T21:58:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Most of the submodule operations work on a set of submodules.\nCalculating and using this set is usually done via:\n\n       module_list \"$@\" | {\n           while read mode sha1 stage sm_path\n           do\n                # the actual operation\n           done\n       }\n\nCurrently the function `module_list` is implemented in the\ngit-submodule.sh as a shell script wrapping a perl script.\nThe rewrite is in C, such that it is faster and can later be\neasily adapted when other functions are rewritten in C.\n\ngit-submodule.sh similar to the builtin commands will navigate\nto the top most directory of the repository and keeping the\nsubdirectories as a variable. As the helper is called from\nwithin the git-submodule.sh script, we are already navigated\nto the root level, but the path arguments are stil relative\nto the subdirectory we were in when calling git-submodule.sh.\nThat's why there is a `--prefix` option pointing to an alternative\npath where to anchor relative path arguments.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis doesn't have the `module_list` subcommand word,\nbut rather uses the `--module-list` mode for now.\n\nHowever this is not an RFC any more, but I consider it stable.\n\nThanks,\nStefan\n\n Makefile                    |   1 +\n builtin.h                   |   1 +\n builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  54 +++-----------------\n git.c                       |   1 +\n 5 files changed, 128 insertions(+), 48 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\ndiff --git a/Makefile b/Makefile\nindex 8c3c724..6fb7484 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -898,6 +898,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 9e04f97..7bf9597 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -118,6 +118,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..3f6d07d\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,119 @@\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 char * const git_submodule_helper_usage[] = {\n+\tN_(\"git submodule--helper [<options>] [<path>...]\"),\n+\tNULL\n+};\n+\n+static char *ps_matched;\n+static int max_prefix_len;\n+static const struct cache_entry **ce_entries;\n+static int ce_alloc, ce_used;\n+static const char *alternative_path;\n+\n+static void module_list_compute(int argc, const char **argv,\n+\t\t\t\tconst char *prefix,\n+\t\t\t\tstruct pathspec *pathspec)\n+{\n+\tint i;\n+\tchar *max_prefix;\n+\tparse_pathspec(pathspec, 0,\n+\t\t       PATHSPEC_PREFER_FULL |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n+\t\t       prefix, argv);\n+\n+\t/* Find common prefix for all pathspec's */\n+\tmax_prefix = common_prefix(pathspec);\n+\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n+\n+\tif (pathspec->nr)\n+\t\tps_matched = xcalloc(1, pathspec->nr);\n+\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"index file corrupt\");\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tconst struct cache_entry *ce = active_cache[i];\n+\n+\t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n+\t\t\t\t    max_prefix_len, ps_matched,\n+\t\t\t\t    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))\n+\t\t\tcontinue;\n+\n+\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\tALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n+\t\t\tce_entries[ce_used++] = ce;\n+\t\t}\n+\t}\n+}\n+\n+static int module_list(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstruct string_list already_printed = STRING_LIST_INIT_NODUP;\n+\tstruct pathspec pathspec;\n+\n+\tmodule_list_compute(argc, argv, prefix, &pathspec);\n+\n+\tif (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor (i = 0; i < ce_used; i++) {\n+\t\tconst struct cache_entry *ce = ce_entries[i];\n+\n+\t\tif (string_list_has_string(&already_printed, ce->name))\n+\t\t\tcontinue;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n+\t\t} else {\n+\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\t\t}\n+\n+\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n+\n+\t\tstring_list_insert(&already_printed, ce->name);\n+\t}\n+\treturn 0;\n+}\n+\n+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tenum {\n+\t\tMODE_UNSPECIFIED = 0,\n+\t\tMODE_MODULE_LIST,\n+\t} cmdmode = MODE_UNSPECIFIED;\n+\n+\tstruct option options[] = {\n+\t\tOPT_CMDMODE(0, \"module-list\", &cmdmode,\n+\t\t\t    N_(\"Get the submodule list\"), MODE_MODULE_LIST),\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+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tswitch (cmdmode) {\n+\tcase MODE_MODULE_LIST:\n+\t\treturn module_list(argc, argv, alternative_path\n+\t\t\t\t\t       ? alternative_path\n+\t\t\t\t\t       : prefix);\n+\t\tbreak;\n+\tcase MODE_UNSPECIFIED:\n+\t\tusage_with_options(git_submodule_helper_usage, options);\n+\t\tbreak;\n+\t}\n+\treturn 0;\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..e0ab06e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -145,48 +145,6 @@ relative_path ()\n \techo \"$result$target\"\n }\n \n-#\n-# Get submodule info for registered submodules\n-# $@ = path to limit submodule list\n-#\n-module_list()\n-{\n-\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n-\t(\n-\t\tgit ls-files -z --error-unmatch --stage -- \"$@\" ||\n-\t\techo \"unmatched pathspec exists\"\n-\t) |\n-\t@@PERL@@ -e '\n-\tmy %unmerged = ();\n-\tmy ($null_sha1) = (\"0\" x 40);\n-\tmy @out = ();\n-\tmy $unmatched = 0;\n-\t$/ = \"\\0\";\n-\twhile (<STDIN>) {\n-\t\tif (/^unmatched pathspec/) {\n-\t\t\t$unmatched = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tchomp;\n-\t\tmy ($mode, $sha1, $stage, $path) =\n-\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n-\t\tnext unless $mode eq \"160000\";\n-\t\tif ($stage ne \"0\") {\n-\t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n-\t\t\t}\n-\t\t\tnext;\n-\t\t}\n-\t\tpush @out, \"$_\\n\";\n-\t}\n-\tif ($unmatched) {\n-\t\tprint \"#unmatched\\n\";\n-\t} else {\n-\t\tprint for (@out);\n-\t}\n-\t'\n-}\n-\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n@@ -532,7 +490,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tmodule_list |\n+\tgit submodule--helper --prefix \"$wt_prefix\" --module-list |\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 --prefix \"$wt_prefix\" --module-list \"$@\" |\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 --prefix \"$wt_prefix\" --module-list \"$@\" |\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 --prefix \"$wt_prefix\" --module-list \"$@\" | {\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 --prefix \"$wt_prefix\" --module-list \"$@\" |\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 --prefix \"$wt_prefix\" --module-list \"$@\" |\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 fe94066..721995e 100644\n--- a/git.c\n+++ b/git.c\n@@ -468,6 +468,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.5.gffc0ba2.dirty\n"},{"id":"267360","messageId":"xmqqbneo6nas.fsf@gitster.dls.corp.google.com","threadId":"39975","inReplyTo":"CAGZ79kY=39j4H=62=VZRm4VOcqzgOAU6tDpJVsqdeqnPtB8hQQ@mail.gmail.com","subject":"Re: [RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-03T22:04:59Z","receivedAt":"2015-08-03T22:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Fri, Jul 31, 2015 at 6:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> +static const char * const git_submodule_helper_usage[] = {\n>>> +     N_(\"git submodule--helper --module_list [<path>...]\"),\n>>\n>> Yuck.  Please do not force --multi_word_opt upon us, which is simply\n>> too ugly to live around here.  --module-list is perhaps OK,\n>\n> I agree there. The way you word it here, it sounds as if the mixture\n> of dashes and underscores are a problem.\n>\n>> but\n>> because submodule--helper would not have an default action, I'd\n>> prefer to make these just \"command words\", i.e.\n>>\n>>     $ git submodule--helper module_list\n>\n> Why would you use an underscore in here as opposed to a dash?\n>      $ git submodule--helper module-list\n>\n> I went with --module-list for now as I see no reason real to make it\n> a command word for now ...\n\nThe biggest reason why we should not add more --command-mode is to\navoid confusion (and copy & paste misdesign by others).  If you use\nthe command-word interface, it is crystal clear that\n\n (1) the word 'module_list' must be the first token after the\n     subcommand name, no need to parse \"subcmd --opt --cmd\", and\n     mislead the users to think incorrectly that ...\n\n (2) ... \"cmd --optA --cmd1 --optB --cmd2\" might be allowed, which\n     would lead you to add code to reject, saying \"cmd1 and cmd2 are\n     incompatible\".\n\nSo I'd prefer to see it fixed before you start supporting more\ncommands in submodule--helper.  It will need unnecessary patch noise\nto fix it later.\n"},{"id":"267362","messageId":"CAGZ79kZL2S2jJ8HmQP6Es_4=5nCt=MwpcDcB0RPv7Yw=1NDodA@mail.gmail.com","threadId":"39975","inReplyTo":"xmqqbneo6nas.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-03T22:13:24Z","receivedAt":"2015-08-03T22:13:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Aug 3, 2015 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> On Fri, Jul 31, 2015 at 6:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Stefan Beller <sbeller@google.com> writes:\n>>>\n>>>> +static const char * const git_submodule_helper_usage[] = {\n>>>> +     N_(\"git submodule--helper --module_list [<path>...]\"),\n>>>\n>>> Yuck.  Please do not force --multi_word_opt upon us, which is simply\n>>> too ugly to live around here.  --module-list is perhaps OK,\n>>\n>> I agree there. The way you word it here, it sounds as if the mixture\n>> of dashes and underscores are a problem.\n>>\n>>> but\n>>> because submodule--helper would not have an default action, I'd\n>>> prefer to make these just \"command words\", i.e.\n>>>\n>>>     $ git submodule--helper module_list\n>>\n>> Why would you use an underscore in here as opposed to a dash?\n>>      $ git submodule--helper module-list\n>>\n>> I went with --module-list for now as I see no reason real to make it\n>> a command word for now ...\n>\n> The biggest reason why we should not add more --command-mode is to\n> avoid confusion (and copy & paste misdesign by others).  If you use\n> the command-word interface, it is crystal clear that\n>\n>  (1) the word 'module_list' must be the first token after the\n>      subcommand name, no need to parse \"subcmd --opt --cmd\", and\n>      mislead the users to think incorrectly that ...\n>\n>  (2) ... \"cmd --optA --cmd1 --optB --cmd2\" might be allowed, which\n>      would lead you to add code to reject, saying \"cmd1 and cmd2 are\n>      incompatible\".\n>\n> So I'd prefer to see it fixed before you start supporting more\n> commands in submodule--helper.  It will need unnecessary patch noise\n> to fix it later.\n\nSo we had this discussion some time ago [1] and my understanding from back\nthen was to rather have --command-mode instead of subcommand words because\nthat's what most git commands use nowadays, so we don't want to add more\nof the competing style. It's also easier to work with as we have a powerful\noption parsing implementation.\n\nIt seems your opinion has swayed. I'll change it then.\n\n[1] $gmane/254076 or $gmane/231376/focus=231478\n"},{"id":"267370","messageId":"1438642688-9660-1-git-send-email-sbeller@google.com","threadId":"39975","inReplyTo":"xmqqbneo6nas.fsf@gitster.dls.corp.google.com","subject":"[PATCH] submodule: implement `module_list` as a builtin helper","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-08-03T22:58:08Z","receivedAt":"2015-08-03T22:58:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Most of the submodule operations work on a set of submodules.\nCalculating and using this set is usually done via:\n\n       module_list \"$@\" | {\n           while read mode sha1 stage sm_path\n           do\n                # the actual operation\n           done\n       }\n\nCurrently the function `module_list` is implemented in the\ngit-submodule.sh as a shell script wrapping a perl script.\nThe rewrite is in C, such that it is faster and can later be\neasily adapted when other functions are rewritten in C.\n\ngit-submodule.sh similar to the builtin commands will navigate\nto the top most directory of the repository and keeping the\nsubdirectories as a variable. As the helper is called from\nwithin the git-submodule.sh script, we are already navigated\nto the root level, but the path arguments are stil relative\nto the subdirectory we were in when calling git-submodule.sh.\nThat's why there is a `--prefix` option pointing to an alternative\npath where to anchor relative path arguments.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Makefile                    |   1 +\n builtin.h                   |   1 +\n builtin/submodule--helper.c | 110 ++++++++++++++++++++++++++++++++++++++++++++\n git-submodule.sh            |  54 +++-------------------\n git.c                       |   1 +\n 5 files changed, 119 insertions(+), 48 deletions(-)\n create mode 100644 builtin/submodule--helper.c\n\ndiff --git a/Makefile b/Makefile\nindex 8c3c724..6fb7484 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -898,6 +898,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 9e04f97..7bf9597 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -118,6 +118,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..fbd9568\n--- /dev/null\n+++ b/builtin/submodule--helper.c\n@@ -0,0 +1,110 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"parse-options.h\"\n+#include \"quote.h\"\n+#include \"pathspec.h\"\n+#include \"dir.h\"\n+#include \"utf8.h\"\n+\n+static char *ps_matched;\n+static const struct cache_entry **ce_entries;\n+static int ce_alloc, ce_used;\n+\n+static void module_list_compute(int argc, const char **argv,\n+\t\t\t\tconst char *prefix,\n+\t\t\t\tstruct pathspec *pathspec)\n+{\n+\tint i;\n+\tchar *max_prefix;\n+\tint max_prefix_len;\n+\tparse_pathspec(pathspec, 0,\n+\t\t       PATHSPEC_PREFER_FULL |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n+\t\t       prefix, argv);\n+\n+\t/* Find common prefix for all pathspec's */\n+\tmax_prefix = common_prefix(pathspec);\n+\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n+\n+\tif (pathspec->nr)\n+\t\tps_matched = xcalloc(1, pathspec->nr);\n+\n+\n+\tif (read_cache() < 0)\n+\t\tdie(\"index file corrupt\");\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tconst struct cache_entry *ce = active_cache[i];\n+\n+\t\tif (!match_pathspec(pathspec, ce->name, ce_namelen(ce),\n+\t\t\t\t    max_prefix_len, ps_matched,\n+\t\t\t\t    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))\n+\t\t\tcontinue;\n+\n+\t\tif (S_ISGITLINK(ce->ce_mode)) {\n+\t\t\tALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);\n+\t\t\tce_entries[ce_used++] = ce;\n+\t\t}\n+\t}\n+}\n+\n+static int module_list(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstruct string_list already_printed = STRING_LIST_INIT_NODUP;\n+\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+\tstatic const char * const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper module_list [--prefix=<path>] [<path>...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, module_list_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tmodule_list_compute(argc, argv, alternative_path\n+\t\t\t\t\t? alternative_path\n+\t\t\t\t\t: prefix, &pathspec);\n+\n+\tif (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {\n+\t\tprintf(\"#unmatched\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tfor (i = 0; i < ce_used; i++) {\n+\t\tconst struct cache_entry *ce = ce_entries[i];\n+\n+\t\tif (string_list_has_string(&already_printed, ce->name))\n+\t\t\tcontinue;\n+\n+\t\tif (ce_stage(ce)) {\n+\t\t\tprintf(\"%06o %s U\\t\", ce->ce_mode, sha1_to_hex(null_sha1));\n+\t\t} else {\n+\t\t\tprintf(\"%06o %s %d\\t\", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));\n+\t\t}\n+\n+\t\tutf8_fprintf(stdout, \"%s\\n\", ce->name);\n+\n+\t\tstring_list_insert(&already_printed, ce->name);\n+\t}\n+\treturn 0;\n+}\n+\n+int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc < 2)\n+\t\tgoto usage;\n+\n+\tif (!strcmp(argv[1], \"module_list\"))\n+\t\treturn module_list(--argc, ++argv, prefix);\n+usage:\n+\tusage(\"git submodule--helper module_list\\n\");\n+}\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 36797c3..af9ecef 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -145,48 +145,6 @@ relative_path ()\n \techo \"$result$target\"\n }\n \n-#\n-# Get submodule info for registered submodules\n-# $@ = path to limit submodule list\n-#\n-module_list()\n-{\n-\teval \"set $(git rev-parse --sq --prefix \"$wt_prefix\" -- \"$@\")\"\n-\t(\n-\t\tgit ls-files -z --error-unmatch --stage -- \"$@\" ||\n-\t\techo \"unmatched pathspec exists\"\n-\t) |\n-\t@@PERL@@ -e '\n-\tmy %unmerged = ();\n-\tmy ($null_sha1) = (\"0\" x 40);\n-\tmy @out = ();\n-\tmy $unmatched = 0;\n-\t$/ = \"\\0\";\n-\twhile (<STDIN>) {\n-\t\tif (/^unmatched pathspec/) {\n-\t\t\t$unmatched = 1;\n-\t\t\tnext;\n-\t\t}\n-\t\tchomp;\n-\t\tmy ($mode, $sha1, $stage, $path) =\n-\t\t\t/^([0-7]+) ([0-9a-f]{40}) ([0-3])\\t(.*)$/;\n-\t\tnext unless $mode eq \"160000\";\n-\t\tif ($stage ne \"0\") {\n-\t\t\tif (!$unmerged{$path}++) {\n-\t\t\t\tpush @out, \"$mode $null_sha1 U\\t$path\\n\";\n-\t\t\t}\n-\t\t\tnext;\n-\t\t}\n-\t\tpush @out, \"$_\\n\";\n-\t}\n-\tif ($unmatched) {\n-\t\tprint \"#unmatched\\n\";\n-\t} else {\n-\t\tprint for (@out);\n-\t}\n-\t'\n-}\n-\n die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n@@ -532,7 +490,7 @@ cmd_foreach()\n \t# command in the subshell (and a recursive call to this function)\n \texec 3<&0\n \n-\tmodule_list |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\"|\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -592,7 +550,7 @@ cmd_init()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -674,7 +632,7 @@ cmd_deinit()\n \t\tdie \"$(eval_gettext \"Use '.' if you really want to deinitialize all submodules\")\"\n \tfi\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -790,7 +748,7 @@ cmd_update()\n \tfi\n \n \tcloned_modules=\n-\tmodule_list \"$@\" | {\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" | {\n \terr=\n \twhile read mode sha1 stage sm_path\n \tdo\n@@ -1222,7 +1180,7 @@ cmd_status()\n \t\tshift\n \tdone\n \n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\n@@ -1299,7 +1257,7 @@ cmd_sync()\n \t\tesac\n \tdone\n \tcd_to_toplevel\n-\tmodule_list \"$@\" |\n+\tgit submodule--helper module_list --prefix \"$wt_prefix\" \"$@\" |\n \twhile read mode sha1 stage sm_path\n \tdo\n \t\tdie_if_unmatched \"$mode\"\ndiff --git a/git.c b/git.c\nindex fe94066..721995e 100644\n--- a/git.c\n+++ b/git.c\n@@ -468,6 +468,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.1.gc648038.dirty\n"}]}