{"thread":{"id":"59938","subject":"[PATCH v1 0/3] check-attr: integrate with sparse-index","startedAt":"2023-07-01T06:55:48Z","lastAt":"2023-08-15T08:07:00Z","messageCount":39,"participants":["Shuqi Liang","Victoria Dye","Junio C Hamano","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"479078","messageId":"20230701064843.147496-1-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":null,"subject":"[PATCH v1 0/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-01T06:48:40Z","receivedAt":"2023-07-01T06:55:48Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Turn on sparse-index feature within `git check-attr` command.\nAdd necessary modifications and test them.\n\nShuqi Liang (3):\n attr.c: read attributes in a sparse directory\n t1092: add tests for `git check-attr`\n check-attr: integrate with sparse-index\n\n attr.c | 64 ++++++++++++++++--------\n builtin/check-attr.c | 3 ++\n t/t1092-sparse-checkout-compatibility.sh | 40 +++++++++++++++\n 3 files changed, 86 insertions(+), 21 deletions(-)\n\n\nbase-commit: 9748a6820043d5815bee770ffa51647e0adc2cf0\n-- \n2.39.0\n\n"},{"id":"479079","messageId":"20230701064843.147496-2-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230701064843.147496-1-cheskaqiqi@gmail.com","subject":"[PATCH v1 1/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-01T06:48:41Z","receivedAt":"2023-07-01T06:55:48Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before this patch,`git check-attr` can't find the attributes of a file\nwithin a sparse directory. In order to read attributes from\n'.gitattributes' files that may be in a sparse directory:\n\nWhen path is in cone mode of sparse checkout:\n\n1.If path is a sparse directory, read the tree OIDs from the sparse\ndirectory.\n\n2.If path is a regular files, read the attributes directly from the blob\ndata stored in the cache.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n attr.c | 64 +++++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 43 insertions(+), 21 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7d39ac4a29..b0d26da102 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -808,35 +808,57 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\t\t\t\t const char *path, unsigned flags)\n {\n+\tstruct attr_stack *stack = NULL;\n+\tint i;\n+\tstruct strbuf path1 = STRBUF_INIT;\n+\tstruct strbuf path2 = STRBUF_INIT;\n+\tchar *first_slash = NULL;\n \tchar *buf;\n \tunsigned long size;\n \n \tif (!istate)\n \t\treturn NULL;\n \n-\t/*\n-\t * The .gitattributes file only applies to files within its\n-\t * parent directory. In the case of cone-mode sparse-checkout,\n-\t * the .gitattributes file is sparse if and only if all paths\n-\t * within that directory are also sparse. Thus, don't load the\n-\t * .gitattributes file since it will not matter.\n-\t *\n-\t * In the case of a sparse index, it is critical that we don't go\n-\t * looking for a .gitattributes file, as doing so would cause the\n-\t * index to expand.\n-\t */\n-\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n-\t\treturn NULL;\n-\n-\tbuf = read_blob_data_from_index(istate, path, &size);\n-\tif (!buf)\n-\t\treturn NULL;\n-\tif (size >= ATTR_MAX_FILE_SIZE) {\n-\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n-\t\treturn NULL;\n+\tfirst_slash = strchr(path, '/');\n+\tif (first_slash) {\n+\t\tstrbuf_add(&path1, path, first_slash - path + 1);\n+\t\tstrbuf_addstr(&path2, first_slash + 1);\n \t}\n \n-\treturn read_attr_from_buf(buf, path, flags);\n+\tif(!path_in_cone_mode_sparse_checkout(path, istate)){\n+\t\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = istate->cache[i];\n+\t\t\tif ( !strcmp(istate->cache[i]->name, path1.buf)&&S_ISSPARSEDIR(ce->ce_mode)) {\n+\t\t\t\tstack = read_attr_from_blob(istate, &ce->oid, path2.buf, flags);\n+\t\t\t}else if(S_ISREG(ce->ce_mode) && !strcmp(istate->cache[i]->name, path)){\n+\t\t\t\tunsigned long sz;\n+\t\t\t\tenum object_type type;\n+\t\t\t\tvoid *data;\n+\n+\t\t\t\tdata = repo_read_object_file(the_repository, &istate->cache[i]->oid,\n+\t\t\t\t\t\t\t&type, &sz);\n+\t\t\t\tif (!data || type != OBJ_BLOB) {\n+\t\t\t\t\tfree(data);\n+\t\t\t\t\tstrbuf_release(&path1);\n+\t\t\t\t\tstrbuf_release(&path2);\n+\t\t\t\t\treturn NULL;\n+\t\t\t\t}\n+\t\t\t\tstack = read_attr_from_buf(data, path, flags);\n+\t\t\t}\n+\t\t}\n+\t}else{\n+\t\tbuf = read_blob_data_from_index(istate, path, &size);\n+\t\tif (!buf)\n+\t\t\treturn NULL;\n+\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n+\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\t stack = read_attr_from_buf(buf, path, flags);\n+\t}\n+\tstrbuf_release(&path1);\n+\tstrbuf_release(&path2);\n+\treturn stack;\n }\n \n static struct attr_stack *read_attr(struct index_state *istate,\n-- \n2.39.0\n\n"},{"id":"479080","messageId":"20230701064843.147496-3-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230701064843.147496-1-cheskaqiqi@gmail.com","subject":"[PATCH v1 2/3] t1092: add tests for `git check-attr`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-01T06:48:42Z","receivedAt":"2023-07-01T06:55:50Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Add tests for `git check-attr`, make sure it behaves as expected when\npath is both inside or outside of sparse-checkout definition.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 29 ++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8a95adf4b5..4edfa3c168 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2259,4 +2259,33 @@ test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n \n+test_expect_success 'check-attr with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes ./deep &&\n+\n+\ttest_all_match git check-attr -a -- deep/a &&\n+\n+\ttest_all_match git add deep/.gitattributes &&\n+\ttest_all_match git check-attr -a --cached -- deep/a\n+'\n+\n+test_expect_success 'check-attr with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\ttest_all_match git check-attr -a -- folder1/a &&\n+\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479081","messageId":"20230701064843.147496-4-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230701064843.147496-1-cheskaqiqi@gmail.com","subject":"[PATCH v1 3/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-01T06:48:43Z","receivedAt":"2023-07-01T06:55:51Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Set the requires-full-index to false for \"diff-tree\".\n\nAdd test to ensure the index is not expanded when the\nsparse index is enabled.\n\nThe `p2000` tests demonstrate a ~63% execution time reduction for\n'git check-attr' using a sparse index.\n\nTest before after\n-----------------------------------------------------------------------\n2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%\n2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%\n2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%\n2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n builtin/check-attr.c | 3 +++\n t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex b22ff748c3..02267f9bc1 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\t\n \tif (repo_read_index(the_repository) < 0) {\n \t\tdie(\"invalid cache\");\n \t}\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 4edfa3c168..317ccc8ec5 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2288,4 +2288,15 @@ test_expect_success 'check-attr with pathspec outside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- folder1/a\n '\n \n+test_expect_success 'sparse-index is not expanded: check-attr' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes ./deep &&\n+\n+\tensure_not_expanded check-attr -a -- deep/a &&\n+\trun_on_all git add deep/.gitattributes &&\n+\tensure_not_expanded check-attr -a --cached -- deep/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479147","messageId":"d1e8af2e-f03c-9fad-7a6f-256545f56f9a@github.com","threadId":"59938","inReplyTo":"20230701064843.147496-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v1 1/3] attr.c: read attributes in a sparse directory","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-03T17:59:19Z","receivedAt":"2023-07-03T17:59:36Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Before this patch,`git check-attr` can't find the attributes of a file\n> within a sparse directory. In order to read attributes from\n> '.gitattributes' files that may be in a sparse directory:\n> \n> When path is in cone mode of sparse checkout:\n> \n> 1.If path is a sparse directory, read the tree OIDs from the sparse\n\ns/path is a sparse directory/path is in a sparse directory(?)\n\n> directory.\n> \n> 2.If path is a regular files, read the attributes directly from the blob\n\ns/files/file\n\n> data stored in the cache.\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> attr.c | 64 +++++++++++++++++++++++++++++++++++++++-------------------\n> 1 file changed, 43 insertions(+), 21 deletions(-)\n> \n> diff --git a/attr.c b/attr.c\n> index 7d39ac4a29..b0d26da102 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -808,35 +808,57 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n> static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> \t\t\t\t\t const char *path, unsigned flags)\n> {\n\nnit: there are a few instances below of spacing inconsistent with project\nstyling: 'if(' instead of 'if (', 'x&&y' instead of 'x && y', etc. Please\nadjust your to match in your next re-roll (using CodingGuidelines and/or\nsurrounding code for reference).\n\n> +\tstruct attr_stack *stack = NULL;\n> +\tint i;\n> +\tstruct strbuf path1 = STRBUF_INIT;\n> +\tstruct strbuf path2 = STRBUF_INIT;\n> +\tchar *first_slash = NULL;\n> \tchar *buf;\n> \tunsigned long size;\n> \n> \tif (!istate)\n> \t\treturn NULL;\n> \n> -\t/*\n> -\t * The .gitattributes file only applies to files within its\n> -\t * parent directory. In the case of cone-mode sparse-checkout,\n> -\t * the .gitattributes file is sparse if and only if all paths\n> -\t * within that directory are also sparse. Thus, don't load the\n> -\t * .gitattributes file since it will not matter.\n> -\t *\n> -\t * In the case of a sparse index, it is critical that we don't go\n> -\t * looking for a .gitattributes file, as doing so would cause the\n> -\t * index to expand.\n> -\t */\n> -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n> -\t\treturn NULL;\n\nCould you add some details to your commit message explaining why the\nreasoning in this comment no longer applies? I agree with your approach, but\nthe extra context will make it easier for reviewers and future readers to\nevaluate whether _they_ agree with it, as well as determine whether your\nimplementation aligns with your stated goal.\n\nAs for this review, I'll assume that we now _always_ want to read\n.gitattributes, regardless of 'SKIP_WORKTREE' or whether .gitattributes is\ncontained within a sparse directory. Please correct me if that\ninterpretation is incorrect!\n\n> -\n> -\tbuf = read_blob_data_from_index(istate, path, &size);\n> -\tif (!buf)\n> -\t\treturn NULL;\n> -\tif (size >= ATTR_MAX_FILE_SIZE) {\n> -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> -\t\treturn NULL;\n> +\tfirst_slash = strchr(path, '/');\n> +\tif (first_slash) {\n> +\t\tstrbuf_add(&path1, path, first_slash - path + 1);\n> +\t\tstrbuf_addstr(&path2, first_slash + 1);\n> \t}\n\nAt this point, 'path1' is the first component of a given path, and 'path2'\nis \"everything else\". If 'path' is 'path/to/my/.gitattributes', 'path1' is\n\"path\" and 'path2' is \"to/my/.gitattributes\". Looks good.\n\n> \n> -\treturn read_attr_from_buf(buf, path, flags);\n> +\tif(!path_in_cone_mode_sparse_checkout(path, istate)){> +\t\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\t\tstruct cache_entry *ce = istate->cache[i];\n> +\t\t\tif ( !strcmp(istate->cache[i]->name, path1.buf)&&S_ISSPARSEDIR(ce->ce_mode)) {\n> +\t\t\t\tstack = read_attr_from_blob(istate, &ce->oid, path2.buf, flags);\n\nHere, you use 'read_attr_from_blob()' to read from the sparse directory's\ntree directly _without_ needing to expand the index. Nice!\n\n> +\t\t\t}else if(S_ISREG(ce->ce_mode) && !strcmp(istate->cache[i]->name, path)){\n> +\t\t\t\tunsigned long sz;\n> +\t\t\t\tenum object_type type;\n> +\t\t\t\tvoid *data;\n> +\n> +\t\t\t\tdata = repo_read_object_file(the_repository, &istate->cache[i]->oid,\n> +\t\t\t\t\t\t\t&type, &sz);\n> +\t\t\t\tif (!data || type != OBJ_BLOB) {\n> +\t\t\t\t\tfree(data);\n> +\t\t\t\t\tstrbuf_release(&path1);\n> +\t\t\t\t\tstrbuf_release(&path2);\n> +\t\t\t\t\treturn NULL;\n> +\t\t\t\t}\n> +\t\t\t\tstack = read_attr_from_buf(data, path, flags);\n> +\t\t\t}\n> +\t\t}\n\n\nOn the whole, this patch updates the the treatment of a 'path' outside the\nsparse-checkout patterns to first iterate through the index and at each\nentry:\n\n1. If the entry is a sparse directory _and_ the first component of 'path'\n matches the sparse directory name, read the .gitattributes with\n 'read_attr_from_blob()'. 'read_attr_from_blob()' reads from the tree\n pointed to by the sparse directory using only the part of 'path' that is\n inside that sparse directory.\n2. If the entry is _not_ a sparse directory _and_ its name matches the full\n 'path', we read the blob by OID into a buffer, then\n 'read_attr_from_buffer()'.\n\nThe general idea behind this makes sense (if .gitattributes is in a sparse\ndirectory, read from the sparse directory tree; if not, directly read the\nindex), but the implementation as it is now has a few gaps/inefficiencies:\n\n- If the sparse directory is not top-level (e.g., a sparse directory at\n 'folder1/foo/'), the .gitattributes will be ignored completely.\n- The iteration through the index continues even after we've read from the\n correct .gitattributes entry.\n- The \"else if\" case above shouldn't functionally be any different from the\n \"else\" case below (both read the .gitattributes blob directly from the\n index) but their implementations are different.\n\nTo avoid those issues, you could adjust the structure of the code to more\nexplicitly match what you described in your commit message:\n\n\tif (*path is inside sparse directory*)\n\t\tstack = read_attr_from_blob(istate, \n\t\t\t\t\t *sparse directory containing path*, \n\t\t\t\t\t *path relative to sparse directory*, \n\t\t\t\t\t flags);\n\telse\n\t\tstack = *read .gitattributes from index blob*\n\nThen fill in the pseudocode bits with concrete details:\n\n- \"read .gitattributes from index blob\" is the most straightforward; it's\n what you have in the \"else\" block below.\n- \"path is inside sparse directory\" can be determined using a combination of\n 'path_in_cone_mode_sparse_checkout()' & 'index_name_pos_sparse()'. An\n example of similar logic can be found in 'entry_is_new_sparse_dir()' in\n 'unpack-trees.c'.\n- \"sparse directory containing path\" and \"path relative to sparse directory\"\n can be determined from the results of 'index_name_pos_sparse()'.\n\n> +\t}else{\n> +\t\tbuf = read_blob_data_from_index(istate, path, &size);\n> +\t\tif (!buf)\n> +\t\t\treturn NULL;\n> +\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n> +\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t\t stack = read_attr_from_buf(buf, path, flags);\n> +\t}\n> +\tstrbuf_release(&path1);\n> +\tstrbuf_release(&path2);\n> +\treturn stack;\n\nThese changes should affect the behavior sparse index-integrated commands\nthat read attributes (e.g. 'git merge'). Would it be possible to test that?\nE.g. take the 't1092' test 'merge with conflict outside cone', but add\nsmudge/clean filters in .gitattributes files inside the affected sparse\ndirectories.\n\n> }\n> \n> static struct attr_stack *read_attr(struct index_state *istate,\n\n"},{"id":"479148","messageId":"52174e5c-421a-256a-f052-85ccaaafca21@github.com","threadId":"59938","inReplyTo":"20230701064843.147496-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v1 2/3] t1092: add tests for `git check-attr`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-03T18:11:38Z","receivedAt":"2023-07-03T18:11:50Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Add tests for `git check-attr`, make sure it behaves as expected when\n> path is both inside or outside of sparse-checkout definition.\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 29 ++++++++++++++++++++++++\n> 1 file changed, 29 insertions(+)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 8a95adf4b5..4edfa3c168 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2259,4 +2259,33 @@ test_expect_success 'worktree is not expanded' '\n> \tensure_not_expanded worktree remove .worktrees/hotfix\n> '\n> \n> +test_expect_success 'check-attr with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_all cp ../.gitattributes ./deep &&\n> +\n> +\ttest_all_match git check-attr -a -- deep/a &&\n\nFirst, ensure 'check-attr' reads the attributes in the untracked\n.gitattributes...\n\n> +\n> +\ttest_all_match git add deep/.gitattributes &&\n> +\ttest_all_match git check-attr -a --cached -- deep/a\n\nThen, once .gitattributes is in the index, 'check-attr --cached' reads the\nattributes from the index. Makes sense.\n\n> +'\n> +\n> +test_expect_success 'check-attr with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_sparse mkdir folder1 &&\n> +\trun_on_all cp ../.gitattributes ./folder1 &&\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\ttest_all_match git check-attr -a -- folder1/a &&\n\nThis test starts the same way as the last, ensuring a .gitattributes file on\ndisk is read. The difference is, this one is outside the sparse cone;\nwithout the previous patch [1], this would not work correctly. Good!\n\n[1] https://lore.kernel.org/git/20230701064843.147496-2-cheskaqiqi@gmail.com/\n\n> +\n> +\tgit -C full-checkout add folder1/.gitattributes &&\n> +\trun_on_sparse git add --sparse folder1/.gitattributes &&\n> +\trun_on_all git commit -m \"add .gitattributes\" &&\n> +\ttest_sparse_match git sparse-checkout reapply &&\n> +\ttest_all_match git check-attr -a --cached -- folder1/a\n\nNow, add the file to the index and reapply the sparse-checkout patterns. In\nboth 'sparse-checkout' and 'sparse-index', the file is removed from disk; in\n'sparse-index', the file is now contained in a sparse directory. Despite\nthis, the attributes are still read correctly by 'check-attr --cached'. \n\nThese tests look great to me! \n\n> +'\n> +\n> test_done\n\n"},{"id":"479150","messageId":"fb4bad5b-6e49-dbad-d9ae-94ec7db9f93c@github.com","threadId":"59938","inReplyTo":"20230701064843.147496-4-cheskaqiqi@gmail.com","subject":"Re: [PATCH v1 3/3] check-attr: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-03T18:21:16Z","receivedAt":"2023-07-03T18:21:49Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Set the requires-full-index to false for \"diff-tree\".\n> \n> Add test to ensure the index is not expanded when the\n> sparse index is enabled.\n> \n> The `p2000` tests demonstrate a ~63% execution time reduction for\n> 'git check-attr' using a sparse index.\n> \n> Test before after\n> -----------------------------------------------------------------------\n> 2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%\n> 2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%\n> 2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%\n> 2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%\n\nGreat results as usual!\n\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> builtin/check-attr.c | 3 +++\n> t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++++\n\nDid you forget to add the perf test to this patch? \n\n> 2 files changed, 14 insertions(+)\n> \n> diff --git a/builtin/check-attr.c b/builtin/check-attr.c\n> index b22ff748c3..02267f9bc1 100644\n> --- a/builtin/check-attr.c\n> +++ b/builtin/check-attr.c\n> @@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, check_attr_options,\n> \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n> \n> +\tprepare_repo_settings(the_repository);\n> +\tthe_repository->settings.command_requires_full_index = 0;\n\nGiven that you updated 'read_attr_from_index()' to handle sparse directories\nin [1], it makes sense that disabling 'command_requires_full_index' is all\nthat's needed to enable the sparse index here.\n\n[1] https://lore.kernel.org/git/20230701064843.147496-2-cheskaqiqi@gmail.com/\n\n> +\t\n> \tif (repo_read_index(the_repository) < 0) {\n> \t\tdie(\"invalid cache\");\n> \t}\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 4edfa3c168..317ccc8ec5 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2288,4 +2288,15 @@ test_expect_success 'check-attr with pathspec outside sparse definition' '\n> \ttest_all_match git check-attr -a --cached -- folder1/a\n> '\n> \n> +test_expect_success 'sparse-index is not expanded: check-attr' '\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_all cp ../.gitattributes ./deep &&\n\nnit: we're only verifying behavior in 'sparse-index', so this should\nprobably be\n\n\tcp .gitattributes ./sparse-index/deep &&\n\n> +\n> +\tensure_not_expanded check-attr -a -- deep/a &&\n> +\trun_on_all git add deep/.gitattributes &&\n\nSimilar to above, this should probably be:\n\n\tgit -C sparse-index add deep/.gitattributes &&\n\n> +\tensure_not_expanded check-attr -a --cached -- deep/a\nIt'd be nice to show that the index is also not expanded files outside of\nthe sparse-checkout cone, e.g. 'folder1/.gitattributes' or\n'folder1/0/.gitattributes' (similar to what you did for the correctness\ntests in [2]).\n\n[2] https://lore.kernel.org/git/20230701064843.147496-3-cheskaqiqi@gmail.com/\n\n> +'\n> +\n> test_done\n\n"},{"id":"479279","messageId":"20230707151839.504494-1-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230701064843.147496-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 0/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-07T15:18:36Z","receivedAt":"2023-07-07T15:19:00Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"\nchange against v1:\n\n* update a new commit message with more details\n\n* update read_attr_from_index function\n\n* add smudge/clean filters in test 'merge with conflict outside cone'\n\n* add the missing perf test\n\n* only verifying behavior in 'sparse-index' in test 'sparse-index is\nnot expanded: check-attr'\n\n* show that the index is also not expanded files outside of\nthe sparse-checkout cone\n\nShuqi Liang (3):\n Enable gitattributes read from sparse directories\n t1092: add tests for `git check-attr`\n check-attr: integrate with sparse-index\n\n attr.c | 42 +++++++++---------\n builtin/check-attr.c | 3 ++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 55 ++++++++++++++++++++++++\n 4 files changed, 80 insertions(+), 21 deletions(-)\n\nRange-diff against v1:\n1: afa27ebe2d < -: ---------- attr.c: read attributes in a sparse directory\n-: ---------- > 1: 0ff2ab9430 Enable gitattributes read from sparse directories\n2: 5bb40b0327 ! 2: 835e1176b0 t1092: add tests for `git check-attr`\n @@ Metadata\n ## Commit message ##\n t1092: add tests for `git check-attr`\n \n + Add smudge/clean filters in .gitattributes files inside the affected\n + sparse directories in test 'merge with conflict outside cone', make sure\n + it behaves as expected when path is outside of sparse-checkout.\n +\n Add tests for `git check-attr`, make sure it behaves as expected when\n path is both inside or outside of sparse-checkout definition.\n \n @@ Commit message\n Signed-off-by: Shuqi Liang \n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'merge with conflict outside cone' '\n + \n + \ttest_all_match git checkout -b merge-tip merge-left &&\n + \ttest_all_match git status --porcelain=v2 &&\n ++\n ++\techo \"a filter=rot13\" >>.gitattributes &&\n ++\trun_on_sparse mkdir folder1 &&\n ++\trun_on_all cp ../.gitattributes ./folder1 &&\n ++\tgit -C full-checkout add folder1/.gitattributes &&\n ++\trun_on_sparse git add --sparse folder1/.gitattributes &&\n ++\trun_on_all git commit -m \"add .gitattributes\" &&\n ++\ttest_sparse_match git sparse-checkout reapply &&\n ++\tgit config filter.rot13.clean \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n ++\tgit config filter.rot13.smudge \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n ++\n + \ttest_all_match test_must_fail git merge -m merge merge-right &&\n + \ttest_all_match git status --porcelain=v2 &&\n + \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n3: abd14ddda7 < -: ---------- check-attr: integrate with sparse-index\n-: ---------- > 3: 672d692e51 check-attr: integrate with sparse-index\n-- \n2.39.0\n\n"},{"id":"479280","messageId":"20230707151839.504494-2-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230707151839.504494-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 1/3] Enable gitattributes read from sparse directories","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-07T15:18:37Z","receivedAt":"2023-07-07T15:19:02Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"'git check-attr' cannot currently find attributes of a file within a\nsparse directory. This is due to .gitattributes files are irrelevant in\nsparse-checkout cone mode, as the file is considered sparse only if all\npaths within its parent directory are also sparse. In addition,\nsearching for a .gitattributes file causes expansion of the sparse\nindex, which is avoided to prevent potential performance degradation.\n\nHowever, this behavior can lead to missing attributes for files inside\nsparse directories, causing inconsistencies in file handling.\n\nTo resolve this, revise 'git check-attr' to allow attribute reading for\nfiles in sparse directories from the corresponding .gitattributes files:\n\n1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\nto check if a path falls within a sparse directory.\n\n2.If path is inside a sparse directory, employ the value of\nindex_name_pos_sparse() to find the sparse directory containing path and\npath relative to sparse directory. Proceed to read attributes from the\ntree OID of the sparse directory using read_attr_from_blob().\n\n3.If path is not inside a sparse directory，ensure that attributes are\nfetched from the index blob with read_blob_data_from_index().\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n attr.c | 42 +++++++++++++++++++++---------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7d39ac4a29..03deb18fb3 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -808,35 +808,35 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\t\t\t\t const char *path, unsigned flags)\n {\n+\tstruct attr_stack *stack = NULL;\n \tchar *buf;\n \tunsigned long size;\n+\tint pos;\n \n \tif (!istate)\n \t\treturn NULL;\n \n-\t/*\n-\t * The .gitattributes file only applies to files within its\n-\t * parent directory. In the case of cone-mode sparse-checkout,\n-\t * the .gitattributes file is sparse if and only if all paths\n-\t * within that directory are also sparse. Thus, don't load the\n-\t * .gitattributes file since it will not matter.\n-\t *\n-\t * In the case of a sparse index, it is critical that we don't go\n-\t * looking for a .gitattributes file, as doing so would cause the\n-\t * index to expand.\n-\t */\n-\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n-\t\treturn NULL;\n+\tpos = index_name_pos_sparse(istate, path, strlen(path));\n+\tpos = -pos-2;\n+\tif (!path_in_cone_mode_sparse_checkout(path, istate) && pos>=0) {\n+\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n+\t\t\treturn NULL;\n \n-\tbuf = read_blob_data_from_index(istate, path, &size);\n-\tif (!buf)\n-\t\treturn NULL;\n-\tif (size >= ATTR_MAX_FILE_SIZE) {\n-\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n-\t\treturn NULL;\n+\t\tif (strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) == 0) {\n+\t\t\tconst char *relative_path = path + ce_namelen(istate->cache[pos]); \n+\t\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n+\t\t}\n+\t} else {\n+\t\tbuf = read_blob_data_from_index(istate, path, &size);\n+\t\tif (!buf)\n+\t\t\treturn NULL;\n+\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n+\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstack = read_attr_from_buf(buf, path, flags);\n \t}\n-\n-\treturn read_attr_from_buf(buf, path, flags);\n+\treturn stack;\n }\n \n static struct attr_stack *read_attr(struct index_state *istate,\n-- \n2.39.0\n\n"},{"id":"479281","messageId":"20230707151839.504494-3-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230707151839.504494-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 2/3] t1092: add tests for `git check-attr`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-07T15:18:38Z","receivedAt":"2023-07-07T15:19:08Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Add smudge/clean filters in .gitattributes files inside the affected\nsparse directories in test 'merge with conflict outside cone', make sure\nit behaves as expected when path is outside of sparse-checkout.\n\nAdd tests for `git check-attr`, make sure it behaves as expected when\npath is both inside or outside of sparse-checkout definition.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8a95adf4b5..839e08d8dd 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1006,6 +1006,17 @@ test_expect_success 'merge with conflict outside cone' '\n \n \ttest_all_match git checkout -b merge-tip merge-left &&\n \ttest_all_match git status --porcelain=v2 &&\n+\n+\techo \"a filter=rot13\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\tgit config filter.rot13.clean \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n+\tgit config filter.rot13.smudge \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n+\n \ttest_all_match test_must_fail git merge -m merge merge-right &&\n \ttest_all_match git status --porcelain=v2 &&\n \n@@ -2259,4 +2270,33 @@ test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n \n+test_expect_success 'check-attr with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes ./deep &&\n+\n+\ttest_all_match git check-attr -a -- deep/a &&\n+\n+\ttest_all_match git add deep/.gitattributes &&\n+\ttest_all_match git check-attr -a --cached -- deep/a\n+'\n+\n+test_expect_success 'check-attr with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\ttest_all_match git check-attr -a -- folder1/a &&\n+\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479282","messageId":"20230707151839.504494-4-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230707151839.504494-1-cheskaqiqi@gmail.com","subject":"[PATCH v2 3/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-07T15:18:39Z","receivedAt":"2023-07-07T15:19:13Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Set the requires-full-index to false for \"diff-tree\".\n\nAdd a test to ensure that the index is not expanded whether the files\nare outside or inside the sparse-checkout cone when the sparse index is\nenabled.\n\nThe `p2000` tests demonstrate a ~63% execution time reduction for\n'git check-attr' using a sparse index.\n\nTest before after\n-----------------------------------------------------------------------\n2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%\n2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%\n2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%\n2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n builtin/check-attr.c | 3 +++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 15 +++++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex b22ff748c3..c1da1d184e 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tif (repo_read_index(the_repository) < 0) {\n \t\tdie(\"invalid cache\");\n \t}\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 96ed3e1d69..39e92b0841 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -134,5 +134,6 @@ test_perf_on_all git diff-files -- $SPARSE_CONE/a\n test_perf_on_all git diff-tree HEAD\n test_perf_on_all git diff-tree HEAD -- $SPARSE_CONE/a\n test_perf_on_all \"git worktree add ../temp && git worktree remove ../temp\"\n+test_perf_on_all git check-attr -a -- $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 839e08d8dd..db2c38ab70 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2299,4 +2299,19 @@ test_expect_success 'check-attr with pathspec outside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- folder1/a\n '\n \n+test_expect_success 'sparse-index is not expanded: check-attr' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\tmkdir ./sparse-index/folder1 &&\n+\tcp ./sparse-index/a ./sparse-index/folder1/a &&\n+\tcp .gitattributes ./sparse-index/deep &&\n+\tcp .gitattributes ./sparse-index/folder1 &&\n+\n+\tgit -C sparse-index add deep/.gitattributes &&\n+\tgit -C sparse-index add --sparse folder1/.gitattributes &&\n+\tensure_not_expanded check-attr -a --cached -- deep/a &&\n+\tensure_not_expanded check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479303","messageId":"xmqqzg4771qg.fsf@gitster.g","threadId":"59938","inReplyTo":"20230707151839.504494-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v2 1/3] Enable gitattributes read from sparse directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-07T23:15:03Z","receivedAt":"2023-07-07T23:15:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> +\tpos = index_name_pos_sparse(istate, path, strlen(path));\n> +\tpos = -pos-2;\n\nWith SP at appropriate places, i.e. \"pos = -pos - 2\".\n\nBut more importantly, where does the -2 come from? For a missing\nentry, we get a negative number, and the location that the cache\nentry with the given path would be inserted can be recovered by\ncomputing -pos - 1, and that is why \n\n\tif (0 <= pos) {\n\t\t... handle existing ce at pos ...\n\t} else if (pos < 0) {\n\t\tpos = -pos - 1;\n\t\t... if such a path were in the index, it would have\n\t\t... been at pos\n\t}\n\nlooks fairly familiar to those who have read our code. Even in such\na case, we do not blindly compute \"-pos - 1\", though.\n\nIn any case, this magic \"adjustment\" of the returned value needs to\nbe explained, perhaps in in-code comment around there.\n\n> +\tif (!path_in_cone_mode_sparse_checkout(path, istate) && pos>=0) {\n\nWith SP at appropriate places, i.e. \" && 0 <= pos\".\n\nThanks.\n"},{"id":"479374","messageId":"20230711133035.16916-1-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230707151839.504494-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 0/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-11T13:30:32Z","receivedAt":"2023-07-11T13:31:38Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"change against v2:\n\n* add SP at appropriate places\n\n* add in-code comment around magic \"adjustment\"\n\nShuqi Liang (3):\n attr.c: read attributes in a sparse directory\n t1092: add tests for `git check-attr`\n check-attr: integrate with sparse-index\n\n attr.c | 47 ++++++++++++--------\n builtin/check-attr.c | 3 ++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 55 ++++++++++++++++++++++++\n 4 files changed, 87 insertions(+), 19 deletions(-)\n\nRange-diff against v2:\n1: 0ff2ab9430 ! 1: 199cc90a5b Enable gitattributes read from sparse directories\n @@ Metadata\n Author: Shuqi Liang \n \n ## Commit message ##\n - Enable gitattributes read from sparse directories\n + attr.c: read attributes in a sparse directory\n \n 'git check-attr' cannot currently find attributes of a file within a\n sparse directory. This is due to .gitattributes files are irrelevant in\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n \tif (!istate)\n \t\treturn NULL;\n \n --\t/*\n + \t/*\n -\t * The .gitattributes file only applies to files within its\n -\t * parent directory. In the case of cone-mode sparse-checkout,\n -\t * the .gitattributes file is sparse if and only if all paths\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n -\t * In the case of a sparse index, it is critical that we don't go\n -\t * looking for a .gitattributes file, as doing so would cause the\n -\t * index to expand.\n --\t */\n ++\t * If the pos value is negative, it means the path is not in the index. \n ++\t * However, the absolute value of pos minus 1 gives us the position where the path \n ++\t * would be inserted in lexicographic order. By subtracting another 1 from this \n ++\t * value (pos = -pos - 2), we find the position of the last index entry \n ++\t * which is lexicographically smaller than the provided path. This would be \n ++\t * the sparse directory containing the path.\n + \t */\n -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n -\t\treturn NULL;\n +\tpos = index_name_pos_sparse(istate, path, strlen(path));\n -+\tpos = -pos-2;\n -+\tif (!path_in_cone_mode_sparse_checkout(path, istate) && pos>=0) {\n -+\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n -+\t\t\treturn NULL;\n ++\tpos = - pos - 2;\n \n -\tbuf = read_blob_data_from_index(istate, path, &size);\n -\tif (!buf)\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n -\tif (size >= ATTR_MAX_FILE_SIZE) {\n -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n -\t\treturn NULL;\n +-\t}\n ++\tif (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n ++\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n ++\t\t\treturn NULL;\n + \n +-\treturn read_attr_from_buf(buf, path, flags);\n +\t\tif (strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) == 0) {\n +\t\t\tconst char *relative_path = path + ce_namelen(istate->cache[pos]); \n +\t\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n +\t\t\treturn NULL;\n +\t\t}\n +\t\tstack = read_attr_from_buf(buf, path, flags);\n - \t}\n --\n --\treturn read_attr_from_buf(buf, path, flags);\n ++\t}\n +\treturn stack;\n }\n \n2: 835e1176b0 = 2: eefce85083 t1092: add tests for `git check-attr`\n3: 672d692e51 = 3: 65c2624504 check-attr: integrate with sparse-index\n-- \n2.39.0\n\n"},{"id":"479375","messageId":"20230711133035.16916-2-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230711133035.16916-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 1/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-11T13:30:33Z","receivedAt":"2023-07-11T13:31:42Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"'git check-attr' cannot currently find attributes of a file within a\nsparse directory. This is due to .gitattributes files are irrelevant in\nsparse-checkout cone mode, as the file is considered sparse only if all\npaths within its parent directory are also sparse. In addition,\nsearching for a .gitattributes file causes expansion of the sparse\nindex, which is avoided to prevent potential performance degradation.\n\nHowever, this behavior can lead to missing attributes for files inside\nsparse directories, causing inconsistencies in file handling.\n\nTo resolve this, revise 'git check-attr' to allow attribute reading for\nfiles in sparse directories from the corresponding .gitattributes files:\n\n1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\nto check if a path falls within a sparse directory.\n\n2.If path is inside a sparse directory, employ the value of\nindex_name_pos_sparse() to find the sparse directory containing path and\npath relative to sparse directory. Proceed to read attributes from the\ntree OID of the sparse directory using read_attr_from_blob().\n\n3.If path is not inside a sparse directory，ensure that attributes are\nfetched from the index blob with read_blob_data_from_index().\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n attr.c | 47 ++++++++++++++++++++++++++++-------------------\n 1 file changed, 28 insertions(+), 19 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7d39ac4a29..be06747b0d 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -808,35 +808,44 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\t\t\t\t const char *path, unsigned flags)\n {\n+\tstruct attr_stack *stack = NULL;\n \tchar *buf;\n \tunsigned long size;\n+\tint pos;\n \n \tif (!istate)\n \t\treturn NULL;\n \n \t/*\n-\t * The .gitattributes file only applies to files within its\n-\t * parent directory. In the case of cone-mode sparse-checkout,\n-\t * the .gitattributes file is sparse if and only if all paths\n-\t * within that directory are also sparse. Thus, don't load the\n-\t * .gitattributes file since it will not matter.\n-\t *\n-\t * In the case of a sparse index, it is critical that we don't go\n-\t * looking for a .gitattributes file, as doing so would cause the\n-\t * index to expand.\n+\t * If the pos value is negative, it means the path is not in the index. \n+\t * However, the absolute value of pos minus 1 gives us the position where the path \n+\t * would be inserted in lexicographic order. By subtracting another 1 from this \n+\t * value (pos = -pos - 2), we find the position of the last index entry \n+\t * which is lexicographically smaller than the provided path. This would be \n+\t * the sparse directory containing the path.\n \t */\n-\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n-\t\treturn NULL;\n+\tpos = index_name_pos_sparse(istate, path, strlen(path));\n+\tpos = - pos - 2;\n \n-\tbuf = read_blob_data_from_index(istate, path, &size);\n-\tif (!buf)\n-\t\treturn NULL;\n-\tif (size >= ATTR_MAX_FILE_SIZE) {\n-\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n-\t\treturn NULL;\n-\t}\n+\tif (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n+\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n+\t\t\treturn NULL;\n \n-\treturn read_attr_from_buf(buf, path, flags);\n+\t\tif (strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) == 0) {\n+\t\t\tconst char *relative_path = path + ce_namelen(istate->cache[pos]); \n+\t\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n+\t\t}\n+\t} else {\n+\t\tbuf = read_blob_data_from_index(istate, path, &size);\n+\t\tif (!buf)\n+\t\t\treturn NULL;\n+\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n+\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstack = read_attr_from_buf(buf, path, flags);\n+\t}\n+\treturn stack;\n }\n \n static struct attr_stack *read_attr(struct index_state *istate,\n-- \n2.39.0\n\n"},{"id":"479376","messageId":"20230711133035.16916-3-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230711133035.16916-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 2/3] t1092: add tests for `git check-attr`","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-11T13:30:34Z","receivedAt":"2023-07-11T13:31:44Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Add smudge/clean filters in .gitattributes files inside the affected\nsparse directories in test 'merge with conflict outside cone', make sure\nit behaves as expected when path is outside of sparse-checkout.\n\nAdd tests for `git check-attr`, make sure it behaves as expected when\npath is both inside or outside of sparse-checkout definition.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8a95adf4b5..839e08d8dd 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -1006,6 +1006,17 @@ test_expect_success 'merge with conflict outside cone' '\n \n \ttest_all_match git checkout -b merge-tip merge-left &&\n \ttest_all_match git status --porcelain=v2 &&\n+\n+\techo \"a filter=rot13\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\tgit config filter.rot13.clean \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n+\tgit config filter.rot13.smudge \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n+\n \ttest_all_match test_must_fail git merge -m merge merge-right &&\n \ttest_all_match git status --porcelain=v2 &&\n \n@@ -2259,4 +2270,33 @@ test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n \n+test_expect_success 'check-attr with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes ./deep &&\n+\n+\ttest_all_match git check-attr -a -- deep/a &&\n+\n+\ttest_all_match git add deep/.gitattributes &&\n+\ttest_all_match git check-attr -a --cached -- deep/a\n+'\n+\n+test_expect_success 'check-attr with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\ttest_all_match git check-attr -a -- folder1/a &&\n+\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479377","messageId":"20230711133035.16916-4-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230711133035.16916-1-cheskaqiqi@gmail.com","subject":"[PATCH v3 3/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-11T13:30:35Z","receivedAt":"2023-07-11T13:31:52Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Set the requires-full-index to false for \"diff-tree\".\n\nAdd a test to ensure that the index is not expanded whether the files\nare outside or inside the sparse-checkout cone when the sparse index is\nenabled.\n\nThe `p2000` tests demonstrate a ~63% execution time reduction for\n'git check-attr' using a sparse index.\n\nTest before after\n-----------------------------------------------------------------------\n2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%\n2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%\n2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%\n2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n builtin/check-attr.c | 3 +++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 15 +++++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex b22ff748c3..c1da1d184e 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tif (repo_read_index(the_repository) < 0) {\n \t\tdie(\"invalid cache\");\n \t}\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 96ed3e1d69..39e92b0841 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -134,5 +134,6 @@ test_perf_on_all git diff-files -- $SPARSE_CONE/a\n test_perf_on_all git diff-tree HEAD\n test_perf_on_all git diff-tree HEAD -- $SPARSE_CONE/a\n test_perf_on_all \"git worktree add ../temp && git worktree remove ../temp\"\n+test_perf_on_all git check-attr -a -- $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 839e08d8dd..db2c38ab70 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2299,4 +2299,19 @@ test_expect_success 'check-attr with pathspec outside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- folder1/a\n '\n \n+test_expect_success 'sparse-index is not expanded: check-attr' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\tmkdir ./sparse-index/folder1 &&\n+\tcp ./sparse-index/a ./sparse-index/folder1/a &&\n+\tcp .gitattributes ./sparse-index/deep &&\n+\tcp .gitattributes ./sparse-index/folder1 &&\n+\n+\tgit -C sparse-index add deep/.gitattributes &&\n+\tgit -C sparse-index add --sparse folder1/.gitattributes &&\n+\tensure_not_expanded check-attr -a --cached -- deep/a &&\n+\tensure_not_expanded check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479381","messageId":"xmqq351u1j5v.fsf@gitster.g","threadId":"59938","inReplyTo":"20230711133035.16916-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 0/3] check-attr: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-11T16:56:28Z","receivedAt":"2023-07-11T16:56:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Seems to have leftover whitespace glitches. They are not a reason\nto reroll alone, but if you need to create v4 then please make sure\nyour patches are free of these.\n\nThanks.\n\n.git/rebase-apply/patch:31: trailing whitespace.\n\t * If the pos value is negative, it means the path is not in the index. \n.git/rebase-apply/patch:32: trailing whitespace.\n\t * However, the absolute value of pos minus 1 gives us the position where the path \n.git/rebase-apply/patch:33: trailing whitespace.\n\t * would be inserted in lexicographic order. By subtracting another 1 from this \n.git/rebase-apply/patch:34: trailing whitespace.\n\t * value (pos = -pos - 2), we find the position of the last index entry \n.git/rebase-apply/patch:35: trailing whitespace.\n\t * which is lexicographically smaller than the provided path. This would be \nwarning: squelched 1 whitespace error\nwarning: 6 lines applied after fixing whitespace errors.\nApplying: attr.c: read attributes in a sparse directory\nApplying: t1092: add tests for `git check-attr`\nApplying: check-attr: integrate with sparse-index\n"},{"id":"479392","messageId":"xmqqedlexoty.fsf@gitster.g","threadId":"59938","inReplyTo":"20230711133035.16916-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 2/3] t1092: add tests for `git check-attr`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-11T18:52:57Z","receivedAt":"2023-07-11T18:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> Add smudge/clean filters in .gitattributes files inside the affected\n> sparse directories in test 'merge with conflict outside cone', make sure\n> it behaves as expected when path is outside of sparse-checkout.\n\nPlease rewrite \"as expected\" into a more concrete form. What is the\nexpectation when path is outside of sparse-checkout? The attributes\nfile does not need to be read and that can be observed by the filter\nprogram not triggering? The attribute file does get read and that\ncan be observed by the filter program running?\n\n> Add tests for `git check-attr`, make sure it behaves as expected when\n> path is both inside or outside of sparse-checkout definition.\n\nDitto.\n\n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++\n> 1 file changed, 40 insertions(+)\n\nThanks.\n\n>\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 8a95adf4b5..839e08d8dd 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1006,6 +1006,17 @@ test_expect_success 'merge with conflict outside cone' '\n> \n> \ttest_all_match git checkout -b merge-tip merge-left &&\n> \ttest_all_match git status --porcelain=v2 &&\n> +\n> +\techo \"a filter=rot13\" >>.gitattributes &&\n> +\trun_on_sparse mkdir folder1 &&\n> +\trun_on_all cp ../.gitattributes ./folder1 &&\n> +\tgit -C full-checkout add folder1/.gitattributes &&\n> +\trun_on_sparse git add --sparse folder1/.gitattributes &&\n> +\trun_on_all git commit -m \"add .gitattributes\" &&\n> +\ttest_sparse_match git sparse-checkout reapply &&\n> +\tgit config filter.rot13.clean \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n> +\tgit config filter.rot13.smudge \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n> +\n> \ttest_all_match test_must_fail git merge -m merge merge-right &&\n> \ttest_all_match git status --porcelain=v2 &&\n> \n> @@ -2259,4 +2270,33 @@ test_expect_success 'worktree is not expanded' '\n> \tensure_not_expanded worktree remove .worktrees/hotfix\n> '\n> \n> +test_expect_success 'check-attr with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_all cp ../.gitattributes ./deep &&\n> +\n> +\ttest_all_match git check-attr -a -- deep/a &&\n> +\n> +\ttest_all_match git add deep/.gitattributes &&\n> +\ttest_all_match git check-attr -a --cached -- deep/a\n> +'\n> +\n> +test_expect_success 'check-attr with pathspec outside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_sparse mkdir folder1 &&\n> +\trun_on_all cp ../.gitattributes ./folder1 &&\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\ttest_all_match git check-attr -a -- folder1/a &&\n> +\n> +\tgit -C full-checkout add folder1/.gitattributes &&\n> +\trun_on_sparse git add --sparse folder1/.gitattributes &&\n> +\trun_on_all git commit -m \"add .gitattributes\" &&\n> +\ttest_sparse_match git sparse-checkout reapply &&\n> +\ttest_all_match git check-attr -a --cached -- folder1/a\n> +'\n> +\n> test_done\n"},{"id":"479393","messageId":"xmqqa5w2xldz.fsf@gitster.g","threadId":"59938","inReplyTo":"20230711133035.16916-4-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 3/3] check-attr: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-11T20:07:20Z","receivedAt":"2023-07-11T20:07:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> Set the requires-full-index to false for \"diff-tree\".\n\nReally...?\n"},{"id":"479395","messageId":"959d9ee6-b2b6-3a2f-c597-856bdaef077b@github.com","threadId":"59938","inReplyTo":"20230711133035.16916-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 2/3] t1092: add tests for `git check-attr`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-11T20:47:18Z","receivedAt":"2023-07-11T20:47:26Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Add smudge/clean filters in .gitattributes files inside the affected\n> sparse directories in test 'merge with conflict outside cone', make sure\n> it behaves as expected when path is outside of sparse-checkout.\n> \n> Add tests for `git check-attr`, make sure it behaves as expected when\n> path is both inside or outside of sparse-checkout definition.\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 40 ++++++++++++++++++++++++\n> 1 file changed, 40 insertions(+)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 8a95adf4b5..839e08d8dd 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -1006,6 +1006,17 @@ test_expect_success 'merge with conflict outside cone' '\n> \n> \ttest_all_match git checkout -b merge-tip merge-left &&\n> \ttest_all_match git status --porcelain=v2 &&\n> +\n> +\techo \"a filter=rot13\" >>.gitattributes &&\n> +\trun_on_sparse mkdir folder1 &&\n> +\trun_on_all cp ../.gitattributes ./folder1 &&\n> +\tgit -C full-checkout add folder1/.gitattributes &&\n> +\trun_on_sparse git add --sparse folder1/.gitattributes &&\n> +\trun_on_all git commit -m \"add .gitattributes\" &&\n> +\ttest_sparse_match git sparse-checkout reapply &&\n> +\tgit config filter.rot13.clean \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n> +\tgit config filter.rot13.smudge \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n> +\n\nIn general, we try to add tests demonstrating behavior in context with the\nimplementation of that behavior. Patch 1 [1] contains the update to\nattribute reading that's being tested here, so this block should be moved\nthere accordingly.\n\nAlso, does this test fail before patch 1 but succeed after? It's a bit\ndifficult to tell how this demonstrates that the in-sparse-directory\n`.gitattributes` is applied properly now but wasn't before. An\nadditional comment in the test or commit message would be helpful for\nunderstanding it better.\n\n[1] https://lore.kernel.org/git/20230711133035.16916-2-cheskaqiqi@gmail.com/\n\n> \ttest_all_match test_must_fail git merge -m merge merge-right &&\n> \ttest_all_match git status --porcelain=v2 &&\n> \n"},{"id":"479396","messageId":"xmqqjzv6w3o2.fsf@gitster.g","threadId":"59938","inReplyTo":"20230711133035.16916-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 1/3] attr.c: read attributes in a sparse directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-11T21:15:25Z","receivedAt":"2023-07-11T21:15:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shuqi Liang writes:\n\n> 'git check-attr' cannot currently find attributes of a file within a\n> sparse directory. This is due to .gitattributes files are irrelevant in\n> sparse-checkout cone mode, as the file is considered sparse only if all\n> paths within its parent directory are also sparse.\n\nI do not quite understand what these two sentences want to say. If\nthe attribute files are truly irrelevant then \"cannot find\" does not\nmatter, because there is no point in finding irrelevant things that\nby definition will not affect the outcome of any commands at all,\nno?\n\n> In addition,\n> searching for a .gitattributes file causes expansion of the sparse\n> index, which is avoided to prevent potential performance degradation.\n\nDoes this sentence want to say that there is a price to pay, in\norder to read an attribute file that is not part of the cones of\ninterest, that you first need to expand the sparse index? I think\nthat is a given and I am not sure what the point of saying it is.\n\n> However, this behavior can lead to missing attributes for files inside\n> sparse directories, causing inconsistencies in file handling.\n\nI agree. Not reading attribute files correctly will lead to a bug.\n\nLet me rephase what (I think) you wrote below to see if I understand\nwhat you are doing correctly.\n\nSuppose that sub1/.gitattributes need to be read, when the calling\ncommand wants to know about attributes of sub1/file. Imagine that\nsub1/ and sub2/ are both outside the cones of interest. It would be\nbetter not to expand sub2/ even though we need to expand sub1/. Not\ncalling ensure_full_index() upfront and instead expanding the\nnecessary subdirectories on demand would be a good way to solve it.\n\nIs that what going on?\n\n> To resolve this, revise 'git check-attr' to allow attribute reading for\n> files in sparse directories from the corresponding .gitattributes files:\n>\n> 1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\n> to check if a path falls within a sparse directory.\n>\n> 2.If path is inside a sparse directory, employ the value of\n> index_name_pos_sparse() to find the sparse directory containing path and\n> path relative to sparse directory. Proceed to read attributes from the\n> tree OID of the sparse directory using read_attr_from_blob().\n>\n> 3.If path is not inside a sparse directory，ensure that attributes are\n> fetched from the index blob with read_blob_data_from_index().\n>\n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> attr.c | 47 ++++++++++++++++++++++++++++-------------------\n> 1 file changed, 28 insertions(+), 19 deletions(-)\n\nThanks.\n\n> diff --git a/attr.c b/attr.c\n> index 7d39ac4a29..be06747b0d 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -808,35 +808,44 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n> static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> \t\t\t\t\t const char *path, unsigned flags)\n> {\n> +\tstruct attr_stack *stack = NULL;\n> \tchar *buf;\n> \tunsigned long size;\n> +\tint pos;\n> \n> \tif (!istate)\n> \t\treturn NULL;\n> \n> \t/*\n> -\t * The .gitattributes file only applies to files within its\n> -\t * parent directory. In the case of cone-mode sparse-checkout,\n> -\t * the .gitattributes file is sparse if and only if all paths\n> -\t * within that directory are also sparse. Thus, don't load the\n> -\t * .gitattributes file since it will not matter.\n\nImagine that you have a tree with sub1/ outside the cones of\ninterest and sub2/ and sub9/ inside the cones of interest, and\nfurther imagine that sub1/.gitattributes and sub2/.gitattributes\ngive attribute X to sub1/file and sub2/file respectively. There\nis no sub9/.gitattributes file.\n\nThen \"git ls-files ':(attr:X)sub[0-9]'\" _could_ have two equally\nsensible behaviours:\n\n (1) Only show sub2/file because sub1/ is outside the cones of\n interest and the user does not want to clutter the output\n from the parts of the tree they are not interested in.\n\n (2) Show both sub1/file and sub2/file, even though sub1/ is outside\n the cones of interest, in response to the fact that the mention\n of \"sub[0-9]\" on the command line is an explicit indication of\n interest by the user (it would become more and more interesting\n if the pathspec gets less specific, like \":(attr:X)\" that is\n treewide, though).\n\nThe original comment seems to say that only behaviour (1) is\nsupported, but I wonder if we eventually want to support both,\nchoice made by the calling code (and perhaps options)? In any case,\noffering the choice of (2) is a good thing in the longer run.\nAnyway...\n\n> +\t * If the pos value is negative, it means the path is not in the index. \n> +\t * However, the absolute value of pos minus 1 gives us the position where the path \n> +\t * would be inserted in lexicographic order. By subtracting another 1 from this \n> +\t * value (pos = -pos - 2), we find the position of the last index entry \n> +\t * which is lexicographically smaller than the provided path. This would be \n> +\t * the sparse directory containing the path.\n\nThat is true only if the directory containing the .gitattribute file\nis sparsified (e.g. sub1/.gitattributes does not appear in the index\nbut sub1/ does; sub2/.gitattributes however does appear in the index\nand there is no sub2/ in the index).\n\nIf not, there are two cases:\n\n * sub2/.gitattributes does appear in the index (and there is no\n sub2/ in the index). \"pos = - pos - 2\" computes a nonsense\n number in this case; hopefully we can reject it early by noticing\n that the resulting pos is negative.\n\n * sub9/.gitattributes does not belong to the project. The pos is\n negative and \"- pos - 2\" does not poihnt at sub9/ (as it is not\n sparse). Depending on what other paths appear in sub9/., the\n path that appears at (-pos-2) may be inside or outside sub9/. In\n the worst case, it could be a sparsified directory that sorts\n directly before sub9/ (say, there is sub8/ that is sparse, which\n may have .gitattributes in it). Would the updated code\n mistakenly check S_ISSPARSEDIR() on sub8/ that has no relevance\n when we are dealing with sub9/.gitattributes that does not exist?\n\n> -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n> -\t\treturn NULL;\n> +\tpos = index_name_pos_sparse(istate, path, strlen(path));\n> +\tpos = - pos - 2;\n> \n> -\tbuf = read_blob_data_from_index(istate, path, &size);\n> -\tif (!buf)\n> -\t\treturn NULL;\n> -\tif (size >= ATTR_MAX_FILE_SIZE) {\n> -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> -\t\treturn NULL;\n> -\t}\n> +\tif (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n> +\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n> +\t\t\treturn NULL;\n\nSo earlier, the code, given say sub1/.gitattributes, checked if that\npath is outside the cones of interest and skipped reading it. But\nthe updated code tries to check the same \"is it outside or inside?\"\ncondition for sub1/ directory itself. Does it make a practical\ndifference that you can demonstrate with a test?\n\nI do not know if the updated code does the right thing for\nsub2/.gitattributes (exists in a non-sparse directory) and\nsub9/.gitattributes (does not exist in non-sparse directory),\nthough.\n\n> +\t\tif (strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) == 0) {\n\nDon't compare with \"==0\", write !strncmp(...) instead.\n\n> +\t\t\tconst char *relative_path = path + ce_namelen(istate->cache[pos]); \n> +\t\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n> +\t\t}\n\nIf the earlier \"- pos - 2\" misidentified the parent sparse directory\nentry in the index and the strncmp() noticed that mistake, we would\ncome here without reading any new attribute stack frame. Don't we\nneed to fallback reading from the path in the correct directory that\nis not at \"- pos - 2\"?\n\nLet's imagine this case where sub/ is a directory outside the cones\nof interest, and our sparse-index may or may not have it as a\ndirectory in the index, and then the caller asks to read from the\n\"sub/sub1/.gitattributes\" file. Even when \"sub/\" is expanded in the\nindex, \"sub/sub1/\" may not and appear as a directory in the index.\n\nThe above \"find relative_path and read from the tree object\" code\nwould of course work when the direct parent directory of\n\".gitattributes\" is visible in the index, but interestingly, it\nwould also work when it does not. E.g. if \"sub/\" is represented as\na directory in the index, then asking for \"sub1/.gitattributes\"\ninside the tree object of \"sub/\" would work as get_tree_entry() used\nby read_attr_from_blob() would get to the right object recursively,\nso that is nice. If that is why \"'- pos - 2' must be the directory\nentry in the index that _would_ include $leadingpath/.gitattributes\nregardless of how many levels of directory hierarchy there are\ninside $leadingpath\" idea was chosen, I'd have to say that it is\nclever ;-)\n\nI however find the \"'- pos - 2' must be the directory entry in the\nindex\" trick hard to reason about and explain. I wonder if we write\nthis in a more straight-forward and stupid way, the result becomes\neasier to read and less prone to future bugs...\n\nThanks.\n"},{"id":"479397","messageId":"e4a77d0f-cf1d-ef76-fe26-ad5e58372a02@github.com","threadId":"59938","inReplyTo":"20230711133035.16916-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v3 1/3] attr.c: read attributes in a sparse directory","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-11T21:24:37Z","receivedAt":"2023-07-11T21:24:43Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> 'git check-attr' cannot currently find attributes of a file within a\n> sparse directory. This is due to .gitattributes files are irrelevant in\n> sparse-checkout cone mode, as the file is considered sparse only if all\n> paths within its parent directory are also sparse. \n\nIf .gitattributes files are irrelevant in sparse-checkout cone mode, then\nwhy are we changing the behavior? If you're challenging that assertion,\nplease state so clearly.\n\n> In addition,> searching for a .gitattributes file causes expansion of the sparse\n> index, which is avoided to prevent potential performance degradation.\n\nThis isn't an unchangeable fact (as your implementation below shows).\nExpanding the index is just the most straightforward approach, but the\nperformance cost of that is (AFAICT) a reason used to justify why we didn't\nread sparse directory attributes in the past.\n\n> \n> However, this behavior can lead to missing attributes for files inside\n> sparse directories, causing inconsistencies in file handling.\n> \n> To resolve this, revise 'git check-attr' to allow attribute reading for\n> files in sparse directories from the corresponding .gitattributes files:\n> \n> 1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\n> to check if a path falls within a sparse directory.\n> \n> 2.If path is inside a sparse directory, employ the value of\n> index_name_pos_sparse() to find the sparse directory containing path and\n> path relative to sparse directory. Proceed to read attributes from the\n> tree OID of the sparse directory using read_attr_from_blob().\n> \n> 3.If path is not inside a sparse directory，ensure that attributes are\n> fetched from the index blob with read_blob_data_from_index().\n\nMakes sense to me.\n\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> attr.c | 47 ++++++++++++++++++++++++++++-------------------\n> 1 file changed, 28 insertions(+), 19 deletions(-)\n> \n> diff --git a/attr.c b/attr.c\n> index 7d39ac4a29..be06747b0d 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -808,35 +808,44 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n> static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> \t\t\t\t\t const char *path, unsigned flags)\n> {\n> +\tstruct attr_stack *stack = NULL;\n> \tchar *buf;\n> \tunsigned long size;\n> +\tint pos;\n> \n> \tif (!istate)\n> \t\treturn NULL;\n> \n> \t/*\n> -\t * The .gitattributes file only applies to files within its\n> -\t * parent directory. In the case of cone-mode sparse-checkout,\n> -\t * the .gitattributes file is sparse if and only if all paths\n> -\t * within that directory are also sparse. Thus, don't load the\n> -\t * .gitattributes file since it will not matter.\n> -\t *\n> -\t * In the case of a sparse index, it is critical that we don't go\n> -\t * looking for a .gitattributes file, as doing so would cause the\n> -\t * index to expand.\n> +\t * If the pos value is negative, it means the path is not in the index. \n> +\t * However, the absolute value of pos minus 1 gives us the position where the path \n> +\t * would be inserted in lexicographic order. By subtracting another 1 from this \n> +\t * value (pos = -pos - 2), we find the position of the last index entry \n> +\t * which is lexicographically smaller than the provided path. This would be \n> +\t * the sparse directory containing the path.\n\nThis is a good explanation of what '-pos - 2' represents, but it doesn't\nexplain why we'd want that value. Could you add a bit of detail around why\n1) we care whether 'pos' identifies a value that exists in the index or not,\nand 2) why we're looking for the sparse directory containing the path?\n\n> \t */\n> -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n> -\t\treturn NULL;\n> +\tpos = index_name_pos_sparse(istate, path, strlen(path));\n> +\tpos = - pos - 2;\n\nnit: don't add the space between '-' and 'pos'. This should be:\n\n\tpos = -pos - 2;\n\n> \n> -\tbuf = read_blob_data_from_index(istate, path, &size);\n> -\tif (!buf)\n> -\t\treturn NULL;\n> -\tif (size >= ATTR_MAX_FILE_SIZE) {\n> -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> -\t\treturn NULL;\n> -\t}\n> +\tif (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n\nTypically, we try to put the less expensive operation first in a condition\nlike this (if the first part of the condition is 'false', the second part\nwon't be evaluated). 'path_in_cone_mode_sparse_checkout()' is more expensive\nthan a simple numerical check, so this should probably be:\n\n\tif (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate)) {\n\nBut on a more general note, why check 'path_in_cone_mode_sparse_checkout()'\nat all? The goal is to determine whether 'path' is inside a sparse\ndirectory, so first you search the index to find where that directory would\nbe, then - if 'path' isn't in the sparse-checkout cone - check whether the\nindex entry you found is a sparse directory. But sparse directories can't\nexist within the sparse-checkout cone in the first place, so the\n'path_in_cone_mode_sparse_checkout()' is redundant. \n\nInstead, 'path_in_cone_mode_sparse_checkout()' (and probably\n'istate->sparse_index', since sparse directories can't exist if the index\nisn't sparse) could be used to avoid calculating 'index_name_pos_sparse()'\nin the first place; the index search operation is generally more expensive\nthan 'path_in_cone_mode_sparse_checkout()', especially when sparse-checkout\nis disabled entirely.\n\n> +\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n> +\t\t\treturn NULL;\n> \n> -\treturn read_attr_from_buf(buf, path, flags);\n> +\t\tif (strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) == 0) {\n\nAll of these nested conditions could be simplified/collapsed into a single,\ntop-level condition:\n\n\tif (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate) &&\n\t S_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n\t !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos]))) {\n\nIMO, this also more clearly reflects _why_ you'd want to enter this\ncondition and read from the index directly:\n\n* If the path is not in the sparse-checkout cone\n* AND the index entry preceding 'path' is a sparse directory\n* AND the sparse directory is the prefix of 'path' (i.e., 'path' is in the\n directory) \n -> Read from the sparse directory's tree\n\nOne other quick sanity check - for the sparse directory prefixing check to\nwork, 'path' needs to be a normalized path relative to the root of the repo.\nIs that guaranteed to be the case here?\n\n> +\t\t\tconst char *relative_path = path + ce_namelen(istate->cache[pos]); \n\nHere, you get the relative path within the sparse directory by skipping past\nthe sparse directory name in 'path'. If 'path' is normalized (see above),\nthis works. Nice!\n\n> +\t\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n> +\t\t}\n> +\t} else {\n> +\t\tbuf = read_blob_data_from_index(istate, path, &size);\n> +\t\tif (!buf)\n> +\t\t\treturn NULL;\n> +\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n> +\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t\tstack = read_attr_from_buf(buf, path, flags);\n> +\t}\n> +\treturn stack;\n> }\n> \n> static struct attr_stack *read_attr(struct index_state *istate,\n\n"},{"id":"479398","messageId":"xmqqfs5uw178.fsf@gitster.g","threadId":"59938","inReplyTo":"xmqqjzv6w3o2.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] attr.c: read attributes in a sparse directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-11T22:08:43Z","receivedAt":"2023-07-11T22:09:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n>> -\t\treturn NULL;\n>> +\tpos = index_name_pos_sparse(istate, path, strlen(path));\n>> +\tpos = - pos - 2;\n>> \n>> -\tbuf = read_blob_data_from_index(istate, path, &size);\n>> -\tif (!buf)\n>> -\t\treturn NULL;\n>> -\tif (size >= ATTR_MAX_FILE_SIZE) {\n>> -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n>> -\t\treturn NULL;\n>> -\t}\n>> +\tif (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n>> +\t\tif (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n>> +\t\t\treturn NULL;\n\nAnother thing I forgot to ask. When we are asked to read\n\".gitattributes\" at the top level, does this code work correctly?\nAs \".gitattributes\" is at the root level, it won't be hidden inside\na sparsified directory in the index, and we do not have to search\nfor its parent. I just wanted to see if the relative_path computation\nand other things we see below will safely be skipped in such a case.\n\nThanks.\n"},{"id":"479474","messageId":"CAMO4yUEVbeLSeOq42V=7RhcLG_4e_D2fBS72Dz-5Oq_u6-RhNw@mail.gmail.com","threadId":"59938","inReplyTo":"xmqqjzv6w3o2.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-13T20:13:39Z","receivedAt":"2023-07-13T20:14:27Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"On Tue, Jul 11, 2023 at 5:15 PM Junio C Hamano wrote:\n>\n> Shuqi Liang writes:\n>\n> > 'git check-attr' cannot currently find attributes of a file within a\n> > sparse directory. This is due to .gitattributes files are irrelevant in\n> > sparse-checkout cone mode, as the file is considered sparse only if all\n> > paths within its parent directory are also sparse.\n>\n> I do not quite understand what these two sentences want to say. If\n> the attribute files are truly irrelevant then \"cannot find\" does not\n> matter, because there is no point in finding irrelevant things that\n> by definition will not affect the outcome of any commands at all,\n> no?\n>\n> > In addition,\n> > searching for a .gitattributes file causes expansion of the sparse\n> > index, which is avoided to prevent potential performance degradation.\n>\n> Does this sentence want to say that there is a price to pay, in\n> order to read an attribute file that is not part of the cones of\n> interest, that you first need to expand the sparse index? I think\n> that is a given and I am not sure what the point of saying it is.\n>\n> > However, this behavior can lead to missing attributes for files inside\n> > sparse directories, causing inconsistencies in file handling.\n>\n> I agree. Not reading attribute files correctly will lead to a bug.\n>\n> Let me rephase what (I think) you wrote below to see if I understand\n> what you are doing correctly.\n>\n> Suppose that sub1/.gitattributes need to be read, when the calling\n> command wants to know about attributes of sub1/file. Imagine that\n> sub1/ and sub2/ are both outside the cones of interest. It would be\n> better not to expand sub2/ even though we need to expand sub1/. Not\n> calling ensure_full_index() upfront and instead expanding the\n> necessary subdirectories on demand would be a good way to solve it.\n>\n> Is that what going on?\n\nSorry for the confusion. I was actually trying to explain why the original\ncomment isn't needed anymore. Here's my updated comment - does this\nmake more sense？\n\nPreviously, the `read_attr_from_index()` function was structured to handle\nattribute reading from a `.gitattributes` file only when the file\npaths were part\nof the cone-mode sparse-checkout. This was based on the fact that the\n`.gitattributes` file only applied to files within its parent\ndirectory. As a result,\nwe avoided loading the `.gitattributes` file if the sparse-checkout was in\ncone-mode, as the file is sparse only if all paths within that directory are\nalso sparse.\n\nHowever, this approach was not capable of handling scenarios where we\nneeded to read attributes from sparse directories。\n\nTo resolve this, revise 'git check-attr' to allow attribute reading for\nfiles in sparse directories from the corresponding .gitattributes files:\n\n1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\nto check if a path falls within a sparse directory.\n\n2.If path is inside a sparse directory, employ the value of\nindex_name_pos_sparse() to find the sparse directory containing path and\npath relative to sparse directory. Proceed to read attributes from the\ntree OID of the sparse directory using read_attr_from_blob().\n\n3.If path is not inside a sparse directory，ensure that attributes are\nfetched from the index blob with read_blob_data_from_index().\n\n\n\n> > diff --git a/attr.c b/attr.c\n> > index 7d39ac4a29..be06747b0d 100644\n> > --- a/attr.c\n> > +++ b/attr.c\n> > @@ -808,35 +808,44 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n> > static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> > const char *path, unsigned flags)\n> > {\n> > + struct attr_stack *stack = NULL;\n> > char *buf;\n> > unsigned long size;\n> > + int pos;\n> >\n> > if (!istate)\n> > return NULL;\n> >\n> > /*\n> > - * The .gitattributes file only applies to files within its\n> > - * parent directory. In the case of cone-mode sparse-checkout,\n> > - * the .gitattributes file is sparse if and only if all paths\n> > - * within that directory are also sparse. Thus, don't load the\n> > - * .gitattributes file since it will not matter.\n>\n> Imagine that you have a tree with sub1/ outside the cones of\n> interest and sub2/ and sub9/ inside the cones of interest, and\n> further imagine that sub1/.gitattributes and sub2/.gitattributes\n> give attribute X to sub1/file and sub2/file respectively. There\n> is no sub9/.gitattributes file.\n>\n> Then \"git ls-files ':(attr:X)sub[0-9]'\" _could_ have two equally\n> sensible behaviours:\n>\n> (1) Only show sub2/file because sub1/ is outside the cones of\n> interest and the user does not want to clutter the output\n> from the parts of the tree they are not interested in.\n>\n> (2) Show both sub1/file and sub2/file, even though sub1/ is outside\n> the cones of interest, in response to the fact that the mention\n> of \"sub[0-9]\" on the command line is an explicit indication of\n> interest by the user (it would become more and more interesting\n> if the pathspec gets less specific, like \":(attr:X)\" that is\n> treewide, though).\n> The original comment seems to say that only behaviour (1) is\n> supported, but I wonder if we eventually want to support both,\n> choice made by the calling code (and perhaps options)? In any case,\n> offering the choice of (2) is a good thing in the longer run.\n> Anyway...\n\nIf we use '--sparse' as an option, then by default we'd only apply (1).\nHowever, if the user types '--sparse', we'd switch to (2). Do you think that's\na good approach?\n\n> > + * If the pos value is negative, it means the path is not in the index.\n> > + * However, the absolute value of pos minus 1 gives us the position where the path\n> > + * would be inserted in lexicographic order. By subtracting another 1 from this\n> > + * value (pos = -pos - 2), we find the position of the last index entry\n> > + * which is lexicographically smaller than the provided path. This would be\n> > + * the sparse directory containing the path.\n>\n> That is true only if the directory containing the .gitattribute file\n> is sparsified (e.g. sub1/.gitattributes does not appear in the index\n> but sub1/ does; sub2/.gitattributes however does appear in the index\n> and there is no sub2/ in the index).\n>\n> If not, there are two cases:\n>\n> * sub2/.gitattributes does appear in the index (and there is no\n> sub2/ in the index). \"pos = - pos - 2\" computes a nonsense\n> number in this case; hopefully we can reject it early by noticing\n> that the resulting pos is negative.\n>\n> * sub9/.gitattributes does not belong to the project. The pos is\n> negative and \"- pos - 2\" does not poihnt at sub9/ (as it is not\n> sparse). Depending on what other paths appear in sub9/., the\n> path that appears at (-pos-2) may be inside or outside sub9/. In\n> the worst case, it could be a sparsified directory that sorts\n> directly before sub9/ (say, there is sub8/ that is sparse, which\n> may have .gitattributes in it). Would the updated code\n> mistakenly check S_ISSPARSEDIR() on sub8/ that has no relevance\n> when we are dealing with sub9/.gitattributes that does not exist?\n\nI made some modifications to the code to address your points. Now, only\ncalculating 'pos' when the path falls outside of the cone. Moreover, we\nonly compute '-pos -2' when 'pos' is negative. I believe this adjustment can\neffectively address the issues you've brought up.\n\n> > - if (!path_in_cone_mode_sparse_checkout(path, istate))\n> > - return NULL;\n> > + pos = index_name_pos_sparse(istate, path, strlen(path));\n> > + pos = - pos - 2;\n> >\n> > - buf = read_blob_data_from_index(istate, path, &size);\n> > - if (!buf)\n> > - return NULL;\n> > - if (size >= ATTR_MAX_FILE_SIZE) {\n> > - warning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> > - return NULL;\n> > - }\n> > + if (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n> > + if (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n> > + return NULL;\n>\n> So earlier, the code, given say sub1/.gitattributes, checked if that\n> path is outside the cones of interest and skipped reading it. But\n> the updated code tries to check the same \"is it outside or inside?\"\n> condition for sub1/ directory itself. Does it make a practical\n> difference that you can demonstrate with a test?\n\nI'm not sure I understand the point. Are you suggesting that checking\n(!S_ISSPARSEDIR(istate->cache[pos]->ce_mode) is unnecessary because\n we don't need to verify it as we already outside the cone?\n\n> I do not know if the updated code does the right thing for\n> sub2/.gitattributes (exists in a non-sparse directory) and\n> sub9/.gitattributes (does not exist in non-sparse directory),\n> though.\n>\n> > + if (strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) == 0) {\n>\n> Don't compare with \"==0\", write !strncmp(...) instead.\n\nWill fix!\n\n> > + const char *relative_path = path + ce_namelen(istate->cache[pos]);\n> > + stack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n> > + }\n>\n> If the earlier \"- pos - 2\" misidentified the parent sparse directory\n> entry in the index and the strncmp() noticed that mistake, we would\n> come here without reading any new attribute stack frame. Don't we\n> need to fallback reading from the path in the correct directory that\n> is not at \"- pos - 2\"?\n>\n> Let's imagine this case where sub/ is a directory outside the cones\n> of interest, and our sparse-index may or may not have it as a\n> directory in the index, and then the caller asks to read from the\n> \"sub/sub1/.gitattributes\" file. Even when \"sub/\" is expanded in the\n> index, \"sub/sub1/\" may not and appear as a directory in the index.\n>\n> The above \"find relative_path and read from the tree object\" code\n> would of course work when the direct parent directory of\n> \".gitattributes\" is visible in the index, but interestingly, it\n> would also work when it does not. E.g. if \"sub/\" is represented as\n> a directory in the index, then asking for \"sub1/.gitattributes\"\n> inside the tree object of \"sub/\" would work as get_tree_entry() used\n> by read_attr_from_blob() would get to the right object recursively,\n> so that is nice. If that is why \"'- pos - 2' must be the directory\n> entry in the index that _would_ include $leadingpath/.gitattributes\n> regardless of how many levels of directory hierarchy there are\n> inside $leadingpath\" idea was chosen, I'd have to say that it is\n> clever ;-)\n>\n> I however find the \"'- pos - 2' must be the directory entry in the\n> index\" trick hard to reason about and explain. I wonder if we write\n> this in a more straight-forward and stupid way, the result becomes\n> easier to read and less prone to future bugs...\n\nHere is my updated code. Is it more straightforward and less prone to bugs?\n\nif (!path_in_cone_mode_sparse_checkout(path, istate)) {\n pos = index_name_pos_sparse(istate, path, strlen(path));\n\n if (pos < 0)\n pos = -pos - 2;\n}\n\nif (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate) &&\nS_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n!strncmp(istate->cache[pos]->name, path,\nce_namelen(istate->cache[pos])) &&\n!normalize_path_copy(normalize_path, path)) {\n relative_path = normalize_path + ce_namelen(istate->cache[pos]);\n stack = read_attr_from_blob(istate, &istate->cache[pos]->oid,\nrelative_path, flags);\n\n stack = read_attr_from_blob(istate, &istate->cache[pos]->oid,\nrelative_path, flags);\n}\n"},{"id":"479475","messageId":"CAMO4yUEh+HMZi8wC1aB=6oLCJkn3CNbw0reVAA-vULfVgF+=NA@mail.gmail.com","threadId":"59938","inReplyTo":"xmqqfs5uw178.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-13T20:22:42Z","receivedAt":"2023-07-13T20:23:00Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"On Tue, Jul 11, 2023 at 6:08 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> >> - if (!path_in_cone_mode_sparse_checkout(path, istate))\n> >> - return NULL;\n> >> + pos = index_name_pos_sparse(istate, path, strlen(path));\n> >> + pos = - pos - 2;\n> >>\n> >> - buf = read_blob_data_from_index(istate, path, &size);\n> >> - if (!buf)\n> >> - return NULL;\n> >> - if (size >= ATTR_MAX_FILE_SIZE) {\n> >> - warning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> >> - return NULL;\n> >> - }\n> >> + if (!path_in_cone_mode_sparse_checkout(path, istate) && 0 <= pos) {\n> >> + if (!S_ISSPARSEDIR(istate->cache[pos]->ce_mode))\n> >> + return NULL;\n>\n> Another thing I forgot to ask. When we are asked to read\n> \".gitattributes\" at the top level, does this code work correctly?\n> As \".gitattributes\" is at the root level, it won't be hidden inside\n> a sparsified directory in the index, and we do not have to search\n> for its parent. I just wanted to see if the relative_path computation\n> and other things we see below will safely be skipped in such a case.\n\nYeah, this code works correctly. I added those tests in t1092 and they\npassed successfully.\n\ntest_expect_success 'check-attr with pathspec inside sparse definition' '\ninit_repos &&\n\n echo \"a -crlf myAttr\" >>.gitattributes &&\n run_on_all cp ../.gitattributes . &&\n\n test_all_match git check-attr -a -- deep/a &&\n\n test_all_match git add .gitattributes &&\n test_all_match git check-attr -a --cached -- deep/a\n'\ntest_expect_success 'check-attr with pathspec inside sparse definition' '\ninit_repos &&\n\n echo \"a -crlf myAttr\" >>.gitattributes &&\n run_on_all cp ../.gitattributes . &&\n\n test_all_match git check-attr -a -- folder1/a &&\n\n test_all_match git add .gitattributes &&\n test_all_match git check-attr -a --cached -- folder1/a\n'\n\nDo I need to modify t1092 to include cases like this?\n"},{"id":"479624","messageId":"20230718232916.31660-1-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230711133035.16916-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 0/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-18T23:29:13Z","receivedAt":"2023-07-18T23:29:49Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"change against v3:\n\n* update a new commit message\n\n* update read_attr_from_index function\n\n* The order of the patches has been rearranged to better illustrate\nthe problem and its solution.\n\n1.t1092: add tests for git check-attr \n2.attr.c: read attributes in a sparse directory\n3.check-attr: integrate with sparse-index\n\nThe new order of patches allows us to introduce a failing test case in\nthe patch 1, which then hen back to \"test_expect_success\" in patch 2.\nThis approach is designed to concretely show why reading attributes\nfrom a sparse directory is needed: without this functionality, the\nsparse index case doesn't work correctly for git check-attr.\n\n* Enhanced the comments in the code to provide more detail. Added\nexplanations as to why 1) it matters whether 'pos' identifies a\nvalue that exists in the index or not, and 2) the rationale behind\nlooking for the sparse directory containing the path\n\n* Add a test 'diff --check with pathspec outside sparse definition'.\nIt starts by disabling the trailing whitespace and space-before-tab\nchecks using the core.whitespace configuration option. Then, it\nspecifically re-enables the trailing whitespace check for a file located\nin a sparse directory. This is accomplished by adding a\nwhitespace=trailing-space rule to the .gitattributes file within that\ndirectory. To ensure that only the .gitattributes file in the index is\nbeing read, and not any .gitattributes files in the working tree, the\ntest removes the .gitattributes file from the working tree after adding\nit to the index. The final part of the test uses 'git diff --check' to\nverify the correct application of the attribute rules. This ensures that\nthe .gitattributes file is correctly read from index and applied, even\nwhen the file's path falls outside of the sparse-checkout definition.\n\n* fix whitespace error \n\nShuqi Liang (3):\n t1092: add tests for 'git check-attr'\n attr.c: read attributes in a sparse directory\n check-attr: integrate with sparse-index\n\n attr.c | 60 +++++++++++++++-------\n builtin/check-attr.c | 3 ++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 63 ++++++++++++++++++++++++\n 4 files changed, 109 insertions(+), 18 deletions(-)\n\nRange-diff against v3:\n1: 199cc90a5b < -: ---------- attr.c: read attributes in a sparse directory\n2: eefce85083 ! 1: 9c43eea9cc t1092: add tests for `git check-attr`\n @@ Metadata\n Author: Shuqi Liang \n \n ## Commit message ##\n - t1092: add tests for `git check-attr`\n + t1092: add tests for 'git check-attr'\n \n - Add smudge/clean filters in .gitattributes files inside the affected\n - sparse directories in test 'merge with conflict outside cone', make sure\n - it behaves as expected when path is outside of sparse-checkout.\n + Add tests for `git check-attr`, make sure attribute file does get read\n + from index when path is either inside or outside of sparse-checkout\n + definition.\n \n - Add tests for `git check-attr`, make sure it behaves as expected when\n - path is both inside or outside of sparse-checkout definition.\n + Add a test named 'diff --check with pathspec outside sparse definition'.\n + It starts by disabling the trailing whitespace and space-before-tab\n + checks using the core.whitespace configuration option. Then, it\n + specifically re-enables the trailing whitespace check for a file located\n + in a sparse directory. This is accomplished by adding a\n + whitespace=trailing-space rule to the .gitattributes file within that\n + directory. To ensure that only the .gitattributes file in the index is\n + being read, and not any .gitattributes files in the working tree, the\n + test removes the .gitattributes file from the working tree after adding\n + it to the index. The final part of the test uses 'git diff --check' to\n + verify the correct application of the attribute rules. This ensures that\n + the .gitattributes file is correctly read from index and applied, even\n + when the file's path falls outside of the sparse-checkout definition.\n \n Helped-by: Victoria Dye \n Signed-off-by: Shuqi Liang \n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'merge with conflict outside cone' '\n - \n - \ttest_all_match git checkout -b merge-tip merge-left &&\n - \ttest_all_match git status --porcelain=v2 &&\n -+\n -+\techo \"a filter=rot13\" >>.gitattributes &&\n -+\trun_on_sparse mkdir folder1 &&\n -+\trun_on_all cp ../.gitattributes ./folder1 &&\n -+\tgit -C full-checkout add folder1/.gitattributes &&\n -+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n -+\trun_on_all git commit -m \"add .gitattributes\" &&\n -+\ttest_sparse_match git sparse-checkout reapply &&\n -+\tgit config filter.rot13.clean \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n -+\tgit config filter.rot13.smudge \"tr 'A-Za-z' 'N-ZA-Mn-za-m'\" &&\n -+\n - \ttest_all_match test_must_fail git merge -m merge merge-right &&\n - \ttest_all_match git status --porcelain=v2 &&\n - \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not e\n +\ttest_all_match git check-attr -a --cached -- deep/a\n +'\n +\n -+test_expect_success 'check-attr with pathspec outside sparse definition' '\n ++test_expect_failure 'check-attr with pathspec outside sparse definition' '\n +\tinit_repos &&\n +\n +\techo \"a -crlf myAttr\" >>.gitattributes &&\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not e\n +\ttest_sparse_match git sparse-checkout reapply &&\n +\ttest_all_match git check-attr -a --cached -- folder1/a\n +'\n ++\n ++test_expect_failure 'diff --check with pathspec outside sparse definition' '\n ++\tinit_repos &&\n ++\n ++\twrite_script edit-contents <<-\\EOF &&\n ++\techo \"a \" >\"$1\"\n ++\tEOF\n ++\n ++\tgit config core.whitespace -trailing-space,-space-before-tab &&\n ++\n ++\techo \"a whitespace=trailing-space,space-before-tab\" >>.gitattributes &&\n ++\trun_on_all mkdir -p folder1 &&\n ++\trun_on_all cp ../.gitattributes ./folder1 &&\n ++\tgit -C full-checkout add folder1/.gitattributes &&\n ++\trun_on_sparse git add --sparse folder1/.gitattributes &&\n ++\trun_on_all rm folder1/.gitattributes &&\n ++\trun_on_all ../edit-contents folder1/a &&\n ++\ttest_all_match test_must_fail git diff --check -- folder1/a\n ++'\n +\n test_done\n-: ---------- > 2: 63ff110b1c attr.c: read attributes in a sparse directory\n3: 65c2624504 ! 3: 7a9c2da30d check-attr: integrate with sparse-index\n @@ Metadata\n ## Commit message ##\n check-attr: integrate with sparse-index\n \n - Set the requires-full-index to false for \"diff-tree\".\n + Set the requires-full-index to false for \"check-attr\".\n \n Add a test to ensure that the index is not expanded whether the files\n are outside or inside the sparse-checkout cone when the sparse index is\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git diff-files -- $SPARSE_CO\n test_done\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'check-attr with pathspec outside sparse definition' '\n - \ttest_all_match git check-attr -a --cached -- folder1/a\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --check with pathspec outside sparse definition' '\n + \ttest_all_match test_must_fail git diff --check -- folder1/a\n '\n \n +test_expect_success 'sparse-index is not expanded: check-attr' '\n-- \n2.39.0\n\n"},{"id":"479625","messageId":"20230718232916.31660-2-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230718232916.31660-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 1/3] t1092: add tests for 'git check-attr'","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-18T23:29:14Z","receivedAt":"2023-07-18T23:29:52Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Add tests for `git check-attr`, make sure attribute file does get read\nfrom index when path is either inside or outside of sparse-checkout\ndefinition.\n\nAdd a test named 'diff --check with pathspec outside sparse definition'.\nIt starts by disabling the trailing whitespace and space-before-tab\nchecks using the core.whitespace configuration option. Then, it\nspecifically re-enables the trailing whitespace check for a file located\nin a sparse directory. This is accomplished by adding a\nwhitespace=trailing-space rule to the .gitattributes file within that\ndirectory. To ensure that only the .gitattributes file in the index is\nbeing read, and not any .gitattributes files in the working tree, the\ntest removes the .gitattributes file from the working tree after adding\nit to the index. The final part of the test uses 'git diff --check' to\nverify the correct application of the attribute rules. This ensures that\nthe .gitattributes file is correctly read from index and applied, even\nwhen the file's path falls outside of the sparse-checkout definition.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8a95adf4b5..90633f383a 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2259,4 +2259,52 @@ test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n \n+test_expect_success 'check-attr with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes ./deep &&\n+\n+\ttest_all_match git check-attr -a -- deep/a &&\n+\n+\ttest_all_match git add deep/.gitattributes &&\n+\ttest_all_match git check-attr -a --cached -- deep/a\n+'\n+\n+test_expect_failure 'check-attr with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\ttest_all_match git check-attr -a -- folder1/a &&\n+\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git check-attr -a --cached -- folder1/a\n+'\n+\n+test_expect_failure 'diff --check with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo \"a \" >\"$1\"\n+\tEOF\n+\n+\tgit config core.whitespace -trailing-space,-space-before-tab &&\n+\n+\techo \"a whitespace=trailing-space,space-before-tab\" >>.gitattributes &&\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n+\trun_on_all rm folder1/.gitattributes &&\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match test_must_fail git diff --check -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479627","messageId":"20230718232916.31660-4-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230718232916.31660-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 3/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-18T23:29:16Z","receivedAt":"2023-07-18T23:29:57Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Set the requires-full-index to false for \"check-attr\".\n\nAdd a test to ensure that the index is not expanded whether the files\nare outside or inside the sparse-checkout cone when the sparse index is\nenabled.\n\nThe `p2000` tests demonstrate a ~63% execution time reduction for\n'git check-attr' using a sparse index.\n\nTest before after\n-----------------------------------------------------------------------\n2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%\n2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%\n2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%\n2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n builtin/check-attr.c | 3 +++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 15 +++++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex b22ff748c3..c1da1d184e 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tif (repo_read_index(the_repository) < 0) {\n \t\tdie(\"invalid cache\");\n \t}\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 96ed3e1d69..39e92b0841 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -134,5 +134,6 @@ test_perf_on_all git diff-files -- $SPARSE_CONE/a\n test_perf_on_all git diff-tree HEAD\n test_perf_on_all git diff-tree HEAD -- $SPARSE_CONE/a\n test_perf_on_all \"git worktree add ../temp && git worktree remove ../temp\"\n+test_perf_on_all git check-attr -a -- $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 3f32c1f972..125b205b0d 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2307,4 +2307,19 @@ test_expect_success 'diff --check with pathspec outside sparse definition' '\n \ttest_all_match test_must_fail git diff --check -- folder1/a\n '\n \n+test_expect_success 'sparse-index is not expanded: check-attr' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\tmkdir ./sparse-index/folder1 &&\n+\tcp ./sparse-index/a ./sparse-index/folder1/a &&\n+\tcp .gitattributes ./sparse-index/deep &&\n+\tcp .gitattributes ./sparse-index/folder1 &&\n+\n+\tgit -C sparse-index add deep/.gitattributes &&\n+\tgit -C sparse-index add --sparse folder1/.gitattributes &&\n+\tensure_not_expanded check-attr -a --cached -- deep/a &&\n+\tensure_not_expanded check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"479626","messageId":"20230718232916.31660-3-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230718232916.31660-1-cheskaqiqi@gmail.com","subject":"[PATCH v4 2/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-07-18T23:29:15Z","receivedAt":"2023-07-18T23:30:00Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before this patch, git check-attr was unable to read the attributes from\na .gitattributes file within a sparse directory. The original comment\nwas operating under the assumption that users are only interested in\nfiles or directories inside the cones. Therefore, in the original code,\nin the case of a cone-mode sparse-checkout, we didn't load the\n.gitattributes file.\n\nHowever, this behavior can lead to missing attributes for files inside\nsparse directories, causing inconsistencies in file handling.\n\nTo resolve this, revise 'git check-attr' to allow attribute reading for\nfiles in sparse directories from the corresponding .gitattributes files:\n\n1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\nto check if a path falls within a sparse directory.\n\n2.If path is inside a sparse directory, employ the value of\nindex_name_pos_sparse() to find the sparse directory containing path and\npath relative to sparse directory. Proceed to read attributes from the\ntree OID of the sparse directory using read_attr_from_blob().\n\n3.If path is not inside a sparse directory，ensure that attributes are\nfetched from the index blob with read_blob_data_from_index().\n\nModify previous tests so such difference is not considered as an error.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n attr.c | 60 +++++++++++++++++-------\n t/t1092-sparse-checkout-compatibility.sh | 4 +-\n 2 files changed, 44 insertions(+), 20 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7d39ac4a29..7650f5481a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -808,35 +808,59 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\t\t\t\t const char *path, unsigned flags)\n {\n+\tstruct attr_stack *stack = NULL;\n \tchar *buf;\n \tunsigned long size;\n+\tint pos = -1;\n+\tchar normalize_path[PATH_MAX];\n+\tconst char *relative_path;\n \n \tif (!istate)\n \t\treturn NULL;\n \n \t/*\n-\t * The .gitattributes file only applies to files within its\n-\t * parent directory. In the case of cone-mode sparse-checkout,\n-\t * the .gitattributes file is sparse if and only if all paths\n-\t * within that directory are also sparse. Thus, don't load the\n-\t * .gitattributes file since it will not matter.\n-\t *\n-\t * In the case of a sparse index, it is critical that we don't go\n-\t * looking for a .gitattributes file, as doing so would cause the\n-\t * index to expand.\n+\t * When handling sparse-checkouts, .gitattributes files\n+\t * may reside within a sparse directory. We distinguish\n+\t * whether a path exists directly in the index or not by\n+\t * evaluating if 'pos' is negative.\n+\t * If 'pos' is negative, the path is not directly present\n+\t * in the index and is likely within a sparse directory.\n+\t * For paths not in the index, The absolute value of 'pos'\n+\t * minus 1 gives us the position where the path would be\n+\t * inserted in lexicographic order within the index.\n+\t * We then subtract another 1 from this value\n+\t * (pos = -pos - 2) to find the position of the last\n+\t * index entry which is lexicographically smaller than\n+\t * the path. This would be the sparse directory containing\n+\t * the path. By identifying the sparse directory containing\n+\t * the path, we can correctly read the attributes specified\n+\t * in the .gitattributes file from the tree object of the\n+\t * sparse directory.\n \t */\n-\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n-\t\treturn NULL;\n+\tif (!path_in_cone_mode_sparse_checkout(path, istate)) {\n+\t\tpos = index_name_pos_sparse(istate, path, strlen(path));\n \n-\tbuf = read_blob_data_from_index(istate, path, &size);\n-\tif (!buf)\n-\t\treturn NULL;\n-\tif (size >= ATTR_MAX_FILE_SIZE) {\n-\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n-\t\treturn NULL;\n+\t\tif (pos < 0)\n+\t\t\tpos = -pos - 2;\n \t}\n \n-\treturn read_attr_from_buf(buf, path, flags);\n+\tif (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate) &&\n+\t S_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n+\t !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) &&\n+\t !normalize_path_copy(normalize_path, path)) {\n+\t\trelative_path = normalize_path + ce_namelen(istate->cache[pos]);\n+\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n+\t} else {\n+\t\tbuf = read_blob_data_from_index(istate, path, &size);\n+\t\tif (!buf)\n+\t\t\treturn NULL;\n+\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n+\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstack = read_attr_from_buf(buf, path, flags);\n+\t}\n+\treturn stack;\n }\n \n static struct attr_stack *read_attr(struct index_state *istate,\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 90633f383a..3f32c1f972 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2271,7 +2271,7 @@ test_expect_success 'check-attr with pathspec inside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- deep/a\n '\n \n-test_expect_failure 'check-attr with pathspec outside sparse definition' '\n+test_expect_success 'check-attr with pathspec outside sparse definition' '\n \tinit_repos &&\n \n \techo \"a -crlf myAttr\" >>.gitattributes &&\n@@ -2288,7 +2288,7 @@ test_expect_failure 'check-attr with pathspec outside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- folder1/a\n '\n \n-test_expect_failure 'diff --check with pathspec outside sparse definition' '\n+test_expect_success 'diff --check with pathspec outside sparse definition' '\n \tinit_repos &&\n \n \twrite_script edit-contents <<-\\EOF &&\n-- \n2.39.0\n\n"},{"id":"479676","messageId":"c3ebe3b4-88b9-8ca2-2ee3-39a3e0d82201@github.com","threadId":"59938","inReplyTo":"20230718232916.31660-2-cheskaqiqi@gmail.com","subject":"Re: [PATCH v4 1/3] t1092: add tests for 'git check-attr'","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-20T18:43:30Z","receivedAt":"2023-07-20T18:43:41Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Add tests for `git check-attr`, make sure attribute file does get read\n> from index when path is either inside or outside of sparse-checkout\n> definition.\n> \n> Add a test named 'diff --check with pathspec outside sparse definition'.\n> It starts by disabling the trailing whitespace and space-before-tab\n> checks using the core.whitespace configuration option. Then, it\n> specifically re-enables the trailing whitespace check for a file located\n> in a sparse directory. This is accomplished by adding a\n> whitespace=trailing-space rule to the .gitattributes file within that\n> directory. To ensure that only the .gitattributes file in the index is\n> being read, and not any .gitattributes files in the working tree, the\n> test removes the .gitattributes file from the working tree after adding\n> it to the index. The final part of the test uses 'git diff --check' to\n> verify the correct application of the attribute rules. This ensures that\n> the .gitattributes file is correctly read from index and applied, even\n> when the file's path falls outside of the sparse-checkout definition.\n\nThanks for the thorough explanation! This presents a compelling case for why\n.gitattributes should be read from sparse directories (if it isn't, the\nbehavior in sparse-index vs. full-checkout and sparse-checkout doesn't\nmatch).\n\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++++++++\n> 1 file changed, 48 insertions(+)\n> \n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 8a95adf4b5..90633f383a 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2259,4 +2259,52 @@ test_expect_success 'worktree is not expanded' '\n> \tensure_not_expanded worktree remove .worktrees/hotfix\n> '\n> \n> +test_expect_success 'check-attr with pathspec inside sparse definition' '\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_all cp ../.gitattributes ./deep &&\n> +\n> +\ttest_all_match git check-attr -a -- deep/a &&\n> +\n> +\ttest_all_match git add deep/.gitattributes &&\n> +\ttest_all_match git check-attr -a --cached -- deep/a\n> +'\n> +\n> +test_expect_failure 'check-attr with pathspec outside sparse definition' '\n\nCould you explain (either in a \"NEEDSWORK\" comment here or in the commit\nmessage) why this is 'test_expect_failure'? \n\n> +\tinit_repos &&\n> +\n> +\techo \"a -crlf myAttr\" >>.gitattributes &&\n> +\trun_on_sparse mkdir folder1 &&\n> +\trun_on_all cp ../.gitattributes ./folder1 &&\n> +\trun_on_all cp a folder1/a &&\n> +\n> +\ttest_all_match git check-attr -a -- folder1/a &&\n> +\n> +\tgit -C full-checkout add folder1/.gitattributes &&\n> +\trun_on_sparse git add --sparse folder1/.gitattributes &&\n> +\trun_on_all git commit -m \"add .gitattributes\" &&\n> +\ttest_sparse_match git sparse-checkout reapply &&\n> +\ttest_all_match git check-attr -a --cached -- folder1/a\n> +'\n> +\n> +test_expect_failure 'diff --check with pathspec outside sparse definition' '\n\nSame here.\n\nHowever, when I apply this patch locally and run this test, I get:\n\n\tok 94 - diff --check with pathspec outside sparse definition # TODO known breakage vanished\n\t# 1 known breakage(s) vanished; please update test(s)\n\nLooking at 'sparse-checkout-out', I see:\n\n\tfolder1/a:1: trailing whitespace.\n\t+a \n\nThis test _should_ fail (as your 'test_expect_failure' indicates), but it\npasses because the outside-of-cone '.gitattributes' is somehow being applied\nto 'folder1/a'. After some debugging, I traced the issue to...\n\n> +\tinit_repos &&\n> +\n> +\twrite_script edit-contents <<-\\EOF &&\n> +\techo \"a \" >\"$1\"\n> +\tEOF\n> +\n> +\tgit config core.whitespace -trailing-space,-space-before-tab &&\n\n...here. This 'git config' doesn't actually apply the configuration to any\nof the test repositories, it applies the config to the parent directory. To\napply the config to the test repos, use:\n\n\ttest_all_match git config core.whitespace -trailing-space,-space-before-tab &&\n\n> +\n> +\techo \"a whitespace=trailing-space,space-before-tab\" >>.gitattributes &&\n> +\trun_on_all mkdir -p folder1 &&\n> +\trun_on_all cp ../.gitattributes ./folder1 &&\n> +\tgit -C full-checkout add folder1/.gitattributes &&\n> +\trun_on_sparse git add --sparse folder1/.gitattributes &&\n\nnit: 'git add --sparse' will work in 'full-checkout' - there's no need to\nhave separate calls for 'full-checkout' and the sparse checkouts. \n\nAlso, please use 'test_(all|sparse)_match' when running Git commands in\nthese tests, even if they're not the command explicitly being tested. It\nadds an extra level of verification to the test essentially \"for free\".\nConversely, using 'run_on_(all|sparse)' could conceal subtle issues in the\ntest or mask bugs in the implementation.\n\n> +\trun_on_all rm folder1/.gitattributes &&\n> +\trun_on_all ../edit-contents folder1/a &&\n\nnit: extra space between 'run_on_all' and '../edit-contents'.\n\n> +\ttest_all_match test_must_fail git diff --check -- folder1/a\n\nBecause '.gitattributes' was added and 'folder1/a' exists on disk, the\n'folder1/' sparse directory is \"unsparsified\" by the time of this check. As\na result, there's essentially no difference in how 'sparse-index' is handled\nvs. 'sparse-checkout'. It would be nice to have this verify the attribute\nparsing in a sparse directory; one way to do that would be something like:\n\n----- 8< ----- 8< ----- 8< ----- 8< ----- 8< ----- 8< ----- 8< ----- 8< -----\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 125b205b0d..183fce8531 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2300,11 +2300,12 @@ test_expect_success 'diff --check with pathspec outside sparse definition' '\n echo \"a whitespace=trailing-space,space-before-tab\" >>.gitattributes &&\n run_on_all mkdir -p folder1 &&\n run_on_all cp ../.gitattributes ./folder1 &&\n- git -C full-checkout add folder1/.gitattributes &&\n- run_on_sparse git add --sparse folder1/.gitattributes &&\n- run_on_all rm folder1/.gitattributes &&\n- run_on_all ../edit-contents folder1/a &&\n- test_all_match test_must_fail git diff --check -- folder1/a\n+ test_all_match git add --sparse folder1/.gitattributes &&\n+ run_on_all ../edit-contents folder1/a &&\n+ test_all_match git add --sparse folder1/a &&\n+\n+ test_sparse_match git sparse-checkout reapply &&\n+ test_all_match test_must_fail git diff --check --cached -- folder1/a\n '\n \n test_expect_success 'sparse-index is not expanded: check-attr' '\n----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 ----- >8 -----\n\nThe main differences from the current patch are:\n\n1. adding 'folder1/a' to the index then \"re-sparsifying\" 'folder1/' with\n 'git sparse-checkout reapply'.\n2. using the '--cached' option to 'git diff' to compare \"index vs. HEAD\"\n rather than \"working tree vs. index\".\n\nFinally, as a general point of feedback - all of the version of this series\nso far have included some minor whitespace/stylistic issues (missing spaces\nin expressions, double spaces, trailing whitespace, etc.). Please check your\npatches carefully before submitting them to avoid excess re-rolls & get your\nchanges merged faster.\n\n[1] https://lore.kernel.org/git/20230718232916.31660-3-cheskaqiqi@gmail.com/\n\n> +'\n> +\n> test_done\n\n"},{"id":"479678","messageId":"5e478d8b-9ef4-864b-41e4-e0a79877d278@github.com","threadId":"59938","inReplyTo":"20230718232916.31660-3-cheskaqiqi@gmail.com","subject":"Re: [PATCH v4 2/3] attr.c: read attributes in a sparse directory","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-07-20T20:18:47Z","receivedAt":"2023-07-20T20:18:59Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> Before this patch, git check-attr was unable to read the attributes from\n> a .gitattributes file within a sparse directory. The original comment\n> was operating under the assumption that users are only interested in\n> files or directories inside the cones. Therefore, in the original code,\n> in the case of a cone-mode sparse-checkout, we didn't load the\n> .gitattributes file.\n> \n> However, this behavior can lead to missing attributes for files inside\n> sparse directories, causing inconsistencies in file handling.\n> \n> To resolve this, revise 'git check-attr' to allow attribute reading for\n> files in sparse directories from the corresponding .gitattributes files:\n> \n> 1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\n> to check if a path falls within a sparse directory.\n> \n> 2.If path is inside a sparse directory, employ the value of\n> index_name_pos_sparse() to find the sparse directory containing path and\n> path relative to sparse directory. Proceed to read attributes from the\n> tree OID of the sparse directory using read_attr_from_blob().\n> \n> 3.If path is not inside a sparse directory，ensure that attributes are\n> fetched from the index blob with read_blob_data_from_index().\n> \n> Modify previous tests so such difference is not considered as an error.\n\nI don't quite follow what you mean here \"such difference\". I see that you\nchanged the 'test_expect_failure' to 'test_expect_success', but that's\nbecause the attributes inside a sparse directory are now being read rather\nthan ignored.\n\nThe rest of the commit message looks good to me, though.\n\n> \n> Helped-by: Victoria Dye \n> Signed-off-by: Shuqi Liang \n> ---\n> attr.c | 60 +++++++++++++++++-------\n> t/t1092-sparse-checkout-compatibility.sh | 4 +-\n> 2 files changed, 44 insertions(+), 20 deletions(-)\n> \n> diff --git a/attr.c b/attr.c\n> index 7d39ac4a29..7650f5481a 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -808,35 +808,59 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n> static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> \t\t\t\t\t const char *path, unsigned flags)\n> {\n> +\tstruct attr_stack *stack = NULL;\n> \tchar *buf;\n> \tunsigned long size;\n> +\tint pos = -1;\n> +\tchar normalize_path[PATH_MAX];\n> +\tconst char *relative_path;\n> \n> \tif (!istate)\n> \t\treturn NULL;\n> \n> \t/*\n> -\t * The .gitattributes file only applies to files within its\n> -\t * parent directory. In the case of cone-mode sparse-checkout,\n> -\t * the .gitattributes file is sparse if and only if all paths\n> -\t * within that directory are also sparse. Thus, don't load the\n> -\t * .gitattributes file since it will not matter.\n> -\t *\n> -\t * In the case of a sparse index, it is critical that we don't go\n> -\t * looking for a .gitattributes file, as doing so would cause the\n> -\t * index to expand.\n> +\t * When handling sparse-checkouts, .gitattributes files\n> +\t * may reside within a sparse directory. We distinguish\n> +\t * whether a path exists directly in the index or not by\n> +\t * evaluating if 'pos' is negative.\n> +\t * If 'pos' is negative, the path is not directly present\n> +\t * in the index and is likely within a sparse directory.\n> +\t * For paths not in the index, The absolute value of 'pos'\n> +\t * minus 1 gives us the position where the path would be\n> +\t * inserted in lexicographic order within the index.\n> +\t * We then subtract another 1 from this value\n> +\t * (pos = -pos - 2) to find the position of the last\n> +\t * index entry which is lexicographically smaller than\n> +\t * the path. This would be the sparse directory containing\n> +\t * the path. By identifying the sparse directory containing\n> +\t * the path, we can correctly read the attributes specified\n> +\t * in the .gitattributes file from the tree object of the\n> +\t * sparse directory.\n> \t */\n> -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n> -\t\treturn NULL;\n> +\tif (!path_in_cone_mode_sparse_checkout(path, istate)) {\n> +\t\tpos = index_name_pos_sparse(istate, path, strlen(path));\n> \n> -\tbuf = read_blob_data_from_index(istate, path, &size);\n> -\tif (!buf)\n> -\t\treturn NULL;\n> -\tif (size >= ATTR_MAX_FILE_SIZE) {\n> -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> -\t\treturn NULL;\n> +\t\tif (pos < 0)\n> +\t\t\tpos = -pos - 2;\n> \t}\n\nWhat 'pos' represents after this block is somewhat confusing/possibly\nmisleading. Consider the possible cases for 'path':\n\n1. 'path' is inside the sparse-checkout cone.\n2. 'path' is not inside the sparse-checkout cone, but it is in the index\n (i.e., the sparse directory has been expanded for some reason).\n3. 'path' is inside a sparse directory.\n\nIn case #1, 'pos' will be '-1'. We never enter the\n'!path_in_cone_mode_sparse_checkout()' if-statement, so the value is never\nupdated.\n\nIn case #2, 'pos' will be positive, since it exists in the index.\n\nIn case #3, the return value of 'index_name_pos_sparse()' will be negative,\nthen assigned the value of '-pos - 2' to (in many cases) create a positive\nvalue pointing to the entry before the insertion point of 'path'.\n\nBased on your explanation in the code comment above, I would assume that, if\n'pos' was >= 0 coming out of this block, it represented the index position\nof the potential sparse directory containing 'path'. But that's not true in\ncase #2.\n\nTo make the purpose of these variables clearer, instead of changing 'pos'\nin-place, you could create a variable like 'sparse_dir_pos'. Like 'pos' is\nnow, it'd be initialized to '-1', but instead of unconditionally assigning\nit the output of 'index_name_pos_sparse()', you'd only assign it if the\noutput of that function is negative:\n\n\tif (!path_in_cone_mode_sparse_checkout(path, istate)) {\n\t\tint pos = index_name_pos_sparse(istate, path, strlen(path));\n\t\tif (pos < 0)\n\t\t\tsparse_dir_pos = -pos - 2;\n\t}\n\nThen, you'd use 'sparse_dir_pos' in the condition below, and it'd always be\n-1 when 'path' is definitely not contained in a sparse directory.\n\n> \n> -\treturn read_attr_from_buf(buf, path, flags);\n> +\tif (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate) &&\n\nnit: the check of '!path_in_cone_mode_sparse_checkout()' is redundant, since\n'pos' will always be '-1' if 'path_in_cone_mode_sparse_checkout()' is true.\n\n> +\t S_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n> +\t !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) &&\n> +\t !normalize_path_copy(normalize_path, path)) {\n\nThe normalization should come before the 'strncmp' so that you're comparing\n'normalize_path' to the index entry. \n\nThat said, I know I asked about path normalization earlier [1], but did you\nconfirm that 'path' _isn't_ normalized? Because if it's normalized by all of\nthe potential callers of this function, the check here would be unnecessary.\n\n[1] https://lore.kernel.org/git/e4a77d0f-cf1d-ef76-fe26-ad5e58372a02@github.com/\n\n> +\t\trelative_path = normalize_path + ce_namelen(istate->cache[pos]);\n\n'relative_path' should be declared here, since it's not used outside this\nblock. \n\n> +\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n> +\t} else {\n> +\t\tbuf = read_blob_data_from_index(istate, path, &size);\n> +\t\tif (!buf)\n> +\t\t\treturn NULL;\n> +\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n> +\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n> +\t\t\treturn NULL;\n> +\t\t}\n> +\t\tstack = read_attr_from_buf(buf, path, flags);\n> +\t}\n> +\treturn stack;\n> }\n> \n> static struct attr_stack *read_attr(struct index_state *istate,\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index 90633f383a..3f32c1f972 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n> @@ -2271,7 +2271,7 @@ test_expect_success 'check-attr with pathspec inside sparse definition' '\n> \ttest_all_match git check-attr -a --cached -- deep/a\n> '\n> \n> -test_expect_failure 'check-attr with pathspec outside sparse definition' '\n> +test_expect_success 'check-attr with pathspec outside sparse definition' '\n\nRe: my suggested change to the test in patch 1 [2], when I applied _this_\npatch, the test still failed for the 'sparse-index' case. It doesn't seem to\nbe a problem with your patch, but rather a bug in 'diff' that can be\nreproduced with this test (using the infrastructure in t1092):\n\ntest_expect_failure 'diff --cached shows differences in sparse directory' '\n\tinit_repos &&\n\n\ttest_all_match git reset --soft update-folder1 &&\n\ttest_all_match git diff --cached -- folder1/a\n'\n\nIt's not immediately obvious to me what the problem is, but my guess is it's\nsome mis-handling of sparse directories in the internal diff machinery.\nGiven the likely complexity of the issue, I'd be content with you leaving\nthe 'diff --check' test as 'test_expect_failure' with a note about the bug\nin 'diff' to fix later. Or, if you do want to investigate & fix it now, I\nwouldn't be opposed to that either. :) \n\n[2] https://lore.kernel.org/git/c3ebe3b4-88b9-8ca2-2ee3-39a3e0d82201@github.com/\n\n> \tinit_repos &&\n> \n> \techo \"a -crlf myAttr\" >>.gitattributes &&\n> @@ -2288,7 +2288,7 @@ test_expect_failure 'check-attr with pathspec outside sparse definition' '\n> \ttest_all_match git check-attr -a --cached -- folder1/a\n> '\n> \n> -test_expect_failure 'diff --check with pathspec outside sparse definition' '\n> +test_expect_success 'diff --check with pathspec outside sparse definition' '\n> \tinit_repos &&\n> \n> \twrite_script edit-contents <<-\\EOF &&\n\n"},{"id":"480127","messageId":"kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"59938","inReplyTo":"5e478d8b-9ef4-864b-41e4-e0a79877d278@github.com","subject":"Re: [PATCH v4 2/3] attr.c: read attributes in a sparse directory","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2023-08-03T16:22:52Z","receivedAt":"2023-08-03T16:23:14Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Here's something odd that I spotted that I think other reviewers haven't\nmentioned. That said, Victoria has already given quite extensive review\nand I trust her judgement on this series, so if I accidentally end up\ncontradicting her, ignore me and trust her instead :)\n\nVictoria Dye writes:\n\n>> -test_expect_failure 'check-attr with pathspec outside sparse definition' '\n>> +test_expect_success 'check-attr with pathspec outside sparse definition' '\n>\n> Re: my suggested change to the test in patch 1 [2], when I applied _this_\n> patch, the test still failed for the 'sparse-index' case. It doesn't seem to\n> be a problem with your patch, but rather a bug in 'diff' that can be\n> reproduced with this test (using the infrastructure in t1092):\n>\n> test_expect_failure 'diff --cached shows differences in sparse directory' '\n> \tinit_repos &&\n>\n> \ttest_all_match git reset --soft update-folder1 &&\n> \ttest_all_match git diff --cached -- folder1/a\n> '\n>\n> It's not immediately obvious to me what the problem is, but my guess is it's\n> some mis-handling of sparse directories in the internal diff machinery.\n> Given the likely complexity of the issue, I'd be content with you leaving\n> the 'diff --check' test as 'test_expect_failure' with a note about the bug\n> in 'diff' to fix later. Or, if you do want to investigate & fix it now, I\n> wouldn't be opposed to that either. :) \n>\n> [2] https://lore.kernel.org/git/c3ebe3b4-88b9-8ca2-2ee3-39a3e0d82201@github.com/\n\nBecause the 'diff --check' test is broken, and 'git check-attr' still\nexpands the index (as noted in the next patch), the code that implements\n'read from a blob if the .gitattributes is not in the index' is not\nexercised by the tests in this patch (it gets exercised in the next\npatch). IOW, you can remove this logic and the tests still pass, like\nso:\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\ndiff --git a/attr.c b/attr.c\nindex 1488b8e18a..abfa2078ac 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -23,6 +23,7 @@\n #include \"thread-utils.h\"\n #include \"tree-walk.h\"\n #include \"object-name.h\"\n+#include \"trace2.h\"\n \n const char git_attr__true[] = \"(builtin)true\";\n const char git_attr__false[] = \"\\0(builtin)false\";\n@@ -847,9 +848,11 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t S_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n \t !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) &&\n \t !normalize_path_copy(normalize_path, path)) {\n-\t\trelative_path = normalize_path + ce_namelen(istate->cache[pos]);\n-\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n+\t\t/* relative_path = normalize_path + ce_namelen(istate->cache[pos]); */\n+\t\t/* stack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags); */\n+\t\ttrace2_printf(\"Tried to read from blob\");\n \t} else {\n+\t\ttrace2_printf(\"Tried to read from index\");\n \t\tbuf = read_blob_data_from_index(istate, path, &size);\n \t\tif (!buf)\n \t\t\treturn NULL;\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n\nIf I were writing patches and encountered this situation, I would squash\npatches 2-3/3 together since both are closely related and quite small,\nbut I'll leave the decision to you + other reviewers.\n"},{"id":"480533","messageId":"20230811142211.4547-1-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230718232916.31660-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 0/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-08-11T14:22:08Z","receivedAt":"2023-08-11T14:22:47Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"MIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nchange against v4:\n\n1/3:\n* Add a commit message to explain why 'test_expect_failure' is set\nand why 'test_expect_success' is set.\n\n* Update 'diff --check with pathspec outside sparse definition' to\ncompare \"index vs HEAD\" rather than \"working tree vs index\".\n\n* Use 'test_all_match' to apply the config to the test repos instead of\nthe parent .\n\n* Use 'test_(all|sparse)_match' when running Git commands in\nthese tests.\n\n2/3:\n* Create a variable named 'sparse_dir_pos' to make the purpose of\nvariable clearer.\n\n* Remove the redundant check of '!path_in_cone_mode_sparse_checkout()'\nsince 'pos' will always be '-1' if 'path_in_cone_mode_sparse_checkout()'\nis true.\n\n* Remove normalize path check because 'prefix_path'(builtin/check-attr.c)\ncall to 'normalize_path_copy_len' (path.c:1124). This confirms that the\npath has indeed been normalized.\n\n* Leave the 'diff --check' test as 'test_expect_failure' with a note about\nthe bug in 'diff' to fix later.\n\n\nShuqi Liang (3):\n t1092: add tests for 'git check-attr'\n attr.c: read attributes in a sparse directory\n check-attr: integrate with sparse-index\n\n attr.c | 57 +++++++++++++------\n builtin/check-attr.c | 3 +\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 72 ++++++++++++++++++++++++\n 4 files changed, 115 insertions(+), 18 deletions(-)\n\nRange-diff against v4:\n1: 9c43eea9cc ! 1: 78d0fc0df1 t1092: add tests for 'git check-attr'\n @@ Commit message\n \n Add a test named 'diff --check with pathspec outside sparse definition'.\n It starts by disabling the trailing whitespace and space-before-tab\n - checks using the core.whitespace configuration option. Then, it\n + checks using the core. whitespace configuration option. Then, it\n specifically re-enables the trailing whitespace check for a file located\n - in a sparse directory. This is accomplished by adding a\n - whitespace=trailing-space rule to the .gitattributes file within that\n - directory. To ensure that only the .gitattributes file in the index is\n - being read, and not any .gitattributes files in the working tree, the\n - test removes the .gitattributes file from the working tree after adding\n - it to the index. The final part of the test uses 'git diff --check' to\n - verify the correct application of the attribute rules. This ensures that\n - the .gitattributes file is correctly read from index and applied, even\n - when the file's path falls outside of the sparse-checkout definition.\n + in a sparse directory by adding a whitespace=trailing-space rule to the\n + .gitattributes file within that directory. Next, create and populate the\n + folder1 directory, and then add the .gitattributes file to the index.\n + Edit the contents of folder1/a, add it to the index, and proceed to\n + \"re-sparsify\" 'folder1/' with 'git sparse-checkout reapply'. Finally,\n + use 'git diff --check --cached' to compare the 'index vs. HEAD',\n + ensuring the correct application of the attribute rules even when the\n + file's path is outside the sparse-checkout definition.\n +\n + Mark the two tests 'check-attr with pathspec outside sparse definition'\n + and 'diff --check with pathspec outside sparse definition' as\n + 'test_expect_failure' to reflect an existing issue where the attributes\n + inside a sparse directory are ignored. Ensure that the 'check-attr'\n + command fails to read the required attributes to demonstrate this\n + expected failure.\n \n Helped-by: Victoria Dye \n Signed-off-by: Shuqi Liang \n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not e\n +\ttest_all_match git check-attr -a -- folder1/a &&\n +\n +\tgit -C full-checkout add folder1/.gitattributes &&\n -+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n -+\trun_on_all git commit -m \"add .gitattributes\" &&\n ++\ttest_sparse_match git add --sparse folder1/.gitattributes &&\n ++\ttest_all_match git commit -m \"add .gitattributes\" &&\n +\ttest_sparse_match git sparse-checkout reapply &&\n -+\ttest_all_match git check-attr -a --cached -- folder1/a\n ++\ttest_all_match git check-attr -a --cached -- folder1/a\n +'\n +\n +test_expect_failure 'diff --check with pathspec outside sparse definition' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not e\n +\techo \"a \" >\"$1\"\n +\tEOF\n +\n -+\tgit config core.whitespace -trailing-space,-space-before-tab &&\n ++\ttest_all_match git config core.whitespace -trailing-space,-space-before-tab &&\n +\n +\techo \"a whitespace=trailing-space,space-before-tab\" >>.gitattributes &&\n +\trun_on_all mkdir -p folder1 &&\n +\trun_on_all cp ../.gitattributes ./folder1 &&\n -+\tgit -C full-checkout add folder1/.gitattributes &&\n -+\trun_on_sparse git add --sparse folder1/.gitattributes &&\n -+\trun_on_all rm folder1/.gitattributes &&\n -+\trun_on_all ../edit-contents folder1/a &&\n -+\ttest_all_match test_must_fail git diff --check -- folder1/a\n ++\ttest_all_match git add --sparse folder1/.gitattributes &&\n ++\trun_on_all ../edit-contents folder1/a &&\n ++\ttest_all_match git add --sparse folder1/a &&\n ++\n ++\ttest_sparse_match git sparse-checkout reapply &&\n ++\ttest_all_match test_must_fail git diff --check --cached -- folder1/a\n +'\n +\n test_done\n2: 63ff110b1c ! 2: ef866930c6 attr.c: read attributes in a sparse directory\n @@ Commit message\n 3.If path is not inside a sparse directory，ensure that attributes are\n fetched from the index blob with read_blob_data_from_index().\n \n - Modify previous tests so such difference is not considered as an error.\n + Change the test 'check-attr with pathspec outside sparse definition' to\n + 'test_expect_success' to reflect that the attributes inside a sparse\n + directory can now be read. Ensure that the sparse index case works\n + correctly for git check-attr to illustrate the successful handling of\n + attributes within sparse directories.\n \n Helped-by: Victoria Dye \n Signed-off-by: Shuqi Liang \n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n +\tstruct attr_stack *stack = NULL;\n \tchar *buf;\n \tunsigned long size;\n -+\tint pos = -1;\n -+\tchar normalize_path[PATH_MAX];\n -+\tconst char *relative_path;\n ++\tint sparse_dir_pos = -1;\n \n \tif (!istate)\n \t\treturn NULL;\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n +\t * minus 1 gives us the position where the path would be\n +\t * inserted in lexicographic order within the index.\n +\t * We then subtract another 1 from this value\n -+\t * (pos = -pos - 2) to find the position of the last\n -+\t * index entry which is lexicographically smaller than\n ++\t * (sparse_dir_pos = -pos - 2) to find the position of the\n ++\t * last index entry which is lexicographically smaller than\n +\t * the path. This would be the sparse directory containing\n +\t * the path. By identifying the sparse directory containing\n +\t * the path, we can correctly read the attributes specified\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n -\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n -\t\treturn NULL;\n +\tif (!path_in_cone_mode_sparse_checkout(path, istate)) {\n -+\t\tpos = index_name_pos_sparse(istate, path, strlen(path));\n ++\t\tint pos = index_name_pos_sparse(istate, path, strlen(path));\n \n -\tbuf = read_blob_data_from_index(istate, path, &size);\n -\tif (!buf)\n @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate\n -\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n -\t\treturn NULL;\n +\t\tif (pos < 0)\n -+\t\t\tpos = -pos - 2;\n ++\t\t\tsparse_dir_pos = -pos - 2;\n \t}\n \n -\treturn read_attr_from_buf(buf, path, flags);\n -+\tif (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate) &&\n -+\t S_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n -+\t !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) &&\n -+\t !normalize_path_copy(normalize_path, path)) {\n -+\t\trelative_path = normalize_path + ce_namelen(istate->cache[pos]);\n -+\t\tstack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n ++\tif (sparse_dir_pos >= 0 &&\n ++\t S_ISSPARSEDIR(istate->cache[sparse_dir_pos]->ce_mode) &&\n ++\t !strncmp(istate->cache[sparse_dir_pos]->name, path, ce_namelen(istate->cache[sparse_dir_pos]))) {\n ++\t\tconst char *relative_path = path + ce_namelen(istate->cache[sparse_dir_pos]);\n ++\t\tstack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags);\n +\t} else {\n +\t\tbuf = read_blob_data_from_index(istate, path, &size);\n +\t\tif (!buf)\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'check-attr with p\n \n \techo \"a -crlf myAttr\" >>.gitattributes &&\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'check-attr with pathspec outside sparse definition' '\n - \ttest_all_match git check-attr -a --cached -- folder1/a\n + \ttest_all_match git check-attr -a --cached -- folder1/a\n '\n \n --test_expect_failure 'diff --check with pathspec outside sparse definition' '\n -+test_expect_success 'diff --check with pathspec outside sparse definition' '\n ++# NEEDSWORK: The 'diff --check' test is left as 'test_expect_failure' due\n ++# to an underlying issue in oneway_diff() within diff-lib.c.\n ++# 'do_oneway_diff()' is not called as expected for paths that could match\n ++# inside of a sparse directory. Specifically, the 'ce_path_match()' function\n ++# fails to recognize files inside a sparse directory (e.g., when 'folder1/'\n ++# is a sparse directory, 'folder1/a' cannot be recognized). The goal is to\n ++# proceed with 'do_oneway_diff()' if the pathspec could match inside of a\n ++# sparse directory.\n + test_expect_failure 'diff --check with pathspec outside sparse definition' '\n \tinit_repos &&\n \n - \twrite_script edit-contents <<-\\EOF &&\n3: 7a9c2da30d ! 3: 310397de6d check-attr: integrate with sparse-index\n @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git diff-files -- $SPARSE_CO\n test_done\n \n ## t/t1092-sparse-checkout-compatibility.sh ##\n -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --check with pathspec outside sparse definition' '\n - \ttest_all_match test_must_fail git diff --check -- folder1/a\n +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff --check with pathspec outside sparse definition' '\n + \ttest_all_match test_must_fail git diff --check --cached -- folder1/a\n '\n \n +test_expect_success 'sparse-index is not expanded: check-attr' '\n @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --check with\n +\tcp .gitattributes ./sparse-index/folder1 &&\n +\n +\tgit -C sparse-index add deep/.gitattributes &&\n -+\tgit -C sparse-index add --sparse folder1/.gitattributes &&\n ++\tgit -C sparse-index add --sparse folder1/.gitattributes &&\n +\tensure_not_expanded check-attr -a --cached -- deep/a &&\n +\tensure_not_expanded check-attr -a --cached -- folder1/a\n +'\n-- \n2.39.0\n\n"},{"id":"480534","messageId":"20230811142211.4547-2-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230811142211.4547-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 1/3] t1092: add tests for 'git check-attr'","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-08-11T14:22:09Z","receivedAt":"2023-08-11T14:22:53Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Add tests for `git check-attr`, make sure attribute file does get read\nfrom index when path is either inside or outside of sparse-checkout\ndefinition.\n\nAdd a test named 'diff --check with pathspec outside sparse definition'.\nIt starts by disabling the trailing whitespace and space-before-tab\nchecks using the core. whitespace configuration option. Then, it\nspecifically re-enables the trailing whitespace check for a file located\nin a sparse directory by adding a whitespace=trailing-space rule to the\n.gitattributes file within that directory. Next, create and populate the\nfolder1 directory, and then add the .gitattributes file to the index.\nEdit the contents of folder1/a, add it to the index, and proceed to\n\"re-sparsify\" 'folder1/' with 'git sparse-checkout reapply'. Finally,\nuse 'git diff --check --cached' to compare the 'index vs. HEAD',\nensuring the correct application of the attribute rules even when the\nfile's path is outside the sparse-checkout definition.\n\nMark the two tests 'check-attr with pathspec outside sparse definition'\nand 'diff --check with pathspec outside sparse definition' as\n'test_expect_failure' to reflect an existing issue where the attributes\ninside a sparse directory are ignored. Ensure that the 'check-attr'\ncommand fails to read the required attributes to demonstrate this\nexpected failure.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n t/t1092-sparse-checkout-compatibility.sh | 49 ++++++++++++++++++++++++\n 1 file changed, 49 insertions(+)\n\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 8a95adf4b5..2d7fa65d81 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2259,4 +2259,53 @@ test_expect_success 'worktree is not expanded' '\n \tensure_not_expanded worktree remove .worktrees/hotfix\n '\n \n+test_expect_success 'check-attr with pathspec inside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_all cp ../.gitattributes ./deep &&\n+\n+\ttest_all_match git check-attr -a -- deep/a &&\n+\n+\ttest_all_match git add deep/.gitattributes &&\n+\ttest_all_match git check-attr -a --cached -- deep/a\n+'\n+\n+test_expect_failure 'check-attr with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\trun_on_sparse mkdir folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\trun_on_all cp a folder1/a &&\n+\n+\ttest_all_match git check-attr -a -- folder1/a &&\n+\n+\tgit -C full-checkout add folder1/.gitattributes &&\n+\ttest_sparse_match git add --sparse folder1/.gitattributes &&\n+\ttest_all_match git commit -m \"add .gitattributes\" &&\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match git check-attr -a --cached -- folder1/a\n+'\n+\n+test_expect_failure 'diff --check with pathspec outside sparse definition' '\n+\tinit_repos &&\n+\n+\twrite_script edit-contents <<-\\EOF &&\n+\techo \"a \" >\"$1\"\n+\tEOF\n+\n+\ttest_all_match git config core.whitespace -trailing-space,-space-before-tab &&\n+\n+\techo \"a whitespace=trailing-space,space-before-tab\" >>.gitattributes &&\n+\trun_on_all mkdir -p folder1 &&\n+\trun_on_all cp ../.gitattributes ./folder1 &&\n+\ttest_all_match git add --sparse folder1/.gitattributes &&\n+\trun_on_all ../edit-contents folder1/a &&\n+\ttest_all_match git add --sparse folder1/a &&\n+\n+\ttest_sparse_match git sparse-checkout reapply &&\n+\ttest_all_match test_must_fail git diff --check --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"480535","messageId":"20230811142211.4547-3-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230811142211.4547-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 2/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-08-11T14:22:10Z","receivedAt":"2023-08-11T14:22:56Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Before this patch, git check-attr was unable to read the attributes from\na .gitattributes file within a sparse directory. The original comment\nwas operating under the assumption that users are only interested in\nfiles or directories inside the cones. Therefore, in the original code,\nin the case of a cone-mode sparse-checkout, we didn't load the\n.gitattributes file.\n\nHowever, this behavior can lead to missing attributes for files inside\nsparse directories, causing inconsistencies in file handling.\n\nTo resolve this, revise 'git check-attr' to allow attribute reading for\nfiles in sparse directories from the corresponding .gitattributes files:\n\n1.Utilize path_in_cone_mode_sparse_checkout() and index_name_pos_sparse\nto check if a path falls within a sparse directory.\n\n2.If path is inside a sparse directory, employ the value of\nindex_name_pos_sparse() to find the sparse directory containing path and\npath relative to sparse directory. Proceed to read attributes from the\ntree OID of the sparse directory using read_attr_from_blob().\n\n3.If path is not inside a sparse directory，ensure that attributes are\nfetched from the index blob with read_blob_data_from_index().\n\nChange the test 'check-attr with pathspec outside sparse definition' to\n'test_expect_success' to reflect that the attributes inside a sparse\ndirectory can now be read. Ensure that the sparse index case works\ncorrectly for git check-attr to illustrate the successful handling of\nattributes within sparse directories.\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n attr.c | 57 ++++++++++++++++--------\n t/t1092-sparse-checkout-compatibility.sh | 10 ++++-\n 2 files changed, 48 insertions(+), 19 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7d39ac4a29..1d34e48ea2 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -808,35 +808,56 @@ static struct attr_stack *read_attr_from_blob(struct index_state *istate,\n static struct attr_stack *read_attr_from_index(struct index_state *istate,\n \t\t\t\t\t const char *path, unsigned flags)\n {\n+\tstruct attr_stack *stack = NULL;\n \tchar *buf;\n \tunsigned long size;\n+\tint sparse_dir_pos = -1;\n \n \tif (!istate)\n \t\treturn NULL;\n \n \t/*\n-\t * The .gitattributes file only applies to files within its\n-\t * parent directory. In the case of cone-mode sparse-checkout,\n-\t * the .gitattributes file is sparse if and only if all paths\n-\t * within that directory are also sparse. Thus, don't load the\n-\t * .gitattributes file since it will not matter.\n-\t *\n-\t * In the case of a sparse index, it is critical that we don't go\n-\t * looking for a .gitattributes file, as doing so would cause the\n-\t * index to expand.\n+\t * When handling sparse-checkouts, .gitattributes files\n+\t * may reside within a sparse directory. We distinguish\n+\t * whether a path exists directly in the index or not by\n+\t * evaluating if 'pos' is negative.\n+\t * If 'pos' is negative, the path is not directly present\n+\t * in the index and is likely within a sparse directory.\n+\t * For paths not in the index, The absolute value of 'pos'\n+\t * minus 1 gives us the position where the path would be\n+\t * inserted in lexicographic order within the index.\n+\t * We then subtract another 1 from this value\n+\t * (sparse_dir_pos = -pos - 2) to find the position of the\n+\t * last index entry which is lexicographically smaller than\n+\t * the path. This would be the sparse directory containing\n+\t * the path. By identifying the sparse directory containing\n+\t * the path, we can correctly read the attributes specified\n+\t * in the .gitattributes file from the tree object of the\n+\t * sparse directory.\n \t */\n-\tif (!path_in_cone_mode_sparse_checkout(path, istate))\n-\t\treturn NULL;\n+\tif (!path_in_cone_mode_sparse_checkout(path, istate)) {\n+\t\tint pos = index_name_pos_sparse(istate, path, strlen(path));\n \n-\tbuf = read_blob_data_from_index(istate, path, &size);\n-\tif (!buf)\n-\t\treturn NULL;\n-\tif (size >= ATTR_MAX_FILE_SIZE) {\n-\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n-\t\treturn NULL;\n+\t\tif (pos < 0)\n+\t\t\tsparse_dir_pos = -pos - 2;\n \t}\n \n-\treturn read_attr_from_buf(buf, path, flags);\n+\tif (sparse_dir_pos >= 0 &&\n+\t S_ISSPARSEDIR(istate->cache[sparse_dir_pos]->ce_mode) &&\n+\t !strncmp(istate->cache[sparse_dir_pos]->name, path, ce_namelen(istate->cache[sparse_dir_pos]))) {\n+\t\tconst char *relative_path = path + ce_namelen(istate->cache[sparse_dir_pos]);\n+\t\tstack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags);\n+\t} else {\n+\t\tbuf = read_blob_data_from_index(istate, path, &size);\n+\t\tif (!buf)\n+\t\t\treturn NULL;\n+\t\tif (size >= ATTR_MAX_FILE_SIZE) {\n+\t\t\twarning(_(\"ignoring overly large gitattributes blob '%s'\"), path);\n+\t\t\treturn NULL;\n+\t\t}\n+\t\tstack = read_attr_from_buf(buf, path, flags);\n+\t}\n+\treturn stack;\n }\n \n static struct attr_stack *read_attr(struct index_state *istate,\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex 2d7fa65d81..dc84b3e2e1 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2271,7 +2271,7 @@ test_expect_success 'check-attr with pathspec inside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- deep/a\n '\n \n-test_expect_failure 'check-attr with pathspec outside sparse definition' '\n+test_expect_success 'check-attr with pathspec outside sparse definition' '\n \tinit_repos &&\n \n \techo \"a -crlf myAttr\" >>.gitattributes &&\n@@ -2288,6 +2288,14 @@ test_expect_failure 'check-attr with pathspec outside sparse definition' '\n \ttest_all_match git check-attr -a --cached -- folder1/a\n '\n \n+# NEEDSWORK: The 'diff --check' test is left as 'test_expect_failure' due\n+# to an underlying issue in oneway_diff() within diff-lib.c.\n+# 'do_oneway_diff()' is not called as expected for paths that could match\n+# inside of a sparse directory. Specifically, the 'ce_path_match()' function\n+# fails to recognize files inside a sparse directory (e.g., when 'folder1/'\n+# is a sparse directory, 'folder1/a' cannot be recognized). The goal is to\n+# proceed with 'do_oneway_diff()' if the pathspec could match inside of a\n+# sparse directory.\n test_expect_failure 'diff --check with pathspec outside sparse definition' '\n \tinit_repos &&\n \n-- \n2.39.0\n\n"},{"id":"480536","messageId":"20230811142211.4547-4-cheskaqiqi@gmail.com","threadId":"59938","inReplyTo":"20230811142211.4547-1-cheskaqiqi@gmail.com","subject":"[PATCH v5 3/3] check-attr: integrate with sparse-index","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-08-11T14:22:11Z","receivedAt":"2023-08-11T14:23:03Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Set the requires-full-index to false for \"check-attr\".\n\nAdd a test to ensure that the index is not expanded whether the files\nare outside or inside the sparse-checkout cone when the sparse index is\nenabled.\n\nThe `p2000` tests demonstrate a ~63% execution time reduction for\n'git check-attr' using a sparse index.\n\nTest before after\n-----------------------------------------------------------------------\n2000.106: git check-attr -a f2/f4/a (full-v3) 0.05 0.05 +0.0%\n2000.107: git check-attr -a f2/f4/a (full-v4) 0.05 0.05 +0.0%\n2000.108: git check-attr -a f2/f4/a (sparse-v3) 0.04 0.02 -50.0%\n2000.109: git check-attr -a f2/f4/a (sparse-v4) 0.04 0.01 -75.0%\n\nHelped-by: Victoria Dye \nSigned-off-by: Shuqi Liang \n---\n builtin/check-attr.c | 3 +++\n t/perf/p2000-sparse-operations.sh | 1 +\n t/t1092-sparse-checkout-compatibility.sh | 15 +++++++++++++++\n 3 files changed, 19 insertions(+)\n\ndiff --git a/builtin/check-attr.c b/builtin/check-attr.c\nindex b22ff748c3..c1da1d184e 100644\n--- a/builtin/check-attr.c\n+++ b/builtin/check-attr.c\n@@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, check_attr_options,\n \t\t\t check_attr_usage, PARSE_OPT_KEEP_DASHDASH);\n \n+\tprepare_repo_settings(the_repository);\n+\tthe_repository->settings.command_requires_full_index = 0;\n+\n \tif (repo_read_index(the_repository) < 0) {\n \t\tdie(\"invalid cache\");\n \t}\ndiff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh\nindex 96ed3e1d69..39e92b0841 100755\n--- a/t/perf/p2000-sparse-operations.sh\n+++ b/t/perf/p2000-sparse-operations.sh\n@@ -134,5 +134,6 @@ test_perf_on_all git diff-files -- $SPARSE_CONE/a\n test_perf_on_all git diff-tree HEAD\n test_perf_on_all git diff-tree HEAD -- $SPARSE_CONE/a\n test_perf_on_all \"git worktree add ../temp && git worktree remove ../temp\"\n+test_perf_on_all git check-attr -a -- $SPARSE_CONE/a\n \n test_done\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex dc84b3e2e1..2a4f35e984 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -2316,4 +2316,19 @@ test_expect_failure 'diff --check with pathspec outside sparse definition' '\n \ttest_all_match test_must_fail git diff --check --cached -- folder1/a\n '\n \n+test_expect_success 'sparse-index is not expanded: check-attr' '\n+\tinit_repos &&\n+\n+\techo \"a -crlf myAttr\" >>.gitattributes &&\n+\tmkdir ./sparse-index/folder1 &&\n+\tcp ./sparse-index/a ./sparse-index/folder1/a &&\n+\tcp .gitattributes ./sparse-index/deep &&\n+\tcp .gitattributes ./sparse-index/folder1 &&\n+\n+\tgit -C sparse-index add deep/.gitattributes &&\n+\tgit -C sparse-index add --sparse folder1/.gitattributes &&\n+\tensure_not_expanded check-attr -a --cached -- deep/a &&\n+\tensure_not_expanded check-attr -a --cached -- folder1/a\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"480654","messageId":"3b2a5b4b-ab8f-746b-6b69-8e8262b6390b@github.com","threadId":"59938","inReplyTo":"20230811142211.4547-1-cheskaqiqi@gmail.com","subject":"Re: [PATCH v5 0/3] check-attr: integrate with sparse-index","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-08-14T16:24:30Z","receivedAt":"2023-08-14T16:25:21Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Shuqi Liang wrote:\n> change against v4:\n\nI've reviewed the patches in this version and all of my prior feedback\nappears to be addressed. Overall, I think this is ready to merge. \n\nI see that you didn't take the suggestion from [1], though. I personally\ndon't consider it a blocking issue, but I am curious to hear your\nthoughts/reasoning behind sticking with your current patch organization over\nwhat was suggested there.\n\n[1] https://lore.kernel.org/git/kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nOtherwise, a couple notes:\n\n> \n> 1/3:\n> * Add a commit message to explain why 'test_expect_failure' is set\n> and why 'test_expect_success' is set.\n> \n> * Update 'diff --check with pathspec outside sparse definition' to\n> compare \"index vs HEAD\" rather than \"working tree vs index\".\n> \n> * Use 'test_all_match' to apply the config to the test repos instead of\n> the parent .\n> \n> * Use 'test_(all|sparse)_match' when running Git commands in\n> these tests.\n> \n> 2/3:\n> * Create a variable named 'sparse_dir_pos' to make the purpose of\n> variable clearer.\n> \n> * Remove the redundant check of '!path_in_cone_mode_sparse_checkout()'\n> since 'pos' will always be '-1' if 'path_in_cone_mode_sparse_checkout()'\n> is true.\n> \n> * Remove normalize path check because 'prefix_path'(builtin/check-attr.c)\n> call to 'normalize_path_copy_len' (path.c:1124). This confirms that the\n> path has indeed been normalized.\n\nNice, thanks for looking into this! I'm glad we're able to avoid the\nnormalization, it simplifies the code quite a bit.\n\n> \n> * Leave the 'diff --check' test as 'test_expect_failure' with a note about\n> the bug in 'diff' to fix later.\n\nMakes sense. The extra detail added in the \"NEEDSWORK\" comment is especially\nhelpful in pointing out which part of the diff machinery causes the issue.\n\n> \n> \n> Shuqi Liang (3):\n> t1092: add tests for 'git check-attr'\n> attr.c: read attributes in a sparse directory\n> check-attr: integrate with sparse-index\n\n"},{"id":"480658","messageId":"xmqq4jl1lfcd.fsf@gitster.g","threadId":"59938","inReplyTo":"3b2a5b4b-ab8f-746b-6b69-8e8262b6390b@github.com","subject":"Re: [PATCH v5 0/3] check-attr: integrate with sparse-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-14T17:10:42Z","receivedAt":"2023-08-14T17:11:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> Shuqi Liang wrote:\n>> change against v4:\n>\n> I've reviewed the patches in this version and all of my prior feedback\n> appears to be addressed. Overall, I think this is ready to merge. \n>\n> I see that you didn't take the suggestion from [1], though. I personally\n> don't consider it a blocking issue, but I am curious to hear your\n> thoughts/reasoning behind sticking with your current patch organization over\n> what was suggested there.\n>\n> [1] https://lore.kernel.org/git/kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com/\n>\n> Otherwise, a couple notes:\n\nThanks for a review Victoria, and Shuqi, thanks for working on the\ntopic.\n\nI too am curious what your response to [1] would be, by the way.\n\n"},{"id":"480669","messageId":"CAMO4yUEwjAfe93Kur5XFzrzXLYpP4=vOg17vDvW-cFmjYLAnOw@mail.gmail.com","threadId":"59938","inReplyTo":"kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v4 2/3] attr.c: read attributes in a sparse directory","fromName":"Shuqi Liang","fromEmail":"cheskaqiqi@gmail.com","sentAt":"2023-08-15T08:05:40Z","receivedAt":"2023-08-15T08:07:00Z","isPatch":true,"sender":{"key":"cheskaqiqi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109261504?v=4"},"body":"Hi Glen,\n\nI just realized I missed your previous email – my apologies for the delay!\nReally appreciate your insights on the patches！\n\nI'm trying to wrap my head around the points you've made. If I'm reading\nyou right, you're suggesting that in my patch2, the logic for 'read from\na blob if the .gitattributes is not in the index' isn't being executed?\nYou think the index is still being expanded and hence this part of the\ncode doesn't influence the test results. In particular, you're referring\nto the 'check-attr with pathspec outside sparse definition' and\n'diff --check with pathspec outside sparse definition' tests, right?\n\nI tried out the code changes you shared, but the tests didn’t pass for\nme. In the original setup, even with the expanded index, the base code\ncouldn't read the attributes from files within a sparse directory.\nSo, I'm inclined to think that the modifications in patch2 have a direct\nbearing on whether the tests pass or fail.\n\nWould love to hear more about your thoughts on this. Thanks again for\ndiving deep into the patches！\n\nOn Fri, Aug 4, 2023 at 12:22 AM Glen Choo wrote:\n>\n> Here's something odd that I spotted that I think other reviewers haven't\n> mentioned. That said, Victoria has already given quite extensive review\n> and I trust her judgement on this series, so if I accidentally end up\n> contradicting her, ignore me and trust her instead :)\n>\n> Victoria Dye writes:\n>\n> >> -test_expect_failure 'check-attr with pathspec outside sparse definition' '\n> >> +test_expect_success 'check-attr with pathspec outside sparse definition' '\n> >\n> > Re: my suggested change to the test in patch 1 [2], when I applied _this_\n> > patch, the test still failed for the 'sparse-index' case. It doesn't seem to\n> > be a problem with your patch, but rather a bug in 'diff' that can be\n> > reproduced with this test (using the infrastructure in t1092):\n> >\n> > test_expect_failure 'diff --cached shows differences in sparse directory' '\n> > init_repos &&\n> >\n> > test_all_match git reset --soft update-folder1 &&\n> > test_all_match git diff --cached -- folder1/a\n> > '\n> >\n> > It's not immediately obvious to me what the problem is, but my guess is it's\n> > some mis-handling of sparse directories in the internal diff machinery.\n> > Given the likely complexity of the issue, I'd be content with you leaving\n> > the 'diff --check' test as 'test_expect_failure' with a note about the bug\n> > in 'diff' to fix later. Or, if you do want to investigate & fix it now, I\n> > wouldn't be opposed to that either. :)\n> >\n> > [2] https://lore.kernel.org/git/c3ebe3b4-88b9-8ca2-2ee3-39a3e0d82201@github.com/\n>\n> Because the 'diff --check' test is broken, and 'git check-attr' still\n> expands the index (as noted in the next patch), the code that implements\n> 'read from a blob if the .gitattributes is not in the index' is not\n> exercised by the tests in this patch (it gets exercised in the next\n> patch). IOW, you can remove this logic and the tests still pass, like\n> so:\n>\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n> diff --git a/attr.c b/attr.c\n> index 1488b8e18a..abfa2078ac 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -23,6 +23,7 @@\n> #include \"thread-utils.h\"\n> #include \"tree-walk.h\"\n> #include \"object-name.h\"\n> +#include \"trace2.h\"\n>\n> const char git_attr__true[] = \"(builtin)true\";\n> const char git_attr__false[] = \"\\0(builtin)false\";\n> @@ -847,9 +848,11 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,\n> S_ISSPARSEDIR(istate->cache[pos]->ce_mode) &&\n> !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) &&\n> !normalize_path_copy(normalize_path, path)) {\n> - relative_path = normalize_path + ce_namelen(istate->cache[pos]);\n> - stack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags);\n> + /* relative_path = normalize_path + ce_namelen(istate->cache[pos]); */\n> + /* stack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags); */\n> + trace2_printf(\"Tried to read from blob\");\n> } else {\n> + trace2_printf(\"Tried to read from index\");\n> buf = read_blob_data_from_index(istate, path, &size);\n> if (!buf)\n> return NULL;\n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----\n>\n> If I were writing patches and encountered this situation, I would squash\n> patches 2-3/3 together since both are closely related and quite small,\n> but I'll leave the decision to you + other reviewers.\n"}]}