Mark `cat-file` sparse-index compatible

From: Kevin Lyles <[email protected]>

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 cat-file operations when working within the sparse index, since we expand the index whether it's needed or not.

Mark 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:
- cat-file returns the correct file contents whether or not the file is in the sparse index
- cat-file warns about expanding to the full index any time you request something outside of the sparse index

---
Mark cat-file sparse-index compatible

Please note that this is my first contribution to git. I've tried to follow the instructions about how to correctly submit a patch (I'm using GitGitGadget as getting Outlook to do plain text e-mail correctly seems impossible), but please let me know if I've missed something.

I've worded the commit message itself as though I'm definitely correct about how cat-file behaves, since I assume we want the final commit message to be definite. However, this change felt a little too easy and I can't help but feel that I might have missed something. So, even though this is just one commit, I'm also including this cover letter going into more detail about the parts that don't need to be part of the commit history.

My motivation for making this change is purely performance. We have a large repository that we enable the sparse index for, and I am developing a pre-commit hook that (among other things) uses git cat-file to get the staged contents of certain files. Without this change, getting the contents of a single small file from the index can take upwards of 10 seconds due to the index expansion. After this change, it only takes ~0.3 seconds unless the file is outside of the sparse index.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v1
Pull-Request: https://github.com/git/git/pull/1770

 builtin/cat-file.c                       |  3 +
 t/t1092-sparse-checkout-compatibility.sh | 71 ++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 5 deletions(-)

index 18fe58d6b8b..1afdfb5cbae 100644
@@ -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)

base-commit: 80ccd8a2602820fdf896a8e8894305225f86f61d

gitgitgadget

Hi, Kevin! Thanks for looking into this.

hyper-nit: For commit messages we would usually use 'git cat-file'. (This recommends both using the full command and single ticks instead of back-ticks.)

I was surprised at first that 'git cat-file' would need it at all, but it is important when referencing objects with ":<path>" as that refers to a path in the index instead of at any revision. It could be helpful to bring this observation to the forefront of your commit message.

The "ensure_expanded" test works without the advice warning about the larger request. It uses the trace2 mechanism to look for the expansion happening under the hood.

(In fact, when I contributed the advice message, it became disabled in these tests using config values during setup.)

index 1954bf6332d..9902190eec4 100755
@@ -180,22 +180,24 @@ init_repos_as_submodules () {
 }
 
 run_on_sparse () {
+	cat >in &&
 	(
 		cd sparse-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
-	) &&
+	) <in &&
 	(
 		cd sparse-index &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
-	)
+	) <in
 }
 
 run_on_all () {
+	cat >in &&
 	(
 		cd full-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
-	) &&
-	run_on_sparse "$@"
+	) <in &&
+	run_on_sparse "$@" <in
 }
 
 test_all_match () {

	GIT_ADVICE=0 test_all_match git cat-file -p :folder1/a &&

	cat >in <<-\EOF &&
	HEAD:deep/a
	:deep/a
	EOF

	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 &&

Please note that this is my first contribution to git. I've tried to follow the instructions about how to correctly submit a patch (I'm using GitGitGadget as getting Outlook to do plain text e-mail correctly seems impossible), but please let me know if I've missed something.

My motivation for making this change is purely performance. We have a large repository that we enable the sparse index for, and I am developing a pre-commit hook that (among other things) uses git cat-file to get the staged contents of certain files. Without this change, getting the contents of a single small file from the index can take upwards of 10 seconds due to the index expansion. After this change, it only takes ~0.3 seconds unless the file is outside of the sparse index.

Kevin Lyles (2):
Allow using stdin in run_on_* functions
Mark 'git cat-file' sparse-index compatible

 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 48 +++++++++++++++++++++---
 2 files changed, 46 insertions(+), 5 deletions(-)

base-commit: 80ccd8a2602820fdf896a8e8894305225f86f61d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v2
Pull-Request: https://github.com/git/git/pull/1770

Range-diff vs v1:

 -:  ----------- > 1:  7067a4c5da2 Allow using stdin in run_on_* functions
 1:  60c15dd246a ! 2:  a92825e502f Mark `cat-file` sparse-index compatible
     @@ Metadata
      Author: Kevin Lyles <[email protected]>
      
       ## Commit message ##
     -    Mark `cat-file` sparse-index compatible
     +    Mark 'git cat-file' sparse-index compatible
      
     -    `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 `cat-file`
     +    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 `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.
     +    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:
     -    - `cat-file` returns the correct file contents whether or not the file
     -      is in the sparse index
     -    - `cat-file` warns about expanding to the full index any time you
     -      request something outside of the sparse index
     +    - '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 <[email protected]>
      
     @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr
       		if (opt_cw)
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
     -@@ t/t1092-sparse-checkout-compatibility.sh: init_repos_as_submodules () {
     - }
     - 
     - run_on_sparse () {
     -+	tee run_on_sparse-checkout run_on_sparse-index &&
     -+
     - 	(
     - 		cd sparse-checkout &&
     - 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
     --	) &&
     -+	) <run_on_sparse-checkout &&
     - 	(
     - 		cd sparse-index &&
     - 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
     --	)
     -+	) <run_on_sparse-index
     - }
     - 
     - run_on_all () {
     -+	tee run_on_all-full run_on_all-sparse &&
     -+
     - 	(
     - 		cd full-checkout &&
     - 		GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
     --	) &&
     --	run_on_sparse "$@"
     -+	) <run_on_all-full &&
     -+	run_on_sparse "$@" <run_on_all-sparse
     - }
     - 
     - test_all_match () {
     -@@ t/t1092-sparse-checkout-compatibility.sh: test_sparse_unstaged () {
     - 	done
     - }
     - 
     --# Usage: test_sprase_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
     -+# Usage: test_sparse_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
     - # Verifies that "git sparse-checkout set <c1> ... <cN>" succeeds and
     - # leaves the sparse index in a state where <s1> ... <sM> are sparse
     - # directories (and <c1> ... <cN> are not).
      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseIndexExpanded' '
       	grep "The sparse index is expanding to a full index" err
       '
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde
      +	echo "new content" >>sparse-index/deep/a &&
      +	run_on_all git add deep/a &&
      +
     -+	test_all_match git cat-file -p HEAD:deep/a &&
     -+	ensure_not_expanded cat-file -p HEAD:deep/a &&
     -+	test_all_match git cat-file -p HEAD:folder1/a &&
     -+	ensure_not_expanded cat-file -p HEAD:folder1/a &&
     -+
      +	test_all_match git cat-file -p :deep/a &&
      +	ensure_not_expanded cat-file -p :deep/a &&
     -+	run_on_all git cat-file -p :folder1/a &&
     -+	test_cmp full-checkout-out sparse-checkout-out &&
     -+	test_cmp full-checkout-out sparse-index-out &&
     -+	test_cmp full-checkout-err sparse-checkout-err &&
     ++	test_all_match git cat-file -p :folder1/a &&
      +	ensure_expanded cat-file -p :folder1/a'
      +
      +test_expect_success 'cat-file --batch' '
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde
      +	echo "new content" >>sparse-index/deep/a &&
      +	run_on_all git add deep/a &&
      +
     -+	cat <<-\EOF | test_all_match git cat-file --batch &&
     -+	HEAD:deep/a
     -+	:deep/a
     -+	EOF
     -+	cat <<-\EOF | ensure_not_expanded cat-file --batch &&
     -+	HEAD:deep/a
     -+	:deep/a
     -+	EOF
     ++	echo ":deep/a">in &&
     ++	test_all_match git cat-file --batch <in &&
     ++	ensure_not_expanded cat-file --batch <in &&
      +
     -+	echo "HEAD:folder1/a" | test_all_match git cat-file --batch &&
     -+	echo "HEAD:folder1/a" | ensure_not_expanded cat-file --batch &&
     ++	echo ":folder1/a">in &&
     ++	test_all_match git cat-file --batch <in &&
     ++	ensure_expanded cat-file --batch <in &&
      +
     -+	echo ":folder1/a" | run_on_all git cat-file --batch &&
     -+	test_cmp full-checkout-out sparse-checkout-out &&
     -+	test_cmp full-checkout-out sparse-index-out &&
     -+	test_cmp full-checkout-err sparse-checkout-err &&
     -+	echo ":folder1/a" | ensure_expanded cat-file --batch &&
     -+
     -+	cat <<-\EOF | run_on_all git cat-file --batch &&
     ++	cat <<-\EOF >in &&
      +	:deep/a
      +	:folder1/a
      +	EOF
     -+	test_cmp full-checkout-out sparse-checkout-out &&
     -+	test_cmp full-checkout-out sparse-index-out &&
     -+	test_cmp full-checkout-err sparse-checkout-err &&
     -+	cat <<-\EOF | ensure_expanded cat-file --batch
     -+	:deep/a
     -+	:folder1/a
     -+	EOF'
     ++	test_all_match git cat-file --batch <in &&
     ++	ensure_expanded cat-file --batch <in'
      +
       test_done

gitgitgadget

From: Kevin Lyles <[email protected]>

The 'run_on_sparse' and 'run_on_all' functions previously did not work correctly for commands accepting standard input. This also indirectly affected 'test_all_match' and 'test_sparse_match'.

Now, we accept standard input and will send it to each command that we run. This does not impact using the functions for commands that don't need standard input.

---
 t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index a2c0e1b4dcc..b140b642b21 100755
@@ -179,22 +179,26 @@ init_repos_as_submodules () {
 }
 
 run_on_sparse () {
+	cat >run_on_sparse-input &&
+
 	(
 		cd sparse-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
-	) &&
+	) <run_on_sparse-input &&
 	(
 		cd sparse-index &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
-	)
+	) <run_on_sparse-input
 }
 
 run_on_all () {
+	cat >run_on_all-input &&
+
 	(
 		cd full-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
-	) &&
-	run_on_sparse "$@"
+	) <run_on_all-input &&
+	run_on_sparse "$@" <run_on_all-input
 }
 
 test_all_match () {

@@ -221,7 +225,7 @@ test_sparse_unstaged () {
 	done
 }
 
-# Usage: test_sprase_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
+# Usage: test_sparse_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
 # Verifies that "git sparse-checkout set <c1> ... <cN>" succeeds and
 # leaves the sparse index in a state where <s1> ... <sM> are sparse
 # directories (and <c1> ... <cN> are not).

gitgitgadget

From: Kevin Lyles <[email protected]>

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

---
 builtin/cat-file.c                       |  3 +++
 t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

index 18fe58d6b8b..1afdfb5cbae 100644
@@ -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)

