Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Allow selection of fork/parent pull requests. #1021

Merged
merged 27 commits into from Aug 1, 2017

Conversation

grokys
Copy link
Contributor

@grokys grokys commented Jun 28, 2017

When working on a fork, previously the PR list would show PRs for that fork, which is often not what is wanted. This PR adds a selector dropdown to select the repository from which to display the pull requests, defaulting to the parent repository if the repository is a fork. If the repository is not a fork, the selector will be hidden.

image

This exposes a similar problem to #734 in that there quickly ends up being no room along the top row of filters for the repository name, meaning it wraps to a new line, causing ugly. We need to think of a new way to display filters for this and #734. cc: @donokuda

Depends on #1004, #1023
Fixes #863

Previously if the user was working on a fork, the PRs for the fork would be shown rather than the PRs for the parent repository. Change the default to show PRs for the parent repository with a (very ugly!) checkbox to switch to showing the PRs for the fork.
Recreate the `PullRequests` `TrackingCollection` when switching to/from showing PRs for a fork. Also use the repository owner name in `ModelService.GetPullRequests` instead of the current username.
Take into account whether the PR is from a fork or not in `PullRequestDetailViewModel`.
Previously we simply did a check to see if `Head.Owner` != `Base.Owner` but that didn't work when we were _in_ a fork and looking at a PR in the parent. In that case both head and base are in a different repository.
This is better than the ugly checkbox there was before, but it's easy for it to get too long and wrap, which looks bad.
grokys added a commit that referenced this pull request Jun 29, 2017
In #1021 we require the repository parent (in the case of forked repositories) - this data is returned from the API but wasn't getting stored in our cache, meaning that we have invalid data when data is returned from the cache as opposed to from the API.

This adds a `cacheVersion` to host caches. This version is checked when the host cache is first loaded, and if it doesn't match the expected version, the cache is invalidated. In this way each time we make a change as required by #1021 we can bump `HostCacheFactory.CacheVersion` to ensure the cache won't be returning invalid data.
`Repository.Owner` is now being used by `ModelService.GetPullRequest`.
@grokys grokys force-pushed the fixes/863-fork-pull-requests branch from 47ae7b4 to 0102285 Compare June 30, 2017 09:34
@tierninho
Copy link

@grokys Tried testing this PR, but the PR page in the github window just hangs, with "Unable to complete read operation after process has exited" in the Output window. Not sure how to get around this.

screen shot 2017-07-11 at 10 08 58 am

Using the same one with different contents means that one cannot downgrade the assembly as the older version throws on the new format.
…-requests

 Conflicts:
	src/GitHub.InlineReviews/Services/PullRequestSession.cs
@grokys
Copy link
Contributor Author

grokys commented Jul 11, 2017

@tierninho could you try now?

@tierninho
Copy link

@grokys same result, still hangs.

@jcansdale
Copy link
Collaborator

Strange. This PR has been working well for me. 😕

@tierninho
Copy link

@grokys and I got this working on my machine after tweaking my setup 👍

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

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

Works on my machine! 😉

grokys and others added 2 commits July 14, 2017 11:03
 Conflicts:
	src/GitHub.App/ViewModels/PullRequestDetailViewModel.cs
This will ensure that required commits/blobs are available.
When merge-base can't be found, fetch the PR base and head from the correct repo URLs.
@jcansdale jcansdale force-pushed the fixes/863-fork-pull-requests branch from 419341b to 1fbc6f7 Compare July 14, 2017 16:03
{
var mergeBase = GetMergeBase(repo, baseSha, headSha);
if (mergeBase == null)
{
var pullHeadRef = $"refs/pull/{pullNumber}/head";
await Fetch(repo, remoteName, baseRef, pullHeadRef);
// TODO: Optimize and error check.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to fix this.

Build was failing because this was excluded.
We no longer know whether the local repo will be on base or head end of the PR.
Fetch using repo URL and PR base/head ref.
@@ -394,6 +401,20 @@ public Task<bool> IsHeadPushed(IRepository repo)
});
}

Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs)
{
var tempRemoteName = $"{cloneUrl.Owner}-{Guid.NewGuid()}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should only create temporary remotes when necessary (when cloneUrl != origin).

jcansdale and others added 6 commits July 18, 2017 14:30
There is a good change the PR base or head comes from `origin`. Use this remote when possible.
…-requests

 Conflicts:
	src/GitHub.App/Models/RemoteRepositoryModel.cs
	src/GitHub.App/Services/GitClient.cs
	src/GitHub.App/ViewModels/PullRequestListViewModel.cs
This was removed by a bad merge in 287ca67
Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

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

Didn't actually mean to block this.

@github github deleted a comment from jcansdale Jul 31, 2017
@grokys grokys merged commit ad6b434 into master Aug 1, 2017
@grokys grokys deleted the fixes/863-fork-pull-requests branch August 1, 2017 13:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants