{"thread":{"id":"57689","subject":"[PATCH 0/4] Sparse index integration with 'git show'","startedAt":"2022-04-07T16:37:29Z","lastAt":"2022-04-27T13:47:54Z","messageCount":26,"participants":["Derrick Stolee via GitGitGadget","Philip Oakley","Junio C Hamano","Derrick Stolee","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"453270","messageId":"pull.1207.git.1649349442.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":null,"subject":"[PATCH 0/4] Sparse index integration with 'git show'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-07T16:37:18Z","receivedAt":"2022-04-07T16:37:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This continues our sequence of integrating builtins with the sparse index.\n\n'git show' is relatively simple to get working in a way that doesn't fail\nwhen it would previously succeed, but there are some sutbleties when the\nuser passes a directory path. If that path happens to be a sparse directory\nentry, we suddenly start succeeding and printing the tree information!\n\nSince this behavior can change depending on the sparse checkout definition\nand the state of index entries within that directory, this new behavior\nwould be more likely to confuse users than help them.\n\nHere is an outline of this series:\n\n * Patch 1: Add more tests around 'git show :' in t1092.\n\n * Patch 2: Make 'git show' stop expanding the index by default. Make note\n of this behavior change in the tests.\n\n * Patches 3-4: Make the subtle changes to object-name.c that help us reject\n sparse directories (patch 3) and print the correct error message (patch\n 4).\n\nPatches 2-4 could realistically be squashed into a single commit, but I\nthought it might be instructive to show these individual steps, especially\nas an example for our GSoC project.\n\nThis series is based on the current 'master'. I know that Victoria intends\nto submit her 'git stash' integration soon, and this provides a way to test\nif our split of test changes in t1092 are easy to merge without conflict. If\nthat is successful, then I will likely submit my integration with the\n'sparse-checkout' builtin after this series is complete.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n t1092: add compatibility tests for 'git show'\n show: integrate with the sparse index\n object-name: reject trees found in the index\n object-name: diagnose trees in index properly\n\n builtin/log.c | 5 ++++\n object-name.c | 25 +++++++++++++++---\n t/t1092-sparse-checkout-compatibility.sh | 33 ++++++++++++++++++++++++\n 3 files changed, 60 insertions(+), 3 deletions(-)\n\n\nbase-commit: 07330a41d66a2c9589b585a3a24ecdcf19994f19\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1207%2Fderrickstolee%2Fsparse-index%2Fgit-show-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1207/derrickstolee/sparse-index/git-show-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1207\n-- \ngitgitgadget\n"},{"id":"453271","messageId":"8c2fdb5a4fc3317c05324da54692036e36fc15f3.1649349442.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.git.1649349442.gitgitgadget@gmail.com","subject":"[PATCH 1/4] t1092: add compatibility tests for 'git show'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-07T16:37:19Z","receivedAt":"2022-04-07T16:37:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 236ab530284..74792b5ebbc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1151,6 +1151,22 @@ test_expect_success 'clean' '\n \ttest_sparse_match test_path_is_dir folder1\n '\n \n+test_expect_success 'show (cached blobs/trees)' '\n+\tinit_repos &&\n+\n+\ttest_all_match git show :a &&\n+\ttest_all_match git show :deep/a &&\n+\ttest_sparse_match git show :folder1/a &&\n+\n+\t# Asking \"git show\" for directories in the index\n+\t# does not work as implemented. The error message is\n+\t# different for a full checkout and a sparse checkout\n+\t# when the directory is outside of the cone.\n+\ttest_all_match test_must_fail git show :deep/ &&\n+\ttest_must_fail git -C full-checkout show :folder1/ &&\n+\ttest_sparse_match test_must_fail git show :folder1/\n+'\n+\n test_expect_success 'submodule handling' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"453272","messageId":"27ab853a9b4f0a50880c92dd8949d6fe9b7f00ba.1649349442.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.git.1649349442.gitgitgadget@gmail.com","subject":"[PATCH 2/4] show: integrate with the sparse index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-07T16:37:20Z","receivedAt":"2022-04-07T16:37:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git show' command can take an input to request the state of an\nobject in the index. This can lead to parsing the index in order to load\na specific file entry. Without the change presented here, a sparse index\nwould expand to a full one, taking much longer than usual to access a\nsimple file.\n\nThere is one behavioral change that happens here, though: we now can\nfind a sparse directory entry within the index! Commands that previously\nfailed because we could not find an entry in the worktree or index now\nsucceed because we _do_ find an entry in the index.\n\nThere might be more work to do to make other situations succeed when\nlooking for an indexed tree, perhaps by looking at or updating the\ncache-tree extension as needed. These situations include having a full\nindex or asking for a directory that is within the sparse-checkout cone\n(and hence is not a sparse directory entry in the index).\n\nFor now, we demonstrate how the sparse index integration is extremely\nsimple for files outside of the cone as well as directories within the\ncone. A later change will resolve this behavior around sparse\ndirectories.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/log.c | 5 +++++\n t/t1092-sparse-checkout-compatibility.sh | 23 +++++++++++++++++++----\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c211d66d1d0..8e2e9912ab9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -661,6 +661,11 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \tinit_log_defaults();\n \tgit_config(git_log_config, NULL);\n \n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n+\n \tmemset(&match_all, 0, sizeof(match_all));\n \trepo_init_revisions(the_repository, &rev, prefix);\n \tgit_config(grep_config, &rev.grep_filter);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 74792b5ebbc..f6a14e08b81 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1159,12 +1159,20 @@ test_expect_success 'show (cached blobs/trees)' '\n \ttest_sparse_match git show :folder1/a &&\n \n \t# Asking \"git show\" for directories in the index\n-\t# does not work as implemented. The error message is\n-\t# different for a full checkout and a sparse checkout\n-\t# when the directory is outside of the cone.\n+\t# changes depending on the existence of a sparse index.\n \ttest_all_match test_must_fail git show :deep/ &&\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n-\ttest_sparse_match test_must_fail git show :folder1/\n+\ttest_must_fail git -C sparse-checkout show :folder1/ &&\n+\n+\tgit -C sparse-index show :folder1/ >actual &&\n+\tgit -C full-checkout show HEAD:folder1 >expect &&\n+\n+\t# The output of \"git show\" includes the way we referenced the\n+\t# objects, so strip that out.\n+\ttest_line_count = 4 actual &&\n+\ttail -n 2 actual >actual-trunc &&\n+\ttail -n 2 expect >expect-trunc &&\n+\ttest_cmp expect-trunc actual-trunc\n '\n \n test_expect_success 'submodule handling' '\n@@ -1388,6 +1396,13 @@ test_expect_success 'sparse index is not expanded: diff' '\n \tensure_not_expanded diff --cached\n '\n \n+test_expect_success 'sparse index is not expanded: show' '\n+\tinit_repos &&\n+\n+\tensure_not_expanded show :a &&\n+\tensure_not_expanded show :deep/a\n+'\n+\n test_expect_success 'sparse index is not expanded: update-index' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"453273","messageId":"f5da532767367844af7ecc477e1c8434a5454fd1.1649349442.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.git.1649349442.gitgitgadget@gmail.com","subject":"[PATCH 3/4] object-name: reject trees found in the index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-07T16:37:21Z","receivedAt":"2022-04-07T16:37:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_oid_with_context_1() method is used when parsing revision\narguments. One particular case is to take a \":\" string and search\nthe index for the given path.\n\nIn the case of a sparse index, this might find a sparse directory entry,\nin which case the contained object is a tree. In the case of a full\nindex, this search within the index would fail.\n\nIn order to maintain identical return state as in a full index, inspect\nthe discovered cache entry to see if it is a sparse directory and reject\nit. This requires being careful around the only_to_die option to be sure\nwe die only at the correct time.\n\nThis changes the behavior of 'git show :', but does not\nbring it entirely into alignment with a full index case. It specifically\nhits the wrong error message within diagnose_invalid_index_path(). That\nerror message will be corrected in a future change.\n\nSigned-off-by: Derrick Stolee \n---\n object-name.c | 19 ++++++++++++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 11 ++---------\n 2 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex f0e327f91f5..2dc5d2549b8 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1881,6 +1881,20 @@ static char *resolve_relative_path(struct repository *r, const char *rel)\n \t\t\t rel);\n }\n \n+static int reject_tree_in_index(struct repository *repo,\n+\t\t\t\tint only_to_die,\n+\t\t\t\tconst struct cache_entry *ce,\n+\t\t\t\tint stage,\n+\t\t\t\tconst char *prefix,\n+\t\t\t\tconst char *cp)\n+{\n+\tif (!S_ISSPARSEDIR(ce->ce_mode))\n+\t\treturn 0;\n+\tif (only_to_die)\n+\t\tdiagnose_invalid_index_path(repo, stage, prefix, cp);\n+\treturn -1;\n+}\n+\n static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\t\t const char *name,\n \t\t\t\t unsigned flags,\n@@ -1955,9 +1969,12 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\t memcmp(ce->name, cp, namelen))\n \t\t\t\tbreak;\n \t\t\tif (ce_stage(ce) == stage) {\n+\t\t\t\tfree(new_path);\n+\t\t\t\tif (reject_tree_in_index(repo, only_to_die, ce,\n+\t\t\t\t\t\t\t stage, prefix, cp))\n+\t\t\t\t\treturn -1;\n \t\t\t\toidcpy(oid, &ce->oid);\n \t\t\t\toc->mode = ce->ce_mode;\n-\t\t\t\tfree(new_path);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tpos++;\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex f6a14e08b81..9d32361110d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1164,15 +1164,8 @@ test_expect_success 'show (cached blobs/trees)' '\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n \ttest_must_fail git -C sparse-checkout show :folder1/ &&\n \n-\tgit -C sparse-index show :folder1/ >actual &&\n-\tgit -C full-checkout show HEAD:folder1 >expect &&\n-\n-\t# The output of \"git show\" includes the way we referenced the\n-\t# objects, so strip that out.\n-\ttest_line_count = 4 actual &&\n-\ttail -n 2 actual >actual-trunc &&\n-\ttail -n 2 expect >expect-trunc &&\n-\ttest_cmp expect-trunc actual-trunc\n+\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n+\tgrep \"is in the index, but not at stage 0\" err\n '\n \n test_expect_success 'submodule handling' '\n-- \ngitgitgadget\n\n"},{"id":"453274","messageId":"99c09ccc2406e4f54c620bd7fb2d1205386e23a6.1649349442.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.git.1649349442.gitgitgadget@gmail.com","subject":"[PATCH 4/4] object-name: diagnose trees in index properly","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-07T16:37:22Z","receivedAt":"2022-04-07T16:37:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'git show :' where '' is a directory, then\nthere is a subtle difference between a full checkout and a sparse\ncheckout. The error message from diagnose_invalid_index_path() reports\nwhether the path is on disk or not. The full checkout will have the\ndirectory on disk, but the path will not be in the index. The sparse\nchecokut could have the directory not exist, specifically when that\ndirectory is outside of the sparse-checkout cone.\n\nIn the case of a sparse index, we have yet another state: the path can\nbe a sparse directory in the index. In this case, the error message from\ndiagnose_invalid_index_path() would erroneously say \"path '' is in\nthe index, but not at stage 0\", which is false.\n\nAdd special casing around sparse directory entries so we get to the\ncorrect error message. This requires two checks in order to get parity\nwith the normal sparse-checkout case.\n\nSigned-off-by: Derrick Stolee \n---\n object-name.c | 6 ++++--\n t/t1092-sparse-checkout-compatibility.sh | 13 +++++++++++--\n 2 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 2dc5d2549b8..4d2746574cd 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1832,7 +1832,8 @@ static void diagnose_invalid_index_path(struct repository *r,\n \t\tpos = -pos - 1;\n \tif (pos < istate->cache_nr) {\n \t\tce = istate->cache[pos];\n-\t\tif (ce_namelen(ce) == namelen &&\n+\t\tif (!S_ISSPARSEDIR(ce->ce_mode) &&\n+\t\t ce_namelen(ce) == namelen &&\n \t\t !memcmp(ce->name, filename, namelen))\n \t\t\tdie(_(\"path '%s' is in the index, but not at stage %d\\n\"\n \t\t\t \"hint: Did you mean ':%d:%s'?\"),\n@@ -1848,7 +1849,8 @@ static void diagnose_invalid_index_path(struct repository *r,\n \t\tpos = -pos - 1;\n \tif (pos < istate->cache_nr) {\n \t\tce = istate->cache[pos];\n-\t\tif (ce_namelen(ce) == fullname.len &&\n+\t\tif (!S_ISSPARSEDIR(ce->ce_mode) &&\n+\t\t ce_namelen(ce) == fullname.len &&\n \t\t !memcmp(ce->name, fullname.buf, fullname.len))\n \t\t\tdie(_(\"path '%s' is in the index, but not '%s'\\n\"\n \t\t\t \"hint: Did you mean ':%d:%s' aka ':%d:./%s'?\"),\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 9d32361110d..921cafb950a 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1164,8 +1164,17 @@ test_expect_success 'show (cached blobs/trees)' '\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n \ttest_must_fail git -C sparse-checkout show :folder1/ &&\n \n-\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n-\tgrep \"is in the index, but not at stage 0\" err\n+\ttest_sparse_match test_must_fail git show :folder1/ &&\n+\n+\t# Change the sparse cone for an extra case:\n+\trun_on_sparse git sparse-checkout set deep/deeper1 &&\n+\n+\t# deep/deeper2 is a sparse directory in the sparse index.\n+\ttest_sparse_match test_must_fail git show :deep/deeper2/ &&\n+\n+\t# deep/deeper2/deepest is not in the sparse index, but\n+\t# will trigger an index expansion.\n+\ttest_sparse_match test_must_fail git show :deep/deeper2/deepest/\n '\n \n test_expect_success 'submodule handling' '\n-- \ngitgitgadget\n"},{"id":"453291","messageId":"2063f989-715a-93b2-a665-e6975d54c8b5@iee.email","threadId":"57689","inReplyTo":"99c09ccc2406e4f54c620bd7fb2d1205386e23a6.1649349442.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] object-name: diagnose trees in index properly","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2022-04-07T20:46:27Z","receivedAt":"2022-04-07T20:50:55Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"spelling nit\n\nOn 07/04/2022 17:37, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> When running 'git show :' where '' is a directory, then\n> there is a subtle difference between a full checkout and a sparse\n> checkout. The error message from diagnose_invalid_index_path() reports\n> whether the path is on disk or not. The full checkout will have the\n> directory on disk, but the path will not be in the index. The sparse\n> checokut could have the directory not exist, specifically when that\n\ns/checokut/checkout/\n> directory is outside of the sparse-checkout cone.\n[...]\n--\nPhilip\n"},{"id":"453436","messageId":"xmqqlewaerqd.fsf@gitster.g","threadId":"57689","inReplyTo":"99c09ccc2406e4f54c620bd7fb2d1205386e23a6.1649349442.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] object-name: diagnose trees in index properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-12T06:32:58Z","receivedAt":"2022-04-12T06:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> When running 'git show :' where '' is a directory, then\n> there is a subtle difference between a full checkout and a sparse\n> checkout. The error message from diagnose_invalid_index_path() reports\n> whether the path is on disk or not. The full checkout will have the\n> directory on disk, but the path will not be in the index. The sparse\n> checokut could have the directory not exist, specifically when that\n> directory is outside of the sparse-checkout cone.\n>\n> In the case of a sparse index, we have yet another state: the path can\n> be a sparse directory in the index. In this case, the error message from\n> diagnose_invalid_index_path() would erroneously say \"path '' is in\n> the index, but not at stage 0\", which is false.\n>\n> Add special casing around sparse directory entries so we get to the\n> correct error message. This requires two checks in order to get parity\n> with the normal sparse-checkout case.\n\nThat all makes sense, but let me ask a more basic (and possibly\nstupid) question and a half. \n\n - When running 'git cmd :' where '' is a directory,\n even before the \"sparse-index\" or \"sparse-checkout\" world, I\n sometimes wished that \":\" resolved to the object name of\n the tree recorded in the cache-tree, if we have one. If we are\n living in the \"sparse-index\" world, and the paths underneath\n '' are not in the index (because we are not interested in\n them), we should be able answer \"git rev-parse :\" with the\n name of the tree object. It is a \"new feature\" regardless of\n sparse-index, but I wonder if it is sensible to add more code to\n engrave in stone that we would not support it and keep treating\n the index as if it is a flat list of paths to blobs (and commits,\n for submodules)?\n\n - When running 'git cmd :' where '' is *not* a\n directory but is not in the index because of \"sparse-index\"\n (i.e. a leading directory of '' is represented as a tree in\n the index), should \":\" index expand the \"tree\" index entry\n on-demand, so that has its own entry in the index, as if\n no \"sparse-index\" is in effect? The tests I saw in the series\n were mostly asserted with test_sparse_match, not test_all_match,\n so I couldn't tell what the expectations are.\n\n - More generally, if path is represented as a\n \"sparse-dir\" entry, should \":/\" cause the index\n entry for path to be expanded on-demand? I am guessing\n that the answer is yes, as we wouldn't be able to even tell if\n such a path / would exist in the index if the\n index were converted to full upfront.\n\nThanks.\n"},{"id":"453445","messageId":"1f027d22-9c5c-7ccf-37e7-611d67de7cfd@github.com","threadId":"57689","inReplyTo":"xmqqlewaerqd.fsf@gitster.g","subject":"Re: [PATCH 4/4] object-name: diagnose trees in index properly","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-12T13:52:45Z","receivedAt":"2022-04-12T13:52:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/12/2022 2:32 AM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> When running 'git show :' where '' is a directory, then\n>> there is a subtle difference between a full checkout and a sparse\n>> checkout. The error message from diagnose_invalid_index_path() reports\n>> whether the path is on disk or not. The full checkout will have the\n>> directory on disk, but the path will not be in the index. The sparse\n>> checokut could have the directory not exist, specifically when that\n>> directory is outside of the sparse-checkout cone.\n>>\n>> In the case of a sparse index, we have yet another state: the path can\n>> be a sparse directory in the index. In this case, the error message from\n>> diagnose_invalid_index_path() would erroneously say \"path '' is in\n>> the index, but not at stage 0\", which is false.\n>>\n>> Add special casing around sparse directory entries so we get to the\n>> correct error message. This requires two checks in order to get parity\n>> with the normal sparse-checkout case.\n> \n> That all makes sense, but let me ask a more basic (and possibly\n> stupid) question and a half.\n\nAll of these questions are good thoughts. \n\n> - When running 'git cmd :' where '' is a directory,\n> even before the \"sparse-index\" or \"sparse-checkout\" world, I\n> sometimes wished that \":\" resolved to the object name of\n> the tree recorded in the cache-tree, if we have one. If we are\n> living in the \"sparse-index\" world, and the paths underneath\n> '' are not in the index (because we are not interested in\n> them), we should be able answer \"git rev-parse :\" with the\n> name of the tree object. It is a \"new feature\" regardless of\n> sparse-index, but...\n\nI also considered \"what if we made this 'just work' for trees in the\nindex?\" The issue I found is that we might need to generate the cache-tree\nextension when it does not exist (or is not up-to-date).\n\nFor this series, I erred on matching existing behavior before\nadding such a feature. It is unfortunate that any work to show\ntrees this way would essentially revert patches 3 and 4 of the\ncurrent series.\n\n> ...I wonder if it is sensible to add more code to\n> engrave in stone that we would not support it and keep treating\n> the index as if it is a flat list of paths to blobs (and commits,\n> for submodules)?\n\nI don't consider any code change to be \"engraved in stone\", including\nthese tests that I am adding. Tests document expected behavior. If we\ndecide to change that expected behavior, then we can change the tests.\nBetter to have tests that change when we are making a choice to change\nbehavior than to not have that evidence of a behavior change at all.\n\nUnfortunately, we did not previously have tests for \"git show :dir\",\nso this is the first time we are documenting the behavior.\n\n> - When running 'git cmd :' where '' is *not* a\n> directory but is not in the index because of \"sparse-index\"\n> (i.e. a leading directory of '' is represented as a tree in\n> the index), should \":\" index expand the \"tree\" index entry\n> on-demand, so that has its own entry in the index, as if\n> no \"sparse-index\" is in effect?\n\nThere was one case of test_sparse_match that could have been a\ntest_all_match: \"git show :folder1/a\". Since folder1/a is discovered in\nthe index, Git doesn't check the worktree for the existence of the file to\nreport any different behavior. Changing the test works without any code\nchange, so I'll update that in v2.\n\n> The tests I saw in the series\n> were mostly asserted with test_sparse_match, not test_all_match,\n> so I couldn't tell what the expectations are.\n\nThe reason for test_sparse_match instead of test_all_match is because the\nerror messages change depending on the existence of the path in the\nworktree. For example \"git show :folder1/\" in the test script would have\nthis output difference:\n\n+ diff -u full-checkout-err sparse-checkout-err\n--- full-checkout-err 2022-04-12 13:35:51.430805689 +0000\n+++ sparse-checkout-err 2022-04-12 13:35:51.430805689 +0000\n@@ -1 +1 @@\n-fatal: path 'folder1/' exists on disk, but not in the index\n+fatal: path 'folder1/' does not exist (neither on disk nor in the index)\n\n> - More generally, if path is represented as a\n> \"sparse-dir\" entry, should \":/\" cause the index\n> entry for path to be expanded on-demand? I am guessing\n> that the answer is yes, as we wouldn't be able to even tell if\n> such a path / would exist in the index if the\n> index were converted to full upfront.\n\nThis is what happens, but it happens inside index_name_pos(), so there\nisn't any change necessary in the builtin to handle this. Tests such as\n\"git show :folder1/a\" demonstrate this behavior.\n\nNow, there could be a _faster_ way to do this if we did not depend\ndirectly on index_name_pos(). Using index_name_pos() will trigger the\nindex expansion, which is necessary for some compatibility with the sparse\nindex.\n\nIn this case, we don't really care what _position_ the cache entry is in,\nwe just want to know if the path exists in the index or not.\n\nFor that, we could extract a helper method out of the implementation of\nindex_name_pos() to be something like index_name_exists(). The common code\nbetween the two implementations is to find a position in the index that\neither corresponds to the exact path _or_ is a sparse directory (or the\npath definitely doesn't exist in the index). From that point,\nindex_name_exists() could do a tree walk to find the path inside the tree.\n\nAll of this is _possible_, but I also understand that running \"git show\n:\" is not common for users to run directly. Instead, I've seen this\nused by IDEs to find the staged version of a file. This is not likely to\nbe run for paths outside of the sparse-checkout cone.\n\nI think that this is not the only place that could benefit from an\nindex_name_exists() method, but I'm not ready to focus on improving\nperformance for queries outside of the sparse-checkout cone.\n\nThanks,\n-Stolee\n"},{"id":"453446","messageId":"xmqqlewacnlb.fsf@gitster.g","threadId":"57689","inReplyTo":"1f027d22-9c5c-7ccf-37e7-611d67de7cfd@github.com","subject":"Re: [PATCH 4/4] object-name: diagnose trees in index properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-12T15:45:20Z","receivedAt":"2022-04-12T15:45:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> The reason for test_sparse_match instead of test_all_match is because the\n> error messages change depending on the existence of the path in the\n> worktree. For example \"git show :folder1/\" in the test script would have\n> this output difference:\n>\n> + diff -u full-checkout-err sparse-checkout-err\n> --- full-checkout-err 2022-04-12 13:35:51.430805689 +0000\n> +++ sparse-checkout-err 2022-04-12 13:35:51.430805689 +0000\n> @@ -1 +1 @@\n> -fatal: path 'folder1/' exists on disk, but not in the index\n> +fatal: path 'folder1/' does not exist (neither on disk nor in the index)\n\nAh, because naturally folder1/ would not be on disk if it is outside\nthe sparse cones? Makes sense.\n\nThanks.\n"},{"id":"453665","messageId":"Ylhp5ubNco+oATOD@google.com","threadId":"57689","inReplyTo":"8c2fdb5a4fc3317c05324da54692036e36fc15f3.1649349442.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] t1092: add compatibility tests for 'git show'","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-04-14T18:37:26Z","receivedAt":"2022-04-14T18:37:39Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> Signed-off-by: Derrick Stolee \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 16 ++++++++++++++++\n> 1 file changed, 16 insertions(+)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 236ab530284..74792b5ebbc 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1151,6 +1151,22 @@ test_expect_success 'clean' '\n> \ttest_sparse_match test_path_is_dir folder1\n> '\n> \n> +test_expect_success 'show (cached blobs/trees)' '\n> +\tinit_repos &&\n> +\n> +\ttest_all_match git show :a &&\n> +\ttest_all_match git show :deep/a &&\n> +\ttest_sparse_match git show :folder1/a &&\n> +\n> +\t# Asking \"git show\" for directories in the index\n> +\t# does not work as implemented. The error message is\n> +\t# different for a full checkout and a sparse checkout\n> +\t# when the directory is outside of the cone.\n> +\ttest_all_match test_must_fail git show :deep/ &&\n> +\ttest_must_fail git -C full-checkout show :folder1/ &&\n> +\ttest_sparse_match test_must_fail git show :folder1/\n> +'\n\nA reminder that directories are not present in a non-sparse index would\nhelp those of us unfamiliar with the differences between\nsparse/non-sparse indexes to understand why the full-checkout cases fail\nhere. Initially I was confused why any of these lookups would fail\nbecause my mental model was \"a sparse-index is a proper subset of the\nnon-sparse index\".\n\n> +\n> test_expect_success 'submodule handling' '\n> \tinit_repos &&\n> \n> -- \n> gitgitgadget\n> \n"},{"id":"453666","messageId":"Ylhp+q96KOt2+OGZ@google.com","threadId":"57689","inReplyTo":"pull.1207.git.1649349442.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] Sparse index integration with 'git show'","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-04-14T18:37:46Z","receivedAt":"2022-04-14T18:37:55Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Hi Stolee,\n\nThanks for the series! All my comments are from the perspective of\nsomeone without much knowledge of the index, much less sparse-index, or\nsparse-checkout. I am not sure whether the project should cater to\nreviewers in my position, but I did have trouble understanding most of\nthis series. I didn't have any concerns with the implementation, but the\ncover letter, commit messages, and tests were fairly confusing and I\nthink can be explained better with a few changes:\n\nOn 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n> This continues our sequence of integrating builtins with the sparse index.\n> \n> 'git show' is relatively simple to get working in a way that doesn't fail\n> when it would previously succeed, but there are some sutbleties when the\n> user passes a directory path. If that path happens to be a sparse directory\n> entry, we suddenly start succeeding and printing the tree information!\n>\n> Since this behavior can change depending on the sparse checkout definition\n> and the state of index entries within that directory, this new behavior\n> would be more likely to confuse users than help them.\n\nThe two paragraphs above did not really convey to me on first\nread-through what the problem was. IIUC the main points are:\n\n* git-show does not currently work with the sparse index.\n* A simple change fixes the above, but causes us to sometimes print\n unexpected information about trees.\n* We can use the simple change in our implementation, but must do\n additional work to make sure we only print the expected information.\n\nI think this could be clarified by:\n* Including examples of the unhelpful output from the simple\n implementation.\n* Describing in more detail the situations that trigger this.\n* Describing why those situations don't currently happen without support\n for sparse indexes.\n\n> Here is an outline of this series:\n> \n> * Patch 1: Add more tests around 'git show :' in t1092.\n> \n> * Patch 2: Make 'git show' stop expanding the index by default. Make note\n> of this behavior change in the tests.\n> \n> * Patches 3-4: Make the subtle changes to object-name.c that help us reject\n> sparse directories (patch 3) and print the correct error message (patch\n> 4).\n> \n> Patches 2-4 could realistically be squashed into a single commit, but I\n> thought it might be instructive to show these individual steps, especially\n> as an example for our GSoC project.\n> \n> This series is based on the current 'master'. I know that Victoria intends\n> to submit her 'git stash' integration soon, and this provides a way to test\n> if our split of test changes in t1092 are easy to merge without conflict. If\n> that is successful, then I will likely submit my integration with the\n> 'sparse-checkout' builtin after this series is complete.\n> \n> Thanks, -Stolee\n\n\nThanks,\nJosh\n"},{"id":"453667","messageId":"Ylhs3t9nFS/IeO2Y@google.com","threadId":"57689","inReplyTo":"27ab853a9b4f0a50880c92dd8949d6fe9b7f00ba.1649349442.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] show: integrate with the sparse index","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-04-14T18:50:06Z","receivedAt":"2022-04-14T18:50:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The 'git show' command can take an input to request the state of an\n> object in the index. This can lead to parsing the index in order to load\n> a specific file entry. Without the change presented here, a sparse index\n> would expand to a full one, taking much longer than usual to access a\n> simple file.\n> \n> There is one behavioral change that happens here, though: we now can\n> find a sparse directory entry within the index! Commands that previously\n> failed because we could not find an entry in the worktree or index now\n> succeed because we _do_ find an entry in the index.\n\nAs with the test in the previous commit, a reminder that sparse-indexes\nare not necessarily subsets of a full index could be helpful here.\n\n\n> There might be more work to do to make other situations succeed when\n> looking for an indexed tree, perhaps by looking at or updating the\n> cache-tree extension as needed. These situations include having a full\n> index or asking for a directory that is within the sparse-checkout cone\n> (and hence is not a sparse directory entry in the index).\n> \n> For now, we demonstrate how the sparse index integration is extremely\n> simple for files outside of the cone as well as directories within the\n> cone. A later change will resolve this behavior around sparse\n> directories.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> builtin/log.c | 5 +++++\n> t/t1092-sparse-checkout-compatibility.sh | 23 +++++++++++++++++++----\n> 2 files changed, 24 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index c211d66d1d0..8e2e9912ab9 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -661,6 +661,11 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n> \tinit_log_defaults();\n> \tgit_config(git_log_config, NULL);\n> \n> +\tif (the_repository->gitdir) {\n> +\t\tprepare_repo_settings(the_repository);\n> +\t\tthe_repository->settings.command_requires_full_index = 0;\n> +\t}\n> +\n> \tmemset(&match_all, 0, sizeof(match_all));\n> \trepo_init_revisions(the_repository, &rev, prefix);\n> \tgit_config(grep_config, &rev.grep_filter);\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 74792b5ebbc..f6a14e08b81 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1159,12 +1159,20 @@ test_expect_success 'show (cached blobs/trees)' '\n> \ttest_sparse_match git show :folder1/a &&\n> \n> \t# Asking \"git show\" for directories in the index\n> -\t# does not work as implemented. The error message is\n> -\t# different for a full checkout and a sparse checkout\n> -\t# when the directory is outside of the cone.\n> +\t# changes depending on the existence of a sparse index.\n\nThe wording here seems awkward after these changes are applied. Without\nother context, it makes it sound to me like the command(s) used to show\na directory change depending on the existence of a sparse index, rather\nthan the fact that the behavior of `git show` changes.\n\n\n> \ttest_all_match test_must_fail git show :deep/ &&\n> \ttest_must_fail git -C full-checkout show :folder1/ &&\n> -\ttest_sparse_match test_must_fail git show :folder1/\n> +\ttest_must_fail git -C sparse-checkout show :folder1/ &&\n> +\n> +\tgit -C sparse-index show :folder1/ >actual &&\n> +\tgit -C full-checkout show HEAD:folder1 >expect &&\n> +\n> +\t# The output of \"git show\" includes the way we referenced the\n> +\t# objects, so strip that out.\n> +\ttest_line_count = 4 actual &&\n> +\ttail -n 2 actual >actual-trunc &&\n> +\ttail -n 2 expect >expect-trunc &&\n> +\ttest_cmp expect-trunc actual-trunc\n> '\n\nIt's not specific to this commit, but in general I think the series of\nchanges to this test would be easier to follow if we used hard-coded\nstrings to compare against, rather than matching parts of files against\neach other. It makes it more clear to the reader exactly which behavior\nis changing, and can make it more obvious why certain output is\nundesirable. However, it would make the test more brittle to future\nchanges.\n\n\n> test_expect_success 'submodule handling' '\n> @@ -1388,6 +1396,13 @@ test_expect_success 'sparse index is not expanded: diff' '\n> \tensure_not_expanded diff --cached\n> '\n> \n> +test_expect_success 'sparse index is not expanded: show' '\n> +\tinit_repos &&\n> +\n> +\tensure_not_expanded show :a &&\n> +\tensure_not_expanded show :deep/a\n> +'\n> +\n> test_expect_success 'sparse index is not expanded: update-index' '\n> \tinit_repos &&\n> \n> -- \n> gitgitgadget\n> \n"},{"id":"453668","messageId":"Ylhui1cBGDca4m19@google.com","threadId":"57689","inReplyTo":"f5da532767367844af7ecc477e1c8434a5454fd1.1649349442.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] object-name: reject trees found in the index","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2022-04-14T18:57:15Z","receivedAt":"2022-04-14T18:57:26Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The get_oid_with_context_1() method is used when parsing revision\n> arguments. One particular case is to take a \":\" string and search\n> the index for the given path.\n> \n> In the case of a sparse index, this might find a sparse directory entry,\n> in which case the contained object is a tree. In the case of a full\n> index, this search within the index would fail.\n\nAnother case where my naive understanding of sparse-indexes caused a lot\nof confusion for me. I don't know if we want to add reminders everywhere\nin this series, but there should probably be at least one commit message\nin the series that points out that sparse indexes are not subsets of\nfull indexes.\n\n\n> In order to maintain identical return state as in a full index, inspect\n> the discovered cache entry to see if it is a sparse directory and reject\n> it. This requires being careful around the only_to_die option to be sure\n> we die only at the correct time.\n> \n> This changes the behavior of 'git show :', but does not\n> bring it entirely into alignment with a full index case. It specifically\n> hits the wrong error message within diagnose_invalid_index_path(). That\n> error message will be corrected in a future change.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> object-name.c | 19 ++++++++++++++++++-\n> t/t1092-sparse-checkout-compatibility.sh | 11 ++---------\n> 2 files changed, 20 insertions(+), 10 deletions(-)\n> \n> diff --git a/object-name.c b/object-name.c\n> index f0e327f91f5..2dc5d2549b8 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -1881,6 +1881,20 @@ static char *resolve_relative_path(struct repository *r, const char *rel)\n> \t\t\t rel);\n> }\n> \n> +static int reject_tree_in_index(struct repository *repo,\n> +\t\t\t\tint only_to_die,\n> +\t\t\t\tconst struct cache_entry *ce,\n> +\t\t\t\tint stage,\n> +\t\t\t\tconst char *prefix,\n> +\t\t\t\tconst char *cp)\n> +{\n> +\tif (!S_ISSPARSEDIR(ce->ce_mode))\n> +\t\treturn 0;\n> +\tif (only_to_die)\n> +\t\tdiagnose_invalid_index_path(repo, stage, prefix, cp);\n> +\treturn -1;\n> +}\n> +\n> static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> \t\t\t\t const char *name,\n> \t\t\t\t unsigned flags,\n> @@ -1955,9 +1969,12 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> \t\t\t memcmp(ce->name, cp, namelen))\n> \t\t\t\tbreak;\n> \t\t\tif (ce_stage(ce) == stage) {\n> +\t\t\t\tfree(new_path);\n> +\t\t\t\tif (reject_tree_in_index(repo, only_to_die, ce,\n> +\t\t\t\t\t\t\t stage, prefix, cp))\n> +\t\t\t\t\treturn -1;\n> \t\t\t\toidcpy(oid, &ce->oid);\n> \t\t\t\toc->mode = ce->ce_mode;\n> -\t\t\t\tfree(new_path);\n> \t\t\t\treturn 0;\n> \t\t\t}\n> \t\t\tpos++;\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index f6a14e08b81..9d32361110d 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1164,15 +1164,8 @@ test_expect_success 'show (cached blobs/trees)' '\n> \ttest_must_fail git -C full-checkout show :folder1/ &&\n> \ttest_must_fail git -C sparse-checkout show :folder1/ &&\n> \n> -\tgit -C sparse-index show :folder1/ >actual &&\n> -\tgit -C full-checkout show HEAD:folder1 >expect &&\n> -\n> -\t# The output of \"git show\" includes the way we referenced the\n> -\t# objects, so strip that out.\n> -\ttest_line_count = 4 actual &&\n> -\ttail -n 2 actual >actual-trunc &&\n> -\ttail -n 2 expect >expect-trunc &&\n> -\ttest_cmp expect-trunc actual-trunc\n> +\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n> +\tgrep \"is in the index, but not at stage 0\" err\n> '\n\nIt might be worth a note that we're demonstrating the current behavior\nhere, but this is not the desired end-state. In other words, explicitly\nnote that this is the \"wrong error message\" referred to in the commit\nmessage.\n\n\n> \n> test_expect_success 'submodule handling' '\n> -- \n> gitgitgadget\n> \n"},{"id":"453672","messageId":"xmqqk0brz7tb.fsf@gitster.g","threadId":"57689","inReplyTo":"Ylhp+q96KOt2+OGZ@google.com","subject":"Re: [PATCH 0/4] Sparse index integration with 'git show'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-14T21:14:24Z","receivedAt":"2022-04-14T21:14:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n>> 'git show' is relatively simple to get working in a way that doesn't fail\n>> when it would previously succeed, but there are some sutbleties when the\n>> user passes a directory path. If that path happens to be a sparse directory\n>> entry, we suddenly start succeeding and printing the tree information!\n>>\n>> Since this behavior can change depending on the sparse checkout definition\n>> and the state of index entries within that directory, this new behavior\n>> would be more likely to confuse users than help them.\n>\n> The two paragraphs above did not really convey to me on first\n> read-through what the problem was. IIUC the main points are:\n>\n> * git-show does not currently work with the sparse index.\n> * A simple change fixes the above, but causes us to sometimes print\n> unexpected information about trees.\n> * We can use the simple change in our implementation, but must do\n> additional work to make sure we only print the expected information.\n>\n> I think this could be clarified by:\n> * Including examples of the unhelpful output from the simple\n> implementation.\n> * Describing in more detail the situations that trigger this.\n> * Describing why those situations don't currently happen without support\n> for sparse indexes.\n\nI think the issues patches 2-4 addresses are not limited to any\nspecific command, but how \":\" syntax is resolved\nto an object name (including the way how error messages are issued\nwhen the given path is not found in the index).\n\nBut the title of the series and presentation place undue stress on\n\"git show\", which I suspect may be confusing to some readers.\n\nFor example, with these patches, wouldn't \"git rev-parse\" start\nworking better, even though the proposed log message for no commit\nin the series talks about \"rev-parse\" and no tests are done with the\n\"rev-parse\" command?\n\nI am not suggesting that commands other than \"show\" should be also\ndiscussed in detail or tested. It would help readers if we said\nthat we are using 'show' merely as an example to demonstrate how\n\":\" syntax interacts with the sparse index\nentries (1) before the series, and (2) how well the improved\nobject-name parsing logic works after the series.\n\n\n"},{"id":"453806","messageId":"e3303456-a46c-10db-d6e8-61cd0ffe79d6@github.com","threadId":"57689","inReplyTo":"Ylhp5ubNco+oATOD@google.com","subject":"Re: [PATCH 1/4] t1092: add compatibility tests for 'git show'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-18T12:23:03Z","receivedAt":"2022-04-18T12:27:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2022 2:37 PM, Josh Steadmon wrote:\n> On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> t/t1092-sparse-checkout-compatibility.sh | 16 ++++++++++++++++\n>> 1 file changed, 16 insertions(+)\n>>\n>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n>> index 236ab530284..74792b5ebbc 100755\n>> --- a/t/t1092-sparse-checkout-compatibility.sh\n>> +++ b/t/t1092-sparse-checkout-compatibility.sh\n>> @@ -1151,6 +1151,22 @@ test_expect_success 'clean' '\n>> \ttest_sparse_match test_path_is_dir folder1\n>> '\n>> \n>> +test_expect_success 'show (cached blobs/trees)' '\n>> +\tinit_repos &&\n>> +\n>> +\ttest_all_match git show :a &&\n>> +\ttest_all_match git show :deep/a &&\n>> +\ttest_sparse_match git show :folder1/a &&\n>> +\n>> +\t# Asking \"git show\" for directories in the index\n>> +\t# does not work as implemented. The error message is\n>> +\t# different for a full checkout and a sparse checkout\n>> +\t# when the directory is outside of the cone.\n>> +\ttest_all_match test_must_fail git show :deep/ &&\n>> +\ttest_must_fail git -C full-checkout show :folder1/ &&\n>> +\ttest_sparse_match test_must_fail git show :folder1/\n>> +'\n> \n> A reminder that directories are not present in a non-sparse index would\n> help those of us unfamiliar with the differences between\n> sparse/non-sparse indexes to understand why the full-checkout cases fail\n> here. Initially I was confused why any of these lookups would fail\n> because my mental model was \"a sparse-index is a proper subset of the\n> non-sparse index\".\n\nAt this point, it would be repetitive to explain the sparse index\nevery time we do anything involving it. Reviewers should expect to\nbe familiar with the topic, or consult the in-tree documentation [1, 2].\n\n[1] https://github.com/git/git/blob/master/Documentation/technical/sparse-index.txt\n[2] https://github.com/git/git/blob/4027e30c5395c9c1aeea85e99f51ac62f5148145/Documentation/technical/index-format.txt#L396-L406\n\nSpecific to this change, sparse directories are not being taken into\naccount, since 'git show' is still in the compatibility mode that\nexpands a sparse index to a full one [3]. Thus, the differences in this\npatch are only related to full-checkout versus sparse-checkout.\n\n[3] https://github.com/git/git/blob/4027e30c5395c9c1aeea85e99f51ac62f5148145/Documentation/technical/sparse-index.txt#L69-L74\n\nThanks,\n-Stolee\n"},{"id":"453807","messageId":"4344a533-8450-1e05-3d4e-642eda330af2@github.com","threadId":"57689","inReplyTo":"Ylhui1cBGDca4m19@google.com","subject":"Re: [PATCH 3/4] object-name: reject trees found in the index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-18T12:31:17Z","receivedAt":"2022-04-18T12:37:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2022 2:57 PM, Josh Steadmon wrote:\n> On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n\n>> This changes the behavior of 'git show :', but does not\n>> bring it entirely into alignment with a full index case. It specifically\n>> hits the wrong error message within diagnose_invalid_index_path(). That\n>> error message will be corrected in a future change.\n\n...\n\n>> +\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n>> +\tgrep \"is in the index, but not at stage 0\" err\n>> '\n> \n> It might be worth a note that we're demonstrating the current behavior\n> here, but this is not the desired end-state. In other words, explicitly\n> note that this is the \"wrong error message\" referred to in the commit\n> message.\n\nThis is mentioned in the commit message. Since the line will be changed\nin the next patch, I didn't think a test comment was worth it.\n\nThanks,\n-Stolee\n"},{"id":"453808","messageId":"4c999e1f-e6bb-72e0-a27d-e072ce9487f8@github.com","threadId":"57689","inReplyTo":"Ylhs3t9nFS/IeO2Y@google.com","subject":"Re: [PATCH 2/4] show: integrate with the sparse index","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-18T12:28:41Z","receivedAt":"2022-04-18T12:37:43Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2022 2:50 PM, Josh Steadmon wrote:\n> On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n\n>> \t# Asking \"git show\" for directories in the index\n>> -\t# does not work as implemented. The error message is\n>> -\t# different for a full checkout and a sparse checkout\n>> -\t# when the directory is outside of the cone.\n>> +\t# changes depending on the existence of a sparse index.\n> \n> The wording here seems awkward after these changes are applied. Without\n> other context, it makes it sound to me like the command(s) used to show\n> a directory change depending on the existence of a sparse index, rather\n> than the fact that the behavior of `git show` changes.\n\nI can see that.\n\n>> +\t# The output of \"git show\" includes the way we referenced the\n>> +\t# objects, so strip that out.\n>> +\ttest_line_count = 4 actual &&\n>> +\ttail -n 2 actual >actual-trunc &&\n>> +\ttail -n 2 expect >expect-trunc &&\n>> +\ttest_cmp expect-trunc actual-trunc\n>> '\n> \n> It's not specific to this commit, but in general I think the series of\n> changes to this test would be easier to follow if we used hard-coded\n> strings to compare against, rather than matching parts of files against\n> each other. It makes it more clear to the reader exactly which behavior\n> is changing, and can make it more obvious why certain output is\n> undesirable. However, it would make the test more brittle to future\n> changes.\n\nThe tests here are designed with this approach in mind: demonstrate\nsuccess by comparing to existing behavior. We don't want to be\ncoupled to the exact behavior of these commands, but we _do_ want to\ndemonstrate that the sparse-checkout or sparse-index features do not\nchange from the full-checkout behavior (unless we are demonstrating an\nexpected difference).\n\nIn particular, using comparisons like this are also robust against\nchanges in the test repository data shape, which has been necessary to\nupdate as bugs are found.\n\nThanks,\n-Stolee\n"},{"id":"453809","messageId":"61829024-32ab-bbdb-7950-8929b17d6add@github.com","threadId":"57689","inReplyTo":"xmqqk0brz7tb.fsf@gitster.g","subject":"Re: [PATCH 0/4] Sparse index integration with 'git show'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-18T12:42:56Z","receivedAt":"2022-04-18T13:02:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/14/2022 5:14 PM, Junio C Hamano wrote:\n> Josh Steadmon writes:\n> \n>>> 'git show' is relatively simple to get working in a way that doesn't fail\n>>> when it would previously succeed, but there are some sutbleties when the\n>>> user passes a directory path. If that path happens to be a sparse directory\n>>> entry, we suddenly start succeeding and printing the tree information!\n>>>\n>>> Since this behavior can change depending on the sparse checkout definition\n>>> and the state of index entries within that directory, this new behavior\n>>> would be more likely to confuse users than help them.\n>>\n>> The two paragraphs above did not really convey to me on first\n>> read-through what the problem was. IIUC the main points are:\n>>\n>> * git-show does not currently work with the sparse index.\n>> * A simple change fixes the above, but causes us to sometimes print\n>> unexpected information about trees.\n>> * We can use the simple change in our implementation, but must do\n>> additional work to make sure we only print the expected information.\n>>\n>> I think this could be clarified by:\n>> * Including examples of the unhelpful output from the simple\n>> implementation.\n>> * Describing in more detail the situations that trigger this.\n>> * Describing why those situations don't currently happen without support\n>> for sparse indexes.\n> \n> I think the issues patches 2-4 addresses are not limited to any\n> specific command, but how \":\" syntax is resolved\n> to an object name (including the way how error messages are issued\n> when the given path is not found in the index).\n\nYes, this is the critical bit. It's the only part of \"git show\"\nthat cares about the index.\n \n> But the title of the series and presentation place undue stress on\n> \"git show\", which I suspect may be confusing to some readers.\n> \n> For example, with these patches, wouldn't \"git rev-parse\" start\n> working better, even though the proposed log message for no commit\n> in the series talks about \"rev-parse\" and no tests are done with the\n> \"rev-parse\" command?\n\nProbably, assuming we unset the command_requires_full_index\nprotection on 'git rev-parse'. This might be a good case for\nShaoxuan to try.\n\n> I am not suggesting that commands other than \"show\" should be also\n> discussed in detail or tested. It would help readers if we said\n> that we are using 'show' merely as an example to demonstrate how\n> \":\" syntax interacts with the sparse index\n> entries (1) before the series, and (2) how well the improved\n> object-name parsing logic works after the series.\n \nI can see that.\nSome background: as we shipped our sparse index integrations in\nthe microsoft/git fork and measured performance of real users, we\nnoticed that 'git show' was a frequently-used command that went\nfrom ~0.5 seconds to ~4 seconds in monorepo situations. This was\nunexpected, because we didn't know about the \":\" syntax.\nFurther, it seemed that some third-party tools were triggering\nthis behavior as a frequent way to check on staged content. That\nmotivated quickly shipping this integration. Performance improved\nto ~0.1 seconds because of the reduced index size.\n\nThanks,\n-Stolee\n"},{"id":"454467","messageId":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.git.1649349442.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Sparse index integration with 'git show'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T20:43:15Z","receivedAt":"2022-04-26T20:43:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This continues our sequence of integrating builtins with the sparse index.\n\n'git show' is relatively simple to get working in a way that doesn't fail\nwhen it would previously succeed, but there are some subtleties when the\nuser passes a directory path using the \":\" syntax to get the path out of the\nindex. If that path happens to be a sparse directory entry, we suddenly\nstart succeeding and printing the tree information!\n\nSince this behavior can change depending on the sparse checkout definition\nand the state of index entries within that directory, this new behavior\nwould be more likely to confuse users than help them.\n\nThis \":\" syntax is shared by other commands like \"git rev-parse\", but we are\nnot adding those integrations at this point.\n\nSome background: as we shipped our sparse index integrations in the\nmicrosoft/git fork and measured performance of real users, we noticed that\n'git show' was a frequently-used command that went from ~0.5 seconds to ~4\nseconds in monorepo situations. This was unexpected, because we didn't know\nabout the \":\" syntax. Further, it seemed that some third-party tools were\ntriggering this behavior as a frequent way to check on staged content. That\nmotivated quickly shipping this integration. Performance improved to ~0.1\nseconds because of the reduced index size. While inspecting our rebase of\nmicrosoft/git commits onto the 2.36.0 release candidates, I noticed this\nintegration would be a simpler example to demonstrate how sparse index\nintegrations should go when the behavior is not too complicated.\n\nHere is an outline of this series:\n\n * Patch 1: Add more tests around 'git show :' in t1092. These tests are\n only to establish the existing differences between the full and\n sparse-checkout cases, since 'git show' is still protected by\n 'command_requires_full_index'.\n\n * Patch 2: Make 'git show' stop expanding the index by default. Make note\n of this behavior change in the tests.\n\n * Patches 3-4: Make the subtle changes to object-name.c that help us reject\n sparse directories (patch 3) and print the correct error message (patch\n 4).\n\n * Patch 5: Now that the common index-parsing code is updated, do the\n minimum change to 'git rev-parse' to avoid expanding the index to parse\n index entries for a sparse index.\n\nPatches 2-4 could realistically be squashed into a single commit, but I\nthought it might be instructive to show these individual steps, especially\nas an example for our GSoC project.\n\nI know that Victoria intends to submit her 'git stash' integration soon, and\nthis provides a way to test if our split of test changes in t1092 are easy\nto merge without conflict. If that is successful, then I will likely submit\nmy integration with the 'sparse-checkout' builtin after this series is\ncomplete. (UPDATE: we inserted a test in the same location of t1092, but\notherwise there are no textual or semantic conflicts.)\n\n\nUpdates in v2\n=============\n\n * The test comment in patch 2 is updated.\n * A commit message typo in patch 4 is fixed.\n * Patch 4 simplified the behavior, but the previous version didn't clean up\n a test comment about that. It now cleans up the test to be simpler.\n * Patch 5 includes an integration with 'git rev-parse'.\n * The cover letter is expanded with more context.\n * The only conflict with Victoria's new 'git stash' patch series is that we\n both added a test in the same position of t1092. Including both new tests\n is the right way to resolve the conflict. Order does not matter.\n\n[1]\nhttps://lore.kernel.org/git/pull.1171.git.1650908957.gitgitgadget@gmail.com/\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n t1092: add compatibility tests for 'git show'\n show: integrate with the sparse index\n object-name: reject trees found in the index\n object-name: diagnose trees in index properly\n rev-parse: integrate with sparse index\n\n builtin/log.c | 5 ++++\n builtin/rev-parse.c | 3 ++\n object-name.c | 25 ++++++++++++++--\n t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++\n 4 files changed, 66 insertions(+), 3 deletions(-)\n\n\nbase-commit: 07330a41d66a2c9589b585a3a24ecdcf19994f19\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1207%2Fderrickstolee%2Fsparse-index%2Fgit-show-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1207/derrickstolee/sparse-index/git-show-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1207\n\nRange-diff vs v1:\n\n 1: 8c2fdb5a4fc = 1: 8c2fdb5a4fc t1092: add compatibility tests for 'git show'\n 2: 27ab853a9b4 ! 2: 2e9d47ab09b show: integrate with the sparse index\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'show (cached blob\n -\t# does not work as implemented. The error message is\n -\t# different for a full checkout and a sparse checkout\n -\t# when the directory is outside of the cone.\n -+\t# changes depending on the existence of a sparse index.\n ++\t# had different behavior depending on the existence\n ++\t# of a sparse index.\n \ttest_all_match test_must_fail git show :deep/ &&\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n -\ttest_sparse_match test_must_fail git show :folder1/\n 3: f5da5327673 = 3: 5a7561637f0 object-name: reject trees found in the index\n 4: 99c09ccc240 ! 4: b730457fccc object-name: diagnose trees in index properly\n @@ Commit message\n checkout. The error message from diagnose_invalid_index_path() reports\n whether the path is on disk or not. The full checkout will have the\n directory on disk, but the path will not be in the index. The sparse\n - checokut could have the directory not exist, specifically when that\n + checkout could have the directory not exist, specifically when that\n directory is outside of the sparse-checkout cone.\n \n In the case of a sparse index, we have yet another state: the path can\n @@ object-name.c: static void diagnose_invalid_index_path(struct repository *r,\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'show (cached blobs/trees)' '\n + \ttest_all_match git show :deep/a &&\n + \ttest_sparse_match git show :folder1/a &&\n + \n +-\t# Asking \"git show\" for directories in the index\n +-\t# had different behavior depending on the existence\n +-\t# of a sparse index.\n ++\t# The error message differs depending on whether\n ++\t# the directory exists in the worktree.\n + \ttest_all_match test_must_fail git show :deep/ &&\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n - \ttest_must_fail git -C sparse-checkout show :folder1/ &&\n +-\ttest_must_fail git -C sparse-checkout show :folder1/ &&\n ++\ttest_sparse_match test_must_fail git show :folder1/ &&\n \n -\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n -\tgrep \"is in the index, but not at stage 0\" err\n -+\ttest_sparse_match test_must_fail git show :folder1/ &&\n -+\n +\t# Change the sparse cone for an extra case:\n +\trun_on_sparse git sparse-checkout set deep/deeper1 &&\n +\n -: ----------- > 5: 69efe637a18 rev-parse: integrate with sparse index\n\n-- \ngitgitgadget\n"},{"id":"454468","messageId":"8c2fdb5a4fc3317c05324da54692036e36fc15f3.1651005800.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] t1092: add compatibility tests for 'git show'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T20:43:16Z","receivedAt":"2022-04-26T20:43:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t1092-sparse-checkout-compatibility.sh | 16 ++++++++++++++++\n 1 file changed, 16 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 236ab530284..74792b5ebbc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1151,6 +1151,22 @@ test_expect_success 'clean' '\n \ttest_sparse_match test_path_is_dir folder1\n '\n \n+test_expect_success 'show (cached blobs/trees)' '\n+\tinit_repos &&\n+\n+\ttest_all_match git show :a &&\n+\ttest_all_match git show :deep/a &&\n+\ttest_sparse_match git show :folder1/a &&\n+\n+\t# Asking \"git show\" for directories in the index\n+\t# does not work as implemented. The error message is\n+\t# different for a full checkout and a sparse checkout\n+\t# when the directory is outside of the cone.\n+\ttest_all_match test_must_fail git show :deep/ &&\n+\ttest_must_fail git -C full-checkout show :folder1/ &&\n+\ttest_sparse_match test_must_fail git show :folder1/\n+'\n+\n test_expect_success 'submodule handling' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"454469","messageId":"2e9d47ab09ba8bca0166ca21bd936f2b4b5bb75e.1651005800.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] show: integrate with the sparse index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T20:43:17Z","receivedAt":"2022-04-26T20:43:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git show' command can take an input to request the state of an\nobject in the index. This can lead to parsing the index in order to load\na specific file entry. Without the change presented here, a sparse index\nwould expand to a full one, taking much longer than usual to access a\nsimple file.\n\nThere is one behavioral change that happens here, though: we now can\nfind a sparse directory entry within the index! Commands that previously\nfailed because we could not find an entry in the worktree or index now\nsucceed because we _do_ find an entry in the index.\n\nThere might be more work to do to make other situations succeed when\nlooking for an indexed tree, perhaps by looking at or updating the\ncache-tree extension as needed. These situations include having a full\nindex or asking for a directory that is within the sparse-checkout cone\n(and hence is not a sparse directory entry in the index).\n\nFor now, we demonstrate how the sparse index integration is extremely\nsimple for files outside of the cone as well as directories within the\ncone. A later change will resolve this behavior around sparse\ndirectories.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/log.c | 5 +++++\n t/t1092-sparse-checkout-compatibility.sh | 24 ++++++++++++++++++++----\n 2 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c211d66d1d0..8e2e9912ab9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -661,6 +661,11 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \tinit_log_defaults();\n \tgit_config(git_log_config, NULL);\n \n+\tif (the_repository->gitdir) {\n+\t\tprepare_repo_settings(the_repository);\n+\t\tthe_repository->settings.command_requires_full_index = 0;\n+\t}\n+\n \tmemset(&match_all, 0, sizeof(match_all));\n \trepo_init_revisions(the_repository, &rev, prefix);\n \tgit_config(grep_config, &rev.grep_filter);\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 74792b5ebbc..3506c0216f0 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1159,12 +1159,21 @@ test_expect_success 'show (cached blobs/trees)' '\n \ttest_sparse_match git show :folder1/a &&\n \n \t# Asking \"git show\" for directories in the index\n-\t# does not work as implemented. The error message is\n-\t# different for a full checkout and a sparse checkout\n-\t# when the directory is outside of the cone.\n+\t# had different behavior depending on the existence\n+\t# of a sparse index.\n \ttest_all_match test_must_fail git show :deep/ &&\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n-\ttest_sparse_match test_must_fail git show :folder1/\n+\ttest_must_fail git -C sparse-checkout show :folder1/ &&\n+\n+\tgit -C sparse-index show :folder1/ >actual &&\n+\tgit -C full-checkout show HEAD:folder1 >expect &&\n+\n+\t# The output of \"git show\" includes the way we referenced the\n+\t# objects, so strip that out.\n+\ttest_line_count = 4 actual &&\n+\ttail -n 2 actual >actual-trunc &&\n+\ttail -n 2 expect >expect-trunc &&\n+\ttest_cmp expect-trunc actual-trunc\n '\n \n test_expect_success 'submodule handling' '\n@@ -1388,6 +1397,13 @@ test_expect_success 'sparse index is not expanded: diff' '\n \tensure_not_expanded diff --cached\n '\n \n+test_expect_success 'sparse index is not expanded: show' '\n+\tinit_repos &&\n+\n+\tensure_not_expanded show :a &&\n+\tensure_not_expanded show :deep/a\n+'\n+\n test_expect_success 'sparse index is not expanded: update-index' '\n \tinit_repos &&\n \n-- \ngitgitgadget\n\n"},{"id":"454470","messageId":"5a7561637f0d845df23b47c9bed2ba74bbc5f07c.1651005800.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] object-name: reject trees found in the index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T20:43:18Z","receivedAt":"2022-04-26T20:43:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe get_oid_with_context_1() method is used when parsing revision\narguments. One particular case is to take a \":\" string and search\nthe index for the given path.\n\nIn the case of a sparse index, this might find a sparse directory entry,\nin which case the contained object is a tree. In the case of a full\nindex, this search within the index would fail.\n\nIn order to maintain identical return state as in a full index, inspect\nthe discovered cache entry to see if it is a sparse directory and reject\nit. This requires being careful around the only_to_die option to be sure\nwe die only at the correct time.\n\nThis changes the behavior of 'git show :', but does not\nbring it entirely into alignment with a full index case. It specifically\nhits the wrong error message within diagnose_invalid_index_path(). That\nerror message will be corrected in a future change.\n\nSigned-off-by: Derrick Stolee \n---\n object-name.c | 19 ++++++++++++++++++-\n t/t1092-sparse-checkout-compatibility.sh | 11 ++---------\n 2 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex f0e327f91f5..2dc5d2549b8 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1881,6 +1881,20 @@ static char *resolve_relative_path(struct repository *r, const char *rel)\n \t\t\t rel);\n }\n \n+static int reject_tree_in_index(struct repository *repo,\n+\t\t\t\tint only_to_die,\n+\t\t\t\tconst struct cache_entry *ce,\n+\t\t\t\tint stage,\n+\t\t\t\tconst char *prefix,\n+\t\t\t\tconst char *cp)\n+{\n+\tif (!S_ISSPARSEDIR(ce->ce_mode))\n+\t\treturn 0;\n+\tif (only_to_die)\n+\t\tdiagnose_invalid_index_path(repo, stage, prefix, cp);\n+\treturn -1;\n+}\n+\n static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\t\t const char *name,\n \t\t\t\t unsigned flags,\n@@ -1955,9 +1969,12 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\t memcmp(ce->name, cp, namelen))\n \t\t\t\tbreak;\n \t\t\tif (ce_stage(ce) == stage) {\n+\t\t\t\tfree(new_path);\n+\t\t\t\tif (reject_tree_in_index(repo, only_to_die, ce,\n+\t\t\t\t\t\t\t stage, prefix, cp))\n+\t\t\t\t\treturn -1;\n \t\t\t\toidcpy(oid, &ce->oid);\n \t\t\t\toc->mode = ce->ce_mode;\n-\t\t\t\tfree(new_path);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tpos++;\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 3506c0216f0..08c9cfd359e 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1165,15 +1165,8 @@ test_expect_success 'show (cached blobs/trees)' '\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n \ttest_must_fail git -C sparse-checkout show :folder1/ &&\n \n-\tgit -C sparse-index show :folder1/ >actual &&\n-\tgit -C full-checkout show HEAD:folder1 >expect &&\n-\n-\t# The output of \"git show\" includes the way we referenced the\n-\t# objects, so strip that out.\n-\ttest_line_count = 4 actual &&\n-\ttail -n 2 actual >actual-trunc &&\n-\ttail -n 2 expect >expect-trunc &&\n-\ttest_cmp expect-trunc actual-trunc\n+\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n+\tgrep \"is in the index, but not at stage 0\" err\n '\n \n test_expect_success 'submodule handling' '\n-- \ngitgitgadget\n\n"},{"id":"454471","messageId":"b730457fccc8b4d98beca70c82400446de4b69c4.1651005800.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] object-name: diagnose trees in index properly","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T20:43:19Z","receivedAt":"2022-04-26T20:43:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen running 'git show :' where '' is a directory, then\nthere is a subtle difference between a full checkout and a sparse\ncheckout. The error message from diagnose_invalid_index_path() reports\nwhether the path is on disk or not. The full checkout will have the\ndirectory on disk, but the path will not be in the index. The sparse\ncheckout could have the directory not exist, specifically when that\ndirectory is outside of the sparse-checkout cone.\n\nIn the case of a sparse index, we have yet another state: the path can\nbe a sparse directory in the index. In this case, the error message from\ndiagnose_invalid_index_path() would erroneously say \"path '' is in\nthe index, but not at stage 0\", which is false.\n\nAdd special casing around sparse directory entries so we get to the\ncorrect error message. This requires two checks in order to get parity\nwith the normal sparse-checkout case.\n\nSigned-off-by: Derrick Stolee \n---\n object-name.c | 6 ++++--\n t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++++++------\n 2 files changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 2dc5d2549b8..4d2746574cd 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1832,7 +1832,8 @@ static void diagnose_invalid_index_path(struct repository *r,\n \t\tpos = -pos - 1;\n \tif (pos < istate->cache_nr) {\n \t\tce = istate->cache[pos];\n-\t\tif (ce_namelen(ce) == namelen &&\n+\t\tif (!S_ISSPARSEDIR(ce->ce_mode) &&\n+\t\t ce_namelen(ce) == namelen &&\n \t\t !memcmp(ce->name, filename, namelen))\n \t\t\tdie(_(\"path '%s' is in the index, but not at stage %d\\n\"\n \t\t\t \"hint: Did you mean ':%d:%s'?\"),\n@@ -1848,7 +1849,8 @@ static void diagnose_invalid_index_path(struct repository *r,\n \t\tpos = -pos - 1;\n \tif (pos < istate->cache_nr) {\n \t\tce = istate->cache[pos];\n-\t\tif (ce_namelen(ce) == fullname.len &&\n+\t\tif (!S_ISSPARSEDIR(ce->ce_mode) &&\n+\t\t ce_namelen(ce) == fullname.len &&\n \t\t !memcmp(ce->name, fullname.buf, fullname.len))\n \t\t\tdie(_(\"path '%s' is in the index, but not '%s'\\n\"\n \t\t\t \"hint: Did you mean ':%d:%s' aka ':%d:./%s'?\"),\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 08c9cfd359e..fa1d5603605 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1158,15 +1158,21 @@ test_expect_success 'show (cached blobs/trees)' '\n \ttest_all_match git show :deep/a &&\n \ttest_sparse_match git show :folder1/a &&\n \n-\t# Asking \"git show\" for directories in the index\n-\t# had different behavior depending on the existence\n-\t# of a sparse index.\n+\t# The error message differs depending on whether\n+\t# the directory exists in the worktree.\n \ttest_all_match test_must_fail git show :deep/ &&\n \ttest_must_fail git -C full-checkout show :folder1/ &&\n-\ttest_must_fail git -C sparse-checkout show :folder1/ &&\n+\ttest_sparse_match test_must_fail git show :folder1/ &&\n \n-\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n-\tgrep \"is in the index, but not at stage 0\" err\n+\t# Change the sparse cone for an extra case:\n+\trun_on_sparse git sparse-checkout set deep/deeper1 &&\n+\n+\t# deep/deeper2 is a sparse directory in the sparse index.\n+\ttest_sparse_match test_must_fail git show :deep/deeper2/ &&\n+\n+\t# deep/deeper2/deepest is not in the sparse index, but\n+\t# will trigger an index expansion.\n+\ttest_sparse_match test_must_fail git show :deep/deeper2/deepest/\n '\n \n test_expect_success 'submodule handling' '\n-- \ngitgitgadget\n\n"},{"id":"454472","messageId":"69efe637a18786b289db79971e9e49137306b57c.1651005800.git.gitgitgadget@gmail.com","threadId":"57689","inReplyTo":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] rev-parse: integrate with sparse index","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-04-26T20:43:20Z","receivedAt":"2022-04-26T20:43:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIt is not obvious that the 'git rev-parse' builtin would use the sparse\nindex, but it is possible to parse paths out of the index using the\n\":\" syntax. The 'git rev-parse' output is only the OID of the\nobject found at that location, but otherwise behaves similarly to 'git\nshow :'. This includes the failure conditions on directories and\nthe error messages depending on whether a path is in the worktree or\nnot.\n\nThe only code change required is to change the\ncommand_requires_full_index setting in builtin/rev-parse.c, and we can\nre-use many existing 'git show' tests for the rev-parse case.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/rev-parse.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 45 +++++++++++++-----------\n 2 files changed, 28 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 8480a59f573..4fc6185b2d1 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -723,6 +723,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tprefix = setup_git_directory();\n \t\t\tgit_config(git_default_config, NULL);\n \t\t\tdid_repo_setup = 1;\n+\n+\t\t\tprepare_repo_settings(the_repository);\n+\t\t\tthe_repository->settings.command_requires_full_index = 0;\n \t\t}\n \n \t\tif (!strcmp(arg, \"--\")) {\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex fa1d5603605..93bcfd20bbc 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1151,29 +1151,32 @@ test_expect_success 'clean' '\n \ttest_sparse_match test_path_is_dir folder1\n '\n \n-test_expect_success 'show (cached blobs/trees)' '\n-\tinit_repos &&\n+for builtin in show rev-parse\n+do\n+\ttest_expect_success \"$builtin (cached blobs/trees)\" \"\n+\t\tinit_repos &&\n \n-\ttest_all_match git show :a &&\n-\ttest_all_match git show :deep/a &&\n-\ttest_sparse_match git show :folder1/a &&\n+\t\ttest_all_match git $builtin :a &&\n+\t\ttest_all_match git $builtin :deep/a &&\n+\t\ttest_sparse_match git $builtin :folder1/a &&\n \n-\t# The error message differs depending on whether\n-\t# the directory exists in the worktree.\n-\ttest_all_match test_must_fail git show :deep/ &&\n-\ttest_must_fail git -C full-checkout show :folder1/ &&\n-\ttest_sparse_match test_must_fail git show :folder1/ &&\n+\t\t# The error message differs depending on whether\n+\t\t# the directory exists in the worktree.\n+\t\ttest_all_match test_must_fail git $builtin :deep/ &&\n+\t\ttest_must_fail git -C full-checkout $builtin :folder1/ &&\n+\t\ttest_sparse_match test_must_fail git $builtin :folder1/ &&\n \n-\t# Change the sparse cone for an extra case:\n-\trun_on_sparse git sparse-checkout set deep/deeper1 &&\n+\t\t# Change the sparse cone for an extra case:\n+\t\trun_on_sparse git sparse-checkout set deep/deeper1 &&\n \n-\t# deep/deeper2 is a sparse directory in the sparse index.\n-\ttest_sparse_match test_must_fail git show :deep/deeper2/ &&\n+\t\t# deep/deeper2 is a sparse directory in the sparse index.\n+\t\ttest_sparse_match test_must_fail git $builtin :deep/deeper2/ &&\n \n-\t# deep/deeper2/deepest is not in the sparse index, but\n-\t# will trigger an index expansion.\n-\ttest_sparse_match test_must_fail git show :deep/deeper2/deepest/\n-'\n+\t\t# deep/deeper2/deepest is not in the sparse index, but\n+\t\t# will trigger an index expansion.\n+\t\ttest_sparse_match test_must_fail git $builtin :deep/deeper2/deepest/\n+\t\"\n+done\n \n test_expect_success 'submodule handling' '\n \tinit_repos &&\n@@ -1396,11 +1399,13 @@ test_expect_success 'sparse index is not expanded: diff' '\n \tensure_not_expanded diff --cached\n '\n \n-test_expect_success 'sparse index is not expanded: show' '\n+test_expect_success 'sparse index is not expanded: show and rev-parse' '\n \tinit_repos &&\n \n \tensure_not_expanded show :a &&\n-\tensure_not_expanded show :deep/a\n+\tensure_not_expanded show :deep/a &&\n+\tensure_not_expanded rev-parse :a &&\n+\tensure_not_expanded rev-parse :deep/a\n '\n \n test_expect_success 'sparse index is not expanded: update-index' '\n-- \ngitgitgadget\n"},{"id":"454474","messageId":"xmqqmtg7pntg.fsf@gitster.g","threadId":"57689","inReplyTo":"pull.1207.v2.git.1651005800.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] Sparse index integration with 'git show'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-26T20:55:07Z","receivedAt":"2022-04-26T20:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> This \":\" syntax is shared by other commands like \"git rev-parse\", but we are\n> not adding those integrations at this point.\n\nThis has been the most curious thing since the initial round. The\nchanges in the series are mostly about the code that parses the\n\":\" syntax and yield an object name (when exists) or give an\nerror messages (otherwise) and die, before the computed object name\ngets used by \"git show\", or any other command that takes an object\nname from the command line.\n\nI guess what has been confusing me was the phrase \"integration\",\nthat you seem to be using to refer only to the final step of setting\nrequire_full_index to 0, while that is the least interesting part of\na series like this one. The work done in patches 3/ and 4/ that\npaves the way to allow us to set the require_full_index to 0 in any\ncommand that needs to work with the \":\" syntax is much more\ninteresting part of the series, and when viewed from that angle, the\nseries is not about preparing \"show\" but about \":\" syntax to\nwork better with the sparse index.\n\n> I know that Victoria intends to submit her 'git stash' integration soon, and\n> this provides a way to test if our split of test changes in t1092 are easy\n> to merge without conflict. If that is successful, then I will likely submit\n> my integration with the 'sparse-checkout' builtin after this series is\n> complete. (UPDATE: we inserted a test in the same location of t1092, but\n> otherwise there are no textual or semantic conflicts.)\n\nYeah, I think I already queued Victoria's and the previous round of\nthis series in 'seen' without problematic conflicts.\n\nLet's take a brief look at the range-diff before I go do something\nelse...\n\n> 2: 27ab853a9b4 ! 2: 2e9d47ab09b show: integrate with the sparse index\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'show (cached blob\n> -\t# does not work as implemented. The error message is\n> -\t# different for a full checkout and a sparse checkout\n> -\t# when the directory is outside of the cone.\n> -+\t# changes depending on the existence of a sparse index.\n> ++\t# had different behavior depending on the existence\n> ++\t# of a sparse index.\n> \ttest_all_match test_must_fail git show :deep/ &&\n> \ttest_must_fail git -C full-checkout show :folder1/ &&\n> -\ttest_sparse_match test_must_fail git show :folder1/\n> 3: f5da5327673 = 3: 5a7561637f0 object-name: reject trees found in the index\n> 4: 99c09ccc240 ! 4: b730457fccc object-name: diagnose trees in index properly\n> @@ Commit message\n> checkout. The error message from diagnose_invalid_index_path() reports\n> whether the path is on disk or not. The full checkout will have the\n> directory on disk, but the path will not be in the index. The sparse\n> - checokut could have the directory not exist, specifically when that\n> + checkout could have the directory not exist, specifically when that\n> directory is outside of the sparse-checkout cone.\n> \n> In the case of a sparse index, we have yet another state: the path can\n> @@ object-name.c: static void diagnose_invalid_index_path(struct repository *r,\n> \n> ## t/t1092-sparse-checkout-compatibility.sh ##\n> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'show (cached blobs/trees)' '\n> + \ttest_all_match git show :deep/a &&\n> + \ttest_sparse_match git show :folder1/a &&\n> + \n> +-\t# Asking \"git show\" for directories in the index\n> +-\t# had different behavior depending on the existence\n> +-\t# of a sparse index.\n> ++\t# The error message differs depending on whether\n> ++\t# the directory exists in the worktree.\n> + \ttest_all_match test_must_fail git show :deep/ &&\n> \ttest_must_fail git -C full-checkout show :folder1/ &&\n> - \ttest_must_fail git -C sparse-checkout show :folder1/ &&\n> +-\ttest_must_fail git -C sparse-checkout show :folder1/ &&\n> ++\ttest_sparse_match test_must_fail git show :folder1/ &&\n> \n> -\ttest_must_fail git -C sparse-index show :folder1/ 2>err &&\n> -\tgrep \"is in the index, but not at stage 0\" err\n> -+\ttest_sparse_match test_must_fail git show :folder1/ &&\n> -+\n> +\t# Change the sparse cone for an extra case:\n> +\trun_on_sparse git sparse-checkout set deep/deeper1 &&\n> +\n> -: ----------- > 5: 69efe637a18 rev-parse: integrate with sparse index\n\nOK, nothing unexpected other than the rev-parse stuff.\n\nThanks.\n"},{"id":"454495","messageId":"bb28d9d3-8152-b6cf-6717-df8534e2871b@github.com","threadId":"57689","inReplyTo":"xmqqmtg7pntg.fsf@gitster.g","subject":"Re: [PATCH v2 0/5] Sparse index integration with 'git show'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-04-27T13:47:47Z","receivedAt":"2022-04-27T13:47:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/26/2022 4:55 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> This \":\" syntax is shared by other commands like \"git rev-parse\", but we are\n>> not adding those integrations at this point.\n> \n> This has been the most curious thing since the initial round. The\n> changes in the series are mostly about the code that parses the\n> \":\" syntax and yield an object name (when exists) or give an\n> error messages (otherwise) and die, before the computed object name\n> gets used by \"git show\", or any other command that takes an object\n> name from the command line.\n> \n> I guess what has been confusing me was the phrase \"integration\",\n> that you seem to be using to refer only to the final step of setting\n> require_full_index to 0, while that is the least interesting part of\n> a series like this one. The work done in patches 3/ and 4/ that\n> paves the way to allow us to set the require_full_index to 0 in any\n> command that needs to work with the \":\" syntax is much more\n> interesting part of the series, and when viewed from that angle, the\n> series is not about preparing \"show\" but about \":\" syntax to\n> work better with the sparse index.\n\nIn general, yes, we do need to be teaching different parts of the codebase\nabout the sparse index. The way I've been tracking that progress is by\nwhich builtins can stop expanding a sparse index to a full one upon parse.\nThis progress indicator also matches the testing strategy, which focuses\non preserving behavior for top-level Git commands (and checking that they\ndon't expand the sparse index when they don't need to).\n\nI understand your thought that it would be better to sell the series to\nreviewers by the interesting pieces under the hood that are changing. I\nthink this is one of the first times where only one of these systems is\nsufficient to make an entire builtin (or two) work with the sparse index.\n\nI'll keep this in mind for pitching future updates.\n\nThanks,\n-Stolee\n"}]}