-
Notifications
You must be signed in to change notification settings - Fork 145
unencapsulate: use "inner" stream when finishing Decompressor #1415
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
unencapsulate: use "inner" stream when finishing Decompressor #1415
Conversation
|
This was quite the exercise in fighting the type system, but I think this is generally what we want... |
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.
Code Review
This pull request introduces a Decompressable trait to handle flushing the underlying stream when a decompressor finishes, preventing UnexpectedEof errors. The implementation is clean, and tests for incomplete gzip and zstd data are included. A suggestion is provided to optimize TransparentDecompressor.
147c387 to
cc52892
Compare
|
Trying something new... added a semi-casual commit just to keep things clear as feedback is addressed during the review process. We'll squash those out at the end. |
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.
Nice! There's more code motion here but I think it really makes things a good bit cleaner to have all the gory details hidden inside that module.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
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.
These all look great!
|
Let's avoid merging main into PRs, can you just squash everything? |
^^^ Always intended on squashing it, which is why I just merged main in for now. More to the theme of making it easier to follow what's going on as we iterate here instead of force pushing repeatedly. |
|
In general let's follow a SOP of keeping things draft if they're not ready to merge - and being "ready to merge" should include having a reasonably clean commit history where fixups are squashed. |
This moves all of the code related to handling decompression out of container/unencapsulate.rs and into a new module `generic_decompress`. The only exposed API is via the existing (relocated) `Decompressor` type. Internal to `generic_decompress` this adds a new trait `ReadWithGetInnerMut`, which allows access to the original, inner, un-decompressed stream. This is used when finishing the decompressor, whether explicitly through calling its `finish()` method, or implicitly by dropping it. For things like GzDecoder, we don't want to read via the actual decompression reader because we don't care about decompressing at this point. Plus, the inner reader may have encountered an error partway through, and trying to decode via decompression will error with UnexpectedEof. Instead, wrap a reader for each content type which implements `ReadWithGetInnerMut`. When we finish decompressing, use the trait method `get_inner_mut()` to read directly from inner stream to flush any data. Resolves: bootc-dev#1407 Signed-off-by: John Eckersberg <[email protected]>
c3c5f5e to
75d5e71
Compare
For things like GzDecoder, we don't want to read via the actual
decompression reader because we don't care about decompressing at this
point. Plus, the inner reader may have encountered an error partway
through, and trying to decode via decompression will error with
UnexpectedEof.
Instead, wrap a reader for each content type which knows how to get a
mutable reference to the underlying data stream. When we finish
decompressing, read directly from this "inner" stream to flush any
data.
Resolves: #1407
Signed-off-by: John Eckersberg [email protected]