code review 5796073: archive/tar: add FileInfoHeader function

archive/tar: add FileInfoHeader function

Fixes issue 3295

[bradfitz, 2012-03-12 21:44:39]

Hello golang-dev@googlegroups.com,

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

[adg, 2012-03-12 22:01:29]

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:87: case fi.Mode()&os.ModeSymlink != 0:
h.Typeflag = TypeSymlink ?

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:99: h.Mode |= c_ISCHR
h.Typeflag = TypeChar ?

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:101: h.Mode |= c_ISBLK
h.Typeflag = TypeBlock ?

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/stat_unix.go
File src/pkg/archive/tar/stat_unix.go (right):

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/stat_unix...
src/pkg/archive/tar/stat_unix.go:28: // TODO(bradfitz): major/minor device
numbers?
why not?

[bradfitz, 2012-03-12 22:03:53]

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:87: case fi.Mode()&os.ModeSymlink != 0:
On 2012/03/12 22:01:30, adg wrote:
> h.Typeflag = TypeSymlink ?

Done.

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:99: h.Mode |= c_ISCHR
On 2012/03/12 22:01:30, adg wrote:
> h.Typeflag = TypeChar ?

Done.

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:101: h.Mode |= c_ISBLK
On 2012/03/12 22:01:30, adg wrote:
> h.Typeflag = TypeBlock ?

Done.

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/stat_unix.go
File src/pkg/archive/tar/stat_unix.go (right):

http://codereview.appspot.com/5796073/diff/6001/src/pkg/archive/tar/stat_unix...
src/pkg/archive/tar/stat_unix.go:28: // TODO(bradfitz): major/minor device
numbers?
On 2012/03/12 22:01:30, adg wrote:
> why not?

portably unpacking them

[bradfitz, 2012-03-12 22:04:18]

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

Please take another look.

[adg, 2012-03-12 22:15:02]

LGTM

[bradfitz, 2012-03-12 22:18:52]

Can you wait a couple hours on this?  (or just patch it in locally if you
need it for Windows?)

Russ said I was doing this wrong before, so I'm curious if I'm still doing
it wrong.

Or if the API sucks (the optional filename parameter, which doesn't seem
avoidable, but I could also see being a readlink function instead of a
string...)


On Mon, Mar 12, 2012 at 3:15 PM, <adg@golang.org> wrote:

> LGTM
>
>
http://codereview.appspot.com/**5796073/<http://codereview.appspot.com/5796073/>
>

[adg, 2012-03-12 22:34:37]

On 13 March 2012 09:18, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Can you wait a couple hours on this?  (or just patch it in locally if you
> need it for Windows?)


Sounds fine to me.

[rsc, 2012-03-15 17:51:48]

For the record, this CL is postponed until after Go 1.

[Mike.Rosset, 2012-03-15 23:18:04]

On 2012/03/12 21:44:39, bradfitz wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg

+1 one for this addition thanks Brad, Even if its not included in Go1 its a nice
change. I currently have some code that does this but I'll port it to your new
function and provide any feed back where I can.

[adg, 2012-05-01 03:35:19]

Ping.

[rsc, 2012-05-03 21:41:47]

http://codereview.appspot.com/5796073/diff/2004/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

http://codereview.appspot.com/5796073/diff/2004/src/pkg/archive/tar/common.go...
src/pkg/archive/tar/common.go:71: // The filename parameter is used only in the
case of symlinks, to call os.Readlink.
This seems wrong.  This is a data constructor.
It has no business making system calls.
If an argument is necessary, it should be the value to use as the symlink.

http://codereview.appspot.com/5796073/diff/2004/src/pkg/archive/tar/stat_unix.go
File src/pkg/archive/tar/stat_unix.go (right):

