Construct arg vector manually rather than parse string

By constructing an arg vector manually, we no longer need to quote arguments

Mandate that args must be passed when building a command

Now you need to provide an args array when building a command.
There are a handful of places where we need to deal with a string,
such as with user-defined custom commands, and for those we now require
that at the callsite they use str.ToArgv to do that. I don't want
to provide a method out of the box for it because I want to discourage its
use.

For some reason we were invoking a command through a shell when amending a
commit, and I don't believe we needed to do that as there was nothing user-
supplied about the command. So I've switched to using a regular command out-
side the shell there

pkg/commands/oscommands/cmd_obj.go

@@ -2,7 +2,9 @@ package oscommands
 
 import (
 	"os/exec"
+	"strings"
 
+	"github.com/samber/lo"
 	"github.com/sasha-s/go-deadlock"
 )
 
@@ -15,6 +17,9 @@ type ICmdObj interface {
 	// into a terminal e.g. 'sh -c git commit' as opposed to 'sh -c "git commit"'
 	ToString() string
 
+	// outputs args vector e.g. ["git", "commit", "-m", "my message"]
+	Args() []string
+
 	AddEnvVars(...string) ICmdObj
 	GetEnvVars() []string
 
@@ -61,8 +66,11 @@ type ICmdObj interface {
 }
 
 type CmdObj struct {
-	cmdStr string
-	cmd    *exec.Cmd
+	// the secureexec package will swap out the first arg with the full path to the binary,
+	// so we store these args separately so that ToString() will output the original
+	args []string
+
+	cmd *exec.Cmd
 
 	runner ICmdObjRunner
 
@@ -104,7 +112,19 @@ func (self *CmdObj) GetCmd() *exec.Cmd {
 }
 
 func (self *CmdObj) ToString() string {
-	return self.cmdStr
+	// if a given arg contains a space, we need to wrap it in quotes
+	quotedArgs := lo.Map(self.args, func(arg string, _ int) string {
+		if strings.Contains(arg, " ") {
+			return `"` + arg + `"`
+		}
+		return arg
+	})
+
+	return strings.Join(quotedArgs, " ")
+}
+
+func (self *CmdObj) Args() []string {
+	return self.args
 }
 
 func (self *CmdObj) AddEnvVars(vars ...string) ICmdObj {

pkg/commands/oscommands/cmd_obj_builder.go

@@ -10,12 +10,10 @@ import (
 )
 
 type ICmdObjBuilder interface {
-	// New returns a new command object based on the string provided
-	New(cmdStr string) ICmdObj
+	// NewFromArgs takes a slice of strings like []string{"git", "commit"} and returns a new command object.
+	New(args []string) ICmdObj
 	// NewShell takes a string like `git commit` and returns an executable shell command for it e.g. `sh -c 'git commit'`
 	NewShell(commandStr string) ICmdObj
-	// NewFromArgs takes a slice of strings like []string{"git", "commit"} and returns a new command object. This can be useful when you don't want to worry about whitespace and quoting and stuff.
-	NewFromArgs(args []string) ICmdObj
 	// Quote wraps a string in quotes with any necessary escaping applied. The reason for bundling this up with the other methods in this interface is that we basically always need to make use of this when creating new command objects.
 	Quote(str string) string
 }
@@ -28,24 +26,12 @@ type CmdObjBuilder struct {
 // poor man's version of explicitly saying that struct X implements interface Y
 var _ ICmdObjBuilder = &CmdObjBuilder{}
 
-func (self *CmdObjBuilder) New(cmdStr string) ICmdObj {
-	args := str.ToArgv(cmdStr)
+func (self *CmdObjBuilder) New(args []string) ICmdObj {
 	cmd := secureexec.Command(args[0], args[1:]...)
 	cmd.Env = os.Environ()
 
 	return &CmdObj{
-		cmdStr: cmdStr,
-		cmd:    cmd,
-		runner: self.runner,
-	}
-}
-
-func (self *CmdObjBuilder) NewFromArgs(args []string) ICmdObj {
-	cmd := secureexec.Command(args[0], args[1:]...)
-	cmd.Env = os.Environ()
-
-	return &CmdObj{
-		cmdStr: strings.Join(args, " "),
+		args:   args,
 		cmd:    cmd,
 		runner: self.runner,
 	}
@@ -67,8 +53,9 @@ func (self *CmdObjBuilder) NewShell(commandStr string) ICmdObj {
 		quotedCommand = self.Quote(commandStr)
 	}
 
-	shellCommand := fmt.Sprintf("%s %s %s", self.platform.Shell, self.platform.ShellArg, quotedCommand)
-	return self.New(shellCommand)
+	cmdArgs := str.ToArgv(fmt.Sprintf("%s %s %s", self.platform.Shell, self.platform.ShellArg, quotedCommand))
+
+	return self.New(cmdArgs)
 }
 
 func (self *CmdObjBuilder) CloneWithNewRunner(decorate func(ICmdObjRunner) ICmdObjRunner) *CmdObjBuilder {
@@ -80,6 +67,9 @@ func (self *CmdObjBuilder) CloneWithNewRunner(decorate func(ICmdObjRunner) ICmdO
 	}
 }
 
+const CHARS_REQUIRING_QUOTES = "\"\\$` "
+
+// If you update this method, be sure to update CHARS_REQUIRING_QUOTES
 func (self *CmdObjBuilder) Quote(message string) string {
 	var quote string
 	if self.platform.OS == "windows" {

pkg/commands/oscommands/cmd_obj_test.go

@@ -0,0 +1,33 @@
+package oscommands
+
+import (
+	"testing"
+)
+
+func TestCmdObjToString(t *testing.T) {
+	quote := func(s string) string {
+		return "\"" + s + "\""
+	}
+
+	scenarios := []struct {
+		cmdArgs  []string
+		expected string
+	}{
+		{
+			cmdArgs:  []string{"git", "push", "myfile.txt"},
+			expected: "git push myfile.txt",
+		},
+		{
+			cmdArgs:  []string{"git", "push", "my file.txt"},
+			expected: "git push \"my file.txt\"",
+		},
+	}
+
+	for _, scenario := range scenarios {
+		cmdObj := &CmdObj{args: scenario.cmdArgs}
+		actual := cmdObj.ToString()
+		if actual != scenario.expected {
+			t.Errorf("Expected %s, got %s", quote(scenario.expected), quote(actual))
+		}
+	}
+}

pkg/commands/oscommands/fake_cmd_obj_runner.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"fmt"
 	"regexp"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -91,7 +92,18 @@ func (self *FakeCmdObjRunner) Expect(expectedCmdStr string, output string, err e
 func (self *FakeCmdObjRunner) ExpectArgs(expectedArgs []string, output string, err error) *FakeCmdObjRunner {
 	self.ExpectFunc(func(cmdObj ICmdObj) (string, error) {
 		args := cmdObj.GetCmd().Args
-		assert.EqualValues(self.t, expectedArgs, args, fmt.Sprintf("command %d did not match expectation", self.expectedCmdIndex+1))
+
+		if runtime.GOOS == "windows" {
+			// thanks to the secureexec package, the first arg is something like
+			// '"C:\\Program Files\\Git\\mingw64\\bin\\<command>.exe"
+			// on windows so we'll just ensure it contains our program
+			assert.Contains(self.t, args[0], expectedArgs[0])
+		} else {
+			// first arg is the program name
+			assert.Equal(self.t, expectedArgs[0], args[0])
+		}
+
+		assert.EqualValues(self.t, expectedArgs[1:], args[1:], fmt.Sprintf("command %d did not match expectation", self.expectedCmdIndex+1))
 
 		return output, err
 	})

pkg/commands/oscommands/os.go

@@ -10,6 +10,7 @@ import (
 	"sync"
 
 	"github.com/go-errors/errors"
+	"github.com/samber/lo"
 
 	"github.com/atotto/clipboard"
 	"github.com/jesseduffield/generics/slices"
@@ -187,12 +188,18 @@ func (c *OSCommand) FileExists(path string) (bool, error) {
 }
 
 // PipeCommands runs a heap of commands and pipes their inputs/outputs together like A | B | C
-func (c *OSCommand) PipeCommands(commandStrings ...string) error {
-	cmds := slices.Map(commandStrings, func(cmdString string) *exec.Cmd {
-		return c.Cmd.New(cmdString).GetCmd()
+func (c *OSCommand) PipeCommands(cmdObjs ...ICmdObj) error {
+	cmds := slices.Map(cmdObjs, func(cmdObj ICmdObj) *exec.Cmd {
+		return cmdObj.GetCmd()
 	})
 
-	logCmdStr := strings.Join(commandStrings, " | ")
+	logCmdStr := strings.Join(
+		lo.Map(cmdObjs, func(cmdObj ICmdObj, _ int) string {
+			return cmdObj.ToString()
+		}),
+		" | ",
+	)
+
 	c.LogCommand(logCmdStr, true)
 
 	for i := 0; i < len(cmds)-1; i++ {

pkg/commands/oscommands/os_default_test.go

@@ -12,20 +12,20 @@ import (
 
 func TestOSCommandRunWithOutput(t *testing.T) {
 	type scenario struct {
-		command string
-		test    func(string, error)
+		args []string
+		test func(string, error)
 	}
 
 	scenarios := []scenario{
 		{
-			"echo -n '123'",
+			[]string{"echo", "-n", "123"},
 			func(output string, err error) {
 				assert.NoError(t, err)
 				assert.EqualValues(t, "123", output)
 			},
 		},
 		{
-			"rmdir unexisting-folder",
+			[]string{"rmdir", "unexisting-folder"},
 			func(output string, err error) {
 				assert.Regexp(t, "rmdir.*unexisting-folder.*", err.Error())
 			},
@@ -34,7 +34,7 @@ func TestOSCommandRunWithOutput(t *testing.T) {
 
 	for _, s := range scenarios {
 		c := NewDummyOSCommand()
-		s.test(c.Cmd.New(s.command).RunWithOutput())
+		s.test(c.Cmd.New(s.args).RunWithOutput())
 	}
 }
 

pkg/commands/oscommands/os_test.go

@@ -10,13 +10,13 @@ import (
 
 func TestOSCommandRun(t *testing.T) {
 	type scenario struct {
-		command string
-		test    func(error)
+		args []string
+		test func(error)
 	}
 
 	scenarios := []scenario{
 		{
-			"rmdir unexisting-folder",
+			[]string{"rmdir", "unexisting-folder"},
 			func(err error) {
 				assert.Regexp(t, "rmdir.*unexisting-folder.*", err.Error())
 			},
@@ -25,7 +25,7 @@ func TestOSCommandRun(t *testing.T) {
 
 	for _, s := range scenarios {
 		c := NewDummyOSCommand()
-		s.test(c.Cmd.New(s.command).Run())
+		s.test(c.Cmd.New(s.args).Run())
 	}
 }