Merge pull request #1499 from github/fixes/1493-wrong-repo

Ensure correct ordering of VSGitExt.ActiveRepositoriesChanged.

Author: jcansdale

src/GitHub.App/Services/TeamExplorerContext.cs

@@ -76,7 +76,7 @@ void Refresh()
 
                     if (newRepositoryPath != repositoryPath)
                     {
-                        log.Debug("Fire PropertyChanged event for ActiveRepository");
+                        log.Debug("ActiveRepository changed to {CloneUrl} @ {Path}", repo?.CloneUrl, newRepositoryPath);
                         ActiveRepository = repo;
                     }
                     else if (newBranchName != branchName)

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

@@ -11,11 +11,13 @@
 using GitHub.Extensions;
 using GitHub.Factories;
 using GitHub.Info;
+using GitHub.Logging;
 using GitHub.Models;
 using GitHub.Primitives;
 using GitHub.Services;
 using GitHub.VisualStudio;
 using ReactiveUI;
+using Serilog;
 using OleMenuCommand = Microsoft.VisualStudio.Shell.OleMenuCommand;
 
 namespace GitHub.ViewModels.GitHubPane
@@ -27,6 +29,7 @@ namespace GitHub.ViewModels.GitHubPane
     [PartCreationPolicy(CreationPolicy.NonShared)]
     public sealed class GitHubPaneViewModel : ViewModelBase, IGitHubPaneViewModel, IDisposable
     {
+        static readonly ILogger log = LogManager.ForContext<GitHubPaneViewModel>();
         static readonly Regex pullUri = CreateRoute("/:owner/:repo/pull/:number");
 
         readonly IViewViewModelFactory viewModelFactory;
@@ -360,18 +363,22 @@ async Task NavigateTo<TViewModel>(Func<TViewModel, Task> initialize, Func<TViewM
 
         async Task UpdateContent(ILocalRepositoryModel repository)
         {
+            log.Debug("UpdateContent called with {CloneUrl}", repository?.CloneUrl);
+
             LocalRepository = repository;
             Connection = null;
             Content = null;
             navigator.Clear();
 
             if (repository == null)
             {
+                log.Debug("Not a git repository: {CloneUrl}", repository?.CloneUrl);
                 Content = notAGitRepository;
                 return;
             }
             else if (string.IsNullOrWhiteSpace(repository.CloneUrl))
             {
+                log.Debug("Not a GitHub repository: {CloneUrl}", repository?.CloneUrl);
                 Content = notAGitHubRepository;
                 return;
             }
@@ -389,16 +396,19 @@ async Task UpdateContent(ILocalRepositoryModel repository)
 
                 if (Connection?.IsLoggedIn == true)
                 {
+                    log.Debug("Found a GitHub repository: {CloneUrl}", repository?.CloneUrl);
                     Content = navigator;
                     await ShowDefaultPage();
                 }
                 else
                 {
+                    log.Debug("Found a a GitHub repository but not logged in: {CloneUrl}", repository?.CloneUrl);
                     Content = loggedOut;
                 }
             }
             else
             {
+                log.Debug("Not a GitHub repository: {CloneUrl}", repository?.CloneUrl);
                 Content = notAGitHubRepository;
             }
         }

src/GitHub.TeamFoundation.14/Services/VSGitExt.cs

@@ -24,6 +24,7 @@ public class VSGitExt : IVSGitExt
         readonly IGitHubServiceProvider serviceProvider;
         readonly IVSUIContext context;
         readonly ILocalRepositoryModelFactory repositoryFactory;
+        readonly object refreshLock = new object();
 
         IGitExt gitService;
         IReadOnlyList<ILocalRepositoryModel> activeRepositories;
@@ -98,7 +99,15 @@ void RefreshActiveRepositories()
         {
             try
             {
-                ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList();
+                lock (refreshLock)
+                {
+                    log.Debug(
+                        "IGitExt.ActiveRepositories (#{Id}) returned {Repositories}",
+                        gitService.GetHashCode(),
+                        gitService?.ActiveRepositories.Select(x => x.RepositoryPath));
+
+                    ActiveRepositories = gitService?.ActiveRepositories.Select(x => repositoryFactory.Create(x.RepositoryPath)).ToList();
+                }
             }
             catch (Exception e)
             {
@@ -118,6 +127,7 @@ private set
             {
                 if (value != activeRepositories)
                 {
+                    log.Debug("ActiveRepositories changed to {Repositories}", value?.Select(x => x.CloneUrl));
                     activeRepositories = value;
                     ActiveRepositoriesChanged?.Invoke();
                 }

test/UnitTests/GitHub.TeamFoundation/VSGitExtTests.cs

@@ -9,6 +9,9 @@
 using NUnit.Framework;
 using NSubstitute;
 using Microsoft.VisualStudio.TeamFoundation.Git.Extensibility;
+using System.Threading.Tasks;
+using System.Linq;
+using GitHub.TeamFoundation.Services;
 
 public class VSGitExtTests
 {
@@ -171,9 +174,26 @@ public void ExceptionRefreshingRepositories_ReturnsEmptyList()
             repoFactory.Received(1).Create(repoPath);
             Assert.That(activeRepositories.Count, Is.EqualTo(0));
         }
+
+        [Test]
+        public async Task ActiveRepositoriesChangedOrderingShouldBeCorrectAcrossThreads()
+        {
+            var gitExt = new MockGitExt();
+            var repoFactory = new MockRepositoryFactory();
+            var target = CreateVSGitExt(gitExt: gitExt, repoFactory: repoFactory);
+            var activeRepositories1 = CreateActiveRepositories("repo1");
+            var activeRepositories2 = CreateActiveRepositories("repo2");
+            var task1 = Task.Run(() => gitExt.ActiveRepositories = activeRepositories1);
+            await Task.Delay(1);
+            var task2 = Task.Run(() => gitExt.ActiveRepositories = activeRepositories2);
+
+            await Task.WhenAll(task1, task2);
+
+            Assert.That(target.ActiveRepositories.Single().LocalPath, Is.EqualTo("repo2"));
+        }
     }
 
-    static IReadOnlyList<IGitRepositoryInfo> CreateActiveRepositories(IList<string> repositoryPaths)
+    static IReadOnlyList<IGitRepositoryInfo> CreateActiveRepositories(params string[] repositoryPaths)
     {
         var repositories = new List<IGitRepositoryInfo>();
         foreach (var repositoryPath in repositoryPaths)
@@ -203,9 +223,8 @@ static VSGitExt CreateVSGitExt(IVSUIContext context = null, IGitExt gitExt = nul
         return vsGitExt;
     }
 
-    static IGitExt CreateGitExt(IList<string> repositoryPaths = null)
+    static IGitExt CreateGitExt(params string[] repositoryPaths)
     {
-        repositoryPaths = repositoryPaths ?? Array.Empty<string>();
         var gitExt = Substitute.For<IGitExt>();
         var repoList = CreateActiveRepositories(repositoryPaths);
         gitExt.ActiveRepositories.Returns(repoList);
@@ -218,4 +237,43 @@ static IVSUIContext CreateVSUIContext(bool isActive)
         context.IsActive.Returns(isActive);
         return context;
     }
+
+    class MockGitExt : IGitExt
+    {
+        IReadOnlyList<IGitRepositoryInfo> activeRepositories = new IGitRepositoryInfo[0];
+
+        public IReadOnlyList<IGitRepositoryInfo> ActiveRepositories
+        {
+            get { return activeRepositories; }
+            set
+            {
+                if (activeRepositories != value)
+                {
+                    activeRepositories = value;
+                    PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(ActiveRepositories)));
+                }
+            }
+        }
+
+        public event PropertyChangedEventHandler PropertyChanged;
+    }
+
+    class MockRepositoryFactory : ILocalRepositoryModelFactory
+    {
+        public ILocalRepositoryModel Create(string localPath)
+        {
+            var result = Substitute.For<ILocalRepositoryModel>();
+            result.LocalPath.Returns(localPath);
+
+            if (localPath == "repo1")
+            {
+                // Trying to force #1493 here by introducing a a delay on the first
+                // ActiveRepositories changed notification so that the second completes
+                // first.
+                Thread.Sleep(10);
+            }
+
+            return result;
+        }
+    }
 }