| Message ID | 20230227050543.218294-2-cheskaqiqi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | check-attr: integrate with sparse-index | expand |
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/ > 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
On 2/27/2023 12:18 PM, Victoria Dye wrote: > Shuqi Liang wrote: >> + 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. This change is part of the performance improvements given by sparse index, but the correctness test you've added only ensures that the end result is correct, not fast. It's possible that even with this change we hit an ensure_full_index() call somewhere in the call stack. To test that the sparse-index stays sparse throughout the process lifetime (when possible) create a test that uses the ensure_not_expanded helper. There are several examples in t1092 to use as a starting point. Victoria is right that it is helpful to first establish test coverage of the builtin for correctness reasons before making this change. It helps to add tests for cases that would require expanding the sparse index, such as checking the attributes for paths outside of the sparse-checkout cone. Once the correctness tests are in place, you can then make this change to the builtin and add the tests that check ensure_not_expanded, since the change at that point is _only_ that we are allowing the builtin to operate upon the sparse index without expanding it immediately after read. A good example of this "final step" is [1], which updates the builtin for 'git diff' as well as adding _only_ the ensure_not_expanded tests. [1] https://lore.kernel.org/git/897611682af64ba6bd0d2dfcfeae56cfe953c45e.1638806161.git.gitgitgadget@gmail.com/ [PATCH v6 6/7] diff: enable and test the sparse index You can look at the surrounding patches for other ideas, but it should be noted that both 'git diff' and 'git blame' in that series had previous correctness tests in t1092 that only needed _edits_, instead of being created from scratch. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: > Victoria is right that it is helpful to first establish test coverage of > the builtin for correctness reasons before making this change. It helps to > add tests for cases that would require expanding the sparse index, such as > checking the attributes for paths outside of the sparse-checkout cone. > > Once the correctness tests are in place, you can then make this change to > the builtin and add the tests that check ensure_not_expanded, since the > change at that point is _only_ that we are allowing the builtin to > operate upon the sparse index without expanding it immediately after read. > > A good example of this "final step" is [1], which updates the builtin for > 'git diff' as well as adding _only_ the ensure_not_expanded tests. Thanks for a detailed help.
Hi Victoria, Hi Derrick Thanks a lot for your valuable feedback! They are really helpful! These two days are very busy for me for preparing the midterm. After 1st March, I'll start to address them in the next roll or start to Investigation around 'git diff-files‘ as soon as possible .
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; + 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 && + 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 && + test_all_match echo "deep/test.c" | git check-attr --stdin -a && + + run_on_all git reset --hard && + test_all_match git check-attr -a deep/test.c && + + run_on_all git reset --hard && + test_all_match git check-attr -a --cached deep/test.c +' + test_done
Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> --- builtin/check-attr.c | 3 +++ t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+)