code review 5777064: archive/tar: catch short writes.

archive/tar: catch short writes.

Also make error messages consistent throughout.

[dsymonds, 2012-03-12 06:22:15]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-03-12 06:27:08]

LGTM

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go...
src/pkg/archive/tar/writer.go:53: tw.err = fmt.Errorf("archive/tar: missed
writing %d bytes", tw.nb)
"I wouldn't say I've been missing it, Bob."

This error message sounds a bit weird.  I might say something more like:

"Header declared %d bytes, but only %d bytes were written."


Also, is a Write after a Flush an error?

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go...
src/pkg/archive/tar/writer.go:200: tw.Flush()
might as well just look at the return value of Flush here, rather than hoping
that Flush set tw.err

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_te...
File src/pkg/archive/tar/writer_test.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_te...
src/pkg/archive/tar/writer_test.go:158: t.Logf("big: %v", big)
leftover debugging?

[dsymonds, 2012-03-12 06:33:16]

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go...
src/pkg/archive/tar/writer.go:53: tw.err = fmt.Errorf("archive/tar: missed
writing %d bytes", tw.nb)
On 2012/03/12 06:27:08, bradfitz wrote:
> "I wouldn't say I've been missing it, Bob."

"I have eight different bosses, Bob!"

> This error message sounds a bit weird.  I might say something more like:
> 
> "Header declared %d bytes, but only %d bytes were written."

Yeah, except we aren't tracking the length of the file. I could add that, but
that's a bigger change than I really wanted to do at this stage. I'll get the
fix in now, and we can wordsmith later.

> Also, is a Write after a Flush an error?

Not necessarily. An empty write is fine, for one.

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go...
src/pkg/archive/tar/writer.go:200: tw.Flush()
On 2012/03/12 06:27:08, bradfitz wrote:
> might as well just look at the return value of Flush here, rather than hoping
> that Flush set tw.err

Other parts already expect flush to set tw.err, and it's simpler to keep this
consistent.

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_te...
File src/pkg/archive/tar/writer_test.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_te...
src/pkg/archive/tar/writer_test.go:158: t.Logf("big: %v", big)
On 2012/03/12 06:27:08, bradfitz wrote:
> leftover debugging?

oops, yes.

[dsymonds, 2012-03-12 06:33:41]

*** Submitted as http://code.google.com/p/go/source/detail?r=7dac9470d5d8 ***

archive/tar: catch short writes.

Also make error messages consistent throughout.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5777064

[rsc, 2012-03-12 17:01:04]

http://codereview.appspot.com/5777064/diff/5001/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

http://codereview.appspot.com/5777064/diff/5001/src/pkg/archive/tar/writer.go...
src/pkg/archive/tar/writer.go:53: tw.err = fmt.Errorf("archive/tar: missed
writing %d bytes", tw.nb)
This seems like a regression.  The code below was already correctly handling
writing zeros if the written data was too short.  Now you are creating a usage
error.  This is an API change.  The old code seems like it was fine.