Repository: cli/cli
Pull request #680 (2020-03-19 15:53:49)

Creating a PR now always prioritizes an existing fork as a push target
Before: the default push target for the current branch in `pr create` was **the first repository found among git remotes that I have write access to**, even if it is the same as the base repository.

Now: the default push target is **the fork of the base repo, if said fork exists** and I have write access to it (even if it's not listed among git remotes), falling back to old behavior otherwise.

This change in the default is to facilitate contributions to projects that have a hard requirement that all pull requests (even those opened by people with write access to the project) always come from forks. If a fork exists, it will automatically get used as a push target.

Fixes https://github.com/cli/cli/issues/350


Patch:
diff --git a/api/queries_repo.go b/api/queries_repo.go
index e3d33b98a..5a5061eda 100644
--- a/api/queries_repo.go
+++ b/api/queries_repo.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -224,6 +225,49 @@ func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) {
 	}, nil
 }
 
+// RepoFindFork finds a fork of repo affiliated with the viewer
+func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) {
+	result := struct {
+		Repository struct {
+			Forks struct {
+				Nodes []Repository
+			}
+		}
+	}{}
+
+	variables := map[string]interface{}{
+		"owner": repo.RepoOwner(),
+		"repo":  repo.RepoName(),
+	}
+
+	if err := client.GraphQL(`
+	query($owner: String!, $repo: String!) {
+		repository(owner: $owner, name: $repo) {
+			forks(first: 1, affiliations: [OWNER, COLLABORATOR]) {
+				nodes {
+					id
+					name
+					owner { login }
+					url
+					viewerPermission
+				}
+			}
+		}
+	}
+	`, variables, &result); err != nil {
+		return nil, err
+	}
+
+	forks := result.Repository.Forks.Nodes
+	// we check ViewerCanPush, even though we expect it to always be true per
+	// `affiliations` condition, to guard against versions of GitHub with a
+	// faulty `affiliations` implementation
+	if len(forks) > 0 && forks[0].ViewerCanPush() {
+		return &forks[0], nil
+	}
+	return nil, &NotFoundError{errors.New("no fork found")}
+}
+
 // RepoCreateInput represents input parameters for RepoCreate
 type RepoCreateInput struct {
 	Name        string `json:"name"`
diff --git a/command/pr_create.go b/command/pr_create.go
index 408145348..238057bf8 100644
--- a/command/pr_create.go
+++ b/command/pr_create.go
@@ -225,11 +225,25 @@ func prCreate(cmd *cobra.Command, _ []string) error {
 			return fmt.Errorf("error forking repo: %w", err)
 		}
 		didForkRepo = true
+	}
+
+	headBranchLabel := headBranch
+	if !ghrepo.IsSame(baseRepo, headRepo) {
+		headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch)
+	}
+
+	// There are two cases when an existing remote for the head repo will be
+	// missing:
+	// 1. the head repo was just created by auto-forking;
+	// 2. an existing fork was discovered by quering the API.
+	//
+	// In either case, we want to add the head repo as a new git remote so we
+	// can push to it.
+	if err != nil {
 		// TODO: support non-HTTPS git remote URLs
-		baseRepoURL := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(baseRepo))
 		headRepoURL := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(headRepo))
-		// TODO: figure out what to name the new git remote
-		gitRemote, err := git.AddRemote("fork", baseRepoURL, headRepoURL)
+		// TODO: prevent clashes with another remote of a same name
+		gitRemote, err := git.AddRemote("fork", headRepoURL)
 		if err != nil {
 			return fmt.Errorf("error adding remote: %w", err)
 		}
@@ -240,11 +254,6 @@ func prCreate(cmd *cobra.Command, _ []string) error {
 		}
 	}
 
-	headBranchLabel := headBranch
-	if !ghrepo.IsSame(baseRepo, headRepo) {
-		headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch)
-	}
-
 	// automatically push the branch if it hasn't been pushed anywhere yet
 	if headBranchPushedTo == nil {
 		if headRemote == nil {
diff --git a/command/repo.go b/command/repo.go
index 701f8cfc1..e4899cf4c 100644
--- a/command/repo.go
+++ b/command/repo.go
@@ -347,7 +347,7 @@ func repoFork(cmd *cobra.Command, args []string) error {
 			}
 		}
 		if remoteDesired {
-			_, err := git.AddRemote("fork", forkedRepo.CloneURL, "")
+			_, err := git.AddRemote("fork", forkedRepo.CloneURL)
 			if err != nil {
 				return fmt.Errorf("failed to add remote: %w", err)
 			}
diff --git a/context/context.go b/context/context.go
index 6c3bd5aee..27744a08a 100644
--- a/context/context.go
+++ b/context/context.go
@@ -51,7 +51,10 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re
 		repos = append(repos, baseOverride)
 	}
 
-	result := ResolvedRemotes{Remotes: remotes}
+	result := ResolvedRemotes{
+		Remotes:   remotes,
+		apiClient: client,
+	}
 	if hasBaseOverride {
 		result.BaseOverride = baseOverride
 	}
@@ -67,6 +70,7 @@ type ResolvedRemotes struct {
 	BaseOverride ghrepo.Interface
 	Remotes      Remotes
 	Network      api.RepoNetworkResult
+	apiClient    *api.Client
 }
 
 // BaseRepo is the first found repository in the "upstream", "github", "origin"
@@ -95,8 +99,30 @@ func (r ResolvedRemotes) BaseRepo() (*api.Repository, error) {
 	return nil, errors.New("not found")
 }
 
-// HeadRepo is the first found repository that has push access
+// 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
+	}
+
+	// try to find a pushable fork among existing remotes
+	for _, repo := range r.Network.Repositories {
+		if repo != nil && repo.Parent != nil && repo.ViewerCanPush() && ghrepo.IsSame(repo.Parent, baseRepo) {
+			return repo, nil
+		}
+	}
+
+	// a fork might still exist on GitHub, so let's query for it
+	var notFound *api.NotFoundError
+	if repo, err := api.RepoFindFork(r.apiClient, baseRepo); err == nil {
+		return repo, nil
+	} else if !errors.As(err, &notFound) {
+		return nil, err
+	}
+
+	// fall back to any listed repository that has push access
 	for _, repo := range r.Network.Repositories {
 		if repo != nil && repo.ViewerCanPush() {
 			return repo, nil
diff --git a/git/remote.go b/git/remote.go
index f98606a2e..9a8b3fccb 100644
--- a/git/remote.go
+++ b/git/remote.go
@@ -71,34 +71,22 @@ func parseRemotes(gitRemotes []string) (remotes RemoteSet) {
 	return
 }
 
-// AddRemote adds a new git remote. The initURL is the remote URL with which the
-// automatic fetch is made and finalURL, if non-blank, is set as the remote URL
-// after the fetch.
-func AddRemote(name, initURL, finalURL string) (*Remote, error) {
-	addCmd := exec.Command("git", "remote", "add", "-f", name, initURL)
+// AddRemote adds a new git remote and auto-fetches objects from it
+func AddRemote(name, u string) (*Remote, error) {
+	addCmd := exec.Command("git", "remote", "add", "-f", name, u)
 	err := utils.PrepareCmd(addCmd).Run()
 	if err != nil {
 		return nil, err
 	}
 
-	if finalURL == "" {
-		finalURL = initURL
-	} else {
-		setCmd := exec.Command("git", "remote", "set-url", name, finalURL)
-		err := utils.PrepareCmd(setCmd).Run()
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	finalURLParsed, err := url.Parse(finalURL)
+	urlParsed, err := url.Parse(u)
 	if err != nil {
 		return nil, err
 	}
 
 	return &Remote{
 		Name:     name,
-		FetchURL: finalURLParsed,
-		PushURL:  finalURLParsed,
+		FetchURL: urlParsed,
+		PushURL:  urlParsed,
 	}, nil
 }


Test patch:
diff --git a/command/pr_create_test.go b/command/pr_create_test.go
index a91dd0744..01b5516d3 100644
--- a/command/pr_create_test.go
+++ b/command/pr_create_test.go
@@ -15,6 +15,10 @@ func TestPRCreate(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes" : [
 		] } } } }
@@ -37,7 +41,7 @@ func TestPRCreate(t *testing.T) {
 	output, err := RunCommand(prCreateCmd, `pr create -t "my title" -b "my body"`)
 	eq(t, err, nil)
 
-	bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
+	bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body)
 	reqBody := struct {
 		Variables struct {
 			Input struct {
@@ -64,6 +68,10 @@ func TestPRCreate_alreadyExists(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes": [
 			{ "url": "https://github.com/OWNER/REPO/pull/123",
@@ -93,6 +101,10 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes": [
 			{ "url": "https://github.com/OWNER/REPO/pull/123",
@@ -121,6 +133,10 @@ func TestPRCreate_web(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 
 	cs, cmdTeardown := initCmdStubber()
 	defer cmdTeardown()
@@ -149,6 +165,10 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) {
 	http := initFakeHTTP()
 
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes" : [
 		] } } } }
@@ -272,6 +292,10 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) {
 	initBlankContext("OWNER/REPO", "cool_bug-fixes")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes" : [
 		] } } } }
@@ -315,7 +339,7 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) {
 	output, err := RunCommand(prCreateCmd, `pr create`)
 	eq(t, err, nil)
 
-	bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
+	bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body)
 	reqBody := struct {
 		Variables struct {
 			Input struct {
@@ -344,6 +368,10 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes" : [
 		] } } } }
@@ -388,7 +416,7 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) {
 	output, err := RunCommand(prCreateCmd, `pr create`)
 	eq(t, err, nil)
 
-	bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
+	bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body)
 	reqBody := struct {
 		Variables struct {
 			Input struct {
@@ -417,6 +445,10 @@ func TestPRCreate_survey_autofill(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "repository": { "pullRequests": { "nodes" : [
 		] } } } }
@@ -442,7 +474,7 @@ func TestPRCreate_survey_autofill(t *testing.T) {
 	output, err := RunCommand(prCreateCmd, `pr create -f`)
 	eq(t, err, nil)
 
-	bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body)
+	bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body)
 	reqBody := struct {
 		Variables struct {
 			Input struct {
@@ -507,6 +539,10 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) {
 	initBlankContext("OWNER/REPO", "feature")
 	http := initFakeHTTP()
 	http.StubRepoResponse("OWNER", "REPO")
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
 	http.StubResponse(200, bytes.NewBufferString(`
 		{ "data": { "createPullRequest": { "pullRequest": {
 			"URL": "https://github.com/OWNER/REPO/pull/12"
diff --git a/context/remote_test.go b/context/remote_test.go
index 04ccbc56c..5c3c46bc7 100644
--- a/context/remote_test.go
+++ b/context/remote_test.go
@@ -1,6 +1,7 @@
 package context
 
 import (
+	"bytes"
 	"errors"
 	"net/url"
 	"testing"
@@ -61,6 +62,14 @@ func Test_translateRemotes(t *testing.T) {
 }
 
 func Test_resolvedRemotes_triangularSetup(t *testing.T) {
+	http := &api.FakeHTTP{}
+	apiClient := api.NewClient(api.ReplaceTripper(http))
+
+	http.StubResponse(200, bytes.NewBufferString(`
+	{ "data": { "repository": { "forks": { "nodes": [
+	] } } } }
+	`))
+
 	resolved := ResolvedRemotes{
 		BaseOverride: nil,
 		Remotes: Remotes{
@@ -89,6 +98,7 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) {
 				},
 			},
 		},
+		apiClient: apiClient,
 	}
 
 	baseRepo, err := resolved.BaseRepo()
@@ -118,6 +128,53 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) {
 	}
 }
 
+func Test_resolvedRemotes_forkLookup(t *testing.T) {
+	http := &api.FakeHTTP{}
+	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"},
+				Owner:  "OWNER",
+				Repo:   "REPO",
+			},
+		},
+		Network: api.RepoNetworkResult{
+			Repositories: []*api.Repository{
+				&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,