Merge pull request #1752 from github/fixes/1749-fetching-from-existing-remote

grokys · web-flow · commit 45bccfdbd482 · 2018-06-21T19:36:47.000+02:00
Create remote with the same name as upstream owner when viewing a PR
diff --git a/src/GitHub.App/Services/GitClient.cs b/src/GitHub.App/Services/GitClient.cs
@@ -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);
+                    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`.
@@ -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)
diff --git a/src/GitHub.App/Services/PullRequestService.cs b/src/GitHub.App/Services/PullRequestService.cs
@@ -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;
                 }
@@ -597,7 +597,7 @@ async Task ExtractToTempFile(
             string tempFilePath)
         {
             string contents;
-            
+
             try
             {
                 contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;
@@ -680,7 +680,7 @@ static string GetSafeBranchName(string name)
         {
             var before = InvalidBranchCharsRegex.Replace(name, "-").TrimEnd('-');
 
-            for (;;)
+            for (; ; )
             {
                 string after = before.Replace("--", "-");
 
diff --git a/src/GitHub.Exports/Primitives/UriString.cs b/src/GitHub.Exports/Primitives/UriString.cs
@@ -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;
             }
         }
@@ -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();
         }
 
@@ -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.
@@ -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;
diff --git a/test/UnitTests/GitHub.App/Services/GitClientTests.cs b/test/UnitTests/GitHub.App/Services/GitClientTests.cs
@@ -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>();
diff --git a/test/UnitTests/GitHub.Primitives/UriStringTests.cs b/test/UnitTests/GitHub.Primitives/UriStringTests.cs
@@ -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")]
@@ -41,7 +40,6 @@ public void ParsesWellFormedUrlComponents(string url, string expectedHost, strin
             Assert.False(cloneUrl.IsFileUri);
         }
 
-        
         [TestCase(@"..\bar\foo")]
         [TestCase(@"..\..\foo")]
         [TestCase(@"../..\foo")]
@@ -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)]
@@ -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)
@@ -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")]
@@ -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/")]
@@ -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)]
@@ -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")]
@@ -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")]
@@ -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")]
@@ -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/")]
@@ -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)]
@@ -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)]
@@ -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));
+        }
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -96,30 +96,30 @@ public Task Fetch(IRepository repository, string remoteName)`
`96`	`96`
`97`	`97`	`public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs)`
`98`	`98`	`{`
`99`		`- var httpsString = cloneUrl.ToRepositoryUrl().ToString();`
`100`		`-`
`101`	`99`	`foreach (var remote in repo.Network.Remotes)`
`102`	`100`	`{`
`103`		`- var remoteUrl = new UriString(remote.Url);`
`104`		`- if (!remoteUrl.IsHypertextTransferProtocol)`
`105`		`- {`
`106`		`- // Only match http urls`
`107`		`- continue;`
`108`		`- }`
`109`		`-`
`110`		`- var remoteHttpsString = remoteUrl.ToRepositoryUrl().ToString();`
`111`		`- if (remoteHttpsString.Equals(httpsString, StringComparison.OrdinalIgnoreCase))`
	`101`	`+ if (UriString.RepositoryUrlsAreEqual(new UriString(remote.Url), cloneUrl))`
`112`	`102`	`{`
`113`		`- return Fetch(repo, defaultOriginName, refspecs);`
	`103`	`+ return Fetch(repo, remote.Name, refspecs);`
`114`	`104`	`}`
`115`	`105`	`}`
`116`	`106`
`117`	`107`	`return Task.Factory.StartNew(() =>`
`118`	`108`	`{`
`119`	`109`	`try`
`120`	`110`	`{`
`121`		`- var tempRemoteName = cloneUrl.Owner + "-" + Guid.NewGuid();`
`122`		`- var remote = repo.Network.Remotes.Add(tempRemoteName, httpsString);`
	`111`	`+ var remoteName = cloneUrl.Owner;`
	`112`	`+ var remoteUri = cloneUrl.ToRepositoryUrl();`
	`113`	`+`
	`114`	`+ var removeRemote = false;`
	`115`	`+ if (repo.Network.Remotes[remoteName] != null)`
	`116`	`+ {`
	`117`	`+ // If a remote with this neme already exists, use a unique name and remove remote afterwards`
	`118`	`+ remoteName = cloneUrl.Owner + "-" + Guid.NewGuid();`
	`119`	`+ removeRemote = true;`
	`120`	`+ }`
	`121`	`+`
	`122`	`+ var remote = repo.Network.Remotes.Add(remoteName, remoteUri.ToString());`
`123`	`123`	`try`
`124`	`124`	`{`
`125`	`125`	#pragma warning disable 0618 // TODO: Replace `Network.Fetch` with `Commands.Fetch`.
`@@ -128,7 +128,10 @@ public Task Fetch(IRepository repo, UriString cloneUrl, params string[] refspecs`
`128`	`128`	`}`
`129`	`129`	`finally`
`130`	`130`	`{`
`131`		`- repo.Network.Remotes.Remove(tempRemoteName);`
	`131`	`+ if (removeRemote)`
	`132`	`+ {`
	`133`	`+ repo.Network.Remotes.Remove(remoteName);`
	`134`	`+ }`
