Merge pull request #893 from rliebz/fs-panic

Avoid panic for missing flag value

app.go

@@ -199,12 +199,12 @@ func (a *App) Run(arguments []string) (err error) {
 	// always appends the completion flag at the end of the command
 	shellComplete, arguments := checkShellCompleteFlag(a, arguments)
 
-	_, err = a.newFlagSet()
+	set, err := a.newFlagSet()
 	if err != nil {
 		return err
 	}
 
-	set, err := parseIter(a, arguments[1:])
+	err = parseIter(set, a, arguments[1:])
 	nerr := normalizeFlags(a.Flags, set)
 	context := NewContext(a, set, nil)
 	if nerr != nil {
@@ -322,12 +322,12 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) {
 	}
 	a.Commands = newCmds
 
-	_, err = a.newFlagSet()
+	set, err := a.newFlagSet()
 	if err != nil {
 		return err
 	}
 
-	set, err := parseIter(a, ctx.Args().Tail())
+	err = parseIter(set, a, ctx.Args().Tail())
 	nerr := normalizeFlags(a.Flags, set)
 	context := NewContext(a, set, ctx)
 

app_test.go

@@ -659,6 +659,17 @@ func TestApp_UseShortOptionHandling(t *testing.T) {
 	expect(t, name, expected)
 }
 
+func TestApp_UseShortOptionHandling_missing_value(t *testing.T) {
+	app := NewApp()
+	app.UseShortOptionHandling = true
+	app.Flags = []Flag{
+		StringFlag{Name: "name, n"},
+	}
+
+	err := app.Run([]string{"", "-n"})
+	expect(t, err, errors.New("flag needs an argument: -n"))
+}
+
 func TestApp_UseShortOptionHandlingCommand(t *testing.T) {
 	var one, two bool
 	var name string
@@ -688,6 +699,21 @@ func TestApp_UseShortOptionHandlingCommand(t *testing.T) {
 	expect(t, name, expected)
 }
 
+func TestApp_UseShortOptionHandlingCommand_missing_value(t *testing.T) {
+	app := NewApp()
+	app.UseShortOptionHandling = true
+	command := Command{
+		Name: "cmd",
+		Flags: []Flag{
+			StringFlag{Name: "name, n"},
+		},
+	}
+	app.Commands = []Command{command}
+
+	err := app.Run([]string{"", "cmd", "-n"})
+	expect(t, err, errors.New("flag needs an argument: -n"))
+}
+
 func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) {
 	var one, two bool
 	var name string
@@ -722,6 +748,25 @@ func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) {
 	expect(t, name, expected)
 }
 
+func TestApp_UseShortOptionHandlingSubCommand_missing_value(t *testing.T) {
+	app := NewApp()
+	app.UseShortOptionHandling = true
+	command := Command{
+		Name: "cmd",
+	}
+	subCommand := Command{
+		Name: "sub",
+		Flags: []Flag{
+			StringFlag{Name: "name, n"},
+		},
+	}
+	command.Subcommands = []Command{subCommand}
+	app.Commands = []Command{command}
+
+	err := app.Run([]string{"", "cmd", "sub", "-n"})
+	expect(t, err, errors.New("flag needs an argument: -n"))
+}
+
 func TestApp_Float64Flag(t *testing.T) {
 	var meters float64
 

command.go

@@ -193,7 +193,12 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
 		args = reorderArgs(args)
 	}
 
-	set, err := parseIter(c, args)
+	set, err := c.newFlagSet()
+	if err != nil {
+		return nil, err
+	}
+
+	err = parseIter(set, c, args)
 	if err != nil {
 		return nil, err
 	}

command_test.go

@@ -70,29 +70,34 @@ func TestParseAndRunShortOpts(t *testing.T) {
 		{[]string{"foo", "test", "-af"}, nil, []string{}},
 		{[]string{"foo", "test", "-cf"}, nil, []string{}},
 		{[]string{"foo", "test", "-acf"}, nil, []string{}},
-		{[]string{"foo", "test", "-invalid"}, errors.New("flag provided but not defined: -invalid"), []string{}},
+		{[]string{"foo", "test", "-invalid"}, errors.New("flag provided but not defined: -invalid"), nil},
 		{[]string{"foo", "test", "-acf", "arg1", "-invalid"}, nil, []string{"arg1", "-invalid"}},
-	}
-
-	var args []string
-	cmd := Command{
-		Name:        "test",
-		Usage:       "this is for testing",
-		Description: "testing",
-		Action: func(c *Context) error {
-			args = c.Args()
-			return nil
-		},
-		SkipArgReorder:         true,
-		UseShortOptionHandling: true,
-		Flags: []Flag{
-			BoolFlag{Name: "abc, a"},
-			BoolFlag{Name: "cde, c"},
-			BoolFlag{Name: "fgh, f"},
-		},
+		{[]string{"foo", "test", "-acfi", "not-arg", "arg1", "-invalid"}, nil, []string{"arg1", "-invalid"}},
+		{[]string{"foo", "test", "-i", "ivalue"}, nil, []string{}},
+		{[]string{"foo", "test", "-i", "ivalue", "arg1"}, nil, []string{"arg1"}},
+		{[]string{"foo", "test", "-i"}, errors.New("flag needs an argument: -i"), nil},
 	}
 
 	for _, c := range cases {
+		var args []string
+		cmd := Command{
+			Name:        "test",
+			Usage:       "this is for testing",
+			Description: "testing",
+			Action: func(c *Context) error {
+				args = c.Args()
+				return nil
+			},
+			SkipArgReorder:         true,
+			UseShortOptionHandling: true,
+			Flags: []Flag{
+				BoolFlag{Name: "abc, a"},
+				BoolFlag{Name: "cde, c"},
+				BoolFlag{Name: "fgh, f"},
+				StringFlag{Name: "ijk, i"},
+			},
+		}
+
 		app := NewApp()
 		app.Commands = []Command{cmd}
 

parse.go

@@ -14,22 +14,17 @@ type iterativeParser interface {
 // iteratively catch parsing errors.  This way we achieve LR parsing without
 // transforming any arguments. Otherwise, there is no way we can discriminate
 // combined short options from common arguments that should be left untouched.
-func parseIter(ip iterativeParser, args []string) (*flag.FlagSet, error) {
+func parseIter(set *flag.FlagSet, ip iterativeParser, args []string) error {
 	for {
-		set, err := ip.newFlagSet()
-		if err != nil {
-			return nil, err
-		}
-
-		err = set.Parse(args)
+		err := set.Parse(args)
 		if !ip.useShortOptionHandling() || err == nil {
-			return set, err
+			return err
 		}
 
 		errStr := err.Error()
 		trimmed := strings.TrimPrefix(errStr, "flag provided but not defined: ")
 		if errStr == trimmed {
-			return nil, err
+			return err
 		}
 
 		// regenerate the initial args with the split short opts
@@ -42,14 +37,21 @@ func parseIter(ip iterativeParser, args []string) (*flag.FlagSet, error) {
 
 			shortOpts := splitShortOptions(set, trimmed)
 			if len(shortOpts) == 1 {
-				return nil, err
+				return err
 			}
 
 			// add each short option and all remaining arguments
 			newArgs = append(newArgs, shortOpts...)
 			newArgs = append(newArgs, args[i+1:]...)
 			args = newArgs
 		}
+
+		// Since custom parsing failed, replace the flag set before retrying
+		newSet, err := ip.newFlagSet()
+		if err != nil {
+			return err
+		}
+		*set = *newSet
 	}
 }