Re: [PATCH 4/9] sparse-checkout: 'add' subcommand

[Elijah Newren <newren@gmail.com>]

On Tue, Aug 20, 2019 at 8:12 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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@microsoft.com>
> ---
>  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?

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

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