Sparse Index: latest integrations #410

derrickstolee · 2021-08-17T19:54:13Z

6e74958f590 p2000: add 'git checkout -' test and decrease depth
3e1d03c41be p2000: compress repo names
cd94f820052 commit: integrate with sparse-index
65e79b8037c sparse-index: recompute cache-tree
e9a9981477e checkout: stop expanding sparse indexes
4b801c854fb t1092: document bad 'git checkout' behavior
71e301501c8 unpack-trees: resolve sparse-directory/file conflicts
5e96df4df58 t1092: test merge conflicts outside cone
defab1b86d3 add: allow operating on a sparse-only index
9fc4313c889 pathspec: stop calling ensure_full_index
0ec03ab021d add: ignore outside the sparse-checkout in refresh()
adf5b15ac3d add: remove ensure_full_index() with --renormalize

These commits are equivalent to those already in next via gitgitgadget#999.

80b8d6c56b8 Merge branch 'sparse-index/add' into stolee/sparse-index/add

This merge resolves conflicts with some work that happened in parallel, but is already in upstream master.

c407b2cb346 t7519: rewrite sparse index test
9dad0d2a3d4 sparse-index: silently return when not using cone-mode patterns
29749209c64 sparse-index: silently return when cache tree fails
e7cdaa01418 unpack-trees: fix nested sparse-dir search
347410c5d5e sparse-checkout: create helper methods
4537233f048 attr: be careful about sparse directories
5282a8614f4 sparse-index: add SPARSE_INDEX_MEMORY_ONLY flag
3a2f3164fcf sparse-checkout: clear tracked sparse dirs
fb47b56719d sparse-checkout: add config to disable deleting dirs

These commits are the ones under review as of gitgitgadget#1009. Recent review made this less stable. It's a slightly different and more robust version of #396.

Note: I'm still not done with the feedback for upstream, but the remaining feedback is "can we add tests that cover these tricky technical bits?" and in microsoft/git these are already covered by the Scalar functional tests (since that's how they were found).

080b02c7b6f diff: ignore sparse paths in diffstat
d91a6474942 merge: make sparse-aware with ORT
df49b5f030c merge-ort: expand only for out-of-cone conflicts
cdecb85d77b t1092: add cherry-pick, rebase tests
0c1ecfbdc68 sequencer: ensure full index if not ORT strategy
406dfbede9e sparse-index: integrate with cherry-pick and rebase

These commits integrate with git merge, git cherry-pick, git revert, and git rebase as of gitgitgadget#1019. This got some feedback that changed how the tests were working so they are more robust. This led to a new commit (0c1ecfb).

cbb0ab3b060 Merge branch 'sparse-index/merge' into vfs-2.33.0
acb8623f226 t7524: test no longer fails

Finally, the commits are merged into vfs-2.33.0 and also we include a fix to a microsoft/git test that is no longer broken.

Cc: @vdye and @ldennington to get a (possibly overwhelming?) taste of sparse-index stuff. If you focus solely on the git merge commits you'll get a feel for what a sparse index integration looks like.

As we increase our list of commands to test in p2000-sparse-operations.sh, we will want to have a slightly smaller test repository. Reduce the size by a factor of four by reducing the depth of the step that creates a big index around a moderately-sized repository. Also add a step to run 'git checkout -' on repeat. This requires having a previous location in the reflog, so add that to the initialization steps. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

