git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Victoria Dye <vdye@github.com>
Date
Mar 29, 2023, 17:49 UTC
Message-ID
<8cb2b7a2-78f2-9221-1234-62814557d2d3@github.com>
In-Reply-To
<pull.1480.v2.git.git.1680107154078.gitgitgadget@gmail.com>
Raghul Nanth A via GitGitGadget wrote:
> From: Raghul Nanth A <nanth.raghul@gmail.com>

Hello! Thanks for working on this patch, it's always nice to get more sparse index support. Since this is your first contribution to the mailing list, though, it's unclear to me whether you're working on this as an independent contributor or if you're interested in the Google Summer of Code project "More Sparse Index Integrations" [1]. If you're doing this for GSoC, could you please prefix the title of this patch with "[GSoC]" (as noted in the application details [2])?

On the topic of GSoC applications - if you are submitting this for GSoC, I'm a bit curious as to why you jumped right into sparse index rather than first submitting a microproject [3]. This is a good first pass at a sparse index integration, but the microproject is a better way to get acquainted with the conventions and requirements of contributing to Git, hence the strong recommendation to complete one first.

[1] https://git.github.io/SoC-2023-Ideas/ [2] https://git.github.io/General-Application-Information/ [3] https://git.github.io/General-Microproject-Information/

Show 15 quoted lines
> 
> Add usage and performance tests for describe
> 
> Performance metrics
> 
>   Test                                                     HEAD~1            HEAD
>   -------------------------------------------------------------------------------------------------
>   2000.2: git describe --dirty (full-v3)                   0.08(0.09+0.01)   0.08(0.06+0.03) +0.0%
>   2000.3: git describe --dirty (full-v4)                   0.09(0.07+0.03)   0.08(0.05+0.04) -11.1%
>   2000.4: git describe --dirty (sparse-v3)                 0.88(0.82+0.06)   0.02(0.01+0.05) -97.7%
>   2000.5: git describe --dirty (sparse-v4)                 0.68(0.60+0.08)   0.02(0.02+0.04) -97.1%
>   2000.6: echo >>new && git describe --dirty (full-v3)     0.08(0.04+0.05)   0.08(0.05+0.04) +0.0%
>   2000.7: echo >>new && git describe --dirty (full-v4)     0.08(0.07+0.03)   0.08(0.05+0.04) +0.0%
>   2000.8: echo >>new && git describe --dirty (sparse-v3)   0.75(0.69+0.07)   0.02(0.03+0.03) -97.3%
>   2000.9: echo >>new && git describe --dirty (sparse-v4)   0.81(0.73+0.09)   0.02(0.01+0.05) -97.5%

As Junio noted [4], this description doesn't provide much information to a reader. Commit messages should (at a minimum) explain what the code change in a commit does, as well as why it's necessary. 

[4] https://lore.kernel.org/git/xmqq355nbii5.fsf@gitster.g/
Show 30 quoted lines
> 
> Signed-off-by: Raghul Nanth A <nanth.raghul@gmail.com>
> ---
>     describe: enable sparse index for describe
>     
>      * Removed describe tests not concerned with sparse index
>     
>      * Added performance metric to commit message
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1480%2FNanthR%2Fdescribe-sparse-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1480/NanthR/describe-sparse-v2
> Pull-Request: https://github.com/git/git/pull/1480
> 
>  builtin/describe.c                       |  2 +
>  t/perf/p2000-sparse-operations.sh        |  3 ++
>  t/t1092-sparse-checkout-compatibility.sh | 11 ++++
>  t/t6121-describe-sparse.sh               | 67 ++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
>  create mode 100755 t/t6121-describe-sparse.sh
> 
> 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;
Looks good.
Show 12 quoted lines
>  			repo_read_index(the_repository);
>  			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,
>  				      NULL, NULL, NULL);
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 3242cfe91a0..db7887470f9 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -43,6 +43,7 @@ test_expect_success 'setup repo and indexes' '
>  	done &&
>  
>  	git sparse-checkout init --cone &&
> +	git tag -a v1.0 -m "Final" &&

This isn't disruptive to the existing performance tests, but allows you to easily test 'git describe'. Nice!

Show 23 quoted lines
>  	git sparse-checkout set $SPARSE_CONE &&
>  	git checkout -b wide $OLD_COMMIT &&
>  
> @@ -125,5 +126,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
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e1..9a4db09178f 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1514,6 +1514,17 @@ test_expect_success 'sparse-index is not expanded: stash' '
>  	ensure_not_expanded stash pop
>  '
>  
> +test_expect_success 'sparse-index is not expanded: describe' '
> +	init_repos &&
> +	# Add tag to be read by describe
> +	ensure_not_expanded tag -a v1.0 -m "Version 1" &&

