cmd/cue: fix regression when injecting vars with cue cmd

The -t and -T flags can be used like:

    cue cmd -t foo=var -T mycmd .

However, we also support a short form, where "cmd" can be omitted:

    cue -t foo=var -T mycmd .

After the refactor in https://cuelang.org/cl/552733,
we made cmd/cue more principled in how it handles the root command,
and we broke those -t and -T flags in the "cue cmd" short form.
We didn't intend to - they just started being treated as global flags.

A simple and easy fix is to actually declare these flags globally,
and not just as part of the "cue cmd" sub-command.
Since each of these flags now exists in two levels,
the only place where they are consumed, the setTags function,
now checks both flag sets and joins their values.

Note that we make the new global flags hidden,
so that "cue help" doesn't start showing them as global flags.
We don't really intend them as global flags for every command.
We also avoid adding them to PersistentFlags,
so that they are not inherited by sub-commands like fmt.

This fix is preferrable to an entire revert of the previous CL,
as otherwise commands like "cue help" would become very slow again.

I've also filed #2519 to propose deprecating the "cue cmd" short form.

Fixes #2510.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: I254e1c4e8ef4d6ffa7d6142c75abee5f1a37a810
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/557127
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Roger Peppe <[email protected]>

cmd/cue/cmd/cmd.go

@@ -181,7 +181,7 @@ Run "cue help commands" for more details on tasks and commands.
 
 	cmd.Flags().SetInterspersed(false)
 
-	addInjectionFlags(cmd.Flags(), true)
+	addInjectionFlags(cmd.Flags(), true, false)
 
 	return cmd
 }

cmd/cue/cmd/common.go

@@ -400,7 +400,7 @@ func newBuildPlan(cmd *Command, args []string, cfg *config) (p *buildPlan, err e
 	}
 	cfg.reFile = re
 
