Merge pull request #1238 from ipfs/improve-stdin-parsing

Improve stdin parsing

commands/cli/parse.go

@@ -228,8 +228,8 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 	// if we have more arg values provided than argument definitions,
 	// and the last arg definition is not variadic (or there are no definitions), return an error
 	notVariadic := len(argDefs) == 0 || !argDefs[len(argDefs)-1].Variadic
-	if notVariadic && numInputs > len(argDefs) {
-		return nil, nil, fmt.Errorf("Expected %v arguments, got %v: %v", len(argDefs), numInputs, inputs)
+	if notVariadic && len(inputs) > len(argDefs) {
+		return nil, nil, fmt.Errorf("Expected %v arguments, got %v: %v", len(argDefs), len(inputs), inputs)
 	}
 
 	stringArgs := make([]string, 0, numInputs)
@@ -250,29 +250,38 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 
 		var err error
 		if argDef.Type == cmds.ArgString {
-			if stdin == nil {
+			if stdin == nil || !argDef.SupportsStdin {
 				// add string values
 				stringArgs, inputs = appendString(stringArgs, inputs)
 
-			} else if argDef.SupportsStdin {
-				// 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 {
+				if len(inputs) > 0 {
+					// don't use stdin if we have inputs
+					stdin = nil
+				} else {
+					// 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 if argDef.Type == cmds.ArgFile {
-			if stdin == nil {
+			if stdin == nil || !argDef.SupportsStdin {
 				// treat stringArg values as file paths
 				fileArgs, inputs, err = appendFile(fileArgs, inputs, argDef, recursive)
 				if err != nil {
 					return nil, nil, err
 				}
 
-			} else if argDef.SupportsStdin {
-				// if we have a stdin, create a file from it
-				fileArgs, stdin = appendStdinAsFile(fileArgs, stdin)
+			} else {
+				if len(inputs) > 0 {
+					// don't use stdin if we have inputs
+					stdin = nil
+				} else {
+					// if we have a stdin, create a file from it
+					fileArgs, stdin = appendStdinAsFile(fileArgs, stdin)
+				}
 			}
 		}
 

commands/cli/parse_test.go

@@ -155,6 +155,23 @@ func TestArgumentParsing(t *testing.T) {
 					commands.StringArg("a", true, true, "some arg").EnableStdin(),
 				},
 			},
+			"stdinenabled2args": &commands.Command{
+				Arguments: []commands.Argument{
+					commands.StringArg("a", true, false, "some arg"),
+					commands.StringArg("b", true, true, "another arg").EnableStdin(),
+				},
+			},
+			"stdinenablednotvariadic": &commands.Command{
+				Arguments: []commands.Argument{
+					commands.StringArg("a", true, false, "some arg").EnableStdin(),
+				},
+			},
+			"stdinenablednotvariadic2args": &commands.Command{
+				Arguments: []commands.Argument{
+					commands.StringArg("a", true, false, "some arg"),
+					commands.StringArg("b", true, false, "another arg").EnableStdin(),
+				},
+			},
 		},
 	}
 
@@ -166,10 +183,10 @@ func TestArgumentParsing(t *testing.T) {
 		}
 		req, _, _, err := Parse(cmd, f, rootCmd)
 		if err != nil {
-			t.Errorf("Command '%v' should have passed parsing", cmd)
+			t.Errorf("Command '%v' should have passed parsing: %v", cmd, err)
 		}
 		if !sameWords(req.Arguments(), res) {
-			t.Errorf("Arguments parsed from '%v' are not '%v'", cmd, res)
+			t.Errorf("Arguments parsed from '%v' are '%v' instead of '%v'", cmd, req.Arguments(), res)
 		}
 	}
 
@@ -220,8 +237,35 @@ func TestArgumentParsing(t *testing.T) {
 	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{"stdin1", "value1"})
-	test([]string{"stdinenabled", "value1", "value2"}, fstdin, []string{"stdin1", "value1", "value2"})
+	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"})
 }