Rework ref usage when finding and creating PRs by williammartin · Pull Request #10621 · cli/cli

williammartin · 2025-03-17T17:14:49Z

Description

Relates to #575

Will also fix: #10644

In reviewing #10513 I found it extremely hard to have confidence. Fundamentally, carrying various forms of potentially illegal state representations around, doing conditional checks everywhere, and splitting and concatenating strings is very challenging.

I decided I really, really wanted to encode the valid states where possible in the type system. So for example, instead of having a boolean "isPushEnabled" toggled on and off throughout various conditional behaviour, we collect it all into a skipPushRefs type that has only the information required.

I've made liberal use of o.Option to indicate when something is optional. Yes, it's possible to put a nil inside an o.Option[ghrepo.Interface] and we don't check for that, which might lead to bugs, but I the approach is still a significant improvement over nil checks everywhere which convey absolutely nothing about the intended optionality of a piece of data.

Finally, I wasted a lot of time trying to converge the refs between create and find, and it didn't work out at all. Fundamentally the acceptable data for each operation varies (create needs baseRepo, baseBranch, qualifiedHeadRef, maybe a head repo for push, while find needs baseRepo, qualifiedHeadRef, optionally a baseBranch and no head repo). Trying to share these data structures resulted in marking lots of data as optional, which just brought back the original problem of the compiler being unable to help.

In some cases I have left behaviour as it used to be even if (or because) I didn't fully understand it, or because changing it would expand an already very large PR for less value. I have left comments in these cases.

As of writing, this passes all the Pull Request A/C tests.

Reviewer Notes

It's probably better to read the files top to bottom rather than trying to work the diffs. It might even make more sense to compare to current trunk rather than the PR this is on top of.

I would be surprised even with all this work if we didn't have bugs after shipping. My hope is that bugs will be easier to ferret out and fix in a sustainable manner going forwards, rather than there being no bugs.

Sorry.

git/client.go

pkg/cmd/pr/shared/finder.go

git/client.go

jtmcg

I definitely like the direction this is headed

git/client.go

git/client_test.go

pkg/cmd/pr/create/create.go

pkg/cmd/pr/shared/finder.go

andyfeller

All in all, I love the changes! I haven't dug into how these affect gh pr view yet but I don't see any major blocking concerns with proceed ✨

pkg/cmd/pr/create/create.go

pkg/cmd/pr/status/status.go

pkg/cmd/pr/shared/finder.go

acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar

williammartin · 2025-03-26T12:23:42Z

acceptance/testdata/pr/pr-create-respects-branch-pushremote.txtar

@@ -1,4 +1,4 @@
-skip 'it creates a fork owned by the user running the test'
+#skip 'it creates a fork owned by the user running the test'


Will revert all these skips these when I'm done.

acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar

pkg/httpmock/registry.go

internal/run/stub.go

pkg/cmd/pr/create/create.go

andyfeller

Just doing a partial review of pkg/cmd/pr packages focused on create, finder, find_refs_resolution by request

tldr: I think these new types / constructs make the code easier to understand. Leaving a variety of questions / nits but nothing to detract or pump the brakes

pkg/cmd/pr/create/create.go

andyfeller · 2025-03-26T13:48:12Z

pkg/cmd/pr/create/create.go

+	QualifiedHeadRef() string
+	UnqualifiedHeadRef() string


question: how would you explain the difference between qualified and unqualified and which would an contributor or maintainer should use for a given purpose?

Good question, it is a challenging distinction and different between the find and the create. The finder uses the Unqualified form to work with the API, and then uses the Qualified form to match on the returned Pull Requests. The create uses the Qualified form with the API. Unqualified is often used with git, Qualified is often used with UI messages.

pkg/cmd/pr/shared/find_refs_resolution.go

pkg/cmd/pr/shared/finder.go

andyfeller

Truly a major effort! There are no major blockers to merging this and finishing the the original PR aside from a few nits.

andyfeller · 2025-03-27T12:09:44Z

acceptance/testdata/pr/pr-status-respects-cross-org.txtar

+# Assert that the PR was created with the correct head repository and refs
+exec gh pr status
+! stdout 'There is no pull request associated with'


So to be clear, the name of this acceptance test is about gh pr status being able to locate the pull request that crosses the user fork-to-upstream relationship, correct?

Beyond the fact that we're using an organization for acceptance testing, would this also work if we tested against a user owned repository rather than an organization?

So to be clear, the name of this acceptance test is about gh pr status being able to locate the pull request that crosses the user fork-to-upstream relationship, correct?

Yes. Sorry, I should have linked that this PR also fixes #10644

would this also work if we tested against a user owned repository rather than an organization?

