code review 5969071: misc/dist: include directories in distribution tar and ...

misc/dist: include directories in distribution tar and zip files.

Fixes issue 3458.

[dave_cheney.net, 2012-04-03 10:33:25]

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

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

[dsymonds, 2012-04-03 12:11:52]

http://codereview.appspot.com/5969071/diff/5001/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5969071/diff/5001/misc/dist/bindist.go#newcode653
misc/dist/bindist.go:653: w, err := zw.CreateHeader(fh)
above here I think you want

if fi.IsDir() {
  fh.Name += "/"
  fh.Method = zip.Store
}

There's no point deflating zero bytes, and some unzip programs require directory
names to end in a slash.

[dave_cheney.net, 2012-04-03 20:33:50]

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

Please take another look.

[dave_cheney.net, 2012-04-03 20:34:14]

Hi David,

Thank you very much for your review. Please see my comment about
zip.FileInfoHeader().

Cheers

Dave

http://codereview.appspot.com/5969071/diff/5001/misc/dist/bindist.go
File misc/dist/bindist.go (right):

http://codereview.appspot.com/5969071/diff/5001/misc/dist/bindist.go#newcode653
misc/dist/bindist.go:653: w, err := zw.CreateHeader(fh)
Done.

Shouldn't zip.FileInfoHeader add the trailing / for us ?


On 2012/04/03 12:11:52, dsymonds wrote:
> above here I think you want
> 
> if fi.IsDir() {
>   fh.Name += "/"
>   fh.Method = zip.Store
> }
> 
> There's no point deflating zero bytes, and some unzip programs require
directory
> names to end in a slash.

[dsymonds, 2012-04-04 00:39:14]

On Wed, Apr 4, 2012 at 6:34 AM,  <dave@cheney.net> wrote:

> Shouldn't zip.FileInfoHeader add the trailing / for us ?

Arguably yes, but it's too late to make such changes.

[dave_cheney.net, 2012-04-04 00:44:59]

C'est la vie

On 04/04/2012, at 10:39, David Symonds <dsymonds@golang.org> wrote:

> On Wed, Apr 4, 2012 at 6:34 AM,  <dave@cheney.net> wrote:
> 
>> Shouldn't zip.FileInfoHeader add the trailing / for us ?
> 
> Arguably yes, but it's too late to make such changes.

[adg, 2012-04-04 02:41:25]

Looks okay, but have you tested this?

[dave_cheney.net, 2012-04-04 02:51:43]

I have tested the tar version and it's matches rsc's example, but I'm unable to
test the windows version and it need to be built on a windows machine (needs
cmd.exe)

On 04/04/2012, at 12:41, adg@golang.org wrote:

> Looks okay, but have you tested this?
> 
> http://codereview.appspot.com/5969071/

[adg, 2012-04-05 01:11:13]

LGTM

[adg, 2012-04-05 01:13:42]

You'll need to sync and upload before I can submit this.

[dave_cheney.net, 2012-04-05 01:28:50]

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

Please take another look.

[adg, 2012-04-05 01:39:20]

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

misc/dist: include directories in distribution tar and zip files.

Fixes issue 3458.

R=adg, dsymonds
CC=golang-dev
http://codereview.appspot.com/5969071

Committer: Andrew Gerrand <adg@golang.org>

[brainman, 2012-04-05 01:58:10]

On 2012/04/05 01:28:50, dfc wrote:
> Hello mailto:adg@golang.org, mailto:dsymonds@golang.org (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

It works on windows. Thank you.

Alex

[adg, 2012-04-05 02:05:06]

Thanks for testing it, Alex.

On 5 April 2012 11:58,  <alex.brainman@gmail.com> wrote:
> On 2012/04/05 01:28:50, dfc wrote:
>>
>> Hello mailto:adg@golang.org, mailto:dsymonds@golang.org (cc:
>
> mailto:golang-dev@googlegroups.com),
>
>> Please take another look.
>
>
> It works on windows. Thank you.
>
> Alex
>
> http://codereview.appspot.com/5969071/

[dave_cheney.net, 2012-04-05 02:27:34]

Thanks Alex and Andrew. 

On 05/04/2012, at 12:04, Andrew Gerrand <adg@golang.org> wrote:

> Thanks for testing it, Alex.
> 
> On 5 April 2012 11:58,  <alex.brainman@gmail.com> wrote:
>> On 2012/04/05 01:28:50, dfc wrote:
>>> 
>>> Hello mailto:adg@golang.org, mailto:dsymonds@golang.org (cc:
>> 
>> mailto:golang-dev@googlegroups.com),
>> 
>>> Please take another look.
>> 
>> 
>> It works on windows. Thank you.
>> 
>> Alex
>> 
>> http://codereview.appspot.com/5969071/