Merge pull request #3259 from bk2204/custom-transfer-args

Expand custom transfer args by using the shell

docs/custom-transfers.md

@@ -56,7 +56,8 @@ A custom transfer process is defined under a settings group called
 
   If the custom transfer process requires any arguments, these can be provided
   here. Typically you would only need this if your process was multi-purpose or
-  particularly flexible, most of the time you won't need it.
+  particularly flexible, most of the time you won't need it.  Note that this
+  string will be expanded by the shell.
 
 * `lfs.customtransfer.<name>.concurrent`
 

docs/man/git-lfs-config.5.ronn

@@ -128,7 +128,7 @@ be scoped inside the configuration for a remote.
 * `lfs.customtransfer.<name>.args`
 
   If the custom transfer process requires any arguments, these can be provided
-  here.
+  here.  This string will be expanded by the shell.
 
 * `lfs.customtransfer.<name>.concurrent`
 

lfshttp/ssh.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/git-lfs/git-lfs/config"
+	"github.com/git-lfs/git-lfs/subprocess"
 	"github.com/git-lfs/git-lfs/tools"
 	"github.com/rubyist/tracerx"
 )
@@ -120,8 +121,7 @@ func sshFormatArgs(cmd string, args []string, needShell bool) (string, []string)
 		return cmd, args
 	}
 
-	joined := cmd + " " + strings.Join(tools.ShellQuote(args), " ")
-	return "sh", []string{"-c", joined}
+	return subprocess.FormatForShellQuotedArgs(cmd, args)
 }
 
 func sshGetLFSExeAndArgs(osEnv config.Environment, gitEnv config.Environment, e Endpoint, method string) (string, []string) {

subprocess/subprocess.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -83,6 +84,43 @@ func Output(cmd *Cmd) (string, error) {
 	return strings.Trim(string(out), " \n"), err
 }
 
+var shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@/.-]+\z`)
+
+// ShellQuoteSingle returns a string which is quoted suitably for sh.
+func ShellQuoteSingle(str string) string {
+	// Quote anything that looks slightly complicated.
+	if shellWordRe.FindStringIndex(str) == nil {
+		return "'" + strings.Replace(str, "'", "'\\''", -1) + "'"
+	}
+	return str
+}
+
+// ShellQuote returns a copied string slice where each element is quoted
+// suitably for sh.
+func ShellQuote(strs []string) []string {
+	dup := make([]string, 0, len(strs))
+
+	for _, str := range strs {
+		dup = append(dup, ShellQuoteSingle(str))
+	}
+	return dup
+}
+
+// FormatForShell takes a command name and an argument string and returns a
+// command and arguments that pass this command to the shell.  Note that neither
+// the command nor the arguments are quoted.  Consider FormatForShellQuoted
+// instead.
+func FormatForShell(name string, args string) (string, []string) {
+	return "sh", []string{"-c", name + " " + args}
+}
+
+// FormatForShellQuotedArgs takes a command name and an argument string and
+// returns a command and arguments that pass this command to the shell.  The
+// arguments are escaped, but the name of the command is not.
+func FormatForShellQuotedArgs(name string, args []string) (string, []string) {
+	return FormatForShell(name, strings.Join(ShellQuote(args), " "))
+}
+
 func Trace(name string, args ...string) {
 	tracerx.Printf("exec: %s %s", name, quotedArgs(args))
 }

subprocess/subprocess_test.go

@@ -0,0 +1,91 @@
+package subprocess
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+type ShellQuoteTestCase struct {
+	Given    []string
+	Expected []string
+}
+
+func (c *ShellQuoteTestCase) Assert(t *testing.T) {
+	actual := ShellQuote(c.Given)
+
+	assert.Equal(t, c.Expected, actual,
+		"tools: expected ShellQuote(%q) to equal %#v (was %#v)",
+		c.Given, c.Expected, actual,
+	)
+}
+
+func TestShellQuote(t *testing.T) {
+	for desc, c := range map[string]ShellQuoteTestCase{
+		"simple":         {[]string{"foo", "bar", "an_id"}, []string{"foo", "bar", "an_id"}},
+		"leading space":  {[]string{" foo", "bar"}, []string{"' foo'", "bar"}},
+		"trailing space": {[]string{"foo", "bar "}, []string{"foo", "'bar '"}},
+		"internal space": {[]string{"foo bar", "baz quux"}, []string{"'foo bar'", "'baz quux'"}},
+		"backslash":      {[]string{`foo\bar`, `b\az`}, []string{`'foo\bar'`, `'b\az'`}},
+		"quotes":         {[]string{`foo"bar`, "b'az"}, []string{`'foo"bar'`, "'b'\\''az'"}},
+		"mixed quotes":   {[]string{`"foo'ba\"r\"'"`}, []string{`'"foo'\''ba\"r\"'\''"'`}},
+	} {
+		t.Run(desc, c.Assert)
+	}
+}
+
+type FormatForShellQuotedArgsTestCase struct {
+	GivenCmd     string
+	GivenArgs    []string
+	ExpectedArgs []string
+}
+
+func (c *FormatForShellQuotedArgsTestCase) Assert(t *testing.T) {
+	actualCmd, actualArgs := FormatForShellQuotedArgs(c.GivenCmd, c.GivenArgs)
+
+	assert.Equal(t, "sh", actualCmd,
+		"tools: expected FormatForShell command to equal 'sh' (was #%v)",
+		actualCmd)
+	assert.Equal(t, c.ExpectedArgs, actualArgs,
+		"tools: expected FormatForShell(%q, %v) to equal %#v (was %#v)",
+		c.GivenCmd, c.GivenArgs, c.ExpectedArgs, actualArgs,
+	)
+}
+
+func TestFormatForShellQuotedArgs(t *testing.T) {
+	for desc, c := range map[string]FormatForShellQuotedArgsTestCase{
+		"simple":      {"foo", []string{"bar", "baz"}, []string{"-c", "foo bar baz"}},
+		"spaces":      {"foo quux", []string{" bar", "baz "}, []string{"-c", "foo quux ' bar' 'baz '"}},
+		"backslashes": {"bin/foo", []string{"\\bar", "b\\az"}, []string{"-c", "bin/foo '\\bar' 'b\\az'"}},
+	} {
+		t.Run(desc, c.Assert)
+	}
+}
+
+type FormatForShellTestCase struct {
+	GivenCmd     string
+	GivenArgs    string
+	ExpectedArgs []string
+}
+
+func (c *FormatForShellTestCase) Assert(t *testing.T) {
+	actualCmd, actualArgs := FormatForShell(c.GivenCmd, c.GivenArgs)
+
+	assert.Equal(t, "sh", actualCmd,
+		"tools: expected FormatForShell command to equal 'sh' (was #%v)",
+		actualCmd)
+	assert.Equal(t, c.ExpectedArgs, actualArgs,
+		"tools: expected FormatForShell(%q, %v) to equal %#v (was %#v)",
+		c.GivenCmd, c.GivenArgs, c.ExpectedArgs, actualArgs,
+	)
+}
+
+func TestFormatForShell(t *testing.T) {
+	for desc, c := range map[string]FormatForShellTestCase{
+		"simple": {"foo", "bar", []string{"-c", "foo bar"}},
+		"spaces": {"foo quux", "bar baz", []string{"-c", "foo quux bar baz"}},
+		"quotes": {"bin/foo", "bar \"baz quux\" 'fred wilma'", []string{"-c", "bin/foo bar \"baz quux\" 'fred wilma'"}},
+	} {
+		t.Run(desc, c.Assert)
+	}
+}

