Brandon Williams <bmw...@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 <bmw...@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
> +'

Reply via email to