diff mbox series

[5/9] sparse-checkout: create 'disable' subcommand

Message ID 5f332b799f90b64e2c0720fd0aba0e163d41587a.1566313865.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series New sparse-checkout builtin and "cone" mode | expand

Commit Message

John Passaro via GitGitGadget Aug. 20, 2019, 3:11 p.m. UTC 
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(-)

Comments

Elijah Newren Aug. 23, 2019, 11:50 p.m. UTC | #1 
On Tue, Aug 20, 2019 at 8:14 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

> -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!


> -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".

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

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

>                 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.

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

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

> +       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
>
diff mbox series

Patch

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.
+
+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.
+
 ## FULL PATTERN SET
 
 By default, the sparse-checkout file uses the same syntax as `.gitignore`
@@ -74,21 +85,6 @@  negate 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:
-
-----------------
-/*
-----------------
-
-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 ec6134fecc..8f97c27ec7 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|add|list]"),
+	N_("git sparse-checkout [init|add|list|disable]"),
 	NULL
 };
 
@@ -79,11 +79,24 @@  static int sc_read_tree(void)
 	return result;
 }
 
-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);
+
+	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)) {
 		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))
 		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"));
+
+	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"));
+
+	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 &&
+	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