-
Notifications
You must be signed in to change notification settings - Fork 85
fix #390 #402
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
base: master
Are you sure you want to change the base?
fix #390 #402
Conversation
Signed-off-by: Robert Jansen <[email protected]>
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.
Summary of Changes
Hello @0x7d8, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a specific bug related to ZIP64 file handling within the zip
crate. The change ensures that the large_file
flag is properly considered when validating file sizes against the ZIP64 threshold, thereby resolving an issue where an incorrect error message might be displayed for legitimately large files.
Highlights
- Bug Fix for Large File Handling: Introduced a conditional check in the
write_data_descriptor
function to ensure that thelarge_file
option is explicitly verified when a file's compressed size exceeds theZIP64_BYTES_THR
threshold. This prevents an erroneous "Large file option has not been set" error from being triggered if thelarge_file
flag is already correctly set.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 correctly fixes a bug where writing a large file with a data descriptor would fail. The change prevents an error when large_file
is enabled. However, the implementation makes the conditional statement where the fix is applied unreachable. I've suggested removing this now-dead code for improved clarity and maintainability.
if file.compressed_size > spec::ZIP64_BYTES_THR && !file.large_file { | ||
return Err(ZipError::Io(io::Error::other( | ||
"Large file option has not been set", | ||
))); |
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.
This else
block is only entered if file.data_descriptor_block()
returns None
, which, based on its implementation, only happens when file.large_file
is true
. Consequently, the added condition !file.large_file
will always evaluate to false
, making this entire if
statement and its body unreachable.
For better code clarity, it would be best to remove the now-unreachable if
block entirely (lines 2011-2015).
Per Gemini's comment, this condition seems to be unreachable. Shouldn't this instead be checked in the case where |
I havent investigated into the codebase much, this seems to resolve my issue without problem, so not sure |
hmm, seeming to recieve drop errors now actually, the entire stream impl seems a bit scuffed ZipWriter drop failed: Io(Custom { kind: Unsupported, error: "seek is not supported" }) |
Seems like the drop error was caused by the aes code, even now, without any errors (no aes enabled), it generates a broken zip file (file is shown as 0 bytes but with correct crc) |
fixes #390 by adding a check to make sure large_file actually isnt set