-
Notifications
You must be signed in to change notification settings - Fork 97
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
Conversation
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>
There was a problem hiding this 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.
ensure_not_expanded clean -fd && | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
# 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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>
There was a problem hiding this 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.
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Sparse index: integrate with `clean` and `stash -u`
Changes
git clean
git stash -u
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 tocheckout-index
doesn't cause major slowdowns: