Refactor repository models (repository refactor part 1)

src/GitHub.App/GlobalSuppressions.cs

@@ -6,6 +6,8 @@
 [assembly: SuppressMessage("Microsoft.Naming", "CA1703:ResourceStringsShouldBeSpelledCorrectly", MessageId = "Git", Scope = "resource", Target = "GitHub.App.Resources.resources")]
 [assembly: SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object,System.Object,System.Object)", Scope = "member", Target = "GitHub.Services.PullRequestService.#CreateTempFile(System.String,System.String,System.String)")]
 [assembly: SuppressMessage("Microsoft.Globalization", "CA1305:SpecifyIFormatProvider", MessageId = "System.String.Format(System.String,System.Object,System.Object,System.Object)", Scope = "member", Target = "GitHub.Services.PullRequestService.#CreateTempFile(System.String,System.String,System.String,System.Text.Encoding)")]
+[assembly: SuppressMessage("Reliability", "CA2007:Do not directly await a Task", Justification = "We always need to think about what thread we're running on")]
+
 // This file is used by Code Analysis to maintain SuppressMessage 
 // attributes that are applied to this project.
 // Project-level suppressions either have no target or are given 

src/GitHub.App/SampleData/GitServiceDesigner.cs

@@ -1,4 +1,5 @@
 using System.Threading.Tasks;
+using GitHub.Models;
 using GitHub.Primitives;
 using GitHub.Services;
 using LibGit2Sharp;
@@ -7,6 +8,9 @@ namespace GitHub.SampleData
 {
     class GitServiceDesigner : IGitService
     {
+        public ILocalRepositoryModel CreateLocalRepositoryModel(string localPath) => null;
+        public IBranch CreateCurrentBranchModel(ILocalRepositoryModel model) => null;
+        public void RefreshCloneUrl(ILocalRepositoryModel localRepositoryModel) { }
         public Task<string> GetLatestPushedSha(string path, string remote = "origin") => Task.FromResult<string>(null);
         public UriString GetRemoteUri(IRepository repo, string remote = "origin") => null;
         public IRepository GetRepository(string path) => null;

src/GitHub.App/SampleData/PullRequestCreationViewModelDesigner.cs

@@ -15,15 +15,21 @@ public class PullRequestCreationViewModelDesigner : PanePageViewModelBase, IPull
     {
         public PullRequestCreationViewModelDesigner()
         {
+            var repositoryModel = new LocalRepositoryModel
+            {
+                Name = "repo",
+                CloneUrl = "http://github.com/user/repo"
+            };
+
             Branches = new List<IBranch>
             {
-                new BranchModel("master", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())),
-                new BranchModel("don/stub-ui", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())),
-                new BranchModel("feature/pr/views", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())),
-                new BranchModel("release-1.0.17.0", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner())),
+                new BranchModel("master", repositoryModel),
+                new BranchModel("don/stub-ui", repositoryModel),
+                new BranchModel("feature/pr/views", repositoryModel),
+                new BranchModel("release-1.0.17.0", repositoryModel),
             }.AsReadOnly();
 
-            TargetBranch = new BranchModel("master", new LocalRepositoryModel("http://github.com/user/repo", new GitServiceDesigner()));
+            TargetBranch = new BranchModel("master", repositoryModel);
             SourceBranch = Branches[2];
 
             SelectedAssignee = "Haacked (Phil Haack)";

src/GitHub.App/Services/PullRequestService.cs