-	if err := setTags(cmd.Flags(), cfg.loadCfg); err != nil {
+	if err := setTags(cfg.loadCfg, cmd.Flags(), cmd.Parent().Flags()); err != nil {
 		return nil, err
 	}
 
@@ -411,11 +411,16 @@ func (p *buildPlan) matchFile(file string) bool {
 	return p.cfg.reFile.MatchString(file)
 }
 
-func setTags(f *pflag.FlagSet, cfg *load.Config) error {
-	tags, _ := f.GetStringArray(string(flagInject))
-	cfg.Tags = tags
-	if b, _ := f.GetBool(string(flagInjectVars)); b {
-		cfg.TagVars = load.DefaultTagVars()
+func setTags(cfg *load.Config, flags ...*pflag.FlagSet) error {
+	// setTags usually receives a subcommmand's flags, plus the parent (root) command's flags,
+	// so that we can support "cue cmd -t foo=bar mycmd" as well as the short form "cue -t foo=bar mycmd".
+	// TODO: once we remove "cue cmd" short-hand support, go back to a single FlagSet parameter.
+	for _, f := range flags {
+		tags, _ := f.GetStringArray(string(flagInject))
+		cfg.Tags = append(cfg.Tags, tags...)
+		if b, _ := f.GetBool(string(flagInjectVars)); b {
+			cfg.TagVars = load.DefaultTagVars()
+		}
 	}
 	return nil
 }
@@ -769,7 +774,7 @@ func buildTools(cmd *Command, args []string) (*cue.Instance, error) {
 		Tools: true,
 	}
 	f := cmd.cmd.Flags()
-	if err := setTags(f, cfg); err != nil {
+	if err := setTags(cfg, f, cmd.cmd.Parent().Flags()); err != nil {
 		return nil, err
 	}
 

cmd/cue/cmd/def.go

@@ -37,7 +37,7 @@ The --expression flag is used to only print parts of a configuration.
 
 	addOutFlags(cmd.Flags(), true)
 	addOrphanFlags(cmd.Flags())
-	addInjectionFlags(cmd.Flags(), false)
+	addInjectionFlags(cmd.Flags(), false, false)
 
 	cmd.Flags().StringArrayP(string(flagExpression), "e", nil, "evaluate this expression only")
 

cmd/cue/cmd/eval.go

@@ -54,7 +54,7 @@ Examples:
 
 	addOutFlags(cmd.Flags(), true)
 	addOrphanFlags(cmd.Flags())
-	addInjectionFlags(cmd.Flags(), false)
+	addInjectionFlags(cmd.Flags(), false, false)
 
 	cmd.Flags().StringArrayP(string(flagExpression), "e", nil, "evaluate this expression only")
 

cmd/cue/cmd/export.go

@@ -94,7 +94,7 @@ yaml    output as YAML
 
 	addOutFlags(cmd.Flags(), true)
 	addOrphanFlags(cmd.Flags())
-	addInjectionFlags(cmd.Flags(), false)
+	addInjectionFlags(cmd.Flags(), false, false)
 
 	cmd.Flags().Bool(string(flagEscape), false, "use HTML escaping")
 	cmd.Flags().StringArrayP(string(flagExpression), "e", nil, "export this expression only")

cmd/cue/cmd/flags.go

@@ -88,11 +88,15 @@ func addOrphanFlags(f *pflag.FlagSet) {
 	f.Bool(string(flagMerge), true, "merge non-CUE files")
 }
 
-func addInjectionFlags(f *pflag.FlagSet, auto bool) {
+func addInjectionFlags(f *pflag.FlagSet, auto, hidden bool) {
 	f.StringArrayP(string(flagInject), "t", nil,
 		"set the value of a tagged field")
 	f.BoolP(string(flagInjectVars), "T", auto,
 		"inject system variables in tags")
+	if hidden {
+		f.Lookup(string(flagInject)).Hidden = true
+		f.Lookup(string(flagInjectVars)).Hidden = true
+	}
 }
 
 type flagName string

cmd/cue/cmd/root.go

@@ -191,6 +191,9 @@ For more information on writing CUE configuration files see cuelang.org.`,
 	subCommands = append(subCommands, newHelpTopics(c)...)
 
 	addGlobalFlags(cmd.PersistentFlags())
+	// We add the injection flags to the root command for the sake of the short form "cue -t foo=bar mycmd".
+	// Note that they are not persistent, so that they aren't inherited by sub-commands like "cue fmt".
+	addInjectionFlags(cmd.Flags(), false, true)
 
 	for _, sub := range subCommands {
 		cmd.AddCommand(sub)

cmd/cue/cmd/testdata/script/cmd_tags.txtar

@@ -1,6 +1,23 @@
 exec cue cmd -t prod -t name=bar tag tags.cue tags_tool.cue
 cmp stdout expect-stdout
 
+# Same as before, but now with the short form.
+# See https://cuelang.org/issue/2510.
+exec cue -t prod -t name=bar tag tags.cue tags_tool.cue
+cmp stdout expect-stdout
+
+# Verify that the new global -t flag added as a fix for issue 2510 above
+# works with the explicit or implicit "cmd" sub-command,
+# but not with other sub-commands like "fmt".
+exec cue eval -t name=bar tags.cue
+stdout 'name: *"bar"'
+exec cue -t name=bar eval tags.cue
+stdout 'name: *"bar"'
+! exec cue fmt -t name=bar tags.cue
+stderr 'unknown shorthand flag'
+! exec cue -t name=bar fmt tags.cue
+stderr 'unknown shorthand flag'
+
 -- expect-stdout --
 prod: bar
 -- tags.cue --

cmd/cue/cmd/vet.go

@@ -70,7 +70,7 @@ func newVetCmd(c *Command) *cobra.Command {
 	}
 
 	addOrphanFlags(cmd.Flags())
-	addInjectionFlags(cmd.Flags(), false)
+	addInjectionFlags(cmd.Flags(), false, false)
 
 	cmd.Flags().BoolP(string(flagConcrete), "c", false,
 		"require the evaluation to be concrete")