Re: [PATCH 5/9] sparse-checkout: create 'disable' subcommand
- From
Elijah Newren <newren@gmail.com>
- Date
- Aug 23, 2019, 23:50 UTC
- Message-ID
- <CABPp-BHon=QgZXPjwyZspq0wW3rFLZqx1HZgE8eMq6DuTCc-0g@mail.gmail.com>
- In-Reply-To
- <5f332b799f90b64e2c0720fd0aba0e163d41587a.1566313865.git.gitgitgadget@gmail.com>
On Tue, Aug 20, 2019 at 8:14 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 36 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 | 45 ++++++++++++++++++++++++--- > t/t1091-sparse-checkout-builtin.sh | 15 +++++++++ > 3 files changed, 67 insertions(+), 19 deletions(-) > > diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt > index 6f540a3443..de04b768ae 100644 > --- a/Documentation/git-sparse-checkout.txt > +++ b/Documentation/git-sparse-checkout.txt > @@ -38,6 +38,10 @@ COMMANDS > Add a set of patterns to the sparse-checkout file, as given over > stdin. Updates 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. > + > SPARSE CHECKOUT > ---------------- > > @@ -60,6 +64,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.
Make sense.
> + > +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.
But why add this paragraph? read-tree is plumbing that we'd rather not force on users, so I'd rather not mention it. And I thought we were setting core.sparseCheckout for the user.
Show 15 quoted lines
> -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: > - > ----------------- > -/* > ----------------- > - > -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.
Wahoo!
Show 7 quoted lines
> -static int sc_enable_config(void)
> +static int sc_set_config(int mode)
> {
> struct argv_array argv = ARGV_ARRAY_INIT;
> int result = 0;
> - argv_array_pushl(&argv, "config", "--add", "core.sparseCheckout", "true", NULL);
> + argv_array_pushl(&argv, "config", "--add", "core.sparseCheckout", NULL);Remove "--add" and add "--worktree".
Show 15 quoted lines
> +
> + switch (mode) {
> + case 1:
> + argv_array_pushl(&argv, "true", NULL);
> + break;
> +
> + case 0:
> + argv_array_pushl(&argv, "false", NULL);
> + break;
> +
> + default:
> + die(_("invalid config mode"));
> + }
>
> if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {Can't we use git_config_set instead of run_command_v_opt?
Show 7 quoted lines
> error(_("failed to enable core.sparseCheckout"));
> @@ -125,7 +138,7 @@ static int sparse_checkout_init(int argc, const char **argv)
> FILE *fp;
> int res;
>
> - if (sc_enable_config())
> + if (sc_set_config(1))Random idea: The code would be more self-documenting, if you just made sc_set_config take a string and you passed either "true" or "false". Then you could also dispense with the switch statement. Yes, you can make typos, but there are only going to be 2-3 places in the code that call it, right?
Show 14 quoted lines
> return 1;
>
> memset(&el, 0, sizeof(el));
> @@ -194,6 +207,28 @@ static int sparse_checkout_add(int argc, const char **argv)
> return sc_read_tree();
> }
>
> +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"));Ooh, preventative setting. This'll probably save some grief for people who get themselves in a weird state and start mucking around manually; very nice.
Show 8 quoted lines
> +
> + sparse_filename = get_sparse_checkout_filename();
> + fp = fopen(sparse_filename, "w");
> + fprintf(fp, "/*\n");
> + fclose(fp);
> +
> + if (sc_read_tree())
> + die(_("error while refreshing working directory"));This can fail e.g. if there are conflicts in the index. Luckily, if they fix them up and then re-run "git sparse-checkout disable" then it can work. But you've already changed the $GIT_DIR/info/sparse-checkout file to a state that won't match what they actually have checked out. Will that cause any problems? (I can't think of any immediately, but I am curious.)
Also, should or can we give a better error message, such as instructing them to try again after they've fixed any problems reported by the sc_read_tree() operation?
Show 31 quoted lines
> +
> + unlink(sparse_filename);
> + free(sparse_filename);
> +
> + return sc_set_config(0);
> +}
> +
> int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
> {
> static struct option builtin_sparse_checkout_options[] = {
> @@ -217,6 +252,8 @@ int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
> return sparse_checkout_init(argc, argv);
> if (!strcmp(argv[0], "add"))
> return sparse_checkout_add(argc, argv);
> + 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 499bd8d6d0..68ca63a6f6 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -120,4 +120,19 @@ test_expect_success 'add to existing sparse-checkout' '
> 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 &&Should this command also have a --worktree flag (just for consistency, even if we know there will only be one worktree so it doesn't matter in practice)? There are probably similar cases I overlooked on the previous patches.
Show 16 quoted lines
> + test_i18ngrep "core.sparsecheckout=false" config && > + ls repo >dir && > + cat >expect <<-EOF && > + a > + deep > + folder1 > + folder2 > + EOF > + test_cmp expect dir > +' > + > test_done > \ No newline at end of file > -- > gitgitgadget >