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.

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
merged 2 commits into from
Apr 24, 2025

Conversation

williammartin
Copy link
Member

Description

Fixes #10862

When reworking push target resolution in #10513 that went into v2.70.0, we made a mistake expecting that a merge ref would always be set on a branch, when the push.default is tracking or upstream. When I wrote this code I did wonder if this would be a normal flow for people but we have no telemetry. Now we know that people expect this to work, and it did work on v2.69.0, as per the A/C test https://github.com/cli/cli/blob/8fbe0928bd0d349cd7203f852cf886fcd49b5b58/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar

I've added another A/C test for the same behaviour but when there is a fork involved. I checked and this failed on v2.69.0, so there is no regression. It fails because of the bug described here:

// If we didn't determine that the git indicated repo had the correct ref, we'll take a look at the other
// remotes and see whether any of them have the same SHA as HEAD. Now, at this point, you might be asking yourself:
// "Why didn't we collect all the SHAs with a single ShowRefs command above, for use in both cases?"
// ...
// That's because the code below has a bug that I've ported from the old code, in order to preserve the existing
// behaviour, and to limit the scope of an already large refactor. The intention of the original code was to loop
// over all the returned refs. However, as it turns out, our implementation of ShowRefs doesn't do that correctly.
// Since it provides the --verify flag, git will return the SHAs for refs up until it hits a ref that doesn't exist,
// at which point it bails out.
//
// Imagine you have a remotes "upstream" and "origin", and you have pushed your branch "feature" to "origin". Since
// the order of remotes is always guaranteed "upstream", "github", "origin", and then everything else unstably sorted,
// we will never get a SHA for origin, as refs/remotes/upstream/feature doesn't exist.
//
// Furthermore, when you really think about it, this code is a bit eager. What happens if you have the same SHA on
// remotes "origin" and "colleague", this will always offer origin. If it were "colleague-a" and "colleague-b", no
// order would be guaranteed between different invocations of pr create, because the order of remotes after "origin"
// is unstable sorted.
//
// All that said, this has been the behaviour for a long, long time, and I do not want to make other behavioural changes
// in what is mostly a refactor.

@Copilot Copilot AI review requested due to automatic review settings April 24, 2025 16:00
@williammartin williammartin requested a review from a team as a code owner April 24, 2025 16:00
@williammartin williammartin linked an issue Apr 24, 2025 that may be closed by this pull request
@williammartin williammartin requested a review from babakks April 24, 2025 16:00
@williammartin williammartin requested a review from BagToad April 24, 2025 16:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the push target determination logic so that when a branch's merge ref is empty (as may occur when push.default is tracking/upstream), the provided branch name is used instead of throwing an error.

  • Fixes handling when merge ref is empty.
  • Updates the test expectation in find_refs_resolution_test.go to reflect the new behavior.

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.

File Description
pkg/cmd/pr/shared/find_refs_resolution_test.go Updated test to expect usage of branch name instead of an error when merge ref is empty
pkg/cmd/pr/shared/find_refs_resolution.go Modified push target resolution to use the branch name if merge ref is empty
Files not reviewed (2)
  • acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar: Language not supported
  • acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar: Language not supported

