Prefetch tests: Delete multi-pack-index before indexing a pack#1213
Merged
derrickstolee merged 1 commit intomicrosoft:masterfrom May 29, 2019
Merged
Conversation
This test deletes an existing .idx file, and then checks that VFS for Git calls `git index-pack` when scanning the pack-files. When we take Git 2.22.0, this test starts to fail. The fix is to delete the multi-pack-index before rebuilding the .idx. This is a reasonable expectation because we won't have a pack covered by the multi-pack-index without having the corresponding .idx file. The reason this broke with Git 2.22.0 is a bit subtle: When running `git index-pack`, Git looks for duplicate objects in order to check for SHA-1 collisions. When the multi-pack-index includes the objects in the pack, Git sees what it thinks are the same object ids and goes to load the content to verify they are equal. In Git 2.22.0, we solved the "many packs" problem by adding the pack-files from the multi-pack-index to the packed_git linked list. Adding to this list will fail if there is no corresponding .idx file. (The multi-pack-index prevents any reads into this .idx file, but existence is required.) Since the .idx file is not there, that add fails. This means that the content lookup fails and Git complains with "fatal: cannot read existing object info" Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Member
|
Excellent investigation, and even better explanation of the issue and its resolution. Thanks! |
dscho
approved these changes
May 29, 2019
Contributor
Author
|
/azp run GitHub VFSForGit Mac Functional Tests |
|
No pipelines are associated with this pull request. |
kewillford
approved these changes
May 29, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This test deletes an existing .idx file, and then checks that
VFS for Git calls
git index-packwhen scanning the pack-files.When we take Git 2.22.0, this test starts to fail. The fix is to
delete the multi-pack-index before rebuilding the .idx. This is
a reasonable expectation because we won't have a pack covered by
the multi-pack-index without having the corresponding .idx file.
The reason this broke with Git 2.22.0 is a bit subtle:
When running
git index-pack, Git looks for duplicate objects inorder to check for SHA-1 collisions. When the multi-pack-index
includes the objects in the pack, Git sees what it thinks are
the same object ids and goes to load the content to verify they
are equal.
In Git 2.22.0, we solved the "many packs" problem by adding the
pack-files from the multi-pack-index to the packed_git linked
list. Adding to this list will fail if there is no corresponding
.idx file. (The multi-pack-index prevents any reads into this
.idx file, but existence is required.) This means that the content
lookup fails and Git complains with "fatal: cannot read existing
object info"
This will unblock taking Git 2.22.0 after the release. See
microsoft/git#140 for details on that update.