[PATCH 0/3] recursive support for ls-files

[PATCH 0/3] recursive support for ls-files

[Brandon Williams]

After looking at the feedback I rerolled a few things, in particular the
--submodule_prefix option that existed to give a submodule context about where
it had been invoked from.  People didn't seem to like the idea of exposing this
to the users (yet anyways) so I removed it as an option and instead have it
being passed to a child process via an environment variable
GIT_INTERNAL_SUBMODULE_PREFIX.  This way we don't have to support anything to
external users at the moment.

Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
King.

Brandon Williams (3):
  submodules: make submodule-prefix option an envvar
  ls-files: optionally recurse into submodules
  ls-files: add pathspec matching for submodules

 Documentation/git-ls-files.txt         |   7 +-
 builtin/ls-files.c                     | 173 ++++++++++++++++++++-------
 cache.h                                |   1 +
 dir.c                                  |  46 +++++++-
 dir.h                                  |   4 +
 environment.c                          |   1 +
 t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
 7 files changed, 398 insertions(+), 43 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/3 v3] submodules: make submodule-prefix option an envvar

[Brandon Williams]

Add a submodule-prefix enviorment variable
'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
--recurse-submodule options.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 cache.h       | 1 +
 environment.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/cache.h b/cache.h
index 3556326..ae88a35 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/environment.c b/environment.c
index ca72464..7380815 100644
--- a/environment.c
+++ b/environment.c
@@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
 	GIT_REPLACE_REF_BASE_ENVIRONMENT,
 	GIT_PREFIX_ENVIRONMENT,
+	GIT_SUBMODULE_PREFIX_ENVIRONMENT,
 	GIT_SHALLOW_FILE_ENVIRONMENT,
 	GIT_COMMON_DIR_ENVIRONMENT,
 	NULL
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 1/3 v3] submodules: make submodule-prefix option an envvar

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Add a submodule-prefix enviorment variable

environment?

> 'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
> --recurse-submodule options.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  cache.h       | 1 +
>  environment.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 3556326..ae88a35 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
>  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
>  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
>  #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
> +#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
>  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
>  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
>  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
> diff --git a/environment.c b/environment.c
> index ca72464..7380815 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
>  	NO_REPLACE_OBJECTS_ENVIRONMENT,
>  	GIT_REPLACE_REF_BASE_ENVIRONMENT,
>  	GIT_PREFIX_ENVIRONMENT,
> +	GIT_SUBMODULE_PREFIX_ENVIRONMENT,
>  	GIT_SHALLOW_FILE_ENVIRONMENT,
>  	GIT_COMMON_DIR_ENVIRONMENT,
>  	NULL

[PATCH 2/3 v3] ls-files: optionally recurse into submodules

[Brandon Williams]

Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules.  This is done by forking off a
process to recursively call ls-files on all submodules. Use environment
variable `GIT_INTERNAL_SUBMODULE_PREFIX` to pass a path to the submodule
which it can use to prepend to output or pathspec matching logic.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |  7 ++-
 builtin/ls-files.c                     | 63 ++++++++++++++++++++++
 t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,10 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode.
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..54ab765 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
+static const char *submodule_prefix;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
 static void write_name(const char *name)
 {
 	/*
+	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
+	 * by the caller.
+	 *
+	 * full_name get reused across output lines to minimize the allocation
+	 * churn.
+	 */
+	static struct strbuf full_name = STRBUF_INIT;
+	if (submodule_prefix && *submodule_prefix) {
+		strbuf_reset(&full_name);
+		strbuf_addstr(&full_name, submodule_prefix);
+		strbuf_addstr(&full_name, name);
+		name = full_name.buf;
+	}
+
+	/*
 	 * With "--full-name", prefix_len=0; this caller needs to pass
 	 * an empty string in that case (a NULL is good for "").
 	 */
@@ -152,6 +170,27 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+	argv_array_pushf(&cp.env_array, "%s=%s%s/",
+			 GIT_SUBMODULE_PREFIX_ENVIRONMENT,
+			 submodule_prefix ? submodule_prefix : "",
+			 ce->name);
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	status = run_command(&cp);
+	if (status)
+		exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
 	int len = max_prefix_len;
@@ -163,6 +202,10 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 			    len, ps_matched,
 			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
 		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+		return;
+	}
 
 	if (tag && *tag && show_valid_bit &&
 	    (ce->ce_flags & CE_VALID)) {
@@ -468,6 +511,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +564,24 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules)
+		submodule_prefix = getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT);
+
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol))
+		die("ls-files --recurse-submodules can only be used in "
+		    "--cached mode");
+
+	if (recurse_submodules && error_unmatch)
+		die("ls-files --recurse-submodules does not support "
+		    "--error-unmatch");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support path "
+		    "arguments");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..caf3815
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	git init submodule &&
+	echo c >submodule/c &&
+	git -C submodule add c &&
+	git -C submodule commit -m "add c" &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	echo a >not_added &&
+	echo b >b/not_added &&
+	echo c >submodule/not_added &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/.gitmodules
+	submodule/c
+	submodule/subsub/d
+	EOF
+
+	git init submodule/subsub &&
+	echo d >submodule/subsub/d &&
+	git -C submodule/subsub add d &&
+	git -C submodule/subsub commit -m "add d" &&
+	git -C submodule submodule add ./subsub &&
+	git -C submodule commit -m "added subsub" &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+	test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep 'can only be used in --cached mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/3 v3] ls-files: add pathspec matching for submodules

[Brandon Williams]

Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule.  The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.

This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally.  If any wildcard character is encountered we'll simply punt
and produce a false positive match.  More accurate matching will be done
once inside the submodule.  This is due to the superproject not knowing
what files could exist in the submodule.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     | 134 ++++++++++++++++++++-------------
 dir.c                                  |  46 ++++++++++-
 dir.h                                  |   4 +
 t/t3007-ls-files-recurse-submodules.sh | 126 +++++++++++++++++++++++++++++--
 4 files changed, 248 insertions(+), 62 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 54ab765..8ecffd1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -177,6 +177,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
+	int i;
 
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
@@ -184,6 +185,29 @@ static void show_gitlink(const struct cache_entry *ce)
 			 GIT_SUBMODULE_PREFIX_ENVIRONMENT,
 			 submodule_prefix ? submodule_prefix : "",
 			 ce->name);
+	/* add options */
+	if (show_eol)
+		argv_array_push(&cp.args, "--eol");
+	if (show_valid_bit)
+		argv_array_push(&cp.args, "-v");
+	if (show_stage)
+		argv_array_push(&cp.args, "--stage");
+	if (show_cached)
+		argv_array_push(&cp.args, "--cached");
+	if (debug_mode)
+		argv_array_push(&cp.args, "--debug");
+	if (line_terminator == '\0')
+		argv_array_push(&cp.args, "-z");
+
+	/*
+	 * Pass in the original pathspec args.  The submodule will be
+	 * responsible for prepending the 'submodule_prefix' prior to comparing
+	 * against the pathspec for matches.
+	 */
+	argv_array_push(&cp.args, "--");
+	for (i = 0; i < pathspec.nr; i++)
+		argv_array_push(&cp.args, pathspec.items[i].original);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -193,57 +217,62 @@ static void show_gitlink(const struct cache_entry *ce)
 
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
+	struct strbuf name = STRBUF_INIT;
 	int len = max_prefix_len;
+	if (submodule_prefix)
+		strbuf_addstr(&name, submodule_prefix);
+	strbuf_addstr(&name, ce->name);
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
-			    len, ps_matched,
-			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
-		return;
-	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
 		show_gitlink(ce);
-		return;
-	}
+	} else if (match_pathspec(&pathspec, name.buf, name.len,
+				  len, ps_matched,
+				  S_ISDIR(ce->ce_mode) ||
+				  S_ISGITLINK(ce->ce_mode))) {
+		if (tag && *tag && show_valid_bit &&
+		    (ce->ce_flags & CE_VALID)) {
+			static char alttag[4];
+			memcpy(alttag, tag, 3);
+			if (isalpha(tag[0]))
+				alttag[0] = tolower(tag[0]);
+			else if (tag[0] == '?')
+				alttag[0] = '!';
+			else {
+				alttag[0] = 'v';
+				alttag[1] = tag[0];
+				alttag[2] = ' ';
+				alttag[3] = 0;
+			}
+			tag = alttag;
+		}
 
-	if (tag && *tag && show_valid_bit &&
-	    (ce->ce_flags & CE_VALID)) {
-		static char alttag[4];
-		memcpy(alttag, tag, 3);
-		if (isalpha(tag[0]))
-			alttag[0] = tolower(tag[0]);
-		else if (tag[0] == '?')
-			alttag[0] = '!';
-		else {
-			alttag[0] = 'v';
-			alttag[1] = tag[0];
-			alttag[2] = ' ';
-			alttag[3] = 0;
+		if (!show_stage) {
+			fputs(tag, stdout);
+		} else {
+			printf("%s%06o %s %d\t",
+			       tag,
+			       ce->ce_mode,
+			       find_unique_abbrev(ce->sha1,abbrev),
+			       ce_stage(ce));
+		}
+		write_eolinfo(ce, ce->name);
+		write_name(ce->name);
+		if (debug_mode) {
+			const struct stat_data *sd = &ce->ce_stat_data;
+
+			printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+			printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+			printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+			printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+			printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
 		}
-		tag = alttag;
 	}
 
-	if (!show_stage) {
-		fputs(tag, stdout);
-	} else {
-		printf("%s%06o %s %d\t",
-		       tag,
-		       ce->ce_mode,
-		       find_unique_abbrev(ce->sha1,abbrev),
-		       ce_stage(ce));
-	}
-	write_eolinfo(ce, ce->name);
-	write_name(ce->name);
-	if (debug_mode) {
-		const struct stat_data *sd = &ce->ce_stat_data;
-
-		printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
-		printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
-		printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
-		printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
-		printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
-	}
+	strbuf_release(&name);
 }
 
 static void show_ru_info(void)
@@ -568,27 +597,28 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		submodule_prefix = getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT);
 
 	if (recurse_submodules &&
-	    (show_stage || show_deleted || show_others || show_unmerged ||
-	     show_killed || show_modified || show_resolve_undo ||
-	     show_valid_bit || show_tag || show_eol))
-		die("ls-files --recurse-submodules can only be used in "
-		    "--cached mode");
+	    (show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo))
+		die("ls-files --recurse-submodules unsupported mode");
 
 	if (recurse_submodules && error_unmatch)
 		die("ls-files --recurse-submodules does not support "
 		    "--error-unmatch");
 
-	if (recurse_submodules && argc)
-		die("ls-files --recurse-submodules does not support path "
-		    "arguments");
-
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 		       prefix, argv);
 
-	/* Find common prefix for all pathspec's */
-	max_prefix = common_prefix(&pathspec);
+	/*
+	 * Find common prefix for all pathspec's
+	 * This is used as a performance optimization which unfortunately cannot
+	 * be done when recursing into submodules
+	 */
+	if (recurse_submodules)
+		max_prefix = NULL;
+	else
+		max_prefix = common_prefix(&pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
-#define DO_MATCH_EXCLUDE   1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE   (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
 
 /*
  * Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 			 item->nowildcard_len - prefix))
 		return MATCHED_FNMATCH;
 
+	/* Perform checks to see if "name" is a super set of the pathspec */
+	if (flags & DO_MATCH_SUBMODULE) {
+		/* name is a literal prefix of the pathspec */
+		if ((namelen < matchlen) &&
+		    (match[namelen] == '/') &&
+		    !ps_strncmp(item, match, name, namelen))
+			return MATCHED_RECURSIVELY;
+
+		/* name" doesn't match up to the first wild character */
+		if (item->nowildcard_len < item->len &&
+		    ps_strncmp(item, match, name,
+			       item->nowildcard_len - prefix))
+			return 0;
+
+		/*
+		 * Here is where we would perform a wildmatch to check if
+		 * "name" can be matched as a directory (or a prefix) against
+		 * the pathspec.  Since wildmatch doesn't have this capability
+		 * at the present we have to punt and say that it is a match,
+		 * potentially returning a false positive
+		 * The submodules themselves will be able to perform more
+		 * accurate matching to determine if the pathspec matches.
+		 */
+		return MATCHED_RECURSIVELY;
+	}
+
 	return 0;
 }
 
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+			 const char *submodule_name,
+			 char *seen)
+{
+	int matched = do_match_pathspec(ps, submodule_name,
+					strlen(submodule_name),
+					0, seen,
+					DO_MATCH_DIRECTORY |
+					DO_MATCH_SUBMODULE);
+	return matched;
+}
+
 int report_path_error(const char *ps_matched,
 		      const struct pathspec *pathspec,
 		      const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
 		       const char *pattern, const char *string,
 		       int prefix);
 
+extern int submodule_path_match(const struct pathspec *ps,
+				const char *submodule_name,
+				char *seen);
+
 static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index caf3815..4a51d38 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+	cat | tr "\n" "\0" >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules -z >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ls-files does not output files not added to a repo' '
 	cat >expect <<-\EOF &&
 	.gitmodules
@@ -69,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	test_cmp expect actual
 '
 
-test_expect_success '--recurse-submodules does not support using path arguments' '
-	test_must_fail git ls-files --recurse-submodules b 2>actual &&
-	test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+	echo e >submodule/subsub/e.txt &&
+	git -C submodule/subsub add e.txt &&
+	git -C submodule/subsub commit -m "adding e.txt" &&
+	echo f >submodule/f.TXT &&
+	echo g >submodule/g.txt &&
+	git -C submodule add f.TXT g.txt &&
+	git -C submodule commit -m "add f and g" &&
+	echo h >h.txt &&
+	mkdir sib &&
+	echo sib >sib/file &&
+	git add h.txt sib/file &&
+	git commit -m "add h and sib/file" &&
+	git init sub &&
+	echo sub >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git submodule add ./sub &&
+	git commit -m "added sub" &&
+
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	h.txt
+	sib/file
+	sub/file
+	submodule/.gitmodules
+	submodule/c
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/d
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+	cat actual &&
+	git ls-files --recurse-submodules "*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules "*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "sub" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su*/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su?/file" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sib/file
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "s??/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s???file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s*file" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
@@ -82,18 +196,14 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
 test_incompatible_with_recurse_submodules () {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "
 		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
-		test_i18ngrep 'can only be used in --cached mode' actual
+		test_i18ngrep 'unsupported mode' actual
 	"
 }
 
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
 test_incompatible_with_recurse_submodules --deleted
 test_incompatible_with_recurse_submodules --modified
 test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
 test_incompatible_with_recurse_submodules --killed
 test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 0/3] recursive support for ls-files

[Jeff King]

On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:

> After looking at the feedback I rerolled a few things, in particular the
> --submodule_prefix option that existed to give a submodule context about where
> it had been invoked from.  People didn't seem to like the idea of exposing this
> to the users (yet anyways) so I removed it as an option and instead have it
> being passed to a child process via an environment variable
> GIT_INTERNAL_SUBMODULE_PREFIX.  This way we don't have to support anything to
> external users at the moment.

I think we can still have it as a command-line argument and declare it
internal. It's not like environment variables cannot also be set by our
callers. :)

I don't mind it as an environment variable, though. In some ways it
makes things easier. I just think "internal versus external" and the
exact implementation are orthogonal.

> Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> King.

Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
try not to have a broken state in the history. It's less important in
this case, because the breakage is not a regression
(--recurse-submodules is a new feature, so you could consider it "not
working" until the 3rd patch). But I think it's still a good rule to
follow, because it makes the commits easier to review, look at later,
etc.

For that matter, I do not understand why options like "-s" get enabled
in patch 3. I do not mind them starting as disabled in patch 2, but it
seems like "pass along some known-safe options" should be its own patch
somewhere between patches 2 and 3.

There are some other options that are ignored (neither disabled nor
passed along to children). Most of them are related to exclusions, which
I _think_ are safe to ignore (they do not do anything interesting unless
you specify "-o", which is explicitly disabled). I'm not sure about
--with-tree, though (or what it would even mean in the context of
recursing).

-Peff

Re: [PATCH 0/3] recursive support for ls-files

[Brandon Williams]

On 09/25, Jeff King wrote:
> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
> 
> > After looking at the feedback I rerolled a few things, in particular the
> > --submodule_prefix option that existed to give a submodule context about where
> > it had been invoked from.  People didn't seem to like the idea of exposing this
> > to the users (yet anyways) so I removed it as an option and instead have it
> > being passed to a child process via an environment variable
> > GIT_INTERNAL_SUBMODULE_PREFIX.  This way we don't have to support anything to
> > external users at the moment.
> 
> I think we can still have it as a command-line argument and declare it
> internal. It's not like environment variables cannot also be set by our
> callers. :)
> 
> I don't mind it as an environment variable, though. In some ways it
> makes things easier. I just think "internal versus external" and the
> exact implementation are orthogonal.
> 

We may still want it to be an option at some point in the future.  This
way we can revisit making it an option once we know more about the other
uses it could have (aside from just being for submodules as someone
suggested).

> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> > King.
> 
> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
> try not to have a broken state in the history. It's less important in
> this case, because the breakage is not a regression
> (--recurse-submodules is a new feature, so you could consider it "not
> working" until the 3rd patch). But I think it's still a good rule to
> follow, because it makes the commits easier to review, look at later,
> etc.
> 
> For that matter, I do not understand why options like "-s" get enabled
> in patch 3. I do not mind them starting as disabled in patch 2, but it
> seems like "pass along some known-safe options" should be its own patch
> somewhere between patches 2 and 3.

I'll keep that in mind for future patches.  I figured that since it was
fixed in the end that would be fine but if things shouldn't be broken at
any state in the patch series I'll make sure to not do that in the
future.

> There are some other options that are ignored (neither disabled nor
> passed along to children). Most of them are related to exclusions, which
> I _think_ are safe to ignore (they do not do anything interesting unless
> you specify "-o", which is explicitly disabled). I'm not sure about
> --with-tree, though (or what it would even mean in the context of
> recursing).

These other features that are disabled now could be enabled in a future
patch.  You're right though I'd have to think about the --with-tree
option a bit more and what it would mean with submodules.

-Brandon

Re: [PATCH 0/3] recursive support for ls-files

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> On 09/25, Jeff King wrote:
>> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
>> 
>> > After looking at the feedback I rerolled a few things, in particular the
>> > --submodule_prefix option that existed to give a submodule context about where
>> > it had been invoked from.  People didn't seem to like the idea of exposing this
>> > to the users (yet anyways) so I removed it as an option and instead have it
>> > being passed to a child process via an environment variable
>> > GIT_INTERNAL_SUBMODULE_PREFIX.  This way we don't have to support anything to
>> > external users at the moment.
>> 
>> I think we can still have it as a command-line argument and declare it
>> internal. It's not like environment variables cannot also be set by our
>> callers. :)
>> 
>> I don't mind it as an environment variable, though. In some ways it
>> makes things easier. I just think "internal versus external" and the
>> exact implementation are orthogonal.
>
> We may still want it to be an option at some point in the future.  This
> way we can revisit making it an option once we know more about the other
> uses it could have (aside from just being for submodules as someone
> suggested).

