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

From: Victoria Dye <vdye@github.com>
Date: Jul 3, 2023, 18:21 UTC
Message-ID: <fb4bad5b-6e49-dbad-d9ae-94ec7db9f93c@github.com>
In-Reply-To: <20230701064843.147496-4-cheskaqiqi@gmail.com>

Shuqi Liang wrote:

Show 14 quoted lines

> Set the requires-full-index to false for "diff-tree".
> 
> Add test to ensure the index is not expanded when the
> sparse index is enabled.
> 
> The `p2000` tests demonstrate a ~63% execution time reduction for
> 'git check-attr' using a sparse index.
> 
> Test                                            before  after
> -----------------------------------------------------------------------
> 2000.106: git check-attr -a f2/f4/a (full-v3)    0.05   0.05 +0.0%
> 2000.107: git check-attr -a f2/f4/a (full-v4)    0.05   0.05 +0.0%
> 2000.108: git check-attr -a f2/f4/a (sparse-v3)  0.04   0.02 -50.0%
> 2000.109: git check-attr -a f2/f4/a (sparse-v4)  0.04   0.01 -75.0%

Great results as usual!

Show 6 quoted lines

> 
> Helped-by: Victoria Dye <vdye@github.com>
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  builtin/check-attr.c                     |  3 +++
>  t/t1092-sparse-checkout-compatibility.sh | 11 +++++++++++

Did you forget to add the perf test to this patch?

Show 12 quoted lines

>  2 files changed, 14 insertions(+)
> 
> diff --git a/builtin/check-attr.c b/builtin/check-attr.c
> index b22ff748c3..02267f9bc1 100644
> --- a/builtin/check-attr.c
> +++ b/builtin/check-attr.c
> @@ -122,6 +122,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, check_attr_options,
>  			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;

Given that you updated 'read_attr_from_index()' to handle sparse directories in [1], it makes sense that disabling 'command_requires_full_index' is all that's needed to enable the sparse index here.

[1] https://lore.kernel.org/git/20230701064843.147496-2-cheskaqiqi@gmail.com/

Show 17 quoted lines

> +	
>  	if (repo_read_index(the_repository) < 0) {
>  		die("invalid cache");
>  	}
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 4edfa3c168..317ccc8ec5 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2288,4 +2288,15 @@ test_expect_success 'check-attr with pathspec outside sparse definition' '
>  	test_all_match git check-attr  -a --cached -- folder1/a
>  '
>  
> +test_expect_success 'sparse-index is not expanded: check-attr' '
> +	init_repos &&
> +
> +	echo "a -crlf myAttr" >>.gitattributes &&
> +	run_on_all cp ../.gitattributes ./deep &&

nit: we're only verifying behavior in 'sparse-index', so this should probably be

	cp .gitattributes ./sparse-index/deep &&

> +
> +	ensure_not_expanded check-attr -a -- deep/a &&
> +	run_on_all git add deep/.gitattributes &&

Similar to above, this should probably be:

	git -C sparse-index add deep/.gitattributes &&

> +	ensure_not_expanded check-attr -a --cached -- deep/a

It'd be nice to show that the index is also not expanded files outside of the sparse-checkout cone, e.g. 'folder1/.gitattributes' or 'folder1/0/.gitattributes' (similar to what you did for the correctness tests in [2]).

[2] https://lore.kernel.org/git/20230701064843.147496-3-cheskaqiqi@gmail.com/

> +'
> +
>  test_done