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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 27, 2016, 18:40 UTC
Message-ID: <xmqqh991nq34.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1474930003-83750-4-git-send-email-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 18 quoted lines

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

Show 24 quoted lines

> @@ -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.

Show 8 quoted lines

>  	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.

Show 7 quoted lines

> +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
> +'