code review 12784045: archive/zip: add File.DataOffset

archive/zip: add File.DataOffset

Accessor to find where the bytes of a file start.

[bradfitz, 2013-08-13 14:55:41]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-08-13 18:29:53]

i can't imagine why you'd need this. context?

[bradfitz, 2013-08-13 18:35:28]

I have a server which keeps multiple versions of mostly-identical zip files
in memory.

This lets the server de-dup identical parts of the zip files by knowing the
cut points in the file.



On Tue, Aug 13, 2013 at 11:29 AM, Russ Cox <rsc@golang.org> wrote:

> i can't imagine why you'd need this. context?
>
>

[dsymonds, 2013-08-13 21:51:35]

LGTM

but it seems like it should have a test at least.

[bradfitz, 2013-08-13 21:57:49]

Russ prefers a CompressedOffset uint64 field be added to struct File.

But that means reading from all over the file in the constructor, which
doesn't seem worth it for the majority of users.




On Tue, Aug 13, 2013 at 2:51 PM, <dsymonds@golang.org> wrote:

> LGTM
>
> but it seems like it should have a test at least.
>
>
https://codereview.appspot.**com/12784045/<https://codereview.appspot.com/127...
>

[adg1, 2013-08-13 22:04:45]

I prefer the method approach. I don't want to slow down typical users who
might open a zip and read each file in order.
On 14 Aug 2013 07:57, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:

> Russ prefers a CompressedOffset uint64 field be added to struct File.
>
> But that means reading from all over the file in the constructor, which
> doesn't seem worth it for the majority of users.
>
>
>
>
> On Tue, Aug 13, 2013 at 2:51 PM, <dsymonds@golang.org> wrote:
>
>> LGTM
>>
>> but it seems like it should have a test at least.
>>
>>
https://codereview.appspot.**com/12784045/<https://codereview.appspot.com/127...
>>
>
>  --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[bradfitz, 2013-08-13 22:18:49]

Or we could make it more general, having a method to return a struct of the
local file header instead of just the body offset.

Alternatively, if the file header in the central directory (the one at the
end of the file) is guaranteed to have its 2-byte "file name length" and
"extra field length" match the same fields of the local file header, we
could trust it.  But since we aren't currently using it and are instead
re-reading it, it makes me think you've already considered that.  (That is:
why does readDirectoryHeader throw away filenameLen and extraLen?)




On Tue, Aug 13, 2013 at 3:04 PM, Andrew Gerrand <adg@google.com> wrote:

> I prefer the method approach. I don't want to slow down typical users who
> might open a zip and read each file in order.
> On 14 Aug 2013 07:57, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
>
>> Russ prefers a CompressedOffset uint64 field be added to struct File.
>>
>> But that means reading from all over the file in the constructor, which
>> doesn't seem worth it for the majority of users.
>>
>>
>>
>>
>> On Tue, Aug 13, 2013 at 2:51 PM, <dsymonds@golang.org> wrote:
>>
>>> LGTM
>>>
>>> but it seems like it should have a test at least.
>>>
>>>
https://codereview.appspot.**com/12784045/<https://codereview.appspot.com/127...
>>>
>>
>>  --
>>
>> ---
>> You received this message because you are subscribed to the Google Groups
>> "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>>
>

[bradfitz, 2013-08-13 22:38:28]

Yeah, looks like the extraLen at least can vary between the central
directory and the local file headers.

So we could do a method on *File returning a LocalFileHeader instead?  It
could be:

type LocalFileHeader struct {
     FileHeader         // embedded
     CompressedOffset int64   // computed
}

?



On Tue, Aug 13, 2013 at 3:18 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Or we could make it more general, having a method to return a struct of
> the local file header instead of just the body offset.
>
> Alternatively, if the file header in the central directory (the one at the
> end of the file) is guaranteed to have its 2-byte "file name length" and
> "extra field length" match the same fields of the local file header, we
> could trust it.  But since we aren't currently using it and are instead
> re-reading it, it makes me think you've already considered that.  (That is:
> why does readDirectoryHeader throw away filenameLen and extraLen?)
>
>
>
>
> On Tue, Aug 13, 2013 at 3:04 PM, Andrew Gerrand <adg@google.com> wrote:
>
>> I prefer the method approach. I don't want to slow down typical users who
>> might open a zip and read each file in order.
>> On 14 Aug 2013 07:57, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
>>
>>> Russ prefers a CompressedOffset uint64 field be added to struct File.
>>>
>>> But that means reading from all over the file in the constructor, which
>>> doesn't seem worth it for the majority of users.
>>>
>>>
>>>
>>>
>>> On Tue, Aug 13, 2013 at 2:51 PM, <dsymonds@golang.org> wrote:
>>>
>>>> LGTM
>>>>
>>>> but it seems like it should have a test at least.
>>>>
>>>>
https://codereview.appspot.**com/12784045/<https://codereview.appspot.com/127...
>>>>
>>>
>>>  --
>>>
>>> ---
>>> You received this message because you are subscribed to the Google
>>> Groups "golang-dev" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to golang-dev+unsubscribe@googlegroups.com.
>>> For more options, visit https://groups.google.com/groups/opt_out.
>>>
>>>
>>>
>>
>

[rsc, 2013-08-13 23:16:50]

LGTM as is

I guess it has to be a method.
Let's not invent even more API.

[bradfitz, 2013-08-13 23:30:04]

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

archive/zip: add File.DataOffset

Accessor to find where the bytes of a file start.

R=golang-dev, rsc, dsymonds, adg
CC=golang-dev
https://codereview.appspot.com/12784045