Added support for the legacy compressions (shrink/reduce/implode). #120

mkrueger · 2024-05-12T21:16:32Z

For a project that will encounter old .zip files I need a rust library that supports shrink/reduce and implode.
I didn't find any so I ported the hw-zip 2.2 implementations to rust: https://www.hanshq.net/zip.html

Pr0methean

I like the idea behind this PR, but the implementation isn't idiomatic Rust and wouldn't be very inefficient. I may have more changes to request once these are done.

src/compression.rs

src/legacy/bitstream.rs

src/legacy/reduce.rs

src/legacy/shrink.rs

Pr0methean · 2024-05-13T00:20:15Z

src/legacy/shrink.rs

+struct Codetab {
+    prefix_code: u16, // INVALID_CODE means the entry is invalid.
+    ext_byte: u8,
+    len: u16,
+    last_dst_pos: usize,
+}


Suggested change

struct Codetab {

prefix_code: u16, // INVALID_CODE means the entry is invalid.

ext_byte: u8,

len: u16,

last_dst_pos: usize,

}

#[repr(packed)]

struct Codetab {

last_dst_pos: usize,

prefix_code: Option<u16>,

ext_byte: u8,

len: u16

}

This should fit into 128 bits on 64-bit platforms and 96 bits on 32-bit platforms, all with no penalty for an unaligned access.

Rust doesn't like that - I get errors. Why repr(packed) ?

It prevents any alignment padding. Usually scalar values are aligned on their size, and arrays on their element size.

src/legacy/shrink.rs

mkrueger · 2024-05-13T03:15:41Z

Wow - thanks for the code review :). Will look into it.

So I tried to go through the comments. Maybe I've forgotton some - there were many.

For short: I've looked into bitstream-io a bit. I'm not sure if it's worth switching to it. It just pulls in another dependency for just ~60 LOC or so. A different thing is the huffman implementation IMO.

I'll try to search for a huffman crate that can be used. bitstream-io contains some huffman stuff as well so it may be worth using - but haven't looked really into it.
My main goal for that was compatibility - I've some shrink/implode .zips in my bbs archive huge test case and I was able to extract those. Not sure if it's really worth putting way more time into exploring to use 3rd party libraries for that.
Esp. given in the amount of code required.
Code cleanup & use rust features - for sure. C code is quite ugly - even if I've a very good implementation from hwzip.

src/legacy/reduce.rs

Pr0methean · 2024-05-13T18:14:50Z

src/legacy/shrink.rs

+struct Codetab {
+    prefix_code: u16, // INVALID_CODE means the entry is invalid.
+    ext_byte: u8,
+    len: u16,
+    last_dst_pos: usize,
+}


It prevents any alignment padding. Usually scalar values are aligned on their size, and arrays on their element size.

src/compression.rs

src/legacy/bitstream.rs

src/legacy/reduce.rs

zmodem

Hey, hwzip author here :)

This is great to see! I'm only a Rust newbie, so probably can't help much with the review, but am happy to answer any questions about the original code.

src/legacy/huffman.rs

Pr0methean

I managed to get through over half of the PR this time!

src/legacy/bitstream.rs

src/legacy/huffman.rs

Pr0methean · 2024-05-14T18:34:26Z

src/legacy/huffman.rs

+            self.sentinel_bits[len] = s;
+            debug_assert!(self.sentinel_bits[len] >= code[len] as u32, "No overflow!");
+            sym_idx[len] = sym_idx[len - 1] + count[len - 1];
+            self.offset_first_sym_idx[len] = sym_idx[len].wrapping_sub(code[len]);


If this wrapped around, would it indicate a bug? If so, use - instead of wrapping_sub (behaves the same in release builds but is checked for overflow in debug builds).

I don't know for sure at least unit tests fail.

src/legacy/huffman.rs

Pr0methean · 2024-05-14T19:13:10Z

src/legacy/implode.rs

