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@xxxxxxxxx> 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@xxxxxxxxx/ > 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. > + > 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. > + 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