index b140b642b21..4d025c5f8d6 100755
@@ -2349,4 +2349,38 @@ 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 <<-\EOF >in &&
+	:deep/a
+	:folder1/a
+	EOF
+	test_all_match git cat-file --batch <in &&
+	ensure_expanded cat-file --batch <in'
+
 test_done

gitgitgadget

Yup. It seems that the patches are getting almost ready?

Thanks for a detailed review.

Please note that this is my first contribution to git. I've tried to follow the instructions about how to correctly submit a patch (I'm using GitGitGadget as getting Outlook to do plain text e-mail correctly seems impossible), but please let me know if I've missed something.

My motivation for making this change is purely performance. We have a large repository that we enable the sparse index for, and I am developing a pre-commit hook that (among other things) uses git cat-file to get the staged contents of certain files. Without this change, getting the contents of a single small file from the index can take upwards of 10 seconds due to the index expansion. After this change, it only takes ~0.3 seconds unless the file is outside of the sparse index.

Kevin Lyles (2):
Allow using stdin in run_on_* functions
Mark 'git cat-file' sparse-index compatible

 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 50 +++++++++++++++++++++---
 2 files changed, 48 insertions(+), 5 deletions(-)

base-commit: 4590f2e9412378c61eac95966709c78766d326ba
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v3
Pull-Request: https://github.com/git/git/pull/1770

Range-diff vs v2:

 1:  7067a4c5da2 = 1:  b310593aec2 Allow using stdin in run_on_* functions
 2:  a92825e502f ! 2:  f4d1461b993 Mark 'git cat-file' sparse-index compatible
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde
      +	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'
     ++	ensure_expanded cat-file -p :folder1/a
     ++'
      +
      +test_expect_success 'cat-file --batch' '
      +	init_repos &&
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'advice.sparseInde
      +	echo "new content" >>sparse-index/deep/a &&
      +	run_on_all git add deep/a &&
      +
     -+	echo ":deep/a">in &&
     ++	echo ":deep/a" >in &&
      +	test_all_match git cat-file --batch <in &&
      +	ensure_not_expanded cat-file --batch <in &&
      +
     -+	echo ":folder1/a">in &&
     ++	echo ":folder1/a" >in &&
      +	test_all_match git cat-file --batch <in &&
      +	ensure_expanded cat-file --batch <in &&
      +
     -+	cat <<-\EOF >in &&
     ++	cat >in <<-\EOF &&
      +	:deep/a
      +	:folder1/a
      +	EOF
      +	test_all_match git cat-file --batch <in &&
     -+	ensure_expanded cat-file --batch <in'
     ++	ensure_expanded cat-file --batch <in
     ++'
      +
       test_done

gitgitgadget

From: Kevin Lyles <[email protected]>

The 'run_on_sparse' and 'run_on_all' functions previously did not work correctly for commands accepting standard input. This also indirectly affected 'test_all_match' and 'test_sparse_match'.

Now, we accept standard input and will send it to each command that we run. This does not impact using the functions for commands that don't need standard input.

---
 t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index 6fa7f5e9587..87953abf872 100755
@@ -179,22 +179,26 @@ init_repos_as_submodules () {
 }
 
 run_on_sparse () {
+	cat >run_on_sparse-input &&
+
 	(
 		cd sparse-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
-	) &&
+	) <run_on_sparse-input &&
 	(
 		cd sparse-index &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
-	)
+	) <run_on_sparse-input
 }
 
 run_on_all () {
+	cat >run_on_all-input &&
+
 	(
 		cd full-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
-	) &&
-	run_on_sparse "$@"
+	) <run_on_all-input &&
+	run_on_sparse "$@" <run_on_all-input
 }
 
 test_all_match () {

@@ -221,7 +225,7 @@ test_sparse_unstaged () {
 	done
 }
 
-# Usage: test_sprase_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
+# Usage: test_sparse_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
 # Verifies that "git sparse-checkout set <c1> ... <cN>" succeeds and
 # leaves the sparse index in a state where <s1> ... <sM> are sparse
 # directories (and <c1> ... <cN> are not).

gitgitgadget

From: Kevin Lyles <[email protected]>

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

---
 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

index 18fe58d6b8b..1afdfb5cbae 100644
@@ -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)

index 87953abf872..fbc10c8042b 100755
@@ -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

gitgitgadget

Imagine that these two commits are applied and appear among hundreds of other commits in the list of contributions. Would this commit title blend in and belong to others?

    $ git log --oneline --no-merges -100

cf. Documentation/SubmittingPatches:describe-changes.

At least, it should somehow hint that the patch is about updating the t1092 test script.

Subject: t1092: allow run_on_* functions to use standard input

or something.

Our convention is to first describe the status quo in the present tense, so "previously did not" -> "do not".

It would be helpful to explain why they do not work, perhaps like so:

... functions do not work for commands that consume their standard input, because they attempt to run the same command in multiple repositories that are set up differently to inspect their behaviour differences.

"affected" -> "affects".

And after presenting the observation on the status quo and pointing out the issue we are going to address, it is our convention to write what to do in imperative mood, as if we are ordering the codebase to "become like so". E.g.