I do not think it makes too much of a difference between environment
and command line option.  We need an update to the "git" potty to
say "you told me to use the submodule-prefix feature, but this
subcommand is not prepared to accept it (yet)" and cause it to error
out either way, which would mean that a series that introduces the
feature needs to touch "git.c" anyway, so I would have expected us
to add command line option first, simply because "git.c" is where it
happens, optionally with the support for the environment variable,
not the other way around.

>> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
>> > King.
>> 
>> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
>> try not to have a broken state in the history. It's less important in
>> this case, because the breakage is not a regression
>> (--recurse-submodules is a new feature, so you could consider it "not
>> working" until the 3rd patch). But I think it's still a good rule to
>> follow, because it makes the commits easier to review, look at later,
>> etc.
>> 
>> For that matter, I do not understand why options like "-s" get enabled
>> in patch 3. I do not mind them starting as disabled in patch 2, but it
>> seems like "pass along some known-safe options" should be its own patch
>> somewhere between patches 2 and 3.

Yes, exactly.

An obvious lazy way out to avoid breakage-in-the-middle and make
incremental progress would be to squash everything into one patch,
but we should and we should be able to do better.

I'd imagine this three-patch series would be more pleasant for
future readers if it were structured like:

 [1/3] introduces the submodule-prefix as a global feature; at the
       least it needs a way to invoke (either an environment, or an
       option to "git" potty, or both) and prevent mistakes by
       erroring out when it is attempted to call a subcommand that
       does not support the feature (yet).

 [2/3] adds the --recurse-submodule feature in a limited form to
       "ls-files".  I'd suggest for this step to pass through all
       options and arguments that are safe and reasonably useful
       to pass through without needing anything more than "ah, this
       option was given, so let's stuff it to the argv-array". An
       attempt to give things that are not yet passed through until
       3/3 to lead to an error that says it is not allowed (yet).

 [3-N] each of the remaining steps after 3/N adds support for one
       more thing to be passed that 2/3 refrained from doing, by
       doing more than just "pass it in argv-array", and then remove
       the "not yet supported" error that added by 2/3 for that one
       thing.  The first of these "more things" would be to support
       pathspecs as the receiving side would need code changes for
       the matching logic.  There may be more, or there may be
       nothing else that requires 4/N, 5/N, etc.

>> There are some other options that are ignored (neither disabled nor
>> passed along to children). Most of them are related to exclusions, which
>> I _think_ are safe to ignore (they do not do anything interesting unless
>> you specify "-o", which is explicitly disabled).

Sure. I agree that some options fall outside of "safe and reasonably
useful to pass through" criteria and it is OK not to support passing
them through.  I however think we should detect mistakes in the
caller, though.

I wonder if this will be common enough in the future that we are
better off adding a bit or two to the parse-options infrastructure.
Thinking out loud, would an enhancement like this be sufficient for
this particular series, and still useful in more general cases?

 - allow the caller of parse_options() to mark each entry with a
   handful of bits with no meaning to parse-options API.  "ls-files"
   may use this mechanism to mark options that to be passed through
   and options that must be rejected when --recurse-submodules is
   asked.

 - parse_options() will mark each entry as "this option was given"
   while doing its work.  Note that "--no-foo" counts as "option
   'foo' was given".

With these, after your call to parse_options() returns, you could
notice that recurse-submodules was asked for by inspecting your own
"static int recurse_submodules" global variable, and then iterate
over builtin_ls_files_options[] array yourself to see if an option
that you marked with "must be rejected while recursing" bit using
mechanism #1 was seen by parse_options() by relying on mechanism #2.

You could also add a new helper macro to parse-options API that
iterates over the options[] array, i.e.

	for_each_parse_options_array(opt) {
		if (!opt_was_used(opt))
                	continue;
		if (opt_custom_bit(opt) & NOT_IN_RECURSIVE)
                	die("'%s' cannot be used with --recurse-submodules",
			    parse_options_name(opt))
	}

or something like that, but we'd need to gain experience with the
mechanism #1 and #2 first before deciding if such a helper is useful
(iow, I do not think we need it from day one, if we go this route).

Re: [PATCH 0/3] recursive support for ls-files

[Brandon Williams]

On 09/25, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > On 09/25, Jeff King wrote:
> >> On Fri, Sep 23, 2016 at 05:13:31PM -0700, Brandon Williams wrote:
> >> 
> >> > After looking at the feedback I rerolled a few things, in particular the
> >> > --submodule_prefix option that existed to give a submodule context about where
> >> > it had been invoked from.  People didn't seem to like the idea of exposing this
> >> > to the users (yet anyways) so I removed it as an option and instead have it
> >> > being passed to a child process via an environment variable
> >> > GIT_INTERNAL_SUBMODULE_PREFIX.  This way we don't have to support anything to
> >> > external users at the moment.
> >> 
> >> I think we can still have it as a command-line argument and declare it
> >> internal. It's not like environment variables cannot also be set by our
> >> callers. :)
> >> 
> >> I don't mind it as an environment variable, though. In some ways it
> >> makes things easier. I just think "internal versus external" and the
> >> exact implementation are orthogonal.
> >
> > We may still want it to be an option at some point in the future.  This
> > way we can revisit making it an option once we know more about the other
> > uses it could have (aside from just being for submodules as someone
> > suggested).
> 
> I do not think it makes too much of a difference between environment
> and command line option.  We need an update to the "git" potty to
> say "you told me to use the submodule-prefix feature, but this
> subcommand is not prepared to accept it (yet)" and cause it to error
> out either way, which would mean that a series that introduces the
> feature needs to touch "git.c" anyway, so I would have expected us
> to add command line option first, simply because "git.c" is where it
> happens, optionally with the support for the environment variable,
> not the other way around.

In a previous email you mentioned that this feature should be completely
hidden from users, which is why I removed the command line option for
this latest series.  If that isn't what you intended that I can
definitely add the option to git.c.  And you would rather we perform the
checking in git.c to see if a subcommand supports the prefix versus
silently ignoring it if it hasn't?  I'm assuming this checking would
also be done in git.c?

> 
> >> > Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
> >> > King.
> >> 
> >> Hmm. It is broken after patch 2, and then fixed in patch 3. Usually we'd
> >> try not to have a broken state in the history. It's less important in
> >> this case, because the breakage is not a regression
> >> (--recurse-submodules is a new feature, so you could consider it "not
> >> working" until the 3rd patch). But I think it's still a good rule to
> >> follow, because it makes the commits easier to review, look at later,
> >> etc.
> >> 
> >> For that matter, I do not understand why options like "-s" get enabled
> >> in patch 3. I do not mind them starting as disabled in patch 2, but it
> >> seems like "pass along some known-safe options" should be its own patch
> >> somewhere between patches 2 and 3.
> 
> Yes, exactly.
> 
> An obvious lazy way out to avoid breakage-in-the-middle and make
> incremental progress would be to squash everything into one patch,
> but we should and we should be able to do better.
> 
> I'd imagine this three-patch series would be more pleasant for
> future readers if it were structured like:
> 
>  [1/3] introduces the submodule-prefix as a global feature; at the
>        least it needs a way to invoke (either an environment, or an
>        option to "git" potty, or both) and prevent mistakes by
>        erroring out when it is attempted to call a subcommand that
>        does not support the feature (yet).
> 
>  [2/3] adds the --recurse-submodule feature in a limited form to
>        "ls-files".  I'd suggest for this step to pass through all
>        options and arguments that are safe and reasonably useful
>        to pass through without needing anything more than "ah, this
>        option was given, so let's stuff it to the argv-array". An
>        attempt to give things that are not yet passed through until
>        3/3 to lead to an error that says it is not allowed (yet).
> 
>  [3-N] each of the remaining steps after 3/N adds support for one
>        more thing to be passed that 2/3 refrained from doing, by
>        doing more than just "pass it in argv-array", and then remove
>        the "not yet supported" error that added by 2/3 for that one
>        thing.  The first of these "more things" would be to support
>        pathspecs as the receiving side would need code changes for
>        the matching logic.  There may be more, or there may be
>        nothing else that requires 4/N, 5/N, etc.

I can do another rework and structure the patch series more inline with
what you are suggesting here.

-- 
Brandon Williams

Re: [PATCH 0/3] recursive support for ls-files

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> In a previous email you mentioned that this feature should be completely
> hidden from users, which is why I removed the command line option for
> this latest series.

I may have said something like that; I do not recall, though, so a
more accurate description might be "I may have said something that
can be (mis)interpreted like so".  Sorry for the confusion.

In any case, an environment is not "completely hidden from users",
so it is not fundamentallly different from a command line option
anyway ;-)

> If that isn't what you intended that I can
> definitely add the option to git.c.  And you would rather we perform the
> checking in git.c to see if a subcommand supports the prefix versus
> silently ignoring it if it hasn't?  I'm assuming this checking would
> also be done in git.c?

I actually do not care strongly _where_ the check happens.  It was
just that in the subcommand dispatcher would be the single place
that is easiest-to-implement to perform that check, that made me
suggest that.  We already have various bits like NEEDS_WORK_TREE,
RUN_SETUP, etc. so REJECT_EXTERNAL_PREFIX (or whatever its name be;
I do not offhand recall current proposal) bit would fit there
naturally, I would think.  Of course, non-built-in commands need to
protect themselves separately, if they want to.

Re: [PATCH 0/3] recursive support for ls-files

[Brandon Williams]

On 09/26, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > In a previous email you mentioned that this feature should be completely
> > hidden from users, which is why I removed the command line option for
> > this latest series.
> 
> I may have said something like that; I do not recall, though, so a
> more accurate description might be "I may have said something that
> can be (mis)interpreted like so".  Sorry for the confusion.
> 
> In any case, an environment is not "completely hidden from users",
> so it is not fundamentallly different from a command line option
> anyway ;-)

No worries, I'm still trying to get a hang of all this :)
True, that wouldn't be completely hidden either.

> > If that isn't what you intended that I can
> > definitely add the option to git.c.  And you would rather we perform the
> > checking in git.c to see if a subcommand supports the prefix versus
> > silently ignoring it if it hasn't?  I'm assuming this checking would
> > also be done in git.c?
> 
> I actually do not care strongly _where_ the check happens.  It was
> just that in the subcommand dispatcher would be the single place
> that is easiest-to-implement to perform that check, that made me
> suggest that.  We already have various bits like NEEDS_WORK_TREE,
> RUN_SETUP, etc. so REJECT_EXTERNAL_PREFIX (or whatever its name be;
> I do not offhand recall current proposal) bit would fit there
> naturally, I would think.  Of course, non-built-in commands need to
> protect themselves separately, if they want to.

That makes sense.  I have an idea of where the check could be made. And
with those flags it may make sense to have the flag be an indicator that
the builtin is ready for submodule type commands ie SUPPORTS_SUBMODULES
or something along those lines.  That way we only need to add the flag
to each command as we go (instead of all commands which don't support
it) and just make sure that the flag is set if the submodule prefix
option is being used.  How does that sound?

-- 
Brandon Williams

Re: [PATCH 0/3] recursive support for ls-files

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> or something along those lines.  That way we only need to add the flag
> to each command as we go ...

Sounds good.  Thanks.

[PATCH 0/4 v4] recursive support for ls-files

[Brandon Williams]

A couple things have changed in v4:

- Restructured the patch series to prevent a breakage mid-way.
- Added an additional patch in the middle to pass through safe options.  This
  way the series is structured in a more coherent manor.
- Added --submodule-prefix to top-level git.c

Hopefully this series addresses some of issues brought up in v3

Brandon Williams (4):
  submodules: make submodule-prefix option
  ls-files: optionally recurse into submodules
  ls-files: pass through safe options for --recurse-submodules
  ls-files: add pathspec matching for submodules

 Documentation/git-ls-files.txt         |   7 +-
 Documentation/git.txt                  |   5 +
 builtin/ls-files.c                     | 187 ++++++++++++++++++++++-------
 cache.h                                |   1 +
 dir.c                                  |  46 +++++++-
 dir.h                                  |   4 +
 environment.c                          |   1 +
 git.c                                  |  21 +++-
 t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
 9 files changed, 437 insertions(+), 44 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/4 v4] submodules: make submodule-prefix option

[Brandon Williams]

Add a submodule-prefix environment variable
'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
--recurse-submodule options to give context to submodules about how they
were invoked.  This option is only allowed for builtins which have
submodule support.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git.txt |  5 +++++
 cache.h               |  1 +
 environment.c         |  1 +
 git.c                 | 19 +++++++++++++++++++
 4 files changed, 26 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..d29967a 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+    [--submodule-prefix=<path>]
     <command> [<args>]
 
 DESCRIPTION
@@ -601,6 +602,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
 	details.  Equivalent to setting the `GIT_NAMESPACE` environment
 	variable.
 
+--submodule-prefix=<path>::
+	Set a prefix which gives submodules context about the superproject that
+	invoked it.  Only allowed for commands which support submodules.
+
 --bare::
 	Treat the repository as a bare repository.  If GIT_DIR
 	environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..ae88a35 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/environment.c b/environment.c
index ca72464..7380815 100644
--- a/environment.c
+++ b/environment.c
@@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
 	GIT_REPLACE_REF_BASE_ENVIRONMENT,
 	GIT_PREFIX_ENVIRONMENT,
+	GIT_SUBMODULE_PREFIX_ENVIRONMENT,
 	GIT_SHALLOW_FILE_ENVIRONMENT,
 	GIT_COMMON_DIR_ENVIRONMENT,
 	NULL
diff --git a/git.c b/git.c
index 1c61151..b2b096a 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--submodule-prefix")) {
+			if (*argc < 2) {
+				fprintf(stderr, "No prefix given for --submodule-prefix.\n" );
+				usage(git_usage_string);
+			}
+			setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+			if (envchanged)
+				*envchanged = 1;
+			(*argv)++;
+			(*argc)--;
+		} else if (skip_prefix(cmd, "--submodule-prefix=", &cmd)) {
+			setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, cmd, 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
 			char *cwd = xgetcwd();
 			is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE		(1<<3)
+#define SUPPORT_SUBMODULES	(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -344,6 +359,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	}
 	commit_pager_choice();
 
+	if (!help && (getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT) &&
+		      !(p->option & SUPPORT_SUBMODULES)))
+		die("%s doesn't support submodules", p->cmd);
+
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 1/4 v4] submodules: make submodule-prefix option

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> +--submodule-prefix=<path>::
> +	Set a prefix which gives submodules context about the superproject that
> +	invoked it.  Only allowed for commands which support submodules.

This, and also the message in die(), uses a phrase "support
submodules", but it is unclear what it exactly means to the end
users and readers.

A "ls-files" that is recursively run as an implementation detail of
the "grep --recurse-submodules" would be taught to support this
option with this series.  Who is supporting submodules in that
context?

I'd imagine (close to) 100% of the people would say it is "grep"
that is supporting submodules, not "ls-files", but what this
paragraph and die() message want to express by the phrase "support
submodules" is the fact that "ls-files" knows how to react to
"--submodule-prefix" option.

I'd suggest not to worry too much about this phrasing at this point,
until we figure out exactly how we want to present these to end
users.  For now, perhaps drop the second sentence and replace it
with "The end-users are not expected to use this option" or
something like that?

> diff --git a/git.c b/git.c
> index 1c61151..b2b096a 100644
> --- a/git.c
> +++ b/git.c
> @@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
>  			if (envchanged)
>  				*envchanged = 1;
> +		} else if (!strcmp(cmd, "--submodule-prefix")) {
> +			if (*argc < 2) {
> +				fprintf(stderr, "No prefix given for --submodule-prefix.\n" );
> +				usage(git_usage_string);
> +			}
> +			setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, (*argv)[1], 1);
> +			if (envchanged)
> +				*envchanged = 1;
> +			(*argv)++;
> +			(*argc)--;
> +		} else if (skip_prefix(cmd, "--submodule-prefix=", &cmd)) {
> +			setenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT, cmd, 1);
> +			if (envchanged)
> +				*envchanged = 1;
>  		} else if (!strcmp(cmd, "--bare")) {
>  			char *cwd = xgetcwd();
>  			is_bare_repository_cfg = 1;
> @@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
>   * RUN_SETUP for reading from the configuration file.
>   */
>  #define NEED_WORK_TREE		(1<<3)
> +#define SUPPORT_SUBMODULES	(1<<4)
>  
>  struct cmd_struct {
>  	const char *cmd;
> @@ -344,6 +359,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  	}
>  	commit_pager_choice();
>  
> +	if (!help && (getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT) &&
> +		      !(p->option & SUPPORT_SUBMODULES)))
> +		die("%s doesn't support submodules", p->cmd);

s/submodules/submodule-prefix/ at least.

>  	if (!help && p->option & NEED_WORK_TREE)
>  		setup_work_tree();

Re: [PATCH 1/4 v4] submodules: make submodule-prefix option

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > +--submodule-prefix=<path>::
> > +	Set a prefix which gives submodules context about the superproject that
> > +	invoked it.  Only allowed for commands which support submodules.
> 
> This, and also the message in die(), uses a phrase "support
> submodules", but it is unclear what it exactly means to the end
> users and readers.
> 
> A "ls-files" that is recursively run as an implementation detail of
> the "grep --recurse-submodules" would be taught to support this
> option with this series.  Who is supporting submodules in that
> context?
> 
> I'd imagine (close to) 100% of the people would say it is "grep"
> that is supporting submodules, not "ls-files", but what this
> paragraph and die() message want to express by the phrase "support
> submodules" is the fact that "ls-files" knows how to react to
> "--submodule-prefix" option.
> 
> I'd suggest not to worry too much about this phrasing at this point,
> until we figure out exactly how we want to present these to end
> users.  For now, perhaps drop the second sentence and replace it
> with "The end-users are not expected to use this option" or
> something like that?

K can do.  The intention is that each command has to do whatever
internal rework needed so that it understands how to interact with
subomodules (be that calling another command or internally supporting
it).

> > +		die("%s doesn't support submodules", p->cmd);
> 
> s/submodules/submodule-prefix/ at least.

So should the #define be SUPPORT_SUBMODULE_PREFIX instead?  That may be
too narrow minded and not looking toward future submodule options
support but I'm not sure.

-- 
Brandon Williams

Re: [PATCH 1/4 v4] submodules: make submodule-prefix option

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>> s/submodules/submodule-prefix/ at least.
>
> So should the #define be SUPPORT_SUBMODULE_PREFIX instead?  That may be
> too narrow minded and not looking toward future submodule options
> support but I'm not sure.

I am not convinced that this prefix needs to be tied/limited to
submodule, at least not yet, though.  I view it as a prefix that
points from above the repository's top, of which submodule support
may be a good sample user, but the caller may not necessarily be
doing or interested in "submodule support".

That's also part of figuring out how we want define the semantics of
this thing and how we want to present it to the end-users, I guess,
so we may have to rename it when we know more, but that's OK.

