code review 54570043: archive/tar: support extended attributes

archive/tar: support extended attributes

This adds support for archives with the SCHILY.xattr field in the
pax header. This is what gnu tar and star generate.
Fixes issue 7154.

[alexl, 2014-01-20 12:32:11]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[gobot, 2014-01-22 06:16:05]

R=dsymonds@golang.org (assigned by bradfitz@golang.org)

[dsymonds, 2014-02-05 23:44:48]

Looks pretty straightforward.

https://codereview.appspot.com/54570043/diff/40001/src/pkg/archive/tar/common.go
File src/pkg/archive/tar/common.go (right):

https://codereview.appspot.com/54570043/diff/40001/src/pkg/archive/tar/common...
src/pkg/archive/tar/common.go:193: paxSchilyXattr = "SCHILY.xattr."
unless you're planning to add other xattrs, name this paxXattr.

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

https://codereview.appspot.com/54570043/diff/40001/src/pkg/archive/tar/reader...
src/pkg/archive/tar/reader.go:143: if strings.HasPrefix(k, paxSchilyXattr) {
stick this in a default: case in the switch above so that this code isn't
redundantly running if one of the exact match cases fires.

https://codereview.appspot.com/54570043/diff/40001/src/pkg/archive/tar/writer...
File src/pkg/archive/tar/writer_test.go (right):

https://codereview.appspot.com/54570043/diff/40001/src/pkg/archive/tar/writer...
src/pkg/archive/tar/writer_test.go:376: t.Fatalf("Couldn't reconver
xattrs:\nhave %+v\nwant %+v",
t.Errorf("xattrs did not survive round trip: got %+v, want %+v", hdr.Xattrs,
xattrs)

[dsymonds, 2014-02-05 23:48:23]

Also, please fill out the CLA:

http://golang.org/doc/contribute.html#copyright

[alexl, 2014-02-10 09:27:51]

Hello golang-codereviews@googlegroups.com, gobot@golang.org, dsymonds@golang.org
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[alexl, 2014-02-10 09:45:18]

On 2014/02/05 23:48:23, dsymonds wrote:
> Also, please fill out the CLA:
> 
> http://golang.org/doc/contribute.html#copyright

I'll have a look at that.

[dsymonds, 2014-02-12 01:34:14]

LGTM

[dsymonds, 2014-02-12 01:35:26]

Can you please 'hg sync' and 'hg upload 54570043'. The current patchset does not
apply cleanly at tip.

[alexl, 2014-02-12 11:36:56]

On 2014/02/12 01:35:26, dsymonds wrote:
> Can you please 'hg sync' and 'hg upload 54570043'. The current patchset does
not
> apply cleanly at tip.

Updated. And i filed for an individual CLA, but has not heard anything back yet.

[dsymonds, 2014-02-12 11:46:34]

I've received and taken care of the CLA. But this patch still doesn't
apply cleanly. Can you sync and upload one more time before I try
applying the diff manually?

[alexl, 2014-02-12 13:10:52]

On 2014/02/12 11:46:34, dsymonds wrote:
> I've received and taken care of the CLA. But this patch still doesn't
> apply cleanly. Can you sync and upload one more time before I try
> applying the diff manually?

I can't say i understand the workflow perfectly, but i did a new hg sync + hg
upload. Hopefully it will be ok now...

[dsymonds, 2014-02-12 22:41:09]

I see the problem. The new test file, xattrs.tar, is choking Rietveld. I can't
patch it over, and it doesn't seem to want to come down in the .tar.bz2 for this
patch.

Can you please email me that file directly (dsymonds@golang.org), and revert it
from this CL?

[alexl, 2014-02-13 08:16:01]

On 2014/02/12 22:41:09, dsymonds wrote:
> I see the problem. The new test file, xattrs.tar, is choking Rietveld. I can't
> patch it over, and it doesn't seem to want to come down in the .tar.bz2 for
this
> patch.
> 
> Can you please email me that file directly (mailto:dsymonds@golang.org), and
revert it
> from this CL?

Updated, xattrs.tar is in the mail.

[dsymonds, 2014-02-13 09:08:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=a15f344a9efa ***

archive/tar: support extended attributes

This adds support for archives with the SCHILY.xattr field in the
pax header. This is what gnu tar and star generate.
Fixes issue 7154.

LGTM=dsymonds
R=golang-codereviews, gobot, dsymonds
CC=golang-codereviews
https://codereview.appspot.com/54570043

Committer: David Symonds <dsymonds@golang.org>

[gobot, 2014-02-13 09:29:30]

This CL appears to have broken the darwin-amd64-cheney builder.

[alexl, 2014-02-13 09:45:58]

On 2014/02/13 09:29:30, gobot wrote:
> This CL appears to have broken the darwin-amd64-cheney builder.

You mean http://build.golang.org/log/8a4031ed22d1c73897a1a844172a3f4bfb5f85ef ?
Seems unrelated. It also happens randomly before, like e.g.
http://build.golang.org/log/e7081df35cc314808df7ed1df2487127cb653b46

[dave_cheney.net, 2014-02-13 10:15:48]

Yeah, issue 7205, it's unrelated to your change, you can stand down.


On Thu, Feb 13, 2014 at 8:45 PM, <alexander.larsson@gmail.com> wrote:

> On 2014/02/13 09:29:30, gobot wrote:
>
>> This CL appears to have broken the darwin-amd64-cheney builder.
>>
>
> You mean
> http://build.golang.org/log/8a4031ed22d1c73897a1a844172a3f4bfb5f85ef ?
> Seems unrelated. It also happens randomly before, like e.g.
> http://build.golang.org/log/e7081df35cc314808df7ed1df2487127cb653b46
>
>
> https://codereview.appspot.com/54570043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[rsc, 2014-02-15 15:24:23]

The X in Xattrs looks more like a C name than a Go name. "ExtAttrs" would be
more in keeping with Go, but why not just "Attrs"?

[dsymonds, 2014-02-16 02:23:36]

These things are called "extended attributes" because the rest of the
header fields are called "attributes". "xattrs" is already the
established abbreviation for that term (e.g. the POSIX flag is
--xattrs), and the rest of this header is already mostly following the
standard abbreviations (e.g. "Uname" instead of "UserName"). If a
programmer cares about these they'll be searching for "xattr".

[rsc, 2014-02-16 15:21:47]

ok