On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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).
