Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index
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
Shuqi Liang
