{"thread":{"id":"59316","subject":"[RFC PATCH 0/1] check-attr: integrate with sparse-index","startedAt":"2023-02-27T05:05:59Z","lastAt":"2023-02-28T18:10:36Z","messageCount":7,"participants":["Shuqi Liang","Victoria Dye","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"472773","messageId":"20230227050543.218294-1-cheskaqiqi@gmail.com","threadId":"59316","inReplyTo":null,"subject":"[RFC PATCH 0/1] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-02-27T05:05:42Z","receivedAt":"2023-02-27T05:05:59Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Integrate git check-attr with sparse-index.\n\nOnly add 'check-attr pathspec inside sparse definition' yet, just want to see \nif the added tests in this patch  on the right track.\nAlso, I have a question about git rev-parse. I read the documentation and wanted to add a test for it, \nbut I noticed that Derrick already integrated it with sparse-index last year. \nIs there anything else I can do with git rev-parse to integrate with sparse-index, or should we just leave it be? \n\nThanks!\n\nShuqi Liang (1):\n  test check-attr pathspec inside sparse definition\n\n builtin/check-attr.c                     |  3 +++\n t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \n2.39.0\n\n"},{"id":"472774","messageId":"20230227050543.218294-2-cheskaqiqi@gmail.com","threadId":"59316","inReplyTo":"20230227050543.218294-1-cheskaqiqi@gmail.com","subject":"[RFC PATCH 1/1] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-02-27T05:05:43Z","receivedAt":"2023-02-27T05:06:04Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>\n---\n builtin/check-attr.c                     |  3 +++\n t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex 0fef10eb6b..f85b91ebba 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -112,6 +112,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\t\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..b28010aa5c 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,23 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'check-attr pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\trun_on_all touch deep/test.c &&\n+\techo \"*.c diff=cpp -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes . &&\n+\ttest_all_match git add .gitattributes &&\n+\ttest_all_match git commit -m \"add .gitattributes\" &&\n+\t\n+\trun_on_all git reset --hard &&\n+\ttest_all_match echo \"deep/test.c\" | git check-attr --stdin  -a &&\n+\n+\trun_on_all git reset --hard &&\n+\ttest_all_match git check-attr -a  deep/test.c &&\n+\n+\trun_on_all git reset --hard &&\n+\ttest_all_match git check-attr -a  --cached deep/test.c \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"472804","messageId":"9fcd235e-9dcd-8b13-156b-800da9121b33@github.com","threadId":"59316","inReplyTo":"20230227050543.218294-1-cheskaqiqi@gmail.com","subject":"Re: [RFC PATCH 0/1] check-attr: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-02-27T17:18:30Z","receivedAt":"2023-02-27T17:18:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Integrate git check-attr with sparse-index.\n\nSince you're only looking for feedback on a test (one that theoretically\npasses without any sparse index changes), the title should reflect that.\nSomething like \"t1092: add a test for check-attr\".\n\nAlso, the line wrapping of the following lines is a bit strange. Please make\nsure to wrap to 72 characters, as it makes responding inline easier for\nreviewers. For the sake of commenting here, I've rewrapped them.\n\n> \n> Also, I have a question about git rev-parse. I read the documentation and\n> wanted to add a test for it, but I noticed that Derrick already integrated\n> it with sparse-index last year. Is there anything else I can do with git\n> rev-parse to integrate with sparse-index, or should we just leave it be? \n\nAh, sorry about that, I forgot to remove it. I don't think there's anything\nelse to do there, so we can leave it be.\n\nI also think the list on the SoC Ideas 2023 [1] is somewhat out of order\n(there are definitely some easier/almost trivial ones marked as \"harder\"\nthan some more involved ones) - I noticed last year, but never got around to\nupdating it. I'll try to submit a pull request to update the order and\nremove 'rev-parse' sometime today or tomorrow. Sorry about that!\n\nIn the meantime, I think the easiest commands will probably be 'git\ndescribe', 'git diff-files', and 'git diff-index' - feel free to switch to\none of those if you'd like. Of course, you're welcome to continue working on\n'check-attr' - it'll be a little more complicated, but I don't think it'll\nbe as bad as, say, 'git mv' was (which needed substantial changes to its\nsparse-checkout behavior before even starting the sparse index integration).\n\n[1] https://git.github.io/SoC-2023-Ideas/\n\n> \n> Thanks!\n> \n> Shuqi Liang (1):\n>   test check-attr pathspec inside sparse definition\n> \n>  builtin/check-attr.c                     |  3 +++\n>  t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++\n>  2 files changed, 22 insertions(+)\n> \n> \n> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n\n"},{"id":"472805","messageId":"315e70e0-ac1f-2f19-f1cc-6b8b24ffb1fc@github.com","threadId":"59316","inReplyTo":"20230227050543.218294-2-cheskaqiqi@gmail.com","subject":"Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-02-27T17:18:40Z","receivedAt":"2023-02-27T17:18:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n\nJust a heads-up, I was CC'd only on the cover letter (and not this patch). \n\n> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>\n\nEven in an RFC (or maybe *especially* in an RFC?), it's important to provide\nsome context around what you're doing in a patch/why you're doing it. It\nseems like you provided that information in your cover letter [1], though,\nso I think this \"series\" would be better off submitted as a single patch,\nwith the cover letter contents...\n\n> ---\n\n...right here! That is, below the '---' line but before the diff summary.\n\n[1] https://lore.kernel.org/git/20230227050543.218294-1-cheskaqiqi@gmail.com/\n\n>  builtin/check-attr.c                     |  3 +++\n>  t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++\n>  2 files changed, 22 insertions(+)\n> \n> diff --git a/builtin/check-attr.c b/builtin/check-attr.c\n> index 0fef10eb6b..f85b91ebba 100644\n> --- a/builtin/check-attr.c\n> +++ b/builtin/check-attr.c\n> @@ -112,6 +112,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n>  \n>  \tgit_config(git_default_config, NULL);\n>  \n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n\nThe test below doesn't do anything special related to the sparse index, so\nthis change is unnecessary (and, as far as I can tell, will break in some\nusage of 'git check-attr'). If you're only looking for feedback on testing,\nit'd better to leave this out.\n\n> +\t\n>  \targc = parse_options(argc, argv, prefix, check_attr_options,\n>  \t\t\t     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n>  \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 801919009e..b28010aa5c 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2055,4 +2055,23 @@ test_expect_success 'grep sparse directory within submodules' '\n>  \ttest_cmp actual expect\n>  '\n>  \n> +test_expect_success 'check-attr pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\trun_on_all touch deep/test.c &&\n> +\techo \"*.c diff=cpp -crlf myAttr\" >>.gitattributes &&\n\nIs there a specific reason you wanted to create a new file, rather than use\nsomething in the existing structure (e.g. 'deep/a'?). If not, I'd recommend\nusing the existing file structure setup by the test in the 'setup' test at\nthe beginning of the file.\n\n> +\trun_on_all cp ../.gitattributes . &&\n> +\ttest_all_match git add .gitattributes &&\n> +\ttest_all_match git commit -m \"add .gitattributes\" &&\n> +\t\n> +\trun_on_all git reset --hard &&\n\nUnless I'm missing something, there's nothing to 'reset' here? Same for the\nother 'reset's you have below. If they're not needed, they should be\nremoved.\n\n> +\ttest_all_match echo \"deep/test.c\" | git check-attr --stdin  -a &&\n\nIn addition to testing that all of them match, it would be helpful to see\n*which* attributes are reported. The test 'ls-files' demonstrates one way of\ndoing that sort of test.\n\n> +\n> +\trun_on_all git reset --hard &&\n> +\ttest_all_match git check-attr -a  deep/test.c &&\n\nBesides the things already noted in my earlier comments, these scenarios\nseem reasonable.\n\n> +\n> +\trun_on_all git reset --hard &&\n> +\ttest_all_match git check-attr -a  --cached deep/test.c \n\n'deep/test.c' isn't in the index, so AFAICT this should return nothing.\nWhile the case of an untracked file is interesting, I think that would be\n*in addition to* a test on a file that exists in the index, e.g. 'deep/a'.\n\n> +'> +\n>  test_done\n\n"},{"id":"472811","messageId":"cd7b9a2f-4f52-9da8-19c3-2c5cda7b8160@github.com","threadId":"59316","inReplyTo":"315e70e0-ac1f-2f19-f1cc-6b8b24ffb1fc@github.com","subject":"Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-02-27T17:57:32Z","receivedAt":"2023-02-27T17:57:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/27/2023 12:18 PM, Victoria Dye wrote:\n> Shuqi Liang wrote:\n\n>> +\tprepare_repo_settings(the_repository);\n>> +\tthe_repository->settings.command_requires_full_index = 0;\n> \n> The test below doesn't do anything special related to the sparse index, so\n> this change is unnecessary (and, as far as I can tell, will break in some\n> usage of 'git check-attr'). If you're only looking for feedback on testing,\n> it'd better to leave this out.\n\nThis change is part of the performance improvements given by sparse index,\nbut the correctness test you've added only ensures that the end result is\ncorrect, not fast.\n\nIt's possible that even with this change we hit an ensure_full_index() call\nsomewhere in the call stack. To test that the sparse-index stays sparse\nthroughout the process lifetime (when possible) create a test that uses the\nensure_not_expanded helper. There are several examples in t1092 to use as\na starting point.\n\nVictoria is right that it is helpful to first establish test coverage of\nthe builtin for correctness reasons before making this change. It helps to\nadd tests for cases that would require expanding the sparse index, such as\nchecking the attributes for paths outside of the sparse-checkout cone.\n\nOnce the correctness tests are in place, you can then make this change to\nthe builtin and add the tests that check ensure_not_expanded, since the\nchange at that point is _only_ that we are allowing the builtin to\noperate upon the sparse index without expanding it immediately after read.\n\nA good example of this \"final step\" is [1], which updates the builtin for\n'git diff' as well as adding _only_ the ensure_not_expanded tests.\n\n[1] https://lore.kernel.org/git/897611682af64ba6bd0d2dfcfeae56cfe953c45e.1638806161.git.gitgitgadget@gmail.com/\n    [PATCH v6 6/7] diff: enable and test the sparse index\n\nYou can look at the surrounding patches for other ideas, but it should be\nnoted that both 'git diff' and 'git blame' in that series had previous\ncorrectness tests in t1092 that only needed _edits_, instead of being\ncreated from scratch.\n\nThanks,\n-Stolee\n"},{"id":"472812","messageId":"xmqqedqbt00v.fsf@gitster.g","threadId":"59316","inReplyTo":"cd7b9a2f-4f52-9da8-19c3-2c5cda7b8160@github.com","subject":"Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-27T18:51:12Z","receivedAt":"2023-02-27T18:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n> Victoria is right that it is helpful to first establish test coverage of\n> the builtin for correctness reasons before making this change. It helps to\n> add tests for cases that would require expanding the sparse index, such as\n> checking the attributes for paths outside of the sparse-checkout cone.\n>\n> Once the correctness tests are in place, you can then make this change to\n> the builtin and add the tests that check ensure_not_expanded, since the\n> change at that point is _only_ that we are allowing the builtin to\n> operate upon the sparse index without expanding it immediately after read.\n>\n> A good example of this \"final step\" is [1], which updates the builtin for\n> 'git diff' as well as adding _only_ the ensure_not_expanded tests.\n\nThanks for a detailed help.\n"},{"id":"472851","messageId":"CAMO4yUF4Z1R_eQJw2AW825DjSjVH5W=u3wn0PYdQt27rN2-XPg@mail.gmail.com","threadId":"59316","inReplyTo":"cd7b9a2f-4f52-9da8-19c3-2c5cda7b8160@github.com","subject":"Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-02-28T18:10:19Z","receivedAt":"2023-02-28T18:10:36Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi  Victoria, Hi  Derrick\n\nThanks a lot for your valuable feedback!  They are really helpful!\n\nThese two days are very busy for me for preparing the midterm. After\n1st  March, I'll start to address them in the next roll or start to\nInvestigation around 'git diff-files‘  as soon as possible .\n\n\n\n-- \nThanks & Regards,\nShuqi\n\nOn Mon, Feb 27, 2023 at 12:57 PM Derrick Stolee\n<derrickstolee@github.com> wrote:\n>\n> On 2/27/2023 12:18 PM, Victoria Dye wrote:\n> > Shuqi Liang wrote:\n>\n> >> +    prepare_repo_settings(the_repository);\n> >> +    the_repository->settings.command_requires_full_index = 0;\n> >\n> > The test below doesn't do anything special related to the sparse index, so\n> > this change is unnecessary (and, as far as I can tell, will break in some\n> > usage of 'git check-attr'). If you're only looking for feedback on testing,\n> > it'd better to leave this out.\n>\n> This change is part of the performance improvements given by sparse index,\n> but the correctness test you've added only ensures that the end result is\n> correct, not fast.\n>\n> It's possible that even with this change we hit an ensure_full_index() call\n> somewhere in the call stack. To test that the sparse-index stays sparse\n> throughout the process lifetime (when possible) create a test that uses the\n> ensure_not_expanded helper. There are several examples in t1092 to use as\n> a starting point.\n>\n> Victoria is right that it is helpful to first establish test coverage of\n> the builtin for correctness reasons before making this change. It helps to\n> add tests for cases that would require expanding the sparse index, such as\n> checking the attributes for paths outside of the sparse-checkout cone.\n>\n> Once the correctness tests are in place, you can then make this change to\n> the builtin and add the tests that check ensure_not_expanded, since the\n> change at that point is _only_ that we are allowing the builtin to\n> operate upon the sparse index without expanding it immediately after read.\n>\n> A good example of this \"final step\" is [1], which updates the builtin for\n> 'git diff' as well as adding _only_ the ensure_not_expanded tests.\n>\n> [1] https://lore.kernel.org/git/897611682af64ba6bd0d2dfcfeae56cfe953c45e.1638806161.git.gitgitgadget@gmail.com/\n>     [PATCH v6 6/7] diff: enable and test the sparse index\n>\n> You can look at the surrounding patches for other ideas, but it should be\n> noted that both 'git diff' and 'git blame' in that series had previous\n> correctness tests in t1092 that only needed _edits_, instead of being\n> created from scratch.\n>\n> Thanks,\n> -Stolee\n"}]}