BATS tests: new too-many-arguments test

...plus a few others. And fixes to actual parsing.

If a command's usage message includes '...' in the
argument list, assume it can take unlimited arguments.
Nothing we can check.

For all others, though, the ALL-CAPS part on the
right-hand side of the usage message will define
an upper bound on the number of arguments accepted
by the command. So in our 'podman --help' test,
generate N+1 args and run that command. We expect
a 125 exit status and a suitably helpful error message.

Not all podman commands or subcommands were checking,
so I fixed that. And, fixed some broken usage messages
(all-caps FLAGS, and '[flags]' at the end of 'ARGS').
Add new checks to the help test to prevent those in
the future.

Plus a little refactoring/cleanup where necessary.

Signed-off-by: Ed Santiago <[email protected]>

cmd/podman/containers/commit.go

@@ -22,15 +22,15 @@ var (
 		Short: "Create new image based on the changed container",
 		Long:  commitDescription,
 		RunE:  commit,
-		Args:  cobra.MinimumNArgs(1),
+		Args:  cobra.RangeArgs(1, 2),
 		Example: `podman commit -q --message "committing container to image" reverent_golick image-committed
   podman commit -q --author "firstName lastName" reverent_golick image-committed
   podman commit -q --pause=false containerID image-committed
   podman commit containerID`,
 	}
 
 	containerCommitCommand = &cobra.Command{
-		Args:  cobra.MinimumNArgs(1),
+		Args:  commitCommand.Args,
 		Use:   commitCommand.Use,
 		Short: commitCommand.Short,
 		Long:  commitCommand.Long,
@@ -82,7 +82,7 @@ func init() {
 
 func commit(cmd *cobra.Command, args []string) error {
 	container := args[0]
-	if len(args) > 1 {
+	if len(args) == 2 {
 		commitOptions.ImageName = args[1]
 	}
 	if !commitOptions.Quiet {

cmd/podman/containers/export.go

@@ -28,7 +28,7 @@ var (
 	}
 
 	containerExportCommand = &cobra.Command{
-		Args:  cobra.MinimumNArgs(1),
+		Args:  cobra.ExactArgs(1),
 		Use:   exportCommand.Use,
 		Short: exportCommand.Short,
 		Long:  exportCommand.Long,

cmd/podman/containers/inspect.go

@@ -10,7 +10,7 @@ import (
 var (
 	// podman container _inspect_
 	inspectCmd = &cobra.Command{
-		Use:   "inspect [flags] CONTAINER",
+		Use:   "inspect [flags] CONTAINER [CONTAINER...]",
 		Short: "Display the configuration of a container",
 		Long:  `Displays the low-level information on a container identified by name or ID.`,
 		RunE:  inspectExec,

cmd/podman/containers/mount.go

@@ -23,7 +23,7 @@ var (
 `
 
 	mountCommand = &cobra.Command{
-		Use:   "mount [flags] [CONTAINER]",
+		Use:   "mount [flags] [CONTAINER...]",
 		Short: "Mount a working container's root filesystem",
 		Long:  mountDescription,
 		RunE:  mount,

cmd/podman/containers/port.go

@@ -89,6 +89,9 @@ func port(cmd *cobra.Command, args []string) error {
 		container = args[0]
 	}
 	port := ""
+	if len(args) > 2 {
+		return errors.Errorf("`port` accepts at most 2 arguments")
+	}
 	if len(args) > 1 && !portOpts.Latest {
 		port = args[1]
 	}

cmd/podman/containers/top.go

@@ -25,7 +25,7 @@ var (
 	topOptions = entities.TopOptions{}
 
 	topCommand = &cobra.Command{
-		Use:   "top [flags] CONTAINER [FORMAT-DESCRIPTORS|ARGS]",
+		Use:   "top [flags] CONTAINER [FORMAT-DESCRIPTORS|ARGS...]",
 		Short: "Display the running processes of a container",
 		Long:  topDescription,
 		RunE:  top,

cmd/podman/generate/systemd.go

@@ -20,7 +20,7 @@ var (
 		Short: "Generate systemd units.",
 		Long:  systemdDescription,
 		RunE:  systemd,
-		Args:  cobra.MinimumNArgs(1),
+		Args:  cobra.ExactArgs(1),
 		Example: `podman generate systemd CTR
   podman generate systemd --new --time 10 CTR
   podman generate systemd --files --name POD`,

cmd/podman/images/build.go

@@ -45,6 +45,7 @@ var (
 		Long:             buildDescription,
 		TraverseChildren: true,
 		RunE:             build,
+		Args:             cobra.MaximumNArgs(1),
 		Example: `podman build .
   podman build --creds=username:password -t imageName -f Containerfile.simple .
   podman build --layers --force-rm --tag imageName .`,

cmd/podman/images/inspect.go

@@ -10,7 +10,7 @@ import (
 var (
 	// Command: podman image _inspect_
 	inspectCmd = &cobra.Command{
-		Use:   "inspect [flags] IMAGE",
+		Use:   "inspect [flags] IMAGE [IMAGE...]",
 		Short: "Display the configuration of an image",
 		Long:  `Displays the low-level information of an image identified by name or ID.`,
 		RunE:  inspectExec,

cmd/podman/images/list.go

@@ -32,7 +32,7 @@ type listFlagType struct {
 var (
 	// Command: podman image _list_
 	listCmd = &cobra.Command{
-		Use:     "list [FLAGS] [IMAGE]",
+		Use:     "list [flags] [IMAGE]",
 		Aliases: []string{"ls"},
 		Args:    cobra.MaximumNArgs(1),
 		Short:   "List images in local storage",

cmd/podman/images/push.go

@@ -29,10 +29,11 @@ var (
 
 	// Command: podman push
 	pushCmd = &cobra.Command{
-		Use:   "push [flags] SOURCE DESTINATION",
+		Use:   "push [flags] SOURCE [DESTINATION]",
 		Short: "Push an image to a specified destination",
 		Long:  pushDescription,
 		RunE:  imagePush,
+		Args:  cobra.RangeArgs(1, 2),
 		Example: `podman push imageID docker://registry.example.com/repository:tag
 		podman push imageID oci-archive:/path/to/layout:image:tag`,
 	}
@@ -45,6 +46,7 @@ var (
 		Short: pushCmd.Short,
 		Long:  pushCmd.Long,
 		RunE:  pushCmd.RunE,
+		Args:  pushCmd.Args,
 		Example: `podman image push imageID docker://registry.example.com/repository:tag
 		podman image push imageID oci-archive:/path/to/layout:image:tag`,
 	}
@@ -96,19 +98,8 @@ func pushFlags(flags *pflag.FlagSet) {
 
 // imagePush is implement the command for pushing images.
 func imagePush(cmd *cobra.Command, args []string) error {
-	var source, destination string
-	switch len(args) {
-	case 1:
-		source = args[0]
-		destination = args[0]
-	case 2:
-		source = args[0]
-		destination = args[1]
-	case 0:
-		fallthrough
-	default:
-		return errors.New("push requires at least one image name, or optionally a second to specify a different destination")
-	}
+	source := args[0]
+	destination := args[len(args)-1]
 
 	// TLS verification in c/image is controlled via a `types.OptionalBool`
 	// which allows for distinguishing among set-true, set-false, unspecified

cmd/podman/images/save.go

@@ -23,8 +23,8 @@ var (
 	saveDescription = `Save an image to docker-archive or oci-archive on the local machine. Default is docker-archive.`
 
 	saveCommand = &cobra.Command{
-		Use:   "save [flags] IMAGE",
-		Short: "Save image to an archive",
+		Use:   "save [flags] IMAGE [IMAGE...]",
+		Short: "Save image(s) to an archive",
 		Long:  saveDescription,
 		RunE:  save,
 		Args: func(cmd *cobra.Command, args []string) error {

cmd/podman/images/trust_show.go

@@ -18,6 +18,7 @@ var (
 		Short:   "Display trust policy for the system",
 		Long:    showTrustDescription,
 		RunE:    showTrust,
+		Args:    cobra.MaximumNArgs(1),
 		Example: "",
 	}
 )

cmd/podman/inspect.go

@@ -10,7 +10,7 @@ import (
 var (
 	// Command: podman _inspect_ Object_ID
 	inspectCmd = &cobra.Command{
-		Use:              "inspect [flags] {CONTAINER_ID | IMAGE_ID}",
+		Use:              "inspect [flags] {CONTAINER_ID | IMAGE_ID} [...]",
 		Short:            "Display the configuration of object denoted by ID",
 		Long:             "Displays the low-level information on an object identified by name or ID",
 		TraverseChildren: true,

cmd/podman/manifest/create.go

@@ -20,7 +20,7 @@ var (
 		Example: `podman manifest create mylist:v1.11
   podman manifest create mylist:v1.11 arch-specific-image-to-add
   podman manifest create --all mylist:v1.11 transport:tagged-image-to-add`,
-		Args: cobra.MinimumNArgs(1),
+		Args: cobra.RangeArgs(1, 2),
 	}
 )
 

cmd/podman/networks/create.go

@@ -8,7 +8,6 @@ import (
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/pkg/domain/entities"
 	"github.com/containers/libpod/pkg/network"
-	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -20,6 +19,7 @@ var (
 		Short:   "network create",
 		Long:    networkCreateDescription,
 		RunE:    networkCreate,
+		Args:    cobra.MaximumNArgs(1),
 		Example: `podman network create podman1`,
 		Annotations: map[string]string{
 			registry.ParentNSRequired: "",
@@ -62,14 +62,10 @@ func networkCreate(cmd *cobra.Command, args []string) error {
 	if err := network.IsSupportedDriver(networkCreateOptions.Driver); err != nil {
 		return err
 	}
-	if len(args) > 1 {
-		return errors.Errorf("only one network can be created at a time")
-	}
-	if len(args) > 0 && !define.NameRegex.MatchString(args[0]) {
-		return define.RegexError
-	}
-
 	if len(args) > 0 {
+		if !define.NameRegex.MatchString(args[0]) {
+			return define.RegexError
+		}
 		name = args[0]
 	}
 	response, err := registry.ContainerEngine().NetworkCreate(registry.Context(), name, networkCreateOptions)

cmd/podman/networks/inspect.go

@@ -16,7 +16,7 @@ import (
 var (
 	networkinspectDescription = `Inspect network`
 	networkinspectCommand     = &cobra.Command{
-		Use:     "inspect NETWORK [NETWORK...] [flags] ",
+		Use:     "inspect [flags] NETWORK [NETWORK...]",
 		Short:   "network inspect",
 		Long:    networkinspectDescription,
 		RunE:    networkInspect,

cmd/podman/pods/top.go

@@ -22,7 +22,7 @@ var (
 	topOptions = entities.PodTopOptions{}
 
 	topCommand = &cobra.Command{
-		Use:   "top [flags] POD [FORMAT-DESCRIPTORS|ARGS]",
+		Use:   "top [flags] POD [FORMAT-DESCRIPTORS|ARGS...]",
 		Short: "Display the running processes of containers in a pod",
 		Long:  topDescription,
 		RunE:  top,

cmd/podman/system/connection.go

@@ -29,7 +29,7 @@ var (
 		return nil
 	}
 	connectionCmd = &cobra.Command{
-		Use:  "connection [flags] destination",
+		Use:  "connection [flags] DESTINATION",
 		Args: cobra.ExactArgs(1),
 		Long: `Store ssh destination information in podman configuration.
   "destination" is of the form [user@]hostname or

cmd/podman/system/unshare.go

@@ -13,7 +13,7 @@ import (
 var (
 	unshareDescription = "Runs a command in a modified user namespace."
 	unshareCommand     = &cobra.Command{
-		Use:   "unshare [flags] [COMMAND [ARG]]",
+		Use:   "unshare [flags] [COMMAND [ARG ...]]",
 		Short: "Run a command in a modified user namespace",
 		Long:  unshareDescription,
 		RunE:  unshare,

cmd/podman/validate/args.go

@@ -25,8 +25,11 @@ func SubCommandExists(cmd *cobra.Command, args []string) error {
 
 // IDOrLatestArgs used to validate a nameOrId was provided or the "--latest" flag
 func IDOrLatestArgs(cmd *cobra.Command, args []string) error {
-	if len(args) > 1 || (len(args) == 0 && !cmd.Flag("latest").Changed) {
-		return fmt.Errorf("`%s` requires a name, id  or the \"--latest\" flag", cmd.CommandPath())
+	if len(args) > 1 {
+		return fmt.Errorf("`%s` accepts at most one argument", cmd.CommandPath())
+	}
+	if len(args) == 0 && !cmd.Flag("latest").Changed {
+		return fmt.Errorf("`%s` requires a name, id, or the \"--latest\" flag", cmd.CommandPath())
 	}
 	return nil
 }

test/system/015-help.bats

@@ -25,7 +25,7 @@ function podman_commands() {
 
 function check_help() {
     local count=0
-    local subcommands_found=0
+    local -A found
 
     for cmd in $(podman_commands "$@"); do
         # Human-readable podman command string, with multiple spaces collapsed
@@ -44,24 +44,37 @@ function check_help() {
 
         # If usage ends in '[command]', recurse into subcommands
         if expr "$usage" : '.*\[command\]$' >/dev/null; then
-            subcommands_found=$(expr $subcommands_found + 1)
+            found[subcommands]=1
             check_help "$@" $cmd
             continue
         fi
 
-        # If usage ends in '[flag]', command takes no more arguments.
-        # Confirm that by running with 'invalid-arg' and expecting failure.
-        if expr "$usage" : '.*\[flags\]$' >/dev/null; then
+        # We had someone write upper-case '[FLAGS]' once. Prevent it.
+        if expr "$usage" : '.*\[FLAG' >/dev/null; then
+            die "'flags' string must be lower-case in usage: $usage"
+        fi
+
+        # We had someone do 'podman foo ARG [flags]' one time. Yeah, no.
+        if expr "$usage" : '.*[A-Z].*\[flag' >/dev/null; then
+            die "'flags' must precede arguments in usage: $usage"
+        fi
+
+        # If usage lists no arguments (strings in ALL CAPS), confirm
+        # by running with 'invalid-arg' and expecting failure.
+        if ! expr "$usage" : '.*[A-Z]' >/dev/null; then
             if [ "$cmd" != "help" ]; then
                 dprint "$command_string invalid-arg"
                 run_podman 125 "$@" $cmd invalid-arg
                 is "$output" "Error: .* takes no arguments" \
                    "'$command_string' with extra (invalid) arguments"
             fi
+            found[takes_no_args]=1
         fi
 
-        # If usage has required arguments, try running without them
-        if expr "$usage" : '.*\[flags\] [A-Z]' >/dev/null; then
+        # If usage has required arguments, try running without them.
+        # The expression here is 'first capital letter is not in [BRACKETS]'.
+        # It is intended to handle 'podman foo [flags] ARG' but not ' [ARG]'.
+        if expr "$usage" : '[^A-Z]\+ [A-Z]' >/dev/null; then
             # Exceptions: these commands don't work rootless
             if is_rootless; then
                 # "pause is not supported for rootless containers"
@@ -80,6 +93,31 @@ function check_help() {
             run_podman 125 "$@" $cmd </dev/null
             is "$output" "Error:.* \(require\|specif\|must\|provide\|need\|choose\|accepts\)" \
                "'$command_string' without required arg"
+
+            found[required_args]=1
+        fi
+
+        # Commands with fixed number of arguments (i.e. no ellipsis): count
+        # the required args, then invoke with one extra. We should get a
+        # usage error.
+        if ! expr "$usage" : ".*\.\.\."; then
+            # "podman help" can take infinite args, so skip that one
+            if [ "$cmd" != "help" ]; then
+                # Get the args part of the command line; this should be
+                # everything from the first CAPITAL LETTER onward. We
+                # don't actually care about the letter itself, so just
+                # make it 'X'. And we don't care about [OPTIONAL] brackets
+                # either. What we do care about is stuff like 'IMAGE | CTR'
+                # which is actually one argument; convert to 'IMAGE-or-CTR'
+                local rhs=$(sed -e 's/^[^A-Z]\+[A-Z]/X/' -e 's/ | /-or-/g' <<<"$usage")
+                local n_args=$(wc -w <<<"$rhs")
+
+                run_podman 125 "$@" $cmd $(seq --format='x%g' 0 $n_args)
+                is "$output" "Error:.* \(takes no arguments\|requires exactly $n_args arg\|accepts at most\|too many arguments\|accepts $n_args arg(s), received\|accepts between .* and .* arg(s), received \)" \
+                   "'$command_string' with >$n_args arguments"
+
+                found[fixed_args]=1
+            fi
         fi
 
         count=$(expr $count + 1)
@@ -101,9 +139,14 @@ function check_help() {
     [ $count -gt 0 ] || \
         die "Internal error: no commands found in 'podman help $@' list"
 
-    # At least the top level must have some subcommands
-    if [ -z "$*" -a $subcommands_found -eq 0 ]; then
-        die "Internal error: did not find any podman subcommands"
+    # Sanity check: make sure the special loops above triggered at least once.
+    # (We've had situations where a typo makes the conditional never run)
+    if [ -z "$*" ]; then
+        for i in subcommands required_args takes_no_args fixed_args; do
+            if [[ -z ${found[$i]} ]]; then
+                die "Internal error: '$i' subtest did not trigger"
+            fi
+        done
     fi
 }
 

test/system/120-load.bats

@@ -107,10 +107,4 @@ verify_iid_and_name() {
        "Diagnostic from 'podman load' without redirection or -i"
 }
 
-@test "podman load - at most 1 arg(s)" {
-    run_podman 125 load 1 2 3
-    is "$output" \
-       "Error: accepts at most 1 arg(s), received 3"
-}
-
 # vim: filetype=sh