[v2,04/11] sparse-checkout: 'set' subcommand

Commit Message

Philippe Blain via GitGitGadget Sept. 19, 2019, 2:43 p.m. UTC

From: Derrick Stolee <[e-mail]>

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.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/git-sparse-checkout.txt |  5 +++++
 builtin/sparse-checkout.c             | 31 ++++++++++++++++++++++++++-
 t/t1091-sparse-checkout-builtin.sh    | 19 ++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

Comments

Elijah Newren Oct. 5, 2019, 10:44 p.m. UTC | #1

On Thu, Sep 19, 2019 at 3:07 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> +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);
> +
> +       clear_pattern_list(pl);

It seems slightly odd that pl is passed in but cleared in this
function rather than in the caller that created pl.  Should this be
moved to the caller, or, alternatively, a comment added to explain
this side-effect for future callers of the function?

The rest of the patch looked good to me.

Elijah Newren Oct. 6, 2019, 12:30 a.m. UTC | #2

On Sat, Oct 5, 2019 at 3:44 PM Elijah Newren <[e-mail]> wrote:
>
> On Thu, Sep 19, 2019 at 3:07 PM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
> > +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);
> > +
> > +       clear_pattern_list(pl);
>
> It seems slightly odd that pl is passed in but cleared in this
> function rather than in the caller that created pl.  Should this be
> moved to the caller, or, alternatively, a comment added to explain
> this side-effect for future callers of the function?
>
> The rest of the patch looked good to me.

Actually, thought of something else.  What if the user calls 'git
sparse-checkout set ...' without first calling 'git sparse-checkout
init'?  Should that report an error to the user, a suggestion to
follow it up with 'sparse-checkout init', or should it just call
sc_set_config() behind the scenes and allow bypassing the init
subcommand?

Derrick Stolee Oct. 7, 2019, 6:26 p.m. UTC | #3

On 10/5/2019 8:30 PM, Elijah Newren wrote:
> On Sat, Oct 5, 2019 at 3:44 PM Elijah Newren <[e-mail]> wrote:
>>
>> On Thu, Sep 19, 2019 at 3:07 PM Derrick Stolee via GitGitGadget
>> <[e-mail]> wrote:
>>> +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);
>>> +
>>> +       clear_pattern_list(pl);
>>
>> It seems slightly odd that pl is passed in but cleared in this
>> function rather than in the caller that created pl.  Should this be
>> moved to the caller, or, alternatively, a comment added to explain
>> this side-effect for future callers of the function?
>>
>> The rest of the patch looked good to me.
> 
> Actually, thought of something else.  What if the user calls 'git
> sparse-checkout set ...' without first calling 'git sparse-checkout
> init'?  Should that report an error to the user, a suggestion to
> follow it up with 'sparse-checkout init', or should it just call
> sc_set_config() behind the scenes and allow bypassing the init
> subcommand?

Maybe a warning would suffice. I still think the workflow of the
following is most correct, and not difficult to recommend:

* "git sparse-checkout init [--cone]" -OR- "git clone --sparse"
* git sparse-checkout set [stuff]
* git sparse-checkout disable

Thanks,
-Stolee

Elijah Newren Oct. 11, 2019, 10:24 p.m. UTC | #4

On Mon, Oct 7, 2019 at 11:26 AM Derrick Stolee <[e-mail]> wrote:
>
> On 10/5/2019 8:30 PM, Elijah Newren wrote:
> > On Sat, Oct 5, 2019 at 3:44 PM Elijah Newren <[e-mail]> wrote:
> >>
> >> On Thu, Sep 19, 2019 at 3:07 PM Derrick Stolee via GitGitGadget
> >> <[e-mail]> wrote:
> >>> +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);
> >>> +
> >>> +       clear_pattern_list(pl);
> >>
> >> It seems slightly odd that pl is passed in but cleared in this
> >> function rather than in the caller that created pl.  Should this be
> >> moved to the caller, or, alternatively, a comment added to explain
> >> this side-effect for future callers of the function?
> >>
> >> The rest of the patch looked good to me.
> >
> > Actually, thought of something else.  What if the user calls 'git
> > sparse-checkout set ...' without first calling 'git sparse-checkout
> > init'?  Should that report an error to the user, a suggestion to
> > follow it up with 'sparse-checkout init', or should it just call
> > sc_set_config() behind the scenes and allow bypassing the init
> > subcommand?
>
> Maybe a warning would suffice. I still think the workflow of the
> following is most correct, and not difficult to recommend:
>
> * "git sparse-checkout init [--cone]" -OR- "git clone --sparse"
> * git sparse-checkout set [stuff]
> * git sparse-checkout disable

Recommending the right thing is easy, but users will call things out
of order despite documentation.  If they call disable before init, I
see no problems that will lead to confusion.  If they call set without
calling init, I can see them being surprised...so I commented on it
and asked if we want a warning or whatever.

Patch

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index 9707ef93b1..87813e5797 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -34,6 +34,11 @@  COMMANDS
 	by Git. Add patterns to the sparse-checkout file to
 	repopulate the working directory.
 
+'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.
+
 SPARSE CHECKOUT
 ----------------
 
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 656e6ebdd5..13333fba6a 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
 };
 
@@ -130,6 +130,33 @@  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);
+
+	clear_pattern_list(pl);
+	return update_working_directory();
+}
+
+static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct pattern_list pl;
+	memset(&pl, 0, sizeof(pl));
+
+	for (i = 1; i < argc; i++)
+		add_pattern(argv[i], NULL, 0, &pl, 0);
+
+	return write_patterns_and_update(&pl);
+}
+
 int cmd_sparse_checkout(int argc, const char **argv, const char *prefix)
 {
 	static struct option builtin_sparse_checkout_options[] = {
@@ -152,6 +179,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 26b4ce9acd..f21ea61494 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -101,5 +101,24 @@  test_expect_success 'clone --sparse' '
 	test_cmp expect dir
 '
 
+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