Naked double dash separator (--) strange behavior

[krostar]

my urfave/cli version is

v2.2.0

Checklist

• Are you running the latest v2 release? The list of releases is here.
• Did you check the manual for your release? The v2 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.

Describe the bug

-- is sometimes present in the output of Context.Args.Slice() and sometimes it is not present, which makes it impossible to properly differentiate arguments before and after the terminator.

To reproduce

package main

import (
	"fmt"

	"github.com/urfave/cli/v2"
)

func main() {
	app := cli.NewApp()
	app.Action = func(c *cli.Context) error {
		for _, arg := range c.Args().Slice() {
			fmt.Printf("%q\n", arg)
		}
		return nil
	}

	fmt.Println("1.")
	_ = app.Run([]string{"", "foo", "--", "bar"})
	fmt.Println("\n2.")
	_ = app.Run([]string{"", "--", "foo", "bar"})
	fmt.Println("\n3.")
	_ = app.Run([]string{"", "foo", "bar", "--"})
}

which produces

#> go run main.go
1.
"foo"
"--"
"bar"

2.
"foo"
"bar"

3.
"foo"
"bar"
"--"

Observed behavior

When argument parsing terminator -- is placed before any other arguments, it is missing from the arguments list. When there are positional arguments before it, it is present in the arguments list.

Expected behavior

I would have expected to find the terminator in the second scenario as such:

#> go run main.go
1.
"foo"
"--"
"bar"

2.
"--"
"foo"
"bar"

3.
"foo"
"bar"
"--"

Run go version and paste its output here

go version go1.14.1 darwin/amd64

Run go env and paste its output here

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="$HOME/Library/Caches>/go-build"
GOENV="$HOME/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="$HOME/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="$WORKIR/$PROJECTNAME/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0k/3xk6gf990yq1szxx_6fx0ltm28gp8s/T/go-build129788219=/tmp/go-build -gno-record-gcc-switches -fno-common"

[atif-konasl]

Hi @krostar , I would like slove this issue. I did analysis the bug and found the bug in flag.go file
would you please assign me this issue?

[krostar]

Hi @atif-konasl, thank you for your reactivity, but I can't assign you to an issue.

Feel free to submit a PR if you know how to solve this and reference it to this issue.

Maintainers will probably be happy to see the bug already have a fix when they give a look to this issue, and your PR.

[atif-konasl]

Hi @krostar, there is a bug in parseOne() method in flag package of go.

func (f *FlagSet) parseOne() (bool, error) {
	if len(f.args) == 0 {
		return false, nil
	}
	s := f.args[0]
	if len(s) < 2 || s[0] != '-' {
		return false, nil
	}
	numMinuses := 1
	if s[1] == '-' {
		numMinuses++
		if len(s) == 2 { // "--" terminates the flags
			f.args = f.args[1:]
			return false, nil
		}
	}
	name := s[numMinuses:]
	if len(name) == 0 || name[0] == '-' || name[0] == '=' {
		return false, f.failf("bad flag syntax: %s", s)
	}

	// it's a flag. does it have an argument?
	f.args = f.args[1:]
	hasValue := false
	value := ""
	for i := 1; i < len(name); i++ { // equals cannot be first
		if name[i] == '=' {
			value = name[i+1:]
			hasValue = true
			name = name[0:i]
			break
		}
	}
	m := f.formal
	flag, alreadythere := m[name] // BUG
	if !alreadythere {
		if name == "help" || name == "h" { // special case for nice help message.
			f.usage()
			return false, ErrHelp
		}
		return false, f.failf("flag provided but not defined: -%s", name)
	}

	if fv, ok := flag.Value.(boolFlag); ok && fv.IsBoolFlag() { // special case: doesn't need an arg
		if hasValue {
			if err := fv.Set(value); err != nil {
				return false, f.failf("invalid boolean value %q for -%s: %v", value, name, err)
			}
		} else {
			if err := fv.Set("true"); err != nil {
				return false, f.failf("invalid boolean flag %s: %v", name, err)
			}
		}
	} else {
		// It must have a value, which might be the next argument.
		if !hasValue && len(f.args) > 0 {
			// value is the next arg
			hasValue = true
			value, f.args = f.args[0], f.args[1:]
		}
		if !hasValue {
			return false, f.failf("flag needs an argument: -%s", name)
		}
		if err := flag.Value.Set(value); err != nil {
			return false, f.failf("invalid value %q for flag -%s: %v", value, name, err)
		}
	}
	if f.actual == nil {
		f.actual = make(map[string]*Flag)
	}
	f.actual[name] = flag
	return true, nil
}

`
The problem occurs when executes this code segment :

numMinuses := 1
	if s[1] == '-' {
		numMinuses++
		if len(s) == 2 { // "--" terminates the flags
			f.args = f.args[1:]
			return false, nil
		}
	}

[coilysiren]

Hi @atif-konasl, @krostar isn't a maintainer for this library and therefore cannot assign you to anything 🙂

I'm assigning this issue to you now, feel free to stand up a PR whenever you have time.

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[meatballhat]

Reopening given stalebot's involvement; will be re-verifying 🔜

[dearchap]

Verified this behaviour using a unit test. Yes the bug is in the golang flag library. Until we replace that with our own parsing dont think its an easy fix.

[dearchap]

@krostar so the "easiest" way to fix is to allow SkipFlagParsing at the level at which the args shouldnt be parsed and this will allow you to capture all the args