Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ccc49e9
Clean up GitServiceTests
jcansdale Jun 13, 2018
d1d6c4d
Use first remote in list when no "origin" remote
jcansdale Jun 13, 2018
2a04e80
Test different combinations of remotes
jcansdale Jun 13, 2018
7b47f79
Make "default" a constant defaultOriginName
jcansdale Jun 13, 2018
636b11e
Add FindOriginalRemoteName to IGitService
jcansdale Jun 13, 2018
b6e4a63
Default to use original origin when fetching
jcansdale Jun 13, 2018
f725bbd
Make IGitService remote names default to null
jcansdale Jun 13, 2018
cdac730
Rename to GetOriginRemoteName
jcansdale Jun 13, 2018
8acf74b
Let GetHttpRemote accept a default remote name
jcansdale Jun 14, 2018
e4f223f
Make PullRequestService use default origin remote
jcansdale Jun 14, 2018
3480932
Make RepositoryForkService use default origin name
jcansdale Jun 14, 2018
651c5f1
Add test for GetOriginRemoteName when no remotes
jcansdale Jun 14, 2018
afb7067
Make SetupLocalRepoMock configure original remote
jcansdale Jun 14, 2018
d9c0cfa
Add space after if
jcansdale Jun 14, 2018
c49765c
Fall back to using remotes from HEAD/master
jcansdale Jun 14, 2018
ba14b3f
Rename to GetDefaultRemoteName and update xmldocs
jcansdale Jun 14, 2018
a090996
Add tests for GitService.GetUri returning null
jcansdale Jun 14, 2018
8dbeee0
Don't change default remote based on HEAD
jcansdale Jun 15, 2018
fbf7c74
Merge branch 'master' into fixes/1729-find-original-remote-name
jcansdale Jun 18, 2018
3944b4f
Don't fall back to using remote from master branch
jcansdale Jun 18, 2018
5330df2
Allow user override origin using ghfvs.origin
jcansdale Jun 18, 2018
3cfb8d2
Improve xmldoc comments for GetHttpRemote
jcansdale Jun 20, 2018
93d3382
Fix build after merge
jcansdale Jun 20, 2018
d9a19ce
Merge branch 'master' into fixes/1729-find-original-remote-name
jcansdale Jun 21, 2018
1629695
Merge branch 'master' into fixes/1729-find-original-remote-name
jcansdale Jun 22, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/GitHub.App/SampleData/GitServiceDesigner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ namespace GitHub.SampleData
class GitServiceDesigner : IGitService
{
public Task<string> GetLatestPushedSha(string path) => Task.FromResult<string>(null);
public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null;
public UriString GetRemoteUri(IRepository repo, string remote = null) => null;
public IRepository GetRepository(string path) => null;
public UriString GetUri(string path, string remote = "origin") => null;
public UriString GetUri(IRepository repository, string remote = "origin") => null;
public UriString GetUri(string path, string remote = null) => null;
public UriString GetUri(IRepository repository, string remote = null) => null;
public string GetDefaultRemoteName(IRepository repo) => null;
}
}
10 changes: 5 additions & 5 deletions src/GitHub.App/Services/GitClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ namespace GitHub.Services
[PartCreationPolicy(CreationPolicy.Shared)]
public class GitClient : IGitClient
{
const string defaultOriginName = "origin";
static readonly ILogger log = LogManager.ForContext<GitClient>();
readonly IGitService gitService;
readonly PullOptions pullOptions;
Expand Down Expand Up @@ -98,10 +97,11 @@ public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs
{
var httpsUrl = UriString.ToUriString(cloneUrl.ToRepositoryUrl());

var originRemote = repo.Network.Remotes[defaultOriginName];
var originName = gitService.GetDefaultRemoteName(repo);
var originRemote = repo.Network.Remotes[originName];
if (originRemote != null && originRemote.Url == httpsUrl)
{
return Fetch(repo, defaultOriginName, refspecs);
return Fetch(repo, originName, refspecs);
}

return Task.Factory.StartNew(() =>
Expand Down Expand Up @@ -335,13 +335,13 @@ public Task UnsetConfig(IRepository repository, string key)
});
}