t/cmd/lfstest-standalonecustomadapter.go

@@ -28,6 +28,10 @@ func main() {
 		os.Exit(1)
 	}
 
+	for _, arg := range os.Args {
+		writeToStderr(fmt.Sprintf("Saw argument %q\n", arg), errWriter)
+	}
+
 	for scanner.Scan() {
 		line := scanner.Text()
 		var req request

t/t-custom-transfers.sh

@@ -29,7 +29,7 @@ begin_test "custom-transfer-wrong-path"
   # use PIPESTATUS otherwise we get exit code from tee
   res=${PIPESTATUS[0]}
   grep "xfer: adapter \"testcustom\" Begin()" pushcustom.log
-  grep "Failed to start custom transfer command" pushcustom.log
+  grep "xfer: Aborting worker process" pushcustom.log
   if [ "$res" = "0" ]; then
     echo "Push should have failed because of an incorrect custom transfer path."
     exit 1
@@ -123,6 +123,7 @@ begin_test "custom-transfer-standalone"
 
   # set up custom transfer adapter to use a specific transfer agent
   git config lfs.customtransfer.testcustom.path lfstest-standalonecustomadapter
+  git config lfs.customtransfer.testcustom.args "--arg1 '--arg2 --arg3' --arg4"
   git config lfs.customtransfer.testcustom.concurrent false
   git config lfs.standalonetransferagent testcustom
   export TEST_STANDALONE_BACKUP_PATH="$(pwd)/test-custom-transfer-standalone-backup"
@@ -187,6 +188,11 @@ begin_test "custom-transfer-standalone"
 
   grep "Terminating test custom adapter gracefully" fetchcustom.log
 
+  # Test argument parsing.
+  grep 'Saw argument "--arg1"' fetchcustom.log
+  grep 'Saw argument "--arg2 --arg3"' fetchcustom.log
+  grep 'Saw argument "--arg4"' fetchcustom.log
+
   objectlist=`find .git/lfs/objects -type f`
   [ "$(echo "$objectlist" | wc -l)" -eq 12 ]
 )