Re: [PATCH 1/4 v4] submodules: make submodule-prefix option

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> >> s/submodules/submodule-prefix/ at least.
> >
> > So should the #define be SUPPORT_SUBMODULE_PREFIX instead?  That may be
> > too narrow minded and not looking toward future submodule options
> > support but I'm not sure.
> 
> I am not convinced that this prefix needs to be tied/limited to
> submodule, at least not yet, though.  I view it as a prefix that
> points from above the repository's top, of which submodule support
> may be a good sample user, but the caller may not necessarily be
> doing or interested in "submodule support".
> 
> That's also part of figuring out how we want define the semantics of
> this thing and how we want to present it to the end-users, I guess,
> so we may have to rename it when we know more, but that's OK.

K we can leave the name as is for now then.  Thankfully it should be a
simple name change since it only lives in 1 file.

-- 
Brandon Williams

[PATCH 2/4 v4] ls-files: optionally recurse into submodules

[Brandon Williams]

Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules.  This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--submodule_prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |   7 +-
 builtin/ls-files.c                     | 143 ++++++++++++++++++++++++---------
 git.c                                  |   2 +-
 t/t3007-ls-files-recurse-submodules.sh | 100 +++++++++++++++++++++++
 4 files changed, 212 insertions(+), 40 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,10 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode.
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..d4bfc60 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
+static const char *submodule_prefix;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
 static void write_name(const char *name)
 {
 	/*
+	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
+	 * by the caller.
+	 *
+	 * full_name get reused across output lines to minimize the allocation
+	 * churn.
+	 */
+	static struct strbuf full_name = STRBUF_INIT;
+	if (submodule_prefix && *submodule_prefix) {
+		strbuf_reset(&full_name);
+		strbuf_addstr(&full_name, submodule_prefix);
+		strbuf_addstr(&full_name, name);
+		name = full_name.buf;
+	}
+
+	/*
 	 * With "--full-name", prefix_len=0; this caller needs to pass
 	 * an empty string in that case (a NULL is good for "").
 	 */
@@ -152,55 +170,84 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
+			 submodule_prefix ? submodule_prefix : "",
+			 ce->name);
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	status = run_command(&cp);
+	if (status)
+		exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
+	struct strbuf name = STRBUF_INIT;
 	int len = max_prefix_len;
+	if (submodule_prefix)
+		strbuf_addstr(&name, submodule_prefix);
+	strbuf_addstr(&name, ce->name);
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
-			    len, ps_matched,
-			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
-		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+	} else if (match_pathspec(&pathspec, name.buf, name.len,
+				  len, ps_matched,
+				  S_ISDIR(ce->ce_mode) ||
+				  S_ISGITLINK(ce->ce_mode))) {
+		if (tag && *tag && show_valid_bit &&
+		    (ce->ce_flags & CE_VALID)) {
+			static char alttag[4];
+			memcpy(alttag, tag, 3);
+			if (isalpha(tag[0]))
+				alttag[0] = tolower(tag[0]);
+			else if (tag[0] == '?')
+				alttag[0] = '!';
+			else {
+				alttag[0] = 'v';
+				alttag[1] = tag[0];
+				alttag[2] = ' ';
+				alttag[3] = 0;
+			}
+			tag = alttag;
+		}
 
-	if (tag && *tag && show_valid_bit &&
-	    (ce->ce_flags & CE_VALID)) {
-		static char alttag[4];
-		memcpy(alttag, tag, 3);
-		if (isalpha(tag[0]))
-			alttag[0] = tolower(tag[0]);
-		else if (tag[0] == '?')
-			alttag[0] = '!';
-		else {
-			alttag[0] = 'v';
-			alttag[1] = tag[0];
-			alttag[2] = ' ';
-			alttag[3] = 0;
+		if (!show_stage) {
+			fputs(tag, stdout);
+		} else {
+			printf("%s%06o %s %d\t",
+			       tag,
+			       ce->ce_mode,
+			       find_unique_abbrev(ce->sha1,abbrev),
+			       ce_stage(ce));
+		}
+		write_eolinfo(ce, ce->name);
+		write_name(ce->name);
+		if (debug_mode) {
+			const struct stat_data *sd = &ce->ce_stat_data;
+
+			printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+			printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+			printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+			printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+			printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
 		}
-		tag = alttag;
 	}
 
-	if (!show_stage) {
-		fputs(tag, stdout);
-	} else {
-		printf("%s%06o %s %d\t",
-		       tag,
-		       ce->ce_mode,
-		       find_unique_abbrev(ce->sha1,abbrev),
-		       ce_stage(ce));
-	}
-	write_eolinfo(ce, ce->name);
-	write_name(ce->name);
-	if (debug_mode) {
-		const struct stat_data *sd = &ce->ce_stat_data;
-
-		printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
-		printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
-		printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
-		printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
-		printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
-	}
+	strbuf_release(&name);
 }
 
 static void show_ru_info(void)
@@ -468,6 +515,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -519,6 +568,24 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules)
+		submodule_prefix = getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT);
+
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol || with_tree ||
+	     (line_terminator == '\0')))
+		die("ls-files --recurse-submodules unsupported mode");
+
+	if (recurse_submodules && error_unmatch)
+		die("ls-files --recurse-submodules does not support "
+		    "--error-unmatch");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support path "
+		    "arguments");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index b2b096a..510b6a2 100644
--- a/git.c
+++ b/git.c
@@ -440,7 +440,7 @@ static struct cmd_struct commands[] = {
 	{ "init-db", cmd_init_db },
 	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
 	{ "log", cmd_log, RUN_SETUP },
-	{ "ls-files", cmd_ls_files, RUN_SETUP },
+	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUBMODULES },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..7d225ac
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	git init submodule &&
+	echo c >submodule/c &&
+	git -C submodule add c &&
+	git -C submodule commit -m "add c" &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	echo a >not_added &&
+	echo b >b/not_added &&
+	echo c >submodule/not_added &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/.gitmodules
+	submodule/c
+	submodule/subsub/d
+	EOF
+
+	git init submodule/subsub &&
+	echo d >submodule/subsub/d &&
+	git -C submodule/subsub add d &&
+	git -C submodule/subsub commit -m "add d" &&
+	git -C submodule submodule add ./subsub &&
+	git -C submodule commit -m "added subsub" &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+	test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep 'unsupported mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/4 v4] ls-files: optionally recurse into submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Allow ls-files to recognize submodules in order to retrieve a list of
> files from a repository's submodules.  This is done by forking off a
> process to recursively call ls-files on all submodules. Use top-level
> --submodule_prefix option to pass a path to the submodule which it can
> use to prepend to output or pathspec matching logic.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  Documentation/git-ls-files.txt         |   7 +-
>  builtin/ls-files.c                     | 143 ++++++++++++++++++++++++---------
>  git.c                                  |   2 +-
>  t/t3007-ls-files-recurse-submodules.sh | 100 +++++++++++++++++++++++
>  4 files changed, 212 insertions(+), 40 deletions(-)
>  create mode 100755 t/t3007-ls-files-recurse-submodules.sh
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 0d933ac..446209e 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -18,7 +18,8 @@ SYNOPSIS
>  		[--exclude-per-directory=<file>]
>  		[--exclude-standard]
>  		[--error-unmatch] [--with-tree=<tree-ish>]
> -		[--full-name] [--abbrev] [--] [<file>...]
> +		[--full-name] [--recurse-submodules]
> +		[--abbrev] [--] [<file>...]
>  
>  DESCRIPTION
>  -----------
> @@ -137,6 +138,10 @@ a space) at the start of each line:
>  	option forces paths to be output relative to the project
>  	top directory.
>  
> +--recurse-submodules::
> +	Recursively calls ls-files on each submodule in the repository.
> +	Currently there is only support for the --cached mode.
> +
>  --abbrev[=<n>]::
>  	Instead of showing the full 40-byte hexadecimal object
>  	lines, show only a partial prefix.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 00ea91a..d4bfc60 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -14,6 +14,7 @@
>  #include "resolve-undo.h"
>  #include "string-list.h"
>  #include "pathspec.h"
> +#include "run-command.h"
>  
>  static int abbrev;
>  static int show_deleted;
> @@ -28,6 +29,8 @@ static int show_valid_bit;
>  static int line_terminator = '\n';
>  static int debug_mode;
>  static int show_eol;
> +static int recurse_submodules;
> +static const char *submodule_prefix;

I would have expected this to added to environment.c in the previous
step, but it is OK--I'd imagine you'd grab this from the environment
and carrying a piece of information from git.c to here by setenv()
followed by getenv() feels somewhat roundabout, though.

>  static const char *prefix;
>  static int max_prefix_len;
> @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
>  static void write_name(const char *name)
>  {
>  	/*
> +	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> +	 * by the caller.

As Peff mentioned in his review in another thread, a large number of
functions in git are not reentrant, and I do not think we would want
to give the impression that those missing a warning are safe to use.

Other than that, this step looks OK.  3/4 and later would be a lot
more fun to review ;-)

Re: [PATCH 2/4 v4] ls-files: optionally recurse into submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> > +static const char *submodule_prefix;
> 
> I would have expected this to added to environment.c in the previous
> step, but it is OK--I'd imagine you'd grab this from the environment
> and carrying a piece of information from git.c to here by setenv()
> followed by getenv() feels somewhat roundabout, though.

If it would make sense to do the caching of prefix string in
environment.c I can move it there and add a get_submodule_prefix()
function which either reads the envvar or the cached value if its
already been read.  Would that be a better route?

> 
> >  static const char *prefix;
> >  static int max_prefix_len;
> > @@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
> >  static void write_name(const char *name)
> >  {
> >  	/*
> > +	 * NEEDSWORK: To make this thread-safe, full_name would have to be owned
> > +	 * by the caller.
> 
> As Peff mentioned in his review in another thread, a large number of
> functions in git are not reentrant, and I do not think we would want
> to give the impression that those missing a warning are safe to use.
> 
> Other than that, this step looks OK.  3/4 and later would be a lot
> more fun to review ;-)

Oh yes, I can remove the comment.  Seemed to miss that bit while
rerolling the series.

-- 
Brandon Williams

[PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

Pass through some known-safe options when recursing into submodules.
(--cached, --stage, -v, -t, -z, --debug, --eol)

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     | 34 ++++++++++++++++++++++++++++++----
 t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++++++-----
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d4bfc60..a39367f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -31,6 +31,7 @@ static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
 static const char *submodule_prefix;
+static struct argv_array recurse_submodules_opts = ARGV_ARRAY_INIT;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(int show_tag)
+{
+	if (show_cached)
+		argv_array_push(&recurse_submodules_opts, "--cached");
+	if (show_stage)
+		argv_array_push(&recurse_submodules_opts, "--stage");
+	if (show_valid_bit)
+		argv_array_push(&recurse_submodules_opts, "-v");
+	if (show_tag)
+		argv_array_push(&recurse_submodules_opts, "-t");
+	if (line_terminator == '\0')
+		argv_array_push(&recurse_submodules_opts, "-z");
+	if (debug_mode)
+		argv_array_push(&recurse_submodules_opts, "--debug");
+	if (show_eol)
+		argv_array_push(&recurse_submodules_opts, "--eol");
+}
+
 /**
  * Recursively call ls-files on a submodule
  */
@@ -184,6 +206,9 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 
+	/* add supported options */
+	argv_array_pushv(&cp.args, recurse_submodules_opts.argv);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -568,14 +593,15 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
-	if (recurse_submodules)
+	if (recurse_submodules) {
 		submodule_prefix = getenv(GIT_SUBMODULE_PREFIX_ENVIRONMENT);
+		compile_submodule_options(show_tag);
+	}
 
 	if (recurse_submodules &&
-	    (show_stage || show_deleted || show_others || show_unmerged ||
+	    (show_deleted || show_others || show_unmerged ||
 	     show_killed || show_modified || show_resolve_undo ||
-	     show_valid_bit || show_tag || show_eol || with_tree ||
-	     (line_terminator == '\0')))
+	     with_tree))
 		die("ls-files --recurse-submodules unsupported mode");
 
 	if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 7d225ac..40767da 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+	cat | tr "\n" "\0" >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules -z >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ls-files does not output files not added to a repo' '
 	cat >expect <<-\EOF &&
 	.gitmodules
@@ -86,15 +98,10 @@ test_incompatible_with_recurse_submodules () {
 	"
 }
 
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
 test_incompatible_with_recurse_submodules --deleted
 test_incompatible_with_recurse_submodules --modified
 test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
 test_incompatible_with_recurse_submodules --killed
 test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Pass through some known-safe options when recursing into submodules.
> (--cached, --stage, -v, -t, -z, --debug, --eol)
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  builtin/ls-files.c                     | 34 ++++++++++++++++++++++++++++++----
>  t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++++++-----
>  2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index d4bfc60..a39367f 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -31,6 +31,7 @@ static int debug_mode;
>  static int show_eol;
>  static int recurse_submodules;
>  static const char *submodule_prefix;
> +static struct argv_array recurse_submodules_opts = ARGV_ARRAY_INIT;

I'd imagine that this is also thread-unsafe, but we do not have to
comment it ;-)

> @@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
>  	}
>  }
>  
> +/*
> + * Compile an argv_array with all of the options supported by --recurse_submodules
> + */
> +static void compile_submodule_options(int show_tag)
> +{
> +	if (show_cached)
> +		argv_array_push(&recurse_submodules_opts, "--cached");
> +	if (show_stage)
> +		argv_array_push(&recurse_submodules_opts, "--stage");
> +	if (show_valid_bit)
> +		argv_array_push(&recurse_submodules_opts, "-v");
> +	if (show_tag)
> +		argv_array_push(&recurse_submodules_opts, "-t");
> +	if (line_terminator == '\0')
> +		argv_array_push(&recurse_submodules_opts, "-z");
> +	if (debug_mode)
> +		argv_array_push(&recurse_submodules_opts, "--debug");
> +	if (show_eol)
> +		argv_array_push(&recurse_submodules_opts, "--eol");
> +}

OK.  These are only the safe ones to pass through?  "compile" or
"assemble" is much less important thing to say than how these are
chosen.  "pass_supported_options()" or something?  I dunno.

>  	if (recurse_submodules &&
> -	    (show_stage || show_deleted || show_others || show_unmerged ||
> +	    (show_deleted || show_others || show_unmerged ||
>  	     show_killed || show_modified || show_resolve_undo ||
> -	     show_valid_bit || show_tag || show_eol || with_tree ||
> -	     (line_terminator == '\0')))
> +	     with_tree))
>  		die("ls-files --recurse-submodules unsupported mode");

Makes sense.

> +test_expect_success 'ls-files correctly outputs files in submodule with -z' '
> +	cat | tr "\n" "\0" >expect <<-\EOF &&
> +	.gitmodules
> +	a
> +	b/b
> +	submodule/c
> +	EOF

Hmm, what do you need "cat" for here?

In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
we seem to avoid using "tr", even though q_to_cr and others do use
it.  I wonder if we had some portability issues with passing NUL
through tr or something?

    ... digs and finds e85fe4d8 ("more tr portability test script
    fixes", 2008-03-12)

So use something like

	perl -pe 'y/\012/\000/' <<\-EOF
        ...
        EOF

instead, perhaps?

> +	git ls-files --recurse-submodules -z >actual &&
> +	test_cmp expect actual
> +'

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> we seem to avoid using "tr", even though q_to_cr and others do use
> it.  I wonder if we had some portability issues with passing NUL
> through tr or something?
>
>     ... digs and finds e85fe4d8 ("more tr portability test script
>     fixes", 2008-03-12)
>
> So use something like
>
> 	perl -pe 'y/\012/\000/' <<\-EOF
>         ...
>         EOF
>
> instead, perhaps?

I actually think it would make more sense to add

    lf_to_nul () {
            perl -pe 'y/\012/\000/'
    }

to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
this route.

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it.  I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> >     ... digs and finds e85fe4d8 ("more tr portability test script
> >     fixes", 2008-03-12)
> >
> > So use something like
> >
> > 	perl -pe 'y/\012/\000/' <<\-EOF
> >         ...
> >         EOF
> >
> > instead, perhaps?
> 
> I actually think it would make more sense to add
> 
>     lf_to_nul () {
>             perl -pe 'y/\012/\000/'
>     }
> 
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.

my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
line feed?

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
> line feed?

Yup.  "man 7 ascii" ;-)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Stefan Beller]

On Tue, Sep 27, 2016 at 1:52 PM, Brandon Williams <bmwill@google.com> wrote:
> On 09/27, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
>> > we seem to avoid using "tr", even though q_to_cr and others do use
>> > it.  I wonder if we had some portability issues with passing NUL
>> > through tr or something?
>> >
>> >     ... digs and finds e85fe4d8 ("more tr portability test script
>> >     fixes", 2008-03-12)
>> >
>> > So use something like
>> >
>> >     perl -pe 'y/\012/\000/' <<\-EOF
>> >         ...
>> >         EOF
>> >
>> > instead, perhaps?
>>
>> I actually think it would make more sense to add
>>
>>     lf_to_nul () {
>>             perl -pe 'y/\012/\000/'
>>     }
>>
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> my mind is drawing a blank, what does the 'lf' in 'lf_to_nul' stand for?
> line feed?
>

Yes, line feed. (Note that Git has to deal with this cross platform new lines
e.g. CRLF is common on Windows, CR was common on MAC, and LF is
Windows, so naming the new line as they are, makes sense here.)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > In nul_to_q and q_to_nul implementations (t/test-lib-functions.sh)
> > we seem to avoid using "tr", even though q_to_cr and others do use
> > it.  I wonder if we had some portability issues with passing NUL
> > through tr or something?
> >
> >     ... digs and finds e85fe4d8 ("more tr portability test script
> >     fixes", 2008-03-12)
> >
> > So use something like
> >
> > 	perl -pe 'y/\012/\000/' <<\-EOF
> >         ...
> >         EOF
> >
> > instead, perhaps?
> 
> I actually think it would make more sense to add
> 
>     lf_to_nul () {
>             perl -pe 'y/\012/\000/'
>     }
> 
> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
> this route.

Turns out this function already exists in test-lib-functions.sh

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>> I actually think it would make more sense to add
>> 
>>     lf_to_nul () {
>>             perl -pe 'y/\012/\000/'
>>     }
>> 
>> to t/test-lib-functions.sh somewhere near q_to_nul if we were to go
>> this route.
>
> Turns out this function already exists in test-lib-functions.sh

;-)

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> > @@ -170,6 +171,27 @@ static void show_killed_files(struct dir_struct *dir)
> >  	}
> >  }
> >  
> > +/*
> > + * Compile an argv_array with all of the options supported by --recurse_submodules
> > + */
> > +static void compile_submodule_options(int show_tag)
> > +{
> > +	if (show_cached)
> > +		argv_array_push(&recurse_submodules_opts, "--cached");
> > +	if (show_stage)
> > +		argv_array_push(&recurse_submodules_opts, "--stage");
> > +	if (show_valid_bit)
> > +		argv_array_push(&recurse_submodules_opts, "-v");
> > +	if (show_tag)
> > +		argv_array_push(&recurse_submodules_opts, "-t");
> > +	if (line_terminator == '\0')
> > +		argv_array_push(&recurse_submodules_opts, "-z");
> > +	if (debug_mode)
> > +		argv_array_push(&recurse_submodules_opts, "--debug");
> > +	if (show_eol)
> > +		argv_array_push(&recurse_submodules_opts, "--eol");
> > +}
> 
> OK.  These are only the safe ones to pass through?  "compile" or
> "assemble" is much less important thing to say than how these are
> chosen.  "pass_supported_options()" or something?  I dunno.

