| Message ID | a577ea4c74d86e57defcc4f45011871b634bcf56.1626901619.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse Index: Integrate with 'git add' | expand |
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).
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 && - # 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