Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse index: integrate with clean and stash -u #430

Merged
merged 3 commits into from
Sep 23, 2021
Merged

Sparse index: integrate with clean and stash -u #430

merged 3 commits into from
Sep 23, 2021

Conversation

vdye
Copy link
Collaborator

@vdye vdye commented Sep 22, 2021

Changes

  • Integrate sparse index changes into git clean
  • Clean up & document behavior in git stash -u
    • Fix bug in handling of outside-of-cone untracked files
    • Add index expansion & performance test
      • In the index expansion test, git stash -u does "expand the index". However, the index expanded is separate from the repository index, temporarily storing only the untracked files. The performance tests below also seem to confirm that this index expansion has a minimal effect on the execution time.

Performance

The performance results aren't as dramatic as in the first stash pull request (potentially because of background processes on my machine?), but still demonstrate a) a substantial improvement in performance from full to sparse index, and b) adding the --sparse flag to checkout-index doesn't cause major slowdowns:

Test                                                              ms/vfs-2.33.0     sparse-index/clean    
----------------------------------------------------------------------------------------------------------
2000.2: git stash && git stash pop (full-v3)                      3.93(2.65+1.19)   4.16(2.78+1.25) +5.9% 
2000.3: git stash && git stash pop (full-v4)                      3.97(2.71+1.18)   4.02(2.81+1.14) +1.3% 
2000.4: git stash && git stash pop (sparse-v3)                    1.38(0.32+1.66)   1.34(0.33+1.81) -2.9% 
2000.5: git stash && git stash pop (sparse-v4)                    1.38(0.33+1.76)   1.34(0.32+1.69) -2.9% 
2000.6: echo >>new && git stash -u && git stash pop (full-v3)     4.49(3.00+1.44)   4.41(2.95+1.43) -1.8% 
2000.7: echo >>new && git stash -u && git stash pop (full-v4)     4.48(3.02+1.42)   4.43(3.00+1.39) -1.1% 
2000.8: echo >>new && git stash -u && git stash pop (sparse-v3)   1.81(0.57+2.05)   1.62(0.40+1.95) -10.5%
2000.9: echo >>new && git stash -u && git stash pop (sparse-v4)   1.86(0.58+2.05)   1.67(0.42+1.99) -10.2%

Needed to maintain existing behavior for stash. This should not cause a major
performance hit, since the index being checked-out contains only the untracked
files stashed with a prior `git stash -u`.

Signed-off-by: Victoria Dye <vdye@github.com>
Copy link
Collaborator

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only holding off approval because I want to take a quick look at that temporary index thing.

Comment on lines +1205 to +1225
ensure_not_expanded clean -fd &&

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test that checks that git clean -fd correctly removes untracked files in sparse directories?

something like:

git sparse-checkout set deep/deeper1 &&
mkdir deep/deeper2 folder1 &&
touch deep/deeper2/untracked &&
touch folder1/untracked &&
git clean -fd &&
test_path_is_missing deep/deeper2/untracked &&
test_path_is_missing folder1/untracked

I expect that the index will be expanded to do this operation, but let's make sure it happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the existing git clean tests to check those cases and explicitly verify paths are present or missing. Also, I verified that the index is expanded when untracked files are inside a sparse directory, so no tests were added to the ensure_not_expanded test.

t/t1092-sparse-checkout-compatibility.sh Outdated Show resolved Hide resolved
Comment on lines +1285 to +1308
# NEEDSWORK: although the full repository's index is _not_ expanded as part of
# stash, a temporary index, which is _not_ sparse, is created when stashing and
# applying a stash of untracked files. As a result, the test reports that it
# finds an instance of `ensure_full_index`, but it does not carry with it the
# performance implications of expanding the full repository index.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good explanation of the problem. I wonder if there is something we can do to make creating this temporary index as a sparse one be better. Do you have a pointer to the place in the code that creates this index?

Copy link
Collaborator Author

@vdye vdye Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes from setting the GIT_INDEX_FILE in update-index (1, 2) and read-tree/checkout-index (1, 2) to temporary filename while either constructing the stash or applying it.

