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

[coilysiren]

@ruzaikr you got it! 🚀

[ruzaikr]

@lynncyrin This is my first Github issue so I would like to quickly check if I'm going in the right direction:

v1...ruzaikr:bugfix/785-no-error-when-same-command-name-used

Do I need to create some tests now?

[coilysiren]

@ruzaikr creating a draft pull request (described on this page) is slightly easier than providing a link to your branch 🙂

WRT the code itself:

• yes you should add tests
• also consider checking if subcommands within the same command have the same name

[ruzaikr]

Thanks @lynncyrin for your comment. I've given it a shot and created a PR :)

[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.

[coilysiren]

there's a WIP PR for this here => #984

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[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.

[stale]

Closing this as it has become stale.

[meatballhat]

@ruzaikr Hello and sorry for the big delay 😭 Any chance you're interested in picking up this work again?

[ruzaikr]

@meatballhat Hello, my friend! Actually, it is my bad because I didn’t follow up on the last feedback I received.

Unfortunately, I am in between jobs right now and time is a bit tight. Is 3 weeks from now acceptable?

[meatballhat]

@ruzaikr Hello and thank you so much for the incredibly fast reply! 3 weeks from now is more than acceptable 🎉 I'm looking forward to seeing your work 🤘🏼

[linrl3]

@meatballhat Hello! This bug is still open. I am using github.com/urfave/cli/v2 v2.25.7. Can I create a PR for this?

[dearchap]

@linrl3 yes please.

[linrl3]

@dearchap Thanks! Please take a look at this PR: #1805

[bartekpacia]

Since #1805 is merged into v2-maint, I think that this issue can be closed?

cc @dearchap