Re: [PATCH v5 05/17] sparse-checkout: add '--stdin' option to set subcommand
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Nov 21, 2019, 11:21 UTC
- Message-ID
- <20191121112111.GQ23183@szeder.dev>
- In-Reply-To
- <0e08898dcb42bd38ca3692b49a7e9f5763150c80.1571666186.git.gitgitgadget@gmail.com>
On Mon, Oct 21, 2019 at 01:56:14PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 13 quoted lines
> 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.
Show 12 quoted lines
> 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>).
Show 34 quoted lines
> + 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.
Show 6 quoted lines
> + 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
Show 34 quoted lines
> 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
>