Issue 4523077: code review 4523077: archive/zip: add Writer (Closed)
|
Created:
13 years, 11 months ago by adg Modified:
13 years, 10 months ago Reviewers:
CC:
bradfitz, dchest, r, rsc, golang-dev Visibility:
Public. |
Descriptionarchive/zip: add Writer
Patch Set 1 #
Total comments: 17
Patch Set 2 : diff -r 4ce4c75f9bb5 https://go.googlecode.com/hg/ #
Total comments: 8
Patch Set 3 : diff -r 4ce4c75f9bb5 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 4 : diff -r da6592e6f18b https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 29f2dc9c98e7 https://go.googlecode.com/hg/ #
Total comments: 18
Patch Set 6 : diff -r 29f2dc9c98e7 https://go.googlecode.com/hg/ #Patch Set 7 : diff -r 8a4444cbb46f https://go.googlecode.com/hg/ #Patch Set 8 : diff -r ce10904a2c0b https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 9 : diff -r 6c73563e7fe6 https://go.googlecode.com/hg/ #
MessagesTotal messages: 22
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Misc... http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go#ne... src/pkg/archive/zip/reader.go:55: // OpenReader will open the Zip file specified by name and return a ReadCloser. kinda a funny type name (just noticing, unrelated to your CL). what's the difference between a zip.Reader and zip.ReadCloser? http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go#ne... src/pkg/archive/zip/reader.go:136: case DEFLATE: all caps here but initial caps with Store? http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/struct.go#ne... src/pkg/archive/zip/struct.go:6: DEFLATE uint16 = 8 case inconsistency http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:40: return os.NewError("closing Writer before interior file") "zip: ..." http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:91: func (w *Writer) Create(name string, method uint16) (io.WriteCloser, os.Error) { this is the convenience method. is method worth it here, or rare enough to force them to use CreateHeader? is it bad to just always compress? if so, can we detect early when it's bad (after a 256 kb or something) and do the appropriate thing? (if this is a convenience method, after all) also, should method be its own type, not uint16? http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:194: if e, ok := recover().(os.Error); ok { you're repeating these lines enough to warrant a helper function: defer recoverWriteError(&err) func recoverWriteError(err *os.Error) { ... } http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:217: type nopCloser struct { ioutil.NopCloser
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go#ne... src/pkg/archive/zip/reader.go:55: // OpenReader will open the Zip file specified by name and return a ReadCloser. On 2011/05/18 14:22:12, bradfitz wrote: > kinda a funny type name (just noticing, unrelated to your CL). > > what's the difference between a zip.Reader and zip.ReadCloser? Close on zip.ReadCloser closes the underlying zip file. See http://codereview.appspot.com/4372047 zip.Reader doesn't have Close method, because it's created with io.ReaderAt -- it's a caller's responsibility to close the file or whatever reader it has.
Sign in to reply to this message.
On Wed, May 18, 2011 at 9:15 AM, <dchest@gmail.com> wrote: > > http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go > File src/pkg/archive/zip/reader.go (right): > > > http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go#ne... > src/pkg/archive/zip/reader.go:55: // OpenReader will open the Zip file > specified by name and return a ReadCloser. > On 2011/05/18 14:22:12, bradfitz wrote: > >> kinda a funny type name (just noticing, unrelated to your CL). >> > > what's the difference between a zip.Reader and zip.ReadCloser? >> > > Close on zip.ReadCloser closes the underlying zip file. > See http://codereview.appspot.com/4372047 Sorry, my question was more rhetorical than serious. I can read the code, I just don't like the interface... it seems oddly named and sparsely documented: http://golang.org/pkg/archive/zip/ ReadCloser sounds like an interface. ReadCloser looks like the common thing, so I'd expect it to be named "zip.Reader". The current zip.Reader looks like a weird case so I'd call it "zip.PartReader" or SectionReader, keeping the short clean name for the common case. > > zip.Reader doesn't have Close method, because it's created with > io.ReaderAt -- it's a caller's responsibility to close the file or > whatever reader it has. > > > http://codereview.appspot.com/4523077/ >
Sign in to reply to this message.
On 2011/05/18 16:20:28, bradfitz wrote: > Sorry, my question was more rhetorical than serious. I can read the code, I > just don't like the interface... it seems oddly named and sparsely > documented: > > http://golang.org/pkg/archive/zip/ > > ReadCloser sounds like an interface. ReadCloser looks like the common > thing, so I'd expect it to be named "zip.Reader". The current zip.Reader > looks like a weird case so I'd call it "zip.PartReader" or SectionReader, > keeping the short clean name for the common case. Sounds good to me. I think if zip.Reader was called zip.PartReader originally, the bug with OpenReader providing no way to close the file would have been avoided.
Sign in to reply to this message.
Changing the names is beyond the scope of this CL. Also I don't buy the argument that ReadCloser sounds like an interface. So does Reader but it's not, here. It's a pretty common name for an implementation too. There's a tar.Reader, flate.Reader, etc. Russ
Sign in to reply to this message.
On May 18, 2011, at 10:34 AM, Russ Cox wrote: > Changing the names is beyond the scope of this CL. > > Also I don't buy the argument that ReadCloser sounds > like an interface. So does Reader but it's not, here. It's a > pretty common name for an implementation too. > There's a tar.Reader, flate.Reader, etc. ReadCloserThanIsConsideredPolite. -rob
Sign in to reply to this message.
On Wed, May 18, 2011 at 10:34 AM, Russ Cox <rsc@google.com> wrote: > Changing the names is beyond the scope of this CL. > That's fine... I just noticed it. Maybe in the future. > Also I don't buy the argument that ReadCloser sounds > like an interface. So does Reader but it's not, here. It's a > pretty common name for an implementation too. > There's a tar.Reader, flate.Reader, etc. > Which is why I'd expect a zip.Reader to be the common zip reader. It seems that zip.Reader currently is the weird case. I don't care a ton, but it feels a bit inconsistent/gross.
Sign in to reply to this message.
> Which is why I'd expect a zip.Reader to be the common zip reader. It seems > that zip.Reader currently is the weird case. > I don't care a ton, but it feels a bit inconsistent/gross. It depends what you think common means. tar.Reader and flate.Reader and zip.Reader all let you read from the data. None of them let you close an underlying file. zip.ReadCloser lets you do both. That's the unusual case. Russ
Sign in to reply to this message.
I have a few doc/interface fixes in mind. I'll create a CL later today and let's debate there. On 2011/05/18 17:50:07, rsc1 wrote: > > Which is why I'd expect a zip.Reader to be the common zip reader. It seems > > that zip.Reader currently is the weird case. > > I don't care a ton, but it feels a bit inconsistent/gross. > > It depends what you think common means. > tar.Reader and flate.Reader and zip.Reader all let > you read from the data. None of them let you close > an underlying file. zip.ReadCloser lets you do both. > That's the unusual case. > > Russ
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go#ne... src/pkg/archive/zip/reader.go:55: // OpenReader will open the Zip file specified by name and return a ReadCloser. On 2011/05/18 14:22:12, bradfitz wrote: > kinda a funny type name (just noticing, unrelated to your CL). > > what's the difference between a zip.Reader and zip.ReadCloser? One you can Read, one you can Read and should Close. Seems obvious to me. As said, this discussion belongs in a new CL. I just want my zip writer reviewed damnit! :-) http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/reader.go#ne... src/pkg/archive/zip/reader.go:136: case DEFLATE: On 2011/05/18 14:22:12, bradfitz wrote: > all caps here but initial caps with Store? DEFLATE is often called DEFLATE. I don't really care. Anyone feel strongly about this? http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/struct.go#ne... src/pkg/archive/zip/struct.go:6: DEFLATE uint16 = 8 On 2011/05/18 14:22:12, bradfitz wrote: > case inconsistency Changed. http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:40: return os.NewError("closing Writer before interior file") On 2011/05/18 14:22:12, bradfitz wrote: > "zip: ..." Done. http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:91: func (w *Writer) Create(name string, method uint16) (io.WriteCloser, os.Error) { On 2011/05/18 14:22:12, bradfitz wrote: > this is the convenience method. is method worth it here, or rare enough to > force them to use CreateHeader? > > is it bad to just always compress? if so, can we detect early when it's bad > (after a 256 kb or something) and do the appropriate thing? (if this is a > convenience method, after all) This is worth considering for the helper method Create only (allowing the user to specify method with CreateHeader). First 256kb seems like a lot. We should have some idea about compressibility after the first 4kb maybe? I'd like to get this CL in with that being a TODO, and then work on it from there. http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:194: if e, ok := recover().(os.Error); ok { On 2011/05/18 14:22:12, bradfitz wrote: > you're repeating these lines enough to warrant a helper function: > > defer recoverWriteError(&err) > > func recoverWriteError(err *os.Error) { > ... > } Done. http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:217: type nopCloser struct { On 2011/05/18 14:22:12, bradfitz wrote: > ioutil.NopCloser Nope. NopCloser is a ReadCloser. I need a WriteCloser here.
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:91: func (w *Writer) Create(name string, method uint16) (io.WriteCloser, os.Error) { On 2011/05/18 22:58:40, adg wrote: > On 2011/05/18 14:22:12, bradfitz wrote: > > this is the convenience method. is method worth it here, or rare enough to > > force them to use CreateHeader? > > > > is it bad to just always compress? if so, can we detect early when it's bad > > (after a 256 kb or something) and do the appropriate thing? (if this is a > > convenience method, after all) > > This is worth considering for the helper method Create only (allowing the user > to specify method with CreateHeader). First 256kb seems like a lot. We should > have some idea about compressibility after the first 4kb maybe? > > I'd like to get this CL in with that being a TODO, and then work on it from > there. TODO is fine. Bigger question is whether anybody cares in a helper function. I'd remove the "method" param here entirely and always DEFLATE. If I want to STORE I can use CreateHeader, right? I don't care strongly, though. http://codereview.appspot.com/4523077/diff/1/src/pkg/archive/zip/writer.go#ne... src/pkg/archive/zip/writer.go:217: type nopCloser struct { On 2011/05/18 22:58:40, adg wrote: > On 2011/05/18 14:22:12, bradfitz wrote: > > ioutil.NopCloser > > Nope. NopCloser is a ReadCloser. I need a WriteCloser here. bleh
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:20: type Writer struct { one-liner comment? http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:85: // It returns a WriteCloser to which the file's contents should be written. you return a WriteCloser but only speak of writing to it. I had this same debate in multipart writer. you should explicitly say whether closing is optional or required. http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:173: return os.NewError("zip: file closed twice") I think double-Close is generally permitted. I saw a comment elsewhere about assuming Close was idempotent, always returning the same error. I'd love to have this clarified to me one way or another, but that's been my assumption so far. http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer_t... File src/pkg/archive/zip/writer_test.go (right): http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer_t... src/pkg/archive/zip/writer_test.go:19: fp, err := os.Create(testFile) Could you do this all in-memory instead of hitting the disk? Our weak ARM & Windows buildbots would be grateful.
Sign in to reply to this message.
LGTM once those comments are added. Hitting memory instead of disk in the test is optional but should be easy. On Wed, May 18, 2011 at 5:28 PM, <bradfitz@google.com> wrote: > > > http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.go > > File src/pkg/archive/zip/writer.go (right): > > > http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... > src/pkg/archive/zip/writer.go:20: type Writer struct { > one-liner comment? > > > http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... > src/pkg/archive/zip/writer.go:85: // It returns a WriteCloser to which > the file's contents should be written. > you return a WriteCloser but only speak of writing to it. I had this > same debate in multipart writer. you should explicitly say whether > closing is optional or required. > > > http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... > src/pkg/archive/zip/writer.go:173: return os.NewError("zip: file closed > twice") > I think double-Close is generally permitted. I saw a comment elsewhere > about assuming Close was idempotent, always returning the same error. > > I'd love to have this clarified to me one way or another, but that's > been my assumption so far. > > > http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer_t... > File src/pkg/archive/zip/writer_test.go (right): > > > http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer_t... > src/pkg/archive/zip/writer_test.go:19: fp, err := os.Create(testFile) > Could you do this all in-memory instead of hitting the disk? Our weak > ARM & Windows buildbots would be grateful. > > > http://codereview.appspot.com/4523077/ >
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:20: type Writer struct { On 2011/05/19 00:28:52, bradfitzgoog wrote: > one-liner comment? Done. http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:85: // It returns a WriteCloser to which the file's contents should be written. On 2011/05/19 00:28:52, bradfitzgoog wrote: > you return a WriteCloser but only speak of writing to it. I had this same > debate in multipart writer. you should explicitly say whether closing is > optional or required. Updated to say it's required. http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:173: return os.NewError("zip: file closed twice") On 2011/05/19 00:28:52, bradfitzgoog wrote: > I think double-Close is generally permitted. I saw a comment elsewhere about > assuming Close was idempotent, always returning the same error. > > I'd love to have this clarified to me one way or another, but that's been my > assumption so far. Let's find out. For now, it'll be nil. http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer_t... File src/pkg/archive/zip/writer_test.go (right): http://codereview.appspot.com/4523077/diff/13001/src/pkg/archive/zip/writer_t... src/pkg/archive/zip/writer_test.go:19: fp, err := os.Create(testFile) On 2011/05/19 00:28:52, bradfitzgoog wrote: > Could you do this all in-memory instead of hitting the disk? Our weak ARM & > Windows buildbots would be grateful. Done.
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/6007/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/6007/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:103: return nil, os.NewError("zip: creating file before previous file closed") I would prefer you just Close the previous one here for them, like I did in multipart/writer. I returned a Closer just for people who like closure, or need a Closer for some other API they're using. But if our APIs don't _require_ one (and they don't), we can do the right thing by default. Better than us returning a Writer and them wrapping it in a WriteCloser which is worse than the mild protections we could provide. If they continue to write to the old one after they call CreateHeader, they'll at least get an error on their old-part Write. If they're ignoring all their errors, meh.
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/6007/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/6007/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:103: return nil, os.NewError("zip: creating file before previous file closed") On 2011/05/20 01:05:35, bradfitz wrote: > I would prefer you just Close the previous one here for them, like I did in > multipart/writer. I returned a Closer just for people who like closure, or need > a Closer for some other API they're using. But if our APIs don't _require_ one > (and they don't), we can do the right thing by default. > > Better than us returning a Writer and them wrapping it in a WriteCloser which is > worse than the mild protections we could provide. > > If they continue to write to the old one after they call CreateHeader, they'll > at least get an error on their old-part Write. > > If they're ignoring all their errors, meh. > Done.
Sign in to reply to this message.
http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:3: // These constants define the compression Methods supported by this package. // Compression methods. no idea why M is capitalized; these are uint16s. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:33: // NewWriter returns a new Writer writing a new zip file to w. s/new zip/zip/ http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:38: // Close finishes writing the zip file by writing the central directory. // It does not (and can not) close the underlying writer. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:94: // Closing the WriteCloser is optional. If so, should it be in the API at all? An io.Writer is easier for clients and probably simplifies this code too. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:95: // Files must be added to a Writer sequentially. The file's contents must be written to the io.Writer before the next call to Create, CreateHeader, or Close. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:97: // TODO(adg): use some heurestic to select compression method flate will already do that for you. I'd just assume deflate and let it do its thing. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:100: Method: Store, s/Store/Deflate/ http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:130: fw.comp = flate.NewWriter(fw.rawCount, 9) 9 is usually overkill; I believe 5 is the common default. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:211: io.Writer w io.Writer As it is, any caller can see the .Writer field of a zip.Writer. The code is not using the embedding at all (it defines an explicit Write method) so might as well not embed and avoid the visibility.
Sign in to reply to this message.
Thanks. I'm still working out some bugs that'll need to be fixed before I submit. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:3: // These constants define the compression Methods supported by this package. On 2011/05/23 21:47:54, rsc wrote: > // Compression methods. > > no idea why M is capitalized; these are uint16s. Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:33: // NewWriter returns a new Writer writing a new zip file to w. On 2011/05/23 21:47:54, rsc wrote: > s/new zip/zip/ Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:38: // Close finishes writing the zip file by writing the central directory. On 2011/05/23 21:47:54, rsc wrote: > // It does not (and can not) close the underlying writer. Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:94: // Closing the WriteCloser is optional. On 2011/05/23 21:47:54, rsc wrote: > If so, should it be in the API at all? An io.Writer is easier for clients and > probably simplifies this code too. Removed. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:95: // Files must be added to a Writer sequentially. On 2011/05/23 21:47:54, rsc wrote: > The file's contents must be written to the io.Writer > before the next call to Create, CreateHeader, or Close. > Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:97: // TODO(adg): use some heurestic to select compression method On 2011/05/23 21:47:54, rsc wrote: > flate will already do that for you. > I'd just assume deflate and let it do its thing. Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:100: Method: Store, On 2011/05/23 21:47:54, rsc wrote: > s/Store/Deflate/ Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:130: fw.comp = flate.NewWriter(fw.rawCount, 9) On 2011/05/23 21:47:54, rsc wrote: > 9 is usually overkill; I believe 5 is the common default. Done. http://codereview.appspot.com/4523077/diff/8004/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:211: io.Writer On 2011/05/23 21:47:54, rsc wrote: > w io.Writer > > As it is, any caller can see the .Writer field of a zip.Writer. The code is not > using the embedding at all > (it defines an explicit Write method) so might as well > not embed and avoid the visibility. Done.
Sign in to reply to this message.
PTAL I finally found and fixed the bug in this. Had something backwards. It passes the tests and the archives it produces are readable by infozip and the zip reader in this package.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4523077/diff/23001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/23001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:231: if e, ok := recover().(os.Error); ok { you care if this silently catches non-os.Error panics without re-panicing them?
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=8c6071fa95f4 *** archive/zip: add Writer R=bradfitz, dchest, r, rsc CC=golang-dev http://codereview.appspot.com/4523077 http://codereview.appspot.com/4523077/diff/23001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/4523077/diff/23001/src/pkg/archive/zip/writer.g... src/pkg/archive/zip/writer.go:231: if e, ok := recover().(os.Error); ok { On 2011/07/10 00:44:18, bradfitz wrote: > you care if this silently catches non-os.Error panics without re-panicing them? Fixed.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
