Merge pull request #2902 from thomas-gardner/commands

commands: remove EnableStdin support for StringArg [v2]

commands/argument.go

@@ -42,13 +42,16 @@ 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
 }
 
 func (a Argument) EnableRecursive() Argument {
 	if a.Type != ArgFile {
-		panic("Only ArgFile arguments can enable recursive")
+		panic("Only FileArgs can enable recursive")
 	}
 
 	a.Recursive = true

commands/cli/parse.go

@@ -1,7 +1,6 @@
 package cli
 
 import (
-	"bytes"
 	"fmt"
 	"os"
 	"path"
@@ -12,9 +11,12 @@ import (
 
 	cmds "github.com/ipfs/go-ipfs/commands"
 	files "github.com/ipfs/go-ipfs/commands/files"
+	logging "gx/ipfs/QmNQynaz7qfriSUJkiEZUrm2Wen1u3Kj9goZzWtrPyu7XR/go-log"
 	u "gx/ipfs/QmZNVWh8LLjAavuQ2JXuFmuYH3C11xo988vSgp7UQrTRj1/go-ipfs-util"
 )
 
+var log = logging.Logger("commands/cli")
+
 // Parse parses the input commandline string (cmd, flags, and args).
 // returns the corresponding command Request object.
 func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *cmds.Command, []string, error) {
@@ -238,6 +240,8 @@ func parseOpts(args []string, root *cmds.Command) (
 	return
 }
 
+const msgStdinInfo = "ipfs: Reading from %s; send Ctrl-d to stop.\n"
+
 func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursive, hidden bool, root *cmds.Command) ([]string, []files.File, error) {
 	// ignore stdin on Windows
 	if runtime.GOOS == "windows" {
@@ -286,36 +290,11 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 
 		fillingVariadic := argDefIndex+1 > len(argDefs)
 
-		var err error
 		if argDef.Type == cmds.ArgString {
 			if len(inputs) > 0 {
-				// If argument is "-" use stdin
-				if inputs[0] == "-" && argDef.SupportsStdin {
-					stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
-					if err != nil {
-						return nil, nil, err
-					}
-				}
-				// add string values
-				stringArgs, inputs = appendString(stringArgs, inputs)
-			} else if !argDef.SupportsStdin {
-				if len(inputs) == 0 {
-					// failure case, we have stdin, but our current
-					// argument doesnt want stdin
-					break
-				}
-
-				stringArgs, inputs = appendString(stringArgs, inputs)
+				stringArgs, inputs = append(stringArgs, inputs[0]), inputs[1:]
 			} else {
-				if stdin != nil && argDef.Required && !fillingVariadic {
-					// if we have a stdin, read it in and use the data as a string value
-					stringArgs, stdin, err = appendStdinAsString(stringArgs, stdin)
-					if err != nil {
-						return nil, nil, err
-					}
-				} else {
-					break
-				}
+				break
 			}
 		} else if argDef.Type == cmds.ArgFile {
 			if len(inputs) > 0 {
@@ -325,7 +304,10 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 				var file files.File
 				var err error
 				if fpath == "-" {
-					file = files.NewReaderFile("", "", stdin, nil)
+					if err = printReadInfo(stdin, msgStdinInfo); err == nil {
+						fpath = stdin.Name()
+						file = files.NewReaderFile("", fpath, stdin, nil)
+					}
 				} else {
 					file, err = appendFile(fpath, argDef, recursive, hidden)
 				}
@@ -337,7 +319,11 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 			} else {
 				if stdin != nil && argDef.SupportsStdin &&
 					argDef.Required && !fillingVariadic {
-					fileArgs[""] = files.NewReaderFile("", "", stdin, nil)
+					if err := printReadInfo(stdin, msgStdinInfo); err != nil {
+						return nil, nil, err
+					}
+					fpath := stdin.Name()
+					fileArgs[fpath] = files.NewReaderFile("", fpath, stdin, nil)
 				} else {
 					break
 				}
@@ -389,22 +375,6 @@ func getArgDef(i int, argDefs []cmds.Argument) *cmds.Argument {
 	return nil
 }
 
-func appendString(args, inputs []string) ([]string, []string) {
-	return append(args, inputs[0]), inputs[1:]
-}
-
-func appendStdinAsString(args []string, stdin *os.File) ([]string, *os.File, error) {
-	buf := new(bytes.Buffer)
-
-	_, err := buf.ReadFrom(stdin)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	input := strings.TrimSpace(buf.String())
-	return append(args, strings.Split(input, "\n")...), nil, nil
-}
-
 const notRecursiveFmtStr = "'%s' is a directory, use the '-%s' flag to specify directories"
 const dirNotSupportedFmtStr = "Invalid path '%s', argument '%s' does not support directories"
 
@@ -435,3 +405,18 @@ func appendFile(fpath string, argDef *cmds.Argument, recursive, hidden bool) (fi
 
 	return files.NewSerialFile(path.Base(fpath), fpath, hidden, stat)
 }
+
+// Inform the user if a file is waiting on input
+func printReadInfo(f *os.File, msg string) error {
+	fInfo, err := f.Stat()
+	if err != nil {
+		log.Error(err)
+		return err
+	}
+
+	if (fInfo.Mode() & os.ModeCharDevice) != 0 {
+		fmt.Fprintf(os.Stderr, msg, f.Name())
+	}
+
+	return nil
+}

commands/cli/parse_test.go

@@ -177,32 +177,49 @@ func TestArgumentParsing(t *testing.T) {
 					commands.StringArg("b", true, false, "another arg"),
 				},
 			},
-			"stdinenabled": {
+			"FileArg": {
 				Arguments: []commands.Argument{
-					commands.StringArg("a", true, true, "some arg").EnableStdin(),
+					commands.FileArg("a", true, false, "some arg"),
 				},
 			},
-			"stdinenabled2args": &commands.Command{
+			"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": {
 				Arguments: []commands.Argument{
 					commands.StringArg("a", true, false, "some arg"),
-					commands.StringArg("b", true, true, "another arg").EnableStdin(),
+					commands.FileArg("a", true, true, "some arg").EnableStdin(),
 				},
 			},
-			"stdinenablednotvariadic": &commands.Command{
+			"StringArg+FileArg+Variadic": {
 				Arguments: []commands.Argument{
-					commands.StringArg("a", true, false, "some arg").EnableStdin(),
+					commands.StringArg("a", true, false, "some arg"),
+					commands.FileArg("a", true, true, "some arg"),
 				},
 			},
-			"stdinenablednotvariadic2args": &commands.Command{
+			"StringArg+FileArg+Variadic+Stdin": {
 				Arguments: []commands.Argument{
 					commands.StringArg("a", true, false, "some arg"),
-					commands.StringArg("b", true, false, "another arg").EnableStdin(),
+					commands.FileArg("a", true, true, "some arg"),
 				},
 			},
 		},
 	}
 
-	test := func(cmd words, f *os.File, res words) {
+	test := func(cmd words, f *os.File, exp words) {
 		if f != nil {
 			if _, err := f.Seek(0, os.SEEK_SET); err != nil {
 				t.Fatal(err)
@@ -212,8 +229,18 @@ func TestArgumentParsing(t *testing.T) {
 		if err != nil {
 			t.Errorf("Command '%v' should have passed parsing: %v", cmd, err)
 		}
-		if !sameWords(req.Arguments(), res) {
-			t.Errorf("Arguments parsed from '%v' are '%v' instead of '%v'", cmd, req.Arguments(), res)
+
+		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)
 		}
 	}
 
@@ -253,59 +280,43 @@ 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")
 
-	// Use a temp file to simulate stdin
-	fileToSimulateStdin := func(t *testing.T, content string) *os.File {
-		fstdin, err := ioutil.TempFile("", "")
+	// 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)
 		if err != nil {
 			t.Fatal(err)
 		}
-		defer os.Remove(fstdin.Name())
 
-		if _, err := io.WriteString(fstdin, content); err != nil {
-			t.Fatal(err)
-		}
-		return fstdin
+		return f
 	}
-
-	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"})
+	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()})
 }

core/commands/bitswap.go

@@ -31,7 +31,7 @@ var unwantCmd = &cmds.Command{
 		Tagline: "Remove a given block from your wantlist.",
 	},
 	Arguments: []cmds.Argument{
-		cmds.StringArg("key", true, true, "Key(s) to remove from your wantlist.").EnableStdin(),
+		cmds.StringArg("key", true, true, "Key(s) to remove from your wantlist."),
 	},
 	Run: func(req cmds.Request, res cmds.Response) {
 		nd, err := req.InvocContext().GetNode()

core/commands/block.go

@@ -55,7 +55,7 @@ on raw ipfs blocks. It outputs the following to stdout:
 	},
 
 	Arguments: []cmds.Argument{
-		cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get.").EnableStdin(),
+		cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get."),
 	},
 	Run: func(req cmds.Request, res cmds.Response) {
 		b, err := getBlockForKey(req, req.Arguments()[0])
@@ -88,7 +88,7 @@ It outputs to stdout, and <key> is a base58 encoded multihash.
 	},
 
 	Arguments: []cmds.Argument{
-		cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get.").EnableStdin(),
+		cmds.StringArg("key", true, false, "The base58 multihash of an existing block to get."),
 	},
 	Run: func(req cmds.Request, res cmds.Response) {
 		b, err := getBlockForKey(req, req.Arguments()[0])

core/commands/bootstrap.go

@@ -47,7 +47,7 @@ in the bootstrap list).
 	},
 
 	Arguments: []cmds.Argument{
-		cmds.StringArg("peer", false, true, peerOptionDesc).EnableStdin(),
+		cmds.StringArg("peer", false, true, peerOptionDesc),
 	},
 
 	Options: []cmds.Option{
@@ -129,7 +129,7 @@ var bootstrapRemoveCmd = &cmds.Command{
 	},
 
 	Arguments: []cmds.Argument{
-		cmds.StringArg("peer", false, true, peerOptionDesc).EnableStdin(),
+		cmds.StringArg("peer", false, true, peerOptionDesc),
 	},
 	Options: []cmds.Option{
 		cmds.BoolOption("all", "Remove all bootstrap peers.").Default(false),

core/commands/cat.go

@@ -20,7 +20,7 @@ var CatCmd = &cmds.Command{
 	},
 
 	Arguments: []cmds.Argument{
-		cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to be outputted.").EnableStdin(),
+		cmds.StringArg("ipfs-path", true, true, "The path to the IPFS object(s) to be outputted."),
 	},
 	Run: func(req cmds.Request, res cmds.Response) {
 		node, err := req.InvocContext().GetNode()

core/commands/dht.go

@@ -459,7 +459,7 @@ NOTE: A value may not exceed 2048 bytes.
 
 	Arguments: []cmds.Argument{
 		cmds.StringArg("key", true, false, "The key to store the value at."),
-		cmds.StringArg("value", true, false, "The value to store.").EnableStdin(),
+		cmds.StringArg("value", true, false, "The value to store."),
 	},
 	Options: []cmds.Option{
 		cmds.BoolOption("verbose", "v", "Print extra information.").Default(false),

core/commands/dns.go

@@ -44,7 +44,7 @@ The resolver can recursively resolve:
 	},
 
 	Arguments: []cmds.Argument{
-		cmds.StringArg("domain-name", true, false, "The domain-name name to resolve.").EnableStdin(),
+		cmds.StringArg("domain-name", true, false, "The domain-name name to resolve."),
 	},
 	Options: []cmds.Option{
 		cmds.BoolOption("recursive", "r", "Resolve until the result is not a DNS link.").Default(false),