[v3,2/2] Mark 'git cat-file' sparse-index compatible

Commit Message

Kevin Lyles Sept. 3, 2024, 5:54 p.m. UTC

From: Kevin Lyles <[e-mail]>

This change affects how 'git cat-file' works with the index when
specifying an object with the ":<path>" syntax (which will give file
contents from the index).

'git cat-file' will expand a sparse index to a full index when needed,
but is currently marked as needing a full index (or rather, not marked
as not needing a full index). This results in much slower 'git cat-file'
operations when working within the sparse index, since we expand the
index whether it's needed or not.

Mark 'git cat-file' as not needing a full index, so that you only pay
the cost of expanding the sparse index to a full index when you request
a file outside of the sparse index.

Add tests to ensure both that:
- 'git cat-file' returns the correct file contents whether or not the
  file is in the sparse index
- 'git cat-file' expands to the full index any time you request
  something outside of the sparse index

Signed-off-by: Kevin Lyles <[e-mail]>
---
 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

Comments

Junio C Hamano Sept. 3, 2024, 7:19 p.m. UTC | #1

"Kevin Lyles via GitGitGadget" <[e-mail]> writes:

> Subject: Re: [PATCH v3 2/2] Mark 'git cat-file' sparse-index compatible

The same comment on the commit title applies here.

> From: Kevin Lyles <[e-mail]>
>
> This change affects how 'git cat-file' works with the index when

Again, we start by describing the status quo (e.g. "'git cat-file'
always expands a sparse-index to a full index"), explaining why it
is undesirable, and hinting what you want to do about it.

And then give an order to the codebase to "become like so".

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 18fe58d6b8b..1afdfb5cbae 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -1047,6 +1047,9 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>  	if (batch.buffer_output < 0)
>  		batch.buffer_output = batch.all_objects;
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;
> +
>  	/* Return early if we're in batch mode? */
>  	if (batch.enabled) {
>  		if (opt_cw)

How should the correctness of a change line this validated, by the
way?  By following manually all the code paths from this point and
making sure that the access to an element (or size) of the index
that is sparsed out is preceded by a lazy rehydration of a tree that
represents a subhierarchy in the sparse-index?  Addition to end-to-end
tests may increase the test coverage, but I am not sure how to ensure
the coverage is exhaustive.

Thanks.

Patch

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 18fe58d6b8b..1afdfb5cbae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -1047,6 +1047,9 @@  int cmd_cat_file(int argc, const char **argv, const char *prefix)
 	if (batch.buffer_output < 0)
 		batch.buffer_output = batch.all_objects;
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	/* Return early if we're in batch mode? */
 	if (batch.enabled) {
 		if (opt_cw)
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 87953abf872..fbc10c8042b 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2358,4 +2358,40 @@  test_expect_success 'advice.sparseIndexExpanded' '
 	grep "The sparse index is expanding to a full index" err
 '
 
+test_expect_success 'cat-file -p' '
+	init_repos &&
+	echo "new content" >>full-checkout/deep/a &&
+	echo "new content" >>sparse-checkout/deep/a &&
+	echo "new content" >>sparse-index/deep/a &&
+	run_on_all git add deep/a &&
+
+	test_all_match git cat-file -p :deep/a &&
+	ensure_not_expanded cat-file -p :deep/a &&
+	test_all_match git cat-file -p :folder1/a &&
+	ensure_expanded cat-file -p :folder1/a
+'
+
+test_expect_success 'cat-file --batch' '
+	init_repos &&
+	echo "new content" >>full-checkout/deep/a &&
+	echo "new content" >>sparse-checkout/deep/a &&
+	echo "new content" >>sparse-index/deep/a &&
+	run_on_all git add deep/a &&
+
+	echo ":deep/a" >in &&
+	test_all_match git cat-file --batch <in &&
+	ensure_not_expanded cat-file --batch <in &&
+
+	echo ":folder1/a" >in &&
+	test_all_match git cat-file --batch <in &&
+	ensure_expanded cat-file --batch <in &&
+
+	cat >in <<-\EOF &&
+	:deep/a
+	:folder1/a
+	EOF
+	test_all_match git cat-file --batch <in &&
+	ensure_expanded cat-file --batch <in
+'
+
 test_done