The test you've added isn't verifying the sparse index compatibility of 'git tag' (which doesn't use the index at all, IIRC), so 'ensure_not_expanded' isn't needed on this line. You should use 'git -C sparse-index tag ...' to perform the action instead.

Show 15 quoted lines
> +	ensure_not_expanded describe --dirty &&
> +	ensure_not_expanded describe &&
> +	echo "test" >>sparse-index/extra.txt &&
> +	ensure_not_expanded describe --dirty &&
> +	ensure_not_expanded describe
> +'
> +
>  test_expect_success 'sparse index is not expanded: diff' '
>  	init_repos &&
>  
> diff --git a/t/t6121-describe-sparse.sh b/t/t6121-describe-sparse.sh
> new file mode 100755
> index 00000000000..ce53603c387
> --- /dev/null
> +++ b/t/t6121-describe-sparse.sh

Is there a specific reason you've created a new test file instead of adding the tests to 't1092-sparse-checkout-compatibility.sh'? Historically, we've used 't1092' for validating both the functional correctness of a command with sparse index enable and the preservation of a sparse index with 'ensure_not_expanded'; you've done the latter in 't1092', but put the former in this new 't6121'. 't1092' also uses a more "interesting" test repo & includes comparison functions for full checkout/sparse-checkout/sparse index, so the tests added for 'git describe' can be more thorough.

Show 70 quoted lines
> @@ -0,0 +1,67 @@
> +#!/bin/sh
> +
> +test_description='git describe in sparse checked out trees'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +check_describe () {
> +	indir= &&
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		-C)
> +			indir="$2"
> +			shift
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done &&
> +	indir=${indir:+"$indir"/} &&
> +	expect="$1"
> +	shift
> +	describe_opts="$@"
> +	test_expect_success "describe $describe_opts" '
> +		git ${indir:+ -C "$indir"} describe $describe_opts >actual &&
> +		echo "$expect" >expect &&
> +		test_cmp expect actual
> +	'
> +}
> +
> +test_expect_success setup '
> +	test_commit initial file one &&
> +	test_commit --annotate A file A &&
> +
> +	test_tick &&
> +
> +	git sparse-checkout init --cone
> +'
> +
> +check_describe A HEAD
> +
> +test_expect_success 'describe --dirty with --work-tree' '
> +	(
> +		cd "$TEST_DIRECTORY" &&
> +		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
> +	) &&
> +	grep "A" out
> +'
> +
> +test_expect_success 'set-up dirty work tree' '
> +	echo >>file
> +'
> +
> +test_expect_success 'describe --dirty with --work-tree (dirty)' '
> +	git describe --dirty >expected &&
> +	(
> +		cd "$TEST_DIRECTORY" &&
> +		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
> +	) &&
> +	test_cmp expected out
> +'
> +test_done
> 
> base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
Previous: Junio C HamanoNext: Junio C Hamano
Message 8 of 21 in “describe: enable sparse index for describe”
  1. describe: enable sparse index for describeRaghul Nanth A via GitGitGadget, Mar 27, 2023
  2. Junio C HamanoMar 27, 2023
  3. Derrick StoleeMar 28, 2023
  4. Junio C HamanoMar 28, 2023
  5. Derrick StoleeMar 28, 2023
  6. describe: enable sparse index for describeRaghul Nanth A via GitGitGadget, Mar 29, 2023
  7. Junio C HamanoMar 29, 2023
  8. Victoria DyeMar 29, 2023
  9. Junio C HamanoMar 29, 2023
  10. Raghul NanthMar 30, 2023
  11. Victoria DyeApr 3, 2023
  12. describe: enable sparse index for describeRaghul Nanth A via GitGitGadget, Mar 30, 2023
  13. Junio C HamanoMar 30, 2023
  14. Junio C HamanoMar 30, 2023
  15. Victoria DyeMar 30, 2023
  16. [GSOC][PATCH] describe: enable sparse index for describeRaghul Nanth A, Mar 31, 2023
  17. Junio C HamanoMar 31, 2023
  18. [GSOC][PATCH v4] describe: enable sparse index for describeRaghul Nanth A, Mar 31, 2023
  19. Victoria DyeApr 3, 2023
  20. [GSOC][PATCH v5] describe: enable sparse index for describeRaghul Nanth A, Apr 3, 2023
  21. describe: enable sparse index for describeRaghul Nanth A, Apr 3, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.