| Message ID | 20230811142211.4547-1-cheskaqiqi@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | check-attr: integrate with sparse-index | expand |
Shuqi Liang wrote: > change against v4: I've reviewed the patches in this version and all of my prior feedback appears to be addressed. Overall, I think this is ready to merge. I see that you didn't take the suggestion from [1], though. I personally don't consider it a blocking issue, but I am curious to hear your thoughts/reasoning behind sticking with your current patch organization over what was suggested there. [1] https://lore.kernel.org/git/kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com/ Otherwise, a couple notes: > > 1/3: > * Add a commit message to explain why 'test_expect_failure' is set > and why 'test_expect_success' is set. > > * Update 'diff --check with pathspec outside sparse definition' to > compare "index vs HEAD" rather than "working tree vs index". > > * Use 'test_all_match' to apply the config to the test repos instead of > the parent . > > * Use 'test_(all|sparse)_match' when running Git commands in > these tests. > > 2/3: > * Create a variable named 'sparse_dir_pos' to make the purpose of > variable clearer. > > * Remove the redundant check of '!path_in_cone_mode_sparse_checkout()' > since 'pos' will always be '-1' if 'path_in_cone_mode_sparse_checkout()' > is true. > > * Remove normalize path check because 'prefix_path'(builtin/check-attr.c) > call to 'normalize_path_copy_len' (path.c:1124). This confirms that the > path has indeed been normalized. Nice, thanks for looking into this! I'm glad we're able to avoid the normalization, it simplifies the code quite a bit. > > * Leave the 'diff --check' test as 'test_expect_failure' with a note about > the bug in 'diff' to fix later. Makes sense. The extra detail added in the "NEEDSWORK" comment is especially helpful in pointing out which part of the diff machinery causes the issue. > > > Shuqi Liang (3): > t1092: add tests for 'git check-attr' > attr.c: read attributes in a sparse directory > check-attr: integrate with sparse-index
Victoria Dye <vdye@github.com> writes: > Shuqi Liang wrote: >> change against v4: > > I've reviewed the patches in this version and all of my prior feedback > appears to be addressed. Overall, I think this is ready to merge. > > I see that you didn't take the suggestion from [1], though. I personally > don't consider it a blocking issue, but I am curious to hear your > thoughts/reasoning behind sticking with your current patch organization over > what was suggested there. > > [1] https://lore.kernel.org/git/kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com/ > > Otherwise, a couple notes: Thanks for a review Victoria, and Shuqi, thanks for working on the topic. I too am curious what your response to [1] would be, by the way.
MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit change against v4: 1/3: * Add a commit message to explain why 'test_expect_failure' is set and why 'test_expect_success' is set. * Update 'diff --check with pathspec outside sparse definition' to compare "index vs HEAD" rather than "working tree vs index". * Use 'test_all_match' to apply the config to the test repos instead of the parent . * Use 'test_(all|sparse)_match' when running Git commands in these tests. 2/3: * Create a variable named 'sparse_dir_pos' to make the purpose of variable clearer. * Remove the redundant check of '!path_in_cone_mode_sparse_checkout()' since 'pos' will always be '-1' if 'path_in_cone_mode_sparse_checkout()' is true. * Remove normalize path check because 'prefix_path'(builtin/check-attr.c) call to 'normalize_path_copy_len' (path.c:1124). This confirms that the path has indeed been normalized. * Leave the 'diff --check' test as 'test_expect_failure' with a note about the bug in 'diff' to fix later. Shuqi Liang (3): t1092: add tests for 'git check-attr' attr.c: read attributes in a sparse directory check-attr: integrate with sparse-index attr.c | 57 +++++++++++++------ builtin/check-attr.c | 3 + t/perf/p2000-sparse-operations.sh | 1 + t/t1092-sparse-checkout-compatibility.sh | 72 ++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 18 deletions(-) Range-diff against v4: 1: 9c43eea9cc ! 1: 78d0fc0df1 t1092: add tests for 'git check-attr' @@ Commit message Add a test named 'diff --check with pathspec outside sparse definition'. It starts by disabling the trailing whitespace and space-before-tab - checks using the core.whitespace configuration option. Then, it + checks using the core. whitespace configuration option. Then, it specifically re-enables the trailing whitespace check for a file located - in a sparse directory. This is accomplished by adding a - whitespace=trailing-space rule to the .gitattributes file within that - directory. To ensure that only the .gitattributes file in the index is - being read, and not any .gitattributes files in the working tree, the - test removes the .gitattributes file from the working tree after adding - it to the index. The final part of the test uses 'git diff --check' to - verify the correct application of the attribute rules. This ensures that - the .gitattributes file is correctly read from index and applied, even - when the file's path falls outside of the sparse-checkout definition. + in a sparse directory by adding a whitespace=trailing-space rule to the + .gitattributes file within that directory. Next, create and populate the + folder1 directory, and then add the .gitattributes file to the index. + Edit the contents of folder1/a, add it to the index, and proceed to + "re-sparsify" 'folder1/' with 'git sparse-checkout reapply'. Finally, + use 'git diff --check --cached' to compare the 'index vs. HEAD', + ensuring the correct application of the attribute rules even when the + file's path is outside the sparse-checkout definition. + + Mark the two tests 'check-attr with pathspec outside sparse definition' + and 'diff --check with pathspec outside sparse definition' as + 'test_expect_failure' to reflect an existing issue where the attributes + inside a sparse directory are ignored. Ensure that the 'check-attr' + command fails to read the required attributes to demonstrate this + expected failure. Helped-by: Victoria Dye <vdye@github.com> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not e + test_all_match git check-attr -a -- folder1/a && + + git -C full-checkout add folder1/.gitattributes && -+ run_on_sparse git add --sparse folder1/.gitattributes && -+ run_on_all git commit -m "add .gitattributes" && ++ test_sparse_match git add --sparse folder1/.gitattributes && ++ test_all_match git commit -m "add .gitattributes" && + test_sparse_match git sparse-checkout reapply && -+ test_all_match git check-attr -a --cached -- folder1/a ++ test_all_match git check-attr -a --cached -- folder1/a +' + +test_expect_failure 'diff --check with pathspec outside sparse definition' ' @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'worktree is not e + echo "a " >"$1" + EOF + -+ git config core.whitespace -trailing-space,-space-before-tab && ++ test_all_match git config core.whitespace -trailing-space,-space-before-tab && + + echo "a whitespace=trailing-space,space-before-tab" >>.gitattributes && + run_on_all mkdir -p folder1 && + run_on_all cp ../.gitattributes ./folder1 && -+ git -C full-checkout add folder1/.gitattributes && -+ run_on_sparse git add --sparse folder1/.gitattributes && -+ run_on_all rm folder1/.gitattributes && -+ run_on_all ../edit-contents folder1/a && -+ test_all_match test_must_fail git diff --check -- folder1/a ++ test_all_match git add --sparse folder1/.gitattributes && ++ run_on_all ../edit-contents folder1/a && ++ test_all_match git add --sparse folder1/a && ++ ++ test_sparse_match git sparse-checkout reapply && ++ test_all_match test_must_fail git diff --check --cached -- folder1/a +' + test_done 2: 63ff110b1c ! 2: ef866930c6 attr.c: read attributes in a sparse directory @@ Commit message 3.If path is not inside a sparse directory，ensure that attributes are fetched from the index blob with read_blob_data_from_index(). - Modify previous tests so such difference is not considered as an error. + Change the test 'check-attr with pathspec outside sparse definition' to + 'test_expect_success' to reflect that the attributes inside a sparse + directory can now be read. Ensure that the sparse index case works + correctly for git check-attr to illustrate the successful handling of + attributes within sparse directories. Helped-by: Victoria Dye <vdye@github.com> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com> @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate + struct attr_stack *stack = NULL; char *buf; unsigned long size; -+ int pos = -1; -+ char normalize_path[PATH_MAX]; -+ const char *relative_path; ++ int sparse_dir_pos = -1; if (!istate) return NULL; @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate + * minus 1 gives us the position where the path would be + * inserted in lexicographic order within the index. + * We then subtract another 1 from this value -+ * (pos = -pos - 2) to find the position of the last -+ * index entry which is lexicographically smaller than ++ * (sparse_dir_pos = -pos - 2) to find the position of the ++ * last index entry which is lexicographically smaller than + * the path. This would be the sparse directory containing + * the path. By identifying the sparse directory containing + * the path, we can correctly read the attributes specified @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate - if (!path_in_cone_mode_sparse_checkout(path, istate)) - return NULL; + if (!path_in_cone_mode_sparse_checkout(path, istate)) { -+ pos = index_name_pos_sparse(istate, path, strlen(path)); ++ int pos = index_name_pos_sparse(istate, path, strlen(path)); - buf = read_blob_data_from_index(istate, path, &size); - if (!buf) @@ attr.c: static struct attr_stack *read_attr_from_blob(struct index_state *istate - warning(_("ignoring overly large gitattributes blob '%s'"), path); - return NULL; + if (pos < 0) -+ pos = -pos - 2; ++ sparse_dir_pos = -pos - 2; } - return read_attr_from_buf(buf, path, flags); -+ if (pos >= 0 && !path_in_cone_mode_sparse_checkout(path, istate) && -+ S_ISSPARSEDIR(istate->cache[pos]->ce_mode) && -+ !strncmp(istate->cache[pos]->name, path, ce_namelen(istate->cache[pos])) && -+ !normalize_path_copy(normalize_path, path)) { -+ relative_path = normalize_path + ce_namelen(istate->cache[pos]); -+ stack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags); ++ if (sparse_dir_pos >= 0 && ++ S_ISSPARSEDIR(istate->cache[sparse_dir_pos]->ce_mode) && ++ !strncmp(istate->cache[sparse_dir_pos]->name, path, ce_namelen(istate->cache[sparse_dir_pos]))) { ++ const char *relative_path = path + ce_namelen(istate->cache[sparse_dir_pos]); ++ stack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags); + } else { + buf = read_blob_data_from_index(istate, path, &size); + if (!buf) @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'check-attr with p echo "a -crlf myAttr" >>.gitattributes && @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'check-attr with pathspec outside sparse definition' ' - test_all_match git check-attr -a --cached -- folder1/a + test_all_match git check-attr -a --cached -- folder1/a ' --test_expect_failure 'diff --check with pathspec outside sparse definition' ' -+test_expect_success 'diff --check with pathspec outside sparse definition' ' ++# NEEDSWORK: The 'diff --check' test is left as 'test_expect_failure' due ++# to an underlying issue in oneway_diff() within diff-lib.c. ++# 'do_oneway_diff()' is not called as expected for paths that could match ++# inside of a sparse directory. Specifically, the 'ce_path_match()' function ++# fails to recognize files inside a sparse directory (e.g., when 'folder1/' ++# is a sparse directory, 'folder1/a' cannot be recognized). The goal is to ++# proceed with 'do_oneway_diff()' if the pathspec could match inside of a ++# sparse directory. + test_expect_failure 'diff --check with pathspec outside sparse definition' ' init_repos && - write_script edit-contents <<-\EOF && 3: 7a9c2da30d ! 3: 310397de6d check-attr: integrate with sparse-index @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git diff-files -- $SPARSE_CO test_done ## t/t1092-sparse-checkout-compatibility.sh ## -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --check with pathspec outside sparse definition' ' - test_all_match test_must_fail git diff --check -- folder1/a +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff --check with pathspec outside sparse definition' ' + test_all_match test_must_fail git diff --check --cached -- folder1/a ' +test_expect_success 'sparse-index is not expanded: check-attr' ' @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --check with + cp .gitattributes ./sparse-index/folder1 && + + git -C sparse-index add deep/.gitattributes && -+ git -C sparse-index add --sparse folder1/.gitattributes && ++ git -C sparse-index add --sparse folder1/.gitattributes && + ensure_not_expanded check-attr -a --cached -- deep/a && + ensure_not_expanded check-attr -a --cached -- folder1/a +'