short opt handling: fix parsing

Only split a given string (e.g., "-abc") into short options (e.g., "-a",
"-b", "-c") if all those are flags.  To further avoid mistakenly
transform common arguments, catch "flag provided but not defined" errors
to iteratively transform short options.

Signed-off-by: Valentin Rothberg <vrothberg@suse.com>
Fixes: https://github.com/projectatomic/libpod/issues/714

command.go

@@ -181,16 +181,49 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
 		return set, set.Parse(append([]string{"--"}, args...))
 	}
 
-	if c.UseShortOptionHandling {
-		args = translateShortOptions(args)
-	}
-
 	if !c.SkipArgReorder {
 		args = reorderArgs(args)
 	}
 
+PARSE:
 	err = set.Parse(args)
 	if err != nil {
+		if c.UseShortOptionHandling {
+			// To enable short-option handling (e.g., "-it" vs "-i -t")
+			// we have to 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.
+			errStr := err.Error()
+			trimmed := strings.TrimPrefix(errStr, "flag provided but not defined: ")
+			if errStr == trimmed {
+				return nil, err
+			}
+			// regenerate the initial args with the split short opts
+			newArgs := Args{}
+			for i, arg := range args {
+				if arg != trimmed {
+					newArgs = append(newArgs, arg)
+					continue
+				}
+				shortOpts := translateShortOptions(set, Args{trimmed})
+				if len(shortOpts) == 1 {
+					return nil, err
+				}
+				// add each short option and all remaining arguments
+				newArgs = append(newArgs, shortOpts...)
+				newArgs = append(newArgs, args[i+1:]...)
+				args = newArgs
+				// now reset the flagset parse again
+				set, err = flagSet(c.Name, c.Flags)
+				if err != nil {
+					return nil, err
+				}
+				set.SetOutput(ioutil.Discard)
+				goto PARSE
+			}
+		}
 		return nil, err
 	}
 
@@ -232,11 +265,25 @@ func reorderArgs(args []string) []string {
 	return append(flags, nonflags...)
 }
 
-func translateShortOptions(flagArgs Args) []string {
+func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string {
+	allCharsFlags := func (s string) bool {
+		for i := range s {
+			f := set.Lookup(string(s[i]))
+			if f == nil {
+				return false
+			}
+		}
+		return true
+	}
+
 	// separate combined flags
 	var flagArgsSeparated []string
 	for _, flagArg := range flagArgs {
 		if strings.HasPrefix(flagArg, "-") && strings.HasPrefix(flagArg, "--") == false && len(flagArg) > 2 {
+			if !allCharsFlags(flagArg[1:]) {
+				flagArgsSeparated = append(flagArgsSeparated, flagArg)
+				continue
+			}
 			for _, flagChar := range flagArg[1:] {
 				flagArgsSeparated = append(flagArgsSeparated, "-"+string(flagChar))
 			}

command_test.go

@@ -57,6 +57,52 @@ func TestCommandFlagParsing(t *testing.T) {
 	}
 }
 
+func TestParseAndRunShortOpts(t *testing.T) {
+	cases := []struct {
+		testArgs               []string
+		expectedErr            error
+		expectedArgs           []string
+	}{
+		{[]string{"foo", "test", "-a"}, nil, []string{}},
+		{[]string{"foo", "test", "-c", "arg1", "arg2"}, nil, []string{"arg1", "arg2"}},
+		{[]string{"foo", "test", "-f"}, nil, []string{}},
+		{[]string{"foo", "test", "-ac", "--fgh"}, nil, []string{}},
+		{[]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", "-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"},
+		},
+	}
+
+	for _, c := range cases {
+		app := NewApp()
+		app.Commands = []Command{cmd}
+
+		err := app.Run(c.testArgs)
+
+		expect(t, err, c.expectedErr)
+		expect(t, args, c.expectedArgs)
+	}
+}
+
 func TestCommand_Run_DoesNotOverwriteErrorFromBefore(t *testing.T) {
 	app := NewApp()
 	app.Commands = []Command{