refactor(internal/cli): rewrite LookupCommand iteratively (#2137)

Rewrite LookupCommand iteratively by replace recursive calls with a for
loop.

Additionally, update tests to expect nil for empty remaining arguments,
and merge the lookup function logic directly into LookupCommand.

Author: julieqiu

internal/cli/cli.go

@@ -111,37 +111,29 @@ func hasFlags(fs *flag.FlagSet) bool {
 	return visited
 }
 
-// LookupCommand recursively looks up the command specified by the given arguments.
+// LookupCommand looks up the command specified by the given arguments.
 // It returns the command, the remaining arguments, and an error if the command
 // is not found.
 func LookupCommand(cmd *Command, args []string) (*Command, []string, error) {
-	if len(args) == 0 {
-		return cmd, nil, nil
-	}
-	subcommand, err := lookup(cmd, args[0])
-	if err != nil {
-		cmd.Flags.Usage()
-		return nil, nil, err
-	}
-	// If the next argument matches a potential flag (first char is `-`), parse the
-	// remaining arguments as flags. Check if argument is a flag before calling
-	// `lookupCommand` again to avoid flags from being treated as subcommands.
-	if len(args) > 1 && args[1][0] == '-' {
-		return subcommand, args[1:], nil
-	}
-	if len(subcommand.Commands) > 0 {
-		return LookupCommand(subcommand, args[1:])
-	}
-	return subcommand, args[1:], nil
-}
-
-// lookup finds a command by its name, and returns an error if the command is
-// not found.
-func lookup(c *Command, name string) (*Command, error) {
-	for _, sub := range c.Commands {
-		if sub.Name() == name {
-			return sub, nil
+	// If the first character of the argument is '-' assume it is a flag.
+	for len(args) > 0 && args[0][0] != '-' {
+		name := args[0]
+		var found *Command
+		for _, sub := range cmd.Commands {
+			if sub.Name() == name {
+				found = sub
+				break
+			}
+		}
+		if found == nil {
+			if len(cmd.Commands) == 0 {
+				break
+			}
+			cmd.Flags.Usage()
+			return nil, nil, fmt.Errorf("invalid command: %q", name)
 		}
+		cmd = found
+		args = args[1:]
 	}
-	return nil, fmt.Errorf("invalid command: %q", name)
+	return cmd, args, nil
 }

internal/cli/cli_test.go

@@ -22,6 +22,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/googleapis/librarian/internal/config"
 )
 
@@ -53,41 +54,6 @@ func TestParseAndSetFlags(t *testing.T) {
 	}
 }
 
-func TestLookup(t *testing.T) {
-	commands := []*Command{
-		{Short: "foo runs the foo command"},
-		{Short: "bar runs the bar command"},
-	}
-
-	for _, test := range []struct {
-		name    string
-		wantErr bool
-	}{
-		{"foo", false},
-		{"bar", false},
-		{"baz", true}, // not found case
-	} {
-		t.Run(test.name, func(t *testing.T) {
-			cmd := &Command{}
-			cmd.Commands = commands
-			sub, err := lookup(cmd, test.name)
-			if test.wantErr {
-				if err == nil {
-					t.Fatal(err)
-				}
-				return
-			}
-
-			if err != nil {
-				t.Fatal(err)
-			}
-			if sub.Name() != test.name {
-				t.Errorf("got = %q, want = %q", sub.Name(), test.name)
-			}
-		})
-	}
-}
-
 func TestRun(t *testing.T) {
 	executed := false
 	cmd := &Command{
@@ -250,7 +216,6 @@ func TestLookupCommand(t *testing.T) {
 		{
 			name:    "no args",
 			cmd:     root,
-			args:    []string{},
 			wantCmd: root,
 		},
 		{
@@ -260,18 +225,16 @@ func TestLookupCommand(t *testing.T) {
 			wantCmd: sub1,
 		},
 		{
-			name:     "find sub2",
-			cmd:      root,
-			args:     []string{"sub2"},
-			wantCmd:  sub2,
-			wantArgs: []string{},
+			name:    "find sub2",
+			cmd:     root,
+			args:    []string{"sub2"},
+			wantCmd: sub2,
 		},
 		{
-			name:     "find sub1sub1",
-			cmd:      root,
-			args:     []string{"sub1", "sub1sub1"},
-			wantCmd:  sub1sub1,
-			wantArgs: []string{},
+			name:    "find sub1sub1",
+			cmd:     root,
+			args:    []string{"sub1", "sub1sub1"},
+			wantCmd: sub1sub1,
 		},
 		{
 			name:     "find sub1sub1 with args",
@@ -317,8 +280,7 @@ func TestLookupCommand(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			gotCmd, gotArgs, err := LookupCommand(test.cmd, test.args)
 			if (err != nil) != test.wantErr {
-				t.Errorf("lookupCommand() error = %v, wantErr %v", err, test.wantErr)
-				return
+				t.Fatalf("error = %v, wantErr %v", err, test.wantErr)
 			}
 			if gotCmd != test.wantCmd {
 				var gotName, wantName string
@@ -328,10 +290,10 @@ func TestLookupCommand(t *testing.T) {
 				if test.wantCmd != nil {
 					wantName = test.wantCmd.Name()
 				}
-				t.Errorf("lookupCommand() gotCmd.Name() = %q, want %q", gotName, wantName)
+				t.Errorf("gotCmd.Name() = %q, want %q", gotName, wantName)
 			}
-			if diff := cmp.Diff(test.wantArgs, gotArgs); diff != "" {
-				t.Errorf("lookupCommand() args mismatch (-want +got):\n%s", diff)
+			if diff := cmp.Diff(test.wantArgs, gotArgs, cmpopts.EquateEmpty()); diff != "" {
+				t.Errorf("mismatch (-want +got):\n%s", diff)
 			}
 		})
 	}