code review 52310044: cmd/pack: rewrite in Go

cmd/pack: rewrite in Go
Replace the pack command, a C program, with a clean reimplementation in Go.
It does not need to reproduce the full feature set and it is no longer used by
the build chain, but has a role in looking inside archives created by the build
chain directly.

Since it's not in C, it is no longer build by dist, so remove it from cmd/dist and
make it a "tool" in cmd/go terminology.

Fixes issue 2705

[r, 2014-01-14 22:06:48]

Hello rsc (cc: golang-codereviews@googlegroups.com),

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

[dave_cheney.net, 2014-01-14 22:36:07]

Maybe also close this long standing issue,
https://code.google.com/p/go/issues/detail?id=2705


On Wed, Jan 15, 2014 at 9:06 AM, <r@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/pack: rewrite in Go
> Replace the pack command, a C program, with a clean reimplementation in
> Go.
> It is not necessary to reproduce the full feature set and it is no
> longer used by
> the build chain, but has a role in looking inside archives created by
> the build
> chain directly.
>
> Since it's not in C, it is no longer build by dist, so remove it from
> cmd/dist and
> make it a "tool" in cmd/go terminology.
>
> Please review this at https://codereview.appspot.com/52310044/
>
> Affected files (+713, -1613 lines):
>   M src/cmd/dist/build.c
>   M src/cmd/go/pkg.go
>   R src/cmd/pack/Makefile
>   R src/cmd/pack/ar.c
>   M src/cmd/pack/doc.go
>   A src/cmd/pack/pack.go
>   A src/cmd/pack/pack_test.go
>
>
> --
> 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.
>

[r, 2014-01-14 23:36:02]

Hello rsc@golang.org, dave@cheney.net (cc: golang-codereviews@googlegroups.com),

Please take another look.

[minux1, 2014-01-15 00:32:30]

https://codereview.appspot.com/52310044/diff/1/src/cmd/pack/pack.go
File src/cmd/pack/pack.go (right):

https://codereview.appspot.com/52310044/diff/1/src/cmd/pack/pack.go#newcode334
src/cmd/pack/pack.go:334: uid := 0 // TODO?
it seems the old C pack always uses 0 for uid and gid, so
remove the "// TODO?" ?

https://codereview.appspot.com/52310044/diff/1/src/cmd/pack/pack_test.go
File src/cmd/pack/pack_test.go (right):

https://codereview.appspot.com/52310044/diff/1/src/cmd/pack/pack_test.go#newc...
src/cmd/pack/pack_test.go:274: uid:   0, // TODO
also here?

[r, 2014-01-15 00:47:47]

https://codereview.appspot.com/52310044/diff/1/src/cmd/pack/pack.go
File src/cmd/pack/pack.go (right):

https://codereview.appspot.com/52310044/diff/1/src/cmd/pack/pack.go#newcode334
src/cmd/pack/pack.go:334: uid := 0 // TODO?
On 2014/01/15 00:32:30, minux wrote:
> it seems the old C pack always uses 0 for uid and gid, so
> remove the "// TODO?" ?

the old C pack was wrong too then, but i'm not sure what should happen here,
really, which is why it's a TODO.

[josharian, 2014-01-15 01:49:37]

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go
File src/cmd/pack/pack.go (right):

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:74: // These veriables hold the decoded operation specified
by the first argument.
s/veriables/variables/

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:203: if err != nil || n != len(buf) {
Seems unlikely you'd get a partial read from a file, but io.ReadFull might be
better here. Maybe something like

switch _, err := io.ReadFull(ar.fd, buf); err {
case io.EOF:
	return nil
case nil:
default:
	log.Fatal("pack: file is not an archive: bad entry")
}

[rsc, 2014-01-15 03:02:48]

LGTM
very nice

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go
File src/cmd/pack/pack.go (right):

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:149: _, err := fd.Read(buf)
io.ReadFull(fd, buf)
same for various reads below.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:188: modeFormat(e.mode),
e.mode.String() will work here too
if you are worried about high bits you can do (e.mode & 0777).String().
then can drop modeFormat.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:203: if err != nil || n != len(buf) {
|| buf[entryLen-2] != '`' || buf[entryLen-1] != '\n'

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:333: mtime := info.ModTime().Unix()
// mtime, uid, gid are all zero so that repeated builds
// produce identical outputs.
mtime := int64(0)
uid := 0
gid := 0

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:341: _, err = io.Copy(ar.fd, fd)
should double-check amount copied against size just to be safe

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:353: // atMost16Bytes truncates the string if necessary so
it is at most 16 bytes long.
This should probably be exactly16Bytes and do the space padding, and then the
format changes to plain %s instead of %-16s. I believe that if your file is
named, say, ☺, %-16s will add 15 spaces instead of 13.

[r, 2014-01-15 16:58:02]

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go
File src/cmd/pack/pack.go (right):

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:149: _, err := fd.Read(buf)
On 2014/01/15 03:02:49, rsc wrote:
> io.ReadFull(fd, buf)
> same for various reads below.

Done.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:188: modeFormat(e.mode),
On 2014/01/15 03:02:49, rsc wrote:
> e.mode.String() will work here too
> if you are worried about high bits you can do (e.mode & 0777).String().
> then can drop modeFormat.

Done.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:203: if err != nil || n != len(buf) {
On 2014/01/15 01:49:37, josharian wrote:
> Seems unlikely you'd get a partial read from a file, but io.ReadFull might be
> better here. Maybe something like
> 
> switch _, err := io.ReadFull(ar.fd, buf); err {
> case io.EOF:
> 	return nil
> case nil:
> default:
> 	log.Fatal("pack: file is not an archive: bad entry")
> }

Done.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:203: if err != nil || n != len(buf) {
On 2014/01/15 03:02:49, rsc wrote:
> || buf[entryLen-2] != '`' || buf[entryLen-1] != '\n'

Done.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:333: mtime := info.ModTime().Unix()
On 2014/01/15 03:02:49, rsc wrote:
> // mtime, uid, gid are all zero so that repeated builds
> // produce identical outputs.
> mtime := int64(0)
> uid := 0
> gid := 0

Done.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:341: _, err = io.Copy(ar.fd, fd)
On 2014/01/15 03:02:49, rsc wrote:
> should double-check amount copied against size just to be safe

Done.

https://codereview.appspot.com/52310044/diff/20001/src/cmd/pack/pack.go#newco...
src/cmd/pack/pack.go:353: // atMost16Bytes truncates the string if necessary so
it is at most 16 bytes long.
On 2014/01/15 03:02:49, rsc wrote:
> This should probably be exactly16Bytes and do the space padding, and then the
> format changes to plain %s instead of %-16s. I believe that if your file is
> named, say, ☺, %-16s will add 15 spaces instead of 13.

Done.

[r, 2014-01-15 17:13:20]

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

cmd/pack: rewrite in Go
Replace the pack command, a C program, with a clean reimplementation in Go.
It does not need to reproduce the full feature set and it is no longer used by
the build chain, but has a role in looking inside archives created by the build
chain directly.

Since it's not in C, it is no longer build by dist, so remove it from cmd/dist
and
make it a "tool" in cmd/go terminology.

Fixes issue 2705

R=rsc, dave, minux.ma, josharian
CC=golang-codereviews
https://codereview.appspot.com/52310044