Fix:(issue_2225) Make external flags more robust.

command_setup.go

@@ -80,9 +80,10 @@ func (cmd *Command) setupDefaults(osArgs []string) {
 		tracef("visiting all flags given AllowExtFlags=true (cmd=%[1]q)", cmd.Name)
 		// add global flags added by other packages
 		flag.VisitAll(func(f *flag.Flag) {
-			// skip test flags
-			if !strings.HasPrefix(f.Name, ignoreFlagPrefix) {
-				cmd.Flags = append(cmd.Flags, &extFlag{f})
+			// skip test flags, and flags whose name a flag of this command
+			// already has, as that flag wins
+			if !strings.HasPrefix(f.Name, ignoreFlagPrefix) && !flagNamesInUse(cmd.Flags, []string{f.Name}) {
+				cmd.Flags = append(cmd.Flags, &extFlag{f: f})
 			}
 		})
 	}

flag_ext.go

@@ -6,11 +6,16 @@ import (
 )
 
 type extFlag struct {
-	f *flag.Flag
+	f        *flag.Flag
+	category string
 }
 
 func (e *extFlag) PreParse() error {
-	if e.f.DefValue != "" {
+	// The flag package sets DefValue to the value's String(), so only reset
+	// a value that has changed since, e.g. in a previous run. Some flags
+	// don't accept their own DefValue, such as klog's -log_backtrace_at
+	// (":0"), and setting it unchanged would fail.
+	if e.f.DefValue != "" && e.f.Value.String() != e.f.DefValue {
 		return e.Set("", e.f.DefValue)
 	}
 
@@ -46,9 +51,35 @@ func (e *extFlag) IsVisible() bool {
 }
 
 func (e *extFlag) TakesValue() bool {
+	return !e.IsBoolFlag()
+}
+
+func (e *extFlag) IsBoolFlag() bool {
+	bf, ok := e.f.Value.(boolFlag)
+	return ok && bf.IsBoolFlag()
+}
+
+func (e *extFlag) IsDefaultVisible() bool {
+	return e.f.DefValue != ""
+}
+
+func (e *extFlag) TypeName() string {
+	name, _ := flag.UnquoteUsage(e.f)
+	return name
+}
+
+func (e *extFlag) IsLocal() bool {
 	return false
 }
 
+func (e *extFlag) GetCategory() string {
+	return e.category
+}
+
+func (e *extFlag) SetCategory(c string) {
+	e.category = c
+}
+
 func (e *extFlag) GetUsage() string {
 	return e.f.Usage
 }

flag_ext_test.go

@@ -0,0 +1,111 @@
+package cli
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"flag"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// newTestCommandLine gives the test its own flag.CommandLine, where
+// AllowExtFlags looks for Go flags.
+func newTestCommandLine(t *testing.T) {
+	orig := flag.CommandLine
+	t.Cleanup(func() { flag.CommandLine = orig })
+	flag.CommandLine = flag.NewFlagSet("foo", flag.ContinueOnError)
+}
+
+// traceLocation does not accept its own default text, like klog's
+// -log_backtrace_at.
+type traceLocation struct{}
+
+func (*traceLocation) String() string { return ":0" }
+
+func (*traceLocation) Set(string) error {
+	return errors.New("syntax error: expect file.go:234")
+}
+
+func TestExtFlagDefaultNotAccepted(t *testing.T) {
+	newTestCommandLine(t)
+	flag.Var(&traceLocation{}, "log_backtrace_at", "")
+
+	cmd := &Command{
+		AllowExtFlags: true,
+		Action:        func(context.Context, *Command) error { return nil },
+	}
+
+	require.NoError(t, cmd.Run(buildTestContext(t), []string{"foo"}))
+}
+
+func TestExtFlagResetOnRerun(t *testing.T) {
+	newTestCommandLine(t)
+	ext := flag.Int("ext", 2, "")
+
+	cmd := &Command{
+		AllowExtFlags: true,
+		Action:        func(context.Context, *Command) error { return nil },
+	}
+
+	require.NoError(t, cmd.Run(buildTestContext(t), []string{"foo", "--ext", "5"}))
+	require.Equal(t, 5, *ext)
+
+	require.NoError(t, cmd.Run(buildTestContext(t), []string{"foo"}))
+	require.Equal(t, 2, *ext)
+}
+
+func TestExtFlagBoolWithoutValue(t *testing.T) {
+	newTestCommandLine(t)
+	ext := flag.Bool("ext", false, "")
+
+	cmd := &Command{
+		AllowExtFlags: true,
+		Action:        func(context.Context, *Command) error { return nil },
+	}
+
+	require.NoError(t, cmd.Run(buildTestContext(t), []string{"foo", "--ext"}))
+	require.True(t, *ext)
+}
+
+func TestExtFlagFromSubcommand(t *testing.T) {
+	newTestCommandLine(t)
+	ext := flag.Int("ext", 2, "")
+
+	cmd := &Command{
+		AllowExtFlags: true,
+		Commands: []*Command{{
+			Name:   "sub",
+			Action: func(context.Context, *Command) error { return nil },
+		}},
+	}
+
+	require.NoError(t, cmd.Run(buildTestContext(t), []string{"foo", "sub", "--ext", "5"}))
+	require.Equal(t, 5, *ext)
+}
+
+func TestExtFlagHelp(t *testing.T) {
+	newTestCommandLine(t)
+	flag.Int("ext", 2, "ext usage")
+	flag.Bool("ext-bool", false, "ext bool usage")
+	flag.String("ext-string", "", "ext string usage")
+	flag.Bool("v", false, "ext v usage")
+
+	var out bytes.Buffer
+	cmd := &Command{
+		AllowExtFlags: true,
+		Writer:        &out,
+		Flags: []Flag{
+			&BoolFlag{Name: "verbose", Aliases: []string{"v"}, Category: "Main"},
+		},
+		Action: func(context.Context, *Command) error { return nil },
+	}
+
+	require.NoError(t, cmd.Run(buildTestContext(t), []string{"foo", "--ext-string", "bob", "--help"}))
+	assert.Contains(t, out.String(), "--ext int            ext usage (default: 2)")
+	assert.Contains(t, out.String(), "--ext-bool           ext bool usage (default: false)")
+	assert.Contains(t, out.String(), "--ext-string string  ext string usage\n")
+	assert.NotContains(t, out.String(), "ext v usage")
+}

flag_test.go

@@ -3657,12 +3657,16 @@ func TestExtFlag(t *testing.T) {
 	assert.Equal(t, []string{"bar"}, extF.Names())
 	assert.True(t, extF.IsVisible())
 	assert.False(t, extF.IsSet())
-	assert.False(t, extF.TakesValue())
+	assert.True(t, extF.TakesValue())
 	assert.Equal(t, "bar usage", extF.GetUsage())
 	assert.Equal(t, "11", extF.GetValue())
 	assert.Equal(t, "10", extF.GetDefaultText())
 	assert.Nil(t, extF.GetEnvVars())
 
+	assert.Equal(t, "", extF.GetCategory())
+	extF.SetCategory("ext")
+	assert.Equal(t, "ext", extF.GetCategory())
+
 	assert.Equal(t, "integer", extF.SchemaType())
 	assert.Equal(t, "", extF.SchemaItemsType())
 }