diff --git a/app_regression_test.go b/app_regression_test.go new file mode 100644 index 0000000..3c8681b --- /dev/null +++ b/app_regression_test.go @@ -0,0 +1,59 @@ +package cli + +import ( + "testing" +) + +// TestRegression tests a regression that was merged between versions 1.20.0 and 1.21.0 +// The included app.Run line worked in 1.20.0, and then was broken in 1.21.0. +// Relevant PR: https://github.com/urfave/cli/pull/872 +func TestVersionOneTwoOneRegression(t *testing.T) { + testData := []struct { + testCase string + appRunInput []string + skipArgReorder bool + }{ + { + testCase: "with_dash_dash", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, + }, + { + testCase: "with_dash_dash_and_skip_reorder", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, + skipArgReorder: true, + }, + { + testCase: "without_dash_dash", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, + }, + { + testCase: "without_dash_dash_and_skip_reorder", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, + skipArgReorder: true, + }, + } + for _, test := range testData { + t.Run(test.testCase, func(t *testing.T) { + // setup + app := NewApp() + app.Commands = []Command{{ + Name: "command", + SkipArgReorder: test.skipArgReorder, + Flags: []Flag{ + StringFlag{ + Name: "flagone", + }, + }, + Action: func(c *Context) error { return nil }, + }} + + // logic under test + err := app.Run(test.appRunInput) + + // assertions + if err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } + }) + } +} diff --git a/command.go b/command.go index b10e4d4..e7cb97a 100644 --- a/command.go +++ b/command.go @@ -190,7 +190,7 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) { } if !c.SkipArgReorder { - args = reorderArgs(args) + args = reorderArgs(c.Flags, args) } set, err := c.newFlagSet() @@ -219,34 +219,73 @@ func (c *Command) useShortOptionHandling() bool { return c.UseShortOptionHandling } -// reorderArgs moves all flags before arguments as this is what flag expects -func reorderArgs(args []string) []string { - var nonflags, flags []string +// reorderArgs moves all flags (via reorderedArgs) before the rest of +// the arguments (remainingArgs) as this is what flag expects. +func reorderArgs(commandFlags []Flag, args []string) []string { + var remainingArgs, reorderedArgs []string - readFlagValue := false + nextIndexMayContainValue := false for i, arg := range args { + + // dont reorder any args after a -- + // read about -- here: + // https://unix.stackexchange.com/questions/11376/what-does-double-dash-mean-also-known-as-bare-double-dash if arg == "--" { - nonflags = append(nonflags, args[i:]...) + remainingArgs = append(remainingArgs, args[i:]...) break - } - if readFlagValue && !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") { - readFlagValue = false - flags = append(flags, arg) - continue - } - readFlagValue = false + // checks if this arg is a value that should be re-ordered next to its associated flag + } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") { + nextIndexMayContainValue = false + reorderedArgs = append(reorderedArgs, arg) - if arg != "-" && strings.HasPrefix(arg, "-") { - flags = append(flags, arg) + // checks if this is an arg that should be re-ordered + } else if argIsFlag(commandFlags, arg) { + // we have determined that this is a flag that we should re-order + reorderedArgs = append(reorderedArgs, arg) + // if this arg does not contain a "=", then the next index may contain the value for this flag + nextIndexMayContainValue = !strings.Contains(arg, "=") - readFlagValue = !strings.Contains(arg, "=") + // simply append any remaining args } else { - nonflags = append(nonflags, arg) + remainingArgs = append(remainingArgs, arg) } } - return append(flags, nonflags...) + return append(reorderedArgs, remainingArgs...) +} + +// argIsFlag checks if an arg is one of our command flags +func argIsFlag(commandFlags []Flag, arg string) bool { + // checks if this is just a `-`, and so definitely not a flag + if arg == "-" { + return false + } + // flags always start with a - + if !strings.HasPrefix(arg, "-") { + return false + } + // this line turns `--flag` into `flag` + if strings.HasPrefix(arg, "--") { + arg = strings.Replace(arg, "-", "", 2) + } + // this line turns `-flag` into `flag` + if strings.HasPrefix(arg, "-") { + arg = strings.Replace(arg, "-", "", 1) + } + // this line turns `flag=value` into `flag` + arg = strings.Split(arg, "=")[0] + // look through all the flags, to see if the `arg` is one of our flags + for _, flag := range commandFlags { + for _, key := range strings.Split(flag.GetName(), ",") { + key := strings.TrimSpace(key) + if key == arg { + return true + } + } + } + // return false if this arg was not one of our flags + return false } // Names returns the names including short names and aliases. diff --git a/command_test.go b/command_test.go index 6235b41..bcfbee7 100644 --- a/command_test.go +++ b/command_test.go @@ -18,7 +18,7 @@ func TestCommandFlagParsing(t *testing.T) { UseShortOptionHandling bool }{ // Test normal "not ignoring flags" flow - {[]string{"test-cmd", "blah", "blah", "-break"}, false, false, errors.New("flag provided but not defined: -break"), false}, + {[]string{"test-cmd", "blah", "blah", "-break"}, false, false, nil, false}, // Test no arg reorder {[]string{"test-cmd", "blah", "blah", "-break"}, false, true, nil, false}, @@ -70,8 +70,13 @@ 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", "--acf"}, errors.New("flag provided but not defined: -acf"), nil}, {[]string{"foo", "test", "-invalid"}, errors.New("flag provided but not defined: -invalid"), nil}, + {[]string{"foo", "test", "-acf", "-invalid"}, errors.New("flag provided but not defined: -invalid"), nil}, + {[]string{"foo", "test", "--invalid"}, errors.New("flag provided but not defined: -invalid"), nil}, + {[]string{"foo", "test", "-acf", "--invalid"}, errors.New("flag provided but not defined: -invalid"), nil}, {[]string{"foo", "test", "-acf", "arg1", "-invalid"}, nil, []string{"arg1", "-invalid"}}, + {[]string{"foo", "test", "-acf", "arg1", "--invalid"}, nil, []string{"arg1", "--invalid"}}, {[]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"}}, diff --git a/help.go b/help.go index 7a4ef69..2280e33 100644 --- a/help.go +++ b/help.go @@ -47,13 +47,18 @@ type helpPrinter func(w io.Writer, templ string, data interface{}) // Prints help for the App or Command with custom template function. type helpPrinterCustom func(w io.Writer, templ string, data interface{}, customFunc map[string]interface{}) -// HelpPrinter is a function that writes the help output. If not set a default -// is used. The function signature is: -// func(w io.Writer, templ string, data interface{}) +// HelpPrinter is a function that writes the help output. If not set explicitly, +// this calls HelpPrinterCustom using only the default template functions. +// +// If custom logic for printing help is required, this function can be +// overridden. If the ExtraInfo field is defined on an App, this function +// should not be modified, as HelpPrinterCustom will be used directly in order +// to capture the extra information. var HelpPrinter helpPrinter = printHelp -// HelpPrinterCustom is same as HelpPrinter but -// takes a custom function for template function map. +// HelpPrinterCustom is a function that writes the help output. It is used as +// the default implementation of HelpPrinter, and may be called directly if +// the ExtraInfo field is set on an App. var HelpPrinterCustom helpPrinterCustom = printHelpCustom // VersionPrinter prints the version for the App @@ -66,20 +71,24 @@ func ShowAppHelpAndExit(c *Context, exitCode int) { } // ShowAppHelp is an action that displays the help. -func ShowAppHelp(c *Context) (err error) { - if c.App.CustomAppHelpTemplate == "" { - HelpPrinter(c.App.Writer, AppHelpTemplate, c.App) - return +func ShowAppHelp(c *Context) error { + template := c.App.CustomAppHelpTemplate + if template == "" { + template = AppHelpTemplate } + + if c.App.ExtraInfo == nil { + HelpPrinter(c.App.Writer, template, c.App) + return nil + } + customAppData := func() map[string]interface{} { - if c.App.ExtraInfo == nil { - return nil - } return map[string]interface{}{ "ExtraInfo": c.App.ExtraInfo, } } - HelpPrinterCustom(c.App.Writer, c.App.CustomAppHelpTemplate, c.App, customAppData()) + HelpPrinterCustom(c.App.Writer, template, c.App, customAppData()) + return nil } @@ -186,11 +195,13 @@ func ShowCommandHelp(ctx *Context, command string) error { for _, c := range ctx.App.Commands { if c.HasName(command) { - if c.CustomHelpTemplate != "" { - HelpPrinterCustom(ctx.App.Writer, c.CustomHelpTemplate, c, nil) - } else { - HelpPrinter(ctx.App.Writer, CommandHelpTemplate, c) + templ := c.CustomHelpTemplate + if templ == "" { + templ = CommandHelpTemplate } + + HelpPrinter(ctx.App.Writer, templ, c) + return nil } } @@ -238,11 +249,15 @@ func ShowCommandCompletions(ctx *Context, command string) { } -func printHelpCustom(out io.Writer, templ string, data interface{}, customFunc map[string]interface{}) { +// printHelpCustom is the default implementation of HelpPrinterCustom. +// +// The customFuncs map will be combined with a default template.FuncMap to +// allow using arbitrary functions in template rendering. +func printHelpCustom(out io.Writer, templ string, data interface{}, customFuncs map[string]interface{}) { funcMap := template.FuncMap{ "join": strings.Join, } - for key, value := range customFunc { + for key, value := range customFuncs { funcMap[key] = value } @@ -261,7 +276,7 @@ func printHelpCustom(out io.Writer, templ string, data interface{}, customFunc m } func printHelp(out io.Writer, templ string, data interface{}) { - printHelpCustom(out, templ, data, nil) + HelpPrinterCustom(out, templ, data, nil) } func checkVersion(c *Context) bool { diff --git a/help_test.go b/help_test.go index b13eef4..e903494 100644 --- a/help_test.go +++ b/help_test.go @@ -4,6 +4,7 @@ import ( "bytes" "flag" "fmt" + "io" "runtime" "strings" "testing" @@ -210,6 +211,180 @@ func TestShowAppHelp_CommandAliases(t *testing.T) { } } +func TestShowCommandHelp_HelpPrinter(t *testing.T) { + doublecho := func(text string) string { + return text + " " + text + } + + tests := []struct { + name string + template string + printer helpPrinter + command string + wantTemplate string + wantOutput string + }{ + { + name: "no-command", + template: "", + printer: func(w io.Writer, templ string, data interface{}) { + fmt.Fprint(w, "yo") + }, + command: "", + wantTemplate: SubcommandHelpTemplate, + wantOutput: "yo", + }, + { + name: "standard-command", + template: "", + printer: func(w io.Writer, templ string, data interface{}) { + fmt.Fprint(w, "yo") + }, + command: "my-command", + wantTemplate: CommandHelpTemplate, + wantOutput: "yo", + }, + { + name: "custom-template-command", + template: "{{doublecho .Name}}", + printer: func(w io.Writer, templ string, data interface{}) { + // Pass a custom function to ensure it gets used + fm := map[string]interface{}{"doublecho": doublecho} + HelpPrinterCustom(w, templ, data, fm) + }, + command: "my-command", + wantTemplate: "{{doublecho .Name}}", + wantOutput: "my-command my-command", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer func(old helpPrinter) { + HelpPrinter = old + }(HelpPrinter) + HelpPrinter = func(w io.Writer, templ string, data interface{}) { + if templ != tt.wantTemplate { + t.Errorf("want template:\n%s\ngot template:\n%s", tt.wantTemplate, templ) + } + + tt.printer(w, templ, data) + } + + var buf bytes.Buffer + app := &App{ + Name: "my-app", + Writer: &buf, + Commands: []Command{ + { + Name: "my-command", + CustomHelpTemplate: tt.template, + }, + }, + } + + err := app.Run([]string{"my-app", "help", tt.command}) + if err != nil { + t.Fatal(err) + } + + got := buf.String() + if got != tt.wantOutput { + t.Errorf("want output %q, got %q", tt.wantOutput, got) + } + }) + } +} + +func TestShowCommandHelp_HelpPrinterCustom(t *testing.T) { + doublecho := func(text string) string { + return text + " " + text + } + + tests := []struct { + name string + template string + printer helpPrinterCustom + command string + wantTemplate string + wantOutput string + }{ + { + name: "no-command", + template: "", + printer: func(w io.Writer, templ string, data interface{}, fm map[string]interface{}) { + fmt.Fprint(w, "yo") + }, + command: "", + wantTemplate: SubcommandHelpTemplate, + wantOutput: "yo", + }, + { + name: "standard-command", + template: "", + printer: func(w io.Writer, templ string, data interface{}, fm map[string]interface{}) { + fmt.Fprint(w, "yo") + }, + command: "my-command", + wantTemplate: CommandHelpTemplate, + wantOutput: "yo", + }, + { + name: "custom-template-command", + template: "{{doublecho .Name}}", + printer: func(w io.Writer, templ string, data interface{}, _ map[string]interface{}) { + // Pass a custom function to ensure it gets used + fm := map[string]interface{}{"doublecho": doublecho} + printHelpCustom(w, templ, data, fm) + }, + command: "my-command", + wantTemplate: "{{doublecho .Name}}", + wantOutput: "my-command my-command", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer func(old helpPrinterCustom) { + HelpPrinterCustom = old + }(HelpPrinterCustom) + HelpPrinterCustom = func(w io.Writer, templ string, data interface{}, fm map[string]interface{}) { + if fm != nil { + t.Error("unexpected function map passed") + } + + if templ != tt.wantTemplate { + t.Errorf("want template:\n%s\ngot template:\n%s", tt.wantTemplate, templ) + } + + tt.printer(w, templ, data, fm) + } + + var buf bytes.Buffer + app := &App{ + Name: "my-app", + Writer: &buf, + Commands: []Command{ + { + Name: "my-command", + CustomHelpTemplate: tt.template, + }, + }, + } + + err := app.Run([]string{"my-app", "help", tt.command}) + if err != nil { + t.Fatal(err) + } + + got := buf.String() + if got != tt.wantOutput { + t.Errorf("want output %q, got %q", tt.wantOutput, got) + } + }) + } +} + func TestShowCommandHelp_CommandAliases(t *testing.T) { app := &App{ Commands: []Command{ @@ -376,6 +551,144 @@ func TestShowAppHelp_HiddenCommand(t *testing.T) { } } +func TestShowAppHelp_HelpPrinter(t *testing.T) { + doublecho := func(text string) string { + return text + " " + text + } + + tests := []struct { + name string + template string + printer helpPrinter + wantTemplate string + wantOutput string + }{ + { + name: "standard-command", + template: "", + printer: func(w io.Writer, templ string, data interface{}) { + fmt.Fprint(w, "yo") + }, + wantTemplate: AppHelpTemplate, + wantOutput: "yo", + }, + { + name: "custom-template-command", + template: "{{doublecho .Name}}", + printer: func(w io.Writer, templ string, data interface{}) { + // Pass a custom function to ensure it gets used + fm := map[string]interface{}{"doublecho": doublecho} + printHelpCustom(w, templ, data, fm) + }, + wantTemplate: "{{doublecho .Name}}", + wantOutput: "my-app my-app", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer func(old helpPrinter) { + HelpPrinter = old + }(HelpPrinter) + HelpPrinter = func(w io.Writer, templ string, data interface{}) { + if templ != tt.wantTemplate { + t.Errorf("want template:\n%s\ngot template:\n%s", tt.wantTemplate, templ) + } + + tt.printer(w, templ, data) + } + + var buf bytes.Buffer + app := &App{ + Name: "my-app", + Writer: &buf, + CustomAppHelpTemplate: tt.template, + } + + err := app.Run([]string{"my-app", "help"}) + if err != nil { + t.Fatal(err) + } + + got := buf.String() + if got != tt.wantOutput { + t.Errorf("want output %q, got %q", tt.wantOutput, got) + } + }) + } +} + +func TestShowAppHelp_HelpPrinterCustom(t *testing.T) { + doublecho := func(text string) string { + return text + " " + text + } + + tests := []struct { + name string + template string + printer helpPrinterCustom + wantTemplate string + wantOutput string + }{ + { + name: "standard-command", + template: "", + printer: func(w io.Writer, templ string, data interface{}, fm map[string]interface{}) { + fmt.Fprint(w, "yo") + }, + wantTemplate: AppHelpTemplate, + wantOutput: "yo", + }, + { + name: "custom-template-command", + template: "{{doublecho .Name}}", + printer: func(w io.Writer, templ string, data interface{}, _ map[string]interface{}) { + // Pass a custom function to ensure it gets used + fm := map[string]interface{}{"doublecho": doublecho} + printHelpCustom(w, templ, data, fm) + }, + wantTemplate: "{{doublecho .Name}}", + wantOutput: "my-app my-app", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + defer func(old helpPrinterCustom) { + HelpPrinterCustom = old + }(HelpPrinterCustom) + HelpPrinterCustom = func(w io.Writer, templ string, data interface{}, fm map[string]interface{}) { + if fm != nil { + t.Error("unexpected function map passed") + } + + if templ != tt.wantTemplate { + t.Errorf("want template:\n%s\ngot template:\n%s", tt.wantTemplate, templ) + } + + tt.printer(w, templ, data, fm) + } + + var buf bytes.Buffer + app := &App{ + Name: "my-app", + Writer: &buf, + CustomAppHelpTemplate: tt.template, + } + + err := app.Run([]string{"my-app", "help"}) + if err != nil { + t.Fatal(err) + } + + got := buf.String() + if got != tt.wantOutput { + t.Errorf("want output %q, got %q", tt.wantOutput, got) + } + }) + } +} + func TestShowAppHelp_CustomAppTemplate(t *testing.T) { app := &App{ Commands: []Command{ diff --git a/parse.go b/parse.go index 2c2005c..660f538 100644 --- a/parse.go +++ b/parse.go @@ -22,28 +22,35 @@ func parseIter(set *flag.FlagSet, ip iterativeParser, args []string) error { } errStr := err.Error() - trimmed := strings.TrimPrefix(errStr, "flag provided but not defined: ") + trimmed := strings.TrimPrefix(errStr, "flag provided but not defined: -") if errStr == trimmed { return err } // regenerate the initial args with the split short opts - newArgs := []string{} + argsWereSplit := false for i, arg := range args { - if arg != trimmed { - newArgs = append(newArgs, arg) + // skip args that are not part of the error message + if name := strings.TrimLeft(arg, "-"); name != trimmed { continue } - shortOpts := splitShortOptions(set, trimmed) + // if we can't split, the error was accurate + shortOpts := splitShortOptions(set, arg) if len(shortOpts) == 1 { return err } - // add each short option and all remaining arguments - newArgs = append(newArgs, shortOpts...) - newArgs = append(newArgs, args[i+1:]...) - args = newArgs + // swap current argument with the split version + args = append(args[:i], append(shortOpts, args[i+1:]...)...) + argsWereSplit = true + break + } + + // This should be an impossible to reach code path, but in case the arg + // splitting failed to happen, this will prevent infinite loops + if !argsWereSplit { + return err } // Since custom parsing failed, replace the flag set before retrying