Re: [PATCH v5 0/3] check-attr: integrate with sparse-index

[Victoria Dye <vdye@github.com>]

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