From 1f62a89c3d56a683a79022610189470e86b167a5 Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 22 Sep 2026 07:37:12 +0000 Subject: [PATCH 1/5] fix: compare a repo against its siblings off the render thread [patch] Selecting a repository ran UpdateSimilarRepos inline on the ImGui thread. For each sibling that is two git ls-files subprocesses, a ReadAllText of every shared file from both working trees, and a whole-file DiffPlex pass. At the dozens of sibling repositories this tool is for, that is on the order of 60 process launches before the frame can render, on every click. Fetch, pull, push and clone all background their git calls for exactly this reason; this path did not. It now runs on a Task, publishing the whole comparison in one assignment so a reader on the render thread sees the previous answer or the next one and never a half-built dictionary. Each run carries a token and only the newest may publish, because clicking through several repositories leaves that many runs in flight and they do not finish in the order they started. The base repository's tracked-file list is now read once per run instead of once per sibling, which is N + 1 calls to git ls-files where there were 2N, with no cache to invalidate since it does not outlive the run. Requesting a comparison drops the previous one rather than leaving it on screen, since it describes whichever repository was selected before. The three render paths that indexed SimilarRepoDiffs directly now ask, and draw a pending line while a run is outstanding. Fixes ktsu-dev/ProjectDirector#414 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JnRJuBBrg4LLgytqvkRmLJ --- ProjectDirector/GitRepository.cs | 59 ++++++++++++- ProjectDirector/ProjectDirector.cs | 137 +++++++++++++++++++++++++---- 2 files changed, 177 insertions(+), 19 deletions(-) diff --git a/ProjectDirector/GitRepository.cs b/ProjectDirector/GitRepository.cs index 6bf6a13..25ba9b5 100644 --- a/ProjectDirector/GitRepository.cs +++ b/ProjectDirector/GitRepository.cs @@ -21,8 +21,65 @@ public abstract class GitRepository public bool IsCloned => Directory.Exists(LocalPath); + private Dictionary> similarRepoDiffs = []; + private int similarRepoRequested; + private int similarRepoApplied; + + /// + /// The last completed comparison of this repository against its siblings. + /// + /// + /// Published as a whole by rather than filled in place, + /// because the comparison runs on a background task while the render thread reads this. A + /// reader therefore sees either the previous answer or the next one, never a half-built + /// dictionary. The inner dictionaries stay writable so a single-file re-diff can update one + /// entry without redoing the whole run. + /// + [JsonIgnore] + public IReadOnlyDictionary> SimilarRepoDiffs => Volatile.Read(ref similarRepoDiffs); + + /// + /// Whether a comparison has been asked for and has not yet published its answer. + /// [JsonIgnore] - public Dictionary> SimilarRepoDiffs { get; } = []; + public bool SimilarReposPending => Volatile.Read(ref similarRepoApplied) != Volatile.Read(ref similarRepoRequested); + + /// + /// Discards the current comparison and returns a token identifying the new request. + /// + /// The token to hand back to . + /// + /// The old answer is dropped rather than left on screen: it describes whichever repository was + /// selected before, so showing it while the next one is computed is worse than showing nothing. + /// + internal int RequestSimilarRepoDiffs() + { + Volatile.Write(ref similarRepoDiffs, []); + return Interlocked.Increment(ref similarRepoRequested); + } + + /// + /// Publishes the result of a comparison, unless a newer one has since been requested. + /// + /// The token from . + /// The comparison to publish. + /// when the result was published, when a newer request superseded it. + /// + /// Clicking through several repositories leaves that many comparisons in flight, and they do + /// not finish in the order they started. Only the newest may publish, so a slow earlier run + /// cannot overwrite the answer for the repository the user is actually looking at. + /// + internal bool TryApplySimilarRepoDiffs(int token, Dictionary> diffs) + { + if (Volatile.Read(ref similarRepoRequested) != token) + { + return false; + } + + Volatile.Write(ref similarRepoDiffs, diffs); + Volatile.Write(ref similarRepoApplied, token); + return true; + } public static GitRepository? Create(GitRemotePath remotePath, FullyQualifiedLocalRepoPath localPath) { diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 783008c..36abe69 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -6,6 +6,7 @@ namespace ktsu.ProjectDirector; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Numerics; using System.Text; @@ -78,15 +79,19 @@ public ProjectDirector() { GitRepository repoA = Options.Repos[Options.BaseRepo]; GitRepository repoB = Options.Repos[Options.CompareRepo]; - DiffResult diff = repoA.SimilarRepoDiffs[Options.CompareRepo][Options.CompareFile]; - ShowDiffLeft(repoA, repoB, diff); + if (TryGetDiff(repoA, Options.CompareRepo, Options.CompareFile, out DiffResult? diff)) + { + ShowDiffLeft(repoA, repoB, diff); + } }); DividerDiff.Add("Right", 0.50f, (dt) => { GitRepository repoA = Options.Repos[Options.BaseRepo]; GitRepository repoB = Options.Repos[Options.CompareRepo]; - DiffResult diff = repoA.SimilarRepoDiffs[Options.CompareRepo][Options.CompareFile]; - ShowDiffRight(repoA, repoB, diff); + if (TryGetDiff(repoA, Options.CompareRepo, Options.CompareFile, out DiffResult? diff)) + { + ShowDiffRight(repoA, repoB, diff); + } }); RestoreDividerStates(); @@ -1115,27 +1120,75 @@ private void ScanRemoteAccountsForRepos() private static FullyQualifiedLocalRepoPath MakeFullyQualifyLocalRepoPath(AbsoluteDirectoryPath localPath) => FullyQualifiedLocalRepoPath.Create(Path.GetFullPath(localPath).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)); + /// + /// Compares a repository against every sibling, on a background task. + /// + /// + /// This runs a pair of git subprocesses and a whole-file diff per sibling, so at the "dozens of + /// sibling repositories" this tool is for it is far too much work to do between frames. Fetch, + /// pull, push and clone all background their git calls for the same reason; this path did not, + /// and it sits on the most frequent interaction there is, selecting a repository. + /// private void UpdateSimilarRepos(GitRepository repo) { + // The sibling list is snapshotted here rather than inside the task, so the background work + // never enumerates Options.Repos while the UI can add to or remove from it. Resolving the + // names here also keeps the unsupported-repository throw on the calling thread, where it is + // still observable, rather than on a task nobody awaits. + Collection> others = []; foreach ((FullyQualifiedGitHubRepoName _, GitRepository otherRepo) in Options.Repos) { if (repo != otherRepo) { - Dictionary diffs = DiffRepos(repo, otherRepo); - if (otherRepo is GitHubRepository gitHubRepo) - { - FullyQualifiedGitHubRepoName otherRepoName = GetFullyQualifiedRepoName(gitHubRepo.OwnerName, gitHubRepo.RepoName); - repo.SimilarRepoDiffs[otherRepoName] = diffs; - } - else - { - throw new InvalidOperationException("Only GitHub Repos are supported at this time"); - } + others.Add(otherRepo is GitHubRepository gitHubRepo + ? new(GetFullyQualifiedRepoName(gitHubRepo.OwnerName, gitHubRepo.RepoName), otherRepo) + : throw new InvalidOperationException("Only GitHub Repos are supported at this time")); + } + } + + int token = repo.RequestSimilarRepoDiffs(); + Task task = new(() => + { + Dictionary> diffs = DiffAgainstAll(repo, others); + if (repo.TryApplySimilarRepoDiffs(token, diffs)) + { + QueueLog($"[{DateTimeOffset.Now}] Compared {repo.RemotePath} against {others.Count} other {(others.Count == 1 ? "repository" : "repositories")}"); } + }); + + task.Start(); + } + + /// + /// Compares one repository against a set of siblings, returning the diffs keyed by sibling. + /// + /// The repository every sibling is compared against. + /// The siblings, paired with the names to key the result by. + /// The diffs for each sibling. + /// + /// The base repository's tracked-file list is read once for the whole run rather than once per + /// sibling, which is what DiffRepos used to do. Across N siblings that is N + 1 calls to + /// git ls-files where there were 2N, and the list cannot go stale mid-run because it does + /// not outlive one. + /// + internal static Dictionary> DiffAgainstAll( + GitRepository repo, + IEnumerable> others) + { + Collection trackedFiles = GitCli.IsRepository(repo.LocalPath) + ? GitCli.ListTrackedFiles(repo.LocalPath) + : []; + + Dictionary> diffs = []; + foreach ((FullyQualifiedGitHubRepoName otherRepoName, GitRepository otherRepo) in others) + { + diffs[otherRepoName] = DiffRepos(repo, trackedFiles, otherRepo); } + + return diffs; } - private static Dictionary DiffRepos(GitRepository repoA, GitRepository repoB) + private static Dictionary DiffRepos(GitRepository repoA, IEnumerable trackedFilesA, GitRepository repoB) { Dictionary diffs = []; @@ -1144,7 +1197,7 @@ private static Dictionary DiffRepos(GitRepository return diffs; } - Collection matches = GitCli.ListTrackedFiles(repoA.LocalPath) + Collection matches = trackedFilesA .Intersect(GitCli.ListTrackedFiles(repoB.LocalPath)) .ToCollection(); @@ -1179,6 +1232,29 @@ private static string ReadFileOrEmpty(string repoPath, string relativePath) } } + /// + /// Looks up one file's diff against a sibling repository. + /// + /// The base repository holding the comparison. + /// The sibling to compare against. + /// The file whose diff is wanted. + /// The diff, when one has been computed. + /// when a diff is available to draw. + /// + /// Every caller of this is a render path, and a comparison now runs in the background, so there + /// is a window in which no diff exists yet. Asking rather than indexing is what keeps a refresh + /// from taking the window down while the user is looking at a file. + /// + private static bool TryGetDiff(GitRepository repo, FullyQualifiedGitHubRepoName otherRepoName, RelativeFilePath filePath, [NotNullWhen(true)] out DiffResult? diff) + { + diff = repo.SimilarRepoDiffs.TryGetValue(otherRepoName, out Dictionary? diffs) + && diffs.TryGetValue(filePath, out DiffResult? found) + ? found + : null; + + return diff is not null; + } + private static DiffResult DiffSingleFile(GitRepository repoA, GitRepository repoB, RelativeFilePath filePath) { if (repoA == repoB || !GitCli.IsRepository(repoA.LocalPath) || !GitCli.IsRepository(repoB.LocalPath)) @@ -1198,7 +1274,15 @@ private static void RefreshFileDiff(GitRepository repoA, GitRepository repoB, Re if (repoB is GitHubRepository gitHubRepo) { FullyQualifiedGitHubRepoName otherRepoName = GetFullyQualifiedRepoName(gitHubRepo.OwnerName, gitHubRepo.RepoName); - repoA.SimilarRepoDiffs[otherRepoName][filePath] = diff; + + // A whole-repository comparison can publish between the frame deciding to draw this + // file and the button being pressed, replacing the dictionary this was about to write + // into. The new comparison already read the file from disk, so dropping this update is + // the right answer rather than reinstating an entry that run left out. + if (repoA.SimilarRepoDiffs.TryGetValue(otherRepoName, out Dictionary? diffs)) + { + diffs[filePath] = diff; + } } else { @@ -1208,6 +1292,12 @@ private static void RefreshFileDiff(GitRepository repoA, GitRepository repoB, Re private void ShowSimilarRepos(GitRepository repo) { + if (repo.SimilarReposPending) + { + ImGui.TextUnformatted("Comparing repositories..."); + return; + } + IEnumerable sortedRepos = repo.SimilarRepoDiffs .ToDictionary(kvp => kvp.Key, kvp => kvp.Value.Sum(x => x.Value.DiffBlocks.Sum(y => y.InsertCountB + y.DeleteCountA) > 0 ? 30 : 70)) .OrderByDescending(kvp => kvp.Value) @@ -1263,6 +1353,12 @@ private void ShowComparedRepo(GitRepository repo) ImGui.SameLine(); ImGui.TextUnformatted($"Comparing {Options.BaseRepo} vs {Options.CompareRepo}"); + if (repo.SimilarReposPending) + { + ImGui.TextUnformatted("Comparing repositories..."); + return; + } + if (repo.SimilarRepoDiffs.TryGetValue(Options.CompareRepo, out Dictionary? diffs)) { IEnumerable sortedDiffs = diffs @@ -1326,7 +1422,12 @@ private void ShowComparedFile(float dt, GitRepository repo) ImGui.TextUnformatted($"Comparing {Options.BaseRepo} vs {Options.CompareRepo}"); ImGui.SameLine(); - DiffResult diff = repo.SimilarRepoDiffs[Options.CompareRepo][Options.CompareFile]; + if (!TryGetDiff(repo, Options.CompareRepo, Options.CompareFile, out DiffResult? diff)) + { + ImGui.TextUnformatted("Comparing repositories..."); + return; + } + ShowWholeDiffSummary(diff); ImGui.PushStyleVar(ImGuiStyleVar.WindowPadding, new Vector2(0, 0)); From 60567c848d3a2c10f7195f1eb92b7dbf72fbf8ac Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 22 Sep 2026 07:37:12 +0000 Subject: [PATCH 2/5] test: cover which comparison wins and what shows while one runs [patch] The request/publish pair is plain and takes no ImGui context, so the ordering rules are driven directly: a superseded run cannot publish, a late one cannot overwrite a newer published answer, and a request clears what was on screen. DiffAgainstAll is covered against throwaway repositories the way GitCliTests drives GitCli, including the not-yet-cloned case, which now runs where a throw would go unobserved. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JnRJuBBrg4LLgytqvkRmLJ --- ProjectDirector.Test/SimilarReposTests.cs | 248 ++++++++++++++++++++++ 1 file changed, 248 insertions(+) create mode 100644 ProjectDirector.Test/SimilarReposTests.cs diff --git a/ProjectDirector.Test/SimilarReposTests.cs b/ProjectDirector.Test/SimilarReposTests.cs new file mode 100644 index 0000000..3a048c3 --- /dev/null +++ b/ProjectDirector.Test/SimilarReposTests.cs @@ -0,0 +1,248 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System; +using System.Collections.Generic; +using System.IO; + +using DiffPlex.Model; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Semantics.Paths; + +/// +/// Covers comparing a repository against its siblings, which moved off the render thread. +/// +/// +/// The comparison is a pair of git subprocesses and a whole-file diff per sibling, so it runs on a +/// background task now. That makes two things worth pinning: which answer is on screen while a run +/// is in flight, and which run wins when several are. Both live on as +/// plain methods so they can be driven without a live ImGui context, as the repository's other +/// decision logic already is. +/// +[TestClass] +public sealed class SimilarReposTests +{ + private static GitHubRepository Repository(string localPath, string repoName) => new() + { + OwnerName = GitHubOwnerName.Create("ktsu-dev"), + RepoName = GitHubRepoName.Create(repoName), + LocalPath = FullyQualifiedLocalRepoPath.Create(localPath), + }; + + private static FullyQualifiedGitHubRepoName Name(string repoName) => + FullyQualifiedGitHubRepoName.Create($"ktsu-dev.{repoName}"); + + private static Dictionary> DiffsNaming(string repoName) => + new() { [Name(repoName)] = [] }; + + [TestMethod] + public void ARepositoryIsNotPendingBeforeAnythingIsAsked() + { + GitHubRepository repo = Repository("/tmp/a", "A"); + + Assert.IsFalse(repo.SimilarReposPending); + Assert.IsEmpty(repo.SimilarRepoDiffs); + } + + [TestMethod] + public void RequestingAComparisonClearsTheLastAnswerAndReportsPending() + { + GitHubRepository repo = Repository("/tmp/a", "A"); + Assert.IsTrue(repo.TryApplySimilarRepoDiffs(repo.RequestSimilarRepoDiffs(), DiffsNaming("B"))); + + _ = repo.RequestSimilarRepoDiffs(); + + // A comparison describes whichever repository was selected when it ran, so leaving the + // previous one on screen while the next is computed would be actively misleading. + Assert.IsEmpty(repo.SimilarRepoDiffs, "The superseded answer stays on screen."); + Assert.IsTrue(repo.SimilarReposPending); + } + + [TestMethod] + public void ApplyingAComparisonPublishesItAndClearsPending() + { + GitHubRepository repo = Repository("/tmp/a", "A"); + + int token = repo.RequestSimilarRepoDiffs(); + bool applied = repo.TryApplySimilarRepoDiffs(token, DiffsNaming("B")); + + Assert.IsTrue(applied); + Assert.IsFalse(repo.SimilarReposPending); + Assert.IsTrue(repo.SimilarRepoDiffs.ContainsKey(Name("B"))); + } + + [TestMethod] + public void ASupersededComparisonCannotPublish() + { + GitHubRepository repo = Repository("/tmp/a", "A"); + + int first = repo.RequestSimilarRepoDiffs(); + int second = repo.RequestSimilarRepoDiffs(); + + // Clicking through several repositories leaves that many runs in flight, and they do not + // finish in the order they started. The stale one must not land on the newer selection. + Assert.IsFalse(repo.TryApplySimilarRepoDiffs(first, DiffsNaming("Stale")), "An older run published over a newer request."); + Assert.IsEmpty(repo.SimilarRepoDiffs); + Assert.IsTrue(repo.SimilarReposPending, "The newer request is still outstanding."); + + Assert.IsTrue(repo.TryApplySimilarRepoDiffs(second, DiffsNaming("Fresh"))); + Assert.IsTrue(repo.SimilarRepoDiffs.ContainsKey(Name("Fresh"))); + } + + [TestMethod] + public void ALateComparisonCannotOverwriteAnAlreadyPublishedNewerOne() + { + GitHubRepository repo = Repository("/tmp/a", "A"); + + int first = repo.RequestSimilarRepoDiffs(); + int second = repo.RequestSimilarRepoDiffs(); + Assert.IsTrue(repo.TryApplySimilarRepoDiffs(second, DiffsNaming("Fresh"))); + + Assert.IsFalse(repo.TryApplySimilarRepoDiffs(first, DiffsNaming("Stale"))); + Assert.IsTrue(repo.SimilarRepoDiffs.ContainsKey(Name("Fresh")), "A late run replaced the answer the user is looking at."); + } + + [TestMethod] + public void ComparingAgainstSiblingsKeysEveryOneOfThemEvenWhenNothingIsShared() + { + string a = CreateRepository([("shared.txt", "one\n"), ("only-a.txt", "a\n")]); + string b = CreateRepository([("shared.txt", "two\n")]); + string c = CreateRepository([("unrelated.txt", "c\n")]); + + try + { + GitHubRepository repoA = Repository(a, "A"); + Dictionary> diffs = + ProjectDirector.DiffAgainstAll( + repoA, + [ + new(Name("B"), Repository(b, "B")), + new(Name("C"), Repository(c, "C")), + ]); + + Assert.AreEqual(2, diffs.Count); + + // Only the tracked files both repositories carry are diffed. + Assert.AreEqual(1, diffs[Name("B")].Count); + Assert.IsTrue(diffs[Name("B")].ContainsKey(RelativeFilePath.Create("shared.txt"))); + Assert.IsNotEmpty(diffs[Name("B")][RelativeFilePath.Create("shared.txt")].DiffBlocks, "shared.txt differs between the two."); + + // A sibling sharing nothing still gets an entry, because the similar-repos table counts + // its matches and would otherwise have no row to report zero on. + Assert.IsEmpty(diffs[Name("C")]); + } + finally + { + TryDeleteDirectory(a); + TryDeleteDirectory(b); + TryDeleteDirectory(c); + } + } + + [TestMethod] + public void IdenticalSharedFilesDiffToNoBlocks() + { + string a = CreateRepository([("shared.txt", "same\n")]); + string b = CreateRepository([("shared.txt", "same\n")]); + + try + { + Dictionary> diffs = + ProjectDirector.DiffAgainstAll(Repository(a, "A"), [new(Name("B"), Repository(b, "B"))]); + + // This is what the "Exact" column counts. + Assert.IsEmpty(diffs[Name("B")][RelativeFilePath.Create("shared.txt")].DiffBlocks); + } + finally + { + TryDeleteDirectory(a); + TryDeleteDirectory(b); + } + } + + [TestMethod] + public void ARepositoryThatIsNotCheckedOutComparesToNothingRatherThanThrowing() + { + string b = CreateRepository([("shared.txt", "one\n")]); + string missing = Path.Combine(Path.GetTempPath(), $"ktsu_pd_absent_{Guid.NewGuid():N}"); + + try + { + Dictionary> diffs = + ProjectDirector.DiffAgainstAll(Repository(missing, "Missing"), [new(Name("B"), Repository(b, "B"))]); + + // Repositories are listed before they are cloned, so this is an ordinary state rather + // than an error — and it now runs on a background task, where a throw goes unobserved. + Assert.IsEmpty(diffs[Name("B")]); + } + finally + { + TryDeleteDirectory(b); + } + } + + [TestMethod] + public void ComparingAgainstNoSiblingsProducesNoEntries() + { + string a = CreateRepository([("shared.txt", "one\n")]); + + try + { + Assert.IsEmpty(ProjectDirector.DiffAgainstAll(Repository(a, "A"), [])); + } + finally + { + TryDeleteDirectory(a); + } + } + + private static string CreateRepository(IEnumerable<(string RelativePath, string Contents)> files) + { + string root = Path.Combine(Path.GetTempPath(), $"ktsu_pd_similar_{Guid.NewGuid():N}"); + _ = Directory.CreateDirectory(root); + + Assert.IsTrue(GitCli.Run("init", root).Succeeded, "git init failed."); + + // Scope identity to this throwaway repository so the test neither depends on nor disturbs + // whatever global configuration the machine happens to carry. + Assert.IsTrue(GitCli.RunIn(root, "config", "user.name", "ProjectDirector").Succeeded); + Assert.IsTrue(GitCli.RunIn(root, "config", "user.email", "ProjectDirector@ktsu.dev").Succeeded); + + foreach ((string relativePath, string contents) in files) + { + File.WriteAllText(Path.Combine(root, relativePath), contents); + } + + Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed."); + + GitResult committed = GitCli.RunIn(root, "commit", "-m", "Add files"); + Assert.IsTrue(committed.Succeeded, $"git commit failed: {committed.FailureText}"); + + return 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) + { + // Covers a missing directory too. A best-effort cleanup of a temp directory is not + // worth failing a test over. + } + catch (UnauthorizedAccessException) + { + } + } +} From 916b8a8e524ab415a29498da5077a8add89cf2e7 Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 22 Sep 2026 07:47:07 +0000 Subject: [PATCH 3/5] refactor: lift the comparison's rules out of the render paths [patch] Only the request/publish pair and DiffAgainstAll were reachable from the test project, leaving the rest of the change in the ImGui layer this repo does not unit-test. CLAUDE.md's answer to that is to pull the part with a rule in it into a plain method, so: - PairSiblings resolves the sibling names and rejects a repository that is not on GitHub, which is why that throw stays on the calling thread. - CompareSiblingsAsync starts the background task and returns it, so a caller that cares can wait for the comparison rather than guess. UpdateSimilarRepos is now one line handing it Options.Repos and QueueLog. - FindDiff replaces the TryGetDiff out-parameter, so the two divider callbacks pass the nullable straight into ShowDiffLeft/Right, which already guard it, instead of repeating the check. - RefreshFileDiff is internal, so its supersession guard is driven directly. The three panels that draw a comparison are mutually exclusive, so the pending state is decided once where they are dispatched instead of at the top of each. New-code line coverage over the changed files goes 34.7% -> 83.6% locally, against SonarCloud's required 80%. What is left uncovered is the render sites themselves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JnRJuBBrg4LLgytqvkRmLJ --- ProjectDirector.Test/SimilarReposTests.cs | 165 ++++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 113 ++++++++++----- 2 files changed, 240 insertions(+), 38 deletions(-) diff --git a/ProjectDirector.Test/SimilarReposTests.cs b/ProjectDirector.Test/SimilarReposTests.cs index 3a048c3..5b82099 100644 --- a/ProjectDirector.Test/SimilarReposTests.cs +++ b/ProjectDirector.Test/SimilarReposTests.cs @@ -3,8 +3,12 @@ namespace ktsu.ProjectDirector.Test; using System; +using System.Collections.Concurrent; using System.Collections.Generic; +using System.Collections.ObjectModel; using System.IO; +using System.Linq; +using System.Threading.Tasks; using DiffPlex.Model; @@ -199,6 +203,167 @@ public void ComparingAgainstNoSiblingsProducesNoEntries() } } + [TestMethod] + public void PairingSiblingsSkipsTheRepositoryBeingComparedAndNamesTheRest() + { + GitHubRepository repoA = Repository("/tmp/a", "A"); + GitHubRepository repoB = Repository("/tmp/b", "B"); + + Collection> pairs = + ProjectDirector.PairSiblings(repoA, [repoA, repoB]); + + Assert.AreEqual(1, pairs.Count, "A repository is not its own sibling."); + Assert.AreEqual(Name("B"), pairs[0].Key); + Assert.AreSame(repoB, pairs[0].Value); + } + + [TestMethod] + public void PairingSiblingsRejectsARepositoryThatIsNotOnGitHub() + { + GitHubRepository repoA = Repository("/tmp/a", "A"); + AzureDevOpsRepository other = new() { LocalPath = FullyQualifiedLocalRepoPath.Create("/tmp/b") }; + + // The throw belongs here, on the calling thread, rather than inside the background task + // where nothing would observe it. + _ = Assert.ThrowsExactly(() => ProjectDirector.PairSiblings(repoA, [repoA, other])); + } + + [TestMethod] + public async Task ComparingSiblingsPublishesOffTheCallingThreadAndReportsWhatItDid() + { + string a = CreateRepository([("shared.txt", "one\n")]); + string b = CreateRepository([("shared.txt", "two\n")]); + + try + { + GitHubRepository repoA = Repository(a, "A"); + GitHubRepository repoB = Repository(b, "B"); + ConcurrentQueue log = []; + + Task task = ProjectDirector.CompareSiblingsAsync(repoA, [repoA, repoB], log.Enqueue); + + // The answer is not there yet, which is what the pending state renders instead of the + // comparison for whichever repository was selected before. + await task.ConfigureAwait(false); + + Assert.IsFalse(repoA.SimilarReposPending); + Assert.IsTrue(repoA.SimilarRepoDiffs.ContainsKey(Name("B"))); + Assert.HasCount(1, log); + StringAssert.Contains(log.Single(), "1 other repository", "One sibling is reported in the singular."); + } + finally + { + TryDeleteDirectory(a); + TryDeleteDirectory(b); + } + } + + [TestMethod] + public async Task ComparingSeveralSiblingsReportsThemInThePlural() + { + string a = CreateRepository([("shared.txt", "one\n")]); + string b = CreateRepository([("shared.txt", "two\n")]); + string c = CreateRepository([("shared.txt", "three\n")]); + + try + { + GitHubRepository repoA = Repository(a, "A"); + ConcurrentQueue log = []; + + await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, Repository(b, "B"), Repository(c, "C")], log.Enqueue).ConfigureAwait(false); + + Assert.AreEqual(2, repoA.SimilarRepoDiffs.Count); + StringAssert.Contains(log.Single(), "2 other repositories"); + } + finally + { + TryDeleteDirectory(a); + TryDeleteDirectory(b); + TryDeleteDirectory(c); + } + } + + [TestMethod] + public async Task NoDiffIsFoundWhileAComparisonIsStillRunning() + { + string a = CreateRepository([("shared.txt", "one\n")]); + string b = CreateRepository([("shared.txt", "two\n")]); + RelativeFilePath shared = RelativeFilePath.Create("shared.txt"); + + try + { + GitHubRepository repoA = Repository(a, "A"); + + // Nothing has been compared, so the render paths must find nothing rather than throw + // the KeyNotFoundException that indexing straight into the dictionary would. + Assert.IsNull(ProjectDirector.FindDiff(repoA, Name("B"), shared)); + + await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, Repository(b, "B")], _ => { }).ConfigureAwait(false); + + Assert.IsNotNull(ProjectDirector.FindDiff(repoA, Name("B"), shared)); + Assert.IsNull(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create("absent.txt")), "A file the two do not share has no diff."); + Assert.IsNull(ProjectDirector.FindDiff(repoA, Name("Unknown"), shared), "A repository that was never compared has no diffs at all."); + } + finally + { + TryDeleteDirectory(a); + TryDeleteDirectory(b); + } + } + + [TestMethod] + public async Task RefreshingOneFileUpdatesThatEntryAndLeavesTheRestAlone() + { + string a = CreateRepository([("shared.txt", "one\n"), ("other.txt", "x\n")]); + string b = CreateRepository([("shared.txt", "two\n"), ("other.txt", "y\n")]); + RelativeFilePath shared = RelativeFilePath.Create("shared.txt"); + + try + { + GitHubRepository repoA = Repository(a, "A"); + GitHubRepository repoB = Repository(b, "B"); + await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, repoB], _ => { }).ConfigureAwait(false); + + Assert.IsNotEmpty(ProjectDirector.FindDiff(repoA, Name("B"), shared)!.DiffBlocks); + + await File.WriteAllTextAsync(Path.Combine(b, "shared.txt"), "one\n").ConfigureAwait(false); + ProjectDirector.RefreshFileDiff(repoA, repoB, shared); + + Assert.IsEmpty(ProjectDirector.FindDiff(repoA, Name("B"), shared)!.DiffBlocks, "The re-diff should see the file the apply button just wrote."); + Assert.IsNotNull(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create("other.txt")), "The rest of the comparison is untouched."); + } + finally + { + TryDeleteDirectory(a); + TryDeleteDirectory(b); + } + } + + [TestMethod] + public void RefreshingOneFileIsDroppedWhenAWholeComparisonHasSupersededIt() + { + GitHubRepository repoA = Repository("/tmp/a", "A"); + GitHubRepository repoB = Repository("/tmp/b", "B"); + + // A comparison can publish between the frame deciding to draw a file and the apply button + // being pressed, replacing the dictionary this was about to write into. + _ = repoA.RequestSimilarRepoDiffs(); + + ProjectDirector.RefreshFileDiff(repoA, repoB, RelativeFilePath.Create("shared.txt")); + + Assert.IsEmpty(repoA.SimilarRepoDiffs, "Dropping the update beats reinstating an entry the newer run left out."); + } + + [TestMethod] + public void RefreshingOneFileRejectsARepositoryThatIsNotOnGitHub() + { + GitHubRepository repoA = Repository("/tmp/a", "A"); + AzureDevOpsRepository other = new() { LocalPath = FullyQualifiedLocalRepoPath.Create("/tmp/b") }; + + _ = Assert.ThrowsExactly( + () => ProjectDirector.RefreshFileDiff(repoA, other, RelativeFilePath.Create("shared.txt"))); + } + private static string CreateRepository(IEnumerable<(string RelativePath, string Contents)> files) { string root = Path.Combine(Path.GetTempPath(), $"ktsu_pd_similar_{Guid.NewGuid():N}"); diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 36abe69..57d1ae4 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -6,7 +6,6 @@ namespace ktsu.ProjectDirector; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.IO; using System.Numerics; using System.Text; @@ -26,6 +25,12 @@ namespace ktsu.ProjectDirector; internal sealed class ProjectDirector { + /// + /// Drawn wherever a comparison is outstanding, in place of the answer for whichever repository + /// was selected before. + /// + private const string PendingComparisonMessage = "Comparing repositories..."; + internal ProjectDirectorOptions Options { get; } private static float FieldWidth => ImGui.GetIO().DisplaySize.X * 0.15f; private DateTime LastSaveOptionsTime { get; set; } = DateTime.MinValue; @@ -79,19 +84,13 @@ public ProjectDirector() { GitRepository repoA = Options.Repos[Options.BaseRepo]; GitRepository repoB = Options.Repos[Options.CompareRepo]; - if (TryGetDiff(repoA, Options.CompareRepo, Options.CompareFile, out DiffResult? diff)) - { - ShowDiffLeft(repoA, repoB, diff); - } + ShowDiffLeft(repoA, repoB, FindDiff(repoA, Options.CompareRepo, Options.CompareFile)); }); DividerDiff.Add("Right", 0.50f, (dt) => { GitRepository repoA = Options.Repos[Options.BaseRepo]; GitRepository repoB = Options.Repos[Options.CompareRepo]; - if (TryGetDiff(repoA, Options.CompareRepo, Options.CompareFile, out DiffResult? diff)) - { - ShowDiffRight(repoA, repoB, diff); - } + ShowDiffRight(repoA, repoB, FindDiff(repoA, Options.CompareRepo, Options.CompareFile)); }); RestoreDividerStates(); @@ -641,7 +640,15 @@ private void ShowTopPanel(float dt) } }); - if (!Options.CompareFile.IsEmpty()) + // One comparison feeds all three of these panels, and exactly one of them draws, so the + // pending state is decided here rather than repeated inside each. Until the comparison + // publishes there is nothing to show but the previous repository's answer, which is + // worse than showing nothing. + if (repo.SimilarReposPending) + { + ShowCollapsiblePanel($"Similar Repos", () => ImGui.TextUnformatted(PendingComparisonMessage)); + } + else if (!Options.CompareFile.IsEmpty()) { ShowCollapsiblePanel($"Compare File", () => ShowComparedFile(dt, repo)); } @@ -1129,14 +1136,26 @@ private void ScanRemoteAccountsForRepos() /// pull, push and clone all background their git calls for the same reason; this path did not, /// and it sits on the most frequent interaction there is, selecting a repository. /// - private void UpdateSimilarRepos(GitRepository repo) + private void UpdateSimilarRepos(GitRepository repo) => _ = CompareSiblingsAsync(repo, Options.Repos.Values, QueueLog); + + /// + /// Pairs each sibling of with the name a comparison is keyed by. + /// + /// The repository being compared, which is not its own sibling. + /// Every known repository. + /// The siblings, paired with their fully qualified names. + /// A repository that is not a GitHub repository. + /// + /// Called before the background task starts rather than inside it, so the work never enumerates + /// Options.Repos while the UI can add to or remove from it, and so the unsupported-repository + /// throw lands on the calling thread where it is still observable rather than on a task nobody awaits. + /// + internal static Collection> PairSiblings(GitRepository repo, IEnumerable repos) { - // The sibling list is snapshotted here rather than inside the task, so the background work - // never enumerates Options.Repos while the UI can add to or remove from it. Resolving the - // names here also keeps the unsupported-repository throw on the calling thread, where it is - // still observable, rather than on a task nobody awaits. + Ensure.NotNull(repos); + Collection> others = []; - foreach ((FullyQualifiedGitHubRepoName _, GitRepository otherRepo) in Options.Repos) + foreach (GitRepository otherRepo in repos) { if (repo != otherRepo) { @@ -1146,17 +1165,42 @@ private void UpdateSimilarRepos(GitRepository repo) } } + return others; + } + + /// + /// Starts a comparison of a repository against every sibling, on a background task. + /// + /// The repository to compare. + /// Every known repository. + /// Reports the finished comparison, on the background thread. + /// The task running the comparison, so a caller that cares can wait for it. + /// + /// The comparison runs a pair of git subprocesses and a whole-file diff per sibling, so at the + /// "dozens of sibling repositories" this tool is for it is far too much work to do between + /// frames. Fetch, pull, push and clone all background their git calls for the same reason; this + /// path did not, and it sits on the most frequent interaction there is, selecting a repository. + /// The render thread discards the task, but returning it keeps the work observable. + /// + internal static Task CompareSiblingsAsync(GitRepository repo, IEnumerable repos, Action log) + { + Ensure.NotNull(repo); + Ensure.NotNull(log); + + Collection> others = PairSiblings(repo, repos); + int token = repo.RequestSimilarRepoDiffs(); Task task = new(() => { Dictionary> diffs = DiffAgainstAll(repo, others); if (repo.TryApplySimilarRepoDiffs(token, diffs)) { - QueueLog($"[{DateTimeOffset.Now}] Compared {repo.RemotePath} against {others.Count} other {(others.Count == 1 ? "repository" : "repositories")}"); + log($"[{DateTimeOffset.Now}] Compared {repo.RemotePath} against {others.Count} other {(others.Count == 1 ? "repository" : "repositories")}"); } }); task.Start(); + return task; } /// @@ -1238,21 +1282,20 @@ private static string ReadFileOrEmpty(string repoPath, string relativePath) /// The base repository holding the comparison. /// The sibling to compare against. /// The file whose diff is wanted. - /// The diff, when one has been computed. - /// when a diff is available to draw. + /// The diff, or when none has been computed. /// /// Every caller of this is a render path, and a comparison now runs in the background, so there /// is a window in which no diff exists yet. Asking rather than indexing is what keeps a refresh /// from taking the window down while the user is looking at a file. /// - private static bool TryGetDiff(GitRepository repo, FullyQualifiedGitHubRepoName otherRepoName, RelativeFilePath filePath, [NotNullWhen(true)] out DiffResult? diff) + internal static DiffResult? FindDiff(GitRepository repo, FullyQualifiedGitHubRepoName otherRepoName, RelativeFilePath filePath) { - diff = repo.SimilarRepoDiffs.TryGetValue(otherRepoName, out Dictionary? diffs) + Ensure.NotNull(repo); + + return repo.SimilarRepoDiffs.TryGetValue(otherRepoName, out Dictionary? diffs) && diffs.TryGetValue(filePath, out DiffResult? found) ? found : null; - - return diff is not null; } private static DiffResult DiffSingleFile(GitRepository repoA, GitRepository repoB, RelativeFilePath filePath) @@ -1268,7 +1311,13 @@ private static DiffResult DiffSingleFile(GitRepository repoA, GitRepository repo return Differ.Instance.CreateLineDiffs(fileContents, otherFileContents, ignoreWhitespace: false, ignoreCase: false); } - private static void RefreshFileDiff(GitRepository repoA, GitRepository repoB, RelativeFilePath filePath) + /// + /// Re-diffs one file after its content changed, leaving the rest of the comparison alone. + /// + /// The base repository holding the comparison. + /// The sibling the file is compared against. + /// The file to re-diff. + internal static void RefreshFileDiff(GitRepository repoA, GitRepository repoB, RelativeFilePath filePath) { DiffResult diff = DiffSingleFile(repoA, repoB, filePath); if (repoB is GitHubRepository gitHubRepo) @@ -1292,12 +1341,6 @@ private static void RefreshFileDiff(GitRepository repoA, GitRepository repoB, Re private void ShowSimilarRepos(GitRepository repo) { - if (repo.SimilarReposPending) - { - ImGui.TextUnformatted("Comparing repositories..."); - return; - } - IEnumerable sortedRepos = repo.SimilarRepoDiffs .ToDictionary(kvp => kvp.Key, kvp => kvp.Value.Sum(x => x.Value.DiffBlocks.Sum(y => y.InsertCountB + y.DeleteCountA) > 0 ? 30 : 70)) .OrderByDescending(kvp => kvp.Value) @@ -1353,12 +1396,6 @@ private void ShowComparedRepo(GitRepository repo) ImGui.SameLine(); ImGui.TextUnformatted($"Comparing {Options.BaseRepo} vs {Options.CompareRepo}"); - if (repo.SimilarReposPending) - { - ImGui.TextUnformatted("Comparing repositories..."); - return; - } - if (repo.SimilarRepoDiffs.TryGetValue(Options.CompareRepo, out Dictionary? diffs)) { IEnumerable sortedDiffs = diffs @@ -1422,9 +1459,9 @@ private void ShowComparedFile(float dt, GitRepository repo) ImGui.TextUnformatted($"Comparing {Options.BaseRepo} vs {Options.CompareRepo}"); ImGui.SameLine(); - if (!TryGetDiff(repo, Options.CompareRepo, Options.CompareFile, out DiffResult? diff)) + if (FindDiff(repo, Options.CompareRepo, Options.CompareFile) is not DiffResult diff) { - ImGui.TextUnformatted("Comparing repositories..."); + ImGui.TextUnformatted(PendingComparisonMessage); return; } From 1a85981e5414d38ada175c2f133f2aaf2394a60a Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 22 Sep 2026 07:49:14 +0000 Subject: [PATCH 4/5] test: address the code-quality findings on the new tests [patch] github-code-quality flagged four things in SimilarReposTests: - Path.Combine discards its base when a later segment turns out rooted. The names here are generated or literal and never rooted, but Path.Join has no such rule and reads the same. - A ContainsKey followed by an indexer is two lookups where TryGetValue is one. - The UnauthorizedAccessException handler was empty; it now carries the same best-effort rationale as the IOException one above it. Its fourth finding, that similarRepoDiffs can be readonly, is wrong: the field is reassigned through Volatile.Write, which needs a writable ref, so readonly does not compile. Replied on the thread rather than changing it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JnRJuBBrg4LLgytqvkRmLJ --- ProjectDirector.Test/SimilarReposTests.cs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ProjectDirector.Test/SimilarReposTests.cs b/ProjectDirector.Test/SimilarReposTests.cs index 5b82099..34d6bc0 100644 --- a/ProjectDirector.Test/SimilarReposTests.cs +++ b/ProjectDirector.Test/SimilarReposTests.cs @@ -131,8 +131,8 @@ public void ComparingAgainstSiblingsKeysEveryOneOfThemEvenWhenNothingIsShared() // Only the tracked files both repositories carry are diffed. Assert.AreEqual(1, diffs[Name("B")].Count); - Assert.IsTrue(diffs[Name("B")].ContainsKey(RelativeFilePath.Create("shared.txt"))); - Assert.IsNotEmpty(diffs[Name("B")][RelativeFilePath.Create("shared.txt")].DiffBlocks, "shared.txt differs between the two."); + Assert.IsTrue(diffs[Name("B")].TryGetValue(RelativeFilePath.Create("shared.txt"), out DiffResult? shared)); + Assert.IsNotEmpty(shared.DiffBlocks, "shared.txt differs between the two."); // A sibling sharing nothing still gets an entry, because the similar-repos table counts // its matches and would otherwise have no row to report zero on. @@ -171,7 +171,7 @@ public void IdenticalSharedFilesDiffToNoBlocks() public void ARepositoryThatIsNotCheckedOutComparesToNothingRatherThanThrowing() { string b = CreateRepository([("shared.txt", "one\n")]); - string missing = Path.Combine(Path.GetTempPath(), $"ktsu_pd_absent_{Guid.NewGuid():N}"); + string missing = Path.Join(Path.GetTempPath(), $"ktsu_pd_absent_{Guid.NewGuid():N}"); try { @@ -326,7 +326,7 @@ public async Task RefreshingOneFileUpdatesThatEntryAndLeavesTheRestAlone() Assert.IsNotEmpty(ProjectDirector.FindDiff(repoA, Name("B"), shared)!.DiffBlocks); - await File.WriteAllTextAsync(Path.Combine(b, "shared.txt"), "one\n").ConfigureAwait(false); + await File.WriteAllTextAsync(Path.Join(b, "shared.txt"), "one\n").ConfigureAwait(false); ProjectDirector.RefreshFileDiff(repoA, repoB, shared); Assert.IsEmpty(ProjectDirector.FindDiff(repoA, Name("B"), shared)!.DiffBlocks, "The re-diff should see the file the apply button just wrote."); @@ -366,7 +366,7 @@ public void RefreshingOneFileRejectsARepositoryThatIsNotOnGitHub() private static string CreateRepository(IEnumerable<(string RelativePath, string Contents)> files) { - string root = Path.Combine(Path.GetTempPath(), $"ktsu_pd_similar_{Guid.NewGuid():N}"); + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_similar_{Guid.NewGuid():N}"); _ = Directory.CreateDirectory(root); Assert.IsTrue(GitCli.Run("init", root).Succeeded, "git init failed."); @@ -378,7 +378,7 @@ private static string CreateRepository(IEnumerable<(string RelativePath, string foreach ((string relativePath, string contents) in files) { - File.WriteAllText(Path.Combine(root, relativePath), contents); + File.WriteAllText(Path.Join(root, relativePath), contents); } Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed."); @@ -408,6 +408,8 @@ private static void TryDeleteDirectory(string path) } catch (UnauthorizedAccessException) { + // Same best-effort rationale as above: a temp directory that will not delete is not + // worth failing a test over. } } } From d9abb53bdb9f9b9acd050fd663797dae08e10b44 Mon Sep 17 00:00:00 2001 From: matt-edmondson Date: Tue, 22 Sep 2026 07:52:04 +0000 Subject: [PATCH 5/5] refactor: filter the siblings with Where instead of an inner if [patch] github-code-quality flagged PairSiblings' foreach as implicitly filtering its sequence. Same change this repo has already made to the branch switches and the element scans. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JnRJuBBrg4LLgytqvkRmLJ --- ProjectDirector/ProjectDirector.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 57d1ae4..405e120 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -1155,14 +1155,11 @@ internal static Collection> others = []; - foreach (GitRepository otherRepo in repos) + foreach (GitRepository otherRepo in repos.Where(otherRepo => repo != otherRepo)) { - if (repo != otherRepo) - { - others.Add(otherRepo is GitHubRepository gitHubRepo - ? new(GetFullyQualifiedRepoName(gitHubRepo.OwnerName, gitHubRepo.RepoName), otherRepo) - : throw new InvalidOperationException("Only GitHub Repos are supported at this time")); - } + others.Add(otherRepo is GitHubRepository gitHubRepo + ? new(GetFullyQualifiedRepoName(gitHubRepo.OwnerName, gitHubRepo.RepoName), otherRepo) + : throw new InvalidOperationException("Only GitHub Repos are supported at this time")); } return others;