Subject: Re: [PATCH v5 05/17] sparse-checkout: add '--stdin' option to set subcommand

From: SZEDER Gábor <[email-masked]>
Date: Thu, 21 Nov 2019 12:21:11 +0100

On Mon, Oct 21, 2019 at 01:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email-masked]>
> 
> The 'git sparse-checkout set' subcommand takes a list of patterns
> and places them in the sparse-checkout file. Then, it updates the
> working directory to match those patterns. For a large list of
> patterns, the command-line call can get very cumbersome.
> 
> Add a '--stdin' option to instead read patterns over standard in.
> 
> Signed-off-by: Derrick Stolee <[email-masked]>
> ---
>  builtin/sparse-checkout.c          | 35 ++++++++++++++++++++++++++++--
>  t/t1091-sparse-checkout-builtin.sh | 20 +++++++++++++++++

No documentation update.

>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 834ee421f0..f2e2bd772d 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -154,6 +154,15 @@ static int write_patterns_and_update(struct pattern_list *pl)
>  	return update_working_directory();
>  }
>  
> +static char const * const builtin_sparse_checkout_set_usage[] = {
> +	N_("git sparse-checkout set [--stdin|<patterns>]"),

In the usage string [...] denotes optional command line parameters.
However, they are not optional, but either '--stdin' or at least one
pattern is required:

  $ git sparse-checkout set
  error: Sparse checkout leaves no entry on working directory
  error: Sparse checkout leaves no entry on working directory
  $ echo $?
  255

So this should be (--stdin | <patterns>).

> +	NULL
> +};
> +
> +static struct sparse_checkout_set_opts {
> +	int use_stdin;
> +} set_opts;
> +
>  static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>  {
>  	static const char *empty_base = "";
> @@ -161,10 +170,32 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>  	struct pattern_list pl;
>  	int result;
>  	int set_config = 0;
> +
> +	static struct option builtin_sparse_checkout_set_options[] = {
> +		OPT_BOOL(0, "stdin", &set_opts.use_stdin,
> +			 N_("read patterns from standard in")),
> +		OPT_END(),
> +	};
> +
>  	memset(&pl, 0, sizeof(pl));
>  
> -	for (i = 1; i < argc; i++)
> -		add_pattern(argv[i], empty_base, 0, &pl, 0);
> +	argc = parse_options(argc, argv, prefix,
> +			     builtin_sparse_checkout_set_options,
> +			     builtin_sparse_checkout_set_usage,
> +			     PARSE_OPT_KEEP_UNKNOWN);
> +
> +	if (set_opts.use_stdin) {
> +		struct strbuf line = STRBUF_INIT;
> +
> +		while (!strbuf_getline(&line, stdin)) {

This reads patterns separated by a newline character.

What if someone is doomed with pathnames containing newline
characters, should we provide a '-z' option for \0-separated patterns?

  $ touch foo bar $'foo\nbar'
  $ git add .
  $ git commit -m 'filename with newline'
  [master (root-commit) 5cd7369] filename with newline
   3 files changed, 0 insertions(+), 0 deletions(-)
   create mode 100644 bar
   create mode 100644 foo
   create mode 100644 "foo\nbar"
  $ git sparse-checkout set foo
  $ ls
  foo
  $ git sparse-checkout set 'foo*'
  $ ls
  foo  foo?bar
  $ git sparse-checkout set $'foo\nbar'
  $ ls
  foo?bar
  # Looks good so far, but...
  $ cat .git/info/sparse-checkout 
  foo
  bar
  $ git read-tree -um HEAD
  $ ls
  bar  foo
  # Not so good after all.
  # Let's try to hand-edit the sparse-checkout file.
  $ echo $'"foo\\nbar"' >.git/info/sparse-checkout 
  $ git read-tree -um HEAD
  error: Sparse checkout leaves no entry on working directory
  $ echo $'foo\\nbar'
  >.git/info/sparse-checkout 
  $ git read-tree -um HEAD
  error: Sparse checkout leaves no entry on working directory
  $ echo $'foo\\\nbar'
  >.git/info/sparse-checkout 
  $ git read-tree -um HEAD
  $ ls
  bar
  # OK, I give up :)

So, it seems that the sparse-checkout file format doesn't support
paths/patterns containing a newline character (or if it does, I
couldn't figure out how), and thus there is no use for a '-z' option.

However, as shown above a newline in a pattern given as parameter
eventually leads to undesired behavior, so I think 'git
sparse-checkout set $'foo\nbar' should error out upfront.

> +			size_t len;
> +			char *buf = strbuf_detach(&line, &len);

Nit: this 'len' variable is unnecessary, as it's only used as an out
variable of this strbuf_detach() call, but strbuf_detach() accepts a
NULL as its second parameter.

> +			add_pattern(buf, empty_base, 0, &pl, 0);
> +		}
> +	} else {
> +		for (i = 0; i < argc; i++)
> +			add_pattern(argv[i], empty_base, 0, &pl, 0);
> +	}

According to the usage string this subcommand needs either '--stdin'
or a set of patterns, but not both, which is in line with my
interpretation of the commit message.  I think this makes sense, and
it's consistent with the '--stdin' option of other git commands.
However, the above option parsing and if-else conditions allow both
'--stdin' and patterns, silently ignoring the patterns, without
erroring out:

  $ echo README | git sparse-checkout set --stdin Makefile
  $ echo $?
  0
  $ find . -name README |wc -l
  51
  $ find . -name Makefile |wc -l
  0

>  	if (!core_apply_sparse_checkout) {
>  		sc_set_config(MODE_ALL_PATTERNS);
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index bf2dc55bb1..a9ff5eb9ec 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -128,4 +128,24 @@ test_expect_success 'set sparse-checkout using builtin' '
>  	test_cmp expect dir
>  '
>  
> +test_expect_success 'set sparse-checkout using --stdin' '
> +	cat >expect <<-EOF &&
> +		/*
> +		!/*/
> +		/folder1/
> +		/folder2/
> +	EOF
> +	git -C repo sparse-checkout set --stdin <expect &&
> +	git -C repo sparse-checkout list >actual &&
> +	test_cmp expect actual &&
> +	test_cmp expect repo/.git/info/sparse-checkout &&
> +	ls repo >dir  &&
> +	cat >expect <<-EOF &&
> +		a
> +		folder1
> +		folder2
> +	EOF
> +	test_cmp expect dir
> +'
> +
>  test_done
> -- 
> gitgitgadget
>