From 18d72a0eb19d6197a62f38b1914ebb5a1fb4dc72 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mislav=20Marohni=C4=87?= <mislav.marohnic@gmail.com>
Date: Sun, 7 Jul 2019 17:53:19 +0200
Subject: [PATCH] [pull-request] Improve push target detection for
 `push.default=upstream`

When `git config push.default` is "upstream" or "tracking", but the
current branch is pushed to a remote without having upstream
configuration set up (for example, via `git push <REMOTE> HEAD` without
`-u`), this change makes it so that the remote tracking branch (the push
target) is still discovered via the same mechanism as if `push.default`
wasn't set (i.e. iterating through all the remotes).
---
 features/pull_request.feature |  5 ++--
 github/branch.go              | 39 -----------------------------
 github/localrepo.go           | 46 +++++++++++++++++++++++++++++------
 3 files changed, 42 insertions(+), 48 deletions(-)

diff --git a/features/pull_request.feature b/features/pull_request.feature
index d71efef5..88bf4c54 100644
--- a/features/pull_request.feature
+++ b/features/pull_request.feature
@@ -947,11 +947,12 @@ Feature: hub pull-request
     Then the output should contain exactly "the://url\n"
 
   Scenario: Current branch is pushed to remote without upstream configuration
-    Given git "push.default" is set to "upstream"
+    Given the "upstream" remote has url "git://github.com/lestephane/coral.git"
     And I am on the "feature" branch pushed to "origin/feature"
+    And git "push.default" is set to "upstream"
     Given the GitHub API server:
       """
-      post('/repos/mislav/coral/pulls') {
+      post('/repos/lestephane/coral/pulls') {
         assert :base  => 'master',
                :head  => 'mislav:feature'
         status 201
diff --git a/github/branch.go b/github/branch.go
index 7676d69b..7f21b6dc 100644
--- a/github/branch.go
+++ b/github/branch.go
@@ -23,45 +23,6 @@ func (b *Branch) LongName() string {
 	return reg.ReplaceAllString(b.Name, "")
 }
 
-// see also RemoteForBranch()
-func (b *Branch) PushTarget(owner string, preferUpstream bool) (branch *Branch) {
-	var err error
-	pushDefault, _ := git.Config("push.default")
-	if pushDefault == "upstream" || pushDefault == "tracking" {
-		branch, err = b.Upstream()
-		if err != nil {
-			return
-		}
-	} else {
-		shortName := b.ShortName()
-		remotes := b.Repo.remotesForPublish(owner)
-
-		var remotesInOrder []Remote
-		if preferUpstream {
-			// reverse the remote lookup order
-			// see OriginNamesInLookupOrder
-			for i := len(remotes) - 1; i >= 0; i-- {
-				remotesInOrder = append(remotesInOrder, remotes[i])
-			}
-		} else {
-			remotesInOrder = remotes
-		}
-
-		for _, remote := range remotesInOrder {
-			if _, err := remote.Project(); err != nil {
-				continue
-			}
-			if git.HasFile("refs", "remotes", remote.Name, shortName) {
-				name := fmt.Sprintf("refs/remotes/%s/%s", remote.Name, shortName)
-				branch = &Branch{b.Repo, name}
-				break
-			}
-		}
-	}
-
-	return
-}
-
 func (b *Branch) RemoteName() string {
 	reg := regexp.MustCompile("^refs/remotes/([^/]+)")
 	if reg.MatchString(b.Name) {
diff --git a/github/localrepo.go b/github/localrepo.go
index e275c6e3..bee18717 100644
--- a/github/localrepo.go
+++ b/github/localrepo.go
@@ -140,23 +140,55 @@ func (r *GitHubRepo) RemoteBranchAndProject(owner string, preferUpstream bool) (
 		return
 	}
 
-	if project != nil {
-		branch = branch.PushTarget(owner, preferUpstream)
-		if branch != nil && branch.IsRemote() {
-			remote, e := r.RemoteByName(branch.RemoteName())
+	if project == nil {
+		return
+	}
+
+	pushDefault, _ := git.Config("push.default")
+	if pushDefault == "upstream" || pushDefault == "tracking" {
+		upstream, e := branch.Upstream()
+		if e == nil && upstream.IsRemote() {
+			remote, e := r.RemoteByName(upstream.RemoteName())
 			if e == nil {
-				project, err = remote.Project()
-				if err != nil {
+				p, e := remote.Project()
+				if e == nil {
+					branch = upstream
+					project = p
 					return
 				}
 			}
 		}
 	}
 
+	shortName := branch.ShortName()
+	remotes := r.remotesForPublish(owner)
+	if preferUpstream {
+		// reverse the remote lookup order; see OriginNamesInLookupOrder
+		remotesInOrder := []Remote{}
+		for i := len(remotes) - 1; i >= 0; i-- {
+			remotesInOrder = append(remotesInOrder, remotes[i])
+		}
+		remotes = remotesInOrder
+	}
+
+	for _, remote := range remotes {
+		p, e := remote.Project()
+		if e != nil {
+			continue
+		}
+		// NOTE: this is similar RemoteForBranch
+		if git.HasFile("refs", "remotes", remote.Name, shortName) {
+			name := fmt.Sprintf("refs/remotes/%s/%s", remote.Name, shortName)
+			branch = &Branch{r, name}
+			project = p
+			return
+		}
+	}
+
+	branch = nil
 	return
 }
 
-// duplicates logic from PushTarget()
 func (r *GitHubRepo) RemoteForBranch(branch *Branch, owner string) *Remote {
 	branchName := branch.ShortName()
 	for _, remote := range r.remotesForPublish(owner) {
-- 
GitLab