+        ImplodeDecoder {
+            compressed_reader: inner,
+            uncompressed_size,
+            stream_read: false,


It's usually not the custom Read impl that decides whether or not to chunk -- those who want chunking will wrap this in a BufReader, and it's best to be prepared for that eventuality even if it's unlikely.

src/legacy/implode.rs

src/legacy/reduce.rs

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

This reverts commit fafdaf1.

Signed-off-by: Chris Hennick <[email protected]>

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

This reverts commit 5d3654e.

Signed-off-by: Chris Hennick <[email protected]>

src/legacy/shrink.rs

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Pr0methean · 2024-05-23T23:27:02Z

cargo clippy --all-targets --all-features is failing; please fix.

Signed-off-by: Chris Hennick <[email protected]>

Pr0methean · 2024-07-20T22:01:13Z

This PR probably needs to have its rebase manually adjusted due to #210.

Pr0methean requested changes May 13, 2024

View reviewed changes

Pr0methean added Please address review comments Some review comments are still open. Please fix failing tests Tests are failing with this change; please fix them. labels May 13, 2024

mkrueger force-pushed the legacy-zip branch 2 times, most recently from e35d857 to a2f1236 Compare May 13, 2024 08:45

Pr0methean requested changes May 13, 2024

View reviewed changes

Pr0methean reviewed May 13, 2024

View reviewed changes

src/compression.rs Outdated Show resolved Hide resolved

src/legacy/bitstream.rs Outdated Show resolved Hide resolved

Pr0methean requested changes May 13, 2024

View reviewed changes

src/legacy/reduce.rs Outdated Show resolved Hide resolved

src/legacy/reduce.rs Show resolved Hide resolved

Pr0methean mentioned this pull request May 14, 2024

Initial support for tokio #121

Open

Pr0methean added the major This >~1000 SLOC PR is likely to cause hard-to-resolve conflicts with other open PRs once merged. label May 14, 2024

zmodem reviewed May 14, 2024

View reviewed changes

src/legacy/huffman.rs Outdated Show resolved Hide resolved

Pr0methean requested changes May 14, 2024

View reviewed changes

mkrueger and others added 17 commits May 14, 2024 21:41

Added support for the legacy compressions (shrink/reduce/implode).

b368df4

Update src/legacy/bitstream.rs

7568f40

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Update src/legacy/reduce.rs

77e247d

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Formatted some comments.

ad8efb6

Added some code review changes.

334b110

Update src/legacy/shrink.rs

bb8d030

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Use Option instead of C like invalid values.

905eeea

Removed comments.

45d79f6

huffman_decode now returns a Result.

db40e5b

Removed legacy-zip feature.

4189412

Refactored huffman implementation a bit.

fafdaf1

read_next_byte no longer uses out param.

45f0f66

Some minor cleanups.

68f3142

Added suggestions from PR review.

d8067eb

Update src/legacy/huffman.rs

d1b7efd

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Revert "Refactored huffman implementation a bit."

6f57340

This reverts commit fafdaf1.

Some refactorings in huffman code.

cdc08b9

Pr0methean and others added 11 commits May 19, 2024 21:04

Fix reuse of name n in src/legacy/huffman.rs

3698198

Signed-off-by: Chris Hennick <[email protected]>

Added lsb tests/refactored to u8.

9d60e97

style: Fix cargo-fmt complaints in huffman.rs

1936dd6

Signed-off-by: Chris Hennick <[email protected]>

Use follower set fixed size array.

3c60457

Update src/legacy/reduce.rs

415d7de

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Update src/legacy/shrink.rs

2dbfc99

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Code cleanups.

9aa2f1e

Update src/legacy/shrink.rs

5d3654e

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Update src/legacy/implode.rs

4377176

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Revert "Update src/legacy/shrink.rs"

885d17c

This reverts commit 5d3654e.

Fix build.

fb948ee

mkrueger force-pushed the legacy-zip branch from 6bd20b1 to fb948ee Compare May 21, 2024 08:59

mkrueger and others added 4 commits May 21, 2024 11:22

Use Vec in CodeQueue.

a2f73e8

Merge branch 'master' into legacy-zip

9703a77

Merge branch 'master' into legacy-zip

c986a73

Merge branch 'master' into legacy-zip

06b4a4b

Signed-off-by: Chris Hennick <[email protected]>

Pr0methean reviewed May 21, 2024

View reviewed changes

src/legacy/shrink.rs Outdated Show resolved Hide resolved

mkrueger and others added 4 commits May 22, 2024 12:47

Merge branch 'master' into legacy-zip

e36f600

Update src/legacy/shrink.rs

ab2a9b1

Co-authored-by: Chris Hennick <[email protected]> Signed-off-by: Mike Krüger <[email protected]>

Merge branch 'master' into legacy-zip

a01bb22

Merge branch 'master' into legacy-zip

b4fd8d6

Pr0methean added 4 commits May 27, 2024 17:42

Merge branch 'master' into legacy-zip

dcb4c98

Signed-off-by: Chris Hennick <[email protected]>

Merge branch 'master' into legacy-zip

d5e44bb

Signed-off-by: Chris Hennick <[email protected]>

Merge branch 'master' into legacy-zip

a4428cd

Merge branch 'master' into legacy-zip

abbb5ec

Signed-off-by: Chris Hennick <[email protected]>

Pr0methean added Please fix rebase conflicts Please rebase this PR against master and fix the conflicts. and removed Please fix failing tests Tests are failing with this change; please fix them. labels Jul 20, 2024

Merge branch 'master' into legacy-zip

e581456

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for the legacy compressions (shrink/reduce/implode). #120

Added support for the legacy compressions (shrink/reduce/implode). #120

mkrueger commented May 12, 2024

Pr0methean left a comment •

edited

Loading

Pr0methean May 13, 2024

mkrueger May 13, 2024

Pr0methean May 13, 2024

mkrueger commented May 13, 2024 •

edited

Loading

Pr0methean May 13, 2024

zmodem left a comment

Pr0methean left a comment

Pr0methean May 14, 2024

mkrueger May 14, 2024

Pr0methean May 14, 2024

Pr0methean commented May 23, 2024

Pr0methean commented Jul 20, 2024 •

edited

Loading

Added support for the legacy compressions (shrink/reduce/implode). #120

Are you sure you want to change the base?

Added support for the legacy compressions (shrink/reduce/implode). #120

Conversation

mkrueger commented May 12, 2024

Pr0methean left a comment • edited Loading

Choose a reason for hiding this comment

Pr0methean May 13, 2024

Choose a reason for hiding this comment

mkrueger May 13, 2024

Choose a reason for hiding this comment

Pr0methean May 13, 2024

Choose a reason for hiding this comment

mkrueger commented May 13, 2024 • edited Loading

Pr0methean May 13, 2024

Choose a reason for hiding this comment

zmodem left a comment

Choose a reason for hiding this comment

Pr0methean left a comment

Choose a reason for hiding this comment

Pr0methean May 14, 2024

Choose a reason for hiding this comment

mkrueger May 14, 2024

Choose a reason for hiding this comment

Pr0methean May 14, 2024

Choose a reason for hiding this comment

Pr0methean commented May 23, 2024

Pr0methean commented Jul 20, 2024 • edited Loading

Pr0methean left a comment •

edited

Loading

mkrueger commented May 13, 2024 •

edited

Loading

Pr0methean commented Jul 20, 2024 •

edited

Loading