Re: [PATCH v6 2/2] diff-files: integrate with sparse index

From: Victoria Dye <vdye@github.com>
Date: Mar 21, 2023, 22:34 UTC
Message-ID: <cea3c428-02e8-a50d-1211-e62f593dc0a1@github.com>
In-Reply-To: <20230320205241.105476-3-cheskaqiqi@gmail.com>

Shuqi Liang wrote:

Show 6 quoted lines

> Originally, diff-files a pathspec that is out-of-cone in a sparse-index
> environment, Git dies with "pathspec '<x>' did not match any files",
> mainly because it does not expand the index so nothing is matched.
> Expand the index when the <pathspec> needs an expanded index, i.e. the
> <pathspec> contains wildcard that may need a full-index or the
> <pathspec> is simply outside of sparse-checkout definition.

...

> +	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
> +		ensure_full_index(the_repository->index);

Looks good! I'm glad you were able to use the tests to confirm that this pathspec-based expansion was needed.

Show 40 quoted lines

> +		
>  	result = run_diff_files(&rev, options);
>  	result = diff_result_code(&rev.diffopt, result);
>  cleanup:
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 3242cfe91a..82751f2ca3 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -125,5 +125,7 @@ test_perf_on_all git checkout-index -f --all
>  test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
>  test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
>  test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
> +test_perf_on_all git diff-files
> +test_perf_on_all git diff-files $SPARSE_CONE/a
>  
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index c1329e2f16..6cbbc51a16 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2097,4 +2097,35 @@ test_expect_failure 'diff-files with pathspec outside sparse definition' '
>  	test_all_match git diff-files folder1/a
>  '
>  
> +test_expect_success 'diff-files pathspec expands index when necessary' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +	
> +	# pathspec that should expand index
> +	! ensure_not_expanded diff-files "*/a" &&
> +	test_must_be_empty sparse-index-err &&
> +
> +	! ensure_not_expanded diff-files "**a" &&
> +	test_must_be_empty sparse-index-err
> +'

Thanks for adding these, it's a good idea to show when the sparse index *is* expanded in addition to when it is not. However, checking that the 'sparse-index-err' is empty won't handle silent failures, so it's probably better to create an 'ensure_expanded' to mirror 'ensure_not_expanded'. The two functions could share pretty much all of their code except for the last line ('test_region ...').

Show 16 quoted lines

> +
> +test_expect_success 'sparse index is not expanded: diff-files' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +
> +	ensure_not_expanded diff-files &&
> +	ensure_not_expanded diff-files deep/a &&
> +	ensure_not_expanded diff-files deep/*
> +'
> +
>  test_done