Compare full head label for determining current PR#566
Compare full head label for determining current PR#566rista404 wants to merge 2 commits intocli:masterfrom
Conversation
Previously only the `currentPRHeadRef` was compared to `pr.HeadLabel()` when determining current PR in `status` command. This handles cross-repo "Current PR" detection.
|
It might also be useful, for forked repos, to see the current user if there is no PR, like so: I can add that as well if you want! |
mislav
left a comment
There was a problem hiding this comment.
Thank you for attempting a fix! I think your workaround would work for your specific edge case, but I don't feel like it addresses the root cause.
| if edge.Node.HeadLabel() == currentPRHeadRef { | ||
| currentPRHeadLabel := currentPRHeadRef | ||
| if edge.Node.IsCrossRepository { | ||
| currentPRHeadLabel = fmt.Sprintf("%s:%s", currentUsername, currentPRHeadRef) |
There was a problem hiding this comment.
I don't think this is the right solution. The currentPRHeadRef value was already supposed to be prefixed with OWNER: in case that the branch is pushed to a fork
Lines 384 to 386 in 704ce31
This doesn't happen, and I suspect that it might be because the main command uses determineBaseRepo() to find out the base repository, but prSelectorForCurrentBranch() uses ctx.BaseRepo(), which can (confusingly) return different results.
I have opened #567 to explore an alternate avenue of fixing this; I would appreciate a review!
I took a look at #561, it wouldn't work for any branch name (regardless of a dot) in forked repos. Traced it to here,
pr.HeadLabel()was compared tocurrentPRHeadRefwhich for cross repo PRs would berista404:test-branch == test-branch. Now it's compared to the full label. Tested it in forked and my own repos.Closes #561