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] 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 22d42cfbb37..d0ff60aa9e7 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 b2dd486973e..d46916f5ea9 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 2eb0872c7a1..ff4f3bf1ebe 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 3ea657abe42..151e5a9edab 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 b40533a5403..79ba0e8c856 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 1e0f47ba032..6d5801e26a6 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 37fecbe8d75..93493a3dad4 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 fef04fb8c8c..3c421bd6471 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()) + } }) } }