https://codereview.appspot.com/6700047/diff/18001/src/pkg/archive/tar/reader.goFile src/pkg/archive/tar/reader.go (right):
https://codereview.appspot.com/6700047/diff/18001/src/pkg/archive/tar/reader.go#newcode201src/pkg/archive/tar/reader.go:201: func parsePAX(tr io.Reader)
(map[string]string, error) {
Ok, that makes sense. Change made.
On 2012/10/31 02:44:37, dsymonds wrote:On 2012/10/23 02:54:14, shanemhansen wrote:On 2012/10/22 06:29:14, dsymonds wrote:
tr -> r
("tr" here returns to the local Reader type, not io.Reader)
I actually think this is what I want. ReadAll(tr) gives me the
contents of the
extended header, which is put in the body of the pax record.
wouldn't
ReadlAll(r) read the underlying reader until EOF?
I mean here, rather than the call site. This function only uses the
functionality of io.Reader (the type it takes), so it should name its parameter
accordingly: "r". The call site correctly invokes this with tr.
File src/pkg/archive/tar/reader.go (right):
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader.go#newcode208src/pkg/archive/tar/reader.go:208: for len(buf) != 0 {
On 2012/10/31 02:44:37, dsymonds wrote:
for len(buf) > 0 {
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader_test.goFile src/pkg/archive/tar/reader_test.go (right):
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader_test.go#newcode343src/pkg/archive/tar/reader_test.go:343: "mtime": "1350244992.023960108"}
On 2012/10/31 02:44:37, dsymonds wrote:
put } on its own line
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader_test.go#newcode348src/pkg/archive/tar/reader_test.go:348: // Assert the values were
written correctly.
On 2012/10/31 02:44:37, dsymonds wrote:
want := &Header{
...
}
if !reflect.DeepEqual(hdr, want) {
t.Errorf("incorrect merge: got %+v, want %+v", hdr, want)
}
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer.goFile src/pkg/archive/tar/writer.go (right):
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer.go#newcode26src/pkg/archive/tar/writer.go:26: // The pid of the current process.
Assumes go doesn't fork.
No problem. Someone asked me to cache the value, I'll change it back.
On 2012/10/31 02:44:37, dsymonds wrote:
Don't make this assumption. Just call os.Getpid() as needed; it's not that
expensive compared to all the other things going on.
If this was a big concern, capturing it in NewWriter would be the better
approach, but I don't think it's warranted.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer.go#newcode143src/pkg/archive/tar/writer.go:143: use_pax_header := len(hdr.Name) > 100
len(hdr.Linkname) > 100
On 2012/10/31 02:44:37, dsymonds wrote:these are not Go names. Drop your underscores and use camelCase. See
if you can
shorten the names too.
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer.go#newcode144src/pkg/archive/tar/writer.go:144: if use_pax_header {
On 2012/10/31 02:44:37, dsymonds wrote:
I think the contents of this if could be its own method, and then you can just
inline the condition.
if len(hdr.Name) > 100 || len(hdr.Linkname) > 100 {
if err := tw.writePAXHeader(hdr); err != nil {
return err
}
}
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.goFile src/pkg/archive/tar/writer_test.go (right):
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode186src/pkg/archive/tar/writer_test.go:186: // Create a pretend large
archive.
On 2012/10/31 02:44:37, dsymonds wrote:
s/pretend //
It looks real enough to me.
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode189src/pkg/archive/tar/writer_test.go:189: t.Fatal(err)
On 2012/10/31 02:44:37, dsymonds wrote:
Please use a better error message throughout here. If we get a test
failure that
just says "permission denied" then it's not going to be as easy to
track down as
"os.Stat: permission denied".
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode196src/pkg/archive/tar/writer_test.go:196: long_name := strings.Repeat("0",
200)
On 2012/10/31 02:44:37, dsymonds wrote:
longName
something more interesting would be good. strings.Repeat("ab", 51)?
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode214src/pkg/archive/tar/writer_test.go:214: // Test that we can get a nice
long name back out of the archive.
On 2012/10/31 02:44:37, dsymonds wrote:
s/nice //
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode215src/pkg/archive/tar/writer_test.go:215: reader :=
NewReader(bytes.NewBuffer(buf.Bytes()))
On 2012/10/31 02:44:37, dsymonds wrote:
you shouldn't have to wrap it in a new buffer. NewReader(&buf) should
work.
Done.
https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode220src/pkg/archive/tar/writer_test.go:220: if !strings.EqualFold(hdr.Name,
long_name) {
Ignorance. I was looking for a strings.Equal function.
A closer reading of the spec reveals that == is defined for the string
type. Thanks for pointing that out.
On 2012/10/31 02:44:37, dsymonds wrote:
why fold?
https://codereview.appspot.com/6700047/
