{"thread":{"id":"44127","subject":"[PATCH 1/2] ls-files: adding support for submodules","startedAt":"2016-09-21T22:05:19Z","lastAt":"2016-09-27T22:23:11Z","messageCount":29,"participants":["Brandon Williams","Junio C Hamano","Jeff King","Stefan Beller","Nazri Ramliy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"302335","messageId":"1474495472-94190-1-git-send-email-bmwill@google.com","threadId":"44127","inReplyTo":null,"subject":"[PATCH 1/2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T22:04:31Z","receivedAt":"2016-09-21T22:05:19Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Allow ls-files to recognize submodules in order to retrieve a list of\nfiles from a repository's submodules.  This is done by forking off a\nprocess to recursively call ls-files on all submodules. Also added a\nsubmodule-prefix command in order to prepend paths to child processes.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/git-ls-files.txt         | 11 +++-\n builtin/ls-files.c                     | 61 +++++++++++++++++++++\n t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++\n 3 files changed, 170 insertions(+), 1 deletion(-)\n create mode 100755 t/t3007-ls-files-recurse-submodules.sh\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 0d933ac..09e4449 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -18,7 +18,9 @@ SYNOPSIS\n \t\t[--exclude-per-directory=<file>]\n \t\t[--exclude-standard]\n \t\t[--error-unmatch] [--with-tree=<tree-ish>]\n-\t\t[--full-name] [--abbrev] [--] [<file>...]\n+\t\t[--full-name] [--recurse-submodules]\n+\t\t[--submodule-prefix=<path>]\n+\t\t[--abbrev] [--] [<file>...]\n \n DESCRIPTION\n -----------\n@@ -137,6 +139,13 @@ a space) at the start of each line:\n \toption forces paths to be output relative to the project\n \ttop directory.\n \n+--recurse-submodules::\n+\tRecursively calls ls-files on each submodule in the repository.\n+\tCurrently there is only support for the --cached mode.\n+\n+--submodule-prefix=<path>::\n+\tPrepend the provided path to the output of each file\n+\n --abbrev[=<n>]::\n \tInstead of showing the full 40-byte hexadecimal object\n \tlines, show only a partial prefix.\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 00ea91a..ffd9ea6 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"string-list.h\"\n #include \"pathspec.h\"\n+#include \"run-command.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -28,6 +29,8 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n+static const char *submodule_prefix;\n+static int recurse_submodules;\n \n static const char *prefix;\n static int max_prefix_len;\n@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)\n static void write_name(const char *name)\n {\n \t/*\n+\t * NEEDSWORK: To make this thread-safe, full_name would have to be owned\n+\t * by the caller.\n+\t *\n+\t * full_name get reused across output lines to minimize the allocation\n+\t * churn.\n+\t */\n+\tstatic struct strbuf full_name = STRBUF_INIT;\n+\tif (submodule_prefix && *submodule_prefix) {\n+\t\tstrbuf_reset(&full_name);\n+\t\tstrbuf_addstr(&full_name, submodule_prefix);\n+\t\tstrbuf_addstr(&full_name, name);\n+\t\tname = full_name.buf;\n+\t}\n+\n+\t/*\n \t * With \"--full-name\", prefix_len=0; this caller needs to pass\n \t * an empty string in that case (a NULL is good for \"\").\n \t */\n@@ -152,6 +170,26 @@ static void show_killed_files(struct dir_struct *dir)\n \t}\n }\n \n+/**\n+ * Recursively call ls-files on a submodule\n+ */\n+static void show_gitlink(const struct cache_entry *ce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tint status;\n+\n+\targv_array_push(&cp.args, \"ls-files\");\n+\targv_array_push(&cp.args, \"--recurse-submodules\");\n+\targv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n+\t\t\t submodule_prefix ? submodule_prefix : \"\",\n+\t\t\t ce->name);\n+\tcp.git_cmd = 1;\n+\tcp.dir = ce->name;\n+\tstatus = run_command(&cp);\n+\tif (status)\n+\t\texit(status);\n+}\n+\n static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n {\n \tint len = max_prefix_len;\n@@ -163,6 +201,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n \t\t\t    len, ps_matched,\n \t\t\t    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))\n \t\treturn;\n+\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {\n+\t\tshow_gitlink(ce);\n+\t\treturn;\n+\t}\n \n \tif (tag && *tag && show_valid_bit &&\n \t    (ce->ce_flags & CE_VALID)) {\n@@ -468,6 +510,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\t{ OPTION_SET_INT, 0, \"full-name\", &prefix_len, NULL,\n \t\t\tN_(\"make the output relative to the project top directory\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },\n+\t\tOPT_STRING(0, \"submodule-prefix\", &submodule_prefix,\n+\t\t\tN_(\"path\"), N_(\"prepend <path> to each file\")),\n+\t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n+\t\t\tN_(\"recurse through submodules\")),\n \t\tOPT_BOOL(0, \"error-unmatch\", &error_unmatch,\n \t\t\tN_(\"if any <file> is not in the index, treat this as an error\")),\n \t\tOPT_STRING(0, \"with-tree\", &with_tree, N_(\"tree-ish\"),\n@@ -519,6 +565,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n+\tif (recurse_submodules &&\n+\t    (show_stage || show_deleted || show_others || show_unmerged ||\n+\t     show_killed || show_modified || show_resolve_undo ||\n+\t     show_valid_bit || show_tag || show_eol))\n+\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n+\t\t    \"--cached mode\");\n+\n+\tif (recurse_submodules && error_unmatch)\n+\t\tdie(\"ls-files --recurse-submodules does not support \"\n+\t\t    \"--error-unmatch\");\n+\n+\tif (recurse_submodules && argc)\n+\t\tdie(\"ls-files --recurse-submodules does not support path \"\n+\t\t    \"arguments\");\n+\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD |\n \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nnew file mode 100755\nindex 0000000..caf3815\n--- /dev/null\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -0,0 +1,99 @@\n+#!/bin/sh\n+\n+test_description='Test ls-files recurse-submodules feature\n+\n+This test verifies the recurse-submodules feature correctly lists files from\n+submodules.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup directory structure and submodules' '\n+\techo a >a &&\n+\tmkdir b &&\n+\techo b >b/b &&\n+\tgit add a b &&\n+\tgit commit -m \"add a and b\" &&\n+\tgit init submodule &&\n+\techo c >submodule/c &&\n+\tgit -C submodule add c &&\n+\tgit -C submodule commit -m \"add c\" &&\n+\tgit submodule add ./submodule &&\n+\tgit commit -m \"added submodule\"\n+'\n+\n+test_expect_success 'ls-files correctly outputs files in submodule' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/c\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files does not output files not added to a repo' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/c\n+\tEOF\n+\n+\techo a >not_added &&\n+\techo b >b/not_added &&\n+\techo c >submodule/not_added &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files recurses more than 1 level' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/.gitmodules\n+\tsubmodule/c\n+\tsubmodule/subsub/d\n+\tEOF\n+\n+\tgit init submodule/subsub &&\n+\techo d >submodule/subsub/d &&\n+\tgit -C submodule/subsub add d &&\n+\tgit -C submodule/subsub commit -m \"add d\" &&\n+\tgit -C submodule submodule add ./subsub &&\n+\tgit -C submodule commit -m \"added subsub\" &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules does not support using path arguments' '\n+\ttest_must_fail git ls-files --recurse-submodules b 2>actual &&\n+\ttest_i18ngrep \"does not support path arguments\" actual\n+'\n+\n+test_expect_success '--recurse-submodules does not support --error-unmatch' '\n+\ttest_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&\n+\ttest_i18ngrep \"does not support --error-unmatch\" actual\n+'\n+\n+test_incompatible_with_recurse_submodules () {\n+\ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n+\t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n+\t\ttest_i18ngrep 'can only be used in --cached mode' actual\n+\t\"\n+}\n+\n+test_incompatible_with_recurse_submodules -v\n+test_incompatible_with_recurse_submodules -t\n+test_incompatible_with_recurse_submodules --deleted\n+test_incompatible_with_recurse_submodules --modified\n+test_incompatible_with_recurse_submodules --others\n+test_incompatible_with_recurse_submodules --stage\n+test_incompatible_with_recurse_submodules --killed\n+test_incompatible_with_recurse_submodules --unmerged\n+test_incompatible_with_recurse_submodules --eol\n+\n+test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302336","messageId":"1474495472-94190-2-git-send-email-bmwill@google.com","threadId":"44127","inReplyTo":"1474495472-94190-1-git-send-email-bmwill@google.com","subject":"[PATCH 2/2] ls-files: add pathspec matching for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T22:04:32Z","receivedAt":"2016-09-21T22:05:23Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Pathspecs can be a bit tricky when trying to apply them to submodules.\nThe main challenge is that the pathspecs will be with respect to the\nsuper module and not with respect to paths in the submodule.  The\napproach this patch takes is to pass in the identical pathspec from the\nsuper module to the submodule in addition to the submodule-prefix, which\nis the path from the root of the super module to the submodule, and then\nwe can compare an entry in the submodule prepended with the\nsubmodule-prefix to the pathspec in order to determine if there is a\nmatch.\n\nThis patch also permits the pathspec logic to perform a prefix match against\nsubmodules since a pathspec could refer to a file inside of a submodule.\nDue to limitations in the wildmatch logic, a prefix match is only done\nliterally.  If any wildcard character is encountered we'll simply punt\nand produce a false positive match.  More accurate matching will be done\nonce inside the submodule.  This is due to the super module not knowing\nwhat files could exist in the submodule.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/ls-files.c                     | 132 ++++++++++++++++++++-------------\n dir.c                                  |  43 ++++++++++-\n dir.h                                  |   4 +\n t/t3007-ls-files-recurse-submodules.sh | 114 ++++++++++++++++++++++++++--\n 4 files changed, 231 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex ffd9ea6..fa4029e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -177,12 +177,34 @@ static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n+\tint i;\n \n \targv_array_push(&cp.args, \"ls-files\");\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \targv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n \t\t\t submodule_prefix ? submodule_prefix : \"\",\n \t\t\t ce->name);\n+\t/* add options */\n+\tif (show_eol)\n+\t\targv_array_push(&cp.args, \"--eol\");\n+\tif (show_valid_bit)\n+\t\targv_array_push(&cp.args, \"-v\");\n+\tif (show_stage)\n+\t\targv_array_push(&cp.args, \"--stage\");\n+\tif (show_cached)\n+\t\targv_array_push(&cp.args, \"--cached\");\n+\tif (debug_mode)\n+\t\targv_array_push(&cp.args, \"--debug\");\n+\n+\t/*\n+\t * Pass in the original pathspec args.  The submodule will be\n+\t * responsible for prepending the 'submodule_prefix' prior to comparing\n+\t * against the pathspec for matches.\n+\t */\n+\targv_array_push(&cp.args, \"--\");\n+\tfor (i = 0; i < pathspec.nr; i++)\n+\t\targv_array_push(&cp.args, pathspec.items[i].original);\n+\n \tcp.git_cmd = 1;\n \tcp.dir = ce->name;\n \tstatus = run_command(&cp);\n@@ -192,57 +214,62 @@ static void show_gitlink(const struct cache_entry *ce)\n \n static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n {\n+\tstruct strbuf name = STRBUF_INIT;\n \tint len = max_prefix_len;\n+\tif (submodule_prefix)\n+\t\tstrbuf_addstr(&name, submodule_prefix);\n+\tstrbuf_addstr(&name, ce->name);\n \n \tif (len >= ce_namelen(ce))\n \t\tdie(\"git ls-files: internal error - cache entry not superset of prefix\");\n \n-\tif (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),\n-\t\t\t    len, ps_matched,\n-\t\t\t    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))\n-\t\treturn;\n-\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {\n+\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n+\t    submodule_path_match(&pathspec, name.buf, ps_matched)) {\n \t\tshow_gitlink(ce);\n-\t\treturn;\n-\t}\n+\t} else if (match_pathspec(&pathspec, name.buf, name.len,\n+\t\t\t\t  len, ps_matched,\n+\t\t\t\t  S_ISDIR(ce->ce_mode) ||\n+\t\t\t\t  S_ISGITLINK(ce->ce_mode))) {\n+\t\tif (tag && *tag && show_valid_bit &&\n+\t\t    (ce->ce_flags & CE_VALID)) {\n+\t\t\tstatic char alttag[4];\n+\t\t\tmemcpy(alttag, tag, 3);\n+\t\t\tif (isalpha(tag[0]))\n+\t\t\t\talttag[0] = tolower(tag[0]);\n+\t\t\telse if (tag[0] == '?')\n+\t\t\t\talttag[0] = '!';\n+\t\t\telse {\n+\t\t\t\talttag[0] = 'v';\n+\t\t\t\talttag[1] = tag[0];\n+\t\t\t\talttag[2] = ' ';\n+\t\t\t\talttag[3] = 0;\n+\t\t\t}\n+\t\t\ttag = alttag;\n+\t\t}\n \n-\tif (tag && *tag && show_valid_bit &&\n-\t    (ce->ce_flags & CE_VALID)) {\n-\t\tstatic char alttag[4];\n-\t\tmemcpy(alttag, tag, 3);\n-\t\tif (isalpha(tag[0]))\n-\t\t\talttag[0] = tolower(tag[0]);\n-\t\telse if (tag[0] == '?')\n-\t\t\talttag[0] = '!';\n-\t\telse {\n-\t\t\talttag[0] = 'v';\n-\t\t\talttag[1] = tag[0];\n-\t\t\talttag[2] = ' ';\n-\t\t\talttag[3] = 0;\n+\t\tif (!show_stage) {\n+\t\t\tfputs(tag, stdout);\n+\t\t} else {\n+\t\t\tprintf(\"%s%06o %s %d\\t\",\n+\t\t\t       tag,\n+\t\t\t       ce->ce_mode,\n+\t\t\t       find_unique_abbrev(ce->sha1,abbrev),\n+\t\t\t       ce_stage(ce));\n+\t\t}\n+\t\twrite_eolinfo(ce, ce->name);\n+\t\twrite_name(ce->name);\n+\t\tif (debug_mode) {\n+\t\t\tconst struct stat_data *sd = &ce->ce_stat_data;\n+\n+\t\t\tprintf(\"  ctime: %d:%d\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n+\t\t\tprintf(\"  mtime: %d:%d\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n+\t\t\tprintf(\"  dev: %d\\tino: %d\\n\", sd->sd_dev, sd->sd_ino);\n+\t\t\tprintf(\"  uid: %d\\tgid: %d\\n\", sd->sd_uid, sd->sd_gid);\n+\t\t\tprintf(\"  size: %d\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n \t\t}\n-\t\ttag = alttag;\n \t}\n \n-\tif (!show_stage) {\n-\t\tfputs(tag, stdout);\n-\t} else {\n-\t\tprintf(\"%s%06o %s %d\\t\",\n-\t\t       tag,\n-\t\t       ce->ce_mode,\n-\t\t       find_unique_abbrev(ce->sha1,abbrev),\n-\t\t       ce_stage(ce));\n-\t}\n-\twrite_eolinfo(ce, ce->name);\n-\twrite_name(ce->name);\n-\tif (debug_mode) {\n-\t\tconst struct stat_data *sd = &ce->ce_stat_data;\n-\n-\t\tprintf(\"  ctime: %d:%d\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n-\t\tprintf(\"  mtime: %d:%d\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n-\t\tprintf(\"  dev: %d\\tino: %d\\n\", sd->sd_dev, sd->sd_ino);\n-\t\tprintf(\"  uid: %d\\tgid: %d\\n\", sd->sd_uid, sd->sd_gid);\n-\t\tprintf(\"  size: %d\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n-\t}\n+\tstrbuf_release(&name);\n }\n \n static void show_ru_info(void)\n@@ -566,27 +593,28 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tsetup_work_tree();\n \n \tif (recurse_submodules &&\n-\t    (show_stage || show_deleted || show_others || show_unmerged ||\n-\t     show_killed || show_modified || show_resolve_undo ||\n-\t     show_valid_bit || show_tag || show_eol))\n-\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n-\t\t    \"--cached mode\");\n+\t    (show_deleted || show_others || show_unmerged ||\n+\t     show_killed || show_modified || show_resolve_undo))\n+\t\tdie(\"ls-files --recurse-submodules unsupported mode\");\n \n \tif (recurse_submodules && error_unmatch)\n \t\tdie(\"ls-files --recurse-submodules does not support \"\n \t\t    \"--error-unmatch\");\n \n-\tif (recurse_submodules && argc)\n-\t\tdie(\"ls-files --recurse-submodules does not support path \"\n-\t\t    \"arguments\");\n-\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD |\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+\t/*\n+\t * Find common prefix for all pathspec's\n+\t * This is used as a performance optimization which unfortunately cannot\n+\t * be done when recursing into submodules\n+\t */\n+\tif (recurse_submodules)\n+\t\tmax_prefix = NULL;\n+\telse\n+\t\tmax_prefix = common_prefix(&pathspec);\n \tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n \n \t/* Treat unmatching pathspec elements as errors */\ndiff --git a/dir.c b/dir.c\nindex 0ea235f..9df6d36 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n-#define DO_MATCH_EXCLUDE   1\n-#define DO_MATCH_DIRECTORY 2\n+#define DO_MATCH_EXCLUDE   (1<<0)\n+#define DO_MATCH_DIRECTORY (1<<1)\n+#define DO_MATCH_SUBMODULE (1<<2)\n \n /*\n  * Does 'match' match the given name?\n@@ -283,6 +284,29 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n \t\t\t item->nowildcard_len - prefix))\n \t\treturn MATCHED_FNMATCH;\n \n+\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n+\tif (flags & DO_MATCH_SUBMODULE) {\n+\t\t/* Check if the name is a literal prefix of the pathspec */\n+\t\tif ((item->match[namelen] == '/') &&\n+\t\t    !ps_strncmp(item, match, name, namelen))\n+\t\t\treturn MATCHED_RECURSIVELY;\n+\n+\t\t/*\n+\t\t * Here is where we would perform a wildmatch to check if\n+\t\t * \"name\" can be matched as a directory (or a prefix) against\n+\t\t * the pathspec.  Since wildmatch doesn't have this capability\n+\t\t * at the present we have to punt and say that it is a match,\n+\t\t * esentially returning a false positive (as long as \"name\"\n+\t\t * matches upto the first wild character).\n+\t\t * The submodules themselves will be able to perform more\n+\t\t * accurate matching to determine if the pathspec matches.\n+\t\t */\n+\t\tif (item->nowildcard_len < item->len &&\n+\t\t    !ps_strncmp(item, match, name,\n+\t\t\t\titem->nowildcard_len - prefix))\n+\t\t\treturn MATCHED_RECURSIVELY;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -386,6 +410,21 @@ int match_pathspec(const struct pathspec *ps,\n \treturn negative ? 0 : positive;\n }\n \n+/**\n+ * Check if a submodule is a superset of the pathspec\n+ */\n+int submodule_path_match(const struct pathspec *ps,\n+\t\t\t const char *submodule_name,\n+\t\t\t char *seen)\n+{\n+\tint matched = do_match_pathspec(ps, submodule_name,\n+\t\t\t\t\tstrlen(submodule_name),\n+\t\t\t\t\t0, seen,\n+\t\t\t\t\tDO_MATCH_DIRECTORY |\n+\t\t\t\t\tDO_MATCH_SUBMODULE);\n+\treturn matched;\n+}\n+\n int report_path_error(const char *ps_matched,\n \t\t      const struct pathspec *pathspec,\n \t\t      const char *prefix)\ndiff --git a/dir.h b/dir.h\nindex da1a858..97c83bb 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,\n \t\t       const char *pattern, const char *string,\n \t\t       int prefix);\n \n+extern int submodule_path_match(const struct pathspec *ps,\n+\t\t\t\tconst char *submodule_name,\n+\t\t\t\tchar *seen);\n+\n static inline int ce_path_match(const struct cache_entry *ce,\n \t\t\t\tconst struct pathspec *pathspec,\n \t\t\t\tchar *seen)\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex caf3815..ca79fda 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -69,9 +69,111 @@ test_expect_success 'ls-files recurses more than 1 level' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--recurse-submodules does not support using path arguments' '\n-\ttest_must_fail git ls-files --recurse-submodules b 2>actual &&\n-\ttest_i18ngrep \"does not support path arguments\" actual\n+test_expect_success '--recurse-submodules and pathspecs setup' '\n+\techo e >submodule/subsub/e.txt &&\n+\tgit -C submodule/subsub add e.txt &&\n+\tgit -C submodule/subsub commit -m \"adding e.txt\" &&\n+\techo f >submodule/f.TXT &&\n+\techo g >submodule/g.txt &&\n+\tgit -C submodule add f.TXT g.txt &&\n+\tgit -C submodule commit -m \"add f and g\" &&\n+\techo h >h.txt &&\n+\tmkdir sib &&\n+\techo sib >sib/file &&\n+\tgit add h.txt sib/file &&\n+\tgit commit -m \"add h and sib/file\" &&\n+\tgit init sub &&\n+\techo sub >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\tgit submodule add ./sub &&\n+\tgit commit -m \"added sub\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\th.txt\n+\tsib/file\n+\tsub/file\n+\tsubmodule/.gitmodules\n+\tsubmodule/c\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/d\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual &&\n+\tcat actual &&\n+\tgit ls-files --recurse-submodules \"*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\th.txt\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \"*.txt\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\th.txt\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \":(icase)*.txt\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\th.txt\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \":(icase)*.txt\" \":(exclude)submodule/subsub/*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\tsub/file\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \"sub\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"sub/\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"sub/file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"su*/file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"su?/file\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\tsib/file\n+\tsub/file\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \"s??/file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"s???file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"s*file\" >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--recurse-submodules does not support --error-unmatch' '\n@@ -82,18 +184,14 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n-\t\ttest_i18ngrep 'can only be used in --cached mode' actual\n+\t\ttest_i18ngrep 'unsupported mode' actual\n \t\"\n }\n \n-test_incompatible_with_recurse_submodules -v\n-test_incompatible_with_recurse_submodules -t\n test_incompatible_with_recurse_submodules --deleted\n test_incompatible_with_recurse_submodules --modified\n test_incompatible_with_recurse_submodules --others\n-test_incompatible_with_recurse_submodules --stage\n test_incompatible_with_recurse_submodules --killed\n test_incompatible_with_recurse_submodules --unmerged\n-test_incompatible_with_recurse_submodules --eol\n \n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302337","messageId":"CAKoko1q7Mb6_cnaA1ecZJ2y1PWUrW4RSu6RiviyN91JV5-QR5g@mail.gmail.com","threadId":"44127","inReplyTo":"1474495472-94190-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T22:08:03Z","receivedAt":"2016-09-21T22:08:09Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"This is another version of the first ls-files patch i sent out in\norder.  In this version I fixed the option\n name to be '--submodule-prefix'.\n"},{"id":"302342","messageId":"xmqq1t0c7ur2.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"CAKoko1q7Mb6_cnaA1ecZJ2y1PWUrW4RSu6RiviyN91JV5-QR5g@mail.gmail.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T22:28:33Z","receivedAt":"2016-09-21T22:28:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> This is another version of the first ls-files patch i sent out in\n> order.  In this version I fixed the option\n>  name to be '--submodule-prefix'.\n\nI understand that many internal changes in your work environment are\ntitled like \"DOing X\", but our convention around here is to label\nthem \"DO X\", as if you are giving an order to somebody else, either\ntelling the codebase \"to be like so\", or telling the patch-monkey\nmaintainer \"to make it so\".  So I'd suggest retitling it to\n\n\tls-files: optionally recurse into submodules\n\nor something like that.  It is an added advantage of being a lot\nmore descriptive than \"adding support\", which does not say what kind\nof support it is adding.\n"},{"id":"302345","messageId":"CAKoko1qch_odsEWba0rtCv-DWO0ABS2yprnwGPCgyT6-7H-LdQ@mail.gmail.com","threadId":"44127","inReplyTo":"xmqq1t0c7ur2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T22:38:22Z","receivedAt":"2016-09-21T22:38:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"yes you mentioned this and I meant to change that before sending it out.\nLooks like it slipped through have slipped through.\n"},{"id":"302346","messageId":"1474497772-97986-1-git-send-email-bmwill@google.com","threadId":"44127","inReplyTo":"CAKoko1qch_odsEWba0rtCv-DWO0ABS2yprnwGPCgyT6-7H-LdQ@mail.gmail.com","subject":"[PATCH 1/2] ls-files: optionally recurse into submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T22:42:52Z","receivedAt":"2016-09-21T22:44:21Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Allow ls-files to recognize submodules in order to retrieve a list of\nfiles from a repository's submodules.  This is done by forking off a\nprocess to recursively call ls-files on all submodules. Also added a\nsubmodule-prefix command in order to prepend paths to child processes.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n Documentation/git-ls-files.txt         | 11 +++-\n builtin/ls-files.c                     | 61 +++++++++++++++++++++\n t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++\n 3 files changed, 170 insertions(+), 1 deletion(-)\n create mode 100755 t/t3007-ls-files-recurse-submodules.sh\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 0d933ac..09e4449 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -18,7 +18,9 @@ SYNOPSIS\n \t\t[--exclude-per-directory=<file>]\n \t\t[--exclude-standard]\n \t\t[--error-unmatch] [--with-tree=<tree-ish>]\n-\t\t[--full-name] [--abbrev] [--] [<file>...]\n+\t\t[--full-name] [--recurse-submodules]\n+\t\t[--submodule-prefix=<path>]\n+\t\t[--abbrev] [--] [<file>...]\n \n DESCRIPTION\n -----------\n@@ -137,6 +139,13 @@ a space) at the start of each line:\n \toption forces paths to be output relative to the project\n \ttop directory.\n \n+--recurse-submodules::\n+\tRecursively calls ls-files on each submodule in the repository.\n+\tCurrently there is only support for the --cached mode.\n+\n+--submodule-prefix=<path>::\n+\tPrepend the provided path to the output of each file\n+\n --abbrev[=<n>]::\n \tInstead of showing the full 40-byte hexadecimal object\n \tlines, show only a partial prefix.\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 00ea91a..ffd9ea6 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -14,6 +14,7 @@\n #include \"resolve-undo.h\"\n #include \"string-list.h\"\n #include \"pathspec.h\"\n+#include \"run-command.h\"\n \n static int abbrev;\n static int show_deleted;\n@@ -28,6 +29,8 @@ static int show_valid_bit;\n static int line_terminator = '\\n';\n static int debug_mode;\n static int show_eol;\n+static const char *submodule_prefix;\n+static int recurse_submodules;\n \n static const char *prefix;\n static int max_prefix_len;\n@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)\n static void write_name(const char *name)\n {\n \t/*\n+\t * NEEDSWORK: To make this thread-safe, full_name would have to be owned\n+\t * by the caller.\n+\t *\n+\t * full_name get reused across output lines to minimize the allocation\n+\t * churn.\n+\t */\n+\tstatic struct strbuf full_name = STRBUF_INIT;\n+\tif (submodule_prefix && *submodule_prefix) {\n+\t\tstrbuf_reset(&full_name);\n+\t\tstrbuf_addstr(&full_name, submodule_prefix);\n+\t\tstrbuf_addstr(&full_name, name);\n+\t\tname = full_name.buf;\n+\t}\n+\n+\t/*\n \t * With \"--full-name\", prefix_len=0; this caller needs to pass\n \t * an empty string in that case (a NULL is good for \"\").\n \t */\n@@ -152,6 +170,26 @@ static void show_killed_files(struct dir_struct *dir)\n \t}\n }\n \n+/**\n+ * Recursively call ls-files on a submodule\n+ */\n+static void show_gitlink(const struct cache_entry *ce)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tint status;\n+\n+\targv_array_push(&cp.args, \"ls-files\");\n+\targv_array_push(&cp.args, \"--recurse-submodules\");\n+\targv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n+\t\t\t submodule_prefix ? submodule_prefix : \"\",\n+\t\t\t ce->name);\n+\tcp.git_cmd = 1;\n+\tcp.dir = ce->name;\n+\tstatus = run_command(&cp);\n+\tif (status)\n+\t\texit(status);\n+}\n+\n static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n {\n \tint len = max_prefix_len;\n@@ -163,6 +201,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n \t\t\t    len, ps_matched,\n \t\t\t    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))\n \t\treturn;\n+\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {\n+\t\tshow_gitlink(ce);\n+\t\treturn;\n+\t}\n \n \tif (tag && *tag && show_valid_bit &&\n \t    (ce->ce_flags & CE_VALID)) {\n@@ -468,6 +510,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\t{ OPTION_SET_INT, 0, \"full-name\", &prefix_len, NULL,\n \t\t\tN_(\"make the output relative to the project top directory\"),\n \t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },\n+\t\tOPT_STRING(0, \"submodule-prefix\", &submodule_prefix,\n+\t\t\tN_(\"path\"), N_(\"prepend <path> to each file\")),\n+\t\tOPT_BOOL(0, \"recurse-submodules\", &recurse_submodules,\n+\t\t\tN_(\"recurse through submodules\")),\n \t\tOPT_BOOL(0, \"error-unmatch\", &error_unmatch,\n \t\t\tN_(\"if any <file> is not in the index, treat this as an error\")),\n \t\tOPT_STRING(0, \"with-tree\", &with_tree, N_(\"tree-ish\"),\n@@ -519,6 +565,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n+\tif (recurse_submodules &&\n+\t    (show_stage || show_deleted || show_others || show_unmerged ||\n+\t     show_killed || show_modified || show_resolve_undo ||\n+\t     show_valid_bit || show_tag || show_eol))\n+\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n+\t\t    \"--cached mode\");\n+\n+\tif (recurse_submodules && error_unmatch)\n+\t\tdie(\"ls-files --recurse-submodules does not support \"\n+\t\t    \"--error-unmatch\");\n+\n+\tif (recurse_submodules && argc)\n+\t\tdie(\"ls-files --recurse-submodules does not support path \"\n+\t\t    \"arguments\");\n+\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD |\n \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nnew file mode 100755\nindex 0000000..caf3815\n--- /dev/null\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -0,0 +1,99 @@\n+#!/bin/sh\n+\n+test_description='Test ls-files recurse-submodules feature\n+\n+This test verifies the recurse-submodules feature correctly lists files from\n+submodules.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup directory structure and submodules' '\n+\techo a >a &&\n+\tmkdir b &&\n+\techo b >b/b &&\n+\tgit add a b &&\n+\tgit commit -m \"add a and b\" &&\n+\tgit init submodule &&\n+\techo c >submodule/c &&\n+\tgit -C submodule add c &&\n+\tgit -C submodule commit -m \"add c\" &&\n+\tgit submodule add ./submodule &&\n+\tgit commit -m \"added submodule\"\n+'\n+\n+test_expect_success 'ls-files correctly outputs files in submodule' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/c\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files does not output files not added to a repo' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/c\n+\tEOF\n+\n+\techo a >not_added &&\n+\techo b >b/not_added &&\n+\techo c >submodule/not_added &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files recurses more than 1 level' '\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\tsubmodule/.gitmodules\n+\tsubmodule/c\n+\tsubmodule/subsub/d\n+\tEOF\n+\n+\tgit init submodule/subsub &&\n+\techo d >submodule/subsub/d &&\n+\tgit -C submodule/subsub add d &&\n+\tgit -C submodule/subsub commit -m \"add d\" &&\n+\tgit -C submodule submodule add ./subsub &&\n+\tgit -C submodule commit -m \"added subsub\" &&\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules does not support using path arguments' '\n+\ttest_must_fail git ls-files --recurse-submodules b 2>actual &&\n+\ttest_i18ngrep \"does not support path arguments\" actual\n+'\n+\n+test_expect_success '--recurse-submodules does not support --error-unmatch' '\n+\ttest_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&\n+\ttest_i18ngrep \"does not support --error-unmatch\" actual\n+'\n+\n+test_incompatible_with_recurse_submodules () {\n+\ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n+\t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n+\t\ttest_i18ngrep 'can only be used in --cached mode' actual\n+\t\"\n+}\n+\n+test_incompatible_with_recurse_submodules -v\n+test_incompatible_with_recurse_submodules -t\n+test_incompatible_with_recurse_submodules --deleted\n+test_incompatible_with_recurse_submodules --modified\n+test_incompatible_with_recurse_submodules --others\n+test_incompatible_with_recurse_submodules --stage\n+test_incompatible_with_recurse_submodules --killed\n+test_incompatible_with_recurse_submodules --unmerged\n+test_incompatible_with_recurse_submodules --eol\n+\n+test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302348","messageId":"xmqqtwd86f0q.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"1474495472-94190-2-git-send-email-bmwill@google.com","subject":"Re: [PATCH 2/2] ls-files: add pathspec matching for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T22:53:41Z","receivedAt":"2016-09-21T22:53:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Pathspecs can be a bit tricky when trying to apply them to submodules.\n> The main challenge is that the pathspecs will be with respect to the\n> super module and not with respect to paths in the submodule.  The\n> approach this patch takes is to pass in the identical pathspec from the\n> super module to the submodule in addition to the submodule-prefix, which\n> is the path from the root of the super module to the submodule, and then\n> we can compare an entry in the submodule prepended with the\n> submodule-prefix to the pathspec in order to determine if there is a\n> match.\n>\n> This patch also permits the pathspec logic to perform a prefix match against\n> submodules since a pathspec could refer to a file inside of a submodule.\n> Due to limitations in the wildmatch logic, a prefix match is only done\n> literally.  If any wildcard character is encountered we'll simply punt\n> and produce a false positive match.  More accurate matching will be done\n> once inside the submodule.  This is due to the super module not knowing\n> what files could exist in the submodule.\n\nSounds sensible.  Just a minor nit in terminology, but I think we\nfairly consistently say \"a superproject contains submodules\" (run\n\"git grep -E 'super *(module|project)'\").\n\nI'd suggest s/super module/superproject/ for consistency.\n\n> diff --git a/dir.c b/dir.c\n> index 0ea235f..9df6d36 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,\n>  \treturn 1;\n>  }\n>  \n> -#define DO_MATCH_EXCLUDE   1\n> -#define DO_MATCH_DIRECTORY 2\n> +#define DO_MATCH_EXCLUDE   (1<<0)\n> +#define DO_MATCH_DIRECTORY (1<<1)\n> +#define DO_MATCH_SUBMODULE (1<<2)\n>  \n>  /*\n>   * Does 'match' match the given name?\n> @@ -283,6 +284,29 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n>  \t\t\t item->nowildcard_len - prefix))\n>  \t\treturn MATCHED_FNMATCH;\n>  \n> +\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n> +\tif (flags & DO_MATCH_SUBMODULE) {\n> +\t\t/* Check if the name is a literal prefix of the pathspec */\n> +\t\tif ((item->match[namelen] == '/') &&\n> +\t\t    !ps_strncmp(item, match, name, namelen))\n> +\t\t\treturn MATCHED_RECURSIVELY;\n\nAn example of this test would be to match pathspec \"sub/file\" with\nsubmodule path \"sub\"?\n\nitem->match[namelen] is accessed without checking if item->match[]\nis long enough here; shouldn't item->len be checked before doing\nthat?\n\n> +\t\t/*\n> +\t\t * Here is where we would perform a wildmatch to check if\n> +\t\t * \"name\" can be matched as a directory (or a prefix) against\n> +\t\t * the pathspec.  Since wildmatch doesn't have this capability\n> +\t\t * at the present we have to punt and say that it is a match,\n> +\t\t * esentially returning a false positive (as long as \"name\"\n> +\t\t * matches upto the first wild character).\n> +\t\t * The submodules themselves will be able to perform more\n> +\t\t * accurate matching to determine if the pathspec matches.\n> +\t\t */\n> +\t\tif (item->nowildcard_len < item->len &&\n> +\t\t    !ps_strncmp(item, match, name,\n> +\t\t\t\titem->nowildcard_len - prefix))\n> +\t\t\treturn MATCHED_RECURSIVELY;\n\nAn example of this test would be to match pathspec \"su?/file\" with\nsubmodule path \"sub\", where the substring up to nowildcard_len is\nthe leading literal string \"su\" that must match with the path (in\nother words, a path \"sib\" will not match \"su?/file\").\n\n> +\t}\n> +\n\nHmph, isn't this the one that is allowed produce false positive but\ncannot afford to give any false negative?  It feels a bit strange\nthat the code checks two cases where we can positively say that it\nis worth descending into, and falling through would give \"no this\nwill never match\".  That sounds like invitation for false negatives.\n\nIOW, I would have expected\n\n        if (flags & DO_MATCH_SUBMODULE) {\n                if (may match in this case)\n                        return MATCHED_RECURSIVE;\n                if (may match in this other case)\n                        return MATCHED_RECURSIVE;\n                ...\n                if (obviously cannot match in this case)\n                        return 0;\n                if (obviously cannot match in this other case)\n                        return 0;\n                /* otherwise we cannot say */\n                return MATCHED_RECURSIVELY;\n        }\n\nas the general code structure.\n\nFully spelled out,\n\n        if (flags & DO_MATCH_SUBMODULE) {\n                /* Check if the name is a literal prefix of the pathspec */\n                if (namelen < item->len &&\n                    item->match[namelen] == '/' &&\n                    !ps_strncmp(item, match, name, namelen))\n                        return MATCHED_RECURSIVE;\n\n                /* Does the literal leading part have chance of matching? */\n                if (item->nowildcard_len < item->len &&\n                    namelen <= item->nowildcard_len &&\n                    ps_strncmp(item, match, name, namelen))\n                        return 0; /* no way \"su?/file\" can match \"sib\" */\n\n                /* Otherwise we cannot say */\n                return MATCHED_RECURSIVELY;\n        }\n\nor something like that.  There may be other \"obviously cannot match\"\ncases we may want to add further.\n\nThanks.\n"},{"id":"302349","messageId":"xmqqponw6e3x.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"CAKoko1qch_odsEWba0rtCv-DWO0ABS2yprnwGPCgyT6-7H-LdQ@mail.gmail.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T23:13:22Z","receivedAt":"2016-09-21T23:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> yes you mentioned this and I meant to change that before sending it out.\n> Looks like it slipped through have slipped through.\n\nI already fixed it up locally when I sent the reply, but thanks for\nresending (which assures me that your local copy is up-to-date and I\ndo not have to worry about having to repeat me in the future, if\nthis ever needs further rerolling ;-).\n"},{"id":"302350","messageId":"CAKoko1oacXxrSMZBvfM9X6iGDq+KcUUZnUrD2qD3X8+ze8vUXg@mail.gmail.com","threadId":"44127","inReplyTo":"xmqqtwd86f0q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] ls-files: add pathspec matching for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T23:23:12Z","receivedAt":"2016-09-21T23:23:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On Wed, Sep 21, 2016 at 3:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Sounds sensible.  Just a minor nit in terminology, but I think we\n> fairly consistently say \"a superproject contains submodules\" (run\n> \"git grep -E 'super *(module|project)'\").\n>\n> I'd suggest s/super module/superproject/ for consistency.\n\nWill do.\n\n> An example of this test would be to match pathspec \"sub/file\" with\n> submodule path \"sub\"?\n\nYep,  I believe there's a test for that case\n\n> item->match[namelen] is accessed without checking if item->match[]\n> is long enough here; shouldn't item->len be checked before doing\n> that?\n\nOh right!  Good catch.\n\n>\n> Hmph, isn't this the one that is allowed produce false positive but\n> cannot afford to give any false negative?  It feels a bit strange\n> that the code checks two cases where we can positively say that it\n> is worth descending into, and falling through would give \"no this\n> will never match\".  That sounds like invitation for false negatives.\n>\n> IOW, I would have expected\n>\n>         if (flags & DO_MATCH_SUBMODULE) {\n>                 if (may match in this case)\n>                         return MATCHED_RECURSIVE;\n>                 if (may match in this other case)\n>                         return MATCHED_RECURSIVE;\n>                 ...\n>                 if (obviously cannot match in this case)\n>                         return 0;\n>                 if (obviously cannot match in this other case)\n>                         return 0;\n>                 /* otherwise we cannot say */\n>                 return MATCHED_RECURSIVELY;\n>         }\n>\n> as the general code structure.\n>\n> Fully spelled out,\n>\n>         if (flags & DO_MATCH_SUBMODULE) {\n>                 /* Check if the name is a literal prefix of the pathspec */\n>                 if (namelen < item->len &&\n>                     item->match[namelen] == '/' &&\n>                     !ps_strncmp(item, match, name, namelen))\n>                         return MATCHED_RECURSIVE;\n>\n>                 /* Does the literal leading part have chance of matching? */\n>                 if (item->nowildcard_len < item->len &&\n>                     namelen <= item->nowildcard_len &&\n>                     ps_strncmp(item, match, name, namelen))\n>                         return 0; /* no way \"su?/file\" can match \"sib\" */\n>\n>                 /* Otherwise we cannot say */\n>                 return MATCHED_RECURSIVELY;\n>         }\n>\n> or something like that.  There may be other \"obviously cannot match\"\n> cases we may want to add further.\n>\n> Thanks.\n\nYou're right it should be structured the other way.\n"},{"id":"302351","messageId":"1474500486-101640-1-git-send-email-bmwill@google.com","threadId":"44127","inReplyTo":"CAKoko1oacXxrSMZBvfM9X6iGDq+KcUUZnUrD2qD3X8+ze8vUXg@mail.gmail.com","subject":"[PATCH 2/2 v2] ls-files: add pathspec matching for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-21T23:28:06Z","receivedAt":"2016-09-21T23:29:27Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Pathspecs can be a bit tricky when trying to apply them to submodules.\nThe main challenge is that the pathspecs will be with respect to the\nsuperproject and not with respect to paths in the submodule.  The\napproach this patch takes is to pass in the identical pathspec from the\nsuperproject to the submodule in addition to the submodule-prefix, which\nis the path from the root of the superproject to the submodule, and then\nwe can compare an entry in the submodule prepended with the\nsubmodule-prefix to the pathspec in order to determine if there is a\nmatch.\n\nThis patch also permits the pathspec logic to perform a prefix match against\nsubmodules since a pathspec could refer to a file inside of a submodule.\nDue to limitations in the wildmatch logic, a prefix match is only done\nliterally.  If any wildcard character is encountered we'll simply punt\nand produce a false positive match.  More accurate matching will be done\nonce inside the submodule.  This is due to the superproject not knowing\nwhat files could exist in the submodule.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/ls-files.c                     | 132 ++++++++++++++++++++-------------\n dir.c                                  |  46 +++++++++++-\n dir.h                                  |   4 +\n t/t3007-ls-files-recurse-submodules.sh | 114 ++++++++++++++++++++++++++--\n 4 files changed, 234 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex ffd9ea6..fa4029e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -177,12 +177,34 @@ static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n+\tint i;\n \n \targv_array_push(&cp.args, \"ls-files\");\n \targv_array_push(&cp.args, \"--recurse-submodules\");\n \targv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n \t\t\t submodule_prefix ? submodule_prefix : \"\",\n \t\t\t ce->name);\n+\t/* add options */\n+\tif (show_eol)\n+\t\targv_array_push(&cp.args, \"--eol\");\n+\tif (show_valid_bit)\n+\t\targv_array_push(&cp.args, \"-v\");\n+\tif (show_stage)\n+\t\targv_array_push(&cp.args, \"--stage\");\n+\tif (show_cached)\n+\t\targv_array_push(&cp.args, \"--cached\");\n+\tif (debug_mode)\n+\t\targv_array_push(&cp.args, \"--debug\");\n+\n+\t/*\n+\t * Pass in the original pathspec args.  The submodule will be\n+\t * responsible for prepending the 'submodule_prefix' prior to comparing\n+\t * against the pathspec for matches.\n+\t */\n+\targv_array_push(&cp.args, \"--\");\n+\tfor (i = 0; i < pathspec.nr; i++)\n+\t\targv_array_push(&cp.args, pathspec.items[i].original);\n+\n \tcp.git_cmd = 1;\n \tcp.dir = ce->name;\n \tstatus = run_command(&cp);\n@@ -192,57 +214,62 @@ static void show_gitlink(const struct cache_entry *ce)\n \n static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n {\n+\tstruct strbuf name = STRBUF_INIT;\n \tint len = max_prefix_len;\n+\tif (submodule_prefix)\n+\t\tstrbuf_addstr(&name, submodule_prefix);\n+\tstrbuf_addstr(&name, ce->name);\n \n \tif (len >= ce_namelen(ce))\n \t\tdie(\"git ls-files: internal error - cache entry not superset of prefix\");\n \n-\tif (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),\n-\t\t\t    len, ps_matched,\n-\t\t\t    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))\n-\t\treturn;\n-\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {\n+\tif (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&\n+\t    submodule_path_match(&pathspec, name.buf, ps_matched)) {\n \t\tshow_gitlink(ce);\n-\t\treturn;\n-\t}\n+\t} else if (match_pathspec(&pathspec, name.buf, name.len,\n+\t\t\t\t  len, ps_matched,\n+\t\t\t\t  S_ISDIR(ce->ce_mode) ||\n+\t\t\t\t  S_ISGITLINK(ce->ce_mode))) {\n+\t\tif (tag && *tag && show_valid_bit &&\n+\t\t    (ce->ce_flags & CE_VALID)) {\n+\t\t\tstatic char alttag[4];\n+\t\t\tmemcpy(alttag, tag, 3);\n+\t\t\tif (isalpha(tag[0]))\n+\t\t\t\talttag[0] = tolower(tag[0]);\n+\t\t\telse if (tag[0] == '?')\n+\t\t\t\talttag[0] = '!';\n+\t\t\telse {\n+\t\t\t\talttag[0] = 'v';\n+\t\t\t\talttag[1] = tag[0];\n+\t\t\t\talttag[2] = ' ';\n+\t\t\t\talttag[3] = 0;\n+\t\t\t}\n+\t\t\ttag = alttag;\n+\t\t}\n \n-\tif (tag && *tag && show_valid_bit &&\n-\t    (ce->ce_flags & CE_VALID)) {\n-\t\tstatic char alttag[4];\n-\t\tmemcpy(alttag, tag, 3);\n-\t\tif (isalpha(tag[0]))\n-\t\t\talttag[0] = tolower(tag[0]);\n-\t\telse if (tag[0] == '?')\n-\t\t\talttag[0] = '!';\n-\t\telse {\n-\t\t\talttag[0] = 'v';\n-\t\t\talttag[1] = tag[0];\n-\t\t\talttag[2] = ' ';\n-\t\t\talttag[3] = 0;\n+\t\tif (!show_stage) {\n+\t\t\tfputs(tag, stdout);\n+\t\t} else {\n+\t\t\tprintf(\"%s%06o %s %d\\t\",\n+\t\t\t       tag,\n+\t\t\t       ce->ce_mode,\n+\t\t\t       find_unique_abbrev(ce->sha1,abbrev),\n+\t\t\t       ce_stage(ce));\n+\t\t}\n+\t\twrite_eolinfo(ce, ce->name);\n+\t\twrite_name(ce->name);\n+\t\tif (debug_mode) {\n+\t\t\tconst struct stat_data *sd = &ce->ce_stat_data;\n+\n+\t\t\tprintf(\"  ctime: %d:%d\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n+\t\t\tprintf(\"  mtime: %d:%d\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n+\t\t\tprintf(\"  dev: %d\\tino: %d\\n\", sd->sd_dev, sd->sd_ino);\n+\t\t\tprintf(\"  uid: %d\\tgid: %d\\n\", sd->sd_uid, sd->sd_gid);\n+\t\t\tprintf(\"  size: %d\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n \t\t}\n-\t\ttag = alttag;\n \t}\n \n-\tif (!show_stage) {\n-\t\tfputs(tag, stdout);\n-\t} else {\n-\t\tprintf(\"%s%06o %s %d\\t\",\n-\t\t       tag,\n-\t\t       ce->ce_mode,\n-\t\t       find_unique_abbrev(ce->sha1,abbrev),\n-\t\t       ce_stage(ce));\n-\t}\n-\twrite_eolinfo(ce, ce->name);\n-\twrite_name(ce->name);\n-\tif (debug_mode) {\n-\t\tconst struct stat_data *sd = &ce->ce_stat_data;\n-\n-\t\tprintf(\"  ctime: %d:%d\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n-\t\tprintf(\"  mtime: %d:%d\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n-\t\tprintf(\"  dev: %d\\tino: %d\\n\", sd->sd_dev, sd->sd_ino);\n-\t\tprintf(\"  uid: %d\\tgid: %d\\n\", sd->sd_uid, sd->sd_gid);\n-\t\tprintf(\"  size: %d\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n-\t}\n+\tstrbuf_release(&name);\n }\n \n static void show_ru_info(void)\n@@ -566,27 +593,28 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tsetup_work_tree();\n \n \tif (recurse_submodules &&\n-\t    (show_stage || show_deleted || show_others || show_unmerged ||\n-\t     show_killed || show_modified || show_resolve_undo ||\n-\t     show_valid_bit || show_tag || show_eol))\n-\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n-\t\t    \"--cached mode\");\n+\t    (show_deleted || show_others || show_unmerged ||\n+\t     show_killed || show_modified || show_resolve_undo))\n+\t\tdie(\"ls-files --recurse-submodules unsupported mode\");\n \n \tif (recurse_submodules && error_unmatch)\n \t\tdie(\"ls-files --recurse-submodules does not support \"\n \t\t    \"--error-unmatch\");\n \n-\tif (recurse_submodules && argc)\n-\t\tdie(\"ls-files --recurse-submodules does not support path \"\n-\t\t    \"arguments\");\n-\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD |\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+\t/*\n+\t * Find common prefix for all pathspec's\n+\t * This is used as a performance optimization which unfortunately cannot\n+\t * be done when recursing into submodules\n+\t */\n+\tif (recurse_submodules)\n+\t\tmax_prefix = NULL;\n+\telse\n+\t\tmax_prefix = common_prefix(&pathspec);\n \tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n \n \t/* Treat unmatching pathspec elements as errors */\ndiff --git a/dir.c b/dir.c\nindex 0ea235f..28e9736 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,\n \treturn 1;\n }\n \n-#define DO_MATCH_EXCLUDE   1\n-#define DO_MATCH_DIRECTORY 2\n+#define DO_MATCH_EXCLUDE   (1<<0)\n+#define DO_MATCH_DIRECTORY (1<<1)\n+#define DO_MATCH_SUBMODULE (1<<2)\n \n /*\n  * Does 'match' match the given name?\n@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,\n \t\t\t item->nowildcard_len - prefix))\n \t\treturn MATCHED_FNMATCH;\n \n+\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n+\tif (flags & DO_MATCH_SUBMODULE) {\n+\t\t/* name is a literal prefix of the pathspec */\n+\t\tif ((namelen < matchlen) &&\n+\t\t    (match[namelen] == '/') &&\n+\t\t    !ps_strncmp(item, match, name, namelen))\n+\t\t\treturn MATCHED_RECURSIVELY;\n+\n+\t\t/* name\" doesn't match up to the first wild character */\n+\t\tif (item->nowildcard_len < item->len &&\n+\t\t    ps_strncmp(item, match, name,\n+\t\t\t       item->nowildcard_len - prefix))\n+\t\t\treturn 0;\n+\n+\t\t/*\n+\t\t * Here is where we would perform a wildmatch to check if\n+\t\t * \"name\" can be matched as a directory (or a prefix) against\n+\t\t * the pathspec.  Since wildmatch doesn't have this capability\n+\t\t * at the present we have to punt and say that it is a match,\n+\t\t * potentially returning a false positive\n+\t\t * The submodules themselves will be able to perform more\n+\t\t * accurate matching to determine if the pathspec matches.\n+\t\t */\n+\t\treturn MATCHED_RECURSIVELY;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,\n \treturn negative ? 0 : positive;\n }\n \n+/**\n+ * Check if a submodule is a superset of the pathspec\n+ */\n+int submodule_path_match(const struct pathspec *ps,\n+\t\t\t const char *submodule_name,\n+\t\t\t char *seen)\n+{\n+\tint matched = do_match_pathspec(ps, submodule_name,\n+\t\t\t\t\tstrlen(submodule_name),\n+\t\t\t\t\t0, seen,\n+\t\t\t\t\tDO_MATCH_DIRECTORY |\n+\t\t\t\t\tDO_MATCH_SUBMODULE);\n+\treturn matched;\n+}\n+\n int report_path_error(const char *ps_matched,\n \t\t      const struct pathspec *pathspec,\n \t\t      const char *prefix)\ndiff --git a/dir.h b/dir.h\nindex da1a858..97c83bb 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,\n \t\t       const char *pattern, const char *string,\n \t\t       int prefix);\n \n+extern int submodule_path_match(const struct pathspec *ps,\n+\t\t\t\tconst char *submodule_name,\n+\t\t\t\tchar *seen);\n+\n static inline int ce_path_match(const struct cache_entry *ce,\n \t\t\t\tconst struct pathspec *pathspec,\n \t\t\t\tchar *seen)\ndiff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh\nindex caf3815..ca79fda 100755\n--- a/t/t3007-ls-files-recurse-submodules.sh\n+++ b/t/t3007-ls-files-recurse-submodules.sh\n@@ -69,9 +69,111 @@ test_expect_success 'ls-files recurses more than 1 level' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--recurse-submodules does not support using path arguments' '\n-\ttest_must_fail git ls-files --recurse-submodules b 2>actual &&\n-\ttest_i18ngrep \"does not support path arguments\" actual\n+test_expect_success '--recurse-submodules and pathspecs setup' '\n+\techo e >submodule/subsub/e.txt &&\n+\tgit -C submodule/subsub add e.txt &&\n+\tgit -C submodule/subsub commit -m \"adding e.txt\" &&\n+\techo f >submodule/f.TXT &&\n+\techo g >submodule/g.txt &&\n+\tgit -C submodule add f.TXT g.txt &&\n+\tgit -C submodule commit -m \"add f and g\" &&\n+\techo h >h.txt &&\n+\tmkdir sib &&\n+\techo sib >sib/file &&\n+\tgit add h.txt sib/file &&\n+\tgit commit -m \"add h and sib/file\" &&\n+\tgit init sub &&\n+\techo sub >sub/file &&\n+\tgit -C sub add file &&\n+\tgit -C sub commit -m \"add file\" &&\n+\tgit submodule add ./sub &&\n+\tgit commit -m \"added sub\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t.gitmodules\n+\ta\n+\tb/b\n+\th.txt\n+\tsib/file\n+\tsub/file\n+\tsubmodule/.gitmodules\n+\tsubmodule/c\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/d\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules >actual &&\n+\ttest_cmp expect actual &&\n+\tcat actual &&\n+\tgit ls-files --recurse-submodules \"*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\th.txt\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \"*.txt\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\th.txt\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tsubmodule/subsub/e.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \":(icase)*.txt\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\th.txt\n+\tsubmodule/f.TXT\n+\tsubmodule/g.txt\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \":(icase)*.txt\" \":(exclude)submodule/subsub/*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\tsub/file\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \"sub\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"sub/\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"sub/file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"su*/file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"su?/file\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--recurse-submodules and pathspecs' '\n+\tcat >expect <<-\\EOF &&\n+\tsib/file\n+\tsub/file\n+\tEOF\n+\n+\tgit ls-files --recurse-submodules \"s??/file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"s???file\" >actual &&\n+\ttest_cmp expect actual &&\n+\tgit ls-files --recurse-submodules \"s*file\" >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--recurse-submodules does not support --error-unmatch' '\n@@ -82,18 +184,14 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '\n test_incompatible_with_recurse_submodules () {\n \ttest_expect_success \"--recurse-submodules and $1 are incompatible\" \"\n \t\ttest_must_fail git ls-files --recurse-submodules $1 2>actual &&\n-\t\ttest_i18ngrep 'can only be used in --cached mode' actual\n+\t\ttest_i18ngrep 'unsupported mode' actual\n \t\"\n }\n \n-test_incompatible_with_recurse_submodules -v\n-test_incompatible_with_recurse_submodules -t\n test_incompatible_with_recurse_submodules --deleted\n test_incompatible_with_recurse_submodules --modified\n test_incompatible_with_recurse_submodules --others\n-test_incompatible_with_recurse_submodules --stage\n test_incompatible_with_recurse_submodules --killed\n test_incompatible_with_recurse_submodules --unmerged\n-test_incompatible_with_recurse_submodules --eol\n \n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302355","messageId":"20160922041854.7754ujcynhk7mdnh@sigill.intra.peff.net","threadId":"44127","inReplyTo":"xmqqponw6e3x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-22T04:18:54Z","receivedAt":"2016-09-22T04:19:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 21, 2016 at 04:13:22PM -0700, Junio C Hamano wrote:\n\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > yes you mentioned this and I meant to change that before sending it out.\n> > Looks like it slipped through have slipped through.\n> \n> I already fixed it up locally when I sent the reply, but thanks for\n> resending (which assures me that your local copy is up-to-date and I\n> do not have to worry about having to repeat me in the future, if\n> this ever needs further rerolling ;-).\n\nWhile we are on the subject, the commit message also uses some past\ntense:\n\n  Allow ls-files to recognize submodules in order to retrieve a list of\n  files from a repository's submodules.  This is done by forking off a\n  process to recursively call ls-files on all submodules. Also added a\n  submodule-prefix command in order to prepend paths to child processes.\n\nThe final sentence should be \"Also add...\".\n\nSince this final bit of logic was sufficiently non-obvious that it only\ncame about in v2, maybe it is worth describing a little more fully:\n\n  Also add a submodule-prefix option, which instructs the child\n  processes to prepend the prefix to each path they output. This makes\n  the output paths match what is on the filesystem (i.e., as if the\n  submodule boundaries were not there at all).\n\nShould this option just be \"--prefix\", or maybe \"--output-prefix\"?\nSubmodules are the obvious use case here, but I could see somebody\nadapting this for other uses (alternatively, if we _do_ want to keep it\njust as an implementation detail for submodules, we should probably\ndiscourage people in the documentation from using it themselves).\n\n-Peff\n"},{"id":"302358","messageId":"20160922062047.jbxsgfabej26jt5i@sigill.intra.peff.net","threadId":"44127","inReplyTo":"1474497772-97986-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH 1/2] ls-files: optionally recurse into submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-22T06:20:47Z","receivedAt":"2016-09-22T06:21:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 21, 2016 at 03:42:52PM -0700, Brandon Williams wrote:\n\n> @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)\n>  static void write_name(const char *name)\n>  {\n>  \t/*\n> +\t * NEEDSWORK: To make this thread-safe, full_name would have to be owned\n> +\t * by the caller.\n\nI'm not sure if that is quite true. You could simply drop the \"static\"\nhere, and pay the malloc cost each time. But if we want to amortize the\nallocation, then yeah, somebody else on the stack needs to own it.\n\nThat being said, I don't know if it is worth pointing this out as a\nNEEDSWORK or not. Most of git is not thread-safe, so I think that's\ngenerally the norm.\n\n> +\t *\n> +\t * full_name get reused across output lines to minimize the allocation\n> +\t * churn.\n\nLikewise, this kind of strbuf-reuse trickery is common in git. I don't\nknow if it's worth a comment or not (though I mind this one much less,\nbecause it's a bit more subtle, being across multiple calls rather than\nin a single loop).\n\n(To be clear, I actually don't mind either _that_ much; usually the\nproblem is under-commenting, not over-commenting. I am just trying to\ngive style pointers since you are new to the project).\n\n> +\tstatic struct strbuf full_name = STRBUF_INIT;\n> +\tif (submodule_prefix && *submodule_prefix) {\n> +\t\tstrbuf_reset(&full_name);\n> +\t\tstrbuf_addstr(&full_name, submodule_prefix);\n> +\t\tstrbuf_addstr(&full_name, name);\n> +\t\tname = full_name.buf;\n> +\t}\n\nI actually wonder if it would be more obvious if we hoisted out the\nprefix copy, too. IOW, have a global:\n\n  /* used to assemble full names in place */\n  static struct strbuf prefixed_name = STRBUF_INIT;\n  static size_t prefixed_name_base;\n\nthen parse --submodule-prefix as:\n\n  strbuf_reset(&prefixed_name);\n  strbuf_addstr(&prefixed_name, arg);\n  prefixed_name_base = prefixed_name.len;\n\nand then here do:\n\n  if (prefixed_name_base) {\n\tstrbuf_addstr(&prefixed_name, name);\n\tname = prefixed_name.buf;\n  }\n  ... do stuff with name ...\n  if (prefixed_name_base)\n\tstrbuf_setlen(&prefixed_name, prefixed_name_base);\n\nI dunno. It is slightly more efficient (we do not memcpy the prefix over\nand over), but I doubt that matters much in practice. I just wonder if\nit makes the buffer reuse a bit more obvious.\n\n> +/**\n> + * Recursively call ls-files on a submodule\n> + */\n> +static void show_gitlink(const struct cache_entry *ce)\n> +{\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\tint status;\n> +\n> +\targv_array_push(&cp.args, \"ls-files\");\n> +\targv_array_push(&cp.args, \"--recurse-submodules\");\n> +\targv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n> +\t\t\t submodule_prefix ? submodule_prefix : \"\",\n> +\t\t\t ce->name);\n> +\tcp.git_cmd = 1;\n> +\tcp.dir = ce->name;\n> +\tstatus = run_command(&cp);\n> +\tif (status)\n> +\t\texit(status);\n> +}\n\nThis doesn't propagate the parent argv at all. So if I run:\n\n  git ls-files -z --recurse-submodules\n\nthen the paths are all NUL-terminated in the parent, but\nnewline-terminated in the submodules. Oops.\n\nI see in the second patch you make an effort to pass in some specific\noptions explicitly. In some ways that is safer (we do not accidentally\npass an argument that needs to be munged). But in some ways it is less\nsafe (as shown by \"-z\" above, anything we fail to pass in show_gitlink()\nmust be explicitly blocked in cmd_ls_files(), or we produce garbage\noutput). It also means that we have a bunch of boilerplate forwarding\nthe options along (e.g., your second patch lets through --stage, but why\nnot other ones like \"--deleted\"?).\n\nSo I dunno. I would have done it more like:\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 03c283e..2a3d65c 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -31,6 +31,7 @@ static int debug_mode;\n static int show_eol;\n static const char *submodule_prefix;\n static int recurse_submodules;\n+static struct argv_array recurse_argv = ARGV_ARRAY_INIT;\n \n static const char *prefix;\n static int max_prefix_len;\n@@ -170,6 +171,32 @@ static void show_killed_files(struct dir_struct *dir)\n \t}\n }\n \n+/*\n+ * Copy all of the parent arguments, but omit --submodule-prefix, as\n+ * we will be adding our own. Other arguments are assumed to behave\n+ * reasonably in the submodule, or to be blocked explicitly in\n+ * cmd_ls_files().\n+ *\n+ * We have to make a full copy here, because parse_options will munge our\n+ * original.\n+ */\n+static void store_recurse_argv(const char **argv)\n+{\n+\tfor (; *argv; argv++) {\n+\t\tconst char *arg = *argv;\n+\n+\t\t/* Yikes, we're reinventing option parsing here. */\n+\t\tif (starts_with(arg, \"--submodule-prefix=\"))\n+\t\t\tcontinue;\n+\t\tif (!strcmp(arg, \"--submodule-prefix\")) {\n+\t\t\tif (argv[1])\n+\t\t\t\targv++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\targv_array_push(&recurse_argv, arg);\n+\t}\n+}\n+\n /**\n  * Recursively call ls-files on a submodule\n  */\n@@ -177,9 +204,10 @@ static void show_gitlink(const struct cache_entry *ce)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint status;\n+\tint i;\n \n-\targv_array_push(&cp.args, \"ls-files\");\n-\targv_array_push(&cp.args, \"--recurse-submodules\");\n+\tfor (i = 0; i < recurse_argv.argc; i++)\n+\t\targv_array_push(&cp.args, recurse_argv.argv[i]);\n \targv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n \t\t\t submodule_prefix ? submodule_prefix : \"\",\n \t\t\t ce->name);\n@@ -535,6 +563,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n \n+\tstore_recurse_argv(argv);\n+\n \targc = parse_options(argc, argv, prefix, builtin_ls_files_options,\n \t\t\tls_files_usage, 0);\n \tel = add_exclude_list(&dir, EXC_CMDL, \"--exclude option\");\n@@ -565,13 +595,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (require_work_tree && !is_inside_work_tree())\n \t\tsetup_work_tree();\n \n-\tif (recurse_submodules &&\n-\t    (show_stage || show_deleted || show_others || show_unmerged ||\n-\t     show_killed || show_modified || show_resolve_undo ||\n-\t     show_valid_bit || show_tag || show_eol))\n-\t\tdie(\"ls-files --recurse-submodules can only be used in \"\n-\t\t    \"--cached mode\");\n-\n \tif (recurse_submodules && error_unmatch)\n \t\tdie(\"ls-files --recurse-submodules does not support \"\n \t\t    \"--error-unmatch\");\n\nwhich I admit is somewhat ugly, due to the \"yikes\" comment above. For\nexample, I think parse_options() will actually allow a unique prefix\nlike \"--submodule-pre=foo/\", which we would not catch. Or worse, we\nwould accidentally munge the pathspec in:\n\n  git ls-files --recurse-submodules -- --submodule-prefix=foo\n\n(or any case where a non-option looked like \"--submodule-prefix=\".\nObviously those are pathological, but it's still nasty.\n\nI think we _could_ actually avoid picking out the --submodule-prefix\nargument entirely, and instead just add our own at the end to override\nit.\n\nI don't think it actually matters that much here.  You _could_ get away\nwith even leaving the old --submodule-prefix in place, and just adding\nour new one at the end to override it. That works, though it does mean\nthat your argv gets continually longer as you recurse modules. Or I\nsuppose another way would be to make the prefix option additive, and\nthen just literally add \"--prefix-add=$name\" at each level. The option\nparser would then build up the real prefix from multiple prefix-add\noptions.\n\nThe final thing I could think of is that we could teach parse_options()\nto record a canonicalized copy of all of the options in an argv_array.\nSo it would normalize \"--submodule-pre foo\" to \"--submodule-prefix=foo\",\nand that makes our after-the-fact parsing much easier.\n\nBut like I said, I dunno. I'm on the fence on which approach is the\nleast ugly (including your original, to just forward specific options).\nThey all have their warts. :)\n\n-Peff\n"},{"id":"302376","messageId":"CAGZ79kYQVhVN1mzJCWCH8bLEFTmgL8yqrN=KUiAY5PT1t7-D6A@mail.gmail.com","threadId":"44127","inReplyTo":"20160922041854.7754ujcynhk7mdnh@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-22T16:04:35Z","receivedAt":"2016-09-22T16:04:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 21, 2016 at 9:18 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Sep 21, 2016 at 04:13:22PM -0700, Junio C Hamano wrote:\n>\n>> Brandon Williams <bmwill@google.com> writes:\n>>\n>> > yes you mentioned this and I meant to change that before sending it out.\n>> > Looks like it slipped through have slipped through.\n>>\n>> I already fixed it up locally when I sent the reply, but thanks for\n>> resending (which assures me that your local copy is up-to-date and I\n>> do not have to worry about having to repeat me in the future, if\n>> this ever needs further rerolling ;-).\n>\n> While we are on the subject, the commit message also uses some past\n> tense:\n>\n>   Allow ls-files to recognize submodules in order to retrieve a list of\n>   files from a repository's submodules.  This is done by forking off a\n>   process to recursively call ls-files on all submodules. Also added a\n>   submodule-prefix command in order to prepend paths to child processes.\n>\n> The final sentence should be \"Also add...\".\n>\n> Since this final bit of logic was sufficiently non-obvious that it only\n> came about in v2, maybe it is worth describing a little more fully:\n>\n>   Also add a submodule-prefix option, which instructs the child\n>   processes to prepend the prefix to each path they output. This makes\n>   the output paths match what is on the filesystem (i.e., as if the\n>   submodule boundaries were not there at all).\n>\n> Should this option just be \"--prefix\", or maybe \"--output-prefix\"?\n\nI think --prefix can easily be confused with the internal prefix that we hand\ndown to each command. --output-prefix works for the ls-files case, but as\nsoon as we do more than just printing out file names, we'd use that prefix for\nmore than just output, e.g. in grep it becomes part of the pathspec IIUC.\nI agree however that we probably don't want to keep it submodule specific.\n\n> Submodules are the obvious use case here, but I could see somebody\n> adapting this for other uses (alternatively, if we _do_ want to keep it\n> just as an implementation detail for submodules, we should probably\n> discourage people in the documentation from using it themselves).\n>\n> -Peff\n"},{"id":"302381","messageId":"xmqq7fa36bwm.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"20160922041854.7754ujcynhk7mdnh@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-22T18:13:13Z","receivedAt":"2016-09-22T18:14:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Should this option just be \"--prefix\", or maybe \"--output-prefix\"?\n> Submodules are the obvious use case here, but I could see somebody\n> adapting this for other uses (alternatively, if we _do_ want to keep it\n> just as an implementation detail for submodules, we should probably\n> discourage people in the documentation from using it themselves).\n\nI agree that this is not specific to submodules; this is closely\nrelated to what we internally call \"prefix\", but is different.\n\nIn any case, I would strongly recommend against exposing this (or\nanything for that matter) \"--prefix\" to the end-user, especially\nbecause this feature is likely to be applicable to many subcommands,\nand some subcommands would want different sort of prefixing made to\ndifferent things.  Think of \"git diff\" that has a way to customize\nthe \"a/\" and \"b/\" part in \"git --diff a/$path b/$path\", and has\nlearned another way to prepend an additional prefix to every line of\noutput via \"--line-prefix\".  We want to give reasonably specific\nnames to things so that readers can tell what it affects from the\nname of an option.\n\nWhat we internally call \"prefix\" and \"--submodule-prefix\" is closely\nrelated in that they both interact with pathspecs.  \"prefix\" gets\nprepended to elements of an end-user supplied pathspec before a\nfull-path-in-the-repository (i.e. a path in the index and a path\nrelative from the top of the working tree) is matched against them.\nThis new thing on the other hand allows the leading part of pathspec\nelements to be above the full-path-in-the-repository.  For example,\nmy primary Git working area is at ~/w/git.git and I could say\n\n    $ cd ~\n    $ git -C w/git.git/ ls-files \\\n        --submodule-prefix=w/git.git -- 'w/git.git/D\\*' |\n      xargs ls -1 -l\n\nThe command starts (eh, at least \"pretends to start\") in the top of\nmy working tree, which means that the \"prefix\" is NULL.  But the\nshell that spawns the \"git\" command is still sitting in my home\ndirectory, and viewed from there, the Documentation subdirectory is\nat w/git.git/Documentation/, which is what I gave the command as the\nsole element of the pathspec.  From \"ls-files\"'s point of view, each\npath it discovers in the index (and in the working tree) is first\nprefixed with --submodule-prefix before it gets matched against this\npathspec and the result is shown with this prefix, so that the\noutput gets relative to the calling shell and xargs would find them\nat expected places.  The --submodule-prefix acts to cancel out the\nfact that the caller sits a few levels above the working tree and\ngave a pathspec with elements relative to that higher level in the\ndirectory hierarchy.\n\nObviously, the above example is *not* using submodules at all, and\ndemonstrates that the mechanism does not have to be used solely to\nimplement recurse-into-submodules behaviour, so in that sense, the\nname of the option is not quite accurate.  It however is a good\ndemonstration why it is *not* \"output prefix\".  It makes me wonder\nif pathspec-prefix is a better name, but that may give an incorrect\nimpression that this would be used only for matching and would not\naffect the output.  Do we need separate options to specify what gets\nprefixed to the in-repository path before paths are matched against\na pathspec (i.e. \"--pathspec-prefix\") and what gets prefixed to the\nresulting paths in the output (i.e. \"--output-path-prefix\")?\n\nA few further random thoughts.\n\n * As Stefan alluded to (much) earlier, it might be a better idea\n   to have these 'prefix' as the global option to \"git\" potty, not\n   to each subcommand that happens to support them;\n\n * It is unclear how this should interact with commands that are run\n   in a subdirectory of the working tree.  E.g. what should the\n   prefix and the pathspec look like if the command in the above\n   example is started in w/git.git/Documentation subdirectory, i.e.\n\n    $ cd ~\n    $ git -C w/git.git/Documentation ls-files \\\n        --submodule-prefix=??????? -- '???????' |\n      xargs ls -1 -l\n\n   Should we error out if we are not at the top of the working tree\n   when --submodule-prefix is given?\n\n * It is further unclear how this should interact with commands that\n   can give \"relative\" paths output if we allowed them to start from\n   a subdirectory of the working tree.  E.g. \"git grep\" gives its\n   output relative to where it was started from:\n\n    $ cd ~/w/git.git/Documentation\n    $ git grep -c 'ERROR.*frotz' -- '*.txt'\n    git-checkout.txt:1\n\n   How should it interact with the --submodule-prefix option and\n   what value should the caller give to the option and how should\n   the caller adjust the pathspec if it wants to get paths relative\n   to my home directory?  I do not think the following is correct,\n   but I am not sure what the correct values of them should be:\n\n    $ cd ~\n    $ git -C w/git.git/Documentation \\\n      --submodule-prefix=w/git.git/ \\\n      grep -c 'ERROR.*frotz' -- 'w/git.git/Documentation/*.txt'\n\n   This becomes a non-issue if we forbid use of this new prefix when\n   \"git\" is not started at the top of the working tree.\n\n"},{"id":"302422","messageId":"20160923034113.4rnps3nogvzxkfjx@sigill.intra.peff.net","threadId":"44127","inReplyTo":"xmqq7fa36bwm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-23T03:41:13Z","receivedAt":"2016-09-23T03:41:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2016 at 11:13:13AM -0700, Junio C Hamano wrote:\n\n> In any case, I would strongly recommend against exposing this (or\n> anything for that matter) \"--prefix\" to the end-user, especially\n> because this feature is likely to be applicable to many subcommands,\n> and some subcommands would want different sort of prefixing made to\n> different things.\n\nFair enough. I was thinking that this was similar to other options like\n\"read-tree --prefix\" or \"archive --prefix\". But if nobody really wants\nit for anything non-internal, then certainly keeping it as an internal\nfeature is an easy way to avoid being stuck with a bad public interface\nin the long term.\n\n> What we internally call \"prefix\" and \"--submodule-prefix\" is closely\n> related in that they both interact with pathspecs.\n\nYeah, I didn't think about pathspecs at all (since they are totally\ndisabled in patch 1, and I hadn't really read through patch 2 carefully\nyet).\n\n>  * As Stefan alluded to (much) earlier, it might be a better idea\n>    to have these 'prefix' as the global option to \"git\" potty, not\n>    to each subcommand that happens to support them;\n\nThat seems like it would be nice, but there's going to be an interim\nperiod where some commands do not respect the global \"--prefix\" at all\n(in the worst case, consider a third party command).\n\n>  * It is unclear how this should interact with commands that are run\n>    in a subdirectory of the working tree.  E.g. what should the\n>    prefix and the pathspec look like if the command in the above\n>    example is started in w/git.git/Documentation subdirectory, i.e.\n> \n>     $ cd ~\n>     $ git -C w/git.git/Documentation ls-files \\\n>         --submodule-prefix=??????? -- '???????' |\n>       xargs ls -1 -l\n> \n>    Should we error out if we are not at the top of the working tree\n>    when --submodule-prefix is given?\n\nWithout thinking too hard on it, it seems like the submodule prefix\njust needs to come after the normal \"prefix\" that we add when moving to\nthe top-level of a tree. So:\n\n  cd foo\n  git ls-files --submodule-prefix=bar\n\nshould show \"foo/bar\". Or another way of thinking about it is that the\nsubmodule prefix is always relative to the current directory. Recursion\ninto submodule would always happen at their top-level, and so would do\nthe right thing.\n\nBut again, that's without thinking hard on it. There may be some corner\ncases.\n\n-Peff\n"},{"id":"302430","messageId":"CAGZ79kZ+LTd5PuT4+Z9K6d+GQ-33E=tLY1Fokbp22uNoygaEtg@mail.gmail.com","threadId":"44127","inReplyTo":"20160923034113.4rnps3nogvzxkfjx@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-23T05:47:17Z","receivedAt":"2016-09-23T05:47:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 22, 2016 at 8:41 PM, Jeff King <peff@peff.net> wrote:\n\n>>  * As Stefan alluded to (much) earlier, it might be a better idea\n>>    to have these 'prefix' as the global option to \"git\" potty, not\n>>    to each subcommand that happens to support them;\n>\n> That seems like it would be nice, but there's going to be an interim\n> period where some commands do not respect the global \"--prefix\" at all\n> (in the worst case, consider a third party command).\n\nMy current line of thinking is to have a new flag in command struct in\ngit.c to enable the global --prefix, (c.f. RUN_SETUP | NEED_WORK_TREE)\nso we'd have a ALLOW_OUTSIDE_PREFIX flag which can be used to enable\nthis feature. In case that flag is not set, but a user tries a\n--prefix=<somewhere>\nwe can still\n\n    die(\"nope, we don't do that\");\n"},{"id":"302431","messageId":"20160923060643.3ubr5gn7qczzs2ut@sigill.intra.peff.net","threadId":"44127","inReplyTo":"CAGZ79kZ+LTd5PuT4+Z9K6d+GQ-33E=tLY1Fokbp22uNoygaEtg@mail.gmail.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-23T06:06:43Z","receivedAt":"2016-09-23T06:06:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2016 at 10:47:17PM -0700, Stefan Beller wrote:\n\n> On Thu, Sep 22, 2016 at 8:41 PM, Jeff King <peff@peff.net> wrote:\n> \n> >>  * As Stefan alluded to (much) earlier, it might be a better idea\n> >>    to have these 'prefix' as the global option to \"git\" potty, not\n> >>    to each subcommand that happens to support them;\n> >\n> > That seems like it would be nice, but there's going to be an interim\n> > period where some commands do not respect the global \"--prefix\" at all\n> > (in the worst case, consider a third party command).\n> \n> My current line of thinking is to have a new flag in command struct in\n> git.c to enable the global --prefix, (c.f. RUN_SETUP | NEED_WORK_TREE)\n> so we'd have a ALLOW_OUTSIDE_PREFIX flag which can be used to enable\n> this feature. In case that flag is not set, but a user tries a\n> --prefix=<somewhere>\n> we can still\n> \n>     die(\"nope, we don't do that\");\n\nYeah, a positive \"I support this\" flag would at least let us correctly\nflag errors, which is the best we can do. That won't work for\nnon-builtins, but perhaps it is good enough in practice.\n\n-Peff\n"},{"id":"302448","messageId":"CAKoko1p+M3X_y+DH+65fUDRZjkiZrKE3Z-jjq=RSjqoi7He8pw@mail.gmail.com","threadId":"44127","inReplyTo":"20160923060643.3ubr5gn7qczzs2ut@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-23T16:16:18Z","receivedAt":"2016-09-23T16:16:25Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"> Yeah, a positive \"I support this\" flag would at least let us correctly\n> flag errors, which is the best we can do. That won't work for\n> non-builtins, but perhaps it is good enough in practice.\n>\n> -Peff\n\n\nSo it sounds like we agree that this prefix option should be pushed to\nthe top level.\nThe question is have we come to a consensus on what we should be\ncalling the option?\nLeave it as submodule-prefix or do we need to come up with a different name?\n\n-Brandon\n"},{"id":"302449","messageId":"CAGZ79kYF6nYL42VQ_mbVzD+2QFLPELe9-8DOQjbSwArM20SCKA@mail.gmail.com","threadId":"44127","inReplyTo":"CAKoko1p+M3X_y+DH+65fUDRZjkiZrKE3Z-jjq=RSjqoi7He8pw@mail.gmail.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-23T16:34:51Z","receivedAt":"2016-09-23T16:34:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Sep 23, 2016 at 9:16 AM, Brandon Williams <bmwill@google.com> wrote:\n>> Yeah, a positive \"I support this\" flag would at least let us correctly\n>> flag errors, which is the best we can do. That won't work for\n>> non-builtins, but perhaps it is good enough in practice.\n>>\n>> -Peff\n>\n>\n> So it sounds like we agree that this prefix option should be pushed to\n> the top level.\n> The question is have we come to a consensus on what we should be\n> calling the option?\n\nThe option itself is very similar to -C, which changes the directory to the\ngiven argument before executing the git command.\ne.g. in git:\n\n    git -C builtin ls-files\n    add.c\n    ...\n\nSo for the submodule case we'd want that plus keeping around that prefix,\nwhich makes me wonder if we could just store the argument of -C into a global\nand use that when --keep-prefix is given, so you'd do a\n\n    git -C path/to/sub --keep-prefix ls-files\n    path/to/sub/file1\n    ...\n\nmaybe --[keep|use]-[path|prefix] ?\n\nYou could of course go with a fully independent option, but how\nwould that work together with -C ?\n(first change the dir and then change again while remembering the prefix?\nor the other way round?)\n\n> Leave it as submodule-prefix or do we need to come up with a different name?\n>\n> -Brandon\n"},{"id":"302454","messageId":"xmqqfuoq1mh9.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"1474500486-101640-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-23T18:48:18Z","receivedAt":"2016-09-23T18:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> -\t/* Find common prefix for all pathspec's */\n> -\tmax_prefix = common_prefix(&pathspec);\n> +\t/*\n> +\t * Find common prefix for all pathspec's\n> +\t * This is used as a performance optimization which unfortunately cannot\n> +\t * be done when recursing into submodules\n> +\t */\n> +\tif (recurse_submodules)\n> +\t\tmax_prefix = NULL;\n> +\telse\n> +\t\tmax_prefix = common_prefix(&pathspec);\n>  \tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n\nThis is OK for now, but for a future enhancement, I think we could\ndo better than this.  In a superproject with a submodule at \"sub/\",\nthe current implementation of the common_prefix() helper would yield\n\"sub/a/\" when given \"sub/a/x\" and \"sub/a/y\" (a pathspec with two\nelements), which we want to avoid.\n\nBut somebody should be able to notice, before \"sub/a/\" is given to\nmax_prefix here, that \"sub/\" is the leaf level in our repository and\nreduce the max_prefix to it.  dir.c::common_prefix_len() might be \na place we could do so, but I didn't think about the ramifications\nof doing so for other callers of common_prefix() or when we are not\nrecursing into submodules.  Doing it in the caller here, i.e.\n\n\tmax_prefix = common_prefix(&pathspec);\n        if (recurse_submodules)\n        \tmax_prefix = chomp_at_submodule_boundary(max_prefix);\n\nis certainly safer.\n\nIf the superproject has submodules at \"a/b/{sub1,sub2,...}\", this\nmatters more.  We do want to notice that we won't have to scan\noutside \"a/b/\" of the index given \"a/b/sub1\" and \"a/b/sub2\" as a\npathspec.\n\nThe common_prefix_len() function also looks beyond symbolic links,\nwhich is another thing that we may want to think about.  In a\nrepository with a symbolic link \"link\" pointing somewhere else, when\nyou give \"link/a/x\" and \"link/a/y\" (a pathspec with two elements),\nwe would get \"link/a/\" as a common prefix, but we won't find\nanything underneath \"link\" in our index.  In such a case, leaving\nthe common prefix to \"link/a/\" _might_ allow us to notice that no\npathspec elements can ever match, so not noticing that the common\nprefix points beyond a symbolic link might be a feature.  I dunno.\n"},{"id":"302456","messageId":"xmqqbmze1kzt.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"1474500486-101640-1-git-send-email-bmwill@google.com","subject":"Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-23T19:20:22Z","receivedAt":"2016-09-23T19:20:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n>  static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n>  {\n> +\tstruct strbuf name = STRBUF_INIT;\n>  \tint len = max_prefix_len;\n> +\tif (submodule_prefix)\n> +\t\tstrbuf_addstr(&name, submodule_prefix);\n> +\tstrbuf_addstr(&name, ce->name);\n> ...  \n> +\t} else if (match_pathspec(&pathspec, name.buf, name.len,\n> +\t\t\t\t  len, ps_matched,\n> +\t\t\t\t  S_ISDIR(ce->ce_mode) ||\n> +\t\t\t\t  S_ISGITLINK(ce->ce_mode))) {\n\nThere is an interesting observation around this code.  Note that it\nis just something to keep in mind, even though I think we are in no\nposition to solve this within the scope of this series, or in fact I\nam not sure if there is anything to \"fix\".\n\nThe expectation here is that the leading part of pathspec elements\ncontain path components above and outside the current working tree,\ne.g. in a superproject with a submodule at \"sub/\", the end-user may\nhave said from the top of the superproject\n\n    git ls-files --recurse-submodules -- sub/file\n\nand the recursing \"ls-files\" is spawned as\n\n    git -C sub ls-files -- sub/file\n\nrelaying the pathspec literally.\n\nThis does not correctly work if the path to the submodule has\nwildcard in it.  Imagine that the submodule were at \"s*b/\".  The\nrecursing invocation would look like:\n\n    git -C \"s*b\" ls-files -- \"s*b/file\"\n\nFurther imagine that the index in the submodule at \"s*b\" has two\npaths in it, i.e.\n\n\tfile\n        oob/file\n\nThe prefix is prepended to them, to turn them into\n\n\ts*b/file\n        s*b/oob/file\n\nand I suspect that the pathspec element \"s*b/file\" would match both\nof them.\n\nThe pathspec machinery has a provision to prevent a similar gotcha\nhappening for the \"prefix\" we internally use.  In a sample\nrepository created like so:\n\n    $ git init\n    $ mkdir -p 's*b/oob' sib\n    $ >sib/file\n    $ cd 's*b'\n    $ >file\n    $ >oob/file\n    $ git add .\n    $ git ls-files -- file\n\nthe \"ls-files\" in the last step gets 's*b/' as the \"prefix\", and the\npathspec is formed by concatenating \"file\" to it, but in a special\nway.  The part that come from the \"prefix\" is marked not to honor\nany wildcard in it, so 's*b/' even though it has an asterisk, it is\nforced to match literally, giving only 's*b/file'.\n\nA saving grace is that \"s*b/file\" in this case is what the end-user\nis giving us, not something we internally generated.  So we can\nsimply blame the end user, saying \"what --recurse-submodules does is\nto (conceptually) flatten the indices of submodules into the index\nof the superproject and show the entries that match your pathspec.\nBecause you gave us 's*b/file', which does match 's*b/oob/file',\nthat is what you get.\"\n\n;-)\n"},{"id":"302463","messageId":"CAKoko1qZCBPx9T4WoYZc0LdDhU5bpUaGa+emFL-1RvVU4MX8Ow@mail.gmail.com","threadId":"44127","inReplyTo":"xmqqbmze1kzt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-23T20:49:46Z","receivedAt":"2016-09-23T20:49:53Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On Fri, Sep 23, 2016 at 12:20 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> There is an interesting observation around this code.  Note that it\n> is just something to keep in mind, even though I think we are in no\n> position to solve this within the scope of this series, or in fact I\n> am not sure if there is anything to \"fix\".\n>\n> The expectation here is that the leading part of pathspec elements\n> contain path components above and outside the current working tree,\n> e.g. in a superproject with a submodule at \"sub/\", the end-user may\n> have said from the top of the superproject\n>\n> A saving grace is that \"s*b/file\" in this case is what the end-user\n> is giving us, not something we internally generated.  So we can\n> simply blame the end user, saying \"what --recurse-submodules does is\n> to (conceptually) flatten the indices of submodules into the index\n> of the superproject and show the entries that match your pathspec.\n> Because you gave us 's*b/file', which does match 's*b/oob/file',\n> that is what you get.\"\n>\n> ;-)\n\nYeah I've been thinking a bit about that as well.  To me, it is\nincredibly silly to\nhave a wildcard character in a filename (its unfortunate that its\nallowed).  We can\neasily do as you suggest and simply blame the user and if they do have wildcard\ncharacters in their filenames they would just need to force the\npathspec code to\ndo checks literally (using the appropriate pathspec magic).  This\nwould just limit their\nability to use actual wildcards in their pathspecs, ie they have to\npick wildcards in their\nfilenames or the ability to do wildmatching.\n\n-Brandon\n"},{"id":"302471","messageId":"CAKoko1od4=PtHkYaU5bdktH4orL_k_G32M=3svgFGwvy6BKKsw@mail.gmail.com","threadId":"44127","inReplyTo":"20160922062047.jbxsgfabej26jt5i@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] ls-files: optionally recurse into submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-23T23:31:13Z","receivedAt":"2016-09-23T23:31:19Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On Wed, Sep 21, 2016 at 11:20 PM, Jeff King <peff@peff.net> wrote:\n>> +/**\n>> + * Recursively call ls-files on a submodule\n>> + */\n>> +static void show_gitlink(const struct cache_entry *ce)\n>> +{\n>> +     struct child_process cp = CHILD_PROCESS_INIT;\n>> +     int status;\n>> +\n>> +     argv_array_push(&cp.args, \"ls-files\");\n>> +     argv_array_push(&cp.args, \"--recurse-submodules\");\n>> +     argv_array_pushf(&cp.args, \"--submodule-prefix=%s%s/\",\n>> +                      submodule_prefix ? submodule_prefix : \"\",\n>> +                      ce->name);\n>> +     cp.git_cmd = 1;\n>> +     cp.dir = ce->name;\n>> +     status = run_command(&cp);\n>> +     if (status)\n>> +             exit(status);\n>> +}\n>\n> This doesn't propagate the parent argv at all. So if I run:\n>\n>   git ls-files -z --recurse-submodules\n>\n> then the paths are all NUL-terminated in the parent, but\n> newline-terminated in the submodules. Oops.\n\nYep definitely missed that.  I can fix that.  I think the main reason\nfor not blindly\ncopying the argv array is that there may be some things we don't want to pass\nto the child. While not in the context of ls-files, I was working on\nrecursive grep\nearlier and with that you can pass a rev to grep.  You can't blindly\ncopy that because\n the rev is meaningless to the the child and may produce broken\noutput.  Instead we\nwould need to pass the actual rev of what the parent has checked out\nin that particular\nrev.  I haven't thought it completely through yet but it did\ndiscourage me from blindly\ncopying the args across.\n\n-Brandon\n"},{"id":"302512","messageId":"CAEY4ZpMm3o2fNb6_3zQYq5x2_EDquGoBPyoafGrrfYQ9B0qouA@mail.gmail.com","threadId":"44127","inReplyTo":"CAGZ79kYF6nYL42VQ_mbVzD+2QFLPELe9-8DOQjbSwArM20SCKA@mail.gmail.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Nazri Ramliy","fromEmail":"ayiehere@gmail.com","sentAt":"2016-09-25T11:03:20Z","receivedAt":"2016-09-25T11:03:28Z","isPatch":true,"sender":{"key":"ayiehere@gmail.com","avatar":"https://avatars.githubusercontent.com/u/164756?v=4"},"body":"On Sat, Sep 24, 2016 at 12:34 AM, Stefan Beller <sbeller@google.com> wrote:\n>     git -C path/to/sub --keep-prefix ls-files\n\nNote that the above can also be written like so (and works\nidentically), due to the fact that -C can be given multiple times:\n\n    git -C path -C to -C sub --keep-prefix ls-files\n\nnazri\n"},{"id":"302771","messageId":"xmqq60phm39w.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"xmqq7fa36bwm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T21:38:51Z","receivedAt":"2016-09-27T21:39:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As this is relevant to what to call the prefix thing that is passed\ndown to an internal re-invocation of ls-files and how to explain it\nto end-users...\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n> I agree that this is not specific to submodules; this is closely\n> related to what we internally call \"prefix\", but is different.\n>\n> In any case, I would strongly recommend against exposing this (or\n> anything for that matter) \"--prefix\" to the end-user,...\n\nI have a slight suspicion that this was what made you say earlier\nthat I was against exposing this at all to end users, but what I\nmeant was a plain boring \"prefix\" is a bad name and nothing more\nthan that.\n\n> What we internally call \"prefix\" and \"--submodule-prefix\" is closely\n> related in that they both interact with pathspecs.  \"prefix\" gets\n> prepended to elements of an end-user supplied pathspec before a\n> full-path-in-the-repository (i.e. a path in the index and a path\n> relative from the top of the working tree) is matched against them.\n\nIn a sense, this new thing is a superset of the existing GIT_PREFIX,\nwhich gives the current working directory from the end user's point\nof view relative to the actual current working directory.  The \"git\"\nwrapper, when running a \"git thing\" from a subdirectory of a working\ntree, chdir(2)s up to the top-level before spawning the \"thing\"\nsubcommand that is not built-in or a third-party \"git-thing\" binary\non user's $PATH; so \"git-thing\" binary will be told via $GIT_PREFIX\nrelative to what directory path-like things the end user supplied to\nthe command (e.g. \"-o outfile\" argument and pathspecs) need to be\ninterpreted.  The new thing can override the $GIT_PREFIX to allow\npath-like things to be interpreted relative to somewhere _above_ the\ntop-level of the working tree.\n\nI am tempted to suggest GIT_SUPER_PREFIX, 50% because I cannot think\nof any better word, 40% because I think it actually makes sense (in\nthe sense that this comes _above_ it, hence \"super\", and also in the\nsense that this is something your \"super\" project would give you),\nand 10% because I hope that being ridiculous would nudge people to\ncome up with a better alternative ;-)\n\nAnd from that point of view ...\n\n>  * It is unclear how this should interact with commands that are run\n>    in a subdirectory of the working tree.  E.g. what should the\n>    prefix and the pathspec look like if the command in the above\n>    example is started in w/git.git/Documentation subdirectory, i.e.\n>\n>     $ cd ~\n>     $ git -C w/git.git/Documentation ls-files \\\n>         --submodule-prefix=??????? -- '???????' |\n>       xargs ls -1 -l\n>\n>    Should we error out if we are not at the top of the working tree\n>    when --submodule-prefix is given?\n\n... the answer to this question becomes clear.  It is not possible\nto _be_ in a subdirectory \"Documentation\" of this working tree and\nin a directory \"~\" above this working tree at the same time, so we\nsimply should forbid running the command from anywhere other than\nthe top of the working tree (i.e. the internal \"prefix\" and\nGIT_PREFIX must be empty) when the super-prefix is set by erroring\nit out.  When we realize that \"prefix\" adds to the paths that are\nsupplied by the user (e.g. when the user says Makefile while in t/\nsubdirectory, i.e. GIT_PREFIX=t/, s/he means t/Makefile), but this\nnew thing subtracts from the paths given by the user (e.g. when the\nuser gives a pathspec 'w/git.git/D*' while setting the super thing\nas w/git.git, because s/he is at ~/, the pathspec matcher\nconceptually subtracts w/git.git/ from the pathspec before matching\nthem against the paths it finds in the index), it becomes clear that\ngiving both at the same time is awkward and not very useful.\n"},{"id":"302773","messageId":"20160927214854.GA180705@google.com","threadId":"44127","inReplyTo":"xmqq60phm39w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-27T21:48:54Z","receivedAt":"2016-09-27T21:49:03Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/27, Junio C Hamano wrote:\n> > What we internally call \"prefix\" and \"--submodule-prefix\" is closely\n> > related in that they both interact with pathspecs.  \"prefix\" gets\n> > prepended to elements of an end-user supplied pathspec before a\n> > full-path-in-the-repository (i.e. a path in the index and a path\n> > relative from the top of the working tree) is matched against them.\n> \n> In a sense, this new thing is a superset of the existing GIT_PREFIX,\n> which gives the current working directory from the end user's point\n> of view relative to the actual current working directory.  The \"git\"\n> wrapper, when running a \"git thing\" from a subdirectory of a working\n> tree, chdir(2)s up to the top-level before spawning the \"thing\"\n> subcommand that is not built-in or a third-party \"git-thing\" binary\n> on user's $PATH; so \"git-thing\" binary will be told via $GIT_PREFIX\n> relative to what directory path-like things the end user supplied to\n> the command (e.g. \"-o outfile\" argument and pathspecs) need to be\n> interpreted.  The new thing can override the $GIT_PREFIX to allow\n> path-like things to be interpreted relative to somewhere _above_ the\n> top-level of the working tree.\n> \n> I am tempted to suggest GIT_SUPER_PREFIX, 50% because I cannot think\n> of any better word, 40% because I think it actually makes sense (in\n> the sense that this comes _above_ it, hence \"super\", and also in the\n> sense that this is something your \"super\" project would give you),\n> and 10% because I hope that being ridiculous would nudge people to\n> come up with a better alternative ;-)\n\nhaha, yeah it seems difficult to come up with a name that is going to\nmean the same thing in 5 years from now.  At least GIT_SUPER_PREFIX\nmakes senese in that it is a path from the superproject down to the\nsubmodule.\n\n> >  * It is unclear how this should interact with commands that are run\n> >    in a subdirectory of the working tree.  E.g. what should the\n> >    prefix and the pathspec look like if the command in the above\n> >    example is started in w/git.git/Documentation subdirectory, i.e.\n> >\n> >     $ cd ~\n> >     $ git -C w/git.git/Documentation ls-files \\\n> >         --submodule-prefix=??????? -- '???????' |\n> >       xargs ls -1 -l\n> >\n> >    Should we error out if we are not at the top of the working tree\n> >    when --submodule-prefix is given?\n> \n> ... the answer to this question becomes clear.  It is not possible\n> to _be_ in a subdirectory \"Documentation\" of this working tree and\n> in a directory \"~\" above this working tree at the same time, so we\n> simply should forbid running the command from anywhere other than\n> the top of the working tree (i.e. the internal \"prefix\" and\n> GIT_PREFIX must be empty) when the super-prefix is set by erroring\n> it out.  When we realize that \"prefix\" adds to the paths that are\n> supplied by the user (e.g. when the user says Makefile while in t/\n> subdirectory, i.e. GIT_PREFIX=t/, s/he means t/Makefile), but this\n> new thing subtracts from the paths given by the user (e.g. when the\n> user gives a pathspec 'w/git.git/D*' while setting the super thing\n> as w/git.git, because s/he is at ~/, the pathspec matcher\n> conceptually subtracts w/git.git/ from the pathspec before matching\n> them against the paths it finds in the index), it becomes clear that\n> giving both at the same time is awkward and not very useful.\n\nWell maybe...I don't really know much about how the prefix interacts in\nevery scenario but would what you describe still work if we are in a sub\ndir of the superproject (which contains other directorys and perhaps a\nsubmodule) and execute a --recurse-submodules command in the\nsubdirectory?  I suspect we don't want to force users to be in the root\ndirectory of the project in order to use --recurse-submodules.\n\n-- \nBrandon Williams\n"},{"id":"302776","messageId":"xmqqwphxknoj.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"20160927214854.GA180705@google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T22:01:00Z","receivedAt":"2016-09-27T22:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> Well maybe...I don't really know much about how the prefix interacts in\n> every scenario but would what you describe still work if we are in a sub\n> dir of the superproject (which contains other directorys and perhaps a\n> submodule) and execute a --recurse-submodules command in the\n> subdirectory?  I suspect we don't want to force users to be in the root\n> directory of the project in order to use --recurse-submodules.\n\nYou need to remember \"must be at the top\" is relevant only to the\ncommand that is invoked with --super-prefix, not the recursive one\nthat drives such a process.\n\nSuppose your superproject is organized like so:\n\n    - file-at-top\n      a/file-in-A\n      a/b (submodule)\n      a/b/file-at-top-of-B\n      c/  (submodule)\n      c/file-at-top-of-C\n\nIf you are in a subdirectory of your superproject, say, a/,\n\n    cd a && git ls-files --recurse-submodules -- \"b*\"\n\nI would expect we would recurse into the submodule at \"a/b\" and find\n\"b/file-at-top-of-B\".  What does the internal invocation to do so\nwould look like?  I would think \"git -C b --super=b ls-files\" that\nis run from \"a\".\n\nYour code would is already prepared to find \"file-at-top-of-B\" in\nthe index of the submodule, prepend \"b/\" to it and report the result\nas \"b/file-at-top-of-B\" when such a call is made, I think.\n\nNow, can you refer to c/ and c/file-at-top-of-C while sitting at a/?\n\n    cd a && git ls-files --recurse-submodules -- \"../c*\"\n\nwould be the top-level invocation.  This would iterate over the\nindex of the superproject, trying to find what matches \"c*\" (or,\n\"../c*\" relative to \"a\" i.e. where you are), find that 'c' that is a\nsubmodule, and invoke \"git -C ../c --super=../c ls-files\"\ninternally, I would imagine.  I think your code is prepared to\naccept this case as well.\n\nIn any case, the \"must be at the top\" does not come into the picture\nat all for the end-user interaction, i.e. invocation of the command\nthat is told to recurse into submodules, so we'd be OK.\n\n"},{"id":"302778","messageId":"20160927220955.GA38615@google.com","threadId":"44127","inReplyTo":"xmqqwphxknoj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-09-27T22:09:55Z","receivedAt":"2016-09-27T22:10:04Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/27, Junio C Hamano wrote:\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > Well maybe...I don't really know much about how the prefix interacts in\n> > every scenario but would what you describe still work if we are in a sub\n> > dir of the superproject (which contains other directorys and perhaps a\n> > submodule) and execute a --recurse-submodules command in the\n> > subdirectory?  I suspect we don't want to force users to be in the root\n> > directory of the project in order to use --recurse-submodules.\n> \n> You need to remember \"must be at the top\" is relevant only to the\n> command that is invoked with --super-prefix, not the recursive one\n> that drives such a process.\n> \n> Suppose your superproject is organized like so:\n> \n>     - file-at-top\n>       a/file-in-A\n>       a/b (submodule)\n>       a/b/file-at-top-of-B\n>       c/  (submodule)\n>       c/file-at-top-of-C\n> \n> If you are in a subdirectory of your superproject, say, a/,\n> \n>     cd a && git ls-files --recurse-submodules -- \"b*\"\n> \n> I would expect we would recurse into the submodule at \"a/b\" and find\n> \"b/file-at-top-of-B\".  What does the internal invocation to do so\n> would look like?  I would think \"git -C b --super=b ls-files\" that\n> is run from \"a\".\n> \n> Your code would is already prepared to find \"file-at-top-of-B\" in\n> the index of the submodule, prepend \"b/\" to it and report the result\n> as \"b/file-at-top-of-B\" when such a call is made, I think.\n> \n> Now, can you refer to c/ and c/file-at-top-of-C while sitting at a/?\n> \n>     cd a && git ls-files --recurse-submodules -- \"../c*\"\n> \n> would be the top-level invocation.  This would iterate over the\n> index of the superproject, trying to find what matches \"c*\" (or,\n> \"../c*\" relative to \"a\" i.e. where you are), find that 'c' that is a\n> submodule, and invoke \"git -C ../c --super=../c ls-files\"\n> internally, I would imagine.  I think your code is prepared to\n> accept this case as well.\n> \n> In any case, the \"must be at the top\" does not come into the picture\n> at all for the end-user interaction, i.e. invocation of the command\n> that is told to recurse into submodules, so we'd be OK.\n\nThanks for the clear explination that makes sense.\n\nAlso, --super-prefix as a name is growing on me :)\n\n-- \nBrandon Williams\n"},{"id":"302781","messageId":"xmqqh991kmnt.fsf@gitster.mtv.corp.google.com","threadId":"44127","inReplyTo":"20160927220955.GA38615@google.com","subject":"Re: [PATCH 1/2] ls-files: adding support for submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-27T22:23:02Z","receivedAt":"2016-09-27T22:23:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n>> If you are in a subdirectory of your superproject, say, a/,\n>> \n>>     cd a && git ls-files --recurse-submodules -- \"b*\"\n>> \n>> I would expect we would recurse into the submodule at \"a/b\" and find\n>> \"b/file-at-top-of-B\".  What does the internal invocation to do so\n>> would look like?  I would think \"git -C b --super=b ls-files\" that\n>> is run from \"a\".\n\nActually, the internal invocation may have to be\n\n\t$ git --super=a/b ls-files -- \"a/b*\"\n\nif the desired overall output needs to be in the \"--full-name\" mode.\nThat is, the top-level recursive one may be\n\n    cd a && git ls-files --recurse-submodules --full-name -- \"b*\"\n\nThis top-level \"ls-files\" will have \"prefix\" set to \"a/\".  Because\nit is run in the \"--full-name\" mode, after finding that the\nsubmodule at \"a/b\" matches the given pathspec and deciding to\nrecurse into it, it needs to arrange that paths stored in the index\nof the submodule are prefixed with \"a/b/\", not with \"b/\", when\nshown.\n\n"}]}