Re: [PATCH 3/7] sparse-checkout: pay attention to prefix for {set, add}

[Derrick Stolee <derrickstolee@github.com>]

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> In cone mode, non-option arguments to set & add are clearly paths, and
> as such, we should pay attention to prefix.
> 
> In non-cone mode, it is not clear that folks intend to provide paths
> since the inputs are gitignore-style patterns.  Paying attention to
> prefix would prevent folks from doing things like
>    git sparse-checkout add /.gitattributes
>    git sparse-checkout add '/toplevel-dir/*'
> In fact, the former will result in
>    fatal: '/.gitattributes' is outside repository...
> while the later will result in
>    fatal: Invalid path '/toplevel-dir': No such file or directory
> despite the fact that both are valid gitignore-style patterns that would
> select real files if added to the sparse-checkout file.  However, these
> commands can be run successfully from the toplevel directory, and many
> gitignore-style patterns ARE paths, and bash completion seems to be
> suggesting directories and files, so perhaps for consistency we pay
> attention to the prefix?  It's not clear what is okay here, but maybe
> that's yet another reason to deprecate non-cone mode as we will do later
> in this series.
> 
> For now, incorporate prefix into the positional arguments for either
> cone or non-cone mode.  For additional discussion of this issue, see
> https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/

Perhaps this was covered in the issue, but for non-cone mode, it
matters if there is a leading slash or not in the pattern. Will
this change make it impossible for a user to input that distinction?

Will there still be a difference between:

	git sparse-checkout set --no-cone /.vs/

and

	git sparse-checkout set --no-cone .vs/

?

> Helped-by: Junio Hamano <gitster@pobox.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>

This could probably use a

  Reported-by: Lessley Dennington <lessleydennington@gmail.com>

> +static void sanitize_paths(int argc, const char **argv, const char *prefix)
> +{
> +	if (!argc)
> +		return;
> +
> +	if (prefix && *prefix) {
> +		/*
> +		 * The args are not pathspecs, so unfortunately we
> +		 * cannot imitate how cmd_add() uses parse_pathspec().
> +		 */
> +		int i;
> +		int prefix_len = strlen(prefix);
> +
> +		for (i = 0; i < argc; i++)
> +			argv[i] = prefix_path(prefix, prefix_len, argv[i]);
> +	}
> +}
> +
>  static char const * const builtin_sparse_checkout_add_usage[] = {
>  	N_("git sparse-checkout add (--stdin | <patterns>)"),
>  	NULL
> @@ -708,6 +726,8 @@ static int sparse_checkout_add(int argc, const char **argv, const char *prefix)
>  			     builtin_sparse_checkout_add_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
>  
> +	sanitize_paths(argc, argv, prefix);
> +
>  	return modify_pattern_list(argc, argv, add_opts.use_stdin, ADD);
>  }
>  
> @@ -759,6 +779,8 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix)
>  	if (!core_sparse_checkout_cone && argc == 0) {
>  		argv = default_patterns;
>  		argc = default_patterns_nr;
> +	} else {
> +		sanitize_paths(argc, argv, prefix);
>  	}

Code changes appear to do as described: apply the prefix everywhere
it matters, no matter the mode.

> +test_expect_success 'set from subdir pays attention to prefix' '
> +	git -C repo sparse-checkout disable &&
> +	git -C repo/deep sparse-checkout set --cone deeper2 ../folder1 &&
> +
> +	git -C repo sparse-checkout list >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	deep/deeper2
> +	folder1
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'add from subdir pays attention to prefix' '
> +	git -C repo sparse-checkout set --cone deep/deeper2 &&
> +	git -C repo/deep sparse-checkout add deeper1/deepest ../folder1 &&
> +
> +	git -C repo sparse-checkout list >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	deep/deeper1/deepest
> +	deep/deeper2
> +	folder1
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_done

These tests could use a non-cone-mode version to demonstrate the behavior
in that mode.

Thanks,
-Stolee