@@ -231,8 +231,8 @@ public async Task<Page<PullRequestListItemModel>> ReadPullRequests(
                     if (hasCheckRuns)
                     {
                         checksHasFailure = checkRuns
-                            .Any(model => model.Conclusion.HasValue 
-                                          && (model.Conclusion.Value == CheckConclusionState.Failure 
+                            .Any(model => model.Conclusion.HasValue
+                                          && (model.Conclusion.Value == CheckConclusionState.Failure
                                               || model.Conclusion.Value == CheckConclusionState.ActionRequired));
 
                         if (!checksHasFailure)
@@ -718,7 +718,7 @@ public IObservable<Unit> SwitchToBranch(ILocalRepositoryModel repository, PullRe
             });
         }
 
-        public IObservable<Tuple<string, int>> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository)
+        public IObservable<(string owner, int number)> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository)
         {
             return Observable.Defer(async () =>
             {
@@ -912,7 +912,7 @@ async Task<bool> IsBranchMarkedAsPullRequest(IRepository repo, string branchName
         {
             var prConfigKey = $"branch.{branchName}.{SettingGHfVSPullRequest}";
             var value = ParseGHfVSConfigKeyValue(await gitClient.GetConfig<string>(repo, prConfigKey));
-            return value != null &&
+            return value != default &&
                 value.Item1 == pullRequest.BaseRepositoryOwner &&
                 value.Item2 == pullRequest.Number;
         }
@@ -983,7 +983,7 @@ static string BuildGHfVSConfigKeyValue(string owner, int number)
             return owner + '#' + number.ToString(CultureInfo.InvariantCulture);
         }
 
-        static Tuple<string, int> ParseGHfVSConfigKeyValue(string value)
+        static (string owner, int number) ParseGHfVSConfigKeyValue(string value)
         {
             if (value != null)
             {
@@ -996,12 +996,12 @@ static Tuple<string, int> ParseGHfVSConfigKeyValue(string value)
 
                     if (int.TryParse(value.Substring(separator + 1), NumberStyles.None, CultureInfo.InvariantCulture, out number))
                     {
-                        return Tuple.Create(owner, number);
+                        return (owner, number);
                     }
                 }
             }
 
-            return null;
+            return default;
         }
 
         class ListItemAdapter : PullRequestListItemModel

src/GitHub.App/Services/TeamExplorerContext.cs

@@ -40,10 +40,7 @@ public class TeamExplorerContext : ITeamExplorerContext
         string solutionPath;
         string repositoryPath;
         UriString cloneUrl;
-        string branchName;
-        string headSha;
-        string trackedSha;
-        Tuple<string, int> pullRequest;
+        (string owner, int number) pullRequest;
 
         ILocalRepositoryModel repositoryModel;
         JoinableTask refreshJoinableTask;
@@ -113,10 +110,7 @@ async Task RefreshAsync()
                 {
                     var newRepositoryPath = repo?.LocalPath;
                     var newCloneUrl = repo?.CloneUrl;
-                    var newBranchName = repo?.CurrentBranch?.Name;
-                    var newHeadSha = repo?.CurrentBranch?.Sha;
-                    var newTrackedSha = repo?.CurrentBranch?.TrackedSha;
-                    var newPullRequest = repo != null ? await pullRequestService.GetPullRequestForCurrentBranch(repo) : null;
+                    var newPullRequest = repo != null ? await pullRequestService.GetPullRequestForCurrentBranch(repo) : default;
 
                     if (newRepositoryPath != repositoryPath)
                     {
@@ -128,33 +122,20 @@ async Task RefreshAsync()
                         log.Debug("ActiveRepository changed to {CloneUrl} @ {Path}", repo?.CloneUrl, newRepositoryPath);
                         ActiveRepository = repo;
                     }
-                    else if (newBranchName != branchName)
-                    {
-                        log.Debug("Fire StatusChanged event when BranchName changes for ActiveRepository");
-                        StatusChanged?.Invoke(this, EventArgs.Empty);
-                    }
-                    else if (newHeadSha != headSha)
-                    {
-                        log.Debug("Fire StatusChanged event when HeadSha changes for ActiveRepository");
-                        StatusChanged?.Invoke(this, EventArgs.Empty);
-                    }
-                    else if (newTrackedSha != trackedSha)
+                    else if (newPullRequest != pullRequest)
                     {
-                        log.Debug("Fire StatusChanged event when TrackedSha changes for ActiveRepository");
+                        log.Debug("Fire StatusChanged event when PullRequest changes for ActiveRepository");
                         StatusChanged?.Invoke(this, EventArgs.Empty);
                     }
-                    else if (newPullRequest != pullRequest)
+                    else if (newRepositoryPath != null)
                     {
-                        log.Debug("Fire StatusChanged event when PullRequest changes for ActiveRepository");
+                        log.Debug("Fire StatusChanged event when on a repository and anything changes");
                         StatusChanged?.Invoke(this, EventArgs.Empty);
                     }
 
                     repositoryPath = newRepositoryPath;
                     cloneUrl = newCloneUrl;
-                    branchName = newBranchName;
-                    headSha = newHeadSha;
                     solutionPath = newSolutionPath;
-                    trackedSha = newTrackedSha;
                     pullRequest = newPullRequest;
                 }
             }

src/GitHub.App/ViewModels/GitHubPane/PullRequestCreationViewModel.cs

@@ -38,6 +38,7 @@ public class PullRequestCreationViewModel : PanePageViewModelBase, IPullRequestC
         readonly IPullRequestService service;
         readonly IModelServiceFactory modelServiceFactory;
         readonly IMessageDraftStore draftStore;
+        readonly IGitService gitService;
         readonly IScheduler timerScheduler;
         readonly CompositeDisposable disposables = new CompositeDisposable();
         ILocalRepositoryModel activeLocalRepo;
@@ -49,8 +50,9 @@ public PullRequestCreationViewModel(
             IModelServiceFactory modelServiceFactory,
             IPullRequestService service,
             INotificationService notifications,
-            IMessageDraftStore draftStore)
-            : this(modelServiceFactory, service, notifications, draftStore, DefaultScheduler.Instance)
+            IMessageDraftStore draftStore,
+            IGitService gitService)
+            : this(modelServiceFactory, service, notifications, draftStore, gitService, DefaultScheduler.Instance)
         {
         }
 
