From 3ea71e6eb68a6385522a4f6c1b0c2a54333a2353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Jun 2020 19:51:26 +0200 Subject: [PATCH 1/4] Add ability to parse non-GitHub.com git remotes --- api/queries_repo.go | 6 ++++ command/pr_create.go | 3 +- command/pr_create_test.go | 16 ++++------ context/blank_context.go | 3 +- context/remote.go | 15 ++++++---- context/remote_test.go | 18 +++++------ internal/ghrepo/repo.go | 40 ++++++++++++++++++------- internal/ghrepo/repo_test.go | 58 +++++++++++++++++++++++++++++++++--- 8 files changed, 114 insertions(+), 45 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 22d42cfbb..d0ff60aa9 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -51,6 +51,12 @@ func (r Repository) RepoName() string { return r.Name } +// RepoHost is the GitHub hostname of the repository +func (r Repository) RepoHost() string { + // FIXME: inherit hostname from the server + return "github.com" +} + // IsFork is true when this repository has a parent repository func (r Repository) IsFork() bool { return r.Parent != nil diff --git a/command/pr_create.go b/command/pr_create.go index b2dd48697..d46916f5e 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -304,8 +304,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } headRemote = &context.Remote{ Remote: gitRemote, - Owner: headRepo.RepoOwner(), - Repo: headRepo.RepoName(), + Repo: headRepo, } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 2eb0872c7..ff4f3bf1e 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -9,6 +9,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" ) @@ -759,13 +760,11 @@ func Test_determineTrackingBranch_noMatch(t *testing.T) { remotes := context.Remotes{ &context.Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "hubot", - Repo: "Spoon-Knife", + Repo: ghrepo.New("hubot", "Spoon-Knife"), }, &context.Remote{ Remote: &git.Remote{Name: "upstream"}, - Owner: "octocat", - Repo: "Spoon-Knife", + Repo: ghrepo.New("octocat", "Spoon-Knife"), }, } @@ -786,13 +785,11 @@ func Test_determineTrackingBranch_hasMatch(t *testing.T) { remotes := context.Remotes{ &context.Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "hubot", - Repo: "Spoon-Knife", + Repo: ghrepo.New("hubot", "Spoon-Knife"), }, &context.Remote{ Remote: &git.Remote{Name: "upstream"}, - Owner: "octocat", - Repo: "Spoon-Knife", + Repo: ghrepo.New("octocat", "Spoon-Knife"), }, } @@ -819,8 +816,7 @@ func Test_determineTrackingBranch_respectTrackingConfig(t *testing.T) { remotes := context.Remotes{ &context.Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "hubot", - Repo: "Spoon-Knife", + Repo: ghrepo.New("hubot", "Spoon-Knife"), }, } diff --git a/context/blank_context.go b/context/blank_context.go index 3ea657abe..151e5a9ed 100644 --- a/context/blank_context.go +++ b/context/blank_context.go @@ -62,8 +62,7 @@ func (c *blankContext) SetRemotes(stubs map[string]string) { ownerWithName := strings.SplitN(repo, "/", 2) c.remotes = append(c.remotes, &Remote{ Remote: &git.Remote{Name: remoteName}, - Owner: ownerWithName[0], - Repo: ownerWithName[1], + Repo: ghrepo.New(ownerWithName[0], ownerWithName[1]), }) } } diff --git a/context/remote.go b/context/remote.go index b40533a54..79ba0e8c8 100644 --- a/context/remote.go +++ b/context/remote.go @@ -57,18 +57,22 @@ func (r Remotes) Less(i, j int) bool { // Remote represents a git remote mapped to a GitHub repository type Remote struct { *git.Remote - Owner string - Repo string + Repo ghrepo.Interface } // RepoName is the name of the GitHub repository func (r Remote) RepoName() string { - return r.Repo + return r.Repo.RepoName() } // RepoOwner is the name of the GitHub account that owns the repo func (r Remote) RepoOwner() string { - return r.Owner + return r.Repo.RepoOwner() +} + +// RepoHost is the GitHub hostname that the remote points to +func (r Remote) RepoHost() string { + return r.Repo.RepoHost() } // TODO: accept an interface instead of git.RemoteSet @@ -86,8 +90,7 @@ func translateRemotes(gitRemotes git.RemoteSet, urlTranslate func(*url.URL) *url } remotes = append(remotes, &Remote{ Remote: r, - Owner: repo.RepoOwner(), - Repo: repo.RepoName(), + Repo: repo, }) } return diff --git a/context/remote_test.go b/context/remote_test.go index 1e0f47ba0..6d5801e26 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -22,9 +22,9 @@ func eq(t *testing.T, got interface{}, expected interface{}) { func Test_Remotes_FindByName(t *testing.T) { list := Remotes{ - &Remote{Remote: &git.Remote{Name: "mona"}, Owner: "monalisa", Repo: "myfork"}, - &Remote{Remote: &git.Remote{Name: "origin"}, Owner: "monalisa", Repo: "octo-cat"}, - &Remote{Remote: &git.Remote{Name: "upstream"}, Owner: "hubot", Repo: "tools"}, + &Remote{Remote: &git.Remote{Name: "mona"}, Repo: ghrepo.New("monalisa", "myfork")}, + &Remote{Remote: &git.Remote{Name: "origin"}, Repo: ghrepo.New("monalisa", "octo-cat")}, + &Remote{Remote: &git.Remote{Name: "upstream"}, Repo: ghrepo.New("hubot", "tools")}, } r, err := list.FindByName("upstream", "origin") @@ -84,13 +84,11 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { Remotes: Remotes{ &Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "OWNER", - Repo: "REPO", + Repo: ghrepo.New("OWNER", "REPO"), }, &Remote{ Remote: &git.Remote{Name: "fork"}, - Owner: "MYSELF", - Repo: "REPO", + Repo: ghrepo.New("MYSELF", "REPO"), }, }, Network: api.RepoNetworkResult{ @@ -157,8 +155,7 @@ func Test_resolvedRemotes_forkLookup(t *testing.T) { Remotes: Remotes{ &Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "OWNER", - Repo: "REPO", + Repo: ghrepo.New("OWNER", "REPO"), }, }, Network: api.RepoNetworkResult{ @@ -190,8 +187,7 @@ func Test_resolvedRemotes_clonedFork(t *testing.T) { Remotes: Remotes{ &Remote{ Remote: &git.Remote{Name: "origin"}, - Owner: "OWNER", - Repo: "REPO", + Repo: ghrepo.New("OWNER", "REPO"), }, }, Network: api.RepoNetworkResult{ diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 37fecbe8d..93493a3da 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -6,13 +6,13 @@ import ( "strings" ) -// TODO these are sprinkled across command, context, config, and ghrepo const defaultHostname = "github.com" // Interface describes an object that represents a GitHub repository type Interface interface { RepoName() string RepoOwner() string + RepoHost() string } // New instantiates a GitHub repository from owner and name arguments @@ -39,32 +39,52 @@ func FromFullName(nwo string) (Interface, error) { return &r, nil } -// FromURL extracts the GitHub repository information from a URL +// FromURL extracts the GitHub repository information from a git remote URL func FromURL(u *url.URL) (Interface, error) { - if !strings.EqualFold(u.Hostname(), defaultHostname) && !strings.EqualFold(u.Hostname(), "www."+defaultHostname) { - return nil, fmt.Errorf("unsupported hostname: %s", u.Hostname()) + if u.Hostname() == "" { + return nil, fmt.Errorf("no hostname detected") } - parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 3) - if len(parts) < 2 { + + parts := strings.SplitN(strings.Trim(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 + + return &ghRepo{ + owner: parts[0], + name: strings.TrimSuffix(parts[1], ".git"), + hostname: normalizeHostname(u.Hostname()), + }, nil +} + +func normalizeHostname(h string) string { + return strings.ToLower(strings.TrimPrefix(h, "www.")) } // IsSame compares two GitHub repositories func IsSame(a, b Interface) bool { return strings.EqualFold(a.RepoOwner(), b.RepoOwner()) && - strings.EqualFold(a.RepoName(), b.RepoName()) + strings.EqualFold(a.RepoName(), b.RepoName()) && + normalizeHostname(a.RepoHost()) == normalizeHostname(b.RepoHost()) } type ghRepo struct { - owner string - name string + owner string + name string + hostname string } func (r ghRepo) RepoOwner() string { return r.owner } + func (r ghRepo) RepoName() string { return r.name } + +func (r ghRepo) RepoHost() string { + if r.hostname != "" { + return r.hostname + } + return defaultHostname +} diff --git a/internal/ghrepo/repo_test.go b/internal/ghrepo/repo_test.go index fef04fb8c..3c421bd64 100644 --- a/internal/ghrepo/repo_test.go +++ b/internal/ghrepo/repo_test.go @@ -12,31 +12,78 @@ func Test_repoFromURL(t *testing.T) { name string input string result string + host string err error }{ { name: "github.com URL", input: "https://github.com/monalisa/octo-cat.git", result: "monalisa/octo-cat", + host: "github.com", + err: nil, + }, + { + name: "github.com URL with trailing slash", + input: "https://github.com/monalisa/octo-cat/", + result: "monalisa/octo-cat", + host: "github.com", err: nil, }, { name: "www.github.com URL", input: "http://www.GITHUB.com/monalisa/octo-cat.git", result: "monalisa/octo-cat", + host: "github.com", err: nil, }, { - name: "unsupported hostname", - input: "https://example.com/one/two", + name: "too many path components", + input: "https://github.com/monalisa/octo-cat/pulls", result: "", - err: errors.New("unsupported hostname: example.com"), + host: "", + err: errors.New("invalid path: /monalisa/octo-cat/pulls"), + }, + { + name: "non-GitHub hostname", + input: "https://example.com/one/two", + result: "one/two", + host: "example.com", + err: nil, }, { name: "filesystem path", input: "/path/to/file", result: "", - err: errors.New("unsupported hostname: "), + host: "", + err: errors.New("no hostname detected"), + }, + { + name: "filesystem path with scheme", + input: "file:///path/to/file", + result: "", + host: "", + err: errors.New("no hostname detected"), + }, + { + name: "github.com SSH URL", + input: "ssh://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + host: "github.com", + err: nil, + }, + { + name: "github.com HTTPS+SSH URL", + input: "https+ssh://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + host: "github.com", + err: nil, + }, + { + name: "github.com git URL", + input: "git://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + host: "github.com", + err: nil, }, } @@ -61,6 +108,9 @@ func Test_repoFromURL(t *testing.T) { if tt.result != got { t.Errorf("expected %q, got %q", tt.result, got) } + if tt.host != repo.RepoHost() { + t.Errorf("expected %q, got %q", tt.host, repo.RepoHost()) + } }) } } From e4448c73e4cabe601c904a467c5520ed82065e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 23 Jun 2020 20:06:50 +0200 Subject: [PATCH 2/4] Respect original hostname in `formatRemoteURL()` --- command/pr_checkout.go | 6 ++++-- command/pr_create.go | 2 +- command/repo.go | 14 +++++++++----- command/root.go | 11 +++++------ command/root_test.go | 6 ++++-- 5 files changed, 23 insertions(+), 16 deletions(-) diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 8ffa39210..ec60d3de0 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -33,7 +33,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { baseRemote, _ := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()) // baseRemoteSpec is a repository URL or a remote name to be used in git fetch - baseURLOrName := formatRemoteURL(cmd, ghrepo.FullName(baseRepo)) + baseURLOrName := formatRemoteURL(cmd, baseRepo) if baseRemote != nil { baseURLOrName = baseRemote.Name } @@ -84,7 +84,9 @@ func prCheckout(cmd *cobra.Command, args []string) error { remote := baseURLOrName mergeRef := ref if pr.MaintainerCanModify { - remote = formatRemoteURL(cmd, fmt.Sprintf("%s/%s", pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name)) + // FIXME: inherit hostname from the server + headRepo := ghrepo.New(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name) + remote = formatRemoteURL(cmd, headRepo) mergeRef = fmt.Sprintf("refs/heads/%s", pr.HeadRefName) } if mc, err := git.Config(fmt.Sprintf("branch.%s.merge", newBranchName)); err != nil || mc == "" { diff --git a/command/pr_create.go b/command/pr_create.go index d46916f5e..11d9ec523 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -295,7 +295,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { // In either case, we want to add the head repo as a new git remote so we // can push to it. if headRemote == nil { - headRepoURL := formatRemoteURL(cmd, ghrepo.FullName(headRepo)) + headRepoURL := formatRemoteURL(cmd, headRepo) // TODO: prevent clashes with another remote of a same name gitRemote, err := git.AddRemote("fork", headRepoURL) diff --git a/command/repo.go b/command/repo.go index 7c15945aa..83788fe78 100644 --- a/command/repo.go +++ b/command/repo.go @@ -186,7 +186,11 @@ func repoClone(cmd *cobra.Command, args []string) error { } cloneURL = currentUser + "/" + cloneURL } - cloneURL = formatRemoteURL(cmd, cloneURL) + repo, err := ghrepo.FromFullName(cloneURL) + if err != nil { + return err + } + cloneURL = formatRemoteURL(cmd, repo) } var repo ghrepo.Interface @@ -221,7 +225,7 @@ func repoClone(cmd *cobra.Command, args []string) error { } func addUpstreamRemote(cmd *cobra.Command, parentRepo ghrepo.Interface, cloneDir string) error { - upstreamURL := formatRemoteURL(cmd, ghrepo.FullName(parentRepo)) + upstreamURL := formatRemoteURL(cmd, parentRepo) cloneCmd := git.GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL) cloneCmd.Stdout = os.Stdout @@ -322,7 +326,7 @@ func repoCreate(cmd *cobra.Command, args []string) error { fmt.Fprintln(out, repo.URL) } - remoteURL := formatRemoteURL(cmd, ghrepo.FullName(repo)) + remoteURL := formatRemoteURL(cmd, repo) if projectDirErr == nil { _, err = git.AddRemote("origin", remoteURL) @@ -497,7 +501,7 @@ func repoFork(cmd *cobra.Command, args []string) error { fmt.Fprintf(out, "%s Renamed %s remote to %s\n", greenCheck, utils.Bold(remoteName), utils.Bold(renameTarget)) } - forkedRepoCloneURL := formatRemoteURL(cmd, ghrepo.FullName(forkedRepo)) + forkedRepoCloneURL := formatRemoteURL(cmd, forkedRepo) _, err = git.AddRemote(remoteName, forkedRepoCloneURL) if err != nil { @@ -515,7 +519,7 @@ func repoFork(cmd *cobra.Command, args []string) error { } } if cloneDesired { - forkedRepoCloneURL := formatRemoteURL(cmd, ghrepo.FullName(forkedRepo)) + forkedRepoCloneURL := formatRemoteURL(cmd, forkedRepo) cloneDir, err := runClone(forkedRepoCloneURL, []string{}) if err != nil { return fmt.Errorf("failed to clone fork: %w", err) diff --git a/command/root.go b/command/root.go index 693603e05..3d53c3a71 100644 --- a/command/root.go +++ b/command/root.go @@ -336,23 +336,22 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return baseRepo, nil } -func formatRemoteURL(cmd *cobra.Command, fullRepoName string) string { +func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string { ctx := contextForCommand(cmd) - protocol := "https" + var protocol string cfg, err := ctx.Config() if err != nil { fmt.Fprintf(colorableErr(cmd), "%s failed to load config: %s. using defaults\n", utils.Yellow("!"), err) } else { - cfgProtocol, _ := cfg.Get(defaultHostname, "git_protocol") - protocol = cfgProtocol + protocol, _ = cfg.Get(repo.RepoHost(), "git_protocol") } if protocol == "ssh" { - return fmt.Sprintf("git@%s:%s.git", defaultHostname, fullRepoName) + return fmt.Sprintf("git@%s:%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) } - return fmt.Sprintf("https://%s/%s.git", defaultHostname, fullRepoName) + return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) } func determineEditor(cmd *cobra.Command) (string, error) { diff --git a/command/root_test.go b/command/root_test.go index dbed9628d..7ca938d6c 100644 --- a/command/root_test.go +++ b/command/root_test.go @@ -2,6 +2,8 @@ package command import ( "testing" + + "github.com/cli/cli/internal/ghrepo" ) func TestChangelogURL(t *testing.T) { @@ -43,7 +45,7 @@ func TestChangelogURL(t *testing.T) { func TestRemoteURLFormatting_no_config(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") - result := formatRemoteURL(repoForkCmd, "OWNER/REPO") + result := formatRemoteURL(repoForkCmd, ghrepo.New("OWNER", "REPO")) eq(t, result, "https://github.com/OWNER/REPO.git") } @@ -56,6 +58,6 @@ hosts: git_protocol: ssh ` initBlankContext(cfg, "OWNER/REPO", "master") - result := formatRemoteURL(repoForkCmd, "OWNER/REPO") + result := formatRemoteURL(repoForkCmd, ghrepo.New("OWNER", "REPO")) eq(t, result, "git@github.com:OWNER/REPO.git") } From 446a4111f768e8c0b4718e35a6a6dd273018fc98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 2 Jul 2020 20:36:10 +0200 Subject: [PATCH 3/4] Respect hostnames when resolving git remotes and URL args to repos --- api/queries_repo.go | 31 +++++++++++++++----- command/issue.go | 48 ++++--------------------------- command/issue_lookup.go | 64 +++++++++++++++++++++++++++++++++++++++++ command/issue_test.go | 18 ++++-------- command/pr_checkout.go | 3 +- command/pr_lookup.go | 26 ++++++++++++----- context/context.go | 22 ++++++++++---- internal/ghrepo/repo.go | 9 ++++++ 8 files changed, 143 insertions(+), 78 deletions(-) create mode 100644 command/issue_lookup.go diff --git a/api/queries_repo.go b/api/queries_repo.go index d0ff60aa9..ba3256f9c 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -34,6 +34,9 @@ type Repository struct { } Parent *Repository + + // pseudo-field that keeps track of host name of this repo + hostname string } // RepositoryOwner is the owner of a GitHub repository @@ -53,8 +56,7 @@ func (r Repository) RepoName() string { // RepoHost is the GitHub hostname of the repository func (r Repository) RepoHost() string { - // FIXME: inherit hostname from the server - return "github.com" + return r.hostname } // IsFork is true when this repository has a parent repository @@ -109,7 +111,7 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { return nil, err } - return &result.Repository, nil + return initRepoHostname(&result.Repository, repo.RepoHost()), nil } // RepoParent finds out the parent repository of a fork @@ -139,7 +141,7 @@ func RepoParent(client *Client, repo ghrepo.Interface) (ghrepo.Interface, error) return nil, nil } - parent := ghrepo.New(query.Repository.Parent.Owner.Login, query.Repository.Parent.Name) + parent := ghrepo.NewWithHost(query.Repository.Parent.Owner.Login, query.Repository.Parent.Name, repo.RepoHost()) return parent, nil } @@ -151,6 +153,11 @@ type RepoNetworkResult struct { // RepoNetwork inspects the relationship between multiple GitHub repositories func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, error) { + var hostname string + if len(repos) > 0 { + hostname = repos[0].RepoHost() + } + queries := make([]string, 0, len(repos)) for i, repo := range repos { queries = append(queries, fmt.Sprintf(` @@ -233,7 +240,7 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e if err := decoder.Decode(&repo); err != nil { return result, err } - result.Repositories = append(result.Repositories, &repo) + result.Repositories = append(result.Repositories, initRepoHostname(&repo, hostname)) } else { return result, fmt.Errorf("unknown GraphQL result key %q", name) } @@ -241,6 +248,14 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e return result, nil } +func initRepoHostname(repo *Repository, hostname string) *Repository { + repo.hostname = hostname + if repo.Parent != nil { + repo.Parent.hostname = hostname + } + return repo +} + // repositoryV3 is the repository result from GitHub API v3 type repositoryV3 struct { NodeID string @@ -271,6 +286,7 @@ func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { Login: result.Owner.Login, }, ViewerPermission: "WRITE", + hostname: repo.RepoHost(), }, nil } @@ -312,7 +328,7 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { // `affiliations` condition, to guard against versions of GitHub with a // faulty `affiliations` implementation if len(forks) > 0 && forks[0].ViewerCanPush() { - return &forks[0], nil + return initRepoHostname(&forks[0], repo.RepoHost()), nil } return nil, &NotFoundError{errors.New("no fork found")} } @@ -374,7 +390,8 @@ func RepoCreate(client *Client, input RepoCreateInput) (*Repository, error) { return nil, err } - return &response.CreateRepository.Repository, nil + // FIXME: support Enterprise hosts + return initRepoHostname(&response.CreateRepository.Repository, "github.com"), nil } func RepositoryReadme(client *Client, fullName string) (string, error) { diff --git a/command/issue.go b/command/issue.go index 814daf3ae..f790b3d05 100644 --- a/command/issue.go +++ b/command/issue.go @@ -1,11 +1,9 @@ package command import ( - "errors" "fmt" "io" "net/url" - "regexp" "strconv" "strings" "time" @@ -244,12 +242,7 @@ func issueView(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) - if err != nil { - return err - } - - issue, err := issueFromArg(apiClient, baseRepo, args[0]) + issue, _, err := issueFromArg(ctx, apiClient, cmd, args[0]) if err != nil { return err } @@ -341,21 +334,6 @@ func printIssuePreview(out io.Writer, issue *api.Issue) error { return nil } -var issueURLRE = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/issues/(\d+)`) - -func issueFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) (*api.Issue, error) { - if issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")); err == nil { - return api.IssueByNumber(apiClient, baseRepo, issueNumber) - } - - if m := issueURLRE.FindStringSubmatch(arg); m != nil { - issueNumber, _ := strconv.Atoi(m[3]) - return api.IssueByNumber(apiClient, baseRepo, issueNumber) - } - - return nil, fmt.Errorf("invalid issue format: %q", arg) -} - func issueCreate(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) apiClient, err := apiClientForContext(ctx) @@ -685,19 +663,11 @@ func issueClose(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) + issue, baseRepo, err := issueFromArg(ctx, apiClient, cmd, args[0]) if err != nil { return err } - issue, err := issueFromArg(apiClient, baseRepo, args[0]) - var idErr *api.IssuesDisabledError - if errors.As(err, &idErr) { - return fmt.Errorf("issues disabled for %s", ghrepo.FullName(baseRepo)) - } else if err != nil { - return fmt.Errorf("failed to find issue #%d: %w", issue.Number, err) - } - if issue.Closed { fmt.Fprintf(colorableErr(cmd), "%s Issue #%d is already closed\n", utils.Yellow("!"), issue.Number) return nil @@ -705,7 +675,7 @@ func issueClose(cmd *cobra.Command, args []string) error { err = api.IssueClose(apiClient, baseRepo, *issue) if err != nil { - return fmt.Errorf("API call failed:%w", err) + return err } fmt.Fprintf(colorableErr(cmd), "%s Closed issue #%d\n", utils.Red("✔"), issue.Number) @@ -720,19 +690,11 @@ func issueReopen(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(apiClient, cmd, ctx) + issue, baseRepo, err := issueFromArg(ctx, apiClient, cmd, args[0]) if err != nil { return err } - issue, err := issueFromArg(apiClient, baseRepo, args[0]) - var idErr *api.IssuesDisabledError - if errors.As(err, &idErr) { - return fmt.Errorf("issues disabled for %s", ghrepo.FullName(baseRepo)) - } else if err != nil { - return fmt.Errorf("failed to find issue #%d: %w", issue.Number, err) - } - if !issue.Closed { fmt.Fprintf(colorableErr(cmd), "%s Issue #%d is already open\n", utils.Yellow("!"), issue.Number) return nil @@ -740,7 +702,7 @@ func issueReopen(cmd *cobra.Command, args []string) error { err = api.IssueReopen(apiClient, baseRepo, *issue) if err != nil { - return fmt.Errorf("API call failed:%w", err) + return err } fmt.Fprintf(colorableErr(cmd), "%s Reopened issue #%d\n", utils.Green("✔"), issue.Number) diff --git a/command/issue_lookup.go b/command/issue_lookup.go new file mode 100644 index 000000000..aa8e0b12c --- /dev/null +++ b/command/issue_lookup.go @@ -0,0 +1,64 @@ +package command + +import ( + "fmt" + "net/url" + "regexp" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/internal/ghrepo" + "github.com/spf13/cobra" +) + +func issueFromArg(ctx context.Context, apiClient *api.Client, cmd *cobra.Command, arg string) (*api.Issue, ghrepo.Interface, error) { + issue, baseRepo, err := issueFromURL(apiClient, arg) + if err != nil { + return nil, nil, err + } + if issue != nil { + return issue, baseRepo, nil + } + + baseRepo, err = determineBaseRepo(apiClient, cmd, ctx) + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } + + issueNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")) + if err != nil { + return nil, nil, fmt.Errorf("invalid issue format: %q", arg) + } + + issue, err = issueFromNumber(apiClient, baseRepo, issueNumber) + return issue, baseRepo, err +} + +var issueURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/issues/(\d+)`) + +func issueFromURL(apiClient *api.Client, s string) (*api.Issue, ghrepo.Interface, error) { + u, err := url.Parse(s) + if err != nil { + return nil, nil, nil + } + + if u.Scheme != "https" && u.Scheme != "http" { + return nil, nil, nil + } + + m := issueURLRE.FindStringSubmatch(u.Path) + if m == nil { + return nil, nil, nil + } + + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + issueNumber, _ := strconv.Atoi(m[3]) + issue, err := issueFromNumber(apiClient, repo, issueNumber) + return issue, repo, err +} + +func issueFromNumber(apiClient *api.Client, repo ghrepo.Interface, issueNumber int) (*api.Issue, error) { + return api.IssueByNumber(apiClient, repo, issueNumber) +} diff --git a/command/issue_test.go b/command/issue_test.go index 418de25d3..105ff8997 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -387,6 +387,7 @@ func TestIssueView_Preview(t *testing.T) { func TestIssueView_web_notFound(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "errors": [ @@ -432,7 +433,6 @@ func TestIssueView_disabledIssues(t *testing.T) { func TestIssueView_web_urlArg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "hasIssuesEnabled": true, "issue": { @@ -857,12 +857,8 @@ func TestIssueClose_issuesDisabled(t *testing.T) { `)) _, err := RunCommand("issue close 13") - if err == nil { - t.Fatalf("expected error when issues are disabled") - } - - if !strings.Contains(err.Error(), "issues disabled") { - t.Fatalf("got unexpected error: %s", err) + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Fatalf("got error: %v", err) } } @@ -930,11 +926,7 @@ func TestIssueReopen_issuesDisabled(t *testing.T) { `)) _, err := RunCommand("issue reopen 2") - if err == nil { - t.Fatalf("expected error when issues are disabled") - } - - if !strings.Contains(err.Error(), "issues disabled") { - t.Fatalf("got unexpected error: %s", err) + if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { + t.Fatalf("got error: %v", err) } } diff --git a/command/pr_checkout.go b/command/pr_checkout.go index ec60d3de0..c81bf4f76 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -84,8 +84,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { remote := baseURLOrName mergeRef := ref if pr.MaintainerCanModify { - // FIXME: inherit hostname from the server - headRepo := ghrepo.New(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name) + headRepo := ghrepo.NewWithHost(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name, baseRepo.RepoHost()) remote = formatRemoteURL(cmd, headRepo) mergeRef = fmt.Sprintf("refs/heads/%s", pr.HeadRefName) } diff --git a/command/pr_lookup.go b/command/pr_lookup.go index a1b0c9638..cf59f0717 100644 --- a/command/pr_lookup.go +++ b/command/pr_lookup.go @@ -2,6 +2,7 @@ package command import ( "fmt" + "net/url" "regexp" "strconv" "strings" @@ -55,16 +56,27 @@ func prFromNumberString(ctx context.Context, apiClient *api.Client, repo ghrepo. return nil, nil } +var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) + func prFromURL(ctx context.Context, apiClient *api.Client, s string) (*api.PullRequest, ghrepo.Interface, error) { - r := regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)`) - if m := r.FindStringSubmatch(s); m != nil { - repo := ghrepo.New(m[1], m[2]) - prNumberString := m[3] - pr, err := prFromNumberString(ctx, apiClient, repo, prNumberString) - return pr, repo, err + u, err := url.Parse(s) + if err != nil { + return nil, nil, nil } - return nil, nil, nil + if u.Scheme != "https" && u.Scheme != "http" { + return nil, nil, nil + } + + m := pullURLRE.FindStringSubmatch(u.Path) + if m == nil { + return nil, nil, nil + } + + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + prNumberString := m[3] + pr, err := prFromNumberString(ctx, apiClient, repo, prNumberString) + return pr, repo, err } func prForCurrentBranch(ctx context.Context, apiClient *api.Client, repo ghrepo.Interface) (*api.PullRequest, error) { diff --git a/context/context.go b/context/context.go index 236f9e722..74253e1d2 100644 --- a/context/context.go +++ b/context/context.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "sort" + "strings" "github.com/cli/cli/api" "github.com/cli/cli/git" @@ -31,22 +32,31 @@ type Context interface { // unusually large number of git remotes const maxRemotesForLookup = 5 +// ResolveRemotesToRepos takes in a list of git remotes and fetches more information about the repositories they map to. +// Only the git remotes belonging to the same hostname are ever looked up; all others are ignored. func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (ResolvedRemotes, error) { sort.Stable(remotes) - lenRemotesForLookup := len(remotes) - if lenRemotesForLookup > maxRemotesForLookup { - lenRemotesForLookup = maxRemotesForLookup - } hasBaseOverride := base != "" baseOverride, _ := ghrepo.FromFullName(base) foundBaseOverride := false - repos := make([]ghrepo.Interface, 0, lenRemotesForLookup) - for _, r := range remotes[:lenRemotesForLookup] { + + var hostname string + var repos []ghrepo.Interface + for i, r := range remotes { + if i == 0 { + hostname = r.RepoHost() + } else if !strings.EqualFold(r.RepoHost(), hostname) { + // ignore all remotes for a hostname different to that of the 1st remote + continue + } repos = append(repos, r) if ghrepo.IsSame(r, baseOverride) { foundBaseOverride = true } + if len(repos) == maxRemotesForLookup { + break + } } if hasBaseOverride && !foundBaseOverride { // additionally, look up the explicitly specified base repo if it's not diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 93493a3da..fbe748204 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -23,6 +23,15 @@ func New(owner, repo string) Interface { } } +// NewWithHost is like New with an explicit host name +func NewWithHost(owner, repo, hostname string) Interface { + return &ghRepo{ + owner: owner, + name: repo, + hostname: hostname, + } +} + // FullName serializes a GitHub repository into an "OWNER/REPO" string func FullName(r Interface) string { return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName()) From a8c37a1f5c3af8a4fd9c854b83075d58e59f085b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 2 Jul 2020 20:49:45 +0200 Subject: [PATCH 4/4] Fix hostname being respected in web browse commands --- command/issue.go | 13 ++++++++++--- command/pr_create.go | 7 +------ command/repo.go | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/command/issue.go b/command/issue.go index 15c74f82a..4cd1f6d8d 100644 --- a/command/issue.go +++ b/command/issue.go @@ -403,8 +403,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/issues/new", ghrepo.FullName(baseRepo)) + openURL := generateRepoURL(baseRepo, "issues/new") if title != "" || body != "" { milestone := "" if len(milestoneTitles) > 0 { @@ -476,7 +475,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } if action == PreviewAction { - openURL := fmt.Sprintf("https://github.com/%s/issues/new/", ghrepo.FullName(baseRepo)) + openURL := generateRepoURL(baseRepo, "issues/new") milestone := "" if len(milestoneTitles) > 0 { milestone = milestoneTitles[0] @@ -512,6 +511,14 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } +func generateRepoURL(repo ghrepo.Interface, p string, args ...interface{}) string { + baseURL := fmt.Sprintf("https://%s/%s/%s", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) + if p != "" { + return baseURL + "/" + fmt.Sprintf(p, args...) + } + return baseURL +} + func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *issueMetadataState) error { if !tb.HasMetadata() { return nil diff --git a/command/pr_create.go b/command/pr_create.go index 11d9ec523..450eb7a2a 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -437,12 +437,7 @@ func withPrAndIssueQueryParams(baseURL, title, body string, assignees, labels, p } func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestone string) (string, error) { - u := fmt.Sprintf( - "https://github.com/%s/compare/%s...%s?expand=1", - ghrepo.FullName(r), - base, - head, - ) + u := generateRepoURL(r, "compare/%s...%s?expand=1", base, head) url, err := withPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestone) if err != nil { return "", err diff --git a/command/repo.go b/command/repo.go index 83788fe78..9d4a1dc6b 100644 --- a/command/repo.go +++ b/command/repo.go @@ -592,7 +592,7 @@ func repoView(cmd *cobra.Command, args []string) error { fullName := ghrepo.FullName(toView) - openURL := fmt.Sprintf("https://github.com/%s", fullName) + openURL := generateRepoURL(toView, "") if web { fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) return utils.OpenInBrowser(openURL)