By using shorter names for the test repos, we will get a slightly more compressed performance summary without comprimising clarity. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Update 'git commit' to allow using the sparse-index in memory without expanding to a full one. The only place that had an ensure_full_index() call was in cache_tree_update(). The recursive algorithm for update_one() was already updated in 2de37c5 (cache-tree: integrate with sparse directory entries, 2021-03-03) to handle sparse directory entries in the index. Most of this change involves testing different command-line options that allow specifying which on-disk changes should be included in the commit. This includes no options (only take currently-staged changes), -a (take all tracked changes), and --include (take a list of specific changes). To simplify testing that these options do not expand the index, update the test that previously verified that 'git status' does not expand the index with a helper method, ensure_not_expanded(). This allows 'git commit' to operate much faster when the sparse-checkout cone is much smaller than the full list of files at HEAD. Here are the relevant lines from p2000-sparse-operations.sh: Test HEAD~1 HEAD ---------------------------------------------------------------------------------- 2000.14: git commit -a -m A (full-v3) 0.35(0.26+0.06) 0.36(0.28+0.07) +2.9% 2000.15: git commit -a -m A (full-v4) 0.32(0.26+0.05) 0.34(0.28+0.06) +6.3% 2000.16: git commit -a -m A (sparse-v3) 0.63(0.59+0.06) 0.04(0.05+0.05) -93.7% 2000.17: git commit -a -m A (sparse-v4) 0.64(0.59+0.08) 0.04(0.04+0.04) -93.8% It is important to compare the full-index case to the sparse-index case, so the improvement for index version v4 is actually an 88% improvement in this synthetic example. In a real repository with over two million files at HEAD and 60,000 files in the sparse-checkout definition, the time for 'git commit -a' went from 2.61 seconds to 134ms. I compared this to the result if the index only contained the paths in the sparse-checkout definition and found the theoretical optimum to be 120ms, so the out-of-cone paths only add a 12% overhead. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

When some commands run with command_requires_full_index=1, then the index can get in a state where the in-memory cache tree is actually equal to the sparse index's cache tree instead of the full one. This results in incorrect entry_count values. By clearing the cache tree before converting to sparse, we avoid this issue. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Previous changes did the necessary improvements to unpack-trees.c and diff-lib.c in order to modify a sparse index based on its comparision with a tree. The only remaining work is to remove some ensure_full_index() calls and add tests that verify that the index is not expanded in our interesting cases. Include 'switch' and 'restore' in these tests, as they share a base implementation with 'checkout'. Here are the relevant performance results from p2000-sparse-operations.sh: Test HEAD~1 HEAD -------------------------------------------------------------------------------- 2000.18: git checkout -f - (full-v3) 0.49(0.43+0.03) 0.47(0.39+0.05) -4.1% 2000.19: git checkout -f - (full-v4) 0.45(0.37+0.06) 0.42(0.37+0.05) -6.7% 2000.20: git checkout -f - (sparse-v3) 0.76(0.71+0.07) 0.04(0.03+0.04) -94.7% 2000.21: git checkout -f - (sparse-v4) 0.75(0.72+0.04) 0.05(0.06+0.04) -93.3% It is important to compare the full index case to the sparse index case, as the previous results for the sparse index were inflated by the index expansion. For index v4, this is an 88% improvement. On an internal repository with over two million paths at HEAD and a sparse-checkout definition containing ~60,000 of those paths, 'git checkout' went from 3.5s to 297ms with this change. The theoretical optimum where only those ~60,000 paths exist was 275ms, so the extra sparse directory entries contribute a 22ms overhead. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Add new branches to the test repo that demonstrate directory/file conflicts in different ways. Since the directory 'folder1/' has adjacent files 'folder1-', 'folder1.txt', and 'folder10' it causes searches for 'folder1/' to land in a different place in the index than a search for 'folder1'. This causes a change in behavior when working with the df-conflict-1 and df-conflict-2 branches, whose only difference is that the first uses 'folder1' as the conflict and the other uses 'folder2' which does not have these adjacent files. We can extend two tests that compare the behavior across different 'git checkout' commands, and we see already that the behavior will be different in some cases and not in others. The difference between the two test loops is that one uses 'git reset --hard' between iterations. Further, we isolate the behavior of creating a staged change within a directory and then checking out a branch where that directory is replaced with a file. A full checkout behaves differently across these two cases, while a sparse-checkout cone behaves consistently. In both cases, the behavior is wrong. In one case, the staged change is dropped entirely. The other case the staged change is kept, replacing the file at that location, but none of the other files in the directory are kept. Likely, the correct behavior in this case is to reject the checkout and report the conflict, leaving HEAD in its previous location. None of the cases behave this way currently. Use comments to demonstrate that the tested behavior is only a documentation of the current, incorrect behavior to ensure we do not _accidentally_ change it. Instead, we would prefer to change it on purpose with a future change. At this point, the sparse-index does not handle these 'git checkout' commands correctly. Or rather, it _does_ reject the 'git checkout' when we have the staged change, but for the wrong reason. It also rejects the 'git checkout' commands when there is no staged change and we want to replace a directory with a file. A fix for that unstaged case will follow in the next change, but that will make the sparse-index agree with the full checkout case in these documented incorrect behaviors. Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

When running unpack_trees() with a sparse index, we attempt to operate on the index without expanding the sparse directory entries. Thus, we operate by manipulating entire directories and passing them to the unpack function. In the case of the 'git checkout' command, this is the twoway_merge() function. There are several cases in twoway_merge() that handle different situations. One new one to add is the case of a directory/file conflict where the directory is sparse. Before the sparse index, such a conflict would appear as a list of file additions and deletions. Now, twoway_merge() initializes 'current', 'oldtree', and 'newtree' from src[0], src[1], and src[2], then sets 'oldtree' to NULL because it is equal to the df_conflict_entry. The way to determine that we have a directory/file conflict is to test that 'current' and 'newtree' disagree on being sparse directory entries. When we are in this case, we want to resolve the situation by calling merged_entry(). This allows replacing the 'current' entry with the 'newtree' entry. This is important for cases where we want to run 'git checkout' across the conflict and have the new HEAD represent the new file type at that path. The first NEEDSWORK comment dropped in t1092 demonstrates this necessary behavior. However, we still are in a confusing state when 'current' corresponds to a staged change within a sparse directory that is not present at HEAD. This should be atypical, because it requires adding a change outside of the sparse-checkout cone, but it is possible. Since we are unable to determine that this is a staged change within twoway_merge(), we cannot add a case to reject the merge at this point. I believe this is due to the use of df_conflict_entry in the place of 'oldtree' instead of using the valud at HEAD, which would provide some perspective to this decision. Any change that would allow this differentiation for staged entries would need to involve information further up in unpack_trees(). That work should be done, sometime, because we are further confusing the behavior of a directory/file conflict when staging a change in the directory. The two cases 'checkout behaves oddly with df-conflict-?' in t1092 demonstrate that even without a sparse-checkout, Git is not consistent in its behavior. Neither of the two options seems correct, either. This change makes the sparse-index behave differently than the typcial sparse-checkout case, but it does match the full checkout behavior in the df-conflict-2 case. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Conflicts can occur outside of the sparse-checkout definition, and in that case users might try to resolve the conflicts in several ways. Document a few of these ways in a test. Make it clear that this behavior is not necessarily the optimal flow, since users can become confused when Git deletes these files from the worktree in later commands. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: 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. 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. 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. This modification to 'git add' creates some behavior change depending on the use of a sparse index. We modify a test in t1092 to demonstrate these changes which will be remedied in future changes. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: 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. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Since b243012 (refresh_index(): add flag to ignore SKIP_WORKTREE entries, 2021-04-08), 'git add --refresh <path>' will output a warning message when the path is outside the sparse-checkout definition. The implementation of this warning happened in parallel with the sparse-index work to add ensure_full_index() calls throughout the codebase. Update this loop to have the proper logic that checks to see if the pathspec is outside the sparse-checkout definition. This avoids the need to expand the sparse directory entry and determine if the path is tracked, untracked, or ignored. We simply avoid updating the stat() information because there isn't even an entry that matches the path! Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