public Task<Remote> GetHttpRemote(IRepository repo, string remote)
public Task<Remote> GetHttpRemote(IRepository repo, string remote = null)
{
Guard.ArgumentNotNull(repo, nameof(repo));
Guard.ArgumentNotEmptyString(remote, nameof(remote));

return Task.Factory.StartNew(() =>
{
remote = remote ?? gitService.GetDefaultRemoteName(repo);
var uri = gitService.GetRemoteUri(repo, remote);
var remoteName = uri.IsHypertextTransferProtocol ? remote : remote + "-http";
var ret = repo.Network.Remotes[remoteName];
Expand Down
12 changes: 6 additions & 6 deletions src/GitHub.App/Services/PullRequestService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ public IObservable<Unit> Checkout(ILocalRepositoryModel repository, IPullRequest
}
else if (repository.CloneUrl.ToRepositoryUrl() == pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl())
{
var remote = await gitClient.GetHttpRemote(repo, "origin");
var remote = await gitClient.GetHttpRemote(repo);
await gitClient.Fetch(repo, remote.Name);
await gitClient.Checkout(repo, localBranchName);
}
Expand Down Expand Up @@ -371,7 +371,7 @@ public IObservable<TreeChanges> GetTreeChanges(ILocalRepositoryModel repository,
// TreeChanges doesn't keep a reference to Repository
using (var repo = gitService.GetRepository(repository.LocalPath))
{
var remote = await gitClient.GetHttpRemote(repo, "origin");
var remote = await gitClient.GetHttpRemote(repo);
await gitClient.Fetch(repo, remote.Name);
var changes = await gitClient.Compare(repo, pullRequest.Base.Sha, pullRequest.Head.Sha, detectRenames: true);
return Observable.Return(changes);
Expand Down Expand Up @@ -437,7 +437,7 @@ public IObservable<Unit> SwitchToBranch(ILocalRepositoryModel repository, IPullR

if (branchName != null)
{
var remote = await gitClient.GetHttpRemote(repo, "origin");
var remote = await gitClient.GetHttpRemote(repo);
await gitClient.Fetch(repo, remote.Name);

var branch = repo.Branches[branchName];
Expand Down Expand Up @@ -497,7 +497,7 @@ public async Task<string> ExtractToTempFile(
{
using (var repo = gitService.GetRepository(repository.LocalPath))
{
var remote = await gitClient.GetHttpRemote(repo, "origin");
var remote = await gitClient.GetHttpRemote(repo);
await ExtractToTempFile(repo, pullRequest.Number, commitSha, relativePath, encoding, tempFilePath);
}
}
Expand Down Expand Up @@ -610,7 +610,7 @@ async Task ExtractToTempFile(
catch (FileNotFoundException)
{
var pullHeadRef = $"refs/pull/{pullRequestNumber}/head";
var remote = await gitClient.GetHttpRemote(repo, "origin");
var remote = await gitClient.GetHttpRemote(repo);
await gitClient.Fetch(repo, remote.Name, commitSha, pullHeadRef);
contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
}
Expand Down Expand Up @@ -663,7 +663,7 @@ async Task<IPullRequestModel> PushAndCreatePR(IModelService modelService,
// PullRequestModel doesn't keep a reference to repo
using (var repo = await Task.Run(() => gitService.GetRepository(sourceRepository.LocalPath)))
{
var remote = await gitClient.GetHttpRemote(repo, "origin");
var remote = await gitClient.GetHttpRemote(repo);
await gitClient.Push(repo, sourceBranch.Name, remote.Name);

if (!repo.Branches[sourceBranch.Name].IsTracking)
Expand Down
2 changes: 1 addition & 1 deletion src/GitHub.App/Services/RepositoryForkService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public IObservable<object> SwitchRemotes(IRepositoryModel destinationRepository,

if (addUpstream)
{
var remote = await gitClient.GetHttpRemote(activeRepo, "origin");
var remote = await gitClient.GetHttpRemote(activeRepo);
currentOrigin = new Uri(remote.Url);
}

Expand Down
8 changes: 7 additions & 1 deletion src/GitHub.Exports.Reactive/Services/IGitClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,13 @@ public interface IGitClient
/// <returns></returns>
Task UnsetConfig(IRepository repository, string key);

Task<Remote> GetHttpRemote(IRepository repo, string remote);
/// <summary>
/// Get a <see cref="Remote"/> that used the http protocol.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be uses?

/// </summary>
/// <param name="repository">The repository</param>
/// <param name="remote">The name of the remote or null to use original remote.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "original remote" mean here? origin? I've never heard of that referred to as the "original remote". Would "default remote" maybe be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was supposed to find the original remote that was created when the repository was cloned. Alas libgit2 returns remotes in alphabetical order, so it's impossible to find the first created remote short of manually parsing the .git\config file. This would be too much of a hack even for my taste. 😉

/// <returns>A <see cref="Remote"/> that uses the http protocol.</returns>
Task<Remote> GetHttpRemote(IRepository repo, string remote = null);

/// <summary>
/// Extracts a file at a specified commit from the repository.
Expand Down
59 changes: 55 additions & 4 deletions src/GitHub.Exports/Services/GitService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ namespace GitHub.Services
[PartCreationPolicy(CreationPolicy.NonShared)]
public class GitService : IGitService
{
const string defaultOriginName = "origin";

/// <summary>
/// Returns the URL of the remote for the specified <see cref="repository"/>. If the repository
/// is null or no remote named origin exists, this method returns null
/// </summary>
/// <param name="repository">The repository to look at for the remote.</param>
/// <param name="remote">The name of the remote to look for</param>
/// <returns>Returns a <see cref="UriString"/> representing the uri of the remote normalized to a GitHub repository url or null if none found.</returns>
public UriString GetUri(IRepository repository, string remote = "origin")
public UriString GetUri(IRepository repository, string remote = null)
{
return UriString.ToUriString(GetRemoteUri(repository, remote)?.ToRepositoryUrl());
}
Expand All @@ -36,7 +38,7 @@ public UriString GetUri(IRepository repository, string remote = "origin")
/// <param name="path">The path to start probing</param>
/// <param name="remote">The name of the remote to look for</param>
/// <returns>Returns a <see cref="UriString"/> representing the uri of the remote normalized to a GitHub repository url or null if none found.</returns>
public UriString GetUri(string path, string remote = "origin")
public UriString GetUri(string path, string remote = null)
{
using (var repo = GetRepository(path))
{
Expand Down Expand Up @@ -66,10 +68,21 @@ public IRepository GetRepository(string path)
/// <param name="repo"></param>
/// <param name="remote">The name of the remote to look for</param>
/// <returns></returns>
public UriString GetRemoteUri(IRepository repo, string remote = "origin")
public UriString GetRemoteUri(IRepository repo, string remote = null)
{
if (repo == null)
{
return null;
}

remote = remote ?? TryGetDefaultRemoteName(repo);
if (remote == null)
{
return null;
}

return repo
?.Network
.Network
.Remotes[remote]
?.Url;
}
Expand Down Expand Up @@ -117,5 +130,43 @@ public Task<string> GetLatestPushedSha(string path)
}
});
}

/// <summary>
/// Find a remote named "origin" or the remote tracking "master".
/// </summary>
/// <param name="repo">The <see cref="IRepository" /> to find a remote for.</param>
/// <returns>The remote named "origin" or the remote tracking "master"</returns>
/// <exception cref="InvalidOperationException">If repository contains no "origin" remote or "master" branch.</exception>
public string GetDefaultRemoteName(IRepository repo)
{
var remoteName = TryGetDefaultRemoteName(repo);
if (remoteName != null)
{
return remoteName;
}

throw new InvalidOperationException("Can't get default remote name because repository contains no `origin` remote or `master` branch.");
}

static string TryGetDefaultRemoteName(IRepository repo)
{
var remotes = repo.Network.Remotes;
var remote = remotes[defaultOriginName];
if (remote != null)
{
// Use remote named "origin"
return remote.Name;
}

var masterBranch = repo.Branches["master"];
var remoteName = masterBranch?.RemoteName;
if (remoteName != null)
{
// Use remote from the "master" branch
return remoteName;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to do what the PR says in the description. The description says "We can fall back to using the first remote as the originating remote/clone URL.", instead this uses master's remote as the default remote.

I'm not sure this is a good idea as I sometimes make master track upstream/master rather than origin/master on forks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree completely. Making master track upstream/master is almost certainly what people should be doing. I imagine it's pretty rare to want your own master that's independant of upstream/master.

Copy link
Contributor

Choose a reason for hiding this comment

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

So do you agree the logic here is wrong? Why does it not do what you say it does in the description, i.e. "fall back to using the first remote as the originating remote/clone URL"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So do you agree the logic here is wrong?

Yup. Fixed in latest push.

Some users like to rename origin and delete master, so this wouldn't have worked for them anyway.

Why does it not do what you say it does in the description, i.e. "fall back to using the first remote as the originating remote/clone URL"?

It turns out that libgit2 sorts the list of remotes in alphabetical order, which completely messes up this logic. 😭

}

return null;
}
}
}
16 changes: 12 additions & 4 deletions src/GitHub.Exports/Services/IGitService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ public interface IGitService
/// </summary>
/// <param name="repository">The repository to look at for the remote.</param>
/// <returns>A <see cref="UriString"/> representing the origin or null if none found.</returns>
UriString GetUri(IRepository repository, string remote = "origin");
UriString GetUri(IRepository repository, string remote = null);

/// <summary>
/// Probes for a git repository and if one is found, returns a <see cref="UriString"/> for the repository's
Expand All @@ -25,8 +25,8 @@ public interface IGitService
/// </remarks>
/// <param name="path">The path to start probing</param>
/// <returns>A <see cref="UriString"/> representing the origin or null if none found.</returns>
UriString GetUri(string path, string remote = "origin");
UriString GetUri(string path, string remote = null);

/// <summary>
/// Probes for a git repository and if one is found, returns a <see cref="IRepositoryModel"/> instance for the
/// repository.
Expand All @@ -44,7 +44,7 @@ public interface IGitService
/// </summary>
/// <param name="repo"></param>
/// <returns></returns>
UriString GetRemoteUri(IRepository repo, string remote = "origin");
UriString GetRemoteUri(IRepository repo, string remote = null);

/// <summary>
/// Finds the latest pushed commit of a file and returns the sha of that commit. Returns null when no commits have
Expand All @@ -53,5 +53,13 @@ public interface IGitService
/// <param name="path">The local path of a repository or a file inside a repository. This cannot be null.</param>
/// <returns></returns>
Task<string> GetLatestPushedSha(string path);

/// <summary>
/// Find a remote named "origin" or the remote tracking "master".
/// </summary>
/// <param name="repo">The <see cref="IRepository" /> to find a remote for.</param>
/// <returns>The remote named "origin" or the remote tracking "master"</returns>
/// <exception cref="InvalidOperationException">If repository contains no "origin" remote or "master" branch.</exception>
string GetDefaultRemoteName(IRepository repo);
}
}
4 changes: 3 additions & 1 deletion test/UnitTests/GitHub.App/Services/GitClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,10 @@ static IRepository MockRepo(string baseSha, string headSha, string mergeBaseSha)

static GitClient CreateGitClient()
{
var gitService = Substitute.For<IGitService>();
gitService.GetDefaultRemoteName(null).ReturnsForAnyArgs("origin");
return new GitClient(
Substitute.For<IGitHubCredentialProvider>(),
Substitute.For<IGitService>());
gitService);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ static LibGit2Sharp.IRepository SetupLocalRepoMock(IGitClient gitClient, IGitSer
l2repo.Branches.Returns(l2branchcol);
l2repo.Head.Returns(l2branch);
gitService.GetRepository(Args.String).Returns(l2repo);
gitService.GetDefaultRemoteName(l2repo).Returns(remote);
return l2repo;
}

Expand Down
Loading