FAQ
Hello golang-dev@googlegroups.com, dsymonds@golang.org, dave@cheney.net,
rogpeppe@gmail.com, remyoudompheng@gmail.com, chressie@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.


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

Search Discussions

  • Shanemhansen at Jan 9, 2013 at 1:52 am
    Hello golang-dev@googlegroups.com, dsymonds@golang.org, dave@cheney.net,
    rogpeppe@gmail.com, remyoudompheng@gmail.com, chressie@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6700047/
  • Dsymonds at Jan 9, 2013 at 2:13 am
    This looks pretty close to me.


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

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/reader.go#newcode248
    src/pkg/archive/tar/reader.go:248: // Check for binary format first.
    This is already in the tree; I think you need to sync your client and do
    an 'hg upload'.

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

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/writer.go#newcode205
    src/pkg/archive/tar/writer.go:205: dir, file := filepath.Split(hdr.Name)
    yeah, as mentioned by chressie1 these should use path rather than
    filepath. tar names are always / separated.

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/writer.go#newcode220
    src/pkg/archive/tar/writer.go:220: _, err := tw.Write(buf.Bytes())
    please snuggle this.

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

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/writer_test.go#newcode232
    src/pkg/archive/tar/writer_test.go:232: {"a=name", "10 a=name\n"},
    //Test case involving multiple acceptable lengths
    a single space after //

    https://codereview.appspot.com/6700047/
  • Shanemhansen at Jan 9, 2013 at 2:40 am
    Hello golang-dev@googlegroups.com, dsymonds@golang.org, dave@cheney.net,
    rogpeppe@gmail.com, remyoudompheng@gmail.com, chressie@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6700047/
  • Shanemhansen at Jan 9, 2013 at 2:40 am
    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/reader.go
    File src/pkg/archive/tar/reader.go (right):

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/reader.go#newcode248
    src/pkg/archive/tar/reader.go:248: // Check for binary format first.
    On 2013/01/09 02:13:17, dsymonds wrote:
    This is already in the tree; I think you need to sync your client and do an 'hg
    upload'.
    Done. I think. I certainly did a hg sync and hg upload.

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

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/writer.go#newcode205
    src/pkg/archive/tar/writer.go:205: dir, file := filepath.Split(hdr.Name)
    On 2013/01/09 02:13:17, dsymonds wrote:
    yeah, as mentioned by chressie1 these should use path rather than
    filepath. tar
    names are always / separated.
    Done.

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/writer.go#newcode220
    src/pkg/archive/tar/writer.go:220: _, err := tw.Write(buf.Bytes())
    On 2013/01/09 02:13:17, dsymonds wrote:
    please snuggle this.
    Done.

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

    https://codereview.appspot.com/6700047/diff/51001/src/pkg/archive/tar/writer_test.go#newcode232
    src/pkg/archive/tar/writer_test.go:232: {"a=name", "10 a=name\n"},
    //Test case involving multiple acceptable lengths
    On 2013/01/09 02:13:17, dsymonds wrote:
    a single space after //
    Done.

    https://codereview.appspot.com/6700047/
  • Dsymonds at Jan 9, 2013 at 2:42 am
    LGTM

    This looks good to me.

    Given the subtlety and complexity of this CL I'll let it sit for a
    couple of days to allow others to chime in (even with just a "LGTM");
    I'll submit it after that if there's no problems turned up.

    Thanks for working through this!

    https://codereview.appspot.com/6700047/
  • Shanemhansen at Jan 9, 2013 at 2:45 am

    On 2013/01/09 02:42:34, dsymonds wrote:
    LGTM
    This looks good to me.
    Given the subtlety and complexity of this CL I'll let it sit for a couple of
    days to allow others to chime in (even with just a "LGTM"); I'll
    submit it after
    that if there's no problems turned up.
    Thanks for working through this!
    Thanks for everyone's patience and feedback, it's been a great
    introduction to an unfamiliar toolset and process. I've appreciated
    getting a better understanding of idiomatic go.


    https://codereview.appspot.com/6700047/
  • Rsc at Jan 9, 2013 at 7:43 pm
    FYI



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

    https://codereview.appspot.com/6700047/diff/53007/src/pkg/archive/tar/reader.go#newcode179
    src/pkg/archive/tar/reader.go:179: nano_buf := string(buf[pos+1:])
    nanoBuf

    https://codereview.appspot.com/6700047/diff/53007/src/pkg/archive/tar/reader.go#newcode213
    src/pkg/archive/tar/reader.go:213: if sp == -1 {
    sp < 0

    https://codereview.appspot.com/6700047/diff/53007/src/pkg/archive/tar/reader.go#newcode228
    src/pkg/archive/tar/reader.go:228: if eq == -1 {
    eq < 0

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

    https://codereview.appspot.com/6700047/diff/53007/src/pkg/archive/tar/writer.go#newcode136
    src/pkg/archive/tar/writer.go:136: // Decide whether or not to use PAX
    extensions
    Please don't use PAX headers for names that are only 101 bytes long.
    An option that should be used before then is to use the split ustar
    header form,
    in which a prefix of at most 155 bytes appears after the Devminor bytes.
    The full name is prefix + "/" + name. I expect that more readers will be
    able to handle this than will know about PAX extensions. I am looking at
    one right now.

    My suggestion also matches what BSD tar does:

    $ tar c
    tmp/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/very/long/name
    xd -c | sed 200q
    0000000 v e r y / v e r y / v e r y / v
    0000010 e r y / v e r y / v e r y / v e
    0000020 r y / v e r y / v e r y / v e r
    0000030 y / v e r y / v e r y / v e r y
    0000040 / v e r y / v e r y / v e r y /
    0000050 v e r y / v e r y / l o n g / n
    0000060 a m e 00 0 0 0 6 4 4 00 0 5 2 4
    0000070 5 5 00 0 0 0 0 0 0 00 0 0 0 0
    0000080 0 0 0 0 0 0 3 1 2 0 7 3 3 4 3
    0000090 4 0 1 0 5 1 5 7 2 00 0 00 00 00
    00000a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000100 00 u s t a r 00 0 0 r s c 00 00 00 00
    0000110 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000120 00 00 00 00 00 00 00 00 00 w h e e l 00 00
    0000130 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000140 00 00 00 00 00 00 00 00 00 0 0 0 0 0 0
    0000150 00 0 0 0 0 0 0 00 t m p / v e r
    0000160 y / v e r y / v e r y / v e r y
    0000170 / v e r y / v e r y / v e r y /
    0000180 v e r y / v e r y / v e r y / v
    0000190 e r y / v e r y / v e r y 00 00 00
    00001a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

    (OS X).

    I tried the same on Linux and I got a tar file using the GNU
    ././@LongLink extension:

    0000000 . / . / @ L o n g L i n k 00 00 00
    0000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000040 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000060 00 00 00 00 0 0 0 0 0 0 0 00 0 0 0 0
    0000070 0 0 0 00 0 0 0 0 0 0 0 00 0 0 0 0
    0000080 0 0 0 0 2 5 1 00 0 0 0 0 0 0 0 0
    0000090 0 0 0 00 0 1 1 5 6 3 00 L 00 00 00
    00000a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000100 00 u s t a r 00 r o o t 00 00 00
    0000110 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000120 00 00 00 00 00 00 00 00 00 r o o t 00 00 00
    0000130 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000140 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000150 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000160 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000170 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000180 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000190 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001a0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00001f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000200 t m p / v e r y / v e r y / v e
    0000210 r y / v e r y / v e r y / v e r
    0000220 y / v e r y / v e r y / v e r y
    0000230 / v e r y / v e r y / v e r y /
    0000240 v e r y / v e r y / v e r y / v
    0000250 e r y / v e r y / v e r y / v e
    0000260 r y / v e r y / v e r y / v e r
    0000270 y / v e r y / v e r y / v e r y
    0000280 / v e r y / v e r y / v e r y /
    0000290 v e r y / v e r y / v e r y / l
    00002a0 o n g / n a m e 00 00 00 00 00 00 00 00
    00002b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00002c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00002d0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00002e0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    00002f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0000300 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

    In neither case did I get a PAX header. I am worried that nothing
    generates
    PAX headers and therefore nothing will read them. I would rather see
    either
    of these forms (preferably the BSD one, which I believe is more
    widespread)
    before the PAX header here.

    I have no problem with this package reading the PAX stuff, only with
    writing it.

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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 9, '13 at 1:46a
activeJan 9, '13 at 7:43p
posts8
users3
websitegolang.org

3 users in discussion

Shanemhansen: 5 posts Dsymonds: 2 posts Rsc: 1 post

People

Translate

site design / logo © 2022 Grokbase