Sparse-checkout: Add subcommand and Windows paths

[derrickstolee]

This is based on ds/sparse-checkout-harden.

The sparse-checkout builtin currently lets users modify their sparse-checkout file with the all-or-nothing "set" subcommand. It may be easier for a user to expand their sparse cone using a "git sparse-checkout add <pattern/path> ..." subcommand. To achieve this while reusing as much code as possible from the "set" subcommand, the first two patches extract methods from sparse_checkout_set().

As part of this "add" subcommand, I created a modify_pattern_list() method that serves as the core interaction with the sparse-checkout file and working directory. It changes how it constructs the in-memory pattern list based on an "enum modify_type" that only has two options: REPLACE and ADD. This could be extended with REMOVE in a later update. I started building the REMOVE logic, but it was more complicated than ADD, and I didn't have time to finish it right now due to other obligations.

Finally, a Windows user contacted me about using Windows-style paths when in cone mode to add a nested directory. This case is fixed and tested in the final patch.

Thanks,
-Stolee

Cc: [email protected], [email protected], [email protected]

[dscho]

I restarted the job (it was failing in a well-known flaky test).

[derrickstolee]

Just a quick note that this currently fails in t1091.37 cone mode replaces backslashes with slashes:

Thanks! I had pushed a WIP to get Windows builds, then pulled them to my Windows machine to fix the results. Thanks for restarting the broken build for the unrelated flaky test.

[derrickstolee]

Note to self: UPDATE THE DOCS BEFORE SUBMITTING.

[derrickstolee]

/submit

[gitgitgadget]

Submitted as [email protected]

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> -static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
> +static void add_patterns_from_input(struct pattern_list *pl,
> +				    int argc, const char **argv)

Separating out what happens _after_ parse_options into its own
helper function makes sense.

Everything, not limited to this function, works on its input, so the
name suffix does not add much value to the readers.  IOW, I do not
think I, as a new reader of this code, would be confused if this
function were called add_patterns().

If we wanted to add more words to the name, perhaps I'd use them to
describe the shape of the input (e.g. "add_patterns_from_acav") but
that is obvious from the list of input parameter, so...

I haven't read the rest of the series, but will we ever call this
helper function with an array we manufacture ourselves?  If the
input array is known to be NULL terminated (and at this step in the
series, it certainly is, as we are using the ac/av supplied by the C
runtime entry point), perhaps we can omit argc from the parameter to
simplify the calling convention a bit?

>  {
>  	int i;
> -	struct pattern_list pl;
> -	int result;
> -	int changed_config = 0;
> -
> -	static struct option builtin_sparse_checkout_set_options[] = {
> -		OPT_BOOL(0, "stdin", &set_opts.use_stdin,
> -			 N_("read patterns from standard in")),
> -		OPT_END(),
> -	};
> -
> -	repo_read_index(the_repository);
> -	require_clean_work_tree(the_repository,
> -				N_("set sparse-checkout patterns"), NULL, 1, 0);
> -
> -	memset(&pl, 0, sizeof(pl));
> -
> -	argc = parse_options(argc, argv, prefix,
> -			     builtin_sparse_checkout_set_options,
> -			     builtin_sparse_checkout_set_usage,
> -			     PARSE_OPT_KEEP_UNKNOWN);
> -
>  	if (core_sparse_checkout_cone) {
>  		struct strbuf line = STRBUF_INIT;
>  
> -		hashmap_init(&pl.recursive_hashmap, pl_hashmap_cmp, NULL, 0);
> -		hashmap_init(&pl.parent_hashmap, pl_hashmap_cmp, NULL, 0);
> -		pl.use_cone_patterns = 1;
> +		hashmap_init(&pl->recursive_hashmap, pl_hashmap_cmp, NULL, 0);
> +		hashmap_init(&pl->parent_hashmap, pl_hashmap_cmp, NULL, 0);
> +		pl->use_cone_patterns = 1;
> ...

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> +	switch (m) {
> +	case ADD:
> +		if (core_sparse_checkout_cone)
> +			add_patterns_cone_mode(argc, argv, &pl);
> +		else
> +			add_patterns_literal(argc, argv, &pl);
> +		break;
> +
> +	case REPLACE:
> +		add_patterns_from_input(&pl, argc, argv);
> +		break;
> +	}

Is it just me or do readers find it irritating to see the order of
the arguments seem a bit random?  Those who like Pseudo-OO-in-C
probably would want to consistently see &pl as the first parameter,
which lets them pretend various flavours of add_patterns_*() are
all "methods" to the pattern list object.

> +static void add_patterns_literal(int argc, const char **argv,
> +				 struct pattern_list *pl)
> +{
> +	char *sparse_filename = get_sparse_checkout_filename();
> +	if (add_patterns_from_file_to_list(sparse_filename, "", 0,
> +					   pl, NULL))
> +		die(_("unable to load existing sparse-checkout patterns"));
> +	free(sparse_filename);
> +	add_patterns_from_input(pl, argc, argv);
> +}

It may make it easier to read the caller if this helper were
replaced with one that does not add anything but just read from the
existing file, i.e.

		if (core_sparse_checkout_cone)
			add_patterns_cone_mode(argc, argv, &pl);
		else {
			read_existing_patterns(&pl);
			add_patterns_from_input(&pl, argc, argv);
		}

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes:

> This is based on ds/sparse-checkout-harden.
>
> The sparse-checkout builtin currently lets users modify their
> sparse-checkout file with the all-or-nothing "set" subcommand. It may be
> easier for a user to expand their sparse cone using a "git sparse-checkout
> add <pattern/path> ..." subcommand.

Makes sense.  

I suspect that people may get greedy and start saying "now we can
add a new directory (say, Documentation/) without repeating what we
already configured, I want a way to add a new directory without some
of the paths in that directory (say, Documentation/ but not
Documentation/technical/)", but that won't happen until people can
use "add", so let's take small incremental steps, and addition of
"add" is such a good first step.

Will queue.  Thanks.

[gitgitgadget]

This branch is now known as ds/sparse-add.

[gitgitgadget]

This patch series was integrated into pu via git@47ae036.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Feb 11, 2020 at 03:02:20PM +0000, Derrick Stolee via GitGitGadget wrote:

> This is based on ds/sparse-checkout-harden.
> 
> The sparse-checkout builtin currently lets users modify their
> sparse-checkout file with the all-or-nothing "set" subcommand. It may be
> easier for a user to expand their sparse cone using a "git sparse-checkout
> add <pattern/path> ..." subcommand. To achieve this while reusing as much
> code as possible from the "set" subcommand, the first two patches extract
> methods from sparse_checkout_set().

FWIW, this makes perfect sense to me. I actually expected "set" to
behave like "add" the first time I tried it out.

-Peff

[gitgitgadget]

This patch series was integrated into pu via git@cf3c3a5.

[gitgitgadget]

This patch series was integrated into pu via git@f406c55.

[gitgitgadget]

This patch series was integrated into pu via git@3afa4ba.

[gitgitgadget]

This patch series was integrated into pu via git@6c744af.

[gitgitgadget]

This patch series was integrated into next via git@3799757.

[gitgitgadget]

This patch series was integrated into pu via git@b3f673c.

[gitgitgadget]

This patch series was integrated into pu via git@c9ad2e9.

[gitgitgadget]

This patch series was integrated into pu via git@6f71fe9.

[gitgitgadget]

This patch series was integrated into pu via git@3cd36d1.

[gitgitgadget]

This patch series was integrated into pu via git@afff814.