Creating two cli.Command with the same Name results in magic

[johnwyles]

If you name a cli.Command the same as another cli.Command only one of them will execute with no error or warning that there is also a duplicate command by the same name and with an exit code of 0. Consider the following:

import (
	"fmt"
	"log"
	"os"

	"github.com/urfave/cli"
)

func main() {
	app := cli.NewApp()
	app.Commands = []cli.Command{
		// This is the only command that WILL execute since it is the first "foo" command found
		cli.Command{
			Action: func(c *cli.Context) { fmt.Println("foo1") }, // Print "foo1"
			Name:   "foo", // Name is "foo"
		},
		// This command will NEVER execute since it is the second "foo" match in the array
		cli.Command{
			Action: func(c *cli.Context) { fmt.Println("foo2") }, // Print "foo2"
			Name:   "foo", // Name is ALSO "foo"
		},
	}
	app.Name = "test"

	if err := app.Run(os.Args); err != nil {
		log.Fatalf("An error occurred: %s", err)
	}
}

Running this code yields the following output:

$ go run test.go foo
foo1
$ echo $?
0

Suggest checking all cli.Commands for any duplicates and exiting with a message and non-zero exit code or running all cli.Commands that match the cli.Command.Name field.

gist link: https://gist.github.com/johnwyles/3129a05391e1f749c661b1efc98abc67

[johnwyles]

I should also point out this is especially more cryptic to debug and discover (as it was for me 😄) when you have moved your cli.Command functions to separate files as you would probably do in most modest sized projects (see here).

[michaeljs1990]

It seems reasonable that this would just blow up when this happens. It shouldn't be a hard check to add internally.

[johnwyles]

I split my commands (e.g. cli.Command) per-file and certainly when you have several if you're not careful about what you name them you can screw this up and the debugging was atrocious - iirc 2 hours of killed

[johnwyles]

Another thought I just had is that another seemingly expected behaviour would be that both would execute in some order (presumably as code reads). I am unsure if you want to blow up or execute both because then the ordering (however one expect ordering to go?) would determine their serial execution and I am unsure if that is known or unknown to the user. With this hindsight perhaps executing both is the expected behavior. Let me know if I need to clarify that point...

[johnwyles]

@michaeljs1990 what are your thoughts on this? should we disallow the same flag and bomb out or should we allow two actions for the same flag? I can easily put together a PR depending on what we think the most common expected behavior would be. As it is neither of the two happen and it will just quietly not execute arbitrarily one or the other of the duplicate flags.

[coilysiren]

Hi @johnwyles! We should definitely do this

should we disallow the same flag and bomb out

I would be very happy to review a PR implementing that ✨

[johnwyles]

@lynncyrin I had not yet dove through to doing an implementation of this but I think it wouldn't be hard if someone would take a good look at: https://github.com/urfave/cli/blob/v2/command.go#L167

and possibly somewhere here:
https://github.com/urfave/cli/blob/v2/app.go#L130-L145

I could be wrong as I am just now glancing back at this after months of working with this.

[ruzaikr]

I would love to do this. Pls assign it to me @lynncyrin :)