Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index

From: Victoria Dye <vdye@github.com>
Date: Feb 27, 2023, 17:18 UTC
Message-ID: <315e70e0-ac1f-2f19-f1cc-6b8b24ffb1fc@github.com>
In-Reply-To: <20230227050543.218294-2-cheskaqiqi@gmail.com>

Shuqi Liang wrote:

Just a heads-up, I was CC'd only on the cover letter (and not this patch).

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>

Even in an RFC (or maybe *especially* in an RFC?), it's important to provide some context around what you're doing in a patch/why you're doing it. It seems like you provided that information in your cover letter [1], though, so I think this "series" would be better off submitted as a single patch, with the cover letter contents...

> ---

...right here! That is, below the '---' line but before the diff summary.

[1] https://lore.kernel.org/git/20230227050543.218294-1-cheskaqiqi@gmail.com/

Show 14 quoted lines

>  builtin/check-attr.c                     |  3 +++
>  t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/builtin/check-attr.c b/builtin/check-attr.c
> index 0fef10eb6b..f85b91ebba 100644
> --- a/builtin/check-attr.c
> +++ b/builtin/check-attr.c
> @@ -112,6 +112,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_default_config, NULL);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;

The test below doesn't do anything special related to the sparse index, so this change is unnecessary (and, as far as I can tell, will break in some usage of 'git check-attr'). If you're only looking for feedback on testing, it'd better to leave this out.

Show 17 quoted lines

> +	
>  	argc = parse_options(argc, argv, prefix, check_attr_options,
>  			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
>  
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..b28010aa5c 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,23 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'check-attr pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	run_on_all touch deep/test.c &&
> +	echo "*.c diff=cpp -crlf myAttr" >>.gitattributes &&

Is there a specific reason you wanted to create a new file, rather than use something in the existing structure (e.g. 'deep/a'?). If not, I'd recommend using the existing file structure setup by the test in the 'setup' test at the beginning of the file.

Show 5 quoted lines

> +	run_on_all cp ../.gitattributes . &&
> +	test_all_match git add .gitattributes &&
> +	test_all_match git commit -m "add .gitattributes" &&
> +	
> +	run_on_all git reset --hard &&

Unless I'm missing something, there's nothing to 'reset' here? Same for the other 'reset's you have below. If they're not needed, they should be removed.

> +	test_all_match echo "deep/test.c" | git check-attr --stdin  -a &&

In addition to testing that all of them match, it would be helpful to see *which* attributes are reported. The test 'ls-files' demonstrates one way of doing that sort of test.

> +
> +	run_on_all git reset --hard &&
> +	test_all_match git check-attr -a  deep/test.c &&

Besides the things already noted in my earlier comments, these scenarios seem reasonable.

> +
> +	run_on_all git reset --hard &&
> +	test_all_match git check-attr -a  --cached deep/test.c

'deep/test.c' isn't in the index, so AFAICT this should return nothing. While the case of an untracked file is interesting, I think that would be *in addition to* a test on a file that exists in the index, e.g. 'deep/a'.

> +'> +
>  test_done