Merge pull request #872 from urfave/pass-through-regression

Fix 1.21.0 pass through regression

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