Change how base repository is resolved

On first run in a git repository, `BaseRepo()` will now prompt the user
which repository should be queried as base repository if there are
multiple git remotes or when we are in the context of a fork.

In non-interactive mode, the prompt is skipped and we default to the
first git remote instead.

After the base repo is resolved, the result is cached in the local
repository using `git config` so that RepositoryNetwork API lookups can
be avoided in the future.

context/context.go

@@ -1,26 +1,27 @@
+// TODO: rename this package to avoid clash with stdlib
 package context
 
 import (
 	"errors"
-	"fmt"
 	"sort"
-	"strings"
 
+	"github.com/AlecAivazis/survey/v2"
 	"github.com/cli/cli/api"
+	"github.com/cli/cli/git"
 	"github.com/cli/cli/internal/ghrepo"
+	"github.com/cli/cli/pkg/iostreams"
+	"github.com/cli/cli/pkg/prompt"
 )
 
 // cap the number of git remotes looked up, since the user might have an
 // unusually large number of git remotes
 const maxRemotesForLookup = 5
 
-// ResolveRemotesToRepos takes in a list of git remotes and fetches more information about the repositories they map to.
-// Only the git remotes belonging to the same hostname are ever looked up; all others are ignored.
-func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (ResolvedRemotes, error) {
+func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (*ResolvedRemotes, error) {
 	sort.Stable(remotes)
 
-	result := ResolvedRemotes{
-		Remotes:   remotes,
+	result := &ResolvedRemotes{
+		remotes:   remotes,
 		apiClient: client,
 	}
 
@@ -31,84 +32,123 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re
 		if err != nil {
 			return result, err
 		}
-		result.BaseOverride = baseOverride
+		result.baseOverride = baseOverride
 	}
 
-	foundBaseOverride := false
-	var hostname string
+	return result, nil
+}
+
+func resolveNetwork(result *ResolvedRemotes) error {
 	var repos []ghrepo.Interface
-	for i, r := range remotes {
-		if i == 0 {
-			hostname = r.RepoHost()
-		} else if !strings.EqualFold(r.RepoHost(), hostname) {
-			// ignore all remotes for a hostname different to that of the 1st remote
-			continue
-		}
+	for _, r := range result.remotes {
 		repos = append(repos, r)
-		if baseOverride != nil && ghrepo.IsSame(r, baseOverride) {
-			foundBaseOverride = true
-		}
 		if len(repos) == maxRemotesForLookup {
 			break
 		}
 	}
-	if baseOverride != nil && !foundBaseOverride {
-		// additionally, look up the explicitly specified base repo if it's not
-		// already covered by git remotes
-		repos = append(repos, baseOverride)
-	}
 
-	networkResult, err := api.RepoNetwork(client, repos)
-	if err != nil {
-		return result, err
-	}
-	result.Network = networkResult
-	return result, nil
+	networkResult, err := api.RepoNetwork(result.apiClient, repos)
+	result.network = &networkResult
+	return err
 }
 
 type ResolvedRemotes struct {
-	BaseOverride ghrepo.Interface
-	Remotes      Remotes
-	Network      api.RepoNetworkResult
+	baseOverride ghrepo.Interface
+	remotes      Remotes
+	network      *api.RepoNetworkResult
 	apiClient    *api.Client
 }
 
-// BaseRepo is the first found repository in the "upstream", "github", "origin"
-// git remote order, resolved to the parent repo if the git remote points to a fork
-func (r ResolvedRemotes) BaseRepo() (*api.Repository, error) {
-	if r.BaseOverride != nil {
-		for _, repo := range r.Network.Repositories {
-			if repo != nil && ghrepo.IsSame(repo, r.BaseOverride) {
-				return repo, nil
-			}
-		}
-		return nil, fmt.Errorf("failed looking up information about the '%s' repository",
-			ghrepo.FullName(r.BaseOverride))
+func (r *ResolvedRemotes) BaseRepo(io *iostreams.IOStreams) (ghrepo.Interface, error) {
+	if r.baseOverride != nil {
+		return r.baseOverride, nil
 	}
 
-	for _, repo := range r.Network.Repositories {
+	// if any of the remotes already has a resolution, respect that
+	for _, r := range r.remotes {
+		if r.Resolved == "base" {
+			return r, nil
+		} else if r.Resolved != "" {
+			repo, err := ghrepo.FromFullName(r.Resolved)
+			if err != nil {
+				return nil, err
+			}
+			return ghrepo.NewWithHost(repo.RepoOwner(), repo.RepoName(), r.RepoHost()), nil
+		}
+	}
+
+	if !io.CanPrompt() {
+		// we cannot prompt, so just resort to the 1st remote
+		return r.remotes[0], nil
+	}
+
+	// from here on, consult the API
+	if r.network == nil {
+		err := resolveNetwork(r)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	var repoNames []string
+	repoMap := map[string]*api.Repository{}
+	add := func(r *api.Repository) {
+		fn := ghrepo.FullName(r)
+		if _, ok := repoMap[fn]; !ok {
+			repoMap[fn] = r
+			repoNames = append(repoNames, fn)
+		}
+	}
+
+	for _, repo := range r.network.Repositories {
 		if repo == nil {
 			continue
 		}
 		if repo.IsFork() {
-			return repo.Parent, nil
+			add(repo.Parent)
 		}
-		return repo, nil
+		add(repo)
 	}
 
-	return nil, errors.New("not found")
+	if len(repoNames) == 0 {
+		return r.remotes[0], nil
+	}
+
+	baseName := repoNames[0]
+	if len(repoNames) > 1 {
+		err := prompt.SurveyAskOne(&survey.Select{
+			Message: "Which should be the base repository (used for e.g. querying issues) for this directory?",
+			Options: repoNames,
+		}, &baseName)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// determine corresponding git remote
+	selectedRepo := repoMap[baseName]
+	resolution := "base"
+	remote, _ := r.RemoteForRepo(selectedRepo)
+	if remote == nil {
+		remote = r.remotes[0]
+		resolution = ghrepo.FullName(selectedRepo)
+	}
+
+	// cache the result to git config
+	err := git.SetRemoteResolution(remote.Name, resolution)
+	return selectedRepo, err
 }
 
-// HeadRepo is a fork of base repo (if any), or the first found repository that
-// has push access
-func (r ResolvedRemotes) HeadRepo() (*api.Repository, error) {
-	baseRepo, err := r.BaseRepo()
-	if err != nil {
-		return nil, err
+func (r *ResolvedRemotes) HeadRepo(baseRepo ghrepo.Interface) (ghrepo.Interface, error) {
+	if r.network == nil {
+		err := resolveNetwork(r)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// try to find a pushable fork among existing remotes
-	for _, repo := range r.Network.Repositories {
+	for _, repo := range r.network.Repositories {
 		if repo != nil && repo.Parent != nil && repo.ViewerCanPush() && ghrepo.IsSame(repo.Parent, baseRepo) {
 			return repo, nil
 		}
@@ -123,7 +163,7 @@ func (r ResolvedRemotes) HeadRepo() (*api.Repository, error) {
 	}
 
 	// fall back to any listed repository that has push access
-	for _, repo := range r.Network.Repositories {
+	for _, repo := range r.network.Repositories {
 		if repo != nil && repo.ViewerCanPush() {
 			return repo, nil
 		}
@@ -132,12 +172,9 @@ func (r ResolvedRemotes) HeadRepo() (*api.Repository, error) {
 }
 
 // RemoteForRepo finds the git remote that points to a repository
-func (r ResolvedRemotes) RemoteForRepo(repo ghrepo.Interface) (*Remote, error) {
-	for i, remote := range r.Remotes {
-		if ghrepo.IsSame(remote, repo) ||
-			// additionally, look up the resolved repository name in case this
-			// git remote points to this repository via a redirect
-			(r.Network.Repositories[i] != nil && ghrepo.IsSame(r.Network.Repositories[i], repo)) {
+func (r *ResolvedRemotes) RemoteForRepo(repo ghrepo.Interface) (*Remote, error) {
+	for _, remote := range r.remotes {
+		if ghrepo.IsSame(remote, repo) {
 			return remote, nil
 		}
 	}

context/remote_test.go

@@ -1,16 +1,13 @@
 package context
 
 import (
-	"bytes"
 	"errors"
 	"net/url"
 	"reflect"
 	"testing"
 
-	"github.com/cli/cli/api"
 	"github.com/cli/cli/git"
 	"github.com/cli/cli/internal/ghrepo"
-	"github.com/cli/cli/pkg/httpmock"
 )
 
 func eq(t *testing.T, got interface{}, expected interface{}) {
@@ -69,163 +66,3 @@ func Test_translateRemotes(t *testing.T) {
 		t.Errorf("got %q", result[0].RepoName())
 	}
 }
-
-func Test_resolvedRemotes_triangularSetup(t *testing.T) {
-	http := &httpmock.Registry{}
-	apiClient := api.NewClient(api.ReplaceTripper(http))
-
-	http.StubResponse(200, bytes.NewBufferString(`
-	{ "data": { "repository": { "forks": { "nodes": [
-	] } } } }
-	`))
-
-	resolved := ResolvedRemotes{
-		BaseOverride: nil,
-		Remotes: Remotes{
-			&Remote{
-				Remote: &git.Remote{Name: "origin"},
-				Repo:   ghrepo.New("OWNER", "REPO"),
-			},
-			&Remote{
-				Remote: &git.Remote{Name: "fork"},
-				Repo:   ghrepo.New("MYSELF", "REPO"),
-			},
-		},
-		Network: api.RepoNetworkResult{
-			Repositories: []*api.Repository{
-				{
-					Name:             "NEWNAME",
-					Owner:            api.RepositoryOwner{Login: "NEWOWNER"},
-					ViewerPermission: "READ",
-				},
-				{
-					Name:             "REPO",
-					Owner:            api.RepositoryOwner{Login: "MYSELF"},
-					ViewerPermission: "ADMIN",
-				},
-			},
-		},
-		apiClient: apiClient,
-	}
-
-	baseRepo, err := resolved.BaseRepo()
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	eq(t, ghrepo.FullName(baseRepo), "NEWOWNER/NEWNAME")
-	baseRemote, err := resolved.RemoteForRepo(baseRepo)
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	if baseRemote.Name != "origin" {
-		t.Errorf("got remote %q", baseRemote.Name)
-	}
-
-	headRepo, err := resolved.HeadRepo()
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	eq(t, ghrepo.FullName(headRepo), "MYSELF/REPO")
-	headRemote, err := resolved.RemoteForRepo(headRepo)
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	if headRemote.Name != "fork" {
-		t.Errorf("got remote %q", headRemote.Name)
-	}
-}
-
-func Test_resolvedRemotes_forkLookup(t *testing.T) {
-	http := &httpmock.Registry{}
-	apiClient := api.NewClient(api.ReplaceTripper(http))
-
-	http.StubResponse(200, bytes.NewBufferString(`
-	{ "data": { "repository": { "forks": { "nodes": [
-		{ "id": "FORKID",
-		  "url": "https://github.com/FORKOWNER/REPO",
-		  "name": "REPO",
-		  "owner": { "login": "FORKOWNER" },
-		  "viewerPermission": "WRITE"
-		}
-	] } } } }
-	`))
-
-	resolved := ResolvedRemotes{
-		BaseOverride: nil,
-		Remotes: Remotes{
-			&Remote{
-				Remote: &git.Remote{Name: "origin"},
-				Repo:   ghrepo.New("OWNER", "REPO"),
-			},
-		},
-		Network: api.RepoNetworkResult{
-			Repositories: []*api.Repository{
-				{
-					Name:             "NEWNAME",
-					Owner:            api.RepositoryOwner{Login: "NEWOWNER"},
-					ViewerPermission: "READ",
-				},
-			},
-		},
-		apiClient: apiClient,
-	}
-
-	headRepo, err := resolved.HeadRepo()
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	eq(t, ghrepo.FullName(headRepo), "FORKOWNER/REPO")
-	_, err = resolved.RemoteForRepo(headRepo)
-	if err == nil {
-		t.Fatal("expected to not find a matching remote")
-	}
-}
-
-func Test_resolvedRemotes_clonedFork(t *testing.T) {
-	resolved := ResolvedRemotes{
-		BaseOverride: nil,
-		Remotes: Remotes{
-			&Remote{
-				Remote: &git.Remote{Name: "origin"},
-				Repo:   ghrepo.New("OWNER", "REPO"),
-			},
-		},
-		Network: api.RepoNetworkResult{
-			Repositories: []*api.Repository{
-				{
-					Name:             "REPO",
-					Owner:            api.RepositoryOwner{Login: "OWNER"},
-					ViewerPermission: "ADMIN",
-					Parent: &api.Repository{
-						Name:             "REPO",
-						Owner:            api.RepositoryOwner{Login: "PARENTOWNER"},
-						ViewerPermission: "READ",
-					},
-				},
-			},
-		},
-	}
-
-	baseRepo, err := resolved.BaseRepo()
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	eq(t, ghrepo.FullName(baseRepo), "PARENTOWNER/REPO")
-	baseRemote, err := resolved.RemoteForRepo(baseRepo)
-	if baseRemote != nil || err == nil {
-		t.Error("did not expect any remote for base")
-	}
-
-	headRepo, err := resolved.HeadRepo()
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	eq(t, ghrepo.FullName(headRepo), "OWNER/REPO")
-	headRemote, err := resolved.RemoteForRepo(headRepo)
-	if err != nil {
-		t.Fatalf("got %v", err)
-	}
-	if headRemote.Name != "origin" {
-		t.Errorf("got remote %q", headRemote.Name)
-	}
-}

git/remote.go

@@ -1,6 +1,7 @@
 package git
 
 import (
+	"fmt"
 	"net/url"
 	"os/exec"
 	"regexp"
@@ -26,6 +27,7 @@ func NewRemote(name string, u string) *Remote {
 // Remote is a parsed git remote
 type Remote struct {
 	Name     string
+	Resolved string
 	FetchURL *url.URL
 	PushURL  *url.URL
 }
@@ -40,7 +42,30 @@ func Remotes() (RemoteSet, error) {
 	if err != nil {
 		return nil, err
 	}
-	return parseRemotes(list), nil
+	remotes := parseRemotes(list)
+
+	// this is affected by SetRemoteResolution
+	remoteCmd := exec.Command("git", "config", "--get-regexp", `^remote\..*\.gh-resolved$`)
+	output, _ := run.PrepareCmd(remoteCmd).Output()
+	for _, l := range outputLines(output) {
+		parts := strings.SplitN(l, " ", 2)
+		if len(parts) < 2 {
+			continue
+		}
+		rp := strings.SplitN(parts[0], ".", 3)
+		if len(rp) < 2 {
+			continue
+		}
+		name := rp[1]
+		for _, r := range remotes {
+			if r.Name == name {
+				r.Resolved = parts[1]
+				break
+			}
+		}
+	}
+
+	return remotes, nil
 }
 
 func parseRemotes(gitRemotes []string) (remotes RemoteSet) {
@@ -109,3 +134,8 @@ func AddRemote(name, u string) (*Remote, error) {
 		PushURL:  urlParsed,
 	}, nil
 }
+
+func SetRemoteResolution(name, resolution string) error {
+	addCmd := exec.Command("git", "config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution)
+	return run.PrepareCmd(addCmd).Run()
+}

pkg/cmd/pr/create/create.go

@@ -127,8 +127,18 @@ func createRun(opts *CreateOptions) error {
 		return err
 	}
 
-	baseRepo, err := repoContext.BaseRepo()
-	if err != nil {
+	var baseRepo *api.Repository
+	if br, err := repoContext.BaseRepo(opts.IO); err == nil {
+		if r, ok := br.(*api.Repository); ok {
+			baseRepo = r
+		} else {
+			var err error
+			baseRepo, err = api.GitHubRepo(client, br)
+			if err != nil {
+				return err
+			}
+		}
+	} else {
 		return fmt.Errorf("could not determine base repository: %w", err)
 	}
 
@@ -155,7 +165,7 @@ func createRun(opts *CreateOptions) error {
 
 	// otherwise, determine the head repository with info obtained from the API
 	if headRepo == nil {
-		if r, err := repoContext.HeadRepo(); err == nil {
+		if r, err := repoContext.HeadRepo(baseRepo); err == nil {
 			headRepo = r
 		}
 	}

pkg/cmd/root/root.go

@@ -134,7 +134,7 @@ func resolvedBaseRepo(f *cmdutil.Factory) func() (ghrepo.Interface, error) {
 		if err != nil {
 			return nil, err
 		}
-		baseRepo, err := repoContext.BaseRepo()
+		baseRepo, err := repoContext.BaseRepo(f.IOStreams)
 		if err != nil {
 			return nil, err
 		}