diff mbox series

[v5,04/17] sparse-checkout: 'set' subcommand

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

Commit Message

Linus Arver via GitGitGadget Oct. 21, 2019, 1:56 p.m. UTC 
From: Derrick Stolee <dstolee@microsoft.com>

The 'git sparse-checkout set' subcommand takes a list of patterns
as arguments and writes them to the sparse-checkout file. Then, it
updates the working directory using 'git read-tree -mu HEAD'.

The 'set' subcommand will replace the entire contents of the
sparse-checkout file. The write_patterns_and_update() method is
extracted from cmd_sparse_checkout() to make it easier to implement
'add' and/or 'remove' subcommands in the future.

If the core.sparseCheckout config setting is disabled, then enable
the config setting in the worktree config. If we set the config
this way and the sparse-checkout fails, then re-disable the config
setting.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-sparse-checkout.txt |  6 ++++
 builtin/sparse-checkout.c             | 45 ++++++++++++++++++++++++++-
 t/t1091-sparse-checkout-builtin.sh    | 32 +++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)

Comments

SZEDER Gábor Nov. 19, 2019, 3:46 p.m. UTC | #1 
On Mon, Oct 21, 2019 at 01:56:13PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> The 'git sparse-checkout set' subcommand takes a list of patterns
> as arguments and writes them to the sparse-checkout file. Then, it
> updates the working directory using 'git read-tree -mu HEAD'.
> 
> The 'set' subcommand will replace the entire contents of the
> sparse-checkout file. The write_patterns_and_update() method is
> extracted from cmd_sparse_checkout() to make it easier to implement
> 'add' and/or 'remove' subcommands in the future.

Yeah, an 'add' subcommand would be great, because 'set' throwing away
all the existing patterns can lead to some frustration:

  # Let's clone with sparse checkout
  $ git clone --sparse ...
  $ cd clone
  $ less README
  $ git sparse-checkout '/some/dir/*'
  # Erm, what was the next step?
  $ less README
  README: No such file or directory
  # Uhoh.

Having said that, being forced to use 'git sparse-checkout set' and
specify all patterns at once does have its own benefits: I needed like
6 subdirectories to build that subset of a big project that I was
interested in, and now all the necessary patterns are saved in my Bash
history, and I will be able to easily dig out the complete command in
the future.  That wouldn't work with using the 'add' subcommand to add
one pattern at a time.

> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index cb74715ca6..bf2dc55bb1 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -96,4 +96,36 @@ test_expect_success 'clone --sparse' '
>  	test_cmp expect dir
>  '
>  
> +test_expect_success 'set enables config' '
> +	git init empty-config &&
> +	(
> +		cd empty-config &&
> +		test_commit test file &&
> +		test_path_is_missing .git/config.worktree &&
> +		test_must_fail git sparse-checkout set nothing &&

This command prints the same error message twice:

  + test_must_fail git sparse-checkout set nothing
  error: Sparse checkout leaves no entry on working directory
  error: Sparse checkout leaves no entry on working directory


> +		test_i18ngrep "sparseCheckout = false" .git/config.worktree &&

The contents of a configuration file surely can't ever be translated,
can it?!

Please use 'test_cmp_config'.

> +		git sparse-checkout set "/*" &&
> +		test_i18ngrep "sparseCheckout = true" .git/config.worktree
> +	)
> +'
> +
> +test_expect_success 'set sparse-checkout using builtin' '
> +	git -C repo sparse-checkout set "/*" "!/*/" "*folder*" &&
> +	cat >expect <<-EOF &&
> +		/*
> +		!/*/
> +		*folder*
> +	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
> -- 
> gitgitgadget
>
Derrick Stolee Nov. 21, 2019, 1:04 p.m. UTC | #2 
On 11/19/2019 10:46 AM, SZEDER Gábor wrote:
> On Mon, Oct 21, 2019 at 01:56:13PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The 'git sparse-checkout set' subcommand takes a list of patterns
>> as arguments and writes them to the sparse-checkout file. Then, it
>> updates the working directory using 'git read-tree -mu HEAD'.
>>
>> The 'set' subcommand will replace the entire contents of the
>> sparse-checkout file. The write_patterns_and_update() method is
>> extracted from cmd_sparse_checkout() to make it easier to implement
>> 'add' and/or 'remove' subcommands in the future.
> 
> Yeah, an 'add' subcommand would be great, because 'set' throwing away
> all the existing patterns can lead to some frustration:

