From d75faab85a7b5fae4353046207cc95572ea7b2f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Mar 2020 16:04:23 +0100 Subject: [PATCH 1/3] 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 has write access. Now: the default push target is the fork the base repo, if said fork exists and has write access, 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 that project) come from forks. --- api/queries_repo.go | 39 +++++++++++++++++++++++++++ command/pr_create.go | 36 +++++++++++++------------ command/pr_create_test.go | 40 +++++++++++++++++++++++++--- context/context.go | 30 +++++++++++++++++++-- context/remote_test.go | 56 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 178 insertions(+), 23 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 048b72b50..79068f0ea 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -3,6 +3,7 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "sort" "strings" @@ -220,6 +221,44 @@ 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 + } + } + } + } + `, variables, &result); err != nil { + return nil, err + } + + if len(result.Repository.Forks.Nodes) > 0 { + return &result.Repository.Forks.Nodes[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 ca7800a19..e4a4bd2de 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -193,7 +193,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { } didForkRepo := false - var headRemote *context.Remote if headRepoErr != nil { if baseRepo.IsPrivate { return fmt.Errorf("cannot fork private repository '%s'", ghrepo.FullName(baseRepo)) @@ -203,19 +202,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("error forking repo: %w", err) } didForkRepo = true - // 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) - if err != nil { - return fmt.Errorf("error adding remote: %w", err) - } - headRemote = &context.Remote{ - Remote: gitRemote, - Owner: headRepo.RepoOwner(), - Repo: headRepo.RepoName(), - } } headBranchLabel := headBranch @@ -223,10 +209,26 @@ func prCreate(cmd *cobra.Command, _ []string) error { headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) } - if headRemote == nil { - headRemote, err = repoContext.RemoteForRepo(headRepo) + headRemote, err := repoContext.RemoteForRepo(headRepo) + // 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 + headRepoURL := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(headRepo)) + // TODO: prevent clashes with another remote of a same name + gitRemote, err := git.AddRemote("fork", headRepoURL, "") if err != nil { - return fmt.Errorf("git remote not found for head repository: %w", err) + return fmt.Errorf("error adding remote: %w", err) + } + headRemote = &context.Remote{ + Remote: gitRemote, + Owner: headRepo.RepoOwner(), + Repo: headRepo.RepoName(), } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 688dfd9c8..ebffb686a 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -14,6 +14,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" : [ ] } } } } @@ -34,7 +38,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 { @@ -61,6 +65,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", @@ -87,6 +95,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() @@ -113,6 +125,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" : [ ] } } } } @@ -232,6 +248,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" : [ ] } } } } @@ -273,7 +293,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 { @@ -302,6 +322,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" : [ ] } } } } @@ -344,7 +368,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 { @@ -373,6 +397,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" : [ ] } } } } @@ -396,7 +424,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 { @@ -457,6 +485,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/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, ¬Found) { + 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/context/remote_test.go b/context/remote_test.go index 04ccbc56c..dd6a5d59a 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,52 @@ 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" } + } + ] } } } } + `)) + + 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, From e3653672a8f9929ed5faec74d81b5c21d695f046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 20 Mar 2020 13:08:33 +0100 Subject: [PATCH 2/3] Simplify `git.AddRemote` implementation The 2nd argument now unused since we've determined that, in practice, we can safely fetch git objects from a freshly created fork on GitHub and that we don't need the remote URL-swapping workaround. --- command/pr_create.go | 2 +- command/repo.go | 2 +- git/remote.go | 24 ++++++------------------ 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/command/pr_create.go b/command/pr_create.go index e4a4bd2de..66efd8a30 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -221,7 +221,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { // TODO: support non-HTTPS git remote URLs headRepoURL := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(headRepo)) // TODO: prevent clashes with another remote of a same name - gitRemote, err := git.AddRemote("fork", headRepoURL, "") + gitRemote, err := git.AddRemote("fork", headRepoURL) if err != nil { return fmt.Errorf("error adding remote: %w", err) } diff --git a/command/repo.go b/command/repo.go index 1dc9b13a5..ec860ed38 100644 --- a/command/repo.go +++ b/command/repo.go @@ -344,7 +344,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/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 } From 5726376ab18f8fbd88f34a677f8223cff663a086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 25 Mar 2020 08:26:06 +0100 Subject: [PATCH 3/3] Guard against faulty `affiliations` GraphQL filter This API was just fixed in github.com, but it will take a while for the change to propagate to GitHub Enterprise installs, so guard ourselves from false positives when querying forks. --- api/queries_repo.go | 9 +++++++-- command/pr_create_test.go | 4 ++++ context/remote_test.go | 3 ++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 8ba8ee148..4bfa35f58 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -245,6 +245,7 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { name owner { login } url + viewerPermission } } } @@ -253,8 +254,12 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { return nil, err } - if len(result.Repository.Forks.Nodes) > 0 { - return &result.Repository.Forks.Nodes[0], nil + 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")} } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 50ff70f9b..a92ad76ff 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -96,6 +96,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", diff --git a/context/remote_test.go b/context/remote_test.go index dd6a5d59a..5c3c46bc7 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -137,7 +137,8 @@ func Test_resolvedRemotes_forkLookup(t *testing.T) { { "id": "FORKID", "url": "https://github.com/FORKOWNER/REPO", "name": "REPO", - "owner": { "login": "FORKOWNER" } + "owner": { "login": "FORKOWNER" }, + "viewerPermission": "WRITE" } ] } } } } `))