Alternatively we can change this to compile all potential options (not
just the ones that are supported now) and then just error the caller
out, as you've suggested, if an unsupported option used.  'pass' may not
be the most descriptive word for this function as it isn't actually
doing the passing but rather generating an argv of options that will be
passed in the event a submodule is found and we need to kick off a child
process for it.

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

>  	if (recurse_submodules &&
> -	    (show_stage || show_deleted || show_others || show_unmerged ||
> +	    (show_deleted || show_others || show_unmerged ||
>  	     show_killed || show_modified || show_resolve_undo ||
> -	     show_valid_bit || show_tag || show_eol || with_tree ||
> -	     (line_terminator == '\0')))
> +	     with_tree))
>  		die("ls-files --recurse-submodules unsupported mode");

Ahh, one more thing, and this comment probably applies to 2/4 not
this one, but if the intention is to shrink this "not supported yet"
check as the series progresses, in the earlier step the check would
need to make sure no pathspec is given, which is first supported in
4/4, I think.  It is not a big deal to require rerolling by itself,
bit if you are rerolling 2/4 for other reasons, then...

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> >  	if (recurse_submodules &&
> > -	    (show_stage || show_deleted || show_others || show_unmerged ||
> > +	    (show_deleted || show_others || show_unmerged ||
> >  	     show_killed || show_modified || show_resolve_undo ||
> > -	     show_valid_bit || show_tag || show_eol || with_tree ||
> > -	     (line_terminator == '\0')))
> > +	     with_tree))
> >  		die("ls-files --recurse-submodules unsupported mode");
> 
> Ahh, one more thing, and this comment probably applies to 2/4 not
> this one, but if the intention is to shrink this "not supported yet"
> check as the series progresses, in the earlier step the check would
> need to make sure no pathspec is given, which is first supported in
> 4/4, I think.  It is not a big deal to require rerolling by itself,
> bit if you are rerolling 2/4 for other reasons, then...

Oh there is a separate if gaurd for pathspecs which is introduced in 2/4
and then removed once pathspec support has been added in 4/4.

-- 
Brandon Williams

Re: [PATCH 3/4 v4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Oh there is a separate if gaurd for pathspecs which is introduced in 2/4
> and then removed once pathspec support has been added in 4/4.

Thanks; I missed to spot that when I wrote the message you are
responding to, but it indeed is there ;-)

[PATCH 4/4 v4] ls-files: add pathspec matching for submodules

[Brandon Williams]

Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule.  The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.

This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally.  If any wildcard character is encountered we'll simply punt
and produce a false positive match.  More accurate matching will be done
once inside the submodule.  This is due to the superproject not knowing
what files could exist in the submodule.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     |  28 ++++++---
 dir.c                                  |  46 +++++++++++++-
 dir.h                                  |   4 ++
 t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
 4 files changed, 174 insertions(+), 12 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a39367f..2488a02 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -199,6 +199,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
+	int i;
 
 	argv_array_pushf(&cp.args, "--submodule-prefix=%s%s/",
 			 submodule_prefix ? submodule_prefix : "",
@@ -209,6 +210,15 @@ static void show_gitlink(const struct cache_entry *ce)
 	/* add supported options */
 	argv_array_pushv(&cp.args, recurse_submodules_opts.argv);
 
+	/*
+	 * Pass in the original pathspec args.  The submodule will be
+	 * responsible for prepending the 'submodule_prefix' prior to comparing
+	 * against the pathspec for matches.
+	 */
+	argv_array_push(&cp.args, "--");
+	for (i = 0; i < pathspec.nr; i++)
+		argv_array_push(&cp.args, pathspec.items[i].original);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -227,7 +237,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
 		show_gitlink(ce);
 	} else if (match_pathspec(&pathspec, name.buf, name.len,
 				  len, ps_matched,
@@ -608,17 +619,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		die("ls-files --recurse-submodules does not support "
 		    "--error-unmatch");
 
-	if (recurse_submodules && argc)
-		die("ls-files --recurse-submodules does not support path "
-		    "arguments");
-
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 		       prefix, argv);
 
-	/* Find common prefix for all pathspec's */
-	max_prefix = common_prefix(&pathspec);
+	/*
+	 * Find common prefix for all pathspec's
+	 * This is used as a performance optimization which unfortunately cannot
+	 * be done when recursing into submodules
+	 */
+	if (recurse_submodules)
+		max_prefix = NULL;
+	else
+		max_prefix = common_prefix(&pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
-#define DO_MATCH_EXCLUDE   1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE   (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
 
 /*
  * Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 			 item->nowildcard_len - prefix))
 		return MATCHED_FNMATCH;
 
+	/* Perform checks to see if "name" is a super set of the pathspec */
+	if (flags & DO_MATCH_SUBMODULE) {
+		/* name is a literal prefix of the pathspec */
+		if ((namelen < matchlen) &&
+		    (match[namelen] == '/') &&
+		    !ps_strncmp(item, match, name, namelen))
+			return MATCHED_RECURSIVELY;
+
+		/* name" doesn't match up to the first wild character */
+		if (item->nowildcard_len < item->len &&
+		    ps_strncmp(item, match, name,
+			       item->nowildcard_len - prefix))
+			return 0;
+
+		/*
+		 * Here is where we would perform a wildmatch to check if
+		 * "name" can be matched as a directory (or a prefix) against
+		 * the pathspec.  Since wildmatch doesn't have this capability
+		 * at the present we have to punt and say that it is a match,
+		 * potentially returning a false positive
+		 * The submodules themselves will be able to perform more
+		 * accurate matching to determine if the pathspec matches.
+		 */
+		return MATCHED_RECURSIVELY;
+	}
+
 	return 0;
 }
 
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+			 const char *submodule_name,
+			 char *seen)
+{
+	int matched = do_match_pathspec(ps, submodule_name,
+					strlen(submodule_name),
+					0, seen,
+					DO_MATCH_DIRECTORY |
+					DO_MATCH_SUBMODULE);
+	return matched;
+}
+
 int report_path_error(const char *ps_matched,
 		      const struct pathspec *pathspec,
 		      const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
 		       const char *pattern, const char *string,
 		       int prefix);
 
+extern int submodule_path_match(const struct pathspec *ps,
+				const char *submodule_name,
+				char *seen);
+
 static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 40767da..4a51d38 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	test_cmp expect actual
 '
 
-test_expect_success '--recurse-submodules does not support using path arguments' '
-	test_must_fail git ls-files --recurse-submodules b 2>actual &&
-	test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+	echo e >submodule/subsub/e.txt &&
+	git -C submodule/subsub add e.txt &&
+	git -C submodule/subsub commit -m "adding e.txt" &&
+	echo f >submodule/f.TXT &&
+	echo g >submodule/g.txt &&
+	git -C submodule add f.TXT g.txt &&
+	git -C submodule commit -m "add f and g" &&
+	echo h >h.txt &&
+	mkdir sib &&
+	echo sib >sib/file &&
+	git add h.txt sib/file &&
+	git commit -m "add h and sib/file" &&
+	git init sub &&
+	echo sub >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git submodule add ./sub &&
+	git commit -m "added sub" &&
+
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	h.txt
+	sib/file
+	sub/file
+	submodule/.gitmodules
+	submodule/c
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/d
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+	cat actual &&
+	git ls-files --recurse-submodules "*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules "*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "sub" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su*/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su?/file" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sib/file
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "s??/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s???file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s*file" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 4/4 v4] ls-files: add pathspec matching for submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> -	/* Find common prefix for all pathspec's */
> -	max_prefix = common_prefix(&pathspec);
> +	/*
> +	 * Find common prefix for all pathspec's
> +	 * This is used as a performance optimization which unfortunately cannot
> +	 * be done when recursing into submodules
> +	 */
> +	if (recurse_submodules)
> +		max_prefix = NULL;
> +	else
> +		max_prefix = common_prefix(&pathspec);
>  	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;

I still wonder if we can do better than this, as this would be a big
cycle-saver especially in recurse-submodules case.

When you get max_prefix that is "a/b/c", there are three cases:

 * a/b/c is a path prefix for an entry in the index, e.g. a/b/c/d;
   you then can safely use it and you do not have to do any
   recursive invocation of ls-files outside "a/b/c".  You may match
   a/b/c/d in the toplevel, or you may recurse a/b/c/e that is a
   submodule, but you won't have to pay attention to submodules
   outside.

 * a leading path of a/b/c, e.g. a/b, is a gitlink or a blob in the
   index; you can use a/b and you only have to recurse into a/b if
   that is a submodule; if a/b is a blob, you'd show nothing.

 * a/b/c itself and no leading path of it appears in the index; you
   know that nothing will match once you know that you are in this
   situation.

Because a gitlink "a/b" sorts at the same location in the index as a
regular blob "a/b" would, by feeding the max_prefix common_prefix()
gives you (i.e. "a/b/c") to index_name_pos() to see which one of the
three situations you are in can be done fairly cheaply, I would
think.  The index_name_pos() call may find "a/b/c" exactly (case 1),
or return a location where "a/b/c" would be inserted in the list of
existing entries.  If there were "a/b" (or "a") in the index, there
wouldn't be any "a/b/x" (or "a/x") at the same time, so a query for
"a/b/c" would land you next to (just after) an existing entry that
is a leading path of it, if such an entry exists, no?  That would
allow you to tell case 2 above fairly cheaply, I would expect.

It is a separate issue if adding that support to 4/4 is a good idea;
I personally think doing it as a separate follow-up patch would make
more sense, so all of the above is tangent.

Re: [PATCH 4/4 v4] ls-files: add pathspec matching for submodules

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > -	/* Find common prefix for all pathspec's */
> > -	max_prefix = common_prefix(&pathspec);
> > +	/*
> > +	 * Find common prefix for all pathspec's
> > +	 * This is used as a performance optimization which unfortunately cannot
> > +	 * be done when recursing into submodules
> > +	 */
> > +	if (recurse_submodules)
> > +		max_prefix = NULL;
> > +	else
> > +		max_prefix = common_prefix(&pathspec);
> >  	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> 
> I still wonder if we can do better than this, as this would be a big
> cycle-saver especially in recurse-submodules case.
> 
> When you get max_prefix that is "a/b/c", there are three cases:
> 
>  * a/b/c is a path prefix for an entry in the index, e.g. a/b/c/d;
>    you then can safely use it and you do not have to do any
>    recursive invocation of ls-files outside "a/b/c".  You may match
>    a/b/c/d in the toplevel, or you may recurse a/b/c/e that is a
>    submodule, but you won't have to pay attention to submodules
>    outside.
> 
>  * a leading path of a/b/c, e.g. a/b, is a gitlink or a blob in the
>    index; you can use a/b and you only have to recurse into a/b if
>    that is a submodule; if a/b is a blob, you'd show nothing.
> 
>  * a/b/c itself and no leading path of it appears in the index; you
>    know that nothing will match once you know that you are in this
>    situation.
> 
> Because a gitlink "a/b" sorts at the same location in the index as a
> regular blob "a/b" would, by feeding the max_prefix common_prefix()
> gives you (i.e. "a/b/c") to index_name_pos() to see which one of the
> three situations you are in can be done fairly cheaply, I would
> think.  The index_name_pos() call may find "a/b/c" exactly (case 1),
> or return a location where "a/b/c" would be inserted in the list of
> existing entries.  If there were "a/b" (or "a") in the index, there
> wouldn't be any "a/b/x" (or "a/x") at the same time, so a query for
> "a/b/c" would land you next to (just after) an existing entry that
> is a leading path of it, if such an entry exists, no?  That would
> allow you to tell case 2 above fairly cheaply, I would expect.
> 
> It is a separate issue if adding that support to 4/4 is a good idea;
> I personally think doing it as a separate follow-up patch would make
> more sense, so all of the above is tangent.

I agree that this could be a big cycle saver. At the present I was
working towards getting a working implementation but this should
definitely be addressed in a follow-up patch to introduce the
optimization to the recurse-submodule mode.  It hopefully wouldn't be
too hard to implement seeing as its using string literals.

-- 
Brandon Williams

[PATCH v5 0/4] recursive support for ls-files

[Brandon Williams]

The big change in this version is the introduction of a --super-prefix option
to the top level git.  After much discussion this seemed to be a better naming
scheme than 'submodule-prefix' as it could be an option other cmds could use
independent of submodules.

In 3/4 I changed the compile_submodule_options function to compile all options that can be realistically passed through and when an option that isn't supported (or rather safe) yet is provided the caller will be errored out.

Brandon Williams (4):
  git: make super-prefix option
  ls-files: optionally recurse into submodules
  ls-files: pass through safe options for --recurse-submodules
  ls-files: add pathspec matching for submodules

 Documentation/git-ls-files.txt         |   7 +-
 Documentation/git.txt                  |   6 +
 builtin/ls-files.c                     | 202 ++++++++++++++++++++++++-------
 cache.h                                |   2 +
 dir.c                                  |  46 +++++++-
 dir.h                                  |   4 +
 environment.c                          |  10 ++
 git.c                                  |  24 +++-
 t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
 9 files changed, 466 insertions(+), 44 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 1/4] git: make super-prefix option

[Brandon Williams]

Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
which can be used to specify a path from above a repository down to its
root.  The immediate use of this option is by commands which have a
--recurse-submodule option in order to give context to submodules about
how they were invoked.  This option is currently only allowed for
builtins which support a super-prefix.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git.txt |  6 ++++++
 cache.h               |  2 ++
 environment.c         | 10 ++++++++++
 git.c                 | 22 ++++++++++++++++++++++
 4 files changed, 40 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..e3309b9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+    [--super-prefix=<path>]
     <command> [<args>]
 
 DESCRIPTION
@@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
 	details.  Equivalent to setting the `GIT_NAMESPACE` environment
 	variable.
 
+--super-prefix=<path>::
+	Set a prefix which gives a path from above a repository down to its
+	root.  One use is to give submodules context about the superproject that
+	invoked it.  Currently for internal use only.
+
 --bare::
 	Treat the repository as a bare repository.  If GIT_DIR
 	environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..01730e1 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
@@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
+extern const char *get_super_prefix();
 extern const char *get_git_work_tree(void);
 
 /*
diff --git a/environment.c b/environment.c
index ca72464..1a2a779 100644
--- a/environment.c
+++ b/environment.c
@@ -100,6 +100,8 @@ static char *work_tree;
 static const char *namespace;
 static size_t namespace_len;
 
+static const char *super_prefix;
+
 static const char *git_dir, *git_common_dir;
 static char *git_object_dir, *git_index_file, *git_graft_file;
 int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -120,6 +122,7 @@ const char * const local_repo_env[] = {
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
 	GIT_REPLACE_REF_BASE_ENVIRONMENT,
 	GIT_PREFIX_ENVIRONMENT,
+	GIT_SUPER_PREFIX_ENVIRONMENT,
 	GIT_SHALLOW_FILE_ENVIRONMENT,
 	GIT_COMMON_DIR_ENVIRONMENT,
 	NULL
@@ -222,6 +225,13 @@ const char *strip_namespace(const char *namespaced_ref)
 	return namespaced_ref + namespace_len;
 }
 
+const char *get_super_prefix()
+{
+	if (!super_prefix)
+		super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
+	return super_prefix;
+}
+
 static int git_work_tree_initialized;
 
 /*
diff --git a/git.c b/git.c
index 1c61151..3e5cd63 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--super-prefix")) {
+			if (*argc < 2) {
+				fprintf(stderr, "No prefix given for --super-prefix.\n" );
+				usage(git_usage_string);
+			}
+			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+			if (envchanged)
+				*envchanged = 1;
+			(*argv)++;
+			(*argc)--;
+		} else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
+			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
 			char *cwd = xgetcwd();
 			is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE		(1<<3)
+#define SUPPORT_SUPER_PREFIX	(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -344,6 +359,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	}
 	commit_pager_choice();
 
+	if (!help && get_super_prefix()) {
+		if (!(p->option & SUPPORT_SUPER_PREFIX))
+			die("%s doesn't support --super-prefix", p->cmd);
+		if (prefix)
+			die("can't have both a prefix and super-prefix");
+	}
+
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v5 1/4] git: make super-prefix option

[Stefan Beller]

On Wed, Sep 28, 2016 at 2:50 PM, Brandon Williams <bmwill@google.com> wrote:

>
>  DESCRIPTION
> @@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
>         details.  Equivalent to setting the `GIT_NAMESPACE` environment
>         variable.
>
> +--super-prefix=<path>::
> +       Set a prefix which gives a path from above a repository down to its
> +       root.  One use is to give submodules context about the superproject that
> +       invoked it.  Currently for internal use only.

I would put the "Currently for internal use only." at the beginning, so end
users don't have to bother reading though the description, when they want to
only use kosher flags. (Well, checking `man git fetch` and searching
for 'internal',
there doesn't seem to be a consistent way how to document internal flags :(.
It doesn't however advertise the flag in the SYNOPSIS. Ok it doesn't advertise
a lot of flags in its SYNOPSIS)

> +const char *get_super_prefix()
> +{
> +       if (!super_prefix)
> +               super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);

The getenv() function returns a pointer to the value in the
environment, or NULL if there is no match.

So in case this is not set (when e.g. the user did not specify the
super prefix),
we would probe it a couple of times.

The caching effect only occurs when the string is set. So this looks
like we save repetitive
calls, but we do not always do that.

Re: [PATCH v5 1/4] git: make super-prefix option

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> which can be used to specify a path from above a repository down to its
> root.  The immediate use of this option is by commands which have a
> --recurse-submodule option in order to give context to submodules about
> how they were invoked.  This option is currently only allowed for
> builtins which support a super-prefix.

Yes, it can be used to specify that, but it is unclear (1) how such
a path thusly specified is used, (2) how it affects the outcome of
the operations, and (3) what it means (the same applies to the
documentation update).

It would help future readers of "git log", "tig blame", etc. to have
a few sentences after the first sentence above.  Perhaps

    ... specify a path from above ... down to its root.  When such a
    super-prefix is specified, the paths reported by Git are
    prefixed with it to make them relative to that directory
    "above".  The paths given by the users on the command line
    (e.g. "git subcmd --output-file=path/to/a/file" and pathspecs)
    are taken relative to that directory "above" to match.

or something like that?

> @@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
>  extern int get_common_dir(struct strbuf *sb, const char *gitdir);
>  extern const char *get_git_namespace(void);
>  extern const char *strip_namespace(const char *namespaced_ref);
> +extern const char *get_super_prefix();

Nice, but 

    extern const char *get_super_prefix(void);

> +const char *get_super_prefix()

Likewise.

> +{
> +	if (!super_prefix)
> +		super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> +	return super_prefix;
> +}

Good.

>  	commit_pager_choice();
>  
> +	if (!help && get_super_prefix()) {
> +		if (!(p->option & SUPPORT_SUPER_PREFIX))
> +			die("%s doesn't support --super-prefix", p->cmd);
> +		if (prefix)
> +			die("can't have both a prefix and super-prefix");
> +	}

Nice.  I'd phrase the latter as "can't use--super-prefix from a
subdirectory", though.

Re: [PATCH v5 1/4] git: make super-prefix option

[Jeff King]

On Wed, Sep 28, 2016 at 02:50:40PM -0700, Brandon Williams wrote:

> Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> which can be used to specify a path from above a repository down to its
> root.  The immediate use of this option is by commands which have a
> --recurse-submodule option in order to give context to submodules about
> how they were invoked.  This option is currently only allowed for
> builtins which support a super-prefix.

What about non-builtins?

E.g., what should

  git --super-prefix=foo bar

do? Should the externals and scripts check the presence of
GIT_INTERNAL_SUPER_PREFIX and barf if it is set? Most scripts would
probably notice eventually when calling some other builtin that doesn't
support SUPER_PREFIX, but it seems hacky to count on that.

There's also the question of 3rd-party programs. If we want to be
conservative, I think you'd want to just always bail in
execv_dashed_external() if --super-prefix is in use. That doesn't give
an option for scripts to say "hey, I support this", but we can perhaps
worry about loosening later.

-Peff

Re: [PATCH v5 1/4] git: make super-prefix option

[Brandon Williams]

On 09/29, Jeff King wrote:
> On Wed, Sep 28, 2016 at 02:50:40PM -0700, Brandon Williams wrote:
> 
> > Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> > which can be used to specify a path from above a repository down to its
> > root.  The immediate use of this option is by commands which have a
> > --recurse-submodule option in order to give context to submodules about
> > how they were invoked.  This option is currently only allowed for
> > builtins which support a super-prefix.
> 
> What about non-builtins?
> 
> E.g., what should
> 
>   git --super-prefix=foo bar
> 
> do? Should the externals and scripts check the presence of
> GIT_INTERNAL_SUPER_PREFIX and barf if it is set? Most scripts would
> probably notice eventually when calling some other builtin that doesn't
> support SUPER_PREFIX, but it seems hacky to count on that.
> 
> There's also the question of 3rd-party programs. If we want to be
> conservative, I think you'd want to just always bail in
> execv_dashed_external() if --super-prefix is in use. That doesn't give
> an option for scripts to say "hey, I support this", but we can perhaps
> worry about loosening later.
> 
> -Peff

That makes sense.

-- 
Brandon Williams

[PATCH v5 2/4] ls-files: optionally recurse into submodules

[Brandon Williams]

Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules.  This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--super-prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |   7 +-
 builtin/ls-files.c                     | 139 ++++++++++++++++++++++++---------
 git.c                                  |   2 +-
 t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
 4 files changed, 208 insertions(+), 40 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,10 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode.
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..e0e5cf5 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,8 +29,10 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
 
 static const char *prefix;
+static const char *super_prefix;
 static int max_prefix_len;
 static int prefix_len;
 static struct pathspec pathspec;
@@ -68,6 +71,19 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
 static void write_name(const char *name)
 {
 	/*
+	 * Prepend the super_prefix to name to construct the full_name to be
+	 * written.  'full_name' gets reused across output lines to minimize the
+	 * allocation churn.
+	 */
+	static struct strbuf full_name = STRBUF_INIT;
+	if (super_prefix && *super_prefix) {
+		strbuf_reset(&full_name);
+		strbuf_addstr(&full_name, super_prefix);
+		strbuf_addstr(&full_name, name);
+		name = full_name.buf;
+	}
+
+	/*
 	 * With "--full-name", prefix_len=0; this caller needs to pass
 	 * an empty string in that case (a NULL is good for "").
 	 */
