Skip to content
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

feat: Allow the archive offset behavior of the reader to be configured. #109

Merged
merged 3 commits into from
May 30, 2024

Conversation

afranchuk
Copy link
Contributor

Aside from supporting the current behavior which allows archives to be preceded by arbitrary data (added in fc749a09), this also allows detection of the offset to use by checking whether a central directory header is at the expected location. This is configurable because if the behavior were based only on detection, there could be false positives if archive data happened to contain a central directory header at the right spot.

The motivation for this was to support slightly non-standard central directory locations (such as described here).

@afranchuk
Copy link
Contributor Author

Reading the PR guidelines, I realize that I need to sign my commit. I'll add my keys and do that (though it may be in 2 weeks because I'm going to be away).

Copy link
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 has some room for improvement.

src/read.rs Outdated Show resolved Hide resolved
src/read/config.rs Outdated Show resolved Hide resolved
src/write.rs Outdated Show resolved Hide resolved
@Pr0methean
Copy link
Member

Be aware that #93 may cause conflicts with this PR if it's merged first.

@Pr0methean
Copy link
Member

@afranchuk When should we expect another revision of this PR? An ETA will be helpful for planning around the other major PRs that are currently open.

src/read/config.rs Outdated Show resolved Hide resolved
@afranchuk
Copy link
Contributor Author

@Pr0methean I'm working on cleaning it up now (just returned yesterday).

Aside from supporting the current behavior which allows archives to be
preceded by arbitrary data (added in fc749a09), this also allows
detection of the offset to use by checking whether a central directory
header is at the expected location. This is configurable because if the
behavior were based only on detection, there could be false positives if
archive data happened to contain a central directory header at the right
spot.
src/read.rs Show resolved Hide resolved
@Pr0methean Pr0methean enabled auto-merge May 29, 2024 22:46
@Pr0methean Pr0methean added waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. and removed Please address review comments Some review comments are still open. labels May 29, 2024
@Pr0methean Pr0methean added this pull request to the merge queue May 30, 2024
Merged via the queue into zip-rs:master with commit 6539545 May 30, 2024
38 checks passed
@Pr0methean Pr0methean removed the waiting to be automatically merged If this PR isn't automatically merged, suspect a broader issue. label Jul 6, 2024
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