Skip to content
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

Find PRs using @{push} #9208

Merged
merged 24 commits into from
Jan 29, 2025
Merged

Conversation

Frederick888
Copy link
Contributor

@Frederick888 Frederick888 commented Jun 14, 2024

0179381ef Find PRs using @{push}

When using a push.default = current central workflow [1], we should use
@{push} instead to locate the remote branch.

In fact, @{push} covers most cases in push.default = upstream too. The
branch.<name>.merge is probably only needed when using RemoteURL and
different remote / local branch names.

[1] https://github.com/tpope/vim-fugitive/issues/1172#issuecomment-522301607

7fc35fd47 Only find PRs w/ branch.<name>.merge if push.default = upstream/tracking

When push.default is not 'upstream' / 'tracking' (or 'nothing'), we can
expect local and remote branch names to be the same and solely rely on
@{push} or RemoteURL.

This fixes the wrong error message 'no pull requests found for branch
"<target branch>"' when the local branch is not pushed in the
push.default = simple / current and upstream = <target branch> setup.

4254818db Find push remote using branch.<name>.pushRemote and remote.pushDefault

When using a push.default = current triangular workflow, apart from
using @{push} to determine the remote branch name, we should also follow
the

1. branch.<name>.pushRemote
2. remote.pushDefault
3. branch.<name>.remote

...list to determine which remote Git pushes to.

2d1e4d625 Add base gh pr view acceptance tests for changes

These are acceptance tests based on @williammartin work within various issues associated with #9208.

These need to be enhanced to account for `gh pr status`, however that should be quicker given these pass based on the updated branch and upstream fix for cross-repo same org changes.

eb16a75ab Expand with gh pr status


8bb2879b8 Reflect coverage for view and status subcommands


be250b3d3 Add renamed acceptance tests


0006091d7 Fix up intra-org fork test setup

[1] https://github.com/cli/cli/commit/96ac8d6a2fb268a5825e155ae398e640d3e472fd


This fixes things like gh pr view in a push.default = simple / current + upstream pointing to target branch.

It does not fix the issues with gh pr create.

Related to: #575.

@Frederick888 Frederick888 force-pushed the find-pr-by-rev-parse-push branch 8 times, most recently from cdc1b5d to 4226579 Compare June 18, 2024 00:28
@Frederick888 Frederick888 marked this pull request as ready for review June 18, 2024 00:31
@Frederick888 Frederick888 requested a review from a team as a code owner June 18, 2024 00:31
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Jun 18, 2024
@Frederick888 Frederick888 force-pushed the find-pr-by-rev-parse-push branch from 4226579 to c8a51dd Compare June 25, 2024 12:40
@williammartin
Copy link
Member

Wow this is an oooooold issue. Thank you for your contribution. The details of the original issue look quite involved, so it may take me some time to fully grok and review this. Cheers.

@williammartin williammartin mentioned this pull request Jul 10, 2024
@williammartin
Copy link
Member

Hey @Frederick888, can you provide some more context about the end-to-end workflow here? I believe this relates to triangular workflows where there are 2 remotes, one configured for push and one for fetch but I think I'm missing something in my understanding.

Here's what I tried locally:

Clone my fork and check the remotes:

➜ gh repo clone williammartin/cli
Cloning into 'cli'...
...
! Repository cli/cli set as the default repository. To learn more about the default repository, run: gh repo set-default --help

➜ cd cli && git remote -v
origin  https://github.com/williammartin/cli.git (fetch)
origin  https://github.com/williammartin/cli.git (push)
upstream        https://github.com/cli/cli.git (fetch)
upstream        https://github.com/cli/cli.git (push)

Configure the triangular workflow:

➜ git config remote.pushDefault origin
➜ git config push.default current

Create a PR from my fork using the syntax mentioned by Mislav:

➜ git checkout -b wm/9208-triage && git commit --allow-empty -m "9208-triage" && git push
Switched to a new branch 'wm/9208-triage'
[wm/9208-triage 6b499b8e] 9208-triage
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 189 bytes | 189.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
remote:
remote: Create a pull request for 'wm/9208-triage' on GitHub by visiting:
remote:      https://github.com/williammartin/cli/pull/new/wm/9208-triage
remote:
To https://github.com/williammartin/cli.git
 * [new branch]        wm/9208-triage -> wm/9208-triage

➜ gh pr create -H williammartin:wm/9208-triage

Creating pull request for williammartin:wm/9208-triage into trunk in cli/cli

? Title 9208-triage
? Choose a template PULL_REQUEST_TEMPLATE.md
? Body <Received>
? What's next? Submit
https://github.com/cli/cli/pull/9306

Then I expect using gh pr view to not find the PR (the original issue):

➜ gh pr view
no pull requests found for branch "wm/9208-triage"

But I anticipated that this PR would fix it:

