Skip to content

refactor: update some extra field#821

Merged
Its-Just-Nans merged 8 commits intomasterfrom
update-extra-field
May 7, 2026
Merged

refactor: update some extra field#821
Its-Just-Nans merged 8 commits intomasterfrom
update-extra-field

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

Start to change to extra field

The function update_aes_extra_data is never tested (we can remove the call and all tests are fine)

I will update this branch with a test

@Its-Just-Nans Its-Just-Nans self-assigned this May 5, 2026
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 refactoring changes look good. The transition from Pod trait usage to explicit write() methods for extra field serialization improves code clarity and follows better practices. All call sites have been updated correctly to use the new APIs. No blocking issues found.


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.

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 refactors the serialization of AexEncryption and Zip64ExtendedInformation extra fields to write directly to a Write trait object, removing the dependency on the Pod trait and simplifying struct definitions. The review feedback identifies a critical missing import for size_of that would prevent compilation and suggests several performance optimizations, specifically using stack-allocated arrays and pre-allocating Vec capacity to avoid unnecessary heap allocations for fixed-size data.

Comment thread src/extra_fields/aex_encryption.rs
Comment thread src/write.rs Outdated
Comment thread src/write.rs Outdated
Comment thread src/write.rs Outdated
Its-Just-Nans and others added 2 commits May 6, 2026 00:22
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: n4n5 <git@n4n5.dev>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: n4n5 <git@n4n5.dev>
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.

One nitpick, and the Gemini feedback looks helpful as well.

Comment thread src/extra_fields/zip64_extended_information.rs Outdated
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>
@Its-Just-Nans Its-Just-Nans changed the title refactor: update some extra field DRAFT refactor: update some extra field May 5, 2026
@Its-Just-Nans
Copy link
Copy Markdown
Member Author

Yes thanks

But I'm drafting it wince I need to add a test for the update_ees function (never tested)

Comment thread src/extra_fields/aex_encryption.rs Fixed
Comment thread src/write.rs Fixed
@Its-Just-Nans Its-Just-Nans changed the title DRAFT refactor: update some extra field refactor: update some extra field May 7, 2026
Comment thread tests/aes_encryption.rs Dismissed
Comment thread tests/aes_encryption.rs Dismissed
@Its-Just-Nans
Copy link
Copy Markdown
Member Author

Its-Just-Nans commented May 7, 2026

I have fixed the nitpick
I commited a test for the function update_aes function e2fd83f
The CodeQL analysis is triggered on a test file. Note that all the passwords are hardcoded in this test file.

The MR will merge when ready

@Its-Just-Nans Its-Just-Nans enabled auto-merge May 7, 2026 13:28
@Its-Just-Nans Its-Just-Nans added this pull request to the merge queue May 7, 2026
Merged via the queue into master with commit 2f0d516 May 7, 2026
133 checks passed
@Its-Just-Nans Its-Just-Nans deleted the update-extra-field branch May 7, 2026 18:34
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.

3 participants