Merge pull request #2952 from ipfs/fix/stdin-handling

rework stdin handling

commands/argument.go

@@ -42,9 +42,6 @@ func FileArg(name string, required, variadic bool, description string) Argument
 //       (`FileArg("file", ArgRequired, ArgStdin, ArgRecursive)`)
 
 func (a Argument) EnableStdin() Argument {
-	if a.Type == ArgString {
-		panic("Only FileArgs can be read from Stdin")
-	}
 	a.SupportsStdin = true
 	return a
 }

commands/cli/parse.go

@@ -56,6 +56,16 @@ func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *c
 		}
 	}
 
+	// This is an ugly hack to maintain our current CLI interface while fixing
+	// other stdin usage bugs. Let this serve as a warning, be careful about the
+	// choices you make, they will haunt you forever.
+	if len(path) == 2 && path[0] == "bootstrap" {
+		if (path[1] == "add" && opts["default"] == true) ||
+			(path[1] == "rm" && opts["all"] == true) {
+			stdin = nil
+		}
+	}
+
 	stringArgs, fileArgs, err := parseArgs(stringVals, stdin, cmd.Arguments, recursive, hidden, root)
 	if err != nil {
 		return req, cmd, path, err
@@ -276,6 +286,7 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 
 	fileArgs := make(map[string]files.File)
 	argDefIndex := 0 // the index of the current argument definition
+
 	for i := 0; i < numInputs; i++ {
 		argDef := getArgDef(argDefIndex, argDefs)
 
@@ -289,14 +300,17 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 		}
 
 		fillingVariadic := argDefIndex+1 > len(argDefs)
-
-		if argDef.Type == cmds.ArgString {
+		switch argDef.Type {
+		case cmds.ArgString:
 			if len(inputs) > 0 {
 				stringArgs, inputs = append(stringArgs, inputs[0]), inputs[1:]
-			} else {
-				break
+			} else if stdin != nil && argDef.SupportsStdin && !fillingVariadic {
+				if err := printReadInfo(stdin, msgStdinInfo); err == nil {
+					fileArgs[stdin.Name()] = files.NewReaderFile("stdin", "", stdin, nil)
+					stdin = nil
+				}
 			}
-		} else if argDef.Type == cmds.ArgFile {
+		case cmds.ArgFile:
 			if len(inputs) > 0 {
 				// treat stringArg values as file paths
 				fpath := inputs[0]
@@ -316,17 +330,13 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 				}
 
 				fileArgs[fpath] = file
-			} else {
-				if stdin != nil && argDef.SupportsStdin &&
-					argDef.Required && !fillingVariadic {
-					if err := printReadInfo(stdin, msgStdinInfo); err != nil {
-						return nil, nil, err
-					}
-					fpath := stdin.Name()
-					fileArgs[fpath] = files.NewReaderFile("", fpath, stdin, nil)
-				} else {
-					break
+			} else if stdin != nil && argDef.SupportsStdin &&
+				argDef.Required && !fillingVariadic {
+				if err := printReadInfo(stdin, msgStdinInfo); err != nil {
+					return nil, nil, err
 				}
+				fpath := stdin.Name()
+				fileArgs[fpath] = files.NewReaderFile("", fpath, stdin, nil)
 			}
 		}
 
@@ -411,15 +421,24 @@ func appendFile(fpath string, argDef *cmds.Argument, recursive, hidden bool) (fi
 
 // Inform the user if a file is waiting on input
 func printReadInfo(f *os.File, msg string) error {
-	fInfo, err := f.Stat()
+	isTty, err := isTty(f)
 	if err != nil {
-		log.Error(err)
 		return err
 	}
 
-	if (fInfo.Mode() & os.ModeCharDevice) != 0 {
+	if isTty {
 		fmt.Fprintf(os.Stderr, msg, f.Name())
 	}
 
 	return nil
 }
+
+func isTty(f *os.File) (bool, error) {
+	fInfo, err := f.Stat()
+	if err != nil {
+		log.Error(err)
+		return false, err
+	}
+
+	return (fInfo.Mode() & os.ModeCharDevice) != 0, nil
+}

commands/cli/parse_test.go

@@ -4,7 +4,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
@@ -178,49 +177,32 @@ func TestArgumentParsing(t *testing.T) {
 					commands.StringArg("b", true, false, "another arg"),
 				},
 			},
-			"FileArg": {
+			"stdinenabled": {
 				Arguments: []commands.Argument{
-					commands.FileArg("a", true, false, "some arg"),
+					commands.StringArg("a", true, true, "some arg").EnableStdin(),
 				},
 			},
-			"FileArg+Variadic": {
-				Arguments: []commands.Argument{
-					commands.FileArg("a", true, true, "some arg"),
-				},
-			},
-			"FileArg+Stdin": {
-				Arguments: []commands.Argument{
-					commands.FileArg("a", true, true, "some arg").EnableStdin(),
-				},
-			},
-			"StringArg+FileArg": {
-				Arguments: []commands.Argument{
-					commands.StringArg("a", true, false, "some arg"),
-					commands.FileArg("a", true, false, "some arg"),
-				},
-			},
-			"StringArg+FileArg+Stdin": {
+			"stdinenabled2args": &commands.Command{
 				Arguments: []commands.Argument{
 					commands.StringArg("a", true, false, "some arg"),
-					commands.FileArg("a", true, true, "some arg").EnableStdin(),
+					commands.StringArg("b", true, true, "another arg").EnableStdin(),
 				},
 			},
-			"StringArg+FileArg+Variadic": {
+			"stdinenablednotvariadic": &commands.Command{
 				Arguments: []commands.Argument{
-					commands.StringArg("a", true, false, "some arg"),
-					commands.FileArg("a", true, true, "some arg"),
+					commands.StringArg("a", true, false, "some arg").EnableStdin(),
 				},
 			},
-			"StringArg+FileArg+Variadic+Stdin": {
+			"stdinenablednotvariadic2args": &commands.Command{
 				Arguments: []commands.Argument{
 					commands.StringArg("a", true, false, "some arg"),
-					commands.FileArg("a", true, true, "some arg"),
+					commands.StringArg("b", true, false, "another arg").EnableStdin(),
 				},
 			},
 		},
 	}
 
-	test := func(cmd words, f *os.File, exp words) {
+	test := func(cmd words, f *os.File, res words) {
 		if f != nil {
 			if _, err := f.Seek(0, os.SEEK_SET); err != nil {
 				t.Fatal(err)
@@ -230,18 +212,8 @@ func TestArgumentParsing(t *testing.T) {
 		if err != nil {
 			t.Errorf("Command '%v' should have passed parsing: %v", cmd, err)
 		}
-
-		parsedWords := make([]string, len(req.Arguments()))
-		copy(parsedWords, req.Arguments())
-
-		if files := req.Files(); files != nil {
-			for file, err := files.NextFile(); err != io.EOF; file, err = files.NextFile() {
-				parsedWords = append(parsedWords, file.FullPath())
-			}
-		}
-
-		if !sameWords(parsedWords, exp) {
-			t.Errorf("Arguments parsed from '%v' are '%v' instead of '%v'", cmd, parsedWords, exp)
+		if !sameWords(req.Arguments(), res) {
+			t.Errorf("Arguments parsed from '%v' are '%v' instead of '%v'", cmd, req.Arguments(), res)
 		}
 	}
 
@@ -281,52 +253,59 @@ func TestArgumentParsing(t *testing.T) {
 	testFail([]string{"reversedoptional"}, nil, "didn't provide any args, 1 required")
 	testFail([]string{"reversedoptional", "value1", "value2", "value3"}, nil, "provided too many args, only takes 1")
 
-	// Since FileArgs are presently stored ordered by Path, the enum string
-	// is used to construct a predictably ordered sequence of filenames.
-	tmpFile := func(t *testing.T, enum string) *os.File {
-		f, err := ioutil.TempFile("", enum)
+	// Use a temp file to simulate stdin
+	fileToSimulateStdin := func(t *testing.T, content string) *os.File {
+		fstdin, err := ioutil.TempFile("", "")
 		if err != nil {
 			t.Fatal(err)
 		}
-		fn, err := filepath.EvalSymlinks(f.Name())
-		if err != nil {
-			t.Fatal(err)
-		}
-		f.Close()
-		f, err = os.Create(fn)
-		if err != nil {
+		defer os.Remove(fstdin.Name())
+
+		if _, err := io.WriteString(fstdin, content); err != nil {
 			t.Fatal(err)
 		}
-
-		return f
+		return fstdin
 	}
-	file1 := tmpFile(t, "1")
-	file2 := tmpFile(t, "2")
-	file3 := tmpFile(t, "3")
-	defer os.Remove(file3.Name())
-	defer os.Remove(file2.Name())
-	defer os.Remove(file1.Name())
-
-	test([]string{"noarg"}, file1, []string{})
-	test([]string{"FileArg", file1.Name()}, nil, []string{file1.Name()})
-	test([]string{"FileArg+Variadic", file1.Name(), file2.Name()}, nil,
-		[]string{file1.Name(), file2.Name()})
-	test([]string{"FileArg+Stdin"}, file1, []string{file1.Name()})
-	test([]string{"FileArg+Stdin", "-"}, file1, []string{file1.Name()})
-	test([]string{"FileArg+Stdin", file1.Name(), "-"}, file2,
-		[]string{file1.Name(), file2.Name()})
-	test([]string{"StringArg+FileArg",
-		"foo", file1.Name()}, nil, []string{"foo", file1.Name()})
-	test([]string{"StringArg+FileArg+Variadic",
-		"foo", file1.Name(), file2.Name()}, nil,
-		[]string{"foo", file1.Name(), file2.Name()})
-	test([]string{"StringArg+FileArg+Stdin",
-		"foo", file1.Name(), "-"}, file2,
-		[]string{"foo", file1.Name(), file2.Name()})
-	test([]string{"StringArg+FileArg+Variadic+Stdin",
-		"foo", file1.Name(), file2.Name()}, file3,
-		[]string{"foo", file1.Name(), file2.Name()})
-	test([]string{"StringArg+FileArg+Variadic+Stdin",
-		"foo", file1.Name(), file2.Name(), "-"}, file3,
-		[]string{"foo", file1.Name(), file2.Name(), file3.Name()})
+
+	test([]string{"stdinenabled", "value1", "value2"}, nil, []string{"value1", "value2"})
+
+	fstdin := fileToSimulateStdin(t, "stdin1")
+	test([]string{"stdinenabled"}, fstdin, []string{"stdin1"})
+	test([]string{"stdinenabled", "value1"}, fstdin, []string{"value1"})
+	test([]string{"stdinenabled", "value1", "value2"}, fstdin, []string{"value1", "value2"})
+
+	fstdin = fileToSimulateStdin(t, "stdin1\nstdin2")
+	test([]string{"stdinenabled"}, fstdin, []string{"stdin1", "stdin2"})
+
+	fstdin = fileToSimulateStdin(t, "stdin1\nstdin2\nstdin3")
+	test([]string{"stdinenabled"}, fstdin, []string{"stdin1", "stdin2", "stdin3"})
+
+	test([]string{"stdinenabled2args", "value1", "value2"}, nil, []string{"value1", "value2"})
+
+	fstdin = fileToSimulateStdin(t, "stdin1")
+	test([]string{"stdinenabled2args", "value1"}, fstdin, []string{"value1", "stdin1"})
+	test([]string{"stdinenabled2args", "value1", "value2"}, fstdin, []string{"value1", "value2"})
+	test([]string{"stdinenabled2args", "value1", "value2", "value3"}, fstdin, []string{"value1", "value2", "value3"})
+
+	fstdin = fileToSimulateStdin(t, "stdin1\nstdin2")
+	test([]string{"stdinenabled2args", "value1"}, fstdin, []string{"value1", "stdin1", "stdin2"})
+
+	test([]string{"stdinenablednotvariadic", "value1"}, nil, []string{"value1"})
+
+	fstdin = fileToSimulateStdin(t, "stdin1")
+	test([]string{"stdinenablednotvariadic"}, fstdin, []string{"stdin1"})
+	test([]string{"stdinenablednotvariadic", "value1"}, fstdin, []string{"value1"})
+
+	test([]string{"stdinenablednotvariadic2args", "value1", "value2"}, nil, []string{"value1", "value2"})
+
+	fstdin = fileToSimulateStdin(t, "stdin1")
+	test([]string{"stdinenablednotvariadic2args", "value1"}, fstdin, []string{"value1", "stdin1"})
+	test([]string{"stdinenablednotvariadic2args", "value1", "value2"}, fstdin, []string{"value1", "value2"})
+	testFail([]string{"stdinenablednotvariadic2args"}, fstdin, "cant use stdin for non stdin arg")
+
+	fstdin = fileToSimulateStdin(t, "stdin1")
+	test([]string{"noarg"}, fstdin, []string{})
+
+	fstdin = fileToSimulateStdin(t, "stdin1")
+	test([]string{"optionalsecond", "value1", "value2"}, fstdin, []string{"value1", "value2"})
 }

commands/command.go

@@ -206,7 +206,7 @@ func (c *Command) GetOptions(path []string) (map[string]Option, error) {
 }
 
 func (c *Command) CheckArguments(req Request) error {
-	args := req.Arguments()
+	args := req.(*request).arguments
 
 	// count required argument definitions
 	numRequired := 0
@@ -218,7 +218,7 @@ func (c *Command) CheckArguments(req Request) error {
 
 	// iterate over the arg definitions
 	valueIndex := 0 // the index of the current value (in `args`)
-	for _, argDef := range c.Arguments {
+	for i, argDef := range c.Arguments {
 		// skip optional argument definitions if there aren't
 		// sufficient remaining values
 		if len(args)-valueIndex <= numRequired && !argDef.Required ||
@@ -235,6 +235,11 @@ func (c *Command) CheckArguments(req Request) error {
 			valueIndex++
 		}
 
+		// in the case of a non-variadic required argument that supports stdin
+		if !found && len(c.Arguments)-1 == i && argDef.SupportsStdin {
+			found = true
+		}
+
 		err := checkArgValue(v, found, argDef)
 		if err != nil {
 			return err
@@ -281,6 +286,10 @@ func (c *Command) ProcessHelp() {
 // checkArgValue returns an error if a given arg value is not valid for the
 // given Argument
 func checkArgValue(v string, found bool, def Argument) error {
+	if def.Variadic && def.SupportsStdin {
+		return nil
+	}
+
 	if !found && def.Required {
 		return fmt.Errorf("Argument '%s' is required", def.Name)
 	}

commands/http/client.go

@@ -128,7 +128,7 @@ func getQuery(req cmds.Request) (string, error) {
 		query.Set(k, str)
 	}
 
-	args := req.Arguments()
+	args := req.StringArguments()
 	argDefs := req.Command().Arguments
 
 	argDefIndex := 0

commands/reqlog.go

@@ -56,7 +56,7 @@ func (rl *ReqLog) Add(req Request) *ReqLogEntry {
 		Active:    true,
 		Command:   strings.Join(req.Path(), "/"),
 		Options:   req.Options(),
-		Args:      req.Arguments(),
+		Args:      req.StringArguments(),
 		ID:        rl.nextID,
 		req:       req,
 		log:       rl,