➜ ~/workspace/cli/bin/gh pr view
no pull requests found for branch "wm/9208-triage"

So what am I misunderstanding? Thanks!

@williammartin
Copy link
Member

williammartin commented Jul 10, 2024

Ah, perhaps what I'm missing is that when we create the local branch it needs to be checked out from the upstream?

Deleting the old branch:

➜ git branch -D wm/9208-triage

Creating a new branch from upstream/trunk:

➜ git checkout -b wm/9208-triage upstream/trunk
branch 'wm/9208-triage' set up to track 'upstream/trunk'.
Switched to a new branch 'wm/9208-triage'

Showing the difference between current release and this PR:

➜ gh pr view
no pull requests found for branch "trunk"

➜  ~/workspace/cli/bin/gh pr view | cat
title:  9208-triage
state:  OPEN
author: williammartin
labels:
assignees:      williammartin
reviewers:
projects:       The GitHub CLI (Needs review 🤔)
milestone:
number: 9306
url:    https://github.com/cli/cli/pull/9306
additions:      0
deletions:      0
auto-merge:     disabled
--
# Description

Triaging #9208, you can safely ignore this.

Is this expected behaviour? I'm trying to figure out why it would be important to gh pr view that the branch is created from upstream.

@williammartin
Copy link
Member

I think the issue here is that when the branch starting point doesn't contain a remote, then the config doesn't contain enough information about the upstream repo, which is where the PR actually lives:

➜ git config --get-regexp "^branch.wm/9208-triage.(remote|merge|pushremote)$" | cat
branch.wm/9208-triage.remote upstream
branch.wm/9208-triage.merge refs/heads/trunk

➜ git checkout -b wm/9208-triage-no-upstream
Switched to a new branch 'wm/9208-triage-no-upstream'

➜ git config --get-regexp "^branch.wm/9208-triage-no-upstream.(remote|merge|pushremote)$" | cat
<EMPTY>

Which is an issue for https://github.com/cli/cli/pull/9208/files#diff-ccedca01e076cd065987253b366873ca2bee2e28c9d0bc13d50d7b7795c0bf4eR328

I guess the solution to this would be as Mislav suggested:

We should perhaps iterate over all remotes and assume one to be a push target if we find a same-named tracking branch on one.

But that seems out of scope for this, if I understand your goal correctly.

Do I understand this correctly @Frederick888?

@Frederick888
Copy link
Contributor Author

Frederick888 commented Jul 11, 2024

can you provide some more context about the end-to-end workflow here? I believe this relates to triangular workflows where there are 2 remotes, one configured for push and one for fetch but I think I'm missing something in my understanding.

It's actually related to both central workflow and triangular.

First of all, make sure you've done git config --global push.default current. Then:

Central:

$ git clone https://github.com/Frederick888/playground.git
Cloning into 'playground'...
remote: Enumerating objects: 164, done.
remote: Counting objects: 100% (68/68), done.
remote: Compressing objects: 100% (39/39), done.
remote: Total 164 (delta 40), reused 55 (delta 29), pack-reused 96
Receiving objects: 100% (164/164), 39.82 KiB | 1.99 MiB/s, done.
Resolving deltas: 100% (56/56), done.
$ cd playground
$ git checkout --guess test-diff
branch 'test-diff' set up to track 'origin/test-diff'.
Switched to a new branch 'test-diff'
$ git branch --set-upstream-to origin/master
branch 'test-diff' set up to track 'origin/master'.
$ cat .git/config
[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "origin"]
        url = https://github.com/Frederick888/playground.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
        remote = origin
        merge = refs/heads/master
[branch "test-diff"]
        remote = origin
        merge = refs/heads/master

# with my patch
$ gh pr view
test diff Frederick888/playground#7
Draft • Frederick888 wants to merge 4 commits into master from test-diff • less than a minute ago
+8 -1 • - Checks pending
[ ... ]
View this pull request on GitHub: https://github.com/Frederick888/playground/pull/7

# trunk 0df5596512ad44dec74ab3dc07a4e6ea3a7c78fc
$ ~/Programming/Go/gh-cli/bin/gh pr view
no pull requests found for branch "master"

Triangular:

