From a47327aee6396df90349e3b61a20da3d11c2ed6c Mon Sep 17 00:00:00 2001
From: William Martin <williammartin@github.com>
Date: Wed, 15 Jan 2025 14:11:24 +0100
Subject: [PATCH] Secret base repo prompting should not use resolved remote

This is because the secret commands don't use the SmartBaseRepo behaviour, and therefore
don't care about the resolved remote.
---
 pkg/cmd/secret/shared/base_repo.go      | 46 +++++++------------------
 pkg/cmd/secret/shared/base_repo_test.go |  8 ++---
 2 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go
index 652d92064..c46752794 100644
--- a/pkg/cmd/secret/shared/base_repo.go
+++ b/pkg/cmd/secret/shared/base_repo.go
@@ -28,10 +28,22 @@ func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Pr
 				return nil, err
 			}
 
-			baseRepo, err = promptForRepo(baseRepo, ambiguousBaseRepoErr.Remotes, prompter)
+			baseRepoOptions := make([]string, len(ambiguousBaseRepoErr.Remotes))
+			for i, remote := range ambiguousBaseRepoErr.Remotes {
+				baseRepoOptions[i] = ghrepo.FullName(remote)
+			}
+
+			selectedBaseRepo, err := prompter.Select("Select a base repo", baseRepoOptions[0], baseRepoOptions)
 			if err != nil {
 				return nil, err
 			}
+
+			selectedRepo, err := ghrepo.FromFullName(baseRepoOptions[selectedBaseRepo])
+			if err != nil {
+				return nil, err
+			}
+
+			return selectedRepo, nil
 		}
 
 		return baseRepo, nil
@@ -54,35 +66,3 @@ func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) base
 		return baseRepo()
 	}
 }
-
-// REVIEW WARNING: I have not had a close look at this function yet, I do not vouch for it.
-func promptForRepo(baseRepo ghrepo.Interface, remotes ghContext.Remotes, prompter prompter.Prompter) (ghrepo.Interface, error) {
-	var defaultRepo string
-	var remoteArray []string
-
-	// TODO: consider whether we should just go with the default order of remotes because then
-	// users that are familiar can just hit enter and achieve the behaviour they had before.
-	if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil {
-		// this is a remote explicitly chosen via `repo set-default`
-		defaultRepo = ghrepo.FullName(defaultRemote)
-	} else if len(remotes) > 0 {
-		// as a fallback, just pick the first remote
-		defaultRepo = ghrepo.FullName(remotes[0])
-	}
-
-	for _, remote := range remotes {
-		remoteArray = append(remoteArray, ghrepo.FullName(remote))
-	}
-
-	baseRepoInput, errInput := prompter.Select("Select a base repo", defaultRepo, remoteArray)
-	if errInput != nil {
-		return baseRepo, errInput
-	}
-
-	selectedRepo, errSelectedRepo := ghrepo.FromFullName(remoteArray[baseRepoInput])
-	if errSelectedRepo != nil {
-		return baseRepo, errSelectedRepo
-	}
-
-	return selectedRepo, nil
-}
diff --git a/pkg/cmd/secret/shared/base_repo_test.go b/pkg/cmd/secret/shared/base_repo_test.go
index 469a9722c..d5cdc5e23 100644
--- a/pkg/cmd/secret/shared/base_repo_test.go
+++ b/pkg/cmd/secret/shared/base_repo_test.go
@@ -102,7 +102,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) {
 		require.True(t, ghrepo.IsSame(ghrepo.New("owner", "repo"), baseRepo))
 	})
 
-	t.Run("when the wrapped base repo func returns a specific error, then the prompter is used for disambiguation, with the resolved remote as the default", func(t *testing.T) {
+	t.Run("when the wrapped base repo func returns a specific error, then the prompter is used for disambiguation, with the remote ordering remaining unchanged", func(t *testing.T) {
 		t.Parallel()
 
 		pm := prompter.NewMockPrompter(t)
@@ -110,8 +110,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) {
 			"Select a base repo",
 			[]string{"owner/fork", "owner/repo"},
 			func(_, def string, opts []string) (int, error) {
-				t.Helper()
-				require.Equal(t, "owner/repo", def)
+				require.Equal(t, "owner/fork", def)
 				return prompter.IndexFor(opts, "owner/repo")
 			},
 		)
@@ -179,8 +178,7 @@ func errMultipleRemotesStubFn() (ghrepo.Interface, error) {
 
 	remote2 := &ghContext.Remote{
 		Remote: &git.Remote{
-			Name:     "upstream",
-			Resolved: "base",
+			Name: "upstream",
 		},
 		Repo: ghrepo.New("owner", "repo"),
 	}