Closed
Conversation
The variables here were duplicative; we don't need two booleans to distinguish between symlinks and regular files. What we do need to handle is the "physical" state versus the "object" state. Symlinks objects are stored as regular files in `bare-user` and `archive`. Prep for more cleanup.
The pull code also could make use of this in both the metadata and content paths. I changed it to own the tempfile malloc (just like `GLnxTmpFile`), since there's no reason to have different lifetimes for the filename and the file, and that way we only have one variable rather than two. The content path turns out to be a special case though, where at least for mirroring archives, we directly pass the file *path* down into `_ostree_repo_commit_loose_final()`. This is prep for `GLnxTmpFile` porting.
The `OstreeRepoContentBareCommit` struct was basically an `OtTmpFile`, so let's make it one. I moved the "convert to `GOutputStream`" logic into the callers, since that bit can't fail; it makes the implementation much simpler since we can just return the result of `ot_open_tmpfile_linkable_at()`. Prep for `GLnxTmpfile` porting.
cgwalters
changed the title
Closed
jlebon
reviewed
Jun 26, 2017
|
|
||
| if (file_input != NULL) | ||
| g_object_unref (file_input); | ||
| /* Include the terminating zero so we can e.g. mmap this file */ |
Member
There was a problem hiding this comment.
One thing I just noticed here. We're creating a new stream that has just the target name + nul, but down there where we create the tmpfile for it, we're still using the size from the GFileInfo, so we don't actually end up writing the trailing zero, right?
Member
Author
There was a problem hiding this comment.
We do write it actually, because we drop into the g_output_stream_splice() path. But our fallocate() is wrong.
I'm actually kind of dubious about the value of writing it, since it doesn't make sense to mmap small files. But let's keep it. Fixup and new commit ⬇️
src/libostree/ostree-repo-pull.c
Outdated
| goto out; | ||
| } | ||
|
|
||
| /* Now delete it, keeping the fd open as the last reference); see comment in |
We need to account for the trailing NUL.
Member
|
☀️ Test successful - status-atomicjenkins |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 26, 2017
The variables here were duplicative; we don't need two booleans to distinguish between symlinks and regular files. What we do need to handle is the "physical" state versus the "object" state. Symlinks objects are stored as regular files in `bare-user` and `archive`. Prep for more cleanup. Closes: #957 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 26, 2017
The pull code also could make use of this in both the metadata and content paths. I changed it to own the tempfile malloc (just like `GLnxTmpFile`), since there's no reason to have different lifetimes for the filename and the file, and that way we only have one variable rather than two. The content path turns out to be a special case though, where at least for mirroring archives, we directly pass the file *path* down into `_ostree_repo_commit_loose_final()`. This is prep for `GLnxTmpFile` porting. Closes: #957 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 26, 2017
The `OstreeRepoContentBareCommit` struct was basically an `OtTmpFile`, so let's make it one. I moved the "convert to `GOutputStream`" logic into the callers, since that bit can't fail; it makes the implementation much simpler since we can just return the result of `ot_open_tmpfile_linkable_at()`. Prep for `GLnxTmpfile` porting. Closes: #957 Approved by: jlebon
rh-atomic-bot
pushed a commit
that referenced
this pull request
Jun 26, 2017
We need to account for the trailing NUL. Closes: #957 Approved by: jlebon
Member
Author
|
Weird, github failed to close this. |
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.
A few patches in prep for #958.