Merge pull request #419 from github-for-unity/fixes/stop-watcher-for-commit

StanleyGoldman · web-flow · commit 1352d604d3ac · 2017-11-22T10:06:29.000-05:00
Refactor HookupHandlers in order accurately wrap tasks
diff --git a/src/GitHub.Api/Git/RepositoryManager.cs b/src/GitHub.Api/Git/RepositoryManager.cs
@@ -1,6 +1,7 @@
 ﻿using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
 using System.Threading.Tasks;
 
 namespace GitHub.Unity
@@ -169,64 +170,62 @@ public int WaitForEvents()
 
         public ITask CommitAllFiles(string message, string body)
         {
-            var add = GitClient.AddAll();
-            add.OnStart += t => IsBusy = true;
-            return add
-                .Then(GitClient.Commit(message, body))
-                .Finally(() => IsBusy = false);
+            var task = GitClient
+                .AddAll()
+                .Then(GitClient.Commit(message, body));
+
+            return HookupHandlers(task, true, true);
         }
 
         public ITask CommitFiles(List<string> files, string message, string body)
         {
-            var add = GitClient.Add(files);
-            add.OnStart += t => IsBusy = true;
-            return add
-                .Then(GitClient.Commit(message, body))
-                .Finally(() => IsBusy = false);
+            var task = GitClient
+                .Add(files)
+                .Then(GitClient.Commit(message, body));
+
+            return HookupHandlers(task, true, true);
         }
 
         public ITask<List<GitLogEntry>> Log()
         {
             var task = GitClient.Log();
-            HookupHandlers(task);
-            return task;
+            return HookupHandlers(task, false, false);
         }
 
         public ITask<GitStatus> Status()
         {
             var task = GitClient.Status();
-            HookupHandlers(task);
-            return task;
+            return HookupHandlers(task, true, false);
         }
 
         public ITask Fetch(string remote)
         {
             var task = GitClient.Fetch(remote);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public ITask Pull(string remote, string branch)
         {
             var task = GitClient.Pull(remote, branch);
-            return HookupHandlers(task, true);
+            return HookupHandlers(task, true, true);
         }
 
         public ITask Push(string remote, string branch)
         {
             var task = GitClient.Push(remote, branch);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public ITask Revert(string changeset)
         {
             var task = GitClient.Revert(changeset);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, true);
         }
 
         public ITask RemoteAdd(string remote, string url)
         {
             var task = GitClient.RemoteAdd(remote, url);
-            HookupHandlers(task);
+            task = HookupHandlers(task, true, false);
             if (!platform.Environment.IsWindows)
             {
                 task.Then(_ => {
@@ -239,7 +238,7 @@ public ITask RemoteAdd(string remote, string url)
         public ITask RemoteRemove(string remote)
         {
             var task = GitClient.RemoteRemove(remote);
-            HookupHandlers(task);
+            task = HookupHandlers(task, true, false);
             if (!platform.Environment.IsWindows)
             {
                 task.Then(_ => {
@@ -252,44 +251,44 @@ public ITask RemoteRemove(string remote)
         public ITask RemoteChange(string remote, string url)
         {
             var task = GitClient.RemoteChange(remote, url);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public ITask SwitchBranch(string branch)
         {
             var task = GitClient.SwitchBranch(branch);
-            return HookupHandlers(task, true);
+            return HookupHandlers(task, true, true);
         }
 
         public ITask DeleteBranch(string branch, bool deleteUnmerged = false)
         {
             var task = GitClient.DeleteBranch(branch, deleteUnmerged);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public ITask CreateBranch(string branch, string baseBranch)
         {
             var task = GitClient.CreateBranch(branch, baseBranch);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public ITask<List<GitLock>> ListLocks(bool local)
         {
             var task = GitClient.ListLocks(local);
-            HookupHandlers(task);
+            HookupHandlers(task, false, false);
             return task;
         }
 
         public ITask LockFile(string file)
         {
             var task = GitClient.Lock(file);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public ITask UnlockFile(string file, bool force)
         {
             var task = GitClient.Unlock(file, force);
-            return HookupHandlers(task);
+            return HookupHandlers(task, true, false);
         }
 
         public void UpdateConfigData()
@@ -317,29 +316,42 @@ private void UpdateHead()
             UpdateCurrentBranchAndRemote(head);
         }
 
-        private ITask HookupHandlers(ITask task, bool disableWatcher = false)
+        private ITask<T> HookupHandlers<T>(ITask<T> task, bool isExclusive, bool filesystemChangesExpected)
         {
-            task.OnStart += t => {
-                Logger.Trace("Start " + task.Name);
-                IsBusy = true;
+            return new ActionTask(CancellationToken.None, () => {
+                    if (isExclusive)
+                    {
+                        Logger.Trace("Starting Operation - Setting Busy Flag");
+                        IsBusy = true;
+                    }
 
-                if (disableWatcher)
-                {
-                    watcher.Stop();
-                }
-            };
+                    if (filesystemChangesExpected)
+                    {
+                        Logger.Trace("Starting Operation - Disable Watcher");
+                        watcher.Stop();
+                    }
+                })
+                .Then(task)
+                .Finally((success, exception, result) => {
+                    if (filesystemChangesExpected)
+                    {
+                        Logger.Trace("Ended Operation - Enable Watcher");
+                        watcher.Start();
+                    }
 
-            task.OnEnd += t => {
-                if (disableWatcher)
-                {
-                    watcher.Start();
-                }
+                    if (isExclusive)
+                    {
+                        Logger.Trace("Ended Operation - Clearing Busy Flag");
+                        IsBusy = false;
+                    }
 
-                IsBusy = false;
+                    if (success)
+                    {
+                        return result;
+                    }
 
-                Logger.Trace("Finish " + task.Name);
-            };
-            return task;
+                    throw exception;
+                });
         }
 
         private void Watcher_OnRemoteBranchDeleted(string remote, string name)
diff --git a/src/tests/TaskSystemIntegrationTests/Tests.cs b/src/tests/TaskSystemIntegrationTests/Tests.cs
@@ -108,6 +108,44 @@ public async Task ProcessReadsFromStandardInput()
             Assert.AreEqual(expectedOutput, output);
         }
 
+        [Test]
+        public async Task ProcessOnStartOnEndTaskOrder()
+        {
+            var values = new List<string>();
+            string process1Value = null;
+            string process2Value = null;
+
+            var process1Task = new FirstNonNullLineProcessTask(Token, TestApp, @"-s 100 -d process1")
+                .Configure(ProcessManager, true).Then((b, s) => {
+                    process1Value = s;
+                    values.Add(s);
+                });
+
+            var process2Task = new FirstNonNullLineProcessTask(Token, TestApp, @"-s 100 -d process2")
+                .Configure(ProcessManager, true).Then((b, s) => {
+                    process2Value = s;
+                    values.Add(s);
+                });
+
+            var combinedTask = process1Task
+                .Then(process2Task);
+
+            combinedTask.OnStart += task => {
+                values.Add("OnStart");
+            };
+
+            combinedTask.OnEnd += task => {
+                values.Add("OnEnd");
+            };
+
+            await combinedTask
+                .StartAsAsync();
+
+            Assert.AreEqual(process1Value, "process1");
+            Assert.AreEqual(process2Value, "process2");
+            Assert.True(values.SequenceEqual(new []{ "process1", "OnStart", "process2", "OnEnd" }));
+        }
+
         [Test]
         public async Task ProcessReturningErrorThrowsException()
         {

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`using System;`
`2`	`2`	`using System.Collections.Generic;`
`3`	`3`	`using System.Linq;`
	`4`	`+using System.Threading;`
`4`	`5`	`using System.Threading.Tasks;`
`5`	`6`
`6`	`7`	`namespace GitHub.Unity`
`@@ -169,64 +170,62 @@ public int WaitForEvents()`
`169`	`170`
`170`	`171`	`public ITask CommitAllFiles(string message, string body)`
`171`	`172`	`{`
`172`		`- var add = GitClient.AddAll();`
`173`		`- add.OnStart += t => IsBusy = true;`
`174`		`- return add`
`175`		`- .Then(GitClient.Commit(message, body))`
`176`		`- .Finally(() => IsBusy = false);`
	`173`	`+ var task = GitClient`
	`174`	`+ .AddAll()`
	`175`	`+ .Then(GitClient.Commit(message, body));`
	`176`	`+`
	`177`	`+ return HookupHandlers(task, true, true);`
`177`	`178`	`}`
`178`	`179`
`179`	`180`	`public ITask CommitFiles(List<string> files, string message, string body)`
`180`	`181`	`{`
`181`		`- var add = GitClient.Add(files);`
`182`		`- add.OnStart += t => IsBusy = true;`
`183`		`- return add`
`184`		`- .Then(GitClient.Commit(message, body))`
`185`		`- .Finally(() => IsBusy = false);`
	`182`	`+ var task = GitClient`
	`183`	`+ .Add(files)`
	`184`	`+ .Then(GitClient.Commit(message, body));`
	`185`	`+`
	`186`	`+ return HookupHandlers(task, true, true);`
`186`	`187`	`}`
`187`	`188`
`188`	`189`	`public ITask<List<GitLogEntry>> Log()`
`189`	`190`	`{`
`190`	`191`	`var task = GitClient.Log();`
`191`		`- HookupHandlers(task);`
`192`		`- return task;`
	`192`	`+ return HookupHandlers(task, false, false);`
`193`	`193`	`}`
`194`	`194`
`195`	`195`	`public ITask<GitStatus> Status()`
`196`	`196`	`{`
`197`	`197`	`var task = GitClient.Status();`
`198`		`- HookupHandlers(task);`
`199`		`- return task;`
	`198`	`+ return HookupHandlers(task, true, false);`
`200`	`199`	`}`
`201`	`200`
`202`	`201`	`public ITask Fetch(string remote)`
`203`	`202`	`{`
`204`	`203`	`var task = GitClient.Fetch(remote);`
`205`		`- return HookupHandlers(task);`
	`204`	`+ return HookupHandlers(task, true, false);`
`206`	`205`	`}`
`207`	`206`
`208`	`207`	`public ITask Pull(string remote, string branch)`
`209`	`208`	`{`
`210`	`209`	`var task = GitClient.Pull(remote, branch);`
`211`		`- return HookupHandlers(task, true);`
	`210`	`+ return HookupHandlers(task, true, true);`
`212`	`211`	`}`
`213`	`212`
`214`	`213`	`public ITask Push(string remote, string branch)`
`215`	`214`	`{`
`216`	`215`	`var task = GitClient.Push(remote, branch);`
`217`		`- return HookupHandlers(task);`
	`216`	`+ return HookupHandlers(task, true, false);`
`218`	`217`	`}`
`219`	`218`
`220`	`219`	`public ITask Revert(string changeset)`
`221`	`220`	`{`
`222`	`221`	`var task = GitClient.Revert(changeset);`
`223`		`- return HookupHandlers(task);`
	`222`	`+ return HookupHandlers(task, true, true);`
`224`	`223`	`}`
`225`	`224`
`226`	`225`	`public ITask RemoteAdd(string remote, string url)`
`227`	`226`	`{`
`228`	`227`	`var task = GitClient.RemoteAdd(remote, url);`
`229`		`- HookupHandlers(task);`
	`228`	`+ task = HookupHandlers(task, true, false);`
`230`	`229`	`if (!platform.Environment.IsWindows)`
`231`	`230`	`{`
`232`	`231`	`task.Then(_ => {`
`@@ -239,7 +238,7 @@ public ITask RemoteAdd(string remote, string url)`
`239`	`238`	`public ITask RemoteRemove(string remote)`
`240`	`239`	`{`
`241`	`240`	`var task = GitClient.RemoteRemove(remote);`
`242`		`- HookupHandlers(task);`
	`241`	`+ task = HookupHandlers(task, true, false);`
`243`	`242`	`if (!platform.Environment.IsWindows)`
`244`	`243`	`{`
`245`	`244`	`task.Then(_ => {`
`@@ -252,44 +251,44 @@ public ITask RemoteRemove(string remote)`
`252`	`251`	`public ITask RemoteChange(string remote, string url)`
`253`	`252`	`{`
`254`	`253`	`var task = GitClient.RemoteChange(remote, url);`
`255`		`- return HookupHandlers(task);`
	`254`	`+ return HookupHandlers(task, true, false);`
`256`	`255`	`}`
`257`	`256`
`258`	`257`	`public ITask SwitchBranch(string branch)`
`259`	`258`	`{`
`260`	`259`	`var task = GitClient.SwitchBranch(branch);`
`261`		`- return HookupHandlers(task, true);`
	`260`	`+ return HookupHandlers(task, true, true);`
`262`	`261`	`}`
`263`	`262`
`264`	`263`	`public ITask DeleteBranch(string branch, bool deleteUnmerged = false)`
`265`	`264`	`{`
`266`	`265`	`var task = GitClient.DeleteBranch(branch, deleteUnmerged);`
`267`		`- return HookupHandlers(task);`
	`266`	`+ return HookupHandlers(task, true, false);`
`268`	`267`	`}`
`269`	`268`
`270`	`269`	`public ITask CreateBranch(string branch, string baseBranch)`
`271`	`270`	`{`
`272`	`271`	`var task = GitClient.CreateBranch(branch, baseBranch);`
`273`		`- return HookupHandlers(task);`
	`272`	`+ return HookupHandlers(task, true, false);`
`274`	`273`	`}`
`275`	`274`
`276`	`275`	`public ITask<List<GitLock>> ListLocks(bool local)`
`277`	`276`	`{`
`278`	`277`	`var task = GitClient.ListLocks(local);`
`279`		`- HookupHandlers(task);`
	`278`	`+ HookupHandlers(task, false, false);`
`280`	`279`	`return task;`
`281`	`280`	`}`
`282`	`281`
`283`	`282`	`public ITask LockFile(string file)`
`284`	`283`	`{`
`285`	`284`	`var task = GitClient.Lock(file);`
`286`		`- return HookupHandlers(task);`
	`285`	`+ return HookupHandlers(task, true, false);`
`287`	`286`	`}`
`288`	`287`
`289`	`288`	`public ITask UnlockFile(string file, bool force)`
`290`	`289`	`{`
`291`	`290`	`var task = GitClient.Unlock(file, force);`
`292`		`- return HookupHandlers(task);`
	`291`	`+ return HookupHandlers(task, true, false);`
`293`	`292`	`}`
`294`	`293`
`295`	`294`	`public void UpdateConfigData()`
`@@ -317,29 +316,42 @@ private void UpdateHead()`
`317`	`316`	`UpdateCurrentBranchAndRemote(head);`
`318`	`317`	`}`
`319`	`318`
`320`		`- private ITask HookupHandlers(ITask task, bool disableWatcher = false)`
	`319`	`+ private ITask<T> HookupHandlers<T>(ITask<T> task, bool isExclusive, bool filesystemChangesExpected)`
`321`	`320`	`{`
`322`		`- task.OnStart += t => {`
`323`		`- Logger.Trace("Start " + task.Name);`
`324`		`- IsBusy = true;`
	`321`	`+ return new ActionTask(CancellationToken.None, () => {`
	`322`	`+ if (isExclusive)`
	`323`	`+ {`
	`324`	`+ Logger.Trace("Starting Operation - Setting Busy Flag");`
	`325`	`+ IsBusy = true;`
	`326`	`+ }`
`325`	`327`
`326`		`- if (disableWatcher)`
`327`		`- {`
`328`		`- watcher.Stop();`
`329`		`- }`
`330`		`- };`
	`328`	`+ if (filesystemChangesExpected)`
	`329`	`+ {`
	`330`	`+ Logger.Trace("Starting Operation - Disable Watcher");`
	`331`	`+ watcher.Stop();`
	`332`	`+ }`
	`333`	`+ })`
	`334`	`+ .Then(task)`
	`335`	`+ .Finally((success, exception, result) => {`
	`336`	`+ if (filesystemChangesExpected)`
	`337`	`+ {`
	`338`	`+ Logger.Trace("Ended Operation - Enable Watcher");`
	`339`	`+ watcher.Start();`
	`340`	`+ }`
`331`	`341`
`332`		`- task.OnEnd += t => {`
`333`		`- if (disableWatcher)`
`334`		`- {`
`335`		`- watcher.Start();`
`336`		`- }`
	`342`	`+ if (isExclusive)`
	`343`	`+ {`
	`344`	`+ Logger.Trace("Ended Operation - Clearing Busy Flag");`
	`345`	`+ IsBusy = false;`
	`346`	`+ }`
`337`	`347`
`338`		`- IsBusy = false;`
	`348`	`+ if (success)`
	`349`	`+ {`
	`350`	`+ return result;`
	`351`	`+ }`
`339`	`352`
`340`		`- Logger.Trace("Finish " + task.Name);`
`341`		`- };`
`342`		`- return task;`
	`353`	`+ throw exception;`
	`354`	`+ });`
`343`	`355`	`}`
`344`	`356`
`345`	`357`	`private void Watcher_OnRemoteBranchDeleted(string remote, string name)`