| Message ID | 0e08898dcb42bd38ca3692b49a7e9f5763150c80.1571666186.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New sparse-checkout builtin and "cone" mode | expand |
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. > + 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 >
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
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>]"), + 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)) { + size_t len; + char *buf = strbuf_detach(&line, &len); + add_pattern(buf, empty_base, 0, &pl, 0); + } + } else { + for (i = 0; i < argc; i++) + add_pattern(argv[i], empty_base, 0, &pl, 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