Check object existence when creating a tree from an index #4840
Conversation
This file contains 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
…ndex When the index does not belong to any repository, we do not do any checks of the target id going in as we cannot verify that it exists. When we then write it out to a repository as a tree, we fail to perform the object existance and type-matching check that we do in other code-paths. This leads to being able to write trees which point to non-existent blobs even with strict object creation enabled.
We have two similar functions, `git_treebuilder_insert` and `append_entry` which are used in different codepaths as part of creating a new tree. The former learnt to check for object existence under strict object creation, but the latter did not. This allowed the creation of a tree from an unowned index to bypass some of the checks and create a tree pointing to a nonexistent object. Extract a single function which performs these checks and call it from both codepaths. In `append_entry` we still do not validate when asked not to, as this is data which is already in the tree and we want to allow users to deal with repositories which already have some invalid data.
pks-t
reviewed
Oct 11, 2018
tests/checkout/tree.c
Outdated
| cl_git_pass(git_index_write_tree(&tree_id, index)); | ||
| cl_git_pass(git_libgit2_opts(GIT_OPT_ENABLE_STRICT_OBJECT_CREATION, 1)); |
There was a problem hiding this comment.
Doesn't this somewhat contradict the sentiment of not verifying object availability for tree entries that have been read from the index and that have not been manually added?
There was a problem hiding this comment.
If we never verify anything that comes from an index, that means you can avoid the strict object creation check very easily by chance by using an unowned index.
I would say the "sentiment" is about not erroring out when reading in data that's already in the repository rather than allowing untouched entries to point to missing objects.
There was a problem hiding this comment.
This is icky, though. Since you're about to go on holiday, I took the liberty of dropping this commit and fixing the index so that it's not pointing to a nonexistent blob.
ethomson
force-pushed
the
cmn/validity-tree-from-unowned-index
branch
from
fda7a1e
to
57c367c
Compare
The testrepo test fixture has an index file that's damaged, missing an object. The index previously had an entry of `src/index.c` with id 3161df8cbf3a006b4ef85be6497a0ea6bde98541, but that object was missing in the repository. This commit adds an object to the repository and updates the index to use that existing blob. Similarly, the index has an entry for `readme` with an id of 97328ac7e3bd0bcd3900cb3e7a624d71dd0df888. This can be restored from other test repositories. With these fixed, now the write tree from index tests can pass since they validate object existence.
ethomson
force-pushed
the
cmn/validity-tree-from-unowned-index
branch
from
57c367c
to
c79e608
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We have
git_treebuilder_insertandappend_entrywhich are used by different codepaths to append entries to the tree we're about to create. The checks in the latter were deficient as it never learnt to verify the target exists like the former did.This can lead to creating a tree pointing to an invalid objects if the index was unonwed and thus it itself could not do the checks during the index entry insertion.
Extract a single function to do these checks and use it in both. In
append_entrywe still avoid checking those entries that already existed in a tree we read from disk to allow users to work with repositories which already have some invalid data.