[PATCH v2 0/5] Sparse index integration with 'git show'

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

This continues our sequence of integrating builtins with the sparse index.

'git show' is relatively simple to get working in a way that doesn't fail
when it would previously succeed, but there are some subtleties when the
user passes a directory path using the ":" syntax to get the path out of the
index. If that path happens to be a sparse directory entry, we suddenly
start succeeding and printing the tree information!

Since this behavior can change depending on the sparse checkout definition
and the state of index entries within that directory, this new behavior
would be more likely to confuse users than help them.

This ":" syntax is shared by other commands like "git rev-parse", but we are
not adding those integrations at this point.

Some background: as we shipped our sparse index integrations in the
microsoft/git fork and measured performance of real users, we noticed that
'git show' was a frequently-used command that went from ~0.5 seconds to ~4
seconds in monorepo situations. This was unexpected, because we didn't know
about the ":" syntax. Further, it seemed that some third-party tools were
triggering this behavior as a frequent way to check on staged content. That
motivated quickly shipping this integration. Performance improved to ~0.1
seconds because of the reduced index size. While inspecting our rebase of
microsoft/git commits onto the 2.36.0 release candidates, I noticed this
integration would be a simpler example to demonstrate how sparse index
integrations should go when the behavior is not too complicated.

Here is an outline of this series:

 * Patch 1: Add more tests around 'git show :' in t1092. These tests are
   only to establish the existing differences between the full and
   sparse-checkout cases, since 'git show' is still protected by
   'command_requires_full_index'.

 * Patch 2: Make 'git show' stop expanding the index by default. Make note
   of this behavior change in the tests.

 * Patches 3-4: Make the subtle changes to object-name.c that help us reject
   sparse directories (patch 3) and print the correct error message (patch
   4).

 * Patch 5: Now that the common index-parsing code is updated, do the
   minimum change to 'git rev-parse' to avoid expanding the index to parse
   index entries for a sparse index.

Patches 2-4 could realistically be squashed into a single commit, but I
thought it might be instructive to show these individual steps, especially
as an example for our GSoC project.

I know that Victoria intends to submit her 'git stash' integration soon, and
this provides a way to test if our split of test changes in t1092 are easy
to merge without conflict. If that is successful, then I will likely submit
my integration with the 'sparse-checkout' builtin after this series is
complete. (UPDATE: we inserted a test in the same location of t1092, but
otherwise there are no textual or semantic conflicts.)


Updates in v2
=============

 * The test comment in patch 2 is updated.
 * A commit message typo in patch 4 is fixed.
 * Patch 4 simplified the behavior, but the previous version didn't clean up
   a test comment about that. It now cleans up the test to be simpler.
 * Patch 5 includes an integration with 'git rev-parse'.
 * The cover letter is expanded with more context.
 * The only conflict with Victoria's new 'git stash' patch series is that we
   both added a test in the same position of t1092. Including both new tests
   is the right way to resolve the conflict. Order does not matter.

[1]
https://lore.kernel.org/git/pull.1171.git.1650908957.gitgitgadget@gmail.com/

Thanks, -Stolee

Derrick Stolee (5):
  t1092: add compatibility tests for 'git show'
  show: integrate with the sparse index
  object-name: reject trees found in the index
  object-name: diagnose trees in index properly
  rev-parse: integrate with sparse index

 builtin/log.c                            |  5 ++++
 builtin/rev-parse.c                      |  3 ++
 object-name.c                            | 25 ++++++++++++++--
 t/t1092-sparse-checkout-compatibility.sh | 36 ++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 3 deletions(-)


base-commit: 07330a41d66a2c9589b585a3a24ecdcf19994f19
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1207%2Fderrickstolee%2Fsparse-index%2Fgit-show-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1207/derrickstolee/sparse-index/git-show-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1207

Range-diff vs v1:

 1:  8c2fdb5a4fc = 1:  8c2fdb5a4fc t1092: add compatibility tests for 'git show'
 2:  27ab853a9b4 ! 2:  2e9d47ab09b show: integrate with the sparse index
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'show (cached blob
      -	# does not work as implemented. The error message is
      -	# different for a full checkout and a sparse checkout
      -	# when the directory is outside of the cone.
     -+	# changes depending on the existence of a sparse index.
     ++	# had different behavior depending on the existence
     ++	# of a sparse index.
       	test_all_match test_must_fail git show :deep/ &&
       	test_must_fail git -C full-checkout show :folder1/ &&
      -	test_sparse_match test_must_fail git show :folder1/
 3:  f5da5327673 = 3:  5a7561637f0 object-name: reject trees found in the index
 4:  99c09ccc240 ! 4:  b730457fccc object-name: diagnose trees in index properly
     @@ Commit message
          checkout. The error message from diagnose_invalid_index_path() reports
          whether the path is on disk or not. The full checkout will have the
          directory on disk, but the path will not be in the index. The sparse
     -    checokut could have the directory not exist, specifically when that
     +    checkout could have the directory not exist, specifically when that
          directory is outside of the sparse-checkout cone.
      
          In the case of a sparse index, we have yet another state: the path can
     @@ object-name.c: static void diagnose_invalid_index_path(struct repository *r,
      
       ## t/t1092-sparse-checkout-compatibility.sh ##
      @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'show (cached blobs/trees)' '
     + 	test_all_match git show :deep/a &&
     + 	test_sparse_match git show :folder1/a &&
     + 
     +-	# Asking "git show" for directories in the index
     +-	# had different behavior depending on the existence
     +-	# of a sparse index.
     ++	# The error message differs depending on whether
     ++	# the directory exists in the worktree.
     + 	test_all_match test_must_fail git show :deep/ &&
       	test_must_fail git -C full-checkout show :folder1/ &&
     - 	test_must_fail git -C sparse-checkout show :folder1/ &&
     +-	test_must_fail git -C sparse-checkout show :folder1/ &&
     ++	test_sparse_match test_must_fail git show :folder1/ &&
       
      -	test_must_fail git -C sparse-index show :folder1/ 2>err &&
      -	grep "is in the index, but not at stage 0" err
     -+	test_sparse_match test_must_fail git show :folder1/ &&
     -+
      +	# Change the sparse cone for an extra case:
      +	run_on_sparse git sparse-checkout set deep/deeper1 &&
      +
 -:  ----------- > 5:  69efe637a18 rev-parse: integrate with sparse index

-- 
gitgitgadget