Hi Glen, I just realized I missed your previous email – my apologies for the delay! Really appreciate your insights on the patches！ I'm trying to wrap my head around the points you've made. If I'm reading you right, you're suggesting that in my patch2, the logic for 'read from a blob if the .gitattributes is not in the index' isn't being executed? You think the index is still being expanded and hence this part of the code doesn't influence the test results. In particular, you're referring to the 'check-attr with pathspec outside sparse definition' and 'diff --check with pathspec outside sparse definition' tests, right? I tried out the code changes you shared, but the tests didn’t pass for me. In the original setup, even with the expanded index, the base code couldn't read the attributes from files within a sparse directory. So, I'm inclined to think that the modifications in patch2 have a direct bearing on whether the tests pass or fail. Would love to hear more about your thoughts on this. Thanks again for diving deep into the patches！ On Fri, Aug 4, 2023 at 12:22 AM Glen Choo <chooglen@xxxxxxxxxx> wrote: > > Here's something odd that I spotted that I think other reviewers haven't > mentioned. That said, Victoria has already given quite extensive review > and I trust her judgement on this series, so if I accidentally end up > contradicting her, ignore me and trust her instead :) > > Victoria Dye <vdye@xxxxxxxxxx> writes: > > >> -test_expect_failure 'check-attr with pathspec outside sparse definition' ' > >> +test_expect_success 'check-attr with pathspec outside sparse definition' ' > > > > Re: my suggested change to the test in patch 1 [2], when I applied _this_ > > patch, the test still failed for the 'sparse-index' case. It doesn't seem to > > be a problem with your patch, but rather a bug in 'diff' that can be > > reproduced with this test (using the infrastructure in t1092): > > > > test_expect_failure 'diff --cached shows differences in sparse directory' ' > > init_repos && > > > > test_all_match git reset --soft update-folder1 && > > test_all_match git diff --cached -- folder1/a > > ' > > > > It's not immediately obvious to me what the problem is, but my guess is it's > > some mis-handling of sparse directories in the internal diff machinery. > > Given the likely complexity of the issue, I'd be content with you leaving > > the 'diff --check' test as 'test_expect_failure' with a note about the bug > > in 'diff' to fix later. Or, if you do want to investigate & fix it now, I > > wouldn't be opposed to that either. :) > > > > [2] https://lore.kernel.org/git/c3ebe3b4-88b9-8ca2-2ee3-39a3e0d82201@xxxxxxxxxx/ > > Because the 'diff --check' test is broken, and 'git check-attr' still > expands the index (as noted in the next patch), the code that implements > 'read from a blob if the .gitattributes is not in the index' is not > exercised by the tests in this patch (it gets exercised in the next > patch). IOW, you can remove this logic and the tests still pass, like > so: > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- > diff --git a/attr.c b/attr.c > index 1488b8e18a..abfa2078ac 100644 > --- a/attr.c > +++ b/attr.c > @@ -23,6 +23,7 @@ > #include "thread-utils.h" > #include "tree-walk.h" > #include "object-name.h" > +#include "trace2.h" > > const char git_attr__true[] = "(builtin)true"; > const char git_attr__false[] = "\0(builtin)false"; > @@ -847,9 +848,11 @@ static struct attr_stack *read_attr_from_index(struct index_state *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); > + /* relative_path = normalize_path + ce_namelen(istate->cache[pos]); */ > + /* stack = read_attr_from_blob(istate, &istate->cache[pos]->oid, relative_path, flags); */ > + trace2_printf("Tried to read from blob"); > } else { > + trace2_printf("Tried to read from index"); > buf = read_blob_data_from_index(istate, path, &size); > if (!buf) > return NULL; > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- > > If I were writing patches and encountered this situation, I would squash > patches 2-3/3 together since both are closely related and quite small, > but I'll leave the decision to you + other reviewers.
