Re: [PATCH v2] dir: check for single file cone patterns

[Victoria Dye <vdye@github.com>]

William Sprent via GitGitGadget wrote:
> From: William Sprent <williams@unity3d.com>
> 
> The sparse checkout documentation states that the cone mode pattern set
> is limited to patterns that either recursively include directories or
> patterns that match all files in a directory. In the sparse checkout
> file, the former manifest in the form:
> 
>     /A/B/C/
> 
> while the latter become a pair of patterns either in the form:
> 
>     /A/B/
>     !/A/B/*/
> 
> or in the special case of matching the toplevel files:
> 
>     /*
>     !/*/
> 
> The 'add_pattern_to_hashsets()' function contains checks which serve to
> disable cone-mode when non-cone patterns are encountered. However, these
> do not catch when the pattern list attempts to match a single file or
> directory, e.g. a pattern in the form:
> 
>     /A/B/C
> 
> This causes sparse-checkout to exhibit unexpected behaviour when such a
> pattern is in the sparse-checkout file and cone mode is enabled.
> Concretely, with the pattern like the above, sparse-checkout, in
> non-cone mode, will only include the directory or file located at
> '/A/B/C'. However, with cone mode enabled, sparse-checkout will instead
> just manifest the toplevel files but not any file located at '/A/B/C'.
> 
> Relatedly, issues occur when supplying the same kind of filter when
> partial cloning with '--filter=sparse:oid=<oid>'. 'upload-pack' will
> correctly just include the objects that match the non-cone pattern
> matching. Which means that checking out the newly cloned repo with the
> same filter, but with cone mode enabled, fails due to missing objects.

This clearly explains the issue you're trying to fix: cone mode
sparse-checkout requires patterns like '/A/B/', !/A/B/*/', '/A/B/C/', etc.,
but invalid patterns like '/A/B/C' (no trailing slash) currently don't force
the switch to non-cone mode, leading to unexpected behavior.

> 
> To fix these issues, add a cone mode pattern check that asserts that
> every pattern is either a directory match or the pattern '/*'. Add a
> test to verify the new pattern check and modify another to reflect that
> non-directory patterns are caught earlier.

I think this is the best way to maintain the current intended behavior of
cone mode sparse-checkout. While the idea of single file "exceptions" to
cone patterns has been brought up in the past (I think most recently at the
Contributor's Summit this past September [1]), it'd be a substantial change
that's definitely out-of-scope of this small bugfix.

[1] https://lore.kernel.org/git/YzXwOsaoCdBhHsX1@nand.local/

> 
> Signed-off-by: William Sprent <williams@unity3d.com>
> ---
>     dir: check for single file cone patterns
>     
>     Resubmitting this without the superfluous double negation of the
>     'strcmp' output.
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1446%2Fwilliams-unity%2Fws%2Fsparse-checkout-pattern-match-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1446/williams-unity/ws/sparse-checkout-pattern-match-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1446
> 
> Range-diff vs v1:
> 
>  1:  cfb4b75c378 ! 1:  d5406e62f6f dir: check for single file cone patterns
>      @@ dir.c: static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_
>        	}
>        
>       +	if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&
>      -+	    !!strcmp(given->pattern, "/*")) {
>      ++	    strcmp(given->pattern, "/*")) {
>       +		/* Not a cone pattern. */
>       +		warning(_("unrecognized pattern: '%s'"), given->pattern);
>       +		goto clear_hashmaps;
> 
> 
>  dir.c                              |  7 +++++++
>  t/t1091-sparse-checkout-builtin.sh | 11 ++++++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/dir.c b/dir.c
> index fbdb24fc819..4e99f0c868f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -732,6 +732,13 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
>  		goto clear_hashmaps;
>  	}
>  
> +	if (!(given->flags & PATTERN_FLAG_MUSTBEDIR) &&
> +	    strcmp(given->pattern, "/*")) {

This condition is only entered if:

- the pattern *does not* end in a trailing slash 
  ('!(given->flags & PATTERN_FLAG_MUSTBEDIR)'), AND
- the pattern *does not* start with '/*' ('strcmp(given->pattern, "/*")')

The '/*'-like patterns are already handled by other parts of
'add_pattern_to_hashsets()', so only the '/A/B/C'-like patterns will trigger
the "unrecognized" warning. Looks good!


> +		/* Not a cone pattern. */
> +		warning(_("unrecognized pattern: '%s'"), given->pattern);
> +		goto clear_hashmaps;
> +	}
> +
>  	prev = given->pattern;
>  	cur = given->pattern + 1;
>  	next = given->pattern + 2;
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index b563d6c263e..627267be153 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -238,7 +238,7 @@ test_expect_success 'cone mode: match patterns' '
>  test_expect_success 'cone mode: warn on bad pattern' '
>  	test_when_finished mv sparse-checkout repo/.git/info/ &&
>  	cp repo/.git/info/sparse-checkout . &&
> -	echo "!/deep/deeper/*" >>repo/.git/info/sparse-checkout &&
> +	echo "!/deep/deeper/*/" >>repo/.git/info/sparse-checkout &&

This test changes because, without the trailing slash on the pattern, the
error would become "unrecognized pattern: !/deep/deeper/*" due to the new
check in 'add_pattern_to_hashsets()'. Changing the test pattern ensures
we're still exercising what the test was originally intended to exercise,
and doesn't indicate a regression.

>  	git -C repo read-tree -mu HEAD 2>err &&
>  	test_i18ngrep "unrecognized negative pattern" err
>  '
> @@ -667,6 +667,15 @@ test_expect_success 'pattern-checks: starting "*"' '
>  	check_read_tree_errors repo "a deep" "disabling cone pattern matching"
>  '
>  
> +test_expect_success 'pattern-checks: non directory pattern' '
> +	cat >repo/.git/info/sparse-checkout <<-\EOF &&
> +	/deep/deeper1/a
> +	EOF
> +	check_read_tree_errors repo deep "disabling cone pattern matching" &&
> +	check_files repo/deep deeper1 &&
> +	check_files repo/deep/deeper1 a
> +'

And this test ensures the new check is working for the appropriate patterns.

This patch looks good to me, thanks for finding & fixing the bug!

> +
>  test_expect_success 'pattern-checks: contained glob characters' '
>  	for c in "[a]" "\\" "?" "*"
>  	do
> 
> base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7