code review 6463050: archive/zip: zip64 support

archive/zip: zip64 support

[serbaut, 2012-08-15 14:20:57]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[r, 2012-08-15 16:26:16]

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

[r, 2012-08-15 16:31:38]

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.

[serbaut, 2012-08-15 21:23:22]

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 
>

[serbaut, 2012-08-15 21:37:39]

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?

[serbaut, 2012-08-15 22:22:18]

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.

[serbaut, 2012-08-15 22:34:24]

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.

[adg, 2012-08-15 23:56:29]

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" ?

[serbaut, 2012-08-16 13:46:57]

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 :/

[serbaut, 2012-08-16 13:49:25]

Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-08-17 01:39:25]

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?

[serbaut, 2012-08-17 07:38:12]

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.

[serbaut, 2012-08-17 08:20:33]

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

[serbaut, 2012-08-17 09:43:26]

Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[serbaut, 2012-08-17 22:06:47]

Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-08-20 02:34:31]

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?

[serbaut, 2012-08-20 11:47:44]

Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[serbaut, 2012-08-20 11:48:01]

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!?

[adg, 2012-08-20 12:32:47]

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.

[serbaut, 2012-08-20 12:51:50]

Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[serbaut, 2012-08-20 12:52:12]

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.

[adg, 2012-08-21 05:42:27]

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

[serbaut, 2012-08-21 07:34:41]

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.

[adg, 2012-08-21 07:46:49]

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.

[serbaut, 2012-08-21 08:12:40]

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

[adg, 2012-08-21 09:17:44]

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.

[serbaut, 2012-08-21 21:02:46]

Hello golang-dev@googlegroups.com, r@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[serbaut, 2012-08-21 21:09:51]

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.

[adg, 2012-08-22 00:53:42]

LGTM

[adg, 2012-08-22 01:05:31]

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

[dvyukov, 2012-10-10 10:00:20]

Fixes issue 3968 as well.

[remyoudompheng, 2013-07-20 21:23:05]

R=close