Refresh after a clone on the render thread, and allow one clone per folder [patch]

ProjectDirector.Test/CloneTrackerTests.cs

@@ -0,0 +1,173 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector.Test;
+
+using System;
+using System.IO;
+using System.Threading;
+using System.Threading.Tasks;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Covers handing a background clone's completion back to the render thread.
+/// </summary>
+/// <remarks>
+/// The Clone button used to refresh the page from the worker thread that ran the clone, racing the
+/// render thread over the repository dictionaries and the repo browser, and nothing stopped a second
+/// click from starting another clone into the same folder.
+/// </remarks>
+[TestClass]
+public sealed class CloneTrackerTests
+{
+	private static FullyQualifiedLocalRepoPath LocalPath(string name) =>
+		FullyQualifiedLocalRepoPath.Create<FullyQualifiedLocalRepoPath>(Path.Join(Path.GetTempPath(), "dev", name));
+
+	[TestMethod]
+	public void ASecondCloneIntoTheSameFolderIsRefusedWhileTheFirstRuns()
+	{
+		CloneTracker clones = new();
+
+		Assert.IsTrue(clones.TryStart(LocalPath("A")));
+		Assert.IsTrue(clones.IsInFlight(LocalPath("A")));
+		Assert.IsFalse(clones.TryStart(LocalPath("A")));
+	}
+
+	[TestMethod]
+	public void ACloneIntoAnotherFolderIsNotBlocked()
+	{
+		CloneTracker clones = new();
+
+		Assert.IsTrue(clones.TryStart(LocalPath("A")));
+		Assert.IsTrue(clones.TryStart(LocalPath("B")));
+	}
+
+	[TestMethod]
+	public void AFolderCanBeClonedAgainOnceItsCloneCompletes()
+	{
+		CloneTracker clones = new();
+		Assert.IsTrue(clones.TryStart(LocalPath("A")));
+
+		clones.Complete(LocalPath("A"));
+
+		Assert.IsFalse(clones.IsInFlight(LocalPath("A")));
+		Assert.IsTrue(clones.TryStart(LocalPath("A")));
+	}
+
+	[TestMethod]
+	public void NoRefreshIsRequestedUntilACloneCompletes()
+	{
+		CloneTracker clones = new();
+		Assert.IsTrue(clones.TryStart(LocalPath("A")));
+
+		Assert.IsFalse(clones.TakeRefreshRequest());
+	}
+
+	[TestMethod]
+	public async Task ACloneCompletedOnAWorkerThreadIsRefreshedByTheCallerThatTakesTheRequestOnce()
+	{
+		CloneTracker clones = new();
+		Assert.IsTrue(clones.TryStart(LocalPath("A")));
+
+		await Task.Run(() => clones.Complete(LocalPath("A"))).ConfigureAwait(false);
+
+		Assert.IsTrue(clones.TakeRefreshRequest());
+		Assert.IsFalse(clones.TakeRefreshRequest());
+	}
+
+	[TestMethod]
+	public async Task TryRunRefusesASecondCloneWhileTheFirstRunsAndRequestsARefreshWhenItEnds()
+	{
+		CloneTracker clones = new();
+		using ManualResetEventSlim release = new();
+
+		Assert.IsTrue(clones.TryRun(LocalPath("A"), release.Wait, out Task first));
+		Assert.IsTrue(clones.IsInFlight(LocalPath("A")));
+		Assert.IsFalse(clones.TryRun(LocalPath("A"), () => Assert.Fail("A duplicate clone ran."), out Task refused));
+		Assert.IsTrue(refused.IsCompleted);
+
+		release.Set();
+		await first.ConfigureAwait(false);
+
+		Assert.IsFalse(clones.IsInFlight(LocalPath("A")));
+		Assert.IsTrue(clones.TakeRefreshRequest());
+	}
+
+	[TestMethod]
+	public async Task ACloneThatThrowsIsStillRecordedAsComplete()
+	{
+		CloneTracker clones = new();
+
+		Assert.IsTrue(clones.TryRun(LocalPath("A"), () => throw new InvalidOperationException("clone failed"), out Task run));
+		_ = await Assert.ThrowsExactlyAsync<InvalidOperationException>(() => run).ConfigureAwait(false);
+
+		Assert.IsFalse(clones.IsInFlight(LocalPath("A")));
+		Assert.IsTrue(clones.TakeRefreshRequest());
+	}
+
+	[TestMethod]
+	public void RefreshIfRequestedRefreshesOnceForACompletedClone()
+	{
+		CloneTracker clones = new();
+		int refreshes = 0;
+
+		clones.RefreshIfRequested(() => refreshes++);
+		Assert.AreEqual(0, refreshes);
+
+		clones.Complete(LocalPath("A"));
+		clones.RefreshIfRequested(() => refreshes++);
+		clones.RefreshIfRequested(() => refreshes++);
+
+		Assert.AreEqual(1, refreshes);
+	}
+
+	[TestMethod]
+	public void MakeCloneClonesTheRemoteIntoTheFolderAndLogsTheResult()
+	{
+		string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_{Guid.NewGuid():N}");
+		try
+		{
+			string origin = Path.Join(root, "origin");
+			string clone = Path.Join(root, "clone");
+			Assert.IsTrue(GitCli.Run("init", origin).Succeeded, "git init failed.");
+			string? description = null;
+			GitResult? result = null;
+
+			ProjectDirector.MakeClone(
+				GitRemotePath.Create<GitRemotePath>(origin),
+				FullyQualifiedLocalRepoPath.Create<FullyQualifiedLocalRepoPath>(clone),
+				(d, r) => (description, result) = (d, r))();
+
+			Assert.AreEqual($"Cloning {origin}", description);
+			Assert.IsNotNull(result);
+			Assert.IsTrue(result.Succeeded);
+			Assert.IsTrue(GitCli.IsRepository(clone));
+		}
+		finally
+		{
+			TryDeleteDirectory(root);
+		}
+	}
+
+	private static void TryDeleteDirectory(string path)
+	{
+		try
+		{
+			// Git marks objects read-only, which blocks a plain recursive delete on Windows.
+			foreach (string file in Directory.EnumerateFiles(path, "*", SearchOption.AllDirectories))
+			{
+				File.SetAttributes(file, FileAttributes.Normal);
+			}
+
+			Directory.Delete(path, recursive: true);
+		}
+		catch (IOException)
+		{
+			// A best-effort cleanup of a temp directory is not worth failing a test over.
+		}
+		catch (UnauthorizedAccessException)
+		{
+			// As above.
+		}
+	}
+}

