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 01/23] 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 fc25a4e9ed860d9dc52d3243b522256cebabc553 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jan 2020 15:37:42 -0600 Subject: [PATCH 02/23] check for disabled issues in issue view command --- api/queries_issue.go | 31 +++++++++++++++++++++++++++++++ command/issue.go | 8 ++++++++ 2 files changed, 39 insertions(+) diff --git a/api/queries_issue.go b/api/queries_issue.go index 7ba12f6ad..c1ffb5253 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -91,6 +91,37 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} return &result.CreateIssue.Issue, nil } +func HasIssuesEnabled(client *Client, ghRepo Repo) (bool, error) { + type response struct { + Repository struct { + HasIssuesEnabled bool + } + } + + query := ` + query($owner: String!, $repo: String!) { + repository(owner: $owner, name: $repo) { + hasIssuesEnabled + } + }` + + owner := ghRepo.RepoOwner() + repo := ghRepo.RepoName() + variables := map[string]interface{}{ + "owner": owner, + "repo": repo, + } + + var resp response + err := client.GraphQL(query, variables, &resp) + if err != nil { + return false, err + } + + return resp.Repository.HasIssuesEnabled, nil + +} + func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPayload, error) { type response struct { Repository struct { diff --git a/command/issue.go b/command/issue.go index 139ab3337..49bf66a7e 100644 --- a/command/issue.go +++ b/command/issue.go @@ -215,6 +215,14 @@ func issueView(cmd *cobra.Command, args []string) error { return err } + issuesEnabled, err := api.HasIssuesEnabled(apiClient, baseRepo) + if err != nil { + return err + } + if !issuesEnabled { + return fmt.Errorf("the '%s/%s' repository has disabled issues", baseRepo.RepoOwner(), baseRepo.RepoName()) + } + issue, err := issueFromArg(apiClient, baseRepo, args[0]) if err != nil { return err From f09b05e628ea8a0af0c9c501154a1c3b40c64128 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 21 Jan 2020 15:47:36 -0600 Subject: [PATCH 03/23] test for disabled issues --- command/issue_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/command/issue_test.go b/command/issue_test.go index e9f616f68..532c3cc35 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -202,6 +202,13 @@ func TestIssueView(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "issue": { "number": 123, @@ -235,6 +242,13 @@ func TestIssueView_preview(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "issue": { "number": 123, @@ -280,6 +294,13 @@ func TestIssueView_notFound(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` { "errors": [ { "message": "Could not resolve to an Issue with the number of 9999." } @@ -303,10 +324,34 @@ func TestIssueView_notFound(t *testing.T) { } } +func TestIssueView_disabledIssues(t *testing.T) { + initBlankContext("OWNER/REPO", "master") + http := initFakeHTTP() + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": false + } } } + `)) + + _, err := RunCommand(issueViewCmd, `issue view 6666`) + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Errorf("error running command `issue view`: %v", err) + } +} + func TestIssueView_urlArg(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "issue": { "number": 123, 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 04/23] 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 05/23] 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 06/23] 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 07/23] 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 08/23] 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 09/23] 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 0b0fd42ef302dec175c3af3505fb6f3b4c219d04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jan 2020 19:35:39 +0100 Subject: [PATCH 10/23] Dump HTTP request/response bodies when `DEBUG=api` --- api/client.go | 23 ++++++++++++++++++++++- command/root.go | 4 ++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/api/client.go b/api/client.go index 2aa82d1ea..040527053 100644 --- a/api/client.go +++ b/api/client.go @@ -7,6 +7,8 @@ import ( "io" "io/ioutil" "net/http" + "regexp" + "strings" ) // ClientOption represents an argument to NewClient @@ -34,13 +36,26 @@ func AddHeader(name, value string) ClientOption { } // VerboseLog enables request/response logging within a RoundTripper -func VerboseLog(out io.Writer) ClientOption { +func VerboseLog(out io.Writer, logBodies bool) ClientOption { return func(tr http.RoundTripper) http.RoundTripper { return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { fmt.Fprintf(out, "> %s %s\n", req.Method, req.URL.RequestURI()) + if logBodies && req.Body != nil && inspectableMIMEType(req.Header.Get("Content-type")) { + newBody := &bytes.Buffer{} + io.Copy(out, io.TeeReader(req.Body, newBody)) + fmt.Fprintln(out) + req.Body = ioutil.NopCloser(newBody) + } res, err := tr.RoundTrip(req) if err == nil { fmt.Fprintf(out, "< HTTP %s\n", res.Status) + if logBodies && res.Body != nil && inspectableMIMEType(res.Header.Get("Content-type")) { + newBody := &bytes.Buffer{} + // TODO: pretty-print response JSON + io.Copy(out, io.TeeReader(res.Body, newBody)) + fmt.Fprintln(out) + res.Body = ioutil.NopCloser(newBody) + } } return res, err }} @@ -179,3 +194,9 @@ func handleHTTPError(resp *http.Response) error { return fmt.Errorf("http error, '%s' failed (%d): '%s'", resp.Request.URL, resp.StatusCode, message) } + +var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`) + +func inspectableMIMEType(t string) bool { + return strings.HasPrefix(t, "text/") || jsonTypeRE.MatchString(t) +} diff --git a/command/root.go b/command/root.go index 5703f30c2..e443d391e 100644 --- a/command/root.go +++ b/command/root.go @@ -84,7 +84,7 @@ func BasicClient() (*api.Client, error) { opts = append(opts, api.AddHeader("Authorization", fmt.Sprintf("token %s", c.Token))) } if verbose := os.Getenv("DEBUG"); verbose != "" { - opts = append(opts, api.VerboseLog(os.Stderr)) + opts = append(opts, api.VerboseLog(os.Stderr, false)) } return api.NewClient(opts...), nil } @@ -113,7 +113,7 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { api.AddHeader("GraphQL-Features", "pe_mobile"), } if verbose := os.Getenv("DEBUG"); verbose != "" { - opts = append(opts, api.VerboseLog(os.Stderr)) + opts = append(opts, api.VerboseLog(os.Stderr, strings.Contains(verbose, "api"))) } return api.NewClient(opts...), nil } From 8c84fe3e3c44f8f411fed83c89a33c261e8f80d2 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Jan 2020 12:37:00 -0600 Subject: [PATCH 11/23] just augment existing queries --- api/queries_issue.go | 39 +++++++-------------------------------- command/issue.go | 8 -------- command/issue_test.go | 34 +++------------------------------- 3 files changed, 10 insertions(+), 71 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index c1ffb5253..50fcbd434 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -91,37 +91,6 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} return &result.CreateIssue.Issue, nil } -func HasIssuesEnabled(client *Client, ghRepo Repo) (bool, error) { - type response struct { - Repository struct { - HasIssuesEnabled bool - } - } - - query := ` - query($owner: String!, $repo: String!) { - repository(owner: $owner, name: $repo) { - hasIssuesEnabled - } - }` - - owner := ghRepo.RepoOwner() - repo := ghRepo.RepoName() - variables := map[string]interface{}{ - "owner": owner, - "repo": repo, - } - - var resp response - err := client.GraphQL(query, variables, &resp) - if err != nil { - return false, err - } - - return resp.Repository.HasIssuesEnabled, nil - -} - func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPayload, error) { type response struct { Repository struct { @@ -267,13 +236,15 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig func IssueByNumber(client *Client, ghRepo Repo, number int) (*Issue, error) { type response struct { Repository struct { - Issue Issue + Issue Issue + HasIssuesEnabled bool } } query := ` query($owner: String!, $repo: String!, $issue_number: Int!) { repository(owner: $owner, name: $repo) { + hasIssuesEnabled issue(number: $issue_number) { title body @@ -306,5 +277,9 @@ func IssueByNumber(client *Client, ghRepo Repo, number int) (*Issue, error) { return nil, err } + if !resp.Repository.HasIssuesEnabled { + return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", ghRepo.RepoOwner(), ghRepo.RepoName()) + } + return &resp.Repository.Issue, nil } diff --git a/command/issue.go b/command/issue.go index 49bf66a7e..139ab3337 100644 --- a/command/issue.go +++ b/command/issue.go @@ -215,14 +215,6 @@ func issueView(cmd *cobra.Command, args []string) error { return err } - issuesEnabled, err := api.HasIssuesEnabled(apiClient, baseRepo) - if err != nil { - return err - } - if !issuesEnabled { - return fmt.Errorf("the '%s/%s' repository has disabled issues", baseRepo.RepoOwner(), baseRepo.RepoName()) - } - issue, err := issueFromArg(apiClient, baseRepo, args[0]) if err != nil { return err diff --git a/command/issue_test.go b/command/issue_test.go index 532c3cc35..9cf4156b4 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -203,14 +203,7 @@ func TestIssueView(t *testing.T) { http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "id": "REPOID", - "hasIssuesEnabled": true - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "issue": { + { "data": { "repository": { "hasIssuesEnabled": true, "issue": { "number": 123, "url": "https://github.com/OWNER/REPO/issues/123" } } } } @@ -243,14 +236,7 @@ func TestIssueView_preview(t *testing.T) { http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "id": "REPOID", - "hasIssuesEnabled": true - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "issue": { + { "data": { "repository": { "hasIssuesEnabled": true, "issue": { "number": 123, "body": "**bold story**", "title": "ix of coins", @@ -294,13 +280,6 @@ func TestIssueView_notFound(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "id": "REPOID", - "hasIssuesEnabled": true - } } } - `)) - http.StubResponse(200, bytes.NewBufferString(` { "errors": [ { "message": "Could not resolve to an Issue with the number of 9999." } @@ -346,14 +325,7 @@ func TestIssueView_urlArg(t *testing.T) { http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "id": "REPOID", - "hasIssuesEnabled": true - } } } - `)) - - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "issue": { + { "data": { "repository": { "hasIssuesEnabled": true, "issue": { "number": 123, "url": "https://github.com/OWNER/REPO/issues/123" } } } } From 1f90579d2a6a0bb150f86587f47cdd458ea03d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jan 2020 22:44:46 +0100 Subject: [PATCH 12/23] Extract common interface for a GitHub Repository Also define a handful of utility methods: - `New(owner, repo)` - `FullName`: the name slash owner pair - `FromFullName`: parse the name slash owner pair - `FromURL`: parse a GitHub.com URL - `IsSame(r1, r2)`: compare two repositories --- api/queries_issue.go | 28 +++++++-------- api/queries_pr.go | 32 +++++++---------- api/queries_repo.go | 18 +++++----- command/issue.go | 17 +++++---- command/pr.go | 9 ++--- command/pr_create.go | 69 +++++++++++------------------------- context/blank_context.go | 22 +++--------- context/config_file.go | 2 ++ context/context.go | 19 +++------- context/remote.go | 36 ++++--------------- context/remote_test.go | 27 -------------- internal/ghrepo/repo.go | 61 +++++++++++++++++++++++++++++++ internal/ghrepo/repo_test.go | 40 +++++++++++++++++++++ 13 files changed, 187 insertions(+), 193 deletions(-) create mode 100644 internal/ghrepo/repo.go create mode 100644 internal/ghrepo/repo_test.go diff --git a/api/queries_issue.go b/api/queries_issue.go index 39a998199..9fc4c37f3 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -2,6 +2,8 @@ package api import ( "fmt" + + "github.com/github/gh-cli/internal/ghrepo" ) type IssuesPayload struct { @@ -88,7 +90,7 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} return &result.CreateIssue.Issue, nil } -func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPayload, error) { +func IssueStatus(client *Client, repo ghrepo.Interface, currentUsername string) (*IssuesPayload, error) { type response struct { Repository struct { Assigned struct { @@ -132,11 +134,9 @@ func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPa } }` - owner := ghRepo.RepoOwner() - repo := ghRepo.RepoName() variables := map[string]interface{}{ - "owner": owner, - "repo": repo, + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), "viewer": currentUsername, } @@ -147,7 +147,7 @@ func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPa } if !resp.Repository.HasIssuesEnabled { - return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", owner, repo) + return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo)) } payload := IssuesPayload{ @@ -168,7 +168,7 @@ func IssueStatus(client *Client, ghRepo Repo, currentUsername string) (*IssuesPa return &payload, nil } -func IssueList(client *Client, ghRepo Repo, state string, labels []string, assigneeString string, limit int) ([]Issue, error) { +func IssueList(client *Client, repo ghrepo.Interface, state string, labels []string, assigneeString string, limit int) ([]Issue, error) { var states []string switch state { case "open", "": @@ -194,12 +194,10 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig } ` - owner := ghRepo.RepoOwner() - repo := ghRepo.RepoName() variables := map[string]interface{}{ "limit": limit, - "owner": owner, - "repo": repo, + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), "states": states, } if len(labels) > 0 { @@ -224,13 +222,13 @@ func IssueList(client *Client, ghRepo Repo, state string, labels []string, assig } if !resp.Repository.HasIssuesEnabled { - return nil, fmt.Errorf("the '%s/%s' repository has disabled issues", owner, repo) + return nil, fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(repo)) } return resp.Repository.Issues.Nodes, nil } -func IssueByNumber(client *Client, ghRepo Repo, number int) (*Issue, error) { +func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, error) { type response struct { Repository struct { Issue Issue @@ -261,8 +259,8 @@ func IssueByNumber(client *Client, ghRepo Repo, number int) (*Issue, error) { }` variables := map[string]interface{}{ - "owner": ghRepo.RepoOwner(), - "repo": ghRepo.RepoName(), + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), "issue_number": number, } diff --git a/api/queries_pr.go b/api/queries_pr.go index 9b064cd9b..af7e30e68 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -3,6 +3,8 @@ package api import ( "fmt" "strings" + + "github.com/github/gh-cli/internal/ghrepo" ) type PullRequestsPayload struct { @@ -127,12 +129,7 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { return } -type Repo interface { - RepoName() string - RepoOwner() string -} - -func PullRequests(client *Client, ghRepo Repo, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { +func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { type edges struct { TotalCount int Edges []struct { @@ -229,11 +226,8 @@ func PullRequests(client *Client, ghRepo Repo, currentPRNumber int, currentPRHea } ` - owner := ghRepo.RepoOwner() - repo := ghRepo.RepoName() - - viewerQuery := fmt.Sprintf("repo:%s/%s state:open is:pr author:%s", owner, repo, currentUsername) - reviewerQuery := fmt.Sprintf("repo:%s/%s state:open review-requested:%s", owner, repo, currentUsername) + viewerQuery := fmt.Sprintf("repo:%s state:open is:pr author:%s", ghrepo.FullName(repo), currentUsername) + reviewerQuery := fmt.Sprintf("repo:%s state:open review-requested:%s", ghrepo.FullName(repo), currentUsername) branchWithoutOwner := currentPRHeadRef if idx := strings.Index(currentPRHeadRef, ":"); idx >= 0 { @@ -243,8 +237,8 @@ func PullRequests(client *Client, ghRepo Repo, currentPRNumber int, currentPRHea variables := map[string]interface{}{ "viewerQuery": viewerQuery, "reviewerQuery": reviewerQuery, - "owner": owner, - "repo": repo, + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), "headRefName": branchWithoutOwner, "number": currentPRNumber, } @@ -289,7 +283,7 @@ func PullRequests(client *Client, ghRepo Repo, currentPRNumber int, currentPRHea return &payload, nil } -func PullRequestByNumber(client *Client, ghRepo Repo, number int) (*PullRequest, error) { +func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*PullRequest, error) { type response struct { Repository struct { PullRequest PullRequest @@ -328,8 +322,8 @@ func PullRequestByNumber(client *Client, ghRepo Repo, number int) (*PullRequest, }` variables := map[string]interface{}{ - "owner": ghRepo.RepoOwner(), - "repo": ghRepo.RepoName(), + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), "pr_number": number, } @@ -342,7 +336,7 @@ func PullRequestByNumber(client *Client, ghRepo Repo, number int) (*PullRequest, return &resp.Repository.PullRequest, nil } -func PullRequestForBranch(client *Client, ghRepo Repo, branch string) (*PullRequest, error) { +func PullRequestForBranch(client *Client, repo ghrepo.Interface, branch string) (*PullRequest, error) { type response struct { Repository struct { PullRequests struct { @@ -383,8 +377,8 @@ func PullRequestForBranch(client *Client, ghRepo Repo, branch string) (*PullRequ } variables := map[string]interface{}{ - "owner": ghRepo.RepoOwner(), - "repo": ghRepo.RepoName(), + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), "headRefName": branchWithoutOwner, } diff --git a/api/queries_repo.go b/api/queries_repo.go index ae07ed1e9..27cd3d6b0 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -7,6 +7,7 @@ import ( "sort" "strings" + "github.com/github/gh-cli/internal/ghrepo" "github.com/pkg/errors" ) @@ -60,10 +61,7 @@ func (r Repository) ViewerCanPush() bool { } // GitHubRepo looks up the node ID of a named repository -func GitHubRepo(client *Client, ghRepo Repo) (*Repository, error) { - owner := ghRepo.RepoOwner() - repo := ghRepo.RepoName() - +func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { query := ` query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { @@ -72,8 +70,8 @@ func GitHubRepo(client *Client, ghRepo Repo) (*Repository, error) { } }` variables := map[string]interface{}{ - "owner": owner, - "name": repo, + "owner": repo.RepoOwner(), + "name": repo.RepoName(), } result := struct { @@ -82,7 +80,7 @@ func GitHubRepo(client *Client, ghRepo Repo) (*Repository, error) { err := client.GraphQL(query, variables, &result) if err != nil || result.Repository.ID == "" { - newErr := fmt.Errorf("failed to determine repository ID for '%s/%s'", owner, repo) + newErr := fmt.Errorf("failed to determine repository ID for '%s'", ghrepo.FullName(repo)) if err != nil { newErr = errors.Wrap(err, newErr.Error()) } @@ -99,7 +97,7 @@ type RepoNetworkResult struct { } // RepoNetwork inspects the relationship between multiple GitHub repositories -func RepoNetwork(client *Client, repos []Repo) (RepoNetworkResult, error) { +func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, error) { queries := []string{} for i, repo := range repos { queries = append(queries, fmt.Sprintf(` @@ -201,8 +199,8 @@ type repositoryV3 struct { } // 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()) +func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { + path := fmt.Sprintf("repos/%s/forks", ghrepo.FullName(repo)) body := bytes.NewBufferString(`{}`) result := repositoryV3{} err := client.REST("POST", path, body, &result) diff --git a/command/issue.go b/command/issue.go index 63974cb20..c89c9507f 100644 --- a/command/issue.go +++ b/command/issue.go @@ -9,8 +9,8 @@ import ( "strings" "github.com/github/gh-cli/api" - "github.com/github/gh-cli/context" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" "github.com/github/gh-cli/pkg/githubtemplate" "github.com/github/gh-cli/utils" "github.com/pkg/errors" @@ -107,7 +107,7 @@ func issueList(cmd *cobra.Command, args []string) error { return err } - fmt.Fprintf(colorableErr(cmd), "\nIssues for %s/%s\n\n", baseRepo.RepoOwner(), baseRepo.RepoName()) + fmt.Fprintf(colorableErr(cmd), "\nIssues for %s\n\n", ghrepo.FullName(baseRepo)) issues, err := api.IssueList(apiClient, baseRepo, state, labels, assignee, limit) if err != nil { @@ -257,7 +257,7 @@ func printIssuePreview(out io.Writer, issue *api.Issue) { var issueURLRE = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/issues/(\d+)`) -func issueFromArg(apiClient *api.Client, baseRepo context.GitHubRepository, arg string) (*api.Issue, error) { +func issueFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) (*api.Issue, error) { if issueNumber, err := strconv.Atoi(arg); err == nil { return api.IssueByNumber(apiClient, baseRepo, issueNumber) } @@ -278,7 +278,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { return err } - fmt.Fprintf(colorableErr(cmd), "\nCreating issue in %s/%s\n\n", baseRepo.RepoOwner(), baseRepo.RepoName()) + fmt.Fprintf(colorableErr(cmd), "\nCreating issue in %s\n\n", ghrepo.FullName(baseRepo)) var templateFiles []string if rootDir, err := git.ToplevelDir(); err == nil { @@ -288,7 +288,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { if isWeb, err := cmd.Flags().GetBool("web"); err == nil && isWeb { // TODO: move URL generation into GitHubRepository - openURL := fmt.Sprintf("https://github.com/%s/%s/issues/new", baseRepo.RepoOwner(), baseRepo.RepoName()) + openURL := fmt.Sprintf("https://github.com/%s/issues/new", ghrepo.FullName(baseRepo)) if len(templateFiles) > 1 { openURL += "/choose" } @@ -306,7 +306,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { return err } if !repo.HasIssuesEnabled { - return fmt.Errorf("the '%s/%s' repository has disabled issues", baseRepo.RepoOwner(), baseRepo.RepoName()) + return fmt.Errorf("the '%s' repository has disabled issues", ghrepo.FullName(baseRepo)) } action := SubmitAction @@ -346,9 +346,8 @@ func issueCreate(cmd *cobra.Command, args []string) error { if action == PreviewAction { openURL := fmt.Sprintf( - "https://github.com/%s/%s/issues/new/?title=%s&body=%s", - baseRepo.RepoOwner(), - baseRepo.RepoName(), + "https://github.com/%s/issues/new/?title=%s&body=%s", + ghrepo.FullName(baseRepo), url.QueryEscape(title), url.QueryEscape(body), ) diff --git a/command/pr.go b/command/pr.go index 5204c1b3d..bcb773b67 100644 --- a/command/pr.go +++ b/command/pr.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" "github.com/github/gh-cli/utils" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -139,7 +140,7 @@ func prList(cmd *cobra.Command, args []string) error { return err } - fmt.Fprintf(colorableErr(cmd), "\nPull requests for %s/%s\n\n", baseRepo.RepoOwner(), baseRepo.RepoName()) + fmt.Fprintf(colorableErr(cmd), "\nPull requests for %s\n\n", ghrepo.FullName(baseRepo)) limit, err := cmd.Flags().GetInt("limit") if err != nil { @@ -275,7 +276,7 @@ func prView(cmd *cobra.Command, args []string) error { } if prNumber > 0 { - openURL = fmt.Sprintf("https://github.com/%s/%s/pull/%d", baseRepo.RepoOwner(), baseRepo.RepoName(), prNumber) + openURL = fmt.Sprintf("https://github.com/%s/pull/%d", ghrepo.FullName(baseRepo), prNumber) if preview { pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNumber) if err != nil { @@ -323,7 +324,7 @@ func printPrPreview(out io.Writer, pr *api.PullRequest) { var prURLRE = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)`) -func prFromArg(apiClient *api.Client, baseRepo context.GitHubRepository, arg string) (*api.PullRequest, error) { +func prFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) (*api.PullRequest, error) { if prNumber, err := strconv.Atoi(arg); err == nil { return api.PullRequestByNumber(apiClient, baseRepo, prNumber) } @@ -357,7 +358,7 @@ func prSelectorForCurrentBranch(ctx context.Context) (prNumber int, prHeadRef st var branchOwner string if branchConfig.RemoteURL != nil { // the branch merges from a remote specified by URL - if r, err := context.RepoFromURL(branchConfig.RemoteURL); err == nil { + if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { branchOwner = r.RepoOwner() } } else if branchConfig.RemoteName != "" { diff --git a/command/pr_create.go b/command/pr_create.go index 7eff43489..f544b504e 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -4,12 +4,12 @@ import ( "fmt" "net/url" "sort" - "strings" "time" "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" "github.com/github/gh-cli/pkg/githubtemplate" "github.com/github/gh-cli/utils" "github.com/pkg/errors" @@ -57,7 +57,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { headRepo, err := repoContext.HeadRepo() if err != nil { if baseRepo.IsPrivate { - return fmt.Errorf("cannot write to private repository '%s/%s'", baseRepo.RepoOwner(), baseRepo.RepoName()) + return fmt.Errorf("cannot write to private repository '%s'", ghrepo.FullName(baseRepo)) } headRepo, err = api.ForkRepo(client, baseRepo) if err != nil { @@ -65,8 +65,8 @@ func prCreate(cmd *cobra.Command, _ []string) error { } 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()) + 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 { @@ -79,7 +79,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } } - if headBranch == baseBranch && isSameRepo(baseRepo, headRepo) { + if headBranch == baseBranch && ghrepo.IsSame(baseRepo, headRepo) { return fmt.Errorf("must be on a branch named differently than %q", baseBranch) } @@ -114,20 +114,19 @@ 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`, headRepo.RepoOwner(), headRepo.RepoName(), headBranch) + openURL := fmt.Sprintf(`https://github.com/%s/pull/%s`, ghrepo.FullName(headRepo), headBranch) fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } headBranchLabel := headBranch - if !isSameRepo(baseRepo, headRepo) { + if !ghrepo.IsSame(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", + fmt.Fprintf(colorableErr(cmd), "\nCreating pull request for %s into %s in %s\n\n", utils.Cyan(headBranchLabel), utils.Cyan(baseBranch), - baseRepo.RepoOwner(), - baseRepo.RepoName()) + ghrepo.FullName(baseRepo)) title, err := cmd.Flags().GetString("title") if err != nil { @@ -191,9 +190,8 @@ func prCreate(cmd *cobra.Command, _ []string) error { fmt.Fprintln(cmd.OutOrStdout(), pr.URL) } else if action == PreviewAction { openURL := fmt.Sprintf( - "https://github.com/%s/%s/compare/%s...%s?expand=1&title=%s&body=%s", - baseRepo.RepoOwner(), - baseRepo.RepoName(), + "https://github.com/%s/compare/%s...%s?expand=1&title=%s&body=%s", + ghrepo.FullName(baseRepo), baseBranch, headBranchLabel, url.QueryEscape(title), @@ -244,12 +242,12 @@ func resolveRemotesToRepos(remotes context.Remotes, client *api.Client, base str } hasBaseOverride := base != "" - baseOverride := repoFromFullName(base) + baseOverride := ghrepo.FromFullName(base) foundBaseOverride := false - repos := []api.Repo{} + repos := []ghrepo.Interface{} for _, r := range remotes[:lenRemotesForLookup] { repos = append(repos, r) - if isSameRepo(r, baseOverride) { + if ghrepo.IsSame(r, baseOverride) { foundBaseOverride = true } } @@ -272,7 +270,7 @@ func resolveRemotesToRepos(remotes context.Remotes, client *api.Client, base str } type resolvedRemotes struct { - baseOverride api.Repo + baseOverride ghrepo.Interface remotes context.Remotes network api.RepoNetworkResult } @@ -282,12 +280,12 @@ type resolvedRemotes struct { func (r resolvedRemotes) BaseRepo() (*api.Repository, error) { if r.baseOverride != nil { for _, repo := range r.network.Repositories { - if repo != nil && isSameRepo(repo, r.baseOverride) { + if repo != nil && ghrepo.IsSame(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()) + return nil, fmt.Errorf("failed looking up information about the '%s' repository", + ghrepo.FullName(r.baseOverride)) } for _, repo := range r.network.Repositories { @@ -314,39 +312,14 @@ func (r resolvedRemotes) HeadRepo() (*api.Repository, error) { } // RemoteForRepo finds the git remote that points to a repository -func (r resolvedRemotes) RemoteForRepo(repo api.Repo) (*context.Remote, error) { +func (r resolvedRemotes) RemoteForRepo(repo ghrepo.Interface) (*context.Remote, error) { for i, remote := range r.remotes { - if isSameRepo(remote, repo) || + 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 && isSameRepo(r.network.Repositories[i], repo)) { + (r.network.Repositories[i] != nil && ghrepo.IsSame(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/blank_context.go b/context/blank_context.go index 733ef38a8..779917e50 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" ) // NewBlank initializes a blank Context suitable for testing @@ -17,22 +18,10 @@ type blankContext struct { authToken string authLogin string branch string - baseRepo GitHubRepository + baseRepo ghrepo.Interface remotes Remotes } -type ghRepo struct { - owner string - name string -} - -func (r ghRepo) RepoOwner() string { - return r.owner -} -func (r ghRepo) RepoName() string { - return r.name -} - func (c *blankContext) AuthToken() (string, error) { return c.authToken, nil } @@ -75,7 +64,7 @@ func (c *blankContext) SetRemotes(stubs map[string]string) { } } -func (c *blankContext) BaseRepo() (GitHubRepository, error) { +func (c *blankContext) BaseRepo() (ghrepo.Interface, error) { if c.baseRepo != nil { return c.baseRepo, nil } @@ -90,8 +79,5 @@ func (c *blankContext) BaseRepo() (GitHubRepository, error) { } func (c *blankContext) SetBaseRepo(nwo string) { - parts := strings.SplitN(nwo, "/", 2) - if len(parts) == 2 { - c.baseRepo = &ghRepo{parts[0], parts[1]} - } + c.baseRepo = ghrepo.FromFullName(nwo) } diff --git a/context/config_file.go b/context/config_file.go index 3079adef4..f3b07393b 100644 --- a/context/config_file.go +++ b/context/config_file.go @@ -10,6 +10,8 @@ import ( "gopkg.in/yaml.v3" ) +const defaultHostname = "github.com" + type configEntry struct { User string Token string `yaml:"oauth_token"` diff --git a/context/context.go b/context/context.go index c71e6d905..66bd3bda0 100644 --- a/context/context.go +++ b/context/context.go @@ -2,9 +2,9 @@ package context import ( "path" - "strings" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" "github.com/mitchellh/go-homedir" ) @@ -16,16 +16,10 @@ type Context interface { Branch() (string, error) SetBranch(string) Remotes() (Remotes, error) - BaseRepo() (GitHubRepository, error) + BaseRepo() (ghrepo.Interface, error) SetBaseRepo(string) } -// GitHubRepository is anything that can be mapped to an OWNER/REPO pair -type GitHubRepository interface { - RepoOwner() string - RepoName() string -} - // New initializes a Context that reads from the filesystem func New() Context { return &fsContext{} @@ -36,7 +30,7 @@ type fsContext struct { config *configEntry remotes Remotes branch string - baseRepo GitHubRepository + baseRepo ghrepo.Interface authToken string } @@ -115,7 +109,7 @@ func (c *fsContext) Remotes() (Remotes, error) { return c.remotes, nil } -func (c *fsContext) BaseRepo() (GitHubRepository, error) { +func (c *fsContext) BaseRepo() (ghrepo.Interface, error) { if c.baseRepo != nil { return c.baseRepo, nil } @@ -134,8 +128,5 @@ func (c *fsContext) BaseRepo() (GitHubRepository, error) { } func (c *fsContext) SetBaseRepo(nwo string) { - parts := strings.SplitN(nwo, "/", 2) - if len(parts) == 2 { - c.baseRepo = &ghRepo{parts[0], parts[1]} - } + c.baseRepo = ghrepo.FromFullName(nwo) } diff --git a/context/remote.go b/context/remote.go index 3a4e7f772..2018a9cde 100644 --- a/context/remote.go +++ b/context/remote.go @@ -6,10 +6,9 @@ import ( "strings" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" ) -const defaultHostname = "github.com" - // Remotes represents a set of git remotes type Remotes []*Remote @@ -75,39 +74,18 @@ func (r Remote) RepoOwner() string { // TODO: accept an interface instead of git.RemoteSet func translateRemotes(gitRemotes git.RemoteSet, urlTranslate func(*url.URL) *url.URL) (remotes Remotes) { for _, r := range gitRemotes { - var owner string - var repo string + var repo ghrepo.Interface if r.FetchURL != nil { - owner, repo, _ = repoFromURL(urlTranslate(r.FetchURL)) + repo, _ = ghrepo.FromURL(urlTranslate(r.FetchURL)) } - if r.PushURL != nil && owner == "" { - owner, repo, _ = repoFromURL(urlTranslate(r.PushURL)) + if r.PushURL != nil && repo == nil { + repo, _ = ghrepo.FromURL(urlTranslate(r.PushURL)) } remotes = append(remotes, &Remote{ Remote: r, - Owner: owner, - Repo: repo, + Owner: repo.RepoOwner(), + Repo: repo.RepoName(), }) } return } - -// RepoFromURL maps a URL to a GitHubRepository -func RepoFromURL(u *url.URL) (GitHubRepository, error) { - owner, repo, err := repoFromURL(u) - if err != nil { - return nil, err - } - return ghRepo{owner, repo}, nil -} - -func repoFromURL(u *url.URL) (string, string, error) { - if !strings.EqualFold(u.Hostname(), defaultHostname) { - return "", "", fmt.Errorf("unsupported hostname: %s", u.Hostname()) - } - parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 3) - if len(parts) < 2 { - return "", "", fmt.Errorf("invalid path: %s", u.Path) - } - return parts[0], strings.TrimSuffix(parts[1], ".git"), nil -} diff --git a/context/remote_test.go b/context/remote_test.go index 359fcaa7f..e083926d8 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -2,38 +2,11 @@ package context import ( "errors" - "net/url" "testing" "github.com/github/gh-cli/git" ) -func Test_repoFromURL(t *testing.T) { - u, _ := url.Parse("http://github.com/monalisa/octo-cat.git") - owner, repo, err := repoFromURL(u) - eq(t, err, nil) - eq(t, owner, "monalisa") - eq(t, repo, "octo-cat") -} - -func Test_repoFromURL_invalid(t *testing.T) { - cases := [][]string{ - []string{ - "https://example.com/one/two", - "unsupported hostname: example.com", - }, - []string{ - "/path/to/disk", - "unsupported hostname: ", - }, - } - for _, c := range cases { - u, _ := url.Parse(c[0]) - _, _, err := repoFromURL(u) - eq(t, err, errors.New(c[1])) - } -} - func Test_Remotes_FindByName(t *testing.T) { list := Remotes{ &Remote{Remote: &git.Remote{Name: "mona"}, Owner: "monalisa", Repo: "myfork"}, diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go new file mode 100644 index 000000000..76ab93a4b --- /dev/null +++ b/internal/ghrepo/repo.go @@ -0,0 +1,61 @@ +package ghrepo + +import ( + "fmt" + "net/url" + "strings" +) + +const defaultHostname = "github.com" + +type Interface interface { + RepoName() string + RepoOwner() string +} + +func New(owner, repo string) Interface { + return &ghRepo{ + owner: owner, + name: repo, + } +} +func FullName(r Interface) string { + return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName()) +} + +func FromFullName(nwo string) Interface { + r := ghRepo{} + parts := strings.SplitN(nwo, "/", 2) + if len(parts) == 2 { + r.owner, r.name = parts[0], parts[1] + } + return &r +} + +func FromURL(u *url.URL) (Interface, error) { + if !strings.EqualFold(u.Hostname(), defaultHostname) { + return nil, fmt.Errorf("unsupported hostname: %s", u.Hostname()) + } + parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 3) + if len(parts) < 2 { + return nil, fmt.Errorf("invalid path: %s", u.Path) + } + return New(parts[0], strings.TrimSuffix(parts[1], ".git")), nil +} + +func IsSame(a, b Interface) bool { + return strings.EqualFold(a.RepoOwner(), b.RepoOwner()) && + strings.EqualFold(a.RepoName(), b.RepoName()) +} + +type ghRepo struct { + owner string + name string +} + +func (r ghRepo) RepoOwner() string { + return r.owner +} +func (r ghRepo) RepoName() string { + return r.name +} diff --git a/internal/ghrepo/repo_test.go b/internal/ghrepo/repo_test.go new file mode 100644 index 000000000..6ff775c84 --- /dev/null +++ b/internal/ghrepo/repo_test.go @@ -0,0 +1,40 @@ +package ghrepo + +import ( + "net/url" + "testing" +) + +func Test_repoFromURL(t *testing.T) { + u, _ := url.Parse("http://github.com/monalisa/octo-cat.git") + repo, err := FromURL(u) + if err != nil { + t.Fatalf("got error %q", err) + } + if repo.RepoOwner() != "monalisa" { + t.Errorf("got owner %q", repo.RepoOwner()) + } + if repo.RepoName() != "octo-cat" { + t.Errorf("got name %q", repo.RepoName()) + } +} + +func Test_repoFromURL_invalid(t *testing.T) { + cases := [][]string{ + []string{ + "https://example.com/one/two", + "unsupported hostname: example.com", + }, + []string{ + "/path/to/disk", + "unsupported hostname: ", + }, + } + for _, c := range cases { + u, _ := url.Parse(c[0]) + _, err := FromURL(u) + if err == nil || err.Error() != c[1] { + t.Errorf("got %q", err) + } + } +} From 90f6a73ba59ed0057e58f56f02201923211cc565 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 22 Jan 2020 22:57:54 +0100 Subject: [PATCH 13/23] Extract helper to print a URL --- command/issue.go | 14 +++++++++----- command/pr_create.go | 6 +----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/command/issue.go b/command/issue.go index 139ab3337..67889cf53 100644 --- a/command/issue.go +++ b/command/issue.go @@ -354,11 +354,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { url.QueryEscape(body), ) // TODO could exceed max url length for explorer - url, err := url.Parse(openURL) - if err != nil { - return err - } - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s%s in your browser.\n", url.Host, url.Path) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) return utils.OpenInBrowser(openURL) } else if action == SubmitAction { params := map[string]interface{}{ @@ -417,3 +413,11 @@ func labelList(issue api.Issue) string { } return list } + +func displayURL(urlStr string) string { + u, err := url.Parse(urlStr) + if err != nil { + return urlStr + } + return u.Hostname() + u.Path +} diff --git a/command/pr_create.go b/command/pr_create.go index a72dff7ff..352fe36b0 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -149,11 +149,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { url.QueryEscape(body), ) // TODO could exceed max url length for explorer - url, err := url.Parse(openURL) - if err != nil { - return err - } - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s%s in your browser.\n", url.Host, url.Path) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) return utils.OpenInBrowser(openURL) } else { panic("Unreachable state") From ea09883b07bb930f2093d680d207184f69718327 Mon Sep 17 00:00:00 2001 From: Amanda Pinsker Date: Wed, 22 Jan 2020 14:45:31 -0800 Subject: [PATCH 14/23] Restyle auth page --- context/config_success.go | 44 ++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/context/config_success.go b/context/config_success.go index 3a6fe978b..1919fc1a4 100644 --- a/context/config_success.go +++ b/context/config_success.go @@ -6,27 +6,37 @@ const oauthSuccessPage = ` Success: GitHub CLI - -

Authentication successful.

-

- You have completed logging into GitHub CLI.
- You may now close this tab and return to the terminal. -

- + +
+

Successfully authenticated GitHub CLI

+

You may now close this tab and return to the terminal.

+
` From 305410cdeef7ca2d75647414236489e40169d6ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 10:26:57 +0100 Subject: [PATCH 15/23] Fix usage synopsis for `pr view` Indicate that the argument is optional --- command/pr.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index 5204c1b3d..76eae4a3e 100644 --- a/command/pr.go +++ b/command/pr.go @@ -67,9 +67,13 @@ var prStatusCmd = &cobra.Command{ RunE: prStatus, } var prViewCmd = &cobra.Command{ - Use: "view { | | }", + Use: "view [{ | | }]", Short: "View a pull request in the browser", - RunE: prView, + Long: `View a pull request specified by the argument in the browser. + +Without an argument, the pull request that belongs to the current +branch is opened.`, + RunE: prView, } func prStatus(cmd *cobra.Command, args []string) error { From f5ad43458ccb15bd73b6f19da995271ebf19b8a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 10:28:29 +0100 Subject: [PATCH 16/23] Avoid using `<...>` in docs When converted to HTML docs, these get interpreted as HTML tags. In theory, we could encapsulate these bits in backticks, but the docs are already in raw Go string literals, and we can't easily escape backticks in that context. Instead, just avoid using `<>` for now. --- command/issue.go | 2 +- command/pr.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/command/issue.go b/command/issue.go index 139ab3337..f48549522 100644 --- a/command/issue.go +++ b/command/issue.go @@ -47,7 +47,7 @@ var issueCmd = &cobra.Command{ An issue can be supplied as argument in any of the following formats: - by number, e.g. "123"; or -- by URL, e.g. "https://github.com///issues/123".`, +- by URL, e.g. "https://github.com/OWNER/REPO/issues/123".`, } var issueCreateCmd = &cobra.Command{ Use: "create", diff --git a/command/pr.go b/command/pr.go index 76eae4a3e..e210d027c 100644 --- a/command/pr.go +++ b/command/pr.go @@ -42,8 +42,8 @@ var prCmd = &cobra.Command{ A pull request can be supplied as argument in any of the following formats: - by number, e.g. "123"; -- by URL, e.g. "https://github.com///pull/123"; or -- by the name of its head branch, e.g. "patch-1" or ":patch-1".`, +- by URL, e.g. "https://github.com/OWNER/REPO/pull/123"; or +- by the name of its head branch, e.g. "patch-1" or "OWNER:patch-1".`, } var prCheckoutCmd = &cobra.Command{ Use: "checkout { | | }", From 02f5a689374c9b3eb65f734ad57008a58148e7e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 10:45:28 +0100 Subject: [PATCH 17/23] Move main package to under `cmd/` It's a Go convention that main packages (one per each binary produced) are scoped under `cmd/`. https://github.com/github/go-lang/blob/master/docs/style-guide.md#directory-structure-and-filenames-layout --- .goreleaser.yml | 1 + Makefile | 2 +- main.go => cmd/gh/main.go | 0 3 files changed, 2 insertions(+), 1 deletion(-) rename main.go => cmd/gh/main.go (100%) diff --git a/.goreleaser.yml b/.goreleaser.yml index b3ee2336c..c8ffa8997 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -8,6 +8,7 @@ before: - go mod tidy builds: - binary: bin/gh + main: ./cmd/gh ldflags: - -s -w -X github.com/github/gh-cli/command.Version={{.Version}} -X github.com/github/gh-cli/command.BuildDate={{time "2006-01-02"}} - -X github.com/github/gh-cli/context.oauthClientID={{.Env.GH_OAUTH_CLIENT_ID}} diff --git a/Makefile b/Makefile index d17f749bf..d3cb03449 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ ifdef GH_OAUTH_CLIENT_SECRET endif bin/gh: $(BUILD_FILES) - @go build -ldflags "$(LDFLAGS)" -o "$@" + @go build -ldflags "$(LDFLAGS)" -o "$@" ./cmd/gh test: go test ./... diff --git a/main.go b/cmd/gh/main.go similarity index 100% rename from main.go rename to cmd/gh/main.go From 6282a3c24ede817b8ddd86f29a6176f1dafbdf91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 11:00:23 +0100 Subject: [PATCH 18/23] Improve readability of error output Ensure a blank line between error and usage output --- cmd/gh/main.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 726b50784..822e86ec6 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -28,6 +28,9 @@ func main() { fmt.Fprintln(os.Stderr, err) _, isFlagError := err.(command.FlagError) if isFlagError || strings.HasPrefix(err.Error(), "unknown command ") { + if !strings.HasSuffix(err.Error(), "\n") { + fmt.Fprintln(os.Stderr) + } fmt.Fprintln(os.Stderr, cmd.UsageString()) } os.Exit(1) From eb6541d8d676e0b25b80fd62e4585324b5764c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 11:03:06 +0100 Subject: [PATCH 19/23] Fix CI build --- .github/workflows/go.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 2dab75d5b..40ab1acb4 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -23,4 +23,4 @@ jobs: - name: Build run: | go test ./... - go build -v . + go build -v ./cmd/gh From 537b0a842980b8ce3940b267d2c97c4d9441171f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 12:48:10 +0100 Subject: [PATCH 20/23] Friendlier output for network connectivity errors For `net.DNSError`, the full error message can be scary. Instead, print "error connecting to HOST" and hint that the user should check their internet connection or githubstatus.com. When $DEBUG is set, the full DNS error is printed like before. Fixes #206 --- cmd/gh/main.go | 37 ++++++++++++++++----- cmd/gh/main_test.go | 78 +++++++++++++++++++++++++++++++++++++++++++++ command/root.go | 12 +++++-- 3 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 cmd/gh/main_test.go diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 822e86ec6..15925a6a9 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -1,7 +1,10 @@ package main import ( + "errors" "fmt" + "io" + "net" "os" "path" "strings" @@ -12,6 +15,7 @@ import ( "github.com/github/gh-cli/utils" "github.com/mattn/go-isatty" "github.com/mgutz/ansi" + "github.com/spf13/cobra" ) var updaterEnabled = "" @@ -24,15 +28,10 @@ func main() { updateMessageChan <- rel }() + hasDebug := os.Getenv("DEBUG") != "" + if cmd, err := command.RootCmd.ExecuteC(); err != nil { - fmt.Fprintln(os.Stderr, err) - _, isFlagError := err.(command.FlagError) - if isFlagError || strings.HasPrefix(err.Error(), "unknown command ") { - if !strings.HasSuffix(err.Error(), "\n") { - fmt.Fprintln(os.Stderr) - } - fmt.Fprintln(os.Stderr, cmd.UsageString()) - } + printError(os.Stderr, err, cmd, hasDebug) os.Exit(1) } @@ -49,6 +48,28 @@ func main() { } } +func printError(out io.Writer, err error, cmd *cobra.Command, debug bool) { + var dnsError *net.DNSError + if errors.As(err, &dnsError) { + fmt.Fprintf(out, "error connecting to %s\n", dnsError.Name) + if debug { + fmt.Fprintln(out, dnsError) + } + fmt.Fprintln(out, "check your internet connection or githubstatus.com") + return + } + + fmt.Fprintln(out, err) + + var flagError *command.FlagError + if errors.As(err, &flagError) || strings.HasPrefix(err.Error(), "unknown command ") { + if !strings.HasSuffix(err.Error(), "\n") { + fmt.Fprintln(out) + } + fmt.Fprintln(out, cmd.UsageString()) + } +} + func shouldCheckForUpdate() bool { errFd := os.Stderr.Fd() return updaterEnabled != "" && (isatty.IsTerminal(errFd) || isatty.IsCygwinTerminal(errFd)) diff --git a/cmd/gh/main_test.go b/cmd/gh/main_test.go new file mode 100644 index 000000000..3a62f6470 --- /dev/null +++ b/cmd/gh/main_test.go @@ -0,0 +1,78 @@ +package main + +import ( + "bytes" + "errors" + "fmt" + "net" + "testing" + + "github.com/github/gh-cli/command" + "github.com/spf13/cobra" +) + +func Test_printError(t *testing.T) { + cmd := &cobra.Command{} + + type args struct { + err error + cmd *cobra.Command + debug bool + } + tests := []struct { + name string + args args + wantOut string + }{ + { + name: "generic error", + args: args{ + err: errors.New("the app exploded"), + cmd: nil, + debug: false, + }, + wantOut: "the app exploded\n", + }, + { + name: "DNS error", + args: args{ + err: fmt.Errorf("DNS oopsie: %w", &net.DNSError{ + Name: "api.github.com", + }), + cmd: nil, + debug: false, + }, + wantOut: `error connecting to api.github.com +check your internet connection or githubstatus.com +`, + }, + { + name: "Cobra flag error", + args: args{ + err: &command.FlagError{Err: errors.New("unknown flag --foo")}, + cmd: cmd, + debug: false, + }, + wantOut: "unknown flag --foo\n\nUsage:\n\n", + }, + { + name: "unknown Cobra command error", + args: args{ + err: errors.New("unknown command foo"), + cmd: cmd, + debug: false, + }, + wantOut: "unknown command foo\n\nUsage:\n\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := &bytes.Buffer{} + printError(out, tt.args.err, tt.args.cmd, tt.args.debug) + if gotOut := out.String(); gotOut != tt.wantOut { + t.Errorf("printError() = %q, want %q", gotOut, tt.wantOut) + } + }) + } +} diff --git a/command/root.go b/command/root.go index 5703f30c2..4141562a7 100644 --- a/command/root.go +++ b/command/root.go @@ -35,13 +35,21 @@ func init() { // RootCmd.PersistentFlags().BoolP("verbose", "V", false, "enable verbose output") RootCmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { - return FlagError{err} + return &FlagError{Err: err} }) } // FlagError is the kind of error raised in flag processing type FlagError struct { - error + Err error +} + +func (fe FlagError) Error() string { + return fe.Err.Error() +} + +func (fe FlagError) Unwrap() error { + return fe.Err } // RootCmd is the entry point of command-line execution From f58dd040745ee68d54e21ddcba01ccaf0f358a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 13:06:21 +0100 Subject: [PATCH 21/23] Avoid saying "number as argument" for `issue/pr view` Since issue URLs, PR URLs, and PR branch names are all accepted as arguments, avoid explicitly requesting "number" as argument. --- command/issue.go | 2 +- command/pr.go | 4 ---- command/pr_test.go | 2 +- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/command/issue.go b/command/issue.go index 139ab3337..ab966fa97 100644 --- a/command/issue.go +++ b/command/issue.go @@ -68,7 +68,7 @@ var issueViewCmd = &cobra.Command{ Use: "view { | | }", Args: func(cmd *cobra.Command, args []string) error { if len(args) < 1 { - return errors.New("requires an issue number as an argument") + return FlagError{errors.New("issue required as argument")} } return nil }, diff --git a/command/pr.go b/command/pr.go index 5204c1b3d..7b31faf51 100644 --- a/command/pr.go +++ b/command/pr.go @@ -285,10 +285,6 @@ func prView(cmd *cobra.Command, args []string) error { } else { pr, err = api.PullRequestForBranch(apiClient, baseRepo, branchWithOwner) if err != nil { - var notFoundErr *api.NotFoundError - if errors.As(err, ¬FoundErr) { - return fmt.Errorf("%s. To open a specific pull request use the pull request's number as an argument", err) - } return err } diff --git a/command/pr_test.go b/command/pr_test.go index 5a324cab1..9c17c3d4f 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -359,7 +359,7 @@ func TestPRView_noResultsForBranch(t *testing.T) { defer restoreCmd() _, err := RunCommand(prViewCmd, "pr view") - if err == nil || err.Error() != `no open pull requests found for branch "blueberries". To open a specific pull request use the pull request's number as an argument` { + if err == nil || err.Error() != `no open pull requests found for branch "blueberries"` { t.Errorf("error running command `pr view`: %v", err) } 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 22/23] 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) + } +} From 30b4eab8e0346f5d54e166ab5381400f93607b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Jan 2020 14:24:22 +0100 Subject: [PATCH 23/23] Use ghrepo.FullName in tests --- command/pr_create_test.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 047118972..94f040f27 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -13,6 +13,7 @@ import ( "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" "github.com/github/gh-cli/git" + "github.com/github/gh-cli/internal/ghrepo" "github.com/github/gh-cli/test" "github.com/github/gh-cli/utils" ) @@ -220,9 +221,7 @@ func Test_resolvedRemotes_clonedFork(t *testing.T) { if err != nil { t.Fatalf("got %v", err) } - if baseRepo.RepoOwner() != "PARENTOWNER" { - t.Errorf("got owner %q", baseRepo.RepoOwner()) - } + 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") @@ -232,9 +231,7 @@ func Test_resolvedRemotes_clonedFork(t *testing.T) { if err != nil { t.Fatalf("got %v", err) } - if headRepo.RepoOwner() != "OWNER" { - t.Errorf("got owner %q", headRepo.RepoOwner()) - } + eq(t, ghrepo.FullName(headRepo), "OWNER/REPO") headRemote, err := resolved.RemoteForRepo(headRepo) if err != nil { t.Fatalf("got %v", err) @@ -279,12 +276,7 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { 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()) - } + eq(t, ghrepo.FullName(baseRepo), "NEWOWNER/NEWNAME") baseRemote, err := resolved.RemoteForRepo(baseRepo) if err != nil { t.Fatalf("got %v", err) @@ -297,9 +289,7 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { if err != nil { t.Fatalf("got %v", err) } - if headRepo.RepoOwner() != "MYSELF" { - t.Errorf("got owner %q", headRepo.RepoOwner()) - } + eq(t, ghrepo.FullName(headRepo), "MYSELF/REPO") headRemote, err := resolved.RemoteForRepo(headRepo) if err != nil { t.Fatalf("got %v", err)