The --renormalize option updates the EOL conversions for the tracked files. However, the loop already ignores files marked with the SKIP_WORKTREE bit, so it will continue to do so with a sparse index because the sparse directory entries also have this bit set. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

dscho

I admittedly only glanced over the test code, but the rest looks pretty good and ready to be merged. Any fixups can be done on top, I would say.

diff.c

attr.c

dir.c

t/t7519-status-fsmonitor.sh

derrickstolee · 2021-08-23T00:44:19Z

I pushed a bunch of fixup! commits which will be squashed into the original commits when I push my next version upstream. I wanted to get those edits into this version before others start to work on top of it.

The one commit that is not a squash is rather trivial: 475a908 changes a test to expect passing, since it was fixed a while ago but the test was not marked as expecting success.

dscho · 2021-08-23T12:19:00Z

@derrickstolee it looks as if all of the Scalar tests failed. Even on Linux... And there seems to be a crash with signal 6 (SIGABRT, most likely a BUG?) in linux-gcc, in the second part, the one run with these settings:

 + export GIT_TEST_SPLIT_INDEX=yes
+ export GIT_TEST_MERGE_ALGORITHM=recursive
+ export GIT_TEST_FULL_IN_PACK_ARRAY=true
+ export GIT_TEST_OE_SIZE=10
+ export GIT_TEST_OE_DELTA_SIZE=5
+ export GIT_TEST_COMMIT_GRAPH=1
+ export GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1
+ export GIT_TEST_MULTI_PACK_INDEX=1
+ export GIT_TEST_ADD_I_USE_BUILTIN=1
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
+ export GIT_TEST_WRITE_REV_INDEX=1
+ export GIT_TEST_CHECKOUT_WORKERS=2

derrickstolee · 2021-08-23T12:36:02Z

@derrickstolee it looks as if all of the Scalar tests failed. Even on Linux... And there seems to be a crash with signal 6 (SIGABRT, most likely a BUG?) in linux-gcc, in the second part, the one run with these settings:

Right. It's almost guaranteed to be due to a36b4ec. Scalar tests were passing (outside of known flakes) before that. The GIT_TEST_MERGE_ALGORITHM=recursive is causing the BUG() somewhere, because I changed the testing strategy based on upstream feedback. Working on it.

sequencer.c

derrickstolee · 2021-08-23T16:12:52Z

This had a lot of churn this morning due to some feedback on the mailing list, resulting in finding some places where I wasn't protecting the sparse index well enough in the recursive merge strategy. That churn was more than I was hoping for.

I planned to merge this and let @vdye and @ldennington work off of vfs-2.33.0 for their future sparse-index integrations. However, this makes me think it would be valuable for me to squash in all the fixup! commits and only merge this after I have the clean set of commits. That might not be until tomorrow, so it might block their work.

What does everyone think of the plan? These are our options:

Merge as-is. The messy commits will go away with the v2.34.0 rebase, anyway. Other work can start immediately.
Squash in the changes, which needs to happen for upstream. This might cause the merge to be delays to Tomorrow or Wednesday, delaying others' work.

vdye · 2021-08-23T16:15:12Z

I don't mind the delay - I created a branch to start working on reset off of the tip of this branch, so I'll end up rebasing on to 2.33.0 before opening the pull request anyway.

ldennington · 2021-08-23T19:27:43Z

I also don't mind the delay - most of my day is being spent on trying to figure out https://github.com/github/git-fundamentals/issues/636.

The sparse index is tested with the FS Monitor hook and extension since f8fe49e (fsmonitor: integrate with sparse index, 2021-07-14). This test was very fragile because it shared an index across sparse and non-sparse behavior. Since that expansion and contraction could cause the index to lose its FS Monitor bitmap and token, behavior is fragile to changes in 'git sparse-checkout set'. Rewrite the test to use two clones of the original repo: full and sparse. This allows us to also keep the test files (actual, expect, trace2.txt) out of the repos we are testing with 'git status'. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

