Repository: cli/cli
Pull request #10187 (2025-01-06 16:09:51)

Myriad fixes to provide clarity on determining tracking ref in PR create
## Description

Intended to be merged onto #10177

PR A/C tests pass:

```
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│  STATUS │ ELAPSED │                        TEST                        │             PACKAGE               │
│─────────┼─────────┼────────────────────────────────────────────────────┼───────────────────────────────────│
│  PASS   │   13.63 │ TestPullRequests/pr-checkout-with-url-from-fork    │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   12.09 │ TestPullRequests/pr-merge-merge-strategy           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   11.92 │ TestPullRequests/pr-merge-rebase-strategy          │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   11.47 │ TestPullRequests/pr-view-same-org-fork             │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   11.44 │ TestPullRequests/pr-create-from-issue-develop-base │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   11.37 │ TestPullRequests/pr-comment                        │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   10.88 │ TestPullRequests/pr-view                           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │   10.70 │ TestPullRequests/pr-create-with-metadata           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    9.40 │ TestPullRequests/pr-create-from-manual-merge-base  │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    8.98 │ TestPullRequests/pr-checkout-by-number             │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    8.63 │ TestPullRequests/pr-list                           │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    8.02 │ TestPullRequests/pr-create-without-upstream-config │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    7.34 │ TestPullRequests/pr-checkout                       │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    7.02 │ TestPullRequests/pr-create-basic                   │ github.com/cli/cli/v2/acceptance  │
│  PASS   │    0.00 │ TestPullRequests                                   │ github.com/cli/cli/v2/acceptance  │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
┌────────────────────────────────────────────────────────────────────────────────────┐
│  STATUS │ ELAPSED │             PACKAGE              │ COVER │ PASS │ FAIL │ SKIP  │
│─────────┼─────────┼──────────────────────────────────┼───────┼──────┼──────┼───────│
│  PASS   │ 24.56s  │ github.com/cli/cli/v2/acceptance │ 15.6% │  15  │  0   │  0    │
└────────────────────────────────────────────────────────────────────────────────────┘
```


Patch:
diff --git a/git/objects.go b/git/objects.go
index ae058a01c..c09683042 100644
--- a/git/objects.go
+++ b/git/objects.go
@@ -54,16 +54,6 @@ 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
-}
-
 type Commit struct {
 	Sha   string
 	Title string
diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go
index 6a1e13849..db160b419 100644
--- a/pkg/cmd/pr/create/create.go
+++ b/pkg/cmd/pr/create/create.go
@@ -518,44 +518,80 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u
 	return nil
 }
 
-func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig *git.BranchConfig) *git.TrackingRef {
-	refsForLookup := []string{"HEAD"}
-	var trackingRefs []git.TrackingRef
+// 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
+}
 
-	if headBranchConfig.RemoteName != "" {
-		tr := git.TrackingRef{
-			RemoteName: headBranchConfig.RemoteName,
-			BranchName: strings.TrimPrefix(headBranchConfig.MergeRef, "refs/heads/"),
+func mustParseTrackingRef(text string) trackingRef {
+	parts := strings.SplitN(string(text), "/", 4)
+	// The only place this is called is tryDetermineTrackingRef, where we are reconstructing
+	// the same tracking ref we passed in. If it doesn't match the expected format, this is a
+	// programmer error we want to know about, so it's ok to panic.
+	if len(parts) != 4 {
+		panic(fmt.Errorf("tracking ref should have four parts: %s", text))
+	}
+	if parts[0] != "refs" || parts[1] != "remotes" {
+		panic(fmt.Errorf("tracking ref should start with refs/remotes/: %s", text))
+	}
+
+	return trackingRef{
+		remoteName: parts[2],
+		branchName: parts[3],
+	}
+}
+
+// tryDetermineTrackingRef is intended to try and find a remote branch on the same commit as the currently checked out
+// HEAD, i.e. the local branch. If there are multiple branches that might match, the first remote is chosen, which in
+// practice is determined by the sorting algorithm applied much earlier in the process, roughly "upstream", "github", "origin",
+// and then everything else unstably sorted.
+func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) (trackingRef, bool) {
+	// To try and determine the tracking ref for a local branch, we first construct a collection of refs
+	// that might be tracking, given the current branch's config, and the list of known remotes.
+	refsForLookup := []string{"HEAD"}
+	if headBranchConfig.RemoteName != "" && headBranchConfig.MergeRef != "" {
+		tr := 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: localBranchName,
+		tr := trackingRef{
+			remoteName: remote.Name,
+			branchName: localBranchName,
 		}
-		trackingRefs = append(trackingRefs, tr)
 		refsForLookup = append(refsForLookup, tr.String())
 	}
 
+	// Then we ask git for details about these refs, for example, refs/remotes/origin/trunk might return a hash
+	// for the remote tracking branch, trunk, for the remote, origin. If there is no ref, the git client returns
+	// no ref information.
+	//
+	// We also first check for the HEAD ref, so that we have the hash of the currently checked out commit.
 	resolvedRefs, _ := gitClient.ShowRefs(context.Background(), refsForLookup)
+
+	// If there is more than one resolved ref, that means that at least one ref was found in addition to the HEAD.
 	if len(resolvedRefs) > 1 {
+		headRef := resolvedRefs[0]
 		for _, r := range resolvedRefs[1:] {
-			if r.Hash != resolvedRefs[0].Hash {
+			// If the hash of the remote ref doesn't match the hash of HEAD then the remote branch is not in the same
+			// state, so it can't be used.
+			if r.Hash != headRef.Hash {
 				continue
 			}
-			for _, tr := range trackingRefs {
-				if tr.String() != r.Name {
-					continue
-				}
-				return &tr
-			}
+			// Otherwise we can parse the returned ref into a tracking ref and return that
+			return mustParseTrackingRef(r.Name), true
 		}
 	}
 
-	return nil
+	return trackingRef{}, false
 }
 
 func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) {
@@ -647,14 +683,14 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) {
 	headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch)
 	if isPushEnabled {
 		// determine whether the head branch is already pushed to a remote
-		if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch, &headBranchConfig); pushedTo != nil {
+		if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found {
 			isPushEnabled = false
-			if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil {
+			if r, err := remotes.FindByName(trackingRef.remoteName); err == nil {
 				headRepo = r
 				headRemote = r
-				headBranchLabel = pushedTo.BranchName
+				headBranchLabel = trackingRef.branchName
 				if !ghrepo.IsSame(baseRepo, headRepo) {
-					headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), pushedTo.BranchName)
+					headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), trackingRef.branchName)
 				}
 			}
 		}


