Fix bug in unmanaged arguments processing†

- Fix a bug that was causing some arguments to be eaten when handling Unmanaged arguments
- Add some tests
- Fix deprecated warnings
- Rename the master branch to main

.github/workflows/ci.yml

@@ -1,7 +1,7 @@
 on:
   push:
     branches:
-      - master
+      - main
   pull_request:
 name: CI
 jobs:

app_test.go

@@ -592,6 +592,10 @@ func TestUnmanaged(t *testing.T) {
 			b:           make([]bool, nbElements),
 			s:           make([]string, nbElements),
 		}
+
+		// We generate a set of valid flags:
+		//   bool-1, bool-2, ... to bool-{nbElements} with short names -a, -b, ..., -e
+		//   string-1, string-2, ... to string-{nbElements} with short names -A, -B, ..., -E
 		for i := 0; i < nbElements; i++ {
 			a.Flag(fmt.Sprintf("bool-%d", i+1), "").Short(rune('a' + i)).BoolVar(&a.b[i])
 			a.Flag(fmt.Sprintf("string-%d", i+1), "").Short(rune('A' + i)).StringVar(&a.s[i])
@@ -639,6 +643,14 @@ func TestUnmanaged(t *testing.T) {
 			[]bool{true, true, false, true, true},
 			[]string{"", "", "", "", ""},
 			[]string{"-cX"}, nil},
+		{"Bad switch mixed with long", true, "-ab -cX-long -de",
+			[]bool{true, true, false, true, true},
+			[]string{"", "", "", "", ""},
+			[]string{"-cX-long"}, nil},
+		{"Bad switch mixed with very long", true, "-ab -cX-very-long -de",
+			[]bool{true, true, false, true, true},
+			[]string{"", "", "", "", ""},
+			[]string{"-cX-very-long"}, nil},
 		{"Many bad switches with args", true, "-ab -var x=1 -var y=2 -de -var z=3 test",
 			[]bool{true, true, false, true, true},
 			[]string{"", "", "", "", ""},

flags.go

@@ -2,7 +2,6 @@ package kingpin
 
 import (
 	"fmt"
-	"strings"
 )
 
 type flagGroup struct {
@@ -104,86 +103,87 @@ func (f *flagGroup) checkDuplicates() error {
 }
 
 func (f *flagGroup) parse(context *ParseContext) (*FlagClause, error) {
-	var token *Token
-	for {
-		token = context.Peek()
-		switch token.Type {
-		case TokenEOL:
-			return nil, nil
+	token := context.Peek()
+	switch token.Type {
+	case TokenEOL:
+		return nil, nil
+
+	case TokenLong, TokenShort:
+		flagToken := token
+		defaultValue := ""
+		var flag *FlagClause
+		var ok bool
+		var err error
+		invert := false
+
+		name := token.Value
+		if token.Type == TokenLong {
+			if flag, invert, err = f.getFlagAlias(name); err != nil {
+				return nil, err
+			} else if flag == nil {
+				err = fmt.Errorf("unknown long flag '%s'", flagToken)
+			}
+		} else if flag, ok = f.short[name]; !ok {
+			err = fmt.Errorf("unknown short flag '%s'", flagToken)
+		}
 
-		case TokenLong, TokenShort:
-			flagToken := token
-			defaultValue := ""
-			var flag *FlagClause
-			var ok bool
-			var err error
-			invert := false
+		if err != nil {
+			if context.appUnmanagedArgs == nil {
+				return nil, err
+			}
 
-			name := token.Value
+			// The current flag is not managed by the application, but we gather it anyway in the unmanaged args
+			current := context.current()
 			if token.Type == TokenLong {
-				if flag, invert, err = f.getFlagAlias(name); err != nil {
-					return nil, err
-				} else if flag == nil {
-					err = fmt.Errorf("unknown long flag '%s'", flagToken)
+				context.Next()
+			} else {
+				remainingArgs := "-"
+				if len(context.args) > 0 && context.rawArgs[len(context.rawArgs)-len(context.args)] == current {
+					// There are more short flags in the current element
+					remainingArgs = context.args[0]
 				}
-			} else if flag, ok = f.short[name]; !ok {
-				err = fmt.Errorf("unknown short flag '%s'", flagToken)
-			}
 
-			if err != nil {
-				if context.appUnmanagedArgs == nil {
-					return nil, err
-				}
-				current := context.current()
-				if token.Type == TokenLong {
-					context.Next()
-				} else {
-					// We have to remove all previous elements from the same short flag element
-					pos := strings.Index(current, token.Value) - 1
-					if pos < 0 {
-						return nil, err
-					}
-					context.argi -= pos
-					context.Elements = context.Elements[:len(context.Elements)-pos]
-					for x := len(current) - pos - 1; x > 0; x-- {
-						// We skip all remaining elements of the group
-						purgedToken := context.Next()
-						// This error isn't supposed to be possible, but let's handle it anyway.
-						if purgedToken.Type == TokenLong {
-							err = fmt.Errorf("while skipping unmanaged shorts flags, skipped long flag '%s' from '%s'", purgedToken.Value, current)
-							return nil, err
-						}
+				// We remove all previous elements from the same short flags group
+				previousElementsCount := len(current) - len(remainingArgs) - 1
+				context.Elements = context.Elements[:len(context.Elements)-previousElementsCount]
+
+				// We consume the remaining short flags of the current group
+				for i := 0; i < len(remainingArgs); i++ {
+					// We consume the remaining short flags of the current group
+					if consumed := context.Next(); consumed.Type != TokenShort {
+						break
 					}
 				}
-				context.appUnmanagedArgs.Unmanaged = append(context.appUnmanagedArgs.Unmanaged, current)
-				return nil, nil
 			}
 
-			context.Next()
-			flag.isSetByUser()
+			context.appUnmanagedArgs.Unmanaged = append(context.appUnmanagedArgs.Unmanaged, current)
+			return nil, nil
+		}
 
-			if fb, ok := flag.value.(boolFlag); ok && fb.IsBoolFlag() {
-				if invert {
-					defaultValue = "false"
-				} else {
-					defaultValue = "true"
-				}
+		context.Next()
+		flag.isSetByUser()
+
+		if fb, ok := flag.value.(boolFlag); ok && fb.IsBoolFlag() {
+			if invert {
+				defaultValue = "false"
 			} else {
-				token = context.Peek()
-				if token.Type != TokenArg {
-					context.Push(token)
-					return nil, fmt.Errorf("expected argument for flag '%s'", flagToken)
-				}
-				context.Next()
-				defaultValue = token.Value
+				defaultValue = "true"
 			}
+		} else {
+			token = context.Peek()
+			if token.Type != TokenArg {
+				context.Push(token)
+				return nil, fmt.Errorf("expected argument for flag '%s'", flagToken)
+			}
+			context.Next()
+			defaultValue = token.Value
+		}
 
-			context.matchedFlag(flag, defaultValue)
-			return flag, nil
+		context.matchedFlag(flag, defaultValue)
+		return flag, nil
 
-		default:
-			return nil, nil
-		}
+	default:
+		return nil, nil
 	}
 }
 

go.mod

@@ -1,4 +1,4 @@
-module github.com/coveord/kingpin/v2
+module github.com/coveooss/kingpin/v2
 
 go 1.17
 

parser.go

@@ -192,13 +192,8 @@ func (p *ParseContext) Next() *Token {
 		return p.Next()
 	}
 
-	if strings.HasPrefix(arg, "--") || (strings.HasPrefix(arg, "-") && strings.Count(arg, "-") > 1) {
-		var parts []string
-		if strings.HasPrefix(arg, "--") {
-			parts = strings.SplitN(arg[2:], "=", 2)
-		} else {
-			parts = strings.SplitN(arg[1:], "=", 2)
-		}
+	if strings.HasPrefix(arg, "--") {
+		parts := strings.SplitN(arg[2:], "=", 2)
 		token := &Token{p.argi, TokenLong, parts[0]}
 		if len(parts) == 2 {
 			p.Push(&Token{p.argi, TokenArg, parts[1]})