$ git clone https://github.com/cli/cli.git
Cloning into 'cli'...
remote: Enumerating objects: 54449, done.
remote: Counting objects: 100% (1458/1458), done.
remote: Compressing objects: 100% (817/817), done.
remote: Total 54449 (delta 811), reused 1155 (delta 629), pack-reused 52991
Receiving objects: 100% (54449/54449), 29.40 MiB | 6.75 MiB/s, done.
Resolving deltas: 100% (36967/36967), done.
$ cd cli
$ git remote rename origin upstream
Renaming remote references: 100% (71/71), done.
$ gh repo fork --remote --remote-name origin
! Frederick888/cli already exists
✓ Added remote origin
$ git config branch.trunk.pushremote upstream
$ git config remote.pushdefault origin
$ gh repo set-default cli/cli
✓ Set cli/cli as the default repository for the current directory
$ git fetch --all
$ git checkout --guess find-pr-by-rev-parse-push
branch 'find-pr-by-rev-parse-push' set up to track 'origin/find-pr-by-rev-parse-push'.
Switched to a new branch 'find-pr-by-rev-parse-push'
$ git branch --set-upstream-to upstream/trunk
branch 'find-pr-by-rev-parse-push' set up to track 'upstream/trunk'.
$ cat .git/config
[core]
        repositoryformatversion = 0
        filemode = true
        bare = false
        logallrefupdates = true
[remote "upstream"]
        url = https://github.com/cli/cli.git
        fetch = +refs/heads/*:refs/remotes/upstream/*
        gh-resolved = base
[branch "trunk"]
        remote = upstream
        merge = refs/heads/trunk
        pushremote = upstream
[remote "origin"]
        url = [email protected]:Frederick888/cli.git
        fetch = +refs/heads/*:refs/remotes/origin/*
[remote]
        pushdefault = origin
[branch "find-pr-by-rev-parse-push"]
        remote = upstream
        merge = refs/heads/trunk

# with my patch
$ gh pr view | cat
title:  Find PRs using `@{push}`
state:  OPEN
author: Frederick888
labels: external
assignees:
reviewers:       (Requested), williammartin (Requested)
projects:       The GitHub CLI (Needs review 🤔)
milestone:
number: 9208
url:    https://github.com/cli/cli/pull/9208
additions:      246
deletions:      35
auto-merge:     disabled

# trunk 0df5596512ad44dec74ab3dc07a4e6ea3a7c78fc
$ ~/Programming/Go/gh-cli/bin/gh pr view | cat
no pull requests found for branch "trunk"

When you check out a new branch, there are some similar things you need to do. But the key is, after pushing the new branch, making sure git rev-parse --abbrev-ref <branch>@{push} prints out the correct remote and branch for both target and feature branches.

I've got a super-charged atrocious git co alias to help me do most these.

I guess the solution to this would be as Mislav suggested:

We should perhaps iterate over all remotes and assume one to be a push target if we find a same-named tracking branch on one.

#575 is a bit more than this issue. What Mislav originally mentioned sounds like something gh could do when a user essentially doesn't have a correct setup and always uses the full-formed git push origin feat:feat or something + even git config branch.<branch>.remote = ..

What I'm trying to address here however, is a perfectly valid setup. All the info that we need to determine the target / feature branches are already in gitconfig. Git itself understands the setup already. It's only that gh's not consuming the info right. While I'm not sure what the best approach should be in the scenario Mislav described, there is no need to deploy such last-resort method here in my setup.

But that seems out of scope for this, if I understand your goal correctly.

Do I understand this correctly @Frederick888?

So yeah, you were on the right track 👍

@williammartin
Copy link
Member

@Frederick888, I started extracting issues to describe each of these enhancements in detail e.g. #9363

I'm having trouble understanding the second commit in this series.

It seems like the first commit makes it so that:

  • If @{push} resolves to something correctly, then we use that
  • Otherwise we use the merge config entry

And after the second commit it makes it so that:

  • If @{push} resolves to something correctly, then we use that
  • Only if push.default is empty or upstream then we use the merge config entry
  • Otherwise ??

I don't understand what is expected to happen in this last case. I don't understand what this change is intended to protect. Thanks!

@Frederick888
Copy link
Contributor Author

@williammartin

* Only if `push.default` is empty or `upstream` then we use the `merge` config entry

Actually here it uses MergeRef only if push.default is upstream, not when it's empty (since simple is the default push.default).

* Otherwise ??

I don't understand what is expected to happen in this last case. I don't understand what this change is intended to protect. Thanks!

Otherwise it uses current branch AFAIU

func branchFunc(f *cmdutil.Factory) func() (string, error) {
return func() (string, error) {
currentBranch, err := f.GitClient.CurrentBranch(context.Background())
if err != nil {
return "", fmt.Errorf("could not determine current branch: %w", err)
}
return currentBranch, nil
}
}

Since @{push} can only be rev-parsed if a user has already pushed, even if a user is using push.default = current (or simple by default which expects 'you [...] to configure an upstream branch with the same name'), before they actually push, without the 2nd commit, branchConfig.Push is empty and gh will pick up branchConfig.MergeRef and report 'no pull requests found for branch "master/main/trunk"', while it should've reported 'no pull requests found for branch "<current feat branch>"'.

@Frederick888 Frederick888 force-pushed the find-pr-by-rev-parse-push branch from 16eed48 to 3693b51 Compare July 25, 2024 06:01
@williammartin
Copy link
Member

Ah right, I see. Because simple is the default, we can just assume that the name of the remote branch is the same as the local branch.

I'm trying to imagine whether this could have broken any other push.default configurations:

  • nothing
  • tracking (which is deprecated but does mean the same as upstream)
  • matching (this seems absolutely ridiculous 😅)

I suppose matching might be broken but it seems mad. Not sure whether we care to support deprecated tracking, though I suppose it's an easy change. nothing seems like it would already not work.

Do you agree?

@Frederick888
Copy link
Contributor Author

I'm trying to imagine whether this could have broken any other push.default configurations:

* `nothing`

Such users are probably of those who always use git push origin local-branch:remote-branch (via some GUIs presumably), which is actually more relevant to the original #575. I'd say my PR neither makes this situation better nor worse.

* `tracking` (which is deprecated **but** does mean the same as `upstream`)

Oh yup, good spot! I absolutely should've added it too. b7521e1 and I'll reword the earlier commit too.

* `matching` (this seems absolutely ridiculous 😅)

Yes I doubt many users actually use this locally. But still it expects 'all branches having the same name on both ends'. So it conveniently falls into the 'otherwise' category.

@Frederick888 Frederick888 force-pushed the find-pr-by-rev-parse-push branch 2 times, most recently from f247b33 to ad62216 Compare July 25, 2024 07:41
@Frederick888
Copy link
Contributor Author

@williammartin Done 😄 (Gonna have to head off soon but let me know if you have other concerns.)

@benknoble
Copy link

Something has to understand "this branch is being pushed to pubfork and the PR filed against pub", and I have to configure git to understand that anyway via @{upstream} and @{push}, so having to also explain this to gh as well is a pain.

I don't know what other users do, but for me push.default = current is quite dangerous, so I also set remote.pushDefault = fork, which means I won't accidentally push to upstream.

I also do this! Though I usually lack write access upstream anyway; in my case, it's to make git push DWIM without having to say anything else.

@Frederick888
Copy link
Contributor Author

@jtmcg Going through you comment and reviving my memory :) I have a few (directly or indirectly relevant) questions.


If the git config has push.default = simple, this will not resolve properly for triangular workflows

According to the manpage:

simple - push the current branch with the same name on the remote

It doesn't say that simple cannot work with a triangular workflow. In fact, if we follow the manpage and adjust our config:

[push]
	default = simple
[branch "find-pr-by-rev-parse-push"]
	; remote = upstream
	; merge = refs/heads/trunk
	remote = origin
	merge = refs/heads/find-pr-by-rev-parse-push

...then it resolves normally:

$ g rev-parse --abbrev-ref @{upstream}
origin/find-pr-by-rev-parse-push
$ g rev-parse --abbrev-ref @{push}
origin/find-pr-by-rev-parse-push

gh pr will always respect the return of git rev-parse --abbrev-ref @{push}

+1 on what @benknoble mentioned regarding @{push} + push.default = current when a branch hasn't been pushed yet. (It was the reason why I needed to write the 3rd commit.)

Though @{push} actually resolves when push.default = simple if branch.*.remote and branch.*.merge are configured correctly I think? Did I confuse myself somehow since both your comments seemed to say simple would break @{push}?


Are there any cases where, maybe due to some limitations from Git, people have to use push.default = upstream + different local / remote branch names, that we are going to break?

One example that I have in mind is popular forks like Neovim, MariaDB, etc. They may have a local main pointing to Neovim origin/main and a vim-main pointing to upstream/main. Sometimes they also have different versions of the same patch to submit to both projects respectively. It's not a typical triangular workflow, more like a dual-central one.


Finally, is the refactoring work something that can be done on top of this PR? In another word, is merging this PR going to drastically increase the refactoring effort?

AFAIU the refactoring is mainly going to remove some hacks and quirks, consolidate the code that locates remote branches, then in the end add push.default = current support. So while this PR adds a few LoC, it also brings in quite a few valuable tests.

Not that I'm eager to snatch that 'Contributor' badge, but I have a personal git config patch that I've been rebasing for some half a year alongside this PR. If it's not too much trouble for you folks, I really hope you can help me wrap this up.

FWIW the PR really isn't that big if we exclude the tests:

$ g df --stat trunk...@{push} -- '*.go' ':!*_test.go'
 git/client.go               | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------
 git/objects.go              |  8 ++++++--
 pkg/cmd/pr/shared/finder.go | 18 ++++++++++++------
 pkg/cmd/pr/status/status.go | 15 +++++++++------
 4 files changed, 93 insertions(+), 32 deletions(-)

@williammartin
Copy link
Member

williammartin commented Jan 21, 2025

It doesn't say that simple cannot work with a triangular workflow. In fact, if we follow the manpage and adjust our config:

[push]
	default = simple
[branch "find-pr-by-rev-parse-push"]
	; remote = upstream
	; merge = refs/heads/trunk
	remote = origin
	merge = refs/heads/find-pr-by-rev-parse-push

I must be losing my mind because I cannot see what is triangular about this configuration. The branch name and the merge config ref have the same name, and there is no pushRemote set to make it triangular across different remotes.

@williammartin
Copy link
Member

Do note that @{push} won't resolve until there is actually a push—that's probably fine for gh pr status and similar, but less so for gh pr create if the push hasn't already happened (but maybe the check happens late enough that the push will have already occurred).

This may be an issue.

The pr create code is pretty...intense...so I definitely couldn't speak with confidence on this one. There's some kind of suspicious logic around using git show-ref --verify to determine whether the current branch is already pushed to a remote with a branch of the same name.

Interestingly, the logic to find whether a PR currently exists doesn't use the same Finder branch parsing logic, instead it requires that the branch name match the name of your locally checked out branch.

@Frederick888
Copy link
Contributor Author

I must be losing my mind because I cannot see what is triangular about this configuration. The branch name and the merge config ref have the same name, and there is no pushRemote set to make it triangular across different remotes.

It's the same as how people have been using push.default = upstream -- git rebase -i upstream/trunk after a git fetch to integrate upstream changes. It involves two remotes. Maybe I was not 100% on the terminologies.

But essentially what I wanted to say was despite the fact that simple was just a more restrictive upstream, as long as the manpage didn't explicitly say it couldn't be used in non-central workflows, we shouldn't assume it couldn't be used in non-central workflows.

PS: a more complete push.default = simple example

[push]
	default = simple
[remote "upstream"]
	url = https://github.com/cli/cli.git
	gh-resolved = base
	fetch = +refs/heads/*:refs/remotes/upstream/*
[branch "trunk"]
	remote = upstream
	pushremote = upstream
	merge = refs/heads/trunk
[remote "origin"]
	url = https://github.com/Frederick888/cli.git
	fetch = +refs/heads/*:refs/remotes/origin/*
	pushurl = [email protected]:Frederick888/cli.git
[branch "find-pr-by-rev-parse-push"]
	remote = origin
	merge = refs/heads/find-pr-by-rev-parse-push

I've been struggling horribly to reason through all of this code, and
after much mental gymnastics I identified the culprit as the overloaded
"branch" string returned by parseCurrentBranch.

This value was either the name of the branch that the PR we're looking for
is associated with, or that name prepended with the owner's name and a :
if we're on a branch, so:

PR branch: featureBranch
branch == "featureBranch"

If on Fork belonging to "ForkOwner"
branch == "ForkOwner:featureBranch"

Since this extra information was bundled up into this single string, it
complicated the responsibilities of parseCurrentBranch's "branch" return
value. Thus, I've teased out "branch" into the new PRRefs struct:

type PRRefs struct{
	BranchName string
	HeadRepo ghrepo.Interface
	BaseRepo ghrepo.Interface
}

This allows the new parsePRRefs function to move all the previous
"branch" string's information into structured data, and allows for a new
method on PRRefs, GetPRLabel(), to create the string that "branch"
previously held to pass into its downstream consumer, namely
findForBranch.

This also allowed for better test coverage, directly connecting the PRRefs
fields to the values contained in the git config. Overall, I am now
confident that this is doing what its supposed to do with respect to my
understanding of the various central and triangular git workflows we are
addressing.
@jtmcg
Copy link
Contributor

jtmcg commented Jan 23, 2025

Alright, I've been swimming through y'all's feedback and some more learnings that we've pieced together after doing some work against a different branch and have come back to this one. I've done some refactoring for test coverage and I finally like the direction this is going...

I'm not going to explain much more for now until I finish tracking down a few bugs, but I've added a lot of info to my commit message that can give some context/motivation for what this change does.

Hopefully this is easier to grok and discuss, so as always, if you find something that jumps out to you please highlight it so I can tackle it.

Y'all have been awesome through what can only be called this quest, and I'm feeling like we're close 🙂

jtmcg added 3 commits January 24, 2025 11:05
There was a lot of copy-pasta code between the finder and pr status. After
some investigation it was clear that the prSelectorForCurrentBranch code
was really just a duplicate of the finder's code without actually making
the API call for the PR. Since the ParsePRRefs helper had already
extracted much of the logic for determining a PR's head ref branch, I was
able to reuse it in gh pr status with a small refactor.
Due to the refactor of BranchConfig, the tests for `pr create` no longer
require a handful of stubbed git commands.

Additionally, I noticed some overlap between `pr create`'s existing logic
with the new finder code. I suspect that the finder's new ParsePRRefs
helper could be leveraged here as well, which would allow us to respect
non-centralized workflows pr create in the same way we are respecting them
in `gh pr view` and `gh pr status`
Comment on lines +341 to +365
if branchConfig.PushRemoteName != "" {
if r, err := rems.FindByName(branchConfig.PushRemoteName); err == nil {
prRefs.HeadRepo = r.Repo
}
} else if branchConfig.PushRemoteURL != nil {
if r, err := ghrepo.FromURL(branchConfig.PushRemoteURL); err == nil {
prRefs.HeadRepo = r
}
} else if remotePushDefault != "" {
if r, err := rems.FindByName(remotePushDefault); err == nil {
prRefs.HeadRepo = r.Repo
}
} else if branchConfig.RemoteName != "" {
// the branch merges from a remote specified by name
rem, _ := f.remotesFn()
if r, err := rem.FindByName(branchConfig.RemoteName); err == nil {
gitRemoteRepo = r
if r, err := rems.FindByName(branchConfig.RemoteName); err == nil {
prRefs.HeadRepo = r.Repo
}
} else if branchConfig.RemoteURL != nil {
if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil {
prRefs.HeadRepo = r
}
}

if gitRemoteRepo != nil {
if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") {
prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/")
}
// prepend `OWNER:` if this branch is pushed to a fork
// This is determined by:
// - The repo having a different owner
// - The repo having the same owner but a different name (private org fork)
// I suspect that the implementation of the second case may be broken in the face
// of a repo rename, where the remote hasn't been updated locally. This is a
// frequent issue in commands that use SmartBaseRepoFunc. It's not any worse than not
// supporting this case at all though.
sameOwner := strings.EqualFold(gitRemoteRepo.RepoOwner(), f.repo.RepoOwner())
sameOwnerDifferentRepoName := sameOwner && !strings.EqualFold(gitRemoteRepo.RepoName(), f.repo.RepoName())
if !sameOwner || sameOwnerDifferentRepoName {
prHeadRef = fmt.Sprintf("%s:%s", gitRemoteRepo.RepoOwner(), prHeadRef)
}
// The PR merges from a branch in the same repo as the base branch (usually the default branch)
if prRefs.HeadRepo == nil {
prRefs.HeadRepo = baseRefRepo
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking for feedback on how to make this better... This is, at least, easy to understand this way 😅

@jtmcg
Copy link
Contributor

jtmcg commented Jan 24, 2025

Alright all, tests are 🟢! This blew up a bit, but I think it's trivialized the whole pr create thing, now. I've asked the rest of the team for review, but I this should be ready for the next release. Thanks again, y'all!

git/objects.go Outdated Show resolved Hide resolved
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

Whew! I'll admit that this is a pretty big PR to review (~2 hours).

Aside from the automated tests passing, @jtmcg, can you confirm that the acceptance tests on this PR are also passing?

git/client.go Outdated Show resolved Hide resolved
git/client.go Outdated Show resolved Hide resolved
git/client.go Outdated Show resolved Hide resolved
git/client.go Outdated Show resolved Hide resolved
git/client.go Outdated Show resolved Hide resolved
pkg/cmd/pr/shared/finder.go Outdated Show resolved Hide resolved
pkg/cmd/pr/shared/finder_test.go Show resolved Hide resolved
pkg/cmd/pr/shared/finder_test.go Outdated Show resolved Hide resolved
pkg/cmd/pr/status/status.go Show resolved Hide resolved
pkg/cmd/pr/shared/finder.go Outdated Show resolved Hide resolved
@BagToad
Copy link
Member

BagToad commented Jan 27, 2025

@jtmcg Do you think this should ship with some user-facing docs changes to describe the new behavior somewhere? Perhaps a help topic and a mention about the help topic in the relevant pr subcommands?

Probably should be a follow-up issue & PR because this PR is already so big, but I wanted to mention it somewhere.

@jtmcg
Copy link
Contributor

jtmcg commented Jan 27, 2025

Whew! I'll admit that this is a pretty big PR to review (~2 hours).

Aside from the automated tests passing, @jtmcg, can you confirm that the acceptance tests on this PR are also passing?

Yup, can confirm that I got them running today 👍

@jtmcg
Copy link
Contributor

jtmcg commented Jan 28, 2025

@jtmcg Do you think this should ship with some user-facing docs changes to describe the new behavior somewhere? Perhaps a help topic and a mention about the help topic in the relevant pr subcommands?

Probably should be a follow-up issue & PR because this PR is already so big, but I wanted to mention it somewhere.

Yeah, that's a good idea, @BagToad. I definitely think we should call this out in the changelog, but also think a writeup would be helpful. Definitely a follow-up issue

Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

After reviewing all of the changes, concerns, and questions from the previous review, I don't know what else needs to be addressed in order to ship this.

Solid work and thank you for diligence getting this shaped up, @jtmcg! ✨

git/client_test.go Outdated Show resolved Hide resolved
git/objects.go Show resolved Hide resolved
pkg/cmd/pr/shared/finder_test.go Show resolved Hide resolved
git/client.go Outdated Show resolved Hide resolved
git/client_test.go Show resolved Hide resolved
pkg/cmd/pr/shared/finder.go Show resolved Hide resolved
pkg/cmd/pr/shared/finder.go Outdated Show resolved Hide resolved
pkg/cmd/pr/shared/finder_test.go Show resolved Hide resolved
@jtmcg jtmcg merged commit de9971d into cli:trunk Jan 29, 2025
9 checks passed
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Feb 4, 2025
This MR contains the following updates:

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

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.66.1`](https://github.com/cli/cli/releases/tag/v2.66.1): GitHub CLI 2.66.1

[Compare Source](cli/cli@v2.66.0...v2.66.1)

#### Hotfix: `gh pr view` fails with provided URL

This addresses a regression in `gh pr view` was reported in [#&#8203;10352](cli/cli#10352). This regression was due to a change in `v2.66.0` that no longer allowed `gh pr` subcommands to execute properly outside of a git repo.

#### What's Changed

-   Hotfix: `gh pr view` fails with provided URL by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10354

**Full Changelog**: cli/cli@v2.66.0...v2.66.1

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

[Compare Source](cli/cli@v2.65.0...v2.66.0)

#### `gh pr view` and `gh pr status` now respect common triangular workflow configurations

Previously, `gh pr view` and `gh pr status` would fail for pull request's (MR) open in triangular workflows. This was due to `gh` being unable to identify the MR's corresponding remote and branch refs on GitHub.

Now, `gh pr view` and `gh pr status` should successfully identify the MR's refs when the following common git configurations are used:

-   [`branch.<branchName>.pushremote`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtpushRemote) is set
-   [`remote.pushDefault`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-remotepushDefault) is set

Branch specific configuration, the former, supersedes repo specific configuration, the latter.

Additionally, if the [`@{push}` revision syntax](https://git-scm.com/docs/gitrevisions#Documentation/gitrevisions.txt-emltbranchnamegtpushemegemmasterpushemempushem) for git resolves for a branch, `gh pr view` and `gh pr status` should work regardless of additional config settings.

For more information, see

-   cli/cli#9363
-   cli/cli#9364
-   cli/cli#9365
-   cli/cli#9374

#### `gh secret list`, `gh secret set`, and `gh secret delete` now require repository selection when multiple `git` remotes are present

Previously, `gh secret list`, `gh secret set`, and `gh secret delete` would determine which remote to target for interacting with GitHub Actions secrets.  Remotes marked as default using `gh repo set-default` or through other `gh` commands had higher priority when figuring out which repository to interact with.  This could have unexpected outcomes when using `gh secret` commands with forked repositories as the upstream repository would generally be selected.

Now, `gh secret` commands require users to disambiguate which repository should be the target if multiple remotes are present and the `-R, --repo` flag is not provided.

For more information, see cli/cli#4688

#### Extension update notices now notify once every 24 hours per extension and can be disabled

Previously, the GitHub CLI would notify users about newer versions every time an extension was executed.  This did not match GitHub CLI notices, which only notified users once every 24 hours and could be disabled through an environment variable.

Now, extension update notices will behave similar to GitHub CLI notices.  To disable extension update notices, set the `GH_NO_EXTENSION_UPDATE_NOTIFIER` environment variable.

For more information, see cli/cli#9925

#### What's Changed

##### ✨ Features

-   Draft for discussing testing around extension update checking behavior by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9985
-   Make extension update check non-blocking by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10239
-   Ensure extension update notices only notify once within 24 hours, provide ability to disable all extension update notices by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#9934
-   feat: make the extension upgrade fancier by [@&#8203;nobe4](https://github.com/nobe4) in cli/cli#10194
-   fix: padded display by [@&#8203;nobe4](https://github.com/nobe4) in cli/cli#10216
-   Update `gh attestation` attestation bundle fetching logic by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10185
-   Require repo disambiguation for secret commands by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10209
-   show error message for rerun workflow older than a month ago by [@&#8203;iamrajhans](https://github.com/iamrajhans) in cli/cli#10227
-   Update `gh attestation verify` table output by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10104
-   Enable MSI building for Windows arm64 by [@&#8203;dennisameling](https://github.com/dennisameling) in cli/cli#10297
-   feat: Add support for creating autolink references by [@&#8203;hoffm](https://github.com/hoffm) in cli/cli#10180
-   Find MRs using `@{push}` by [@&#8203;Frederick888](https://github.com/Frederick888) in cli/cli#9208
-   feat: Add support for viewing autolink references by [@&#8203;hoffm](https://github.com/hoffm) in cli/cli#10324
-   Update `gh attestation` bundle fetching logic by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10339

##### 🐛 Fixes

-   gh gist delete: prompt for gist id by [@&#8203;danochoa](https://github.com/danochoa) in cli/cli#10154
-   Better handling for waiting for codespaces to become ready by [@&#8203;cmbrose](https://github.com/cmbrose) in cli/cli#10198
-   Fix: `gh gist view` and `gh gist edit` prompts with no TTY by [@&#8203;mateusmarquezini](https://github.com/mateusmarquezini) in cli/cli#10048
-   Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10197
-   Add job to deployment workflow to validate the tag name for a given release by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10121
-   \[gh run list] Stop progress indicator on failure from `--workflow` flag by [@&#8203;iamazeem](https://github.com/iamazeem) in cli/cli#10323
-   Update deployment.yml by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10340

##### 📚 Docs & Chores

-   Add affected version heading to bug report issue form by [@&#8203;BagToad](https://github.com/BagToad) in cli/cli#10269
-   chore: fix some comments by [@&#8203;petercover](https://github.com/petercover) in cli/cli#10296
-   Update triage.md to reflect FR experiment outcome by [@&#8203;jtmcg](https://github.com/jtmcg) in cli/cli#10196
-   Clear up --with-token fine grained PAT usage by [@&#8203;williammartin](https://github.com/williammartin) in cli/cli#10186
-   Correct help documentation around template use in `gh issue create` by [@&#8203;andyfeller](https://github.com/andyfeller) in cli/cli#10208
-   chore: fix some function names in comment by [@&#8203;zhuhaicity](https://github.com/zhuhaicity) in cli/cli#10225
-   Tiny typo fix by [@&#8203;robmorgan](https://github.com/robmorgan) in cli/cli#10265
-   add install instructions for Manjaro Linux by [@&#8203;AMS21](https://github.com/AMS21) in cli/cli#10236
-   Update test to be compatible with latest Glamour v0.8.0 by [@&#8203;ottok](https://github.com/ottok) in cli/cli#10151
-   Add more `gh attestation verify` integration tests by [@&#8203;malancas](https://github.com/malancas) in cli/cli#10102

##### :dependabot: Dependencies

-   Bump github.com/mattn/go-colorable from 0.1.13 to 0.1.14 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10215
-   Bump github.com/sigstore/protobuf-specs from 0.3.2 to 0.3.3 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10214
-   Bump github.com/gabriel-vasile/mimetype from 1.4.7 to 1.4.8 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10184
-   Bump google.golang.org/protobuf from 1.36.2 to 1.36.3 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10250
-   Bump golangci-linter and address failures to prepare for Go 1.24 strictness by [@&#8203;mikelolasagasti](https://github.com/mikelolasagasti) in cli/cli#10279
-   Bump github.com/google/go-containerregistry from 0.20.2 to 0.20.3 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10257
-   Bump actions/attest-build-provenance from 2.1.0 to 2.2.0 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10300
-   Bump google.golang.org/protobuf from 1.36.3 to 1.36.4 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10306
-   Upgrade sigstore-go to v0.7.0: fixes [#&#8203;10114](cli/cli#10114) formatting issue by [@&#8203;codysoyland](https://github.com/codysoyland) in cli/cli#10309
-   Bump github.com/in-toto/attestation from 1.1.0 to 1.1.1 by [@&#8203;dependabot](https://github.com/dependabot) in cli/cli#10319

#### New Contributors

Big thank you to our many new *and* longtime contributors making this release happen!! ❤️ ✨

-   [@&#8203;zhuhaicity](https://github.com/zhuhaicity) made their first contribution in cli/cli#10225
-   [@&#8203;danochoa](https://github.com/danochoa) made their first contribution in cli/cli#10154
-   [@&#8203;robmorgan](https://github.com/robmorgan) made their first contribution in cli/cli#10265
-   [@&#8203;iamrajhans](https://github.com/iamrajhans) made their first contribution in cli/cli#10227
-   [@&#8203;AMS21](https://github.com/AMS21) made their first contribution in cli/cli#10236
-   [@&#8203;petercover](https://github.com/petercover) made their first contribution in cli/cli#10296
-   [@&#8203;ottok](https://github.com/ottok) made their first contribution in cli/cli#10151
-   [@&#8203;dennisameling](https://github.com/dennisameling) made their first contribution in cli/cli#10297
-   [@&#8203;iamazeem](https://github.com/iamazeem) made their first contribution in cli/cli#10323
-   [@&#8203;Frederick888](https://github.com/Frederick888) made their first contribution in cli/cli#9208

**Full Changelog**: cli/cli@v2.65.0...v2.66.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:eyJjcmVhdGVkSW5WZXIiOiIzOS4xNDMuMCIsInVwZGF0ZWRJblZlciI6IjM5LjE0Ni40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
Status: Pending Release 🥚
Development

Successfully merging this pull request may close these issues.

9 participants