@@ -152,55 +168,84 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
+			 super_prefix ? super_prefix : "",
+			 ce->name);
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	status = run_command(&cp);
+	if (status)
+		exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
+	struct strbuf name = STRBUF_INIT;
 	int len = max_prefix_len;
+	if (super_prefix)
+		strbuf_addstr(&name, super_prefix);
+	strbuf_addstr(&name, ce->name);
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
-			    len, ps_matched,
-			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
-		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+	} else if (match_pathspec(&pathspec, name.buf, name.len,
+				  len, ps_matched,
+				  S_ISDIR(ce->ce_mode) ||
+				  S_ISGITLINK(ce->ce_mode))) {
+		if (tag && *tag && show_valid_bit &&
+		    (ce->ce_flags & CE_VALID)) {
+			static char alttag[4];
+			memcpy(alttag, tag, 3);
+			if (isalpha(tag[0]))
+				alttag[0] = tolower(tag[0]);
+			else if (tag[0] == '?')
+				alttag[0] = '!';
+			else {
+				alttag[0] = 'v';
+				alttag[1] = tag[0];
+				alttag[2] = ' ';
+				alttag[3] = 0;
+			}
+			tag = alttag;
+		}
 
-	if (tag && *tag && show_valid_bit &&
-	    (ce->ce_flags & CE_VALID)) {
-		static char alttag[4];
-		memcpy(alttag, tag, 3);
-		if (isalpha(tag[0]))
-			alttag[0] = tolower(tag[0]);
-		else if (tag[0] == '?')
-			alttag[0] = '!';
-		else {
-			alttag[0] = 'v';
-			alttag[1] = tag[0];
-			alttag[2] = ' ';
-			alttag[3] = 0;
+		if (!show_stage) {
+			fputs(tag, stdout);
+		} else {
+			printf("%s%06o %s %d\t",
+			       tag,
+			       ce->ce_mode,
+			       find_unique_abbrev(ce->sha1,abbrev),
+			       ce_stage(ce));
+		}
+		write_eolinfo(ce, ce->name);
+		write_name(ce->name);
+		if (debug_mode) {
+			const struct stat_data *sd = &ce->ce_stat_data;
+
+			printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+			printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+			printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+			printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+			printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
 		}
-		tag = alttag;
 	}
 
-	if (!show_stage) {
-		fputs(tag, stdout);
-	} else {
-		printf("%s%06o %s %d\t",
-		       tag,
-		       ce->ce_mode,
-		       find_unique_abbrev(ce->sha1,abbrev),
-		       ce_stage(ce));
-	}
-	write_eolinfo(ce, ce->name);
-	write_name(ce->name);
-	if (debug_mode) {
-		const struct stat_data *sd = &ce->ce_stat_data;
-
-		printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
-		printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
-		printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
-		printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
-		printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
-	}
+	strbuf_release(&name);
 }
 
 static void show_ru_info(void)
@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -484,6 +531,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
+	super_prefix = get_super_prefix();
 	git_config(git_default_config, NULL);
 
 	if (read_cache() < 0)
@@ -519,6 +567,21 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol || with_tree ||
+	     (line_terminator == '\0')))
+		die("ls-files --recurse-submodules unsupported mode");
+
+	if (recurse_submodules && error_unmatch)
+		die("ls-files --recurse-submodules does not support "
+		    "--error-unmatch");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support path "
+		    "arguments");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index 3e5cd63..86d9be4 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@ static struct cmd_struct commands[] = {
 	{ "init-db", cmd_init_db },
 	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
 	{ "log", cmd_log, RUN_SETUP },
-	{ "ls-files", cmd_ls_files, RUN_SETUP },
+	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..7d225ac
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	git init submodule &&
+	echo c >submodule/c &&
+	git -C submodule add c &&
+	git -C submodule commit -m "add c" &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	echo a >not_added &&
+	echo b >b/not_added &&
+	echo c >submodule/not_added &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/.gitmodules
+	submodule/c
+	submodule/subsub/d
+	EOF
+
+	git init submodule/subsub &&
+	echo d >submodule/subsub/d &&
+	git -C submodule/subsub add d &&
+	git -C submodule/subsub commit -m "add d" &&
+	git -C submodule submodule add ./subsub &&
+	git -C submodule commit -m "added subsub" &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_i18ngrep "does not support path arguments" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+	test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep 'unsupported mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v5 2/4] ls-files: optionally recurse into submodules

[Stefan Beller]

On Wed, Sep 28, 2016 at 2:50 PM, Brandon Williams <bmwill@google.com> wrote:
> Allow ls-files to recognize submodules in order to retrieve a list of
> files from a repository's submodules.  This is done by forking off a
> process to recursively call ls-files on all submodules. Use top-level
> --super-prefix option to pass a path to the submodule which it can
> use to prepend to output or pathspec matching logic.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  Documentation/git-ls-files.txt         |   7 +-
>  builtin/ls-files.c                     | 139 ++++++++++++++++++++++++---------
>  git.c                                  |   2 +-
>  t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
>  4 files changed, 208 insertions(+), 40 deletions(-)
>  create mode 100755 t/t3007-ls-files-recurse-submodules.sh
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index 0d933ac..446209e 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -18,7 +18,8 @@ SYNOPSIS
>                 [--exclude-per-directory=<file>]
>                 [--exclude-standard]
>                 [--error-unmatch] [--with-tree=<tree-ish>]
> -               [--full-name] [--abbrev] [--] [<file>...]
> +               [--full-name] [--recurse-submodules]
> +               [--abbrev] [--] [<file>...]
>
>  DESCRIPTION
>  -----------
> @@ -137,6 +138,10 @@ a space) at the start of each line:
>         option forces paths to be output relative to the project
>         top directory.
>
> +--recurse-submodules::
> +       Recursively calls ls-files on each submodule in the repository.
> +       Currently there is only support for the --cached mode.
> +
>  --abbrev[=<n>]::
>         Instead of showing the full 40-byte hexadecimal object
>         lines, show only a partial prefix.
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index 00ea91a..e0e5cf5 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -14,6 +14,7 @@
>  #include "resolve-undo.h"
>  #include "string-list.h"
>  #include "pathspec.h"
> +#include "run-command.h"
>
>  static int abbrev;
>  static int show_deleted;
> @@ -28,8 +29,10 @@ static int show_valid_bit;
>  static int line_terminator = '\n';
>  static int debug_mode;
>  static int show_eol;
> +static int recurse_submodules;
>
>  static const char *prefix;
> +static const char *super_prefix;
>  static int max_prefix_len;
>  static int prefix_len;
>  static struct pathspec pathspec;
> @@ -68,6 +71,19 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
>  static void write_name(const char *name)
>  {
>         /*
> +        * Prepend the super_prefix to name to construct the full_name to be
> +        * written.  'full_name' gets reused across output lines to minimize the
> +        * allocation churn.
> +        */

When doing these tricks with the allocation churn (i.e. we make it
hard to read and understand
for a reader, then we should do it completely, i.e. keep full_name in
the strbuf and
only do a strbuf_setlen to reset the buffer just a bit. With this
implementation we burden the
reader/user to understand how the memory is kept over multiple calls
to this function,
but we still do more work than expected). So either I'd not worry
about performance
and provide an 'obvious correct' implementation, with e.g. no static
here and we free the memory
correctly. Or you'd go the performance route, but then we'd usually
ask for numbers.
(How much faster is it; Does the trickyness trade off well to the
performance gain?)


> +       static struct strbuf full_name = STRBUF_INIT;
> +       if (super_prefix && *super_prefix) {

Why do we have to check twice here? Wouldn't just

    if (super_prefix) {
        ...

be enough?

Re: [PATCH v5 2/4] ls-files: optionally recurse into submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> +--recurse-submodules::
> +	Recursively calls ls-files on each submodule in the repository.
> +	Currently there is only support for the --cached mode.

Good to describe it, but at this step, there is only support for the
"--cached" mode and it does not take a pathspec.

Also as the series progresses, I would expect this to be updated in
patches $n/4 (2 < $n).

> +	if (recurse_submodules && argc)
> +		die("ls-files --recurse-submodules does not support path "
> +		    "arguments");

Hmm, s/path arguments/pathspec/, perhaps, as the latter is specified
in the glossary?

[PATCH v5 3/4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

Pass through some known-safe options when recursing into submodules.
(--cached, --stage, -v, -t, -z, --debug, --eol)

Other options are compiled into an argv_array but if an unsafe option is
given the caller will be errored out.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     | 51 ++++++++++++++++++++++++++++++++--
 t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++----
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e0e5cf5..f377e36 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
 
 static const char *prefix;
 static const char *super_prefix;
@@ -168,6 +169,45 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+	if (line_terminator == '\0')
+		argv_array_push(&submodules_options, "-z");
+	if (show_tag)
+		argv_array_push(&submodules_options, "-t");
+	if (show_valid_bit)
+		argv_array_push(&submodules_options, "-v");
+	if (show_cached)
+		argv_array_push(&submodules_options, "--cached");
+	if (show_deleted)
+		argv_array_push(&submodules_options, "--deleted");
+	if (show_modified)
+		argv_array_push(&submodules_options, "--modified");
+	if (show_others)
+		argv_array_push(&submodules_options, "--others");
+	if (dir->flags & DIR_SHOW_IGNORED)
+		argv_array_push(&submodules_options, "--ignored");
+	if (show_stage)
+		argv_array_push(&submodules_options, "--stage");
+	if (show_killed)
+		argv_array_push(&submodules_options, "--killed");
+	if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
+		argv_array_push(&submodules_options, "--directory");
+	if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
+		argv_array_push(&submodules_options, "--empty-directory");
+	if (show_unmerged)
+		argv_array_push(&submodules_options, "--unmerged");
+	if (show_resolve_undo)
+		argv_array_push(&submodules_options, "--resolve-undo");
+	if (show_eol)
+		argv_array_push(&submodules_options, "--eol");
+	if (debug_mode)
+		argv_array_push(&submodules_options, "--debug");
+}
+
 /**
  * Recursively call ls-files on a submodule
  */
@@ -182,6 +222,9 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 
+	/* add supported options */
+	argv_array_pushv(&cp.args, submodules_options.argv);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -567,11 +610,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules)
+		compile_submodule_options(&dir, show_tag);
+
 	if (recurse_submodules &&
-	    (show_stage || show_deleted || show_others || show_unmerged ||
+	    (show_deleted || show_others || show_unmerged ||
 	     show_killed || show_modified || show_resolve_undo ||
-	     show_valid_bit || show_tag || show_eol || with_tree ||
-	     (line_terminator == '\0')))
+	     with_tree))
 		die("ls-files --recurse-submodules unsupported mode");
 
 	if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 7d225ac..79107d8 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+	lf_to_nul >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules -z >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ls-files does not output files not added to a repo' '
 	cat >expect <<-\EOF &&
 	.gitmodules
@@ -86,15 +98,10 @@ test_incompatible_with_recurse_submodules () {
 	"
 }
 
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
 test_incompatible_with_recurse_submodules --deleted
 test_incompatible_with_recurse_submodules --modified
 test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
 test_incompatible_with_recurse_submodules --killed
 test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
 
 test_done
-- 
2.8.0.rc3.226.g39d4020

[PATCH v5 4/4] ls-files: add pathspec matching for submodules

[Brandon Williams]

Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule.  The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.

This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally.  If any wildcard character is encountered we'll simply punt
and produce a false positive match.  More accurate matching will be done
once inside the submodule.  This is due to the superproject not knowing
what files could exist in the submodule.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     |  28 ++++++---
 dir.c                                  |  46 +++++++++++++-
 dir.h                                  |   4 ++
 t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
 4 files changed, 174 insertions(+), 12 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index f377e36..c8c5c29 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -215,6 +215,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
+	int i;
 
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",
@@ -225,6 +226,15 @@ static void show_gitlink(const struct cache_entry *ce)
 	/* add supported options */
 	argv_array_pushv(&cp.args, submodules_options.argv);
 
+	/*
+	 * Pass in the original pathspec args.  The submodule will be
+	 * responsible for prepending the 'submodule_prefix' prior to comparing
+	 * against the pathspec for matches.
+	 */
+	argv_array_push(&cp.args, "--");
+	for (i = 0; i < pathspec.nr; i++)
+		argv_array_push(&cp.args, pathspec.items[i].original);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -243,7 +253,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
 		show_gitlink(ce);
 	} else if (match_pathspec(&pathspec, name.buf, name.len,
 				  len, ps_matched,
@@ -623,17 +634,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		die("ls-files --recurse-submodules does not support "
 		    "--error-unmatch");
 
-	if (recurse_submodules && argc)
-		die("ls-files --recurse-submodules does not support path "
-		    "arguments");
-
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 		       prefix, argv);
 
-	/* Find common prefix for all pathspec's */
-	max_prefix = common_prefix(&pathspec);
+	/*
+	 * Find common prefix for all pathspec's
+	 * This is used as a performance optimization which unfortunately cannot
+	 * be done when recursing into submodules
+	 */
+	if (recurse_submodules)
+		max_prefix = NULL;
+	else
+		max_prefix = common_prefix(&pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
-#define DO_MATCH_EXCLUDE   1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE   (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
 
 /*
  * Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 			 item->nowildcard_len - prefix))
 		return MATCHED_FNMATCH;
 
+	/* Perform checks to see if "name" is a super set of the pathspec */
+	if (flags & DO_MATCH_SUBMODULE) {
+		/* name is a literal prefix of the pathspec */
+		if ((namelen < matchlen) &&
+		    (match[namelen] == '/') &&
+		    !ps_strncmp(item, match, name, namelen))
+			return MATCHED_RECURSIVELY;
+
+		/* name" doesn't match up to the first wild character */
+		if (item->nowildcard_len < item->len &&
+		    ps_strncmp(item, match, name,
+			       item->nowildcard_len - prefix))
+			return 0;
+
+		/*
+		 * Here is where we would perform a wildmatch to check if
+		 * "name" can be matched as a directory (or a prefix) against
+		 * the pathspec.  Since wildmatch doesn't have this capability
+		 * at the present we have to punt and say that it is a match,
+		 * potentially returning a false positive
+		 * The submodules themselves will be able to perform more
+		 * accurate matching to determine if the pathspec matches.
+		 */
+		return MATCHED_RECURSIVELY;
+	}
+
 	return 0;
 }
 
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+			 const char *submodule_name,
+			 char *seen)
+{
+	int matched = do_match_pathspec(ps, submodule_name,
+					strlen(submodule_name),
+					0, seen,
+					DO_MATCH_DIRECTORY |
+					DO_MATCH_SUBMODULE);
+	return matched;
+}
+
 int report_path_error(const char *ps_matched,
 		      const struct pathspec *pathspec,
 		      const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
 		       const char *pattern, const char *string,
 		       int prefix);
 
+extern int submodule_path_match(const struct pathspec *ps,
+				const char *submodule_name,
+				char *seen);
+
 static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 79107d8..5475855 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	test_cmp expect actual
 '
 
