From c23dfba7018a4666892af705d89150a5f1ac8293 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 28 Jun 2018 16:41:02 +0200 Subject: [PATCH 1/2] 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 Fixes: https://github.com/projectatomic/libpod/issues/714 --- command.go | 57 ++++++++++++++++++++++++++++++++++++++++++++----- command_test.go | 46 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/command.go b/command.go index 2acb976..56b633c 100644 --- a/command.go +++ b/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)) } diff --git a/command_test.go b/command_test.go index c84b762..9388f6a 100644 --- a/command_test.go +++ b/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{ From 3e5a935ed3cafadcddc6f5ab2fe7ddd2aa0c3cea Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 21 Aug 2018 08:33:42 +0200 Subject: [PATCH 2/2] fix `go vet` warning command_test.go:342:3 value declared but not used Signed-off-by: Valentin Rothberg --- command_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/command_test.go b/command_test.go index 9388f6a..8c2650e 100644 --- a/command_test.go +++ b/command_test.go @@ -339,7 +339,6 @@ func TestCommandSkipFlagParsing(t *testing.T) { } for _, c := range cases { - value := "" args := []string{} app := &App{ Commands: []Command{ @@ -351,7 +350,6 @@ func TestCommandSkipFlagParsing(t *testing.T) { }, Action: func(c *Context) { fmt.Printf("%+v\n", c.String("flag")) - value = c.String("flag") args = c.Args() }, },