http://codereview.appspot.com/5796073/diff/2004/src/pkg/archive/tar/stat_unix...
src/pkg/archive/tar/stat_unix.go:16: sysStat = func(fi os.FileInfo, h *Header)
error {
Please declare a real function and use

func init() { 
    sysStat = sysStatUnix
}

or something like that.

[bradfitz, 2012-05-24 17:51:08]

Hello adg@golang.org, rsc@golang.org, mike.rosset@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-05-24 17:55:38]

Russ didn't like the old mostly-data-constructor-but-also-does-Readlink.
 Maybe this way is less offensive.  Or at least differently offensive?

2012/5/24 <bradfitz@golang.org>

> Hello adg@golang.org, rsc@golang.org, mike.rosset@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5796073/<http://codereview.appspot.com/5796073/>
>

[rsc, 2012-05-24 17:58:37]

No.  The argument needs to be an os.FileInfo.

[bradfitz, 2012-05-24 18:06:44]

On Thu, May 24, 2012 at 10:58 AM, Russ Cox <rsc@golang.org> wrote:

> No.  The argument needs to be an os.FileInfo.
>

It does? Okay. Signatures welcome.

I couldn't come up with anything I liked that dealt with symlinks.

[rsc, 2012-05-24 18:16:12]

Symlinks are a special case.  What's wrong with

FileInfoHeader(fi os.FileInfo, symlink string)

?

[bradfitz, 2012-05-24 18:25:14]

On Thu, May 24, 2012 at 11:15 AM, Russ Cox <rsc@golang.org> wrote:

> Symlinks are a special case.  What's wrong with
>
> FileInfoHeader(fi os.FileInfo, symlink string)
>
> ?
>

So all callers look like:

fi, err := os.Lstat(filename)
...
target, _ := os.Readlink(filename)
h, err := tar.FileInfoHeader(fi, target)

?

That Readlink line before looks a little weird.  But sure, if you're happy
with that.

[rsc, 2012-05-24 18:34:19]

> So all callers look like:

No, just the callers that think symlinks are worth worrying about.  :-)

Russ

[bradfitz, 2012-05-24 18:35:49]

Hello adg@golang.org, rsc@golang.org, mike.rosset@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-05-24 20:48:53]

Can we not check a symlink into the repository?
Surely it could be created during the test if necessary.

http://codereview.appspot.com/5796073/diff/22003/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

http://codereview.appspot.com/5796073/diff/22003/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:72: // If fi represents a symlink, the provided
symlink target is used.
// If fi describes a symlink, FileInfoHeader records link as the link target.

http://codereview.appspot.com/5796073/diff/22003/src/pkg/archive/tar/common.g...
src/pkg/archive/tar/common.go:73: func FileInfoHeader(fi os.FileInfo,
symlinkTarget string) (*Header, error) {
s/symlinkTarget/link/

[bradfitz, 2012-05-24 21:00:03]

Hello adg@golang.org, rsc@golang.org, mike.rosset@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-05-24 21:03:12]

LGTM

[bradfitz, 2012-05-24 21:11:04]

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

archive/tar: add FileInfoHeader function

Fixes issue 3295

R=adg, rsc, mike.rosset
CC=golang-dev
http://codereview.appspot.com/5796073

[bradfitz, 2012-05-24 21:13:21]

Note that I also updated deps_test.go in this CL (after talking to Russ),
as well as api/next.txt.

We probably don't need to update api/next.txt aggressively (it's not an
error if we forget it), but might as well if we remember.  I always do an
all.bash run before submitting anyway, so I just pasted it in once I saw
the all.bash noise.

On Thu, May 24, 2012 at 2:11 PM, <bradfitz@golang.org> wrote:

> *** Submitted as
>
http://code.google.com/p/go/**source/detail?r=608f3c08cb96<http://code.google...
>
> archive/tar: add FileInfoHeader function
>
> Fixes issue 3295
>
> R=adg, rsc, mike.rosset
> CC=golang-dev
>
http://codereview.appspot.com/**5796073<http://codereview.appspot.com/5796073>
>
>
>
http://codereview.appspot.com/**5796073/<http://codereview.appspot.com/5796073/>
>