-test_expect_success '--recurse-submodules does not support using path arguments' '
-	test_must_fail git ls-files --recurse-submodules b 2>actual &&
-	test_i18ngrep "does not support path arguments" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+	echo e >submodule/subsub/e.txt &&
+	git -C submodule/subsub add e.txt &&
+	git -C submodule/subsub commit -m "adding e.txt" &&
+	echo f >submodule/f.TXT &&
+	echo g >submodule/g.txt &&
+	git -C submodule add f.TXT g.txt &&
+	git -C submodule commit -m "add f and g" &&
+	echo h >h.txt &&
+	mkdir sib &&
+	echo sib >sib/file &&
+	git add h.txt sib/file &&
+	git commit -m "add h and sib/file" &&
+	git init sub &&
+	echo sub >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git submodule add ./sub &&
+	git commit -m "added sub" &&
+
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	h.txt
+	sib/file
+	sub/file
+	submodule/.gitmodules
+	submodule/c
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/d
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+	cat actual &&
+	git ls-files --recurse-submodules "*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules "*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "sub" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su*/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su?/file" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sib/file
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "s??/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s???file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s*file" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
-- 
2.8.0.rc3.226.g39d4020

[PATCH v6 0/4] recursive support for ls-files

[Brandon Williams]

Minor fixes per the comments on version 5.

Brandon Williams (4):
  git: make super-prefix option
  ls-files: optionally recurse into submodules
  ls-files: pass through safe options for --recurse-submodules
  ls-files: add pathspec matching for submodules

 Documentation/git-ls-files.txt         |   7 +-
 Documentation/git.txt                  |   6 +
 builtin/ls-files.c                     | 202 ++++++++++++++++++++++++-------
 cache.h                                |   2 +
 dir.c                                  |  46 +++++++-
 dir.h                                  |   4 +
 environment.c                          |  10 ++
 git.c                                  |  28 ++++-
 t/t3007-ls-files-recurse-submodules.sh | 209 +++++++++++++++++++++++++++++++++
 9 files changed, 470 insertions(+), 44 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

-- 
2.10.0

[PATCH v6 1/4] git: make super-prefix option

[Brandon Williams]

Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
which can be used to specify a path from above a repository down to its
root.  When such a super-prefix is specified, the paths reported by Git
are prefixed with it to make them relative to that directory "above".
The paths given by the user on the command line
(e.g. "git subcmd --output-file=path/to/a/file" and pathspecs) are taken
relative to the directory "above" to match.

The immediate use of this option is by commands which have a
--recurse-submodule option in order to give context to submodules about
how they were invoked.  This option is currently only allowed for
builtins which support a super-prefix.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git.txt |  6 ++++++
 cache.h               |  2 ++
 environment.c         | 10 ++++++++++
 git.c                 | 26 ++++++++++++++++++++++++++
 4 files changed, 44 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..2188ae6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+    [--super-prefix=<path>]
     <command> [<args>]
 
 DESCRIPTION
@@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
 	details.  Equivalent to setting the `GIT_NAMESPACE` environment
 	variable.
 
+--super-prefix=<path>::
+	Currently for internal use only.  Set a prefix which gives a path from
+	above a repository down to its root.  One use is to give submodules
+	context about the superproject that invoked it.
+
 --bare::
 	Treat the repository as a bare repository.  If GIT_DIR
 	environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..8cf495d 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
@@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
+extern const char *get_super_prefix(void);
 extern const char *get_git_work_tree(void);
 
 /*
diff --git a/environment.c b/environment.c
index ca72464..13f3d70 100644
--- a/environment.c
+++ b/environment.c
@@ -100,6 +100,8 @@ static char *work_tree;
 static const char *namespace;
 static size_t namespace_len;
 
+static const char *super_prefix;
+
 static const char *git_dir, *git_common_dir;
 static char *git_object_dir, *git_index_file, *git_graft_file;
 int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -120,6 +122,7 @@ const char * const local_repo_env[] = {
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
 	GIT_REPLACE_REF_BASE_ENVIRONMENT,
 	GIT_PREFIX_ENVIRONMENT,
+	GIT_SUPER_PREFIX_ENVIRONMENT,
 	GIT_SHALLOW_FILE_ENVIRONMENT,
 	GIT_COMMON_DIR_ENVIRONMENT,
 	NULL
@@ -222,6 +225,13 @@ const char *strip_namespace(const char *namespaced_ref)
 	return namespaced_ref + namespace_len;
 }
 
+const char *get_super_prefix(void)
+{
+	if (!super_prefix)
+		super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
+	return super_prefix;
+}
+
 static int git_work_tree_initialized;
 
 /*
diff --git a/git.c b/git.c
index 1c61151..f756b62 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--super-prefix")) {
+			if (*argc < 2) {
+				fprintf(stderr, "No prefix given for --super-prefix.\n" );
+				usage(git_usage_string);
+			}
+			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+			if (envchanged)
+				*envchanged = 1;
+			(*argv)++;
+			(*argc)--;
+		} else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
+			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
 			char *cwd = xgetcwd();
 			is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE		(1<<3)
+#define SUPPORT_SUPER_PREFIX	(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -344,6 +359,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	}
 	commit_pager_choice();
 
+	if (!help && get_super_prefix()) {
+		if (!(p->option & SUPPORT_SUPER_PREFIX))
+			die("%s doesn't support --super-prefix", p->cmd);
+		if (prefix)
+			die("can't use --super-prefix from a subdirectory");
+	}
+
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
@@ -558,6 +580,10 @@ static void execv_dashed_external(const char **argv)
 	const char *tmp;
 	int status;
 
+	if (get_super_prefix()) {
+		die("%s doesn't support --super-prefix", argv[0]);
+	}
+
 	if (use_pager == -1)
 		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
-- 
2.10.0

Re: [PATCH v6 1/4] git: make super-prefix option

[Stefan Beller]

On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:

>
> +const char *get_super_prefix(void)
> +{
> +       if (!super_prefix)
> +               super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> +       return super_prefix;
> +}
> +

As said earlier, is the following a valid thought:

> The getenv() function returns a pointer to the value in the
> environment, or NULL if there is no match.
> So in case this is not set (when e.g. the user did not specify the
> super prefix), we would probe it a couple of times.
> The caching effect only occurs when the string is set. So this looks
> like we save repetitive calls, but we do not always do that.

>
> +       if (get_super_prefix()) {
> +               die("%s doesn't support --super-prefix", argv[0]);
> +       }
> +

Nit: no braces, please.

Re: [PATCH v6 1/4] git: make super-prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
>
>>
>> +const char *get_super_prefix(void)
>> +{
>> +       if (!super_prefix)
>> +               super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
>> +       return super_prefix;
>> +}
>> +
>
> As said earlier, is the following a valid thought:
>
>> The getenv() function returns a pointer to the value in the
>> environment, or NULL if there is no match.
>> So in case this is not set (when e.g. the user did not specify the
>> super prefix), we would probe it a couple of times.
>> The caching effect only occurs when the string is set. So this looks
>> like we save repetitive calls, but we do not always do that.

That reading is correct.  If the code wants to do the caching, it
should do so correctly.  Unless we expect that some callers might
want to be able to invalidate the cache,

	get_super_prefix(void)
	{
		static int initialized;
		if (!initialized) {
			super_prefix = getenv(...);
			initialized = 1;
		}
                return super_prefix;
	}                

would suffice.

Thanks for careful reading.

Re: [PATCH v6 1/4] git: make super-prefix option

[Jeff King]

On Tue, Oct 04, 2016 at 10:31:51AM -0700, Stefan Beller wrote:

> On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
> 
> >
> > +const char *get_super_prefix(void)
> > +{
> > +       if (!super_prefix)
> > +               super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
> > +       return super_prefix;
> > +}
> > +
> 
> As said earlier, is the following a valid thought:
> 
> > The getenv() function returns a pointer to the value in the
> > environment, or NULL if there is no match.
> > So in case this is not set (when e.g. the user did not specify the
> > super prefix), we would probe it a couple of times.
> > The caching effect only occurs when the string is set. So this looks
> > like we save repetitive calls, but we do not always do that.

I think your concern is valid. If it is not set, we will do an O(n)
search through the whole environment on each call.

I also think the result of getenv() needs to be copied. In some
implementations it persists for the life of the program, but that's not
guaranteed; it may be overwritten by unrelated calls to getenv() or
setenv().

-Peff

[PATCH v6 2/4] ls-files: optionally recurse into submodules

[Brandon Williams]

Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules.  This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--super-prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |   8 +-
 builtin/ls-files.c                     | 138 ++++++++++++++++++++++++---------
 git.c                                  |   2 +-
 t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
 4 files changed, 208 insertions(+), 40 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..ea01d45 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,11 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode without a
+	pathspec.
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..63befed 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,8 +29,10 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
 
 static const char *prefix;
+static const char *super_prefix;
 static int max_prefix_len;
 static int prefix_len;
 static struct pathspec pathspec;
@@ -68,11 +71,24 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
 static void write_name(const char *name)
 {
 	/*
+	 * Prepend the super_prefix to name to construct the full_name to be
+	 * written.
+	 */
+	struct strbuf full_name = STRBUF_INIT;
+	if (super_prefix) {
+		strbuf_addstr(&full_name, super_prefix);
+		strbuf_addstr(&full_name, name);
+		name = full_name.buf;
+	}
+
+	/*
 	 * With "--full-name", prefix_len=0; this caller needs to pass
 	 * an empty string in that case (a NULL is good for "").
 	 */
 	write_name_quoted_relative(name, prefix_len ? prefix : NULL,
 				   stdout, line_terminator);
+
+	strbuf_release(&full_name);
 }
 
 static void show_dir_entry(const char *tag, struct dir_entry *ent)
@@ -152,55 +168,84 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
+			 super_prefix ? super_prefix : "",
+			 ce->name);
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	status = run_command(&cp);
+	if (status)
+		exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
+	struct strbuf name = STRBUF_INIT;
 	int len = max_prefix_len;
+	if (super_prefix)
+		strbuf_addstr(&name, super_prefix);
+	strbuf_addstr(&name, ce->name);
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
-			    len, ps_matched,
-			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
-		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+	} else if (match_pathspec(&pathspec, name.buf, name.len,
+				  len, ps_matched,
+				  S_ISDIR(ce->ce_mode) ||
+				  S_ISGITLINK(ce->ce_mode))) {
+		if (tag && *tag && show_valid_bit &&
+		    (ce->ce_flags & CE_VALID)) {
+			static char alttag[4];
+			memcpy(alttag, tag, 3);
+			if (isalpha(tag[0]))
+				alttag[0] = tolower(tag[0]);
+			else if (tag[0] == '?')
+				alttag[0] = '!';
+			else {
+				alttag[0] = 'v';
+				alttag[1] = tag[0];
+				alttag[2] = ' ';
+				alttag[3] = 0;
+			}
+			tag = alttag;
+		}
 
-	if (tag && *tag && show_valid_bit &&
-	    (ce->ce_flags & CE_VALID)) {
-		static char alttag[4];
-		memcpy(alttag, tag, 3);
-		if (isalpha(tag[0]))
-			alttag[0] = tolower(tag[0]);
-		else if (tag[0] == '?')
-			alttag[0] = '!';
-		else {
-			alttag[0] = 'v';
-			alttag[1] = tag[0];
-			alttag[2] = ' ';
-			alttag[3] = 0;
+		if (!show_stage) {
+			fputs(tag, stdout);
+		} else {
+			printf("%s%06o %s %d\t",
+			       tag,
+			       ce->ce_mode,
+			       find_unique_abbrev(ce->sha1,abbrev),
+			       ce_stage(ce));
+		}
+		write_eolinfo(ce, ce->name);
+		write_name(ce->name);
+		if (debug_mode) {
+			const struct stat_data *sd = &ce->ce_stat_data;
+
+			printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+			printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+			printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+			printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+			printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
 		}
-		tag = alttag;
 	}
 
-	if (!show_stage) {
-		fputs(tag, stdout);
-	} else {
-		printf("%s%06o %s %d\t",
-		       tag,
-		       ce->ce_mode,
-		       find_unique_abbrev(ce->sha1,abbrev),
-		       ce_stage(ce));
-	}
-	write_eolinfo(ce, ce->name);
-	write_name(ce->name);
-	if (debug_mode) {
-		const struct stat_data *sd = &ce->ce_stat_data;
-
-		printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
-		printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
-		printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
-		printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
-		printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
-	}
+	strbuf_release(&name);
 }
 
 static void show_ru_info(void)
@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -484,6 +531,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
+	super_prefix = get_super_prefix();
 	git_config(git_default_config, NULL);
 
 	if (read_cache() < 0)
@@ -519,6 +567,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol || with_tree ||
+	     (line_terminator == '\0')))
+		die("ls-files --recurse-submodules unsupported mode");
+
+	if (recurse_submodules && error_unmatch)
+		die("ls-files --recurse-submodules does not support "
+		    "--error-unmatch");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support pathspec");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index f756b62..1bbd5ae 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@ static struct cmd_struct commands[] = {
 	{ "init-db", cmd_init_db },
 	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
 	{ "log", cmd_log, RUN_SETUP },
-	{ "ls-files", cmd_ls_files, RUN_SETUP },
+	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..b5a53c3
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	git init submodule &&
+	echo c >submodule/c &&
+	git -C submodule add c &&
+	git -C submodule commit -m "add c" &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	echo a >not_added &&
+	echo b >b/not_added &&
+	echo c >submodule/not_added &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/.gitmodules
+	submodule/c
+	submodule/subsub/d
+	EOF
+
+	git init submodule/subsub &&
+	echo d >submodule/subsub/d &&
+	git -C submodule/subsub add d &&
+	git -C submodule/subsub commit -m "add d" &&
+	git -C submodule submodule add ./subsub &&
+	git -C submodule commit -m "added subsub" &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_i18ngrep "does not support pathspec" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+	test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep 'unsupported mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
-- 
2.10.0

[PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

Pass through some known-safe options when recursing into submodules.
(--cached, --stage, -v, -t, -z, --debug, --eol)

Other options are compiled into an argv_array but if an unsafe option is
given the caller will be errored out.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     | 51 ++++++++++++++++++++++++++++++++--
 t/t3007-ls-files-recurse-submodules.sh | 17 ++++++++----
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 63befed..6f744ef 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
 
 static const char *prefix;
 static const char *super_prefix;
@@ -168,6 +169,45 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+	if (line_terminator == '\0')
+		argv_array_push(&submodules_options, "-z");
+	if (show_tag)
+		argv_array_push(&submodules_options, "-t");
+	if (show_valid_bit)
+		argv_array_push(&submodules_options, "-v");
+	if (show_cached)
+		argv_array_push(&submodules_options, "--cached");
+	if (show_deleted)
+		argv_array_push(&submodules_options, "--deleted");
+	if (show_modified)
+		argv_array_push(&submodules_options, "--modified");
+	if (show_others)
+		argv_array_push(&submodules_options, "--others");
+	if (dir->flags & DIR_SHOW_IGNORED)
+		argv_array_push(&submodules_options, "--ignored");
+	if (show_stage)
+		argv_array_push(&submodules_options, "--stage");
+	if (show_killed)
+		argv_array_push(&submodules_options, "--killed");
+	if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
+		argv_array_push(&submodules_options, "--directory");
+	if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
+		argv_array_push(&submodules_options, "--empty-directory");
+	if (show_unmerged)
+		argv_array_push(&submodules_options, "--unmerged");
+	if (show_resolve_undo)
+		argv_array_push(&submodules_options, "--resolve-undo");
+	if (show_eol)
+		argv_array_push(&submodules_options, "--eol");
+	if (debug_mode)
+		argv_array_push(&submodules_options, "--debug");
+}
+
 /**
  * Recursively call ls-files on a submodule
  */
@@ -182,6 +222,9 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 
+	/* add supported options */
+	argv_array_pushv(&cp.args, submodules_options.argv);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -567,11 +610,13 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules)
+		compile_submodule_options(&dir, show_tag);
+
 	if (recurse_submodules &&
-	    (show_stage || show_deleted || show_others || show_unmerged ||
+	    (show_deleted || show_others || show_unmerged ||
 	     show_killed || show_modified || show_resolve_undo ||
-	     show_valid_bit || show_tag || show_eol || with_tree ||
-	     (line_terminator == '\0')))
+	     with_tree))
 		die("ls-files --recurse-submodules unsupported mode");
 
 	if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index b5a53c3..e76fa30 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+	lf_to_nul >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules -z >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ls-files does not output files not added to a repo' '
 	cat >expect <<-\EOF &&
 	.gitmodules
@@ -86,15 +98,10 @@ test_incompatible_with_recurse_submodules () {
 	"
 }
 
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
 test_incompatible_with_recurse_submodules --deleted
 test_incompatible_with_recurse_submodules --modified
 test_incompatible_with_recurse_submodules --others
-test_incompatible_with_recurse_submodules --stage
 test_incompatible_with_recurse_submodules --killed
 test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
 
 test_done
-- 
2.10.0

