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

[Junio C Hamano <gitster@pobox.com>]

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 ;-)