Skip to content

/ VisualStudio

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

Jump to bottom

Find merge base when external PR branch has been deleted #1212

Merged
merged 7 commits into from Sep 7, 2017
Merged

Find merge base when external PR branch has been deleted #1212

merged 7 commits into from Sep 7, 2017

Conversation

@jcansdale
Copy link
Collaborator

@jcansdale 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 2 commits Aug 30, 2017
@jcansdale
Fetch via `refs/pull/<PR>/head` in case branch deleted
Loading status checks…
d339a8e 
Deleted branches can only be fetched by owner of repo.
Fetch via special `refs/pull/<PR>/head` refspec to avoid this issue.
@jcansdale
Fetch PR head before base
Loading status checks…
e2ef9f3 
This avoids two fetches in the simple case where head is downstream of base.
@meaghanlewis
Copy link
Contributor

@meaghanlewis 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
Copy link
Collaborator Author

@jcansdale 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 2 commits Sep 5, 2017
@jcansdale
Update tests for new version of GetPullRequestMergeBase
Loading status checks…
5833138 
The new version of this method takes a PR number.
@jcansdale
Merge branch 'master' into fixes/1190-find-merge-base
Loading status checks…
e024a78
@jcansdale
Copy link
Collaborator Author

@jcansdale 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
Copy link
Collaborator Author

@jcansdale 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 2 commits Sep 5, 2017
@jcansdale
Add test and comments about PR base/head fetch order
9ee7fa1 
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
Merge branch 'fixes/1190-find-merge-base' of https://github.com/githu…
Loading status checks…
9804e12 
…b/VisualStudio.git
@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
Merge branch 'master' into fixes/1190-find-merge-base
Loading status checks…
7672f71
@jcansdale jcansdale requested a review from grokys Sep 5, 2017
@grokys
grokys approved these changes Sep 7, 2017
View changes
Copy link
Contributor

@grokys 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
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
Reviewers

@grokys grokys

Assignees
No one assigned
Labels
Ready for Review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@jcansdale @meaghanlewis @grokys
You can’t perform that action at this time.