Re: [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> +static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
> +{
> +	if (line_terminator == '\0')
> +		argv_array_push(&submodules_options, "-z");
> +	if (show_tag)
> +		argv_array_push(&submodules_options, "-t");
> +	if (show_valid_bit)
> +		argv_array_push(&submodules_options, "-v");
> +	if (show_cached)
> +		argv_array_push(&submodules_options, "--cached");
> +	if (show_deleted)
> +		argv_array_push(&submodules_options, "--deleted");
> +	if (show_modified)
> +		argv_array_push(&submodules_options, "--modified");
> +	if (show_others)
> +		argv_array_push(&submodules_options, "--others");
> +	if (dir->flags & DIR_SHOW_IGNORED)
> +		argv_array_push(&submodules_options, "--ignored");
> +	if (show_stage)
> +		argv_array_push(&submodules_options, "--stage");
> +	if (show_killed)
> +		argv_array_push(&submodules_options, "--killed");
> +	if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
> +		argv_array_push(&submodules_options, "--directory");
> +	if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
> +		argv_array_push(&submodules_options, "--empty-directory");
> +	if (show_unmerged)
> +		argv_array_push(&submodules_options, "--unmerged");
> +	if (show_resolve_undo)
> +		argv_array_push(&submodules_options, "--resolve-undo");
> +	if (show_eol)
> +		argv_array_push(&submodules_options, "--eol");
> +	if (debug_mode)
> +		argv_array_push(&submodules_options, "--debug");
> +}

With this and 4/4 applied, the documentation still says "--cached"
is the only supported option.

Does it really make sense to pass all of these?  I understand "-z"
and I suspect things like "-t" and "-v" that affect "how" things are
shown may also happen to work, but I am not sure how much it makes
sense for options that affect "what" things are shown.

What does it even mean to ask for say "--unmerged" to be shown, for
example, from the superproject?  Recurse into submodules whose cache
entries in the index of the superproject are unmerged, or something
else?

I am inclined to say that it is probably better to keep the
"--cached only" as documented, at least on the "what are shown"
side.

Thanks.

Re: [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

On 09/29, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > +static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
> > +{
> > +	if (line_terminator == '\0')
> > +		argv_array_push(&submodules_options, "-z");
> > +	if (show_tag)
> > +		argv_array_push(&submodules_options, "-t");
> > +	if (show_valid_bit)
> > +		argv_array_push(&submodules_options, "-v");
> > +	if (show_cached)
> > +		argv_array_push(&submodules_options, "--cached");
> > +	if (show_deleted)
> > +		argv_array_push(&submodules_options, "--deleted");
> > +	if (show_modified)
> > +		argv_array_push(&submodules_options, "--modified");
> > +	if (show_others)
> > +		argv_array_push(&submodules_options, "--others");
> > +	if (dir->flags & DIR_SHOW_IGNORED)
> > +		argv_array_push(&submodules_options, "--ignored");
> > +	if (show_stage)
> > +		argv_array_push(&submodules_options, "--stage");
> > +	if (show_killed)
> > +		argv_array_push(&submodules_options, "--killed");
> > +	if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
> > +		argv_array_push(&submodules_options, "--directory");
> > +	if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
> > +		argv_array_push(&submodules_options, "--empty-directory");
> > +	if (show_unmerged)
> > +		argv_array_push(&submodules_options, "--unmerged");
> > +	if (show_resolve_undo)
> > +		argv_array_push(&submodules_options, "--resolve-undo");
> > +	if (show_eol)
> > +		argv_array_push(&submodules_options, "--eol");
> > +	if (debug_mode)
> > +		argv_array_push(&submodules_options, "--debug");
> > +}
> 
> With this and 4/4 applied, the documentation still says "--cached"
> is the only supported option.
> 
> Does it really make sense to pass all of these?  I understand "-z"
> and I suspect things like "-t" and "-v" that affect "how" things are
> shown may also happen to work, but I am not sure how much it makes
> sense for options that affect "what" things are shown.
> 
> What does it even mean to ask for say "--unmerged" to be shown, for
> example, from the superproject?  Recurse into submodules whose cache
> entries in the index of the superproject are unmerged, or something
> else?
> 
> I am inclined to say that it is probably better to keep the
> "--cached only" as documented, at least on the "what are shown"
> side.
> 
> Thanks.

You're right that probably makes the most sense for now.

-- 
Brandon Williams

Re: [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

Pass through some known-safe options when recursing into submodules.
(--cached, -v, -t, -z, --debug, --eol)

If other unsafe options are given the caller will be errored out.

Signed-off-by: Brandon Williams <bmwill@google.com>
---

Something more like this correct? I ditched the extra parameters and
reworded the commit msg to reflect this.

 builtin/ls-files.c                     | 30 +++++++++++++++++++++++++++---
 t/t3007-ls-files-recurse-submodules.sh | 16 ++++++++++++----
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 63befed..b6144a5 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
 
 static const char *prefix;
 static const char *super_prefix;
@@ -168,6 +169,25 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+	if (line_terminator == '\0')
+		argv_array_push(&submodules_options, "-z");
+	if (show_tag)
+		argv_array_push(&submodules_options, "-t");
+	if (show_valid_bit)
+		argv_array_push(&submodules_options, "-v");
+	if (show_cached)
+		argv_array_push(&submodules_options, "--cached");
+	if (show_eol)
+		argv_array_push(&submodules_options, "--eol");
+	if (debug_mode)
+		argv_array_push(&submodules_options, "--debug");
+}
+
 /**
  * Recursively call ls-files on a submodule
  */
@@ -182,6 +202,9 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 
+	/* add supported options */
+	argv_array_pushv(&cp.args, submodules_options.argv);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -567,11 +590,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules)
+		compile_submodule_options(&dir, show_tag);
+
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||
-	     show_killed || show_modified || show_resolve_undo ||
-	     show_valid_bit || show_tag || show_eol || with_tree ||
-	     (line_terminator == '\0')))
+	     show_killed || show_modified || show_resolve_undo || with_tree))
 		die("ls-files --recurse-submodules unsupported mode");
 
 	if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index b5a53c3..33a2ea7 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+	lf_to_nul >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules -z >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ls-files does not output files not added to a repo' '
 	cat >expect <<-\EOF &&
 	.gitmodules
@@ -86,15 +98,11 @@ test_incompatible_with_recurse_submodules () {
 	"
 }
 
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
 test_incompatible_with_recurse_submodules --deleted
 test_incompatible_with_recurse_submodules --modified
 test_incompatible_with_recurse_submodules --others
 test_incompatible_with_recurse_submodules --stage
 test_incompatible_with_recurse_submodules --killed
 test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
 
 test_done
-- 
2.10.0

[PATCH v6 4/4] ls-files: add pathspec matching for submodules

[Brandon Williams]

Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule.  The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.

This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally.  If any wildcard character is encountered we'll simply punt
and produce a false positive match.  More accurate matching will be done
once inside the submodule.  This is due to the superproject not knowing
what files could exist in the submodule.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |   3 +-
 builtin/ls-files.c                     |  27 +++++++--
 dir.c                                  |  46 +++++++++++++-
 dir.h                                  |   4 ++
 t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
 5 files changed, 175 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index ea01d45..51ec9a1 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -140,8 +140,7 @@ a space) at the start of each line:
 
 --recurse-submodules::
 	Recursively calls ls-files on each submodule in the repository.
-	Currently there is only support for the --cached mode without a
-	pathspec.
+	Currently there is only support for the --cached.
 
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 6f744ef..82ec811 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -215,6 +215,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
+	int i;
 
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",
@@ -225,6 +226,15 @@ static void show_gitlink(const struct cache_entry *ce)
 	/* add supported options */
 	argv_array_pushv(&cp.args, submodules_options.argv);
 
+	/*
+	 * Pass in the original pathspec args.  The submodule will be
+	 * responsible for prepending the 'submodule_prefix' prior to comparing
+	 * against the pathspec for matches.
+	 */
+	argv_array_push(&cp.args, "--");
+	for (i = 0; i < pathspec.nr; i++)
+		argv_array_push(&cp.args, pathspec.items[i].original);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -243,7 +253,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
 		show_gitlink(ce);
 	} else if (match_pathspec(&pathspec, name.buf, name.len,
 				  len, ps_matched,
@@ -623,16 +634,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		die("ls-files --recurse-submodules does not support "
 		    "--error-unmatch");
 
-	if (recurse_submodules && argc)
-		die("ls-files --recurse-submodules does not support pathspec");
-
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 		       prefix, argv);
 
-	/* Find common prefix for all pathspec's */
-	max_prefix = common_prefix(&pathspec);
+	/*
+	 * Find common prefix for all pathspec's
+	 * This is used as a performance optimization which unfortunately cannot
+	 * be done when recursing into submodules
+	 */
+	if (recurse_submodules)
+		max_prefix = NULL;
+	else
+		max_prefix = common_prefix(&pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
-#define DO_MATCH_EXCLUDE   1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE   (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
 
 /*
  * Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 			 item->nowildcard_len - prefix))
 		return MATCHED_FNMATCH;
 
+	/* Perform checks to see if "name" is a super set of the pathspec */
+	if (flags & DO_MATCH_SUBMODULE) {
+		/* name is a literal prefix of the pathspec */
+		if ((namelen < matchlen) &&
+		    (match[namelen] == '/') &&
+		    !ps_strncmp(item, match, name, namelen))
+			return MATCHED_RECURSIVELY;
+
+		/* name" doesn't match up to the first wild character */
+		if (item->nowildcard_len < item->len &&
+		    ps_strncmp(item, match, name,
+			       item->nowildcard_len - prefix))
+			return 0;
+
+		/*
+		 * Here is where we would perform a wildmatch to check if
+		 * "name" can be matched as a directory (or a prefix) against
+		 * the pathspec.  Since wildmatch doesn't have this capability
+		 * at the present we have to punt and say that it is a match,
+		 * potentially returning a false positive
+		 * The submodules themselves will be able to perform more
+		 * accurate matching to determine if the pathspec matches.
+		 */
+		return MATCHED_RECURSIVELY;
+	}
+
 	return 0;
 }
 
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+			 const char *submodule_name,
+			 char *seen)
+{
+	int matched = do_match_pathspec(ps, submodule_name,
+					strlen(submodule_name),
+					0, seen,
+					DO_MATCH_DIRECTORY |
+					DO_MATCH_SUBMODULE);
+	return matched;
+}
+
 int report_path_error(const char *ps_matched,
 		      const struct pathspec *pathspec,
 		      const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
 		       const char *pattern, const char *string,
 		       int prefix);
 
+extern int submodule_path_match(const struct pathspec *ps,
+				const char *submodule_name,
+				char *seen);
+
 static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index e76fa30..5475855 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	test_cmp expect actual
 '
 
-test_expect_success '--recurse-submodules does not support using path arguments' '
-	test_must_fail git ls-files --recurse-submodules b 2>actual &&
-	test_i18ngrep "does not support pathspec" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+	echo e >submodule/subsub/e.txt &&
+	git -C submodule/subsub add e.txt &&
+	git -C submodule/subsub commit -m "adding e.txt" &&
+	echo f >submodule/f.TXT &&
+	echo g >submodule/g.txt &&
+	git -C submodule add f.TXT g.txt &&
+	git -C submodule commit -m "add f and g" &&
+	echo h >h.txt &&
+	mkdir sib &&
+	echo sib >sib/file &&
+	git add h.txt sib/file &&
+	git commit -m "add h and sib/file" &&
+	git init sub &&
+	echo sub >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git submodule add ./sub &&
+	git commit -m "added sub" &&
+
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	h.txt
+	sib/file
+	sub/file
+	submodule/.gitmodules
+	submodule/c
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/d
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+	cat actual &&
+	git ls-files --recurse-submodules "*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules "*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "sub" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su*/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su?/file" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sib/file
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "s??/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s???file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s*file" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
-- 
2.10.0

Re: [PATCH v6 4/4] ls-files: add pathspec matching for submodules

[Stefan Beller]

On Thu, Sep 29, 2016 at 2:48 PM, Brandon Williams <bmwill@google.com> wrote:
> Pathspecs can be a bit tricky when trying to apply them to submodules.
> The main challenge is that the pathspecs will be with respect to the
> superproject and not with respect to paths in the submodule.  The
> approach this patch takes is to pass in the identical pathspec from the
> superproject to the submodule in addition to the submodule-prefix, which
> is the path from the root of the superproject to the submodule, and then
> we can compare an entry in the submodule prepended with the
> submodule-prefix to the pathspec in order to determine if there is a
> match.
>
> This patch also permits the pathspec logic to perform a prefix match against
> submodules since a pathspec could refer to a file inside of a submodule.
> Due to limitations in the wildmatch logic, a prefix match is only done
> literally.  If any wildcard character is encountered we'll simply punt
> and produce a false positive match.  More accurate matching will be done
> once inside the submodule.  This is due to the superproject not knowing
> what files could exist in the submodule.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  Documentation/git-ls-files.txt         |   3 +-
>  builtin/ls-files.c                     |  27 +++++++--
>  dir.c                                  |  46 +++++++++++++-
>  dir.h                                  |   4 ++
>  t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
>  5 files changed, 175 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index ea01d45..51ec9a1 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -140,8 +140,7 @@ a space) at the start of each line:
>
>  --recurse-submodules::
>         Recursively calls ls-files on each submodule in the repository.
> -       Currently there is only support for the --cached mode without a
> -       pathspec.
> +       Currently there is only support for the --cached.

s/--cached/--cached mode/ ?
The "the" in front of --cached sounds a bit strange for a non native
speaker here.


> +       /*
> +        * Find common prefix for all pathspec's
> +        * This is used as a performance optimization which unfortunately cannot
> +        * be done when recursing into submodules
> +        */
> +       if (recurse_submodules)
> +               max_prefix = NULL;
> +       else
> +               max_prefix = common_prefix(&pathspec);

Nit of the day:
While this is readable, you may want to explore how this reads shorter as

    max_prefix = recurse_submodules ? NULL : common_prefix(&pathspec);

?

> +       git ls-files --recurse-submodules "sub" >actual &&
> +       test_cmp expect actual &&
> +       git ls-files --recurse-submodules "sub/" >actual &&
> +       test_cmp expect actual &&
> +       git ls-files --recurse-submodules "sub/file" >actual &&
> +       test_cmp expect actual &&
> +       git ls-files --recurse-submodules "su*/file" >actual &&
> +       test_cmp expect actual &&
> +       git ls-files --recurse-submodules "su?/file" >actual &&
> +       test_cmp expect actual
> +'

> +
> +       git ls-files --recurse-submodules "s??/file" >actual &&
> +       test_cmp expect actual &&
> +       git ls-files --recurse-submodules "s???file" >actual &&
> +       test_cmp expect actual &&
> +       git ls-files --recurse-submodules "s*file" >actual &&
> +       test_cmp expect actual
>  '

Thanks for the tests!

[PATCH v7 0/4] recursive support for ls-files

[Brandon Williams]

Few minor fixes pointed out Stefan

Brandon Williams (4):
  git: make super-prefix option
  ls-files: optionally recurse into submodules
  ls-files: pass through safe options for --recurse-submodules
  ls-files: add pathspec matching for submodules

 Documentation/git-ls-files.txt         |   7 +-
 Documentation/git.txt                  |   6 +
 builtin/ls-files.c                     | 181 +++++++++++++++++++++-------
 cache.h                                |   2 +
 dir.c                                  |  46 +++++++-
 dir.h                                  |   4 +
 environment.c                          |  13 ++
 git.c                                  |  27 ++++-
 t/t3007-ls-files-recurse-submodules.sh | 210 +++++++++++++++++++++++++++++++++
 9 files changed, 452 insertions(+), 44 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

-- 
2.10.0

[PATCH v7 1/4] git: make super-prefix option

[Brandon Williams]

Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
which can be used to specify a path from above a repository down to its
root.  When such a super-prefix is specified, the paths reported by Git
are prefixed with it to make them relative to that directory "above".
The paths given by the user on the command line
(e.g. "git subcmd --output-file=path/to/a/file" and pathspecs) are taken
relative to the directory "above" to match.

The immediate use of this option is by commands which have a
--recurse-submodule option in order to give context to submodules about
how they were invoked.  This option is currently only allowed for
builtins which support a super-prefix.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git.txt |  6 ++++++
 cache.h               |  2 ++
 environment.c         | 13 +++++++++++++
 git.c                 | 25 +++++++++++++++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7913fc2..2188ae6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,6 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
+    [--super-prefix=<path>]
     <command> [<args>]
 
 DESCRIPTION
