From 487b62d3b9c2da18b0ba296970fd101309a3a253 Mon Sep 17 00:00:00 2001 From: Jonathan Lloyd Date: Mon, 12 Oct 2020 23:39:18 +0100 Subject: [PATCH 1/3] Clone repos using canonical username/repo name capitalization GitHub user/repo names are case insentitive. This can cause issues when dealing with repos programmatically using systems that aren't aware of this (e.g. go packages). This commit updates the cloning logic of the CLI to query the API for the canonical capitalization and uses that for the clone operation. Fixes: #1845 --- pkg/cmd/repo/clone/clone.go | 67 +++++++++++++++++++++----------- pkg/cmd/repo/clone/clone_test.go | 37 +++++++++++++++++- 2 files changed, 80 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index 239fbc21c..08b1cc13b 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -82,44 +82,65 @@ func cloneRun(opts *CloneOptions) error { } apiClient := api.NewClientFromHTTP(httpClient) - cloneURL := opts.Repository - if !strings.Contains(cloneURL, ":") { - if !strings.Contains(cloneURL, "/") { + + respositoryIsURL := strings.Contains(opts.Repository, ":") + repositoryIsFullName := !respositoryIsURL && strings.Contains(opts.Repository, "/") + + var repo ghrepo.Interface + var protocol string + if respositoryIsURL { + repoURL, err := git.ParseURL(opts.Repository) + if err != nil { + return err + } + repo, err = ghrepo.FromURL(repoURL) + if err != nil { + return err + } + if repoURL.Scheme == "git+ssh" { + repoURL.Scheme = "ssh" + } + protocol = repoURL.Scheme + } else { + var fullName string + if repositoryIsFullName { + fullName = opts.Repository + } else { currentUser, err := api.CurrentLoginName(apiClient, ghinstance.OverridableDefault()) if err != nil { return err } - cloneURL = currentUser + "/" + cloneURL + fullName = currentUser + "/" + opts.Repository } - repo, err := ghrepo.FromFullName(cloneURL) + + repo, err = ghrepo.FromFullName(fullName) if err != nil { return err } - protocol, err := cfg.Get(repo.RepoHost(), "git_protocol") + protocol, err = cfg.Get(repo.RepoHost(), "git_protocol") if err != nil { return err } - cloneURL = ghrepo.FormatRemoteURL(repo, protocol) } - var repo ghrepo.Interface + // Load the repo from the API to get the username/repo name in its + // canonical capitalization + var canonicalRepo ghrepo.Interface + canonicalRepo, err = api.GitHubRepo(apiClient, repo) + if err != nil { + return err + } + canonicalCloneURL := ghrepo.FormatRemoteURL(canonicalRepo, protocol) + + cloneDir, err := git.RunClone(canonicalCloneURL, opts.GitArgs) + if err != nil { + return err + } + + // If the repo is a fork, add the parent as an upstream var parentRepo ghrepo.Interface - - // TODO: consider caching and reusing `git.ParseSSHConfig().Translator()` - // here to handle hostname aliases in SSH remotes - if u, err := git.ParseURL(cloneURL); err == nil { - repo, _ = ghrepo.FromURL(u) - } - - if repo != nil { - parentRepo, err = api.RepoParent(apiClient, repo) - if err != nil { - return err - } - } - - cloneDir, err := git.RunClone(cloneURL, opts.GitArgs) + parentRepo, err = api.RepoParent(apiClient, canonicalRepo) if err != nil { return err } diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index 54aa2f1cb..8fedca21a 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -77,17 +77,32 @@ func Test_RepoClone(t *testing.T) { { name: "HTTPS URL", args: "https://github.com/OWNER/REPO", - want: "git clone https://github.com/OWNER/REPO", + want: "git clone https://github.com/OWNER/REPO.git", }, { name: "SSH URL", args: "git@github.com:OWNER/REPO.git", want: "git clone git@github.com:OWNER/REPO.git", }, + { + name: "Non-canonical capitalization", + args: "git@github.com:Ower/Repo.git", + want: "git clone git@github.com:OWNER/REPO.git", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "name": "REPO", + "owner": { + "login": "OWNER" + } + } } } + `)) reg.Register( httpmock.GraphQL(`query RepositoryFindParent\b`), httpmock.StringResponse(` @@ -119,6 +134,16 @@ func Test_RepoClone(t *testing.T) { func Test_RepoClone_hasParent(t *testing.T) { reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "name": "REPO", + "owner": { + "login": "OWNER" + } + } } } + `)) reg.Register( httpmock.GraphQL(`query RepositoryFindParent\b`), httpmock.StringResponse(` @@ -155,6 +180,16 @@ func Test_RepoClone_withoutUsername(t *testing.T) { { "data": { "viewer": { "login": "OWNER" }}}`)) + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "name": "REPO", + "owner": { + "login": "OWNER" + } + } } } + `)) reg.Register( httpmock.GraphQL(`query RepositoryFindParent\b`), httpmock.StringResponse(` From a5ec03d00094950684828787dc21f95d8ee6418a Mon Sep 17 00:00:00 2001 From: Jonathan Lloyd Date: Mon, 12 Oct 2020 23:56:51 +0100 Subject: [PATCH 2/3] Improve test args --- pkg/cmd/repo/clone/clone_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index 8fedca21a..875183626 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -86,8 +86,8 @@ func Test_RepoClone(t *testing.T) { }, { name: "Non-canonical capitalization", - args: "git@github.com:Ower/Repo.git", - want: "git clone git@github.com:OWNER/REPO.git", + args: "Owner/Repo", + want: "git clone https://github.com/OWNER/REPO.git", }, } for _, tt := range tests { From 8f44aee76a683f996d44974a2b1daaae2cec48cf Mon Sep 17 00:00:00 2001 From: Jonathan Lloyd Date: Tue, 13 Oct 2020 20:41:16 +0100 Subject: [PATCH 3/3] Load repo and parent in single query --- api/queries_repo.go | 23 +++++++++++++++-------- pkg/cmd/repo/clone/clone.go | 15 ++++----------- pkg/cmd/repo/clone/clone_test.go | 23 ++++++----------------- 3 files changed, 25 insertions(+), 36 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index fdbd13e92..2e000b979 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -88,16 +88,23 @@ func (r Repository) ViewerCanTriage() bool { func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { query := ` + fragment repo on Repository { + id + name + owner { login } + hasIssuesEnabled + description + viewerPermission + defaultBranchRef { + name + } + } + query RepositoryInfo($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { - id - name - owner { login } - hasIssuesEnabled - description - viewerPermission - defaultBranchRef { - name + ...repo + parent { + ...repo } } }` diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index 08b1cc13b..6dc225538 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -126,8 +126,7 @@ func cloneRun(opts *CloneOptions) error { // Load the repo from the API to get the username/repo name in its // canonical capitalization - var canonicalRepo ghrepo.Interface - canonicalRepo, err = api.GitHubRepo(apiClient, repo) + canonicalRepo, err := api.GitHubRepo(apiClient, repo) if err != nil { return err } @@ -139,18 +138,12 @@ func cloneRun(opts *CloneOptions) error { } // If the repo is a fork, add the parent as an upstream - var parentRepo ghrepo.Interface - parentRepo, err = api.RepoParent(apiClient, canonicalRepo) - if err != nil { - return err - } - - if parentRepo != nil { - protocol, err := cfg.Get(parentRepo.RepoHost(), "git_protocol") + if canonicalRepo.Parent != nil { + protocol, err := cfg.Get(canonicalRepo.Parent.RepoHost(), "git_protocol") if err != nil { return err } - upstreamURL := ghrepo.FormatRemoteURL(parentRepo, protocol) + upstreamURL := ghrepo.FormatRemoteURL(canonicalRepo.Parent, protocol) err = git.AddUpstreamRemote(upstreamURL, cloneDir) if err != nil { diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index 875183626..f696cd354 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -103,13 +103,6 @@ func Test_RepoClone(t *testing.T) { } } } } `)) - reg.Register( - httpmock.GraphQL(`query RepositoryFindParent\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "parent": null - } } } - `)) httpClient := &http.Client{Transport: reg} @@ -141,19 +134,15 @@ func Test_RepoClone_hasParent(t *testing.T) { "name": "REPO", "owner": { "login": "OWNER" + }, + "parent": { + "name": "ORIG", + "owner": { + "login": "hubot" + } } } } } `)) - reg.Register( - httpmock.GraphQL(`query RepositoryFindParent\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "parent": { - "owner": {"login": "hubot"}, - "name": "ORIG" - } - } } } - `)) httpClient := &http.Client{Transport: reg}