Skip to content

refactor: Remove ZipFileData fields#790

Merged
Pr0methean merged 7 commits intomasterfrom
remove-zipfiledata_fields
Apr 29, 2026
Merged

refactor: Remove ZipFileData fields#790
Pr0methean merged 7 commits intomasterfrom
remove-zipfiledata_fields

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Contributor

@amazon-q-developer amazon-q-developer Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR removes three redundant fields (encrypted, is_utf8, using_data_descriptor) from ZipFileData and replaces them with computed methods. The changes appear functionally correct with no logic errors or security issues. However, the PR title "Remove zipfiledata fields" does not conform to the Conventional Commits format required by the repository guidelines. The title should start with a type prefix like refactor:, fix:, feat:, etc. Consider updating it to something like: refactor: remove redundant fields from ZipFileData


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

@Its-Just-Nans Its-Just-Nans changed the title Remove zipfiledata fields refactor: Remove ZipFileData fields Apr 26, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 bumps the crate version to 9.0.0 and refactors the ZipFileData struct by removing the redundant encrypted, is_utf8, and using_data_descriptor boolean fields, instead deriving these states directly from the flags bitfield. Helper methods were introduced to maintain the API, and all call sites have been updated accordingly. Review feedback identified a logic error in the flags() method that causes data loss by discarding existing bits in the flags field, as well as a redundant UTF-8 check during file initialization.

Comment thread src/types.rs
Comment thread src/types.rs Outdated
Comment on lines +460 to +462
if std::str::from_utf8(&file_name_raw).is_ok() && !file_name.is_ascii() {
flags |= ZipFlags::LanguageEncoding.as_u16();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The check std::str::from_utf8(&file_name_raw).is_ok() is redundant here. file_name_raw is created directly from file_name.as_bytes() on line 428, and since file_name is a Box, it is guaranteed to be valid UTF-8.

Suggested change
if std::str::from_utf8(&file_name_raw).is_ok() && !file_name.is_ascii() {
flags |= ZipFlags::LanguageEncoding.as_u16();
}
if !file_name.is_ascii() {
flags |= ZipFlags::LanguageEncoding.as_u16();
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pr0methean

Yes but later the file_name could be not UTF-8

@Its-Just-Nans Its-Just-Nans added this to the 9.0.0 milestone Apr 26, 2026
@Its-Just-Nans Its-Just-Nans self-assigned this Apr 26, 2026
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Copy link
Copy Markdown
Member

@Pr0methean Pr0methean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, but please address the remaining Gemini suggestion.

@Its-Just-Nans
Copy link
Copy Markdown
Member Author

Mostly looks good, but please address the remaining Gemini suggestion.

I don't think gemini i really correct here

  1. it was always like that so far, and this MR is not about fixing an issue but just refactoring
  2. Gemini is technically rights but we don't really use the "compression settings" bits
        (For Method 6 - Imploding)
        Bit 1: If the compression method used was type 6,
               Imploding, then this bit, if set, indicates
               an 8K sliding dictionary was used.  If clear,
               then a 4K sliding dictionary was used.

        Bit 2: If the compression method used was type 6,
               Imploding, then this bit, if set, indicates
               3 Shannon-Fano trees were used to encode the
               sliding dictionary output.  If clear, then 2
               Shannon-Fano trees were used.

        (For Methods 8 and 9 - Deflating)
        Bit 2  Bit 1
          0      0    Normal (-en) compression option was used.
          0      1    Maximum (-exx/-ex) compression option was used.
          1      0    Fast (-ef) compression option was used.
          1      1    Super Fast (-es) compression option was used.

        (For Method 14 - LZMA)
        Bit 1: If the compression method used was type 14,
               LZMA, then this bit, if set, indicates
               an end-of-stream (EOS) marker is used to
               mark the end of the compressed data stream.
               If clear, then an EOS marker is not present
               and the compressed data size must be known
               to extract.

        Note:  Bits 1 and 2 are undefined if the compression
               method is any other.

Imploding -> we cannot compress with imploding with the current crate (decompress only)
Deflating -> technically yes we should set these bytes
LZMA -> we cannot compress with LZMA with the current crate (decompress only)

@Pr0methean Pr0methean added this pull request to the merge queue Apr 29, 2026
Merged via the queue into master with commit 515e840 Apr 29, 2026
133 checks passed
@Pr0methean Pr0methean deleted the remove-zipfiledata_fields branch April 29, 2026 23:47
This was referenced Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants