STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #680 from cli/pr-create-push-default

Browse source 
Creating a PR now always prioritizes an existing fork as a push target
This commit is contained in:
Mislav Marohnić 2020-03-30 13:38:11 +02:00 • committed by GitHub
commit 1fb0eefd36
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 193 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

44
api/queries_repo.go
View file

@ -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"`

25
command/pr_create.go
View file

@ -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 {

44
command/pr_create_test.go
View file

@ -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"

2
command/repo.go
View file

@ -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)
}

30
context/context.go
View file

@ -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

57
context/remote_test.go
View file

@ -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,

24
git/remote.go
View file

@ -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
}