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
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0d59eef
Optionally show PRs for forked repo.
grokys Jun 26, 2017
bb42210
Recreate collection when switching to/from fork.
grokys Jun 26, 2017
dd8d37f
Display correct PR in PR details view.
grokys Jun 26, 2017
4890567
Change how we determine if PR is from fork.
grokys Jun 27, 2017
a39ffe3
Use correct fork for inline comments.
grokys Jun 28, 2017
d242bd1
Make fork selection a dropdown link.
grokys Jun 28, 2017
0102285
Fix failing tests.
grokys Jun 30, 2017
8cbeacd
Merge branch 'master' into fixes/863-fork-pull-requests
grokys Jul 7, 2017
19663bc
Merge branch 'feature/cache-versioning' into fixes/863-fork-pull-requ…
grokys Jul 11, 2017
64ad14b
Use new config key for PR owner/number.
grokys Jul 11, 2017
7d31743
Harden parsing of PR owner/number.
grokys Jul 11, 2017
00fdbd5
Merge remote-tracking branch 'origin/master' into fixes/863-fork-pull…
grokys Jul 11, 2017
63fe9b4
Fixed failing tests.
grokys Jul 11, 2017
ddcae61
Merge branch 'master' into fixes/863-fork-pull-requests
grokys Jul 12, 2017
287ca67
Merge branch 'master' into fixes/863-fork-pull-requests
grokys Jul 14, 2017
fd4f72c
GetPullRequestMergeBase before GetFileContent
jcansdale Jul 14, 2017
1fbc6f7
Make GetPullRequestMergeBase fetch from correct repo URLs
jcansdale Jul 14, 2017
3d0eab0
Made GitClientTests tests pass
jcansdale Jul 14, 2017
c563a47
Include DesignTimeStyleHelper in solution build
jcansdale Jul 14, 2017
fee9231
Remove dependency on `refs/pull/PR/head` remote refs
jcansdale Jul 17, 2017
e56cfa2
Always fetch PR base/head using https
jcansdale Jul 18, 2017
903ef04
Use `origin` remote when possible
jcansdale Jul 18, 2017
7d04482
Add xmldoc comment for fetch from URI method
jcansdale Jul 18, 2017
4ff299a
Merge remote-tracking branch 'origin/master' into fixes/863-fork-pull…
grokys Jul 31, 2017
4aec576
Set RemoteRepository in Load.
grokys Jul 31, 2017
b7f3803
Merge branch 'master' into fixes/863-fork-pull-requests
grokys Jul 31, 2017
82bf5ce
Merge branch 'master' into fixes/863-fork-pull-requests
grokys Aug 1, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/GitHub.App/GitHub.App.csproj
Expand Up @@ -134,6 +134,7 @@
<ItemGroup>
<Compile Include="Models\IssueCommentModel.cs" />
<Compile Include="Models\PullRequestReviewCommentModel.cs" />
<Compile Include="Models\PullRequestDetailArgument.cs" />
<Compile Include="ViewModels\ViewModelBase.cs" />
<None Include="..\..\script\Key.snk" Condition="$(Buildtype) == 'Internal'">
<Link>Key.snk</Link>
Expand Down
21 changes: 21 additions & 0 deletions src/GitHub.App/Models/PullRequestDetailArgument.cs
@@ -0,0 +1,21 @@
using System;
using GitHub.ViewModels;

