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

From: Glen Choo <chooglen@google.com>
Date: Aug 3, 2023, 16:22 UTC
Message-ID: <kl6la5v82izn.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <5e478d8b-9ef4-864b-41e4-e0a79877d278@github.com>

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:

Show 23 quoted lines

>> -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.