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

Create remote with the same name as upstream owner when viewing a PR #1752

Merged
grokys merged 13 commits into from
Jun 21, 2018
Merged

Create remote with the same name as upstream owner when viewing a PR #1752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cc30981
Add failing test for feting from existing remote
jcansdale Jun 19, 2018
8650e71
Fetch from the matched remote name
jcansdale Jun 19, 2018
42cfd7c
Check we create remote with name of upstream owner
jcansdale Jun 19, 2018
0ff13a9
Create remote with same name as repository owner
jcansdale Jun 19, 2018
bc1b52e
Compare Urls ignoring any trailing .git
jcansdale Jun 19, 2018
9e4209f
Ignore case when comparing remote Urls
jcansdale Jun 19, 2018
3cdd1f6
Add RepositoryUrlsAreEqual method to UriString
jcansdale Jun 19, 2018
d1ca52c
Update to use UriString.RepositoryUrlsAreEqual
jcansdale Jun 19, 2018
fdd2b48
Fix failing FetchUsingHttps test
jcansdale Jun 19, 2018
62374bc
Merge branch 'master' into fixes/1749-fetching-from-existing-remote
jcansdale Jun 19, 2018
60091c3
Merge branch 'master' into fixes/1749-fetching-from-existing-remote
jcansdale Jun 19, 2018
2d0cb4c
Remove extra new lines
jcansdale Jun 21, 2018
6a78d53
Merge branch 'master' into fixes/1749-fetching-from-existing-remote
grokys Jun 21, 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
33 changes: 18 additions & 15 deletions src/GitHub.App/Services/GitClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,30 +96,30 @@ public Task Fetch(IRepository repository, string remoteName)