namespace GitHub.Models
{
/// <summary>
/// Passes arguments to a <see cref="PullRequestDetailViewModel"/>
/// </summary>
public class PullRequestDetailArgument
{
/// <summary>
/// Gets or sets the repository containing the pull request.
/// </summary>
public IRemoteRepositoryModel Repository { get; set; }

/// <summary>
/// Gets or sets the number of the pull request.
/// </summary>
public int Number { get; set; }
}
}
3 changes: 2 additions & 1 deletion src/GitHub.App/Models/RemoteRepositoryModel.cs
Expand Up @@ -21,7 +21,7 @@ public class RemoteRepositoryModel : RepositoryModel, IRemoteRepositoryModel,
/// <param name="isPrivate">Whether the repository is private.</param>
/// <param name="isFork">Whether the repository is a fork.</param>
/// <param name="ownerAccount">The repository owner account.</param>
public RemoteRepositoryModel(long id, string name, UriString cloneUrl, bool isPrivate, bool isFork, IAccount ownerAccount)
public RemoteRepositoryModel(long id, string name, UriString cloneUrl, bool isPrivate, bool isFork, IAccount ownerAccount, [AllowNull] IRemoteRepositoryModel parent)
: base(name, cloneUrl)
{
Id = id;
Expand All @@ -31,6 +31,7 @@ public RemoteRepositoryModel(long id, string name, UriString cloneUrl, bool isPr
// this is an assumption, we'd have to load the repo information from octokit to know for sure
// probably not worth it for this ctor
DefaultBranch = new BranchModel("master", this);
Parent = parent;
}

/// <summary>
Expand Down
Expand Up @@ -73,7 +73,8 @@ public PullRequestDetailViewModelDesigner()

public IPullRequestModel Model { get; }
public IPullRequestSession Session { get; }
public ILocalRepositoryModel Repository { get; }
public ILocalRepositoryModel LocalRepository { get; }
public IRemoteRepositoryModel RemoteRepository { get; }
public string SourceBranchDisplayName { get; set; }
public string TargetBranchDisplayName { get; set; }
public int CommentCount { get; set; }
Expand Down
5 changes: 5 additions & 0 deletions src/GitHub.App/SampleData/PullRequestListViewModelDesigner.cs
Expand Up @@ -55,6 +55,9 @@ public PullRequestListViewModelDesigner()
SelectedAuthor = Authors.ElementAt(1);
}

public IReadOnlyList<IRemoteRepositoryModel> Repositories { get; }
public IRemoteRepositoryModel SelectedRepository { get; set; }

public ITrackingCollection<IPullRequestModel> PullRequests { get; set; }
public IPullRequestModel SelectedPullRequest { get; set; }

Expand All @@ -63,6 +66,8 @@ public PullRequestListViewModelDesigner()

public ObservableCollection<IAccount> Authors { get; set; }
public IAccount SelectedAuthor { get; set; }
public bool RepositoryIsFork { get; set; } = true;
public bool ShowPullRequestsForFork { get; set; }

public ObservableCollection<IAccount> Assignees { get; set; }
public IAccount SelectedAssignee { get; set; }
Expand Down
2 changes: 1 addition & 1 deletion src/GitHub.App/SampleData/SampleViewModels.cs
Expand Up @@ -321,7 +321,7 @@ public static IRemoteRepositoryModel Create(string name = null, string owner = n
{
name = name ?? "octocat";
owner = owner ?? "github";
return new RemoteRepositoryModel(0, name, new UriString("http://github.com/" + name + "/" + owner), false, false, new AccountDesigner() { Login = owner });
return new RemoteRepositoryModel(0, name, new UriString("http://github.com/" + name + "/" + owner), false, false, new AccountDesigner() { Login = owner }, null);
}
}

Expand Down
22 changes: 19 additions & 3 deletions src/GitHub.App/Services/GitClient.cs
Expand Up @@ -359,20 +359,36 @@ public Task<bool> IsModified(IRepository repository, string path, [AllowNull] by
});
}

public async Task<string> GetPullRequestMergeBase(IRepository repo, string remoteName, string baseSha, string headSha, string baseRef, int pullNumber)
public async Task<string> GetPullRequestMergeBase(IRepository repo,
UriString baseCloneUrl, UriString headCloneUrl, string baseSha, string headSha, string baseRef, int pullNumber)
{
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.

await Fetch(repo, baseCloneUrl, baseRef);
await Fetch(repo, headCloneUrl, $"refs/pull/{pullNumber}/head");

mergeBase = GetMergeBase(repo, baseSha, headSha);
}

return mergeBase;
}

async Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs)
{
var tempRemoteName = $"{cloneUrl.Host}-{Guid.NewGuid()}";
var remote = repo.Network.Remotes.Add(tempRemoteName, cloneUrl.ToRepositoryUrl().ToString());
try
{
await Fetch(repo, remote.Name, refspecs);
}
finally
{
repo.Network.Remotes.Remove(tempRemoteName);
}
}

