code review 12421043: archive/zip: allow user-extensible compression methods

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.

[Dustin, 2013-08-03 23:18:29]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
http://go.googlecode.com/hg/

[bradfitz, 2013-08-05 13:52:32]

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....
src/pkg/archive/zip/reader.go:47: // Decompressor is a function that wraps a
Reader with a decompressing Reader.
but it returns a Closer too. who closes it? when?

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader....
src/pkg/archive/zip/reader.go:55: // RegisterDecompressor allows custom
decompressors for a specified method ID
end in a period.

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader....
src/pkg/archive/zip/reader.go:57: decompressors[method] = d
I wonder if this should panic on duplicate registration. We seem to do that in
most other places. (http in mux and NPN alternate proto)

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/writer.go
File src/pkg/archive/zip/writer.go (right):

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/writer....
src/pkg/archive/zip/writer.go:33: // Compressor is a function that wraps a
Writer with a compressing Writer.
same comment

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/writer....
src/pkg/archive/zip/writer.go:41: // RegisterCompressor allows custom
compressors for a specified method ID
same

[Dustin, 2013-08-05 17:54:09]

Thanks for the review.

I uploaded a new changeset which hopefully resolves all these issues.

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....
src/pkg/archive/zip/reader.go:47: // Decompressor is a function that wraps a
Reader with a decompressing Reader.
On 2013/08/05 13:52:32, bradfitz wrote:
> but it returns a Closer too. who closes it? when?

Done.

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader....
src/pkg/archive/zip/reader.go:55: // RegisterDecompressor allows custom
decompressors for a specified method ID
On 2013/08/05 13:52:32, bradfitz wrote:
> end in a period.

Done.

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader....
src/pkg/archive/zip/reader.go:57: decompressors[method] = d
On 2013/08/05 13:52:32, bradfitz wrote:
> I wonder if this should panic on duplicate registration. We seem to do that in
> most other places. (http in mux and NPN alternate proto)

Done.

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/reader....
src/pkg/archive/zip/reader.go:57: decompressors[method] = d
On 2013/08/05 13:52:32, bradfitz wrote:
> I wonder if this should panic on duplicate registration. We seem to do that in
> most other places. (http in mux and NPN alternate proto)

I was thinking this would allow a user to replace Deflate with something else
(e.g. higher compression).  It's probably not the right place for that, though.

Since this increases complexity some, should there be tests for duplicate
registration?

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/writer.go
File src/pkg/archive/zip/writer.go (right):

https://codereview.appspot.com/12421043/diff/8001/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 13:52:32, bradfitz wrote:
> same comment

Done.

https://codereview.appspot.com/12421043/diff/8001/src/pkg/archive/zip/writer....
src/pkg/archive/zip/writer.go:41: // RegisterCompressor allows custom
compressors for a specified method ID
On 2013/08/05 13:52:32, bradfitz wrote:
> same

Done.

[bradfitz, 2013-08-05 18:03:23]

Maybe update the consts too:

const (
    Store   uint16 = 0
    Deflate uint16 = 8
)

Or would adding LZMA there be distracting?

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.
Maybe:

// A Compressor returns a compressing writer, writing to the
// provided writer. On Close, any pending data should be flushed.

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.
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.

[Dustin, 2013-08-05 18:12:45]

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.

[bradfitz, 2013-08-05 23:37:27]

LGTM but will let adg finish review & submit.

[bradfitz, 2013-08-05 23:38:26]

Actually he's off in tropical paradise or something.

I'll submit tomorrow morning if there are no objections before then.



On Mon, Aug 5, 2013 at 4:37 PM, <bradfitz@golang.org> wrote:

> LGTM but will let adg finish review & submit.
>
>
>
https://codereview.appspot.**com/12421043/<https://codereview.appspot.com/124...
>

[rsc, 2013-08-05 23:40:47]

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...
src/pkg/archive/zip/reader.go:60: if _, ok := decompressors[method]; ok {
this should be locked

[Dustin, 2013-08-06 02:23:58]

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.

[bradfitz, 2013-08-06 04:04:22]

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

[Dustin, 2013-08-06 05:09:06]

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.

[bradfitz, 2013-08-06 15:04:35]

Looks like you haven't submitted a CLA.

See http://golang.org/doc/contribute.html#copyright and let me know when
that's done.



On Mon, Aug 5, 2013 at 10:09 PM, <dsallings@gmail.com> wrote:

> 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.
>
>
https://codereview.appspot.**com/12421043/<https://codereview.appspot.com/124...
>

[Dustin, 2013-08-06 17:25:46]

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).

[bradfitz, 2013-08-06 19:03:06]

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...
>

[bradfitz, 2013-08-06 19:03:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=f9f44b2d4d79 ***

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.

R=golang-dev, bradfitz, rsc
CC=golang-dev
https://codereview.appspot.com/12421043

Committer: Brad Fitzpatrick <bradfitz@golang.org>