|
|
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
don't do the CONTRIBUTORS change in your own CL. we'll take care of that separately. the codereview tool will let you mail the CL (although it will complain) even if you're not in the file. -rob
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:244: if tag == 0x0001 { // zip64 extra this is ugly code. plus what are all these magic numbers? http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:271: // will get the crc32 from here until that test is fixed. or you could fix the test or you could explain why it shouldn't be fixed. this is not the place for a remark like this.
Ok, it looked like a fatal error but maybe it wasnt. Thanks. On Wednesday, August 15, 2012 6:26:15 PM UTC+2, Rob Pike wrote: > > don't do the CONTRIBUTORS change in your own CL. we'll take care of > that separately. the codereview tool will let you mail the CL > (although it will complain) even if you're not in the file. > > -rob >
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:244: if tag == 0x0001 { // zip64 extra On 2012年08月15日 16:31:38, r wrote: > this is ugly code. plus what are all these magic numbers? Which part is ugly, the tag comparison or the whole loop? I can make a const for the tag but I dont think that sizes are magic and need consts. Maybe a comment about the zip64 extra format is enough? http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:271: // will get the crc32 from here until that test is fixed. On 2012年08月15日 16:31:38, r wrote: > or you could fix the test or you could explain why it shouldn't be fixed. this > is not the place for a remark like this. Agreed. But the file is broken and I have to remove it from the test or change the test so that it reports it as broken. Which one do you prefer?
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:271: // will get the crc32 from here until that test is fixed. On 2012年08月15日 21:37:39, serbaut wrote: > On 2012年08月15日 16:31:38, r wrote: > > or you could fix the test or you could explain why it shouldn't be fixed. this > > is not the place for a remark like this. > > Agreed. But the file is broken and I have to remove it from the test or change > the test so that it reports it as broken. Which one do you prefer? My bad. The file test should fail but it stopped failing when I ignored the data descriptor crc32 that the test messed with. So the test can remain.
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:244: if tag == 0x0001 { // zip64 extra On 2012年08月15日 21:37:39, serbaut wrote: > On 2012年08月15日 16:31:38, r wrote: > > this is ugly code. plus what are all these magic numbers? I can't come up with a simpler way to write this code. We have to unpack zero to four values depending on the size of the data. This is the spec for the block: -Zip64 Extended Information Extra Field (0x0001): The following is the layout of the zip64 extended information "extra" block. If one of the size or offset fields in the Local or Central directory record is too small to hold the required data, a Zip64 extended information record is created. The order of the fields in the zip64 extended information record is fixed, but the fields will only appear if the corresponding Local or Central directory record field is set to 0xFFFF or 0xFFFFFFFF. Note: all fields stored in Intel low-byte/high-byte order. Value Size Description ----- ---- ----------- (ZIP64) 0x0001 2 bytes Tag for this "extra" block type Size 2 bytes Size of this "extra" block Original Size 8 bytes Original uncompressed file size Compressed Size 8 bytes Size of compressed data Relative Header Offset 8 bytes Offset of local header record Disk Start Number 4 bytes Number of the disk on which this file starts This entry in the Local header must include BOTH original and compressed file size fields. If encrypting the central directory and bit 13 of the general purpose bit flag is set indicating masking, the value stored in the Local Header for the original file size will be zero.
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:255: _ = b.uint32() // ignore disk number what if size is > 28? won't we read too little? and why would the size ever be less than 24? http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:357: return -1 this seems like an error worth knowing about? http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:363: _ = b.uint32() // number of the disk with the start of the zip64 end of central directory everywhere you're doing this you should do b = b[4:] instead. there's no need to do the conversion just to throw it away http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:371: func readDirectory64End(r io.ReaderAt, offset int64, dir *directoryEnd) (err error) { s/dir/d/ http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:65: if h.CompressedSize64 > 0xffffffff || h.UncompressedSize64 > 0xffffffff || h.offset > 0xffffffff { perhaps header should have a method 'isZip64() bool', so this can be if h.isZip64() { http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:65: if h.CompressedSize64 > 0xffffffff || h.UncompressedSize64 > 0xffffffff || h.offset > 0xffffffff { the code above doesn't have comments because it's all self-explanatory for instance b.uint16(h.Flags) obviously means "write h.Flags as a uint16" but the code you have added has many mysterious constants and none of it is documented. please add comments to every non-obvious line explaining what is going on eg b.uint32(0xffffffff) // write zip64 directory header (or whatever it is) http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:65: if h.CompressedSize64 > 0xffffffff || h.UncompressedSize64 > 0xffffffff || h.offset > 0xffffffff { const uint32max = 0xffffffff would be very helpful here http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:105: directoryRecords := uint64(len(w.dir)) records size offset would be fine names here this entire function is about the directory http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:135: directoryRecords = 0xffff comment why http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:228: b.uint32(0) // since we are writing a data descriptor crc32, really? why? I didn't see that in the spec. http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:297: fh.ReaderVersion = 45 more magic numbers and shouldn't this happen earlier, under "update FileHeader" ?
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:255: _ = b.uint32() // ignore disk number On 2012年08月15日 23:56:29, adg wrote: > what if size is > 28? won't we read too little? > and why would the size ever be less than 24? Yes, if they change the spec and add something to zip64 extra that would be a problem. According to the spec you can have the 64 bit uncompressed size in the extra but the other fields in the normal so the size would be just 8 bytes. http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:357: return -1 On 2012年08月15日 23:56:29, adg wrote: > this seems like an error worth knowing about? directoryEndOffset-directory64LocLen could be < 0 but I can add a check for that and return any io error that follows. http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/reader.go... src/pkg/archive/zip/reader.go:363: _ = b.uint32() // number of the disk with the start of the zip64 end of central directory On 2012年08月15日 23:56:29, adg wrote: > everywhere you're doing this you should do > b = b[4:] > instead. there's no need to do the conversion just to throw it away It makes it easier to read and less magic imo. The conversion itself is free in the grand scheme of things. I can change it if you really want. http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:228: b.uint32(0) // since we are writing a data descriptor crc32, On 2012年08月15日 23:56:29, adg wrote: > really? why? I didn't see that in the spec. The spec is unclear. According to pkware: "When using the Data Descriptor, the values would be written as ZERO." http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7073588 The data descriptor should be used when you can't re-write the local header because you cant seek the output (streaming). An additional problem is that in the case of zip64 you don't even know if you need to allocate space in the local header for the zip64 extra. It is a mess :/
Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
http://codereview.appspot.com/6463050/diff/9001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/6463050/diff/9001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:121: b.uint16(45) // version made by please put 45 in a meaningful constant. zip64version ? is that what it is? http://codereview.appspot.com/6463050/diff/9001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:298: if fh.CompressedSize64 > 0xffffffff || fh.UncompressedSize64 > 0xffffffff { if fh.isZip64?
http://codereview.appspot.com/6463050/diff/9001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/6463050/diff/9001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:121: b.uint16(45) // version made by On 2012年08月17日 01:39:25, adg wrote: > please put 45 in a meaningful constant. > > zip64version ? is that what it is? It is "(zip) version needed to extract". Do you think b.uint16(zipVersion45) is better? I would have to change line 190 from "fh.ReaderVersion = 0x14" (no idea why that is in hex) to "fh.ReaderVersion = zipVersion20" in that case. http://codereview.appspot.com/6463050/diff/9001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:298: if fh.CompressedSize64 > 0xffffffff || fh.UncompressedSize64 > 0xffffffff { On 2012年08月17日 01:39:25, adg wrote: > if fh.isZip64? Oops.
http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go File src/pkg/archive/zip/writer.go (right): http://codereview.appspot.com/6463050/diff/2001/src/pkg/archive/zip/writer.go... src/pkg/archive/zip/writer.go:228: b.uint32(0) // since we are writing a data descriptor crc32, On 2012年08月15日 23:56:29, adg wrote: > really? why? I didn't see that in the spec. This is in the spec at least: CRC-32: (4 bytes) ... If bit 3 of the general purpose flag is set, this field is set to zero in the local header and the correct value is put in the data descriptor and in the central directory. ... In go1.0.2 CreateHeader writes the values supplied in FileHeader (sizes, crc) to the local header before they are known (uncompressed size can be of course). Later the correct values are written to the data descriptor and central directory so it is fairly easy to create a broken file (not sure if implementations really reads the local file header since it is hard to get it right).
Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.g... src/pkg/archive/zip/reader.go:291: return ErrChecksum This isn't the right error. This error should be returned when the *computed* checksum doesn't match the data. Not when the two stored checksums do not match. return errors.New("zip: data descriptor and directory checksum mismatch") or even just return ErrFormat http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.g... src/pkg/archive/zip/reader.go:342: p, err := findDirectory64End(r, directoryEndOffset) don't return d if an error occurred: if err == nil && p >= 0 { err = readDirectoryEnd(...) } if err ! = nil { return nil, err } return d, nil http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:58: zipVersion45 = 45 // 4.5 (reads and writes zip64 archives) http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:77: CompressedSize uint32 // deprecated; use CompressedSize64 http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:78: UncompressedSize uint32 // deprecated; use UncompressedSize64 http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:115: fh.SetModTime(fi.ModTime()) if fh.UncompressedSize64 > uint32max { fh.CompressedSize = uint32max } yeah?
Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.g... src/pkg/archive/zip/reader.go:291: return ErrChecksum On 2012年08月20日 02:34:31, adg wrote: > This isn't the right error. This error should be returned when the *computed* > checksum doesn't match the data. Not when the two stored checksums do not match. > > return errors.New("zip: data descriptor and directory checksum mismatch") > > or even just > > return ErrFormat The test "Bad-CRC32-in-data-descriptor" messes with the data descriptor crc32 so if we return anything else here that test has to be modified to expect a different error. Right now it expects ErrChecksum even if the two crc32 are different. While it is a format error it is also a checksum error since one of the checksums will be incorrect. Not sure if it is worth to change behaviour (the test) for this? http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.g... src/pkg/archive/zip/reader.go:342: p, err := findDirectory64End(r, directoryEndOffset) On 2012年08月20日 02:34:31, adg wrote: > don't return d if an error occurred: > > if err == nil && p >= 0 { > err = readDirectoryEnd(...) > } > if err ! = nil { > return nil, err > } > return d, nil ok http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:107: if size > (1<<32 - 1) { Remove the 4GB test. http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:115: fh.SetModTime(fi.ModTime()) On 2012年08月20日 02:34:31, adg wrote: > if fh.UncompressedSize64 > uint32max { > fh.CompressedSize = uint32max > } > > yeah? Do you mean fh.UncompressedSize = uint32max? Otherwise I don't understand. I added TestFileHeaderRoundTrip64 to check for this and realized I need to patch UncompressedSize64 in FileInfo() if it isn't supplied. No very elegant, need some help here!?
http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.go File src/pkg/archive/zip/reader.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/reader.g... src/pkg/archive/zip/reader.go:291: return ErrChecksum On 2012年08月20日 11:48:01, serbaut wrote: > On 2012年08月20日 02:34:31, adg wrote: > > This isn't the right error. This error should be returned when the *computed* > > checksum doesn't match the data. Not when the two stored checksums do not > match. > > > > return errors.New("zip: data descriptor and directory checksum mismatch") > > > > or even just > > > > return ErrFormat > > The test "Bad-CRC32-in-data-descriptor" messes with the data descriptor crc32 so > if we return anything else here that test has to be modified to expect a > different error. Right now it expects ErrChecksum even if the two crc32 are > different. > > While it is a format error it is also a checksum error since one of the > checksums will be incorrect. Not sure if it is worth to change behaviour (the > test) for this? In that case, leave it as is. (return ErrChecksum) http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:97: func (fi headerFileInfo) Size() int64 { return int64(fi.fh.UncompressedSize) } > I added TestFileHeaderRoundTrip64 to check for this and realized I need to patch > UncompressedSize64 in FileInfo() if it isn't supplied. No very elegant, need > some help here!? I think this should work fine: func (fi headerFileInfo) Size() int64 { if s := fi.fh.UncompressedSize64 > 0 { return s } if s := fi.fh.UncompressedSize > 0 { return int64(s) } return 0 } http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:107: if size > (1<<32 - 1) { On 2012年08月20日 11:48:01, serbaut wrote: > Remove the 4GB test. good catch http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:115: fh.SetModTime(fi.ModTime()) On 2012年08月20日 11:48:01, serbaut wrote: > On 2012年08月20日 02:34:31, adg wrote: > > if fh.UncompressedSize64 > uint32max { > > fh.CompressedSize = uint32max > > } > > > > yeah? > > Do you mean fh.UncompressedSize = uint32max? Otherwise I don't understand. Yeah, I did mean that. Typo. I meant you should clamp, instead of wrap, the 32bit value, as you have elsewhere.
Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/6463050/diff/10004/src/pkg/archive/zip/struct.g... src/pkg/archive/zip/struct.go:97: func (fi headerFileInfo) Size() int64 { return int64(fi.fh.UncompressedSize) } On 2012年08月20日 12:32:47, adg wrote: > > I added TestFileHeaderRoundTrip64 to check for this and realized I need to > patch > > UncompressedSize64 in FileInfo() if it isn't supplied. No very elegant, need > > some help here!? > > I think this should work fine: > > func (fi headerFileInfo) Size() int64 { > if s := fi.fh.UncompressedSize64 > 0 { > return s > } > if s := fi.fh.UncompressedSize > 0 { > return int64(s) > } > return 0 > } Yes, that seems less ugly.
http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test.go File src/pkg/archive/zip/zip_test.go (right): http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:89: ModifiedTime: 1234, Let's use realistic values here. 1337 1207 http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:99: // Ignore these fields: don't do this. just compare the four fields you're interested in with simple == statements. you'll give better error messages that way. http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:145: r, err := NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len())) NewReader(buf) will work fine http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:149: rc, err := r.File[0].Open() f := r.File[0] rc, err := f.Open() and so on for all occurrences of r.File[0] http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:159: gotEnd := make([]byte, len(end)) instead, do gotEnd, err := ioutil.ReadAll(rc) so it reads until EOF http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:171: if r.File[0].UncompressedSize != 0xffffffff { if got, want := f.UncompressedSize, uint32max; got != want { http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:172: t.Errorf("UncompressedSize %d, want %d", t.Errorf("UncompressedSize %d, want %d", got, want) http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:176: if r.File[0].UncompressedSize64 != (1<<32)+uint64(len(end)) { same as above
http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test.go File src/pkg/archive/zip/zip_test.go (right): http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:89: ModifiedTime: 1234, On 2012年08月21日 05:42:28, adg wrote: > Let's use realistic values here. > 1337 > 1207 This was just copy/paste from the test above. Do you want me to change that as well? http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:99: // Ignore these fields: On 2012年08月21日 05:42:28, adg wrote: > don't do this. just compare the four fields you're interested in with simple == > statements. you'll give better error messages that way. Again, same as test above. Change both? http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:145: r, err := NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len())) On 2012年08月21日 05:42:28, adg wrote: > NewReader(buf) will work fine Perhaps it can be simpler but NewReader(buf) wont work: func NewReader(r io.ReaderAt, size int64) and bytes.Buffer does not implement io.ReaderAt http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:149: rc, err := r.File[0].Open() On 2012年08月21日 05:42:28, adg wrote: > f := r.File[0] > rc, err := f.Open() > > and so on for all occurrences of r.File[0] Done. http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:159: gotEnd := make([]byte, len(end)) On 2012年08月21日 05:42:28, adg wrote: > instead, do > > gotEnd, err := ioutil.ReadAll(rc) > > so it reads until EOF Done. http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:171: if r.File[0].UncompressedSize != 0xffffffff { On 2012年08月21日 05:42:28, adg wrote: > if got, want := f.UncompressedSize, uint32max; got != want { Done. http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:172: t.Errorf("UncompressedSize %d, want %d", On 2012年08月21日 05:42:28, adg wrote: > t.Errorf("UncompressedSize %d, want %d", got, want) Done. http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:176: if r.File[0].UncompressedSize64 != (1<<32)+uint64(len(end)) { On 2012年08月21日 05:42:28, adg wrote: > same as above Done.
http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test.go File src/pkg/archive/zip/zip_test.go (right): http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:89: ModifiedTime: 1234, On 2012年08月21日 07:34:41, serbaut wrote: > On 2012年08月21日 05:42:28, adg wrote: > > Let's use realistic values here. > > 1337 > > 1207 > > This was just copy/paste from the test above. Do you want me to change that as > well? eh, leave it then. :-/ http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:99: // Ignore these fields: On 2012年08月21日 07:34:41, serbaut wrote: > On 2012年08月21日 05:42:28, adg wrote: > > don't do this. just compare the four fields you're interested in with simple > == > > statements. you'll give better error messages that way. > > Again, same as test above. Change both? Actually, no. I think you should also test that UncompressedSize is set to 0xffffffff, and in the test above check that UncompressedSize64 is set appropriately, too. http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:145: r, err := NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len())) On 2012年08月21日 07:34:41, serbaut wrote: > On 2012年08月21日 05:42:28, adg wrote: > > NewReader(buf) will work fine > > Perhaps it can be simpler but NewReader(buf) wont work: > > func NewReader(r io.ReaderAt, size int64) > and > bytes.Buffer does not implement io.ReaderAt Ah, missed that. Carry on.
http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test.go File src/pkg/archive/zip/zip_test.go (right): http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:99: // Ignore these fields: On 2012年08月21日 07:46:50, adg wrote: > On 2012年08月21日 07:34:41, serbaut wrote: > > On 2012年08月21日 05:42:28, adg wrote: > > > don't do this. just compare the four fields you're interested in with simple > > == > > > statements. you'll give better error messages that way. > > > > Again, same as test above. Change both? > > Actually, no. I think you should also test that UncompressedSize is set to > 0xffffffff, and in the test above check that UncompressedSize64 is set > appropriately, too. do you mean if got, want := fh2.Name if got, want := UncompressedSize64 if ... instead of if !reflect.DeepEqual(fh, fh2) {
On 21 August 2012 18:12, <serbaut@gmail.com> wrote: > > http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test.go > File src/pkg/archive/zip/zip_test.go (right): > > http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... > src/pkg/archive/zip/zip_test.go:99: // Ignore these fields: > On 2012年08月21日 07:46:50, adg wrote: >> >> On 2012年08月21日 07:34:41, serbaut wrote: >> > On 2012年08月21日 05:42:28, adg wrote: >> > > don't do this. just compare the four fields you're interested in > > with simple >> >> > == >> > > statements. you'll give better error messages that way. >> > >> > Again, same as test above. Change both? > > >> Actually, no. I think you should also test that UncompressedSize is > > set to >> >> 0xffffffff, and in the test above check that UncompressedSize64 is set >> appropriately, too. > > > do you mean > > if got, want := fh2.Name > if got, want := UncompressedSize64 > if ... > > instead of > > if !reflect.DeepEqual(fh, fh2) { Yep.
Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test.go File src/pkg/archive/zip/zip_test.go (right): http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:89: ModifiedTime: 1234, On 2012年08月21日 07:46:50, adg wrote: > On 2012年08月21日 07:34:41, serbaut wrote: > > On 2012年08月21日 05:42:28, adg wrote: > > > Let's use realistic values here. > > > 1337 > > > 1207 > > > > This was just copy/paste from the test above. Do you want me to change that as > > well? > > eh, leave it then. :-/ I think they actually are realistic values, see msDosTimeToTime() in struct.go http://codereview.appspot.com/6463050/diff/14007/src/pkg/archive/zip/zip_test... src/pkg/archive/zip/zip_test.go:99: // Ignore these fields: On 2012年08月21日 05:42:28, adg wrote: > don't do this. just compare the four fields you're interested in with simple == > statements. you'll give better error messages that way. Made a function that checks the header, hope that is ok.
LGTM
*** Submitted as http://code.google.com/p/go/source/detail?r=9ebf39aaedaa *** archive/zip: zip64 support R=golang-dev, r, adg CC=golang-dev http://codereview.appspot.com/6463050 Committer: Andrew Gerrand <adg@golang.org>