FAQ
Sorry again for the delay. I think I've got a handle on this; the rest
of the review should proceed quicker.


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

https://codereview.appspot.com/6700047/diff/18001/src/pkg/archive/tar/reader.go#newcode201
src/pkg/archive/tar/reader.go:201: func parsePAX(tr io.Reader)
(map[string]string, error) {
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.

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

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader.go#newcode208
src/pkg/archive/tar/reader.go:208: for len(buf) != 0 {
for len(buf) > 0 {

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

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader_test.go#newcode343
src/pkg/archive/tar/reader_test.go:343: "mtime": "1350244992.023960108"}
put } on its own line

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/reader_test.go#newcode348
src/pkg/archive/tar/reader_test.go:348: // Assert the values were
written correctly.
want := &Header{
...
}
if !reflect.DeepEqual(hdr, want) {
t.Errorf("incorrect merge: got %+v, want %+v", hdr, want)
}

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

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer.go#newcode26
src/pkg/archive/tar/writer.go:26: // The pid of the current process.
Assumes go doesn't fork.
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#newcode143
src/pkg/archive/tar/writer.go:143: use_pax_header := len(hdr.Name) > 100
len(hdr.Linkname) > 100
these are not Go names. Drop your underscores and use camelCase. See if
you can shorten the names too.

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer.go#newcode144
src/pkg/archive/tar/writer.go:144: if use_pax_header {
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
}
}

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

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode186
src/pkg/archive/tar/writer_test.go:186: // Create a pretend large
archive.
s/pretend //

It looks real enough to me.

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode189
src/pkg/archive/tar/writer_test.go:189: t.Fatal(err)
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".

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode196
src/pkg/archive/tar/writer_test.go:196: long_name := strings.Repeat("0",
200)
longName

something more interesting would be good. strings.Repeat("ab", 51)?

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode214
src/pkg/archive/tar/writer_test.go:214: // Test that we can get a nice
long name back out of the archive.
s/nice //

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode215
src/pkg/archive/tar/writer_test.go:215: reader :=
NewReader(bytes.NewBuffer(buf.Bytes()))
you shouldn't have to wrap it in a new buffer. NewReader(&buf) should
work.

https://codereview.appspot.com/6700047/diff/27001/src/pkg/archive/tar/writer_test.go#newcode220
src/pkg/archive/tar/writer_test.go:220: if !strings.EqualFold(hdr.Name,
long_name) {
why fold?

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

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 31, '12 at 2:44a
activeOct 31, '12 at 12:21p
posts4
users2
websitegolang.org

2 users in discussion

Dsymonds: 2 posts Shanemhansen: 2 posts

People

Translate

site design / logo © 2022 Grokbase