From d75faab85a7b5fae4353046207cc95572ea7b2f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 19 Mar 2020 16:04:23 +0100 Subject: [PATCH 01/19] Creating a PR now always prioritizes an existing fork as a push target Before: the default push target for the current branch in `pr create` was the first repository found among git remotes that has write access. Now: the default push target is the fork the base repo, if said fork exists and has write access, falling back to old behavior otherwise. This change in the default is to facilitate contributions to projects that have a hard requirement that all pull requests (even those opened by people with write access to that project) come from forks. --- api/queries_repo.go | 39 +++++++++++++++++++++++++++ command/pr_create.go | 36 +++++++++++++------------ command/pr_create_test.go | 40 +++++++++++++++++++++++++--- context/context.go | 30 +++++++++++++++++++-- context/remote_test.go | 56 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 178 insertions(+), 23 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 048b72b50..79068f0ea 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -3,6 +3,7 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "sort" "strings" @@ -220,6 +221,44 @@ func ForkRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { }, nil } +// RepoFindFork finds a fork of repo affiliated with the viewer +func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { + result := struct { + Repository struct { + Forks struct { + Nodes []Repository + } + } + }{} + + variables := map[string]interface{}{ + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), + } + + if err := client.GraphQL(` + query($owner: String!, $repo: String!) { + repository(owner: $owner, name: $repo) { + forks(first: 1, affiliations: [OWNER, COLLABORATOR]) { + nodes { + id + name + owner { login } + url + } + } + } + } + `, variables, &result); err != nil { + return nil, err + } + + if len(result.Repository.Forks.Nodes) > 0 { + return &result.Repository.Forks.Nodes[0], nil + } + return nil, &NotFoundError{errors.New("no fork found")} +} + // RepoCreateInput represents input parameters for RepoCreate type RepoCreateInput struct { Name string `json:"name"` diff --git a/command/pr_create.go b/command/pr_create.go index ca7800a19..e4a4bd2de 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -193,7 +193,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { } didForkRepo := false - var headRemote *context.Remote if headRepoErr != nil { if baseRepo.IsPrivate { return fmt.Errorf("cannot fork private repository '%s'", ghrepo.FullName(baseRepo)) @@ -203,19 +202,6 @@ func prCreate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("error forking repo: %w", err) } didForkRepo = true - // TODO: support non-HTTPS git remote URLs - 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 { - return fmt.Errorf("error adding remote: %w", err) - } - headRemote = &context.Remote{ - Remote: gitRemote, - Owner: headRepo.RepoOwner(), - Repo: headRepo.RepoName(), - } } headBranchLabel := headBranch @@ -223,10 +209,26 @@ func prCreate(cmd *cobra.Command, _ []string) error { headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) } - if headRemote == nil { - headRemote, err = repoContext.RemoteForRepo(headRepo) + headRemote, err := repoContext.RemoteForRepo(headRepo) + // There are two cases when an existing remote for the head repo will be + // missing: + // 1. the head repo was just created by auto-forking; + // 2. an existing fork was discovered by quering the API. + // + // In either case, we want to add the head repo as a new git remote so we + // can push to it. + if err != nil { + // TODO: support non-HTTPS git remote URLs + headRepoURL := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(headRepo)) + // TODO: prevent clashes with another remote of a same name + gitRemote, err := git.AddRemote("fork", headRepoURL, "") if err != nil { - return fmt.Errorf("git remote not found for head repository: %w", err) + return fmt.Errorf("error adding remote: %w", err) + } + headRemote = &context.Remote{ + Remote: gitRemote, + Owner: headRepo.RepoOwner(), + Repo: headRepo.RepoName(), } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 688dfd9c8..ebffb686a 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -14,6 +14,10 @@ func TestPRCreate(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes" : [ ] } } } } @@ -34,7 +38,7 @@ func TestPRCreate(t *testing.T) { output, err := RunCommand(prCreateCmd, `pr create -t "my title" -b "my body"`) eq(t, err, nil) - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { Variables struct { Input struct { @@ -61,6 +65,10 @@ func TestPRCreate_alreadyExists(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -87,6 +95,10 @@ func TestPRCreate_web(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) cs, cmdTeardown := initCmdStubber() defer cmdTeardown() @@ -113,6 +125,10 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes" : [ ] } } } } @@ -232,6 +248,10 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { initBlankContext("OWNER/REPO", "cool_bug-fixes") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes" : [ ] } } } } @@ -273,7 +293,7 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { output, err := RunCommand(prCreateCmd, `pr create`) eq(t, err, nil) - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { Variables struct { Input struct { @@ -302,6 +322,10 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes" : [ ] } } } } @@ -344,7 +368,7 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { output, err := RunCommand(prCreateCmd, `pr create`) eq(t, err, nil) - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { Variables struct { Input struct { @@ -373,6 +397,10 @@ func TestPRCreate_survey_autofill(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes" : [ ] } } } } @@ -396,7 +424,7 @@ func TestPRCreate_survey_autofill(t *testing.T) { output, err := RunCommand(prCreateCmd, `pr create -f`) eq(t, err, nil) - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) reqBody := struct { Variables struct { Input struct { @@ -457,6 +485,10 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "createPullRequest": { "pullRequest": { "URL": "https://github.com/OWNER/REPO/pull/12" diff --git a/context/context.go b/context/context.go index 6c3bd5aee..27744a08a 100644 --- a/context/context.go +++ b/context/context.go @@ -51,7 +51,10 @@ func ResolveRemotesToRepos(remotes Remotes, client *api.Client, base string) (Re repos = append(repos, baseOverride) } - result := ResolvedRemotes{Remotes: remotes} + result := ResolvedRemotes{ + Remotes: remotes, + apiClient: client, + } if hasBaseOverride { result.BaseOverride = baseOverride } @@ -67,6 +70,7 @@ type ResolvedRemotes struct { BaseOverride ghrepo.Interface Remotes Remotes Network api.RepoNetworkResult + apiClient *api.Client } // BaseRepo is the first found repository in the "upstream", "github", "origin" @@ -95,8 +99,30 @@ func (r ResolvedRemotes) BaseRepo() (*api.Repository, error) { return nil, errors.New("not found") } -// HeadRepo is the first found repository that has push access +// HeadRepo is a fork of base repo (if any), or the first found repository that +// has push access func (r ResolvedRemotes) HeadRepo() (*api.Repository, error) { + baseRepo, err := r.BaseRepo() + if err != nil { + return nil, err + } + + // try to find a pushable fork among existing remotes + for _, repo := range r.Network.Repositories { + if repo != nil && repo.Parent != nil && repo.ViewerCanPush() && ghrepo.IsSame(repo.Parent, baseRepo) { + return repo, nil + } + } + + // a fork might still exist on GitHub, so let's query for it + var notFound *api.NotFoundError + if repo, err := api.RepoFindFork(r.apiClient, baseRepo); err == nil { + return repo, nil + } else if !errors.As(err, ¬Found) { + return nil, err + } + + // fall back to any listed repository that has push access for _, repo := range r.Network.Repositories { if repo != nil && repo.ViewerCanPush() { return repo, nil diff --git a/context/remote_test.go b/context/remote_test.go index 04ccbc56c..dd6a5d59a 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -1,6 +1,7 @@ package context import ( + "bytes" "errors" "net/url" "testing" @@ -61,6 +62,14 @@ func Test_translateRemotes(t *testing.T) { } func Test_resolvedRemotes_triangularSetup(t *testing.T) { + http := &api.FakeHTTP{} + apiClient := api.NewClient(api.ReplaceTripper(http)) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) + resolved := ResolvedRemotes{ BaseOverride: nil, Remotes: Remotes{ @@ -89,6 +98,7 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { }, }, }, + apiClient: apiClient, } baseRepo, err := resolved.BaseRepo() @@ -118,6 +128,52 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { } } +func Test_resolvedRemotes_forkLookup(t *testing.T) { + http := &api.FakeHTTP{} + apiClient := api.NewClient(api.ReplaceTripper(http)) + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + { "id": "FORKID", + "url": "https://github.com/FORKOWNER/REPO", + "name": "REPO", + "owner": { "login": "FORKOWNER" } + } + ] } } } } + `)) + + resolved := ResolvedRemotes{ + BaseOverride: nil, + Remotes: Remotes{ + &Remote{ + Remote: &git.Remote{Name: "origin"}, + Owner: "OWNER", + Repo: "REPO", + }, + }, + Network: api.RepoNetworkResult{ + Repositories: []*api.Repository{ + &api.Repository{ + Name: "NEWNAME", + Owner: api.RepositoryOwner{Login: "NEWOWNER"}, + ViewerPermission: "READ", + }, + }, + }, + apiClient: apiClient, + } + + headRepo, err := resolved.HeadRepo() + if err != nil { + t.Fatalf("got %v", err) + } + eq(t, ghrepo.FullName(headRepo), "FORKOWNER/REPO") + _, err = resolved.RemoteForRepo(headRepo) + if err == nil { + t.Fatal("expected to not find a matching remote") + } +} + func Test_resolvedRemotes_clonedFork(t *testing.T) { resolved := ResolvedRemotes{ BaseOverride: nil, From ef17aa520d74e88619e038c0fffb5cfdb9b48d3a Mon Sep 17 00:00:00 2001 From: Amanda Pinsker Date: Thu, 19 Mar 2020 10:33:34 -0700 Subject: [PATCH 02/19] Add unicode to pr status --- command/pr.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/command/pr.go b/command/pr.go index bd7743f46..85a3e2b86 100644 --- a/command/pr.go +++ b/command/pr.go @@ -412,28 +412,28 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { var summary string if checks.Failing > 0 { if checks.Failing == checks.Total { - summary = utils.Red("All checks failing") + summary = utils.Red("× All checks failing") } else { summary = utils.Red(fmt.Sprintf("%d/%d checks failing", checks.Failing, checks.Total)) } } else if checks.Pending > 0 { summary = utils.Yellow("Checks pending") } else if checks.Passing == checks.Total { - summary = utils.Green("Checks passing") + summary = utils.Green("✓ Checks passing") } - fmt.Fprintf(w, " - %s", summary) + fmt.Fprintf(w, "%s", summary) } if reviews.ChangesRequested { - fmt.Fprintf(w, " - %s", utils.Red("Changes requested")) + fmt.Fprintf(w, "%s", utils.Red("+ Changes requested")) } else if reviews.ReviewRequired { - fmt.Fprintf(w, " - %s", utils.Yellow("Review required")) + fmt.Fprintf(w, "%s", utils.Yellow("- Review required")) } else if reviews.Approved { - fmt.Fprintf(w, " - %s", utils.Green("Approved")) + fmt.Fprintf(w, "%s", utils.Green("✓ Approved")) } } else { s := strings.Title(strings.ToLower(pr.State)) - fmt.Fprintf(w, " - %s", prStateColorFunc(s)) + fmt.Fprintf(w, "%s", prStateColorFunc(s)) } fmt.Fprint(w, "\n") From be7ffe3099abb73d8e319e650d501a193ac81b25 Mon Sep 17 00:00:00 2001 From: Amanda Pinsker Date: Thu, 19 Mar 2020 13:16:00 -0700 Subject: [PATCH 03/19] Tweak unicode some more --- command/pr.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/command/pr.go b/command/pr.go index 85a3e2b86..3755344b8 100644 --- a/command/pr.go +++ b/command/pr.go @@ -398,7 +398,7 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { prStateColorFunc = utils.Red } - fmt.Fprintf(w, " %s %s %s", prStateColorFunc(prNumber), text.Truncate(50, replaceExcessiveWhitespace(pr.Title)), utils.Cyan("["+pr.HeadLabel()+"]")) + fmt.Fprintf(w, " %s %s %s ", prStateColorFunc(prNumber), text.Truncate(50, replaceExcessiveWhitespace(pr.Title)), utils.Cyan("["+pr.HeadLabel()+"]")) checks := pr.ChecksStatus() reviews := pr.ReviewStatus() @@ -412,24 +412,24 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { var summary string if checks.Failing > 0 { if checks.Failing == checks.Total { - summary = utils.Red("× All checks failing") + summary = utils.Red("× All checks failing ") } else { - summary = utils.Red(fmt.Sprintf("%d/%d checks failing", checks.Failing, checks.Total)) + summary = utils.Red(fmt.Sprintf("× %d/%d checks failing ", checks.Failing, checks.Total)) } } else if checks.Pending > 0 { - summary = utils.Yellow("Checks pending") + summary = utils.Yellow("- Checks pending ") } else if checks.Passing == checks.Total { - summary = utils.Green("✓ Checks passing") + summary = utils.Green("✓ Checks passing ") } fmt.Fprintf(w, "%s", summary) } if reviews.ChangesRequested { - fmt.Fprintf(w, "%s", utils.Red("+ Changes requested")) + fmt.Fprintf(w, "%s", utils.Red("+ Changes requested ")) } else if reviews.ReviewRequired { - fmt.Fprintf(w, "%s", utils.Yellow("- Review required")) + fmt.Fprintf(w, "%s", utils.Yellow("- Review required ")) } else if reviews.Approved { - fmt.Fprintf(w, "%s", utils.Green("✓ Approved")) + fmt.Fprintf(w, "%s", utils.Green("✓ Approved ")) } } else { s := strings.Title(strings.ToLower(pr.State)) From fe7def13d3e7b5cca9287298927424097c207104 Mon Sep 17 00:00:00 2001 From: Amanda Pinsker Date: Thu, 19 Mar 2020 13:16:07 -0700 Subject: [PATCH 04/19] Update tests --- command/pr_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index 4b9d0c6de..9fda8980c 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -144,9 +144,9 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) { } expected := []string{ - "- Checks passing - Changes requested", - "- Checks pending - Approved", - "- 1/3 checks failing - Review required", + "✓ Checks passing + Changes requested", + "- Checks pending ✓ Approved", + "× 1/3 checks failing - Review required", } for _, line := range expected { From 57942fcaf889f0dc47dee345a5b4cc47c60bcd5c Mon Sep 17 00:00:00 2001 From: Amanda Pinsker Date: Thu, 19 Mar 2020 13:30:35 -0700 Subject: [PATCH 05/19] Add unicode for merge states back in --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 3755344b8..b7a26b8b7 100644 --- a/command/pr.go +++ b/command/pr.go @@ -433,7 +433,7 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { } } else { s := strings.Title(strings.ToLower(pr.State)) - fmt.Fprintf(w, "%s", prStateColorFunc(s)) + fmt.Fprintf(w, "- %s", prStateColorFunc(s)) } fmt.Fprint(w, "\n") From e3653672a8f9929ed5faec74d81b5c21d695f046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 20 Mar 2020 13:08:33 +0100 Subject: [PATCH 06/19] Simplify `git.AddRemote` implementation The 2nd argument now unused since we've determined that, in practice, we can safely fetch git objects from a freshly created fork on GitHub and that we don't need the remote URL-swapping workaround. --- command/pr_create.go | 2 +- command/repo.go | 2 +- git/remote.go | 24 ++++++------------------ 3 files changed, 8 insertions(+), 20 deletions(-) diff --git a/command/pr_create.go b/command/pr_create.go index e4a4bd2de..66efd8a30 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -221,7 +221,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { // TODO: support non-HTTPS git remote URLs headRepoURL := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(headRepo)) // TODO: prevent clashes with another remote of a same name - gitRemote, err := git.AddRemote("fork", headRepoURL, "") + gitRemote, err := git.AddRemote("fork", headRepoURL) if err != nil { return fmt.Errorf("error adding remote: %w", err) } diff --git a/command/repo.go b/command/repo.go index 1dc9b13a5..ec860ed38 100644 --- a/command/repo.go +++ b/command/repo.go @@ -344,7 +344,7 @@ func repoFork(cmd *cobra.Command, args []string) error { } } if remoteDesired { - _, err := git.AddRemote("fork", forkedRepo.CloneURL, "") + _, err := git.AddRemote("fork", forkedRepo.CloneURL) if err != nil { return fmt.Errorf("failed to add remote: %w", err) } diff --git a/git/remote.go b/git/remote.go index f98606a2e..9a8b3fccb 100644 --- a/git/remote.go +++ b/git/remote.go @@ -71,34 +71,22 @@ 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) +// AddRemote adds a new git remote and auto-fetches objects from it +func AddRemote(name, u string) (*Remote, error) { + addCmd := exec.Command("git", "remote", "add", "-f", name, u) 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(finalURL) + urlParsed, err := url.Parse(u) if err != nil { return nil, err } return &Remote{ Name: name, - FetchURL: finalURLParsed, - PushURL: finalURLParsed, + FetchURL: urlParsed, + PushURL: urlParsed, }, nil } From 7c73cf88ae5b9ea8c45b20c946b72f7bda226bcf Mon Sep 17 00:00:00 2001 From: Amanda Pinsker Date: Tue, 24 Mar 2020 15:01:21 -0700 Subject: [PATCH 07/19] Remove formatting from Fprint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Mislav Marohnić --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index b7a26b8b7..22cd5dcaf 100644 --- a/command/pr.go +++ b/command/pr.go @@ -421,7 +421,7 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { } else if checks.Passing == checks.Total { summary = utils.Green("✓ Checks passing ") } - fmt.Fprintf(w, "%s", summary) + fmt.Fprint(w, summary) } if reviews.ChangesRequested { From 5726376ab18f8fbd88f34a677f8223cff663a086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 25 Mar 2020 08:26:06 +0100 Subject: [PATCH 08/19] Guard against faulty `affiliations` GraphQL filter This API was just fixed in github.com, but it will take a while for the change to propagate to GitHub Enterprise installs, so guard ourselves from false positives when querying forks. --- api/queries_repo.go | 9 +++++++-- command/pr_create_test.go | 4 ++++ context/remote_test.go | 3 ++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 8ba8ee148..4bfa35f58 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -245,6 +245,7 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { name owner { login } url + viewerPermission } } } @@ -253,8 +254,12 @@ func RepoFindFork(client *Client, repo ghrepo.Interface) (*Repository, error) { return nil, err } - if len(result.Repository.Forks.Nodes) > 0 { - return &result.Repository.Forks.Nodes[0], nil + forks := result.Repository.Forks.Nodes + // we check ViewerCanPush, even though we expect it to always be true per + // `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 nil, &NotFoundError{errors.New("no fork found")} } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 50ff70f9b..a92ad76ff 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -96,6 +96,10 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", diff --git a/context/remote_test.go b/context/remote_test.go index dd6a5d59a..5c3c46bc7 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -137,7 +137,8 @@ func Test_resolvedRemotes_forkLookup(t *testing.T) { { "id": "FORKID", "url": "https://github.com/FORKOWNER/REPO", "name": "REPO", - "owner": { "login": "FORKOWNER" } + "owner": { "login": "FORKOWNER" }, + "viewerPermission": "WRITE" } ] } } } } `)) From 11327030e708ec612e241d7a46bb4eedec7de1b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 25 Mar 2020 09:05:51 +0100 Subject: [PATCH 09/19] Tweak whitespace printing in `pr status` --- command/pr.go | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/command/pr.go b/command/pr.go index 22cd5dcaf..aa22464f4 100644 --- a/command/pr.go +++ b/command/pr.go @@ -398,13 +398,15 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { prStateColorFunc = utils.Red } - fmt.Fprintf(w, " %s %s %s ", prStateColorFunc(prNumber), text.Truncate(50, replaceExcessiveWhitespace(pr.Title)), utils.Cyan("["+pr.HeadLabel()+"]")) + fmt.Fprintf(w, " %s %s %s", prStateColorFunc(prNumber), text.Truncate(50, replaceExcessiveWhitespace(pr.Title)), utils.Cyan("["+pr.HeadLabel()+"]")) checks := pr.ChecksStatus() reviews := pr.ReviewStatus() if pr.State == "OPEN" { - if checks.Total > 0 || reviews.ChangesRequested || reviews.Approved { + reviewStatus := reviews.ChangesRequested || reviews.Approved || reviews.ReviewRequired + if checks.Total > 0 || reviewStatus { + // show checks & reviews on their own line fmt.Fprintf(w, "\n ") } @@ -412,28 +414,33 @@ func printPrs(w io.Writer, totalCount int, prs ...api.PullRequest) { var summary string if checks.Failing > 0 { if checks.Failing == checks.Total { - summary = utils.Red("× All checks failing ") + summary = utils.Red("× All checks failing") } else { - summary = utils.Red(fmt.Sprintf("× %d/%d checks failing ", checks.Failing, checks.Total)) + summary = utils.Red(fmt.Sprintf("× %d/%d checks failing", checks.Failing, checks.Total)) } } else if checks.Pending > 0 { - summary = utils.Yellow("- Checks pending ") + summary = utils.Yellow("- Checks pending") } else if checks.Passing == checks.Total { - summary = utils.Green("✓ Checks passing ") + summary = utils.Green("✓ Checks passing") } fmt.Fprint(w, summary) } + if checks.Total > 0 && reviewStatus { + // add padding between checks & reviews + fmt.Fprint(w, " ") + } + if reviews.ChangesRequested { - fmt.Fprintf(w, "%s", utils.Red("+ Changes requested ")) + fmt.Fprint(w, utils.Red("+ Changes requested")) } else if reviews.ReviewRequired { - fmt.Fprintf(w, "%s", utils.Yellow("- Review required ")) + fmt.Fprint(w, utils.Yellow("- Review required")) } else if reviews.Approved { - fmt.Fprintf(w, "%s", utils.Green("✓ Approved ")) + fmt.Fprint(w, utils.Green("✓ Approved")) } } else { s := strings.Title(strings.ToLower(pr.State)) - fmt.Fprintf(w, "- %s", prStateColorFunc(s)) + fmt.Fprintf(w, " - %s", prStateColorFunc(s)) } fmt.Fprint(w, "\n") From c36b84e550dc6ddc71dc3b1e89f4c2ac852a5368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 26 Mar 2020 19:21:40 +0100 Subject: [PATCH 10/19] Convert `git.VerifyRef` into a more versatile `ShowRefs` This is so we can pass a list of full qualified refs and have them resolved for both existence and their commit hashes. --- command/pr_checkout.go | 2 +- command/pr_checkout_test.go | 12 ++++++------ git/git.go | 29 +++++++++++++++++++++++++---- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 336a5a8d1..fc2a743ec 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -66,7 +66,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { cmdQueue = append(cmdQueue, []string{"git", "fetch", headRemote.Name, refSpec}) // local branch already exists - if git.VerifyRef("refs/heads/" + newBranchName) { + if _, err := git.ShowRefs("refs/heads/" + newBranchName); err == nil { cmdQueue = append(cmdQueue, []string{"git", "checkout", newBranchName}) cmdQueue = append(cmdQueue, []string{"git", "merge", "--ff-only", fmt.Sprintf("refs/remotes/%s", remoteBranch)}) } else { diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index 1a36756a7..e155e2a82 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -46,7 +46,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { switch strings.Join(cmd.Args, " ") { - case "git show-ref --verify --quiet refs/heads/feature": + case "git show-ref --verify -- refs/heads/feature": return &errorStub{"exit status: 1"} default: ranCommands = append(ranCommands, cmd.Args) @@ -98,7 +98,7 @@ func TestPRCheckout_urlArg(t *testing.T) { ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { switch strings.Join(cmd.Args, " ") { - case "git show-ref --verify --quiet refs/heads/feature": + case "git show-ref --verify -- refs/heads/feature": return &errorStub{"exit status: 1"} default: ranCommands = append(ranCommands, cmd.Args) @@ -147,7 +147,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { switch strings.Join(cmd.Args, " ") { - case "git show-ref --verify --quiet refs/heads/feature": + case "git show-ref --verify -- refs/heads/feature": return &errorStub{"exit status: 1"} default: ranCommands = append(ranCommands, cmd.Args) @@ -210,7 +210,7 @@ func TestPRCheckout_branchArg(t *testing.T) { ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { switch strings.Join(cmd.Args, " ") { - case "git show-ref --verify --quiet refs/heads/feature": + case "git show-ref --verify -- refs/heads/feature": return &errorStub{"exit status: 1"} default: ranCommands = append(ranCommands, cmd.Args) @@ -260,7 +260,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { switch strings.Join(cmd.Args, " ") { - case "git show-ref --verify --quiet refs/heads/feature": + case "git show-ref --verify -- refs/heads/feature": return &test.OutputStub{} default: ranCommands = append(ranCommands, cmd.Args) @@ -313,7 +313,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { ranCommands := [][]string{} restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { switch strings.Join(cmd.Args, " ") { - case "git show-ref --verify --quiet refs/heads/feature": + case "git show-ref --verify -- refs/heads/feature": return &errorStub{"exit status: 1"} default: ranCommands = append(ranCommands, cmd.Args) diff --git a/git/git.go b/git/git.go index 37253462a..275aec819 100644 --- a/git/git.go +++ b/git/git.go @@ -13,10 +13,31 @@ import ( "github.com/cli/cli/utils" ) -func VerifyRef(ref string) bool { - showRef := exec.Command("git", "show-ref", "--verify", "--quiet", ref) - err := utils.PrepareCmd(showRef).Run() - return err == nil +// Ref represents a git commit reference +type Ref struct { + Hash string + Name string +} + +// ShowRefs resolves fully-qualified refs to commit hashes +func ShowRefs(ref ...string) ([]Ref, error) { + args := append([]string{"show-ref", "--verify", "--"}, ref...) + showRef := exec.Command("git", args...) + output, err := utils.PrepareCmd(showRef).Output() + + var refs []Ref + for _, line := range outputLines(output) { + parts := strings.SplitN(line, " ", 2) + if len(parts) < 2 { + continue + } + refs = append(refs, Ref{ + Hash: parts[0], + Name: parts[1], + }) + } + + return refs, err } // CurrentBranch reads the checked-out branch for the git repository From 3907914225c9f4154d3c89297c418c35bfaa6f6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 26 Mar 2020 19:23:17 +0100 Subject: [PATCH 11/19] Guard against filesystem paths in `git.ReadBranchConfig` A valid remote specificication read from `branch..remote` git config can be a URL, a `.`, a filesystem path, and a remote name. When detecting the remote name, be sure not to take filesystem paths instead. --- git/git.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git/git.go b/git/git.go index 275aec819..40b1d877e 100644 --- a/git/git.go +++ b/git/git.go @@ -173,7 +173,7 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) { continue } cfg.RemoteURL = u - } else { + } else if !isFilesystemPath(parts[1]) { cfg.RemoteName = parts[1] } case "merge": @@ -183,6 +183,10 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) { return } +func isFilesystemPath(p string) bool { + return p == "." || strings.HasPrefix(p, "./") || strings.HasPrefix(p, "/") +} + // ToplevelDir returns the top-level directory path of the current repository func ToplevelDir() (string, error) { showCmd := exec.Command("git", "rev-parse", "--show-toplevel") From cba8331d55940fca70528418b9b5c31844b9b8f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 26 Mar 2020 19:26:36 +0100 Subject: [PATCH 12/19] Avoid auto-forking/pushing an already pushed branch in `pr create` A branch is considered already pushed if its HEAD commit hash matches the commit hash of one of the remote tracking branches: `refs/remotes//` This mechanism allows the user to: - Choose a remote as push target for their branch different than the one that would automatically get chosen for them by `pr create`; - Avoid a `git push` operation (and its associated git hooks) if a push was not necessary in the first place. --- command/pr_create.go | 113 +++++++++++++++++++++++++------- command/pr_create_test.go | 131 +++++++++++++++++++++++++++++++++++++- git/git.go | 10 +++ 3 files changed, 227 insertions(+), 27 deletions(-) diff --git a/command/pr_create.go b/command/pr_create.go index 2be922987..408145348 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/url" + "strings" "time" "github.com/cli/cli/api" @@ -75,7 +76,27 @@ func prCreate(cmd *cobra.Command, _ []string) error { if err != nil { return fmt.Errorf("could not determine the current branch: %w", err) } - headRepo, headRepoErr := repoContext.HeadRepo() + + var headRepo ghrepo.Interface + var headRemote *context.Remote + + // determine whether the head branch is already pushed to a remote + headBranchPushedTo := determineTrackingBranch(remotes, headBranch) + if headBranchPushedTo != nil { + for _, r := range remotes { + if r.Name != headBranchPushedTo.RemoteName { + continue + } + headRepo = r + headRemote = r + break + } + } + + // otherwise, determine the head repository with info obtained from the API + if headRepo == nil { + headRepo, _ = repoContext.HeadRepo() + } baseBranch, err := cmd.Flags().GetString("base") if err != nil { @@ -193,8 +214,9 @@ func prCreate(cmd *cobra.Command, _ []string) error { } didForkRepo := false - var headRemote *context.Remote - if headRepoErr != nil { + // if a head repository could not be determined so far, automatically create + // one by forking the base repository + if headRepo == nil { if baseRepo.IsPrivate { return fmt.Errorf("cannot fork private repository '%s'", ghrepo.FullName(baseRepo)) } @@ -223,29 +245,31 @@ func prCreate(cmd *cobra.Command, _ []string) error { headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) } - if headRemote == nil { - headRemote, err = repoContext.RemoteForRepo(headRepo) - if err != nil { - return fmt.Errorf("git remote not found for head repository: %w", 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 - waitSeconds := 2 * pushTries - fmt.Fprintf(cmd.ErrOrStderr(), "waiting %s before retrying...\n", utils.Pluralize(waitSeconds, "second")) - time.Sleep(time.Duration(waitSeconds) * time.Second) - continue + // automatically push the branch if it hasn't been pushed anywhere yet + if headBranchPushedTo == nil { + if headRemote == nil { + headRemote, err = repoContext.RemoteForRepo(headRepo) + if err != nil { + return fmt.Errorf("git remote not found for head repository: %w", err) } - return err } - break + + pushTries := 0 + maxPushTries := 3 + for { + 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 + waitSeconds := 2 * pushTries + fmt.Fprintf(cmd.ErrOrStderr(), "waiting %s before retrying...\n", utils.Pluralize(waitSeconds, "second")) + time.Sleep(time.Duration(waitSeconds) * time.Second) + continue + } + return err + } + break + } } if action == SubmitAction { @@ -275,6 +299,47 @@ func prCreate(cmd *cobra.Command, _ []string) error { return nil } +func determineTrackingBranch(remotes context.Remotes, headBranch string) *git.TrackingRef { + refsForLookup := []string{"HEAD"} + var trackingRefs []git.TrackingRef + + headBranchConfig := git.ReadBranchConfig(headBranch) + if headBranchConfig.RemoteName != "" { + tr := git.TrackingRef{ + RemoteName: headBranchConfig.RemoteName, + BranchName: strings.TrimPrefix(headBranchConfig.MergeRef, "refs/heads/"), + } + trackingRefs = append(trackingRefs, tr) + refsForLookup = append(refsForLookup, tr.String()) + } + + for _, remote := range remotes { + tr := git.TrackingRef{ + RemoteName: remote.Name, + BranchName: headBranch, + } + trackingRefs = append(trackingRefs, tr) + refsForLookup = append(refsForLookup, tr.String()) + } + + resolvedRefs, _ := git.ShowRefs(refsForLookup...) + if len(resolvedRefs) > 1 { + for _, r := range resolvedRefs[1:] { + if r.Hash != resolvedRefs[0].Hash { + continue + } + for _, tr := range trackingRefs { + if tr.String() != r.Name { + continue + } + return &tr + } + } + } + + return nil +} + func generateCompareURL(r ghrepo.Interface, base, head, title, body string) string { u := fmt.Sprintf( "https://github.com/%s/compare/%s...%s?expand=1", diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 874b02a86..a91dd0744 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/cli/cli/context" + "github.com/cli/cli/git" ) func TestPRCreate(t *testing.T) { @@ -27,6 +28,8 @@ func TestPRCreate(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push @@ -72,6 +75,8 @@ func TestPRCreate_alreadyExists(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log @@ -100,6 +105,8 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git rev-parse @@ -118,6 +125,8 @@ func TestPRCreate_web(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push @@ -129,9 +138,9 @@ func TestPRCreate_web(t *testing.T) { eq(t, output.String(), "") eq(t, output.Stderr(), "Opening github.com/OWNER/REPO/compare/master...feature in your browser.\n") - eq(t, len(cs.Calls), 4) - eq(t, strings.Join(cs.Calls[2].Args, " "), "git push --set-upstream origin HEAD:feature") - browserCall := cs.Calls[3].Args + eq(t, len(cs.Calls), 6) + eq(t, strings.Join(cs.Calls[4].Args, " "), "git push --set-upstream origin HEAD:feature") + browserCall := cs.Calls[5].Args eq(t, browserCall[len(browserCall)-1], "https://github.com/OWNER/REPO/compare/master...feature?expand=1") } @@ -153,6 +162,8 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub(" M git/git.go") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push @@ -223,6 +234,8 @@ func TestPRCreate_cross_repo_same_branch(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push @@ -272,6 +285,8 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git rev-parse @@ -342,6 +357,8 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,the sky above the port") // git log cs.Stub("was the color of a television, turned to a dead channel") // git show @@ -413,6 +430,8 @@ func TestPRCreate_survey_autofill(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("1234567890,the sky above the port") // git log cs.Stub("was the color of a television, turned to a dead channel") // git show @@ -456,6 +475,8 @@ func TestPRCreate_defaults_error_autofill(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("") // git log @@ -472,6 +493,8 @@ func TestPRCreate_defaults_error_web(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("") // git log @@ -493,6 +516,8 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { cs, cmdTeardown := initCmdStubber() defer cmdTeardown() + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) cs.Stub("") // git status cs.Stub("") // git log cs.Stub("") // git rev-parse @@ -525,3 +550,103 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { stderr := string(output.Stderr()) eq(t, strings.Contains(stderr, "warning: could not compute title or body defaults: could not find any commits"), true) } + +func Test_determineTrackingBranch_empty(t *testing.T) { + cs, cmdTeardown := initCmdStubber() + defer cmdTeardown() + + remotes := context.Remotes{} + + cs.Stub("") // git config --get-regexp (ReadBranchConfig) + cs.Stub("deadbeef HEAD") // git show-ref --verify (ShowRefs) + + ref := determineTrackingBranch(remotes, "feature") + if ref != nil { + t.Errorf("expected nil result, got %v", ref) + } +} + +func Test_determineTrackingBranch_noMatch(t *testing.T) { + cs, cmdTeardown := initCmdStubber() + defer cmdTeardown() + + remotes := context.Remotes{ + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Owner: "hubot", + Repo: "Spoon-Knife", + }, + &context.Remote{ + Remote: &git.Remote{Name: "upstream"}, + Owner: "octocat", + Repo: "Spoon-Knife", + }, + } + + cs.Stub("") // git config --get-regexp (ReadBranchConfig) + cs.Stub(`deadbeef HEAD +deadb00f refs/remotes/origin/feature`) // git show-ref --verify (ShowRefs) + + ref := determineTrackingBranch(remotes, "feature") + if ref != nil { + t.Errorf("expected nil result, got %v", ref) + } +} + +func Test_determineTrackingBranch_hasMatch(t *testing.T) { + cs, cmdTeardown := initCmdStubber() + defer cmdTeardown() + + remotes := context.Remotes{ + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Owner: "hubot", + Repo: "Spoon-Knife", + }, + &context.Remote{ + Remote: &git.Remote{Name: "upstream"}, + Owner: "octocat", + Repo: "Spoon-Knife", + }, + } + + cs.Stub("") // git config --get-regexp (ReadBranchConfig) + cs.Stub(`deadbeef HEAD +deadb00f refs/remotes/origin/feature +deadbeef refs/remotes/upstream/feature`) // git show-ref --verify (ShowRefs) + + ref := determineTrackingBranch(remotes, "feature") + if ref == nil { + t.Fatal("expected result, got nil") + } + + eq(t, cs.Calls[1].Args, []string{"git", "show-ref", "--verify", "--", "HEAD", "refs/remotes/origin/feature", "refs/remotes/upstream/feature"}) + + eq(t, ref.RemoteName, "upstream") + eq(t, ref.BranchName, "feature") +} + +func Test_determineTrackingBranch_respectTrackingConfig(t *testing.T) { + cs, cmdTeardown := initCmdStubber() + defer cmdTeardown() + + remotes := context.Remotes{ + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Owner: "hubot", + Repo: "Spoon-Knife", + }, + } + + cs.Stub(`branch.feature.remote origin +branch.feature.merge refs/heads/great-feat`) // git config --get-regexp (ReadBranchConfig) + cs.Stub(`deadbeef HEAD +deadb00f refs/remotes/origin/feature`) // git show-ref --verify (ShowRefs) + + ref := determineTrackingBranch(remotes, "feature") + if ref != nil { + t.Errorf("expected nil result, got %v", ref) + } + + eq(t, cs.Calls[1].Args, []string{"git", "show-ref", "--verify", "--", "HEAD", "refs/remotes/origin/great-feat", "refs/remotes/origin/feature"}) +} diff --git a/git/git.go b/git/git.go index 40b1d877e..ce32742a4 100644 --- a/git/git.go +++ b/git/git.go @@ -19,6 +19,16 @@ type Ref struct { Name string } +// TrackingRef represents a ref for a remote tracking branch +type TrackingRef struct { + RemoteName string + BranchName string +} + +func (r TrackingRef) String() string { + return "refs/remotes/" + r.RemoteName + "/" + r.BranchName +} + // ShowRefs resolves fully-qualified refs to commit hashes func ShowRefs(ref ...string) ([]Ref, error) { args := append([]string{"show-ref", "--verify", "--"}, ref...) From c0bc938ea4368ba5e7b889e25364a040d208f3f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 09:54:11 +0100 Subject: [PATCH 13/19] Prevent crash when encountering "STALE" check conclusion The "STALE" conclusion has shipped this January and is basically a conclusion that can only be reported by GitHub and not explicitly set by any integrations. --- api/pull_request_test.go | 8 +++++--- api/queries_pr.go | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/api/pull_request_test.go b/api/pull_request_test.go index 82386108d..609a27e7c 100644 --- a/api/pull_request_test.go +++ b/api/pull_request_test.go @@ -22,7 +22,9 @@ func TestPullRequest_ChecksStatus(t *testing.T) { { "status": "COMPLETED", "conclusion": "FAILURE" }, { "status": "COMPLETED", - "conclusion": "ACTION_REQUIRED" } + "conclusion": "ACTION_REQUIRED" }, + { "status": "COMPLETED", + "conclusion": "STALE" } ] } } @@ -32,8 +34,8 @@ func TestPullRequest_ChecksStatus(t *testing.T) { eq(t, err, nil) checks := pr.ChecksStatus() - eq(t, checks.Total, 7) - eq(t, checks.Pending, 2) + eq(t, checks.Total, 8) + eq(t, checks.Pending, 3) eq(t, checks.Failing, 3) eq(t, checks.Passing, 2) } diff --git a/api/queries_pr.go b/api/queries_pr.go index b2e5f2f77..0f11f4392 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -120,7 +120,7 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { summary.Passing++ case "ERROR", "FAILURE", "CANCELLED", "TIMED_OUT", "ACTION_REQUIRED": summary.Failing++ - case "EXPECTED", "REQUESTED", "QUEUED", "PENDING", "IN_PROGRESS": + case "EXPECTED", "REQUESTED", "QUEUED", "PENDING", "IN_PROGRESS", "STALE": summary.Pending++ default: panic(fmt.Errorf("unsupported status: %q", state)) From a6335396cf42abff4fbd68f0737879b6d0849cc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 13:11:17 +0100 Subject: [PATCH 14/19] repo fork: simplify printing the name of an existing forked repo --- command/repo.go | 10 +--------- command/repo_test.go | 3 +-- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/command/repo.go b/command/repo.go index be930c0c8..39de4bb86 100644 --- a/command/repo.go +++ b/command/repo.go @@ -301,14 +301,6 @@ func repoFork(cmd *cobra.Command, args []string) error { s.FinalMSG = utils.Gray(fmt.Sprintf("- %s\n", loading)) s.Start() - authLogin, err := ctx.AuthLogin() - if err != nil { - s.Stop() - return fmt.Errorf("could not determine current username: %w", err) - } - - possibleFork := ghrepo.New(authLogin, toFork.RepoName()) - forkedRepo, err := api.ForkRepo(apiClient, toFork) if err != nil { s.Stop() @@ -325,7 +317,7 @@ func repoFork(cmd *cobra.Command, args []string) error { if created_ago > time.Minute { fmt.Fprintf(out, "%s %s %s\n", utils.Yellow("!"), - utils.Bold(ghrepo.FullName(possibleFork)), + utils.Bold(ghrepo.FullName(forkedRepo)), "already exists") } else { fmt.Fprintf(out, "%s Created fork %s\n", greenCheck, utils.Bold(ghrepo.FullName(forkedRepo))) diff --git a/command/repo_test.go b/command/repo_test.go index 6001be817..cc5cefe84 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -18,9 +18,8 @@ import ( func TestRepoFork_already_forked(t *testing.T) { initContext = func() context.Context { ctx := context.NewBlank() - ctx.SetBaseRepo("REPO") + ctx.SetBaseRepo("OWNER/REPO") ctx.SetBranch("master") - ctx.SetAuthLogin("someone") ctx.SetRemotes(map[string]string{ "origin": "OWNER/REPO", }) From 0f98d56649c0373ae2a3dd48bf0cb9173156f015 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 13:11:36 +0100 Subject: [PATCH 15/19] Rename underscored variable to camelcase --- command/repo.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/repo.go b/command/repo.go index 39de4bb86..6c77300da 100644 --- a/command/repo.go +++ b/command/repo.go @@ -313,8 +313,8 @@ func repoFork(cmd *cobra.Command, args []string) error { // returns the fork repo data even if it already exists -- with no change in status code or // anything. We thus check the created time to see if the repo is brand new or not; if it's not, // we assume the fork already existed and report an error. - created_ago := Since(forkedRepo.CreatedAt) - if created_ago > time.Minute { + createdAgo := Since(forkedRepo.CreatedAt) + if createdAgo > time.Minute { fmt.Fprintf(out, "%s %s %s\n", utils.Yellow("!"), utils.Bold(ghrepo.FullName(forkedRepo)), From 965f2704f34acf03482d25eb08adcf5408d5e441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 13:27:22 +0100 Subject: [PATCH 16/19] repo fork: reuse existing git remote Avoid adding a new git remote for a fork if an existing remote is found that points to the exact forked repo. --- command/repo.go | 9 +++++++++ command/repo_test.go | 26 +++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/command/repo.go b/command/repo.go index 6c77300da..7f31b816c 100644 --- a/command/repo.go +++ b/command/repo.go @@ -328,6 +328,15 @@ func repoFork(cmd *cobra.Command, args []string) error { } if inParent { + remotes, err := ctx.Remotes() + if err != nil { + return err + } + if remote, err := remotes.FindByRepo(forkedRepo.RepoOwner(), forkedRepo.RepoName()); err == nil { + fmt.Fprintf(out, "%s Using existing remote %s\n", greenCheck, utils.Bold(remote.Name)) + return nil + } + remoteDesired := remotePref == "true" if remotePref == "prompt" { err = Confirm("Would you like to add a remote for the fork?", &remoteDesired) diff --git a/command/repo_test.go b/command/repo_test.go index cc5cefe84..6f4591b60 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -40,6 +40,31 @@ func TestRepoFork_already_forked(t *testing.T) { } } +func TestRepoFork_reuseRemote(t *testing.T) { + initContext = func() context.Context { + ctx := context.NewBlank() + ctx.SetBaseRepo("OWNER/REPO") + ctx.SetBranch("master") + ctx.SetRemotes(map[string]string{ + "upstream": "OWNER/REPO", + "origin": "someone/REPO", + }) + return ctx + } + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + defer http.StubWithFixture(200, "forkResult.json")() + + output, err := RunCommand(repoForkCmd, "repo fork") + if err != nil { + t.Errorf("got unexpected error: %v", err) + } + if !strings.Contains(output.String(), "Using existing remote origin") { + t.Errorf("output did not match: %q", output) + return + } +} + func stubSince(d time.Duration) func() { originalSince := Since Since = func(t time.Time) time.Duration { @@ -579,7 +604,6 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { } } - func TestRepoView(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() From c17251d106a8c368c75c2e4dd6944a2c98033afb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 13:52:31 +0100 Subject: [PATCH 17/19] Add table-driven test for `repoFromURL` --- internal/ghrepo/repo_test.go | 76 +++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 28 deletions(-) diff --git a/internal/ghrepo/repo_test.go b/internal/ghrepo/repo_test.go index 6ff775c84..cc0f231b8 100644 --- a/internal/ghrepo/repo_test.go +++ b/internal/ghrepo/repo_test.go @@ -1,40 +1,60 @@ package ghrepo import ( + "errors" + "fmt" "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) + tests := []struct { + name string + input string + result string + err error + }{ + { + name: "github.com URL", + input: "https://github.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + err: nil, + }, + { + name: "unsupported hostname", + input: "https://example.com/one/two", + result: "", + err: errors.New("unsupported hostname: example.com"), + }, + { + name: "filesystem path", + input: "/path/to/file", + result: "", + err: errors.New("unsupported hostname: "), + }, } - 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) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := url.Parse(tt.input) + if err != nil { + t.Fatalf("got error %q", err) + } + + repo, err := FromURL(u) + if err != nil { + if tt.err == nil { + t.Fatalf("got error %q", err) + } else if tt.err.Error() == err.Error() { + return + } + t.Fatalf("got error %q", err) + } + + got := fmt.Sprintf("%s/%s", repo.RepoOwner(), repo.RepoName()) + if tt.result != got { + t.Errorf("expected %q, got %q", tt.result, got) + } + }) } } From 303f60815f72f9fc1b7d449eb3dbf503c89d9ffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 13:55:16 +0100 Subject: [PATCH 18/19] Support `www.github.com` git remote URLs --- internal/ghrepo/repo.go | 2 +- internal/ghrepo/repo_test.go | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index f683d9249..fe3a0eba7 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -33,7 +33,7 @@ func FromFullName(nwo string) Interface { } func FromURL(u *url.URL) (Interface, error) { - if !strings.EqualFold(u.Hostname(), defaultHostname) { + if !strings.EqualFold(u.Hostname(), defaultHostname) && !strings.EqualFold(u.Hostname(), "www."+defaultHostname) { return nil, fmt.Errorf("unsupported hostname: %s", u.Hostname()) } parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 3) diff --git a/internal/ghrepo/repo_test.go b/internal/ghrepo/repo_test.go index cc0f231b8..fef04fb8c 100644 --- a/internal/ghrepo/repo_test.go +++ b/internal/ghrepo/repo_test.go @@ -20,6 +20,12 @@ func Test_repoFromURL(t *testing.T) { result: "monalisa/octo-cat", err: nil, }, + { + name: "www.github.com URL", + input: "http://www.GITHUB.com/monalisa/octo-cat.git", + result: "monalisa/octo-cat", + err: nil, + }, { name: "unsupported hostname", input: "https://example.com/one/two", From 894899c71d666d48c90a1bad53d1568ccab92ae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 27 Mar 2020 13:58:50 +0100 Subject: [PATCH 19/19] Add code docs for `ghrepo` package --- internal/ghrepo/repo.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index fe3a0eba7..a4bfa82d6 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -8,21 +8,26 @@ import ( const defaultHostname = "github.com" +// Interface describes an object that represents a GitHub repository type Interface interface { RepoName() string RepoOwner() string } +// New instantiates a GitHub repository from owner and name arguments func New(owner, repo string) Interface { return &ghRepo{ owner: owner, name: repo, } } + +// FullName serializes a GitHub repository into an "OWNER/REPO" string func FullName(r Interface) string { return fmt.Sprintf("%s/%s", r.RepoOwner(), r.RepoName()) } +// FromFullName extracts the GitHub repository inforation from an "OWNER/REPO" string func FromFullName(nwo string) Interface { var r ghRepo parts := strings.SplitN(nwo, "/", 2) @@ -32,6 +37,7 @@ func FromFullName(nwo string) Interface { return &r } +// FromURL extracts the GitHub repository information from a 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()) @@ -43,6 +49,7 @@ func FromURL(u *url.URL) (Interface, error) { return New(parts[0], strings.TrimSuffix(parts[1], ".git")), nil } +// IsSame compares two GitHub repositories func IsSame(a, b Interface) bool { return strings.EqualFold(a.RepoOwner(), b.RepoOwner()) && strings.EqualFold(a.RepoName(), b.RepoName())