public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs)
{
var httpsString = cloneUrl.ToRepositoryUrl().ToString();

foreach (var remote in repo.Network.Remotes)
{
var remoteUrl = new UriString(remote.Url);
if (!remoteUrl.IsHypertextTransferProtocol)
{
// Only match http urls
continue;
}

var remoteHttpsString = remoteUrl.ToRepositoryUrl().ToString();
if (remoteHttpsString.Equals(httpsString, StringComparison.OrdinalIgnoreCase))
if (UriString.RepositoryUrlsAreEqual(new UriString(remote.Url), cloneUrl))
{
return Fetch(repo, defaultOriginName, refspecs);
Copy link
Collaborator Author

@jcansdale jcansdale Jun 20, 2018

Choose a reason for hiding this comment

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

There was a bug here. This should have used remote.Name not defaultOriginName.

return Fetch(repo, remote.Name, refspecs);
}
}

return Task.Factory.StartNew(() =>
{
try
{
var tempRemoteName = cloneUrl.Owner + "-" + Guid.NewGuid();
var remote = repo.Network.Remotes.Add(tempRemoteName, httpsString);
var remoteName = cloneUrl.Owner;
var remoteUri = cloneUrl.ToRepositoryUrl();

var removeRemote = false;
if (repo.Network.Remotes[remoteName] != null)
{
// If a remote with this neme already exists, use a unique name and remove remote afterwards
remoteName = cloneUrl.Owner + "-" + Guid.NewGuid();
removeRemote = true;
}

var remote = repo.Network.Remotes.Add(remoteName, remoteUri.ToString());
try
{
#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`.
Expand All @@ -128,7 +128,10 @@ public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs
}
finally
{
repo.Network.Remotes.Remove(tempRemoteName);
if (removeRemote)
{
repo.Network.Remotes.Remove(remoteName);
}
}
}
catch (Exception ex)
Expand Down
6 changes: 3 additions & 3 deletions src/GitHub.App/Services/PullRequestService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ async Task<string> CreateRemote(IRepository repo, UriString cloneUri)
{
foreach (var remote in repo.Network.Remotes)
{
if (remote.Url == cloneUri)
if (UriString.RepositoryUrlsAreEqual(new UriString(remote.Url), cloneUri))
{
return remote.Name;
}
Expand Down Expand Up @@ -597,7 +597,7 @@ async Task ExtractToTempFile(
string tempFilePath)
{
string contents;

try
{
contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
Expand Down Expand Up @@ -680,7 +680,7 @@ static string GetSafeBranchName(string name)
{
var before = InvalidBranchCharsRegex.Replace(name, "-").TrimEnd('-');

for (;;)
for (; ; )
{
string after = before.Replace("--", "-");

Expand Down
28 changes: 23 additions & 5 deletions src/GitHub.Exports/Primitives/UriString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ public UriString(string uriString) : base(NormalizePath(uriString))

if (RepositoryName != null)
{
NameWithOwner = Owner != null
? string.Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName)
NameWithOwner = Owner != null
? string.Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName)
: RepositoryName;
}
}
Expand All @@ -70,12 +70,12 @@ void SetUri(Uri uri)
{
RepositoryName = GetRepositoryName(uri.Segments.Last());
}

if (uri.Segments.Length > 2)
{
Owner = (uri.Segments[uri.Segments.Length - 2] ?? "").TrimEnd('/').ToNullIfEmpty();
}

IsHypertextTransferProtocol = uri.IsHypertextTransferProtocol();
}

Expand Down Expand Up @@ -210,6 +210,24 @@ public override UriString Combine(string addition)
return String.Concat(Value, addition);
}

/// <summary>
/// Compare repository URLs ignoring any trailing ".git" or difference in case.
/// </summary>
/// <returns>True if URLs reference the same repository.</returns>
public static bool RepositoryUrlsAreEqual(UriString uri1, UriString uri2)
{
if (!uri1.IsHypertextTransferProtocol || !uri2.IsHypertextTransferProtocol)
{
// Not a repository URL
return false;
}

// Normalize repository URLs
var str1 = uri1.ToRepositoryUrl().ToString();
var str2 = uri2.ToRepositoryUrl().ToString();
return string.Equals(str1, str2, StringComparison.OrdinalIgnoreCase);
}

public override string ToString()
{
// Makes this look better in the debugger.
Expand Down Expand Up @@ -251,7 +269,7 @@ static string NormalizePath(string path)

static string GetRepositoryName(string repositoryNameSegment)
{
if (String.IsNullOrEmpty(repositoryNameSegment)
if (String.IsNullOrEmpty(repositoryNameSegment)
|| repositoryNameSegment.Equals("/", StringComparison.Ordinal))
{
return null;
Expand Down
50 changes: 50 additions & 0 deletions test/UnitTests/GitHub.App/Services/GitClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,56 @@ public async Task UseExistingRemoteWhenPossible(string fetchUrl, string remoteNa
}
}

[TestCase("https://github.com/upstream_owner/repo", "origin", "https://github.com/origin_owner/repo",
"upstream_owner", "https://github.com/upstream_owner/repo")]
public async Task CreateRemoteWithNameOfOwner(string fetchUrl, string remoteName, string remoteUrl,
string expectRemoteName, string expectRemoteUrl)
{
var repo = CreateRepository(remoteName, remoteUrl);
var fetchUri = new UriString(fetchUrl);
var refSpec = "refSpec";
var gitClient = CreateGitClient();

await gitClient.Fetch(repo, fetchUri, refSpec);

repo.Network.Remotes.Received(1).Add(expectRemoteName, expectRemoteUrl);
repo.Network.Remotes.Received(0).Remove(Arg.Any<string>());
}

[TestCase("https://github.com/same_name/repo", "same_name", "https://github.com/different_name/repo",
"same_name", "https://github.com/same_name/repo")]
public async Task UseTemporaryRemoteWhenSameRemoteWithDifferentUrlExists(string fetchUrl, string remoteName, string remoteUrl,
string expectRemoteName, string expectRemoteUrl)
{
var repo = CreateRepository(remoteName, remoteUrl);
var fetchUri = new UriString(fetchUrl);
var refSpec = "refSpec";
var gitClient = CreateGitClient();

await gitClient.Fetch(repo, fetchUri, refSpec);

repo.Network.Remotes.Received(0).Add(expectRemoteName, expectRemoteUrl);
repo.Network.Remotes.Received(1).Add(Arg.Any<string>(), expectRemoteUrl);
repo.Network.Remotes.Received(1).Remove(Arg.Any<string>());
}

[TestCase("https://github.com/owner/repo", "origin", "https://github.com/owner/repo", "origin")]
[TestCase("https://github.com/owner/repo", "not_origin", "https://github.com/owner/repo", "not_origin")]
public async Task FetchFromExistingRemote(string fetchUrl, string remoteName, string remoteUrl, string expectRemoteName)
{
var repo = CreateRepository(remoteName, remoteUrl);
var fetchUri = new UriString(fetchUrl);
var refSpec = "refSpec";
var gitClient = CreateGitClient();

await gitClient.Fetch(repo, fetchUri, refSpec);

var remote = repo.Network.Remotes[expectRemoteName];
#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`.
repo.Network.Received(1).Fetch(remote, Arg.Any<string[]>(), Arg.Any<FetchOptions>());
#pragma warning restore 0618
}

static IRepository CreateRepository(string remoteName, string remoteUrl)
{
var remote = Substitute.For<Remote>();
Expand Down
39 changes: 26 additions & 13 deletions test/UnitTests/GitHub.Primitives/UriStringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ public class UriStringTests
{
public class TheConstructor : TestBaseClass
{

[TestCase("http://192.168.1.3/foo/bar.git", "192.168.1.3", "foo", "bar")]
[TestCase("http://haacked@example.com/foo/bar", "example.com", "foo", "bar")]
[TestCase("http://haacked:password@example.com/foo/bar", "example.com", "foo", "bar")]
Expand Down Expand Up @@ -41,7 +40,6 @@ public void ParsesWellFormedUrlComponents(string url, string expectedHost, strin
Assert.False(cloneUrl.IsFileUri);
}


[TestCase(@"..\bar\foo")]
[TestCase(@"..\..\foo")]
[TestCase(@"../..\foo")]
Expand All @@ -67,7 +65,6 @@ public void ParsesLocalFileUris(string path)
Assert.True(cloneUrl.IsFileUri);
}


[TestCase("complete garbage", "", "", null)]
[TestCase(@"..\other_folder", "", "", "other_folder")]
[TestCase("http://example.com", "example.com", null, null)]
Expand All @@ -89,7 +86,6 @@ public void ParsesWeirdUrlsAsWellAsPossible(string url, string expectedHost, str
Assert.That(cloneUrl.RepositoryName, Is.EqualTo(repositoryName));
}


[TestCase(@"http:\\example.com/bar\baz")]
[TestCase(@"http://example.com/bar/baz")]
public void NormalizesSeparator(string uriString)
Expand All @@ -107,7 +103,6 @@ public void AcceptsNullConversion()

public class TheNameWithOwnerProperty : TestBaseClass
{

[TestCase("http://192.168.1.3/foo/bar.git", "foo/bar")]
[TestCase("http://192.168.1.3/foo/bar", "foo/bar")]
[TestCase("http://192.168.1.3/foo/bar/baz/qux", "baz/qux")]
Expand All @@ -125,7 +120,6 @@ public void DependsOnOwnerAndRepoNameNotBeingNull(string url, string expectedNam

public class TheCombineMethod : TestBaseClass
{

[TestCase("http://example.com", "foo/bar", @"http://example.com/foo/bar")]
[TestCase("http://example.com/", "foo/bar", @"http://example.com/foo/bar")]
[TestCase("http://example.com/", "/foo/bar/", @"http://example.com/foo/bar/")]
Expand All @@ -146,7 +140,6 @@ public void ComparesHostInsensitively(string uriString, string path, string expe

public class TheIsValidUriProperty : TestBaseClass
{

[TestCase("http://example.com/", true)]
[TestCase("file:///C:/dev/exp/foo", true)]
[TestCase("garbage", false)]
Expand All @@ -159,7 +152,6 @@ public void ReturnWhetherTheUriIsParseableByUri(string uriString, bool expected)

public class TheToRepositoryUrlMethod : TestBaseClass
{

[TestCase("file:///C:/dev/exp/foo", "file:///C:/dev/exp/foo")]
[TestCase("http://example.com/", "http://example.com/")]
[TestCase("http://haacked@example.com/foo/bar", "http://example.com/foo/bar")]
Expand All @@ -176,7 +168,6 @@ public void ConvertsToWebUrl(string uriString, string expected)
Assert.That(new UriString(uriString).ToRepositoryUrl(), Is.EqualTo(new Uri(expected)));
}


[TestCase("file:///C:/dev/exp/foo", "file:///C:/dev/exp/foo")]
[TestCase("http://example.com/", "http://example.com/")]
[TestCase("http://haacked@example.com/foo/bar", "http://example.com/baz/bar")]
Expand Down Expand Up @@ -208,7 +199,7 @@ public void ConvertsWithNewOwner(string uriString, string expected, string owner
[TestCase("git@example.com:org/repo.git", "https://example.com/org/repo")]
[TestCase("ssh://git@github.com:443/shana/cef", "https://github.com/shana/cef")]
[TestCase("ssh://git@example.com:23/haacked/encourage", "https://example.com:23/haacked/encourage")]

[TestCase("asdf", null)]
[TestCase("", null)]
[TestCase("file:///C:/dev/exp/foo", "file:///C:/dev/exp/foo")]
Expand All @@ -233,7 +224,6 @@ public void ShouldNeverThrow(string url, string expected)

public class TheAdditionOperator : TestBaseClass
{

[TestCase("http://example.com", "foo/bar", @"http://example.com/foo/bar")]
[TestCase("http://example.com/", "foo/bar", @"http://example.com/foo/bar")]
[TestCase("http://example.com/", "/foo/bar/", @"http://example.com/foo/bar/")]
Expand Down Expand Up @@ -293,7 +283,6 @@ public void ConvertsNullToNull()

public class TheIsHypertextTransferProtocolProperty : TestBaseClass
{

[TestCase("http://example.com", true)]
[TestCase("HTTP://example.com", true)]
[TestCase("https://example.com", true)]
Expand All @@ -310,7 +299,6 @@ public void IsTrueOnlyForHttpAndHttps(string url, bool expected)

public class TheEqualsMethod : TestBaseClass
{

[TestCase("https://github.com/foo/bar", "https://github.com/foo/bar", true)]
[TestCase("https://github.com/foo/bar", "https://github.com/foo/BAR", false)]
[TestCase("https://github.com/foo/bar", "https://github.com/foo/bar/", false)]
Expand All @@ -334,4 +322,29 @@ public void MakesUriStringSuitableForDictionaryKey()
Assert.True(dictionary.ContainsKey(new UriString("https://github.com/foo/bar")));
}
}

public class TheRepositoryUrlsAreEqualMethod
{
[TestCase("https://github.com/owner/repo", "https://github.com/owner/repo", true)]
[TestCase("https://github.com/owner/repo", "HTTPS://GITHUB.COM/OWNER/REPO", true)]
[TestCase("https://github.com/owner/repo.git", "https://github.com/owner/repo", true)]
[TestCase("https://github.com/owner/repo", "https://github.com/owner/repo.git", true)]
[TestCase("https://github.com/owner/repo", "https://github.com/different_owner/repo", false)]
[TestCase("https://github.com/owner/repo", "https://github.com/owner/different_repo", false)]
[TestCase("ssh://git@github.com:443/shana/cef", "https://github.com/shana/cef", false)]
[TestCase("file://github.com/github/visualstudio", "https://github.com/github/visualstudio", false)]
[TestCase("http://github.com/github/visualstudio", "https://github.com/github/visualstudio", false,
Description = "http is different to https")]
[TestCase("ssh://git@github.com:443/shana/cef", "ssh://git@github.com:443/shana/cef", false,
Description = "The same but not a repository URL")]
public void RepositoryUrlsAreEqual(string url1, string url2, bool expectEqual)
{
var uriString1 = new UriString(url1);
var uriString2 = new UriString(url2);

var equal = UriString.RepositoryUrlsAreEqual(uriString1, uriString2);

Assert.That(equal, Is.EqualTo(expectEqual));
}
}
}