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

[Derrick Stolee <stolee@gmail.com>]

On 11/21/2019 6:21 AM, SZEDER Gábor wrote:
> On Mon, Oct 21, 2019 at 01:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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 <dstolee@microsoft.com>
>> ---
>>  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.

I will add this to the list of things to do as a follow-up. There are
lots of ways users can abuse the command-line right now, especially in
cone mode.

The goal right now is to get the typical usage figured out, then we can
look for atypical cases (such as newlines in filenames, which...who even
does that?).

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

I'll add this to the list, too.

Thanks,
-Stolee