v1 regression: v1.22.2-and-over skips over flag-like arguments to subcommand string slice flags

[cyphar]

my urfave/cli version is

v1.22.1 was the last working version, and all versions up to v1.22.4 are broken.

Checklist

• Are you running the latest v1 release? The list of releases is here.
• Did you check the manual for your release? The v1 manual is here
• Did you perform a search about this problem? Here's the Github guide about searching.

Dependency Management

• My project is using go modules.
• My project is using vendoring.
• My project is automatically downloading the latest version.
• I am unsure of what my dependency management setup is.

Describe the bug

If you try to use a flag-like argument to a string slice argument that is not a valid flag, the argument gets ignored and instead the next argument is used instead (which, if it is a valid string slice argument it is swallowed as a flag value).

To reproduce

First, set up a basic Go dev environment for the test:

% mkdir -p /tmp/testcli
% cd /tmp/testcli
% go mod init example.com/testcli
go: creating new go.mod: module example.com/testcli
% go get github.com/urfave/cli@v1.22.4
go: downloading github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d

And then try to run the following program:

package main

import (
	"fmt"
	"os"

	"github.com/urfave/cli"
)

func main() {
	app := cli.NewApp()
	app.Commands = []cli.Command{
		{
			Name: "config",
			Flags: []cli.Flag{
				cli.StringSliceFlag{
					Name: "config.cmd",
				},
			},
			Action: func(ctx *cli.Context) error {
				fmt.Printf("args: %v\n", os.Args)
				fmt.Printf("leftover args: %v\n", ctx.Args())
				fmt.Printf("config.cmd: %v\n", ctx.StringSlice("config.cmd"))
				return nil
			},
		},
	}

	err := app.Run(os.Args)
	if err != nil {
		fmt.Printf("err: %v\n", err)
	}
}

Like so:

% go run main.go config --config.cmd "-c" --config.cmd "ls -la"

Observed behavior

If you use 1.22.2 or later of github.com/urfave/cli, you find:

% go run main.go config --config.cmd "-c" --config.cmd "ls -la"
args: [/tmp/go-build948656450/b001/exe/main config --config.cmd -c --config.cmd ls -la]
leftover args: [ls -la -c]
config.cmd: [--config.cmd]

What appears to be happening is that the -c argument to the --config.cmd flag is being skipped -- though it's not being treated as an unknown flag (there's no error about unknown flags) and instead is being treated as a positional argument. As as result, --config.cmd is being used as an argument (presumably because it's a known flag?) and the rest of the arguments are treated as positional arguments.

However, if you instead call the program like so you get the correct output:

% go run main.go config --config.cmd="-c" --config.cmd "ls -la"
args: [/tmp/go-build358735402/b001/exe/main config --config.cmd=-c --config.cmd ls -la]
leftover args: []
config.cmd: [-c ls -la]

Expected behavior

If you use 1.22.1 or earlier of github.com/urfave/cli, you find:

% go run main.go config --config.cmd "-c" --config.cmd "ls -la"
args: [/tmp/go-build278409428/b001/exe/main config --config.cmd -c --config.cmd ls -la]
leftover args: []
config.cmd: [-c ls -la]

Additional context

I am trying to update my version of github.com/urfave/cli in umoci and this is an issue I hit almost immediately when trying to do the update.

N.B. This issue doesn't happen unless you use subcommands. If you try to do this with the top-level app.Flags then it works as expected.

Run go version and paste its output here

go version go1.14.4 linux/amd64

Run go env and paste its output here

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cyphar/.cache/go-build"
GOENV="/home/cyphar/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cyphar/.local"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib64/go/1.14"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib64/go/1.14/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/cyphar/tmp/testcli/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build827330497=/tmp/go-build -gno-record-gcc-switches"

[coilysiren]

Thanks for the well written bug report 👍 we get regressions of this sort fairly frequently, unfortunately. You're free to try your hand at fixing it, but I would just warn you that you'd probably end up creating regression for someone else's case 😅

[cyphar]

Is it an issue of not enough test cases in urfave/cli to catch these sorts of regressions, or lack of documentation about what the intended semantics are?

For the bug at hand, using the above program, I've bisected it to 7d0751f:

7d0751fa1309a01b405cde327cc43275d13016a2 is the first bad commit
commit 7d0751fa1309a01b405cde327cc43275d13016a2
Author: Lynn Cyrin <lynn@textio.com>
Date:   Wed Sep 11 19:25:51 2019 -0700

    add argIsFlag check

 command.go | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

But I think the bug is older and 7d0751f just exposed it by making it so that unknown flags also get re-ordered. It seems to me like the issue is with this check in reorderArgs (which appears to assume that flag arguments won't start with - -- I would expect that this should just be if readFlagValue):

		if readFlagValue && !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") {
			readFlagValue = false
			reorderedArgs = append(reorderedArgs, arg)
			continue
		}
		readFlagValue = false

So I think that this invocation only happened to work in the past, because the argument re-ordering would re-order all arguments that looked like flags rather than just known arguments (now that known arguments are kept in place, the arguments get re-ordered to --config.cmd --config.cmd -c "ls -la").

Before I go about writing a patch for this, I should probably ask whether urfave/cli is intended to support things like -a -b where -a takes a positional argument. I believe that this is the behaviour of getopt(3) and all getopt-clone systems, but since this does seem like a fairly fundamental assumption in urfave/cli, I thought I should ask before I spend time writing up a patch.

As a workaround I can just disable argument re-ordering (which I don't use anyway).

[cyphar]

Ah okay, so the newest version looks a bit different -- I think this line should be } else if nextIndexMayContainValue {

		if arg == "--" {
			remainingArgs = append(remainingArgs, args[i:]...)
			break

			// 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)

			// checks if this is an arg that should be re-ordered
		} else if argIsFlag(commandFlags, arg) {
			// ...

But that will cause issues with boolean flags (or any flag that doesn't take an argument) because you could end up reordering some later argument (--bool --str str --foo bar will be reordered to --bool --str --foo str bar or something similar). So maybe we could use TakesValue() to figure out whether nextIndexMayContainValue -- is there a reason that TakesValue() isn't being used today?

[coilysiren]

Is it an issue of not enough test cases in urfave/cli to catch these sorts of regressions, or lack of documentation about what the intended semantics are?

Tests. There's essentially no documentation about this - and I'm willing to bet that writing tests for this is easier than writing documentation.

For the bug at hand, using the above program, I've bisected it to 7d0751f

Which, in case it's not obvious, is a change that itself was fixing a regression: #872.

Before I go about writing a patch for this, I should probably ask whether urfave/cli is intended to support things like -a -b where -a takes a positional argument. I believe that this is the behaviour of getopt(3) and all getopt-clone systems, but since this does seem like a fairly fundamental assumption in urfave/cli, I thought I should ask before I spend time writing up a patch.

I don't know what we for this case offhand, but whatever we do now is what we should continue to do. This is exactly the kind of thing that would cause a massive regression in someone's system if it was changed.

is there a reason that TakesValue() isn't being used today?

There is not.

[coilysiren]

Oh, there's also an existing PR working on that exact same area of the code: #1135

[cyphar]

Yeah this is a duplicate of #1135.