From ceaac7c9152121e6ba0f3b492b3254d61346f92a Mon Sep 17 00:00:00 2001
From: baude <bbaude@redhat.com>
Date: Mon, 20 Nov 2017 09:32:03 -0600
Subject: [PATCH] Handle ShortOptions and SkipArgReorder

There was a bug in parsing when both ShortOptions
and SkipArgReorder were being used together.

Signed-off-by: baude <bbaude@redhat.com>
---
 command.go      | 129 ++++++++++++++++++++++++++++++++----------------
 command_test.go |   1 +
 2 files changed, 87 insertions(+), 43 deletions(-)

diff --git a/command.go b/command.go
index b559811..7d0357b 100644
--- a/command.go
+++ b/command.go
@@ -115,57 +115,29 @@ func (c Command) Run(ctx *Context) (err error) {
 		return err
 	}
 	set.SetOutput(ioutil.Discard)
-
+	firstFlagIndex, terminatorIndex := getIndexes(ctx)
+	flagArgs, regularArgs := getAllArgs(ctx.Args(), firstFlagIndex, terminatorIndex)
+	if c.UseShortOptionHandling {
+		flagArgs = translateShortOptions(flagArgs)
+	}
 	if c.SkipFlagParsing {
 		err = set.Parse(append([]string{"--"}, ctx.Args().Tail()...))
 	} else if !c.SkipArgReorder {
-		firstFlagIndex := -1
-		terminatorIndex := -1
-		for index, arg := range ctx.Args() {
-			if arg == "--" {
-				terminatorIndex = index
-				break
-			} else if arg == "-" {
-				// Do nothing. A dash alone is not really a flag.
-				continue
-			} else if strings.HasPrefix(arg, "-") && firstFlagIndex == -1 {
-				firstFlagIndex = index
-			}
-		}
-
 		if firstFlagIndex > -1 {
-			args := ctx.Args()
-			regularArgs := make([]string, len(args[1:firstFlagIndex]))
-			copy(regularArgs, args[1:firstFlagIndex])
-
-			var flagArgs []string
-			if terminatorIndex > -1 {
-				flagArgs = args[firstFlagIndex:terminatorIndex]
-				regularArgs = append(regularArgs, args[terminatorIndex:]...)
-			} else {
-				flagArgs = args[firstFlagIndex:]
-			}
-			// separate combined flags
-			if c.UseShortOptionHandling {
-				var flagArgsSeparated []string
-				for _, flagArg := range flagArgs {
-					if strings.HasPrefix(flagArg, "-") && strings.HasPrefix(flagArg, "--") == false && len(flagArg) > 2 {
-						for _, flagChar := range flagArg[1:] {
-							flagArgsSeparated = append(flagArgsSeparated, "-"+string(flagChar))
-						}
-					} else {
-						flagArgsSeparated = append(flagArgsSeparated, flagArg)
-					}
-				}
-				err = set.Parse(append(flagArgsSeparated, regularArgs...))
-			} else {
-				err = set.Parse(append(flagArgs, regularArgs...))
-			}
+			err = set.Parse(append(flagArgs, regularArgs...))
 		} else {
 			err = set.Parse(ctx.Args().Tail())
 		}
+	} else if c.UseShortOptionHandling {
+		if terminatorIndex == -1 && firstFlagIndex > -1 {
+			// Handle shortname AND no options
+			err = set.Parse(append(regularArgs, flagArgs...))
+		} else {
+			// Handle shortname and options
+			err = set.Parse(flagArgs)
+		}
 	} else {
-		err = set.Parse(ctx.Args().Tail())
+		err = set.Parse(append(regularArgs, flagArgs...))
 	}
 
 	nerr := normalizeFlags(c.Flags, set)
@@ -233,6 +205,77 @@ func (c Command) Run(ctx *Context) (err error) {
 	return err
 }
 
+func getIndexes(ctx *Context) (int, int) {
+	firstFlagIndex := -1
+	terminatorIndex := -1
+	for index, arg := range ctx.Args() {
+		if arg == "--" {
+			terminatorIndex = index
+			break
+		} else if arg == "-" {
+			// Do nothing. A dash alone is not really a flag.
+			continue
+		} else if strings.HasPrefix(arg, "-") && firstFlagIndex == -1 {
+			firstFlagIndex = index
+		}
+	}
+	if len(ctx.Args()) > 0 && !strings.HasPrefix(ctx.Args()[0], "-") && firstFlagIndex == -1 {
+		return -1, -1
+	}
+
+	return firstFlagIndex, terminatorIndex
+
+}
+
+// copyStringslice takes a string slice and copies it
+func copyStringSlice(slice []string, start, end int) []string {
+	newSlice := make([]string, end-start)
+	copy(newSlice, slice[start:end])
+	return newSlice
+}
+
+// getAllArgs extracts and returns two string slices representing
+// regularArgs and flagArgs
+func getAllArgs(args []string, firstFlagIndex, terminatorIndex int) ([]string, []string) {
+	var regularArgs []string
+	// if there are no options, the we set the index to 1 manually
+	if firstFlagIndex == -1 {
+		firstFlagIndex = 1
+		regularArgs = copyStringSlice(args, 0, len(args))
+	} else {
+		regularArgs = copyStringSlice(args, 1, firstFlagIndex)
+	}
+	var flagArgs []string
+	// a flag terminatorIndex was found in the input. we need to collect
+	// flagArgs based on it.
+	if terminatorIndex > -1 {
+		flagArgs = copyStringSlice(args, firstFlagIndex, terminatorIndex)
+		additionalRegularArgs := copyStringSlice(args, terminatorIndex, len(args))
+		regularArgs = append(regularArgs, additionalRegularArgs...)
+		for _, i := range additionalRegularArgs {
+			regularArgs = append(regularArgs, i)
+		}
+	} else {
+		flagArgs = args[firstFlagIndex:]
+	}
+	return flagArgs, regularArgs
+}
+
+func translateShortOptions(flagArgs Args) []string {
+	// separate combined flags
+	var flagArgsSeparated []string
+	for _, flagArg := range flagArgs {
+		if strings.HasPrefix(flagArg, "-") && strings.HasPrefix(flagArg, "--") == false && len(flagArg) > 2 {
+			for _, flagChar := range flagArg[1:] {
+				flagArgsSeparated = append(flagArgsSeparated, "-"+string(flagChar))
+			}
+		} else {
+			flagArgsSeparated = append(flagArgsSeparated, flagArg)
+		}
+	}
+	return flagArgsSeparated
+}
+
 // Names returns the names including short names and aliases.
 func (c Command) Names() []string {
 	names := []string{c.Name}
diff --git a/command_test.go b/command_test.go
index d9d7094..e69750a 100644
--- a/command_test.go
+++ b/command_test.go
@@ -22,6 +22,7 @@ func TestCommandFlagParsing(t *testing.T) {
 
 		// Test no arg reorder
 		{[]string{"test-cmd", "blah", "blah", "-break"}, false, true, nil, false},
+		{[]string{"test-cmd", "blah", "blah", "-break", "ls", "-l"}, false, true, nil, true},
 
 		{[]string{"test-cmd", "blah", "blah"}, true, false, nil, false},   // Test SkipFlagParsing without any args that look like flags
 		{[]string{"test-cmd", "blah", "-break"}, true, false, nil, false}, // Test SkipFlagParsing with random flag arg