From fe26c35a8f67ddfb778eb20100e453b5856b86ae Mon Sep 17 00:00:00 2001 From: Haacked Date: Wed, 29 Jul 2015 16:37:46 -0700 Subject: [PATCH 01/14] Add a ToWebUri method to UriString Note that ToUri isn't used anywhere, so I replaced it. --- src/GitHub.Exports/Primitives/UriString.cs | 80 ++---- .../GitHub.Primitives/UriStringTests.cs | 269 ++++++++++++++++++ src/UnitTests/UnitTests.csproj | 1 + 3 files changed, 292 insertions(+), 58 deletions(-) create mode 100644 src/UnitTests/GitHub.Primitives/UriStringTests.cs diff --git a/src/GitHub.Exports/Primitives/UriString.cs b/src/GitHub.Exports/Primitives/UriString.cs index 09cca08dfc..92a66efe99 100644 --- a/src/GitHub.Exports/Primitives/UriString.cs +++ b/src/GitHub.Exports/Primitives/UriString.cs @@ -1,12 +1,11 @@ using System; -using System.Collections.Generic; -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Linq; using System.Runtime.Serialization; using System.Text.RegularExpressions; using GitHub.Extensions; +using static System.String; namespace GitHub.Primitives { @@ -31,6 +30,7 @@ public class UriString : StringEquivalent, IEquatable public UriString(string uriString) : base(NormalizePath(uriString)) { + if (uriString == null) throw new ArgumentNullException(nameof(uriString), "Cannot create a null UriString"); if (uriString.Length == 0) return; if (Uri.TryCreate(uriString, UriKind.Absolute, out url)) { @@ -47,7 +47,7 @@ public UriString(string uriString) : base(NormalizePath(uriString)) if (RepositoryName != null) { NameWithOwner = Owner != null - ? string.Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName) + ? Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName) : RepositoryName; } } @@ -71,43 +71,6 @@ void SetUri(Uri uri) } IsHypertextTransferProtocol = uri.IsHypertextTransferProtocol(); - - if (String.IsNullOrEmpty(uri.Query)) return; - - try - { - var query = ParseQueryString(uri); - - if (query.ContainsKey("branch") && !String.IsNullOrEmpty(query["branch"])) - { - Branch = query["branch"].Replace("%2F", "/"); - } - - if (query.ContainsKey("pr") && !String.IsNullOrEmpty(query["pr"])) - { - PullRequest = query["pr"].Replace("%2F", "/"); - } - - if (query.ContainsKey("filepath") && !String.IsNullOrEmpty(query["filepath"])) - { - RelativePathToOpen = query["filepath"].Replace("%2F", "/").Replace('/', '\\'); - } - } - catch //(Exception ex) - { - //log.WarnException("Failed to read URI query", ex); - } - } - - // This is not a complete query string parsing algorithm, but it's good enough for our needs. - static IDictionary ParseQueryString(Uri uri) - { - Debug.Assert(uri.Query.StartsWith('?'), - String.Format(CultureInfo.InvariantCulture, "Uri.Query doesn't start with '?': '{0}'", uri.Query)); - return uri.Query.Substring(1).Split(new[] {'&'}, StringSplitOptions.RemoveEmptyEntries) - .Select(pair => pair.Split('=')) - .ToDictionary(pair => pair.First(), pair => pair.Length > 1 ? pair[1] : null, - StringComparer.OrdinalIgnoreCase); } void SetFilePath(Uri uri) @@ -144,8 +107,6 @@ bool ParseScpSyntax(string scpString) return false; } - public string Branch { get; private set; } - public string PullRequest { get; set; } public string Host { get; private set; } public string Owner { get; private set; } @@ -154,19 +115,24 @@ bool ParseScpSyntax(string scpString) public string NameWithOwner { get; private set; } - public string RelativePathToOpen { get; private set; } - public bool IsFileUri { get; private set; } - public bool IsValidUri - { - get { return url != null; } - } + public bool IsValidUri => url != null; - public Uri ToUri() + public Uri ToWebUri() { - if (url == null) - throw new InvalidOperationException("This Uri String is not a valid Uri"); + if (url == null || (url.Scheme != Uri.UriSchemeHttp && url.Scheme != Uri.UriSchemeHttps)) + { + return new UriBuilder + { + Scheme = Uri.UriSchemeHttps, + Host = Host, + Path = NameWithOwner, + Port = url?.Port == 80 + ? -1 + : (url?.Port ?? -1) + }.Uri; + } return url; } @@ -186,9 +152,7 @@ public static implicit operator UriString(string value) [SuppressMessage("Microsoft.Usage", "CA2225:OperatorOverloadsHaveNamedAlternates")] public static implicit operator string(UriString uriString) { - if (uriString == null) return null; - - return uriString.Value; + return uriString?.Value; } [SuppressMessage("Microsoft.Usage", "CA2234:PassSystemUriObjectsInsteadOfStrings", Justification = "No.")] @@ -197,7 +161,7 @@ public override UriString Combine(string addition) if (url != null) { var urlBuilder = new UriBuilder(url); - if (!String.IsNullOrEmpty(urlBuilder.Query)) + if (!IsNullOrEmpty(urlBuilder.Query)) { var query = urlBuilder.Query; if (query.StartsWith("?", StringComparison.Ordinal)) @@ -221,7 +185,7 @@ public override UriString Combine(string addition) } return ToUriString(urlBuilder.Uri); } - return String.Concat(Value, addition); + return Concat(Value, addition); } public override string ToString() @@ -253,12 +217,12 @@ static string GetSerializedValue(SerializationInfo info) static string NormalizePath(string path) { - return path == null ? null : path.Replace('\\', '/'); + return path?.Replace('\\', '/'); } static string GetRepositoryName(string repositoryNameSegment) { - if (String.IsNullOrEmpty(repositoryNameSegment) + if (IsNullOrEmpty(repositoryNameSegment) || repositoryNameSegment.Equals("/", StringComparison.Ordinal)) { return null; diff --git a/src/UnitTests/GitHub.Primitives/UriStringTests.cs b/src/UnitTests/GitHub.Primitives/UriStringTests.cs new file mode 100644 index 0000000000..4a7ca258d6 --- /dev/null +++ b/src/UnitTests/GitHub.Primitives/UriStringTests.cs @@ -0,0 +1,269 @@ +using System; +using System.Collections.Generic; +using GitHub.Primitives; +using Xunit; + +public class UriStringTests +{ + public class TheConstructor + { + [Theory] + [InlineData("http://192.168.1.3/foo/bar.git", "192.168.1.3", "foo", "bar")] + [InlineData("http://haacked@example.com/foo/bar", "example.com", "foo", "bar")] + [InlineData("http://haacked:password@example.com/foo/bar", "example.com", "foo", "bar")] + [InlineData("http://example.com/foo/bar.git", "example.com", "foo", "bar")] + [InlineData("http://example.com/foo/bar", "example.com", "foo", "bar")] + [InlineData("http://example.com:1234/foo/bar.git", "example.com", "foo", "bar")] + [InlineData("https://github.com/github/Windows.git", "github.com", "github", "Windows")] + [InlineData("https://github.com/libgit2/libgit2.github.com", "github.com", "libgit2", "libgit2.github.com")] + [InlineData("https://github.com/github/Windows", "github.com", "github", "Windows")] + [InlineData("git@192.168.1.2:github/Windows.git", "192.168.1.2", "github", "Windows")] + [InlineData("git@github.com:github/Windows.git", "github.com", "github", "Windows")] + [InlineData("git@github.com:github/Windows", "github.com", "github", "Windows")] + [InlineData("git@example.com:org/repo.git", "example.com", "org", "repo")] + [InlineData("ssh://git@github.com:443/benstraub/libgit2", "github.com", "benstraub", "libgit2")] + [InlineData("git://git@github.com:443/benstraub/libgit2", "github.com", "benstraub", "libgit2")] + [InlineData("jane;fingerprint=9e:1a:5e:27:16:4d:2a:13:90:2c:64:41:bd:25:fd:35@foo.com:github/Windows.git", + "foo.com", "github", "Windows")] + [InlineData("https://haacked@bitbucket.org/haacked/test-mytest.git", "bitbucket.org", "haacked", "test-mytest")] + [InlineData("https://git01.codeplex.com/nuget", "git01.codeplex.com", null, "nuget")] + [InlineData("https://example.com/vpath/foo/bar", "example.com", "foo", "bar")] + [InlineData("https://example.com/vpath/foo/bar.git", "example.com", "foo", "bar")] + [InlineData("https://github.com/github/Windows.git?pr=24&branch=pr/23&filepath=relative/to/the/path.md", + "github.com", "github", "Windows")] + public void ParsesWellFormedUrlComponents(string url, string expectedHost, string owner, string repositoryName) + { + var cloneUrl = new UriString(url); + + Assert.Equal(cloneUrl.Host, expectedHost); + Assert.Equal(cloneUrl.Owner, owner); + Assert.Equal(cloneUrl.RepositoryName, repositoryName); + Assert.False(cloneUrl.IsFileUri); + } + + [Theory] + [InlineData(@"..\bar\foo")] + [InlineData(@"..\..\foo")] + [InlineData(@"../..\foo")] + [InlineData(@"../../foo")] + [InlineData(@"..\..\foo.git")] + [InlineData(@"c:\dev\exp\foo")] + [InlineData(@"c:\dev\bar\..\exp\foo")] + [InlineData("c:/dev/exp/foo")] + [InlineData(@"c:\dev\exp\foo.git")] + [InlineData(@"c:\dev\exp\foo.git")] + [InlineData("c:/dev/exp/foo.git")] + [InlineData("c:/dev/exp/bar/../foo.git")] + [InlineData("file:///C:/dev/exp/foo")] + [InlineData("file://C:/dev/exp/foo")] + [InlineData("file://C:/dev/exp/foo.git")] + public void ParsesLocalFileUris(string path) + { + var cloneUrl = new UriString(path); + + Assert.Equal(cloneUrl.Host, ""); + Assert.Equal(cloneUrl.Owner, ""); + Assert.Equal(cloneUrl.RepositoryName, "foo"); + Assert.Equal(cloneUrl.ToString(), path.Replace('\\', '/')); + Assert.True(cloneUrl.IsFileUri); + } + + [Theory] + [InlineData("complete garbage", "", "", null)] + [InlineData(@"..\other_folder", "", "", "other_folder")] + [InlineData("http://example.com", "example.com", null, null)] + [InlineData("http://example.com?bar", "example.com", null, null)] + [InlineData("https://example.com?bar", "example.com", null, null)] + [InlineData("ssh://git@example.com/Windows.git", "example.com", null, "Windows")] + [InlineData("blah@bar.com:/", "bar.com", null, null)] + [InlineData("blah@bar.com/", "bar.com", null, null)] + [InlineData("blah@bar.com", "bar.com", null, null)] + [InlineData("blah@bar.com:/Windows.git", "bar.com", null, "Windows")] + [InlineData("blah@baz.com/Windows.git", "baz.com", null, "Windows")] + [InlineData("ssh://git@github.com:github/Windows.git", "github.com", "github", "Windows")] + [InlineData("ssh://git@example.com/Windows.git", "example.com", null, "Windows")] + public void ParsesWeirdUrlsAsWellAsPossible(string url, string expectedHost, string owner, string repositoryName) + { + var cloneUrl = new UriString(url); + + Assert.Equal(cloneUrl.Host, expectedHost); + Assert.Equal(cloneUrl.Owner, owner); + Assert.Equal(cloneUrl.RepositoryName, repositoryName); + } + + [Theory] + [InlineData(@"http:\\example.com/bar\baz")] + [InlineData(@"http://example.com/bar/baz")] + public void NormalizesSeparator(string uriString) + { + var path = new UriString(uriString); + new UriString("http://example.com/bar/baz").Equals(path); + } + + [Fact] + public void EnsuresArgumentNotNull() + { + Assert.Throws(() => new UriString(null)); + } + } + + public class TheNameWithOwnerProperty + { + [Theory] + [InlineData("http://192.168.1.3/foo/bar.git", "foo/bar")] + [InlineData("http://192.168.1.3/foo/bar", "foo/bar")] + [InlineData("http://192.168.1.3/foo/bar/baz/qux", "baz/qux")] + [InlineData("https://github.com/github/Windows.git", "github/Windows")] + [InlineData("https://github.com/github/", "github")] + [InlineData("blah@bar.com:/Windows.git", "Windows")] + [InlineData("git@github.com:github/Windows.git", "github/Windows")] + public void DependsOnOwnerAndRepoNameNotBeingNull(string url, string expectedNameWithOwner) + { + var cloneUrl = new UriString(url); + + Assert.Equal(cloneUrl.NameWithOwner, expectedNameWithOwner); + } + } + + public class TheCombineMethod + { + [Theory] + [InlineData("http://example.com", "foo/bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", "foo/bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", "/foo/bar/", @"http://example.com/foo/bar/")] + [InlineData("http://example.com/foo", "bar/", @"http://example.com/foo/bar/")] + [InlineData("http://example.com", @"foo\bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", @"foo\bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", @"/foo\bar/", @"http://example.com/foo/bar/")] + [InlineData("http://example.com/foo", @"bar\", @"http://example.com/foo/bar/")] + [InlineData("http://example.com/foo?bar", @"baz", @"http://example.com/foo?bar&baz")] + [InlineData("http://example.com/foo?bar", @"&baz", @"http://example.com/foo?bar&baz")] + [InlineData("http://example.com/foo?", @"bar", @"http://example.com/foo?bar")] + [InlineData("http://example.com/foo?", @"&bar", @"http://example.com/foo?&bar")] + public void ComparesHostInsensitively(string uriString, string path, string expected) + { + Assert.Equal(new UriString(uriString).Combine(path), (UriString)expected); + } + } + + public class TheIsValidUriProperty + { + [Theory] + [InlineData("http://example.com/", true)] + [InlineData("file:///C:/dev/exp/foo", true)] + [InlineData("garbage", false)] + [InlineData("git@192.168.1.2:github/Windows.git", false)] + public void ReturnWhetherTheUriIsParseableByUri(string uriString, bool expected) + { + Assert.Equal(new UriString(uriString).IsValidUri, expected); + } + } + + public class TheToUriMethod + { + [Theory] + [InlineData("http://example.com/", "http://example.com/")] + [InlineData("http://haacked@example.com/foo/bar", "http://example.com/foo/bar")] + [InlineData("https://github.com/github/Windows", "https://github.com/github/Windows")] + [InlineData("http://example.com:4000/github/Windows", "http://example.com:4000/github/Windows")] + [InlineData("git@192.168.1.2:github/Windows.git", "https://192.168.1.2/github/Windows")] + [InlineData("git@example.com:org/repo.git", "https://example.com/org/repo")] + [InlineData("ssh://git@github.com:443/shana/cef", "https://github.com/shana/cef")] + [InlineData("ssh://git@example.com:23/haacked/encourage", "https://example.com:23/haacked/encourage")] + public void ConvertsToWebUrl(string uriString, string expected) + { + Assert.Equal(new Uri(expected), new UriString(uriString).ToWebUri()); + } + } + + public class TheAdditionOperator + { + [Theory] + [InlineData("http://example.com", "foo/bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", "foo/bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", "/foo/bar/", @"http://example.com/foo/bar/")] + [InlineData("http://example.com/foo", "bar/", @"http://example.com/foo/bar/")] + [InlineData("http://example.com", @"foo\bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", @"foo\bar", @"http://example.com/foo/bar")] + [InlineData("http://example.com/", @"/foo\bar/", @"http://example.com/foo/bar/")] + [InlineData("http://example.com/foo", @"bar\", @"http://example.com/foo/bar/")] + [InlineData("http://example.com/foo?bar", @"baz", @"http://example.com/foo?bar&baz")] + [InlineData("http://example.com/foo?bar", @"&baz", @"http://example.com/foo?bar&baz")] + [InlineData("http://example.com/foo?", @"bar", @"http://example.com/foo?bar")] + [InlineData("http://example.com/foo?", @"&bar", @"http://example.com/foo?&bar")] + public void CombinesPaths(string uriString, string addition, string expected) + { + UriString path = uriString; + var newPath = path + addition; + Assert.Equal(newPath, (UriString)expected); + } + } + + public class ImplicitConversionToString + { + [Fact] + public void ConvertsBackToString() + { + var uri = new UriString("http://github.com/foo/bar/"); + string cloneUri = uri; + Assert.Equal(cloneUri, "http://github.com/foo/bar/"); + } + + [Fact] + public void ConvertsNullToNull() + { + UriString uri = null; + Assert.Null(uri); + string cloneUri = uri; + Assert.Null(cloneUri); + } + } + + public class ImplicitConversionFromString + { + [Fact] + public void ConvertsToCloneUri() + { + UriString cloneUri = "http://github.com/foo/bar/"; + Assert.Equal(cloneUri.Host, "github.com"); + } + + [Fact] + public void ConvertsNullToNull() + { + UriString cloneUri = (string)null; + Assert.Null(cloneUri); + } + } + + public class TheIsHypertextTransferProtocolProperty + { + [Theory] + [InlineData("http://example.com", true)] + [InlineData("HTTP://example.com", true)] + [InlineData("https://example.com", true)] + [InlineData("HTTPs://example.com", true)] + [InlineData("ftp://example.com", false)] + [InlineData("c:/example.com", false)] + [InlineData("git@github.com:github/Windows", false)] + public void IsTrueOnlyForHttpAndHttps(string url, bool expected) + { + var uri = new UriString(url); + Assert.Equal(uri.IsHypertextTransferProtocol, expected); + } + } + + public class TheEqualsMethod + { + [Theory] + [InlineData("https://github.com/foo/bar", "https://github.com/foo/bar", true)] + [InlineData("https://github.com/foo/bar", "https://github.com/foo/BAR", false)] + [InlineData("https://github.com/foo/bar", "https://github.com/foo/bar/", false)] + [InlineData("https://github.com/foo/bar", null, false)] + public void ReturnsTrueForCaseSensitiveEquality(string source, string compare, bool expected) + { + Assert.Equal(expected, source.Equals(compare)); + Assert.Equal(expected, EqualityComparer.Default.Equals(source, compare)); + } + } +} diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index 4a0ac80abc..1e1eb57700 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -142,6 +142,7 @@ + From 675c82c7d7382d2833c02766fee5e103a4b569df Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 09:44:37 -0700 Subject: [PATCH 02/14] Add method for appending relative paths to urls Sadly we can't just do this: new Uri(url, path); because you get different results if the url ends with "/" or not. So we need to make absolutely sure it _does_ end with "/". --- src/GitHub.Extensions/UriExtensions.cs | 16 ++++++++++++ .../GitHub.Extensions/UriExtensionTests.cs | 25 +++++++++++++++++++ src/UnitTests/UnitTests.csproj | 1 + 3 files changed, 42 insertions(+) create mode 100644 src/UnitTests/GitHub.Extensions/UriExtensionTests.cs diff --git a/src/GitHub.Extensions/UriExtensions.cs b/src/GitHub.Extensions/UriExtensions.cs index 2c53ab5233..7be534f112 100644 --- a/src/GitHub.Extensions/UriExtensions.cs +++ b/src/GitHub.Extensions/UriExtensions.cs @@ -5,6 +5,22 @@ namespace GitHub.Extensions { public static class UriExtensions { + /// + /// Appends a relative path to the URL. + /// + /// + /// The Uri constructor for combining relative URLs have a different behavior with URLs that end with / + /// than those that don't. + /// + public static Uri Append(this Uri uri, string relativePath) + { + if (!uri.AbsolutePath.EndsWith("/", StringComparison.Ordinal)) + { + uri = new Uri(uri + "/"); + } + return new Uri(uri, new Uri(relativePath, UriKind.Relative)); + } + public static bool IsHypertextTransferProtocol(this Uri uri) { return uri.Scheme == "http" || uri.Scheme == "https"; diff --git a/src/UnitTests/GitHub.Extensions/UriExtensionTests.cs b/src/UnitTests/GitHub.Extensions/UriExtensionTests.cs new file mode 100644 index 0000000000..43877869c4 --- /dev/null +++ b/src/UnitTests/GitHub.Extensions/UriExtensionTests.cs @@ -0,0 +1,25 @@ +using System; +using GitHub.Extensions; +using Xunit; + +public class UriExtensionTests +{ + public class TheAppendMethod + { + [Theory] + [InlineData("https://github.com/foo/bar", "graphs", "https://github.com/foo/bar/graphs")] + [InlineData("https://github.com/foo/bar/", "graphs", "https://github.com/foo/bar/graphs")] + [InlineData("https://github.com", "bippety/boppety", "https://github.com/bippety/boppety")] + [InlineData("https://github.com/", "bippety/boppety", "https://github.com/bippety/boppety")] + [InlineData("https://github.com/foo/bar", "bippety/boppety", "https://github.com/foo/bar/bippety/boppety")] + public void AppendsRelativePath(string url, string relativePath, string expected) + { + var uri = new Uri(url, UriKind.Absolute); + var expectedUri = new Uri(expected, UriKind.Absolute); + + var result = uri.Append(relativePath); + + Assert.Equal(expectedUri, result); + } + } +} diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index 1e1eb57700..e4a41fd4c0 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -141,6 +141,7 @@ + From 7cfd1a1673116331f26a48d43994eb7c4b18b6ea Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 09:49:31 -0700 Subject: [PATCH 03/14] :fire: Remove the ToHttps method We no longer need this. --- src/GitHub.Extensions/UriExtensions.cs | 24 --------------------- src/UnitTests/GitHub.Extensions/URITests.cs | 21 ------------------ 2 files changed, 45 deletions(-) diff --git a/src/GitHub.Extensions/UriExtensions.cs b/src/GitHub.Extensions/UriExtensions.cs index 7be534f112..aa57bbc5c0 100644 --- a/src/GitHub.Extensions/UriExtensions.cs +++ b/src/GitHub.Extensions/UriExtensions.cs @@ -38,30 +38,6 @@ public static Uri WithAbsolutePath(this Uri uri, string absolutePath) return new Uri(uri, new Uri(absolutePath, UriKind.Relative)); } - [return: AllowNull] - public static Uri ToHttps([AllowNull] this Uri uri) - { - if (uri == null) - return null; - - var str = uri.ToString(); - if (str.EndsWith(".git", StringComparison.Ordinal)) - str = str.Remove(str.Length - 4); - - if (str.StartsWith("git@github.com:", StringComparison.Ordinal)) - str = str.Replace("git@github.com:", "https://github.com/"); - - if (!Uri.TryCreate(str, UriKind.Absolute, out uri)) - return null; - - var uriBuilder = new UriBuilder(uri); - - uriBuilder.Scheme = Uri.UriSchemeHttps; - // trick to keep uriBuilder from explicitly appending :80 to the HTTPS URI - uriBuilder.Port = -1; - return uriBuilder.Uri; - } - public static string ToUpperInvariantString(this Uri uri) { return uri == null ? "" : uri.ToString().ToUpperInvariant(); diff --git a/src/UnitTests/GitHub.Extensions/URITests.cs b/src/UnitTests/GitHub.Extensions/URITests.cs index 0f43bf1927..f8bca209dc 100644 --- a/src/UnitTests/GitHub.Extensions/URITests.cs +++ b/src/UnitTests/GitHub.Extensions/URITests.cs @@ -1,7 +1,4 @@ using System; -using GitHub.Services; -using Microsoft.TeamFoundation.Git.Controls.Extensibility; -using NSubstitute; using Xunit; using GitHub.Extensions; @@ -28,22 +25,4 @@ public void OnlyParsesUrlsWithThreeParts(string uri, string expected) Assert.Equal(new Uri(uri).GetRepo(), expected); } } - - public class TheHttpsMethod : TestBaseClass - { - [Theory] - [InlineData(null, null)] - [InlineData("", null)] - [InlineData("git@github.com:bla/test.git", "https://github.com/bla/test")] - [InlineData("git://github.com/bla/test.git", "https://github.com/bla/test")] - public void CreatesHttpsUrl(string uri, string expected) - { - Uri value = null; - Uri.TryCreate(uri, UriKind.RelativeOrAbsolute, out value); - - var ret = value.ToHttps(); - var ret2 = ret != null ? ret.ToString() : null; - Assert.Equal(expected, ret2); - } - } } From f401897c1e374b5a0d73b15be5cae68d798afdd8 Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 10:01:22 -0700 Subject: [PATCH 04/14] Preserve HTTP scheme for enterprise repositories Fixes #13 We need to preserve the format of the origin for non github.com repositories (such as GitHub Enterprise instances). Many of them are not set up with HTTPS because they're behind a firewall etc. --- .../Services/ITeamExplorerServiceHolder.cs | 4 +++ src/GitHub.Exports/Services/Services.cs | 23 +++++--------- .../Base/TeamExplorerGitRepoInfo.cs | 3 ++ .../Base/TeamExplorerItemBase.cs | 13 +++----- .../Base/TeamExplorerNavigationItemBase.cs | 21 +++++++------ .../Home/GraphsNavigationItemTests.cs | 31 +++++++++++++++++++ src/UnitTests/UnitTests.csproj | 8 +++++ 7 files changed, 71 insertions(+), 32 deletions(-) create mode 100644 src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs diff --git a/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs b/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs index 7472ffc366..fb1db2cd27 100644 --- a/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs +++ b/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs @@ -46,6 +46,10 @@ public interface ITeamExplorerServiceHolder public interface IGitAwareItem { IGitRepositoryInfo ActiveRepo { get; } + + /// + /// Represents the web URL of the repository on GitHub.com, even if the origin is an SSH address. + /// Uri ActiveRepoUri { get; } string ActiveRepoName { get; } } diff --git a/src/GitHub.Exports/Services/Services.cs b/src/GitHub.Exports/Services/Services.cs index c903803074..a6ba7f68f6 100644 --- a/src/GitHub.Exports/Services/Services.cs +++ b/src/GitHub.Exports/Services/Services.cs @@ -12,6 +12,7 @@ using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; using GitHub.Models; using GitHub.Info; +using GitHub.Primitives; namespace GitHub.VisualStudio { @@ -127,7 +128,7 @@ public static Uri GetRepoUrlFromSolution(IVsSolution solution) return null; using (var repo = new Repository(repoPath)) { - return GetUriFromRepository(repo); + return GetUriFromRepository(repo)?.ToWebUri(); } } @@ -144,21 +145,13 @@ public static Repository GetRepoFromSolution(this IVsSolution solution) return new Repository(repoPath); } - public static Uri GetUriFromRepository(Repository repo) + public static UriString GetUriFromRepository(Repository repo) { - if (repo == null) - return null; - var remote = repo.Network.Remotes.FirstOrDefault(x => x.Name.Equals("origin", StringComparison.Ordinal)); - if (remote == null) - return null; - Uri uri; - var url = remote.Url; - // fixup ssh urls - if (url.StartsWith("git@github.com:", StringComparison.Ordinal)) - url = url.Replace("git@github.com:", "https://github.com/"); - if (!Uri.TryCreate(url, UriKind.Absolute, out uri)) - return null; - return uri; + return repo + ?.Network + .Remotes + .FirstOrDefault(x => x.Name.Equals("origin", StringComparison.Ordinal)) + ?.Url; } public static Repository GetRepoFromIGit(this IGitRepositoryInfo repoInfo) diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs b/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs index bcbe0c4fe3..b557154802 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs @@ -27,6 +27,9 @@ public IGitRepositoryInfo ActiveRepo } } + /// + /// Represents the web URL of the repository on GitHub.com, even if the origin is an SSH address. + /// [AllowNull] public Uri ActiveRepoUri { [return: AllowNull] get; set; } public string ActiveRepoName { get; set; } diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs index a5dedc599e..ec9ae0dbb1 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs @@ -49,16 +49,13 @@ protected virtual void RepoChanged() var repo = ActiveRepo; if (repo != null) { - var gitRepo = Services.GetRepoFromIGit(repo); + var gitRepo = repo.GetRepoFromIGit(); var uri = Services.GetUriFromRepository(gitRepo); - if (uri != null) + var name = uri.RepositoryName; + if (name != null) { - var name = uri.GetRepo(); - if (name != null) - { - ActiveRepoUri = uri; - ActiveRepoName = ActiveRepoUri.GetUser() + "/" + ActiveRepoUri.GetRepo(); - } + ActiveRepoUri = uri.ToWebUri(); + ActiveRepoName = uri.NameWithOwner; } } } diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs index 670949a76b..ebd38727af 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs @@ -28,7 +28,14 @@ public TeamExplorerNavigationItemBase(ISimpleApiClientFactory apiFactory, ITeamE IsVisible = false; IsEnabled = true; - OnThemeChanged(); + try + { + OnThemeChanged(); + } + catch (ArgumentNullException) + { + // This throws in the unit test runner. + } VSColorTheme.ThemeChanged += _ => { OnThemeChanged(); @@ -63,17 +70,13 @@ protected void OpenInBrowser(Lazy browser, string endpoint { var uri = ActiveRepoUri; Debug.Assert(uri != null, "OpenInBrowser: uri should never be null"); +#if !DEBUG if (uri == null) return; +#endif + var browseUrl = uri.Append(endpoint); - var https = uri.ToHttps(); - if (https == null) - return; - - if (!Uri.TryCreate(https.ToString() + "/" + endpoint, UriKind.Absolute, out uri)) - return; - - OpenInBrowser(browser, uri); + OpenInBrowser(browser, browseUrl); } void Unsubscribe() diff --git a/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs b/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs new file mode 100644 index 0000000000..388e3c0920 --- /dev/null +++ b/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs @@ -0,0 +1,31 @@ +using System; +using GitHub.Api; +using GitHub.Services; +using GitHub.VisualStudio.TeamExplorer.Home; +using NSubstitute; +using Xunit; + +public class GraphsNavigationItemTests +{ + public class TheExecuteMethod + { + [Theory] + [InlineData("https://github.com/foo/bar", "https://github.com/foo/bar/graphs")] + [InlineData("https://github.com/foo/bar/", "https://github.com/foo/bar/graphs")] + public void BrowsesToTheCorrectURL(string origin, string expectedUrl) + { + var apiFactory = Substitute.For(); + var browser = Substitute.For(); + var lazyBrowser = new Lazy(() => browser); + var holder = Substitute.For(); + var graphsNavigationItem = new GraphsNavigationItem(apiFactory, lazyBrowser, holder) + { + ActiveRepoUri = new Uri(origin) + }; + + graphsNavigationItem.Execute(); + + browser.Received().OpenUrl(new Uri(expectedUrl)); + } + } +} diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index e4a41fd4c0..f223553acd 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -42,10 +42,17 @@ ..\..\packages\Rx-Testing.2.2.5-custom\lib\net45\Microsoft.Reactive.Testing.dll True + + False + ..\..\lib\Microsoft.TeamFoundation.Controls.dll + False ..\..\lib\Microsoft.TeamFoundation.Git.Controls.dll + + ..\..\lib\Microsoft.TeamFoundation.Git.Provider.dll + @@ -147,6 +154,7 @@ + From ac526ca1a7b9e16b51c82eab8329a48bbd8cd7a0 Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 10:40:30 -0700 Subject: [PATCH 05/14] Use UriString and remove duplicate parsing logic We have some places where we used custom parsing logic for extracting owner and repo from a repository URL. Instead, we should use the robust logic from UriString. --- src/GitHub.Api/SimpleApiClient.cs | 17 ++++--- src/GitHub.Api/SimpleApiClientFactory.cs | 16 +++---- src/GitHub.Exports/Api/ISimpleApiClient.cs | 6 +-- .../Api/ISimpleApiClientFactory.cs | 4 +- .../Services/ITeamExplorerServiceHolder.cs | 7 +-- src/GitHub.Extensions/UriExtensions.cs | 44 +------------------ .../Base/TeamExplorerGitRepoInfo.cs | 5 +-- .../Base/TeamExplorerItemBase.cs | 14 +++--- .../Base/TeamExplorerNavigationItemBase.cs | 2 +- src/UnitTests/GitHub.Extensions/URITests.cs | 28 ------------ .../GitHub.Primitives/UriStringTests.cs | 13 ++++++ .../Home/GraphsNavigationItemTests.cs | 2 +- src/UnitTests/UnitTests.csproj | 1 - 13 files changed, 48 insertions(+), 111 deletions(-) delete mode 100644 src/UnitTests/GitHub.Extensions/URITests.cs diff --git a/src/GitHub.Api/SimpleApiClient.cs b/src/GitHub.Api/SimpleApiClient.cs index 34a17d3030..f044d60176 100644 --- a/src/GitHub.Api/SimpleApiClient.cs +++ b/src/GitHub.Api/SimpleApiClient.cs @@ -2,7 +2,6 @@ using System.Diagnostics; using System.Threading; using System.Threading.Tasks; -using GitHub.Extensions; using GitHub.Primitives; using GitHub.Services; using Octokit; @@ -11,8 +10,8 @@ namespace GitHub.Api { public class SimpleApiClient : ISimpleApiClient { - public HostAddress HostAddress { get; private set; } - public Uri OriginalUrl { get; private set; } + public HostAddress HostAddress { get; } + public UriString OriginalUrl { get; } readonly GitHubClient client; readonly Lazy enterpriseProbe; @@ -24,7 +23,7 @@ public class SimpleApiClient : ISimpleApiClient bool? isEnterprise; bool? hasWiki; - internal SimpleApiClient(HostAddress hostAddress, Uri repoUrl, GitHubClient githubClient, + internal SimpleApiClient(HostAddress hostAddress, UriString repoUrl, GitHubClient githubClient, Lazy enterpriseProbe, Lazy wikiProbe) { HostAddress = hostAddress; @@ -51,19 +50,19 @@ async Task GetRepositoryInternal() { if (owner == null && OriginalUrl != null) { - var own = OriginalUrl.GetUser(); - var name = OriginalUrl.GetRepo(); + var ownerLogin = OriginalUrl.Owner; + var repositoryName = OriginalUrl.RepositoryName; - if (own != null && name != null) + if (ownerLogin != null && repositoryName != null) { - var repo = await client.Repository.Get(own, name); + var repo = await client.Repository.Get(ownerLogin, repositoryName); if (repo != null) { hasWiki = await HasWikiInternal(repo); isEnterprise = await IsEnterpriseInternal(); repositoryCache = repo; } - owner = own; + owner = ownerLogin; } } } diff --git a/src/GitHub.Api/SimpleApiClientFactory.cs b/src/GitHub.Api/SimpleApiClientFactory.cs index 2d5ba4dea1..c203244084 100644 --- a/src/GitHub.Api/SimpleApiClientFactory.cs +++ b/src/GitHub.Api/SimpleApiClientFactory.cs @@ -16,7 +16,7 @@ public class SimpleApiClientFactory : ISimpleApiClientFactory readonly Lazy lazyEnterpriseProbe; readonly Lazy lazyWikiProbe; - static readonly Dictionary cache = new Dictionary(); + static readonly Dictionary cache = new Dictionary(); [ImportingConstructor] public SimpleApiClientFactory(IProgram program, Lazy enterpriseProbe, Lazy wikiProbe) @@ -26,21 +26,21 @@ public SimpleApiClientFactory(IProgram program, Lazy enter lazyWikiProbe = wikiProbe; } - public ISimpleApiClient Create(Uri repoUrl) + public ISimpleApiClient Create(UriString repositoryUrl) { - var contains = cache.ContainsKey(repoUrl); + var contains = cache.ContainsKey(repositoryUrl); if (contains) - return cache[repoUrl]; + return cache[repositoryUrl]; lock (cache) { - if (!cache.ContainsKey(repoUrl)) + if (!cache.ContainsKey(repositoryUrl)) { - var hostAddress = HostAddress.Create(repoUrl); + var hostAddress = HostAddress.Create(repositoryUrl); var apiBaseUri = hostAddress.ApiUri; - cache.Add(repoUrl, new SimpleApiClient(hostAddress, repoUrl, new GitHubClient(productHeader, new SimpleCredentialStore(hostAddress), apiBaseUri), lazyEnterpriseProbe, lazyWikiProbe)); + cache.Add(repositoryUrl, new SimpleApiClient(hostAddress, repositoryUrl, new GitHubClient(productHeader, new SimpleCredentialStore(hostAddress), apiBaseUri), lazyEnterpriseProbe, lazyWikiProbe)); } - return cache[repoUrl]; + return cache[repositoryUrl]; } } diff --git a/src/GitHub.Exports/Api/ISimpleApiClient.cs b/src/GitHub.Exports/Api/ISimpleApiClient.cs index a7e242962e..f8e89da9fc 100644 --- a/src/GitHub.Exports/Api/ISimpleApiClient.cs +++ b/src/GitHub.Exports/Api/ISimpleApiClient.cs @@ -1,7 +1,5 @@ -using System; -using System.Threading.Tasks; +using System.Threading.Tasks; using GitHub.Primitives; -using GitHub.Services; using Octokit; namespace GitHub.Api @@ -9,7 +7,7 @@ namespace GitHub.Api public interface ISimpleApiClient { HostAddress HostAddress { get; } - Uri OriginalUrl { get; } + UriString OriginalUrl { get; } Task GetRepository(); bool HasWiki(); bool IsEnterprise(); diff --git a/src/GitHub.Exports/Api/ISimpleApiClientFactory.cs b/src/GitHub.Exports/Api/ISimpleApiClientFactory.cs index 9a3c69f06b..ba4f3b92e3 100644 --- a/src/GitHub.Exports/Api/ISimpleApiClientFactory.cs +++ b/src/GitHub.Exports/Api/ISimpleApiClientFactory.cs @@ -1,10 +1,10 @@ -using System; +using GitHub.Primitives; namespace GitHub.Api { public interface ISimpleApiClientFactory { - ISimpleApiClient Create(Uri repoUrl); + ISimpleApiClient Create(UriString repositoryUrl); void ClearFromCache(ISimpleApiClient client); } } diff --git a/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs b/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs index fb1db2cd27..7356dc1b7f 100644 --- a/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs +++ b/src/GitHub.Exports/Services/ITeamExplorerServiceHolder.cs @@ -1,5 +1,6 @@ -using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; -using System; +using System; +using GitHub.Primitives; +using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; namespace GitHub.Services { @@ -50,7 +51,7 @@ public interface IGitAwareItem /// /// Represents the web URL of the repository on GitHub.com, even if the origin is an SSH address. /// - Uri ActiveRepoUri { get; } + UriString ActiveRepoUri { get; } string ActiveRepoName { get; } } } diff --git a/src/GitHub.Extensions/UriExtensions.cs b/src/GitHub.Extensions/UriExtensions.cs index aa57bbc5c0..78976dd2d7 100644 --- a/src/GitHub.Extensions/UriExtensions.cs +++ b/src/GitHub.Extensions/UriExtensions.cs @@ -1,5 +1,4 @@ -using NullGuard; -using System; +using System; namespace GitHub.Extensions { @@ -30,46 +29,5 @@ public static bool IsSameHost(this Uri uri, Uri compareUri) { return uri.Host.Equals(compareUri.Host, StringComparison.OrdinalIgnoreCase); } - - public static Uri WithAbsolutePath(this Uri uri, string absolutePath) - { - absolutePath = absolutePath.EnsureStartsWith('/'); - - return new Uri(uri, new Uri(absolutePath, UriKind.Relative)); - } - - public static string ToUpperInvariantString(this Uri uri) - { - return uri == null ? "" : uri.ToString().ToUpperInvariant(); - } - - [return: AllowNull] - public static string GetUser(this Uri uri) - { - var parts = uri.Segments; - // only parse urls in the format domain/user/repo - if (parts.Length < 3) - return null; - var u = parts[1]; - if (u == null) - return null; - u = u.TrimEnd('/'); - return u; - } - - [return: AllowNull] - public static string GetRepo(this Uri uri) - { - var parts = uri.Segments; - // only parse urls in the format domain/user/repo - if (parts.Length < 3) - return null; - var name = parts[2]; - if (name == null) - return null; - if (name.EndsWith(".git", StringComparison.Ordinal)) - name = name.Remove(name.Length - 4); - return name; - } } } diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs b/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs index b557154802..9a75cb4efa 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerGitRepoInfo.cs @@ -1,8 +1,7 @@ -using GitHub.Api; +using GitHub.Primitives; using GitHub.Services; using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; using NullGuard; -using System; namespace GitHub.VisualStudio.Base { @@ -31,7 +30,7 @@ public IGitRepositoryInfo ActiveRepo /// Represents the web URL of the repository on GitHub.com, even if the origin is an SSH address. /// [AllowNull] - public Uri ActiveRepoUri { [return: AllowNull] get; set; } + public UriString ActiveRepoUri { [return: AllowNull] get; set; } public string ActiveRepoName { get; set; } } } diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs index ec9ae0dbb1..effb4dfb20 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs @@ -1,14 +1,12 @@ using System.ComponentModel.Composition; +using System.Threading.Tasks; +using GitHub.Api; using GitHub.Models; +using GitHub.Primitives; +using GitHub.Services; using GitHub.VisualStudio.Helpers; using NullGuard; using Octokit; -using System; -using GitHub.Services; -using GitHub.Api; -using System.Threading.Tasks; -using GitHub.Primitives; -using GitHub.Extensions; namespace GitHub.VisualStudio.Base { @@ -54,7 +52,7 @@ protected virtual void RepoChanged() var name = uri.RepositoryName; if (name != null) { - ActiveRepoUri = uri.ToWebUri(); + ActiveRepoUri = uri; ActiveRepoName = uri.NameWithOwner; } } @@ -68,7 +66,7 @@ protected async Task IsAGitHubRepo() SimpleApiClient = apiFactory.Create(uri); - if (!HostAddress.IsGitHubDotComUri(uri)) + if (!HostAddress.IsGitHubDotComUri(uri.ToWebUri())) { var repo = await SimpleApiClient.GetRepository(); return repo.FullName == ActiveRepoName && SimpleApiClient.IsEnterprise(); diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs index ebd38727af..1432022fc8 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs @@ -74,7 +74,7 @@ protected void OpenInBrowser(Lazy browser, string endpoint if (uri == null) return; #endif - var browseUrl = uri.Append(endpoint); + var browseUrl = uri.ToWebUri().Append(endpoint); OpenInBrowser(browser, browseUrl); } diff --git a/src/UnitTests/GitHub.Extensions/URITests.cs b/src/UnitTests/GitHub.Extensions/URITests.cs deleted file mode 100644 index f8bca209dc..0000000000 --- a/src/UnitTests/GitHub.Extensions/URITests.cs +++ /dev/null @@ -1,28 +0,0 @@ -using System; -using Xunit; -using GitHub.Extensions; - -public class URITests -{ - public class TheGetUserMethod : TestBaseClass - { - [Theory] - [InlineData("git://test/athing", null)] - [InlineData("git://test/user/athing", "user")] - public void OnlyParsesUrlsWithThreeParts(string uri, string expected) - { - Assert.Equal(new Uri(uri).GetUser(), expected); - } - } - - public class TheGetRepoMethod : TestBaseClass - { - [Theory] - [InlineData("git://test/athing", null)] - [InlineData("git://test/user/athing", "athing")] - public void OnlyParsesUrlsWithThreeParts(string uri, string expected) - { - Assert.Equal(new Uri(uri).GetRepo(), expected); - } - } -} diff --git a/src/UnitTests/GitHub.Primitives/UriStringTests.cs b/src/UnitTests/GitHub.Primitives/UriStringTests.cs index 4a7ca258d6..3a7f12bf4c 100644 --- a/src/UnitTests/GitHub.Primitives/UriStringTests.cs +++ b/src/UnitTests/GitHub.Primitives/UriStringTests.cs @@ -265,5 +265,18 @@ public void ReturnsTrueForCaseSensitiveEquality(string source, string compare, b Assert.Equal(expected, source.Equals(compare)); Assert.Equal(expected, EqualityComparer.Default.Equals(source, compare)); } + + [Fact] + public void MakesUriStringSuitableForDictionaryKey() + { + var dictionary = new Dictionary + { + { new UriString("https://github.com/foo/bar"), "whatever" } + }; + + Assert.False(dictionary.ContainsKey("https://github.com/foo/not-bar")); + Assert.True(dictionary.ContainsKey("https://github.com/foo/bar")); + Assert.True(dictionary.ContainsKey(new UriString("https://github.com/foo/bar"))); + } } } diff --git a/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs b/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs index 388e3c0920..bd721331ff 100644 --- a/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs +++ b/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs @@ -20,7 +20,7 @@ public void BrowsesToTheCorrectURL(string origin, string expectedUrl) var holder = Substitute.For(); var graphsNavigationItem = new GraphsNavigationItem(apiFactory, lazyBrowser, holder) { - ActiveRepoUri = new Uri(origin) + ActiveRepoUri = origin }; graphsNavigationItem.Execute(); diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index f223553acd..db7766e630 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -149,7 +149,6 @@ - From b5249aba0fa3844d938f81f20968c2acf6718e83 Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 10:41:11 -0700 Subject: [PATCH 06/14] Include AccountCacheItemTests I noticed this was no longer included in the project. Probably should be. --- src/UnitTests/UnitTests.csproj | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index db7766e630..74242ec150 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -147,6 +147,7 @@ + @@ -257,9 +258,7 @@ - - - + From 4f726d97eb7aef01614e0dc2da25012c93aab3e4 Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 10:42:26 -0700 Subject: [PATCH 07/14] Include this in the project too. --- src/UnitTests/UnitTests.csproj | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/UnitTests/UnitTests.csproj b/src/UnitTests/UnitTests.csproj index 74242ec150..05d71b9caa 100644 --- a/src/UnitTests/UnitTests.csproj +++ b/src/UnitTests/UnitTests.csproj @@ -258,7 +258,9 @@ - + + + From f09fb86c6712b602aa80d0ff10e34c46b9964cf7 Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 13:44:00 -0700 Subject: [PATCH 08/14] ToWebUrl needs to normalize URLs --- src/GitHub.Exports/Primitives/UriString.cs | 24 +++++++++---------- .../GitHub.Primitives/UriStringTests.cs | 2 ++ .../Home/GraphsNavigationItemTests.cs | 2 ++ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/GitHub.Exports/Primitives/UriString.cs b/src/GitHub.Exports/Primitives/UriString.cs index 92a66efe99..d13399253e 100644 --- a/src/GitHub.Exports/Primitives/UriString.cs +++ b/src/GitHub.Exports/Primitives/UriString.cs @@ -121,19 +121,19 @@ bool ParseScpSyntax(string scpString) public Uri ToWebUri() { - if (url == null || (url.Scheme != Uri.UriSchemeHttp && url.Scheme != Uri.UriSchemeHttps)) + var scheme = url != null && IsHypertextTransferProtocol + ? url.Scheme + : Uri.UriSchemeHttps; + + return new UriBuilder { - return new UriBuilder - { - Scheme = Uri.UriSchemeHttps, - Host = Host, - Path = NameWithOwner, - Port = url?.Port == 80 - ? -1 - : (url?.Port ?? -1) - }.Uri; - } - return url; + Scheme = scheme, + Host = Host, + Path = NameWithOwner, + Port = url?.Port == 80 + ? -1 + : (url?.Port ?? -1) + }.Uri; } /// diff --git a/src/UnitTests/GitHub.Primitives/UriStringTests.cs b/src/UnitTests/GitHub.Primitives/UriStringTests.cs index 3a7f12bf4c..468096bba1 100644 --- a/src/UnitTests/GitHub.Primitives/UriStringTests.cs +++ b/src/UnitTests/GitHub.Primitives/UriStringTests.cs @@ -165,6 +165,8 @@ public class TheToUriMethod [InlineData("http://example.com/", "http://example.com/")] [InlineData("http://haacked@example.com/foo/bar", "http://example.com/foo/bar")] [InlineData("https://github.com/github/Windows", "https://github.com/github/Windows")] + [InlineData("https://github.com/github/Windows.git", "https://github.com/github/Windows")] + [InlineData("https://haacked@github.com/github/Windows.git", "https://github.com/github/Windows")] [InlineData("http://example.com:4000/github/Windows", "http://example.com:4000/github/Windows")] [InlineData("git@192.168.1.2:github/Windows.git", "https://192.168.1.2/github/Windows")] [InlineData("git@example.com:org/repo.git", "https://example.com/org/repo")] diff --git a/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs b/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs index bd721331ff..275f5d55b0 100644 --- a/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs +++ b/src/UnitTests/GitHub.VisualStudio/TeamExplorer/Home/GraphsNavigationItemTests.cs @@ -10,6 +10,8 @@ public class GraphsNavigationItemTests public class TheExecuteMethod { [Theory] + [InlineData("https://github.com/foo/bar.git", "https://github.com/foo/bar/graphs")] + [InlineData("https://haacked@github.com/foo/bar.git", "https://github.com/foo/bar/graphs")] [InlineData("https://github.com/foo/bar", "https://github.com/foo/bar/graphs")] [InlineData("https://github.com/foo/bar/", "https://github.com/foo/bar/graphs")] public void BrowsesToTheCorrectURL(string origin, string expectedUrl) From 5a01995e326618c4b87e734735acc1853553de2f Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 13:46:03 -0700 Subject: [PATCH 09/14] :art: Rename ToWebUri to ToRepositoryUrl This method name makes more sense. --- src/GitHub.Exports/Primitives/UriString.cs | 6 +++++- src/GitHub.Exports/Services/Services.cs | 2 +- src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs | 2 +- .../Base/TeamExplorerNavigationItemBase.cs | 2 +- src/UnitTests/GitHub.Primitives/UriStringTests.cs | 4 ++-- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/GitHub.Exports/Primitives/UriString.cs b/src/GitHub.Exports/Primitives/UriString.cs index d13399253e..f708c96e71 100644 --- a/src/GitHub.Exports/Primitives/UriString.cs +++ b/src/GitHub.Exports/Primitives/UriString.cs @@ -119,7 +119,11 @@ bool ParseScpSyntax(string scpString) public bool IsValidUri => url != null; - public Uri ToWebUri() + /// + /// Attempts a best-effort to convert the remote origin to a GitHub Repository URL. + /// + /// + public Uri ToRepositoryUrl() { var scheme = url != null && IsHypertextTransferProtocol ? url.Scheme diff --git a/src/GitHub.Exports/Services/Services.cs b/src/GitHub.Exports/Services/Services.cs index a6ba7f68f6..97c15f24d9 100644 --- a/src/GitHub.Exports/Services/Services.cs +++ b/src/GitHub.Exports/Services/Services.cs @@ -128,7 +128,7 @@ public static Uri GetRepoUrlFromSolution(IVsSolution solution) return null; using (var repo = new Repository(repoPath)) { - return GetUriFromRepository(repo)?.ToWebUri(); + return GetUriFromRepository(repo)?.ToRepositoryUrl(); } } diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs index effb4dfb20..ec14926bbe 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerItemBase.cs @@ -66,7 +66,7 @@ protected async Task IsAGitHubRepo() SimpleApiClient = apiFactory.Create(uri); - if (!HostAddress.IsGitHubDotComUri(uri.ToWebUri())) + if (!HostAddress.IsGitHubDotComUri(uri.ToRepositoryUrl())) { var repo = await SimpleApiClient.GetRepository(); return repo.FullName == ActiveRepoName && SimpleApiClient.IsEnterprise(); diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs index 1432022fc8..2bf1b5bbcc 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs @@ -74,7 +74,7 @@ protected void OpenInBrowser(Lazy browser, string endpoint if (uri == null) return; #endif - var browseUrl = uri.ToWebUri().Append(endpoint); + var browseUrl = uri.ToRepositoryUrl().Append(endpoint); OpenInBrowser(browser, browseUrl); } diff --git a/src/UnitTests/GitHub.Primitives/UriStringTests.cs b/src/UnitTests/GitHub.Primitives/UriStringTests.cs index 468096bba1..488ed19db2 100644 --- a/src/UnitTests/GitHub.Primitives/UriStringTests.cs +++ b/src/UnitTests/GitHub.Primitives/UriStringTests.cs @@ -159,7 +159,7 @@ public void ReturnWhetherTheUriIsParseableByUri(string uriString, bool expected) } } - public class TheToUriMethod + public class TheToRepositoryUrlMethod { [Theory] [InlineData("http://example.com/", "http://example.com/")] @@ -174,7 +174,7 @@ public class TheToUriMethod [InlineData("ssh://git@example.com:23/haacked/encourage", "https://example.com:23/haacked/encourage")] public void ConvertsToWebUrl(string uriString, string expected) { - Assert.Equal(new Uri(expected), new UriString(uriString).ToWebUri()); + Assert.Equal(new Uri(expected), new UriString(uriString).ToRepositoryUrl()); } } From 56add349cef5b1022e95c2c5a01cc8bf47b2530d Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 13:50:33 -0700 Subject: [PATCH 10/14] Make sure we don't crash with file remote origin --- src/GitHub.Exports/Primitives/UriString.cs | 2 ++ src/UnitTests/GitHub.Primitives/UriStringTests.cs | 1 + 2 files changed, 3 insertions(+) diff --git a/src/GitHub.Exports/Primitives/UriString.cs b/src/GitHub.Exports/Primitives/UriString.cs index f708c96e71..6af894e505 100644 --- a/src/GitHub.Exports/Primitives/UriString.cs +++ b/src/GitHub.Exports/Primitives/UriString.cs @@ -125,6 +125,8 @@ bool ParseScpSyntax(string scpString) /// public Uri ToRepositoryUrl() { + if (url != null && IsFileUri) return url; + var scheme = url != null && IsHypertextTransferProtocol ? url.Scheme : Uri.UriSchemeHttps; diff --git a/src/UnitTests/GitHub.Primitives/UriStringTests.cs b/src/UnitTests/GitHub.Primitives/UriStringTests.cs index 488ed19db2..c2b4783ef5 100644 --- a/src/UnitTests/GitHub.Primitives/UriStringTests.cs +++ b/src/UnitTests/GitHub.Primitives/UriStringTests.cs @@ -162,6 +162,7 @@ public void ReturnWhetherTheUriIsParseableByUri(string uriString, bool expected) public class TheToRepositoryUrlMethod { [Theory] + [InlineData("file:///C:/dev/exp/foo", "file:///C:/dev/exp/foo")] [InlineData("http://example.com/", "http://example.com/")] [InlineData("http://haacked@example.com/foo/bar", "http://example.com/foo/bar")] [InlineData("https://github.com/github/Windows", "https://github.com/github/Windows")] From 2232405cf7ab6d02146a44763531ca035f493d53 Mon Sep 17 00:00:00 2001 From: Haacked Date: Thu, 30 Jul 2015 15:11:54 -0700 Subject: [PATCH 11/14] Explicitly call string static methods Let's not go overboard with C# 6 --- src/GitHub.Exports/Primitives/UriString.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/GitHub.Exports/Primitives/UriString.cs b/src/GitHub.Exports/Primitives/UriString.cs index 6af894e505..4ff21457b1 100644 --- a/src/GitHub.Exports/Primitives/UriString.cs +++ b/src/GitHub.Exports/Primitives/UriString.cs @@ -5,7 +5,6 @@ using System.Runtime.Serialization; using System.Text.RegularExpressions; using GitHub.Extensions; -using static System.String; namespace GitHub.Primitives { @@ -47,7 +46,7 @@ public UriString(string uriString) : base(NormalizePath(uriString)) if (RepositoryName != null) { NameWithOwner = Owner != null - ? Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName) + ? string.Format(CultureInfo.InvariantCulture, "{0}/{1}", Owner, RepositoryName) : RepositoryName; } } @@ -167,7 +166,7 @@ public override UriString Combine(string addition) if (url != null) { var urlBuilder = new UriBuilder(url); - if (!IsNullOrEmpty(urlBuilder.Query)) + if (!string.IsNullOrEmpty(urlBuilder.Query)) { var query = urlBuilder.Query; if (query.StartsWith("?", StringComparison.Ordinal)) @@ -191,7 +190,7 @@ public override UriString Combine(string addition) } return ToUriString(urlBuilder.Uri); } - return Concat(Value, addition); + return string.Concat(Value, addition); } public override string ToString() @@ -228,7 +227,7 @@ static string NormalizePath(string path) static string GetRepositoryName(string repositoryNameSegment) { - if (IsNullOrEmpty(repositoryNameSegment) + if (string.IsNullOrEmpty(repositoryNameSegment) || repositoryNameSegment.Equals("/", StringComparison.Ordinal)) { return null; From 53ba917f8ce55d45bb076a756c071c1ae43fe16c Mon Sep 17 00:00:00 2001 From: Haacked Date: Fri, 31 Jul 2015 14:47:38 -0700 Subject: [PATCH 12/14] This can be made readonly --- src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs index 2bf1b5bbcc..fb0e6c89bb 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs @@ -18,7 +18,7 @@ namespace GitHub.VisualStudio.Base { public class TeamExplorerNavigationItemBase : TeamExplorerItemBase, ITeamExplorerNavigationItem2, INotifyPropertySource { - Octicon octicon; + readonly Octicon octicon; public TeamExplorerNavigationItemBase(ISimpleApiClientFactory apiFactory, ITeamExplorerServiceHolder holder, Octicon octicon) : base(apiFactory, holder) From dc96ded5ee945757093451fd79e0bed2aff04907 Mon Sep 17 00:00:00 2001 From: Haacked Date: Fri, 31 Jul 2015 14:48:07 -0700 Subject: [PATCH 13/14] :art: Sort and remove unused usings --- .../Base/TeamExplorerNavigationItemBase.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs index fb0e6c89bb..3db6324176 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs @@ -1,18 +1,15 @@ using System; using System.Diagnostics; -using System.Threading.Tasks; +using System.Drawing; using GitHub.Api; -using GitHub.Primitives; +using GitHub.Extensions; using GitHub.Services; +using GitHub.UI; using GitHub.VisualStudio.Helpers; using Microsoft.TeamFoundation.Controls; -using NullGuard; -using GitHub.Extensions; -using System.Threading; -using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; -using GitHub.UI; using Microsoft.VisualStudio.PlatformUI; -using System.Drawing; +using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility; +using NullGuard; namespace GitHub.VisualStudio.Base { From 59bb54260f62c6f09c91baa85414aa843fca2782 Mon Sep 17 00:00:00 2001 From: Haacked Date: Fri, 31 Jul 2015 14:48:22 -0700 Subject: [PATCH 14/14] Move exception handling inside the method --- .../Base/TeamExplorerNavigationItemBase.cs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs index 3db6324176..dd3abd0aad 100644 --- a/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs +++ b/src/GitHub.VisualStudio/Base/TeamExplorerNavigationItemBase.cs @@ -25,14 +25,7 @@ public TeamExplorerNavigationItemBase(ISimpleApiClientFactory apiFactory, ITeamE IsVisible = false; IsEnabled = true; - try - { - OnThemeChanged(); - } - catch (ArgumentNullException) - { - // This throws in the unit test runner. - } + OnThemeChanged(); VSColorTheme.ThemeChanged += _ => { OnThemeChanged(); @@ -49,11 +42,18 @@ public override async void Invalidate() void OnThemeChanged() { - var color = VSColorTheme.GetThemedColor(EnvironmentColors.ToolWindowTextColorKey); - var brightness = color.GetBrightness(); - var dark = brightness > 0.5f; + try + { + var color = VSColorTheme.GetThemedColor(EnvironmentColors.ToolWindowTextColorKey); + var brightness = color.GetBrightness(); + var dark = brightness > 0.5f; - Icon = SharedResources.GetDrawingForIcon(octicon, dark ? Colors.DarkThemeNavigationItem : Colors.LightThemeNavigationItem, dark ? "dark" : "light"); + Icon = SharedResources.GetDrawingForIcon(octicon, dark ? Colors.DarkThemeNavigationItem : Colors.LightThemeNavigationItem, dark ? "dark" : "light"); + } + catch (ArgumentNullException) + { + // This throws in the unit test runner. + } } void UpdateRepo(IGitRepositoryInfo repo)