1) Remove reference of EntryReader to StoredEntry. 2) don't use opaque type in SyncStoredReader::reader

[muja]

Currently it's necessary to have 4 separate variables to read:

let f = std::fs::File(p)?;
let zip = f.read_zip()?;
let entry = zip.by_name("foo")?;
let reader = entry.reader();
// start reading

This PR makes it possible to combine lines 3 and 4:

let reader = zip.by_name("foo")?.reader();

It's probably also faster because the copies are very small and the entry needs not be derefenced here, but that was not my primary goal.

The 2nd part is the opaque type which makes it hard for users to specify it in non-impl places. I saw no benefit in that.

If desired, I can split the PRs per commit.

[fasterthanlime]

How would you feel about getting commit rights for this repo?

You seem to care about it more than I do right now, I’m happy to let you take it where you think it needs to go for now!

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (2197bdd) 75.06% compared to head (c2a5a05) 75.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   75.06%   75.10%   +0.04%     
==========================================
  Files          18       17       -1     
  Lines        1636     1639       +3     
==========================================
+ Hits         1228     1231       +3     
  Misses        408      408              

Impacted Files	Coverage Δ
crates/rc-zip/src/reader/sync/entry_reader.rs	70.19% <100.00%> (+0.81%)	⬆️
crates/rc-zip/src/reader/sync/read_zip.rs	95.45% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[fasterthanlime]

How would you feel about getting commit rights for this repo?

You seem to care about it more than I do right now, I’m happy to let you take it where you think it needs to go for now!

@muja I’ve just added you as collaborator for this repo — you may need to accept the invite. Once that’s done, feel free to merge this PR!

[muja]

I don't know if that's a good idea. I know next to nothing about the ZIP format and also wouldn't feel comfortable committing code here without having them reviewed since I also don't know about the usage patterns in the wild or when something's a breaking change. For that, the test coverage is too low I imagine. But maybe it's possible to reach out to past contributors whether they would be up for it.

[fasterthanlime]

I don't know if that's a good idea. I know next to nothing about the ZIP format and also wouldn't feel comfortable committing code here without having them reviewed since I also don't know about the usage patterns in the wild or when something's a breaking change. For that, the test coverage is too low I imagine. But maybe it's possible to reach out to past contributors whether they would be up for it.

So! There's no past contributors, not really (@Geal reviewed some early drafts but that's about it), and as you can see, rc-zip is barely used: https://lib.rs/crates/rc-zip/rev

You can go ahead and merge this PR - if you have doubts for subsequent PRs, feel free to ping me and I'll take a look.

[muja]

There are the authors of #17 and #18 who may be candidates. But I'll accept your invitation.

and as you can see, rc-zip is barely used: https://lib.rs/crates/rc-zip/rev

Yeah I was wondering about that. The fact that this can extract files in parallel and doesn't mutate the file makes this extremely handy.

Also I found out that circular exposes UB under Stacked Borrows.

[fasterthanlime]

Also I found out that circular exposes UB under Stacked Borrows.

That's a good point - I'm not sure it's actively being maintained, getting rid of the dependency could be a good exercise if you want to get more familiar with rc-zip!

I'm not sure I would go with the same architecture nowadays tbh. It's really awkward to work with. But.. it should be fairly correct.

[fasterthanlime]

I'm looking at this again, I like the changes with a couple tune-ups, I'll merge this as-is and adjust later!