@@ -59,17 +61,20 @@ public PullRequestCreationViewModel(
             IPullRequestService service,
             INotificationService notifications,
             IMessageDraftStore draftStore,
+            IGitService gitService,
             IScheduler timerScheduler)
         {
             Guard.ArgumentNotNull(modelServiceFactory, nameof(modelServiceFactory));
             Guard.ArgumentNotNull(service, nameof(service));
             Guard.ArgumentNotNull(notifications, nameof(notifications));
             Guard.ArgumentNotNull(draftStore, nameof(draftStore));
+            Guard.ArgumentNotNull(gitService, nameof(gitService));
             Guard.ArgumentNotNull(timerScheduler, nameof(timerScheduler));
 
             this.service = service;
             this.modelServiceFactory = modelServiceFactory;
             this.draftStore = draftStore;
+            this.gitService = gitService;
             this.timerScheduler = timerScheduler;
 
             this.WhenAnyValue(x => x.Branches)
@@ -137,7 +142,7 @@ public async Task InitializeAsync(ILocalRepositoryModel repository, IConnection
         {
             modelService = await modelServiceFactory.CreateAsync(connection);
             activeLocalRepo = repository;
-            SourceBranch = repository.CurrentBranch;
+            SourceBranch = gitService.CreateCurrentBranchModel(repository);
 
             var obs = modelService.ApiClient.GetRepository(repository.Owner, repository.Name)
                 .Select(r => new RemoteRepositoryModel(r))
@@ -207,7 +212,7 @@ async Task LoadInitialState(string draftKey)
 
         void LoadDescriptionFromCommits()
         {
-            SourceBranch = activeLocalRepo.CurrentBranch;
+            SourceBranch = gitService.CreateCurrentBranchModel(activeLocalRepo);
 
             var uniqueCommits = this.WhenAnyValue(
                 x => x.SourceBranch,

src/GitHub.App/ViewModels/GitHubPane/PullRequestDetailViewModel.cs

@@ -39,6 +39,7 @@ public sealed class PullRequestDetailViewModel : PanePageViewModelBase, IPullReq
         readonly ITeamExplorerContext teamExplorerContext;
         readonly ISyncSubmodulesCommand syncSubmodulesCommand;
         readonly IViewViewModelFactory viewViewModelFactory;
+        readonly IGitService gitService;
         IModelService modelService;
         PullRequestDetailModel model;
         IActorViewModel author;
@@ -77,7 +78,8 @@ public PullRequestDetailViewModel(
             ITeamExplorerContext teamExplorerContext,
             IPullRequestFilesViewModel files,
             ISyncSubmodulesCommand syncSubmodulesCommand,
-            IViewViewModelFactory viewViewModelFactory)
+            IViewViewModelFactory viewViewModelFactory,
+            IGitService gitService)
         {
             Guard.ArgumentNotNull(pullRequestsService, nameof(pullRequestsService));
             Guard.ArgumentNotNull(sessionManager, nameof(sessionManager));
@@ -86,6 +88,7 @@ public PullRequestDetailViewModel(
             Guard.ArgumentNotNull(teamExplorerContext, nameof(teamExplorerContext));
             Guard.ArgumentNotNull(syncSubmodulesCommand, nameof(syncSubmodulesCommand));
             Guard.ArgumentNotNull(viewViewModelFactory, nameof(viewViewModelFactory));
+            Guard.ArgumentNotNull(gitService, nameof(gitService));
 
             this.pullRequestsService = pullRequestsService;
             this.sessionManager = sessionManager;
@@ -94,6 +97,7 @@ public PullRequestDetailViewModel(
             this.teamExplorerContext = teamExplorerContext;
             this.syncSubmodulesCommand = syncSubmodulesCommand;
             this.viewViewModelFactory = viewViewModelFactory;
+            this.gitService = gitService;
             Files = files;
 
             Checkout = ReactiveCommand.CreateFromObservable(
@@ -401,7 +405,8 @@ public async Task Load(PullRequestDetailModel pullRequest)
 
                 var localBranches = await pullRequestsService.GetLocalBranches(LocalRepository, pullRequest).ToList();
 
-                IsCheckedOut = localBranches.Contains(LocalRepository.CurrentBranch);
+                var currentBranch = gitService.CreateCurrentBranchModel(LocalRepository);
+                IsCheckedOut = localBranches.Contains(currentBranch);
 
                 if (IsCheckedOut)
                 {

src/GitHub.Exports.Reactive/Services/IPullRequestService.cs

@@ -173,7 +173,7 @@ IObservable<IPullRequestModel> CreatePullRequest(IModelService modelService,
         /// This method does not do an API request - it simply checks the mark left in the git
         /// config by <see cref="Checkout(ILocalRepositoryModel, PullRequestDetailModel, string)"/>.
         /// </remarks>
-        IObservable<Tuple<string, int>> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository);
+        IObservable<(string owner, int number)> GetPullRequestForCurrentBranch(ILocalRepositoryModel repository);
 
         /// <summary>
         /// Gets the encoding for the specified local file.

src/GitHub.Exports/Models/BranchModel.cs

@@ -32,7 +32,8 @@ public BranchModel(LibGit2Sharp.Branch branch, IRepositoryModel repo, IGitServic
             Extensions.Guard.ArgumentNotNull(repo, nameof(repo));
             Name = DisplayName = branch.FriendlyName;
 #pragma warning disable 0618 // TODO: Replace `Branch.Remote` with `Repository.Network.Remotes[branch.RemoteName]`.
-            Repository = branch.IsRemote ? new LocalRepositoryModel(branch.Remote.Url, gitService) : repo;
+            // NOTE: This method expects a localPath not a URL!
+            Repository = branch.IsRemote ? gitService.CreateLocalRepositoryModel(branch.Remote.Url) : repo;
 #pragma warning restore 0618
             IsTracking = branch.IsTracking;
             Sha = branch.Tip?.Sha;

src/GitHub.Exports/Models/ILocalRepositoryModel.cs

@@ -1,7 +1,4 @@
-using GitHub.Exports;
-using GitHub.Primitives;
-using System.ComponentModel;
-using System.Threading.Tasks;
+using System.ComponentModel;
 
 namespace GitHub.Models
 {
@@ -14,26 +11,5 @@ public interface ILocalRepositoryModel : IRepositoryModel, INotifyPropertyChange
         /// Gets the path to the repository on the filesystem.
         /// </summary>
         string LocalPath { get; }
-
-        /// <summary>
-        /// Gets the current branch.
-        /// </summary>
-        IBranch CurrentBranch { get; }
-
-        /// <summary>
-        /// Updates the url information based on the local path
-        /// </summary>
-        void Refresh();
-
-        /// <summary>
-        /// Generates a http(s) url to the repository in the remote server, optionally
-        /// pointing to a specific file and specific line range in it.
-        /// </summary>
-        /// <param name="linkType">The type of repository link to create</param>
-        /// <param name="path">The file to generate an url to. Optional.</param>
-        /// <param name="startLine">A specific line, or (if specifying the <paramref name="endLine"/> as well) the start of a range</param>
-        /// <param name="endLine">The end of a line range on the specified file.</param>
-        /// <returns>An UriString with the generated url, or null if the repository has no remote server configured or if it can't be found locally</returns>
-        Task<UriString> GenerateUrl(LinkType linkType, string path = null, int startLine = -1, int endLine = -1);
     }
 }

src/GitHub.Exports/Models/IRepositoryModel.cs

@@ -17,7 +17,7 @@ public interface IRepositoryModel
         /// <summary>
         /// Gets the repository clone URL.
         /// </summary>
-        UriString CloneUrl { get; }
+        UriString CloneUrl { get; set; }
 
         /// <summary>
         /// Gets the name of the owner of the repository, taken from the clone URL.