Cli: v2 bug: naked double dash separator (--) strange behavior

[krostar]

my urfave/cli version is

v2.2.0

Checklist

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

Dependency Management

• [x] 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

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

[lynncyrin]

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[bot]]

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.