archive/zip: allow user-extensible compression methods
This change replaces the hard-coded switch on compression method
in zipfile reader and writer with a map into which users can
register compressors and decompressors in their init()s.
https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader.go#newcode47 src/pkg/archive/zip/reader.go:47: // Decompressor is a function that wraps a Reader ...
11 years, 7 months ago
(2013-08-05 13:52:32 UTC)
#2
Thanks so much for the doc help. I'm bad at Englishing. I don't think it ...
11 years, 7 months ago
(2013-08-05 18:12:45 UTC)
#5
Thanks so much for the doc help. I'm bad at Englishing.
I don't think it makes sense to include lzma in the predefined consts. I can
add it (and others) if you'd like, though.
There are actually a lot in the spec:
0 - The file is stored (no compression)
1 - The file is Shrunk
2 - The file is Reduced with compression factor 1
3 - The file is Reduced with compression factor 2
4 - The file is Reduced with compression factor 3
5 - The file is Reduced with compression factor 4
6 - The file is Imploded
7 - Reserved for Tokenizing compression algorithm
8 - The file is Deflated
9 - Enhanced Deflating using Deflate64(tm)
10 - PKWARE Data Compression Library Imploding (old IBM TERSE)
11 - Reserved by PKWARE
12 - File is compressed using BZIP2 algorithm
13 - Reserved by PKWARE
14 - LZMA (EFS)
15 - Reserved by PKWARE
16 - Reserved by PKWARE
17 - Reserved by PKWARE
18 - File is compressed using IBM TERSE (new)
19 - IBM LZ77 z Architecture (PFS)
97 - WavPack compressed data
98 - PPMd version I, Rev 1
https://codereview.appspot.com/12421043/diff/15001/src/pkg/archive/zip/writer.go
File src/pkg/archive/zip/writer.go (right):
https://codereview.appspot.com/12421043/diff/15001/src/pkg/archive/zip/writer...
src/pkg/archive/zip/writer.go:33: // Compressor is a function that wraps a
Writer with a compressing Writer.
On 2013/08/05 18:03:23, bradfitz wrote:
> Maybe:
>
> // A Compressor returns a compressing writer, writing to the
> // provided writer. On Close, any pending data should be flushed.
Done.
https://codereview.appspot.com/12421043/diff/15001/src/pkg/archive/zip/writer...
src/pkg/archive/zip/writer.go:44: // RegisterCompressor allows custom
compressors for a specified method ID.
On 2013/08/05 18:03:23, bradfitz wrote:
> Change the verb from "allows" to "registers":
>
> // RegisterCompressor registers a compressor for a specified method ID.
> // The common methods Store and Deflate are built in.
Done.
LGTM https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go#newcode60 src/pkg/archive/zip/reader.go:60: if _, ok := decompressors[method]; ok { this ...
11 years, 7 months ago
(2013-08-05 23:40:47 UTC)
#8
https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go#newcode60 src/pkg/archive/zip/reader.go:60: if _, ok := decompressors[method]; ok { On 2013/08/05 ...
11 years, 7 months ago
(2013-08-06 02:23:58 UTC)
#9
https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):
https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader...
src/pkg/archive/zip/reader.go:60: if _, ok := decompressors[method]; ok {
On 2013/08/05 23:40:47, rsc wrote:
> this should be locked
The intention is to do all this stuff during init() -- certainly not very
dynamically. Would you prefer that be more clearly documented or I add locks
around all the map accesses?
A third possibility is to do something more like what crypto/hash does and use a
slice large enough to hold known values. It's a bit less flexible, though.
It's a uint16 type, but the spec only lists through 98.
I can do whichever.
https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go#newcode53 src/pkg/archive/zip/reader.go:53: var decompressors = map[uint16]Decompressor{ make this: var ( demu ...
11 years, 7 months ago
(2013-08-06 04:04:22 UTC)
#10
https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):
https://codereview.appspot.com/12421043/diff/18002/src/pkg/archive/zip/reader...
src/pkg/archive/zip/reader.go:53: var decompressors = map[uint16]Decompressor{
make this:
var (
demu sync.RWMuytex // guards decompressors
decompressors = ....
)
And likewise in the other file with, say, "comu".
We only really need one lock, though. I just want it above the variables it
guards.
Alternatively, move all this new stuff to "register.go" and do one var blocl:
var (
mu sync.RWMutex // guards following two maps
decompressors = ...
compressors = ....
)
func Register
func Register
etc
I did the register.go thing. I copied the header from another file and noticed after ...
11 years, 7 months ago
(2013-08-06 05:09:06 UTC)
#11
I did the register.go thing. I copied the header from another file and noticed
after upload it's copyright 2010.
Bonus is that it does make the reader and writer changes much smaller. Because
of the introduction of locks, I added functions for accessing the method via the
rlock.
Sorry about that. I filled that the contributor thing with this gmail address (not the ...
11 years, 7 months ago
(2013-08-06 17:25:46 UTC)
#13
Sorry about that. I filled that the contributor thing with this gmail address
(not the address I generally use, but I figured that'd be easier for you guys).
LGTM This turned out well. We can add more addresses to the CONTRIBUTORS file if ...
11 years, 7 months ago
(2013-08-06 19:03:06 UTC)
#14
LGTM
This turned out well.
We can add more addresses to the CONTRIBUTORS file if you'd like.
On Tue, Aug 6, 2013 at 10:25 AM, <dsallings@gmail.com> wrote:
> Sorry about that. I filled that the contributor thing with this gmail
> address (not the address I generally use, but I figured that'd be easier
> for you guys).
>
>
https://codereview.appspot.**com/12421043/<https://codereview.appspot.com/124...
>
Issue 12421043: code review 12421043: archive/zip: allow user-extensible compression methods
Created 11 years, 7 months ago by Dustin
Modified 11 years, 7 months ago
Reviewers:
Base URL:
Comments: 18