repo/commit: Split up metadata/content commit paths#881
Closed
cgwalters wants to merge 6 commits intoostreedev:masterfrom
Closed
repo/commit: Split up metadata/content commit paths#881cgwalters wants to merge 6 commits intoostreedev:masterfrom
cgwalters wants to merge 6 commits intoostreedev:masterfrom
Conversation
First, the streaming metadata API is pretty dumb, since metadata should be small. Really we should have supported a `GBytes` version. Currently, this API *is* used when we do local pulls, so this commit has test coverage. However, I plan to change the object import to avoid using this. But that's fine, since I can't think of why someone would use this API. Next, the only difference between `ostree_repo_write_metadata()` and `ostree_repo_write_metadata_trusted()` is whether or not we pass an output checksum; so just dedup the implementations. Also while I'm here break out the input length validation and do it early in the streaming case.
Similar to metadata, for `write_content_trusted()` we can just call `_write_content()` with a `NULL` output checksum.
If we have an expected checksum, call `fstatat(repo_dfd, checksum)` early on before we do much else. This actually duplicates code, but future work here is going to split up the metadata/content commit paths, so they'll need to diverge anyways.
As the comment in the code says; in the expected checksum case, the caller really has to have a normal form already.
There was a lot of conditionals inside `write_object()` differentating between metadata/content, and then for content, on the different repo types. Further, in the metadata path since the logic is simpler, can present a non-streaming API, and further use `OtTmpfile`, etc. Splitting them up helps drop a lot of conditionals. We introduce a small `CleanupUnlinkat` that allows us to fully convert to the new code style in both functions. This itself is still prep for fully switching to `GLnxTmpfile`.
Member
Author
|
Prep for #861 |
Member
Author
|
Medium risk, needs some careful review. I'm likely to add some test cases here too to cover some of the API. May split out some of the prep inside this as a separate PR. |
cgwalters
added a commit
to cgwalters/ostree
that referenced
this pull request
May 24, 2017
Member
|
Looks good! So I think this is sane. Just want to take a second quick look in the terminal. |
Member
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 1, 2017
Similar to metadata, for `write_content_trusted()` we can just call `_write_content()` with a `NULL` output checksum. Closes: #881 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 1, 2017
If we have an expected checksum, call `fstatat(repo_dfd, checksum)` early on before we do much else. This actually duplicates code, but future work here is going to split up the metadata/content commit paths, so they'll need to diverge anyways. Closes: #881 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 1, 2017
As the comment in the code says; in the expected checksum case, the caller really has to have a normal form already. Closes: #881 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 1, 2017
There was a lot of conditionals inside `write_object()` differentating between metadata/content, and then for content, on the different repo types. Further, in the metadata path since the logic is simpler, can present a non-streaming API, and further use `OtTmpfile`, etc. Splitting them up helps drop a lot of conditionals. We introduce a small `CleanupUnlinkat` that allows us to fully convert to the new code style in both functions. This itself is still prep for fully switching to `GLnxTmpfile`. Closes: #881 Approved by: jlebon
|
☀️ Test successful - status-atomicjenkins |
cgwalters
added a commit
to cgwalters/ostree
that referenced
this pull request
Jun 12, 2017
This came up in: ostreedev#881 Basically doing streaming for metadata is dumb. Split up the metadata/content paths so we pass metadata around as `GVariant`. This drops the last internal caller of `ostree_repo_write_metadata_stream_trusted()` which was the dumb function mentioned.
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 12, 2017
This came up in: #881 Basically doing streaming for metadata is dumb. Split up the metadata/content paths so we pass metadata around as `GVariant`. This drops the last internal caller of `ostree_repo_write_metadata_stream_trusted()` which was the dumb function mentioned. Closes: #923 Approved by: jlebon
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
3 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.
There was a lot of conditionals inside
write_object()differentatingbetween metadata/content, and then for content, on the different repo
types. Further, in the metadata path since the logic is simpler, can
present a non-streaming API, and further use
OtTmpfile, etc.Splitting them up helps drop a lot of conditionals. We introduce a small
CleanupUnlinkatthat allows us to fully convert to the new code style in bothfunctions.
This itself is still prep for fully switching to
GLnxTmpfile.