| Message ID | 791c6c2c9ade5b065fc0f367e00c52a493d086ef.1626901619.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Sparse Index: Integrate with 'git add' | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > > Disable command_requires_full_index for 'git add'. This does not require > any additional removals of ensure_full_index(). The main reason is that > 'git add' discovers changes based on the pathspec and the worktree > itself. These are then inserted into the index directly, and calls to > index_name_pos() or index_file_exists() already call expand_to_path() at > the appropriate time to support a sparse-index. OK. With that explained, it still is quite surprising that we only need this change (eh, rather, doing this change is safe without doing anything else). > - # This "git add folder1/a" fails with a warning > - # in the sparse repos, differing from the full > - # repo. This is intentional. > - test_sparse_match test_must_fail git add folder1/a && > - test_sparse_match test_must_fail git add --refresh folder1/a && > - test_all_match git status --porcelain=v2 && And nice to see a known limitation lifted. > test_all_match git add . && > test_all_match git status --porcelain=v2 && > test_all_match git commit -m folder1/new && > @@ -635,7 +628,12 @@ test_expect_success 'sparse-index is not expanded' ' > git -C sparse-index reset --hard && > ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 && > git -C sparse-index reset --hard && > - ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 > + ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 && > + > + echo >>sparse-index/README.md && > + ensure_not_expanded add -A && > + echo >>sparse-index/extra.txt && > + ensure_not_expanded add extra.txt > ' > > # NEEDSWORK: a sparse-checkout behaves differently from a full checkout
On 7/21/21 6:19 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Derrick Stolee <dstolee@microsoft.com> >> >> Disable command_requires_full_index for 'git add'. This does not require >> any additional removals of ensure_full_index(). The main reason is that >> 'git add' discovers changes based on the pathspec and the worktree >> itself. These are then inserted into the index directly, and calls to >> index_name_pos() or index_file_exists() already call expand_to_path() at >> the appropriate time to support a sparse-index. > > OK. With that explained, it still is quite surprising that we only > need this change (eh, rather, doing this change is safe without > doing anything else). Yes, all of the hard work was done by the earlier work to expand a sparse index when we search for a specific path that lands within a sparse directory. See 95e0321 (read-cache: expand on query into sparse-directory entry, 2021-04-01) for the specifics. >> - # This "git add folder1/a" fails with a warning >> - # in the sparse repos, differing from the full >> - # repo. This is intentional. >> - test_sparse_match test_must_fail git add folder1/a && >> - test_sparse_match test_must_fail git add --refresh folder1/a && >> - test_all_match git status --porcelain=v2 && > > And nice to see a known limitation lifted. Thank you for pointing this out. This actually starts to _fail_ now that we allow sparse indexes in 'git add', but it's because the error messages don't match, not that the 'test_must_fail' is violated. Patch 4 adds a similar test that is then set to work in patch 5. That allows us a clear way to describe the behavior change and to motivate the fix in patch 5. This could be explained better, perhaps by merging Patch 4 into this one. That helps describe how this specific case changes behavior (for the worse) in this patch, but is handled in a careful way later, once the behavior change is documented. If there is a better way to reorganize these patches, then I could try another approach. Thanks, -Stolee
On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > Disable command_requires_full_index for 'git add'. This does not require > any additional removals of ensure_full_index(). The main reason is that > 'git add' discovers changes based on the pathspec and the worktree > itself. These are then inserted into the index directly, and calls to > index_name_pos() or index_file_exists() already call expand_to_path() at > the appropriate time to support a sparse-index. Nice. > Add a test to check that 'git add -A' and 'git add <file>' does not > expand the index at all, as long as <file> is not within a sparse > directory. This does not help the global 'git add .' case. Good idea. > We can measure the improvement using p2000-sparse-operations.sh with > these results: > > Test HEAD~1 HEAD > ------------------------------------------------------------------------------ > 2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7% > 2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5% > 2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2% > 2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4% > > While the 91% improvement seems impressive, it's important to recognize > that previously we had significant overhead for expanding the > sparse-index. Comparing to the full index case, 'git add -A' goes from > 0.37s to 0.05s, which is "only" an 86% improvement. Hehe. Yep, it's so "disappointing" to "only" have the code be 7x faster. :-) Out of curiosity, IIRC any operation involving the index took ~10s on some of the Microsoft repos. What does the speedup look like over there for these changes to git-add? > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > builtin/add.c | 3 +++ > t/t1092-sparse-checkout-compatibility.sh | 14 ++++++-------- > 2 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/builtin/add.c b/builtin/add.c > index b773b5a4993..c76e6ddd359 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix) > add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize; > require_pathspec = !(take_worktree_changes || (0 < addremove_explicit)); > > + prepare_repo_settings(the_repository); > + the_repository->settings.command_requires_full_index = 0; > + > hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); > > /* > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh > index a3c01d588d8..a11d9d7f35d 100755 > --- a/t/t1092-sparse-checkout-compatibility.sh > +++ b/t/t1092-sparse-checkout-compatibility.sh > @@ -340,13 +340,6 @@ test_expect_success 'status/add: outside sparse cone' ' > > test_sparse_match git status --porcelain=v2 && > > - # This "git add folder1/a" fails with a warning > - # in the sparse repos, differing from the full > - # repo. This is intentional. > - test_sparse_match test_must_fail git add folder1/a && > - test_sparse_match test_must_fail git add --refresh folder1/a && > - test_all_match git status --porcelain=v2 && > - Why was this chunk removed? Nothing in the commit message mentions this, and it's not clear to me the reason for it. I tried adding it back in at the end of the series and it still works (and further I can't change test_sparse_match to test_all_match and have the test work). > test_all_match git add . && > test_all_match git status --porcelain=v2 && > test_all_match git commit -m folder1/new && > @@ -635,7 +628,12 @@ test_expect_success 'sparse-index is not expanded' ' > git -C sparse-index reset --hard && > ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 && > git -C sparse-index reset --hard && > - ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 > + ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 && > + > + echo >>sparse-index/README.md && > + ensure_not_expanded add -A && > + echo >>sparse-index/extra.txt && > + ensure_not_expanded add extra.txt ...and here's the extra test you mentioned in the commit message. Looks good. > ' > > # NEEDSWORK: a sparse-checkout behaves differently from a full checkout > -- > gitgitgadget
On 7/23/2021 1:45 PM, Elijah Newren wrote: > On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: ... >> Test HEAD~1 HEAD >> ------------------------------------------------------------------------------ >> 2000.6: git add -A (full-index-v3) 0.35(0.30+0.05) 0.37(0.29+0.06) +5.7% >> 2000.7: git add -A (full-index-v4) 0.31(0.26+0.06) 0.33(0.27+0.06) +6.5% >> 2000.8: git add -A (sparse-index-v3) 0.57(0.53+0.07) 0.05(0.04+0.08) -91.2% >> 2000.9: git add -A (sparse-index-v4) 0.58(0.55+0.06) 0.05(0.05+0.06) -91.4% >> >> While the 91% improvement seems impressive, it's important to recognize >> that previously we had significant overhead for expanding the >> sparse-index. Comparing to the full index case, 'git add -A' goes from >> 0.37s to 0.05s, which is "only" an 86% improvement. > > Hehe. Yep, it's so "disappointing" to "only" have the code be 7x faster. :-) > > Out of curiosity, IIRC any operation involving the index took ~10s on > some of the Microsoft repos. What does the speedup look like over > there for these changes to git-add? The latest numbers I have for a repo with ~2 million tracked files is that index reads take about half a second (because of the threaded reads) and writes take at least one second. There was a lot of work by Ben Peart, Jeff Hostetler, and Kevin Willford to reduce this cost as much as possible a few years ago. VFS for Git is still limited by this bottleneck, but Scalar's use of sparse-checkout enables the use of the sparse index. We have an experimental release [1] out to users right now, and I will report to the mailing list about how that went after we get sufficient adoption that the data can be significant. When focusing on individual users I can find things like one user seeing "git commit" going from 4.3s to 0.35s and "git add" going from 6.1s to 0.13s. (The "git add" time might also be conflated with a change from the FS Monitor hook to the builtin FS Monitor.) [1] https://github.com/microsoft/git/releases/tag/v2.32.0.vfs.0.102.exp Thanks, -Stolee
On 7/23/2021 1:45 PM, Elijah Newren wrote: > On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> ... >> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh >> index a3c01d588d8..a11d9d7f35d 100755 >> --- a/t/t1092-sparse-checkout-compatibility.sh >> +++ b/t/t1092-sparse-checkout-compatibility.sh >> @@ -340,13 +340,6 @@ test_expect_success 'status/add: outside sparse cone' ' >> >> test_sparse_match git status --porcelain=v2 && >> >> - # This "git add folder1/a" fails with a warning >> - # in the sparse repos, differing from the full >> - # repo. This is intentional. >> - test_sparse_match test_must_fail git add folder1/a && >> - test_sparse_match test_must_fail git add --refresh folder1/a && >> - test_all_match git status --porcelain=v2 && >> - > > Why was this chunk removed? Nothing in the commit message mentions > this, and it's not clear to me the reason for it. > > I tried adding it back in at the end of the series and it still works > (and further I can't change test_sparse_match to test_all_match and > have the test work). I mentioned this in a reply to Junio, but this hunk removal is confusing. As of this patch, this hunk causes a failure due to an error message not matching, specifically this error: + diff -u sparse-checkout-err sparse-index-err --- sparse-checkout-err 2021-07-26 13:30:50.304291264 +0000 +++ sparse-index-err 2021-07-26 13:30:50.308291259 +0000 @@ -1,5 +1 @@ -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" +fatal: pathspec 'folder1/a' did not match any files A similar test is added as a failure case in patch 4, then marked as success in patch 5. This organization of test changes could be organized better, so I will work on that in v2, along with your other suggestions. Thanks, -Stolee
diff --git a/builtin/add.c b/builtin/add.c index b773b5a4993..c76e6ddd359 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -528,6 +528,9 @@ int cmd_add(int argc, const char **argv, const char *prefix) add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize; require_pathspec = !(take_worktree_changes || (0 < addremove_explicit)); + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); /* diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index a3c01d588d8..a11d9d7f35d 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -340,13 +340,6 @@ test_expect_success 'status/add: outside sparse cone' ' test_sparse_match git status --porcelain=v2 && - # This "git add folder1/a" fails with a warning - # in the sparse repos, differing from the full - # repo. This is intentional. - test_sparse_match test_must_fail git add folder1/a && - test_sparse_match test_must_fail git add --refresh folder1/a && - test_all_match git status --porcelain=v2 && - test_all_match git add . && test_all_match git status --porcelain=v2 && test_all_match git commit -m folder1/new && @@ -635,7 +628,12 @@ test_expect_success 'sparse-index is not expanded' ' git -C sparse-index reset --hard && ensure_not_expanded checkout rename-out-to-out -- deep/deeper1 && git -C sparse-index reset --hard && - ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 + ensure_not_expanded restore -s rename-out-to-out -- deep/deeper1 && + + echo >>sparse-index/README.md && + ensure_not_expanded add -A && + echo >>sparse-index/extra.txt && + ensure_not_expanded add extra.txt ' # NEEDSWORK: a sparse-checkout behaves differently from a full checkout