Re: [PATCH v4 2/3] attr.c: read attributes in a sparse directory

>
> 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@github.com> 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@github.com/
>
> 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.