tools/str_tools.go

@@ -46,22 +46,6 @@ func QuotedFields(s string) []string {
 	return out
 }
 
-// ShellQuote returns a copied string slice where each element is quoted
-// suitably for sh.
-func ShellQuote(strs []string) []string {
-	dup := make([]string, 0, len(strs))
-
-	for _, str := range strs {
-		// Quote anything that looks slightly complicated.
-		if shellWordRe.FindStringIndex(str) == nil {
-			dup = append(dup, "'"+strings.Replace(str, "'", "'\\''", -1)+"'")
-		} else {
-			dup = append(dup, str)
-		}
-	}
-	return dup
-}
-
 // Ljust returns a copied string slice where each element is left justified to
 // match the width of the longest element in the set.
 func Ljust(strs []string) []string {
@@ -129,8 +113,7 @@ func Indent(str string) string {
 }
 
 var (
-	tabRe       = regexp.MustCompile(`(?m)^[ \t]+`)
-	shellWordRe = regexp.MustCompile(`\A[A-Za-z0-9_@.-]+\z`)
+	tabRe = regexp.MustCompile(`(?m)^[ \t]+`)
 )
 
 // Undent removes all leading tabs in the given string "str", line-wise.

tools/str_tools_test.go

@@ -71,34 +71,6 @@ func TestQuotedFields(t *testing.T) {
 	}
 }
 
-type ShellQuoteTestCase struct {
-	Given    []string
-	Expected []string
-}
-
-func (c *ShellQuoteTestCase) Assert(t *testing.T) {
-	actual := ShellQuote(c.Given)
-
-	assert.Equal(t, c.Expected, actual,
-		"tools: expected ShellQuote(%q) to equal %#v (was %#v)",
-		c.Given, c.Expected, actual,
-	)
-}
-
-func TestShellQuote(t *testing.T) {
-	for desc, c := range map[string]ShellQuoteTestCase{
-		"simple":         {[]string{"foo", "bar", "an_id"}, []string{"foo", "bar", "an_id"}},
-		"leading space":  {[]string{" foo", "bar"}, []string{"' foo'", "bar"}},
-		"trailing space": {[]string{"foo", "bar "}, []string{"foo", "'bar '"}},
-		"internal space": {[]string{"foo bar", "baz quux"}, []string{"'foo bar'", "'baz quux'"}},
-		"backslash":      {[]string{`foo\bar`, `b\az`}, []string{`'foo\bar'`, `'b\az'`}},
-		"quotes":         {[]string{`foo"bar`, "b'az"}, []string{`'foo"bar'`, "'b'\\''az'"}},
-		"mixed quotes":   {[]string{`"foo'ba\"r\"'"`}, []string{`'"foo'\''ba\"r\"'\''"'`}},
-	} {
-		t.Run(desc, c.Assert)
-	}
-}
-
 func TestLongestReturnsEmptyStringGivenEmptySet(t *testing.T) {
 	assert.Equal(t, "", Longest(nil))
 }

tq/custom.go

@@ -128,7 +128,8 @@ func (a *customAdapter) WorkerStarting(workerNum int) (interface{}, error) {
 	// Start a process per worker
 	// If concurrent = false we have already dialled back workers to 1
 	a.Trace("xfer: starting up custom transfer process %q for worker %d", a.name, workerNum)
-	cmd := subprocess.ExecCommand(a.path, a.args)
+	cmdName, cmdArgs := subprocess.FormatForShell(subprocess.ShellQuoteSingle(a.path), a.args)
+	cmd := subprocess.ExecCommand(cmdName, cmdArgs...)
 	outp, err := cmd.StdoutPipe()
 	if err != nil {
 		return nil, fmt.Errorf("Failed to get stdout for custom transfer command %q remote: %v", a.path, err)