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 1/2] 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 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 2/2] 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)