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

[Brandon Williams]

On 09/27, Junio C Hamano wrote:
> Brandon Williams  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

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

[Junio C Hamano]

Brandon Williams  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 
> ---
>  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=]
>   [--exclude-standard]
>   [--error-unmatch] [--with-tree=]
> - [--full-name] [--abbrev] [--] [...]
> + [--full-name] [--recurse-submodules]
> + [--abbrev] [--] [...]
>  
>  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[=]::
>   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 ;-)