When creating the stash (with untracked entries included) update-index will be invoked while the temporary index file does not exist, causing it to enter this block in do_read_index - because it doesn't ever read the existing index, the sparse_index is never set and it's treated as a full index. I tried adding a check to set sparse_index based on the sparse_index repo setting and/or command_requires_full_index, but I hit some segfaults when testing, so it needed a closer look.

The more complicated case is on the unstashing side - read-tree also gets a non-existent index file, but it never actually reads in the index before writing out the result of unpack_trees (so I tried adding in do_read_index is never reached). In that case, the sparse_index flag is only ever set if a sparse directory is written to it, which (I think) won't happen when unpacking a tree without comparing to the index.

All that to say - I agree that there's more that can be done to preserve "sparseness" of the temp index. I think complications come up when dealing with untracked files in sparse directories (likely part of what caused the segfaults in my earlier testing), but I don't see it being impossible to handle.

Add repository setting to not require full index and test to verify index is
not expanded in `git clean`. Existing test for `git clean` expanded to verify
behavior when cleaning untracked files in sparse directories is consistent.

Signed-off-by: Victoria Dye <vdye@github.com>
Test cases specific to handling untracked files in `git stash` a) ensure
that files outside the sparse checkout definition are handled as-expected
and b) document the index expansion inside of `git stash -u`. Note that, in b),
it is not the full repository index that is expanded - it is the temporary,
standalone index containing the stashed untracked files only.

Signed-off-by: Victoria Dye <vdye@github.com>
Copy link
Collaborator

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to full approval, because the temporary index thing can be done separately.

@vdye vdye merged commit 7db06bd into microsoft:vfs-2.33.0 Sep 23, 2021
@vdye vdye deleted the sparse-index/clean branch September 23, 2021 14:49
dscho pushed a commit that referenced this pull request Oct 30, 2021
Sparse index: integrate with `clean` and `stash -u`
derrickstolee pushed a commit that referenced this pull request Oct 30, 2021
Sparse index: integrate with `clean` and `stash -u`
derrickstolee pushed a commit that referenced this pull request Oct 31, 2021
Sparse index: integrate with `clean` and `stash -u`
derrickstolee pushed a commit that referenced this pull request Nov 4, 2021
Sparse index: integrate with `clean` and `stash -u`
derrickstolee pushed a commit that referenced this pull request Nov 4, 2021
Sparse index: integrate with `clean` and `stash -u`
derrickstolee pushed a commit that referenced this pull request Nov 10, 2021
Sparse index: integrate with `clean` and `stash -u`
derrickstolee pushed a commit that referenced this pull request Nov 15, 2021
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 12, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 19, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 20, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 20, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 20, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 20, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 20, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 20, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 25, 2022
Sparse index: integrate with `clean` and `stash -u`
ldennington pushed a commit to ldennington/git that referenced this pull request Jan 25, 2022
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Feb 1, 2022
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jun 17, 2022
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jun 17, 2022
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jun 17, 2022
Sparse index: integrate with `clean` and `stash -u`
vdye added a commit that referenced this pull request Jun 1, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jul 7, 2023
Sparse index: integrate with `clean` and `stash -u`
vdye added a commit that referenced this pull request Jul 19, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Aug 8, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Aug 8, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Aug 11, 2023
Sparse index: integrate with `clean` and `stash -u`
jeffhostetler pushed a commit that referenced this pull request Aug 23, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Nov 3, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Nov 3, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Nov 3, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Nov 8, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Nov 14, 2023
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Nov 20, 2023
Sparse index: integrate with `clean` and `stash -u`
vdye added a commit that referenced this pull request Feb 27, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Apr 23, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Apr 23, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Apr 23, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Apr 24, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Apr 29, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request May 14, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request May 14, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jun 3, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jun 3, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jul 17, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jul 17, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jul 17, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jul 18, 2024
Sparse index: integrate with `clean` and `stash -u`
mjcheetham pushed a commit that referenced this pull request Jul 23, 2024
Sparse index: integrate with `clean` and `stash -u`
dscho pushed a commit that referenced this pull request Jul 25, 2024
Sparse index: integrate with `clean` and `stash -u`
mjcheetham pushed a commit that referenced this pull request Jul 29, 2024
Sparse index: integrate with `clean` and `stash -u`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants