From 7a614ce6973847bd43554c9cddff687f3115beb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 17 Jan 2020 15:26:21 +0100 Subject: [PATCH 1/8] Support triangular git workflows in `pr create` - The local git remotes are scanned and resolved to GitHub repositories - The "base" repo is the first result resolved to its parent repo (if a fork) - The name of the default branch is read from the base repo - The "head" repo is the first repo that has push access --- api/client.go | 30 ++++-- api/queries_pr.go | 10 +- api/queries_repo.go | 145 +++++++++++++++++++++++++- command/pr_create.go | 243 +++++++++++++++++++++++++++++++------------ context/remote.go | 20 ++++ 5 files changed, 364 insertions(+), 84 deletions(-) diff --git a/api/client.go b/api/client.go index 2aa82d1ea..f3100815d 100644 --- a/api/client.go +++ b/api/client.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net/http" + "strings" ) // ClientOption represents an argument to NewClient @@ -69,9 +70,27 @@ type Client struct { type graphQLResponse struct { Data interface{} - Errors []struct { - Message string + Errors []GraphQLError +} + +// GraphQLError is a single error returned in a GraphQL response +type GraphQLError struct { + Type string + Path []string + Message string +} + +// GraphQLErrorResponse contains errors returned in a GraphQL response +type GraphQLErrorResponse struct { + Errors []GraphQLError +} + +func (gr GraphQLErrorResponse) Error() string { + errorMessages := make([]string, 0, len(gr.Errors)) + for _, e := range gr.Errors { + errorMessages = append(errorMessages, e.Message) } + return fmt.Sprintf("graphql error: '%s'", strings.Join(errorMessages, ", ")) } // GraphQL performs a GraphQL request and parses the response @@ -151,14 +170,9 @@ func handleResponse(resp *http.Response, data interface{}) error { } if len(gr.Errors) > 0 { - errorMessages := gr.Errors[0].Message - for _, e := range gr.Errors[1:] { - errorMessages += ", " + e.Message - } - return fmt.Errorf("graphql error: '%s'", errorMessages) + return &GraphQLErrorResponse{Errors: gr.Errors} } return nil - } func handleHTTPError(resp *http.Response) error { diff --git a/api/queries_pr.go b/api/queries_pr.go index 9603bf0f2..382d4255b 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -403,12 +403,8 @@ func PullRequestForBranch(client *Client, ghRepo Repo, branch string) (*PullRequ return nil, &NotFoundError{fmt.Errorf("no open pull requests found for branch %q", branch)} } -func CreatePullRequest(client *Client, ghRepo Repo, params map[string]interface{}) (*PullRequest, error) { - repo, err := GitHubRepo(client, ghRepo) - if err != nil { - return nil, err - } - +// CreatePullRequest creates a pull request in a GitHub repository +func CreatePullRequest(client *Client, repo Repository, params map[string]interface{}) (*PullRequest, error) { query := ` mutation CreatePullRequest($input: CreatePullRequestInput!) { createPullRequest(input: $input) { @@ -434,7 +430,7 @@ func CreatePullRequest(client *Client, ghRepo Repo, params map[string]interface{ } }{} - err = client.GraphQL(query, variables, &result) + err := client.GraphQL(query, variables, &result) if err != nil { return nil, err } diff --git a/api/queries_repo.go b/api/queries_repo.go index f974b41cb..2b34794f7 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1,15 +1,59 @@ package api import ( + "bytes" + "encoding/json" "fmt" + "sort" + "strings" "github.com/pkg/errors" ) // Repository contains information about a GitHub repo type Repository struct { - ID string + ID string + Name string + Owner struct { + Login string + } + + IsPrivate bool HasIssuesEnabled bool + ViewerPermission string + DefaultBranchRef struct { + Name string + Target struct { + OID string + } + } + + Parent *Repository +} + +// RepoOwner is the login name of the owner +func (r Repository) RepoOwner() string { + return r.Owner.Login +} + +// RepoName is the name of the repository +func (r Repository) RepoName() string { + return r.Name +} + +// IsFork is true when this repository has a parent repository +func (r Repository) IsFork() bool { + return r.Parent != nil +} + +// ViewerCanPush is true when the requesting user has push access +func (r Repository) ViewerCanPush() bool { + switch r.ViewerPermission { + case "ADMIN", "MAINTAIN", "WRITE": + return true + default: + return false + } } // GitHubRepo looks up the node ID of a named repository @@ -44,3 +88,102 @@ func GitHubRepo(client *Client, ghRepo Repo) (*Repository, error) { return &result.Repository, nil } + +// RepoNetworkResult describes the relationship between related repositories +type RepoNetworkResult struct { + ViewerLogin string + Repositories []*Repository +} + +// RepoNetwork inspects the relationship between multiple GitHub repositories +func RepoNetwork(client *Client, repos []Repo) (RepoNetworkResult, error) { + queries := []string{} + for i, repo := range repos { + queries = append(queries, fmt.Sprintf(` + repo_%03d: repository(owner: %q, name: %q) { + ...repo + parent { + ...repo + } + } + `, i, repo.RepoOwner(), repo.RepoName())) + } + + type ViewerOrRepo struct { + Login string + Repository + } + + graphqlResult := map[string]*json.RawMessage{} + result := RepoNetworkResult{} + + err := client.GraphQL(fmt.Sprintf(` + fragment repo on Repository { + id + name + owner { login } + viewerPermission + defaultBranchRef { + name + target { oid } + } + isPrivate + } + query { + viewer { login } + %s + } + `, strings.Join(queries, "")), nil, &graphqlResult) + graphqlError, isGraphQLError := err.(*GraphQLErrorResponse) + if isGraphQLError { + // If the only errors are that certain repositories are not found, + // continue processing this response instead of returning an error + tolerated := true + for _, ge := range graphqlError.Errors { + if ge.Type != "NOT_FOUND" { + tolerated = false + } + } + if tolerated { + err = nil + } + } + if err != nil { + return result, err + } + + keys := []string{} + for key := range graphqlResult { + keys = append(keys, key) + } + // sort keys to ensure `repo_{N}` entries are processed in order + sort.Sort(sort.StringSlice(keys)) + + for _, name := range keys { + jsonMessage := graphqlResult[name] + if name == "viewer" { + viewerResult := struct { + Login string + }{} + decoder := json.NewDecoder(bytes.NewReader([]byte(*jsonMessage))) + if err := decoder.Decode(&viewerResult); err != nil { + return result, err + } + result.ViewerLogin = viewerResult.Login + } else if strings.HasPrefix(name, "repo_") { + if jsonMessage == nil { + result.Repositories = append(result.Repositories, nil) + continue + } + repo := Repository{} + decoder := json.NewDecoder(bytes.NewReader([]byte(*jsonMessage))) + if err := decoder.Decode(&repo); err != nil { + return result, err + } + result.Repositories = append(result.Repositories, &repo) + } else { + return result, fmt.Errorf("unknown GraphQL result key %q", name) + } + } + return result, nil +} diff --git a/command/pr_create.go b/command/pr_create.go index a72dff7ff..48030bc20 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -3,6 +3,8 @@ package command import ( "fmt" "net/url" + "sort" + "strings" "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" @@ -15,42 +17,62 @@ import ( func prCreate(cmd *cobra.Command, _ []string) error { ctx := contextForCommand(cmd) - - ucc, err := git.UncommittedChangeCount() + remotes, err := ctx.Remotes() if err != nil { return err } - if ucc > 0 { + + client, err := apiClientForContext(ctx) + if err != nil { + return errors.Wrap(err, "could not initialize API client") + } + + baseRepoOverride, _ := cmd.Flags().GetString("repo") + repoContext, err := resolveRemotesToRepos(remotes, client, baseRepoOverride) + if err != nil { + return err + } + + baseRepo, err := repoContext.BaseRepo() + if err != nil { + return errors.Wrap(err, "could not determine the base repository") + } + + headBranch, err := ctx.Branch() + if err != nil { + return errors.Wrap(err, "could not determine the current branch") + } + + baseBranch, err := cmd.Flags().GetString("base") + if err != nil { + return err + } + if baseBranch == "" { + baseBranch = baseRepo.DefaultBranchRef.Name + } + + headRepo, err := repoContext.HeadRepo() + if err != nil { + // TODO: auto-fork repository and add new git remote + return errors.Wrap(err, "could not determine the head repository") + } + + if headBranch == baseBranch && isSameRepo(baseRepo, headRepo) { + return fmt.Errorf("must be on a branch named differently than %q", baseBranch) + } + + fmt.Fprintf(colorableErr(cmd), "\nCreating pull request for %s into %s in %s/%s\n\n", utils.Cyan(headBranch), utils.Cyan(baseBranch), baseRepo.RepoOwner(), baseRepo.RepoName()) + + headRemote, err := repoContext.RemoteForRepo(headRepo) + if err != nil { + return errors.Wrap(err, "") + } + + if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change")) } - - repo, err := ctx.BaseRepo() - if err != nil { - return errors.Wrap(err, "could not determine GitHub repo") - } - - head, err := ctx.Branch() - if err != nil { - return errors.Wrap(err, "could not determine current branch") - } - - remote, err := guessRemote(ctx) - if err != nil { - return err - } - - target, err := cmd.Flags().GetString("base") - if err != nil { - return err - } - if target == "" { - // TODO use default branch - target = "master" - } - - fmt.Fprintf(colorableErr(cmd), "\nCreating pull request for %s into %s in %s/%s\n\n", utils.Cyan(head), utils.Cyan(target), repo.RepoOwner(), repo.RepoName()) - - if err = git.Push(remote, fmt.Sprintf("HEAD:%s", head)); err != nil { + // TODO: respect existing upstream configuration of the current branch + if err = git.Push(headRemote.Name, fmt.Sprintf("HEAD:%s", headBranch)); err != nil { return err } @@ -59,7 +81,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { return errors.Wrap(err, "could not parse web") } if isWeb { - openURL := fmt.Sprintf(`https://github.com/%s/%s/pull/%s`, repo.RepoOwner(), repo.RepoName(), head) + openURL := fmt.Sprintf(`https://github.com/%s/%s/pull/%s`, baseRepo.RepoOwner(), baseRepo.RepoName(), headBranch) fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } @@ -104,20 +126,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - base, err := cmd.Flags().GetString("base") - if err != nil { - return errors.Wrap(err, "could not parse base") - } - if base == "" { - // TODO: use default branch for the repo - base = "master" - } - - client, err := apiClientForContext(ctx) - if err != nil { - return errors.Wrap(err, "could not initialize api client") - } - isDraft, err := cmd.Flags().GetBool("draft") if err != nil { return errors.Wrap(err, "could not parse draft") @@ -128,10 +136,11 @@ func prCreate(cmd *cobra.Command, _ []string) error { "title": title, "body": body, "draft": isDraft, - "baseRefName": base, - "headRefName": head, + "baseRefName": baseBranch, + "headRefName": headBranch, } + repo := api.Repository{} pr, err := api.CreatePullRequest(client, repo, params) if err != nil { return errors.Wrap(err, "failed to create pull request") @@ -141,10 +150,10 @@ func prCreate(cmd *cobra.Command, _ []string) error { } else if action == PreviewAction { openURL := fmt.Sprintf( "https://github.com/%s/%s/compare/%s...%s?expand=1&title=%s&body=%s", - repo.RepoOwner(), - repo.RepoName(), - target, - head, + baseRepo.RepoOwner(), + baseRepo.RepoName(), + baseBranch, + headBranch, url.QueryEscape(title), url.QueryEscape(body), ) @@ -163,22 +172,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { } -func guessRemote(ctx context.Context) (string, error) { - remotes, err := ctx.Remotes() - if err != nil { - return "", errors.Wrap(err, "could not read git remotes") - } - - // TODO: consolidate logic with fsContext.BaseRepo - // TODO: check if the GH repo that the remote points to is writeable - remote, err := remotes.FindByName("upstream", "github", "origin", "*") - if err != nil { - return "", errors.Wrap(err, "could not determine suitable remote") - } - - return remote.Name, nil -} - var prCreateCmd = &cobra.Command{ Use: "create", Short: "Create a pull request", @@ -196,3 +189,117 @@ func init() { "The branch into which you want your code merged") prCreateCmd.Flags().BoolP("web", "w", false, "Open the web browser to create a pull request") } + +const maxRemotesForLookup = 5 + +func resolveRemotesToRepos(remotes context.Remotes, client *api.Client, base string) (resolvedRemotes, error) { + sort.Stable(remotes) + lenRemotesForLookup := len(remotes) + if lenRemotesForLookup > maxRemotesForLookup { + lenRemotesForLookup = maxRemotesForLookup + } + + hasBaseOverride := base != "" + baseOverride := repoFromFullName(base) + foundBaseOverride := false + repos := []api.Repo{} + for _, r := range remotes[:lenRemotesForLookup] { + repos = append(repos, r) + if isSameRepo(r, baseOverride) { + foundBaseOverride = true + } + } + if hasBaseOverride && !foundBaseOverride { + repos = append(repos, baseOverride) + } + + result := resolvedRemotes{remotes: remotes} + if hasBaseOverride { + result.baseOverride = baseOverride + } + networkResult, err := api.RepoNetwork(client, repos) + if err != nil { + return result, err + } + result.network = networkResult + return result, nil +} + +type resolvedRemotes struct { + baseOverride api.Repo + remotes context.Remotes + network api.RepoNetworkResult +} + +// 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 && isSameRepo(repo, r.baseOverride) { + return repo, nil + } + } + return nil, fmt.Errorf("failed looking up information about the '%s/%s' repository", + r.baseOverride.RepoOwner(), r.baseOverride.RepoName()) + } + + for _, repo := range r.network.Repositories { + if repo == nil { + continue + } + if repo.IsFork() { + return repo.Parent, nil + } + return repo, nil + } + + return nil, errors.New("not found") +} + +// HeadRepo is the first found repository that has push access +func (r resolvedRemotes) HeadRepo() (*api.Repository, error) { + for _, repo := range r.network.Repositories { + if repo != nil && repo.ViewerCanPush() { + return repo.Parent, nil + } + } + return nil, errors.New("none of the repositories have push access") +} + +// RemoteForRepo finds the git remote that points to a repository +func (r resolvedRemotes) RemoteForRepo(repo api.Repo) (*context.Remote, error) { + for i, remote := range r.remotes { + if isSameRepo(remote, repo) || + // FIXME: express better that this is because of repo renames + (r.network.Repositories[i] != nil && isSameRepo(r.network.Repositories[i], repo)) { + return remote, nil + } + } + return nil, errors.New("not found") +} + +type ghRepo struct { + owner string + name string +} + +func repoFromFullName(nwo string) (r ghRepo) { + parts := strings.SplitN(nwo, "/", 2) + if len(parts) == 2 { + r.owner, r.name = parts[0], parts[1] + } + return +} + +func (r ghRepo) RepoOwner() string { + return r.owner +} +func (r ghRepo) RepoName() string { + return r.name +} + +func isSameRepo(a, b api.Repo) bool { + return strings.EqualFold(a.RepoOwner(), b.RepoOwner()) && + strings.EqualFold(a.RepoName(), b.RepoName()) +} diff --git a/context/remote.go b/context/remote.go index a660a85cb..3a4e7f772 100644 --- a/context/remote.go +++ b/context/remote.go @@ -35,6 +35,26 @@ func (r Remotes) FindByRepo(owner, name string) (*Remote, error) { return nil, fmt.Errorf("no matching remote found") } +func remoteNameSortScore(name string) int { + switch strings.ToLower(name) { + case "upstream": + return 3 + case "github": + return 2 + case "origin": + return 1 + default: + return 0 + } +} + +// https://golang.org/pkg/sort/#Interface +func (r Remotes) Len() int { return len(r) } +func (r Remotes) Swap(i, j int) { r[i], r[j] = r[j], r[i] } +func (r Remotes) Less(i, j int) bool { + return remoteNameSortScore(r[i].Name) > remoteNameSortScore(r[j].Name) +} + // Remote represents a git remote mapped to a GitHub repository type Remote struct { *git.Remote From 6c49614db72e16150bd7bf343831ee82ddd41de7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Jan 2020 22:56:15 +0100 Subject: [PATCH 2/8] Fix tests --- api/client_test.go | 5 +++-- api/queries_pr.go | 2 +- command/pr_create.go | 13 +++++++----- command/pr_create_test.go | 43 ++++++++++++++++++++++++++++++--------- 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/api/client_test.go b/api/client_test.go index 4d7c64917..12bfbe408 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -2,7 +2,6 @@ package api import ( "bytes" - "fmt" "io/ioutil" "reflect" "testing" @@ -47,5 +46,7 @@ func TestGraphQLError(t *testing.T) { response := struct{}{} http.StubResponse(200, bytes.NewBufferString(`{"errors":[{"message":"OH NO"}]}`)) err := client.GraphQL("", nil, &response) - eq(t, err, fmt.Errorf("graphql error: 'OH NO'")) + if err == nil || err.Error() != "graphql error: 'OH NO'" { + t.Fatalf("got %q", err.Error()) + } } diff --git a/api/queries_pr.go b/api/queries_pr.go index 382d4255b..9b064cd9b 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -404,7 +404,7 @@ func PullRequestForBranch(client *Client, ghRepo Repo, branch string) (*PullRequ } // CreatePullRequest creates a pull request in a GitHub repository -func CreatePullRequest(client *Client, repo Repository, params map[string]interface{}) (*PullRequest, error) { +func CreatePullRequest(client *Client, repo *Repository, params map[string]interface{}) (*PullRequest, error) { query := ` mutation CreatePullRequest($input: CreatePullRequestInput!) { createPullRequest(input: $input) { diff --git a/command/pr_create.go b/command/pr_create.go index 48030bc20..f701081ab 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -61,8 +61,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("must be on a branch named differently than %q", baseBranch) } - fmt.Fprintf(colorableErr(cmd), "\nCreating pull request for %s into %s in %s/%s\n\n", utils.Cyan(headBranch), utils.Cyan(baseBranch), baseRepo.RepoOwner(), baseRepo.RepoName()) - headRemote, err := repoContext.RemoteForRepo(headRepo) if err != nil { return errors.Wrap(err, "") @@ -86,6 +84,12 @@ func prCreate(cmd *cobra.Command, _ []string) error { return utils.OpenInBrowser(openURL) } + fmt.Fprintf(colorableErr(cmd), "\nCreating pull request for %s into %s in %s/%s\n\n", + utils.Cyan(headBranch), + utils.Cyan(baseBranch), + baseRepo.RepoOwner(), + baseRepo.RepoName()) + title, err := cmd.Flags().GetString("title") if err != nil { return errors.Wrap(err, "could not parse title") @@ -140,8 +144,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { "headRefName": headBranch, } - repo := api.Repository{} - pr, err := api.CreatePullRequest(client, repo, params) + pr, err := api.CreatePullRequest(client, baseRepo, params) if err != nil { return errors.Wrap(err, "failed to create pull request") } @@ -261,7 +264,7 @@ func (r resolvedRemotes) BaseRepo() (*api.Repository, error) { func (r resolvedRemotes) HeadRepo() (*api.Repository, error) { for _, repo := range r.network.Repositories { if repo != nil && repo.ViewerCanPush() { - return repo.Parent, nil + return repo, nil } } return nil, errors.New("none of the repositories have push access") diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 96ac2eb25..77539b16d 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -52,8 +52,15 @@ func TestPRCreate(t *testing.T) { http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "id": "REPOID" + { "data": { "repo_000": { + "id": "REPOID", + "name": "REPO", + "owner": {"login": "OWNER"}, + "defaultBranchRef": { + "name": "master", + "target": {"oid": "deadbeef"} + }, + "viewerPermission": "WRITE" } } } `)) http.StubResponse(200, bytes.NewBufferString(` @@ -103,7 +110,20 @@ func TestPRCreate_web(t *testing.T) { initContext = func() context.Context { return ctx } - initFakeHTTP() + http := initFakeHTTP() + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repo_000": { + "id": "REPOID", + "name": "REPO", + "owner": {"login": "OWNER"}, + "defaultBranchRef": { + "name": "master", + "target": {"oid": "deadbeef"} + }, + "viewerPermission": "WRITE" + } } } + `)) ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { @@ -116,11 +136,7 @@ func TestPRCreate_web(t *testing.T) { eq(t, err, nil) eq(t, output.String(), "") - eq(t, output.Stderr(), ` -Creating pull request for feature into master in OWNER/REPO - -Opening https://github.com/OWNER/REPO/pull/feature in your browser. -`) + eq(t, output.Stderr(), "Opening https://github.com/OWNER/REPO/pull/feature in your browser.\n") eq(t, len(ranCommands), 3) eq(t, strings.Join(ranCommands[1], " "), "git push --set-upstream origin HEAD:feature") @@ -139,8 +155,15 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "id": "REPOID" + { "data": { "repo_000": { + "id": "REPOID", + "name": "REPO", + "owner": {"login": "OWNER"}, + "defaultBranchRef": { + "name": "master", + "target": {"oid": "deadbeef"} + }, + "viewerPermission": "WRITE" } } } `)) http.StubResponse(200, bytes.NewBufferString(` From fa30c16ad55b8bbc65df5b55e7f1e3004cc334c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Jan 2020 23:14:52 +0100 Subject: [PATCH 3/8] Fix web-based `pr create` for forks --- command/pr_create.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/command/pr_create.go b/command/pr_create.go index f701081ab..b53dd15d0 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -63,7 +63,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { headRemote, err := repoContext.RemoteForRepo(headRepo) if err != nil { - return errors.Wrap(err, "") + return errors.Wrap(err, "git remote not found for head repository") } if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 { @@ -79,13 +79,17 @@ func prCreate(cmd *cobra.Command, _ []string) error { return errors.Wrap(err, "could not parse web") } if isWeb { - openURL := fmt.Sprintf(`https://github.com/%s/%s/pull/%s`, baseRepo.RepoOwner(), baseRepo.RepoName(), headBranch) + openURL := fmt.Sprintf(`https://github.com/%s/%s/pull/%s`, headRepo.RepoOwner(), headRepo.RepoName(), headBranch) fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } + headBranchLabel := headBranch + if !isSameRepo(baseRepo, headRepo) { + headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) + } fmt.Fprintf(colorableErr(cmd), "\nCreating pull request for %s into %s in %s/%s\n\n", - utils.Cyan(headBranch), + utils.Cyan(headBranchLabel), utils.Cyan(baseBranch), baseRepo.RepoOwner(), baseRepo.RepoName()) @@ -156,7 +160,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { baseRepo.RepoOwner(), baseRepo.RepoName(), baseBranch, - headBranch, + headBranchLabel, url.QueryEscape(title), url.QueryEscape(body), ) From 2aaffc69a2414ec81067537192abb72f3db0f46a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 21 Jan 2020 23:20:50 +0100 Subject: [PATCH 4/8] Clean up obsolete struct --- api/queries_repo.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 2b34794f7..955af9a4d 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -109,11 +109,6 @@ func RepoNetwork(client *Client, repos []Repo) (RepoNetworkResult, error) { `, i, repo.RepoOwner(), repo.RepoName())) } - type ViewerOrRepo struct { - Login string - Repository - } - graphqlResult := map[string]*json.RawMessage{} result := RepoNetworkResult{} From e2a825effb6fa616639769a7d21655d386b6a885 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jan 2020 10:40:06 +0100 Subject: [PATCH 5/8] Auto-fork on `pr create` if no pushable target found --- api/queries_repo.go | 38 ++++++++++++++++++++++++++++++--- command/pr_create.go | 51 +++++++++++++++++++++++++++++++++++++------- git/remote.go | 35 ++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 11 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 955af9a4d..b77dc2d34 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -14,9 +14,7 @@ import ( type Repository struct { ID string Name string - Owner struct { - Login string - } + Owner RepositoryOwner IsPrivate bool HasIssuesEnabled bool @@ -31,6 +29,11 @@ type Repository struct { Parent *Repository } +// RepositoryOwner is the owner of a GitHub repository +type RepositoryOwner struct { + Login string +} + // RepoOwner is the login name of the owner func (r Repository) RepoOwner() string { return r.Owner.Login @@ -182,3 +185,32 @@ func RepoNetwork(client *Client, repos []Repo) (RepoNetworkResult, error) { } return result, nil } + +// repositoryV3 is the repository result from GitHub API v3 +type repositoryV3 struct { + NodeID string + Name string + Owner struct { + Login string + } +} + +// ForkRepo forks the repository on GitHub and returns the new repository +func ForkRepo(client *Client, repo Repo) (*Repository, error) { + path := fmt.Sprintf("repos/%s/%s/forks", repo.RepoOwner(), repo.RepoName()) + body := bytes.NewBufferString(`{}`) + result := repositoryV3{} + err := client.REST("POST", path, body, &result) + if err != nil { + return nil, err + } + + return &Repository{ + ID: result.NodeID, + Name: result.Name, + Owner: RepositoryOwner{ + Login: result.Owner.Login, + }, + ViewerPermission: "WRITE", + }, nil +} diff --git a/command/pr_create.go b/command/pr_create.go index b53dd15d0..4f8a2a70a 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -5,6 +5,7 @@ import ( "net/url" "sort" "strings" + "time" "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" @@ -51,27 +52,61 @@ func prCreate(cmd *cobra.Command, _ []string) error { baseBranch = baseRepo.DefaultBranchRef.Name } + didForkRepo := false + var headRemote *context.Remote headRepo, err := repoContext.HeadRepo() if err != nil { - // TODO: auto-fork repository and add new git remote - return errors.Wrap(err, "could not determine the head repository") + if baseRepo.IsPrivate { + return fmt.Errorf("cannot write to private repository '%s/%s'", baseRepo.RepoOwner(), baseRepo.RepoName()) + } + headRepo, err = api.ForkRepo(client, baseRepo) + if err != nil { + return fmt.Errorf("error forking repo: %w", err) + } + didForkRepo = true + // TODO: support non-HTTPS git remote URLs + baseRepoURL := fmt.Sprintf("https://github.com/%s/%s.git", baseRepo.RepoOwner(), baseRepo.RepoName()) + headRepoURL := fmt.Sprintf("https://github.com/%s/%s.git", headRepo.RepoOwner(), headRepo.RepoName()) + // 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(), + } } if headBranch == baseBranch && isSameRepo(baseRepo, headRepo) { return fmt.Errorf("must be on a branch named differently than %q", baseBranch) } - headRemote, err := repoContext.RemoteForRepo(headRepo) - if err != nil { - return errors.Wrap(err, "git remote not found for head repository") + if headRemote == nil { + headRemote, err = repoContext.RemoteForRepo(headRepo) + if err != nil { + return errors.Wrap(err, "git remote not found for head repository") + } } if ucc, err := git.UncommittedChangeCount(); err == nil && ucc > 0 { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", utils.Pluralize(ucc, "uncommitted change")) } - // TODO: respect existing upstream configuration of the current branch - if err = git.Push(headRemote.Name, fmt.Sprintf("HEAD:%s", headBranch)); err != nil { - return err + pushTries := 0 + maxPushTries := 3 + for { + // TODO: respect existing upstream configuration of the current branch + if err := git.Push(headRemote.Name, fmt.Sprintf("HEAD:%s", headBranch)); err != nil { + if didForkRepo && pushTries < maxPushTries { + pushTries++ + // first wait 2 seconds after forking, then 4s, then 6s + time.Sleep(time.Duration(2*pushTries) * time.Second) + continue + } + return err + } + break } isWeb, err := cmd.Flags().GetBool("web") diff --git a/git/remote.go b/git/remote.go index ba29049c2..9bb24146f 100644 --- a/git/remote.go +++ b/git/remote.go @@ -2,8 +2,11 @@ package git import ( "net/url" + "os/exec" "regexp" "strings" + + "github.com/github/gh-cli/utils" ) var remoteRE = regexp.MustCompile(`(.+)\s+(.+)\s+\((push|fetch)\)`) @@ -67,3 +70,35 @@ 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) + 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(initURL) + if err != nil { + return nil, err + } + + return &Remote{ + Name: name, + FetchURL: finalURLParsed, + PushURL: finalURLParsed, + }, nil +} From a767fd7910d28d6005972f55a1a9ce0e1e24acbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jan 2020 18:37:50 +0100 Subject: [PATCH 6/8] Add code comments for tricky parts --- api/queries_repo.go | 5 +++++ command/pr_create.go | 7 ++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index b77dc2d34..ae07ed1e9 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -112,6 +112,9 @@ func RepoNetwork(client *Client, repos []Repo) (RepoNetworkResult, error) { `, i, repo.RepoOwner(), repo.RepoName())) } + // Since the query is constructed dynamically, we can't parse a response + // format using a static struct. Instead, hold the raw JSON data until we + // decide how to parse it manually. graphqlResult := map[string]*json.RawMessage{} result := RepoNetworkResult{} @@ -157,6 +160,8 @@ func RepoNetwork(client *Client, repos []Repo) (RepoNetworkResult, error) { // sort keys to ensure `repo_{N}` entries are processed in order sort.Sort(sort.StringSlice(keys)) + // Iterate over keys of GraphQL response data and, based on its name, + // dynamically allocate the target struct an individual message gets decoded to. for _, name := range keys { jsonMessage := graphqlResult[name] if name == "viewer" { diff --git a/command/pr_create.go b/command/pr_create.go index 4f8a2a70a..7eff43489 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -232,6 +232,8 @@ func init() { prCreateCmd.Flags().BoolP("web", "w", false, "Open the web browser to create a pull request") } +// cap the number of git remotes looked up, since the user might have an +// unusally large number of git remotes const maxRemotesForLookup = 5 func resolveRemotesToRepos(remotes context.Remotes, client *api.Client, base string) (resolvedRemotes, error) { @@ -252,6 +254,8 @@ func resolveRemotesToRepos(remotes context.Remotes, client *api.Client, base str } } if hasBaseOverride && !foundBaseOverride { + // additionally, look up the explicitly specified base repo if it's not + // already covered by git remotes repos = append(repos, baseOverride) } @@ -313,7 +317,8 @@ func (r resolvedRemotes) HeadRepo() (*api.Repository, error) { func (r resolvedRemotes) RemoteForRepo(repo api.Repo) (*context.Remote, error) { for i, remote := range r.remotes { if isSameRepo(remote, repo) || - // FIXME: express better that this is because of repo renames + // additionally, look up the resolved repository name in case this + // git remote points to this repository via a redirect (r.network.Repositories[i] != nil && isSameRepo(r.network.Repositories[i], repo)) { return remote, nil } From 6799e7f57044f6280f4c0f4c13bc3741d86ac40d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jan 2020 19:33:53 +0100 Subject: [PATCH 7/8] Avoid resetting the current branch when `--repo` is used This leads to unwanted consequences in `pr create` --- command/root.go | 1 - 1 file changed, 1 deletion(-) diff --git a/command/root.go b/command/root.go index 5703f30c2..f2bfab638 100644 --- a/command/root.go +++ b/command/root.go @@ -93,7 +93,6 @@ func contextForCommand(cmd *cobra.Command) context.Context { ctx := initContext() if repo, err := cmd.Flags().GetString("repo"); err == nil && repo != "" { ctx.SetBaseRepo(repo) - ctx.SetBranch("master") } return ctx } From f10b8d80950a80b59d190e0888e0246fce99fd3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 14:05:15 +0100 Subject: [PATCH 8/8] Add unit tests for remotes-to-repos resolver --- command/pr_create_test.go | 120 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 77539b16d..047118972 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -10,6 +10,7 @@ import ( "strings" "testing" + "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" "github.com/github/gh-cli/git" "github.com/github/gh-cli/test" @@ -188,3 +189,122 @@ Creating pull request for feature into master in OWNER/REPO `) } + +func Test_resolvedRemotes_clonedFork(t *testing.T) { + resolved := resolvedRemotes{ + baseOverride: nil, + remotes: context.Remotes{ + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Owner: "OWNER", + Repo: "REPO", + }, + }, + network: api.RepoNetworkResult{ + Repositories: []*api.Repository{ + &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) + } + if baseRepo.RepoOwner() != "PARENTOWNER" { + t.Errorf("got owner %q", baseRepo.RepoOwner()) + } + 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) + } + if headRepo.RepoOwner() != "OWNER" { + t.Errorf("got owner %q", headRepo.RepoOwner()) + } + headRemote, err := resolved.RemoteForRepo(headRepo) + if err != nil { + t.Fatalf("got %v", err) + } + if headRemote.Name != "origin" { + t.Errorf("got remote %q", headRemote.Name) + } +} + +func Test_resolvedRemotes_triangularSetup(t *testing.T) { + resolved := resolvedRemotes{ + baseOverride: nil, + remotes: context.Remotes{ + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Owner: "OWNER", + Repo: "REPO", + }, + &context.Remote{ + Remote: &git.Remote{Name: "fork"}, + Owner: "MYSELF", + Repo: "REPO", + }, + }, + network: api.RepoNetworkResult{ + Repositories: []*api.Repository{ + &api.Repository{ + Name: "NEWNAME", + Owner: api.RepositoryOwner{Login: "NEWOWNER"}, + ViewerPermission: "READ", + }, + &api.Repository{ + Name: "REPO", + Owner: api.RepositoryOwner{Login: "MYSELF"}, + ViewerPermission: "ADMIN", + }, + }, + }, + } + + baseRepo, err := resolved.BaseRepo() + if err != nil { + t.Fatalf("got %v", err) + } + if baseRepo.RepoOwner() != "NEWOWNER" { + t.Errorf("got owner %q", baseRepo.RepoOwner()) + } + if baseRepo.RepoName() != "NEWNAME" { + t.Errorf("got name %q", baseRepo.RepoName()) + } + 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) + } + if headRepo.RepoOwner() != "MYSELF" { + t.Errorf("got owner %q", headRepo.RepoOwner()) + } + headRemote, err := resolved.RemoteForRepo(headRepo) + if err != nil { + t.Fatalf("got %v", err) + } + if headRemote.Name != "fork" { + t.Errorf("got remote %q", headRemote.Name) + } +}