ProjectDirector/CloneTracker.cs

@@ -0,0 +1,120 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector;
+
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using System.Threading.Tasks;
+
+/// <summary>
+/// Tracks the clones running in the background and hands their completion back to the render thread.
+/// </summary>
+/// <remarks>
+/// A clone finishes on a worker thread, and refreshing the page from there mutated
+/// <see cref="ProjectDirectorOptions.ClonedRepos"/>, <see cref="ProjectDirectorOptions.Repos"/> and the
+/// repo browser while the render thread was enumerating them. So a finished clone only records that a
+/// refresh is wanted, and the tick that runs on the render thread takes the request and refreshes.
+/// The same record of what is in flight keeps a second click from starting another clone into the
+/// folder the first one is still writing.
+/// </remarks>
+internal sealed class CloneTracker
+{
+	private readonly HashSet<FullyQualifiedLocalRepoPath> inFlight = [];
+	private readonly Lock gate = new();
+	private int refreshRequested;
+
+	/// <summary>
+	/// Records a clone into <paramref name="localPath"/> as started, unless one already is.
+	/// </summary>
+	/// <param name="localPath">The folder being cloned into.</param>
+	/// <returns><see langword="true"/> if the caller should start the clone.</returns>
+	internal bool TryStart(FullyQualifiedLocalRepoPath localPath)
+	{
+		lock (gate)
+		{
+			return inFlight.Add(localPath);
+		}
+	}
+
+	/// <summary>
+	/// Gets a value indicating whether a clone into <paramref name="localPath"/> is running.
+	/// </summary>
+	/// <param name="localPath">The folder to ask about.</param>
+	/// <returns><see langword="true"/> while the clone runs.</returns>
+	internal bool IsInFlight(FullyQualifiedLocalRepoPath localPath)
+	{
+		lock (gate)
+		{
+			return inFlight.Contains(localPath);
+		}
+	}
+
+	/// <summary>
+	/// Records a clone as finished, whether or not it succeeded, and asks for a refresh. Safe to call
+	/// from any thread.
+	/// </summary>
+	/// <param name="localPath">The folder that was cloned into.</param>
+	internal void Complete(FullyQualifiedLocalRepoPath localPath)
+	{
+		lock (gate)
+		{
+			_ = inFlight.Remove(localPath);
+		}
+
+		_ = Interlocked.Exchange(ref refreshRequested, 1);
+	}
+
+	/// <summary>
+	/// Runs <paramref name="clone"/> on the thread pool, unless a clone into <paramref name="localPath"/>
+	/// is already running.
+	/// </summary>
+	/// <param name="localPath">The folder being cloned into.</param>
+	/// <param name="clone">The clone itself. It must not touch UI state.</param>
+	/// <param name="run">The running clone, or a completed task if the clone was refused.</param>
+	/// <returns><see langword="true"/> if the clone was started.</returns>
+	/// <remarks>
+	/// The clone is recorded as complete however it ends, so a clone that throws can be retried.
+	/// </remarks>
+	internal bool TryRun(FullyQualifiedLocalRepoPath localPath, Action clone, out Task run)
+	{
+		if (!TryStart(localPath))
+		{
+			run = Task.CompletedTask;
+			return false;
+		}
+
+		run = Task.Run(() =>
+		{
+			try
+			{
+				clone();
+			}
+			finally
+			{
+				Complete(localPath);
+			}
+		});
+
+		return true;
+	}
+
+	/// <summary>
+	/// Calls <paramref name="refresh"/> if a clone has completed since the last call. Call from the
+	/// render thread.
+	/// </summary>
+	/// <param name="refresh">Refreshes the page.</param>
+	internal void RefreshIfRequested(Action refresh)
+	{
+		if (TakeRefreshRequest())
+		{
+			refresh();
+		}
+	}
+
+	/// <summary>
+	/// Takes the pending refresh request, if any. Call from the render thread.
+	/// </summary>
+	/// <returns><see langword="true"/> once for each run of completions since the last call.</returns>
+	internal bool TakeRefreshRequest() => Interlocked.Exchange(ref refreshRequested, 0) != 0;
+}

