Re: [PATCH 3/5] pathspec: stop calling ensure_full_index

[Elijah Newren <newren@gmail.com>]

On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The add_pathspec_matches_against_index() focuses on matching a pathspec
> to file entries in the index. This already works correctly for its only
> use: checking if untracked files exist in the index.
>
> The compatibility checks in t1092 already test that 'git add <dir>'
> works for a directory outside of the sparse cone. That provides coverage
> for removing this guard.
>
> This finalizes our ability to run 'git add .' without expanding a sparse
> index to a full one. This is evidenced by an update to t1092 and by
> these performance numbers for p2000-sparse-operations.sh:
>
> Test                                    HEAD~1            HEAD
> --------------------------------------------------------------------------------
> 2000.10: git add . (full-index-v3)      0.37(0.28+0.07)   0.36(0.27+0.06) -2.7%
> 2000.11: git add . (full-index-v4)      0.33(0.26+0.06)   0.32(0.28+0.05) -3.0%
> 2000.12: git add . (sparse-index-v3)    0.57(0.53+0.07)   0.06(0.06+0.07) -89.5%
> 2000.13: git add . (sparse-index-v4)    0.57(0.53+0.07)   0.05(0.03+0.09) -91.2%
>
> While the ~90% improvement is shown by the test results, it is worth
> noting that expanding the sparse index was adding overhead in previous
> commits. Comparing to the full index case, we see the performance go
> from 0.33s to 0.05s, an 85% improvement.

These perf improvements are pretty sweet.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  pathspec.c                               | 2 --
>  t/t1092-sparse-checkout-compatibility.sh | 7 +++----
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 08f8d3eedc3..44306fdaca2 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -37,8 +37,6 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,
>                         num_unmatched++;
>         if (!num_unmatched)
>                 return;
> -       /* TODO: audit for interaction with sparse-index. */
> -       ensure_full_index(istate);
>         for (i = 0; i < istate->cache_nr; i++) {
>                 const struct cache_entry *ce = istate->cache[i];
>                 if (sw_action == PS_IGNORE_SKIP_WORKTREE && ce_skip_worktree(ce))
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index a11d9d7f35d..f9e2f5f4aa1 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -322,9 +322,6 @@ test_expect_success 'commit including unstaged changes' '
>  test_expect_success 'status/add: outside sparse cone' '
>         init_repos &&
>
> -       # adding a "missing" file outside the cone should fail
> -       test_sparse_match test_must_fail git add folder1/a &&
> -

So this is removed because of non-matching errors.  In particular,
sparse-checkout shows
"""
The following pathspecs didn't match any eligible path, but they do match index
entries outside the current sparse checkout:
folder1/a
hint: Disable or modify the sparsity rules if you intend to update such entries.
hint: Disable this message with "git config advice.updateSparsePath false"
"""
while sparse-index now shows:
"""
fatal: pathspec 'folder1/a' did not match any files
"""

The new error seems entirely reasonable to me.  No objection here.


But allow me to go on a bit of a diversion...

If we modify this setup slightly by running:

$ mkdir folder1
$ echo garbage >folder1/a
$ git add folder1/a

Then you'll get the first of those errors in both the sparse-index and
the sparse-checkout.  I also like this behavior.

If you unset the SKIP_WORKTREE bit manually, and then add:

$ git update-index --no-skip-worktree folder1/a
$ git add folder1/a

Then the file is added with no error or warning.  I like this behavior too.

If you further change the setup with:

$ echo more garbage >folder1/z
$ git add folder1/z

Then you get no error, despite folder1/z being an untracked file
outside of sparsity paths.  No bueno.  :-(

>         # folder1 is at HEAD, but outside the sparse cone
>         run_on_sparse mkdir folder1 &&
>         cp initial-repo/folder1/a sparse-checkout/folder1/a &&
> @@ -633,7 +630,9 @@ test_expect_success 'sparse-index is not expanded' '
>         echo >>sparse-index/README.md &&
>         ensure_not_expanded add -A &&
>         echo >>sparse-index/extra.txt &&
> -       ensure_not_expanded add extra.txt
> +       ensure_not_expanded add extra.txt &&
> +       echo >>sparse-index/untracked.txt &&
> +       ensure_not_expanded add .

:-)

>  '
>
>  # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> --

So I added a lot of comments here, in part because I thought I'd test
a bit more of what I said in response to your cover letter and see how
close to it we were.  The patch in question looks fine.

I just added an aside as a convenient place to check whether the
behavior at the end of the series matches what you proposed in the
cover letter, or what I proposed in response.  It appears it matches
neither (though that's not due to this specific patch).