Skip to content

Fix detecting PR for current branch pushed to fork#567

Merged
mislav merged 1 commit intomasterfrom
pr-selector-base
Mar 3, 2020
Merged

Fix detecting PR for current branch pushed to fork#567
mislav merged 1 commit intomasterfrom
pr-selector-base

Conversation

@mislav
Copy link
Contributor

@mislav mislav commented Mar 2, 2020 •
edited
Loading

Now passing base repo as reference to prSelectorForCurrentBranch() so that it doesn't have to (inexactly) determine the base repo itself and risk it being different than the base repo determined during pr status/view main command logic.

Fixes #561, closes #566

@mislav
Fix detecting PR for current branch pushed to fork
201217d 
Now passing base repo as reference to `prSelectorForCurrentBranch()` so
that it doesn't have to (inexactly) determine the base repo itself and
risk it being different than the base repo determined during `pr
status/view`.
@mislav mislav mentioned this pull request Mar 2, 2020
Closed
@rista404
Copy link
Contributor

rista404 commented Mar 2, 2020

@mislav this doesn't work for my test forked repo, the currentPRHeadRef doesn't have the OWNER bit.

Actually in my testing the other day, I've found that git.ReadBranchConfig() would return empty RemoteURL and RemoteName, because running git config --get-regexp '^branch.test-branch.*$' would return nothing (pattern '^branch.*.*$' wouldn't include that branch). It would only work after doing:

git remote add upstream git://github.com/username/base-repo.git
git branch -u upstream/test-branch

Since I'm not an expert in working with forked repos, I just assumed this was expected. Because RemoteURL and RemoteName were both empty, prSelectorForCurrentBranch wouldn't define branchOwner and that // prepend OWNER: would never run.

Hope this helps!

@mislav
Copy link
Contributor Author

mislav commented Mar 3, 2020

@rista404 Ah, so it didn't work when your branch didn't have upstream configuration, but it started working after you added upstream configuration with git branch -u? Thank you for reporting! That's good to know, and smells like another bug separate from the one being fixed in this PR 🙇

@mislav mislav merged commit a73880f into master Mar 3, 2020
@mislav mislav deleted the pr-selector-base branch March 3, 2020 21:04
@mislav mislav mentioned this pull request Mar 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@vilmibm vilmibm vilmibm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CLI claims no pr associated with current branch even when one is clearly listed

3 participants

@mislav @rista404 @vilmibm