os/exec: fix Command with relative paths

Command was (and is) documented like:
"If name contains no path separators, Command uses LookPath to
resolve the path to a complete name if possible. Otherwise it
uses name directly."

But that wasn't true. It always did LookPath, and then
set a sticky error that the user couldn't unset.
And then if cmd.Dir was changed, Start would still fail
due to the earlier sticky error being set.

This keeps LookPath in the same place as before (so no user
visible changes in cmd.Path after Command), but only does
it when the documentation says it will happen.

Also, clarify the docs about a relative Dir path.

No change in any existing behavior, except using Command
is now possible with relative paths. Previously it only
worked if you built the *Cmd by hand.

Fixes #7228

LGTM=iant
R=iant
CC=adg, golang-codereviews
https://golang.org/cl/59580044

diff --git a/src/pkg/os/exec/exec.go b/src/pkg/os/exec/exec.go
index 491cc242bb27a0c150db5f8ea6736312be3ba5d1..ea4f692a31c3dd66b7e19c4bfc42bf717a2c5891 100644 (file)
--- a/src/pkg/os/exec/exec.go
+++ b/src/pkg/os/exec/exec.go
@@ -33,7 +33,8 @@ type Cmd struct {
        // Path is the path of the command to run.
        //
        // This is the only field that must be set to a non-zero
-       // value.
+       // value. If Path is relative, it is evaluated relative
+       // to Dir.
        Path string
 
        // Args holds command line arguments, including the command as Args[0].
@@ -84,7 +85,7 @@ type Cmd struct {
        // available after a call to Wait or Run.
        ProcessState *os.ProcessState
 
-       err             error // last error (from LookPath, stdin, stdout, stderr)
+       lookPathErr     error // LookPath error, if any.
        finished        bool  // when Wait was called
        childFiles      []*os.File
        closeAfterStart []io.Closer
@@ -96,8 +97,7 @@ type Cmd struct {
 // Command returns the Cmd struct to execute the named program with
 // the given arguments.
 //
-// It sets Path and Args in the returned structure and zeroes the
-// other fields.
+// It sets only the Path and Args in the returned structure.
 //
 // If name contains no path separators, Command uses LookPath to
 // resolve the path to a complete name if possible. Otherwise it uses
@@ -107,19 +107,31 @@ type Cmd struct {
 // followed by the elements of arg, so arg should not include the
 // command name itself. For example, Command("echo", "hello")
 func Command(name string, arg ...string) *Cmd {
-       aname, err := LookPath(name)
-       if err != nil {
-               aname = name
-       }
-       return &Cmd{
-               Path: aname,
+       cmd := &Cmd{
+               Path: name,
                Args: append([]string{name}, arg...),
-               err:  err,
        }
+       if !containsPathSeparator(name) {
+               if lp, err := LookPath(name); err != nil {
+                       cmd.lookPathErr = err
+               } else {
+                       cmd.Path = lp
+               }
+       }
+       return cmd
+}
+
+func containsPathSeparator(s string) bool {
+       for i := 0; i < len(s); i++ {
+               if os.IsPathSeparator(s[i]) {
+                       return true
+               }
+       }
+       return false
 }
 
 // interfaceEqual protects against panics from doing equality tests on
-// two interfaces with non-comparable underlying types
+// two interfaces with non-comparable underlying types.
 func interfaceEqual(a, b interface{}) bool {
        defer func() {
                recover()
@@ -235,10 +247,10 @@ func (c *Cmd) Run() error {
 
 // Start starts the specified command but does not wait for it to complete.
 func (c *Cmd) Start() error {
-       if c.err != nil {
+       if c.lookPathErr != nil {
                c.closeDescriptors(c.closeAfterStart)
                c.closeDescriptors(c.closeAfterWait)
-               return c.err
+               return c.lookPathErr
        }
        if c.Process != nil {
                return errors.New("exec: already started")

diff --git a/src/pkg/os/exec/exec_test.go b/src/pkg/os/exec/exec_test.go
index ad71503a838b9254e92bab26e550971f9d1893d6..54d69bff0deabf0a9f6babe78a29a7c636541501 100644 (file)
--- a/src/pkg/os/exec/exec_test.go
+++ b/src/pkg/os/exec/exec_test.go
@@ -44,6 +44,33 @@ func TestEcho(t *testing.T) {
        }
 }
 
+func TestCommandRelativeName(t *testing.T) {
+       // Run our own binary as a relative path
+       // (e.g. "_test/exec.test") our parent directory.
+       base := filepath.Base(os.Args[0]) // "exec.test"
+       dir := filepath.Dir(os.Args[0])   // "/tmp/go-buildNNNN/os/exec/_test"
+       if dir == "." {
+               t.Skip("skipping; running test at root somehow")
+       }
+       parentDir := filepath.Dir(dir) // "/tmp/go-buildNNNN/os/exec"
+       dirBase := filepath.Base(dir)  // "_test"
+       if dirBase == "." {
+               t.Skipf("skipping; unexpected shallow dir of %q", dir)
+       }
+
+       cmd := exec.Command(filepath.Join(dirBase, base), "-test.run=TestHelperProcess", "--", "echo", "foo")
+       cmd.Dir = parentDir
+       cmd.Env = []string{"GO_WANT_HELPER_PROCESS=1"}
+
+       out, err := cmd.Output()
+       if err != nil {
+               t.Errorf("echo: %v", err)
+       }
+       if g, e := string(out), "foo\n"; g != e {
+               t.Errorf("echo: want %q, got %q", e, g)
+       }
+}
+
 func TestCatStdin(t *testing.T) {
        // Cat, testing stdin and stdout.
        input := "Input string\nLine 2"