Repository: cli/cli (go)

Issue:
`remoteResolver` does not cache remotes
## Description

While I was reading the [code](https://github.com/cli/cli/blob/5402e207ee89f2f3dc52779c3edde632485074cd/pkg/cmd/factory/remote_resolver.go) for the `remoteResolver`, which is intended to call `git` to get the list of `remotes`, and then do some filtering and sorting, I noticed this line:

https://github.com/cli/cli/blob/5402e207ee89f2f3dc52779c3edde632485074cd/pkg/cmd/factory/remote_resolver.go#L72

I don't believe this works as intended, because the walrus operator results in `cachedRemotes` being shadowed since it is in an inner scope. See https://go.dev/play/p/YIENXfxFfbP for an example of shadowing.

I do believe on first read that errors are cached correctly:

https://github.com/cli/cli/blob/5402e207ee89f2f3dc52779c3edde632485074cd/pkg/cmd/factory/remote_resolver.go#L41

I also believe the code before this was caching correctly:

https://github.com/cli/cli/pull/1517/files#diff-cd947847f830f7a9e26366e15f74794df6937388a36f41146a1ba7d987d0efd7L96

---

Here's a minimal test to demonstrate that the cache branch isn't being hit in the happy path. If made into a real test, it should probably check that the correct remotes are returned as well.

```go
func TestRemoteResolverCachesRemotes(t *testing.T) {
	var readRemotesCalled bool

	rr := &remoteResolver{
		readRemotes: func() (git.RemoteSet, error) {
			if readRemotesCalled {
				return git.RemoteSet{}, errors.New("readRemotes should only be called once")
			}

			readRemotesCalled = true
			return git.RemoteSet{
				git.NewRemote("origin", "https://github.com/owner/repo.git"),
			}, nil
		},
		getConfig: func() (gh.Config, error) {
			cfg := &ghmock.ConfigMock{}
			cfg.AuthenticationFunc = func() gh.AuthConfig {
				authCfg := &config.AuthConfig{}
				authCfg.SetHosts([]string{"github.com"})
				authCfg.SetDefaultHost("github.com", "default")
				return authCfg
			}
			return cfg, nil
		},
		urlTranslator: identityTranslator{},
	}

	resolver := rr.Resolver()
	_, err := resolver()
	require.NoError(t, err)
	_, err = resolver()
	require.NoError(t, err)
}
```

### Expected Output

The expected output for this issue is that:
 * A comment is left confirming or refuting my statements above
 * A comment that indicates the author has confirmed that caching is in fact **safe** (this code has been this way for 4 years)
 * The code is fixed, if necessary
 * There are tests

Pull request:
Fix `remoteResolver` caching issue
Fixes #10103.

Patch:
diff --git a/pkg/cmd/factory/remote_resolver.go b/pkg/cmd/factory/remote_resolver.go
index b008ade7e..b5cad393e 100644
--- a/pkg/cmd/factory/remote_resolver.go
+++ b/pkg/cmd/factory/remote_resolver.go
@@ -21,25 +21,24 @@ type remoteResolver struct {
 	readRemotes   func() (git.RemoteSet, error)
 	getConfig     func() (gh.Config, error)
 	urlTranslator context.Translator
+	cachedRemotes context.Remotes
+	remotesError  error
 }
 
 func (rr *remoteResolver) Resolver() func() (context.Remotes, error) {
-	var cachedRemotes context.Remotes
-	var remotesError error
-
 	return func() (context.Remotes, error) {
-		if cachedRemotes != nil || remotesError != nil {
-			return cachedRemotes, remotesError
+		if rr.cachedRemotes != nil || rr.remotesError != nil {
+			return rr.cachedRemotes, rr.remotesError
 		}
 
 		gitRemotes, err := rr.readRemotes()
 		if err != nil {
-			remotesError = err
+			rr.remotesError = err
 			return nil, err
 		}
 		if len(gitRemotes) == 0 {
-			remotesError = errors.New("no git remotes found")
-			return nil, remotesError
+			rr.remotesError = errors.New("no git remotes found")
+			return nil, rr.remotesError
 		}
 
 		sshTranslate := rr.urlTranslator
@@ -68,30 +67,31 @@ func (rr *remoteResolver) Resolver() func() (context.Remotes, error) {
 		// Sort remotes
 		sort.Sort(resolvedRemotes)
 
-		// Filter remotes by hosts
-		// Note that this is not caching correctly: https://github.com/cli/cli/issues/10103
-		cachedRemotes := resolvedRemotes.FilterByHosts(hosts)
+		rr.cachedRemotes = resolvedRemotes.FilterByHosts(hosts)
 
 		// Filter again by default host if one is set
 		// For config file default host fallback to cachedRemotes if none match
 		// For environment default host (GH_HOST) do not fallback to cachedRemotes if none match
 		if src != "default" {
-			filteredRemotes := cachedRemotes.FilterByHosts([]string{defaultHost})
+			filteredRemotes := rr.cachedRemotes.FilterByHosts([]string{defaultHost})
 			if isHostEnv(src) || len(filteredRemotes) > 0 {
-				cachedRemotes = filteredRemotes
+				rr.cachedRemotes = filteredRemotes
 			}
 		}
 
-		if len(cachedRemotes) == 0 {
+		if len(rr.cachedRemotes) == 0 {
 			if isHostEnv(src) {
-				return nil, fmt.Errorf("none of the git remotes configured for this repository correspond to the %s environment variable. Try adding a matching remote or unsetting the variable.", src)
+				rr.remotesError = fmt.Errorf("none of the git remotes configured for this repository correspond to the %s environment variable. Try adding a matching remote or unsetting the variable", src)
+				return nil, rr.remotesError
 			} else if cfg.Authentication().HasEnvToken() {
-				return nil, errors.New("set the GH_HOST environment variable to specify which GitHub host to use")
+				rr.remotesError = errors.New("set the GH_HOST environment variable to specify which GitHub host to use")
+				return nil, rr.remotesError
 			}
-			return nil, errors.New("none of the git remotes configured for this repository point to a known GitHub host. To tell gh about a new GitHub host, please use `gh auth login`")
+			rr.remotesError = errors.New("none of the git remotes configured for this repository point to a known GitHub host. To tell gh about a new GitHub host, please use `gh auth login`")
+			return nil, rr.remotesError
 		}
 
-		return cachedRemotes, nil
+		return rr.cachedRemotes, nil
 	}
 }
 


Test patch:
diff --git a/pkg/cmd/factory/remote_resolver_test.go b/pkg/cmd/factory/remote_resolver_test.go
index 8d537826e..77d4a9d65 100644
--- a/pkg/cmd/factory/remote_resolver_test.go
+++ b/pkg/cmd/factory/remote_resolver_test.go
@@ -1,14 +1,17 @@
 package factory
 
 import (
+	"errors"
 	"net/url"
 	"testing"
 
+	"github.com/cli/cli/v2/context"
 	"github.com/cli/cli/v2/git"
 	"github.com/cli/cli/v2/internal/config"
 	"github.com/cli/cli/v2/internal/gh"
 	ghmock "github.com/cli/cli/v2/internal/gh/mock"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type identityTranslator struct{}
@@ -288,3 +291,95 @@ func Test_remoteResolver(t *testing.T) {
 		})
 	}
 }
+
+func Test_remoteResolver_Caching(t *testing.T) {
+	t.Run("cache remotes", func(t *testing.T) {
+		var readRemotesCalled bool
+
+		rr := &remoteResolver{
+			readRemotes: func() (git.RemoteSet, error) {
+				if readRemotesCalled {
+					return git.RemoteSet{}, errors.New("readRemotes should only be called once")
+				}
+
+				readRemotesCalled = true
+				return git.RemoteSet{
+					git.NewRemote("origin", "https://github.com/owner/repo.git"),
+				}, nil
+			},
+			getConfig: func() (gh.Config, error) {
+				cfg := &ghmock.ConfigMock{}
+				cfg.AuthenticationFunc = func() gh.AuthConfig {
+					authCfg := &config.AuthConfig{}
+					authCfg.SetHosts([]string{"github.com"})
+					authCfg.SetDefaultHost("github.com", "default")
+					return authCfg
+				}
+				return cfg, nil
+			},
+			urlTranslator: identityTranslator{},
+		}
+
+		resolver := rr.Resolver()
+
+		expectedRemoteNames := []string{"origin"}
+		remotes, err := resolver()
+		require.NoError(t, err)
+		require.Equal(t, expectedRemoteNames, mapRemotesToNames(remotes))
+
+		require.Equal(t, readRemotesCalled, true)
+
+		cachedRemotes, err := resolver()
+		require.NoError(t, err, "expected no error to be cached")
+		require.Equal(t, expectedRemoteNames, mapRemotesToNames(cachedRemotes), "expected the remotes to be cached")
+	})
+
+	t.Run("cache error", func(t *testing.T) {
+		var readRemotesCalled bool
+
+		rr := &remoteResolver{
+			readRemotes: func() (git.RemoteSet, error) {
+				if readRemotesCalled {
+					return git.RemoteSet{
+						git.NewRemote("origin", "https://github.com/owner/repo.git"),
+					}, nil
+				}
+
+				readRemotesCalled = true
+				return git.RemoteSet{}, errors.New("error to be cached")
+			},
+			getConfig: func() (gh.Config, error) {
+				cfg := &ghmock.ConfigMock{}
+				cfg.AuthenticationFunc = func() gh.AuthConfig {
+					authCfg := &config.AuthConfig{}
+					authCfg.SetHosts([]string{"github.com"})
+					authCfg.SetDefaultHost("github.com", "default")
+					return authCfg
+				}
+				return cfg, nil
+			},
+			urlTranslator: identityTranslator{},
+		}
+
+		resolver := rr.Resolver()
+
+		expectedErr := errors.New("error to be cached")
+		remotes, err := resolver()
+		require.Equal(t, expectedErr, err)
+		require.Empty(t, remotes, "should return no remotes")
+
+		require.Equal(t, readRemotesCalled, true)
+
+		cachedRemotes, err := resolver()
+		require.Equal(t, expectedErr, err, "expected the error to be cached")
+		require.Empty(t, cachedRemotes, "should return no remotes")
+	})
+}
+
+func mapRemotesToNames(remotes context.Remotes) []string {
+	names := make([]string, len(remotes))
+	for i, r := range remotes {
+		names[i] = r.Name
+	}
+	return names
+}