The use of args here was added in this commit and it does seem that it was a mistake; I believe the flags variable should have been used instead.

I wanted to discuss backwards-compatibility for this change.
Before this PR cobra would pass all the arguments on the command-line (which was incorrect).
With this PR cobra will pass only the arguments after removing the sub-command and flags, as it does when calling the *Run/RunE functions.

It is possible that some programs worked around this bug and fixing it may affect them.

However, considering that before this PR the help function sometime received all the args (when using the --help flag) but no args at all when called from the help command, it makes it less likely that a workaround used the actual args parameter.

So, I feel it is ok to make this change but if we do maybe we should include a note in the release notes.

@jpmcb how do you feel about this?

Sending only the remaining arguments is better when using -h. If we think this may break someone, we can do this only if a new flag is set like HelpGetRemainingArgs so users can opt-int to use the new behavior.

When using help command I'm not sure arguments make sense. You use this to learn about a command before you use it. You use -h,--help to get help for a command while trying to use it. You expect that adding -h,--help anywhere in the current command will show the help and getting more specific help for the current argument is nice improvement. Personally I never use help command since -h is much easier to type.

-h, --help use case:

Trying to run a command:

$ foo bar --baz
error: ...

It did not work, add -h:

$ foo bar --baz -h
help on foo bar...

Use it correctly:

$ foo bar --baz missing-value
success

help use case

What is the foo bar command?

$ foo help bar
help on foo bar

Your use case description is correct. However, there is another (obscure) situation, which is actually the one I'm really trying to fix 😄.

The tanzu CLI uses plugins to provide many of its sub-commands. So a user may do:
tanzu help cluster list
to get the help on the cluster list command.
However, the tanzu CLI only has cluster as a sub-command and needs to call that "cluster" plugin to ask it for the help for the cluster list command. Therefore, the arguments passed to the original tanzu help command are required to know which sub-command of the plugin the user is asking about.

Currently, the tanzu CLI code has to use os.Args to get that info: https://github.com/vmware-tanzu/tanzu-cli/blob/6a11ce93cd4e811e213e8439e090e1d73a053fd3/pkg/cli/plugin_cmd.go#L192-L201

Thanks for explaning this, so this is needed when a command loads its sub commands dynamically. The help commands may need to do the same dynamic loading to display the help.

I don't feel this has any backwards-compatibility implications since the args value was always empty before this change for this case.

I think we are missing documentation explaning that Run, RunE, and HelpFunc accept the remaining arguments after parsing. Previously HelpFunc sometimes accepted no arguments and sometimes accepted the raw parsed arguments.

Can be here:

// SetHelpFunc sets help function. Can be defined by Application.
// The help function is called with the remaining arguments after parsing
// the flags. If flag parsing is disabled it is called with all arguments.
func (c *Command) SetHelpFunc(f func(*Command, []string)) {
	c.helpFunc = f
}

Sending only the remaining arguments is better when using -h. If we think this may break someone, we can do this only if a new flag is set like HelpGetRemainingArgs so users can opt-int to use the new behavior.

When using help command I'm not sure arguments make sense. You use this to learn about a command before you use it. You use -h,--help to get help for a command while trying to use it. You expect that adding -h,--help anywhere in the current command will show the help and getting more specific help for the current argument is nice improvement. Personally I never use help command since -h is much easier to type.

-h, --help use case:

Trying to run a command:

$ foo bar --baz
error: ...

It did not work, add -h:

$ foo bar --baz -h
help on foo bar...

Use it correctly:

$ foo bar --baz missing-value
success

help use case

What is the foo bar command?

$ foo help bar
help on foo bar

We repeat the code of the help function many times, maybe add a helper struct keeping the , so we can do:

 h := helper{t: t, command: "child", args: []string{"args1", "arg2"}}
 rootCmd.SetHelpFunc(h.helpFunc)
 _, err := executeCommand(rootCmd, "help", "child", "arg1", "arg2")

 if !h.helpCalled {
     ...
 }

I like it!
I tried to do what you suggest.

We can extract lines 1098 to 1108 to a argsEqual() helper that can be useful in other tests, and can be replaced with slices.Equal when we can required Go 1.18. Can also be a good place to document why we cannot use slices.Equal.

Would be little bit nicer since since we don't have duplicate the error here:

if !argsEqual(args, o,expectedArgs) {
    o.err = fmt.Errorf("Expected args %v, got %v", o.expectedArgs, args)
}

Any reason for returning a help function instead of implementing one?

This could be:

func (o *overridingHelp) helpFunc(c *Command, args []string) {
   ...
}

And used later as:

rootCmd.SetHelpFunc(override.helpFunc)

Why test additional flags separately? this way we don't cover all cases.

In both help command you don't test additional flags - if you want to have the same behavior for help command, would it be good to ensure that parsed flags are not passed to the help function?

Thanks for explaining and adding the comment.

I think this will communicates better the intent of the code:

// The call to execute() above has parsed the flags.
// We therefore only pass the remaining arguments to the help function.
var remainingArgs []string
if cmd.DisableFlagParsing {
    // For commands that have DisableFlagParsing == true, the flag parsing was
    // not done, therefore use the full set of arguments, which include flags.
    remainingArgs = flags
} else {
    remainingArgs = cmd.Flags().Args()

}

Thanks for explaning this, so this is needed when a command loads its sub commands dynamically. The help commands may need to do the same dynamic loading to display the help.

I think we are missing documentation explaning that Run, RunE, and HelpFunc accept the remaining arguments after parsing. Previously HelpFunc sometimes accepted no arguments and sometimes accepted the raw parsed arguments.

Can be here:

// SetHelpFunc sets help function. Can be defined by Application.
// The help function is called with the remaining arguments after parsing
// the flags. If flag parsing is disabled it is called with all arguments.
func (c *Command) SetHelpFunc(f func(*Command, []string)) {
	c.helpFunc = f
}