Merge pull request #1139 from AtnNn/flags

Improve command line parsing

cmd/ipfs/main.go

@@ -93,6 +93,12 @@ func main() {
 		fmt.Fprintf(w, "Use 'ipfs %s --help' for information about this command\n", cmdPath)
 	}
 
+	// Handle `ipfs help'
+	if len(os.Args) == 2 && os.Args[1] == "help" {
+		printHelp(false, os.Stdout)
+		os.Exit(0)
+	}
+
 	// parse the commandline into a command invocation
 	parseErr := invoc.Parse(ctx, os.Args[1:])
 
@@ -110,13 +116,6 @@ func main() {
 		}
 	}
 
-	// here we handle the cases where
-	// - commands with no Run func are invoked directly.
-	// - the main command is invoked.
-	if invoc.cmd == nil || invoc.cmd.Run == nil {
-		printHelp(false, os.Stdout)
-		os.Exit(0)
-	}
 
 	// ok now handle parse error (which means cli input was wrong,
 	// e.g. incorrect number of args, or nonexistent subcommand)
@@ -132,6 +131,14 @@ func main() {
 		os.Exit(1)
 	}
 
+	// here we handle the cases where
+	// - commands with no Run func are invoked directly.
+	// - the main command is invoked.
+	if invoc.cmd == nil || invoc.cmd.Run == nil {
+		printHelp(false, os.Stdout)
+		os.Exit(0)
+	}
+
 	// ok, finally, run the command invocation.
 	intrh, ctx := invoc.SetupInterruptHandler(ctx)
 	defer intrh.Close()

commands/cli/helptext.go

@@ -14,7 +14,8 @@ const (
 	requiredArg = "<%v>"
 	optionalArg = "[<%v>]"
 	variadicArg = "%v..."
-	optionFlag  = "-%v"
+	shortFlag  = "-%v"
+	longFlag  = "--%v"
 	optionType  = "(%v)"
 
 	whitespace = "\r\n\t "
@@ -219,6 +220,14 @@ func argumentText(cmd *cmds.Command) []string {
 	return lines
 }
 
+func optionFlag(flag string) string {
+	if len(flag) == 1 {
+		return fmt.Sprintf(shortFlag, flag)
+	} else {
+		return fmt.Sprintf(longFlag, flag)
+	}
+}
+
 func optionText(cmd ...*cmds.Command) []string {
 	// get a slice of the options we want to list out
 	options := make([]cmds.Option, 0)
@@ -241,7 +250,7 @@ func optionText(cmd ...*cmds.Command) []string {
 
 			names := sortByLength(opt.Names())
 			if len(names) >= j+1 {
-				lines[i] += fmt.Sprintf(optionFlag, names[j])
+				lines[i] += optionFlag(names[j])
 			}
 			if len(names) > j+1 {
 				lines[i] += ", "

commands/cli/parse.go

@@ -2,7 +2,6 @@ package cli
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"os"
 	"runtime"
@@ -13,20 +12,12 @@ import (
 	u "github.com/ipfs/go-ipfs/util"
 )
 
-// ErrInvalidSubcmd signals when the parse error is not found
-var ErrInvalidSubcmd = errors.New("subcommand not found")
-
 // 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) {
-	path, input, cmd := parsePath(input, root)
-	if len(path) == 0 {
-		return nil, nil, path, ErrInvalidSubcmd
-	}
-
-	opts, stringVals, err := parseOptions(input)
+	path, opts, stringVals, cmd, err := parseOpts(input, root)
 	if err != nil {
-		return nil, cmd, path, err
+		return nil, nil, path, err
 	}
 
 	optDefs, err := root.GetOptions(path)
@@ -34,14 +25,6 @@ func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *c
 		return nil, cmd, path, err
 	}
 
-	// check to make sure there aren't any undefined options
-	for k := range opts {
-		if _, found := optDefs[k]; !found {
-			err = fmt.Errorf("Unrecognized option: -%s", k)
-			return nil, cmd, path, err
-		}
-	}
-
 	req, err := cmds.NewRequest(path, opts, nil, nil, cmd, optDefs)
 	if err != nil {
 		return nil, cmd, path, err
@@ -75,67 +58,142 @@ func Parse(input []string, stdin *os.File, root *cmds.Command) (cmds.Request, *c
 	return req, cmd, path, nil
 }
 
-// parsePath separates the command path and the opts and args from a command string
-// returns command path slice, rest slice, and the corresponding *cmd.Command
-func parsePath(input []string, root *cmds.Command) ([]string, []string, *cmds.Command) {
-	cmd := root
-	path := make([]string, 0, len(input))
-	input2 := make([]string, 0, len(input))
+// Parse a command line made up of sub-commands, short arguments, long arguments and positional arguments
+func parseOpts(args []string, root *cmds.Command) (
+	path []string,
+	opts map[string]interface{},
+	stringVals []string,
+	cmd *cmds.Command,
+	err error,
+) {
+	path = make([]string, 0, len(args))
+	stringVals = make([]string, 0, len(args))
+	optDefs := map[string]cmds.Option{}
+	opts = map[string]interface{}{}
+	cmd = root
+
+	// parseFlag checks that a flag is valid and saves it into opts
+	// Returns true if the optional second argument is used
+	parseFlag := func(name string, arg *string, mustUse bool) (bool, error) {
+		if _, ok := opts[name]; ok {
+			return false, fmt.Errorf("Duplicate values for option '%s'", name)
+		}
 
-	for i, blob := range input {
-		if strings.HasPrefix(blob, "-") {
-			input2 = append(input2, blob)
-			continue
+		optDef, found := optDefs[name]
+		if !found {
+			err = fmt.Errorf("Unrecognized option '%s'", name)
+			return false, err
 		}
 
-		sub := cmd.Subcommand(blob)
-		if sub == nil {
-			input2 = append(input2, input[i:]...)
-			break
+		if optDef.Type() == cmds.Bool {
+			if mustUse {
+				return false, fmt.Errorf("Option '%s' takes no arguments, but was passed '%s'", name, *arg)
+			}
+			opts[name] = ""
+			return false, nil
+		} else {
+			if arg == nil {
+				return true, fmt.Errorf("Missing argument for option '%s'", name)
+			}
+			opts[name] = *arg
+			return true, nil
 		}
-		cmd = sub
-		path = append(path, blob)
 	}
 
-	return path, input2, cmd
-}
-
-// parseOptions parses the raw string values of the given options
-// returns the parsed options as strings, along with the CLI args
-func parseOptions(input []string) (map[string]interface{}, []string, error) {
-	opts := make(map[string]interface{})
-	args := []string{}
-
-	for i := 0; i < len(input); i++ {
-		blob := input[i]
+	optDefs, err = root.GetOptions(path)
+	if err != nil {
+		return
+	}
 
-		if strings.HasPrefix(blob, "-") {
-			name := blob[1:]
-			value := ""
+	consumed := false
+	for i, arg := range args {
+		switch {
+		case consumed:
+			// arg was already consumed by the preceding flag
+			consumed = false
+			continue
 
-			// support single and double dash
-			if strings.HasPrefix(name, "-") {
-				name = name[1:]
+		case arg == "--":
+			// treat all remaining arguments as positional arguments
+			stringVals = append(stringVals, args[i+1:]...)
+			return
+
+		case strings.HasPrefix(arg, "--"):
+			// arg is a long flag, with an optional argument specified
+			// using `=' or in args[i+1]
+			var slurped bool
+			var next *string
+			split := strings.SplitN(arg, "=", 2)
+			if len(split) == 2 {
+				slurped = false
+				arg = split[0]
+				next = &split[1]
+			} else {
+				slurped = true
+				if i+1 < len(args) {
+					next = &args[i+1]
+				} else {
+					next = nil
+				}
 			}
-
-			if strings.Contains(name, "=") {
-				split := strings.SplitN(name, "=", 2)
-				name = split[0]
-				value = split[1]
+			consumed, err = parseFlag(arg[2:], next, len(split) == 2)
+			if err != nil {
+				return
 			}
-
-			if _, ok := opts[name]; ok {
-				return nil, nil, fmt.Errorf("Duplicate values for option '%s'", name)
+			if !slurped {
+				consumed = false
 			}
 
-			opts[name] = value
+		case strings.HasPrefix(arg, "-") && arg != "-":
+			// args is one or more flags in short form, followed by an optional argument
+			// all flags except the last one have type bool
+			for arg = arg[1:]; len(arg) != 0; arg = arg[1:] {
+				var rest *string
+				var slurped bool
+				mustUse := false
+				if len(arg) > 1 {
+					slurped = false
+					str := arg[1:]
+					if len(str) > 0 && str[0] == '=' {
+						str = str[1:]
+						mustUse = true
+					}
+					rest = &str
+				} else {
+					slurped = true
+					if i+1 < len(args) {
+						rest = &args[i+1]
+					} else {
+						rest = nil
+					}
+				}
+				var end bool
+				end, err = parseFlag(arg[0:1], rest, mustUse)
+				if err != nil {
+					return
+				}
+				if end {
+					consumed = slurped
+					break
+				}
+			}
 
-		} else {
-			args = append(args, blob)
+		default:
+			// arg is a sub-command or a positional argument
+			sub := cmd.Subcommand(arg)
+			if sub != nil {
+				cmd = sub
+				path = append(path, arg)
+				optDefs, err = root.GetOptions(path)
+				if err != nil {
+					return
+				}
+			} else {
+				stringVals = append(stringVals, arg)
+			}
 		}
 	}
-
-	return opts, args, nil
+	return
 }
 
 func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursive bool) ([]string, []files.File, error) {
@@ -171,7 +229,7 @@ func parseArgs(inputs []string, stdin *os.File, argDefs []cmds.Argument, recursi
 	// 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", len(argDefs), numInputs)
+		return nil, nil, fmt.Errorf("Expected %v arguments, got %v: %v", len(argDefs), numInputs, inputs)
 	}
 
 	stringArgs := make([]string, 0, numInputs)

commands/cli/parse_test.go

 package cli
 
 import (
-	//"fmt"
+	"strings"
 	"testing"
 
 	"github.com/ipfs/go-ipfs/commands"
@@ -11,43 +11,79 @@ func TestOptionParsing(t *testing.T) {
 	subCmd := &commands.Command{}
 	cmd := &commands.Command{
 		Options: []commands.Option{
-			commands.StringOption("b", "some option"),
+			commands.StringOption("string", "s", "a string"),
+			commands.BoolOption("bool", "b", "a bool"),
 		},
 		Subcommands: map[string]*commands.Command{
 			"test": subCmd,
 		},
 	}
 
-	opts, input, err := parseOptions([]string{"--beep", "-boop=lol", "test2", "-c", "beep", "--foo=5"})
-	/*for k, v := range opts {
-	    fmt.Printf("%s: %s\n", k, v)
-	  }
-	  fmt.Printf("%s\n", input)*/
-	if err != nil {
-		t.Error("Should have passed")
-	}
-	if len(opts) != 4 || opts["beep"] != "" || opts["boop"] != "lol" || opts["c"] != "" || opts["foo"] != "5" {
-		t.Errorf("Returned options were defferent than expected: %v", opts)
-	}
-	if len(input) != 2 || input[0] != "test2" || input[1] != "beep" {
-		t.Errorf("Returned input was different than expected: %v", input)
+	type kvs map[string]interface{}
+	type words []string
+
+	sameWords := func(a words, b words) bool {
+		for i, w := range a {
+			if w != b[i] {
+				return false
+			}
+		}
+		return true
 	}
 
-	_, _, err = parseOptions([]string{"-beep=1", "-boop=2", "-beep=3"})
-	if err == nil {
-		t.Error("Should have failed (duplicate option name)")
+	sameKVs := func(a kvs, b kvs) bool {
+		if len(a) != len(b) {
+			return false
+		}
+		for k, v := range a {
+			if v != b[k] {
+				return false
+			}
+		}
+		return true
 	}
 
-	path, args, sub := parsePath([]string{"test", "beep", "boop"}, cmd)
-	if len(path) != 1 || path[0] != "test" {
-		t.Errorf("Returned path was defferent than expected: %v", path)
+	testHelper := func(args string, expectedOpts kvs, expectedWords words, expectErr bool) {
+		_, opts, input, _, err := parseOpts(strings.Split(args, " "), cmd)
+		if expectErr {
+			if err == nil {
+				t.Errorf("Command line '%v' parsing should have failed", args)
+			}
+		} else if err != nil {
+			t.Errorf("Command line '%v' failed to parse: %v", args, err)
+		} else if !sameWords(input, expectedWords) || !sameKVs(opts, expectedOpts) {
+			t.Errorf("Command line '%v':\n  parsed as  %v %v\n  instead of %v %v",
+				args, opts, input, expectedOpts, expectedWords)
+		}
 	}
-	if len(args) != 2 || args[0] != "beep" || args[1] != "boop" {
-		t.Errorf("Returned args were different than expected: %v", args)
+
+	testFail := func(args string) {
+		testHelper(args, kvs{}, words{}, true)
 	}
-	if sub != subCmd {
-		t.Errorf("Returned command was different than expected")
+
+	test := func(args string, expectedOpts kvs, expectedWords words) {
+		testHelper(args, expectedOpts, expectedWords, false)
 	}
+
+	test("-", kvs{}, words{"-"})
+	testFail("-b -b")
+	test("beep boop", kvs{}, words{"beep", "boop"})
+	test("test beep boop", kvs{}, words{"beep", "boop"})
+	testFail("-s")
+	test("-s foo", kvs{"s": "foo"}, words{})
+	test("-sfoo", kvs{"s": "foo"}, words{})
+	test("-s=foo", kvs{"s": "foo"}, words{})
+	test("-b", kvs{"b": ""}, words{})
+	test("-bs foo", kvs{"b": "", "s": "foo"}, words{})
+	test("-sb", kvs{"s": "b"}, words{})
+	test("-b foo", kvs{"b": ""}, words{"foo"})
+	test("--bool foo", kvs{"bool": ""}, words{"foo"})
+	testFail("--bool=foo")
+	testFail("--string")
+	test("--string foo", kvs{"string": "foo"}, words{})
+	test("--string=foo", kvs{"string": "foo"}, words{})
+	test("-- -b", kvs{}, words{"-b"})
+	test("foo -b", kvs{"b": ""}, words{"foo"})
 }
 
 func TestArgumentParsing(t *testing.T) {

repo/fsrepo/fsrepo.go

@@ -40,7 +40,7 @@ Please run the ipfs migration tool before continuing.
 ` + migrationInstructions
 
 var (
-	ErrNoRepo    = errors.New("no ipfs repo found. please run: ipfs init")
+	ErrNoRepo    = func (path string) error { return fmt.Errorf("no ipfs repo found in '%s'. please run: ipfs init ", path) }
 	ErrNoVersion = errors.New("no version file found, please run 0-to-1 migration tool.\n" + migrationInstructions)
 	ErrOldRepo   = errors.New("ipfs repo found in old '~/.go-ipfs' location, please run migration tool.\n" + migrationInstructions)
 )
@@ -172,7 +172,7 @@ func checkInitialized(path string) error {
 		if isInitializedUnsynced(alt) {
 			return ErrOldRepo
 		}
-		return ErrNoRepo
+		return ErrNoRepo(path)
 	}
 	return nil
 }

test/sharness/lib/test-lib.sh

@@ -105,7 +105,7 @@ test_wait_open_tcp_port_10_sec() {
 # was setting really weird things and am not sure why.
 test_config_set() {
 
-	# grab flags (like -bool in "ipfs config -bool")
+	# grab flags (like --bool in "ipfs config --bool")
 	test_cfg_flags="" # unset in case.
 	test "$#" = 3 && { test_cfg_flags=$1; shift; }
 
@@ -184,7 +184,7 @@ test_config_ipfs_gateway_writable() {
 	test_config_ipfs_gateway_readonly $1
 
 	test_expect_success "prepare config -- gateway writable" '
-		test_config_set -bool Gateway.Writable true ||
+		test_config_set --bool Gateway.Writable true ||
 		test_fsh cat "\"$IPFS_PATH/config\""
 	'
 }

test/sharness/t0021-config.sh

@@ -7,7 +7,7 @@ test_description="Test config command"
 # we use a function so that we can run it both offline + online
 test_config_cmd_set() {
 
-  # flags (like -bool in "ipfs config -bool")
+  # flags (like --bool in "ipfs config --bool")
   cfg_flags="" # unset in case.
   test "$#" = 3 && { cfg_flags=$1; shift; }
 
@@ -41,8 +41,8 @@ test_config_cmd() {
   test_config_cmd_set "beep" "boop"
   test_config_cmd_set "beep1" "boop2"
   test_config_cmd_set "beep1" "boop2"
-  test_config_cmd_set "-bool" "beep2" "true"
-  test_config_cmd_set "-bool" "beep2" "false"
+  test_config_cmd_set "--bool" "beep2" "true"
+  test_config_cmd_set "--bool" "beep2" "false"
 
 }
 

test/sharness/t0080-repo.sh

@@ -17,7 +17,7 @@ test_expect_success "'ipfs add afile' succeeds" '
 '
 
 test_expect_success "added file was pinned" '
-	ipfs pin ls -type=recursive >actual &&
+	ipfs pin ls --type=recursive >actual &&
 	grep "$HASH" actual
 '
 
@@ -49,7 +49,7 @@ test_expect_success "file no longer pinned" '
 	echo "$HASH_WELCOME_DOCS" >expected2 &&
 	ipfs refs -r "$HASH_WELCOME_DOCS" >>expected2 &&
 	echo QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn >> expected2 &&
-	ipfs pin ls -type=recursive >actual2 &&
+	ipfs pin ls --type=recursive >actual2 &&
 	test_sort_cmp expected2 actual2
 '
 
@@ -102,10 +102,10 @@ test_expect_success "adding multiblock random file succeeds" '
 	MBLOCKHASH=`ipfs add -q multiblock`
 '
 
-test_expect_success "'ipfs pin ls -type=indirect' is correct" '
+test_expect_success "'ipfs pin ls --type=indirect' is correct" '
 	ipfs refs "$MBLOCKHASH" >refsout &&
 	ipfs refs -r "$HASH_WELCOME_DOCS" >>refsout &&
-	ipfs pin ls -type=indirect >indirectpins &&
+	ipfs pin ls --type=indirect >indirectpins &&
 	test_sort_cmp refsout indirectpins
 '
 
@@ -121,27 +121,27 @@ test_expect_success "pin something directly" '
 	test_cmp expected10 actual10
 '
 
-test_expect_success "'ipfs pin ls -type=direct' is correct" '
+test_expect_success "'ipfs pin ls --type=direct' is correct" '
 	echo "$DIRECTPIN" >directpinexpected &&
-	ipfs pin ls -type=direct >directpinout &&
+	ipfs pin ls --type=direct >directpinout &&
 	test_sort_cmp directpinexpected directpinout
 '
 
-test_expect_success "'ipfs pin ls -type=recursive' is correct" '
+test_expect_success "'ipfs pin ls --type=recursive' is correct" '
 	echo "$MBLOCKHASH" >rp_expected &&
 	echo "$HASH_WELCOME_DOCS" >>rp_expected &&
 	echo QmUNLLsPACCz1vLxQVkXqqLX5R1X345qqfHbsf67hvA3Nn >>rp_expected &&
 	ipfs refs -r "$HASH_WELCOME_DOCS" >>rp_expected &&
-	ipfs pin ls -type=recursive >rp_actual &&
+	ipfs pin ls --type=recursive >rp_actual &&
 	test_sort_cmp rp_expected rp_actual
 '
 
-test_expect_success "'ipfs pin ls -type=all' is correct" '
+test_expect_success "'ipfs pin ls --type=all' is correct" '
 	cat directpinout >allpins &&
 	cat rp_actual >>allpins &&
 	cat indirectpins >>allpins &&
 	cat allpins | sort | uniq >> allpins_uniq &&
-	ipfs pin ls -type=all >actual_allpins &&
+	ipfs pin ls --type=all >actual_allpins &&
 	test_sort_cmp allpins_uniq actual_allpins
 '
 

test/sharness/t0081-repo-pinning.sh

@@ -143,7 +143,7 @@ test_expect_success "added dir was NOT pinned indirectly" '
 '
 
 test_expect_success "nothing is pinned directly" '
-	ipfs pin ls -type=direct >actual4 &&
+	ipfs pin ls --type=direct >actual4 &&
 	test_must_be_empty actual4
 '
 

test/sharness/t0100-name.sh

@@ -13,7 +13,7 @@ test_init_ipfs
 # test publishing a hash
 
 test_expect_success "'ipfs name publish' succeeds" '
-	PEERID=`ipfs id -format="<id>"` &&
+	PEERID=`ipfs id --format="<id>"` &&
 	ipfs name publish "$HASH_WELCOME_DOCS" >publish_out
 '
 
@@ -34,7 +34,7 @@ test_expect_success "resolve output looks good" '
 # now test with a path
 
 test_expect_success "'ipfs name publish' succeeds" '
-	PEERID=`ipfs id -format="<id>"` &&
+	PEERID=`ipfs id --format="<id>"` &&
 	ipfs name publish "/ipfs/$HASH_WELCOME_DOCS/help" >publish_out
 '