static string GetMergeBase(IRepository repo, string a, string b)
{
var aCommit = repo.Lookup<Commit>(a);
Expand Down
38 changes: 30 additions & 8 deletions src/GitHub.App/Services/ModelService.cs
Expand Up @@ -154,7 +154,7 @@ IObservable<AccountCacheItem> GetUserFromCache()
/// <param name="repo"></param>
/// <param name="collection"></param>
/// <returns></returns>
public ITrackingCollection<IPullRequestModel> GetPullRequests(ILocalRepositoryModel repo,
public ITrackingCollection<IPullRequestModel> GetPullRequests(IRepositoryModel repo,
ITrackingCollection<IPullRequestModel> collection)
{
// Since the api to list pull requests returns all the data for each pr, cache each pr in its own entry
Expand All @@ -164,7 +164,7 @@ IObservable<AccountCacheItem> GetUserFromCache()
// and replaces it instead of appending, so items get refreshed in-place as they come in.

var keyobs = GetUserFromCache()
.Select(user => string.Format(CultureInfo.InvariantCulture, "{0}|{1}:{2}", CacheIndex.PRPrefix, user.Login, repo.Name));
.Select(user => string.Format(CultureInfo.InvariantCulture, "{0}|{1}:{2}", CacheIndex.PRPrefix, repo.Owner, repo.Name));

var source = Observable.Defer(() => keyobs
.SelectMany(key =>
Expand All @@ -189,16 +189,16 @@ IObservable<AccountCacheItem> GetUserFromCache()
return collection;
}

public IObservable<IPullRequestModel> GetPullRequest(ILocalRepositoryModel repo, int number)
public IObservable<IPullRequestModel> GetPullRequest(string owner, string name, int number)
{
return Observable.Defer(() =>
{
return hostCache.GetAndRefreshObject(PRPrefix + '|' + number, () =>
Observable.CombineLatest(
apiClient.GetPullRequest(repo.CloneUrl.Owner, repo.CloneUrl.RepositoryName, number),
apiClient.GetPullRequestFiles(repo.CloneUrl.Owner, repo.CloneUrl.RepositoryName, number).ToList(),
apiClient.GetIssueComments(repo.CloneUrl.Owner, repo.CloneUrl.RepositoryName, number).ToList(),
apiClient.GetPullRequestReviewComments(repo.CloneUrl.Owner, repo.CloneUrl.RepositoryName, number).ToList(),
apiClient.GetPullRequest(owner, name, number),
apiClient.GetPullRequestFiles(owner, name, number).ToList(),
apiClient.GetIssueComments(owner, name, number).ToList(),
apiClient.GetPullRequestReviewComments(owner, name, number).ToList(),
(pr, files, comments, reviewComments) => new
{
PullRequest = pr,
Expand All @@ -217,6 +217,19 @@ public IObservable<IPullRequestModel> GetPullRequest(ILocalRepositoryModel repo,
});
}

public IObservable<IRemoteRepositoryModel> GetRepository(string owner, string repo)
{
var keyobs = GetUserFromCache()
.Select(user => string.Format(CultureInfo.InvariantCulture, "{0}|{1}", CacheIndex.RepoPrefix, user.Login));

return Observable.Defer(() => keyobs
.SelectMany(key =>
hostCache.GetAndFetchLatest(
key,
() => apiClient.GetRepository(owner, repo).Select(RepositoryCacheItem.Create))
.Select(Create)));
}

public ITrackingCollection<IRemoteRepositoryModel> GetRepositories(ITrackingCollection<IRemoteRepositoryModel> collection)
{
var keyobs = GetUserFromCache()
Expand Down Expand Up @@ -391,7 +404,8 @@ IRemoteRepositoryModel Create(RepositoryCacheItem item)
new UriString(item.CloneUrl),
item.Private,
item.Fork,
Create(item.Owner))
Create(item.Owner),
item.Parent != null ? Create(item.Parent) : null)
{
CreatedAt = item.CreatedAt,
UpdatedAt = item.UpdatedAt
Expand Down Expand Up @@ -505,6 +519,7 @@ public RepositoryCacheItem(Repository apiRepository)
CreatedAt = apiRepository.CreatedAt;
UpdatedAt = apiRepository.UpdatedAt;
Timestamp = apiRepository.UpdatedAt;
Parent = apiRepository.Parent != null ? new RepositoryCacheItem(apiRepository.Parent) : null;
}

public long Id { get; set; }
Expand All @@ -521,6 +536,13 @@ public AccountCacheItem Owner
public bool Fork { get; set; }
public DateTimeOffset CreatedAt { get; set; }
public DateTimeOffset UpdatedAt { get; set; }

[AllowNull]
public RepositoryCacheItem Parent
{
[return: AllowNull]
get; set;
}
}

[NullGuard(ValidationFlags.None)]
Expand Down
77 changes: 54 additions & 23 deletions src/GitHub.App/Services/PullRequestService.cs
Expand Up @@ -25,7 +25,7 @@ namespace GitHub.Services
public class PullRequestService : IPullRequestService
{
const string SettingCreatedByGHfVS = "created-by-ghfvs";
const string SettingGHfVSPullRequest = "ghfvs-pr";
const string SettingGHfVSPullRequest = "ghfvs-pr-owner-number";

static readonly Regex InvalidBranchCharsRegex = new Regex(@"[^0-9A-Za-z\-]", RegexOptions.ECMAScript);
static readonly Regex BranchCapture = new Regex(@"branch\.(?<branch>.+)\.ghfvs-pr", RegexOptions.ECMAScript);
Expand Down Expand Up @@ -143,7 +143,7 @@ public IObservable<Unit> Checkout(ILocalRepositoryModel repository, IPullRequest

// Store the PR number in the branch config with the key "ghfvs-pr".
var prConfigKey = $"branch.{localBranchName}.{SettingGHfVSPullRequest}";
await gitClient.SetConfig(repo, prConfigKey, pullRequest.Number.ToString());
await gitClient.SetConfig(repo, prConfigKey, BuildGHfVSConfigKeyValue(pullRequest));

return Observable.Return(Unit.Default);
});
Expand Down Expand Up @@ -215,9 +215,9 @@ public IObservable<bool> EnsureLocalBranchesAreMarkedAsPullRequests(ILocalReposi

foreach (var branch in branches)
{
if (!await IsBranchMarkedAsPullRequest(repo, branch.Name, pullRequest.Number))
if (!await IsBranchMarkedAsPullRequest(repo, branch.Name, pullRequest))
{
await MarkBranchAsPullRequest(repo, branch.Name, pullRequest.Number);
await MarkBranchAsPullRequest(repo, branch.Name, pullRequest);
result = true;
}
}
Expand All @@ -226,13 +226,11 @@ public IObservable<bool> EnsureLocalBranchesAreMarkedAsPullRequests(ILocalReposi
});
}

public bool IsPullRequestFromFork(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
public bool IsPullRequestFromRepository(ILocalRepositoryModel repository, IPullRequestModel pullRequest)
{
if (pullRequest.Head?.Label != null && pullRequest.Base?.Label != null)
if (pullRequest.Head?.RepositoryCloneUrl != null)
{
var headOwner = pullRequest.Head.Label.Split(':')[0];
var baseOwner = pullRequest.Base.Label.Split(':')[0];
return headOwner != baseOwner;
return repository.CloneUrl.ToRepositoryUrl() == pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl();
}

return false;
Expand Down Expand Up @@ -271,24 +269,25 @@ public IObservable<Unit> SwitchToBranch(ILocalRepositoryModel repository, IPullR
}

await gitClient.Checkout(repo, branchName);
await MarkBranchAsPullRequest(repo, branchName, pullRequest.Number);
await MarkBranchAsPullRequest(repo, branchName, pullRequest);
}

return Observable.Return(Unit.Default);
});
}

public IObservable<int> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository)
public IObservable<Tuple<string, int>> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository)
{
return Observable.Defer(() =>
return Observable.Defer(async () =>
{
var repo = gitService.GetRepository(repository.LocalPath);
var configKey = string.Format(
CultureInfo.InvariantCulture,
"branch.{0}.{1}",
repo.Head.FriendlyName,
SettingGHfVSPullRequest);
return gitClient.GetConfig<int>(repo, configKey).ToObservable();
var value = await gitClient.GetConfig<string>(repo, configKey);
return Observable.Return(ParseGHfVSConfigKeyValue(value));
});
}

Expand All @@ -301,12 +300,13 @@ public IObservable<int> GetPullRequestForCurrentBranch(ILocalRepositoryModel rep
return Observable.Defer(async () =>
{
var repo = gitService.GetRepository(repository.LocalPath);
var remote = await gitClient.GetHttpRemote(repo, "origin");

var baseUrl = pullRequest.Base.RepositoryCloneUrl;
var headUrl = pullRequest.Head.RepositoryCloneUrl;
var baseSha = pullRequest.Base.Sha;
var headSha = pullRequest.Head.Sha;
var baseRef = pullRequest.Base.Ref;
string mergeBase = await gitClient.GetPullRequestMergeBase(repo, remote.Name, baseSha, headSha, baseRef, pullRequest.Number);
string mergeBase = await gitClient.GetPullRequestMergeBase(
repo, baseUrl, headUrl, baseSha, headSha, baseRef, pullRequest.Number);
if (mergeBase == null)
{
throw new FileNotFoundException($"Couldn't find merge base between {baseSha} and {headSha}.");
Expand Down Expand Up @@ -435,31 +435,35 @@ static string CreateTempFile(string fileName, string commitSha, string contents,
IRepository repository,
IPullRequestModel pullRequest)
{
if (!IsPullRequestFromFork(localRepository, pullRequest))
if (IsPullRequestFromRepository(localRepository, pullRequest))
{
return new[] { pullRequest.Head.Ref };
}
else
{
var pr = pullRequest.Number.ToString(CultureInfo.InvariantCulture);
var key = BuildGHfVSConfigKeyValue(pullRequest);

return repository.Config
.Select(x => new { Branch = BranchCapture.Match(x.Key).Groups["branch"].Value, Value = x.Value })
.Where(x => !string.IsNullOrWhiteSpace(x.Branch) && x.Value == pr)
.Where(x => !string.IsNullOrWhiteSpace(x.Branch) && x.Value == key)
.Select(x => x.Branch);
}
}

async Task<bool> IsBranchMarkedAsPullRequest(IRepository repo, string branchName, int number)
async Task<bool> IsBranchMarkedAsPullRequest(IRepository repo, string branchName, IPullRequestModel pullRequest)
{
var prConfigKey = $"branch.{branchName}.{SettingGHfVSPullRequest}";
return await gitClient.GetConfig<int>(repo, prConfigKey) == number;
var value = ParseGHfVSConfigKeyValue(await gitClient.GetConfig<string>(repo, prConfigKey));
return value != null &&
value.Item1 == pullRequest.Base.RepositoryCloneUrl.Owner &&
value.Item2 == pullRequest.Number;
}

async Task MarkBranchAsPullRequest(IRepository repo, string branchName, int number)
async Task MarkBranchAsPullRequest(IRepository repo, string branchName, IPullRequestModel pullRequest)
{
// Store the PR number in the branch config with the key "ghfvs-pr".
var prConfigKey = $"branch.{branchName}.{SettingGHfVSPullRequest}";
await gitClient.SetConfig(repo, prConfigKey, number.ToString());
await gitClient.SetConfig(repo, prConfigKey, BuildGHfVSConfigKeyValue(pullRequest));
}

async Task<IPullRequestModel> PushAndCreatePR(IRepositoryHost host,
Expand Down Expand Up @@ -499,5 +503,32 @@ static string GetSafeBranchName(string name)
before = after;
}
}

static string BuildGHfVSConfigKeyValue(IPullRequestModel pullRequest)
{
return pullRequest.Base.RepositoryCloneUrl.Owner + '#' +
pullRequest.Number.ToString(CultureInfo.InvariantCulture);
}

static Tuple<string, int> ParseGHfVSConfigKeyValue(string value)
{
if (value != null)
{
var separator = value.IndexOf('#');

if (separator != -1)
{
var owner = value.Substring(0, separator);
int number;

if (int.TryParse(value.Substring(separator + 1), NumberStyles.None, CultureInfo.InvariantCulture, out number))
{
return Tuple.Create(owner, number);
}
}
}

return null;
}
}
}