{"thread":{"id":"59529","subject":"[GSOC][PATCH v1] diff-index: enable diff-index","startedAt":"2023-04-03T19:06:03Z","lastAt":"2023-05-02T17:36:22Z","messageCount":10,"participants":["Raghul Nanth A","Junio C Hamano","Victoria Dye","RAGHUL NANTH","Shuqi Liang"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474705","messageId":"20230403190538.361840-1-nanth.raghul@gmail.com","threadId":"59529","inReplyTo":null,"subject":"[GSOC][PATCH v1] diff-index: enable diff-index","fromName":"Raghul Nanth A","fromEmail":"nanth.raghul@gmail.com","sentAt":"2023-04-03T19:05:38Z","receivedAt":"2023-04-03T19:06:03Z","isPatch":true,"sender":{"key":"nanth.raghul@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61490162?v=4"},"body":"Uses the run_diff_index() function to generate its diff. This function\nhas been made sparse-index aware in the series that led to 8d2c3732\n(Merge branch 'ld/sparse-diff-blame', 2021-12-21). Hence we can just\nset the requires-full-index to false for \"diff-index\".\n\nPerformance metrics\n\n  Test                                        HEAD~1            HEAD\n  ------------------------------------------------------------------------------------\n  2000.2: git diff-index HEAD (full-v3)       0.09(0.05+0.05)   0.09(0.06+0.04) +0.0%\n  2000.3: git diff-index HEAD (full-v4)       0.09(0.05+0.05)   0.09(0.06+0.03) +0.0%\n  2000.4: git diff-index HEAD (sparse-v3)     0.32(0.28+0.05)   0.01(0.01+0.04) -96.9%\n  2000.5: git diff-index HEAD (sparse-v4)     0.34(0.29+0.06)   0.01(0.02+0.03) -97.1%\n  2000.6: git diff-index HEAD~1 (full-v3)     3.77(3.62+0.14)   3.37(3.27+0.09) -10.6%\n  2000.7: git diff-index HEAD~1 (full-v4)     3.18(3.07+0.11)   3.20(3.10+0.09) +0.6%\n  2000.8: git diff-index HEAD~1 (sparse-v3)   3.78(3.65+0.12)   0.22(0.20+0.06) -94.2%\n  2000.9: git diff-index HEAD~1 (sparse-v4)   3.86(3.74+0.12)   0.28(0.28+0.04) -92.7%\n\nSigned-off-by: Raghul Nanth A <nanth.raghul@gmail.com>\n---\n builtin/diff-index.c                     |  4 ++++\n t/perf/p2000-sparse-operations.sh        |  2 ++\n t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++++++++++++\n 3 files changed, 24 insertions(+)\n\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 35dc9b23ee..8b9871d611 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -24,6 +24,10 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_cache_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 \tprefix = precompose_argv_prefix(argc, argv, prefix);\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..9e74cb22b9 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-index HEAD\n+test_perf_on_all git diff-index HEAD~1\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..13801f327d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1996,6 +1996,24 @@ test_expect_success 'sparse index is not expanded: rm' '\n \tensure_not_expanded rm -r deep\n '\n \n+test_expect_success 'sparse index is not expanded: diff-index' '\n+\tinit_repos &&\n+\n+\techo \"new\" >>sparse-index/g &&\n+\tgit -C sparse-index add g &&\n+\tgit -C sparse-index commit -m \"dummy\" &&\n+\tensure_not_expanded diff-index HEAD~1\n+'\n+\n+test_expect_success 'match all: diff-index' '\n+\tinit_repos &&\n+\n+\ttest_all_match git diff-index HEAD &&\n+\trun_on_all rm g &&\n+\ttest_all_match git diff-index HEAD &&\n+\ttest_all_match git diff-index HEAD --cached\n+'\n+\n test_expect_success 'grep with and --cached' '\n \tinit_repos &&\n \n-- \n2.40.0\n\n"},{"id":"474729","messageId":"xmqqv8ic33jb.fsf@gitster.g","threadId":"59529","inReplyTo":"20230403190538.361840-1-nanth.raghul@gmail.com","subject":"Re: [GSOC][PATCH v1] diff-index: enable diff-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-04T00:16:24Z","receivedAt":"2023-04-04T00:16:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Raghul Nanth A <nanth.raghul@gmail.com> writes:\n\n> Uses the run_diff_index() function to generate its diff.\n\nThe sentence lacks a subject.\n> +\ttest_all_match git diff-index HEAD --cached\n\nSee \"git help cli\".  Do not write rev after a dashed option.\n\nThanks.\n"},{"id":"474869","messageId":"91d3fd23-8120-db65-481a-e9f56017bb04@github.com","threadId":"59529","inReplyTo":"20230403190538.361840-1-nanth.raghul@gmail.com","subject":"Re: [GSOC][PATCH v1] diff-index: enable diff-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-05T17:53:20Z","receivedAt":"2023-04-05T17:54:05Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Raghul Nanth A wrote:\n> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\n> index 3242cfe91a..9e74cb22b9 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-index HEAD\n> +test_perf_on_all git diff-index HEAD~1\n\nWhat is the benefit of testing 'diff-index' with 'HEAD' *and* 'HEAD~1'? I\nwouldn't expect internal behavior in the command to change based on the\nrevision, so the performance should be nearly identical. I'd much rather see\n'diff-index --cached' and/or other options & pathspecs exercised.\n\n>  \n>  test_done\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 801919009e..13801f327d 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1996,6 +1996,24 @@ test_expect_success 'sparse index is not expanded: rm' '\n>  \tensure_not_expanded rm -r deep\n>  '\n>  \n> +test_expect_success 'sparse index is not expanded: diff-index' '\n> +\tinit_repos &&\n> +\n> +\techo \"new\" >>sparse-index/g &&\n> +\tgit -C sparse-index add g &&\n> +\tgit -C sparse-index commit -m \"dummy\" &&\n> +\tensure_not_expanded diff-index HEAD~1\n\nAs with the other tests, please exercise different options and pathspecs\nwith 'diff-index' to improve coverage.\n\n> +'\n> +\n> +test_expect_success 'match all: diff-index' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git diff-index HEAD &&\n> +\trun_on_all rm g &&\n> +\ttest_all_match git diff-index HEAD &&\n> +\ttest_all_match git diff-index HEAD --cached\n> +'\n\nIn addition to the '--cached' option, please test different pathspecs\n(especially different wildcard variations; see the 'git grep' [1] and 'git\ndiff-files' [2] integrations for examples you could build off of).\n\nSeeing that 'diff-files' needed 'pathspec_needs_expanded_index', it's\npossible that this command needs similar treatment. I'm curious as to\nwhether 'diff' needs it as well - the tests in 't1092' don't cover 'diff'\nwith pathspecs, so it might be behaving incorrectly. If that's the case, it\nwould be nice to see pathspecs handled all in one place\n('run_diff_index()'?), if possible.\n\n[1] https://lore.kernel.org/git/20220923041842.27817-1-shaoxuan.yuan02@gmail.com/\n[2] https://lore.kernel.org/git/20230322161820.3609-1-cheskaqiqi@gmail.com/\n\n> +\n>  test_expect_success 'grep with and --cached' '\n>  \tinit_repos &&\n>  \n\n"},{"id":"474878","messageId":"xmqqwn2quo05.fsf@gitster.g","threadId":"59529","inReplyTo":"91d3fd23-8120-db65-481a-e9f56017bb04@github.com","subject":"Re: [GSOC][PATCH v1] diff-index: enable diff-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-05T19:28:58Z","receivedAt":"2023-04-05T19:29:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye <vdye@github.com> writes:\n\n> Raghul Nanth A wrote:\n>> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\n>> index 3242cfe91a..9e74cb22b9 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-index HEAD\n>> +test_perf_on_all git diff-index HEAD~1\n>\n> What is the benefit of testing 'diff-index' with 'HEAD' *and* 'HEAD~1'? I\n> wouldn't expect internal behavior in the command to change based on the\n> revision, so the performance should be nearly identical. I'd much rather see\n> 'diff-index --cached' and/or other options & pathspecs exercised.\n\nGood point.  Comparing with HEAD~1 has a chance to compare _more_\npaths (i.e. paths changed in the working tree plus paths changed\nbetween the two commits), though it feels a bit too subtle if that\nis what these two tests meant.\n\nTesting with pathspec limited comparison, limiting within the cone\nof interest or extending to outside the cone, does sound like a good\nidea.  \"diff-index --cached\" to ignore working tree changes is also\nan obvious thing we want to see working well.\n\n> Seeing that 'diff-files' needed 'pathspec_needs_expanded_index', it's\n> possible that this command needs similar treatment. I'm curious as to\n> whether 'diff' needs it as well - the tests in 't1092' don't cover 'diff'\n> with pathspecs, so it might be behaving incorrectly. If that's the case, it\n> would be nice to see pathspecs handled all in one place\n> ('run_diff_index()'?), if possible.\n\nThanks for a careful review and comment.\n"},{"id":"475035","messageId":"20230408112342.404318-1-nanth.raghul@gmail.com","threadId":"59529","inReplyTo":"20230403190538.361840-1-nanth.raghul@gmail.com","subject":"[GSOC][PATCH v2] diff-index: enable sparse index","fromName":"Raghul Nanth A","fromEmail":"nanth.raghul@gmail.com","sentAt":"2023-04-08T11:23:42Z","receivedAt":"2023-04-08T11:25:12Z","isPatch":true,"sender":{"key":"nanth.raghul@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61490162?v=4"},"body":"diff-index uses the run_diff_index() function to generate its diff. This\nfunction has been made sparse-index aware in the series that led to\n8d2c3732 (Merge branch 'ld/sparse-diff-blame', 2021-12-21). Hence we can\njust set the requires-full-index to false for \"diff-index\".\n\nPerformance metrics\n\n  Test                                                  HEAD~1            HEAD\n  ----------------------------------------------------------------------------------------------\n  2000.2: echo >>a && git diff-index HEAD (full-v3)     0.09(0.06+0.04)   0.09(0.07+0.03) +0.0%\n  2000.3: echo >>a && git diff-index HEAD (full-v4)     0.09(0.06+0.04)   0.09(0.05+0.05) +0.0%\n  2000.4: echo >>a && git diff-index HEAD (sparse-v3)   0.37(0.31+0.06)   0.01(0.02+0.03) -97.3%\n  2000.5: echo >>a && git diff-index HEAD (sparse-v4)   0.30(0.26+0.05)   0.01(0.01+0.04) -96.7%\n  2000.6: git diff-index HEAD **a (full-v3)             0.06(0.05+0.01)   0.06(0.06+0.01) +0.0%\n  2000.7: git diff-index HEAD **a (full-v4)             0.06(0.05+0.01)   0.06(0.04+0.02) +0.0%\n  2000.8: git diff-index HEAD **a (sparse-v3)           0.29(0.25+0.03)   0.01(0.01+0.00) -96.6%\n  2000.9: git diff-index HEAD **a (sparse-v4)           0.37(0.34+0.02)   0.01(0.01+0.00) -97.3%\n  2000.10: git diff-index --cached HEAD (full-v3)       0.05(0.03+0.01)   0.05(0.03+0.02) +0.0%\n  2000.11: git diff-index --cached HEAD (full-v4)       0.05(0.03+0.01)   0.05(0.02+0.02) +0.0%\n  2000.12: git diff-index --cached HEAD (sparse-v3)     0.35(0.33+0.01)   0.01(0.00+0.00) -97.1%\n  2000.13: git diff-index --cached HEAD (sparse-v4)     0.35(0.32+0.02)   0.01(0.00+0.00) -97.1%\n---\n\nSorry for the late reply. Got caught up in school work\n  * Fixed commit message\n  * Added check to expand index if needed (based on diff-files)\n  * Added pathspec based tests\n\n builtin/diff-index.c                     |  9 +++++\n t/perf/p2000-sparse-operations.sh        |  3 ++\n t/t1092-sparse-checkout-compatibility.sh | 44 ++++++++++++++++++++++++\n 3 files changed, 56 insertions(+)\n\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 35dc9b23ee..e67cf5a1db 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -24,6 +24,14 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tusage(diff_cache_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+\tif (pathspec_needs_expanded_index(the_repository->index,\n+\t\t\t\t\t  &rev.diffopt.pathspec))\n+\t\tensure_full_index(the_repository->index);\n+\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.abbrev = 0;\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n@@ -69,6 +77,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tperror(\"repo_read_index\");\n \t\treturn -1;\n \t}\n+\n \tresult = run_diff_index(&rev, option);\n \tresult = diff_result_code(&rev.diffopt, result);\n \trelease_revisions(&rev);\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 3242cfe91a..62499d3aa8 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -125,5 +125,8 @@ 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 'echo >>a && git diff-index HEAD'\n+test_perf_on_all git diff-index HEAD \"**a\"\n+test_perf_on_all git diff-index --cached HEAD\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 801919009e..24bc716c48 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1996,6 +1996,50 @@ test_expect_success 'sparse index is not expanded: rm' '\n \tensure_not_expanded rm -r deep\n '\n \n+test_expect_success 'sparse index is not expanded: diff-index' '\n+\tinit_repos &&\n+\n+\techo \"new\" >>sparse-index/g &&\n+\tgit -C sparse-index add g &&\n+\tgit -C sparse-index commit -m \"dummy\" &&\n+\tensure_not_expanded diff-index HEAD~1 &&\n+\n+\techo \"text\" >>sparse-index/deep/a &&\n+\n+\tensure_not_expanded diff-index HEAD deep/a &&\n+\tensure_not_expanded diff-index HEAD deep/*\n+'\n+test_expect_success 'diff-index pathspec expands index when necessary' '\n+\tinit_repos &&\n+\n+\techo \"text\" >>sparse-index/deep/a &&\n+\n+\t# pathspec that should expand index\n+\t! ensure_not_expanded diff-index \"*/a\" &&\n+\t! ensure_not_expanded diff-index \"**a\"\n+'\n+\n+test_expect_success 'diff-index with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\ttest_sparse_match test_must_fail git diff-index HEAD folder2/a\n+'\n+\n+test_expect_success 'match all: diff-index' '\n+\tinit_repos &&\n+\n+\ttest_all_match git diff-index HEAD &&\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo text >>$1\n+\tEOF\n+\trun_on_all ../edit-contents g &&\n+\trun_on_all git add g &&\n+\trun_on_all git commit -m \"two\" &&\n+\trun_on_all rm g &&\n+\ttest_all_match git diff-index HEAD &&\n+\ttest_all_match git diff-index --cached HEAD~1\n+'\n+\n test_expect_success 'grep with and --cached' '\n \tinit_repos &&\n \n-- \n2.40.0\n\n"},{"id":"475316","messageId":"62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com","threadId":"59529","inReplyTo":"20230408112342.404318-1-nanth.raghul@gmail.com","subject":"Re: [GSOC][PATCH v2] diff-index: enable sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-04-13T21:14:32Z","receivedAt":"2023-04-13T21:14:39Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Raghul Nanth A wrote:\n> diff-index uses the run_diff_index() function to generate its diff. This\n> function has been made sparse-index aware in the series that led to\n> 8d2c3732 (Merge branch 'ld/sparse-diff-blame', 2021-12-21). Hence we can\n> just set the requires-full-index to false for \"diff-index\".\n> \n> Performance metrics\n> \n>   Test                                                  HEAD~1            HEAD\n>   ----------------------------------------------------------------------------------------------\n>   2000.2: echo >>a && git diff-index HEAD (full-v3)     0.09(0.06+0.04)   0.09(0.07+0.03) +0.0%\n>   2000.3: echo >>a && git diff-index HEAD (full-v4)     0.09(0.06+0.04)   0.09(0.05+0.05) +0.0%\n>   2000.4: echo >>a && git diff-index HEAD (sparse-v3)   0.37(0.31+0.06)   0.01(0.02+0.03) -97.3%\n>   2000.5: echo >>a && git diff-index HEAD (sparse-v4)   0.30(0.26+0.05)   0.01(0.01+0.04) -96.7%\n>   2000.6: git diff-index HEAD **a (full-v3)             0.06(0.05+0.01)   0.06(0.06+0.01) +0.0%\n>   2000.7: git diff-index HEAD **a (full-v4)             0.06(0.05+0.01)   0.06(0.04+0.02) +0.0%\n>   2000.8: git diff-index HEAD **a (sparse-v3)           0.29(0.25+0.03)   0.01(0.01+0.00) -96.6%\n>   2000.9: git diff-index HEAD **a (sparse-v4)           0.37(0.34+0.02)   0.01(0.01+0.00) -97.3%\n>   2000.10: git diff-index --cached HEAD (full-v3)       0.05(0.03+0.01)   0.05(0.03+0.02) +0.0%\n>   2000.11: git diff-index --cached HEAD (full-v4)       0.05(0.03+0.01)   0.05(0.02+0.02) +0.0%\n>   2000.12: git diff-index --cached HEAD (sparse-v3)     0.35(0.33+0.01)   0.01(0.00+0.00) -97.1%\n>   2000.13: git diff-index --cached HEAD (sparse-v4)     0.35(0.32+0.02)   0.01(0.00+0.00) -97.1%\n> ---\n> \n> Sorry for the late reply. Got caught up in school work\n>   * Fixed commit message\n>   * Added check to expand index if needed (based on diff-files)\n>   * Added pathspec based tests\n\nPlease include the range-diff comparing the previous version to the new one\nin your future iterations & patch series in general. GitGitGadget adds it by\ndefault, but if you're using 'send-email' you should be able to use the\n'--range-diff' option to generate it (see MyFirstContribution [1] for more\ninformation).\n\n[1] https://git-scm.com/docs/MyFirstContribution#v2-git-send-email\n\n> \n>  builtin/diff-index.c                     |  9 +++++\n>  t/perf/p2000-sparse-operations.sh        |  3 ++\n>  t/t1092-sparse-checkout-compatibility.sh | 44 ++++++++++++++++++++++++\n>  3 files changed, 56 insertions(+)\n> \n> diff --git a/builtin/diff-index.c b/builtin/diff-index.c\n> index 35dc9b23ee..e67cf5a1db 100644\n> --- a/builtin/diff-index.c\n> +++ b/builtin/diff-index.c\n> @@ -24,6 +24,14 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n>  \t\tusage(diff_cache_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> +\tif (pathspec_needs_expanded_index(the_repository->index,\n> +\t\t\t\t\t  &rev.diffopt.pathspec))\n> +\t\tensure_full_index(the_repository->index);\n\nRe: my last review [2] - did you look into the behavior of 'diff' with\npathspecs and whether this 'pathspec_needs_expanded_index()' could be\ncentralized (in e.g. 'run_diff_index()')? What did you find?\n\n[2] https://lore.kernel.org/git/91d3fd23-8120-db65-481a-e9f56017bb04@github.com/\n\n> +\n>  \trepo_init_revisions(the_repository, &rev, prefix);\n>  \trev.abbrev = 0;\n>  \tprefix = precompose_argv_prefix(argc, argv, prefix);\n> @@ -69,6 +77,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n>  \t\tperror(\"repo_read_index\");\n>  \t\treturn -1;\n>  \t}\n> +\n>  \tresult = run_diff_index(&rev, option);\n>  \tresult = diff_result_code(&rev.diffopt, result);\n>  \trelease_revisions(&rev);\n> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\n> index 3242cfe91a..62499d3aa8 100755\n> --- a/t/perf/p2000-sparse-operations.sh\n> +++ b/t/perf/p2000-sparse-operations.sh\n> @@ -125,5 +125,8 @@ 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 'echo >>a && git diff-index HEAD'\n> +test_perf_on_all git diff-index HEAD \"**a\"\n> +test_perf_on_all git diff-index --cached HEAD\n>  \n>  test_done\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 801919009e..24bc716c48 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1996,6 +1996,50 @@ test_expect_success 'sparse index is not expanded: rm' '\n>  \tensure_not_expanded rm -r deep\n>  '\n>  \n> +test_expect_success 'sparse index is not expanded: diff-index' '\n> +\tinit_repos &&\n> +\n> +\techo \"new\" >>sparse-index/g &&\n> +\tgit -C sparse-index add g &&\n> +\tgit -C sparse-index commit -m \"dummy\" &&\n> +\tensure_not_expanded diff-index HEAD~1 &&\n> +\n> +\techo \"text\" >>sparse-index/deep/a &&\n> +\n> +\tensure_not_expanded diff-index HEAD deep/a &&\n> +\tensure_not_expanded diff-index HEAD deep/*\n> +'\n\nnit: please add a newline here (after the 'sparse index is not expanded:\ndiff-index' test) to stay consistent with the other tests in the file.\n\n> +test_expect_success 'diff-index pathspec expands index when necessary' '\n> +\tinit_repos &&\n> +\n> +\techo \"text\" >>sparse-index/deep/a &&\n> +\n> +\t# pathspec that should expand index\n> +\t! ensure_not_expanded diff-index \"*/a\" &&\n\nUsing '! ensure_not_expanded' will fail if the command expands the index\n_or_ if the command fails altogether, which could inadvertently make these\ntests pass even when there's a breakage in 'diff-index'. An\n'ensure_expanded' function was created in [3] to test these types of cases;\nyou can use that here if you base your branch on 'sl/diff-files-sparse' (see\nSubmittingPatches for more information [4]).\n\n[3] https://lore.kernel.org/git/20230322161820.3609-3-cheskaqiqi@gmail.com/\n[4] https://git-scm.com/docs/SubmittingPatches#base-branch\n\n> +\t! ensure_not_expanded diff-index \"**a\"\n\nGit pathspec syntax [5] does not follow glob rules (without the ':(glob)'\nprefix, at least), so the '**' doesn't do anything special here that a\nsingle '*' wouldn't do. So, to make it clear that you aren't using glob\npatterns, it might be better to use '*a' instead. \n\nAlso, why are the wildcard pathspecs here in double-quotes, but the ones in\nthe previous test ('sparse index is not expanded: diff-index') aren't?\n\n[5] https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec\n\n> +'\n> +\n> +test_expect_success 'diff-index with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\ttest_sparse_match test_must_fail git diff-index HEAD folder2/a\n> +'\n> +\n> +test_expect_success 'match all: diff-index' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git diff-index HEAD &&\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo text >>$1\n> +\tEOF\n> +\trun_on_all ../edit-contents g &&\n> +\trun_on_all git add g &&\n> +\trun_on_all git commit -m \"two\" &&\n> +\trun_on_all rm g &&\n> +\ttest_all_match git diff-index HEAD &&\n> +\ttest_all_match git diff-index --cached HEAD~1\n> +'\n> +\n>  test_expect_success 'grep with and --cached' '\n>  \tinit_repos &&\n>  \n\n"},{"id":"475697","messageId":"CAPnUp-=3aoG9WwCcLnMZ4UL90j+snL8qUePPmm02WQK9tUkCzw@mail.gmail.com","threadId":"59529","inReplyTo":"62821012-4fc3-5ad8-695c-70f7ab14a8c9@github.com","subject":"Re: [GSOC][PATCH v2] diff-index: enable sparse index","fromName":"RAGHUL NANTH","fromEmail":"nanth.raghul@gmail.com","sentAt":"2023-04-19T15:15:31Z","receivedAt":"2023-04-19T15:17:25Z","isPatch":true,"sender":{"key":"nanth.raghul@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61490162?v=4"},"body":"On Fri, Apr 14, 2023 at 2:44 AM Victoria Dye <vdye@github.com> wrote:\n>\n> Please include the range-diff comparing the previous version to the new one\n> in your future iterations & patch series in general. GitGitGadget adds it by\n> default, but if you're using 'send-email' you should be able to use the\n> '--range-diff' option to generate it (see MyFirstContribution [1] for more\n> information).\n>\n\nYeah, I will keep this in mind. Sorry about that\n\n> Re: my last review [2] - did you look into the behavior of 'diff' with\n> pathspecs and whether this 'pathspec_needs_expanded_index()' could be\n> centralized (in e.g. 'run_diff_index()')? What did you find?\n\nI hadn't understood the review properly. I just thought you wanted to\nmake sure the function was added to diiff-index itself. I have read\nthrough some of it, but I am still not 100% sure of the behaviour.\nWill run through it more to get more definitive answers\n\n> Using '! ensure_not_expanded' will fail if the command expands the index\n> _or_ if the command fails altogether, which could inadvertently make these\n> tests pass even when there's a breakage in 'diff-index'. An\n> 'ensure_expanded' function was created in [3] to test these types of cases;\n> you can use that here if you base your branch on 'sl/diff-files-sparse' (see\n> SubmittingPatches for more information [4]).\n>\n> [3] https://lore.kernel.org/git/20230322161820.3609-3-cheskaqiqi@gmail.com/\n> [4] https://git-scm.com/docs/SubmittingPatches#base-branch\n>\n> > +     ! ensure_not_expanded diff-index \"**a\"\n\nYeah, I saw this function, but since this wasn't integrated into\nmaster, I wasn't sure how I would go about using it. I will base my\nwork off of the mentioned branch for now then. As for making sure the\nfunction doesn't give false positives, it should be fine in this\ncurrent case, since I did try to manually run through the commands\njust as a guarantee, and that seemed to run fine, but yes, I will make\nsure to make those updates\n\n\n> Git pathspec syntax [5] does not follow glob rules (without the ':(glob)'\n> prefix, at least), so the '**' doesn't do anything special here that a\n> single '*' wouldn't do. So, to make it clear that you aren't using glob\n> patterns, it might be better to use '*a' instead.\n>\n> Also, why are the wildcard pathspecs here in double-quotes, but the ones in\n> the previous test ('sparse index is not expanded: diff-index') aren't?\n\n\nThe double quotes were just to use the glob provided by pathspec. As\nfor why the previous ones don't have them, they are just using regular\npathspecs.\n\nI will make the necessary changes as mentioned here.\n\nThank you,\nRaghul\n"},{"id":"475877","messageId":"20230422212500.476955-1-cheskaqiqi@gmail.com","threadId":"59529","inReplyTo":"CAPnUp-=3aoG9WwCcLnMZ4UL90j+snL8qUePPmm02WQK9tUkCzw@mail.gmail.com","subject":"Re: [GSOC][PATCH v2] diff-index: enable sparse index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-04-22T21:25:00Z","receivedAt":"2023-04-22T21:25:19Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":">> Re: my last review [2] - did you look into the behavior of 'diff' with\n>> pathspecs and whether this 'pathspec_needs_expanded_index()' could be\n>> centralized (in e.g. 'run_diff_index()')? What did you find?\n\n>I hadn't understood the review properly. I just thought you wanted to\n>make sure the function was added to diiff-index itself. I have read\n>through some of it, but I am still not 100% sure of the behaviour.\n>Will run through it more to get more definitive answers\n\n\nHello Raghul!\n\nI hope this email finds you well. I recently came across your patch and \nnoticed that you might be facing some difficulties with a specific issue.\nI've reviewed your patch and thought I'd share a few suggestions that \nmight help you overcome the issue.The code below I've already test it.\nBut there must have many detail I did not handle.\n\nIn the builtin/diff.c file, the cmd_diff() function can call either \n'run_diff_files()' or 'run_diff_index()' depending on the situation.\nwhen you run 'git diff',run_diff_files() is called to find differences\nbetween the working directory and the index. when you run \n'git diff --cached'. 'run_diff_index()' is called to find difference \nbetween the indexand the commit.\n\nBoth the \"diff-index\" and \"diff\" commands share the \"run_diff_index\" \nfunction. So, we can handling of pathspecs in one place(run_diff_index).\nDoing this we can simplify the codebase and make it easier to maintain.\n\n1.add test for diff in t1092. We will find the test will fail.\n\ntest_expect_success 'git diff with pathspec expands index when necessary' '\n\tinit_repos &&\n\n\techo \"new\" >>sparse-index/deep/a &&\n\tgit -C sparse-index add deep/a &&\n\n\t# pathspec that should expand index\n\tensure_expanded diff --cached \"*/a\" &&\n\n\twrite_script edit-conflict <<-\\EOF &&\n\techo test >>\"$1\"\n\tEOF\n\n\trun_on_all ../edit-contents deep/a &&\n\tensure_expanded diff HEAD \"*/a\"\n'\n\n\n2.\"run_diff_index\" is in 'diff-lib.c'.We can add \n'pathspec_needs_expanded_index' in front of 'do_diff_cache()'(process\nthe index before the start of the diff process).\n\nint run_diff_index(struct rev_info *revs, unsigned int option)\n{\n\t......\n\t......\n\tif (merge_base) {\n\t\tdiff_get_merge_base(revs, &oid);\n\t\tname = oid_to_hex_r(merge_base_hex, &oid);\n\t} else {\n\t\toidcpy(&oid, &ent->item->oid);\n\t\tname = ent->name;\n\t}\n\n\n\tif (pathspec_needs_expanded_index(revs->diffopt.repo->index, &revs->diffopt.pathspec))\n\t\tensure_full_index(revs->diffopt.repo->index);\n\n\n\tif (diff_cache(revs, &oid, name, cached))\n\t\texit(128);\n\n\t.......\n\t......\n\t.......\n}\n\n\n3.Delete 'the pathspec_needs_expanded_index' function you have in your \n'builtin/diff-index.c' in last patch.\n\n4.Run the test again, then the test for 'git diff' and your test for \n'git diff-index'will all pass!\n\nI hope these suggestions prove helpful to you. If you have any questions\nor would like to discuss further, please don't hesitate to reach out.\n\n-----------------------------------------------------------------------\nBest,\nShuqi\n\n\n"},{"id":"476414","messageId":"20230502094658.608646-1-nanth.raghul@gmail.com","threadId":"59529","inReplyTo":"20230422212500.476955-1-cheskaqiqi@gmail.com","subject":"[GSOC] diff-index: enable sparse index","fromName":"Raghul Nanth A","fromEmail":"nanth.raghul@gmail.com","sentAt":"2023-05-02T09:46:58Z","receivedAt":"2023-05-02T09:47:54Z","isPatch":false,"sender":{"key":"nanth.raghul@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61490162?v=4"},"body":"Hey,\n  Thanks for the info. Your explanations make sense and I will make the appropriate changes. I had two questions I had two questions regarding this: \n  I have been trying to base my changes off the 'sl/diff-files-sparse' branch, but I am not sure how I would go about doing that. I thought I would be just pulling changes from some remote repo but I couldn't find one. So, could you let me know how I could do that?\n  Also, I don't seem to have been CC'd on this email. Just wanted to point that out, so that I don't accidentally miss conversations.\n\nThanks,\nRaghul\n"},{"id":"476443","messageId":"522272ca-f294-b2c5-aea7-e264c9faab85@github.com","threadId":"59529","inReplyTo":"20230502094658.608646-1-nanth.raghul@gmail.com","subject":"Re: [GSOC] diff-index: enable sparse index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-05-02T17:35:57Z","receivedAt":"2023-05-02T17:36:22Z","isPatch":false,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Raghul Nanth A wrote:\n> Hey,\n>   Thanks for the info. Your explanations make sense and I will make the\n>   appropriate changes. I had two questions I had two questions regarding\n>   this: \n>   I have been trying to base my changes off the 'sl/diff-files-sparse'\n>   branch, but I am not sure how I would go about doing that. I thought I\n>   would be just pulling changes from some remote repo but I couldn't find\n>   one. So, could you let me know how I could do that?\n\nYou should be able to find that branch in the https://github.com/gitster/git\nremote.\n\n>   Also, I don't seem to have been CC'd on this email. Just wanted to point\n>   that out, so that I don't accidentally miss conversations.\n> \n> Thanks,\n> Raghul\n\n"}]}