ProjectDirector/ProjectDirector.cs

@@ -18,7 +18,7 @@
 using ktsu.ImGui.Widgets;
 using ktsu.ImGui.Styler;
 using Octokit;
 // using OpenAI.Chat;
 using Semantics.Paths;
 
 #pragma warning disable CA1506
@@ -56,7 +56,12 @@
 	/// </summary>
 	private GitHubOwnerName? OwnerPendingTokenPopup { get; set; }
 
+	/// <summary>
+	/// The clones running in the background, whose completion the tick hands to <see cref="RefreshPage"/>.
+	/// </summary>
+	private CloneTracker Clones { get; } = new();
+
 	// private ChatClient ChatClient { get; init; }
 
 	private static void Main(string[] _)
 	{
@@ -79,7 +84,7 @@
 		_ = MakeLoadedOptionsSafe(Options, QueueLog);
 
 		Options.Save();
 		// ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken));
 		DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerCols = new("VerticalDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerRows = new("HorizontalDivider", DividerResized, ImGuiWidgets.DividerLayout.Rows);
@@ -681,6 +686,8 @@
 				result => QueueLogIfAny(ApplyOwnerToken(owner, result)));
 		}
 
+		Clones.RefreshIfRequested(RefreshPage);
+
 		_ = PopupSetDevDirectory.ShowIfOpen();
 		_ = PopupAddNewGitHubOwner.ShowIfOpen();
 		_ = PopupSetGitHubOwnerToken.ShowIfOpen();
@@ -703,13 +710,11 @@
 			{
 				if (!Options.ClonedRepos.ContainsValue(Options.BaseRepo))
 				{
-					if (ImGui.Button("Clone", new Vector2(FieldWidth, 0)))
+					// The button is hidden while its clone runs, and the page is refreshed by the next
+					// tick, on the render thread, rather than by the clone.
+					if (!Clones.IsInFlight(repo.LocalPath) && ImGui.Button("Clone", new Vector2(FieldWidth, 0)))
 					{
-						Task.Run(() => QueueGitLog($"Cloning {repo.RemotePath}", GitCli.Run("clone", repo.RemotePath.ToString(), repo.LocalPath.ToString())))
-						.ContinueWith((t) => RefreshPage(),
-						new CancellationToken(),
-						TaskContinuationOptions.OnlyOnRanToCompletion | TaskContinuationOptions.ExecuteSynchronously,
-						TaskScheduler.Current);
+						_ = Clones.TryRun(repo.LocalPath, MakeClone(repo.RemotePath, repo.LocalPath, QueueGitLog), out _);
 					}
 				}
 				else
@@ -738,7 +743,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -794,6 +799,16 @@
 		}
 	}
 
+	/// <summary>
+	/// Makes the clone the Clone button runs in the background.
+	/// </summary>
+	/// <param name="remotePath">The repository to clone.</param>
+	/// <param name="localPath">The folder to clone it into.</param>
+	/// <param name="log">Records the result. Called from the thread the clone runs on.</param>
+	/// <returns>The clone, which touches no UI state.</returns>
+	internal static Action MakeClone(GitRemotePath remotePath, FullyQualifiedLocalRepoPath localPath, Action<string, GitResult> log) =>
+		() => log($"Cloning {remotePath}", GitCli.Run("clone", remotePath.ToString(), localPath.ToString()));
+
 	private void RefreshPage()
 	{
 		if (Options.Repos.TryGetValue(Options.BaseRepo, out GitRepository? repo))
@@ -2170,7 +2185,7 @@
 
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;