Subject: Re: [PATCH] describe: enable sparse index for describe

From: Junio C Hamano <[email-masked]>
Date: Mon, 27 Mar 2023 11:26:07 -0700

"Raghul Nanth A via GitGitGadget" <[email-masked]> writes:

>  builtin/describe.c                       |   2 +
>  t/perf/p2000-sparse-operations.sh        |  14 +-
>  t/t1092-sparse-checkout-compatibility.sh |  10 +
>  t/t6121-describe-sparse.sh               | 675 +++++++++++++++++++++++
>  4 files changed, 697 insertions(+), 4 deletions(-)
>  create mode 100755 t/t6121-describe-sparse.sh

This copying of a file with 600+ lines only to touch up a handful
lines (like a 20+ lines patch) is almost criminal.  Imagine the
effort to keep them in sync over time, when "describe" itself may
learn new features and improved output, independent from the
sparse-index compatibility.

Can't we do better than this with a bit of refactoring?

> diff --git a/builtin/describe.c b/builtin/describe.c
> index 5b5930f5c8c..7ff9b5e4b20 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -654,6 +654,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  			int fd, result;
>  
>  			setup_work_tree();
> +			prepare_repo_settings(the_repository);
> +			the_repository->settings.command_requires_full_index = 0;

Offhand, the only case I know that "describe" even _needs_ a working
tree or the index is when asked to do the "--dirty" thing.  To
figure out if the working tree files are modified, the code calls
into run_diff_index(), but has that codepath been made sparse-index
aware already?

> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 3242cfe91a0..a8a9ed79441 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -59,7 +59,8 @@ test_expect_success 'setup repo and indexes' '
>  		git sparse-checkout set $SPARSE_CONE &&
>  		git config index.version 3 &&
>  		git update-index --index-version=3 &&
> -		git checkout HEAD~4
> +		git checkout HEAD~4 &&
> +		git tag -a v1.0 -m "Final"
>  	) &&
>  	git -c core.sparseCheckoutCone=true clone --branch=wide --sparse . full-v4 &&
>  	(
> @@ -68,7 +69,8 @@ test_expect_success 'setup repo and indexes' '
>  		git sparse-checkout set $SPARSE_CONE &&
>  		git config index.version 4 &&
>  		git update-index --index-version=4 &&
> -		git checkout HEAD~4
> +		git checkout HEAD~4 &&
> +		git tag -a v1.0 -m "Final"
>  	) &&
>  	git -c core.sparseCheckoutCone=true clone --branch=wide --sparse . sparse-v3 &&
>  	(
> @@ -77,7 +79,8 @@ test_expect_success 'setup repo and indexes' '
>  		git sparse-checkout set $SPARSE_CONE &&
>  		git config index.version 3 &&
>  		git update-index --index-version=3 &&
> -		git checkout HEAD~4
> +		git checkout HEAD~4 &&
> +		git tag -a v1.0 -m "Final"
>  	) &&
>  	git -c core.sparseCheckoutCone=true clone --branch=wide --sparse . sparse-v4 &&
>  	(
> @@ -86,7 +89,8 @@ test_expect_success 'setup repo and indexes' '
>  		git sparse-checkout set $SPARSE_CONE &&
>  		git config index.version 4 &&
>  		git update-index --index-version=4 &&
> -		git checkout HEAD~4
> +		git checkout HEAD~4 &&
> +		git tag -a v1.0 -m "Final"
>  	)
>  '

It is unclear from the proposed commit log what the relevance of
adding a step to create an annotated tag to these tests.  It is not
like any later step uses that tag to figure out anything.  There may
be good reasons to add these tags (otherwise you would not be adding
them to these tests), but please explain why in the proposed log
message so that future readers of the "git log -p" do not have to
ask this question.

> @@ -125,5 +129,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 describe --dirty
> +test_perf_on_all 'echo >> new && git describe --dirty'
>  
>  test_done

Just like '>', '>>' is a rediraction operator and should have SP
before it (you got it right) and no SP between it and its operand.
I.e.

	echo >>new && git describe --dirty

You have the same in t1092, I think.