@@ -601,6 +602,11 @@ foo.bar= ...`) sets `foo.bar` to the empty string.
 	details.  Equivalent to setting the `GIT_NAMESPACE` environment
 	variable.
 
+--super-prefix=<path>::
+	Currently for internal use only.  Set a prefix which gives a path from
+	above a repository down to its root.  One use is to give submodules
+	context about the superproject that invoked it.
+
 --bare::
 	Treat the repository as a bare repository.  If GIT_DIR
 	environment is not set, it is set to the current working
diff --git a/cache.h b/cache.h
index 3556326..8cf495d 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUPER_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUPER_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
@@ -468,6 +469,7 @@ extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
+extern const char *get_super_prefix(void);
 extern const char *get_git_work_tree(void);
 
 /*
diff --git a/environment.c b/environment.c
index ca72464..d12d7db 100644
--- a/environment.c
+++ b/environment.c
@@ -100,6 +100,8 @@ static char *work_tree;
 static const char *namespace;
 static size_t namespace_len;
 
+static const char *super_prefix;
+
 static const char *git_dir, *git_common_dir;
 static char *git_object_dir, *git_index_file, *git_graft_file;
 int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
@@ -120,6 +122,7 @@ const char * const local_repo_env[] = {
 	NO_REPLACE_OBJECTS_ENVIRONMENT,
 	GIT_REPLACE_REF_BASE_ENVIRONMENT,
 	GIT_PREFIX_ENVIRONMENT,
+	GIT_SUPER_PREFIX_ENVIRONMENT,
 	GIT_SHALLOW_FILE_ENVIRONMENT,
 	GIT_COMMON_DIR_ENVIRONMENT,
 	NULL
@@ -222,6 +225,16 @@ const char *strip_namespace(const char *namespaced_ref)
 	return namespaced_ref + namespace_len;
 }
 
+const char *get_super_prefix(void)
+{
+	static int initialized;
+	if (!initialized) {
+		super_prefix = getenv(GIT_SUPER_PREFIX_ENVIRONMENT);
+		initialized = 1;
+	}
+	return super_prefix;
+}
+
 static int git_work_tree_initialized;
 
 /*
diff --git a/git.c b/git.c
index 1c61151..469a83f 100644
--- a/git.c
+++ b/git.c
@@ -164,6 +164,20 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_WORK_TREE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--super-prefix")) {
+			if (*argc < 2) {
+				fprintf(stderr, "No prefix given for --super-prefix.\n" );
+				usage(git_usage_string);
+			}
+			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, (*argv)[1], 1);
+			if (envchanged)
+				*envchanged = 1;
+			(*argv)++;
+			(*argc)--;
+		} else if (skip_prefix(cmd, "--super-prefix=", &cmd)) {
+			setenv(GIT_SUPER_PREFIX_ENVIRONMENT, cmd, 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else if (!strcmp(cmd, "--bare")) {
 			char *cwd = xgetcwd();
 			is_bare_repository_cfg = 1;
@@ -310,6 +324,7 @@ static int handle_alias(int *argcp, const char ***argv)
  * RUN_SETUP for reading from the configuration file.
  */
 #define NEED_WORK_TREE		(1<<3)
+#define SUPPORT_SUPER_PREFIX	(1<<4)
 
 struct cmd_struct {
 	const char *cmd;
@@ -344,6 +359,13 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	}
 	commit_pager_choice();
 
+	if (!help && get_super_prefix()) {
+		if (!(p->option & SUPPORT_SUPER_PREFIX))
+			die("%s doesn't support --super-prefix", p->cmd);
+		if (prefix)
+			die("can't use --super-prefix from a subdirectory");
+	}
+
 	if (!help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
@@ -558,6 +580,9 @@ static void execv_dashed_external(const char **argv)
 	const char *tmp;
 	int status;
 
+	if (get_super_prefix())
+		die("%s doesn't support --super-prefix", argv[0]);
+
 	if (use_pager == -1)
 		use_pager = check_pager_config(argv[0]);
 	commit_pager_choice();
-- 
2.10.0

[PATCH v7 2/4] ls-files: optionally recurse into submodules

[Brandon Williams]

Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules.  This is done by forking off a
process to recursively call ls-files on all submodules. Use top-level
--super-prefix option to pass a path to the submodule which it can
use to prepend to output or pathspec matching logic.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |   8 +-
 builtin/ls-files.c                     | 138 ++++++++++++++++++++++++---------
 git.c                                  |   2 +-
 t/t3007-ls-files-recurse-submodules.sh | 100 ++++++++++++++++++++++++
 4 files changed, 208 insertions(+), 40 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..ea01d45 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
 		[--exclude-per-directory=<file>]
 		[--exclude-standard]
 		[--error-unmatch] [--with-tree=<tree-ish>]
-		[--full-name] [--abbrev] [--] [<file>...]
+		[--full-name] [--recurse-submodules]
+		[--abbrev] [--] [<file>...]
 
 DESCRIPTION
 -----------
@@ -137,6 +138,11 @@ a space) at the start of each line:
 	option forces paths to be output relative to the project
 	top directory.
 
+--recurse-submodules::
+	Recursively calls ls-files on each submodule in the repository.
+	Currently there is only support for the --cached mode without a
+	pathspec.
+
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
 	lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..63befed 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,8 +29,10 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
 
 static const char *prefix;
+static const char *super_prefix;
 static int max_prefix_len;
 static int prefix_len;
 static struct pathspec pathspec;
@@ -68,11 +71,24 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)
 static void write_name(const char *name)
 {
 	/*
+	 * Prepend the super_prefix to name to construct the full_name to be
+	 * written.
+	 */
+	struct strbuf full_name = STRBUF_INIT;
+	if (super_prefix) {
+		strbuf_addstr(&full_name, super_prefix);
+		strbuf_addstr(&full_name, name);
+		name = full_name.buf;
+	}
+
+	/*
 	 * With "--full-name", prefix_len=0; this caller needs to pass
 	 * an empty string in that case (a NULL is good for "").
 	 */
 	write_name_quoted_relative(name, prefix_len ? prefix : NULL,
 				   stdout, line_terminator);
+
+	strbuf_release(&full_name);
 }
 
 static void show_dir_entry(const char *tag, struct dir_entry *ent)
@@ -152,55 +168,84 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
+			 super_prefix ? super_prefix : "",
+			 ce->name);
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "--recurse-submodules");
+
+	cp.git_cmd = 1;
+	cp.dir = ce->name;
+	status = run_command(&cp);
+	if (status)
+		exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
+	struct strbuf name = STRBUF_INIT;
 	int len = max_prefix_len;
+	if (super_prefix)
+		strbuf_addstr(&name, super_prefix);
+	strbuf_addstr(&name, ce->name);
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
-			    len, ps_matched,
-			    S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
-		return;
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+		show_gitlink(ce);
+	} else if (match_pathspec(&pathspec, name.buf, name.len,
+				  len, ps_matched,
+				  S_ISDIR(ce->ce_mode) ||
+				  S_ISGITLINK(ce->ce_mode))) {
+		if (tag && *tag && show_valid_bit &&
+		    (ce->ce_flags & CE_VALID)) {
+			static char alttag[4];
+			memcpy(alttag, tag, 3);
+			if (isalpha(tag[0]))
+				alttag[0] = tolower(tag[0]);
+			else if (tag[0] == '?')
+				alttag[0] = '!';
+			else {
+				alttag[0] = 'v';
+				alttag[1] = tag[0];
+				alttag[2] = ' ';
+				alttag[3] = 0;
+			}
+			tag = alttag;
+		}
 
-	if (tag && *tag && show_valid_bit &&
-	    (ce->ce_flags & CE_VALID)) {
-		static char alttag[4];
-		memcpy(alttag, tag, 3);
-		if (isalpha(tag[0]))
-			alttag[0] = tolower(tag[0]);
-		else if (tag[0] == '?')
-			alttag[0] = '!';
-		else {
-			alttag[0] = 'v';
-			alttag[1] = tag[0];
-			alttag[2] = ' ';
-			alttag[3] = 0;
+		if (!show_stage) {
+			fputs(tag, stdout);
+		} else {
+			printf("%s%06o %s %d\t",
+			       tag,
+			       ce->ce_mode,
+			       find_unique_abbrev(ce->sha1,abbrev),
+			       ce_stage(ce));
+		}
+		write_eolinfo(ce, ce->name);
+		write_name(ce->name);
+		if (debug_mode) {
+			const struct stat_data *sd = &ce->ce_stat_data;
+
+			printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
+			printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
+			printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
+			printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
+			printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
 		}
-		tag = alttag;
 	}
 
-	if (!show_stage) {
-		fputs(tag, stdout);
-	} else {
-		printf("%s%06o %s %d\t",
-		       tag,
-		       ce->ce_mode,
-		       find_unique_abbrev(ce->sha1,abbrev),
-		       ce_stage(ce));
-	}
-	write_eolinfo(ce, ce->name);
-	write_name(ce->name);
-	if (debug_mode) {
-		const struct stat_data *sd = &ce->ce_stat_data;
-
-		printf("  ctime: %d:%d\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
-		printf("  mtime: %d:%d\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
-		printf("  dev: %d\tino: %d\n", sd->sd_dev, sd->sd_ino);
-		printf("  uid: %d\tgid: %d\n", sd->sd_uid, sd->sd_gid);
-		printf("  size: %d\tflags: %x\n", sd->sd_size, ce->ce_flags);
-	}
+	strbuf_release(&name);
 }
 
 static void show_ru_info(void)
@@ -468,6 +513,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			N_("make the output relative to the project top directory"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+		OPT_BOOL(0, "recurse-submodules", &recurse_submodules,
+			N_("recurse through submodules")),
 		OPT_BOOL(0, "error-unmatch", &error_unmatch,
 			N_("if any <file> is not in the index, treat this as an error")),
 		OPT_STRING(0, "with-tree", &with_tree, N_("tree-ish"),
@@ -484,6 +531,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
+	super_prefix = get_super_prefix();
 	git_config(git_default_config, NULL);
 
 	if (read_cache() < 0)
@@ -519,6 +567,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules &&
+	    (show_stage || show_deleted || show_others || show_unmerged ||
+	     show_killed || show_modified || show_resolve_undo ||
+	     show_valid_bit || show_tag || show_eol || with_tree ||
+	     (line_terminator == '\0')))
+		die("ls-files --recurse-submodules unsupported mode");
+
+	if (recurse_submodules && error_unmatch)
+		die("ls-files --recurse-submodules does not support "
+		    "--error-unmatch");
+
+	if (recurse_submodules && argc)
+		die("ls-files --recurse-submodules does not support pathspec");
+
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
diff --git a/git.c b/git.c
index 469a83f..df73768 100644
--- a/git.c
+++ b/git.c
@@ -443,7 +443,7 @@ static struct cmd_struct commands[] = {
 	{ "init-db", cmd_init_db },
 	{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
 	{ "log", cmd_log, RUN_SETUP },
-	{ "ls-files", cmd_ls_files, RUN_SETUP },
+	{ "ls-files", cmd_ls_files, RUN_SETUP | SUPPORT_SUPER_PREFIX },
 	{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo },
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
new file mode 100755
index 0000000..b5a53c3
--- /dev/null
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files from
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodules' '
+	echo a >a &&
+	mkdir b &&
+	echo b >b/b &&
+	git add a b &&
+	git commit -m "add a and b" &&
+	git init submodule &&
+	echo c >submodule/c &&
+	git -C submodule add c &&
+	git -C submodule commit -m "add c" &&
+	git submodule add ./submodule &&
+	git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly outputs files in submodule' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files does not output files not added to a repo' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	echo a >not_added &&
+	echo b >b/not_added &&
+	echo c >submodule/not_added &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files recurses more than 1 level' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/.gitmodules
+	submodule/c
+	submodule/subsub/d
+	EOF
+
+	git init submodule/subsub &&
+	echo d >submodule/subsub/d &&
+	git -C submodule/subsub add d &&
+	git -C submodule/subsub commit -m "add d" &&
+	git -C submodule submodule add ./subsub &&
+	git -C submodule commit -m "added subsub" &&
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules does not support using path arguments' '
+	test_must_fail git ls-files --recurse-submodules b 2>actual &&
+	test_i18ngrep "does not support pathspec" actual
+'
+
+test_expect_success '--recurse-submodules does not support --error-unmatch' '
+	test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
+	test_i18ngrep "does not support --error-unmatch" actual
+'
+
+test_incompatible_with_recurse_submodules () {
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep 'unsupported mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules -z
+test_incompatible_with_recurse_submodules -v
+test_incompatible_with_recurse_submodules -t
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --modified
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --stage
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --eol
+
+test_done
-- 
2.10.0

[PATCH v7 3/4] ls-files: pass through safe options for --recurse-submodules

[Brandon Williams]

Pass through some known-safe options when recursing into submodules.
(--cached, -v, -t, -z, --debug, --eol)

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/ls-files.c                     | 30 +++++++++++++++++++++++++++---
 t/t3007-ls-files-recurse-submodules.sh | 16 ++++++++++++----
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 63befed..b6144a5 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
 static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
 
 static const char *prefix;
 static const char *super_prefix;
@@ -168,6 +169,25 @@ static void show_killed_files(struct dir_struct *dir)
 	}
 }
 
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+	if (line_terminator == '\0')
+		argv_array_push(&submodules_options, "-z");
+	if (show_tag)
+		argv_array_push(&submodules_options, "-t");
+	if (show_valid_bit)
+		argv_array_push(&submodules_options, "-v");
+	if (show_cached)
+		argv_array_push(&submodules_options, "--cached");
+	if (show_eol)
+		argv_array_push(&submodules_options, "--eol");
+	if (debug_mode)
+		argv_array_push(&submodules_options, "--debug");
+}
+
 /**
  * Recursively call ls-files on a submodule
  */
@@ -182,6 +202,9 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_push(&cp.args, "ls-files");
 	argv_array_push(&cp.args, "--recurse-submodules");
 
+	/* add supported options */
+	argv_array_pushv(&cp.args, submodules_options.argv);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -567,11 +590,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
+	if (recurse_submodules)
+		compile_submodule_options(&dir, show_tag);
+
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||
-	     show_killed || show_modified || show_resolve_undo ||
-	     show_valid_bit || show_tag || show_eol || with_tree ||
-	     (line_terminator == '\0')))
+	     show_killed || show_modified || show_resolve_undo || with_tree))
 		die("ls-files --recurse-submodules unsupported mode");
 
 	if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index b5a53c3..33a2ea7 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+	lf_to_nul >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	submodule/c
+	EOF
+
+	git ls-files --recurse-submodules -z >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'ls-files does not output files not added to a repo' '
 	cat >expect <<-\EOF &&
 	.gitmodules
@@ -86,15 +98,11 @@ test_incompatible_with_recurse_submodules () {
 	"
 }
 
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
 test_incompatible_with_recurse_submodules --deleted
 test_incompatible_with_recurse_submodules --modified
 test_incompatible_with_recurse_submodules --others
 test_incompatible_with_recurse_submodules --stage
 test_incompatible_with_recurse_submodules --killed
 test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
 
 test_done
-- 
2.10.0

[PATCH v7 4/4] ls-files: add pathspec matching for submodules

[Brandon Williams]

Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule.  The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.

This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally.  If any wildcard character is encountered we'll simply punt
and produce a false positive match.  More accurate matching will be done
once inside the submodule.  This is due to the superproject not knowing
what files could exist in the submodule.

Signed-off-by: Brandon Williams <bmwill@google.com>
---
 Documentation/git-ls-files.txt         |   3 +-
 builtin/ls-files.c                     |  27 +++++++--
 dir.c                                  |  46 +++++++++++++-
 dir.h                                  |   4 ++
 t/t3007-ls-files-recurse-submodules.sh | 108 ++++++++++++++++++++++++++++++++-
 5 files changed, 175 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index ea01d45..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -140,8 +140,7 @@ a space) at the start of each line:
 
 --recurse-submodules::
 	Recursively calls ls-files on each submodule in the repository.
-	Currently there is only support for the --cached mode without a
-	pathspec.
+	Currently there is only support for the --cached mode.
 
 --abbrev[=<n>]::
 	Instead of showing the full 40-byte hexadecimal object
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b6144a5..0f25914 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -195,6 +195,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
+	int i;
 
 	argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
 			 super_prefix ? super_prefix : "",
@@ -205,6 +206,15 @@ static void show_gitlink(const struct cache_entry *ce)
 	/* add supported options */
 	argv_array_pushv(&cp.args, submodules_options.argv);
 
+	/*
+	 * Pass in the original pathspec args.  The submodule will be
+	 * responsible for prepending the 'submodule_prefix' prior to comparing
+	 * against the pathspec for matches.
+	 */
+	argv_array_push(&cp.args, "--");
+	for (i = 0; i < pathspec.nr; i++)
+		argv_array_push(&cp.args, pathspec.items[i].original);
+
 	cp.git_cmd = 1;
 	cp.dir = ce->name;
 	status = run_command(&cp);
@@ -223,7 +233,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
-	if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
 		show_gitlink(ce);
 	} else if (match_pathspec(&pathspec, name.buf, name.len,
 				  len, ps_matched,
@@ -602,16 +613,20 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		die("ls-files --recurse-submodules does not support "
 		    "--error-unmatch");
 
-	if (recurse_submodules && argc)
-		die("ls-files --recurse-submodules does not support pathspec");
-
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD |
 		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
 		       prefix, argv);
 
-	/* Find common prefix for all pathspec's */
-	max_prefix = common_prefix(&pathspec);
+	/*
+	 * Find common prefix for all pathspec's
+	 * This is used as a performance optimization which unfortunately cannot
+	 * be done when recursing into submodules
+	 */
+	if (recurse_submodules)
+		max_prefix = NULL;
+	else
+		max_prefix = common_prefix(&pathspec);
 	max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
 
 	/* Treat unmatching pathspec elements as errors */
diff --git a/dir.c b/dir.c
index 0ea235f..28e9736 100644
--- a/dir.c
+++ b/dir.c
@@ -207,8 +207,9 @@ int within_depth(const char *name, int namelen,
 	return 1;
 }
 
-#define DO_MATCH_EXCLUDE   1
-#define DO_MATCH_DIRECTORY 2
+#define DO_MATCH_EXCLUDE   (1<<0)
+#define DO_MATCH_DIRECTORY (1<<1)
+#define DO_MATCH_SUBMODULE (1<<2)
 
 /*
  * Does 'match' match the given name?
@@ -283,6 +284,32 @@ static int match_pathspec_item(const struct pathspec_item *item, int prefix,
 			 item->nowildcard_len - prefix))
 		return MATCHED_FNMATCH;
 
+	/* Perform checks to see if "name" is a super set of the pathspec */
+	if (flags & DO_MATCH_SUBMODULE) {
+		/* name is a literal prefix of the pathspec */
+		if ((namelen < matchlen) &&
+		    (match[namelen] == '/') &&
+		    !ps_strncmp(item, match, name, namelen))
+			return MATCHED_RECURSIVELY;
+
+		/* name" doesn't match up to the first wild character */
+		if (item->nowildcard_len < item->len &&
+		    ps_strncmp(item, match, name,
+			       item->nowildcard_len - prefix))
+			return 0;
+
+		/*
+		 * Here is where we would perform a wildmatch to check if
+		 * "name" can be matched as a directory (or a prefix) against
+		 * the pathspec.  Since wildmatch doesn't have this capability
+		 * at the present we have to punt and say that it is a match,
+		 * potentially returning a false positive
+		 * The submodules themselves will be able to perform more
+		 * accurate matching to determine if the pathspec matches.
+		 */
+		return MATCHED_RECURSIVELY;
+	}
+
 	return 0;
 }
 
@@ -386,6 +413,21 @@ int match_pathspec(const struct pathspec *ps,
 	return negative ? 0 : positive;
 }
 
+/**
+ * Check if a submodule is a superset of the pathspec
+ */
+int submodule_path_match(const struct pathspec *ps,
+			 const char *submodule_name,
+			 char *seen)
+{
+	int matched = do_match_pathspec(ps, submodule_name,
+					strlen(submodule_name),
+					0, seen,
+					DO_MATCH_DIRECTORY |
+					DO_MATCH_SUBMODULE);
+	return matched;
+}
+
 int report_path_error(const char *ps_matched,
 		      const struct pathspec *pathspec,
 		      const char *prefix)
diff --git a/dir.h b/dir.h
index da1a858..97c83bb 100644
--- a/dir.h
+++ b/dir.h
@@ -304,6 +304,10 @@ extern int git_fnmatch(const struct pathspec_item *item,
 		       const char *pattern, const char *string,
 		       int prefix);
 
+extern int submodule_path_match(const struct pathspec *ps,
+				const char *submodule_name,
+				char *seen);
+
 static inline int ce_path_match(const struct cache_entry *ce,
 				const struct pathspec *pathspec,
 				char *seen)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 33a2ea7..a542617 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -81,9 +81,111 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	test_cmp expect actual
 '
 
-test_expect_success '--recurse-submodules does not support using path arguments' '
-	test_must_fail git ls-files --recurse-submodules b 2>actual &&
-	test_i18ngrep "does not support pathspec" actual
+test_expect_success '--recurse-submodules and pathspecs setup' '
+	echo e >submodule/subsub/e.txt &&
+	git -C submodule/subsub add e.txt &&
+	git -C submodule/subsub commit -m "adding e.txt" &&
+	echo f >submodule/f.TXT &&
+	echo g >submodule/g.txt &&
+	git -C submodule add f.TXT g.txt &&
+	git -C submodule commit -m "add f and g" &&
+	echo h >h.txt &&
+	mkdir sib &&
+	echo sib >sib/file &&
+	git add h.txt sib/file &&
+	git commit -m "add h and sib/file" &&
+	git init sub &&
+	echo sub >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git submodule add ./sub &&
+	git commit -m "added sub" &&
+
+	cat >expect <<-\EOF &&
+	.gitmodules
+	a
+	b/b
+	h.txt
+	sib/file
+	sub/file
+	submodule/.gitmodules
+	submodule/c
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/d
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+	cat actual &&
+	git ls-files --recurse-submodules "*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules "*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	submodule/subsub/e.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	h.txt
+	submodule/f.TXT
+	submodule/g.txt
+	EOF
+
+	git ls-files --recurse-submodules ":(icase)*.txt" ":(exclude)submodule/subsub/*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "sub" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "sub/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su*/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "su?/file" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--recurse-submodules and pathspecs' '
+	cat >expect <<-\EOF &&
+	sib/file
+	sub/file
+	EOF
+
+	git ls-files --recurse-submodules "s??/file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s???file" >actual &&
+	test_cmp expect actual &&
+	git ls-files --recurse-submodules "s*file" >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '--recurse-submodules does not support --error-unmatch' '
-- 
2.10.0

Re: [PATCH v7 0/4] recursive support for ls-files

[Stefan Beller]

On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
> Few minor fixes pointed out Stefan

Nit: This is not very informative, so you could provide an "interdiff"
to the last patch, (see https://github.com/trast/tbdiff for a
sophisticated tool,
I usually do a git diff origin/sb/<what-junio-queued>)
That helps reviewers as they do not need to review it all again, but
they may remember what they annotated and see how you addressed it.

Thanks,
Stefan

Re: [PATCH v7 0/4] recursive support for ls-files

[Stefan Beller]

On Fri, Oct 7, 2016 at 11:34 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
>> Few minor fixes pointed out Stefan
>

The series itself looks good though. :)

Thanks,
Stefan

Re: [PATCH v7 0/4] recursive support for ls-files

[Brandon Williams]

On 10/07, Stefan Beller wrote:
> On Fri, Oct 7, 2016 at 11:34 AM, Stefan Beller <sbeller@google.com> wrote:
> > On Fri, Oct 7, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
> >> Few minor fixes pointed out Stefan
> >
> 
> The series itself looks good though. :)
> 
> Thanks,
> Stefan

Thanks! And I'll keep that in mind for the future.  Still lots to learn
about contributing to the project.

-- 
Brandon Williams