Merge branch 'master' into versioned-docs

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)
+			}
+		})
+	}
+}

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.

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"}},

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
 	}
-	customAppData := func() map[string]interface{} {
+
 	if c.App.ExtraInfo == nil {
+		HelpPrinter(c.App.Writer, template, c.App)
 		return nil
 	}
+
+	customAppData := func() map[string]interface{} {
 		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 {

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{

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