[golang-dev] Re: code review 6700047: archive/tar: read/write extended pax/gnu tar archives (issue 6700047)

https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode192
src/pkg/archive/tar/writer.go:192: dir, file := filepath.Split(hdr.Name)
According to my reading of the python tarfile implementation, they use
/.
https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode193
src/pkg/archive/tar/writer.go:193: ext.Name = filepath.Join(dir,
Sorry, I'm not sure what you mean by cleaning the path.
https://codereview.appspot.com/6700047/

https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go#newcode198
src/pkg/archive/tar/writer.go:198: fmt.Fprint(&buf,
paxHeader(fmt.Sprintf("path=%s", hdr.Name)))
no need for fmt.Sprintf just to concatenate two strings.
"path=" + hdr.Name

same for hdr.Linkname

https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go#newcode219
src/pkg/archive/tar/writer.go:219: padding := 2 // Extra padding for
space and newline
const padding = 2

https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer.go#newcode221
src/pkg/archive/tar/writer.go:221: size += len(strconv.Itoa(size))
This still can't be right. What if this += involves a decimal carry?
Then the output will be one longer than what is computed here.

please add a test case that covers this, and fix this code.

https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer_test.go
File src/pkg/archive/tar/writer_test.go (right):

https://codereview.appspot.com/6700047/diff/30004/src/pkg/archive/tar/writer_test.go#newcode228
src/pkg/archive/tar/writer_test.go:228: paxTests :=
map[string]string{"name=/etc/hosts": "19 name=/etc/hosts\n",
one test case per line so we can read it please.

also add a case that yields a single digit length.

https://codereview.appspot.com/6700047/

https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode192
src/pkg/archive/tar/writer.go:192: dir, file := filepath.Split(hdr.Name)
/.

exactly. and therefore you should use the path package (which handles /
separated paths) and not path/filepath (which uses os.PathSeparator
separated paths)

https://codereview.appspot.com/6700047/diff/34007/src/pkg/archive/tar/writer.go#newcode193
src/pkg/archive/tar/writer.go:193: ext.Name = filepath.Join(dir,
From [0]: "Clean returns the shortest path name equivalent to path by
purely lexical processing."

that means if someone wants to have a path like "a/b/../../c///" in his
tarball, he'll get instead the cleaned path equivalent "c".

that is bad because of 2 things: (1) it's not what the tarball creator
expects, because its not documented; (2) the trailing slash is gone.

i think cleaning the path would make sense, but then it needs to be done
(1) for all hdr.Name (not only the ones which are >100 chars) and (2)
the trailing slash needs to be appended (that's what gnu tar 1.26
does).. but this might break go1 compatibility.

[0]: http://golang.org/pkg/path/#Clean

https://codereview.appspot.com/6700047/