RFE: Stricter handling of duplicate flags

[mtrmac]

TL:DR

If a flag with the same flag name is defined both as a persistent flag in a parent, and as a local flag in a subcommand, Cobra AFAICS not strictly distinguish between the two.

Why is This Needed

For compatibility with previous versions that did not use Cobra, we need to accept command --flag $subcommand …; as well as command $subcommand --flag; this is deprecated, sometimes in in favor of command same-name --flag (i.e. the same flag name, for some subcommands), or command other-name --src-flag (a different flag name, for other subcommands), but the deprecated variant needs to be accepted and interpreted correctly. Admittedly that’s a pretty rare corner case.

What Breaks:

• We would like to distinguish between command --flag subcommand and command subcommand --flag, so that we can print a warning. The way merging and parsing flags is currently implemented (basically with local.AddFlagSet(parent)), that’s not the case; the local flag is always used and the parent’s persistent one is ignored. Setting TraverseChildren makes this work as expected, it seems - but https://github.com/spf13/cobra/blob/8eaca5f0f49ad747a0722d39dca7a75c34abd21a/user_guide.md#local-flag-on-parent-commands talks about local flags, not persistent flags, so this looks like an undocumented/unreliable implementation artifact.

• We would like for --help to show only the non-deprecated --flag (on the same-name subcommand), but always hide the --flag in parent’s --help. That seems impossible to do, because --help is based on Command.LocalFlags, which is (merged flags - parent's persistent flags); this implementation can’t tell the difference between a --flag defined both on the local subcommand and inherited from the parent, and a --flag only inherited from the parent. This seems hard to fix, because Command.Flags is both the input everyone uses to define new flags, and the data structure affected by mergePersistentFlags, so any change to Flags would be caller-visible and possibly breaking. We could prevent inheritance of --flag by using a non-persistent flag on the root command, but that would break compatibility with existing users in other subcommands. What does work is using a separate local (non-persistent) flag on the root command and a separate local flag on the subcommand; then we can set the Hidden flag independently, but the downside is that this leaks all over the CLI codebase.

Any pointers to how to achieve this cleanly with the documented API, or how a PR could like, would be welcome.

Reproducer

With github.com/spf13/cobra@v1.2.1 and github.com/spf13/pflag@v1.0.5 : this passes as is, but we’d like two of the test cases to have a different outcome.

package main

import (
	"bytes"
	"fmt"
	"strings"

	"github.com/spf13/cobra"
)

type options struct {
	deprecatedFlag bool
	sameNameFlag   bool
	srcFlag        bool
}

func dummyRun(_ *cobra.Command, _ []string) {}

func createApp() (*cobra.Command, *options) {
	opts := options{}

	rootCommand := &cobra.Command{Use: "main", Long: "Top-level main help"}
	rootCommand.PersistentFlags().BoolVar(&opts.deprecatedFlag, "flag", false, "deprecated - use per-subcommand flags")
	if err := rootCommand.PersistentFlags().MarkHidden("flag"); err != nil {
		panic(`unable to mark "flag" as hidden`)
	}

	sameNameCmd := &cobra.Command{Use: "same-name", Long: "same-name help", Run: dummyRun}
	sameNameCmd.Flags().BoolVar(&opts.sameNameFlag, "flag", false, "this is not deprecated")
	rootCommand.AddCommand(sameNameCmd)

	otherNameCmd := &cobra.Command{Use: "other-name", Long: "other-name help", Run: dummyRun}
	otherNameCmd.Flags().BoolVar(&opts.srcFlag, "src-flag", false, "this is not deprecated either")
	rootCommand.AddCommand(otherNameCmd)
	return rootCommand, &opts
}

func main() {
	for _, t := range []struct {
		args     []string
		expected options
	}{
		{[]string{"same-name"}, options{}}, // No flags
		{[]string{"same-name", "--flag"}, options{sameNameFlag: true}},

		// We’d like to instead get: {[]string{"--flag", "same-name"}, options{deprecatedFlag: true}},
		{[]string{"--flag", "same-name"}, options{sameNameFlag: true}},

		{[]string{"--flag", "other-name"}, options{deprecatedFlag: true}},
		{[]string{"other-name", "--src-flag"}, options{srcFlag: true}},
	} {
		app, opts := createApp()
		app.SetArgs(t.args)
		if err := app.Execute(); err != nil {
			panic(fmt.Sprintf("Execute failed for %#v", t.args))
		}
		if *opts != t.expected {
			panic(fmt.Sprintf("Unexpected parsing results for %#v: %#v vs. %#v", t.args, opts, t.expected))
		}
	}

	for _, t := range []struct {
		args    []string
		visible bool
	}{
		{[]string{"--help"}, false},
		{[]string{"same-name", "--help"}, false}, // We’d like to have true here
		{[]string{"other-name", "--help"}, false},
	} {
		app, _ := createApp()
		stdout := bytes.Buffer{}
		app.SetOut(&stdout)
		app.SetArgs(t.args)
		if err := app.Execute(); err != nil {
			panic(fmt.Sprintf("Execute failed for %#v", t.args))
		}
		help := stdout.String()
		visible := strings.Contains(help, "--flag")
		if visible != t.visible {
			panic(fmt.Sprintf("Unexpected visibility for %#v: %v vs. %v", t.args, visible, t.visible))
		}
	}
}

[github-actions]

This issue is being marked as stale due to a long period of inactivity

[mtrmac]

Setting TraverseChildren makes this work as expected, it seems - but https://github.com/spf13/cobra/blob/8eaca5f0f49ad747a0722d39dca7a75c34abd21a/user_guide.md#local-flag-on-parent-commands talks about local flags, not persistent flags, so this looks like an undocumented/unreliable implementation artifact.

This is what we ended up doing in containers/skopeo#1369 .

[johnSchnake]

Since its acknowledged that this is a pretty niche corner case, has a known workaround, and is stale (~1y with no movement) I'm going to go ahead a close this.

Feel free to reopen if you think the conversation needs to continue.