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 merge base when external PR branch has been deleted #1212

Merged
merged 7 commits into from Sep 7, 2017

Conversation

Projects
None yet
3 participants
@jcansdale
Copy link
Collaborator

jcansdale commented Aug 31, 2017

There was an issue when a PR has been submitted from a fork, the PR had been accepted and the owner deleted the PR branch. GitHub for Visual Studio was unable to find the original (now deleted) branch to show the PR details. This should allow those old PRs to still be displayed.

There are likely other edge cases that I need to think about (hence WIP for the moment).

Fixes #1190

jcansdale added some commits Aug 30, 2017

Fetch via `refs/pull/<PR>/head` in case branch deleted
Deleted branches can only be fetched by owner of repo.
Fetch via special `refs/pull/<PR>/head` refspec to avoid this issue.
Fetch PR head before base
This avoids two fetches in the simple case where head is downstream of base.
@meaghanlewis

This comment has been minimized.

Copy link
Contributor

meaghanlewis commented Aug 31, 2017

I tested this with some PR's that I previously wasn't able to view, and they appear to load now with your fix. 👍

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Sep 1, 2017

@meaghanlewis,

I tested this with some PR's that I previously wasn't able to view, and they appear to load now with your fix.

That is good news. I think this is by far the most common scenario. Someone submits a PR from and fork and deletes the branch once the PR has been accepted.

I'm trying to think what other possible edge cases there might be. In the above case I was able to take advantage of the special refs/pull/<PR>/head branch name. It seems these remain available even after the PR has been closed and its branch deleted.

I'm wondering what would happen if the PR base branch has been deleted (there isn't an equivalent refs/pull/<PR>/base). Obviously this won't happen very often with the default master branch.

Could you try the following:

  1. Create a PR from a fork onto a branch other than master.
  2. Accept and close the PR.
  3. Delete the branch the PR was merged onto.
  • I'm interested to know if you can still view the PR details.
  • There might be a difference between using an account that owns the repo and one that can only view it.
  • Be sure to create a fresh clone of the repo (if the commits already exist, it won't need to fetch from the deleted branch)

@jcansdale jcansdale changed the title [WIP] Find merge bese when external PR branch has been deleted [WIP] Find merge base when external PR branch has been deleted Sep 1, 2017

jcansdale added some commits Sep 5, 2017

Update tests for new version of GetPullRequestMergeBase
The new version of this method takes a PR number.
@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Sep 5, 2017

I'm wondering what would happen if the PR base branch has been deleted (there isn't an equivalent refs/pull//base).

I've done some further testing with this. If the PR base branch has been deleted, we can's simply do a fetch using the base branch name (or SHA). This seems to be the case whether or not the user has permissions to undelete the branch.

As a workaround for this, I'm fetching the head branch using refs/pull/<PR>/head, before checking if the required base commits are available. This means that if the base is directly upstream of the head, there is no need to fetch the head branch (which will very often be the case).

The problematic case would be if commits have been added to the base branch of the PR, before the PR is closed and the base branch deleted (not just the head branch, which would be the usual case).

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Sep 5, 2017

The problematic case would be if commits have been added to the base branch of the PR, before the PR is closed and the base branch deleted (not just the head branch, which would be the usual case).

I've so far failed to produce this problematic case. Maybe it isn't an issue after all? 😕

jcansdale added some commits Sep 5, 2017

Add test and comments about PR base/head fetch order
The PR base branch might no longer exist, so we fetch using `refs/pull/<PR>/head` first.
This will often fetch the base commits, even when the base branch no longer exists.

@jcansdale jcansdale changed the title [WIP] Find merge base when external PR branch has been deleted Find merge base when external PR branch has been deleted Sep 5, 2017

@jcansdale jcansdale requested a review from grokys Sep 5, 2017

@grokys

grokys approved these changes Sep 7, 2017

Copy link
Contributor

grokys left a comment

LGTM! Not done any extensive testing because @meaghanlewis appears to have done that, but the code changes look like they make sense.

@meaghanlewis meaghanlewis merged commit 91e349d into master Sep 7, 2017

5 checks passed

GitHub CLA @jcansdale has accepted the GitHub Contributor License Agreement.
Details
VisualStudio Build #7872388 succeeded in 91s
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins/build_log Jenkins Build Log
Details

@meaghanlewis meaghanlewis deleted the fixes/1190-find-merge-base branch Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.