[PATCH] Mark `cat-file` sparse-index compatible

[PATCH] Mark `cat-file` sparse-index compatible

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles+github@epic.com>

`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

Signed-off-by: Kevin Lyles <klyles+github@epic.com>
---
    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(-)

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 a2c0e1b4dcc..0f36246ae84 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -179,22 +179,26 @@ 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 () {
@@ -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).
@@ -2345,4 +2349,61 @@ 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 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 &&
+	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 &&
+
+	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 "HEAD:folder1/a" | test_all_match git cat-file --batch &&
+	echo "HEAD:folder1/a" | ensure_not_expanded cat-file --batch &&
+
+	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 &&
+	: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_done

base-commit: 80ccd8a2602820fdf896a8e8894305225f86f61d
-- 
gitgitgadget

Re: [PATCH] Mark `cat-file` sparse-index compatible

[Derrick Stolee]

On 8/26/24 2:08 PM, Kevin Lyles via GitGitGadget wrote:
 > From: Kevin Lyles <klyles+github@epic.com>
Hi, Kevin! Thanks for looking into this.
 > `cat-file` will expand a sparse index to a full index when needed, but

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

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

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.

 > 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

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

 > Signed-off-by: Kevin Lyles <klyles+github@epic.com>
 > ---
 >      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(-)
 >
 > 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 a2c0e1b4dcc..0f36246ae84 100755
 > --- a/t/t1092-sparse-checkout-compatibility.sh
 > +++ b/t/t1092-sparse-checkout-compatibility.sh
 > @@ -179,22 +179,26 @@ init_repos_as_submodules () {
 >   }
 >
 >   run_on_sparse () {
 > +	tee run_on_sparse-checkout run_on_sparse-index &&> +

This is an interesting update to this helper method, which takes stdin
and copies it to these two files for consumption by the two calls below.
It really only needs to go to one file and then have that file be
consumed twice. This diff seems to accomplish what you want:

--- >8 ---

diff --git a/t/t1092-sparse-checkout-compatibility.sh 
b/t/t1092-sparse-checkout-compatibility.sh
index 1954bf6332d..9902190eec4 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -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 () {


--- >8 ---

Whatever happens, this subtlety seems valuable to add to the commit
message to help readers to expect it.

 > -# Usage: test_sprase_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"
 > +# Usage: test_sparse_checkout_set "<c1> ... <cN>" "<s1> ... <sM>"

Thanks for fixing this typo.

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

These commands shouldn't touch the index at all, as they are loading the
object ID the same as 'git rev-parse'.

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

These are the interesting cases.

 > +	test_cmp full-checkout-out sparse-checkout-out &&
 > +	test_cmp full-checkout-out sparse-index-out &&
 > +	test_cmp full-checkout-err sparse-checkout-err &&

I figure that you have decided to use run_on_all and not test_all_match
because of an error message that doesn't match. Across the full and
sparse cases. This would be a good time to focus on what is different
about them, perhaps with something like

	grep "error message contents" sparse-index-err

The difference shouldn't be the new advice message due to the mentioned
change to the config of these repos. If that was the case, then the
following would work, but shouldn't be necessary:

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

What is the reason for there being a difference in stderr in the
sparse index case? We should drill down here to make sure the behavior
is appropriate.

 > +	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 &&
 > +
 > +	cat <<-\EOF | test_all_match git cat-file --batch &&
 > +	HEAD:deep/a
 > +	:deep/a
 > +	EOF

This heredoc should instead be used to write to a file to use twice:

	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 "HEAD:folder1/a" | test_all_match git cat-file --batch &&
 > +	echo "HEAD:folder1/a" | ensure_not_expanded cat-file --batch &&

I'm not sure that these are interesting cases as they will not cause an
index read and thus are not affected by sparse-checkout.

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

This is a more interesting case, again using outside of cone paths.

I would still recommend reusing an input file:

	echo ":folder1/a" >in &&
	test_all_match git cat-file --batch <in &&
	ensure_expanded cat-file --batch <in &&

 > +
 > +	cat <<-\EOF | run_on_all git cat-file --batch &&
 > +	:deep/a
 > +	:folder1/a
 > +	EOF

Again, point the heredoc to a file and then into the test:

	cat >in <<-\EOF &&
	:deep/a
	:folder1/a
	EOF
	test_all_match git cat-file --batch <in &&
	ensure_expanded cat-file --batch <in &&

Thanks for working on this!

-Stolee

[PATCH v2 0/2] Mark cat-file sparse-index compatible

[Kevin Lyles via GitGitGadget]

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 <klyles+github@epic.com>
      
       ## 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 <klyles+github@epic.com>
      
     @@ 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

[PATCH v2 1/2] Allow using stdin in run_on_* functions

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles@epic.com>

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.

Signed-off-by: Kevin Lyles <klyles@epic.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index a2c0e1b4dcc..b140b642b21 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -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

[PATCH v2 2/2] Mark 'git cat-file' sparse-index compatible

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles+github@epic.com>

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 <klyles+github@epic.com>
---
 builtin/cat-file.c                       |  3 +++
 t/t1092-sparse-checkout-compatibility.sh | 34 ++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

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 b140b642b21..4d025c5f8d6 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -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

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

[Derrick Stolee]

On 8/30/24 5:10 PM, Kevin Lyles via GitGitGadget wrote:
> From: Kevin Lyles <klyles+github@epic.com>

> +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'


There's one style issue that I missed in the previous version,
which is that the final single-quote (') should be on a new line
after the last line of the test. like this:

---

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

nit: typically we would write this as "cat >in <<-\EOF &&"

> +	:deep/a
> +	:folder1/a
> +	EOF
> +	test_all_match git cat-file --batch <in &&
> +	ensure_expanded cat-file --batch <in'
> +

Pointing out the final single-quote here, too.

Thank you for updating the heredocs and other format things that
I noticed in the previous version. Also, thanks for splitting the
patch into two parts.

Thanks for working on this!
-Stolee

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

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> Thank you for updating the heredocs and other format things that
> I noticed in the previous version. Also, thanks for splitting the
> patch into two parts.
>
> Thanks for working on this!

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

Thanks for a detailed review.

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

[Kevin Lyles via GitGitGadget]

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

[PATCH v3 1/2] Allow using stdin in run_on_* functions

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles@epic.com>

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.

Signed-off-by: Kevin Lyles <klyles@epic.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 6fa7f5e9587..87953abf872 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -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

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

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles+github@epic.com>

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 <klyles+github@epic.com>
---
 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

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

Re: [PATCH v3 1/2] Allow using stdin in run_on_* functions

[Junio C Hamano]

"Kevin Lyles via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH v3 1/2] Allow using stdin in run_on_* functions

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.

> From: Kevin Lyles <klyles@epic.com>
>
> The 'run_on_sparse' and 'run_on_all' functions previously did not work
> correctly for commands accepting standard input.


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.

> This also indirectly
> affected 'test_all_match' and 'test_sparse_match'.

"affected" -> "affects".

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

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.


> Signed-off-by: Kevin Lyles <klyles@epic.com>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 6fa7f5e9587..87953abf872 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -179,22 +179,26 @@ init_repos_as_submodules () {
>  }
>  
>  run_on_sparse () {
> +	cat >run_on_sparse-input &&

Mixture of word_with_underscore-and-dash-separation look unexpected;
use "run-on-sparse-input" instead, as the output files seem to be
named that way?

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

Shouldn't the temporary file be removed at the end, or are the
callers expected to live with them?  Unless the test is about
"ls-files -u" or "status", that is usually OK.

>  }
>  
>  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).

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

[Junio C Hamano]

"Kevin Lyles via GitGitGadget" <gitgitgadget@gmail.com> 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 <klyles+github@epic.com>
>
> 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 v4 0/2] builtin/cat-file: mark 'git cat-file' sparse-index compatible

[Kevin Lyles via GitGitGadget]

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 <klyles@epic.com>
      
       ## 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 <klyles@epic.com>
      
     @@ 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 <klyles+github@epic.com>
      
       ## 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

[PATCH v4 1/2] t1092: allow run_on_* functions to use standard input

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles@epic.com>

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.

Signed-off-by: Kevin Lyles <klyles@epic.com>
---
 t/t1092-sparse-checkout-compatibility.sh | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 6fa7f5e9587..4cbe9b1465d 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -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

[PATCH v4 2/2] builtin/cat-file: mark 'git cat-file' sparse-index compatible

[Kevin Lyles via GitGitGadget]

From: Kevin Lyles <klyles+github@epic.com>

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

Signed-off-by: Kevin Lyles <klyles+github@epic.com>
---
 builtin/cat-file.c                       |  3 ++
 t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

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 4cbe9b1465d..eb32da2a7f2 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
-- 
gitgitgadget

Re: [PATCH v4 1/2] t1092: allow run_on_* functions to use standard input

[Junio C Hamano]

"Kevin Lyles via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Kevin Lyles <klyles@epic.com>
>
> 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'.
>
> ...
> -# 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).

Reads much better.  Will queue.  Thanks.

Re: [PATCH v4 2/2] builtin/cat-file: mark 'git cat-file' sparse-index compatible

[Junio C Hamano]

"Kevin Lyles via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Kevin Lyles <klyles+github@epic.com>
>
> 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).

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.

> '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
>
> Signed-off-by: Kevin Lyles <klyles+github@epic.com>
> ---

Nicely explained.

> @@ -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;
> +

OK.  This command does not start parsing the command line arguments
before this point, and this is really a good place to toggle the bit
off.

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 4cbe9b1465d..eb32da2a7f2 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
> +'

OK.  These are about the object names given from the command line.

> +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
> +'

And these are about the object names fed via the --batch mechanism.

Looking good.

Will queue.  Thanks.