`132`	`135`	`}`
`133`	`136`	`}`
`134`	`137`	`catch (Exception ex)`
Original file line number	Diff line number	Diff line change
`@@ -563,7 +563,7 @@ async Task<string> CreateRemote(IRepository repo, UriString cloneUri)`
`563`	`563`	`{`
`564`	`564`	`foreach (var remote in repo.Network.Remotes)`
`565`	`565`	`{`
`566`		`- if (remote.Url == cloneUri)`
	`566`	`+ if (UriString.RepositoryUrlsAreEqual(new UriString(remote.Url), cloneUri))`
`567`	`567`	`{`
`568`	`568`	`return remote.Name;`
`569`	`569`	`}`
`@@ -597,7 +597,7 @@ async Task ExtractToTempFile(`
`597`	`597`	`string tempFilePath)`
`598`	`598`	`{`
`599`	`599`	`string contents;`
`600`		`-`
	`600`	`+`
`601`	`601`	`try`
`602`	`602`	`{`
`603`	`603`	`contents = await gitClient.ExtractFile(repo, commitSha, relativePath) ?? string.Empty;`
`@@ -680,7 +680,7 @@ static string GetSafeBranchName(string name)`
`680`	`680`	`{`
`681`	`681`	`var before = InvalidBranchCharsRegex.Replace(name, "-").TrimEnd('-');`
`682`	`682`
`683`		`- for (;;)`
	`683`	`+ for (; ; )`
`684`	`684`	`{`
`685`	`685`	`string after = before.Replace("--", "-");`
`686`	`686`
Original file line number	Diff line number	Diff line change
`@@ -45,8 +45,8 @@ public UriString(string uriString) : base(NormalizePath(uriString))`
`45`	`45`
`46`	`46`	`if (RepositoryName != null)`
`47`	`47`	`{`
`48`		`- NameWithOwner = Owner != null`
`49`		`- ? string.Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName)`
	`48`	`+ NameWithOwner = Owner != null`
	`49`	`+ ? string.Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName)`
`50`	`50`	`: RepositoryName;`
`51`	`51`	`}`
`52`	`52`	`}`
`@@ -70,12 +70,12 @@ void SetUri(Uri uri)`
`70`	`70`	`{`
`71`	`71`	`RepositoryName = GetRepositoryName(uri.Segments.Last());`
`72`	`72`	`}`
`73`		`-`
	`73`	`+`
`74`	`74`	`if (uri.Segments.Length > 2)`
`75`	`75`	`{`
`76`	`76`	`Owner = (uri.Segments[uri.Segments.Length - 2] ?? "").TrimEnd('/').ToNullIfEmpty();`
`77`	`77`	`}`
`78`		`-`
	`78`	`+`
`79`	`79`	`IsHypertextTransferProtocol = uri.IsHypertextTransferProtocol();`
`80`	`80`	`}`
`81`	`81`
`@@ -210,6 +210,24 @@ public override UriString Combine(string addition)`
`210`	`210`	`return String.Concat(Value, addition);`
`211`	`211`	`}`
`212`	`212`
	`213`	`+ /// <summary>`
	`214`	`+ /// Compare repository URLs ignoring any trailing ".git" or difference in case.`
	`215`	`+ /// </summary>`
	`216`	`+ /// <returns>True if URLs reference the same repository.</returns>`
	`217`	`+ public static bool RepositoryUrlsAreEqual(UriString uri1, UriString uri2)`
	`218`	`+ {`
	`219`	`+ if (!uri1.IsHypertextTransferProtocol \|\| !uri2.IsHypertextTransferProtocol)`
	`220`	`+ {`
	`221`	`+ // Not a repository URL`
	`222`	`+ return false;`
	`223`	`+ }`
	`224`	`+`
	`225`	`+ // Normalize repository URLs`
	`226`	`+ var str1 = uri1.ToRepositoryUrl().ToString();`
	`227`	`+ var str2 = uri2.ToRepositoryUrl().ToString();`
	`228`	`+ return string.Equals(str1, str2, StringComparison.OrdinalIgnoreCase);`
	`229`	`+ }`
	`230`	`+`
`213`	`231`	`public override string ToString()`
`214`	`232`	`{`
`215`	`233`	`// Makes this look better in the debugger.`
`@@ -251,7 +269,7 @@ static string NormalizePath(string path)`
`251`	`269`
`252`	`270`	`static string GetRepositoryName(string repositoryNameSegment)`
`253`	`271`	`{`
`254`		`- if (String.IsNullOrEmpty(repositoryNameSegment)`
	`272`	`+ if (String.IsNullOrEmpty(repositoryNameSegment)`
`255`	`273`	`\|\| repositoryNameSegment.Equals("/", StringComparison.Ordinal))`
`256`	`274`	`{`
`257`	`275`	`return null;`