Test patch:
diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go
index 9d6584e6c..6c0ff11e2 100644
--- a/pkg/cmd/pr/create/create_test.go
+++ b/pkg/cmd/pr/create/create_test.go
@@ -1622,12 +1622,13 @@ func Test_createRun(t *testing.T) {
 	}
 }
 
-func Test_determineTrackingBranch(t *testing.T) {
+func Test_tryDetermineTrackingRef(t *testing.T) {
 	tests := []struct {
-		name     string
-		cmdStubs func(*run.CommandStubber)
-		remotes  context.Remotes
-		assert   func(ref *git.TrackingRef, t *testing.T)
+		name                string
+		cmdStubs            func(*run.CommandStubber)
+		remotes             context.Remotes
+		expectedTrackingRef trackingRef
+		expectedFound       bool
 	}{
 		{
 			name: "empty",
@@ -1635,54 +1636,53 @@ func Test_determineTrackingBranch(t *testing.T) {
 				cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "")
 				cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD")
 			},
-			assert: func(ref *git.TrackingRef, t *testing.T) {
-				assert.Nil(t, ref)
-			},
+			expectedTrackingRef: trackingRef{},
+			expectedFound:       false,
 		},
 		{
 			name: "no match",
 			cmdStubs: func(cs *run.CommandStubber) {
 				cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "")
-				cs.Register("git show-ref --verify -- HEAD refs/remotes/origin/feature refs/remotes/upstream/feature", 0, "abc HEAD\nbca refs/remotes/origin/feature")
+				cs.Register("git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature", 0, "abc HEAD\nbca refs/remotes/upstream/feature")
 			},
 			remotes: context.Remotes{
-				&context.Remote{
-					Remote: &git.Remote{Name: "origin"},
-					Repo:   ghrepo.New("hubot", "Spoon-Knife"),
-				},
 				&context.Remote{
 					Remote: &git.Remote{Name: "upstream"},
 					Repo:   ghrepo.New("octocat", "Spoon-Knife"),
 				},
+				&context.Remote{
+					Remote: &git.Remote{Name: "origin"},
+					Repo:   ghrepo.New("hubot", "Spoon-Knife"),
+				},
 			},
-			assert: func(ref *git.TrackingRef, t *testing.T) {
-				assert.Nil(t, ref)
-			},
+			expectedTrackingRef: trackingRef{},
+			expectedFound:       false,
 		},
 		{
 			name: "match",
 			cmdStubs: func(cs *run.CommandStubber) {
 				cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "")
-				cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature refs/remotes/upstream/feature$`, 0, heredoc.Doc(`
+				cs.Register(`git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature$`, 0, heredoc.Doc(`
 		deadbeef HEAD
-		deadb00f refs/remotes/origin/feature
-		deadbeef refs/remotes/upstream/feature
+		deadb00f refs/remotes/upstream/feature
+		deadbeef refs/remotes/origin/feature
 	`))
 			},
 			remotes: context.Remotes{
-				&context.Remote{
-					Remote: &git.Remote{Name: "origin"},
-					Repo:   ghrepo.New("hubot", "Spoon-Knife"),
-				},
 				&context.Remote{
 					Remote: &git.Remote{Name: "upstream"},
 					Repo:   ghrepo.New("octocat", "Spoon-Knife"),
 				},
+				&context.Remote{
+					Remote: &git.Remote{Name: "origin"},
+					Repo:   ghrepo.New("hubot", "Spoon-Knife"),
+				},
 			},
-			assert: func(ref *git.TrackingRef, t *testing.T) {
-				assert.Equal(t, "upstream", ref.RemoteName)
-				assert.Equal(t, "feature", ref.BranchName)
+			expectedTrackingRef: trackingRef{
+				remoteName: "origin",
+				branchName: "feature",
 			},
+			expectedFound: true,
 		},
 		{
 			name: "respect tracking config",
@@ -1702,9 +1702,8 @@ func Test_determineTrackingBranch(t *testing.T) {
 					Repo:   ghrepo.New("hubot", "Spoon-Knife"),
 				},
 			},
-			assert: func(ref *git.TrackingRef, t *testing.T) {
-				assert.Nil(t, ref)
-			},
+			expectedTrackingRef: trackingRef{},
+			expectedFound:       false,
 		},
 	}
 	for _, tt := range tests {
@@ -1719,8 +1718,10 @@ func Test_determineTrackingBranch(t *testing.T) {
 				GitPath: "some/path/git",
 			}
 			headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature")
-			ref := determineTrackingBranch(gitClient, tt.remotes, "feature", &headBranchConfig)
-			tt.assert(ref, t)
+			ref, found := tryDetermineTrackingRef(gitClient, tt.remotes, "feature", headBranchConfig)
+
+			assert.Equal(t, tt.expectedTrackingRef, ref)
+			assert.Equal(t, tt.expectedFound, found)
 		})
 	}
 }