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

[Stefan Beller <sbeller@google.com>]

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!