While the sparse-index is only enabled when core.sparseCheckoutCone is also enabled, it is possible for the user to modify the sparse-checkout file manually in a way that does not match cone-mode patterns. In this case, we should refuse to convert an index into a sparse index, since the sparse_checkout_patterns will not be initialized with recursive and parent path hashsets. Also silently return if there are no cache entries, which is a simple case: there are no paths to make sparse! Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

If cache_tree_update() returns a non-zero value, then it could not create the cache tree. This is likely due to a path having a merge conflict. Since we are already returning early, let's return silently to avoid making it seem like we failed to write the index at all. If we remove our dependence on the cache tree within convert_to_sparse(), then we could still recover from this scenario and have a sparse index. When constructing the cache-tree extension in convert_to_sparse(), it is possible that we construct a tree object that is new to the object database. Without the WRITE_TREE_MISSING_OK flag, this results in an error that halts our conversion to a sparse index. Add this flag to remove this limitation. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

``` 6e74958 p2000: add 'git checkout -' test and decrease depth 3e1d03c p2000: compress repo names cd94f82 commit: integrate with sparse-index 65e79b8 sparse-index: recompute cache-tree e9a9981 checkout: stop expanding sparse indexes 4b801c8 t1092: document bad 'git checkout' behavior 71e3015 unpack-trees: resolve sparse-directory/file conflicts 5e96df4 t1092: test merge conflicts outside cone defab1b add: allow operating on a sparse-only index 9fc4313 pathspec: stop calling ensure_full_index 0ec03ab add: ignore outside the sparse-checkout in refresh() adf5b15 add: remove ensure_full_index() with --renormalize ``` These commits are equivalent to those already in `next` via gitgitgadget#999. ``` 80b8d6c Merge branch 'sparse-index/add' into stolee/sparse-index/add ``` This merge resolves conflicts with some work that happened in parallel, but is already in upstream `master`. ``` c407b2c t7519: rewrite sparse index test 9dad0d2 sparse-index: silently return when not using cone-mode patterns 2974920 sparse-index: silently return when cache tree fails e7cdaa0 unpack-trees: fix nested sparse-dir search 347410c sparse-checkout: create helper methods 4537233 attr: be careful about sparse directories 5282a86 sparse-index: add SPARSE_INDEX_MEMORY_ONLY flag 3a2f316 sparse-checkout: clear tracked sparse dirs fb47b56 sparse-checkout: add config to disable deleting dirs ``` These commits are the ones under review as of gitgitgadget#1009. Recent review made this less stable. It's a slightly different and more robust version of #396. > Note: I'm still not done with the feedback for upstream, but the remaining feedback is "can we add tests that cover these tricky technical bits?" and in `microsoft/git` these are already covered by the Scalar functional tests (since that's how they were found). ``` 080b02c diff: ignore sparse paths in diffstat d91a647 merge: make sparse-aware with ORT df49b5f merge-ort: expand only for out-of-cone conflicts cdecb85 t1092: add cherry-pick, rebase tests 0c1ecfb sequencer: ensure full index if not ORT strategy 406dfbe sparse-index: integrate with cherry-pick and rebase ``` These commits integrate with `git merge`, `git cherry-pick`, `git revert`, and `git rebase` as of gitgitgadget#1019. This got some feedback that changed how the tests were working so they are more robust. This led to a new commit (0c1ecfb). ``` cbb0ab3 Merge branch 'sparse-index/merge' into vfs-2.33.0 acb8623 t7524: test no longer fails ``` Finally, the commits are merged into `vfs-2.33.0` and also we include a fix to a `microsoft/git` test that is no longer broken.

This branch is exactly #410, but with one more commit: enabling the sparse index by default in d59110a. Having this in the `vfs-2.33.0` branch helps build confidence that the sparse index is doing what it should be doing by running in the Scalar functional tests and in our test branches. If we want to cut a new `microsoft/git` release without enabling the sparse index, we can simply revert this commit.