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

From: Victoria Dye <vdye@github.com>
Date: Aug 14, 2023, 16:24 UTC
Message-ID: <3b2a5b4b-ab8f-746b-6b69-8e8262b6390b@github.com>
In-Reply-To: <20230811142211.4547-1-cheskaqiqi@gmail.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:

Show 25 quoted lines

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

Show 6 quoted lines

> 
> 
> Shuqi Liang (3):
>   t1092: add tests for 'git check-attr'
>   attr.c: read attributes in a sparse directory
>   check-attr: integrate with sparse-index