remoteResolver does not cache remotes #10103

Description

While I was reading the code 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:

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:

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.

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