The test clones into a user namespace, so I think maybe you've got this inverted? I would not expect it to work for orgs, which we know doesn't work correctly with the ref format :

pkg/cmd/pr/create/create.go

andyfeller · 2025-03-27T12:56:07Z

pkg/cmd/pr/create/create_test.go

+				cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
+				cs.Register("git show-ref --verify -- HEAD refs/remotes/origin/feature", 1, "")


question: are the multiple calls from trying to determine tracking ref? shouldn't this only be called once?

Wondering why we're registering the same git show-ref command twice, I was trying to trace the code to answer the question myself, but not sure why. 🤔

question: is there a reason to mix raw strings and double quoted strings?

Just trying to following the trail:

cli/pkg/cmd/pr/create/create.go

Lines 574 to 579 in ce4f5c5

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

cli/pkg/cmd/pr/create/create.go

Lines 688 to 692 in ce4f5c5

if isPushEnabled {

// TODO: This doesn't respect the @{push} revision resolution or triagular workflows assembled with

// remote.pushDefault, or branch.<branchName>.pushremote config settings. The finder's ParsePRRefs

// may be able to replace this function entirely.

if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found {

cli/pkg/cmd/pr/create/create.go

Lines 266 to 267 in ce4f5c5

func createRun(opts *CreateOptions) error {

ctx, err := NewCreateContext(opts)

question: are the multiple calls from trying to determine tracking ref? shouldn't this only be called once?

I think this is explained in the comments here:

cli/pkg/cmd/pr/create/create.go

Lines 783 to 816 in bee6278

// If we were able to determine a head repo, then let's check that the remote tracking ref matches the SHA of

// HEAD. If it does, then we don't need to push, otherwise we'll need to ask the user to tell us where to push.

if headRepo, present := defaultPRHead.Repo.Value(); present {

// We may not find a remote because the git branch config may have a URL rather than a remote name.

// Ideally, we would return a sentinel error from RemoteForRepo that we could compare to, but the

// refactor that introduced this code was already large enough.

headRemote, _ := resolvedRemotes.RemoteForRepo(headRepo)

if headRemote != nil {

resolvedRefs, _ := opts.GitClient.ShowRefs(

context.Background(),

[]string{

"HEAD",

fmt.Sprintf("refs/remotes/%s/%s", headRemote.Name, defaultPRHead.BranchName),

},

)

// Two refs returned means we can compare HEAD to the remote tracking branch.

// If we had a matching ref, then we can skip pushing.

refsMatch := len(resolvedRefs) == 2 && resolvedRefs[0].Hash == resolvedRefs[1].Hash

if refsMatch {

qualifiedHeadRef := shared.NewQualifiedHeadRefWithoutOwner(defaultPRHead.BranchName)

if headRepo.RepoOwner() != baseRepo.RepoOwner() {

qualifiedHeadRef = shared.NewQualifiedHeadRef(headRepo.RepoOwner(), defaultPRHead.BranchName)

}

return newCreateContext(skipPushRefs{

qualifiedHeadRef: qualifiedHeadRef,

baseRefs: baseRefs,

}), nil

}

}

}

// If we didn't determine that the git indicated repo had the correct ref, we'll take a look at the other

TL;DR: if we think we found the repo from git, but it's not on the correct ref, we fall back to guessing. There is a bug in the previous code that makes reuse in both cases wrong. It can be fixed, but I didn't want to expand the scope of changes just to rarely avoid calling show refs twice.

question: is there a reason to mix raw strings and double quoted strings?

Copy and pasting from places that were using heredoc. I'll make them consistent.

andyfeller · 2025-03-27T13:18:19Z

pkg/cmd/pr/create/create_test.go

+	// When the command is run
+	reg := &httpmock.Registry{}
+	reg.StubRepoInfoResponse("OWNER", "REPO", "master")
+	defer reg.Verify(t)
+
+	reg.Register(
+		httpmock.GraphQL(`mutation PullRequestCreate\b`),
+		httpmock.GraphQLMutation(`
+				{ "data": { "createPullRequest": { "pullRequest": {
+					"URL": "https://github.com/OWNER/REPO/pull/12"
+				} } } }`, func(input map[string]interface{}) {
+			assert.Equal(t, "REPOID", input["repositoryId"].(string))
+			assert.Equal(t, "master", input["baseRefName"].(string))
+			assert.Equal(t, "OTHEROWNER:feature", input["headRefName"].(string))
+		}))


note: while following this test, I just wanted to permalink to StubRepoInfoResponse source which is responsible for sourcing these assertion values in order to make sure I was following along

cli/pkg/httpmock/legacy.go

Lines 9 to 23 in ce4f5c5

func (r *Registry) StubRepoInfoResponse(owner, repo, branch string) {

r.Register(

GraphQL(`query RepositoryInfo\b`),

StringResponse(fmt.Sprintf(`

{ "data": { "repository": {

"id": "REPOID",

"name": "%s",

"owner": {"login": "%s"},

"description": "",

"defaultBranchRef": {"name": "%s"},

"hasIssuesEnabled": true,

"viewerPermission": "WRITE"

} } }

`, repo, owner, branch)))

}

Yeh it's spooky action at a distance.

andyfeller · 2025-03-27T13:48:49Z

pkg/cmd/pr/shared/find_refs_resolution_test.go

+	}
+}
+
+func TestPRFindRefs(t *testing.T) {


thought: would love discussing the various architecture approaches to tests like this at some point.

this 1 test contains multiple, parallel test runs including a table-driven test. it makes me step by and wonder the benefits and trade offs to the different approaches, wondering when we choose which.

Sure. Strong opinion: Tests that don't share the same assertions shouldn't be tabled together. Weak opinion: Subtests allow for nice behavioural descriptions compared to top level tests. Not much more than that.

pkg/cmd/pr/shared/find_refs_resolution_test.go

pkg/cmd/pr/shared/finder.go

pkg/httpmock/registry.go

jtmcg

Woo! Good work on this. I appreciate where this went and all the heavy lifting you've done. I think only one of my comments is really actionable, but otherwise I'm happy with this. Don't let me be a blocker to others giving this a ✅

pkg/cmd/pr/shared/find_refs_resolution_test.go

pkg/cmd/pr/create/create_test.go

pkg/cmd/pr/shared/find_refs_resolution.go

pkg/httpmock/registry.go

Co-authored-by: Andy Feller <[email protected]>

williammartin added 2 commits March 17, 2025 16:50

Prepare for removal of ParsePRRefs

70b5af1

Shim the PRResolver

8f8731c

williammartin commented Mar 17, 2025

View reviewed changes

git/client.go Show resolved Hide resolved

williammartin commented Mar 17, 2025

View reviewed changes

pkg/cmd/pr/shared/finder.go Outdated Show resolved Hide resolved

williammartin commented Mar 17, 2025

View reviewed changes

pkg/cmd/pr/shared/finder.go Outdated Show resolved Hide resolved

williammartin force-pushed the wm/pr-resolver-prep branch from f8e918b to 67d2c76 Compare March 17, 2025 17:20

williammartin commented Mar 17, 2025

View reviewed changes

git/client.go Show resolved Hide resolved

williammartin changed the title ~~Wm/pr resolver prep~~ Introduce PRResolver Mar 17, 2025

jtmcg reviewed Mar 17, 2025

View reviewed changes

andyfeller reviewed Mar 17, 2025

View reviewed changes

williammartin force-pushed the wm/pr-resolver-prep branch from 67d2c76 to 055f2e0 Compare March 18, 2025 14:35

williammartin force-pushed the wm/pr-resolver-prep branch 2 times, most recently from c1dfebb to 95ecc45 Compare March 25, 2025 16:58

BagToad reviewed Mar 25, 2025

View reviewed changes

acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar Show resolved Hide resolved

williammartin changed the title ~~Introduce PRResolver~~ Rework ref usage when finding and creating PRs Mar 26, 2025

WIP

e7dc32f

williammartin commented Mar 26, 2025

View reviewed changes

acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar Outdated Show resolved Hide resolved

williammartin commented Mar 26, 2025

View reviewed changes

pkg/httpmock/registry.go Outdated Show resolved Hide resolved

williammartin commented Mar 26, 2025

View reviewed changes

pkg/httpmock/registry.go Show resolved Hide resolved

williammartin commented Mar 26, 2025

View reviewed changes

internal/run/stub.go Show resolved Hide resolved

remove branchConfig

7ce312f

williammartin force-pushed the wm/pr-resolver-prep branch from 95ecc45 to 7ce312f Compare March 26, 2025 12:35

williammartin added 2 commits March 26, 2025 13:46

remove remotesfn

9b9cf58

remove debug

8931ccf

williammartin commented Mar 26, 2025

View reviewed changes

pkg/cmd/pr/create/create.go Outdated Show resolved Hide resolved

williammartin added 3 commits March 26, 2025 14:41

test qualified head ref

390958c

Test PRFindRefs

4beffb0

Run all tests (oops)

613cf31

andyfeller reviewed Mar 26, 2025

View reviewed changes

williammartin marked this pull request as ready for review March 26, 2025 17:02

williammartin requested a review from a team as a code owner March 26, 2025 17:02

williammartin requested review from BagToad and removed request for a team March 26, 2025 17:02

williammartin added 11 commits March 26, 2025 18:03

missing imports

7f6d2b6

test creation

aff8c3c

Cleanup todos

b0582ba

doc baseTrackingbranch

d1f665f

reorder embeddings

a84aad4

avoid name and variable shadowing

583b920

doc refs

de1b09f

Fix typo

71a8100

reformat resolve comment

eabbb04

doc creation refs methods

2a926e1

revert comment skips

bee6278

andyfeller reviewed Mar 27, 2025

View reviewed changes

jtmcg reviewed Mar 27, 2025

View reviewed changes

BagToad approved these changes Mar 31, 2025

View reviewed changes

williammartin and others added 9 commits April 2, 2025 12:21

DRY cached branch config client

213fbe5

Explain test assertion

d8b267c

Use ref constructors

b6ae3e3

remove candlejack comment

f391630

Update pkg/cmd/pr/shared/find_refs_resolution.go

180895f

Co-authored-by: Andy Feller <[email protected]>

USA USA

76904a5

remove susiness

04a1a52

Update pkg/cmd/pr/shared/find_refs_resolution_test.go

0794e43

Co-authored-by: Andy Feller <[email protected]>

Update pkg/httpmock/registry.go

fe88e7e

Co-authored-by: Andy Feller <[email protected]>

williammartin merged commit 8b67d4e into kw/575-detect-push-target-for-local-branches-without-upstream-configuration Apr 2, 2025
14 checks passed

williammartin deleted the wm/pr-resolver-prep branch April 2, 2025 11:21

williammartin mentioned this pull request Apr 2, 2025

gh pr create: Support Git's @{push} revision syntax for determining head ref #10513

Merged

		@@ -1,4 +1,4 @@
		skip 'it creates a fork owned by the user running the test'
		#skip 'it creates a fork owned by the user running the test'

		cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 1, "")
		cs.Register("git show-ref --verify -- HEAD refs/remotes/origin/feature", 1, "")

	// 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 isPushEnabled {
	// TODO: This doesn't respect the @{push} revision resolution or triagular workflows assembled with
	// remote.pushDefault, or branch.<branchName>.pushremote config settings. The finder's ParsePRRefs
	// may be able to replace this function entirely.
	if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found {

	func createRun(opts *CreateOptions) error {
	ctx, err := NewCreateContext(opts)

	// If we were able to determine a head repo, then let's check that the remote tracking ref matches the SHA of
	// HEAD. If it does, then we don't need to push, otherwise we'll need to ask the user to tell us where to push.
	if headRepo, present := defaultPRHead.Repo.Value(); present {
	// We may not find a remote because the git branch config may have a URL rather than a remote name.
	// Ideally, we would return a sentinel error from RemoteForRepo that we could compare to, but the
	// refactor that introduced this code was already large enough.
	headRemote, _ := resolvedRemotes.RemoteForRepo(headRepo)
	if headRemote != nil {
	resolvedRefs, _ := opts.GitClient.ShowRefs(
	context.Background(),
	[]string{
	"HEAD",
	fmt.Sprintf("refs/remotes/%s/%s", headRemote.Name, defaultPRHead.BranchName),
	},
	)

	// Two refs returned means we can compare HEAD to the remote tracking branch.
	// If we had a matching ref, then we can skip pushing.
	refsMatch := len(resolvedRefs) == 2 && resolvedRefs[0].Hash == resolvedRefs[1].Hash
	if refsMatch {
	qualifiedHeadRef := shared.NewQualifiedHeadRefWithoutOwner(defaultPRHead.BranchName)
	if headRepo.RepoOwner() != baseRepo.RepoOwner() {
	qualifiedHeadRef = shared.NewQualifiedHeadRef(headRepo.RepoOwner(), defaultPRHead.BranchName)
	}

	return newCreateContext(skipPushRefs{
	qualifiedHeadRef: qualifiedHeadRef,
	baseRefs: baseRefs,
	}), nil
	}
	}
	}

	// If we didn't determine that the git indicated repo had the correct ref, we'll take a look at the other

	func (r *Registry) StubRepoInfoResponse(owner, repo, branch string) {
	r.Register(
	GraphQL(`query RepositoryInfo\b`),
	StringResponse(fmt.Sprintf(`
	{ "data": { "repository": {
	"id": "REPOID",
	"name": "%s",
	"owner": {"login": "%s"},
	"description": "",
	"defaultBranchRef": {"name": "%s"},
	"hasIssuesEnabled": true,
	"viewerPermission": "WRITE"
	} } }
	`, repo, owner, branch)))
	}

Conversation

williammartin commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtmcg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williammartin Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtmcg left a comment

williammartin commented Mar 17, 2025 •

edited

Loading

williammartin Mar 26, 2025 •

edited

Loading