@@ -336,9 +336,9 @@ func tryDetermineDefaultPushTarget(gitClient GitConfigClient, localBranchName st
// push.default = upstream or tracking, then we use the branch name from the merge ref.
remoteBranch := localBranchName
if pushDefault == git.PushDefaultUpstream || pushDefault == git.PushDefaultTracking {
Copy link
Preview

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.

Copy link

@cs278 cs278 left a comment

Choose a reason for hiding this comment

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

Works as expected:

$ ./gh version
gh version 2.71.1-1-g8fbe0928b (2025-04-24)
https://github.com/cli/cli/releases/latest
$ ./gh pr create --assignee cs278 
? Where should we push the 'issue/8670-update-docs' branch?  [Use arrows to move, type to filter]

Copy link
Member

@babakks babakks left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@BagToad BagToad left a comment

Choose a reason for hiding this comment

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

All AC passes except the one you describe which is expected:

--- FAIL: TestPullRequests (0.00s)
    --- PASS: TestPullRequests/pr-view (8.32s)
    --- PASS: TestPullRequests/pr-create-without-upstream-config (8.37s)
    --- PASS: TestPullRequests/pr-checkout-by-number (9.58s)
    --- PASS: TestPullRequests/pr-view-status-respects-simple-pushdefault (9.72s)
    --- PASS: TestPullRequests/pr-view-status-respects-push-destination (9.85s)
    --- PASS: TestPullRequests/pr-view-same-org-fork (15.88s)
    --- PASS: TestPullRequests/pr-merge-merge-strategy (16.07s)
    --- PASS: TestPullRequests/pr-status-respects-cross-org (16.90s)
    --- PASS: TestPullRequests/pr-list (7.50s)
    --- PASS: TestPullRequests/pr-create-respects-push-destination (17.42s)
    --- PASS: TestPullRequests/pr-create-with-metadata (8.48s)
    --- PASS: TestPullRequests/pr-merge-rebase-strategy (10.32s)
    --- PASS: TestPullRequests/pr-view-status-respects-remote-pushdefault (19.19s)
    --- PASS: TestPullRequests/pr-create-no-local-repo (5.86s)
    --- PASS: TestPullRequests/pr-create-respects-user-colon-branch-syntax (15.92s)
    --- PASS: TestPullRequests/pr-create-from-issue-develop-base (14.59s)
    --- PASS: TestPullRequests/pr-create-push-default-upstream-no-merge-ref (7.81s)
    --- PASS: TestPullRequests/pr-create-from-manual-merge-base (8.03s)
    --- FAIL: TestPullRequests/pr-create-push-default-upstream-no-merge-ref-fork (13.06s)
    --- PASS: TestPullRequests/pr-view-outside-repo (7.06s)
    --- PASS: TestPullRequests/pr-comment-edit-last-without-comments-errors (6.99s)
    --- PASS: TestPullRequests/pr-create-remote-ref-with-branch-name-slash (16.48s)
    --- PASS: TestPullRequests/pr-create-guesses-remote-from-sha (17.42s)
    --- PASS: TestPullRequests/pr-create-respects-branch-pushremote (17.73s)
    --- PASS: TestPullRequests/pr-comment-edit-last-with-comments (10.34s)
    --- PASS: TestPullRequests/pr-create-guesses-remote-from-sha-with-branch-name-slash (17.17s)
    --- PASS: TestPullRequests/pr-comment-edit-last-without-comments-creates (8.13s)
    --- PASS: TestPullRequests/pr-create-basic (6.97s)
    --- PASS: TestPullRequests/pr-create-respects-simple-pushdefault (9.26s)
    --- PASS: TestPullRequests/pr-comment-new (8.07s)
    --- PASS: TestPullRequests/pr-checkout (8.17s)
    --- PASS: TestPullRequests/pr-create-edit-with-project (22.84s)
    --- PASS: TestPullRequests/pr-create-respects-remote-pushdefault (15.67s)
    --- PASS: TestPullRequests/pr-view-status-respects-branch-pushremote (16.63s)
    --- PASS: TestPullRequests/pr-checkout-with-url-from-fork (18.15s)
FAIL
FAIL    github.com/cli/cli/v2/acceptance        53.408s
FAIL

@jtmcg jtmcg enabled auto-merge April 24, 2025 16:36
@BagToad BagToad disabled auto-merge April 24, 2025 16:39
@BagToad BagToad enabled auto-merge (squash) April 24, 2025 16:39
@BagToad BagToad merged commit fb97b3e into trunk Apr 24, 2025
7 checks passed
@BagToad BagToad deleted the 10862-creating-pr-cannot-determine-remote-branch-name branch April 24, 2025 16:41
rancorm pushed a commit to rancorm/cli that referenced this pull request May 3, 2025
* Fix pr create when push.default tracking and no merge ref

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

---------

Co-authored-by: Tyler McGoffin <jtmcg@github.com>
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request May 10, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cli/cli](https://github.com/cli/cli) | minor | `v2.69.0` -> `v2.72.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>cli/cli (cli/cli)</summary>

### [`v2.72.0`](https://github.com/cli/cli/releases/tag/v2.72.0): GitHub CLI 2.72.0

[Compare Source](cli/cli@v2.71.2...v2.72.0)

#### :accessibility: Accessibility public preview

This release marks the public preview of several accessibility improvements to the GitHub CLI that have been under development over the past year in partnership with our friends at [Charm](https://github.com/charmbracelet) including:

-   customizable and contrasting colors
-   non-interactive user input prompting
-   text-based spinners

These new experiences are captured in a new `gh a11y` help topic command, which goes into greater detail into the motivation behind each of them as well as opt-in configuration settings / environment variables.

We would like you to share your feedback and join us on this journey through one of [GitHub Accessibility feedback channels](https://accessibility.github.com/feedback)! 🙌

#### What's Changed

##### ✨ Features

-   Introduce `gh accessibility` help topic highlighting GitHub CLI accessibility experiences by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10890
-   \[gh pr view] Support `closingIssuesReferences` JSON field by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10544

##### 🐛 Fixes

-   Fix expected error output of `TestRepo/repo-set-default` by [@&#8203;aconsuegra](https://github.com/aconsuegra) in cli/cli#10884
-   Ensure accessible password and auth token prompters disable echo mode by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10885
-   Fix: Accessible multiselect prompt respects default selections by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10901

#### New Contributors

-   [@&#8203;aconsuegra](https://github.com/aconsuegra) made their first contribution in cli/cli#10884

**Full Changelog**: cli/cli@v2.71.2...v2.72.0

### [`v2.71.2`](https://github.com/cli/cli/releases/tag/v2.71.2): GitHub CLI 2.71.2

[Compare Source](cli/cli@v2.71.1...v2.71.2)

#### What's Changed

-   Fix pr create when push.default tracking and no merge ref by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10863

**Full Changelog**: cli/cli@v2.71.1...v2.71.2

### [`v2.71.1`](https://github.com/cli/cli/releases/tag/v2.71.1): GitHub CLI 2.71.1

[Compare Source](cli/cli@v2.71.0...v2.71.1)

#### What's Changed

-   Fix pr create when branch name contains slashes by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10859

**Full Changelog**: cli/cli@v2.71.0...v2.71.1

### [`v2.71.0`](https://github.com/cli/cli/releases/tag/v2.71.0): GitHub CLI 2.71.0

[Compare Source](cli/cli@v2.70.0...v2.71.0)

#### What's Changed

##### ✨ Features

-   `gh pr create`: Support Git's `@{push}` revision syntax for determining head ref by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10513
-   Introduce option to opt-out of spinners by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10773
-   Update configuration support for accessible colors by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10820
-   `gh config`: add config settings for accessible prompter and disabling spinner by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10846

##### 🐛 Fixes

-   Fix multi pages search for gh search by [@&#8203;leudz](https://github.com/leudz) in cli/cli#10767
-   Fix: `project` commands use shared progress indicator by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10817
-   Issue commands should parse args early by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10811
-   Feature detect v1 projects on `issue view` by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10813
-   Feature detect v1 projects on non web-mode `issue create` by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10815
-   Feature detect v1 projects on web mode issue create by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10818
-   Feature detect v1 projects on issue edit by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10819

##### 📚 Docs & Chores

-   Refactor Sigstore verifier logic by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10750

##### :dependabot: Dependencies

-   chore(deps): bump github.com/sigstore/sigstore-go from 0.7.1 to 0.7.2 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10787
-   Bump google.golang.org/grpc from 1.71.0 to 1.71.1 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10758

#### New Contributors

-   [@&#8203;leudz](https://github.com/leudz) made their first contribution in cli/cli#10767

**Full Changelog**: cli/cli@v2.70.0...v2.71.0

### [`v2.70.0`](https://github.com/cli/cli/releases/tag/v2.70.0): GitHub CLI 2.70.0

[Compare Source](cli/cli@v2.69.0...v2.70.0)

#### Accessibility

This release contains dark shipped changes that are part of a larger GitHub CLI accessibility preview still under development.  More information about these will be announced later this month including various channels to work with GitHub and GitHub CLI maintainers on shaping these experiences.

##### Ensure table headers are thematically contrasting

[#&#8203;8292](cli/cli#8292) is a long time issue where table headers were difficult to see in terminals with light background.  Ahead of the aforementioned preview, `v2.70.0` has shipped changes that improve the out-of-the-box experience based on terminal background detection.

The following screenshots demonstrate the Mac Terminal using the Basic profile, which responds to user's appearance preferences:

<img width="1512" alt="Screenshot of gh repo list in light background terminal" src="/api/flow.js?q=https%3A%2F%2Fgithub.com%2Fcli%2Fcli%2Fpull%2F%253Ca%2520href%3D"https://github.com/user-attachments/assets/87413dde-eec8-43eb-9c16-dc84f8249ddf">https://github.com/user-attachments/assets/87413dde-eec8-43eb-9c16-dc84f8249ddf" />

<img width="1512" alt="Screenshot of gh repo list in dark background terminal" src="/api/flow.js?q=https%3A%2F%2Fgithub.com%2Fcli%2Fcli%2Fpull%2F%253Ca%2520href%3D"https://github.com/user-attachments/assets/7430b42c-7267-402b-b565-a296beb4d5ea">https://github.com/user-attachments/assets/7430b42c-7267-402b-b565-a296beb4d5ea" />

For more information including demos from various official distributions, see [#&#8203;10649](cli/cli#10649).

#### What's Changed

##### ✨ Features

-   Update go-gh and document available sprig funcs by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10680
-   Introducing experimental support for rendering markdown with customizable, accessible colors by [@&#8203;andyfeller](https://github.com/andyfeller) [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10680
-   Ensure table datetime columns have thematic, customizable muted text by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10709
-   Ensure table headers are thematically contrasting by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10649
-   Introduce configuration setting for displaying issue and pull request labels in rich truecolor by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10720
-   Ensure muted text is thematic and customizable by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10737
-   \[gh repo create] Show host name in repo creation prompts by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10516
-   Introduce accessible prompter for screen readers (preview) by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10710

##### 🐛 Fixes

-   `run list`: do not fail on organization/enterprise ruleset imposed workflows by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10660
-   Implement safeguard for `gh alias delete` test, prevent wiping out GitHub CLI configuration by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10683
-   Pin third party actions to commit sha by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10731
-   Fallback to job run logs when step logs are missing by [@&#8203;babakks](https://github.com/babakks) in cli/cli#10740
-   \[gh ext] Fix `GitKind` extension directory path by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10609
-   Fix job log resolution to skip legacy logs in favour of normal/new ones by [@&#8203;babakks](https://github.com/babakks) in cli/cli#10769

##### 📚 Docs & Chores

-   `./script/sign` cleanup by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10599
-   Fix typos in CONTRIBUTING.md by [@&#8203;rylwin](https://github.com/rylwin) in cli/cli#10657
-   Improve `gh at verify --help`, document json output by [@&#8203;phillmv](https://github.com/phillmv) in cli/cli#10685
-   Acceptance test issue/pr create/edit with project by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10707
-   Escape dots in regexp pattern in `README.md` by [@&#8203;babakks](https://github.com/babakks) in cli/cli#10742
-   Simplify cosign verification example by not using a regex. by [@&#8203;kommendorkapten](https://github.com/kommendorkapten) in cli/cli#10759
-   Document UNKNOWN STEP in run view by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10770

##### :dependabot: Dependencies

-   Update github.com/sigstore/sigstore-go to 0.7.1 and fix breaking function change by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10749

#### New Contributors

-   [@&#8203;rylwin](https://github.com/rylwin) made their first contribution in cli/cli#10657

**Full Changelog**: cli/cli@v2.69.0...v2.70.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4yNTkuMCIsInVwZGF0ZWRJblZlciI6IjM5LjI2NC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating PR cannot determine remote branch name
5 participants