Skip to content

Fix pr create when push.default tracking and no merge ref #10863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BagToad merged 2 commits into from
Apr 24, 2025
Merged

Fix pr create when push.default tracking and no merge ref #10863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fbe092
Fix pr create when push.default tracking and no merge ref
williammartin Apr 24, 2025
607f161
Update pkg/cmd/pr/shared/find_refs_resolution.go
jtmcg Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
skip 'it creates a fork owned by the user running the test'
skip 'this never worked, but could be fixed if we fixed show-refs'

# Setup environment variables used for testscript
env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
env FORK=${REPO}-fork

# Use gh as a credential helper
exec gh auth setup-git

# Get the current username for the fork owner
exec gh api user --jq .login
stdout2env USER

# Create a repository to act as upstream with a file so it has a default branch
exec gh repo create ${ORG}/${REPO} --add-readme --private

# Defer repo cleanup of upstream
defer gh repo delete --yes ${ORG}/${REPO}

# Create a user fork of repository. This will be owned by USER.
exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK}
sleep 5

# Defer repo cleanup of fork
defer gh repo delete --yes ${USER}/${FORK}

# Retrieve fork repository information
exec gh repo view ${USER}/${FORK} --json id --jq '.id'
stdout2env FORK_ID

# Clone the repo
exec gh repo clone ${USER}/${FORK}
cd ${FORK}

# Configure push.default so that it should use the merge ref
exec git config push.default upstream

# But prepare a branch that doesn't have a tracking merge ref
exec git checkout -b feature-branch
exec git commit --allow-empty -m 'Empty Commit'
exec git push origin feature-branch

# Create the PR
exec gh pr create --title 'Feature Title' --body 'Feature Body'
stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1

# Assert that the PR was created with the correct head repository and refs
exec gh pr view --json headRefName,headRepository,baseRefName,isCrossRepository
stdout {"baseRefName":"main","headRefName":"feature-branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true}
33 changes: 33 additions & 0 deletions acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
skip 'it creates a fork owned by the user running the test'

# Setup environment variables used for testscript
env REPO=${SCRIPT_NAME}-${RANDOM_STRING}

# Use gh as a credential helper
exec gh auth setup-git

# Get the current username for the fork owner
exec gh api user --jq .login
stdout2env USER

# Create a repository to act as upstream with a file so it has a default branch
exec gh repo create ${ORG}/${REPO} --add-readme --private

# Defer repo cleanup of upstream
defer gh repo delete --yes ${ORG}/${REPO}

# Clone the repo
exec gh repo clone ${ORG}/${REPO}
cd ${REPO}

# Configure push.default so that it should use the merge ref
exec git config push.default upstream

# But prepare a branch that doesn't have a tracking merge ref
exec git checkout -b feature-branch
exec git commit --allow-empty -m 'Empty Commit'
exec git push origin feature-branch

# Create the PR
exec gh pr create --title 'Feature Title' --body 'Feature Body'
stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1
8 changes: 4 additions & 4 deletions pkg/cmd/pr/shared/find_refs_resolution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,12 +333,12 @@ func tryDetermineDefaultPushTarget(gitClient GitConfigClient, localBranchName st
}

// We assume the PR's branch name is the same as whatever was provided, unless the user has specified
// push.default = upstream or tracking, then we use the branch name from the merge ref.
// push.default = upstream or tracking, then we use the branch name from the merge ref if it exists. Otherwise, we fall back to the local branch name
remoteBranch := localBranchName
if pushDefault == git.PushDefaultUpstream || pushDefault == git.PushDefaultTracking {
Copy link

Copilot AI Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an inline comment to clarify that, if the merge ref is empty, the local branch name is intentionally preserved as the remote branch name.

Suggested change
if pushDefault == git.PushDefaultUpstream || pushDefault == git.PushDefaultTracking {
if pushDefault == git.PushDefaultUpstream || pushDefault == git.PushDefaultTracking {
// If the merge ref is empty, the local branch name is intentionally preserved as the remote branch name.

Copilot uses AI. Check for mistakes.
remoteBranch = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/")
if remoteBranch == "" {
return defaultPushTarget{}, fmt.Errorf("could not determine remote branch name")
mergeRef := strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/")
if mergeRef != "" {
remoteBranch = mergeRef
}
}

Expand Down
8 changes: 5 additions & 3 deletions pkg/cmd/pr/shared/find_refs_resolution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ func TestTryDetermineDefaultPRHead(t *testing.T) {
})
}

t.Run("but if the merge ref is empty, error", func(t *testing.T) {
t.Run("but if the merge ref is empty, use the provided branch name", func(t *testing.T) {
t.Parallel()

repoResolvedFromPushRemoteClient := stubGitConfigClient{
Expand All @@ -474,12 +474,14 @@ func TestTryDetermineDefaultPRHead(t *testing.T) {
pushDefaultFn: stubPushDefault(git.PushDefaultUpstream, nil),
}

_, err := TryDetermineDefaultPRHead(
defaultPRHead, err := TryDetermineDefaultPRHead(
repoResolvedFromPushRemoteClient,
stubRemoteToRepoResolver(ghContext.Remotes{&baseRemote, &forkRemote}, nil),
"feature-branch",
)
require.Error(t, err)
require.NoError(t, err)

require.Equal(t, "feature-branch", defaultPRHead.BranchName)
})
})

Expand Down
Loading