{"thread":{"id":"62007","subject":"[PATCH] Mark `cat-file` sparse-index compatible","startedAt":"2024-08-26T18:08:56Z","lastAt":"2024-09-04T16:36:00Z","messageCount":17,"participants":["Kevin Lyles via GitGitGadget","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"501691","messageId":"pull.1770.git.git.1724695732305.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":null,"subject":"[PATCH] Mark `cat-file` sparse-index compatible","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-26T18:08:52Z","receivedAt":"2024-08-26T18:08:56Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"From: Kevin Lyles \n\n`cat-file` will expand a sparse index to a full index when needed, but\nis currently marked as needing a full index (or rather, not marked as\nnot needing a full index). This results in much slower `cat-file`\noperations when working within the sparse index, since we expand the\nindex whether it's needed or not.\n\nMark `cat-file` as not needing a full index, so that you only pay the\ncost of expanding the sparse index to a full index when you request a\nfile outside of the sparse index.\n\nAdd tests to ensure both that:\n- `cat-file` returns the correct file contents whether or not the file\n is in the sparse index\n- `cat-file` warns about expanding to the full index any time you\n request something outside of the sparse index\n\nSigned-off-by: Kevin Lyles \n---\n Mark cat-file sparse-index compatible\n \n Please note that this is my first contribution to git. I've tried to\n follow the instructions about how to correctly submit a patch (I'm using\n GitGitGadget as getting Outlook to do plain text e-mail correctly seems\n impossible), but please let me know if I've missed something.\n \n I've worded the commit message itself as though I'm definitely correct\n about how cat-file behaves, since I assume we want the final commit\n message to be definite. However, this change felt a little too easy and\n I can't help but feel that I might have missed something. So, even\n though this is just one commit, I'm also including this cover letter\n going into more detail about the parts that don't need to be part of the\n commit history.\n \n My motivation for making this change is purely performance. We have a\n large repository that we enable the sparse index for, and I am\n developing a pre-commit hook that (among other things) uses git cat-file\n to get the staged contents of certain files. Without this change,\n getting the contents of a single small file from the index can take\n upwards of 10 seconds due to the index expansion. After this change, it\n only takes ~0.3 seconds unless the file is outside of the sparse index.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v1\nPull-Request: https://github.com/git/git/pull/1770\n\n builtin/cat-file.c | 3 +\n t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++++--\n 2 files changed, 69 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8b..1afdfb5cbae 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \t/* Return early if we're in batch mode? */\n \tif (batch.enabled) {\n \t\tif (opt_cw)\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a2c0e1b4dcc..0f36246ae84 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -179,22 +179,26 @@ init_repos_as_submodules () {\n }\n \n run_on_sparse () {\n+\ttee run_on_sparse-checkout run_on_sparse-index &&\n+\n \t(\n \t\tcd sparse-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n-\t) &&\n+\t) ../sparse-index-out 2>../sparse-index-err\n-\t)\n+\t) ../full-checkout-out 2>../full-checkout-err\n-\t) &&\n-\trun_on_sparse \"$@\"\n+\t) ... \" \" ... \"\n+# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n # Verifies that \"git sparse-checkout set ... \" succeeds and\n # leaves the sparse index in a state where ... are sparse\n # directories (and ... are not).\n@@ -2345,4 +2349,61 @@ test_expect_success 'advice.sparseIndexExpanded' '\n \tgrep \"The sparse index is expanding to a full index\" err\n '\n \n+test_expect_success 'cat-file -p' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\ttest_all_match git cat-file -p HEAD:deep/a &&\n+\tensure_not_expanded cat-file -p HEAD:deep/a &&\n+\ttest_all_match git cat-file -p HEAD:folder1/a &&\n+\tensure_not_expanded cat-file -p HEAD:folder1/a &&\n+\n+\ttest_all_match git cat-file -p :deep/a &&\n+\tensure_not_expanded cat-file -p :deep/a &&\n+\trun_on_all git cat-file -p :folder1/a &&\n+\ttest_cmp full-checkout-out sparse-checkout-out &&\n+\ttest_cmp full-checkout-out sparse-index-out &&\n+\ttest_cmp full-checkout-err sparse-checkout-err &&\n+\tensure_expanded cat-file -p :folder1/a'\n+\n+test_expect_success 'cat-file --batch' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\tcat <<-\\EOF | test_all_match git cat-file --batch &&\n+\tHEAD:deep/a\n+\t:deep/a\n+\tEOF\n+\tcat <<-\\EOF | ensure_not_expanded cat-file --batch &&\n+\tHEAD:deep/a\n+\t:deep/a\n+\tEOF\n+\n+\techo \"HEAD:folder1/a\" | test_all_match git cat-file --batch &&\n+\techo \"HEAD:folder1/a\" | ensure_not_expanded cat-file --batch &&\n+\n+\techo \":folder1/a\" | run_on_all git cat-file --batch &&\n+\ttest_cmp full-checkout-out sparse-checkout-out &&\n+\ttest_cmp full-checkout-out sparse-index-out &&\n+\ttest_cmp full-checkout-err sparse-checkout-err &&\n+\techo \":folder1/a\" | ensure_expanded cat-file --batch &&\n+\n+\tcat <<-\\EOF | run_on_all git cat-file --batch &&\n+\t:deep/a\n+\t:folder1/a\n+\tEOF\n+\ttest_cmp full-checkout-out sparse-checkout-out &&\n+\ttest_cmp full-checkout-out sparse-index-out &&\n+\ttest_cmp full-checkout-err sparse-checkout-err &&\n+\tcat <<-\\EOF | ensure_expanded cat-file --batch\n+\t:deep/a\n+\t:folder1/a\n+\tEOF'\n+\n test_done\n\nbase-commit: 80ccd8a2602820fdf896a8e8894305225f86f61d\n-- \ngitgitgadget\n"},{"id":"501797","messageId":"31e9a542-c02b-4245-b3ad-8e1ade2211c6@gmail.com","threadId":"62007","inReplyTo":"pull.1770.git.git.1724695732305.gitgitgadget@gmail.com","subject":"Re: [PATCH] Mark `cat-file` sparse-index compatible","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-08-29T01:59:04Z","receivedAt":"2024-08-29T01:59:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/26/24 2:08 PM, Kevin Lyles via GitGitGadget wrote:\n > From: Kevin Lyles \nHi, Kevin! Thanks for looking into this.\n > `cat-file` will expand a sparse index to a full index when needed, but\n\nhyper-nit: For commit messages we would usually use 'git cat-file'.\n(This recommends both using the full command and single ticks instead\nof back-ticks.)\n\n > is currently marked as needing a full index (or rather, not marked as\n > not needing a full index). This results in much slower `cat-file`\n > operations when working within the sparse index, since we expand the\n > index whether it's needed or not.\n\nI was surprised at first that 'git cat-file' would need it at all,\nbut it is important when referencing objects with \":\" as that\nrefers to a path in the index instead of at any revision. It could be\nhelpful to bring this observation to the forefront of your commit\nmessage.\n\n > Mark `cat-file` as not needing a full index, so that you only pay the\n > cost of expanding the sparse index to a full index when you request a\n > file outside of the sparse index.\n >\n > Add tests to ensure both that:\n > - `cat-file` returns the correct file contents whether or not the file\n > is in the sparse index\n > - `cat-file` warns about expanding to the full index any time you\n > request something outside of the sparse index\n\nThe \"ensure_expanded\" test works without the advice warning about the\nlarger request. It uses the trace2 mechanism to look for the expansion\nhappening under the hood.\n\n(In fact, when I contributed the advice message, it became disabled in\nthese tests using config values during setup.)\n\n > Signed-off-by: Kevin Lyles \n > ---\n > Mark cat-file sparse-index compatible\n >\n > Please note that this is my first contribution to git. I've tried to\n > follow the instructions about how to correctly submit a patch (I'm using\n > GitGitGadget as getting Outlook to do plain text e-mail correctly seems\n > impossible), but please let me know if I've missed something.\n >\n > I've worded the commit message itself as though I'm definitely correct\n > about how cat-file behaves, since I assume we want the final commit\n > message to be definite. However, this change felt a little too easy and\n > I can't help but feel that I might have missed something. So, even\n > though this is just one commit, I'm also including this cover letter\n > going into more detail about the parts that don't need to be part of the\n > commit history.\n >\n > My motivation for making this change is purely performance. We have a\n > large repository that we enable the sparse index for, and I am\n > developing a pre-commit hook that (among other things) uses git cat-file\n > to get the staged contents of certain files. Without this change,\n > getting the contents of a single small file from the index can take\n > upwards of 10 seconds due to the index expansion. After this change, it\n > only takes ~0.3 seconds unless the file is outside of the sparse index.\n >\n > Published-As: \nhttps://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v1\n > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git \npr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v1\n > Pull-Request: https://github.com/git/git/pull/1770\n >\n > builtin/cat-file.c | 3 +\n > t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++++--\n > 2 files changed, 69 insertions(+), 5 deletions(-)\n >\n > diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n > index 18fe58d6b8b..1afdfb5cbae 100644\n > --- a/builtin/cat-file.c\n > +++ b/builtin/cat-file.c\n > @@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const \nchar *prefix)\n > \tif (batch.buffer_output < 0)\n > \t\tbatch.buffer_output = batch.all_objects;\n >\n > +\tprepare_repo_settings(the_repository);\n > +\tthe_repository->settings.command_requires_full_index = 0;\n > +\n > \t/* Return early if we're in batch mode? */\n > \tif (batch.enabled) {\n > \t\tif (opt_cw)\n > diff --git a/t/t1092-sparse-checkout-compatibility.sh \nb/t/t1092-sparse-checkout-compatibility.sh\n > index a2c0e1b4dcc..0f36246ae84 100755\n > --- a/t/t1092-sparse-checkout-compatibility.sh\n > +++ b/t/t1092-sparse-checkout-compatibility.sh\n > @@ -179,22 +179,26 @@ init_repos_as_submodules () {\n > }\n >\n > run_on_sparse () {\n > +\ttee run_on_sparse-checkout run_on_sparse-index &&> +\n\nThis is an interesting update to this helper method, which takes stdin\nand copies it to these two files for consumption by the two calls below.\nIt really only needs to go to one file and then have that file be\nconsumed twice. This diff seems to accomplish what you want:\n\n--- >8 ---\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh \nb/t/t1092-sparse-checkout-compatibility.sh\nindex 1954bf6332d..9902190eec4 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -180,22 +180,24 @@ init_repos_as_submodules () {\n }\n\n run_on_sparse () {\n+\tcat >in &&\n \t(\n \t\tcd sparse-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n-\t) &&\n+\t) ../sparse-index-out 2>../sparse-index-err\n-\t)\n+\t) in &&\n \t(\n \t\tcd full-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n-\t) &&\n-\trun_on_sparse \"$@\"\n+\t) 8 ---\n\nWhatever happens, this subtlety seems valuable to add to the commit\nmessage to help readers to expect it.\n\n > -# Usage: test_sprase_checkout_set \" ... \" \" ... \"\n > +# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n\nThanks for fixing this typo.\n\n > +test_expect_success 'cat-file -p' '\n > +\tinit_repos &&\n > +\techo \"new content\" >>full-checkout/deep/a &&\n > +\techo \"new content\" >>sparse-checkout/deep/a &&\n > +\techo \"new content\" >>sparse-index/deep/a &&\n > +\trun_on_all git add deep/a &&\n > +\n > +\ttest_all_match git cat-file -p HEAD:deep/a &&\n > +\tensure_not_expanded cat-file -p HEAD:deep/a &&\n > +\ttest_all_match git cat-file -p HEAD:folder1/a &&\n > +\tensure_not_expanded cat-file -p HEAD:folder1/a &&\n\nThese commands shouldn't touch the index at all, as they are loading the\nobject ID the same as 'git rev-parse'.\n\n > +\n > +\ttest_all_match git cat-file -p :deep/a &&\n > +\tensure_not_expanded cat-file -p :deep/a &&\n > +\trun_on_all git cat-file -p :folder1/a &&\n\nThese are the interesting cases.\n\n > +\ttest_cmp full-checkout-out sparse-checkout-out &&\n > +\ttest_cmp full-checkout-out sparse-index-out &&\n > +\ttest_cmp full-checkout-err sparse-checkout-err &&\n\nI figure that you have decided to use run_on_all and not test_all_match\nbecause of an error message that doesn't match. Across the full and\nsparse cases. This would be a good time to focus on what is different\nabout them, perhaps with something like\n\n\tgrep \"error message contents\" sparse-index-err\n\nThe difference shouldn't be the new advice message due to the mentioned\nchange to the config of these repos. If that was the case, then the\nfollowing would work, but shouldn't be necessary:\n\n\tGIT_ADVICE=0 test_all_match git cat-file -p :folder1/a &&\n\nWhat is the reason for there being a difference in stderr in the\nsparse index case? We should drill down here to make sure the behavior\nis appropriate.\n\n > +\tensure_expanded cat-file -p :folder1/a'\n > +\n > +test_expect_success 'cat-file --batch' '\n > +\tinit_repos &&\n > +\techo \"new content\" >>full-checkout/deep/a &&\n > +\techo \"new content\" >>sparse-checkout/deep/a &&\n > +\techo \"new content\" >>sparse-index/deep/a &&\n > +\trun_on_all git add deep/a &&\n > +\n > +\tcat <<-\\EOF | test_all_match git cat-file --batch &&\n > +\tHEAD:deep/a\n > +\t:deep/a\n > +\tEOF\n\nThis heredoc should instead be used to write to a file to use twice:\n\n\tcat >in <<-\\EOF &&\n\tHEAD:deep/a\n\t:deep/a\n\tEOF\n\n\ttest_all_match git cat-file --batch +\techo \"HEAD:folder1/a\" | test_all_match git cat-file --batch &&\n > +\techo \"HEAD:folder1/a\" | ensure_not_expanded cat-file --batch &&\n\nI'm not sure that these are interesting cases as they will not cause an\nindex read and thus are not affected by sparse-checkout.\n\n > +\techo \":folder1/a\" | run_on_all git cat-file --batch &&\n > +\ttest_cmp full-checkout-out sparse-checkout-out &&\n > +\ttest_cmp full-checkout-out sparse-index-out &&\n > +\ttest_cmp full-checkout-err sparse-checkout-err &&\n > +\techo \":folder1/a\" | ensure_expanded cat-file --batch &&\n\nThis is a more interesting case, again using outside of cone paths.\n\nI would still recommend reusing an input file:\n\n\techo \":folder1/a\" >in &&\n\ttest_all_match git cat-file --batch +\n > +\tcat <<-\\EOF | run_on_all git cat-file --batch &&\n > +\t:deep/a\n > +\t:folder1/a\n > +\tEOF\n\nAgain, point the heredoc to a file and then into the test:\n\n\tcat >in <<-\\EOF &&\n\t:deep/a\n\t:folder1/a\n\tEOF\n\ttest_all_match git cat-file --batch 1: 7067a4c5da2 Allow using stdin in run_on_* functions\n 1: 60c15dd246a ! 2: a92825e502f Mark `cat-file` sparse-index compatible\n @@ Metadata\n Author: Kevin Lyles \n \n ## Commit message ##\n - Mark `cat-file` sparse-index compatible\n + Mark 'git cat-file' sparse-index compatible\n \n - `cat-file` will expand a sparse index to a full index when needed, but\n - is currently marked as needing a full index (or rather, not marked as\n - not needing a full index). This results in much slower `cat-file`\n + This change affects how 'git cat-file' works with the index when\n + specifying an object with the \":\" syntax (which will give file\n + contents from the index).\n +\n + 'git cat-file' will expand a sparse index to a full index when needed,\n + but is currently marked as needing a full index (or rather, not marked\n + as not needing a full index). This results in much slower 'git cat-file'\n operations when working within the sparse index, since we expand the\n index whether it's needed or not.\n \n - Mark `cat-file` as not needing a full index, so that you only pay the\n - cost of expanding the sparse index to a full index when you request a\n - file outside of the sparse index.\n + Mark 'git cat-file' as not needing a full index, so that you only pay\n + the cost of expanding the sparse index to a full index when you request\n + a file outside of the sparse index.\n \n Add tests to ensure both that:\n - - `cat-file` returns the correct file contents whether or not the file\n - is in the sparse index\n - - `cat-file` warns about expanding to the full index any time you\n - request something outside of the sparse index\n + - 'git cat-file' returns the correct file contents whether or not the\n + file is in the sparse index\n + - 'git cat-file' expands to the full index any time you request\n + something outside of the sparse index\n \n Signed-off-by: Kevin Lyles \n \n @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr\n \t\tif (opt_cw)\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: init_repos_as_submodules () {\n - }\n - \n - run_on_sparse () {\n -+\ttee run_on_sparse-checkout run_on_sparse-index &&\n -+\n - \t(\n - \t\tcd sparse-checkout &&\n - \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n --\t) &&\n -+\t) ../sparse-index-out 2>../sparse-index-err\n --\t)\n -+\t) ../full-checkout-out 2>../full-checkout-err\n --\t) &&\n --\trun_on_sparse \"$@\"\n -+\t) ... \" \" ... \"\n -+# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n - # Verifies that \"git sparse-checkout set ... \" succeeds and\n - # leaves the sparse index in a state where ... are sparse\n - # directories (and ... are not).\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseIndexExpanded' '\n \tgrep \"The sparse index is expanding to a full index\" err\n '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde\n +\techo \"new content\" >>sparse-index/deep/a &&\n +\trun_on_all git add deep/a &&\n +\n -+\ttest_all_match git cat-file -p HEAD:deep/a &&\n -+\tensure_not_expanded cat-file -p HEAD:deep/a &&\n -+\ttest_all_match git cat-file -p HEAD:folder1/a &&\n -+\tensure_not_expanded cat-file -p HEAD:folder1/a &&\n -+\n +\ttest_all_match git cat-file -p :deep/a &&\n +\tensure_not_expanded cat-file -p :deep/a &&\n -+\trun_on_all git cat-file -p :folder1/a &&\n -+\ttest_cmp full-checkout-out sparse-checkout-out &&\n -+\ttest_cmp full-checkout-out sparse-index-out &&\n -+\ttest_cmp full-checkout-err sparse-checkout-err &&\n ++\ttest_all_match git cat-file -p :folder1/a &&\n +\tensure_expanded cat-file -p :folder1/a'\n +\n +test_expect_success 'cat-file --batch' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde\n +\techo \"new content\" >>sparse-index/deep/a &&\n +\trun_on_all git add deep/a &&\n +\n -+\tcat <<-\\EOF | test_all_match git cat-file --batch &&\n -+\tHEAD:deep/a\n -+\t:deep/a\n -+\tEOF\n -+\tcat <<-\\EOF | ensure_not_expanded cat-file --batch &&\n -+\tHEAD:deep/a\n -+\t:deep/a\n -+\tEOF\n ++\techo \":deep/a\">in &&\n ++\ttest_all_match git cat-file --batch in &&\n ++\ttest_all_match git cat-file --batch in &&\n +\t:deep/a\n +\t:folder1/a\n +\tEOF\n -+\ttest_cmp full-checkout-out sparse-checkout-out &&\n -+\ttest_cmp full-checkout-out sparse-index-out &&\n -+\ttest_cmp full-checkout-err sparse-checkout-err &&\n -+\tcat <<-\\EOF | ensure_expanded cat-file --batch\n -+\t:deep/a\n -+\t:folder1/a\n -+\tEOF'\n ++\ttest_all_match git cat-file --batch \n\nThe 'run_on_sparse' and 'run_on_all' functions previously did not work\ncorrectly for commands accepting standard input. This also indirectly\naffected 'test_all_match' and 'test_sparse_match'.\n\nNow, we accept standard input and will send it to each command that we\nrun. This does not impact using the functions for commands that don't\nneed standard input.\n\nSigned-off-by: Kevin Lyles \n---\n t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex a2c0e1b4dcc..b140b642b21 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -179,22 +179,26 @@ init_repos_as_submodules () {\n }\n \n run_on_sparse () {\n+\tcat >run_on_sparse-input &&\n+\n \t(\n \t\tcd sparse-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n-\t) &&\n+\t) ../sparse-index-out 2>../sparse-index-err\n-\t)\n+\t) run_on_all-input &&\n+\n \t(\n \t\tcd full-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n-\t) &&\n-\trun_on_sparse \"$@\"\n+\t) ... \" \" ... \"\n+# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n # Verifies that \"git sparse-checkout set ... \" succeeds and\n # leaves the sparse index in a state where ... are sparse\n # directories (and ... are not).\n-- \ngitgitgadget\n\n"},{"id":"501941","messageId":"a92825e502f1795910b869165779279b89212939.1725052243.git.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":"pull.1770.v2.git.git.1725052243.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] Mark 'git cat-file' sparse-index compatible","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-30T21:10:43Z","receivedAt":"2024-08-30T21:10:49Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"From: Kevin Lyles \n\nThis change affects how 'git cat-file' works with the index when\nspecifying an object with the \":\" syntax (which will give file\ncontents from the index).\n\n'git cat-file' will expand a sparse index to a full index when needed,\nbut is currently marked as needing a full index (or rather, not marked\nas not needing a full index). This results in much slower 'git cat-file'\noperations when working within the sparse index, since we expand the\nindex whether it's needed or not.\n\nMark 'git cat-file' as not needing a full index, so that you only pay\nthe cost of expanding the sparse index to a full index when you request\na file outside of the sparse index.\n\nAdd tests to ensure both that:\n- 'git cat-file' returns the correct file contents whether or not the\n file is in the sparse index\n- 'git cat-file' expands to the full index any time you request\n something outside of the sparse index\n\nSigned-off-by: Kevin Lyles \n---\n builtin/cat-file.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++\n 2 files changed, 37 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8b..1afdfb5cbae 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \t/* Return early if we're in batch mode? */\n \tif (batch.enabled) {\n \t\tif (opt_cw)\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex b140b642b21..4d025c5f8d6 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2349,4 +2349,38 @@ test_expect_success 'advice.sparseIndexExpanded' '\n \tgrep \"The sparse index is expanding to a full index\" err\n '\n \n+test_expect_success 'cat-file -p' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\ttest_all_match git cat-file -p :deep/a &&\n+\tensure_not_expanded cat-file -p :deep/a &&\n+\ttest_all_match git cat-file -p :folder1/a &&\n+\tensure_expanded cat-file -p :folder1/a'\n+\n+test_expect_success 'cat-file --batch' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\techo \":deep/a\">in &&\n+\ttest_all_match git cat-file --batch in &&\n+\ttest_all_match git cat-file --batch in &&\n+\t:deep/a\n+\t:folder1/a\n+\tEOF\n+\ttest_all_match git cat-file --batch From: Kevin Lyles \n\n> +test_expect_success 'cat-file -p' '\n> +\tinit_repos &&\n> +\techo \"new content\" >>full-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-index/deep/a &&\n> +\trun_on_all git add deep/a &&\n> +\n> +\ttest_all_match git cat-file -p :deep/a &&\n> +\tensure_not_expanded cat-file -p :deep/a &&\n> +\ttest_all_match git cat-file -p :folder1/a &&\n> +\tensure_expanded cat-file -p :folder1/a'\n\n\nThere's one style issue that I missed in the previous version,\nwhich is that the final single-quote (') should be on a new line\nafter the last line of the test. like this:\n\n---\n\n\ttest_all_match git cat-file -p :folder1/a &&\n\tensure_expanded cat-file -p :folder1/a\n'\n\n---\n\n> +test_expect_success 'cat-file --batch' '\n> +\tinit_repos &&\n> +\techo \"new content\" >>full-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-index/deep/a &&\n> +\trun_on_all git add deep/a &&\n> +\n> +\techo \":deep/a\">in &&\n> +\ttest_all_match git cat-file --batch +\tensure_not_expanded cat-file --batch +\n> +\techo \":folder1/a\">in &&\n> +\ttest_all_match git cat-file --batch +\tensure_expanded cat-file --batch +\n> +\tcat <<-\\EOF >in &&\n\nnit: typically we would write this as \"cat >in <<-\\EOF &&\"\n\n> +\t:deep/a\n> +\t:folder1/a\n> +\tEOF\n> +\ttest_all_match git cat-file --batch +\tensure_expanded cat-file --batch +\n\nPointing out the final single-quote here, too.\n\nThank you for updating the heredocs and other format things that\nI noticed in the previous version. Also, thanks for splitting the\npatch into two parts.\n\nThanks for working on this!\n-Stolee\n\n\n"},{"id":"502047","messageId":"xmqqjzfslkkk.fsf@gitster.g","threadId":"62007","inReplyTo":"83ec9352-d274-4d05-a6b8-d33924f46205@gmail.com","subject":"Re: [PATCH v2 2/2] Mark 'git cat-file' sparse-index compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-03T17:21:31Z","receivedAt":"2024-09-03T17:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Thank you for updating the heredocs and other format things that\n> I noticed in the previous version. Also, thanks for splitting the\n> patch into two parts.\n>\n> Thanks for working on this!\n\nYup. It seems that the patches are getting almost ready?\n\nThanks for a detailed review.\n"},{"id":"502050","messageId":"pull.1770.v3.git.git.1725386044.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":"pull.1770.v2.git.git.1725052243.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] Mark 'git cat-file' sparse-index compatible","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-03T17:54:02Z","receivedAt":"2024-09-03T17:54:08Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"Please note that this is my first contribution to git. I've tried to follow\nthe instructions about how to correctly submit a patch (I'm using\nGitGitGadget as getting Outlook to do plain text e-mail correctly seems\nimpossible), but please let me know if I've missed something.\n\nMy motivation for making this change is purely performance. We have a large\nrepository that we enable the sparse index for, and I am developing a\npre-commit hook that (among other things) uses git cat-file to get the\nstaged contents of certain files. Without this change, getting the contents\nof a single small file from the index can take upwards of 10 seconds due to\nthe index expansion. After this change, it only takes ~0.3 seconds unless\nthe file is outside of the sparse index.\n\nKevin Lyles (2):\n Allow using stdin in run_on_* functions\n Mark 'git cat-file' sparse-index compatible\n\n builtin/cat-file.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 50 +++++++++++++++++++++---\n 2 files changed, 48 insertions(+), 5 deletions(-)\n\n\nbase-commit: 4590f2e9412378c61eac95966709c78766d326ba\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v3\nPull-Request: https://github.com/git/git/pull/1770\n\nRange-diff vs v2:\n\n 1: 7067a4c5da2 = 1: b310593aec2 Allow using stdin in run_on_* functions\n 2: a92825e502f ! 2: f4d1461b993 Mark 'git cat-file' sparse-index compatible\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde\n +\ttest_all_match git cat-file -p :deep/a &&\n +\tensure_not_expanded cat-file -p :deep/a &&\n +\ttest_all_match git cat-file -p :folder1/a &&\n -+\tensure_expanded cat-file -p :folder1/a'\n ++\tensure_expanded cat-file -p :folder1/a\n ++'\n +\n +test_expect_success 'cat-file --batch' '\n +\tinit_repos &&\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde\n +\techo \"new content\" >>sparse-index/deep/a &&\n +\trun_on_all git add deep/a &&\n +\n -+\techo \":deep/a\">in &&\n ++\techo \":deep/a\" >in &&\n +\ttest_all_match git cat-file --batch in &&\n ++\techo \":folder1/a\" >in &&\n +\ttest_all_match git cat-file --batch in &&\n ++\tcat >in <<-\\EOF &&\n +\t:deep/a\n +\t:folder1/a\n +\tEOF\n +\ttest_all_match git cat-file --batch \n\nThe 'run_on_sparse' and 'run_on_all' functions previously did not work\ncorrectly for commands accepting standard input. This also indirectly\naffected 'test_all_match' and 'test_sparse_match'.\n\nNow, we accept standard input and will send it to each command that we\nrun. This does not impact using the functions for commands that don't\nneed standard input.\n\nSigned-off-by: Kevin Lyles \n---\n t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 6fa7f5e9587..87953abf872 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -179,22 +179,26 @@ init_repos_as_submodules () {\n }\n \n run_on_sparse () {\n+\tcat >run_on_sparse-input &&\n+\n \t(\n \t\tcd sparse-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n-\t) &&\n+\t) ../sparse-index-out 2>../sparse-index-err\n-\t)\n+\t) run_on_all-input &&\n+\n \t(\n \t\tcd full-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n-\t) &&\n-\trun_on_sparse \"$@\"\n+\t) ... \" \" ... \"\n+# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n # Verifies that \"git sparse-checkout set ... \" succeeds and\n # leaves the sparse index in a state where ... are sparse\n # directories (and ... are not).\n-- \ngitgitgadget\n\n"},{"id":"502052","messageId":"f4d1461b99350151970528e12a92290cb65f7860.1725386044.git.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":"pull.1770.v3.git.git.1725386044.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] Mark 'git cat-file' sparse-index compatible","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-03T17:54:04Z","receivedAt":"2024-09-03T17:54:10Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"From: Kevin Lyles \n\nThis change affects how 'git cat-file' works with the index when\nspecifying an object with the \":\" syntax (which will give file\ncontents from the index).\n\n'git cat-file' will expand a sparse index to a full index when needed,\nbut is currently marked as needing a full index (or rather, not marked\nas not needing a full index). This results in much slower 'git cat-file'\noperations when working within the sparse index, since we expand the\nindex whether it's needed or not.\n\nMark 'git cat-file' as not needing a full index, so that you only pay\nthe cost of expanding the sparse index to a full index when you request\na file outside of the sparse index.\n\nAdd tests to ensure both that:\n- 'git cat-file' returns the correct file contents whether or not the\n file is in the sparse index\n- 'git cat-file' expands to the full index any time you request\n something outside of the sparse index\n\nSigned-off-by: Kevin Lyles \n---\n builtin/cat-file.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8b..1afdfb5cbae 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \t/* Return early if we're in batch mode? */\n \tif (batch.enabled) {\n \t\tif (opt_cw)\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 87953abf872..fbc10c8042b 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2358,4 +2358,40 @@ test_expect_success 'advice.sparseIndexExpanded' '\n \tgrep \"The sparse index is expanding to a full index\" err\n '\n \n+test_expect_success 'cat-file -p' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\ttest_all_match git cat-file -p :deep/a &&\n+\tensure_not_expanded cat-file -p :deep/a &&\n+\ttest_all_match git cat-file -p :folder1/a &&\n+\tensure_expanded cat-file -p :folder1/a\n+'\n+\n+test_expect_success 'cat-file --batch' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\techo \":deep/a\" >in &&\n+\ttest_all_match git cat-file --batch in &&\n+\ttest_all_match git cat-file --batch in <<-\\EOF &&\n+\t:deep/a\n+\t:folder1/a\n+\tEOF\n+\ttest_all_match git cat-file --batch writes:\n\n> Subject: Re: [PATCH v3 1/2] Allow using stdin in run_on_* functions\n\nImagine that these two commits are applied and appear among hundreds\nof other commits in the list of contributions. Would this commit\ntitle blend in and belong to others?\n\n $ git log --oneline --no-merges -100\n\ncf. Documentation/SubmittingPatches:describe-changes.\n\nAt least, it should somehow hint that the patch is about updating\nthe t1092 test script.\n\n Subject: t1092: allow run_on_* functions to use standard input\n\nor something.\n\n> From: Kevin Lyles \n>\n> The 'run_on_sparse' and 'run_on_all' functions previously did not work\n> correctly for commands accepting standard input.\n\n\nOur convention is to first describe the status quo in the present\ntense, so \"previously did not\" -> \"do not\".\n\nIt would be helpful to explain why they do not work, perhaps like\nso:\n\n ... functions do not work for commands that consume their\n standard input, because they attempt to run the same command in\n multiple repositories that are set up differently to inspect\n their behaviour differences.\n\n> This also indirectly\n> affected 'test_all_match' and 'test_sparse_match'.\n\n\"affected\" -> \"affects\".\n\n> Now, we accept standard input and will send it to each command that we\n> run. This does not impact using the functions for commands that don't\n> need standard input.\n\nAnd after presenting the observation on the status quo and pointing\nout the issue we are going to address, it is our convention to write\nwhat to do in imperative mood, as if we are ordering the codebase to\n\"become like so\". E.g.\n\n To allow these commands to consume the standard input, first\n slurp the contents fed from the standard input to a temporary\n file, and then run each attempt with its standard input\n redirected from the temporary file. This way, these multiple\n attempts will all see the same contents from their standard\n input.\n\n Note that a command that does not read from the standard input\n does not have to redirect its standard input from Signed-off-by: Kevin Lyles \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----\n> 1 file changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 6fa7f5e9587..87953abf872 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -179,22 +179,26 @@ init_repos_as_submodules () {\n> }\n> \n> run_on_sparse () {\n> +\tcat >run_on_sparse-input &&\n\nMixture of word_with_underscore-and-dash-separation look unexpected;\nuse \"run-on-sparse-input\" instead, as the output files seem to be\nnamed that way?\n\n> \t(\n> \t\tcd sparse-checkout &&\n> \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n> -\t) &&\n> +\t) \t(\n> \t\tcd sparse-index &&\n> \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-index-out 2>../sparse-index-err\n> -\t)\n> +\t) }\n> \n> run_on_all () {\n> +\tcat >run_on_all-input &&\n> +\n> \t(\n> \t\tcd full-checkout &&\n> \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n> -\t) &&\n> -\trun_on_sparse \"$@\"\n> +\t) +\trun_on_sparse \"$@\" }\n> \n> test_all_match () {\n> @@ -221,7 +225,7 @@ test_sparse_unstaged () {\n> \tdone\n> }\n> \n> -# Usage: test_sprase_checkout_set \" ... \" \" ... \"\n> +# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n> # Verifies that \"git sparse-checkout set ... \" succeeds and\n> # leaves the sparse index in a state where ... are sparse\n> # directories (and ... are not).\n"},{"id":"502056","messageId":"xmqqle08ilzd.fsf@gitster.g","threadId":"62007","inReplyTo":"f4d1461b99350151970528e12a92290cb65f7860.1725386044.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] Mark 'git cat-file' sparse-index compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-03T19:19:18Z","receivedAt":"2024-09-03T19:19:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kevin Lyles via GitGitGadget\" writes:\n\n> Subject: Re: [PATCH v3 2/2] Mark 'git cat-file' sparse-index compatible\n\nThe same comment on the commit title applies here.\n\n> From: Kevin Lyles \n>\n> This change affects how 'git cat-file' works with the index when\n\nAgain, we start by describing the status quo (e.g. \"'git cat-file'\nalways expands a sparse-index to a full index\"), explaining why it\nis undesirable, and hinting what you want to do about it.\n\nAnd then give an order to the codebase to \"become like so\".\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 18fe58d6b8b..1afdfb5cbae 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n> \tif (batch.buffer_output < 0)\n> \t\tbatch.buffer_output = batch.all_objects;\n> \n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n> +\n> \t/* Return early if we're in batch mode? */\n> \tif (batch.enabled) {\n> \t\tif (opt_cw)\n\nHow should the correctness of a change line this validated, by the\nway? By following manually all the code paths from this point and\nmaking sure that the access to an element (or size) of the index\nthat is sparsed out is preceded by a lazy rehydration of a tree that\nrepresents a subhierarchy in the sparse-index? Addition to end-to-end\ntests may increase the test coverage, but I am not sure how to ensure\nthe coverage is exhaustive.\n\nThanks.\n"},{"id":"502072","messageId":"pull.1770.v4.git.git.1725401207.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":"pull.1770.v3.git.git.1725386044.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] builtin/cat-file: mark 'git cat-file' sparse-index compatible","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-03T22:06:45Z","receivedAt":"2024-09-03T22:06:51Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"I believe I've addressed all issues raised except the question about how to\nexhaustively validate the changes. I was under the impression that the\ncombination of the existing 'git cat-file' test cases and my new cases were\nenough, but I have thus far been unable to get code coverage working locally\nto validate that myself, due to spurious failures in other tests. I've\nkicked off another run with most unrelated tests removed to see if that\nhelps.\n\nPlease note that this is my first contribution to git. I've tried to follow\nthe instructions about how to correctly submit a patch (I'm using\nGitGitGadget as getting Outlook to do plain text e-mail correctly seems\nimpossible), but please let me know if I've missed something.\n\nMy motivation for making this change is purely performance. We have a large\nrepository that we enable the sparse index for, and I am developing a\npre-commit hook that (among other things) uses git cat-file to get the\nstaged contents of certain files. Without this change, getting the contents\nof a single small file from the index can take upwards of 10 seconds due to\nthe index expansion. After this change, it only takes ~0.3 seconds unless\nthe file is outside of the sparse index.\n\nKevin Lyles (2):\n t1092: allow run_on_* functions to use standard input\n builtin/cat-file: mark 'git cat-file' sparse-index compatible\n\n builtin/cat-file.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 50 +++++++++++++++++++++---\n 2 files changed, 48 insertions(+), 5 deletions(-)\n\n\nbase-commit: 4590f2e9412378c61eac95966709c78766d326ba\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v4\nPull-Request: https://github.com/git/git/pull/1770\n\nRange-diff vs v3:\n\n 1: b310593aec2 ! 1: 73fe71abcd5 Allow using stdin in run_on_* functions\n @@ Metadata\n Author: Kevin Lyles \n \n ## Commit message ##\n - Allow using stdin in run_on_* functions\n + t1092: allow run_on_* functions to use standard input\n \n - The 'run_on_sparse' and 'run_on_all' functions previously did not work\n - correctly for commands accepting standard input. This also indirectly\n - affected 'test_all_match' and 'test_sparse_match'.\n + The 'run_on_sparse' and 'run_on_all' functions do not work correctly for\n + commands accepting standard input, because they run the same command\n + multiple times and the first instance consumes it. This also indirectly\n + affects 'test_all_match' and 'test_sparse_match'.\n \n - Now, we accept standard input and will send it to each command that we\n - run. This does not impact using the functions for commands that don't\n - need standard input.\n + To allow these functions to work with commands accepting standard input,\n + first slurp standard input to a temporary file, and then run the command\n + with its standard input redirected from the temporary file. This ensures\n + that each command sees the same contents from its standard input.\n +\n + Note that this does not impact commands that do not read from standard\n + input; they continue to ignore it. Additionally, existing uses of the\n + run_on_* functions do not need to do anything differently, as the\n + standard input of the test environment is already connected to\n + /dev/null.\n +\n + We do not explicitly clean up the input files because they are cleaned\n + up with the rest of the test repositories and their contents may be\n + useful for figuring out which command failed when a test case fails.\n \n Signed-off-by: Kevin Lyles \n \n @@ t/t1092-sparse-checkout-compatibility.sh: init_repos_as_submodules () {\n }\n \n run_on_sparse () {\n -+\tcat >run_on_sparse-input &&\n ++\tcat >run-on-sparse-input &&\n +\n \t(\n \t\tcd sparse-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n -\t) &&\n -+\t) ../sparse-index-out 2>../sparse-index-err\n -\t)\n -+\t) run_on_all-input &&\n ++\tcat >run-on-all-input &&\n +\n \t(\n \t\tcd full-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n -\t) &&\n -\trun_on_sparse \"$@\"\n -+\t) \n \n ## Commit message ##\n - Mark 'git cat-file' sparse-index compatible\n + builtin/cat-file: mark 'git cat-file' sparse-index compatible\n \n This change affects how 'git cat-file' works with the index when\n specifying an object with the \":\" syntax (which will give file\n contents from the index).\n \n - 'git cat-file' will expand a sparse index to a full index when needed,\n - but is currently marked as needing a full index (or rather, not marked\n - as not needing a full index). This results in much slower 'git cat-file'\n - operations when working within the sparse index, since we expand the\n - index whether it's needed or not.\n + 'git cat-file' expands a sparse index to a full index any time contents\n + are requested from the index by specifying an object with the \":\"\n + syntax. This is true even when the requested file is part of the sparse\n + index, and results in much slower 'git cat-file' operations when working\n + within the sparse index.\n \n Mark 'git cat-file' as not needing a full index, so that you only pay\n the cost of expanding the sparse index to a full index when you request\n\n-- \ngitgitgadget\n"},{"id":"502073","messageId":"73fe71abcd578316a81615feee9561015c2a1c53.1725401207.git.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":"pull.1770.v4.git.git.1725401207.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] t1092: allow run_on_* functions to use standard input","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-03T22:06:46Z","receivedAt":"2024-09-03T22:06:52Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"From: Kevin Lyles \n\nThe 'run_on_sparse' and 'run_on_all' functions do not work correctly for\ncommands accepting standard input, because they run the same command\nmultiple times and the first instance consumes it. This also indirectly\naffects 'test_all_match' and 'test_sparse_match'.\n\nTo allow these functions to work with commands accepting standard input,\nfirst slurp standard input to a temporary file, and then run the command\nwith its standard input redirected from the temporary file. This ensures\nthat each command sees the same contents from its standard input.\n\nNote that this does not impact commands that do not read from standard\ninput; they continue to ignore it. Additionally, existing uses of the\nrun_on_* functions do not need to do anything differently, as the\nstandard input of the test environment is already connected to\n/dev/null.\n\nWe do not explicitly clean up the input files because they are cleaned\nup with the rest of the test repositories and their contents may be\nuseful for figuring out which command failed when a test case fails.\n\nSigned-off-by: Kevin Lyles \n---\n t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 6fa7f5e9587..4cbe9b1465d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -179,22 +179,26 @@ init_repos_as_submodules () {\n }\n \n run_on_sparse () {\n+\tcat >run-on-sparse-input &&\n+\n \t(\n \t\tcd sparse-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../sparse-checkout-out 2>../sparse-checkout-err\n-\t) &&\n+\t) ../sparse-index-out 2>../sparse-index-err\n-\t)\n+\t) run-on-all-input &&\n+\n \t(\n \t\tcd full-checkout &&\n \t\tGIT_PROGRESS_DELAY=100000 \"$@\" >../full-checkout-out 2>../full-checkout-err\n-\t) &&\n-\trun_on_sparse \"$@\"\n+\t) ... \" \" ... \"\n+# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n # Verifies that \"git sparse-checkout set ... \" succeeds and\n # leaves the sparse index in a state where ... are sparse\n # directories (and ... are not).\n-- \ngitgitgadget\n\n"},{"id":"502074","messageId":"ac913257309960d86a9c11e825c76621c6ac405c.1725401207.git.gitgitgadget@gmail.com","threadId":"62007","inReplyTo":"pull.1770.v4.git.git.1725401207.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] builtin/cat-file: mark 'git cat-file' sparse-index compatible","fromName":"Kevin Lyles via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-03T22:06:47Z","receivedAt":"2024-09-03T22:06:53Z","isPatch":true,"sender":{"key":"klyles@epic.com","avatar":null},"body":"From: Kevin Lyles \n\nThis change affects how 'git cat-file' works with the index when\nspecifying an object with the \":\" syntax (which will give file\ncontents from the index).\n\n'git cat-file' expands a sparse index to a full index any time contents\nare requested from the index by specifying an object with the \":\"\nsyntax. This is true even when the requested file is part of the sparse\nindex, and results in much slower 'git cat-file' operations when working\nwithin the sparse index.\n\nMark 'git cat-file' as not needing a full index, so that you only pay\nthe cost of expanding the sparse index to a full index when you request\na file outside of the sparse index.\n\nAdd tests to ensure both that:\n- 'git cat-file' returns the correct file contents whether or not the\n file is in the sparse index\n- 'git cat-file' expands to the full index any time you request\n something outside of the sparse index\n\nSigned-off-by: Kevin Lyles \n---\n builtin/cat-file.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 18fe58d6b8b..1afdfb5cbae 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \t/* Return early if we're in batch mode? */\n \tif (batch.enabled) {\n \t\tif (opt_cw)\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 4cbe9b1465d..eb32da2a7f2 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2358,4 +2358,40 @@ test_expect_success 'advice.sparseIndexExpanded' '\n \tgrep \"The sparse index is expanding to a full index\" err\n '\n \n+test_expect_success 'cat-file -p' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\ttest_all_match git cat-file -p :deep/a &&\n+\tensure_not_expanded cat-file -p :deep/a &&\n+\ttest_all_match git cat-file -p :folder1/a &&\n+\tensure_expanded cat-file -p :folder1/a\n+'\n+\n+test_expect_success 'cat-file --batch' '\n+\tinit_repos &&\n+\techo \"new content\" >>full-checkout/deep/a &&\n+\techo \"new content\" >>sparse-checkout/deep/a &&\n+\techo \"new content\" >>sparse-index/deep/a &&\n+\trun_on_all git add deep/a &&\n+\n+\techo \":deep/a\" >in &&\n+\ttest_all_match git cat-file --batch in &&\n+\ttest_all_match git cat-file --batch in <<-\\EOF &&\n+\t:deep/a\n+\t:folder1/a\n+\tEOF\n+\ttest_all_match git cat-file --batch writes:\n\n> From: Kevin Lyles \n>\n> The 'run_on_sparse' and 'run_on_all' functions do not work correctly for\n> commands accepting standard input, because they run the same command\n> multiple times and the first instance consumes it. This also indirectly\n> affects 'test_all_match' and 'test_sparse_match'.\n>\n> ...\n> -# Usage: test_sprase_checkout_set \" ... \" \" ... \"\n> +# Usage: test_sparse_checkout_set \" ... \" \" ... \"\n> # Verifies that \"git sparse-checkout set ... \" succeeds and\n> # leaves the sparse index in a state where ... are sparse\n> # directories (and ... are not).\n\nReads much better. Will queue. Thanks.\n"},{"id":"502151","messageId":"xmqqcyljbclu.fsf@gitster.g","threadId":"62007","inReplyTo":"ac913257309960d86a9c11e825c76621c6ac405c.1725401207.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] builtin/cat-file: mark 'git cat-file' sparse-index compatible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T16:35:57Z","receivedAt":"2024-09-04T16:36:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kevin Lyles via GitGitGadget\" writes:\n\n> From: Kevin Lyles \n>\n> This change affects how 'git cat-file' works with the index when\n> specifying an object with the \":\" syntax (which will give file\n> contents from the index).\n\nThe above is not as suitable as the first paragraph as the one that\ncomes next, which describes the status quo and highlights what the\nproblem is. With the few paragraphs below, that talk about the\ninteraction among \":\" syntax, get_oid_with_context(), and\nthe sparse-index, I think we can just remove it.\n\n> 'git cat-file' expands a sparse index to a full index any time contents\n> are requested from the index by specifying an object with the \":\"\n> syntax. This is true even when the requested file is part of the sparse\n> index, and results in much slower 'git cat-file' operations when working\n> within the sparse index.\n>\n> Mark 'git cat-file' as not needing a full index, so that you only pay\n> the cost of expanding the sparse index to a full index when you request\n> a file outside of the sparse index.\n>\n> Add tests to ensure both that:\n> - 'git cat-file' returns the correct file contents whether or not the\n> file is in the sparse index\n> - 'git cat-file' expands to the full index any time you request\n> something outside of the sparse index\n>\n> Signed-off-by: Kevin Lyles \n> ---\n\nNicely explained.\n\n> @@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n> \tif (batch.buffer_output < 0)\n> \t\tbatch.buffer_output = batch.all_objects;\n> \n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n> +\n\nOK. This command does not start parsing the command line arguments\nbefore this point, and this is really a good place to toggle the bit\noff.\n\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 4cbe9b1465d..eb32da2a7f2 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2358,4 +2358,40 @@ test_expect_success 'advice.sparseIndexExpanded' '\n> \tgrep \"The sparse index is expanding to a full index\" err\n> '\n> \n> +test_expect_success 'cat-file -p' '\n> +\tinit_repos &&\n> +\techo \"new content\" >>full-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-index/deep/a &&\n> +\trun_on_all git add deep/a &&\n> +\n> +\ttest_all_match git cat-file -p :deep/a &&\n> +\tensure_not_expanded cat-file -p :deep/a &&\n> +\ttest_all_match git cat-file -p :folder1/a &&\n> +\tensure_expanded cat-file -p :folder1/a\n> +'\n\nOK. These are about the object names given from the command line.\n\n> +test_expect_success 'cat-file --batch' '\n> +\tinit_repos &&\n> +\techo \"new content\" >>full-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-checkout/deep/a &&\n> +\techo \"new content\" >>sparse-index/deep/a &&\n> +\trun_on_all git add deep/a &&\n> +\n> +\techo \":deep/a\" >in &&\n> +\ttest_all_match git cat-file --batch +\tensure_not_expanded cat-file --batch +\n> +\techo \":folder1/a\" >in &&\n> +\ttest_all_match git cat-file --batch +\tensure_expanded cat-file --batch +\n> +\tcat >in <<-\\EOF &&\n> +\t:deep/a\n> +\t:folder1/a\n> +\tEOF\n> +\ttest_all_match git cat-file --batch +\tensure_expanded cat-file --batch +'\n\nAnd these are about the object names fed via the --batch mechanism.\n\nLooking good.\n\nWill queue. Thanks.\n"}]}