To allow these commands to consume the standard input, first slurp the contents fed from the standard input to a temporary file, and then run each attempt with its standard input redirected from the temporary file. This way, these multiple attempts will all see the same contents from their standard input.

Note that a command that does not read from the standard input does not have to redirect its standard input from </dev/null when using these run_on_* helper functions, as the standard input of the test environment is already connected to /dev/null.

or something, perhaps.

The same comment on the commit title applies here.

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".

I believe I've addressed all issues raised except the question about how to exhaustively validate the changes. I was under the impression that the combination of the existing 'git cat-file' test cases and my new cases were enough, but I have thus far been unable to get code coverage working locally to validate that myself, due to spurious failures in other tests. I've kicked off another run with most unrelated tests removed to see if that helps.

Please note that this is my first contribution to git. I've tried to follow the instructions about how to correctly submit a patch (I'm using GitGitGadget as getting Outlook to do plain text e-mail correctly seems impossible), but please let me know if I've missed something.

My motivation for making this change is purely performance. We have a large repository that we enable the sparse index for, and I am developing a pre-commit hook that (among other things) uses git cat-file to get the staged contents of certain files. Without this change, getting the contents of a single small file from the index can take upwards of 10 seconds due to the index expansion. After this change, it only takes ~0.3 seconds unless the file is outside of the sparse index.

Kevin Lyles (2):
t1092: allow run_on_* functions to use standard input builtin/cat-file: mark 'git cat-file' sparse-index compatible

 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 50 +++++++++++++++++++++---
 2 files changed, 48 insertions(+), 5 deletions(-)

base-commit: 4590f2e9412378c61eac95966709c78766d326ba
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1770%2Fklylesatepic%2Fkl%2Fmark-cat-file-sparse-index-compatible-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1770/klylesatepic/kl/mark-cat-file-sparse-index-compatible-v4
Pull-Request: https://github.com/git/git/pull/1770

Range-diff vs v3:

1: b310593aec2 ! 1: 73fe71abcd5 Allow using stdin in run_on_* functions

     @@ Metadata
      Author: Kevin Lyles <[email protected]>
      
       ## Commit message ##
     -    Allow using stdin in run_on_* functions
     +    t1092: allow run_on_* functions to use standard input
      
     -    The 'run_on_sparse' and 'run_on_all' functions previously did not work
     -    correctly for commands accepting standard input. This also indirectly
     -    affected 'test_all_match' and 'test_sparse_match'.
     +    The 'run_on_sparse' and 'run_on_all' functions do not work correctly for
     +    commands accepting standard input, because they run the same command
     +    multiple times and the first instance consumes it. This also indirectly
     +    affects 'test_all_match' and 'test_sparse_match'.
      
     -    Now, we accept standard input and will send it to each command that we
     -    run. This does not impact using the functions for commands that don't
     -    need standard input.
     +    To allow these functions to work with commands accepting standard input,
     +    first slurp standard input to a temporary file, and then run the command
     +    with its standard input redirected from the temporary file. This ensures
     +    that each command sees the same contents from its standard input.
     +
     +    Note that this does not impact commands that do not read from standard
     +    input; they continue to ignore it. Additionally, existing uses of the
     +    run_on_* functions do not need to do anything differently, as the
     +    standard input of the test environment is already connected to
     +    /dev/null.
     +
     +    We do not explicitly clean up the input files because they are cleaned
     +    up with the rest of the test repositories and their contents may be
     +    useful for figuring out which command failed when a test case fails.
      
          Signed-off-by: Kevin Lyles <[email protected]>
      
     @@ t/t1092-sparse-checkout-compatibility.sh: init_repos_as_submodules () {
       }
       
       run_on_sparse () {
     -+	cat >run_on_sparse-input &&
     ++	cat >run-on-sparse-input &&
      +
       	(
       		cd sparse-checkout &&
       		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
      -	) &&
     -+	) <run_on_sparse-input &&
     ++	) <run-on-sparse-input &&
       	(
       		cd sparse-index &&
       		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
      -	)
     -+	) <run_on_sparse-input
     ++	) <run-on-sparse-input
       }
       
       run_on_all () {
     -+	cat >run_on_all-input &&
     ++	cat >run-on-all-input &&
      +
       	(
       		cd full-checkout &&
       		GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
      -	) &&
      -	run_on_sparse "$@"
     -+	) <run_on_all-input &&
     -+	run_on_sparse "$@" <run_on_all-input
     ++	) <run-on-all-input &&
     ++	run_on_sparse "$@" <run-on-all-input
       }
       
       test_all_match () {

2: f4d1461b993 ! 2: ac913257309 Mark 'git cat-file' sparse-index compatible
@@ Metadata
Author: Kevin Lyles <[email protected]>

## Commit message ##
- Mark 'git cat-file' sparse-index compatible
+ builtin/cat-file: mark 'git cat-file' sparse-index compatible

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.
+ 'git cat-file' expands a sparse index to a full index any time contents
+ are requested from the index by specifying an object with the ":<path>"
+ syntax. This is true even when the requested file is part of the sparse
+ index, and results in much slower 'git cat-file' operations when working
+ within the sparse index.

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

gitgitgadget

From: Kevin Lyles <[email protected]>

The 'run_on_sparse' and 'run_on_all' functions do not work correctly for commands accepting standard input, because they run the same command multiple times and the first instance consumes it. This also indirectly affects 'test_all_match' and 'test_sparse_match'.

To allow these functions to work with commands accepting standard input, first slurp standard input to a temporary file, and then run the command with its standard input redirected from the temporary file. This ensures that each command sees the same contents from its standard input.

Note that this does not impact commands that do not read from standard input; they continue to ignore it. Additionally, existing uses of the run_on_* functions do not need to do anything differently, as the standard input of the test environment is already connected to /dev/null.

We do not explicitly clean up the input files because they are cleaned up with the rest of the test repositories and their contents may be useful for figuring out which command failed when a test case fails.

---
 t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index 6fa7f5e9587..4cbe9b1465d 100755
@@ -179,22 +179,26 @@ init_repos_as_submodules () {
 }
 
 run_on_sparse () {
+	cat >run-on-sparse-input &&
+
 	(
 		cd sparse-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-checkout-out 2>../sparse-checkout-err
-	) &&
+	) <run-on-sparse-input &&
 	(
 		cd sparse-index &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../sparse-index-out 2>../sparse-index-err
-	)
+	) <run-on-sparse-input
 }
 
 run_on_all () {
+	cat >run-on-all-input &&
+
 	(
 		cd full-checkout &&
 		GIT_PROGRESS_DELAY=100000 "$@" >../full-checkout-out 2>../full-checkout-err
-	) &&
-	run_on_sparse "$@"
+	) <run-on-all-input &&
+	run_on_sparse "$@" <run-on-all-input
 }
 
 test_all_match () {

@@ -221,7 +225,7 @@ test_sparse_unstaged () {
 	done
 }
 
-# Usage: test_sprase_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
+# Usage: test_sparse_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
 # Verifies that "git sparse-checkout set <c1> ... <cN>" succeeds and
 # leaves the sparse index in a state where <s1> ... <sM> are sparse
 # directories (and <c1> ... <cN> are not).

gitgitgadget

From: Kevin Lyles <[email protected]>

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' expands a sparse index to a full index any time contents are requested from the index by specifying an object with the ":<path>" syntax. This is true even when the requested file is part of the sparse index, and results in much slower 'git cat-file' operations when working within the sparse index.

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

---
 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

index 18fe58d6b8b..1afdfb5cbae 100644
@@ -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)

index 4cbe9b1465d..eb32da2a7f2 100755
@@ -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

gitgitgadget

The above is not as suitable as the first paragraph as the one that comes next, which describes the status quo and highlights what the problem is. With the few paragraphs below, that talk about the interaction among ":<path>" syntax, get_oid_with_context(), and the sparse-index, I think we can just remove it.

Nicely explained.