Skip to content

refactor: change extra_data to extra_fields#833

Open
Its-Just-Nans wants to merge 11 commits into
masterfrom
refactor-extra-data
Open

refactor: change extra_data to extra_fields#833
Its-Just-Nans wants to merge 11 commits into
masterfrom
refactor-extra-data

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

Part 1 of the extra field rewrite

@Its-Just-Nans Its-Just-Nans self-assigned this May 11, 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.

This PR successfully refactors the extra field handling system by introducing a CustomExtraField type to replace the previous raw Vec<u8> approach. The changes improve type safety and code organization while maintaining backward compatibility. All modifications follow consistent patterns across the codebase, and the test suite has been appropriately updated to reflect the new API. No blocking issues identified.


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.

Comment thread src/write.rs Fixed
Comment thread src/write.rs Fixed
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 ZIP extra field management by replacing raw byte buffers in ExtendedFileOptions with a structured CustomExtraField type, updating the writer and associated tests accordingly. Feedback highlights a bug in CustomExtraField::new_from_raw where the payload incorrectly includes size bytes, and identifies a redundant bounds check. Furthermore, an optimization was suggested to minimize memory allocations when collecting serialized extra fields.

Comment thread src/extra_fields/mod.rs Outdated
Comment thread src/extra_fields/mod.rs Outdated
Comment thread src/write.rs Outdated
Comment thread src/write.rs Fixed
Comment thread src/write.rs Fixed
Comment thread src/write.rs Fixed
Comment thread src/write.rs Fixed
@Its-Just-Nans Its-Just-Nans requested a review from Pr0methean May 11, 2026 20:13
@Pr0methean Pr0methean mentioned this pull request May 14, 2026
1 task
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.

This is a partial review (covers all except src/write.rs).

Comment thread src/extra_fields/mod.rs Outdated
Comment thread src/extra_fields/mod.rs
Comment thread src/extra_fields/mod.rs
Comment thread src/extra_fields/mod.rs Outdated
Comment thread src/extra_fields/mod.rs Outdated
Comment thread src/write.rs Outdated
Comment thread README.md
Its-Just-Nans and others added 4 commits May 18, 2026 17:58
Co-authored-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Signed-off-by: n4n5 <git@n4n5.dev>
Co-authored-by: Chris Hennick <4961925+Pr0methean@users.noreply.github.com>
Signed-off-by: n4n5 <git@n4n5.dev>
@Its-Just-Nans
Copy link
Copy Markdown
Member Author

@Pr0methean

Thanks for the review. Note that this MR is only the PART 1 of the extra field review.
I'm splitting the MR in multiple MRs for an easier review. But note that some code on this MR will change with the next one

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

Its-Just-Nans commented May 19, 2026

CI is showing a bug in the latest nightly

cargo +nightly miri test --target aarch64_be-unknown-linux-gnu --all --no-default-features zip64_with_leading_junk

EDIT: see #843

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