Skip to content

Improve detecting PR for the current branch#96

Merged
mislav merged 5 commits intomasterfrom
pr-current-branch
Dec 2, 2019
Merged

Improve detecting PR for the current branch#96
mislav merged 5 commits intomasterfrom
pr-current-branch

Conversation

@mislav
Copy link
Copy Markdown
Contributor

@mislav mislav commented Nov 20, 2019

When on the e.g. "development" branch, the CLI could erroneously associate it with a PR that also has "development" as its head, but comes from a fork. https://github.slack.com/archives/CLLG3RMAR/p1574107160421000

This implements a stricter current branch match that eliminates false positives.

Now reads git branch configuration and handles these cases:

  • Upstream branch is differently named:

    branch ["foo"]
      remote origin
      merge  refs/heads/bar
    
  • Upstream branch tracks a non-default remote:

    branch ["foo"]
      remote other-remote
      merge  refs/heads/foo
    
  • Upstream configuration is with remote URL instead of name:

    branch ["foo"]
      remote https://github.com/OWNER/REPO.git
      merge  refs/heads/bar
    
  • Upstream configuration pulls from special PR head refspec:

    branch ["foo"]
      remote origin
      merge  refs/pull/123/head
    

Ultimately, all this ensures that pr view can be used without arguments on any branch created with pr checkout and that the correct PR will be opened.

Commands to extend this behavior to (TODO):

  • pr view
  • pr status

When on a `patch-1` branch locally, `gh pr view` would happily open the
first open PR it finds with "patch-1" as its head, even those coming
from forks.
Now reads git branch configuration and handles these cases:

    branch ["foo"]
      remote origin
      merge  refs/heads/bar

    branch ["foo"]
      remote other-remote
      merge  refs/heads/foo

    branch ["foo"]
      remote https://github.com/OWNER/REPO.git
      merge  refs/heads/bar

    branch ["foo"]
      remote origin
      merge  refs/pull/123/head
Copy link
Copy Markdown
Contributor

@probablycorey probablycorey left a comment

Choose a reason for hiding this comment

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

👍 for the tests. The code looks great, but I think a little more documentation would help us maintain it in the future.

}
}

pr, err := api.PullRequestForBranch(apiClient, baseRepo, branchWithOwner)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good but it's a pretty dense. Maybe pull it out into a few functions, or variables like prNumberInRef := prHeadRE.FindStringSubmatch(branchConfig.MergeRef). I'm pretty stingy with comments, but this might be the type of code where some make sense.

@mislav mislav merged commit a73ba78 into master Dec 2, 2019
@mislav mislav deleted the pr-current-branch branch December 2, 2019 19:46
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.

2 participants