I plan to extend this feature when this series lands.

> Having said that, being forced to use 'git sparse-checkout set' and
> specify all patterns at once does have its own benefits: I needed like
> 6 subdirectories to build that subset of a big project that I was
> interested in, and now all the necessary patterns are saved in my Bash
> history, and I will be able to easily dig out the complete command in
> the future.  That wouldn't work with using the 'add' subcommand to add
> one pattern at a time.

In general, this "set" command is created first because tools can interact
with it more easily than "add" and "remove". Users would probably prefer the
"add" and "remove" way to interact.
 
>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
>> index cb74715ca6..bf2dc55bb1 100755
>> --- a/t/t1091-sparse-checkout-builtin.sh
>> +++ b/t/t1091-sparse-checkout-builtin.sh
>> @@ -96,4 +96,36 @@ test_expect_success 'clone --sparse' '
>>  	test_cmp expect dir
>>  '
>>  
>> +test_expect_success 'set enables config' '
>> +	git init empty-config &&
>> +	(
>> +		cd empty-config &&
>> +		test_commit test file &&
>> +		test_path_is_missing .git/config.worktree &&
>> +		test_must_fail git sparse-checkout set nothing &&
> 
> This command prints the same error message twice:
> 
>   + test_must_fail git sparse-checkout set nothing
>   error: Sparse checkout leaves no entry on working directory
>   error: Sparse checkout leaves no entry on working directory

At this commit, I do not see two identical lines, but instead the second
line says

	error: failed to update index with new sparse-checkout paths

(that "paths" should probably be "patterns")

Thanks,
-Stolee
SZEDER Gábor Nov. 21, 2019, 1:36 p.m. UTC | #3 
On Thu, Nov 21, 2019 at 08:04:35AM -0500, Derrick Stolee wrote:
> On 11/19/2019 10:46 AM, SZEDER Gábor wrote:
> > On Mon, Oct 21, 2019 at 01:56:13PM +0000, Derrick Stolee via GitGitGadget wrote:
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> The 'git sparse-checkout set' subcommand takes a list of patterns
> >> as arguments and writes them to the sparse-checkout file. Then, it
> >> updates the working directory using 'git read-tree -mu HEAD'.
> >>
> >> The 'set' subcommand will replace the entire contents of the
> >> sparse-checkout file. The write_patterns_and_update() method is
> >> extracted from cmd_sparse_checkout() to make it easier to implement
> >> 'add' and/or 'remove' subcommands in the future.
> > 
> > Yeah, an 'add' subcommand would be great, because 'set' throwing away
> > all the existing patterns can lead to some frustration:
> 
> I plan to extend this feature when this series lands.
> 
> > Having said that, being forced to use 'git sparse-checkout set' and
> > specify all patterns at once does have its own benefits: I needed like
> > 6 subdirectories to build that subset of a big project that I was
> > interested in, and now all the necessary patterns are saved in my Bash
> > history, and I will be able to easily dig out the complete command in
> > the future.  That wouldn't work with using the 'add' subcommand to add
> > one pattern at a time.
> 
> In general, this "set" command is created first because tools can interact
> with it more easily than "add" and "remove". Users would probably prefer the
> "add" and "remove" way to interact.

Makes sense.

However, I'd like to add that in the meantime I got to like the 'set'
subcommand more and more.  I enabled-disabled sparse checkout a lot of
times while testing and trying to poke holes, and to be able to set up
everything with only one single command that I can easily recall from
the shell's history was a great help.

> >> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> >> index cb74715ca6..bf2dc55bb1 100755
> >> --- a/t/t1091-sparse-checkout-builtin.sh
> >> +++ b/t/t1091-sparse-checkout-builtin.sh
> >> @@ -96,4 +96,36 @@ test_expect_success 'clone --sparse' '
> >>  	test_cmp expect dir
> >>  '
> >>  
> >> +test_expect_success 'set enables config' '
> >> +	git init empty-config &&
> >> +	(
> >> +		cd empty-config &&
> >> +		test_commit test file &&
> >> +		test_path_is_missing .git/config.worktree &&
> >> +		test_must_fail git sparse-checkout set nothing &&
> > 
> > This command prints the same error message twice:
> > 
> >   + test_must_fail git sparse-checkout set nothing
> >   error: Sparse checkout leaves no entry on working directory
> >   error: Sparse checkout leaves no entry on working directory
> 
> At this commit, I do not see two identical lines, but instead the second
> line says
> 
> 	error: failed to update index with new sparse-checkout paths

You're right, I must have looked at and copied the results from a test
run of the full patch series or even from 'next'.  The error message
changes with commit efd9c53b6d (sparse-checkout: update working
directory in-process, 2019-10-21) to what I shown above.

Interestingly, at one point in my testing I managed to get a different
error three times:

  $ ~/src/git/git sparse-checkout set foo
  error: Entry 'dir/bar' not uptodate. Cannot update sparse checkout.
  error: Entry 'dir/bar' not uptodate. Cannot update sparse checkout.
  error: Entry 'dir/bar' not uptodate. Cannot update sparse checkout.

However, when I later re-run the same sequence of commands that lead
to that three errors I got that error only twice, and couldn't
reproduce it since.

Style nit, seeing "error: Sparse..." and "error: Entry...": error
messages should start with a lower-case letter.

> (that "paths" should probably be "patterns")

That would be better indeed, but that error message goes away by the
end of the series anyway...
diff mbox series

Patch

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index 930a361567..b933043b3d 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -42,6 +42,12 @@  To avoid interfering with other worktrees, it first enables the
 `extensions.worktreeConfig` setting and makes sure to set the
 `core.sparseCheckout` setting in the worktree-specific config file.
 
+'set'::
+	Write a set of patterns to the sparse-checkout file, as given as
+	a list of arguments following the 'set' subcommand. Update the
+	working directory to match the new patterns. Enable the
+	core.sparseCheckout config setting if it is not already enabled.
+
 SPARSE CHECKOUT
 ---------------
 
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 6c336b7ab3..834ee421f0 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|list|set) <options>"),
 	NULL
 };
 
