{"thread":{"id":"59340","subject":"[RFC][PATCH] t1092: add tests for `git diff-files`","startedAt":"2023-03-04T02:58:13Z","lastAt":"2023-05-11T05:05:00Z","messageCount":73,"participants":["Shuqi Liang","Derrick Stolee","Junio C Hamano","Victoria Dye"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472989","messageId":"20230304025740.107830-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":null,"subject":"[RFC][PATCH] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-04T02:57:40Z","receivedAt":"2023-03-04T02:58:13Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"To make sure git diff-files behaves as expected when\ninside or outside of sparse-checkout definition.\n\nAdd test for git diff-files:\nPath is within sparse-checkout cone\nPath is outside sparse-checkout cone\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 32 ++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..f4815c619a 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2054,5 +2054,37 @@ test_expect_success 'grep sparse directory within submodules' '\n \tgit grep --cached --recurse-submodules a -- \"*/folder1/*\" >actual &&\n \ttest_cmp actual expect\n '\n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files deep/a &&\n+\ttest_all_match git diff-files --find-object=HEAD:a\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\trun_on_sparse mkdir newdirectory &&\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git sparse-checkout set newdirectory &&\n+\ttest_sparse_match git add newdirectory/testfile &&\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git sparse-checkout set &&\n+\n+\ttest_sparse_match git diff-files &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile\n+'\n \n test_done\n-- \n2.39.0\n\n"},{"id":"473058","messageId":"99252618-28a9-6aa7-880a-f8ab0714bbb9@github.com","threadId":"59340","inReplyTo":"20230304025740.107830-1-cheskaqiqi@gmail.com","subject":"Re: [RFC][PATCH] t1092: add tests for `git diff-files`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-06T14:14:35Z","receivedAt":"2023-03-06T14:17:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/3/2023 9:57 PM, Shuqi Liang wrote:\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2054,5 +2054,37 @@ test_expect_success 'grep sparse directory within submodules' '\n> \tgit grep --cached --recurse-submodules a -- \"*/folder1/*\" >actual &&\n> \ttest_cmp actual expect\n> '\n> +test_expect_success 'diff-files with pathspec inside sparse definition' '\n\nnit: you need an empty line between the previous test's closing quote\nand the start of your new test.\n\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>$1\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files deep/a &&\n> +\ttest_all_match git diff-files --find-object=HEAD:a\n> +'\n> +\n> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>$1\n> +\tEOF\n> +\n> +\trun_on_sparse mkdir newdirectory &&\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git sparse-checkout set newdirectory &&\n> +\ttest_sparse_match git add newdirectory/testfile &&\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git sparse-checkout set &&\n\nThese uses of 'git sparse-checkout set' are probably not necessary\nif you use \"git add --sparse newdirectory/testfile\". It does present\nan interesting modification of your test case: what if the file\nexists on-disk but outside of the sparse-checkout definition? What\nhappens in each case? What if the file is different from the staged\nversion?\n\n> +\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile\n> +'\n\nThese tests look like a good start here. I was first confused as\nto why you were doing such steps to modify the sparse-checkout\ndefinition, but I see it is critical that you have staged changes\noutside of the sparse-checkout cone. These kinds of details, the\n\"why\" you are doing subtle things, are great to add to the commit\nmessage.\n\n> To make sure git diff-files behaves as expected when\n> inside or outside of sparse-checkout definition.\n> \n> Add test for git diff-files:\n> Path is within sparse-checkout cone\n> Path is outside sparse-checkout cone\n\nWith that in mind, here is a way you could edit your commit\nmessage to be more informative:\n\n Before integrating the 'git diff-files' builtin with the sparse\n index feature, add tests to t1092-sparse-checkout-compatibility.sh\n to ensure it currently works with sparse-checkout and will still\n work with sparse index after that integration.\n\n When adding tests against a sparse-checkout definition, we must\n test two modes: all changes are within the sparse-checkout cone\n and some changes are outside the sparse-checkout cone. In order to\n have staged changes outside of the sparse-checkout cone, create a\n 'newdirectory/testfile' and add it to the index, while leaving it\n outside of the sparse-checkout definition.\n\n(If you decide to add tests for the case of 'newdirectory/testfile'\nbeing present on-disk with or without modifications, then you can\nexpand your commit message to include details about those tests,\ntoo.)\n\nThanks,\n-Stolee\n\n"},{"id":"473110","messageId":"20230307065813.77059-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230304025740.107830-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-07T06:58:11Z","receivedAt":"2023-03-07T06:59:35Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Turn on sparse-index feature within `git diff-files` command.\nAdd necessary modifications and test them.\n\nChanges since v1:\n\n1.Add an empty line between the previous test's closing quote\nand the start of new test.\n\n2.Use \"git add --sparse newdirectory/testfile\" instead of \n'git sparse-checkout set' to have staged changes outside \nof the sparse-checkout cone\n\n3.Edit commit message to be more informative\n\n(sorry to send this patch twice ,I forgot to --inreply-to the origin one)\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 52 ++++++++++++++++++++++++\n 3 files changed, 58 insertions(+)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \n2.39.0\n\n"},{"id":"473111","messageId":"20230307065813.77059-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230307065813.77059-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-07T06:58:13Z","receivedAt":"2023-03-07T07:00:05Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`\nand test to ensure the index is not expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 14 ++++++++++++++\n 3 files changed, 20 insertions(+)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..82751f2ca3 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 9382428352..7cc02b882b 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2093,4 +2093,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files --find-object=HEAD:a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473112","messageId":"20230307065813.77059-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230307065813.77059-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-07T06:58:12Z","receivedAt":"2023-03-07T07:00:05Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin\nwith the sparse index feature, add tests to\nt1092-sparse-checkout-compatibility.sh to ensure it currently works\nwith sparse-checkout and will still work with sparse index\nafter that integration.\n\nWhen adding tests against a sparse-checkout\ndefinition, we test two modes: all changes are\nwithin the sparse-checkout cone and some changes are outside\nthe sparse-checkout cone.\n\nIn order to have staged changes outside of\nthe sparse-checkout cone, create a 'newdirectory/testfile' and\nadd it to the index, while leaving it outside of\nthe sparse-checkout definition.Test 'newdirectory/testfile'\nbeing present on-disk without modifications, then change content inside\n'newdirectory/testfile' in order to test 'newdirectory/testfile'\nbeing present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 38 ++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..9382428352 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,42 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files deep/a &&\n+\ttest_all_match git diff-files --find-object=HEAD:a\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\n+\t#add file to the index but outside of cone\n+\trun_on_sparse mkdir newdirectory &&\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git add --sparse newdirectory/testfile &&\n+\n+\t#file present on-disk without modifications\n+\ttest_sparse_match git diff-files &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile &&\n+\n+\t#file present on-disk with modifications\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git diff-files &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473147","messageId":"xmqqy1o8xuis.fsf@gitster.g","threadId":"59340","inReplyTo":"20230307065813.77059-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v2 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T18:53:47Z","receivedAt":"2023-03-07T19:09:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 801919009e..9382428352 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2055,4 +2055,42 @@ test_expect_success 'grep sparse directory within submodules' '\n> \ttest_cmp actual expect\n> '\n> \n> +test_expect_success 'diff-files with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>$1\n> +\tEOF\n\n(Documentation/CodingGuidelines)\n\n - Redirection operators should be written with space before, but no\n space after them. In other words, write 'echo test >\"$file\"'\n instead of 'echo test> $file' or 'echo test > $file'. Note that\n even though it is not required by POSIX to double-quote the\n redirection target in a variable (as shown above), our code does so\n because some versions of bash issue a warning without the quotes.\n\n> +\t#add file to the index but outside of cone\n\nCan you have a SP after \"#\" here to make it more readable?\n\n> +\trun_on_sparse mkdir newdirectory &&\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git add --sparse newdirectory/testfile &&\n\nWe create a new directory that is outside the cone, with or without\nusing the sparse-index feature. We know we are violating the cone,\nand have to override the safety with the \"--sparse\" option. OK.\n\nWhat output do we expect out of \"git add\" to match in the two cases?\n\n> +\t#file present on-disk without modifications\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n\nAs \"diff-files\" is about comparing between the index and the working\ntree, the new path should not appear in the output when the sparse\ncheckout feature with or without the sparse-index feature is NOT in\nuse. Does the picture get different when we are sparse? IOW, would\nwe notice that we now have newdirectory/testfile that is supposed to\nbe missing in the index and show that in the output?\n\n> +\ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile &&\n\nWhat does HEAD:testfile refer to in this test? This expects \"diff-files\"\ninvocation to fail, and perhaps in your test it failed in both test\nrepositories the same way, but are they failing for the right reason?\n\nIn a non-sparse repository whose HEAD commit does not have\n'testfile' (e.g. \"git\" source tree), I get\n\n $ git diff-files --find-object=HEAD:testfile\n error: unable to resolve 'HEAD:testfile'\n\nwithout sparse checkout or sparse index. It is unclear what value\nwe get out of having this test here.\n\n> +\t#file present on-disk with modifications\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\ttest_sparse_match test_must_fail git diff-files --find-object=HEAD:testfile\n\nDitto.\n\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"473226","messageId":"CAMO4yUHEDtZfu+NgsWNjckxAun9kU+8GoyB_poWT8Lam095Wtw@mail.gmail.com","threadId":"59340","inReplyTo":"xmqqy1o8xuis.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-08T22:04:52Z","receivedAt":"2023-03-08T22:05:10Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Junio\n\nOn Tue, Mar 7, 2023 at 1:53 PM Junio C Hamano wrote:\n\n> (Documentation/CodingGuidelines)\n>\n> - Redirection operators should be written with space before, but no\n> space after them. In other words, write 'echo test >\"$file\"'\n> instead of 'echo test> $file' or 'echo test > $file'. Note that\n> even though it is not required by POSIX to double-quote the\n> redirection target in a variable (as shown above), our code does so\n> because some versions of bash issue a warning without the quotes.\n\n\nThanks for the styling reminders! I should go back and reread CodingGuidelines\nmore often.\n\n\n> > + #add file to the index but outside of cone\n>\n> Can you have a SP after \"#\" here to make it more readable?\n\nWill do!\n\n\n> We create a new directory that is outside the cone, with or without\n> using the sparse-index feature. We know we are violating the cone,\n> and have to override the safety with the \"--sparse\" option. OK.\n>\n> What output do we expect out of \"git add\" to match in the two cases?\n>\n> > + #file present on-disk without modifications\n> > + test_sparse_match git diff-files &&\n> > + test_sparse_match git diff-files newdirectory/testfile &&\n>\n> As \"diff-files\" is about comparing between the index and the working\n> tree, the new path should not appear in the output when the sparse\n> checkout feature with or without the sparse-index feature is NOT in\n> use. Does the picture get different when we are sparse? IOW, would\n> we notice that we now have newdirectory/testfile that is supposed to\n> be missing in the index and show that in the output?\n\nI'm a bit caught up here.\nDo you mean I need to add a test for \"git add\" also?\n\nwhen we use \"git add\" instead of \"git add --sparse \", we will get different.\nCause newdirectory/testfile is missing in the index so diff-files will not\nwork in these cases.\n\n\n> In a non-sparse repository whose HEAD commit does not have\n> 'testfile' (e.g. \"git\" source tree), I get\n>\n> $ git diff-files --find-object=HEAD:testfile\n> error: unable to resolve 'HEAD:testfile'\n>\n> without sparse checkout or sparse index. It is unclear what value\n> we get out of having this test here.\n\nThanks for pointing out the error. HEAD:testfile is useless for the test here.\n\n-----------------------------------\nThanks\nShuqi\n"},{"id":"473233","messageId":"xmqqbkl2sw8o.fsf@gitster.g","threadId":"59340","inReplyTo":"CAMO4yUHEDtZfu+NgsWNjckxAun9kU+8GoyB_poWT8Lam095Wtw@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-08T22:40:07Z","receivedAt":"2023-03-08T22:40:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n>> We create a new directory that is outside the cone, with or without\n>> using the sparse-index feature. We know we are violating the cone,\n>> and have to override the safety with the \"--sparse\" option. OK.\n>>\n>> What output do we expect out of \"git add\" to match in the two cases?\n>>\n>> > + #file present on-disk without modifications\n>> > + test_sparse_match git diff-files &&\n>> > + test_sparse_match git diff-files newdirectory/testfile &&\n>>\n>> As \"diff-files\" is about comparing between the index and the working\n>> tree, the new path should not appear in the output when the sparse\n>> checkout feature with or without the sparse-index feature is NOT in\n>> use. Does the picture get different when we are sparse? IOW, would\n>> we notice that we now have newdirectory/testfile that is supposed to\n>> be missing in the index and show that in the output?\n>\n> I'm a bit caught up here.\n> Do you mean I need to add a test for \"git add\" also?\n\nNot really. The above two tests are happy with _any_ output coming\nout of \"git diff-files\" (and \"git diff-files nd/tf\") as long as they\nmatch between sparse checkouts, one of which uses and the other does\nnot use the sparse index feature. I was wondering if we want to be\na bit stricter than that. Thinks like \"not only the two output must\nmatch, they both must be empty\".\n"},{"id":"473245","messageId":"20230309013314.119128-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230307065813.77059-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T01:33:12Z","receivedAt":"2023-03-09T01:33:38Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v2:\n\n1. According to Documentation/CodingGuidelines\n\n write 'echo test >\"$1\"'\n instead of 'echo test> $1'\n\n2. Add a SP after \"#\" to make sentence more readable.\n\n3. When file present on-disk without modifications, add\ntest to make sure not only the two output must\nmatch, they both must be empty.\n\n4. Remove the useless test for HEAD:testfile.\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 53 ++++++++++++++++++++++++\n 3 files changed, 59 insertions(+)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \n2.39.0\n\n"},{"id":"473246","messageId":"20230309013314.119128-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230309013314.119128-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T01:33:13Z","receivedAt":"2023-03-09T01:33:42Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin\nwith the sparse index feature, add tests to\nt1092-sparse-checkout-compatibility.sh to ensure it currently works\nwith sparse-checkout and will still work with sparse index\nafter that integration.\n\nWhen adding tests against a sparse-checkout\ndefinition, we test two modes: all changes are\nwithin the sparse-checkout cone and some changes are outside\nthe sparse-checkout cone.\n\nIn order to have staged changes outside of\nthe sparse-checkout cone, create a 'newdirectory/testfile' and\nadd it to the index, while leaving it outside of\nthe sparse-checkout definition.Test 'newdirectory/testfile'\nbeing present on-disk without modifications, then change content inside\n'newdirectory/testfile' in order to test 'newdirectory/testfile'\nbeing present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..bdf3cf25d4 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,44 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files deep/a \n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# add file to the index but outside of cone\n+\trun_on_sparse mkdir newdirectory &&\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git add --sparse newdirectory/testfile &&\n+\n+\t# file present on-disk without modifications\n+\ttest_sparse_match git diff-files &&\n+\t! test_file_not_empty sparse-checkout-out &&\n+\t! test_file_not_empty sparse-index-out &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\t! test_file_not_empty sparse-checkout-out &&\n+\t! test_file_not_empty sparse-index-out &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git diff-files &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_file_not_empty sparse-checkout-out\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473247","messageId":"20230309013314.119128-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230309013314.119128-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T01:33:14Z","receivedAt":"2023-03-09T01:33:43Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`\nand test to ensure the index is not expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..82751f2ca3 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex bdf3cf25d4..bc26c2b82a 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2095,4 +2095,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_file_not_empty sparse-checkout-out\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473248","messageId":"xmqqlek6pr2b.fsf@gitster.g","threadId":"59340","inReplyTo":"20230309013314.119128-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-09T03:00:12Z","receivedAt":"2023-03-09T03:00:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> +\t! test_file_not_empty sparse-checkout-out &&\n\nIf you looked at existing uses of this test helper, you probably\nwouldn't have written this.\n\n $ git grep -e test_file_not_empty t/t[0-9]\\*.sh | wc -l\n 34\n $ git grep -e '! test_file_not_empty' t/t[0-9]\\*.sh | wc -l\n 0\n\nThis is because test_file_not_empty is designed to fail loudly with\na complaint when the given file is empty. Its implementation reads\nlike so:\n\n # Check if the file exists and has a size greater than zero\n test_file_not_empty () {\n test \"$#\" = 2 && BUG \"2 param\"\n if ! test -s \"$1\"\n then\n echo \"'$1' is not a non-empty file.\"\n false\n fi\n }\n\nIn the successful case in your test, you expect the file to be empty\n(I didn't check if it should be empty or not---I am just taking your\nword for it). It means that the \"! test_file_not_empty\" is expected\nto keep complaining that it is NOT a non-empty file.\n\nNot very nice, no?\n\nPerhaps test_must_be_empty is what you wanted to use.\n\n> +\t! test_file_not_empty sparse-index-out &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\t! test_file_not_empty sparse-checkout-out &&\n> +\t! test_file_not_empty sparse-index-out &&\n> +\n> +\t# file present on-disk with modifications\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\ttest_file_not_empty sparse-checkout-out\n\nNow, are we happy if the file is not empty and has any garbage in\nit? Don't we know what the list of different paths are and what the\ncommon output between the two should look like?\n\nIn general \"as long as it is not empty, any garbage is fine\" is a\npoor primitive to use in tests, unless (1) we are testing output\nthat is deliberately designed to be unstable, or (2) we know the\nprogram that produces output will always show an empty result when\nit fails in any way.\n\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"473261","messageId":"20230309063952.42362-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230309013314.119128-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T06:39:50Z","receivedAt":"2023-03-09T06:40:22Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v3:\n\n1.Use 'test_must_be_empty' instead of '! test_file_not_empty'\n\n2.remove useless 'test_file_not_empty sparse-checkout-out' \nin 'file present on-disk with modifications'\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 52 ++++++++++++++++++++++++\n 3 files changed, 58 insertions(+)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \n2.39.0\n\n"},{"id":"473262","messageId":"20230309063952.42362-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230309063952.42362-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T06:39:52Z","receivedAt":"2023-03-09T06:40:25Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`\nand test to ensure the index is not expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..82751f2ca3 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 18d3b4f313..7cc6287627 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2094,4 +2094,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_sparse_match git diff-files newdirectory/testfile \n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473263","messageId":"20230309063952.42362-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230309063952.42362-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T06:39:51Z","receivedAt":"2023-03-09T06:40:33Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin\nwith the sparse index feature, add tests to\nt1092-sparse-checkout-compatibility.sh to ensure it currently works\nwith sparse-checkout and will still work with sparse index\nafter that integration.\n\nWhen adding tests against a sparse-checkout\ndefinition, we test two modes: all changes are\nwithin the sparse-checkout cone and some changes are outside\nthe sparse-checkout cone.\n\nIn order to have staged changes outside of\nthe sparse-checkout cone, create a 'newdirectory/testfile' and\nadd it to the index, while leaving it outside of\nthe sparse-checkout definition.Test 'newdirectory/testfile'\nbeing present on-disk without modifications, then change content inside\n'newdirectory/testfile' in order to test 'newdirectory/testfile'\nbeing present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..18d3b4f313 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,43 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files deep/a \n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# add file to the index but outside of cone\n+\trun_on_sparse mkdir newdirectory &&\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git add --sparse newdirectory/testfile &&\n+\n+\t# file present on-disk without modifications\n+\ttest_sparse_match git diff-files &&\n+\ttest_must_be_empty sparse-checkout-out &&\n+\ttest_must_be_empty sparse-index-out &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_must_be_empty sparse-checkout-out &&\n+\ttest_must_be_empty sparse-index-out &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git diff-files &&\n+\ttest_sparse_match git diff-files newdirectory/testfile \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473297","messageId":"xmqqmt4lc03s.fsf@gitster.g","threadId":"59340","inReplyTo":"20230309063952.42362-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v4 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-09T17:20:55Z","receivedAt":"2023-03-09T17:21:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\ttest_all_match git diff-files &&\n\nAn extra space on this line.\n\n> +\ttest_all_match git diff-files deep/a \n\nAnd on this line.\n\nNo need to resend only to correct the above two, but if you are\ngoing to reroll to fix something else, please make sure fixing\nthem.\n\n> +'\n> +\n> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\t# add file to the index but outside of cone\n> +\trun_on_sparse mkdir newdirectory &&\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git add --sparse newdirectory/testfile &&\n> +\n> +\t# file present on-disk without modifications\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_must_be_empty sparse-checkout-out &&\n> +\ttest_must_be_empty sparse-index-out &&\n\nAs output from checkout and index are known to be identical (that is\none of the things that test_sparse_match does), I do not think there\nis much point checking -out from both sides.\n\nIf we know \"diff-files\" invocation above should never send anything\nto the standard error, then checking that sparse-checkout-err is\nempty may have value, though.\n\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\ttest_must_be_empty sparse-checkout-out &&\n> +\ttest_must_be_empty sparse-index-out &&\n\nDitto.\n\n> +\t# file present on-disk with modifications\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile \n\nWe do not care what the actual output is in this case?\n\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"473327","messageId":"CAMO4yUFs5zSafO1pGFZqBU9R58G8ENhfTh5qNayeFMRPrCa+Jg@mail.gmail.com","threadId":"59340","inReplyTo":"xmqqmt4lc03s.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-09T23:21:04Z","receivedAt":"2023-03-09T23:21:21Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Junio\n\nOn Thu, Mar 9, 2023 at 12:20 PM Junio C Hamano wrote:\n\n>\n> > + run_on_all ../edit-contents deep/a &&\n> > +\n> > + test_all_match git diff-files &&\n>\n> An extra space on this line.\n>\n> > + test_all_match git diff-files deep/a\n>\n> And on this line.\n\nWill do !\n\n\n> As output from checkout and index are known to be identical (that is\n> one of the things that test_sparse_match does), I do not think there\n> is much point checking -out from both sides.\n>\n> If we know \"diff-files\" invocation above should never send anything\n> to the standard error, then checking that sparse-checkout-err is\n> empty may have value, though.\n\nAgree!\n\n> > + # file present on-disk with modifications\n> > + run_on_sparse ../edit-contents newdirectory/testfile &&\n> > + test_sparse_match git diff-files &&\n> > + test_sparse_match git diff-files newdirectory/testfile\n>\n> We do not care what the actual output is in this case?\n\nI wonder if the method below is good to test the actual output for '\nfile present on-disk with modifications' :\n\n cat >expect <<-EOF &&\n :100644 100644 8e27be7d6154a1f68ea9160ef0e18691d20560dc\n0000000000000000000000000000000000000000 M newdirectory/testfile\n EOF\n\n # file present on-disk with modifications\n run_on_sparse ../edit-contents newdirectory/testfile &&\n test_sparse_match git diff-files &&\n test_cmp expect sparse-checkout-out &&\n test_sparse_match git diff-files newdirectory/testfile &&\n test_cmp expect sparse-checkout-out\n\n-------------------\nThanks,\nShuqi\n"},{"id":"473329","messageId":"xmqq356d8pdg.fsf@gitster.g","threadId":"59340","inReplyTo":"CAMO4yUFs5zSafO1pGFZqBU9R58G8ENhfTh5qNayeFMRPrCa+Jg@mail.gmail.com","subject":"Re: [PATCH v4 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-09T23:40:59Z","receivedAt":"2023-03-09T23:41:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> I wonder if the method below is good to test the actual output for '\n> file present on-disk with modifications' :\n>\n> cat >expect <<-EOF &&\n> :100644 100644 8e27be7d6154a1f68ea9160ef0e18691d20560dc\n> 0000000000000000000000000000000000000000 M newdirectory/testfile\n> EOF\n\nHardcoding 8e27be assumes we only support SHA-1 but there is a CI\ntest job that runs everything in SHA-256 mode, so it is likely\nto break if you wrote it like so. Something along the lines of ...\n\n\tFN=newdirectory/testfile &&\n\tOID=$(git hash-object $FN) &&\n\tZERO=$(test_oid zero) &&\n\techo \":100644 100644 $OID $ZERO M new $FN\" >expect\n\n... may have a better chance to be correct, but I didn't test ;-)\n"},{"id":"473333","messageId":"20230310050021.123769-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230309063952.42362-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-10T05:00:19Z","receivedAt":"2023-03-10T05:00:43Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v4:\n\n1. In 'diff-files with pathspec inside sparse definition' '\n\nAdd some extra space to make test more readable.\n\n2.In 'diff-files with pathspec outside sparse definition' '\n\n(1)\nRemove redundant tests since output from checkout and index \nare known to be identical. \n\n(2)\nAdd test to check \nsparse-checkout-err (sparse-index-err) is empty.\n\n(3)\nAdd test to test the actual output for\n'file present on-disk with modifications'.\n\n\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 61 ++++++++++++++++++++++++\n 3 files changed, 67 insertions(+)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \n2.39.0\n\n"},{"id":"473334","messageId":"20230310050021.123769-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230310050021.123769-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-10T05:00:20Z","receivedAt":"2023-03-10T05:00:45Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin\nwith the sparse index feature, add tests to\nt1092-sparse-checkout-compatibility.sh to ensure it currently works\nwith sparse-checkout and will still work with sparse index\nafter that integration.\n\nWhen adding tests against a sparse-checkout\ndefinition, we test two modes: all changes are\nwithin the sparse-checkout cone and some changes are outside\nthe sparse-checkout cone.\n\nIn order to have staged changes outside of\nthe sparse-checkout cone, create a 'newdirectory/testfile' and\nadd it to the index, while leaving it outside of\nthe sparse-checkout definition.Test 'newdirectory/testfile'\nbeing present on-disk without modifications, then change content inside\n'newdirectory/testfile' in order to test 'newdirectory/testfile'\nbeing present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..9b71d7f5f9 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,52 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a \n+\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# add file to the index but outside of cone\n+\trun_on_sparse mkdir newdirectory &&\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git add --sparse newdirectory/testfile &&\n+\n+\t# file present on-disk without modifications\n+\ttest_sparse_match git diff-files &&\n+\ttest_must_be_empty sparse-checkout-out &&\n+\ttest_must_be_empty sparse-checkout-err &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_must_be_empty sparse-checkout-out &&\n+\ttest_must_be_empty sparse-checkout-err &&\n+\n+\t# file present on-disk with modifications\n+\tFN=newdirectory/testfile &&\n+\tOID=$(git -C sparse-checkout hash-object $FN) &&\n+\tZERO=$(test_oid zero) &&\n+\techo \":100644 100644 $OID $ZERO M\t$FN\" >expect &&\n+\n+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n+\ttest_sparse_match git diff-files &&\n+\ttest_cmp expect sparse-checkout-out &&\n+\ttest_sparse_match git diff-files newdirectory/testfile &&\n+\ttest_cmp expect sparse-checkout-out\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473335","messageId":"20230310050021.123769-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230310050021.123769-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-10T05:00:21Z","receivedAt":"2023-03-10T05:00:52Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`\nand test to ensure the index is not expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..82751f2ca3 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 9b71d7f5f9..4f582164a3 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2103,4 +2103,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_cmp expect sparse-checkout-out\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473374","messageId":"c5a32703-29a7-4e9f-f669-6a017ffdce60@github.com","threadId":"59340","inReplyTo":"20230310050021.123769-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v5 2/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-10T18:23:57Z","receivedAt":"2023-03-10T18:24:18Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Remove full index requirement for `git diff-files`\n> and test to ensure the index is not expanded in `git diff-files`.\n> \n> The `p2000` tests demonstrate a ~96% execution time reduction for 'git\n> diff-files' and a ~97% execution time reduction for 'git diff-files'\n> for a file using a sparse index:\n> \n> Test before after\n> -----------------------------------------------------------------\n> 2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n> 2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n> 2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n> 2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n> 2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n> 2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n> 2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n> 2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nThese are great performance results!\n\n> \n> Signed-off-by: Shuqi Liang \n> ---\n> builtin/diff-files.c | 4 ++++\n> t/perf/p2000-sparse-operations.sh | 2 ++\n> t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++++\n> 3 files changed, 19 insertions(+)\n> \n> diff --git a/builtin/diff-files.c b/builtin/diff-files.c\n> index dc991f753b..360464e6ef 100644\n> --- a/builtin/diff-files.c\n> +++ b/builtin/diff-files.c\n> @@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n> \t\tusage(diff_files_usage);\n> \n> \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> +\n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n\nLooks good.\n\n> +\n> \trepo_init_revisions(the_repository, &rev, prefix);\n> \trev.abbrev = 0;\n> \n> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\n> index 3242cfe91a..82751f2ca3 100755\n> --- a/t/perf/p2000-sparse-operations.sh\n> +++ b/t/perf/p2000-sparse-operations.sh\n> @@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n> test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n> test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n> test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n> +test_perf_on_all git diff-files\n> +test_perf_on_all git diff-files $SPARSE_CONE/a\n> \n> test_done\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 9b71d7f5f9..4f582164a3 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2103,4 +2103,17 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n> \ttest_cmp expect sparse-checkout-out\n> '\n> \n> +test_expect_success 'sparse index is not expanded: diff-files' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\tensure_not_expanded diff-files &&\n> +\tensure_not_expanded diff-files deep/a \n\nIIRC, in many cases, the internal diff machinery won't expand a sparse index\neven if the pathspec matches files outside the sparse-checkout definition.\nDoes 'ensure_not_expanded diff-files folder1/a' work? What about\n'ensure_not_expanded diff-files \"*a\"'?\n\n> +'\n> +\n> test_done\n\n"},{"id":"473375","messageId":"b537d855-edb7-4f67-de08-d651868247a5@github.com","threadId":"59340","inReplyTo":"20230310050021.123769-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-10T18:23:51Z","receivedAt":"2023-03-10T18:24:20Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n\nHi Shuqi! \n\nApologies for taking so long to review; thanks for working on this.\n\n> Before integrating the 'git diff-files' builtin\n> with the sparse index feature, add tests to\n> t1092-sparse-checkout-compatibility.sh to ensure it currently works\n> with sparse-checkout and will still work with sparse index\n> after that integration.\n> \n> When adding tests against a sparse-checkout\n> definition, we test two modes: all changes are\n> within the sparse-checkout cone and some changes are outside\n> the sparse-checkout cone.\n> \n> In order to have staged changes outside of\n> the sparse-checkout cone, create a 'newdirectory/testfile' and\n> add it to the index, while leaving it outside of\n> the sparse-checkout definition.Test 'newdirectory/testfile'\n\nnit: missing space after \"definition.\"\n\n> being present on-disk without modifications, then change content inside\n> 'newdirectory/testfile' in order to test 'newdirectory/testfile'\n> being present on-disk with modifications.\n\nGenerally, you don't need to create a new file to get to this state, and I'd\npersonally advise against it. Personally, I find that adding a new file can\nmake the test more cumbersome than is necessary. I'll see if I can suggest\nan alternative later on.\n\n> \n> Signed-off-by: Shuqi Liang \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++\n> 1 file changed, 48 insertions(+)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 801919009e..9b71d7f5f9 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2055,4 +2055,52 @@ test_expect_success 'grep sparse directory within submodules' '\n> \ttest_cmp actual expect\n> '\n> \n> +test_expect_success 'diff-files with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\ttest_all_match git diff-files &&\n> +\n> +\ttest_all_match git diff-files deep/a \n> +\n\nI'd be interested in seeing an additional test case for a pathspec with\nwildcards or other \"magic\" [1], e.g. 'git diff-files \"deep/*\"'. In past\nsparse index integrations, there has occasionally been a need for special\nhandling of those types of pathspecs [2][3], so it would be good for the\ntest to cover cases like that.\n\nOtherwise, this test looks good.\n\n[1] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec\n[2] https://lore.kernel.org/git/822d7344587f698e73abba1ca726c3a905f7b403.1638201164.git.gitgitgadget@gmail.com/\n[3] https://lore.kernel.org/git/20220807041335.1790658-3-shaoxuan.yuan02@gmail.com/\n\n> +'\n> +\n> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n\nBefore messing with modified files on disk, it'd be nice to show a\n\"baseline\" of correct behavior when a pathspec points to out-of-cone files,\ne.g. 'test_all_match git diff-files folder2/a'.\n\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\t# add file to the index but outside of cone\n> +\trun_on_sparse mkdir newdirectory &&\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git add --sparse newdirectory/testfile &&\n> +\n> +\t# file present on-disk without modifications\n\nFrom the comment you've added here, it looks like the state you want to test\nis \"file outside sparse checkout definition exists on disk\". However, since\none of the goals of this test is to verify sparse index behavior once its\nintegrated with the 'diff-files' command, whether the \"outside of sparse-\ncheckout definition\" file belongs to a sparse directory entry is an\nimportant (and fairly nuanced) factor to consider.\n\nThe main difference between a \"regular\" sparse-checkout and a sparse\nindex-enabled sparse-checkout is the existence of \"sparse directory\"\nentries: index entries with 'SKIP_WORKTREE' that represent directories and\ntheir contents. In the context of these tests, the thing we really want to\nverify is that the sparse index-enabled case produces the same results as\nthe full index when an operation needs to get some information out of a\nsparse directory. \n\nComing back to this test, the 'newdirectory/testfile' you create, while\noutside the sparse-checkout definition, never actually belongs to a sparse\ndirectory because 'newdirectory' is never collapsed - in fact, I don't\nthink it even gets the SKIP_WORKTREE bit in the index. To ensure you have a\nsparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout\nreapply' after removing 'newdirectory/testfile' from disk - which doesn't\nhelp if you want to test what happens when the file exists on disk!\n\nIn fact, because of built-in safeguards around on-disk files and\nsparse-checkout, there isn't really a way in Git to materialize a file\nwithout also expanding its sparse directory and removing SKIP_WORKTREE. If\nyou want to preserve a sparse directory, you should write the contents of an\nexisting file inside that sparse directory to disk manually:\n\n\trun_on_sparse mkdir folder1 &&\n\trun_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in the base commit\n\nGit's index will not be touched by doing this, meaning the next command you\ninvoke (in this case, 'diff-files') will need to reconcile the file on disk\nwith what it sees in the index.\n\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_must_be_empty sparse-checkout-out &&\n> +\ttest_must_be_empty sparse-checkout-err &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\ttest_must_be_empty sparse-checkout-out &&\n> +\ttest_must_be_empty sparse-checkout-err &&\n\nThese checks should be 'test_all_match' rather than 'test_sparse_match'.\nSince (through other tests) we're confident that 'git diff-files' on an\nunmodified, non-sparse-checkout repository will give us an empty result, you\nwouldn't need the additional 'test_must_be_empty' checks.\n\nA bit of a \"spoiler\": when I tested this out locally, I found that the diff\nwas *not* empty for the sparse-checkout cases, until I ran 'git status'\n(which strips the 'SKIP_WORKTREE' bit from the file and writes it to the\nindex). That's not the desired behavior, so there's a bug in the\nsparse-checkout logic used in 'diff-files' that needs to be fixed (my first\nguess would be that 'clear_skip_worktree_from_present_files()' is not being\napplied when the index is read).\n\nIf you'd like any help investigating this or get stuck, please let me know -\nI'd be happy to assist!\n\n> +\n> +\t# file present on-disk with modifications\n> +\tFN=newdirectory/testfile &&\n> +\tOID=$(git -C sparse-checkout hash-object $FN) &&\n> +\tZERO=$(test_oid zero) &&\n> +\techo \":100644 100644 $OID $ZERO M\t$FN\" >expect &&\n> +\n> +\trun_on_sparse ../edit-contents newdirectory/testfile &&\n> +\ttest_sparse_match git diff-files &&\n> +\ttest_cmp expect sparse-checkout-out &&\n> +\ttest_sparse_match git diff-files newdirectory/testfile &&\n> +\ttest_cmp expect sparse-checkout-out\n\nSimilarly, you should use 'run_on_all' to modify the file & 'test_all_match'\nto verify that they all match here. It would demonstrate that we don't\nexpect any \"special\" behavior from sparse-checkout, meaning you can probably\navoid checking the result verbatim. \n\nFinally, as with the earlier test, it'd be nice to show that the result is\nthe same with a wildcard pathspec, e.g. 'git diff-files \"folder*/a\"'.\n\n> +'\n> +\n> test_done\n\n"},{"id":"473813","messageId":"20230320205241.105476-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230310050021.123769-1-cheskaqiqi@gmail.com","subject":"[RFC PATCH v6 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-20T20:52:39Z","receivedAt":"2023-03-20T20:55:23Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Did not fix the logic of spare-checkout yet. Leave the spare diff-files \nwith pathspec outside sparse definition as 'test_expect_failure' now.\nbut will fix soon.\n\nChanges since v5:\n\n1. Add space after \"definition.\"\n\n2. Add test case for a pathspec with wildcards or other \"magic\"\n\n3. Before messing with modified files on disk, add a \"baseline\" \nof correct behavior when a pathspec points to out-of-cone files.\n\n4. Write the contents of an\nexisting file inside that sparse directory to disk manually\n\n5. Use 'test_all_match' rather than 'test_sparse_match'. \nwouldn't need the additional 'test_must_be_empty' checks.\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 8 +++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++++++++\n 3 files changed, 83 insertions(+)\n\n\nbase-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n-- \n2.39.0\n\n"},{"id":"473814","messageId":"20230320205241.105476-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230320205241.105476-1-cheskaqiqi@gmail.com","subject":"[PATCH v6 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-20T20:52:41Z","receivedAt":"2023-03-20T20:55:25Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Originally, diff-files a pathspec that is out-of-cone in a sparse-index\nenvironment, Git dies with \"pathspec '' did not match any files\",\nmainly because it does not expand the index so nothing is matched.\nExpand the index when the needs an expanded index, i.e. the\n contains wildcard that may need a full-index or the\n is simply outside of sparse-checkout definition.\n\nRemove full index requirement for `git diff-files`\nand add test to ensure the index only expanded when necessary\nin `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 8 ++++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 31 ++++++++++++++++++++++++\n 3 files changed, 41 insertions(+)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..d88875aa07 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \n@@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tresult = -1;\n \t\tgoto cleanup;\n \t}\n+\n+\tif (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n+\t\tensure_full_index(the_repository->index);\n+\t\t\n \tresult = run_diff_files(&rev, options);\n \tresult = diff_result_code(&rev.diffopt, result);\n cleanup:\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..82751f2ca3 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex c1329e2f16..6cbbc51a16 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2097,4 +2097,35 @@ test_expect_failure 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files folder1/a\n '\n \n+test_expect_success 'diff-files pathspec expands index when necessary' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\t\n+\t# pathspec that should expand index\n+\t! ensure_not_expanded diff-files \"*/a\" &&\n+\ttest_must_be_empty sparse-index-err &&\n+\n+\t! ensure_not_expanded diff-files \"**a\" &&\n+\ttest_must_be_empty sparse-index-err\n+'\n+\n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files deep/*\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473815","messageId":"20230320205241.105476-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230320205241.105476-1-cheskaqiqi@gmail.com","subject":"[PATCH v6 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-20T20:52:40Z","receivedAt":"2023-03-20T20:55:28Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin\nwith the sparse index feature, add tests to\nt1092-sparse-checkout-compatibility.sh to ensure it currently works\nwith sparse-checkout and will still work with sparse index\nafter that integration.\n\nWhen adding tests against a sparse-checkout\ndefinition, we test two modes: all changes are\nwithin the sparse-checkout cone and some changes are outside\nthe sparse-checkout cone.\n\nIn order to have staged changes outside of\nthe sparse-checkout cone, make a directory called 'folder1' and\ncopy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base\ncommit. These make 'folder1/a' in the index, while leaving it outside of\nthe sparse-checkout definition. Test 'folder1/a'being present on-disk\nwithout modifications, then change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 42 ++++++++++++++++++++++++\n 1 file changed, 42 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..c1329e2f16 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,46 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a && \n+\n+\t# test wildcard\n+\ttest_all_match git diff-files deep/*\n+'\n+\n+test_expect_failure 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# Add file to the index but outside of cone for sparse-checkout cases.\n+\t# Add file to the index without sparse-checkout cases to ensure all have \n+\t# same output.\n+\trun_on_all mkdir folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\t# file present on-disk without modifications\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files folder1/a &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473816","messageId":"CAMO4yUFsGQbeu=wbqG8EuptYFDv9c1tB8Y0RAU_UJ-GdbMAfVg@mail.gmail.com","threadId":"59340","inReplyTo":"b537d855-edb7-4f67-de08-d651868247a5@github.com","subject":"Re: [PATCH v5 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-20T20:55:13Z","receivedAt":"2023-03-20T20:55:51Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"On Fri, Mar 10, 2023 at 1:23 PM Victoria Dye wrote:\n\n Hi Victoria!\n\nSorry for the late reply! I was having midterm exams.\n\nThank you for the feedback on the patch .\nI appreciate your time giving me the advice for improvement.\n\n> > In order to have staged changes outside of\n> > the sparse-checkout cone, create a 'newdirectory/testfile' and\n> > add it to the index, while leaving it outside of\n> > the sparse-checkout definition.Test 'newdirectory/testfile'\n\n> nit: missing space after \"definition.\"\n\nWill do!\n\n> I'd be interested in seeing an additional test case for a pathspec with\n> wildcards or other \"magic\" [1], e.g. 'git diff-files \"deep/*\"'. In past\n> sparse index integrations, there has occasionally been a need for special\n> handling of those types of pathspecs [2][3], so it would be good for the\n> test to cover cases like that.\n\nWill do!\n\n> From the comment you've added here, it looks like the state you want to test\n> is \"file outside sparse checkout definition exists on disk\". However, since\n> one of the goals of this test is to verify sparse index behavior once its\n> integrated with the 'diff-files' command, whether the \"outside of sparse-\n> checkout definition\" file belongs to a sparse directory entry is an\n> important (and fairly nuanced) factor to consider.\n>\n> The main difference between a \"regular\" sparse-checkout and a sparse\n> index-enabled sparse-checkout is the existence of \"sparse directory\"\n> entries: index entries with 'SKIP_WORKTREE' that represent directories and\n> their contents. In the context of these tests, the thing we really want to\n> verify is that the sparse index-enabled case produces the same results as\n> the full index when an operation needs to get some information out of a\n> sparse directory.\n>\n> Coming back to this test, the 'newdirectory/testfile' you create, while\n> outside the sparse-checkout definition, never actually belongs to a sparse\n> directory because 'newdirectory' is never collapsed - in fact, I don't\n> think it even gets the SKIP_WORKTREE bit in the index. To ensure you have a\n> sparse directory & SKIP_WORKTREE, you'd need to run 'git sparse-checkout\n> reapply' after removing 'newdirectory/testfile' from disk - which doesn't\n> help if you want to test what happens when the file exists on disk!\n\nThanks for the explanation here! I learn a lot.\n\n> In fact, because of built-in safeguards around on-disk files and\n> sparse-checkout, there isn't really a way in Git to materialize a file\n> without also expanding its sparse directory and removing SKIP_WORKTREE. If\n> you want to preserve a sparse directory, you should write the contents of an\n> existing file inside that sparse directory to disk manually:\n>\n> run_on_sparse mkdir folder1 &&\n> run_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in the base commit\n>\n> Git's index will not be touched by doing this, meaning the next command you\n> invoke (in this case, 'diff-files') will need to reconcile the file on disk\n> with what it sees in the index.\n>\n> > + test_sparse_match git diff-files &&\n> > + test_must_be_empty sparse-checkout-out &&\n> > + test_must_be_empty sparse-checkout-err &&\n> > + test_sparse_match git diff-files newdirectory/testfile &&\n> > + test_must_be_empty sparse-checkout-out &&\n> > + test_must_be_empty sparse-checkout-err &&\n>\n> These checks should be 'test_all_match' rather than 'test_sparse_match'.\n> Since (through other tests) we're confident that 'git diff-files' on an\n> unmodified, non-sparse-checkout repository will give us an empty result, you\n> wouldn't need the additional 'test_must_be_empty' checks.\n>\n> A bit of a \"spoiler\": when I tested this out locally, I found that the diff\n> was *not* empty for the sparse-checkout cases, until I ran 'git status'\n> (which strips the 'SKIP_WORKTREE' bit from the file and writes it to the\n> index). That's not the desired behavior, so there's a bug in the\n> sparse-checkout logic used in 'diff-files' that needs to be fixed (my first\n> guess would be that 'clear_skip_worktree_from_present_files()' is not being\n> applied when the index is read).\n\nYeah. After I use\n\n run_on_sparse mkdir folder1 &&\n run_on_sparse cp a folder1/a && # `folder1/a` is identical to `a` in\nthe base commit\n\n diff was *not* empty for the sparse-checkout cases\n\n when I look into builtin/diff-files.c\n'repo_read_index_preload' call 'repo_read_index' which call\n 'clear_skip_worktree_from_present_files()' to apply when the index is read.\n\nI got stuck in here and do not have the idea to investigate it. Any\ntips would be helpful!\n\n\n> > + run_on_sparse ../edit-contents newdirectory/testfile &&\n> > + test_sparse_match git diff-files &&\n> > + test_cmp expect sparse-checkout-out &&\n> > + test_sparse_match git diff-files newdirectory/testfile &&\n> > + test_cmp expect sparse-checkout-out\n>\n> Similarly, you should use 'run_on_all' to modify the file & 'test_all_match'\n> to verify that they all match here. It would demonstrate that we don't\n> expect any \"special\" behavior from sparse-checkout, meaning you can probably\n> avoid checking the result verbatim.\n\n> Finally, as with the earlier test, it'd be nice to show that the result is\n> the same with a wildcard pathspec, e.g. 'git diff-files \"folder*/a\"'.\n\nWill do!\n----------------------------------------------------------------------------\nThanks\nShuqi\n"},{"id":"473874","messageId":"af4cd38c-bd57-6e32-867d-a205ff0bb93b@github.com","threadId":"59340","inReplyTo":"20230320205241.105476-1-cheskaqiqi@gmail.com","subject":"Re: [RFC PATCH v6 0/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-21T18:38:05Z","receivedAt":"2023-03-21T18:38:12Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Did not fix the logic of spare-checkout yet. Leave the spare diff-files \n> with pathspec outside sparse definition as 'test_expect_failure' now.\n> but will fix soon.\n> \n> Changes since v5:\n> \n> 1. Add space after \"definition.\"\n> \n> 2. Add test case for a pathspec with wildcards or other \"magic\"\n> \n> 3. Before messing with modified files on disk, add a \"baseline\" \n> of correct behavior when a pathspec points to out-of-cone files.\n> \n> 4. Write the contents of an\n> existing file inside that sparse directory to disk manually\n> \n> 5. Use 'test_all_match' rather than 'test_sparse_match'. \n> wouldn't need the additional 'test_must_be_empty' checks.\n> \n> \n> Shuqi Liang (2):\n> t1092: add tests for `git diff-files`\n> diff-files: integrate with sparse index\n> \n> builtin/diff-files.c | 8 +++\n> t/perf/p2000-sparse-operations.sh | 2 +\n> t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++++++++\n> 3 files changed, 83 insertions(+)\n\n[forgot to Reply-All - Shuqi, sorry for the duplicate email!]\n\nIn future iterations, please also include a range-diff in your version\niterations. The description of changes is useful, but the range-diff\nprovides a much more detailed and comprehensive summary of the changes\n(making it exceptionally helpful for reviewers). I *think* you can just add\nthe '--range-diff ' option to 'git format-patch' (see\nMyFirstContribution [1] for more detailed instructions).\n\n[1] https://git-scm.com/docs/MyFirstContribution#v2-git-send-email\n\nFor anyone that's interested, here's the range-diff vs. v5:\n\n1: fb9ec0901c ! 1: 14bbcf41e0 t1092: add tests for `git diff-files`\n @@ Commit message\n the sparse-checkout cone.\n \n In order to have staged changes outside of\n - the sparse-checkout cone, create a 'newdirectory/testfile' and\n - add it to the index, while leaving it outside of\n - the sparse-checkout definition.Test 'newdirectory/testfile'\n - being present on-disk without modifications, then change content inside\n - 'newdirectory/testfile' in order to test 'newdirectory/testfile'\n - being present on-disk with modifications.\n + the sparse-checkout cone, make a directory called 'folder1' and\n + copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base\n + commit. These make 'folder1/a' in the index, while leaving it outside of\n + the sparse-checkout definition. Test 'folder1/a'being present on-disk\n + without modifications, then change content inside 'folder1/a' in order\n + to test 'folder1/a' being present on-disk with modifications.\n \n Signed-off-by: Shuqi Liang \n \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc\n +\n +\ttest_all_match git diff-files &&\n +\n -+\ttest_all_match git diff-files deep/a \n ++\ttest_all_match git diff-files deep/a && \n +\n ++\t# test wildcard\n ++\ttest_all_match git diff-files deep/*\n +'\n +\n -+test_expect_success 'diff-files with pathspec outside sparse definition' '\n ++test_expect_failure 'diff-files with pathspec outside sparse definition' '\n +\tinit_repos &&\n +\n ++\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n ++\n +\twrite_script edit-contents <<-\\EOF &&\n +\techo text >>\"$1\"\n +\tEOF\n +\n -+\t# add file to the index but outside of cone\n -+\trun_on_sparse mkdir newdirectory &&\n -+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n -+\ttest_sparse_match git add --sparse newdirectory/testfile &&\n ++\t# Add file to the index but outside of cone for sparse-checkout cases.\n ++\t# Add file to the index without sparse-checkout cases to ensure all have \n ++\t# same output.\n ++\trun_on_all mkdir folder1 &&\n ++\trun_on_all cp a folder1/a &&\n +\n +\t# file present on-disk without modifications\n -+\ttest_sparse_match git diff-files &&\n -+\ttest_must_be_empty sparse-checkout-out &&\n -+\ttest_must_be_empty sparse-checkout-err &&\n -+\ttest_sparse_match git diff-files newdirectory/testfile &&\n -+\ttest_must_be_empty sparse-checkout-out &&\n -+\ttest_must_be_empty sparse-checkout-err &&\n ++\ttest_all_match git diff-files &&\n ++\ttest_all_match git diff-files folder1/a &&\n +\n +\t# file present on-disk with modifications\n -+\tFN=newdirectory/testfile &&\n -+\tOID=$(git -C sparse-checkout hash-object $FN) &&\n -+\tZERO=$(test_oid zero) &&\n -+\techo \":100644 100644 $OID $ZERO M\t$FN\" >expect &&\n -+\n -+\trun_on_sparse ../edit-contents newdirectory/testfile &&\n -+\ttest_sparse_match git diff-files &&\n -+\ttest_cmp expect sparse-checkout-out &&\n -+\ttest_sparse_match git diff-files newdirectory/testfile &&\n -+\ttest_cmp expect sparse-checkout-out\n ++\trun_on_all ../edit-contents folder1/a &&\n ++\ttest_all_match git diff-files &&\n ++\ttest_all_match git diff-files folder1/a\n +'\n +\n test_done\n2: 04e24f7db5 ! 2: 734cd24f0c diff-files: integrate with sparse index\n @@ Metadata\n ## Commit message ##\n diff-files: integrate with sparse index\n \n + Originally, diff-files a pathspec that is out-of-cone in a sparse-index\n + environment, Git dies with \"pathspec '' did not match any files\",\n + mainly because it does not expand the index so nothing is matched.\n + Expand the index when the needs an expanded index, i.e. the\n + contains wildcard that may need a full-index or the\n + is simply outside of sparse-checkout definition.\n +\n Remove full index requirement for `git diff-files`\n - and test to ensure the index is not expanded in `git diff-files`.\n + and add test to ensure the index only expanded when necessary\n + in `git diff-files`.\n \n The `p2000` tests demonstrate a ~96% execution time reduction for 'git\n diff-files' and a ~97% execution time reduction for 'git diff-files'\n @@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \n +@@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char *prefix)\n + \t\tresult = -1;\n + \t\tgoto cleanup;\n + \t}\n ++\n ++\tif (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n ++\t\tensure_full_index(the_repository->index);\n ++\t\t\n + \tresult = run_diff_files(&rev, options);\n + \tresult = diff_result_code(&rev.diffopt, result);\n + cleanup:\n \n ## t/perf/p2000-sparse-operations.sh ##\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all\n test_done\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '\n - \ttest_cmp expect sparse-checkout-out\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with pathspec outside sparse definition' '\n + \ttest_all_match git diff-files folder1/a\n '\n \n ++test_expect_success 'diff-files pathspec expands index when necessary' '\n ++\tinit_repos &&\n ++\n ++\twrite_script edit-contents <<-\\EOF &&\n ++\techo text >>\"$1\"\n ++\tEOF\n ++\n ++\trun_on_all ../edit-contents deep/a &&\n ++\t\n ++\t# pathspec that should expand index\n ++\t! ensure_not_expanded diff-files \"*/a\" &&\n ++\ttest_must_be_empty sparse-index-err &&\n ++\n ++\t! ensure_not_expanded diff-files \"**a\" &&\n ++\ttest_must_be_empty sparse-index-err\n ++'\n ++\n +test_expect_success 'sparse index is not expanded: diff-files' '\n +\tinit_repos &&\n +\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with p\n +\n +\trun_on_all ../edit-contents deep/a &&\n +\n -+\tensure_not_expanded diff-files &&\n -+\tensure_not_expanded diff-files deep/a \n ++\tensure_not_expanded diff-files &&\n ++\tensure_not_expanded diff-files deep/a &&\n ++\tensure_not_expanded diff-files deep/*\n +'\n +\n test_done\n\n> \n> \n> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1\n\n"},{"id":"473886","messageId":"d940fe05-de86-5069-1d77-f4c7d0d368b6@github.com","threadId":"59340","inReplyTo":"20230320205241.105476-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-21T21:21:46Z","receivedAt":"2023-03-21T21:21:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Before integrating the 'git diff-files' builtin\n> with the sparse index feature, add tests to\n> t1092-sparse-checkout-compatibility.sh to ensure it currently works\n> with sparse-checkout and will still work with sparse index\n> after that integration.\n> \n> When adding tests against a sparse-checkout\n> definition, we test two modes: all changes are\n> within the sparse-checkout cone and some changes are outside\n> the sparse-checkout cone.\n> \n> In order to have staged changes outside of\n> the sparse-checkout cone, make a directory called 'folder1' and\n> copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base\n> commit. These make 'folder1/a' in the index, while leaving it outside of\n> the sparse-checkout definition. Test 'folder1/a'being present on-disk\n> without modifications, then change content inside 'folder1/a' in order\n> to test 'folder1/a' being present on-disk with modifications.\n\nThe word wrapping on this message (and your other commits/cover letter) is a\nbit strange. By convention, it should be consistently wrapped to 72 columns\nper line. Most text editors have some way of configuring that so you don't\nneed to do it manually.\n\n> \n> Signed-off-by: Shuqi Liang \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 42 ++++++++++++++++++++++++\n> 1 file changed, 42 insertions(+)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 801919009e..c1329e2f16 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2055,4 +2055,46 @@ test_expect_success 'grep sparse directory within submodules' '\n> \ttest_cmp actual expect\n> '\n> \n> +test_expect_success 'diff-files with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\ttest_all_match git diff-files &&\n> +\n> +\ttest_all_match git diff-files deep/a && \n> +\n> +\t# test wildcard\n> +\ttest_all_match git diff-files deep/*\n> +'\n> +\n> +test_expect_failure 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n\nMakes sense. In a sparse-checkout, folder2/a isn't in the working tree, so \n'diff-files' (which compares working tree vs. index) doesn't really apply.\n\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\t# Add file to the index but outside of cone for sparse-checkout cases.\n> +\t# Add file to the index without sparse-checkout cases to ensure all have \n> +\t# same output.\n> +\trun_on_all mkdir folder1 &&\n\nThe test is failing because of this line. It should be 'mkdir -p folder1';\nas you have it now, the command fails because 'folder1' already exists in\n'full-checkout'. Alternatively, you could use 'run_on_sparse mkdir folder1',\nbut using '-p' seems like the less fragile approach. \n\nFor reference, the way I figured that out was to run 't1092' as follows:\n\n\tcd t && ./t1092-sparse-checkout-compatibility.sh -xvdi --run=1,82\n\nWhat the options correspond to:\n* -x: print the commands called in the test (equivalent of calling 'set -x'\n in the test script).\n* -v: print stdout/stderr to the console.\n* -d: do not remove the \"trash directory\" of the test.\n* -i: stop test execution once it encounters a failure.\n* --run: run only the specified tests (1 is the first test - 'setup' - and\n 82 is 'diff-files with pathspec outside sparse definition').\n\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\t# file present on-disk without modifications\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files folder1/a &&\n\nThe strange thing is, once I fixed the 'mkdir' issue in my local copy of\nthese patches, these 'test_all_match diff-files' calls succeeded. It turns\nout that 'git diff-files' in the 'full-checkout', like in 'sparse-checkout',\nreports a difference in 'folder1/a' that doesn't actually exist. So the bug\nisn't in sparse-checkout as I initially assumed [1], but rather in\ndiff-files itself.\n\nAt this point, I'd say the diff-files bug is out-of-scope of this sparse\nindex integration; in your implementation, sparse-checkout and sparse index\nwork the same as a full checkout, it's just that the \"normal\" full checkout\nbehavior is wrong. My recommendation would be that you keep this test as-is\nand, to force the failure, add a check that 'full-checkout-out' is empty.\nThen, in a \"NEEDSWORK\" comment on the test (like the one on 'diff with\nrenames and conflicts') that indicates that the 'diff-files' behavior is\nwrong.\n\nI'll try to make some time this week to look into the 'diff-files' bug.\nSorry for the back-and-forth and distraction from your sparse index\nintegration. Other than the 'mkdir' issue, these updated tests look great!\n\n[1] https://lore.kernel.org/git/b537d855-edb7-4f67-de08-d651868247a5@github.com/\n\n> +\n> +\t# file present on-disk with modifications\n> +\trun_on_all ../edit-contents folder1/a &&\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files folder1/a\n> +'\n> +\n> test_done\n\n"},{"id":"473887","messageId":"xmqq1qlhhk4s.fsf@gitster.g","threadId":"59340","inReplyTo":"d940fe05-de86-5069-1d77-f4c7d0d368b6@github.com","subject":"Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-21T21:25:39Z","receivedAt":"2023-03-21T21:25:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> The strange thing is, once I fixed the 'mkdir' issue in my local copy of\n> these patches, these 'test_all_match diff-files' calls succeeded. It turns\n> out that 'git diff-files' in the 'full-checkout', like in 'sparse-checkout',\n> reports a difference in 'folder1/a' that doesn't actually exist. So the bug\n> isn't in sparse-checkout as I initially assumed [1], but rather in\n> diff-files itself.\n\nIs that a bug, or just a common \"ah, you forgot to refresh the index\"?\n"},{"id":"473891","messageId":"3e2371e9-2d7d-27c4-58dd-296fcee49e88@github.com","threadId":"59340","inReplyTo":"xmqq1qlhhk4s.fsf@gitster.g","subject":"Re: [PATCH v6 1/2] t1092: add tests for `git diff-files`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-21T22:19:29Z","receivedAt":"2023-03-21T22:19:34Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> Victoria Dye writes:\n> \n>> The strange thing is, once I fixed the 'mkdir' issue in my local copy of\n>> these patches, these 'test_all_match diff-files' calls succeeded. It turns\n>> out that 'git diff-files' in the 'full-checkout', like in 'sparse-checkout',\n>> reports a difference in 'folder1/a' that doesn't actually exist. So the bug\n>> isn't in sparse-checkout as I initially assumed [1], but rather in\n>> diff-files itself.\n> \n> Is that a bug, or just a common \"ah, you forgot to refresh the index\"?\n\nAh, you're right - I completely forgot about 'diff-files' not refreshing the\nindex (since 'diff', by default, does). The ctime is (often, but not always)\ndifferent on the copied file than what's in the index, so 'diff-files' shows\nthe file as \"modified\" if the index isn't refreshed. \n\nGoing back to these tests, the goal is to make sure that 'diff-files' finds\nthe correct index entry (possibly in a sparse directory) and compares that\ncorrectly to what's on disk. But since we want to ignore ctime differences,\nwe could use '--stat' (or '-p', or '--num-stat':\n\n\trun_on_all mkdir -p folder1 &&\n\trun_on_all cp a folder1/a &&\n\n\t# file present on-disk without modifications\n\t# use `--stat` to ignore file creation time differences in\n\t# unrefreshed index\n\ttest_all_match git diff-files --stat &&\n\ttest_all_match git diff-files --stat folder1/a &&\n\nI don't think that makes the test any less comprehensive (especially since\nlater on in the same test, we modify the contents of 'folder1/a' and get the\nexpected \"modified\" status in 'git diff-files' without '--stat'), but it\navoids potential breakages related to inconsistency in file creation time.\n\n"},{"id":"473892","messageId":"cea3c428-02e8-a50d-1211-e62f593dc0a1@github.com","threadId":"59340","inReplyTo":"20230320205241.105476-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v6 2/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-21T22:34:02Z","receivedAt":"2023-03-21T22:34:09Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Originally, diff-files a pathspec that is out-of-cone in a sparse-index\n> environment, Git dies with \"pathspec '' did not match any files\",\n> mainly because it does not expand the index so nothing is matched.\n> Expand the index when the needs an expanded index, i.e. the\n> contains wildcard that may need a full-index or the\n> is simply outside of sparse-checkout definition.\n\n...\n\n> +\tif (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n> +\t\tensure_full_index(the_repository->index);\n\nLooks good! I'm glad you were able to use the tests to confirm that this\npathspec-based expansion was needed.\n\n> +\t\t\n> \tresult = run_diff_files(&rev, options);\n> \tresult = diff_result_code(&rev.diffopt, result);\n> cleanup:\n> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\n> index 3242cfe91a..82751f2ca3 100755\n> --- a/t/perf/p2000-sparse-operations.sh\n> +++ b/t/perf/p2000-sparse-operations.sh\n> @@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n> test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n> test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n> test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n> +test_perf_on_all git diff-files\n> +test_perf_on_all git diff-files $SPARSE_CONE/a\n> \n> test_done\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index c1329e2f16..6cbbc51a16 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2097,4 +2097,35 @@ test_expect_failure 'diff-files with pathspec outside sparse definition' '\n> \ttest_all_match git diff-files folder1/a\n> '\n> \n> +test_expect_success 'diff-files pathspec expands index when necessary' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\t\n> +\t# pathspec that should expand index\n> +\t! ensure_not_expanded diff-files \"*/a\" &&\n> +\ttest_must_be_empty sparse-index-err &&\n> +\n> +\t! ensure_not_expanded diff-files \"**a\" &&\n> +\ttest_must_be_empty sparse-index-err\n> +'\n\nThanks for adding these, it's a good idea to show when the sparse index *is*\nexpanded in addition to when it is not. However, checking that the\n'sparse-index-err' is empty won't handle silent failures, so it's probably\nbetter to create an 'ensure_expanded' to mirror 'ensure_not_expanded'. The\ntwo functions could share pretty much all of their code except for the last\nline ('test_region ...').\n\n> +\n> +test_expect_success 'sparse index is not expanded: diff-files' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\tensure_not_expanded diff-files &&\n> +\tensure_not_expanded diff-files deep/a &&\n> +\tensure_not_expanded diff-files deep/*\n> +'\n> +\n> test_done\n\n"},{"id":"473924","messageId":"20230322161820.3609-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230320205241.105476-1-cheskaqiqi@gmail.com","subject":"[PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-22T16:18:18Z","receivedAt":"2023-03-22T16:18:51Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"\nChanges since v6:\n\n1. Fix word wrap in commit message.\n\n2. Use 'mkdir -p folder1' since full-checkout already have folder1.\n\n3. Use `--stat` to ignore file creation time differences in unrefreshed\nindex.\n\n4. In 'diff-files with pathspec outside sparse definition' add \n'git diff-files \"folder*/a\" to show that the result is the same with a \nwildcard pathspec.\n\n5. Create an 'ensure_expanded' to handle silent failures.\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 8 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 98 ++++++++++++++++++++++++\n 3 files changed, 108 insertions(+)\n\nRange-diff against v6:\n1: 2a994e60bc ! 1: e2dcf9921e t1092: add tests for `git diff-files`\n @@ Metadata\n ## Commit message ##\n t1092: add tests for `git diff-files`\n \n - Before integrating the 'git diff-files' builtin\n - with the sparse index feature, add tests to\n - t1092-sparse-checkout-compatibility.sh to ensure it currently works\n - with sparse-checkout and will still work with sparse index\n - after that integration.\n + Before integrating the 'git diff-files' builtin with the sparse index\n + feature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\n + it currently works with sparse-checkout and will still work with sparse\n + index after that integration.\n \n - When adding tests against a sparse-checkout\n - definition, we test two modes: all changes are\n - within the sparse-checkout cone and some changes are outside\n - the sparse-checkout cone.\n + When adding tests against a sparse-checkout definition, we test two\n + modes: all changes are within the sparse-checkout cone and some changes\n + are outside the sparse-checkout cone.\n \n - In order to have staged changes outside of\n - the sparse-checkout cone, make a directory called 'folder1' and\n - copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base\n - commit. These make 'folder1/a' in the index, while leaving it outside of\n - the sparse-checkout definition. Test 'folder1/a'being present on-disk\n + In order to have staged changes outside of the sparse-checkout cone,\n + make a directory called 'folder1' and copy `a` into 'folder1/a'.\n + 'folder1/a' is identical to `a` in the base commit. These make\n + 'folder1/a' in the index, while leaving it outside of the\n + sparse-checkout definition. Test 'folder1/a'being present on-disk\n without modifications, then change content inside 'folder1/a' in order\n to test 'folder1/a' being present on-disk with modifications.\n \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc\n +\ttest_all_match git diff-files deep/*\n +'\n +\n -+test_expect_failure 'diff-files with pathspec outside sparse definition' '\n ++test_expect_success 'diff-files with pathspec outside sparse definition' '\n +\tinit_repos &&\n +\n +\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc\n +\t# Add file to the index but outside of cone for sparse-checkout cases.\n +\t# Add file to the index without sparse-checkout cases to ensure all have \n +\t# same output.\n -+\trun_on_all mkdir folder1 &&\n ++\trun_on_all mkdir -p folder1 &&\n +\trun_on_all cp a folder1/a &&\n +\n +\t# file present on-disk without modifications\n -+\ttest_all_match git diff-files &&\n -+\ttest_all_match git diff-files folder1/a &&\n ++\t# use `--stat` to ignore file creation time differences in\n ++\t# unrefreshed index\n ++\ttest_all_match git diff-files --stat &&\n ++\ttest_all_match git diff-files --stat folder1/a &&\n ++\ttest_all_match git diff-files --stat \"folder*/a\" &&\n +\n +\t# file present on-disk with modifications\n +\trun_on_all ../edit-contents folder1/a &&\n +\ttest_all_match git diff-files &&\n -+\ttest_all_match git diff-files folder1/a\n ++\ttest_all_match git diff-files folder1/a &&\n ++\ttest_all_match git diff-files \"folder*/a\" \n +'\n +\n test_done\n2: ac730e372d ! 2: fb8edaf583 diff-files: integrate with sparse index\n @@ Commit message\n contains wildcard that may need a full-index or the\n is simply outside of sparse-checkout definition.\n \n - Remove full index requirement for `git diff-files`\n - and add test to ensure the index only expanded when necessary\n - in `git diff-files`.\n + Remove full index requirement for `git diff-files`.Create an\n + 'ensure_expanded' to handle silent failures. Add test to\n + ensure the index only expanded when necessary in `git diff-files`.\n \n The `p2000` tests demonstrate a ~96% execution time reduction for 'git\n diff-files' and a ~97% execution time reduction for 'git diff-files'\n @@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char\n +\n +\tif (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n +\t\tensure_full_index(the_repository->index);\n -+\t\t\n ++\n \tresult = run_diff_files(&rev, options);\n \tresult = diff_result_code(&rev.diffopt, result);\n cleanup:\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all\n test_done\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with pathspec outside sparse definition' '\n - \ttest_all_match git diff-files folder1/a\n +@@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {\n + \ttest_region ! index ensure_full_index trace2.txt\n + }\n + \n ++ensure_expanded () {\n ++\trm -f trace2.txt &&\n ++\tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n ++\tthen\n ++\t\techo >>sparse-index/untracked.txt\n ++\tfi &&\n ++\n ++\tif test \"$1\" = \"!\"\n ++\tthen\n ++\t\tshift &&\n ++\t\ttest_must_fail env \\\n ++\t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n ++\t\t\tgit -C sparse-index \"$@\" \\\n ++\t\t\t>sparse-index-out \\\n ++\t\t\t2>sparse-index-error || return 1\n ++\telse\n ++\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n ++\t\t\tgit -C sparse-index \"$@\" \\\n ++\t\t\t>sparse-index-out \\\n ++\t\t\t2>sparse-index-error || return 1\n ++\tfi &&\n ++\ttest_region index ensure_full_index trace2.txt\n ++}\n ++\n + test_expect_success 'sparse-index is not expanded' '\n + \tinit_repos &&\n + \n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '\n + \ttest_all_match git diff-files \"folder*/a\" \n '\n \n +test_expect_success 'diff-files pathspec expands index when necessary' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with p\n +\trun_on_all ../edit-contents deep/a &&\n +\t\n +\t# pathspec that should expand index\n -+\t! ensure_not_expanded diff-files \"*/a\" &&\n -+\ttest_must_be_empty sparse-index-err &&\n -+\n -+\t! ensure_not_expanded diff-files \"**a\" &&\n -+\ttest_must_be_empty sparse-index-err\n ++\tensure_expanded diff-files \"*/a\" &&\n ++\tensure_expanded diff-files \"**a\" \n +'\n +\n +test_expect_success 'sparse index is not expanded: diff-files' '\n-- \n2.39.0\n\n"},{"id":"473925","messageId":"20230322161820.3609-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230322161820.3609-1-cheskaqiqi@gmail.com","subject":"[PATCH v7 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-22T16:18:19Z","receivedAt":"2023-03-22T16:18:52Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin with the sparse index\nfeature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\nit currently works with sparse-checkout and will still work with sparse\nindex after that integration.\n\nWhen adding tests against a sparse-checkout definition, we test two\nmodes: all changes are within the sparse-checkout cone and some changes\nare outside the sparse-checkout cone.\n\nIn order to have staged changes outside of the sparse-checkout cone,\nmake a directory called 'folder1' and copy `a` into 'folder1/a'.\n'folder1/a' is identical to `a` in the base commit. These make\n'folder1/a' in the index, while leaving it outside of the\nsparse-checkout definition. Test 'folder1/a'being present on-disk\nwithout modifications, then change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..d23041e27a 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2055,4 +2055,50 @@ test_expect_success 'grep sparse directory within submodules' '\n \ttest_cmp actual expect\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a && \n+\n+\t# test wildcard\n+\ttest_all_match git diff-files deep/*\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# Add file to the index but outside of cone for sparse-checkout cases.\n+\t# Add file to the index without sparse-checkout cases to ensure all have \n+\t# same output.\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\t# file present on-disk without modifications\n+\t# use `--stat` to ignore file creation time differences in\n+\t# unrefreshed index\n+\ttest_all_match git diff-files --stat &&\n+\ttest_all_match git diff-files --stat folder1/a &&\n+\ttest_all_match git diff-files --stat \"folder*/a\" &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files folder1/a &&\n+\ttest_all_match git diff-files \"folder*/a\" \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473926","messageId":"20230322161820.3609-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230322161820.3609-1-cheskaqiqi@gmail.com","subject":"[PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-22T16:18:20Z","receivedAt":"2023-03-22T16:18:54Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Originally, diff-files a pathspec that is out-of-cone in a sparse-index\nenvironment, Git dies with \"pathspec '' did not match any files\",\nmainly because it does not expand the index so nothing is matched.\nExpand the index when the needs an expanded index, i.e. the\n contains wildcard that may need a full-index or the\n is simply outside of sparse-checkout definition.\n\nRemove full index requirement for `git diff-files`.Create an\n'ensure_expanded' to handle silent failures. Add test to\nensure the index only expanded when necessary in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 8 ++++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 52 ++++++++++++++++++++++++\n 3 files changed, 62 insertions(+)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..db90592090 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \n@@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tresult = -1;\n \t\tgoto cleanup;\n \t}\n+\n+\tif (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n+\t\tensure_full_index(the_repository->index);\n+\n \tresult = run_diff_files(&rev, options);\n \tresult = diff_result_code(&rev.diffopt, result);\n cleanup:\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..82751f2ca3 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all\n test_perf_on_all git update-index --add --remove $SPARSE_CONE/a\n test_perf_on_all \"git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a\"\n test_perf_on_all git grep --cached --sparse bogus -- \"f2/f1/f1/*\"\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex d23041e27a..152f3f752e 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1401,6 +1401,30 @@ ensure_not_expanded () {\n \ttest_region ! index ensure_full_index trace2.txt\n }\n \n+ensure_expanded () {\n+\trm -f trace2.txt &&\n+\tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n+\tthen\n+\t\techo >>sparse-index/untracked.txt\n+\tfi &&\n+\n+\tif test \"$1\" = \"!\"\n+\tthen\n+\t\tshift &&\n+\t\ttest_must_fail env \\\n+\t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n+\t\t\tgit -C sparse-index \"$@\" \\\n+\t\t\t>sparse-index-out \\\n+\t\t\t2>sparse-index-error || return 1\n+\telse\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n+\t\t\tgit -C sparse-index \"$@\" \\\n+\t\t\t>sparse-index-out \\\n+\t\t\t2>sparse-index-error || return 1\n+\tfi &&\n+\ttest_region index ensure_full_index trace2.txt\n+}\n+\n test_expect_success 'sparse-index is not expanded' '\n \tinit_repos &&\n \n@@ -2101,4 +2125,32 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files \"folder*/a\" \n '\n \n+test_expect_success 'diff-files pathspec expands index when necessary' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\t\n+\t# pathspec that should expand index\n+\tensure_expanded diff-files \"*/a\" &&\n+\tensure_expanded diff-files \"**a\" \n+'\n+\n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files deep/*\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"473970","messageId":"xmqqilesbbph.fsf@gitster.g","threadId":"59340","inReplyTo":"20230322161820.3609-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-22T23:36:26Z","receivedAt":"2023-03-22T23:36:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> 3. Use `--stat` to ignore file creation time differences in unrefreshed\n> index.\n\nI am curious about this one. Why is this a preferred solution over\nsay \"run 'update-index --refresh' before running diff-files\"?\n\nNote that this is merely \"I am curious\", not \"I think it is wrong\".\n\nThanks.\n"},{"id":"473980","messageId":"CAMO4yUFshQ_bP3gXeZhfHQ3OevC+_3qKwa-iy2nNGScvRouu6Q@mail.gmail.com","threadId":"59340","inReplyTo":"xmqqilesbbph.fsf@gitster.g","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-23T07:42:21Z","receivedAt":"2023-03-23T07:42:44Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"On Wed, Mar 22, 2023 at 7:36 PM Junio C Hamano wrote:\n\n> > 3. Use `--stat` to ignore file creation time differences in unrefreshed\n> > index.\n>\n> I am curious about this one. Why is this a preferred solution over\n> say \"run 'update-index --refresh' before running diff-files\"?\n>\n> Note that this is merely \"I am curious\", not \"I think it is wrong\".\n\nHi Junio\n\nThank you for your question, it has prompted me to consider the matter\nfurther =) I think both solutions, using git diff-files --stat and using git\nupdate-index --refresh before git diff-files, can produce the same output but\nin different ways.\n\nWhen the index file is not up-to-date, git diff-files may show differences\nbetween the working directory and the index that are caused by file creation\ntime differences, rather than actual changes to the file contents. By using git\ndiff-files --stat, which ignores file creation time differences.\n\nWhile 'git update-index --refresh' updates the index file to match the contents\nof the working tree. By running this command before git diff-files, we can\nensure that the index file is up-to-date and that the output of git diff-files\naccurately reflects the differences between the working directory and the index.\n\nMaybe using git update-index --refresh would be more direct and\nstraightforward solution.\n\n(Hi Victoria, do you have any comments? =)\n\n\nThanks\nShuqi\n"},{"id":"473993","messageId":"xmqqlejna20n.fsf@gitster.g","threadId":"59340","inReplyTo":"CAMO4yUFshQ_bP3gXeZhfHQ3OevC+_3qKwa-iy2nNGScvRouu6Q@mail.gmail.com","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T16:03:20Z","receivedAt":"2023-03-23T16:03:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> When the index file is not up-to-date, git diff-files may show differences\n> between the working directory and the index that are caused by file creation\n> time differences, rather than actual changes to the file contents. By using git\n> diff-files --stat, which ignores file creation time differences.\n\nUse of \"diff-files --stat\" would mean that the contents of the blob\nregistered in the index will be inspected, which can be used to hide\nthe \"stat dirty\" condition.\n\nBut doesn't it cut both ways? Starting from a clean index that has\nup-to-date stat information for paths, we may want to test what\n\"stat dirty\" changes diff-files reports when we touch paths in the\nworking tree, both inside and outside the spase cones. A test with\n\"--stat\" will not achieve that, exactly because it does not pay\nattention to and hides the stat dirtiness.\n\nOn the other hand, if \"update-index --refresh\" is used in the test,\nwe may discover breakages caused by \"update-index\" not handling\nthe sparse index correctly. It would be outside the topic of this\nseries, so avoiding it would be simpler, but (1) if it is not broken,\nthen as you said, it would be a more direct way to test diff-files,\nand (2) if it is broken, it would need to be fixed anyway, before or\nafter this series. So, I dunno...\n\nThanks.\n"},{"id":"474020","messageId":"29eb319d-baf0-22d5-12b4-3e8ee7323050@github.com","threadId":"59340","inReplyTo":"CAMO4yUFshQ_bP3gXeZhfHQ3OevC+_3qKwa-iy2nNGScvRouu6Q@mail.gmail.com","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-03-23T17:25:40Z","receivedAt":"2023-03-23T17:26:15Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> On Wed, Mar 22, 2023 at 7:36 PM Junio C Hamano wrote:\n> \n>>> 3. Use `--stat` to ignore file creation time differences in unrefreshed\n>>> index.\n>>\n>> I am curious about this one. Why is this a preferred solution over\n>> say \"run 'update-index --refresh' before running diff-files\"?\n>>\n>> Note that this is merely \"I am curious\", not \"I think it is wrong\".\n> \n> Hi Junio\n> \n> Thank you for your question, it has prompted me to consider the matter\n> further =) I think both solutions, using git diff-files --stat and using git\n> update-index --refresh before git diff-files, can produce the same output but\n> in different ways.\n\nWhile they'll (ideally) give the same user-facing result, there is a\ndifference in how they exercise 'diff-files' because of how 'update-index\n--refresh' will affect SKIP_WORKTREE and sparse directories.\n\nUsing the same scenario you've set up for your test, suppose I start with a\nfresh copy of the 't1092' repo. In the 'sparse-index' repo copy, 'folder1/'\nwill be a sparse directory:\n\n$ git ls-files -t --sparse folder1/\nS folder1/\n\n(note: \"S\" indicates that SKIP_WORKTREE is applied to the entry)\n\nNow suppose I copy 'a' into 'folder1/' and run 'update-index --refresh'\nthen 'ls-files' again:\n\n$ git update-index --refresh\n$ git ls-files -t --sparse folder1/\nS folder1/0/\nH folder1/a\n\n(note: \"H\" indicates that 'folder1/a' does not have SKIP_WORKTREE applied)\n\nThe sparse directory has been expanded and SKIP_WORKTREE has been removed\nfrom the file that's now present on-disk. This was an intentional \"safety\"\nmeasure added in [1] to address the growing volume of bugs and complexities\nin scenarios where SKIP_WORKTREE files existed on disk.\n\nUltimately, the main difference between this test with & without\n'update-index' is who applies those index corrections when initially reading\nthe index: 'update-index' or 'diff-files'. I lean towards the latter because\nthe former is tested (almost identically) in 'update-index modify outside\nsparse definition' earlier in 't1092'.\n\n[1] https://lore.kernel.org/git/pull.1114.v2.git.1642175983.gitgitgadget@gmail.com/\n\n> \n> When the index file is not up-to-date, git diff-files may show differences\n> between the working directory and the index that are caused by file creation\n> time differences, rather than actual changes to the file contents. By using git\n> diff-files --stat, which ignores file creation time differences.\n\nMore or less, yes. Internally, 'diff-files' will \"see\" the file creation\ndifferences, but the '--stat' format doesn't print them.\n\n> \n> While 'git update-index --refresh' updates the index file to match the contents\n> of the working tree. By running this command before git diff-files, we can\n> ensure that the index file is up-to-date and that the output of git diff-files\n> accurately reflects the differences between the working directory and the index.\n\nThis isn't quite true - 'update-index' only updates the *contents* of index\nentries (or, colloquially, \"stage them for commit\") for files explicitly\nprovided as arguments. Separately, though, '--refresh' updates *all* index\nentries' cached 'stat' information. \n\nGoing a bit deeper: with no arguments, 'update-index' will read the index,\ndo nothing to it, then write it only if something has changed. In almost all\ncases, reading the index doesn't cause any changes to it, making it a no-op.\nHowever, the removal of SKIP_WORKTREE is done on read (including a refresh\nof the entry's stat information), so a even plain 'update-index' *without*\n'--refresh' would write a modified index to disk. In your test, that means:\n\n\trun_on_sparse mkdir -p folder1 &&\n\trun_on_sparse cp a folder1/a &&\n\trun_on_all git update-index &&\n\ttest_all_match git diff-files\n\nwould get you the same result as:\n\n\trun_on_sparse mkdir -p folder1 &&\n\trun_on_sparse cp a folder1/a &&\n\trun_on_all git update-index --refresh &&\n\ttest_all_match git diff-files\n\n> \n> Maybe using git update-index --refresh would be more direct and\n> straightforward solution.\n> \n> (Hi Victoria, do you have any comments? =)\n\nI hope the above explanation is helpful. I still think '--stat' is the best\nway to test this case, but I'm interested to hear your/others' thoughts on\nthe matter given the additional context.\n\n> \n> \n> Thanks\n> Shuqi\n\n"},{"id":"474068","messageId":"CAMO4yUFG2EnEPM3AqXbywpZp2rYU_emJgE5h3_tY+u2ZMXqrhA@mail.gmail.com","threadId":"59340","inReplyTo":"xmqqlejna20n.fsf@gitster.g","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-03-23T23:59:03Z","receivedAt":"2023-03-23T23:59:19Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Junio\n\nOn Thu, Mar 23, 2023 at 12:03 PM Junio C Hamano wrote:\n\n> > When the index file is not up-to-date, git diff-files may show differences\n> > between the working directory and the index that are caused by file creation\n> > time differences, rather than actual changes to the file contents. By using git\n> > diff-files --stat, which ignores file creation time differences.\n>\n> Use of \"diff-files --stat\" would mean that the contents of the blob\n> registered in the index will be inspected, which can be used to hide\n> the \"stat dirty\" condition.\n>\n> But doesn't it cut both ways? Starting from a clean index that has\n> up-to-date stat information for paths, we may want to test what\n> \"stat dirty\" changes diff-files reports when we touch paths in the\n> working tree, both inside and outside the spase cones. A test with\n> \"--stat\" will not achieve that, exactly because it does not pay\n> attention to and hides the stat dirtiness.\n\nIn this case, we can only use 'git diff-files --stat' when files are\npresent on disk without modifications. Since we know in the\nfull-checkout case 'diff-files --stat' will give empty output, so\nsparse-checkout and sparse-index are also empty. These make\nsure that the paths in the working tree are not dirty. So we do not\nneed to pay attention to 'stat dirty' change.\n\nWhen 'file present on-disk with modifications'. We use 'git diff-files'\ninstead of 'git diff-files --stat' so we can get the expected\n\"modified\" status but avoids potential breakages related to\ninconsistency in the file creation time.\n\n# file present on-disk without modifications\n# use `--stat` to ignore file creation time differences in\n# unrefreshed index\ntest_all_match git diff-files --stat &&\ntest_all_match git diff-files --stat folder1/a &&\ntest_all_match git diff-files --stat \"folder*/a\" &&\n\n# file present on-disk with modifications\nrun_on_all ../edit-contents folder1/a &&\ntest_all_match git diff-files &&\ntest_all_match git diff-files folder1/a &&\ntest_all_match git diff-files \"folder*/a\"\n\n> On the other hand, if \"update-index --refresh\" is used in the test,\n> we may discover breakages caused by \"update-index\" not handling\n> the sparse index correctly. It would be outside the topic of this\n> series, so avoiding it would be simpler, but (1) if it is not broken,\n> then as you said, it would be a more direct way to test diff-files,\n> and (2) if it is broken, it would need to be fixed anyway, before or\n> after this series. So, I dunno...\n\nThanks\nShuqi\n"},{"id":"475317","messageId":"xmqq7cufxy5j.fsf@gitster.g","threadId":"59340","inReplyTo":"20230322161820.3609-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-13T21:36:24Z","receivedAt":"2023-04-13T21:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> Changes since v6:\n>\n> 1. Fix word wrap in commit message.\n>\n> 2. Use 'mkdir -p folder1' since full-checkout already have folder1.\n>\n> 3. Use `--stat` to ignore file creation time differences in unrefreshed\n> index.\n>\n> 4. In 'diff-files with pathspec outside sparse definition' add \n> 'git diff-files \"folder*/a\" to show that the result is the same with a \n> wildcard pathspec.\n>\n> 5. Create an 'ensure_expanded' to handle silent failures.\n\nIt seems that review comments have petered out.\n\nShall we mark this ready for 'next'?\n\nThanks.\n"},{"id":"475318","messageId":"c8091eaf-7c45-119c-fb67-9d7590364902@github.com","threadId":"59340","inReplyTo":"xmqq7cufxy5j.fsf@gitster.g","subject":"Re: [PATCH v7 0/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-13T21:38:53Z","receivedAt":"2023-04-13T21:39:00Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> Shuqi Liang writes:\n> \n>> Changes since v6:\n>>\n>> 1. Fix word wrap in commit message.\n>>\n>> 2. Use 'mkdir -p folder1' since full-checkout already have folder1.\n>>\n>> 3. Use `--stat` to ignore file creation time differences in unrefreshed\n>> index.\n>>\n>> 4. In 'diff-files with pathspec outside sparse definition' add \n>> 'git diff-files \"folder*/a\" to show that the result is the same with a \n>> wildcard pathspec.\n>>\n>> 5. Create an 'ensure_expanded' to handle silent failures.\n> \n> It seems that review comments have petered out.\n> \n> Shall we mark this ready for 'next'?\n\nSorry for the delay - I noticed a couple more things that might warrant\nchanges before moving to 'next' and am in the process of writing up that\nresponse. I'll send it within the hour.\n\n> \n> Thanks.\n\n"},{"id":"475319","messageId":"c382017a-8c65-24ba-5092-6b46428d8b9b@github.com","threadId":"59340","inReplyTo":"20230322161820.3609-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-13T21:54:57Z","receivedAt":"2023-04-13T21:55:03Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> diff --git a/builtin/diff-files.c b/builtin/diff-files.c\n> index dc991f753b..db90592090 100644\n> --- a/builtin/diff-files.c\n> +++ b/builtin/diff-files.c\n> @@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n> \t\tusage(diff_files_usage);\n> \n> \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> +\n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n> +\n> \trepo_init_revisions(the_repository, &rev, prefix);\n> \trev.abbrev = 0;\n> \n> @@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n> \t\tresult = -1;\n> \t\tgoto cleanup;\n> \t}\n> +\n> +\tif (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n> +\t\tensure_full_index(the_repository->index);\n\nAfter reviewing the 'diff-index' integration [1], I'm wondering whether we\nactually need pathspec expansion at all in this case. 'diff-files' compares\nthe working tree and the index, and will output a difference if the file on\ndisk differs from the index. But, if a file is SKIP_WORKTREE'd, that diff\nwill (I think?) always be empty. So, why would we need to expand a sparse\ndirectory to match a pathspec to its contents if we *know* that the diff\nwill be empty?\n\n[1] https://lore.kernel.org/git/20230408112342.404318-1-nanth.raghul@gmail.com/\n\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index d23041e27a..152f3f752e 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1401,6 +1401,30 @@ ensure_not_expanded () {\n> \ttest_region ! index ensure_full_index trace2.txt\n> }\n> \n> +ensure_expanded () {\n> +\trm -f trace2.txt &&\n> +\tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n> +\tthen\n> +\t\techo >>sparse-index/untracked.txt\n> +\tfi &&\n> +\n> +\tif test \"$1\" = \"!\"\n> +\tthen\n> +\t\tshift &&\n> +\t\ttest_must_fail env \\\n> +\t\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> +\t\t\tgit -C sparse-index \"$@\" \\\n> +\t\t\t>sparse-index-out \\\n> +\t\t\t2>sparse-index-error || return 1\n> +\telse\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> +\t\t\tgit -C sparse-index \"$@\" \\\n> +\t\t\t>sparse-index-out \\\n> +\t\t\t2>sparse-index-error || return 1\n> +\tfi &&\n> +\ttest_region index ensure_full_index trace2.txt\n> +}\n\nThis implementation duplicates a lot of the code from 'ensure_not_expanded'.\nCan 'ensure_expanded' and 'ensure_not_expanded' be refactored to call a\ncommon helper function (which contains the common code) instead?\n\n> +\n> test_expect_success 'sparse-index is not expanded' '\n> \tinit_repos &&\n> \n> @@ -2101,4 +2125,32 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n> \ttest_all_match git diff-files \"folder*/a\" \n> '\n> \n> +test_expect_success 'diff-files pathspec expands index when necessary' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\t\n> +\t# pathspec that should expand index\n> +\tensure_expanded diff-files \"*/a\" &&\n> +\tensure_expanded diff-files \"**a\" \n\nSimilar to the comments in my 'diff-index' review [2]:\n\n- The '**' in the pathspec doesn't do anything special unless using an\n explicit ':(glob)' pathspec. To make it clear that you're not trying to\n use a glob pathspec, you can use '*a' instead.\n- Why are these pathspecs in quotes, but those in 'sparse index is not\n expanded: diff-files' are?\n\n[2] https://lore.kernel.org/git/62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com/\n\n> +'\n> +\n> +test_expect_success 'sparse index is not expanded: diff-files' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\tensure_not_expanded diff-files &&\n> +\tensure_not_expanded diff-files deep/a &&\n> +\tensure_not_expanded diff-files deep/*\n> +'\n> +\n> test_done\n\n"},{"id":"475320","messageId":"4ffff8e9-3e03-9c5f-4a42-b9102ed24e66@github.com","threadId":"59340","inReplyTo":"20230322161820.3609-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v7 1/2] t1092: add tests for `git diff-files`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-13T21:56:35Z","receivedAt":"2023-04-13T21:56:41Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> +test_expect_success 'diff-files with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\ttest_all_match git diff-files &&\n> +\n> +\ttest_all_match git diff-files deep/a && \n> +\n> +\t# test wildcard\n> +\ttest_all_match git diff-files deep/*\n\nShould this pathspec be quoted (like you do for \"folder*/a\" below)?\n\n> +'\n> +\n> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\t# Add file to the index but outside of cone for sparse-checkout cases.\n> +\t# Add file to the index without sparse-checkout cases to ensure all have \n> +\t# same output.\n> +\trun_on_all mkdir -p folder1 &&\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\t# file present on-disk without modifications\n> +\t# use `--stat` to ignore file creation time differences in\n> +\t# unrefreshed index\n> +\ttest_all_match git diff-files --stat &&\n> +\ttest_all_match git diff-files --stat folder1/a &&\n> +\ttest_all_match git diff-files --stat \"folder*/a\" &&\n> +\n> +\t# file present on-disk with modifications\n> +\trun_on_all ../edit-contents folder1/a &&\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files folder1/a &&\n> +\ttest_all_match git diff-files \"folder*/a\" \n> +'\n> +\n> test_done\n\n"},{"id":"475723","messageId":"CAMO4yUF1P1Sv1aVJ1aw9US-QeNYD-GfaS7ndr=bwp-dgvOyexA@mail.gmail.com","threadId":"59340","inReplyTo":"c382017a-8c65-24ba-5092-6b46428d8b9b@github.com","subject":"Re: [PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-20T04:50:00Z","receivedAt":"2023-04-20T04:50:18Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Victoria,\n\nSorry for the late reply. I'm still in the middle of my final exams period.\n\nOn Thu, Apr 13, 2023 at 5:55 PM Victoria Dye wrote:\n>\n> Shuqi Liang wrote:\n> > diff --git a/builtin/diff-files.c b/builtin/diff-files.c\n> > index dc991f753b..db90592090 100644\n> > --- a/builtin/diff-files.c\n> > +++ b/builtin/diff-files.c\n> > @@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n> > usage(diff_files_usage);\n> >\n> > git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> > +\n> > + prepare_repo_settings(the_repository);\n> > + the_repository->settings.command_requires_full_index = 0;\n> > +\n> > repo_init_revisions(the_repository, &rev, prefix);\n> > rev.abbrev = 0;\n> >\n> > @@ -80,6 +84,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n> > result = -1;\n> > goto cleanup;\n> > }\n> > +\n> > + if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))\n> > + ensure_full_index(the_repository->index);\n>\n> After reviewing the 'diff-index' integration [1], I'm wondering whether we\n> actually need pathspec expansion at all in this case. 'diff-files' compares\n> the working tree and the index, and will output a difference if the file on\n> disk differs from the index. But, if a file is SKIP_WORKTREE'd, that diff\n> will (I think?) always be empty. So, why would we need to expand a sparse\n> directory to match a pathspec to its contents if we *know* that the diff\n> will be empty?\n>\n> [1] https://lore.kernel.org/git/20230408112342.404318-1-nanth.raghul@gmail.com/\n\n\nIt's true that in the case of 'diff-files', expanding the sparse directory to\nmatch a pathspec to its contents might not be necessary. If we don't use\npathspec expansion in this case. It could optimize for performance better.\n\nHowever, there could be some edge cases. if a user manually modifies the\ncontents of a SKIP_WORKTREE file in the working tree, the diff between\nthe working tree and the index would no longer be empty. So I think, In this\ncase, expanding the sparse directory might still be necessary to ensure the\ncorrect behavior of the 'diff-files' command.\n\n\n\n> > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> > index d23041e27a..152f3f752e 100755\n> > --- a/t/t1092-sparse-checkout-compatibility.sh\n> > +++ b/t/t1092-sparse-checkout-compatibility.sh\n> > @@ -1401,6 +1401,30 @@ ensure_not_expanded () {\n> > test_region ! index ensure_full_index trace2.txt\n> > }\n> >\n> > +ensure_expanded () {\n> > + rm -f trace2.txt &&\n> > + if test -z \"$WITHOUT_UNTRACKED_TXT\"\n> > + then\n> > + echo >>sparse-index/untracked.txt\n> > + fi &&\n> > +\n> > + if test \"$1\" = \"!\"\n> > + then\n> > + shift &&\n> > + test_must_fail env \\\n> > + GIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> > + git -C sparse-index \"$@\" \\\n> > + >sparse-index-out \\\n> > + 2>sparse-index-error || return 1\n> > + else\n> > + GIT_TRACE2_EVENT=\"$(pwd)/trace2.txt\" \\\n> > + git -C sparse-index \"$@\" \\\n> > + >sparse-index-out \\\n> > + 2>sparse-index-error || return 1\n> > + fi &&\n> > + test_region index ensure_full_index trace2.txt\n> > +}\n>\n> This implementation duplicates a lot of the code from 'ensure_not_expanded'.\n> Can 'ensure_expanded' and 'ensure_not_expanded' be refactored to call a\n> common helper function (which contains the common code) instead?\n\nWill do!\n\n> > +\n> > test_expect_success 'sparse-index is not expanded' '\n> > init_repos &&\n> >\n> > @@ -2101,4 +2125,32 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n> > test_all_match git diff-files \"folder*/a\"\n> > '\n> >\n> > +test_expect_success 'diff-files pathspec expands index when necessary' '\n> > + init_repos &&\n> > +\n> > + write_script edit-contents <<-\\EOF &&\n> > + echo text >>\"$1\"\n> > + EOF\n> > +\n> > + run_on_all ../edit-contents deep/a &&\n> > +\n> > + # pathspec that should expand index\n> > + ensure_expanded diff-files \"*/a\" &&\n> > + ensure_expanded diff-files \"**a\"\n>\n> Similar to the comments in my 'diff-index' review [2]:\n>\n> - The '**' in the pathspec doesn't do anything special unless using an\n> explicit ':(glob)' pathspec. To make it clear that you're not trying to\n> use a glob pathspec, you can use '*a' instead.\n\nWill do !\n\n> - Why are these pathspecs in quotes, but those in 'sparse index is not\n> expanded: diff-files' are?\n>\n> [2] https://lore.kernel.org/git/62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com/\n\n\nI quote around the pathspec to prevent shell expansion of the pathspec\npatterns by the shell before they are passed to the git command. \"*a\" and\n\"*/a\" have special characters ' * '. I use quotes to tell the shell\nto treat them\nas regular characters.\n\nIn 'sparse index is not expanded: diff-files''deep/a' does not contain any\nspecial characters that the shell would try to expand. So I use it without\ndouble quotes. And I think I need to add a double quote to 'deep/*'.\n\nThanks,\nShuqi\n"},{"id":"475735","messageId":"069a53ef-63b8-c1e3-7502-6728bda50665@github.com","threadId":"59340","inReplyTo":"CAMO4yUF1P1Sv1aVJ1aw9US-QeNYD-GfaS7ndr=bwp-dgvOyexA@mail.gmail.com","subject":"Re: [PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-20T15:26:40Z","receivedAt":"2023-04-20T15:26:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Hi Victoria,\n> \n> Sorry for the late reply. I'm still in the middle of my final exams period.\n\nNo problem at all, thanks for following up!\n\n> It's true that in the case of 'diff-files', expanding the sparse directory to\n> match a pathspec to its contents might not be necessary. If we don't use\n> pathspec expansion in this case. It could optimize for performance better.\n> \n> However, there could be some edge cases. if a user manually modifies the\n> contents of a SKIP_WORKTREE file in the working tree, the diff between\n> the working tree and the index would no longer be empty. So I think, In this\n> case, expanding the sparse directory might still be necessary to ensure the\n> correct behavior of the 'diff-files' command.\n\nIf a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be\nremoved from the file and the index expanded automatically [1]. If that\nmechanism is working properly, there would be no need to manually check the\npathspec and expand the index.\n\nHave you tried removing the 'pathspec_needs_expanded_index()' and running\nthe tests? If so, is 'diff-files' producing incorrect results? \n\n[1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/\n\n"},{"id":"475775","messageId":"CAMO4yUESBZw2Jr8y4NW_2N7640o2o2mpq58+nnC+3qffG3Y8=Q@mail.gmail.com","threadId":"59340","inReplyTo":"069a53ef-63b8-c1e3-7502-6728bda50665@github.com","subject":"Re: [PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-21T01:10:49Z","receivedAt":"2023-04-21T01:11:06Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Victoria,\n\n> If a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be\n> removed from the file and the index expanded automatically [1]. If that\n> mechanism is working properly, there would be no need to manually check the\n> pathspec and expand the index.\n>\n> Have you tried removing the 'pathspec_needs_expanded_index()' and running\n> the tests? If so, is 'diff-files' producing incorrect results?\n>\n> [1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/\n\nAs per your suggestion, I tried removing pathspec_needs_expanded_index()\nfrom the code, and 'diff-files pathspec expands index when necessary'\ntest failed.\n\nSo, I'm thinking about keeping it to ensure everything works properly.\nI'd like to know your thoughts on this. Should we keep it or explore\nother options?\n\nThanks,\nShuqi\n"},{"id":"475820","messageId":"111153b4-dba0-b533-fe49-57a6d5d3ba22@github.com","threadId":"59340","inReplyTo":"CAMO4yUESBZw2Jr8y4NW_2N7640o2o2mpq58+nnC+3qffG3Y8=Q@mail.gmail.com","subject":"Re: [PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-21T21:26:23Z","receivedAt":"2023-04-21T21:26:52Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Hi Victoria,\n> \n>> If a user manually modifies a SKIP_WORKTREE file, SKIP_WORKTREE will be\n>> removed from the file and the index expanded automatically [1]. If that\n>> mechanism is working properly, there would be no need to manually check the\n>> pathspec and expand the index.\n>>\n>> Have you tried removing the 'pathspec_needs_expanded_index()' and running\n>> the tests? If so, is 'diff-files' producing incorrect results?\n>>\n>> [1] https://lore.kernel.org/git/11d46a399d26c913787b704d2b7169cafc28d639.1642175983.git.gitgitgadget@gmail.com/\n> \n> As per your suggestion, I tried removing pathspec_needs_expanded_index()\n> from the code, and 'diff-files pathspec expands index when necessary'\n> test failed.\n> \n> So, I'm thinking about keeping it to ensure everything works properly.\n> I'd like to know your thoughts on this. Should we keep it or explore\n> other options?\n\nDid the test fail because the index wasn't expanded in a case where you\npreviously expended it to be expanded? Or because of the returned results of\n'diff-files' are invalid?\n\nOnly the latter represents incorrect behavior. If we're aren't expanding the\nindex for a case that was causing index expansion before *and* the\nuser-facing behavior is as-expected, that's the best-case scenario for a\nsparse index integration!\n\nTaking a step back, it's important to remember that the overarching goal of\nthe project is not just to switch 'command_requires_full_index' to '0'\neverywhere, but to find all of the places where Git is working with the\nindex and make sure that work can be done on a sparse directory.\n\nIn most cases, it's possible to adapt an index-related operation to work\nwith sparse directories (albeit with varying levels of complexity). The use\nof 'ensure_full_index()' is reserved for cases where it is _impossible_ to\nmake Git perform a given action on a sparse directory - expanding the index\ncompletely eliminates the performance gains had by using a sparse index, so\nit should be avoided at all costs.\n\nI hope that helps!\n\n> \n> Thanks,\n> Shuqi\n\n"},{"id":"475878","messageId":"CAMO4yUEsB=Rnoh44V1dykCkymF6qQJTiQyn_3s=L1PedaUcN7g@mail.gmail.com","threadId":"59340","inReplyTo":"111153b4-dba0-b533-fe49-57a6d5d3ba22@github.com","subject":"Re: [PATCH v7 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-22T21:25:44Z","receivedAt":"2023-04-22T21:26:02Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Victoria,\n\nOn Fri, Apr 21, 2023 at 5:26 PM Victoria Dye wrote:\n> Only the latter represents incorrect behavior. If we're aren't expanding the\n> index for a case that was causing index expansion before *and* the\n> user-facing behavior is as-expected, that's the best-case scenario for a\n> sparse index integration!\n>\n> Taking a step back, it's important to remember that the overarching goal of\n> the project is not just to switch 'command_requires_full_index' to '0'\n> everywhere, but to find all of the places where Git is working with the\n> index and make sure that work can be done on a sparse directory.\n>\n> In most cases, it's possible to adapt an index-related operation to work\n> with sparse directories (albeit with varying levels of complexity). The use\n> of 'ensure_full_index()' is reserved for cases where it is _impossible_ to\n> make Git perform a given action on a sparse directory - expanding the index\n> completely eliminates the performance gains had by using a sparse index, so\n> it should be avoided at all costs.\n>\n> I hope that helps!\n\nThanks for reminding me about the ultimate goal of sparse index\nintegration! I've learned a lot from it. After looking into the test\nfailure, it seems that the index didn't expand in cases where I expected\nit to. I'll go ahead and update my patch.\n\nThanks,\nShuqi\n"},{"id":"475888","messageId":"20230423010721.1402736-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230322161820.3609-1-cheskaqiqi@gmail.com","subject":"[PATCH v8 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-23T01:07:19Z","receivedAt":"2023-04-23T01:08:37Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v7:\n\n* Refactor the ensure_expanded and ensure_not_expanded functions by \nintroducing a helper function, ensure_index_state.\n\n* Delete the test 'diff-files pathspec expands index when necessary'.\n\n* Delete 'the pathspec_needs_expanded_index' function.\n\n* Add double quotes to \"deep/*\"\n\n* Change \"**a\" to \"*a\"\n\n* Updata commit message.\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 81 +++++++++++++++++++++++-\n 3 files changed, 85 insertions(+), 2 deletions(-)\n\nRange-diff:\n1: e2dcf9921e ! 1: d7f921c1a6 t1092: add tests for `git diff-files`\n @@ Commit message\n Signed-off-by: Shuqi Liang \n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse directory within submodules' '\n - \ttest_cmp actual expect\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded: write-tree' '\n + \tensure_not_expanded write-tree\n '\n \n +test_expect_success 'diff-files with pathspec inside sparse definition' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc\n +\ttest_all_match git diff-files deep/a && \n +\n +\t# test wildcard\n -+\ttest_all_match git diff-files deep/*\n ++\ttest_all_match git diff-files \"deep/*\"\n +'\n +\n +test_expect_success 'diff-files with pathspec outside sparse definition' '\n2: fb8edaf583 < -: ---------- diff-files: integrate with sparse index\n-: ---------- > 2: b44384ac94 diff-files: integrate with sparse index\n-- \n2.39.0\n\n"},{"id":"475889","messageId":"20230423010721.1402736-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230423010721.1402736-1-cheskaqiqi@gmail.com","subject":"[PATCH v8 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-23T01:07:20Z","receivedAt":"2023-04-23T01:08:37Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin with the sparse index\nfeature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\nit currently works with sparse-checkout and will still work with sparse\nindex after that integration.\n\nWhen adding tests against a sparse-checkout definition, we test two\nmodes: all changes are within the sparse-checkout cone and some changes\nare outside the sparse-checkout cone.\n\nIn order to have staged changes outside of the sparse-checkout cone,\nmake a directory called 'folder1' and copy `a` into 'folder1/a'.\n'folder1/a' is identical to `a` in the base commit. These make\n'folder1/a' in the index, while leaving it outside of the\nsparse-checkout definition. Test 'folder1/a'being present on-disk\nwithout modifications, then change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 0c784813f1..3c140103c5 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2108,4 +2108,50 @@ test_expect_success 'sparse-index is not expanded: write-tree' '\n \tensure_not_expanded write-tree\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a && \n+\n+\t# test wildcard\n+\ttest_all_match git diff-files \"deep/*\"\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# Add file to the index but outside of cone for sparse-checkout cases.\n+\t# Add file to the index without sparse-checkout cases to ensure all have \n+\t# same output.\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\t# file present on-disk without modifications\n+\t# use `--stat` to ignore file creation time differences in\n+\t# unrefreshed index\n+\ttest_all_match git diff-files --stat &&\n+\ttest_all_match git diff-files --stat folder1/a &&\n+\ttest_all_match git diff-files --stat \"folder*/a\" &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files folder1/a &&\n+\ttest_all_match git diff-files \"folder*/a\" \n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"475890","messageId":"20230423010721.1402736-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230423010721.1402736-1-cheskaqiqi@gmail.com","subject":"[PATCH v8 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-23T01:07:21Z","receivedAt":"2023-04-23T01:08:37Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`. Refactor the\nensure_expanded and ensure_not_expanded functions by introducing a\ncommon helper function, ensure_index_state. Add test to ensure the index\nis no expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 +++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 35 ++++++++++++++++++++++--\n 3 files changed, 39 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 60d1de0662..29165b3493 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- \"f2/f1/f1/*\"\n test_perf_on_all git write-tree\n test_perf_on_all git describe --dirty\n test_perf_on_all 'echo >>new && git describe --dirty'\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 3c140103c5..7ebcfe785e 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,7 +1377,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \t! test_region index ensure_full_index trace2.txt\n '\n \n-ensure_not_expanded () {\n+ensure_index_state () {\n+\tlocal expected_expansion=\"$1\"\n+\tshift\n+\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n@@ -1398,7 +1401,21 @@ ensure_not_expanded () {\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n \tfi &&\n-\ttest_region ! index ensure_full_index trace2.txt\n+\n+\tif [ \"$expected_expansion\" = \"expanded\" ]\n+\tthen\n+\t\ttest_region index ensure_full_index trace2.txt\n+\telse\n+\t\ttest_region ! index ensure_full_index trace2.txt\n+\tfi\n+}\n+\n+ensure_expanded () {\n+\tensure_index_state \"expanded\" \"$@\"\n+}\n+\n+ensure_not_expanded () {\n+\tensure_index_state \"not_expanded\" \"$@\"\n }\n \n test_expect_success 'sparse-index is not expanded' '\n@@ -2154,4 +2171,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files \"folder*/a\" \n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files \"deep/*\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476037","messageId":"xmqqo7nb3nmy.fsf@gitster.g","threadId":"59340","inReplyTo":"20230423010721.1402736-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v8 0/2] diff-files: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T16:57:57Z","receivedAt":"2023-04-25T16:58:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> Changes since v7:\n>\n> * Refactor the ensure_expanded and ensure_not_expanded functions by \n> introducing a helper function, ensure_index_state.\n>\n> * Delete the test 'diff-files pathspec expands index when necessary'.\n>\n> * Delete 'the pathspec_needs_expanded_index' function.\n>\n> * Add double quotes to \"deep/*\"\n>\n> * Change \"**a\" to \"*a\"\n>\n> * Updata commit message.\n\nThese patches seem to have some whitespace errors.\n"},{"id":"476386","messageId":"xmqqttwv3dz5.fsf@gitster.g","threadId":"59340","inReplyTo":"20230423010721.1402736-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v8 0/2] diff-files: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T22:04:46Z","receivedAt":"2023-05-01T22:04:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> Changes since v7:\n>\n> * Refactor the ensure_expanded and ensure_not_expanded functions by \n> introducing a helper function, ensure_index_state.\n>\n> * Delete the test 'diff-files pathspec expands index when necessary'.\n>\n> * Delete 'the pathspec_needs_expanded_index' function.\n>\n> * Add double quotes to \"deep/*\"\n>\n> * Change \"**a\" to \"*a\"\n>\n> * Updata commit message.\n\nThis round did not see any reactions; is everybody happy to see us\ndeclare victory and merge it down to 'next'?\n\nThanks.\n"},{"id":"476391","messageId":"427dff83-536d-46ee-6326-15e2f548082c@github.com","threadId":"59340","inReplyTo":"20230423010721.1402736-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v8 2/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-05-01T22:26:51Z","receivedAt":"2023-05-01T22:26:58Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 3c140103c5..7ebcfe785e 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1377,7 +1377,10 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n> \t! test_region index ensure_full_index trace2.txt\n> '\n> \n> -ensure_not_expanded () {\n> +ensure_index_state () {\n> +\tlocal expected_expansion=\"$1\"\n> +\tshift\n> +\n> \trm -f trace2.txt &&\n> \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n> \tthen\n> @@ -1398,7 +1401,21 @@ ensure_not_expanded () {\n> \t\t\t>sparse-index-out \\\n> \t\t\t2>sparse-index-error || return 1\n> \tfi &&\n> -\ttest_region ! index ensure_full_index trace2.txt\n> +\n> +\tif [ \"$expected_expansion\" = \"expanded\" ]\n> +\tthen\n> +\t\ttest_region index ensure_full_index trace2.txt\n> +\telse\n> +\t\ttest_region ! index ensure_full_index trace2.txt\n> +\tfi\n> +}\n> +\n> +ensure_expanded () {\n> +\tensure_index_state \"expanded\" \"$@\"\n> +}\n> +\n> +ensure_not_expanded () {\n> +\tensure_index_state \"not_expanded\" \"$@\"\n> }\n\nThis still seems a bit more complicated than necessary (mainly due to the\nnew string comparison & local arg). What about something like this (applied\non top)?\n\n-------- 8< -------- 8< -------- 8< --------\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 9d11d28891..333822f322 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,10 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \t! test_region index ensure_full_index trace2.txt\n '\n \n-ensure_index_state () {\n-\tlocal expected_expansion=\"$1\"\n-\tshift\n-\n+run_sparse_index_trace2 () {\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n@@ -1400,22 +1397,17 @@ ensure_index_state () {\n \t\t\tgit -C sparse-index \"$@\" \\\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n-\tfi &&\n-\n-\tif [ \"$expected_expansion\" = \"expanded\" ]\n-\tthen\n-\t\ttest_region index ensure_full_index trace2.txt\n-\telse\n-\t\ttest_region ! index ensure_full_index trace2.txt\n \tfi\n }\n \n ensure_expanded () {\n-\tensure_index_state \"expanded\" \"$@\"\n+\trun_sparse_index_trace2 \"$@\" &&\n+\ttest_region index ensure_full_index trace2.txt\n }\n \n ensure_not_expanded () {\n-\tensure_index_state \"not_expanded\" \"$@\"\n+\trun_sparse_index_trace2 \"$@\" &&\n+\ttest_region ! index ensure_full_index trace2.txt\n }\n \n test_expect_success 'sparse-index is not expanded' '\n-------- >8 -------- >8 -------- >8 --------\n\nThat said, given that this is my only complaint with this iteration (and\nit's pretty subjective), if others are happy with it then I'm not opposed to\nmerging to 'next'.\n\n"},{"id":"476440","messageId":"20230502172335.478312-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230423010721.1402736-1-cheskaqiqi@gmail.com","subject":"[PATCH v9 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-02T17:23:33Z","receivedAt":"2023-05-02T17:24:04Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v8:\n\n* Fix white space problem.\n\n* Simplified the refactored function based on Victoria's suggestion.\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 73 +++++++++++++++++++++++-\n 3 files changed, 77 insertions(+), 2 deletions(-)\n\nRange-diff against v8:\n1: d7f921c1a6 ! 1: d78513af83 t1092: add tests for `git diff-files`\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\n +\ttest_all_match git diff-files &&\n +\n -+\ttest_all_match git diff-files deep/a && \n ++\ttest_all_match git diff-files deep/a &&\n +\n +\t# test wildcard\n +\ttest_all_match git diff-files \"deep/*\"\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\tEOF\n +\n +\t# Add file to the index but outside of cone for sparse-checkout cases.\n -+\t# Add file to the index without sparse-checkout cases to ensure all have \n ++\t# Add file to the index without sparse-checkout cases to ensure all have\n +\t# same output.\n +\trun_on_all mkdir -p folder1 &&\n +\trun_on_all cp a folder1/a &&\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\trun_on_all ../edit-contents folder1/a &&\n +\ttest_all_match git diff-files &&\n +\ttest_all_match git diff-files folder1/a &&\n -+\ttest_all_match git diff-files \"folder*/a\" \n ++\ttest_all_match git diff-files \"folder*/a\"\n +'\n +\n test_done\n2: b44384ac94 ! 2: a2454befa0 diff-files: integrate with sparse index\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'index.sparse disa\n '\n \n -ensure_not_expanded () {\n -+ensure_index_state () {\n -+\tlocal expected_expansion=\"$1\"\n -+\tshift\n -+\n ++run_sparse_index_trace2 () {\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n @@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {\n + \t\t\tgit -C sparse-index \"$@\" \\\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n - \tfi &&\n --\ttest_region ! index ensure_full_index trace2.txt\n -+\n -+\tif [ \"$expected_expansion\" = \"expanded\" ]\n -+\tthen\n -+\t\ttest_region index ensure_full_index trace2.txt\n -+\telse\n -+\t\ttest_region ! index ensure_full_index trace2.txt\n +-\tfi &&\n +\tfi\n +}\n +\n +ensure_expanded () {\n -+\tensure_index_state \"expanded\" \"$@\"\n ++\trun_sparse_index_trace2 \"$@\" &&\n ++\ttest_region index ensure_full_index trace2.txt\n +}\n +\n +ensure_not_expanded () {\n -+\tensure_index_state \"not_expanded\" \"$@\"\n ++\trun_sparse_index_trace2 \"$@\" &&\n + \ttest_region ! index ensure_full_index trace2.txt\n }\n \n - test_expect_success 'sparse-index is not expanded' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '\n - \ttest_all_match git diff-files \"folder*/a\" \n + \ttest_all_match git diff-files \"folder*/a\"\n '\n \n +test_expect_success 'sparse index is not expanded: diff-files' '\n-- \n2.39.0\n\n"},{"id":"476441","messageId":"20230502172335.478312-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230502172335.478312-1-cheskaqiqi@gmail.com","subject":"[PATCH v9 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-02T17:23:34Z","receivedAt":"2023-05-02T17:24:07Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin with the sparse index\nfeature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\nit currently works with sparse-checkout and will still work with sparse\nindex after that integration.\n\nWhen adding tests against a sparse-checkout definition, we test two\nmodes: all changes are within the sparse-checkout cone and some changes\nare outside the sparse-checkout cone.\n\nIn order to have staged changes outside of the sparse-checkout cone,\nmake a directory called 'folder1' and copy `a` into 'folder1/a'.\n'folder1/a' is identical to `a` in the base commit. These make\n'folder1/a' in the index, while leaving it outside of the\nsparse-checkout definition. Test 'folder1/a'being present on-disk\nwithout modifications, then change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 0c784813f1..053435bb0c 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2108,4 +2108,50 @@ test_expect_success 'sparse-index is not expanded: write-tree' '\n \tensure_not_expanded write-tree\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a &&\n+\n+\t# test wildcard\n+\ttest_all_match git diff-files \"deep/*\"\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# Add file to the index but outside of cone for sparse-checkout cases.\n+\t# Add file to the index without sparse-checkout cases to ensure all have\n+\t# same output.\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\t# file present on-disk without modifications\n+\t# use `--stat` to ignore file creation time differences in\n+\t# unrefreshed index\n+\ttest_all_match git diff-files --stat &&\n+\ttest_all_match git diff-files --stat folder1/a &&\n+\ttest_all_match git diff-files --stat \"folder*/a\" &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files folder1/a &&\n+\ttest_all_match git diff-files \"folder*/a\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476442","messageId":"20230502172335.478312-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230502172335.478312-1-cheskaqiqi@gmail.com","subject":"[PATCH v9 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-02T17:23:35Z","receivedAt":"2023-05-02T17:24:14Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`. Refactor the\nensure_expanded and ensure_not_expanded functions by introducing a\ncommon helper function, ensure_index_state. Add test to ensure the index\nis no expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 60d1de0662..29165b3493 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- \"f2/f1/f1/*\"\n test_perf_on_all git write-tree\n test_perf_on_all git describe --dirty\n test_perf_on_all 'echo >>new && git describe --dirty'\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 053435bb0c..333822f322 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \t! test_region index ensure_full_index trace2.txt\n '\n \n-ensure_not_expanded () {\n+run_sparse_index_trace2 () {\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n@@ -1397,7 +1397,16 @@ ensure_not_expanded () {\n \t\t\tgit -C sparse-index \"$@\" \\\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n-\tfi &&\n+\tfi\n+}\n+\n+ensure_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n+\ttest_region index ensure_full_index trace2.txt\n+}\n+\n+ensure_not_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n \ttest_region ! index ensure_full_index trace2.txt\n }\n \n@@ -2154,4 +2163,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files \"folder*/a\"\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files \"deep/*\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476453","messageId":"xmqqjzxqzgbi.fsf@gitster.g","threadId":"59340","inReplyTo":"20230502172335.478312-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-02T19:25:21Z","receivedAt":"2023-05-02T19:25:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n\nIn \"sparse\" directories at this point of test, \"folder2\" is outside\nthe cone(s) of interest and is not instantiated. The reason why\nthe command fails is because the command line parsing that is\ngeneric to all users of the revision machinery requires you to have\na disambiguating double-dash before such a pathspec that tries to\nmatch a path that does not exist in the working tree and is not\nspecific to \"diff-files\".\n\nI wonder how interesting and useful this test is. Without\naccompanying test that uses disambiguating double-dash properly,\ne.g. \"git diff-files -- folder2\", I doubt it is very much useful.\n\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\t# Add file to the index but outside of cone for sparse-checkout cases.\n> +\t# Add file to the index without sparse-checkout cases to ensure all have\n> +\t# same output.\n> +\trun_on_all mkdir -p folder1 &&\n> +\trun_on_all cp a folder1/a &&\n\nNow, \"folder1\" also has not been instantiated in sparse ones while\nthe full one of course has it, so \"-p\" in \"mkdir -p\" makes sense.\nAfter these commands, all three will share the same \"folder1/a\".\n\n> +\t# file present on-disk without modifications\n> +\t# use `--stat` to ignore file creation time differences in\n> +\t# unrefreshed index\n> +\ttest_all_match git diff-files --stat &&\n> +\ttest_all_match git diff-files --stat folder1/a &&\n> +\ttest_all_match git diff-files --stat \"folder*/a\" &&\n\nBecause in all three repositories, \"folder1/a\" exists in the working\ntree, the \"you need to disambiguate\" error like the first test\n(whose utility I questioned) would not trigger.\n\nWhat does this demonstrate, though? That instantiating a file on\nthe working tree, even outside the cone(s) of interest in a sparsely\nchecked out working tree, makes it part of the interesting set\nautomatically? As there is no difference between the indexed\ncontents and what is in the working tree, we cannot tell from this\ntest if that is the case (not a complaint, just an observation).\n\nBut ...\n\n> +\t# file present on-disk with modifications\n> +\trun_on_all ../edit-contents folder1/a &&\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files folder1/a &&\n> +\ttest_all_match git diff-files \"folder*/a\"\n\n... it is shown by doing the same test with modified contents?\n\nFor consistency with the earlier \"the same contents\" test, we should\nuse \"--stat\" here, too. Or even \"--stat -p\".\n\nAlternatively, we could refresh the index before running diff-files\n(here and also before the earlier \"the same contents\" test), I\nguess.\n\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"476518","messageId":"2fa835b8-1c9b-67d3-aa4a-70a978b5f20d@github.com","threadId":"59340","inReplyTo":"xmqqjzxqzgbi.fsf@gitster.g","subject":"Re: [PATCH v9 1/2] t1092: add tests for `git diff-files`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-05-03T16:37:37Z","receivedAt":"2023-05-03T16:37:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> Shuqi Liang writes:\n> \n>> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n>> +\tinit_repos &&\n>> +\n>> +\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n> \n> In \"sparse\" directories at this point of test, \"folder2\" is outside\n> the cone(s) of interest and is not instantiated. The reason why\n> the command fails is because the command line parsing that is\n> generic to all users of the revision machinery requires you to have\n> a disambiguating double-dash before such a pathspec that tries to\n> match a path that does not exist in the working tree and is not\n> specific to \"diff-files\".\n> \n> I wonder how interesting and useful this test is. Without\n> accompanying test that uses disambiguating double-dash properly,\n> e.g. \"git diff-files -- folder2\", I doubt it is very much useful.\n\nI agree, this test isn't helpful as-is. With the '--', 'diff-files'\nshouldn't fail, so in the interest of avoiding unexpected regressions in the\nfuture (masked by 'test_must_fail'), I think this test should be updated as\nyou described. \n\n>> +\t# file present on-disk without modifications\n>> +\t# use `--stat` to ignore file creation time differences in\n>> +\t# unrefreshed index\n>> +\ttest_all_match git diff-files --stat &&\n>> +\ttest_all_match git diff-files --stat folder1/a &&\n>> +\ttest_all_match git diff-files --stat \"folder*/a\" &&\n> \n> Because in all three repositories, \"folder1/a\" exists in the working\n> tree, the \"you need to disambiguate\" error like the first test\n> (whose utility I questioned) would not trigger.\n> \n> What does this demonstrate, though? That instantiating a file on\n> the working tree, even outside the cone(s) of interest in a sparsely\n> checked out working tree, makes it part of the interesting set\n> automatically? As there is no difference between the indexed\n> contents and what is in the working tree, we cannot tell from this\n> test if that is the case (not a complaint, just an observation).\n\nThis was meant [1] to check whether there are any issues expanding the index\n(specifically, the 'folder1/' sparse directory) before comparing to the \nnow-on-disk 'folder1/a'. \n\nHowever...\n\n[1] https://lore.kernel.org/git/b537d855-edb7-4f67-de08-d651868247a5@github.com/\n\n> \n> But ...\n> \n>> +\t# file present on-disk with modifications\n>> +\trun_on_all ../edit-contents folder1/a &&\n>> +\ttest_all_match git diff-files &&\n>> +\ttest_all_match git diff-files folder1/a &&\n>> +\ttest_all_match git diff-files \"folder*/a\"\n> \n> ... it is shown by doing the same test with modified contents?\n> \n> For consistency with the earlier \"the same contents\" test, we should\n> use \"--stat\" here, too. Or even \"--stat -p\".\n> \n> Alternatively, we could refresh the index before running diff-files\n> (here and also before the earlier \"the same contents\" test), I\n> guess.\n\n...to your point, we probably don't need both the \"unmodified folder1/a\ndiff-files\" *and* \"modified folder1/a diff-files\" tests. In fact, the empty\noutput of \"unmodified folder1/a\" could be caused by either \"this file is\nunmodified\" or \"this file isn't in the index\", so the test might pass even\nif there's an issue with index expansion. That isn't a problem in the\n\"modified folder1/a\" case, since we're expecting to see - and comparing the\ncontents of - a diff.\n\nI think we can drop the 'diff-files --stat' tests and go straight to the\n'run_on_all ../edit-contents folder1/a'. Adding '--' here to disambiguate\nthe pathspecs might be nice as well.\n\n> \n>> +'\n>> +\n>> test_done\n> \n> Thanks.\n\nThanks for the detailed review, apologies for missing these issues earlier.\n\n"},{"id":"476572","messageId":"20230503215549.511999-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230502172335.478312-1-cheskaqiqi@gmail.com","subject":"[PATCH v10 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-03T21:55:47Z","receivedAt":"2023-05-03T21:56:22Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v9:\n\n* Replace the unhelpful test with a double-dash case to prevent\nregressions. \n\n* Remove the unmodified test because its empty output could pass\neven with an index expansion issue.\n\n* Update the relevant commit message.\n\n* Did not add \" -- \" in modified test as suggested sine \n\"folder1/a\" exists in the working tree\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-\n 3 files changed, 70 insertions(+), 2 deletions(-)\n\nRange-diff against v9:\n1: d78513af83 ! 1: 3b284bdf3b t1092: add tests for `git diff-files`\n @@ Commit message\n make a directory called 'folder1' and copy `a` into 'folder1/a'.\n 'folder1/a' is identical to `a` in the base commit. These make\n 'folder1/a' in the index, while leaving it outside of the\n - sparse-checkout definition. Test 'folder1/a'being present on-disk\n - without modifications, then change content inside 'folder1/a' in order\n + sparse-checkout definition. Change content inside 'folder1/a' in order\n to test 'folder1/a' being present on-disk with modifications.\n \n Signed-off-by: Shuqi Liang \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +test_expect_success 'diff-files with pathspec outside sparse definition' '\n +\tinit_repos &&\n +\n -+\ttest_sparse_match test_must_fail git diff-files folder2/a &&\n ++\ttest_sparse_match git diff-files -- folder2/a &&\n +\n +\twrite_script edit-contents <<-\\EOF &&\n +\techo text >>\"$1\"\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\trun_on_all mkdir -p folder1 &&\n +\trun_on_all cp a folder1/a &&\n +\n -+\t# file present on-disk without modifications\n -+\t# use `--stat` to ignore file creation time differences in\n -+\t# unrefreshed index\n -+\ttest_all_match git diff-files --stat &&\n -+\ttest_all_match git diff-files --stat folder1/a &&\n -+\ttest_all_match git diff-files --stat \"folder*/a\" &&\n -+\n +\t# file present on-disk with modifications\n +\trun_on_all ../edit-contents folder1/a &&\n +\ttest_all_match git diff-files &&\n2: a2454befa0 = 2: 15472db302 diff-files: integrate with sparse index\n-- \n2.39.0\n\n"},{"id":"476573","messageId":"20230503215549.511999-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230503215549.511999-1-cheskaqiqi@gmail.com","subject":"[PATCH v10 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-03T21:55:48Z","receivedAt":"2023-05-03T21:56:22Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin with the sparse index\nfeature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\nit currently works with sparse-checkout and will still work with sparse\nindex after that integration.\n\nWhen adding tests against a sparse-checkout definition, we test two\nmodes: all changes are within the sparse-checkout cone and some changes\nare outside the sparse-checkout cone.\n\nIn order to have staged changes outside of the sparse-checkout cone,\nmake a directory called 'folder1' and copy `a` into 'folder1/a'.\n'folder1/a' is identical to `a` in the base commit. These make\n'folder1/a' in the index, while leaving it outside of the\nsparse-checkout definition. Change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 0c784813f1..eddae7ee08 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2108,4 +2108,43 @@ test_expect_success 'sparse-index is not expanded: write-tree' '\n \tensure_not_expanded write-tree\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a &&\n+\n+\t# test wildcard\n+\ttest_all_match git diff-files \"deep/*\"\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match git diff-files -- folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# Add file to the index but outside of cone for sparse-checkout cases.\n+\t# Add file to the index without sparse-checkout cases to ensure all have\n+\t# same output.\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\t# file present on-disk with modifications\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files folder1/a &&\n+\ttest_all_match git diff-files \"folder*/a\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476574","messageId":"20230503215549.511999-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230503215549.511999-1-cheskaqiqi@gmail.com","subject":"[PATCH v10 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-03T21:55:49Z","receivedAt":"2023-05-03T21:56:25Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`. Refactor the\nensure_expanded and ensure_not_expanded functions by introducing a\ncommon helper function, ensure_index_state. Add test to ensure the index\nis no expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 60d1de0662..29165b3493 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- \"f2/f1/f1/*\"\n test_perf_on_all git write-tree\n test_perf_on_all git describe --dirty\n test_perf_on_all 'echo >>new && git describe --dirty'\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex eddae7ee08..ed9bf466c2 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \t! test_region index ensure_full_index trace2.txt\n '\n \n-ensure_not_expanded () {\n+run_sparse_index_trace2 () {\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n@@ -1397,7 +1397,16 @@ ensure_not_expanded () {\n \t\t\tgit -C sparse-index \"$@\" \\\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n-\tfi &&\n+\tfi\n+}\n+\n+ensure_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n+\ttest_region index ensure_full_index trace2.txt\n+}\n+\n+ensure_not_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n \ttest_region ! index ensure_full_index trace2.txt\n }\n \n@@ -2147,4 +2156,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files \"folder*/a\"\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files \"deep/*\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476581","messageId":"xmqqpm7hm1yy.fsf@gitster.g","threadId":"59340","inReplyTo":"20230503215549.511999-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v10 1/2] t1092: add tests for `git diff-files`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-03T23:25:57Z","receivedAt":"2023-05-03T23:26:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> +\ttest_sparse_match git diff-files -- folder2/a &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n\n> +\t# Add file to the index but outside of cone for sparse-checkout cases.\n> +\t# Add file to the index without sparse-checkout cases to ensure all have\n> +\t# same output.\n\nAre these two sentences supposed to explain the following two\ncommands that are run in all three repositories? As far as I can\ntell, no command in this test adds anything to the index. Perhaps\nit is a leftover/stale comment from previous rounds or something?\n\n> +\trun_on_all mkdir -p folder1 &&\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\t# file present on-disk with modifications\n> +\trun_on_all ../edit-contents folder1/a &&\n\nWith the above three commands taken together, we have made folder1/a\non the working tree different from what is in the index, so we can\nexpect to see differences between the index and the working tree\nfiles. \"# file present on-disk with modifications\" is a good way to\nsummarize a half of what we are trying to achieve, with the other\nhalf being that we try to do that to a path outside the cone of\ninterest.\n\nSo, perhaps get rid of this comment between the step 2 and 3 of the\npreparation, and rewrite the comment before the step 1 (i.e. \"mkdir\n-p\") of the preparation to explain the whole thing, perhaps like:\n\n\t# The directory \"folder1\" is outside the cone of interest\n\t# and may not exist in the sparse checkout repositories.\n # Create it as needed, add file \"folder1/a\" there with\n\t# contents that is different from the staged version.\n\nto explain what scenario these three run_on_all commands are trying\nto create?\n\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files folder1/a &&\n> +\ttest_all_match git diff-files \"folder*/a\"\n\nI think Victoria suggested to use the double-dash disambiguators for\nthese tests, and it may not be a bad idea to do so, i.e.\n\n\ttest_all_match git diff-files &&\n\ttest_all_match git diff-files -- folder1/a &&\n\ttest_all_match git diff-files -- folder\\*/a\n\n> +'\n> +\n> test_done\n\nThanks.\n"},{"id":"476784","messageId":"20230508184652.4283-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230503215549.511999-1-cheskaqiqi@gmail.com","subject":"[PATCH v11 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-08T18:46:50Z","receivedAt":"2023-05-08T18:47:09Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Changes since v10:\n\n* Rewrite the comment before the \"mkdir -p\"\n\n* Add \" -- \" in modified test to prevent regressions.\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-\n 3 files changed, 70 insertions(+), 2 deletions(-)\n\nRange-diff against v10:\n1: 3b284bdf3b ! 1: 3e96a0c136 t1092: add tests for `git diff-files`\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\techo text >>\"$1\"\n +\tEOF\n +\n -+\t# Add file to the index but outside of cone for sparse-checkout cases.\n -+\t# Add file to the index without sparse-checkout cases to ensure all have\n -+\t# same output.\n ++\t# The directory \"folder1\" is outside the cone of interest\n ++\t# and may not exist in the sparse checkout repositories.\n ++\t# Create it as needed, add file \"folder1/a\" there with\n ++\t# contents that is different from the staged version.\n +\trun_on_all mkdir -p folder1 &&\n +\trun_on_all cp a folder1/a &&\n +\n -+\t# file present on-disk with modifications\n +\trun_on_all ../edit-contents folder1/a &&\n +\ttest_all_match git diff-files &&\n -+\ttest_all_match git diff-files folder1/a &&\n -+\ttest_all_match git diff-files \"folder*/a\"\n ++\ttest_all_match git diff-files -- folder1/a &&\n ++\ttest_all_match git diff-files -- \"folder*/a\"\n +'\n +\n test_done\n2: 15472db302 ! 2: 2c53fedf08 diff-files: integrate with sparse index\n @@ t/t1092-sparse-checkout-compatibility.sh: ensure_not_expanded () {\n }\n \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '\n - \ttest_all_match git diff-files \"folder*/a\"\n + \ttest_all_match git diff-files -- \"folder*/a\"\n '\n \n +test_expect_success 'sparse index is not expanded: diff-files' '\n-- \n2.39.0\n\n"},{"id":"476785","messageId":"20230508184652.4283-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230508184652.4283-1-cheskaqiqi@gmail.com","subject":"[PATCH v11 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-08T18:46:51Z","receivedAt":"2023-05-08T18:47:14Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin with the sparse index\nfeature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\nit currently works with sparse-checkout and will still work with sparse\nindex after that integration.\n\nWhen adding tests against a sparse-checkout definition, we test two\nmodes: all changes are within the sparse-checkout cone and some changes\nare outside the sparse-checkout cone.\n\nIn order to have staged changes outside of the sparse-checkout cone,\nmake a directory called 'folder1' and copy `a` into 'folder1/a'.\n'folder1/a' is identical to `a` in the base commit. These make\n'folder1/a' in the index, while leaving it outside of the\nsparse-checkout definition. Change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 0c784813f1..efc709afa5 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2108,4 +2108,43 @@ test_expect_success 'sparse-index is not expanded: write-tree' '\n \tensure_not_expanded write-tree\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files deep/a &&\n+\n+\t# test wildcard\n+\ttest_all_match git diff-files \"deep/*\"\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match git diff-files -- folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# The directory \"folder1\" is outside the cone of interest\n+\t# and may not exist in the sparse checkout repositories.\n+\t# Create it as needed, add file \"folder1/a\" there with\n+\t# contents that is different from the staged version.\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files -- folder1/a &&\n+\ttest_all_match git diff-files -- \"folder*/a\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476786","messageId":"20230508184652.4283-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230508184652.4283-1-cheskaqiqi@gmail.com","subject":"[PATCH v11 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-08T18:46:52Z","receivedAt":"2023-05-08T18:47:16Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`. Refactor the\nensure_expanded and ensure_not_expanded functions by introducing a\ncommon helper function, ensure_index_state. Add test to ensure the index\nis no expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------\n2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 60d1de0662..29165b3493 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- \"f2/f1/f1/*\"\n test_perf_on_all git write-tree\n test_perf_on_all git describe --dirty\n test_perf_on_all 'echo >>new && git describe --dirty'\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex efc709afa5..176153738a 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \t! test_region index ensure_full_index trace2.txt\n '\n \n-ensure_not_expanded () {\n+run_sparse_index_trace2 () {\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n@@ -1397,7 +1397,16 @@ ensure_not_expanded () {\n \t\t\tgit -C sparse-index \"$@\" \\\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n-\tfi &&\n+\tfi\n+}\n+\n+ensure_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n+\ttest_region index ensure_full_index trace2.txt\n+}\n+\n+ensure_not_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n \ttest_region ! index ensure_full_index trace2.txt\n }\n \n@@ -2147,4 +2156,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files -- \"folder*/a\"\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files deep/a &&\n+\tensure_not_expanded diff-files \"deep/*\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476836","messageId":"8489e272-ccb9-62b1-992e-d305bb27c895@github.com","threadId":"59340","inReplyTo":"20230508184652.4283-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v11 1/2] t1092: add tests for `git diff-files`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-05-08T22:25:45Z","receivedAt":"2023-05-08T22:25:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> +test_expect_success 'diff-files with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\trun_on_all ../edit-contents deep/a &&\n> +\n> +\ttest_all_match git diff-files &&\n> +\n> +\ttest_all_match git diff-files deep/a &&\n> +\n> +\t# test wildcard\n> +\ttest_all_match git diff-files \"deep/*\"\n\nYou added the '--' separator below, but not here. Was that intentional, or\nshould these have it as well? It doesn't make much of a practical difference\nin this case, but it would be nice to remain consistent across all tests of\n'diff-files' that you're adding. \n\nThe same goes for some of the tests in patch 2 [1] ('sparse index is not\nexpanded: diff-files' and the perf tests).\n\n[1] https://lore.kernel.org/git/20230508184652.4283-3-cheskaqiqi@gmail.com/\n\n> +'\n> +\n> +test_expect_success 'diff-files with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\ttest_sparse_match git diff-files -- folder2/a &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>\"$1\"\n> +\tEOF\n> +\n> +\t# The directory \"folder1\" is outside the cone of interest\n> +\t# and may not exist in the sparse checkout repositories.\n> +\t# Create it as needed, add file \"folder1/a\" there with\n> +\t# contents that is different from the staged version.\n\nnit: 'folder1/' *definitely* won't be present in the sparse-checkout\nrepositories, so \"will not\" would be more accurate than \"may not\".\nOtherwise, this comment is clearer than before & better explains what's\ngoing on here.\n\n> +\trun_on_all mkdir -p folder1 &&\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\trun_on_all ../edit-contents folder1/a &&\n> +\ttest_all_match git diff-files &&\n> +\ttest_all_match git diff-files -- folder1/a &&\n> +\ttest_all_match git diff-files -- \"folder*/a\"\n> +'\n> +\n> test_done\n\n"},{"id":"476916","messageId":"20230509194241.469477-1-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230508184652.4283-1-cheskaqiqi@gmail.com","subject":"[PATCH v12 0/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-09T19:42:39Z","receivedAt":"2023-05-09T19:43:03Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"* Add the '--' in all test to remain consistent.\n\n* Change 'may not' to 'will not'.\n\n\nShuqi Liang (2):\n t1092: add tests for `git diff-files`\n diff-files: integrate with sparse index\n\n builtin/diff-files.c | 4 ++\n t/perf/p2000-sparse-operations.sh | 2 +\n t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-\n 3 files changed, 70 insertions(+), 2 deletions(-)\n\nRange-diff against v11:\n1: 3e96a0c136 ! 1: eb74730813 t1092: add tests for `git diff-files`\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\n +\ttest_all_match git diff-files &&\n +\n -+\ttest_all_match git diff-files deep/a &&\n ++\ttest_all_match git diff-files -- deep/a &&\n +\n +\t# test wildcard\n -+\ttest_all_match git diff-files \"deep/*\"\n ++\ttest_all_match git diff-files -- \"deep/*\"\n +'\n +\n +test_expect_success 'diff-files with pathspec outside sparse definition' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n\n +\tEOF\n +\n +\t# The directory \"folder1\" is outside the cone of interest\n -+\t# and may not exist in the sparse checkout repositories.\n ++\t# and will not exist in the sparse checkout repositories.\n +\t# Create it as needed, add file \"folder1/a\" there with\n +\t# contents that is different from the staged version.\n +\trun_on_all mkdir -p folder1 &&\n2: 2c53fedf08 ! 2: 11affce5b7 diff-files: integrate with sparse index\n @@ Commit message\n diff-files' and a ~97% execution time reduction for 'git diff-files'\n for a file using a sparse index:\n \n - Test before after\n - -----------------------------------------------------------------\n - 2000.78: git diff-files (full-v3) 0.09 0.08 -11.1%\n - 2000.79: git diff-files (full-v4) 0.09 0.09 +0.0%\n - 2000.80: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n - 2000.81: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n - 2000.82: git diff-files f2/f4/a (full-v3) 0.06 0.07 +16.7%\n - 2000.83: git diff-files f2/f4/a (full-v4) 0.08 0.08 +0.0%\n - 2000.84: git diff-files f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n - 2000.85: git diff-files f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n + Test before after\n + -----------------------------------------------------------------------\n + 2000.94: git diff-files (full-v3) 0.09 0.08 -11.1%\n + 2000.95: git diff-files (full-v4) 0.09 0.09 +0.0%\n + 2000.96: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n + 2000.97: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n + 2000.98: git diff-files -- f2/f4/a (full-v3) 0.06 0.07 +16.7%\n + 2000.99: git diff-files -- f2/f4/a (full-v4) 0.08 0.08 +0.0%\n + 2000.100: git diff-files -- f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n + 2000.101: git diff-files -- f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n \n Signed-off-by: Shuqi Liang \n \n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git grep --cached bogus -- \"\n test_perf_on_all git describe --dirty\n test_perf_on_all 'echo >>new && git describe --dirty'\n +test_perf_on_all git diff-files\n -+test_perf_on_all git diff-files $SPARSE_CONE/a\n ++test_perf_on_all git diff-files -- $SPARSE_CONE/a\n \n test_done\n \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with p\n +\trun_on_all ../edit-contents deep/a &&\n +\n +\tensure_not_expanded diff-files &&\n -+\tensure_not_expanded diff-files deep/a &&\n -+\tensure_not_expanded diff-files \"deep/*\"\n ++\tensure_not_expanded diff-files -- deep/a &&\n ++\tensure_not_expanded diff-files -- \"deep/*\"\n +'\n +\n test_done\n-- \n2.39.0\n\n"},{"id":"476917","messageId":"20230509194241.469477-2-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230509194241.469477-1-cheskaqiqi@gmail.com","subject":"[PATCH v12 1/2] t1092: add tests for `git diff-files`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-09T19:42:40Z","receivedAt":"2023-05-09T19:43:05Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before integrating the 'git diff-files' builtin with the sparse index\nfeature, add tests to t1092-sparse-checkout-compatibility.sh to ensure\nit currently works with sparse-checkout and will still work with sparse\nindex after that integration.\n\nWhen adding tests against a sparse-checkout definition, we test two\nmodes: all changes are within the sparse-checkout cone and some changes\nare outside the sparse-checkout cone.\n\nIn order to have staged changes outside of the sparse-checkout cone,\nmake a directory called 'folder1' and copy `a` into 'folder1/a'.\n'folder1/a' is identical to `a` in the base commit. These make\n'folder1/a' in the index, while leaving it outside of the\nsparse-checkout definition. Change content inside 'folder1/a' in order\nto test 'folder1/a' being present on-disk with modifications.\n\nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 39 ++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 0c784813f1..b06b522030 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2108,4 +2108,43 @@ test_expect_success 'sparse-index is not expanded: write-tree' '\n \tensure_not_expanded write-tree\n '\n \n+test_expect_success 'diff-files with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\ttest_all_match git diff-files &&\n+\n+\ttest_all_match git diff-files -- deep/a &&\n+\n+\t# test wildcard\n+\ttest_all_match git diff-files -- \"deep/*\"\n+'\n+\n+test_expect_success 'diff-files with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match git diff-files -- folder2/a &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\t# The directory \"folder1\" is outside the cone of interest\n+\t# and will not exist in the sparse checkout repositories.\n+\t# Create it as needed, add file \"folder1/a\" there with\n+\t# contents that is different from the staged version.\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git diff-files &&\n+\ttest_all_match git diff-files -- folder1/a &&\n+\ttest_all_match git diff-files -- \"folder*/a\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"476918","messageId":"20230509194241.469477-3-cheskaqiqi@gmail.com","threadId":"59340","inReplyTo":"20230509194241.469477-1-cheskaqiqi@gmail.com","subject":"[PATCH v12 2/2] diff-files: integrate with sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-05-09T19:42:41Z","receivedAt":"2023-05-09T19:43:08Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Remove full index requirement for `git diff-files`. Refactor the\nensure_expanded and ensure_not_expanded functions by introducing a\ncommon helper function, ensure_index_state. Add test to ensure the index\nis no expanded in `git diff-files`.\n\nThe `p2000` tests demonstrate a ~96% execution time reduction for 'git\ndiff-files' and a ~97% execution time reduction for 'git diff-files'\nfor a file using a sparse index:\n\nTest before after\n-----------------------------------------------------------------------\n2000.94: git diff-files (full-v3) 0.09 0.08 -11.1%\n2000.95: git diff-files (full-v4) 0.09 0.09 +0.0%\n2000.96: git diff-files (sparse-v3) 0.52 0.02 -96.2%\n2000.97: git diff-files (sparse-v4) 0.51 0.02 -96.1%\n2000.98: git diff-files -- f2/f4/a (full-v3) 0.06 0.07 +16.7%\n2000.99: git diff-files -- f2/f4/a (full-v4) 0.08 0.08 +0.0%\n2000.100: git diff-files -- f2/f4/a (sparse-v3) 0.46 0.01 -97.8%\n2000.101: git diff-files -- f2/f4/a (sparse-v4) 0.51 0.02 -96.1%\n\nSigned-off-by: Shuqi Liang \n---\n builtin/diff-files.c | 4 ++++\n t/perf/p2000-sparse-operations.sh | 2 ++\n t/t1092-sparse-checkout-compatibility.sh | 27 ++++++++++++++++++++++--\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex dc991f753b..360464e6ef 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -27,6 +27,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n+\n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 60d1de0662..901cc493ef 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -129,5 +129,7 @@ test_perf_on_all git grep --cached bogus -- \"f2/f1/f1/*\"\n test_perf_on_all git write-tree\n test_perf_on_all git describe --dirty\n test_perf_on_all 'echo >>new && git describe --dirty'\n+test_perf_on_all git diff-files\n+test_perf_on_all git diff-files -- $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex b06b522030..e58bfbfcb4 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1377,7 +1377,7 @@ test_expect_success 'index.sparse disabled inline uses full index' '\n \t! test_region index ensure_full_index trace2.txt\n '\n \n-ensure_not_expanded () {\n+run_sparse_index_trace2 () {\n \trm -f trace2.txt &&\n \tif test -z \"$WITHOUT_UNTRACKED_TXT\"\n \tthen\n@@ -1397,7 +1397,16 @@ ensure_not_expanded () {\n \t\t\tgit -C sparse-index \"$@\" \\\n \t\t\t>sparse-index-out \\\n \t\t\t2>sparse-index-error || return 1\n-\tfi &&\n+\tfi\n+}\n+\n+ensure_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n+\ttest_region index ensure_full_index trace2.txt\n+}\n+\n+ensure_not_expanded () {\n+\trun_sparse_index_trace2 \"$@\" &&\n \ttest_region ! index ensure_full_index trace2.txt\n }\n \n@@ -2147,4 +2156,18 @@ test_expect_success 'diff-files with pathspec outside sparse definition' '\n \ttest_all_match git diff-files -- \"folder*/a\"\n '\n \n+test_expect_success 'sparse index is not expanded: diff-files' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>\"$1\"\n+\tEOF\n+\n+\trun_on_all ../edit-contents deep/a &&\n+\n+\tensure_not_expanded diff-files &&\n+\tensure_not_expanded diff-files -- deep/a &&\n+\tensure_not_expanded diff-files -- \"deep/*\"\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"477018","messageId":"f51a8d77-c480-f021-38c4-78a9d75cdd11@github.com","threadId":"59340","inReplyTo":"20230509194241.469477-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v12 0/2] diff-files: integrate with sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-05-11T03:41:52Z","receivedAt":"2023-05-11T03:42:02Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> * Add the '--' in all test to remain consistent.\n> \n> * Change 'may not' to 'will not'.\n> \n> \n> Shuqi Liang (2):\n> t1092: add tests for `git diff-files`\n> diff-files: integrate with sparse index\n> \n> builtin/diff-files.c | 4 ++\n> t/perf/p2000-sparse-operations.sh | 2 +\n> t/t1092-sparse-checkout-compatibility.sh | 66 +++++++++++++++++++++++-\n> 3 files changed, 70 insertions(+), 2 deletions(-)\n> \n\nThis iteration looks good to me. Thanks for keeping up with the reviews and\ngetting this to a polished state!\n\n"},{"id":"477021","messageId":"xmqqbkirzcej.fsf@gitster.g","threadId":"59340","inReplyTo":"f51a8d77-c480-f021-38c4-78a9d75cdd11@github.com","subject":"Re: [PATCH v12 0/2] diff-files: integrate with sparse index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-11T05:04:52Z","receivedAt":"2023-05-11T05:05:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> This iteration looks good to me. Thanks for keeping up with the reviews and\n> getting this to a polished state!\n\nThanks, both. Let's merge the topic to 'next'.\n\n"}]}