code review 5331044: exec: introduce ExitError

exec: introduce ExitError

The existing code uses *os.Waitmsg as an os.Error,
but *os.Waitmsg is really just a stringer.

Introduce an explicit error type for the real error.

Not to be submitted until just before error goes in;
the gofix for error updates type assertions
        err.(*os.Waitmsg)
to
        err.(*exec.ExitError)

The seemingly redundant String method will become
an Error method when error goes in, and will no longer
be redundant.

[rsc, 2011-10-28 01:42:47]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2011-10-28 01:47:48]

LGTM

I seem to recall that it was like this at some point during code review
iterations.


On Thu, Oct 27, 2011 at 6:42 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> exec: introduce ExitError
>
> The existing code uses *os.Waitmsg as an os.Error,
> but *os.Waitmsg is really just a stringer.
>
> Introduce an explicit error type for the real error.
>
> Not to be submitted until just before error goes in;
> the gofix for error updates type assertions
>        err.(*os.Waitmsg)
> to
>        err.(*exec.ExitError)
>
> The seemingly redundant String method will become
> an Error method when error goes in, and will no longer
> be redundant.
>
> Please review this at
http://codereview.appspot.com/**5331044/<http://codereview.appspot.com/5331044/>
>
> Affected files:
>  M src/pkg/exec/exec.go
>  M src/pkg/exec/exec_test.go
>
>
> Index: src/pkg/exec/exec.go
> ==============================**==============================**=======
> --- a/src/pkg/exec/exec.go
> +++ b/src/pkg/exec/exec.go
> @@ -203,7 +203,7 @@
>  // status.
>  //
>  // If the command fails to run or doesn't complete successfully, the
> -// error is of type *os.Waitmsg. Other error types may be
> +// error is of type *ExitError. Other error types may be
>  // returned for I/O problems.
>  func (c *Cmd) Run() os.Error {
>        if err := c.Start(); err != nil {
> @@ -256,6 +256,15 @@
>        return nil
>  }
>
> +// An ExitError reports an unsuccessful exit by a command.
> +type ExitError struct {
> +       *os.Waitmsg
> +}
> +
> +func (e *ExitError) String() string {
> +       return e.Waitmsg.String()
> +}
> +
>  // Wait waits for the command to exit.
>  // It must have been started by Start.
>  //
> @@ -264,7 +273,7 @@
>  // status.
>  //
>  // If the command fails to run or doesn't complete successfully, the
> -// error is of type *os.Waitmsg. Other error types may be
> +// error is of type *ExitError. Other error types may be
>  // returned for I/O problems.
>  func (c *Cmd) Wait() os.Error {
>        if c.Process == nil {
> @@ -290,7 +299,7 @@
>        if err != nil {
>                return err
>        } else if !msg.Exited() || msg.ExitStatus() != 0 {
> -               return msg
> +               return &ExitError{msg}
>        }
>
>        return copyError
> Index: src/pkg/exec/exec_test.go
> ==============================**==============================**=======
> --- a/src/pkg/exec/exec_test.go
> +++ b/src/pkg/exec/exec_test.go
> @@ -53,8 +53,8 @@
>  func TestCatGoodAndBadFile(t *testing.T) {
>        // Testing combined output and error values.
>        bs, err := helperCommand("cat", "/bogus/file.foo", "exec_test.go").
> **CombinedOutput()
> -       if _, ok := err.(*os.Waitmsg); !ok {
> -               t.Errorf("expected Waitmsg from cat combined; got %T: %v",
> err, err)
> +       if _, ok := err.(*ExitError); !ok {
> +               t.Errorf("expected *ExitError from cat combined; got %T:
> %v", err, err)
>        }
>        s := string(bs)
>        sp := strings.SplitN(s, "\n", 2)
> @@ -81,12 +81,12 @@
>  func TestExitStatus(t *testing.T) {
>        // Test that exit values are returned correctly
>        err := helperCommand("exit", "42").Run()
> -       if werr, ok := err.(*os.Waitmsg); ok {
> +       if werr, ok := err.(*ExitError); ok {
>                if s, e := werr.String(), "exit status 42"; s != e {
>                        t.Errorf("from exit 42 got exit %q, want %q", s, e)
>                }
>        } else {
> -               t.Fatalf("expected Waitmsg from exit 42; got %T: %v", err,
> err)
> +               t.Fatalf("expected *ExitError from exit 42; got %T: %v",
> err, err)
>        }
>  }
>
>
>
>

[rsc, 2011-11-02 01:49:47]

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

exec: introduce ExitError

The existing code uses *os.Waitmsg as an os.Error,
but *os.Waitmsg is really just a stringer.

Introduce an explicit error type for the real error.

Not to be submitted until just before error goes in;
the gofix for error updates type assertions
        err.(*os.Waitmsg)
to
        err.(*exec.ExitError)

The seemingly redundant String method will become
an Error method when error goes in, and will no longer
be redundant.

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