Re: [PATCH v2 06/11] sparse-checkout: create 'disable' subcommand

From: Elijah Newren <newren@gmail.com>
Date: Oct 6, 2019, 04:10 UTC
Message-ID: <CABPp-BHYCCD+Vx5fq35jH82eHc1-P53Lz_aGNpHJNcx9kg2K-A@mail.gmail.com>
In-Reply-To: <b62b76013f98e57089c9b0c03209e28757977dd7.1568904188.git.gitgitgadget@gmail.com>

On Thu, Sep 19, 2019 at 1:46 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 26 quoted lines

>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The instructions for disabling a sparse-checkout to a full
> working directory are complicated and non-intuitive. Add a
> subcommand, 'git sparse-checkout disable', to perform those
> steps for the user.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-sparse-checkout.txt | 26 ++++++++-----------
>  builtin/sparse-checkout.c             | 37 ++++++++++++++++++++++++---
>  t/t1091-sparse-checkout-builtin.sh    | 15 +++++++++++
>  3 files changed, 59 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index 87813e5797..da95b28b1c 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -39,6 +39,10 @@ COMMANDS
>         a list of arguments following the 'set' subcommand. Update the
>         working directory to match the new patterns.
>
> +'disable'::
> +       Remove the sparse-checkout file, set `core.sparseCheckout` to
> +       `false`, and restore the working directory to include all files.

Good, so 'init' (and maybe 'set'?) will set core.sparseCheckout, and disable will unset it, so the user doesn't have to worry about it...

Show 10 quoted lines

> +
>  SPARSE CHECKOUT
>  ----------------
>
> @@ -61,6 +65,13 @@ Then it compares the new skip-worktree value with the previous one. If
>  skip-worktree turns from set to unset, it will add the corresponding
>  file back. If it turns from unset to set, that file will be removed.
>
> +To repopulate the working directory with all files, use the
> +`git sparse-checkout disable` command.

Good.

> +Sparse checkout support in 'git checkout' and similar commands is
> +disabled by default. You need to set `core.sparseCheckout` to `true`
> +in order to have sparse checkout support.

Aren't we having the user use 'git sparse-checkout init' to do that? Why guide them to the core.sparseCheckout option? And why mention it without extensions.worktreeConfig?

Show 18 quoted lines

> +
>  ## FULL PATTERN SET
>
>  By default, the sparse-checkout file uses the same syntax as `.gitignore`
> @@ -75,21 +86,6 @@ using negative patterns. For example, to remove the file `unwanted`:
>  !unwanted
>  ----------------
>
> -Another tricky thing is fully repopulating the working directory when you
> -no longer want sparse checkout. You cannot just disable "sparse
> -checkout" because skip-worktree bits are still in the index and your working
> -directory is still sparsely populated. You should re-populate the working
> -directory with the `$GIT_DIR/info/sparse-checkout` file content as
> -follows:
> -
> -----------------
> -/*
> -----------------

Yaay, glad to see this removed.

Show 26 quoted lines

> -Then you can disable sparse checkout. Sparse checkout support in 'git
> -read-tree' and similar commands is disabled by default. You need to
> -set `core.sparseCheckout` to `true` in order to have sparse checkout
> -support.
>
>  SEE ALSO
>  --------
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index f726fcd6b8..f858f0b1b5 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|set] <options>"),
> +       N_("git sparse-checkout [init|list|set|disable] <options>"),
>         NULL
>  };
>
> @@ -74,7 +74,7 @@ static int update_working_directory(void)
>         return result;
>  }
>
> -static int sc_enable_config(void)
> +static int sc_set_config(int mode)

Nice to see this change from the RFC round; do we want to use an enum instead of an int, or is the int good enough? (No strong opinion here, just asking.)

Show 51 quoted lines

>  {
>         struct argv_array argv = ARGV_ARRAY_INIT;
>
> @@ -83,7 +83,12 @@ static int sc_enable_config(void)
>                 return 1;
>         }
>
> -       argv_array_pushl(&argv, "config", "--worktree", "core.sparseCheckout", "true", NULL);
> +       argv_array_pushl(&argv, "config", "--worktree", "core.sparseCheckout", NULL);
> +
> +       if (mode)
> +               argv_array_pushl(&argv, "true", NULL);
> +       else
> +               argv_array_pushl(&argv, "false", NULL);
>
>         if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>                 error(_("failed to enable core.sparseCheckout"));
> @@ -101,7 +106,7 @@ static int sparse_checkout_init(int argc, const char **argv)
>         int res;
>         struct object_id oid;
>
> -       if (sc_enable_config())
> +       if (sc_set_config(1))
>                 return 1;
>
>         memset(&pl, 0, sizeof(pl));
> @@ -188,6 +193,28 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>         return write_patterns_and_update(&pl);
>  }
>
> +static int sparse_checkout_disable(int argc, const char **argv)
> +{
> +       char *sparse_filename;
> +       FILE *fp;
> +
> +       if (sc_set_config(1))
> +               die(_("failed to change config"));
> +
> +       sparse_filename = get_sparse_checkout_filename();
> +       fp = fopen(sparse_filename, "w");
> +       fprintf(fp, "/*\n");
> +       fclose(fp);
> +
> +       if (update_working_directory())
> +               die(_("error while refreshing working directory"));
> +
> +       unlink(sparse_filename);
> +       free(sparse_filename);
> +
> +       return sc_set_config(0);
> +}

So we update the .git/info/sparse-checkout file first (or the worktree-specific equivalent), then call update_working_directory() which can fail -- in particular if the user calls it when they have any conflicted files. But then the sparse-checkout file has already been emptied, so it did make some changes, just not all the changes the user would expect, leaving them in an intermediate state with an error message that doesn't explain how to recover. Would it be worth checking for this case, and telling the user to fix up conflicts then re-run the disable command? Would it make more sense to just replace the 'read-tree -mu HEAD' with something that doesn't error out in such a case? Or is this just a shortcoming of an experimental feature that we'll get to later? (I'm okay with the last of those, since we also still need to address defaults of several other commands when sparse checkouts are active[1].)

[1] https://public-inbox.org/git/CABPp-BGuFhDwWZBRaD3nA8ui46wor-4=Ha1G1oApsfF8KNpfGQ@mail.gmail.com/

Show 37 quoted lines

> +
>  int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
>  {
>         static struct option builtin_sparse_checkout_options[] = {
> @@ -212,6 +239,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
>                         return sparse_checkout_init(argc, argv);
>                 if (!strcmp(argv[0], "set"))
>                         return sparse_checkout_set(argc, argv, prefix);
> +               if (!strcmp(argv[0], "disable"))
> +                       return sparse_checkout_disable(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 02ba9ec314..22fa032d6d 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -140,5 +140,20 @@ test_expect_success 'set sparse-checkout using --stdin' '
>         test_cmp expect dir
>  '
>
> +test_expect_success 'sparse-checkout disable' '
> +       git -C repo sparse-checkout disable &&
> +       test_path_is_missing repo/.git/info/sparse-checkout &&
> +       git -C repo config --list >config &&
> +       test_i18ngrep "core.sparsecheckout=false" config &&
> +       ls repo >dir &&
> +       cat >expect <<-EOF &&
> +               a
> +               deep
> +               folder1
> +               folder2
> +       EOF
> +       test_cmp expect dir
> +'
> +
>  test_done

The rest of the patch looks good.