On 8/23/2019 7:30 PM, Elijah Newren wrote:
> On Tue, Aug 20, 2019 at 8:12 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
>>
>> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>>
>> The 'git sparse-checkout add' subcommand takes a list of patterns
>> over stdin and writes them to the sparse-checkout file. Then, it
>> updates the working directory using 'git read-tree -mu HEAD'.
>
> As mentioned in response to the cover letter, I'd rather see it take
> patterns as positional arguments (though requiring a '--' argument
> before any patterns that start with a hyphen). It could also take
> --stdin to read from stdin.
>
>> Note: if a user adds a negative pattern that would lead to the
>> removal of a non-empty directory, then Git may not delete that
>> directory (on Windows).
>
> This sounds like you're re-iterating a bug mentioned earlier, but if
> someone in the future comes and reads this comment it might sound like
> you're saying git can avoid clearing a directory for optimization or
> other reasons. (And, of course, it'd be nice to figure out why this
> bug exists.)
>
> Another question this brings up, though, is that you worked around
> this bug in 'init' so why would you not also do so for 'add'? Seems
> inconsistent to me.
>
>> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>> ---
>> Documentation/git-sparse-checkout.txt | 4 ++++
>> builtin/sparse-checkout.c | 32 ++++++++++++++++++++++++++-
>> t/t1091-sparse-checkout-builtin.sh | 20 +++++++++++++++++
>> 3 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
>> index 50c53ee60a..6f540a3443 100644
>> --- a/Documentation/git-sparse-checkout.txt
>> +++ b/Documentation/git-sparse-checkout.txt
>> @@ -34,6 +34,10 @@ COMMANDS
>> by Git. Add patterns to the sparse-checkout file to
>> repopulate the working directory.
>>
>> +'add'::
>> + Add a set of patterns to the sparse-checkout file, as given over
>> + stdin. Updates the working directory to match the new patterns.
>> +
>> SPARSE CHECKOUT
>> ----------------
>>
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 86d24e6295..ec6134fecc 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -8,7 +8,7 @@
>> #include "strbuf.h"
>>
>> static char const * const builtin_sparse_checkout_usage[] = {
>> - N_("git sparse-checkout [init|list]"),
>> + N_("git sparse-checkout [init|add|list]"),
>> NULL
>> };
>>
>> @@ -166,6 +166,34 @@ static int sparse_checkout_init(int argc, const char **argv)
>> return sc_read_tree();
>> }
>>
>> +static int sparse_checkout_add(int argc, const char **argv)
>> +{
>> + struct exclude_list el;
>> + char *sparse_filename;
>> + FILE *fp;
>> + struct strbuf line = STRBUF_INIT;
>> +
>> + memset(&el, 0, sizeof(el));
>> +
>> + sparse_filename = get_sparse_checkout_filename();
>> + add_excludes_from_file_to_list(sparse_filename, "", 0, &el, NULL);
>
> el is an exclude_list and we call add_excludes_..., but it's actually
> an *include* list. This is going to cause errors at some point, and
> will cause lots of headaches.
>
>> +
>> + fp = fopen(sparse_filename, "w");
>> + write_excludes_to_file(fp, &el);
>> +
>> + while (!strbuf_getline(&line, stdin)) {
>> + strbuf_trim(&line);
>> + fprintf(fp, "%s\n", line.buf);
>> + }
>
> Should we first check whether these excludes are already in the
> sparse-checkout file?
>
>> + fclose(fp);
>> + free(sparse_filename);
>> +
>> + clear_exclude_list(&el);
>> +
>> + return sc_read_tree();
>
> What if someone calls 'git sparse-checkout add' without first calling
> 'git sparse-checkout init'? As far as I can tell, core.sparseCheckout
> will be unset (i.e. treated as false), meaning that this operation
> will do some work, but result in no changes and a report of success.
> After users try to figure out why it won't work, they eventually run
> 'git sparse-checkout init', which will delete all the entries they
> previously added with the 'add' subcommand.
>
> What should happen instead?
If someone runs 'git sparse-checkout init' after an 'add', the
sparse-checkout file has contents, so the init does not overwrite
those.
(In the update I'm working on, 'init' doesn't delete folders, so
it will only remove the files that do not match the patterns.)
>> +}
>> +
>> int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
>> {
>> static struct option builtin_sparse_checkout_options[] = {
>> @@ -187,6 +215,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
>> return sparse_checkout_list(argc, argv);
>> if (!strcmp(argv[0], "init"))
>> return sparse_checkout_init(argc, argv);
>> + if (!strcmp(argv[0], "add"))
>> + return sparse_checkout_add(argc, argv);
>> }
>>
>> usage_with_options(builtin_sparse_checkout_usage,
>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
>> index b7d5f15830..499bd8d6d0 100755
>> --- a/t/t1091-sparse-checkout-builtin.sh
>> +++ b/t/t1091-sparse-checkout-builtin.sh
>> @@ -100,4 +100,24 @@ test_expect_success 'clone --sparse' '
>> test_cmp expect dir
>> '
>>
>> +test_expect_success 'add to existing sparse-checkout' '
>> + echo "/folder2/*" | git -C repo sparse-checkout add &&
>
> I've always been using '/folder2/' in sparse-checkout, without the
> trailing asterisk. That seems more friendly for cone mode too. Are
> there benefits to keeping the trailing asterisk?
I think I've been seeing issues with pattern matching on Windows without
the trailing asterisk. I'm currently double-checking to make sure this
is important or not.
>> + cat >expect <<-EOF &&
>> + /*
>> + !/*/*
>> + /folder1/*
>> + /folder2/*
>> + EOF
>> + 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
>> \ No newline at end of file
I'm trying to fix this newline issue everywhere.
Thanks,
-Stolee
