STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #704 from cli/branch-push-detection

Browse source 
Avoid auto-forking/pushing an already pushed branch in `pr create`
This commit is contained in:
Mislav Marohnić 2020-03-30 13:28:49 +02:00 • committed by GitHub
commit 04324191fe
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 264 additions and 39 deletions
Download patch file Download diff file Expand all files Collapse all files

2
command/pr_checkout.go
View file

@ -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 {

12
command/pr_checkout_test.go
View file

@ -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)

113
command/pr_create.go
View file

@ -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",

131
command/pr_create_test.go
View file

@ -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"})
}

45
git/git.go
View file

@ -13,10 +13,41 @@ 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
}
// 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...)
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
@ -152,7 +183,7 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) {
continue
}
cfg.RemoteURL = u
} else {
} else if !isFilesystemPath(parts[1]) {
cfg.RemoteName = parts[1]
}
case "merge":
@ -162,6 +193,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")