@@ -140,6 +140,47 @@  static int sparse_checkout_init(int argc, const char **argv)
 	return update_working_directory();
 }
 
+static int write_patterns_and_update(struct pattern_list *pl)
+{
+	char *sparse_filename;
+	FILE *fp;
+
+	sparse_filename = get_sparse_checkout_filename();
+	fp = fopen(sparse_filename, "w");
+	write_patterns_to_file(fp, pl);
+	fclose(fp);
+	free(sparse_filename);
+
+	return update_working_directory();
+}
+
+static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
+{
+	static const char *empty_base = "";
+	int i;
+	struct pattern_list pl;
+	int result;
+	int set_config = 0;
+	memset(&pl, 0, sizeof(pl));
+
+	for (i = 1; i < argc; i++)
+		add_pattern(argv[i], empty_base, 0, &pl, 0);
+
+	if (!core_apply_sparse_checkout) {
+		sc_set_config(MODE_ALL_PATTERNS);
+		core_apply_sparse_checkout = 1;
+		set_config = 1;
+	}
+
+	result = write_patterns_and_update(&pl);
+
+	if (result && set_config)
+		sc_set_config(MODE_NO_PATTERNS);
+
+	clear_pattern_list(&pl);
+	return result;
+}
+
 int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_options[] = {
@@ -162,6 +203,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], "set"))
+			return sparse_checkout_set(argc, argv, prefix);
 	}
 
 	usage_with_options(builtin_sparse_checkout_usage,
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index cb74715ca6..bf2dc55bb1 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -96,4 +96,36 @@  test_expect_success 'clone --sparse' '
 	test_cmp expect dir
 '
 
+test_expect_success 'set enables config' '
+	git init empty-config &&
+	(
+		cd empty-config &&
+		test_commit test file &&
+		test_path_is_missing .git/config.worktree &&
+		test_must_fail git sparse-checkout set nothing &&
+		test_i18ngrep "sparseCheckout = false" .git/config.worktree &&
+		git sparse-checkout set "/*" &&
+		test_i18ngrep "sparseCheckout = true" .git/config.worktree
+	)
+'
+
+test_expect_success 'set sparse-checkout using builtin' '
+	git -C repo sparse-checkout set "/*" "!/*/" "*folder*" &&
+	cat >expect <<-EOF &&
+		/*
+		!/*/
+		*folder*
+	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