From ba8c1a530167e6b2f90791468e9d38707e2377d2 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 31 Aug 2026 17:14:45 -0700 Subject: [PATCH 1/7] Make linked worktrees identifiable in recents and the window title DiffViewer already opened linked worktrees correctly, but nothing on screen distinguished one from another. Recent-context rows were titled from the path leaf, which for a worktree is usually the branch name and never names the repository, so two worktrees of one repo rendered near-identically. Adds a libgit2-free GitWorktreeLayout helper that resolves the commondir pointer, plus IGitWorktreeEnumerator for listing every worktree attached to a repository. Two details are non-obvious: libgit2 omits the main worktree from its worktree list, so it is synthesized from commondir; and a pruned worktree is still listed but throws on every property access, so names come from the administrative directory instead. RepositoryShape now carries CommonGitDirectory and WorktreeName, and recent rows persist the repository and worktree names captured at launch time. Rows written by earlier versions load with null labels and fall back to the old path-leaf rendering, healing on next launch. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- CHANGELOG.md | 12 + .../RecentContexts/RecentContextItemTests.cs | 64 +++- .../RecentsJsonSerializerTests.cs | 65 +++++ .../LibGit2GitWorktreeEnumeratorTests.cs | 182 ++++++++++++ DiffViewer.Tests/Services/PreDiffPassTests.cs | 2 +- .../Services/RepositoryServiceTests.cs | 38 +++ DiffViewer.Tests/Services/TempRepo.cs | 56 ++++ .../Utility/GitWorktreeLayoutTests.cs | 201 +++++++++++++ .../ViewModels/DiffPaneViewModelTests.cs | 2 +- .../MainViewModelCommitMetadataTests.cs | 5 +- .../MainViewModelContextMenuTests.cs | 3 +- .../MainViewModelKeyboardShortcutTests.cs | 5 +- .../MainViewModelWindowTitleTests.cs | 59 ++++ DiffViewer/Models/RecentLaunchContext.cs | 16 +- DiffViewer/Models/RepositoryShape.cs | 20 +- DiffViewer/Services/IGitWorktreeEnumerator.cs | 76 +++++ .../Services/LibGit2GitWorktreeEnumerator.cs | 275 ++++++++++++++++++ DiffViewer/Services/RecentContextsService.cs | 10 +- DiffViewer/Services/RecentsJsonSerializer.cs | 16 +- DiffViewer/Services/RepositoryService.cs | 9 +- DiffViewer/Utility/GitWorktreeLayout.cs | 201 +++++++++++++ DiffViewer/ViewModels/MainViewModel.cs | 17 +- .../ViewModels/RecentContextsViewModel.cs | 27 +- 23 files changed, 1342 insertions(+), 19 deletions(-) create mode 100644 DiffViewer.Tests/Services/LibGit2GitWorktreeEnumeratorTests.cs create mode 100644 DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs create mode 100644 DiffViewer.Tests/ViewModels/MainViewModelWindowTitleTests.cs create mode 100644 DiffViewer/Services/IGitWorktreeEnumerator.cs create mode 100644 DiffViewer/Services/LibGit2GitWorktreeEnumerator.cs create mode 100644 DiffViewer/Utility/GitWorktreeLayout.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 95dffd1..d2887ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,18 @@ body. Keep section headings exact and write notes in Markdown. ## [Unreleased] +### Changed + +- **Worktrees are now identifiable in the UI.** DiffViewer already + worked when pointed at a linked worktree, but nothing on screen said + so. The recent-contexts dropdown labelled rows with the leaf + directory name, which for a worktree is usually the branch name and + never mentions the repository — two worktrees of one repo were + effectively indistinguishable. Recent rows now render as + `DiffViewer [feature-x] · HEAD → WT`, and the window title appends + the worktree name after the path. Rows saved by earlier versions keep + working and pick up their labels the next time that diff is opened. + ## [1.9.0] - 2026-06-06 ### Added diff --git a/DiffViewer.Tests/RecentContexts/RecentContextItemTests.cs b/DiffViewer.Tests/RecentContexts/RecentContextItemTests.cs index 4f6bfca..b8f5edd 100644 --- a/DiffViewer.Tests/RecentContexts/RecentContextItemTests.cs +++ b/DiffViewer.Tests/RecentContexts/RecentContextItemTests.cs @@ -58,12 +58,72 @@ public void Tooltip_PrRow_IncludesPullRequestUrl() item.Tooltip.Should().Contain("Last resolved head:"); } - private static RecentLaunchContext MakeLocal(string repoPath, string leftRef) + [Fact] + public void Title_WhenRowPointsAtALinkedWorktree_ShowsRepositoryAndWorktreeName() + { + // The worktree directory is named after the branch and lives + // nowhere near the repo, so the path leaf alone ("wt-feature-x") + // never mentions the repository the user is working in. + var ctx = MakeLocal( + @"C:\worktrees\wt-feature-x", + leftRef: "HEAD", + repositoryName: "DiffViewer", + worktreeName: "feature-x"); + var item = new RecentContextItem(ctx); + + item.Title.Should().Be("DiffViewer [feature-x] · HEAD → WT"); + } + + [Fact] + public void Title_WhenRowPointsAtTheMainWorktree_OmitsTheBracketedName() + { + var ctx = MakeLocal( + @"C:\repos\diffviewer", + leftRef: "main", + repositoryName: "DiffViewer"); + var item = new RecentContextItem(ctx); + + item.Title.Should().Be("DiffViewer · main → WT"); + } + + [Fact] + public void Title_WhenRowPredatesWorktreeLabels_FallsBackToThePathLeaf() + { + // Rows written by an older binary carry no labels; they must keep + // rendering rather than showing a blank repository name. + var ctx = MakeLocal(@"C:\repos\diffviewer", leftRef: "main"); + var item = new RecentContextItem(ctx); + + item.Title.Should().Be("diffviewer · main → WT"); + } + + [Fact] + public void Title_ForAPullRequestRowInAWorktree_StillShowsTheWorktreeName() + { + var left = new DiffSide.CommitIsh("abc1234"); + var right = new DiffSide.CommitIsh("def5678"); + var id = ContextIdentityFactory.Create(@"C:\worktrees\wt-feature-x", left, right); + var ctx = new RecentLaunchContext( + id, left, right, DateTimeOffset.UtcNow, + new PullRequestRef("github.com", "geevensingh", "diffviewer", 42), + RepositoryName: "DiffViewer", + WorktreeName: "feature-x"); + + new RecentContextItem(ctx).Title + .Should().Be("DiffViewer [feature-x] · PR geevensingh/diffviewer#42"); + } + + private static RecentLaunchContext MakeLocal( + string repoPath, + string leftRef, + string? repositoryName = null, + string? worktreeName = null) { var left = new DiffSide.CommitIsh(leftRef); var right = new DiffSide.WorkingTree(); var id = ContextIdentityFactory.Create(repoPath, left, right); - return new RecentLaunchContext(id, left, right, DateTimeOffset.UtcNow); + return new RecentLaunchContext( + id, left, right, DateTimeOffset.UtcNow, null, repositoryName, worktreeName); } private static RecentLaunchContext MakePr(string repoPath, PullRequestRef pr) diff --git a/DiffViewer.Tests/RecentContexts/RecentsJsonSerializerTests.cs b/DiffViewer.Tests/RecentContexts/RecentsJsonSerializerTests.cs index 51ef573..1a01b4c 100644 --- a/DiffViewer.Tests/RecentContexts/RecentsJsonSerializerTests.cs +++ b/DiffViewer.Tests/RecentContexts/RecentsJsonSerializerTests.cs @@ -11,6 +11,71 @@ namespace DiffViewer.Tests.RecentContexts; public class RecentsJsonSerializerTests { + [Fact] + public void RoundTrip_PreservesWorktreeLabels() + { + var left = new DiffSide.CommitIsh("HEAD"); + var right = new DiffSide.WorkingTree(); + var doc = new RecentsDoc(RecentsDoc.CurrentVersion, new[] + { + new RecentLaunchContext( + ContextIdentityFactory.Create(@"C:\worktrees\wt-feature-x", left, right), + left, right, DateTimeOffset.UtcNow, null, + RepositoryName: "DiffViewer", + WorktreeName: "feature-x"), + }); + + var roundTripped = RecentsJsonSerializer.Deserialize(RecentsJsonSerializer.Serialize(doc)); + + roundTripped.Items[0].RepositoryName.Should().Be("DiffViewer"); + roundTripped.Items[0].WorktreeName.Should().Be("feature-x"); + } + + [Fact] + public void Serialize_ForAMainWorktreeRow_OmitsTheWorktreeName() + { + var left = new DiffSide.CommitIsh("main"); + var right = new DiffSide.WorkingTree(); + var doc = new RecentsDoc(RecentsDoc.CurrentVersion, new[] + { + new RecentLaunchContext( + ContextIdentityFactory.Create(@"C:\repos\diffviewer", left, right), + left, right, DateTimeOffset.UtcNow, null, + RepositoryName: "DiffViewer"), + }); + + var json = RecentsJsonSerializer.Serialize(doc); + + json.Should().Contain("\"repositoryName\": \"DiffViewer\""); + json.Should().NotContain("worktreeName"); + } + + [Fact] + public void Deserialize_ForAFileWrittenBeforeWorktreeLabels_LeavesThemNull() + { + // A row from an older binary has neither key. It must still load + // - the labels are display sugar, not identity. + var json = """ + { + "version": 2, + "items": [ + { + "repoPath": "C:\\repos\\foo", + "left": { "type": "commit", "reference": "main" }, + "right": { "type": "workingTree" }, + "lastUsedUtc": "2026-05-14T18:00:00.0000000Z" + } + ] + } + """; + + var doc = RecentsJsonSerializer.Deserialize(json); + + doc.Items.Should().ContainSingle(); + doc.Items[0].RepositoryName.Should().BeNull(); + doc.Items[0].WorktreeName.Should().BeNull(); + } + [Fact] public void RoundTrip_PreservesAllFields_ForMixedSides() { diff --git a/DiffViewer.Tests/Services/LibGit2GitWorktreeEnumeratorTests.cs b/DiffViewer.Tests/Services/LibGit2GitWorktreeEnumeratorTests.cs new file mode 100644 index 0000000..b369363 --- /dev/null +++ b/DiffViewer.Tests/Services/LibGit2GitWorktreeEnumeratorTests.cs @@ -0,0 +1,182 @@ +using System; +using System.IO; +using System.Linq; +using DiffViewer.Services; +using FluentAssertions; +using Xunit; + +namespace DiffViewer.Tests.Services; + +/// +/// Behavioral tests for against +/// real on-disk repositories. The interesting cases are the ones libgit2 +/// does not hand us directly: the main worktree (absent from the +/// worktree list) and prunable worktrees (whose directory is gone). +/// +public sealed class LibGit2GitWorktreeEnumeratorTests : IDisposable +{ + private readonly TempRepo _repo = new(); + private readonly LibGit2GitWorktreeEnumerator _enumerator = new(); + + public LibGit2GitWorktreeEnumeratorTests() + { + _repo.WriteFile("a.txt", "hello\n"); + _repo.InitialCommit(); + } + + public void Dispose() => _repo.Dispose(); + + [Fact] + public void Enumerate_WhenRepoHasNoLinkedWorktrees_ReturnsOnlyTheMainWorktree() + { + var result = _enumerator.Enumerate(_repo.Path); + + result.Should().ContainSingle(); + result[0].IsMain.Should().BeTrue(); + result[0].IsCurrent.Should().BeTrue(); + result[0].IsMissing.Should().BeFalse(); + SamePath(result[0].WorkingDirectory, _repo.Path).Should().BeTrue(); + } + + [Fact] + public void Enumerate_WhenLinkedWorktreeExists_ReturnsBothMainAndLinked() + { + var worktreePath = _repo.AddWorktree("feature-a"); + + var result = _enumerator.Enumerate(_repo.Path); + + result.Should().HaveCount(2); + result.Should().ContainSingle(e => e.IsMain); + var linked = result.Single(e => !e.IsMain); + linked.Name.Should().Be("feature-a"); + SamePath(linked.WorkingDirectory, worktreePath).Should().BeTrue(); + } + + [Fact] + public void Enumerate_AlwaysListsTheMainWorktreeFirst() + { + _repo.AddWorktree("zzz-last"); + _repo.AddWorktree("aaa-first"); + + var result = _enumerator.Enumerate(_repo.Path); + + result[0].IsMain.Should().BeTrue(); + result.Skip(1).Select(e => e.Name).Should().ContainInOrder("aaa-first", "zzz-last"); + } + + [Fact] + public void Enumerate_FromInsideLinkedWorktree_StillReportsTheMainWorktree() + { + // libgit2 lists only *linked* worktrees, so the main entry has to + // be synthesized by resolving the commondir pointer. This is the + // case that regresses if that resolution breaks. + var worktreePath = _repo.AddWorktree("feature-a"); + + var result = _enumerator.Enumerate(worktreePath); + + result.Should().HaveCount(2); + var main = result.Single(e => e.IsMain); + SamePath(main.WorkingDirectory, _repo.Path).Should().BeTrue(); + } + + [Fact] + public void Enumerate_FromInsideLinkedWorktree_ReturnsTheSameWorktreeSetAsFromMain() + { + _repo.AddWorktree("feature-a"); + var second = _repo.AddWorktree("feature-b"); + + var fromMain = _enumerator.Enumerate(_repo.Path); + var fromLinked = _enumerator.Enumerate(second); + + fromLinked.Select(e => e.WorkingDirectory.ToLowerInvariant()) + .Should().BeEquivalentTo(fromMain.Select(e => e.WorkingDirectory.ToLowerInvariant())); + } + + [Fact] + public void Enumerate_MarksOnlyTheQueriedWorktreeAsCurrent() + { + var worktreePath = _repo.AddWorktree("feature-a"); + + var result = _enumerator.Enumerate(worktreePath); + + result.Should().ContainSingle(e => e.IsCurrent); + SamePath(result.Single(e => e.IsCurrent).WorkingDirectory, worktreePath).Should().BeTrue(); + } + + [Fact] + public void Enumerate_ReportsTheBranchCheckedOutInEachWorktree() + { + _repo.AddWorktree("feature-a"); + + var result = _enumerator.Enumerate(_repo.Path); + + // `git worktree add ` creates and checks out a branch of + // the same name; the main worktree keeps its own branch. The two + // are necessarily different — git forbids sharing a branch. + result.Single(e => !e.IsMain).HeadFriendlyName.Should().Be("feature-a"); + result.Single(e => e.IsMain).HeadFriendlyName.Should().NotBeNullOrWhiteSpace(); + result.Single(e => e.IsMain).HeadFriendlyName.Should().NotBe("feature-a"); + } + + [Fact] + public void Enumerate_WhenWorktreeDirectoryWasDeleted_StillReportsItAsMissing() + { + var worktreePath = _repo.AddWorktree("feature-a"); + TempRepo.DeleteDirectory(worktreePath); + + var result = _enumerator.Enumerate(_repo.Path); + + var linked = result.Single(e => !e.IsMain); + linked.IsMissing.Should().BeTrue(); + SamePath(linked.WorkingDirectory, worktreePath).Should().BeTrue(); + } + + [Fact] + public void Enumerate_WhenOneWorktreeIsMissing_StillReportsTheHealthyOnes() + { + var doomed = _repo.AddWorktree("feature-doomed"); + _repo.AddWorktree("feature-healthy"); + TempRepo.DeleteDirectory(doomed); + + var result = _enumerator.Enumerate(_repo.Path); + + result.Should().HaveCount(3); + result.Single(e => e.Name == "feature-healthy").IsMissing.Should().BeFalse(); + } + + [Fact] + public void Enumerate_WhenPathIsNotARepository_ReturnsEmpty() + { + var scratch = Path.Combine(Path.GetTempPath(), "diffviewer-not-a-repo-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(scratch); + try + { + _enumerator.Enumerate(scratch).Should().BeEmpty(); + } + finally + { + Directory.Delete(scratch, recursive: true); + } + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void Enumerate_WhenPathIsBlank_ReturnsEmpty(string path) + { + _enumerator.Enumerate(path).Should().BeEmpty(); + } + + [Fact] + public void Enumerate_WhenPathDoesNotExist_ReturnsEmpty() + { + _enumerator.Enumerate(Path.Combine(Path.GetTempPath(), "definitely-not-here-" + Guid.NewGuid().ToString("N"))) + .Should().BeEmpty(); + } + + private static bool SamePath(string a, string b) => + string.Equals( + Path.TrimEndingDirectorySeparator(Path.GetFullPath(a)), + Path.TrimEndingDirectorySeparator(Path.GetFullPath(b)), + StringComparison.OrdinalIgnoreCase); +} diff --git a/DiffViewer.Tests/Services/PreDiffPassTests.cs b/DiffViewer.Tests/Services/PreDiffPassTests.cs index ee9f96c..ccee193 100644 --- a/DiffViewer.Tests/Services/PreDiffPassTests.cs +++ b/DiffViewer.Tests/Services/PreDiffPassTests.cs @@ -375,7 +375,7 @@ private static FileEntryViewModel MakeEntry( private sealed class FakeRepo : IRepositoryService { - public RepositoryShape Shape => new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false); + public RepositoryShape Shape => new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false, @"C:\repo\.git"); public IReadOnlyList CurrentChanges { get; } = Array.Empty(); public event EventHandler? ChangeListUpdated { add { } remove { } } public event EventHandler? RepositoryLost { add { } remove { } } diff --git a/DiffViewer.Tests/Services/RepositoryServiceTests.cs b/DiffViewer.Tests/Services/RepositoryServiceTests.cs index 7afaebf..d75f58c 100644 --- a/DiffViewer.Tests/Services/RepositoryServiceTests.cs +++ b/DiffViewer.Tests/Services/RepositoryServiceTests.cs @@ -38,6 +38,44 @@ public void Shape_DetectsFreshRepoState() svc.Shape.WorkingDirectory.Should().NotBeNull(); } + [Fact] + public void Shape_ForAMainWorktree_ReportsNoWorktreeName() + { + using var t = new TempRepo(); + t.WriteFile("a.txt", "alpha\n"); + t.InitialCommit(); + using var svc = new RepositoryService(t.Path); + + svc.Shape.IsLinkedWorktree.Should().BeFalse(); + svc.Shape.WorktreeName.Should().BeNull(); + // For a main worktree the git dir *is* the common dir. + Normalize(svc.Shape.CommonGitDirectory).Should().Be(Normalize(svc.Shape.GitDir)); + } + + [Fact] + public void Shape_ForALinkedWorktree_ReportsItsNameAndTheSharedCommonDirectory() + { + using var t = new TempRepo(); + t.WriteFile("a.txt", "alpha\n"); + t.InitialCommit(); + var worktreePath = t.AddWorktree("feature-a"); + + using var mainSvc = new RepositoryService(t.Path); + using var worktreeSvc = new RepositoryService(worktreePath); + + worktreeSvc.Shape.IsLinkedWorktree.Should().BeTrue(); + worktreeSvc.Shape.WorktreeName.Should().Be("feature-a"); + // Both worktrees resolve to the same common dir - that is what + // makes "same repository, different checkout" recognizable. + Normalize(worktreeSvc.Shape.CommonGitDirectory) + .Should().Be(Normalize(mainSvc.Shape.CommonGitDirectory)); + Normalize(worktreeSvc.Shape.GitDir) + .Should().NotBe(Normalize(worktreeSvc.Shape.CommonGitDirectory)); + } + + private static string Normalize(string path) => + Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)).ToLowerInvariant(); + [Fact] public void EnumerateChanges_CommitVsCommit_ReturnsAddedDeletedModified() { diff --git a/DiffViewer.Tests/Services/TempRepo.cs b/DiffViewer.Tests/Services/TempRepo.cs index d9ffe6f..0da68b9 100644 --- a/DiffViewer.Tests/Services/TempRepo.cs +++ b/DiffViewer.Tests/Services/TempRepo.cs @@ -13,6 +13,7 @@ internal sealed class TempRepo : IDisposable { private readonly string _tempPath; private readonly Signature _author = new("Test", "test@example.com", DateTimeOffset.UtcNow); + private readonly List _worktreePaths = new(); public string Path => _tempPath; public Signature Author => _author; @@ -158,8 +159,43 @@ public Commit Stash(string? message = null) return stash.WorkTree; } + /// Create a linked worktree (git worktree add) in its + /// own temp directory outside this repo, and return its path. Requires + /// at least one commit on HEAD. When is + /// null, git creates a new branch named and + /// checks it out; otherwise the given committish is checked out. + /// The directory is cleaned up by . + public string AddWorktree(string name, string? committish = null) + { + var path = System.IO.Path.Combine( + System.IO.Path.GetTempPath(), + "diffviewer-test-wt-" + Guid.NewGuid().ToString("N")); + _worktreePaths.Add(path); + + using var repo = new Repository(_tempPath); + if (committish is null) + { + repo.Worktrees.Add(name, path, isLocked: false); + } + else + { + repo.Worktrees.Add(committish, name, path, isLocked: false); + } + return path; + } + + /// Delete a worktree's directory from disk without telling + /// git — the state git worktree list reports as "prunable". + /// Used to exercise the missing-worktree path. + public static void DeleteDirectory(string path) => ForceDelete(path); + public void Dispose() { + foreach (var worktreePath in _worktreePaths) + { + ForceDelete(worktreePath); + } + try { // LibGit2Sharp marks .git internals read-only on Windows; clear before delete. @@ -177,4 +213,24 @@ public void Dispose() // Best-effort cleanup - don't fail the test on temp-dir leak. } } + + /// Best-effort recursive delete that first clears the + /// read-only attribute LibGit2Sharp sets on git internals — plain + /// throws on those. + private static void ForceDelete(string path) + { + try + { + if (!Directory.Exists(path)) return; + foreach (var f in Directory.EnumerateFiles(path, "*", SearchOption.AllDirectories)) + { + try { File.SetAttributes(f, FileAttributes.Normal); } catch { } + } + Directory.Delete(path, recursive: true); + } + catch + { + // Best-effort cleanup - don't fail the test on temp-dir leak. + } + } } diff --git a/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs b/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs new file mode 100644 index 0000000..ab89b41 --- /dev/null +++ b/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs @@ -0,0 +1,201 @@ +using System; +using System.IO; +using DiffViewer.Utility; +using FluentAssertions; +using Xunit; + +namespace DiffViewer.Tests.Utility; + +/// +/// Tests for against synthetic on-disk +/// layouts. Real repositories are exercised by the enumerator and +/// repository-service tests; these cover the pure path reasoning, +/// including the layouts that are awkward to create for real (a bare +/// hub) and the malformed ones that must degrade rather than throw. +/// +public sealed class GitWorktreeLayoutTests : IDisposable +{ + private readonly string _root; + + public GitWorktreeLayoutTests() + { + _root = Path.Combine(Path.GetTempPath(), "diffviewer-layout-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(_root); + } + + public void Dispose() + { + try { Directory.Delete(_root, recursive: true); } catch { } + } + + [Fact] + public void IsLinkedWorktree_ForAMainWorktreeGitDirectory_IsFalse() + { + var gitDir = CreateDirectory(".git"); + + GitWorktreeLayout.IsLinkedWorktree(gitDir).Should().BeFalse(); + } + + [Fact] + public void IsLinkedWorktree_WhenCommondirPointerIsPresent_IsTrue() + { + var gitDir = CreateLinkedWorktreeGitDirectory("feature-a"); + + GitWorktreeLayout.IsLinkedWorktree(gitDir).Should().BeTrue(); + } + + [Fact] + public void ResolveCommonDirectory_ForAMainWorktree_ReturnsTheDirectoryUnchanged() + { + var gitDir = CreateDirectory(".git"); + + GitWorktreeLayout.ResolveCommonDirectory(gitDir).Should().Be(gitDir); + } + + [Fact] + public void ResolveCommonDirectory_FollowsARelativeCommondirPointer() + { + var gitDir = CreateLinkedWorktreeGitDirectory("feature-a"); + + var resolved = GitWorktreeLayout.ResolveCommonDirectory(gitDir); + + Normalize(resolved).Should().Be(Normalize(Path.Combine(_root, ".git"))); + } + + [Fact] + public void ResolveCommonDirectory_FollowsAnAbsoluteCommondirPointer() + { + var commonDir = CreateDirectory(".git"); + var gitDir = CreateDirectory(Path.Combine(".git", "worktrees", "feature-a")); + File.WriteAllText(Path.Combine(gitDir, "commondir"), commonDir); + + Normalize(GitWorktreeLayout.ResolveCommonDirectory(gitDir)).Should().Be(Normalize(commonDir)); + } + + [Fact] + public void ResolveCommonDirectory_WhenPointerIsEmpty_FallsBackToTheGivenDirectory() + { + var gitDir = CreateDirectory(Path.Combine(".git", "worktrees", "feature-a")); + File.WriteAllText(Path.Combine(gitDir, "commondir"), " \n"); + + GitWorktreeLayout.ResolveCommonDirectory(gitDir).Should().Be(gitDir); + } + + [Fact] + public void TryGetWorktreeName_ForALinkedWorktree_ReturnsTheAdministrativeDirectoryName() + { + var gitDir = CreateLinkedWorktreeGitDirectory("feature-a"); + + GitWorktreeLayout.TryGetWorktreeName(gitDir).Should().Be("feature-a"); + } + + [Fact] + public void TryGetWorktreeName_ForAMainWorktree_ReturnsNull() + { + var gitDir = CreateDirectory(".git"); + + GitWorktreeLayout.TryGetWorktreeName(gitDir).Should().BeNull(); + } + + [Fact] + public void TryGetRepositoryName_ForAStandardCheckout_ReturnsTheDirectoryHoldingDotGit() + { + GitWorktreeLayout.TryGetRepositoryName(@"C:\Repos\DiffViewer\.git").Should().Be("DiffViewer"); + } + + [Fact] + public void TryGetRepositoryName_IgnoresATrailingSeparator() + { + GitWorktreeLayout.TryGetRepositoryName(@"C:\Repos\DiffViewer\.git\").Should().Be("DiffViewer"); + } + + [Fact] + public void TryGetRepositoryName_ForABareHub_TrimsTheDotGitSuffix() + { + // The "one bare clone plus N worktrees" layout: the common + // directory is the bare repo itself, conventionally .git. + GitWorktreeLayout.TryGetRepositoryName(@"C:\Repos\DiffViewer.git").Should().Be("DiffViewer"); + } + + [Fact] + public void TryGetRepositoryName_ForABareHubWithoutTheSuffix_UsesTheDirectoryName() + { + GitWorktreeLayout.TryGetRepositoryName(@"C:\Repos\DiffViewer").Should().Be("DiffViewer"); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public void TryGetRepositoryName_WhenPathIsBlank_ReturnsNull(string path) + { + GitWorktreeLayout.TryGetRepositoryName(path).Should().BeNull(); + } + + [Fact] + public void Describe_ForAnOrdinaryCheckout_ReturnsTheRepositoryNameAndNoWorktreeName() + { + var workingDirectory = CreateDirectory("DiffViewer"); + Directory.CreateDirectory(Path.Combine(workingDirectory, ".git")); + + var labels = GitWorktreeLayout.Describe(workingDirectory); + + labels.RepositoryName.Should().Be("DiffViewer"); + labels.WorktreeName.Should().BeNull(); + } + + [Fact] + public void Describe_ForALinkedWorktree_ReturnsBothTheRepositoryAndWorktreeName() + { + // A linked worktree's .git is a *file* pointing at the shared + // repo's administrative directory - the layout that makes the + // repository name invisible from the worktree path alone. + CreateLinkedWorktreeGitDirectory("feature-x"); + var workingDirectory = CreateDirectory("some-unrelated-folder-name"); + File.WriteAllText( + Path.Combine(workingDirectory, ".git"), + $"gitdir: {Path.Combine(_root, ".git", "worktrees", "feature-x")}\n"); + + var labels = GitWorktreeLayout.Describe(workingDirectory); + + labels.RepositoryName.Should().Be(Path.GetFileName(_root)); + labels.WorktreeName.Should().Be("feature-x"); + } + + [Fact] + public void Describe_WhenPathIsNotARepository_ReturnsNothing() + { + var labels = GitWorktreeLayout.Describe(CreateDirectory("plain-folder")); + + labels.RepositoryName.Should().BeNull(); + labels.WorktreeName.Should().BeNull(); + } + + [Fact] + public void TryResolveGitDirectory_WhenPointerFileIsMalformed_ReturnsNull() + { + var workingDirectory = CreateDirectory("broken"); + File.WriteAllText(Path.Combine(workingDirectory, ".git"), "this is not a gitdir pointer"); + + GitWorktreeLayout.TryResolveGitDirectory(workingDirectory).Should().BeNull(); + } + + private string CreateDirectory(string relativePath) + { + var full = Path.Combine(_root, relativePath); + Directory.CreateDirectory(full); + return full; + } + + /// Build <root>/.git/worktrees/<name> with the + /// relative commondir pointer git itself writes. + private string CreateLinkedWorktreeGitDirectory(string name) + { + CreateDirectory(".git"); + var gitDir = CreateDirectory(Path.Combine(".git", "worktrees", name)); + File.WriteAllText(Path.Combine(gitDir, "commondir"), "../..\n"); + return gitDir; + } + + private static string Normalize(string path) => + Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)).ToLowerInvariant(); +} diff --git a/DiffViewer.Tests/ViewModels/DiffPaneViewModelTests.cs b/DiffViewer.Tests/ViewModels/DiffPaneViewModelTests.cs index c64022b..6ce3bee 100644 --- a/DiffViewer.Tests/ViewModels/DiffPaneViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/DiffPaneViewModelTests.cs @@ -2251,7 +2251,7 @@ private sealed class FakeRepository : IRepositoryService public bool RightIsBinaryOverride { get; set; } public int ReadCount; - public RepositoryShape Shape => new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false); + public RepositoryShape Shape => new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false, @"C:\repo\.git"); public IReadOnlyList CurrentChanges { get; } = Array.Empty(); public event EventHandler? ChangeListUpdated { add { } remove { } } diff --git a/DiffViewer.Tests/ViewModels/MainViewModelCommitMetadataTests.cs b/DiffViewer.Tests/ViewModels/MainViewModelCommitMetadataTests.cs index e983f82..4b89c49 100644 --- a/DiffViewer.Tests/ViewModels/MainViewModelCommitMetadataTests.cs +++ b/DiffViewer.Tests/ViewModels/MainViewModelCommitMetadataTests.cs @@ -58,7 +58,8 @@ private MainViewModel BuildVm( GitDir: Path.Combine(_repoRoot, ".git"), IsBare: false, IsHeadUnborn: false, IsSparseCheckout: false, IsPartialClone: false, - HasInProgressOperation: false); + HasInProgressOperation: false, + CommonGitDirectory: Path.Combine(_repoRoot, ".git")); return new MainViewModel( repository: repo, @@ -186,7 +187,7 @@ public void FriendlyName_FlowsFromService_ThroughPanelAndIntoDialog() private sealed class FakeRepo : IRepositoryService { - public RepositoryShape Shape_ { get; set; } = new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false); + public RepositoryShape Shape_ { get; set; } = new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false, @"C:\repo\.git"); public RepositoryShape Shape => Shape_; #pragma warning disable CS0067 diff --git a/DiffViewer.Tests/ViewModels/MainViewModelContextMenuTests.cs b/DiffViewer.Tests/ViewModels/MainViewModelContextMenuTests.cs index d52515a..621e088 100644 --- a/DiffViewer.Tests/ViewModels/MainViewModelContextMenuTests.cs +++ b/DiffViewer.Tests/ViewModels/MainViewModelContextMenuTests.cs @@ -615,7 +615,8 @@ public FakeRepositoryService(string repoRoot) => IsHeadUnborn: false, IsSparseCheckout: false, IsPartialClone: false, - HasInProgressOperation: false); + HasInProgressOperation: false, + CommonGitDirectory: System.IO.Path.Combine(repoRoot, ".git")); public RepositoryShape Shape { get; } diff --git a/DiffViewer.Tests/ViewModels/MainViewModelKeyboardShortcutTests.cs b/DiffViewer.Tests/ViewModels/MainViewModelKeyboardShortcutTests.cs index f68926a..a25e0c0 100644 --- a/DiffViewer.Tests/ViewModels/MainViewModelKeyboardShortcutTests.cs +++ b/DiffViewer.Tests/ViewModels/MainViewModelKeyboardShortcutTests.cs @@ -1042,7 +1042,8 @@ private static RepositoryShape MakeShape(string root) => GitDir: Path.Combine(root, ".git"), IsBare: false, IsHeadUnborn: false, IsSparseCheckout: false, IsPartialClone: false, - HasInProgressOperation: false); + HasInProgressOperation: false, + CommonGitDirectory: Path.Combine(root, ".git")); private static FileChange ModifiedChange(string path) => new(Path: path, OldPath: null, @@ -1124,7 +1125,7 @@ private static async Task RunOnUiSyncContextAsync( private sealed class FakeRepoForKeyboard : IRepositoryService { - public RepositoryShape Shape_ { get; set; } = new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false); + public RepositoryShape Shape_ { get; set; } = new(@"C:\repo", @"C:\repo", @"C:\repo\.git", false, false, false, false, false, @"C:\repo\.git"); public RepositoryShape Shape => Shape_; #pragma warning disable CS0067 diff --git a/DiffViewer.Tests/ViewModels/MainViewModelWindowTitleTests.cs b/DiffViewer.Tests/ViewModels/MainViewModelWindowTitleTests.cs new file mode 100644 index 0000000..4e5eda0 --- /dev/null +++ b/DiffViewer.Tests/ViewModels/MainViewModelWindowTitleTests.cs @@ -0,0 +1,59 @@ +using DiffViewer.Models; +using DiffViewer.ViewModels; +using FluentAssertions; +using Xunit; + +namespace DiffViewer.Tests.ViewModels; + +/// +/// Window-title composition. Kept as a static seam on +/// so it is testable without constructing the +/// whole per-context view-model graph. +/// +public class MainViewModelWindowTitleTests +{ + [Fact] + public void BuildWindowTitle_ForAMainWorktree_ShowsPathAndSides() + { + var title = MainViewModel.BuildWindowTitle( + MakeShape(@"C:\Repos\DiffViewer", worktreeName: null), + new DiffSide.CommitIsh("HEAD"), + new DiffSide.WorkingTree()); + + title.Should().Be(@"DiffViewer — C:\Repos\DiffViewer (HEAD ⇢ working tree)"); + } + + [Fact] + public void BuildWindowTitle_ForALinkedWorktree_AppendsTheWorktreeName() + { + var title = MainViewModel.BuildWindowTitle( + MakeShape(@"C:\worktrees\wt-feature-x", worktreeName: "feature-x"), + new DiffSide.CommitIsh("HEAD"), + new DiffSide.WorkingTree()); + + title.Should().Be(@"DiffViewer — C:\worktrees\wt-feature-x [feature-x] (HEAD ⇢ working tree)"); + } + + [Fact] + public void BuildWindowTitle_ForACommitComparison_RendersBothReferences() + { + var title = MainViewModel.BuildWindowTitle( + MakeShape(@"C:\Repos\DiffViewer", worktreeName: null), + new DiffSide.CommitIsh("main"), + new DiffSide.CommitIsh("feature/x")); + + title.Should().Be(@"DiffViewer — C:\Repos\DiffViewer (main ⇢ feature/x)"); + } + + private static RepositoryShape MakeShape(string repoRoot, string? worktreeName) => + new(RepoRoot: repoRoot, + WorkingDirectory: repoRoot, + GitDir: System.IO.Path.Combine(repoRoot, ".git"), + IsBare: false, + IsHeadUnborn: false, + IsSparseCheckout: false, + IsPartialClone: false, + HasInProgressOperation: false, + CommonGitDirectory: @"C:\Repos\DiffViewer\.git", + WorktreeName: worktreeName); +} diff --git a/DiffViewer/Models/RecentLaunchContext.cs b/DiffViewer/Models/RecentLaunchContext.cs index bff1520..6cc1b05 100644 --- a/DiffViewer/Models/RecentLaunchContext.cs +++ b/DiffViewer/Models/RecentLaunchContext.cs @@ -14,7 +14,7 @@ namespace DiffViewer.Models; /// the row re-resolves the review (heads can move between launches — /// see D8). /// -/// Equality is record-equality (all five members), but the +/// Equality is record-equality (all members), but the /// recents service dedups by Identity (plus /// (, ) /// when present) only: re-launching with a differently-cased path @@ -25,10 +25,22 @@ namespace DiffViewer.Models; /// #42 in the same repo remain separate rows. See /// RecentContextsService for that policy. /// +/// Name of the repository this row's path +/// belongs to (e.g. DiffViewer), captured at launch time. +/// null for rows written before worktree labelling existed, or +/// when the path could not be inspected; the UI falls back to the +/// path's leaf name. +/// Git's name for the linked worktree this +/// row points at, or null when the path is a repository's main +/// worktree. Two worktrees of one repository are separate rows with +/// identical ref labels, so this is what makes them tellable +/// apart. public sealed record RecentLaunchContext( ContextIdentity Identity, DiffSide LeftDisplay, DiffSide RightDisplay, DateTimeOffset LastUsedUtc, - IReviewRef? Review = null); + IReviewRef? Review = null, + string? RepositoryName = null, + string? WorktreeName = null); diff --git a/DiffViewer/Models/RepositoryShape.cs b/DiffViewer/Models/RepositoryShape.cs index fcbecb9..6861ba7 100644 --- a/DiffViewer/Models/RepositoryShape.cs +++ b/DiffViewer/Models/RepositoryShape.cs @@ -14,6 +14,15 @@ namespace DiffViewer.Models; /// True if core.sparseCheckout=true. /// True if any remote has promisor=true. /// True if a merge / rebase / cherry-pick / revert / stash-pop is in progress. +/// Absolute path to the repository's +/// common git directory — the one holding the object database +/// and shared refs. Equal to for a main +/// worktree; for a linked worktree it is the shared directory the +/// worktree points back at, and so is the same value for every worktree +/// of a repository. This is what identifies "the same repo" across +/// worktrees. +/// Git's name for this linked worktree, or +/// null when this is the repository's main worktree. public sealed record RepositoryShape( string RepoRoot, string? WorkingDirectory, @@ -22,4 +31,13 @@ public sealed record RepositoryShape( bool IsHeadUnborn, bool IsSparseCheckout, bool IsPartialClone, - bool HasInProgressOperation); + bool HasInProgressOperation, + string CommonGitDirectory, + string? WorktreeName = null) +{ + /// + /// True when this repository was opened through a linked worktree + /// (git worktree add) rather than its main checkout. + /// + public bool IsLinkedWorktree => WorktreeName is not null; +} diff --git a/DiffViewer/Services/IGitWorktreeEnumerator.cs b/DiffViewer/Services/IGitWorktreeEnumerator.cs new file mode 100644 index 0000000..c4f230c --- /dev/null +++ b/DiffViewer/Services/IGitWorktreeEnumerator.cs @@ -0,0 +1,76 @@ +using System.Collections.Generic; + +namespace DiffViewer.Services; + +/// +/// Stateless, repo-path-keyed enumeration of every worktree attached to +/// a Git repository — the main worktree plus every linked worktree +/// created by git worktree add. +/// +/// Why a sibling to rather than +/// a member of it: refs and worktrees answer different questions and +/// have different failure modes. Ref enumeration reads the object +/// database; worktree enumeration reads administrative files under +/// .git/worktrees and stats directories that may no longer +/// exist. Keeping them apart lets the worktree side own its +/// "prunable entry" semantics without complicating the ref result +/// shape. +/// +/// Error policy matches : +/// invalid paths, non-repos, and libgit2 errors return an empty list, +/// never a throw. A single unreadable worktree degrades to an entry +/// with set rather than losing +/// the whole enumeration. +/// +public interface IGitWorktreeEnumerator +{ + /// + /// Enumerate every worktree attached to the repository containing + /// . The main worktree sorts + /// first; linked worktrees follow, ordered by + /// . Returns an empty list (never + /// null) when the path doesn't resolve or libgit2 throws. + /// + IReadOnlyList Enumerate(string canonicalRepoPath); +} + +/// +/// One worktree returned by . +/// +/// Display name. For linked worktrees this is git's +/// own worktree name (the directory name under .git/worktrees, +/// which is normally — but not necessarily — the leaf of +/// ). Git does not name the main +/// worktree, so the main entry uses the leaf of its working +/// directory. +/// Absolute path to the worktree's +/// working directory, canonicalized with any trailing separator +/// trimmed so it compares equal to a +/// repo path. For a +/// missing worktree this is the last known path — the directory is +/// gone, but the path is still what the administrative files +/// record. +/// Friendly name of the branch checked +/// out in this worktree (e.g. master), or null when HEAD +/// is detached, unborn, or unreadable. Git forbids checking the same +/// branch out in two worktrees, so this doubles as a natural +/// disambiguator in pickers. +/// True for the repository's main worktree — the +/// one whose .git is a real directory rather than a pointer +/// file. False for every linked worktree. +/// True when this entry is the worktree that +/// the queried path itself resolved to. +/// True when the worktree is locked +/// (git worktree lock), meaning git will refuse to prune it. +/// True when the working directory no longer +/// exists on disk — git calls this "prunable". Surfaced rather than +/// filtered so a picker can show it as unavailable instead of +/// silently dropping a worktree the user believes exists. +public sealed record WorktreeEntry( + string Name, + string WorkingDirectory, + string? HeadFriendlyName, + bool IsMain, + bool IsCurrent, + bool IsLocked, + bool IsMissing); diff --git a/DiffViewer/Services/LibGit2GitWorktreeEnumerator.cs b/DiffViewer/Services/LibGit2GitWorktreeEnumerator.cs new file mode 100644 index 0000000..349e602 --- /dev/null +++ b/DiffViewer/Services/LibGit2GitWorktreeEnumerator.cs @@ -0,0 +1,275 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using DiffViewer.Models; +using DiffViewer.Utility; +using LibGit2Sharp; + +namespace DiffViewer.Services; + +/// +/// LibGit2Sharp-backed . Opens, +/// reads, closes — short-lived handles per +/// call. Safe to construct as an app-level singleton (no mutable state). +/// +/// The main worktree has to be synthesized. libgit2's +/// worktree list — like git worktree list --porcelain's +/// underlying data — enumerates only linked worktrees. The main +/// worktree is found instead by resolving the repository's common +/// directory: a linked worktree's git dir is +/// <common>/worktrees/<name> and contains a +/// commondir file pointing back (usually the relative string +/// ../..). Opening that common directory yields the main +/// worktree's working directory and HEAD. +/// +/// Bare hubs are supported. The "one bare clone plus N +/// worktrees" layout has no main working directory at all; libgit2 +/// reports a null working directory for the common dir and this +/// implementation simply omits the main entry rather than inventing +/// a path. +/// +public sealed class LibGit2GitWorktreeEnumerator : IGitWorktreeEnumerator +{ + public IReadOnlyList Enumerate(string canonicalRepoPath) + { + if (string.IsNullOrWhiteSpace(canonicalRepoPath)) return Array.Empty(); + + try + { + if (!Repository.IsValid(canonicalRepoPath)) return Array.Empty(); + + using var repo = new Repository(canonicalRepoPath); + + var gitDir = repo.Info.Path; + var commonDir = GitWorktreeLayout.ResolveCommonDirectory(gitDir); + var currentWorkingDirectory = Canonicalize(repo.Info.WorkingDirectory); + + var entries = new List(); + + var main = BuildMainEntry(repo, gitDir, commonDir, currentWorkingDirectory); + if (main is not null) entries.Add(main); + + entries.AddRange( + BuildLinkedEntries(repo, commonDir, currentWorkingDirectory) + .OrderBy(e => e.Name, StringComparer.Ordinal)); + + return entries; + } + catch (Exception) + { + return Array.Empty(); + } + } + + /// + /// Build the main-worktree entry, or null when the repository + /// has no main working directory (a bare hub). When the queried repo + /// is the main worktree, its already-open handle is reused + /// rather than reopening the same directory. + /// + private static WorktreeEntry? BuildMainEntry( + Repository repo, + string gitDir, + string commonDir, + string? currentWorkingDirectory) + { + var queriedRepoIsMain = PathsEqual(Canonicalize(gitDir), Canonicalize(commonDir)); + if (queriedRepoIsMain) + { + return DescribeMain(repo, currentWorkingDirectory); + } + + try + { + using var mainRepo = new Repository(commonDir); + return DescribeMain(mainRepo, currentWorkingDirectory); + } + catch (Exception) + { + // The common directory is unreadable or no longer a valid + // repository. Linked worktrees may still enumerate, so + // degrade to "no main entry" rather than failing the call. + return null; + } + } + + private static WorktreeEntry? DescribeMain(Repository mainRepo, string? currentWorkingDirectory) + { + var workingDirectory = Canonicalize(mainRepo.Info.WorkingDirectory); + if (workingDirectory is null) return null; + + return new WorktreeEntry( + Name: LeafName(workingDirectory), + WorkingDirectory: workingDirectory, + HeadFriendlyName: TryReadHeadName(mainRepo), + IsMain: true, + IsCurrent: PathsEqual(workingDirectory, currentWorkingDirectory), + IsLocked: false, + IsMissing: !Directory.Exists(workingDirectory)); + } + + /// + /// Describe every linked worktree. + /// + /// Names come from the administrative directory, not from + /// libgit2. A worktree whose directory has been deleted still + /// appears in libgit2's worktree list, but every property on it + /// throws — the underlying lookup fails and LibGit2Sharp dereferences + /// the null. Enumerating <common>/worktrees/* instead + /// gives a name for every worktree git knows about, healthy or + /// prunable, and libgit2 is then consulted only for the details it + /// can actually supply. + /// + private static IEnumerable BuildLinkedEntries( + Repository repo, + string commonDir, + string? currentWorkingDirectory) + { + string[] adminDirectories; + try + { + var worktreesRoot = Path.Combine(commonDir, "worktrees"); + if (!Directory.Exists(worktreesRoot)) yield break; + adminDirectories = Directory.GetDirectories(worktreesRoot); + } + catch (Exception) + { + yield break; + } + + foreach (var adminDirectory in adminDirectories) + { + var name = Path.GetFileName(adminDirectory); + if (string.IsNullOrEmpty(name)) continue; + + var entry = TryDescribeLinked(repo, adminDirectory, name, currentWorkingDirectory); + if (entry is not null) yield return entry; + } + } + + private static WorktreeEntry? TryDescribeLinked( + Repository repo, + string adminDirectory, + string name, + string? currentWorkingDirectory) + { + string? workingDirectory = null; + string? headName = null; + bool? isLocked = null; + + try + { + var worktree = repo.Worktrees[name]; + if (worktree is not null) + { + isLocked = worktree.IsLocked; + using var worktreeRepo = worktree.WorktreeRepository; + workingDirectory = Canonicalize(worktreeRepo.Info.WorkingDirectory); + headName = TryReadHeadName(worktreeRepo); + } + } + catch (Exception) + { + // Pruned worktree: libgit2 can't look it up, so fall through + // to the administrative files, which survive the deletion. + } + + workingDirectory ??= TryReadAdministrativeWorkingDirectory(adminDirectory); + if (workingDirectory is null) return null; + + return new WorktreeEntry( + Name: name, + WorkingDirectory: workingDirectory, + HeadFriendlyName: headName, + IsMain: false, + IsCurrent: PathsEqual(workingDirectory, currentWorkingDirectory), + IsLocked: isLocked ?? AdministrativeLockExists(adminDirectory), + IsMissing: !Directory.Exists(workingDirectory)); + } + + /// + /// Recover a linked worktree's working directory from + /// <admin>/gitdir, which records the path of the + /// worktree's .git pointer file. Its parent directory is the + /// working directory. This is the only source of truth once the + /// worktree directory itself is gone. + /// + private static string? TryReadAdministrativeWorkingDirectory(string adminDirectory) + { + try + { + var gitDirFile = Path.Combine(adminDirectory, "gitdir"); + if (!File.Exists(gitDirFile)) return null; + + var pointer = File.ReadAllText(gitDirFile).Trim(); + if (pointer.Length == 0) return null; + + var parent = Path.GetDirectoryName(Path.GetFullPath(pointer)); + return Canonicalize(parent); + } + catch (Exception) + { + return null; + } + } + + /// + /// Lock state for a worktree libgit2 could not look up. Git records + /// a lock as the presence of a locked file in the worktree's + /// administrative directory. + /// + private static bool AdministrativeLockExists(string adminDirectory) + { + try + { + return File.Exists(Path.Combine(adminDirectory, "locked")); + } + catch (Exception) + { + return false; + } + } + + /// + /// Friendly branch name of a worktree's HEAD, or null when + /// HEAD is detached, unborn, or unreadable. A detached HEAD's + /// friendly name is the useless literal "(no branch)", so it + /// is normalized away here rather than in the UI. + /// + private static string? TryReadHeadName(Repository repo) + { + try + { + if (repo.Info.IsHeadDetached || repo.Info.IsHeadUnborn) return null; + var name = repo.Head?.FriendlyName; + return string.IsNullOrWhiteSpace(name) ? null : name; + } + catch (Exception) + { + return null; + } + } + + private static string? Canonicalize(string? path) + { + if (string.IsNullOrWhiteSpace(path)) return null; + try + { + return ContextIdentityFactory.CanonicalizeRepoPath(path); + } + catch (Exception) + { + return null; + } + } + + private static string LeafName(string path) + { + var leaf = Path.GetFileName(path); + return string.IsNullOrEmpty(leaf) ? path : leaf; + } + + private static bool PathsEqual(string? a, string? b) => + a is not null && b is not null && string.Equals(a, b, StringComparison.OrdinalIgnoreCase); +} diff --git a/DiffViewer/Services/RecentContextsService.cs b/DiffViewer/Services/RecentContextsService.cs index 7ae63d1..089ba12 100644 --- a/DiffViewer/Services/RecentContextsService.cs +++ b/DiffViewer/Services/RecentContextsService.cs @@ -5,6 +5,7 @@ using System.Threading; using System.Threading.Tasks; using DiffViewer.Models; +using DiffViewer.Utility; namespace DiffViewer.Services; @@ -84,8 +85,15 @@ public async Task RecordLaunchAsync( await _gate.WaitAsync(ct).ConfigureAwait(false); try { + // Worktree labels are captured here rather than persisted by + // the caller because this is the moment the row is minted, + // and MergeAndCap replaces any existing row wholesale — so + // rows written by older binaries heal on next launch. + var labels = GitWorktreeLayout.Describe(identity.CanonicalRepoPath); + var fresh = new RecentLaunchContext( - identity, leftDisplay, rightDisplay, DateTimeOffset.UtcNow, review); + identity, leftDisplay, rightDisplay, DateTimeOffset.UtcNow, review, + labels.RepositoryName, labels.WorktreeName); var doc = await RecentsStore.ReadAndMutateAsync( _filePath, diff --git a/DiffViewer/Services/RecentsJsonSerializer.cs b/DiffViewer/Services/RecentsJsonSerializer.cs index b4dbb0a..6506cae 100644 --- a/DiffViewer/Services/RecentsJsonSerializer.cs +++ b/DiffViewer/Services/RecentsJsonSerializer.cs @@ -103,6 +103,17 @@ private static JsonObject SerializeItem(RecentLaunchContext item) // deserialises correctly (missing provider = "github"). row["pullRequest"] = SerializeReview(review); } + // Worktree labels are additive display metadata: omitted when + // unknown, and ignored by binaries that predate them. Both + // directions of version drift stay readable. + if (item.RepositoryName is { } repositoryName) + { + row["repositoryName"] = repositoryName; + } + if (item.WorktreeName is { } worktreeName) + { + row["worktreeName"] = worktreeName; + } return row; } @@ -153,7 +164,10 @@ private static JsonObject SerializeItem(RecentLaunchContext item) var pullRequest = TryDeserializeReview(obj["pullRequest"]); var identity = ContextIdentityFactory.Create(repoPath, left, right); - return new RecentLaunchContext(identity, left, right, lastUsed, pullRequest); + return new RecentLaunchContext( + identity, left, right, lastUsed, pullRequest, + TryString(obj, "repositoryName"), + TryString(obj, "worktreeName")); } private static IReviewRef? TryDeserializeReview(JsonNode? node) diff --git a/DiffViewer/Services/RepositoryService.cs b/DiffViewer/Services/RepositoryService.cs index fd74e0a..0887908 100644 --- a/DiffViewer/Services/RepositoryService.cs +++ b/DiffViewer/Services/RepositoryService.cs @@ -878,15 +878,20 @@ private static RepositoryShape BuildShape(Repository repo) } catch (LibGit2SharpException) { } + var gitDirectory = repo.Info.Path; + var commonGitDirectory = GitWorktreeLayout.ResolveCommonDirectory(gitDirectory); + return new RepositoryShape( RepoRoot: repo.Info.WorkingDirectory ?? repo.Info.Path, WorkingDirectory: repo.Info.WorkingDirectory, - GitDir: repo.Info.Path, + GitDir: gitDirectory, IsBare: repo.Info.IsBare, IsHeadUnborn: repo.Info.IsHeadUnborn, IsSparseCheckout: sparse, IsPartialClone: partialClone, - HasInProgressOperation: repo.Info.CurrentOperation != CurrentOperation.None); + HasInProgressOperation: repo.Info.CurrentOperation != CurrentOperation.None, + CommonGitDirectory: commonGitDirectory, + WorktreeName: GitWorktreeLayout.TryGetWorktreeName(gitDirectory)); } private static bool IsRepoLossException(Exception ex) => ex is diff --git a/DiffViewer/Utility/GitWorktreeLayout.cs b/DiffViewer/Utility/GitWorktreeLayout.cs new file mode 100644 index 0000000..123239e --- /dev/null +++ b/DiffViewer/Utility/GitWorktreeLayout.cs @@ -0,0 +1,201 @@ +using System; +using System.IO; + +namespace DiffViewer.Utility; + +/// +/// Pure path/file reasoning about git's worktree on-disk layout. No +/// libgit2 dependency, so it is cheap to call and trivially testable. +/// +/// The layout: a repository has one common +/// directory — the .git directory holding the object +/// database and shared refs. The main worktree's git directory +/// is the common directory. Each linked worktree instead gets +/// its own git directory at <common>/worktrees/<name>, +/// containing a commondir file that points back (normally the +/// relative string ../..). The presence of that file is the +/// definitive "am I a linked worktree" test. +/// +public static class GitWorktreeLayout +{ + private const string CommonDirFileName = "commondir"; + private const string GitDirectoryName = ".git"; + + /// + /// True when is a linked worktree's + /// git directory rather than a repository's common directory. + /// + public static bool IsLinkedWorktree(string gitDirectory) + { + if (string.IsNullOrWhiteSpace(gitDirectory)) return false; + try + { + return File.Exists(Path.Combine(gitDirectory, CommonDirFileName)); + } + catch (Exception) + { + return false; + } + } + + /// + /// Resolve the repository's common directory. Returns + /// unchanged when it is already the + /// common directory, or when the commondir pointer is + /// unreadable or empty. + /// + public static string ResolveCommonDirectory(string gitDirectory) + { + if (string.IsNullOrWhiteSpace(gitDirectory)) return gitDirectory; + + try + { + var commonDirFile = Path.Combine(gitDirectory, CommonDirFileName); + if (!File.Exists(commonDirFile)) return gitDirectory; + + var contents = File.ReadAllText(commonDirFile).Trim(); + if (contents.Length == 0) return gitDirectory; + + // The pointer is normally relative to the worktree's git + // directory; Path.Combine leaves an absolute pointer alone. + return Path.GetFullPath(Path.Combine(gitDirectory, contents)); + } + catch (Exception) + { + return gitDirectory; + } + } + + /// + /// Git's name for the worktree owning , + /// or null when it is not a linked worktree. The name is the + /// directory leaf under <common>/worktrees — normally, + /// but not necessarily, the leaf of the worktree's working + /// directory. + /// + public static string? TryGetWorktreeName(string gitDirectory) + { + if (!IsLinkedWorktree(gitDirectory)) return null; + + try + { + var trimmed = Path.TrimEndingDirectorySeparator(Path.GetFullPath(gitDirectory)); + var leaf = Path.GetFileName(trimmed); + return string.IsNullOrEmpty(leaf) ? null : leaf; + } + catch (Exception) + { + return null; + } + } + + /// + /// Describe the repository at in + /// the terms a user recognizes: the repository's own name, and the + /// name of the worktree they are looking at (or null when it + /// is the main one). Both are null when the path is not a + /// git working directory. + /// + /// Deliberately libgit2-free — this runs on the recents + /// write path for every launch, and a handful of file probes is + /// much cheaper than opening a repository handle. + /// + public static WorktreeLabels Describe(string workingDirectory) + { + var gitDirectory = TryResolveGitDirectory(workingDirectory); + if (gitDirectory is null) return WorktreeLabels.None; + + return new WorktreeLabels( + RepositoryName: TryGetRepositoryName(ResolveCommonDirectory(gitDirectory)), + WorktreeName: TryGetWorktreeName(gitDirectory)); + } + + /// + /// Resolve a working directory's git directory. For an ordinary + /// checkout .git is a directory. For a linked worktree it is + /// instead a file containing gitdir: <path>, pointing at + /// the worktree's administrative directory. Returns null when + /// neither shape is found. + /// + public static string? TryResolveGitDirectory(string workingDirectory) + { + if (string.IsNullOrWhiteSpace(workingDirectory)) return null; + + try + { + var candidate = Path.Combine(workingDirectory, GitDirectoryName); + if (Directory.Exists(candidate)) return candidate; + if (!File.Exists(candidate)) return null; + + var contents = File.ReadAllText(candidate).Trim(); + const string pointerPrefix = "gitdir:"; + if (!contents.StartsWith(pointerPrefix, StringComparison.OrdinalIgnoreCase)) return null; + + var target = contents[pointerPrefix.Length..].Trim(); + if (target.Length == 0) return null; + + return Path.GetFullPath(Path.Combine(workingDirectory, target)); + } + catch (Exception) + { + return null; + } + } + + /// + /// The repository's own name, derived from its common directory — + /// the name a user would call the project, shared by every worktree. + /// + /// Two layouts are handled. In the usual one the common + /// directory is <repo>/.git, so the name is the parent + /// leaf. In the "bare hub plus N worktrees" layout the common + /// directory is the bare repository itself (conventionally + /// <name>.git), so the name is that leaf with the + /// suffix trimmed. + /// + public static string? TryGetRepositoryName(string commonGitDirectory) + { + if (string.IsNullOrWhiteSpace(commonGitDirectory)) return null; + + try + { + var trimmed = Path.TrimEndingDirectorySeparator(Path.GetFullPath(commonGitDirectory)); + var leaf = Path.GetFileName(trimmed); + + if (string.Equals(leaf, GitDirectoryName, StringComparison.OrdinalIgnoreCase)) + { + var parent = Path.GetDirectoryName(trimmed); + if (string.IsNullOrEmpty(parent)) return null; + var parentLeaf = Path.GetFileName(Path.TrimEndingDirectorySeparator(parent)); + return string.IsNullOrEmpty(parentLeaf) ? parent : parentLeaf; + } + + if (string.IsNullOrEmpty(leaf)) return null; + + return leaf.EndsWith(GitDirectoryName, StringComparison.OrdinalIgnoreCase) + && leaf.Length > GitDirectoryName.Length + ? leaf[..^GitDirectoryName.Length] + : leaf; + } + catch (Exception) + { + return null; + } + } +} + +/// +/// Human-facing names for a checkout: the repository it belongs to, and +/// which worktree of that repository it is. +/// +/// The repository's name, shared by every +/// worktree (e.g. DiffViewer), or null when it could not +/// be determined. +/// Git's name for this linked worktree, or +/// null when this is the main worktree or the path is not a +/// repository. +public sealed record WorktreeLabels(string? RepositoryName, string? WorktreeName) +{ + /// Nothing could be determined about the path. + public static WorktreeLabels None { get; } = new(null, null); +} diff --git a/DiffViewer/ViewModels/MainViewModel.cs b/DiffViewer/ViewModels/MainViewModel.cs index 21a1473..9dc917b 100644 --- a/DiffViewer/ViewModels/MainViewModel.cs +++ b/DiffViewer/ViewModels/MainViewModel.cs @@ -675,7 +675,7 @@ public MainViewModel( DiffPane = new DiffPaneViewModel(_repository, diffService, _isCommitVsCommit, settingsService, imageDecoder); DiffPane.SetPullRequestContext(_pullRequestWatcher is not null); - WindowTitle = $"DiffViewer — {repository.Shape.RepoRoot} ({FormatSideForTitle(left)} ⇢ {FormatSideForTitle(right)})"; + WindowTitle = BuildWindowTitle(repository.Shape, left, right); // Recents dropdown is wired only when both the singleton service // and a usable repo path are available. Empty-state cold-launch @@ -766,6 +766,21 @@ public async Task LoadInitialChangesAsync(CancellationToken ct = default) public void LoadInitialChanges() => LoadInitialChangesAsync().GetAwaiter().GetResult(); + /// + /// Build the window title. A linked worktree gets its name appended + /// in brackets after the path, because the path alone is easy to + /// misread when several worktrees of one repository are open at once + /// and their directory names don't match their branches. + /// + internal static string BuildWindowTitle(RepositoryShape shape, DiffSide left, DiffSide right) + { + var location = shape.IsLinkedWorktree + ? $"{shape.RepoRoot} [{shape.WorktreeName}]" + : shape.RepoRoot; + + return $"DiffViewer — {location} ({FormatSideForTitle(left)} ⇢ {FormatSideForTitle(right)})"; + } + /// /// Format a for inclusion in the window title. /// Renders WorkingTree as the human-friendly "working tree" string diff --git a/DiffViewer/ViewModels/RecentContextsViewModel.cs b/DiffViewer/ViewModels/RecentContextsViewModel.cs index 5298e33..8754fc0 100644 --- a/DiffViewer/ViewModels/RecentContextsViewModel.cs +++ b/DiffViewer/ViewModels/RecentContextsViewModel.cs @@ -247,12 +247,16 @@ public RecentContextItem(RecentLaunchContext source) public RecentLaunchContext Source { get; } /// Primary line. e.g. "DevTools · main → <working-tree>" - /// for local rows, "DevTools · PR owner/repo#42" for review-mode rows. + /// for local rows, "DevTools · PR owner/repo#42" for review-mode rows. + /// A row pointing at a linked worktree carries the worktree's name in + /// brackets — "DiffViewer [feature-x] · HEAD → WT" — because + /// otherwise every worktree of a repository renders identically apart + /// from a path the dropdown doesn't show. public string Title { get { - var name = SafeBaseName(Source.Identity.CanonicalRepoPath); + var name = RepositoryLabel; if (Source.Review is { } review) { return $"{name} · PR {review.Slug}"; @@ -261,6 +265,25 @@ public string Title } } + /// + /// The repository portion of . Prefers the + /// repository name captured at launch time, falling back to the + /// path's leaf for rows written before worktree labelling existed. + /// + private string RepositoryLabel + { + get + { + var name = string.IsNullOrWhiteSpace(Source.RepositoryName) + ? SafeBaseName(Source.Identity.CanonicalRepoPath) + : Source.RepositoryName!; + + return string.IsNullOrWhiteSpace(Source.WorktreeName) + ? name + : $"{name} [{Source.WorktreeName}]"; + } + } + /// Secondary line: relative-time label (e.g. "2h ago"). public string Subtitle => RelativeTimeFormatter.Format(Source.LastUsedUtc); From c855548d8782ecca3acde5306a1db8e43f453d60 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 10:20:02 -0700 Subject: [PATCH 2/7] Add worktree switching to the New diff dialog and the context bar Pointing a diff at a different worktree meant remembering where `git worktree add` put it and typing the path. Both the repo-path row in the New diff dialog and the context bar now offer a worktree list: main first, each entry showing its checked-out branch and path, with pruned worktrees listed as unavailable rather than silently dropped. The context-bar switcher reuses WorktreePickerViewModel with a write-back that runs an in-place context switch instead of editing a text box, carrying both diff sides over unchanged. The picker is offered on every local form, not just working-tree ones. Two worktrees share an object database and all shared refs, so `main..feature` resolves identically from either - but HEAD is per-worktree, and the dialog advertises HEAD~3 as valid input, so the choice changes the answer for commit-ish forms too. Extracts LocalRepoFormViewModelBase along the way. The five local forms each carried their own copy of the repo path, its canonicalization, and the deferred-error handling - three of them verbatim identical - and the worktree picker would have been a sixth copy of the same pattern. The repo-path row in NewDiffDialog.xaml was duplicated five times likewise, and is now one RepoPathField control, which also retires the Browse handler's Tag="RepoPath" visual-tree walk in favour of a named element. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- CHANGELOG.md | 12 ++ .../RecentContextsViewModelWorktreeTests.cs | 154 ++++++++++++++++ .../Services/NewDiffDialogHostTests.cs | 1 + DiffViewer.Tests/StubWorktreeEnumerator.cs | 30 +++ .../BranchVsMergeBaseFormViewModelTests.cs | 2 +- .../ViewModels/NewDiffDialogViewModelTests.cs | 1 + .../ViewModels/NewDiffFormViewModelTests.cs | 2 +- .../ViewModels/ViewStashFormViewModelTests.cs | 2 +- .../WorktreePickerViewModelTests.cs | 173 ++++++++++++++++++ DiffViewer/App.xaml.cs | 5 +- DiffViewer/AppServices.cs | 3 +- DiffViewer/CompositionRoot.cs | 3 +- DiffViewer/Services/IDiffModeProvider.cs | 5 + DiffViewer/Services/NewDiffDialogHost.cs | 5 +- .../BranchVsMergeBaseFormViewModel.cs | 98 +++------- .../ViewModels/CommitVsCommitFormViewModel.cs | 73 ++------ .../ViewModels/LocalRepoFormViewModelBase.cs | 120 ++++++++++++ DiffViewer/ViewModels/MainViewModel.cs | 6 +- .../ViewModels/NewDiffDialogViewModel.cs | 5 +- .../ViewModels/RecentContextsViewModel.cs | 65 ++++++- .../ViewModels/ViewStashFormViewModel.cs | 52 ++---- .../WorkingTreeVsCommitFormViewModel.cs | 83 ++------- .../WorkingTreeVsHeadFormViewModel.cs | 41 +---- .../ViewModels/WorktreePickerViewModel.cs | 135 ++++++++++++++ DiffViewer/Views/NewDiffDialog.xaml | 90 +-------- DiffViewer/Views/NewDiffDialog.xaml.cs | 52 ------ DiffViewer/Views/RecentsBarView.xaml | 23 ++- DiffViewer/Views/RepoPathField.xaml | 47 +++++ DiffViewer/Views/RepoPathField.xaml.cs | 50 +++++ DiffViewer/Views/WorktreePicker.xaml | 108 +++++++++++ DiffViewer/Views/WorktreePicker.xaml.cs | 86 +++++++++ 31 files changed, 1108 insertions(+), 424 deletions(-) create mode 100644 DiffViewer.Tests/RecentContexts/RecentContextsViewModelWorktreeTests.cs create mode 100644 DiffViewer.Tests/StubWorktreeEnumerator.cs create mode 100644 DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs create mode 100644 DiffViewer/ViewModels/LocalRepoFormViewModelBase.cs create mode 100644 DiffViewer/ViewModels/WorktreePickerViewModel.cs create mode 100644 DiffViewer/Views/RepoPathField.xaml create mode 100644 DiffViewer/Views/RepoPathField.xaml.cs create mode 100644 DiffViewer/Views/WorktreePicker.xaml create mode 100644 DiffViewer/Views/WorktreePicker.xaml.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index d2887ed..23dc6e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,18 @@ body. Keep section headings exact and write notes in Markdown. ## [Unreleased] +### Added + +- **Worktree switching.** The repository-path row in the "New diff" + dialog gained a **Worktree…** button listing every worktree of the + repository you typed — main first, each showing its checked-out + branch and path — so pointing a diff at a different checkout no + longer means remembering where `git worktree add` put it. The + context bar gained a matching **Worktree** button that re-opens the + comparison you are already looking at, rooted at another worktree, + with both sides unchanged. Worktrees whose directory has been + deleted are listed as unavailable rather than silently omitted. + ### Changed - **Worktrees are now identifiable in the UI.** DiffViewer already diff --git a/DiffViewer.Tests/RecentContexts/RecentContextsViewModelWorktreeTests.cs b/DiffViewer.Tests/RecentContexts/RecentContextsViewModelWorktreeTests.cs new file mode 100644 index 0000000..25542e9 --- /dev/null +++ b/DiffViewer.Tests/RecentContexts/RecentContextsViewModelWorktreeTests.cs @@ -0,0 +1,154 @@ +using System; +using System.Collections.Generic; +using System.ComponentModel; +using System.Threading; +using System.Threading.Tasks; +using DiffViewer.Models; +using DiffViewer.Services; +using DiffViewer.ViewModels; +using FluentAssertions; +using Xunit; + +namespace DiffViewer.Tests.RecentContexts; + +/// +/// The context bar's worktree switcher: re-opens the current comparison +/// rooted at a different worktree of the same repository, carrying both +/// sides over unchanged. +/// +public class RecentContextsViewModelWorktreeTests +{ + private const string CurrentRepo = @"C:\repos\diffviewer"; + private const string OtherWorktree = @"C:\worktrees\feature-x"; + + [Fact] + public void WorktreeSwitch_WithNoEnumeratorWired_IsDisabled() + { + var vm = MakeViewModel(new FakeSwitcher(), worktreeEnumerator: null); + + vm.IsWorktreeSwitchEnabled.Should().BeFalse(); + vm.WorktreePicker.Should().BeNull(); + } + + [Fact] + public void WorktreeSwitch_WithNoSwitcherWired_IsDisabled() + { + var vm = MakeViewModel(switcher: null, worktreeEnumerator: new StubWorktreeEnumerator()); + + vm.IsWorktreeSwitchEnabled.Should().BeFalse(); + vm.WorktreePicker.Should().BeNull(); + } + + [Fact] + public void WorktreeSwitch_WhenFullyWired_IsEnabledAndPointedAtTheCurrentRepo() + { + var vm = MakeViewModel(new FakeSwitcher(), new StubWorktreeEnumerator()); + + vm.IsWorktreeSwitchEnabled.Should().BeTrue(); + vm.WorktreePicker!.CanonicalRepoPath.Should().Be(CurrentRepo); + } + + [Fact] + public async Task PickingAnotherWorktree_RelaunchesTheSameSidesAtThatPath() + { + var switcher = new FakeSwitcher(); + var vm = MakeViewModel(switcher, new StubWorktreeEnumerator()); + + vm.WorktreePicker!.PickWorktreeCommand.Execute(Linked(OtherWorktree)); + await switcher.WaitForSwitchAsync(); + + switcher.SwitchToCalls.Should().Be(1); + var local = switcher.LastLaunched.Should().BeOfType().Subject; + local.Parsed.RepoPath.Should().Be(OtherWorktree); + // Both sides carry over verbatim - the point is to ask the same + // question of a different checkout. + local.Parsed.Left.Should().Be(new DiffSide.CommitIsh("HEAD")); + local.Parsed.Right.Should().Be(new DiffSide.WorkingTree()); + } + + [Fact] + public void PickingTheWorktreeAlreadyOpen_DoesNotSwitch() + { + var switcher = new FakeSwitcher(); + var vm = MakeViewModel(switcher, new StubWorktreeEnumerator()); + + vm.WorktreePicker!.PickWorktreeCommand.Execute(Linked(CurrentRepo)); + + switcher.SwitchToCalls.Should().Be(0); + } + + [Fact] + public void PickingTheCurrentWorktree_IsPathCaseInsensitive() + { + var switcher = new FakeSwitcher(); + var vm = MakeViewModel(switcher, new StubWorktreeEnumerator()); + + vm.WorktreePicker!.PickWorktreeCommand.Execute(Linked(CurrentRepo.ToUpperInvariant())); + + switcher.SwitchToCalls.Should().Be(0); + } + + [Fact] + public void PickingAMissingWorktree_DoesNotSwitch() + { + var switcher = new FakeSwitcher(); + var vm = MakeViewModel(switcher, new StubWorktreeEnumerator()); + + vm.WorktreePicker!.PickWorktreeCommand.Execute(Linked(OtherWorktree) with { IsMissing = true }); + + switcher.SwitchToCalls.Should().Be(0); + } + + private static RecentContextsViewModel MakeViewModel( + IContextSwitcher? switcher, + IGitWorktreeEnumerator? worktreeEnumerator) + { + var left = new DiffSide.CommitIsh("HEAD"); + var right = new DiffSide.WorkingTree(); + var identity = ContextIdentityFactory.Create(CurrentRepo, left, right); + return new RecentContextsViewModel( + new NullRecentContextsService(), + switcher, + identity, + newDiffDialogHost: null, + worktreeEnumerator: worktreeEnumerator); + } + + private static WorktreeEntry Linked(string workingDirectory) => new( + Name: "feature-x", + WorkingDirectory: workingDirectory, + HeadFriendlyName: "feature-x", + IsMain: false, + IsCurrent: false, + IsLocked: false, + IsMissing: false); + + private sealed class FakeSwitcher : IContextSwitcher + { + private readonly TaskCompletionSource _switched = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + public int SwitchToCalls { get; private set; } + public DiffLaunchSource? LastLaunched { get; private set; } + public bool IsSwitching => false; + +#pragma warning disable CS0067 // Interface-required; not raised by this fake. + public event PropertyChangedEventHandler? PropertyChanged; +#pragma warning restore CS0067 + + public Task SwitchToRecentAsync(RecentLaunchContext recent, CancellationToken ct = default) => + Task.FromResult(true); + + public Task SwitchToAsync(DiffLaunchSource source, CancellationToken ct = default) + { + SwitchToCalls++; + LastLaunched = source; + _switched.TrySetResult(); + return Task.FromResult(true); + } + + /// The picker's write-back is fire-and-forget, so tests + /// that assert on a switch must wait for it to actually land. + public Task WaitForSwitchAsync() => _switched.Task.WaitAsync(TimeSpan.FromSeconds(5)); + } +} diff --git a/DiffViewer.Tests/Services/NewDiffDialogHostTests.cs b/DiffViewer.Tests/Services/NewDiffDialogHostTests.cs index af5f679..bb801c2 100644 --- a/DiffViewer.Tests/Services/NewDiffDialogHostTests.cs +++ b/DiffViewer.Tests/Services/NewDiffDialogHostTests.cs @@ -270,6 +270,7 @@ private static NewDiffDialogHost MakeHost() DiffModeRegistry.BuildDefault(), new NoOpValidator(), new NoOpRefEnumerator(), + new StubWorktreeEnumerator(), new FakeRecents(), new FakeClipboard(), () => null); diff --git a/DiffViewer.Tests/StubWorktreeEnumerator.cs b/DiffViewer.Tests/StubWorktreeEnumerator.cs new file mode 100644 index 0000000..6ed25ac --- /dev/null +++ b/DiffViewer.Tests/StubWorktreeEnumerator.cs @@ -0,0 +1,30 @@ +using System; +using System.Collections.Generic; +using DiffViewer.Services; + +namespace DiffViewer.Tests; + +/// +/// Test double for . Returns a +/// caller-supplied list regardless of the path, or nothing by default — +/// which is what most form tests want, since they exercise validation +/// and launch-source construction rather than worktree discovery. +/// +internal sealed class StubWorktreeEnumerator : IGitWorktreeEnumerator +{ + private readonly IReadOnlyList _entries; + + public StubWorktreeEnumerator(params WorktreeEntry[] entries) + { + _entries = entries ?? Array.Empty(); + } + + /// Paths this stub was asked about, in call order. + public List EnumeratedPaths { get; } = new(); + + public IReadOnlyList Enumerate(string canonicalRepoPath) + { + EnumeratedPaths.Add(canonicalRepoPath); + return _entries; + } +} diff --git a/DiffViewer.Tests/ViewModels/BranchVsMergeBaseFormViewModelTests.cs b/DiffViewer.Tests/ViewModels/BranchVsMergeBaseFormViewModelTests.cs index 56b698a..f3bba1f 100644 --- a/DiffViewer.Tests/ViewModels/BranchVsMergeBaseFormViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/BranchVsMergeBaseFormViewModelTests.cs @@ -62,7 +62,7 @@ private static FormDependencies Deps( FakeValidator validator, FakeEnumerator enumerator, string? prefilledRepoPath = null) - => new(validator, enumerator, new NullRecentContextsService(), prefilledRepoPath); + => new(validator, enumerator, new StubWorktreeEnumerator(), new NullRecentContextsService(), prefilledRepoPath); [Fact] public void Empty_NotValid_NoError() diff --git a/DiffViewer.Tests/ViewModels/NewDiffDialogViewModelTests.cs b/DiffViewer.Tests/ViewModels/NewDiffDialogViewModelTests.cs index ee4d0e7..f28c7c7 100644 --- a/DiffViewer.Tests/ViewModels/NewDiffDialogViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/NewDiffDialogViewModelTests.cs @@ -39,6 +39,7 @@ private static NewDiffDialogViewModel MakeVm( registry, validator ?? new FakeValidator(), new StubRefEnumerator(), + new StubWorktreeEnumerator(), new NullRecentContextsService(), prefilledRepoPath, initialProviderId, diff --git a/DiffViewer.Tests/ViewModels/NewDiffFormViewModelTests.cs b/DiffViewer.Tests/ViewModels/NewDiffFormViewModelTests.cs index 7cd9c8f..7739ed9 100644 --- a/DiffViewer.Tests/ViewModels/NewDiffFormViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/NewDiffFormViewModelTests.cs @@ -55,7 +55,7 @@ private static FormDependencies Deps( IDiffLaunchValidator validator, string? prefilledRepoPath = null, string? seedPullRequestUrl = null) - => new(validator, new StubRefEnumerator(), new NullRecentContextsService(), + => new(validator, new StubRefEnumerator(), new StubWorktreeEnumerator(), new NullRecentContextsService(), prefilledRepoPath, seedPullRequestUrl); // === WorkingTreeVsHeadFormViewModel === diff --git a/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs b/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs index 5be529f..fcdbbb8 100644 --- a/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs @@ -121,7 +121,7 @@ private static ViewStashFormViewModel CreateForm( { var validator = new FakeValidator(repoValid); var enumerator = new FakeStashEnumerator(stashes ?? Array.Empty()); - var deps = new FormDependencies(validator, enumerator, new NullRecentContextsService(), repoPath); + var deps = new FormDependencies(validator, enumerator, new StubWorktreeEnumerator(), new NullRecentContextsService(), repoPath); return new ViewStashFormViewModel(deps, enumerateRunner: work => Task.FromResult(work())); } diff --git a/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs b/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs new file mode 100644 index 0000000..889cc8f --- /dev/null +++ b/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs @@ -0,0 +1,173 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using DiffViewer.Services; +using DiffViewer.ViewModels; +using FluentAssertions; +using Xunit; + +namespace DiffViewer.Tests.ViewModels; + +/// +/// Behavior of the worktree picker attached to the "New diff" dialog's +/// repo-path input. +/// +public class WorktreePickerViewModelTests +{ + [Fact] + public void IsEnabled_WithNoRepoPath_IsFalse() + { + var picker = MakePicker(out _, out _); + + picker.IsEnabled.Should().BeFalse(); + } + + [Fact] + public void IsEnabled_OnceARepoPathResolves_IsTrue() + { + var picker = MakePicker(out _, out _); + + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + picker.IsEnabled.Should().BeTrue(); + } + + [Fact] + public async Task EnsureLoadedAsync_WithNoRepoPath_DoesNotEnumerate() + { + var picker = MakePicker(out var enumerator, out _); + + await picker.EnsureLoadedAsync(); + + enumerator.EnumeratedPaths.Should().BeEmpty(); + picker.IsLoaded.Should().BeFalse(); + } + + [Fact] + public async Task EnsureLoadedAsync_EnumeratesAgainstTheCurrentRepoPath() + { + var picker = MakePicker(out var enumerator, out _, MainWorktree(), Linked("feature-a")); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + enumerator.EnumeratedPaths.Should().ContainSingle().Which.Should().Be(@"C:\repos\diffviewer"); + picker.Worktrees.Should().HaveCount(2); + picker.IsLoaded.Should().BeTrue(); + } + + [Fact] + public async Task EnsureLoadedAsync_CalledTwice_EnumeratesOnlyOnce() + { + var picker = MakePicker(out var enumerator, out _, MainWorktree()); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + await picker.EnsureLoadedAsync(); + + enumerator.EnumeratedPaths.Should().HaveCount(1); + } + + [Fact] + public async Task ChangingTheRepoPath_DiscardsThePreviousEnumeration() + { + var picker = MakePicker(out _, out _, MainWorktree(), Linked("feature-a")); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + await picker.EnsureLoadedAsync(); + + picker.CanonicalRepoPath = @"C:\repos\other"; + + picker.IsLoaded.Should().BeFalse(); + picker.Worktrees.Should().BeEmpty(); + } + + [Fact] + public async Task HasAlternativeWorktrees_WithOnlyTheMainWorktree_IsFalse() + { + // A repo with no linked worktrees would otherwise render a + // one-row list pointing at where the user already is. + var picker = MakePicker(out _, out _, MainWorktree()); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + picker.HasAlternativeWorktrees.Should().BeFalse(); + } + + [Fact] + public async Task HasAlternativeWorktrees_WithALinkedWorktree_IsTrue() + { + var picker = MakePicker(out _, out _, MainWorktree(), Linked("feature-a")); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + picker.HasAlternativeWorktrees.Should().BeTrue(); + } + + [Fact] + public void PickWorktree_WritesTheWorkingDirectoryBack() + { + var picker = MakePicker(out _, out var written); + + picker.PickWorktreeCommand.Execute(Linked("feature-a")); + + written.Should().ContainSingle().Which.Should().Be(@"C:\worktrees\feature-a"); + } + + [Fact] + public void PickWorktree_ForAMissingWorktree_WritesNothing() + { + // The row is shown so the user understands what git still knows + // about - not so they can diff a directory that isn't there. + var picker = MakePicker(out _, out var written); + + picker.PickWorktreeCommand.Execute(Linked("feature-a") with { IsMissing = true }); + + written.Should().BeEmpty(); + } + + [Fact] + public void PickWorktree_WithNoSelection_WritesNothing() + { + var picker = MakePicker(out _, out var written); + + picker.PickWorktreeCommand.Execute(null); + + written.Should().BeEmpty(); + } + + private static WorktreePickerViewModel MakePicker( + out StubWorktreeEnumerator enumerator, + out List written, + params WorktreeEntry[] entries) + { + var stub = new StubWorktreeEnumerator(entries); + var writes = new List(); + enumerator = stub; + written = writes; + return new WorktreePickerViewModel( + stub, + writeBack: writes.Add, + initialCanonicalRepoPath: null, + enumerateRunner: work => Task.FromResult(work())); + } + + private static WorktreeEntry MainWorktree() => new( + Name: "diffviewer", + WorkingDirectory: @"C:\repos\diffviewer", + HeadFriendlyName: "master", + IsMain: true, + IsCurrent: true, + IsLocked: false, + IsMissing: false); + + private static WorktreeEntry Linked(string name) => new( + Name: name, + WorkingDirectory: $@"C:\worktrees\{name}", + HeadFriendlyName: name, + IsMain: false, + IsCurrent: false, + IsLocked: false, + IsMissing: false); +} diff --git a/DiffViewer/App.xaml.cs b/DiffViewer/App.xaml.cs index 503dad6..03446fd 100644 --- a/DiffViewer/App.xaml.cs +++ b/DiffViewer/App.xaml.cs @@ -148,11 +148,13 @@ protected override async void OnStartup(StartupEventArgs e) var diffLaunchValidator = new DiffLaunchValidator(new ProcessCommandLineEnvironment()); var diffModeRegistry = DiffModeRegistry.BuildDefault(); var refEnumerator = new LibGit2GitRefEnumerator(); + var worktreeEnumerator = new LibGit2GitWorktreeEnumerator(); var clipboardService = new WpfClipboardService(); var newDiffDialogHost = new NewDiffDialogHost( diffModeRegistry, diffLaunchValidator, refEnumerator, + worktreeEnumerator, recents, clipboardService, ownerLookup: () => Application.Current?.MainWindow); @@ -162,7 +164,8 @@ protected override async void OnStartup(StartupEventArgs e) prResolver, missingClonePromptHost, newDiffDialogHost, GitHubClient: githubClient, PullRequestLocalFetcher: fetcher, - WindowVisibilityProbe: new WpfWindowVisibilityProbe()); + WindowVisibilityProbe: new WpfWindowVisibilityProbe(), + WorktreeEnumerator: worktreeEnumerator); _coordinator = new MainWindowCoordinator( services, diff --git a/DiffViewer/AppServices.cs b/DiffViewer/AppServices.cs index 15c5d90..99f9183 100644 --- a/DiffViewer/AppServices.cs +++ b/DiffViewer/AppServices.cs @@ -32,7 +32,8 @@ public sealed record AppServices( INewDiffDialogHost NewDiffDialogHost, IGitHubClient GitHubClient, IPullRequestLocalFetcher PullRequestLocalFetcher, - IWindowVisibilityProbe WindowVisibilityProbe) + IWindowVisibilityProbe WindowVisibilityProbe, + IGitWorktreeEnumerator? WorktreeEnumerator = null) { public IContextSwitcher? ContextSwitcher { get; set; } } diff --git a/DiffViewer/CompositionRoot.cs b/DiffViewer/CompositionRoot.cs index 9871b9a..dd7a8cd 100644 --- a/DiffViewer/CompositionRoot.cs +++ b/DiffViewer/CompositionRoot.cs @@ -191,7 +191,8 @@ public static async Task BuildContextAsync( clipboardService: clipboardService, imageDecoder: imageDecoder, initialFile: parsed.InitialFile, - pullRequestWatcher: pullRequestWatcher); + pullRequestWatcher: pullRequestWatcher, + worktreeEnumerator: services.WorktreeEnumerator); await vm.LoadInitialChangesAsync(ct).ConfigureAwait(true); return vm; diff --git a/DiffViewer/Services/IDiffModeProvider.cs b/DiffViewer/Services/IDiffModeProvider.cs index a82b476..14fc8e2 100644 --- a/DiffViewer/Services/IDiffModeProvider.cs +++ b/DiffViewer/Services/IDiffModeProvider.cs @@ -55,6 +55,10 @@ public interface IDiffModeProvider /// Powers the per-input ref picker /// popup. Forms with commit-ish inputs construct one /// per input from this. +/// Powers the worktree picker +/// attached to the repo-path input. Consumed by +/// on behalf of every local +/// form; PR-URL forms ignore it. /// Source of "Recent refs in this repo" /// for the picker (filtered + deduped per repo path; see /// ). @@ -69,6 +73,7 @@ public interface IDiffModeProvider public sealed record FormDependencies( IDiffLaunchValidator Validator, IGitRefEnumerator RefEnumerator, + IGitWorktreeEnumerator WorktreeEnumerator, IRecentContextsService RecentContexts, string? PrefilledRepoPath = null, string? SeedPullRequestUrl = null); diff --git a/DiffViewer/Services/NewDiffDialogHost.cs b/DiffViewer/Services/NewDiffDialogHost.cs index 75a32b1..ac7f3bd 100644 --- a/DiffViewer/Services/NewDiffDialogHost.cs +++ b/DiffViewer/Services/NewDiffDialogHost.cs @@ -41,6 +41,7 @@ public sealed class NewDiffDialogHost : INewDiffDialogHost private readonly DiffModeRegistry _registry; private readonly IDiffLaunchValidator _validator; private readonly IGitRefEnumerator _refEnumerator; + private readonly IGitWorktreeEnumerator _worktreeEnumerator; private readonly IRecentContextsService _recentContexts; private readonly IClipboardService _clipboard; private readonly Func _ownerLookup; @@ -50,6 +51,7 @@ public NewDiffDialogHost( DiffModeRegistry registry, IDiffLaunchValidator validator, IGitRefEnumerator refEnumerator, + IGitWorktreeEnumerator worktreeEnumerator, IRecentContextsService recentContexts, IClipboardService clipboard, Func ownerLookup) @@ -57,6 +59,7 @@ public NewDiffDialogHost( _registry = registry ?? throw new ArgumentNullException(nameof(registry)); _validator = validator ?? throw new ArgumentNullException(nameof(validator)); _refEnumerator = refEnumerator ?? throw new ArgumentNullException(nameof(refEnumerator)); + _worktreeEnumerator = worktreeEnumerator ?? throw new ArgumentNullException(nameof(worktreeEnumerator)); _recentContexts = recentContexts ?? throw new ArgumentNullException(nameof(recentContexts)); _clipboard = clipboard ?? throw new ArgumentNullException(nameof(clipboard)); _ownerLookup = ownerLookup ?? throw new ArgumentNullException(nameof(ownerLookup)); @@ -69,7 +72,7 @@ public NewDiffDialogHost( var owner = _ownerLookup(); var vm = new NewDiffDialogViewModel( - _registry, _validator, _refEnumerator, _recentContexts, + _registry, _validator, _refEnumerator, _worktreeEnumerator, _recentContexts, effectiveRepoPath, initialProviderIdOverride ?? _lastProviderId, clipboardPrUrl); diff --git a/DiffViewer/ViewModels/BranchVsMergeBaseFormViewModel.cs b/DiffViewer/ViewModels/BranchVsMergeBaseFormViewModel.cs index 9d6942b..a1e97de 100644 --- a/DiffViewer/ViewModels/BranchVsMergeBaseFormViewModel.cs +++ b/DiffViewer/ViewModels/BranchVsMergeBaseFormViewModel.cs @@ -25,16 +25,11 @@ namespace DiffViewer.ViewModels; /// it fails (orphaned histories or unresolvable refs) the validation /// error surfaces in the dialog footer and OK stays disabled. /// -public sealed partial class BranchVsMergeBaseFormViewModel : NewDiffFormViewModelBase +public sealed partial class BranchVsMergeBaseFormViewModel : LocalRepoFormViewModelBase { private readonly IGitRefEnumerator _enumerator; - private string? _canonicalRepoPath; - private string? _repoPathError; private string? _resolvedMergeBaseSha; - [ObservableProperty] - private string _repoPath; - [ObservableProperty] private string _branch; @@ -45,10 +40,9 @@ public sealed partial class BranchVsMergeBaseFormViewModel : NewDiffFormViewMode public RefPickerViewModel MergeBasePartnerPicker { get; } public BranchVsMergeBaseFormViewModel(FormDependencies deps) - : base(deps.Validator) + : base(deps) { _enumerator = deps.RefEnumerator; - _repoPath = deps.PrefilledRepoPath ?? string.Empty; _branch = string.Empty; _mergeBasePartner = string.Empty; BranchPicker = new RefPickerViewModel( @@ -57,81 +51,41 @@ public BranchVsMergeBaseFormViewModel(FormDependencies deps) MergeBasePartnerPicker = new RefPickerViewModel( deps.RefEnumerator, deps.RecentContexts, writeBack: value => MergeBasePartner = value); - // Canonicalize the prefilled repo path BEFORE the first - // Validate() so both pickers are enabled on dialog open when - // launching with an already-open context. Mirrors the - // OnRepoPathChanged ordering. - TryUpdateCanonicalRepoPath(); - SyncPickerRepoPath(); - Validate(); - // Seed merge-base partner from origin/HEAD whenever the - // prefilled repo path resolves and partner is still empty. - // Drives "branch + OK" as the steady state for the PR-review - // form. See TrySeedDefaultPartner for the guards. - TrySeedDefaultPartner(); - } - - partial void OnRepoPathChanged(string value) - { - TryUpdateCanonicalRepoPath(); - SyncPickerRepoPath(); - Validate(); - // Re-seed on every repo-path change. "Empty partner" is never - // a positive user choice (HasRequiredInputs requires it - // non-empty), so re-filling it after a user clears + switches - // repos is help, not magic. Guard inside the helper keeps - // non-empty partner values untouched. - TrySeedDefaultPartner(); + InitializeRepoPath(); } partial void OnBranchChanged(string value) => Validate(); partial void OnMergeBasePartnerChanged(string value) => Validate(); - protected override bool HasRequiredInputs => - !string.IsNullOrWhiteSpace(RepoPath) - && !string.IsNullOrWhiteSpace(Branch) + protected override bool HasRequiredLocalInputs => + !string.IsNullOrWhiteSpace(Branch) && !string.IsNullOrWhiteSpace(MergeBasePartner); - /// - /// Resolve the user's repo-path input into either a canonical - /// repository root (stored in , - /// consumed by both pickers for branch enumeration and by - /// ) or a deferred validation - /// message (stored in , surfaced by - /// once the rest of the form - /// is populated). See the WorkingTreeVsCommit form's copy of this - /// method for the bug-history rationale behind decoupling - /// canonicalization from validation. - /// - private void TryUpdateCanonicalRepoPath() + protected override void OnRepoPathResolved() { - _canonicalRepoPath = null; - _repoPathError = null; - if (string.IsNullOrWhiteSpace(RepoPath)) return; + BranchPicker.CanonicalRepoPath = CanonicalRepoPath; + MergeBasePartnerPicker.CanonicalRepoPath = CanonicalRepoPath; - var result = Validator.ValidateRepoPath(RepoPath); - if (result is RepoPathValidation.Valid v) - { - _canonicalRepoPath = v.CanonicalPath; - } - else - { - _repoPathError = ((RepoPathValidation.Invalid)result).Message; - } + // Re-seed on every repo-path change. "Empty partner" is never + // a positive user choice (HasRequiredInputs requires it + // non-empty), so re-filling it after a user clears + switches + // repos is help, not magic. Guard inside the helper keeps + // non-empty partner values untouched. + TrySeedDefaultPartner(); } protected override string? ComputeValidationError() { _resolvedMergeBaseSha = null; if (!HasRequiredInputs) return null; - if (_repoPathError is not null) return _repoPathError; + if (RepoPathError is not null) return RepoPathError; // Validate both refs first so the user sees the most // diagnostic error (an unresolvable ref) rather than the // less-informative "no common ancestor" that would come back // if we jumped straight to FindMergeBase. - var branchResult = Validator.ValidateCommitIsh(_canonicalRepoPath!, Branch); - var partnerResult = Validator.ValidateCommitIsh(_canonicalRepoPath!, MergeBasePartner); + var branchResult = Validator.ValidateCommitIsh(CanonicalRepoPath!, Branch); + var partnerResult = Validator.ValidateCommitIsh(CanonicalRepoPath!, MergeBasePartner); var branchError = (branchResult as CommitIshValidation.Invalid)?.Message; var partnerError = (partnerResult as CommitIshValidation.Invalid)?.Message; @@ -139,7 +93,7 @@ private void TryUpdateCanonicalRepoPath() if (branchError is not null) return branchError; if (partnerError is not null) return partnerError; - var mergeBase = _enumerator.TryComputeMergeBase(_canonicalRepoPath!, Branch, MergeBasePartner); + var mergeBase = _enumerator.TryComputeMergeBase(CanonicalRepoPath!, Branch, MergeBasePartner); if (mergeBase is null) { return $"No common ancestor between `{Branch}` and `{MergeBasePartner}`."; @@ -161,18 +115,12 @@ public override DiffLaunchSource BuildLaunchSource() } var parsed = new ParsedCommandLine( - _canonicalRepoPath ?? RepoPath, + CanonicalRepoPath ?? RepoPath, new DiffSide.CommitIsh(_resolvedMergeBaseSha), new DiffSide.CommitIsh(Branch)); return new DiffLaunchSource.Local(parsed); } - private void SyncPickerRepoPath() - { - BranchPicker.CanonicalRepoPath = _canonicalRepoPath; - MergeBasePartnerPicker.CanonicalRepoPath = _canonicalRepoPath; - } - /// /// If the partner field is currently empty and the repo path /// resolves, ask the enumerator for the repo's default remote @@ -181,15 +129,15 @@ private void SyncPickerRepoPath() /// doesn't resolve, the partner is already non-empty, or the /// repo has no origin/HEAD symref (older clones, /// manually-configured remotes). Assignment to the partner - /// re-triggers via the source-generated - /// setter. + /// re-triggers via + /// the source-generated setter. /// private void TrySeedDefaultPartner() { if (!string.IsNullOrWhiteSpace(MergeBasePartner)) return; - if (_canonicalRepoPath is null) return; + if (CanonicalRepoPath is null) return; - var seed = _enumerator.TryGetDefaultRemoteBranch(_canonicalRepoPath); + var seed = _enumerator.TryGetDefaultRemoteBranch(CanonicalRepoPath); if (string.IsNullOrWhiteSpace(seed)) return; MergeBasePartner = seed; } diff --git a/DiffViewer/ViewModels/CommitVsCommitFormViewModel.cs b/DiffViewer/ViewModels/CommitVsCommitFormViewModel.cs index 7743984..0bbf618 100644 --- a/DiffViewer/ViewModels/CommitVsCommitFormViewModel.cs +++ b/DiffViewer/ViewModels/CommitVsCommitFormViewModel.cs @@ -14,14 +14,8 @@ namespace DiffViewer.ViewModels; /// they share the form's canonical repo path so both target the same /// repository's branches / tags / recent refs. /// -public sealed partial class CommitVsCommitFormViewModel : NewDiffFormViewModelBase +public sealed partial class CommitVsCommitFormViewModel : LocalRepoFormViewModelBase { - private string? _canonicalRepoPath; - private string? _repoPathError; - - [ObservableProperty] - private string _repoPath; - [ObservableProperty] private string _baseCommit; @@ -32,9 +26,8 @@ public sealed partial class CommitVsCommitFormViewModel : NewDiffFormViewModelBa public RefPickerViewModel CompareCommitPicker { get; } public CommitVsCommitFormViewModel(FormDependencies deps) - : base(deps.Validator) + : base(deps) { - _repoPath = deps.PrefilledRepoPath ?? string.Empty; _baseCommit = string.Empty; _compareCommit = string.Empty; BaseCommitPicker = new RefPickerViewModel( @@ -43,62 +36,26 @@ public CommitVsCommitFormViewModel(FormDependencies deps) CompareCommitPicker = new RefPickerViewModel( deps.RefEnumerator, deps.RecentContexts, writeBack: value => CompareCommit = value); - // Canonicalize the prefilled repo path BEFORE the first - // Validate() so both pickers are enabled on dialog open when - // launching with an already-open context. Mirrors the - // OnRepoPathChanged ordering. - TryUpdateCanonicalRepoPath(); - SyncPickerRepoPath(); - Validate(); - } - - partial void OnRepoPathChanged(string value) - { - TryUpdateCanonicalRepoPath(); - SyncPickerRepoPath(); - Validate(); + InitializeRepoPath(); } partial void OnBaseCommitChanged(string value) => Validate(); partial void OnCompareCommitChanged(string value) => Validate(); - protected override bool HasRequiredInputs => - !string.IsNullOrWhiteSpace(RepoPath) - && !string.IsNullOrWhiteSpace(BaseCommit) + protected override bool HasRequiredLocalInputs => + !string.IsNullOrWhiteSpace(BaseCommit) && !string.IsNullOrWhiteSpace(CompareCommit); - /// - /// Resolve the user's repo-path input into either a canonical - /// repository root (stored in , - /// consumed by both pickers for branch enumeration and by - /// ) or a deferred validation - /// message (stored in , surfaced by - /// once the rest of the form - /// is populated). See the WorkingTreeVsCommit form's copy of this - /// method for the bug-history rationale behind decoupling - /// canonicalization from validation. - /// - private void TryUpdateCanonicalRepoPath() + protected override void OnRepoPathResolved() { - _canonicalRepoPath = null; - _repoPathError = null; - if (string.IsNullOrWhiteSpace(RepoPath)) return; - - var result = Validator.ValidateRepoPath(RepoPath); - if (result is RepoPathValidation.Valid v) - { - _canonicalRepoPath = v.CanonicalPath; - } - else - { - _repoPathError = ((RepoPathValidation.Invalid)result).Message; - } + BaseCommitPicker.CanonicalRepoPath = CanonicalRepoPath; + CompareCommitPicker.CanonicalRepoPath = CanonicalRepoPath; } protected override string? ComputeValidationError() { if (!HasRequiredInputs) return null; - if (_repoPathError is not null) return _repoPathError; + if (RepoPathError is not null) return RepoPathError; // Validate BOTH commit-ish fields against the canonical repo // path and surface every error at once. Stopping at the first @@ -106,8 +63,8 @@ private void TryUpdateCanonicalRepoPath() // which is the opposite of helpful when the user mistyped both // (or, more commonly, used a default-branch name like `main` // for a repo whose default branch is `master`). - var baseResult = Validator.ValidateCommitIsh(_canonicalRepoPath!, BaseCommit); - var compareResult = Validator.ValidateCommitIsh(_canonicalRepoPath!, CompareCommit); + var baseResult = Validator.ValidateCommitIsh(CanonicalRepoPath!, BaseCommit); + var compareResult = Validator.ValidateCommitIsh(CanonicalRepoPath!, CompareCommit); var baseError = (baseResult as CommitIshValidation.Invalid)?.Message; var compareError = (compareResult as CommitIshValidation.Invalid)?.Message; @@ -123,15 +80,9 @@ private void TryUpdateCanonicalRepoPath() public override DiffLaunchSource BuildLaunchSource() { var parsed = new ParsedCommandLine( - _canonicalRepoPath ?? RepoPath, + CanonicalRepoPath ?? RepoPath, new DiffSide.CommitIsh(BaseCommit), new DiffSide.CommitIsh(CompareCommit)); return new DiffLaunchSource.Local(parsed); } - - private void SyncPickerRepoPath() - { - BaseCommitPicker.CanonicalRepoPath = _canonicalRepoPath; - CompareCommitPicker.CanonicalRepoPath = _canonicalRepoPath; - } } diff --git a/DiffViewer/ViewModels/LocalRepoFormViewModelBase.cs b/DiffViewer/ViewModels/LocalRepoFormViewModelBase.cs new file mode 100644 index 0000000..9091f37 --- /dev/null +++ b/DiffViewer/ViewModels/LocalRepoFormViewModelBase.cs @@ -0,0 +1,120 @@ +using CommunityToolkit.Mvvm.ComponentModel; +using DiffViewer.Services; + +namespace DiffViewer.ViewModels; + +/// +/// Base for every "New diff" form whose input starts with a local +/// repository path. Owns the repo-path property, its canonicalization, +/// and the worktree picker attached to it, so the concrete forms are +/// left holding only the inputs that actually differ between modes. +/// +/// Canonicalization is decoupled from validation, and the +/// distinction is load-bearing. is +/// refreshed on every keystroke because the ref and worktree pickers +/// need a resolved repository the moment one is typed. The +/// corresponding error is stashed in rather +/// than surfaced immediately, so a half-typed path doesn't flash a +/// validation message under the field; concrete forms decide when to +/// surface it from . +/// +/// Construction order matters. Derived constructors must +/// finish initializing their own fields — including any +/// instances — and then call +/// as their last statement. That +/// resolves the prefilled path and fires +/// before the first , which +/// is what leaves the pickers enabled on open when the dialog was +/// launched from an already-loaded context. +/// +public abstract partial class LocalRepoFormViewModelBase : NewDiffFormViewModelBase +{ + protected LocalRepoFormViewModelBase(FormDependencies deps) + : base(deps.Validator) + { + _repoPath = deps.PrefilledRepoPath ?? string.Empty; + WorktreePicker = new WorktreePickerViewModel( + deps.WorktreeEnumerator, + writeBack: value => RepoPath = value); + } + + [ObservableProperty] + private string _repoPath; + + /// + /// The validated repository root for the current , + /// or null when it doesn't resolve. Concrete forms pass this + /// to the validator for their commit-ish inputs and into + /// . + /// + protected string? CanonicalRepoPath { get; private set; } + + /// + /// Deferred validation message for , or + /// null when the path resolves. See the class remarks for why + /// this is not surfaced eagerly. + /// + protected string? RepoPathError { get; private set; } + + /// Picker listing the other worktrees of this repository. + public WorktreePickerViewModel WorktreePicker { get; } + + partial void OnRepoPathChanged(string value) + { + TryUpdateCanonicalRepoPath(); + SyncPickers(); + Validate(); + } + + /// + /// Resolve the prefilled repo path and prime the pickers. Derived + /// constructors call this last; see the class remarks. + /// + protected void InitializeRepoPath() + { + TryUpdateCanonicalRepoPath(); + SyncPickers(); + Validate(); + } + + /// + /// Called after changes so a form can + /// re-point its own ref pickers. The worktree picker is handled by + /// the base; forms with no ref pickers need not override. + /// + protected virtual void OnRepoPathResolved() + { + } + + /// + /// Extra required-input gate for the concrete form. The repo path + /// itself is already covered. + /// + protected virtual bool HasRequiredLocalInputs => true; + + protected sealed override bool HasRequiredInputs => + !string.IsNullOrWhiteSpace(RepoPath) && HasRequiredLocalInputs; + + private void SyncPickers() + { + WorktreePicker.CanonicalRepoPath = CanonicalRepoPath; + OnRepoPathResolved(); + } + + private void TryUpdateCanonicalRepoPath() + { + CanonicalRepoPath = null; + RepoPathError = null; + if (string.IsNullOrWhiteSpace(RepoPath)) return; + + var result = Validator.ValidateRepoPath(RepoPath); + if (result is RepoPathValidation.Valid valid) + { + CanonicalRepoPath = valid.CanonicalPath; + } + else + { + RepoPathError = ((RepoPathValidation.Invalid)result).Message; + } + } +} diff --git a/DiffViewer/ViewModels/MainViewModel.cs b/DiffViewer/ViewModels/MainViewModel.cs index 9dc917b..4757a05 100644 --- a/DiffViewer/ViewModels/MainViewModel.cs +++ b/DiffViewer/ViewModels/MainViewModel.cs @@ -638,7 +638,8 @@ public MainViewModel( IClipboardService? clipboardService = null, IImageDecoder? imageDecoder = null, string? initialFile = null, - IPullRequestWatcher? pullRequestWatcher = null) + IPullRequestWatcher? pullRequestWatcher = null, + IGitWorktreeEnumerator? worktreeEnumerator = null) { _repository = repository ?? throw new ArgumentNullException(nameof(repository)); _left = left ?? throw new ArgumentNullException(nameof(left)); @@ -684,7 +685,8 @@ public MainViewModel( if (recentContextsService is not null) { var identity = ContextIdentityFactory.Create(repository.Shape.RepoRoot, left, right); - _recents = new RecentContextsViewModel(recentContextsService, contextSwitcher, identity, newDiffDialogHost); + _recents = new RecentContextsViewModel( + recentContextsService, contextSwitcher, identity, newDiffDialogHost, worktreeEnumerator); _scope.Register(_recents); } diff --git a/DiffViewer/ViewModels/NewDiffDialogViewModel.cs b/DiffViewer/ViewModels/NewDiffDialogViewModel.cs index a99db84..b234a20 100644 --- a/DiffViewer/ViewModels/NewDiffDialogViewModel.cs +++ b/DiffViewer/ViewModels/NewDiffDialogViewModel.cs @@ -30,6 +30,7 @@ public sealed partial class NewDiffDialogViewModel : ObservableObject { private readonly IDiffLaunchValidator _validator; private readonly IGitRefEnumerator _refEnumerator; + private readonly IGitWorktreeEnumerator _worktreeEnumerator; private readonly IRecentContextsService _recentContexts; private readonly string? _prefilledRepoPath; private readonly string? _seedPullRequestUrl; @@ -44,6 +45,7 @@ public NewDiffDialogViewModel( DiffModeRegistry registry, IDiffLaunchValidator validator, IGitRefEnumerator refEnumerator, + IGitWorktreeEnumerator worktreeEnumerator, IRecentContextsService recentContexts, string? prefilledRepoPath = null, string? initialProviderId = null, @@ -52,6 +54,7 @@ public NewDiffDialogViewModel( ArgumentNullException.ThrowIfNull(registry); _validator = validator ?? throw new ArgumentNullException(nameof(validator)); _refEnumerator = refEnumerator ?? throw new ArgumentNullException(nameof(refEnumerator)); + _worktreeEnumerator = worktreeEnumerator ?? throw new ArgumentNullException(nameof(worktreeEnumerator)); _recentContexts = recentContexts ?? throw new ArgumentNullException(nameof(recentContexts)); _prefilledRepoPath = prefilledRepoPath; _seedPullRequestUrl = seedPullRequestUrl; @@ -114,7 +117,7 @@ public NewDiffFormViewModelBase CurrentForm if (!_formCache.TryGetValue(SelectedProvider, out var form)) { var deps = new FormDependencies( - _validator, _refEnumerator, _recentContexts, + _validator, _refEnumerator, _worktreeEnumerator, _recentContexts, _prefilledRepoPath, _seedPullRequestUrl); form = SelectedProvider.CreateForm(deps); _formCache[SelectedProvider] = form; diff --git a/DiffViewer/ViewModels/RecentContextsViewModel.cs b/DiffViewer/ViewModels/RecentContextsViewModel.cs index 8754fc0..158dec1 100644 --- a/DiffViewer/ViewModels/RecentContextsViewModel.cs +++ b/DiffViewer/ViewModels/RecentContextsViewModel.cs @@ -39,6 +39,13 @@ namespace DiffViewer.ViewModels; /// wired only when both a and a /// are provided; tests that don't /// exercise that path leave them null and the button hides. +/// +/// WorktreePicker re-points the current diff at +/// another worktree of the same repository, keeping both sides +/// untouched. It reuses — the +/// same VM the "New diff" dialog binds — with a write-back that runs +/// an in-place context switch instead of editing a text box. Wired +/// only when a switcher and a worktree enumerator are supplied. /// public sealed class RecentContextsViewModel : ObservableObject, IDisposable { @@ -52,7 +59,8 @@ public RecentContextsViewModel( IRecentContextsService service, IContextSwitcher? switcher, ContextIdentity? currentIdentity, - INewDiffDialogHost? newDiffDialogHost = null) + INewDiffDialogHost? newDiffDialogHost = null, + IGitWorktreeEnumerator? worktreeEnumerator = null) { _service = service ?? throw new ArgumentNullException(nameof(service)); _switcher = switcher; @@ -66,6 +74,61 @@ public RecentContextsViewModel( } NewDiffCommand = new AsyncRelayCommand(OpenNewDiffAsync, () => IsNewDiffEnabled); + + IsWorktreeSwitchEnabled = worktreeEnumerator is not null + && _switcher is not null + && _currentIdentity is not null; + WorktreePicker = IsWorktreeSwitchEnabled + ? new WorktreePickerViewModel( + worktreeEnumerator!, + writeBack: path => _ = SwitchToWorktreeAsync(path), + initialCanonicalRepoPath: _currentIdentity!.Value.CanonicalRepoPath) + : null; + } + + /// + /// Picker listing the worktrees of the active repository, or + /// null when worktree switching isn't wired (cold-launch + /// empty state, tests). The view hides its trigger when null. + /// + public WorktreePickerViewModel? WorktreePicker { get; } + + /// True when the worktree switcher should be shown. + public bool IsWorktreeSwitchEnabled { get; } + + /// + /// Re-open the current comparison rooted at another worktree. + /// Both sides are carried over verbatim: the point is to ask the + /// same question of a different checkout. Note that a ref like + /// HEAD is per-worktree, so the answer legitimately differs. + /// + private async Task SwitchToWorktreeAsync(string workingDirectory) + { + if (_switcher is null || _currentIdentity is null) return; + if (string.IsNullOrWhiteSpace(workingDirectory)) return; + if (ContextIdentityFactory.RepoPathsEqual( + workingDirectory, _currentIdentity.Value.CanonicalRepoPath)) + { + // Already here; a switch would tear down and rebuild the + // whole context for no change. + return; + } + + var parsed = new ParsedCommandLine( + workingDirectory, + _currentIdentity.Value.Left, + _currentIdentity.Value.Right); + + try + { + await _switcher.SwitchToAsync( + new DiffLaunchSource.Local(parsed), CancellationToken.None).ConfigureAwait(true); + } + catch + { + // The switcher surfaces its own errors to the user; a + // failed switch must not take the shell down with it. + } } /// MRU-ordered snapshot from the singleton service. diff --git a/DiffViewer/ViewModels/ViewStashFormViewModel.cs b/DiffViewer/ViewModels/ViewStashFormViewModel.cs index e787498..72aad8a 100644 --- a/DiffViewer/ViewModels/ViewStashFormViewModel.cs +++ b/DiffViewer/ViewModels/ViewStashFormViewModel.cs @@ -17,21 +17,10 @@ namespace DiffViewer.ViewModels; /// working-tree commit compared against its parent (HEAD at stash /// time). /// -public sealed partial class ViewStashFormViewModel : NewDiffFormViewModelBase +public sealed partial class ViewStashFormViewModel : LocalRepoFormViewModelBase { private readonly IGitRefEnumerator _enumerator; private readonly Func, Task>? _enumerateRunner; - private string? _canonicalRepoPath; - - [ObservableProperty] - private string _repoPath; - - [ObservableProperty] - private bool _isLoading; - - [ObservableProperty] - private bool _isLoaded; - private IReadOnlyList _stashes = Array.Empty(); /// The enumerated stash list. Bound to the inline ListBox. @@ -46,6 +35,12 @@ public sealed partial class ViewStashFormViewModel : NewDiffFormViewModelBase /// Drives the inline empty-state hint. public bool IsEmpty => IsLoaded && _stashes.Count == 0; + [ObservableProperty] + private bool _isLoading; + + [ObservableProperty] + private bool _isLoaded; + /// The stash the user clicked. null when nothing is /// selected yet. [ObservableProperty] @@ -54,30 +49,24 @@ public sealed partial class ViewStashFormViewModel : NewDiffFormViewModelBase public ViewStashFormViewModel( FormDependencies deps, Func, Task>? enumerateRunner = null) - : base(deps.Validator) + : base(deps) { _enumerator = deps.RefEnumerator ?? throw new ArgumentNullException(nameof(deps)); _enumerateRunner = enumerateRunner; - _repoPath = deps.PrefilledRepoPath ?? string.Empty; - Validate(); - if (!string.IsNullOrWhiteSpace(_repoPath)) - { - _ = EnumerateStashesAsync(); - } + InitializeRepoPath(); } - partial void OnRepoPathChanged(string value) + protected override void OnRepoPathResolved() { - // Reset stash state when repo changes. + // Reset stash state when the repo changes. _stashes = Array.Empty(); SelectedStash = null; IsLoaded = false; OnPropertyChanged(nameof(Stashes)); OnPropertyChanged(nameof(HasStashes)); OnPropertyChanged(nameof(IsEmpty)); - Validate(); - if (!string.IsNullOrWhiteSpace(value)) + if (!string.IsNullOrWhiteSpace(RepoPath)) { _ = EnumerateStashesAsync(); } @@ -85,21 +74,12 @@ partial void OnRepoPathChanged(string value) partial void OnSelectedStashChanged(StashEntry? value) => Validate(); - protected override bool HasRequiredInputs => - !string.IsNullOrWhiteSpace(RepoPath) && SelectedStash is not null; + protected override bool HasRequiredLocalInputs => SelectedStash is not null; protected override string? ComputeValidationError() { - _canonicalRepoPath = null; - if (string.IsNullOrWhiteSpace(RepoPath)) return null; - - var repoResult = Validator.ValidateRepoPath(RepoPath); - if (repoResult is not RepoPathValidation.Valid valid) - { - return ((RepoPathValidation.Invalid)repoResult).Message; - } - - _canonicalRepoPath = valid.CanonicalPath; + if (RepoPathError is not null) return RepoPathError; + if (CanonicalRepoPath is null) return null; if (IsLoaded && _stashes.Count == 0) { @@ -161,7 +141,7 @@ public override DiffLaunchSource BuildLaunchSource() var stash = SelectedStash ?? throw new InvalidOperationException("No stash selected."); var parsed = new ParsedCommandLine( - _canonicalRepoPath ?? RepoPath, + CanonicalRepoPath ?? RepoPath, new DiffSide.CommitIsh($"{stash.SymbolicName}^1"), new DiffSide.CommitIsh(stash.SymbolicName)); return new DiffLaunchSource.Local(parsed); diff --git a/DiffViewer/ViewModels/WorkingTreeVsCommitFormViewModel.cs b/DiffViewer/ViewModels/WorkingTreeVsCommitFormViewModel.cs index 6d18e76..08dce1c 100644 --- a/DiffViewer/ViewModels/WorkingTreeVsCommitFormViewModel.cs +++ b/DiffViewer/ViewModels/WorkingTreeVsCommitFormViewModel.cs @@ -15,14 +15,8 @@ namespace DiffViewer.ViewModels; /// canonical root, that path is pushed into the picker so its branch /// / tag / recent-ref enumeration targets the right repo. /// -public sealed partial class WorkingTreeVsCommitFormViewModel : NewDiffFormViewModelBase +public sealed partial class WorkingTreeVsCommitFormViewModel : LocalRepoFormViewModelBase { - private string? _canonicalRepoPath; - private string? _repoPathError; - - [ObservableProperty] - private string _repoPath; - [ObservableProperty] private string _commitIsh; @@ -30,98 +24,43 @@ public sealed partial class WorkingTreeVsCommitFormViewModel : NewDiffFormViewMo public RefPickerViewModel CommitIshPicker { get; } public WorkingTreeVsCommitFormViewModel(FormDependencies deps) - : base(deps.Validator) + : base(deps) { - _repoPath = deps.PrefilledRepoPath ?? string.Empty; _commitIsh = string.Empty; CommitIshPicker = new RefPickerViewModel( deps.RefEnumerator, deps.RecentContexts, writeBack: value => CommitIsh = value); - // Canonicalize the prefilled repo path BEFORE the first - // Validate() so the picker is enabled on dialog open when - // launching with an already-open context. Without this, the - // user would have to touch the repo-path text box to wake the - // picker up. Mirrors the OnRepoPathChanged ordering. - TryUpdateCanonicalRepoPath(); - SyncPickerRepoPath(); - Validate(); - } - - partial void OnRepoPathChanged(string value) - { - TryUpdateCanonicalRepoPath(); - SyncPickerRepoPath(); - Validate(); + InitializeRepoPath(); } partial void OnCommitIshChanged(string value) => Validate(); - protected override bool HasRequiredInputs => - !string.IsNullOrWhiteSpace(RepoPath) && !string.IsNullOrWhiteSpace(CommitIsh); + protected override bool HasRequiredLocalInputs => !string.IsNullOrWhiteSpace(CommitIsh); - /// - /// Resolve the user's repo-path input into either a canonical - /// repository root (stored in , - /// consumed by the picker for branch enumeration and by - /// ) or a deferred validation - /// message (stored in , surfaced by - /// once the rest of the form - /// is populated). Runs exactly once per repo-path change so the - /// picker stays in sync without re-validating on every keystroke - /// into the commit-ish field. Decoupling canonicalization from - /// is what makes the picker - /// reachable before the user types a commit-ish — historically the - /// canonical path was a side effect of validation, so the picker - /// was perma-disabled on dialog open. - /// - private void TryUpdateCanonicalRepoPath() - { - _canonicalRepoPath = null; - _repoPathError = null; - if (string.IsNullOrWhiteSpace(RepoPath)) return; - - var result = Validator.ValidateRepoPath(RepoPath); - if (result is RepoPathValidation.Valid v) - { - _canonicalRepoPath = v.CanonicalPath; - } - else - { - _repoPathError = ((RepoPathValidation.Invalid)result).Message; - } - } + protected override void OnRepoPathResolved() => + CommitIshPicker.CanonicalRepoPath = CanonicalRepoPath; protected override string? ComputeValidationError() { // Suppress every error message until all required fields are // populated — friendlier UX than flashing "Cannot resolve foo" - // while the user is mid-type. _repoPathError still drives the - // picker's enablement via TryUpdateCanonicalRepoPath; here it + // while the user is mid-type. RepoPathError still drives the + // picker's enablement via the base's canonicalization; here it // only governs what the dialog footer says. if (!HasRequiredInputs) return null; - if (_repoPathError is not null) return _repoPathError; + if (RepoPathError is not null) return RepoPathError; - var commitResult = Validator.ValidateCommitIsh(_canonicalRepoPath!, CommitIsh); + var commitResult = Validator.ValidateCommitIsh(CanonicalRepoPath!, CommitIsh); return commitResult is CommitIshValidation.Invalid invalid ? invalid.Message : null; } public override DiffLaunchSource BuildLaunchSource() { var parsed = new ParsedCommandLine( - _canonicalRepoPath ?? RepoPath, + CanonicalRepoPath ?? RepoPath, new DiffSide.CommitIsh(CommitIsh), new DiffSide.WorkingTree()); return new DiffLaunchSource.Local(parsed); } - - /// Push the latest canonical repo root into the picker. - /// Null when couldn't - /// resolve the user's input — the picker reads - /// off this value so a - /// null here disables the Pick… button. - private void SyncPickerRepoPath() - { - CommitIshPicker.CanonicalRepoPath = _canonicalRepoPath; - } } diff --git a/DiffViewer/ViewModels/WorkingTreeVsHeadFormViewModel.cs b/DiffViewer/ViewModels/WorkingTreeVsHeadFormViewModel.cs index 32f58d5..d27b212 100644 --- a/DiffViewer/ViewModels/WorkingTreeVsHeadFormViewModel.cs +++ b/DiffViewer/ViewModels/WorkingTreeVsHeadFormViewModel.cs @@ -1,4 +1,3 @@ -using CommunityToolkit.Mvvm.ComponentModel; using DiffViewer.Models; using DiffViewer.Services; @@ -9,46 +8,26 @@ namespace DiffViewer.ViewModels; /// On submit, builds the same the CLI /// produces for an argv of [repoPath]: /// left = CommitIsh("HEAD"), right = WorkingTree. +/// +/// Which worktree the path points at is not incidental here: +/// HEAD is per-worktree, so this form compares against a +/// different commit depending on the checkout chosen in the worktree +/// picker. /// -public sealed partial class WorkingTreeVsHeadFormViewModel : NewDiffFormViewModelBase +public sealed partial class WorkingTreeVsHeadFormViewModel : LocalRepoFormViewModelBase { - /// The validated repo root, populated by - /// . - /// Reset to null on every input change. - private string? _canonicalRepoPath; - - [ObservableProperty] - private string _repoPath; - public WorkingTreeVsHeadFormViewModel(FormDependencies deps) - : base(deps.Validator) + : base(deps) { - _repoPath = deps.PrefilledRepoPath ?? string.Empty; - Validate(); + InitializeRepoPath(); } - partial void OnRepoPathChanged(string value) => Validate(); - - protected override bool HasRequiredInputs => !string.IsNullOrWhiteSpace(RepoPath); - - protected override string? ComputeValidationError() - { - _canonicalRepoPath = null; - if (string.IsNullOrWhiteSpace(RepoPath)) return null; - - var result = Validator.ValidateRepoPath(RepoPath); - if (result is RepoPathValidation.Valid v) - { - _canonicalRepoPath = v.CanonicalPath; - return null; - } - return ((RepoPathValidation.Invalid)result).Message; - } + protected override string? ComputeValidationError() => RepoPathError; public override DiffLaunchSource BuildLaunchSource() { var parsed = new ParsedCommandLine( - _canonicalRepoPath ?? RepoPath, + CanonicalRepoPath ?? RepoPath, new DiffSide.CommitIsh("HEAD"), new DiffSide.WorkingTree()); return new DiffLaunchSource.Local(parsed); diff --git a/DiffViewer/ViewModels/WorktreePickerViewModel.cs b/DiffViewer/ViewModels/WorktreePickerViewModel.cs new file mode 100644 index 0000000..e0fcc40 --- /dev/null +++ b/DiffViewer/ViewModels/WorktreePickerViewModel.cs @@ -0,0 +1,135 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using CommunityToolkit.Mvvm.ComponentModel; +using CommunityToolkit.Mvvm.Input; +using DiffViewer.Services; + +namespace DiffViewer.ViewModels; + +/// +/// Backs the worktree-picker +/// attached to the repo-path input in the "New diff" dialog. Lists every +/// worktree of the repository the user has already typed, so switching +/// from one checkout to another is a click instead of remembering where +/// git worktree add put things. +/// +/// Why every local form gets one: it would be tempting to +/// show this only for working-tree modes, since two worktrees of a +/// repository share one object database and all shared refs — so +/// main..feature resolves identically from either. But +/// HEAD is per-worktree, and the dialog advertises +/// HEAD~3 as valid input, so which worktree you point at changes +/// the answer for commit-ish forms too. +/// +/// Lifetime mirrors : one +/// per form, constructed once, handed a write-back callback that +/// replaces the form's repo path, and re-pointed via +/// whenever that path changes. +/// +public sealed partial class WorktreePickerViewModel : ObservableObject +{ + private readonly IGitWorktreeEnumerator _enumerator; + private readonly Action _writeBack; + private readonly Func>, Task>>? _enumerateRunner; + + public WorktreePickerViewModel( + IGitWorktreeEnumerator enumerator, + Action writeBack, + string? initialCanonicalRepoPath = null, + Func>, Task>>? enumerateRunner = null) + { + _enumerator = enumerator ?? throw new ArgumentNullException(nameof(enumerator)); + _writeBack = writeBack ?? throw new ArgumentNullException(nameof(writeBack)); + _enumerateRunner = enumerateRunner; + _canonicalRepoPath = initialCanonicalRepoPath; + } + + /// The canonical repo path the picker operates against. + /// Setting this discards the cached enumeration; + /// re-enumerates on demand. + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(IsEnabled))] + private string? _canonicalRepoPath; + + partial void OnCanonicalRepoPathChanged(string? value) + { + _worktrees = Array.Empty(); + IsLoaded = false; + OnPropertyChanged(nameof(Worktrees)); + OnPropertyChanged(nameof(HasAlternativeWorktrees)); + } + + /// True when the picker has a repo path to work from; the + /// trigger button binds to this. + public bool IsEnabled => !string.IsNullOrWhiteSpace(CanonicalRepoPath); + + [ObservableProperty] + private bool _isLoading; + + [ObservableProperty] + private bool _isLoaded; + + private IReadOnlyList _worktrees = Array.Empty(); + + /// Every worktree of the current repository, main first. + public IReadOnlyList Worktrees => _worktrees; + + /// + /// True once loading found somewhere else to go. Drives the popup's + /// empty state: a repository with no linked worktrees should say so + /// rather than showing a one-row list of where the user already is. + /// + public bool HasAlternativeWorktrees => _worktrees.Count > 1; + + /// + /// Enumerate worktrees for the current repo path, off the UI thread. + /// Idempotent — repeated calls while loaded or loading are no-ops, + /// so the popup can call it on every open. + /// + public async Task EnsureLoadedAsync() + { + if (IsLoaded || IsLoading) return; + if (string.IsNullOrWhiteSpace(CanonicalRepoPath)) return; + + IsLoading = true; + try + { + var repoPath = CanonicalRepoPath; + var enumerate = () => _enumerator.Enumerate(repoPath!); + var result = _enumerateRunner is not null + ? await _enumerateRunner(enumerate).ConfigureAwait(true) + : await Task.Run(enumerate).ConfigureAwait(true); + + // The user may have re-pointed the picker mid-load. Drop + // stale results rather than overwrite the new state. + if (!string.Equals(CanonicalRepoPath, repoPath, StringComparison.Ordinal)) + { + return; + } + + _worktrees = result; + IsLoaded = true; + OnPropertyChanged(nameof(Worktrees)); + OnPropertyChanged(nameof(HasAlternativeWorktrees)); + } + finally + { + IsLoading = false; + } + } + + /// + /// Write the chosen worktree's working directory back into the + /// form's repo path. Worktrees whose directory is gone are rejected + /// — the row is shown so the user understands what git still knows + /// about, not so they can diff against a directory that isn't there. + /// + [RelayCommand] + private void PickWorktree(WorktreeEntry? entry) + { + if (entry is null || entry.IsMissing) return; + _writeBack(entry.WorkingDirectory); + } +} diff --git a/DiffViewer/Views/NewDiffDialog.xaml b/DiffViewer/Views/NewDiffDialog.xaml index 9724d31..a6e2c74 100644 --- a/DiffViewer/Views/NewDiffDialog.xaml +++ b/DiffViewer/Views/NewDiffDialog.xaml @@ -34,23 +34,7 @@ - - - - - - - - + + + + + + + + + + + diff --git a/DiffViewer/Views/WorktreePicker.xaml.cs b/DiffViewer/Views/WorktreePicker.xaml.cs new file mode 100644 index 0000000..6ff8ac7 --- /dev/null +++ b/DiffViewer/Views/WorktreePicker.xaml.cs @@ -0,0 +1,86 @@ +using System; +using System.Windows; +using System.Windows.Controls; +using System.Windows.Controls.Primitives; +using DiffViewer.ViewModels; + +namespace DiffViewer.Views; + +/// +/// View wiring for the worktree-picker popup. Mirrors +/// : the control's +/// is a +/// owned by the surrounding form, +/// and / are exposed +/// as dependency properties so the host can drive them from a toggle +/// button in pure XAML. +/// +public partial class WorktreePicker : UserControl +{ + /// Drives the embedded . Two-way + /// so the popup's StaysOpen=False auto-dismiss flows back to + /// the host toggle button's IsChecked state. + public static readonly DependencyProperty IsOpenProperty = + DependencyProperty.Register( + nameof(IsOpen), + typeof(bool), + typeof(WorktreePicker), + new FrameworkPropertyMetadata( + defaultValue: false, + FrameworkPropertyMetadataOptions.BindsTwoWayByDefault)); + + public bool IsOpen + { + get => (bool)GetValue(IsOpenProperty); + set => SetValue(IsOpenProperty, value); + } + + /// The element the popup anchors itself to. + public static readonly DependencyProperty PlacementTargetProperty = + DependencyProperty.Register( + nameof(PlacementTarget), + typeof(UIElement), + typeof(WorktreePicker), + new PropertyMetadata(null)); + + public UIElement? PlacementTarget + { + get => (UIElement?)GetValue(PlacementTargetProperty); + set => SetValue(PlacementTargetProperty, value); + } + + public WorktreePicker() + { + InitializeComponent(); + } + + /// + /// Enumerate when the popup opens rather than when the form is + /// constructed, so opening the dialog never pays for a worktree scan + /// the user didn't ask for. Fire-and-forget; the VM handles + /// re-entrancy and stale results internally. + /// + private async void OnPopupOpened(object sender, EventArgs e) + { + if (DataContext is not WorktreePickerViewModel vm) return; + try + { + await vm.EnsureLoadedAsync(); + } + catch + { + // EnsureLoadedAsync swallows enumerator failures internally + // (LibGit2GitWorktreeEnumerator returns an empty list on + // throw). Belt and suspenders so an unexpected exception + // never tears down the dialog. + } + } + + /// Close the popup after a pick. The Button's Click fires + /// after its Command, so the VM has already written the chosen + /// path back into the form by the time we run. + private void OnWorktreeRowClicked(object sender, RoutedEventArgs e) + { + IsOpen = false; + } +} From ea6d556985d03622463cf6a9cd03545d68a0966a Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 10:34:43 -0700 Subject: [PATCH 3/7] Normalize the resolved common git directory CommonGitDirectory is documented as identical across every worktree of a repository, which is what makes "same repo, different checkout" recognizable. It wasn't: libgit2 reports a main worktree's git directory with a trailing separator, while the commondir pointer resolves through Path.GetFullPath without one, so the two forms compared unequal. No consumer relied on it yet, so this is a trap rather than a live bug. The repository-service test now compares the raw strings instead of normalizing both sides, which is what let the gap through. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- .../Services/RepositoryServiceTests.cs | 12 ++++++---- .../Utility/GitWorktreeLayoutTests.cs | 24 +++++++++++++++++++ DiffViewer/Utility/GitWorktreeLayout.cs | 24 +++++++++++++------ 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/DiffViewer.Tests/Services/RepositoryServiceTests.cs b/DiffViewer.Tests/Services/RepositoryServiceTests.cs index d75f58c..4a1cd98 100644 --- a/DiffViewer.Tests/Services/RepositoryServiceTests.cs +++ b/DiffViewer.Tests/Services/RepositoryServiceTests.cs @@ -65,10 +65,14 @@ public void Shape_ForALinkedWorktree_ReportsItsNameAndTheSharedCommonDirectory() worktreeSvc.Shape.IsLinkedWorktree.Should().BeTrue(); worktreeSvc.Shape.WorktreeName.Should().Be("feature-a"); - // Both worktrees resolve to the same common dir - that is what - // makes "same repository, different checkout" recognizable. - Normalize(worktreeSvc.Shape.CommonGitDirectory) - .Should().Be(Normalize(mainSvc.Shape.CommonGitDirectory)); + // Compared raw, not normalized: the whole point of + // CommonGitDirectory is that two worktrees of one repository + // produce the identical string. libgit2 reports a main + // worktree's git dir with a trailing separator and the + // commondir pointer resolves without one, so this invariant + // only holds because the layout helper normalizes both. + worktreeSvc.Shape.CommonGitDirectory + .Should().Be(mainSvc.Shape.CommonGitDirectory); Normalize(worktreeSvc.Shape.GitDir) .Should().NotBe(Normalize(worktreeSvc.Shape.CommonGitDirectory)); } diff --git a/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs b/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs index ab89b41..b373073 100644 --- a/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs +++ b/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs @@ -52,6 +52,30 @@ public void ResolveCommonDirectory_ForAMainWorktree_ReturnsTheDirectoryUnchanged GitWorktreeLayout.ResolveCommonDirectory(gitDir).Should().Be(gitDir); } + [Fact] + public void ResolveCommonDirectory_StripsATrailingSeparator() + { + // libgit2 reports a main worktree's git directory with a + // trailing separator, but the commondir pointer resolves + // without one. Both must land on the same string or two + // worktrees of a repository compare as unrelated. + var gitDir = CreateDirectory(".git"); + var withSeparator = gitDir + Path.DirectorySeparatorChar; + + GitWorktreeLayout.ResolveCommonDirectory(withSeparator) + .Should().Be(GitWorktreeLayout.ResolveCommonDirectory(gitDir)); + } + + [Fact] + public void ResolveCommonDirectory_FromMainAndLinkedWorktrees_ReturnsTheIdenticalString() + { + var mainGitDir = CreateDirectory(".git") + Path.DirectorySeparatorChar; + var linkedGitDir = CreateLinkedWorktreeGitDirectory("feature-a"); + + GitWorktreeLayout.ResolveCommonDirectory(linkedGitDir) + .Should().Be(GitWorktreeLayout.ResolveCommonDirectory(mainGitDir)); + } + [Fact] public void ResolveCommonDirectory_FollowsARelativeCommondirPointer() { diff --git a/DiffViewer/Utility/GitWorktreeLayout.cs b/DiffViewer/Utility/GitWorktreeLayout.cs index 123239e..ac1cf98 100644 --- a/DiffViewer/Utility/GitWorktreeLayout.cs +++ b/DiffViewer/Utility/GitWorktreeLayout.cs @@ -39,10 +39,17 @@ public static bool IsLinkedWorktree(string gitDirectory) } /// - /// Resolve the repository's common directory. Returns - /// unchanged when it is already the - /// common directory, or when the commondir pointer is - /// unreadable or empty. + /// Resolve the repository's common directory, normalized so that + /// every worktree of a repository produces the identical string. + /// Returns the normalized when it is + /// already the common directory, or when the commondir + /// pointer is unreadable or empty. + /// + /// Normalization is not cosmetic: libgit2 reports a main + /// worktree's git directory with a trailing separator but the + /// commondir pointer resolves without one, so comparing the + /// two raw forms would report two worktrees of one repository as + /// unrelated. /// public static string ResolveCommonDirectory(string gitDirectory) { @@ -51,14 +58,14 @@ public static string ResolveCommonDirectory(string gitDirectory) try { var commonDirFile = Path.Combine(gitDirectory, CommonDirFileName); - if (!File.Exists(commonDirFile)) return gitDirectory; + if (!File.Exists(commonDirFile)) return Normalize(gitDirectory); var contents = File.ReadAllText(commonDirFile).Trim(); - if (contents.Length == 0) return gitDirectory; + if (contents.Length == 0) return Normalize(gitDirectory); // The pointer is normally relative to the worktree's git // directory; Path.Combine leaves an absolute pointer alone. - return Path.GetFullPath(Path.Combine(gitDirectory, contents)); + return Normalize(Path.Combine(gitDirectory, contents)); } catch (Exception) { @@ -66,6 +73,9 @@ public static string ResolveCommonDirectory(string gitDirectory) } } + private static string Normalize(string path) => + Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); + /// /// Git's name for the worktree owning , /// or null when it is not a linked worktree. The name is the From a36c3a2f61630ff72474580621b78a966794ffb8 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 11:26:29 -0700 Subject: [PATCH 4/7] Normalize the common git directory on the probing-failure path too ResolveCommonDirectory normalized every success path but returned the raw string from its catch block, which reintroduces exactly the trailing-separator mismatch the method exists to eliminate: a probing failure would silently make two worktrees of one repository compare as unrelated. Normalize is now best-effort itself, so the failure path can use it without risking a throw from a helper documented to degrade. Also corrects the WorktreePicker comment that claimed EnsureLoadedAsync swallows enumerator failures. It does not - it has try/finally, not try/catch, and relies on the enumerator returning an empty list. The view-level handler is the actual safety net, being the async void boundary where an unexpected exception would tear the dialog down. Both found by Copilot code review on PR #24. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- .../Utility/GitWorktreeLayoutTests.cs | 25 +++++++++++++++++ DiffViewer/Utility/GitWorktreeLayout.cs | 27 ++++++++++++++++--- DiffViewer/Views/WorktreePicker.xaml.cs | 10 ++++--- 3 files changed, 55 insertions(+), 7 deletions(-) diff --git a/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs b/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs index b373073..6833f74 100644 --- a/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs +++ b/DiffViewer.Tests/Utility/GitWorktreeLayoutTests.cs @@ -105,6 +105,31 @@ public void ResolveCommonDirectory_WhenPointerIsEmpty_FallsBackToTheGivenDirecto GitWorktreeLayout.ResolveCommonDirectory(gitDir).Should().Be(gitDir); } + [Fact] + public void ResolveCommonDirectory_WhenThePathCannotBeExpanded_ReturnsItWithoutThrowing() + { + // A path the framework refuses to expand takes the failure + // branch. It must still come back rather than throwing out of a + // helper every caller treats as defensive. + var unexpandable = "C:\\repo\0bad"; + + var act = () => GitWorktreeLayout.ResolveCommonDirectory(unexpandable); + + act.Should().NotThrow(); + act().Should().Be(unexpandable); + } + + [Fact] + public void ResolveCommonDirectory_NeverReturnsATrailingSeparator() + { + // The invariant the whole helper exists for: every return path, + // including the failure branch, hands back a comparable string. + var gitDir = CreateDirectory(".git") + Path.DirectorySeparatorChar; + + GitWorktreeLayout.ResolveCommonDirectory(gitDir) + .Should().NotEndWith(Path.DirectorySeparatorChar.ToString()); + } + [Fact] public void TryGetWorktreeName_ForALinkedWorktree_ReturnsTheAdministrativeDirectoryName() { diff --git a/DiffViewer/Utility/GitWorktreeLayout.cs b/DiffViewer/Utility/GitWorktreeLayout.cs index ac1cf98..4005959 100644 --- a/DiffViewer/Utility/GitWorktreeLayout.cs +++ b/DiffViewer/Utility/GitWorktreeLayout.cs @@ -69,12 +69,33 @@ public static string ResolveCommonDirectory(string gitDirectory) } catch (Exception) { - return gitDirectory; + // Still normalize: returning the raw string here would + // reintroduce exactly the trailing-separator mismatch this + // method exists to eliminate, so a probing failure would + // silently make two worktrees of one repository compare as + // unrelated. Normalize is itself best-effort. + return Normalize(gitDirectory); } } - private static string Normalize(string path) => - Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); + /// + /// Canonical form used for every value this helper hands back: + /// absolute, with any trailing separator removed. Best-effort — a + /// path the framework refuses to expand (invalid characters, too + /// long) is returned unchanged rather than throwing, because every + /// caller here is documented to degrade rather than fail. + /// + private static string Normalize(string path) + { + try + { + return Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); + } + catch (Exception) + { + return path; + } + } /// /// Git's name for the worktree owning , diff --git a/DiffViewer/Views/WorktreePicker.xaml.cs b/DiffViewer/Views/WorktreePicker.xaml.cs index 6ff8ac7..1558891 100644 --- a/DiffViewer/Views/WorktreePicker.xaml.cs +++ b/DiffViewer/Views/WorktreePicker.xaml.cs @@ -69,10 +69,12 @@ private async void OnPopupOpened(object sender, EventArgs e) } catch { - // EnsureLoadedAsync swallows enumerator failures internally - // (LibGit2GitWorktreeEnumerator returns an empty list on - // throw). Belt and suspenders so an unexpected exception - // never tears down the dialog. + // EnsureLoadedAsync does not catch: it relies on the + // enumerator contract, and LibGit2GitWorktreeEnumerator + // returns an empty list rather than throwing. This handler + // is the actual safety net — an async void event handler is + // where an unexpected exception would otherwise tear the + // dialog down. } } From a1844b5df615c666ef2df876da34faf4765bf04a Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 11:40:34 -0700 Subject: [PATCH 5/7] Fix worktree picker load coalescing, empty-state, and popup scrolling Five issues from the second Copilot review pass, all confirmed against the code: EnsureLoadedAsync dropped a stale result and returned. Because the IsLoading guard turns away any caller arriving mid-load, a repo-path change during enumeration could leave an already-open popup empty with nothing left to trigger a reload. It now loops and re-enumerates the new path, which is what the turned-away caller was relying on. HasAlternativeWorktrees counted rows, but the enumerator deliberately omits the main worktree for a bare hub, so a single linked worktree read as "no alternatives". It now asks whether any entry is not the current one. A listed-but-unreachable pruned worktree still counts, so the popup never claims there is nowhere to go while visibly listing somewhere. The empty-state message was bound to the negation of that flag, so it rendered alongside "Loading..." asserting an answer enumeration had not produced yet. Added ShowsEmptyState, gated on IsLoaded. The popup body was a StackPanel, which measures children with unbounded height - the ScrollViewer never established a viewport, so the Border's MaxHeight clipped lower rows out of reach entirely. Now a Grid with a star-sized list row, matching RefPicker. RecordLaunchAsync captures the worktree labels but had no service-level coverage; the existing tests use nonexistent paths. Added tests over a synthetic on-disk layout covering main and linked worktrees, disk round-trip, healing of a legacy unlabeled row, and the non-repository path. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- .../RecentContextsServiceTests.cs | 123 ++++++++++++++++++ .../WorktreePickerViewModelTests.cs | 95 ++++++++++++++ .../ViewModels/WorktreePickerViewModel.cs | 70 +++++++--- DiffViewer/Views/WorktreePicker.xaml | 33 +++-- 4 files changed, 292 insertions(+), 29 deletions(-) diff --git a/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs b/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs index da67404..1d6c4c5 100644 --- a/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs +++ b/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs @@ -23,8 +23,131 @@ public RecentContextsServiceTests() public void Dispose() { try { if (File.Exists(_path)) File.Delete(_path); } catch { /* best-effort */ } + foreach (var directory in _scratchDirectories) + { + try { Directory.Delete(directory, recursive: true); } catch { /* best-effort */ } + } + } + + private readonly List _scratchDirectories = new(); + + /// Build a synthetic checkout on disk: a working directory + /// whose .git is either a real directory (main worktree) or a + /// pointer file into a shared repo's administrative directory + /// (linked worktree). Enough for the label capture on the recents + /// write path, which reads files rather than opening libgit2. + private string CreateSyntheticCheckout(string repositoryName, string? worktreeName = null) + { + var root = Path.Combine(Path.GetTempPath(), $"recents-repo-{Guid.NewGuid():N}"); + _scratchDirectories.Add(root); + + var repository = Path.Combine(root, repositoryName); + var commonGitDir = Path.Combine(repository, ".git"); + Directory.CreateDirectory(commonGitDir); + + if (worktreeName is null) return repository; + + var administrative = Path.Combine(commonGitDir, "worktrees", worktreeName); + Directory.CreateDirectory(administrative); + File.WriteAllText(Path.Combine(administrative, "commondir"), "../..\n"); + + // The worktree directory name deliberately differs from the + // worktree's git name, which is the case that makes the path + // leaf useless as a label. + var worktreeDirectory = Path.Combine(root, "checkouts", $"wt-{worktreeName}"); + Directory.CreateDirectory(worktreeDirectory); + File.WriteAllText(Path.Combine(worktreeDirectory, ".git"), $"gitdir: {administrative}\n"); + return worktreeDirectory; + } + + [Fact] + public async Task RecordLaunchAsync_ForAMainWorktree_StampsTheRepositoryNameAndNoWorktreeName() + { + var repoPath = CreateSyntheticCheckout("DiffViewer"); + var svc = new RecentContextsService(_path); + + await svc.RecordLaunchAsync( + ContextIdentityFactory.Create(repoPath, Left, Right), Left, Right); + + svc.Current.Should().ContainSingle(); + svc.Current[0].RepositoryName.Should().Be("DiffViewer"); + svc.Current[0].WorktreeName.Should().BeNull(); + } + + [Fact] + public async Task RecordLaunchAsync_ForALinkedWorktree_StampsBothLabels() + { + var worktreePath = CreateSyntheticCheckout("DiffViewer", worktreeName: "feature-x"); + var svc = new RecentContextsService(_path); + + await svc.RecordLaunchAsync( + ContextIdentityFactory.Create(worktreePath, Left, Right), Left, Right); + + svc.Current[0].RepositoryName.Should().Be("DiffViewer"); + svc.Current[0].WorktreeName.Should().Be("feature-x"); + } + + [Fact] + public async Task RecordLaunchAsync_PersistsTheLabelsToDisk() + { + var worktreePath = CreateSyntheticCheckout("DiffViewer", worktreeName: "feature-x"); + var svc = new RecentContextsService(_path); + + await svc.RecordLaunchAsync( + ContextIdentityFactory.Create(worktreePath, Left, Right), Left, Right); + + var reloaded = new RecentContextsService(_path); + await reloaded.LoadAsync(); + + reloaded.Current[0].RepositoryName.Should().Be("DiffViewer"); + reloaded.Current[0].WorktreeName.Should().Be("feature-x"); + } + + [Fact] + public async Task RecordLaunchAsync_ReRecordingAnUnlabeledRow_HealsItInPlace() + { + // A row written before worktree labelling existed carries none. + // Re-launching the same diff must fill them in rather than + // leaving a permanently unlabeled duplicate-looking entry. + var worktreePath = CreateSyntheticCheckout("DiffViewer", worktreeName: "feature-x"); + var identity = ContextIdentityFactory.Create(worktreePath, Left, Right); + + await RecentsStore.ReadAndMutateAsync(_path, _ => RecentsDoc.From(new[] + { + new RecentLaunchContext(identity, Left, Right, DateTimeOffset.UtcNow.AddDays(-1)), + })); + + var svc = new RecentContextsService(_path); + await svc.LoadAsync(); + svc.Current.Should().ContainSingle(); + svc.Current[0].RepositoryName.Should().BeNull("precondition: the legacy row is unlabeled"); + + await svc.RecordLaunchAsync(identity, Left, Right); + + svc.Current.Should().ContainSingle("re-recording bumps the existing row rather than adding one"); + svc.Current[0].RepositoryName.Should().Be("DiffViewer"); + svc.Current[0].WorktreeName.Should().Be("feature-x"); } + [Fact] + public async Task RecordLaunchAsync_ForAPathThatIsNotARepository_LeavesTheLabelsNull() + { + // The label lookup is best-effort; an unreadable path must not + // fail the launch record. + var svc = new RecentContextsService(_path); + var identity = ContextIdentityFactory.Create( + Path.Combine(Path.GetTempPath(), $"not-a-repo-{Guid.NewGuid():N}"), Left, Right); + + await svc.RecordLaunchAsync(identity, Left, Right); + + svc.Current.Should().ContainSingle(); + svc.Current[0].RepositoryName.Should().BeNull(); + svc.Current[0].WorktreeName.Should().BeNull(); + } + + private static readonly DiffSide Left = new DiffSide.CommitIsh("HEAD"); + private static readonly DiffSide Right = new DiffSide.WorkingTree(); + [Fact] public void Current_BeforeLoad_IsEmpty() { diff --git a/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs b/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs index 889cc8f..2c15394 100644 --- a/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/WorktreePickerViewModelTests.cs @@ -137,6 +137,101 @@ public void PickWorktree_WithNoSelection_WritesNothing() written.Should().BeEmpty(); } + [Fact] + public async Task EnsureLoadedAsync_WhenTheRepoPathChangesMidFlight_LoadsTheNewPath() + { + // The IsLoading guard turns away a caller that arrives during a + // load, so a stale result must trigger a re-enumeration rather + // than being dropped - otherwise an already-open popup sits + // empty with nothing left to reload it. + var stub = new StubWorktreeEnumerator(MainWorktree(), Linked("feature-a")); + WorktreePickerViewModel? picker = null; + var repointed = false; + + picker = new WorktreePickerViewModel( + stub, + writeBack: _ => { }, + initialCanonicalRepoPath: @"C:\repos\first", + enumerateRunner: work => + { + var result = work(); + if (!repointed) + { + // Simulate the user re-pointing the field while this + // enumeration was in flight. + repointed = true; + picker!.CanonicalRepoPath = @"C:\repos\second"; + } + return Task.FromResult(result); + }); + + await picker.EnsureLoadedAsync(); + + picker.IsLoaded.Should().BeTrue(); + picker.IsLoading.Should().BeFalse(); + picker.Worktrees.Should().HaveCount(2); + stub.EnumeratedPaths.Should().Equal(@"C:\repos\first", @"C:\repos\second"); + } + + [Fact] + public async Task HasAlternativeWorktrees_ForABareHubWithOneLinkedWorktree_IsTrue() + { + // The enumerator omits the main worktree for a bare hub, so a + // row count would call this "no alternatives" even though the + // listed worktree is somewhere else to go. + var picker = MakePicker(out _, out _, Linked("feature-a")); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + picker.HasAlternativeWorktrees.Should().BeTrue(); + } + + [Fact] + public async Task HasAlternativeWorktrees_WhenTheOnlyEntryIsTheCurrentOne_IsFalse() + { + var picker = MakePicker(out _, out _, MainWorktree()); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + picker.HasAlternativeWorktrees.Should().BeFalse(); + } + + [Fact] + public void ShowsEmptyState_BeforeLoadingCompletes_IsFalse() + { + // Otherwise the popup asserts "no other worktrees" next to the + // "Loading..." label, before enumeration has an answer. + var picker = MakePicker(out _, out _, MainWorktree(), Linked("feature-a")); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + picker.IsLoaded.Should().BeFalse(); + picker.ShowsEmptyState.Should().BeFalse(); + } + + [Fact] + public async Task ShowsEmptyState_AfterLoadingARepoWithNoAlternatives_IsTrue() + { + var picker = MakePicker(out _, out _, MainWorktree()); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + picker.ShowsEmptyState.Should().BeTrue(); + } + + [Fact] + public async Task ShowsEmptyState_AfterLoadingARepoWithAlternatives_IsFalse() + { + var picker = MakePicker(out _, out _, MainWorktree(), Linked("feature-a")); + picker.CanonicalRepoPath = @"C:\repos\diffviewer"; + + await picker.EnsureLoadedAsync(); + + picker.ShowsEmptyState.Should().BeFalse(); + } + private static WorktreePickerViewModel MakePicker( out StubWorktreeEnumerator enumerator, out List written, diff --git a/DiffViewer/ViewModels/WorktreePickerViewModel.cs b/DiffViewer/ViewModels/WorktreePickerViewModel.cs index e0fcc40..39c5632 100644 --- a/DiffViewer/ViewModels/WorktreePickerViewModel.cs +++ b/DiffViewer/ViewModels/WorktreePickerViewModel.cs @@ -57,8 +57,7 @@ partial void OnCanonicalRepoPathChanged(string? value) { _worktrees = Array.Empty(); IsLoaded = false; - OnPropertyChanged(nameof(Worktrees)); - OnPropertyChanged(nameof(HasAlternativeWorktrees)); + RaiseListDerivedChanged(); } /// True when the picker has a repo path to work from; the @@ -69,6 +68,7 @@ partial void OnCanonicalRepoPathChanged(string? value) private bool _isLoading; [ObservableProperty] + [NotifyPropertyChangedFor(nameof(ShowsEmptyState))] private bool _isLoaded; private IReadOnlyList _worktrees = Array.Empty(); @@ -77,11 +77,24 @@ partial void OnCanonicalRepoPathChanged(string? value) public IReadOnlyList Worktrees => _worktrees; /// - /// True once loading found somewhere else to go. Drives the popup's - /// empty state: a repository with no linked worktrees should say so - /// rather than showing a one-row list of where the user already is. + /// True when the enumeration found somewhere other than here to go. + /// + /// Deliberately "any entry that isn't current" rather than a + /// row count. The enumerator omits the main worktree for a bare hub, + /// so counting rows would report a single linked worktree as no + /// alternative at all. A row that is present but unreachable (a + /// pruned worktree) still counts, so the popup never claims there is + /// nothing to switch to while visibly listing something. + /// + public bool HasAlternativeWorktrees => _worktrees.Any(entry => !entry.IsCurrent); + + /// + /// True when the popup should say the repository has nowhere else to + /// go. Gated on so the message does not sit + /// next to "Loading…" claiming an answer enumeration hasn't produced + /// yet. /// - public bool HasAlternativeWorktrees => _worktrees.Count > 1; + public bool ShowsEmptyState => IsLoaded && !HasAlternativeWorktrees; /// /// Enumerate worktrees for the current repo path, off the UI thread. @@ -96,23 +109,33 @@ public async Task EnsureLoadedAsync() IsLoading = true; try { - var repoPath = CanonicalRepoPath; - var enumerate = () => _enumerator.Enumerate(repoPath!); - var result = _enumerateRunner is not null - ? await _enumerateRunner(enumerate).ConfigureAwait(true) - : await Task.Run(enumerate).ConfigureAwait(true); - - // The user may have re-pointed the picker mid-load. Drop - // stale results rather than overwrite the new state. - if (!string.Equals(CanonicalRepoPath, repoPath, StringComparison.Ordinal)) + // Loop rather than bail on a stale result. The picker can be + // re-pointed mid-flight, and a caller that arrives during + // the load is turned away by the IsLoading guard above — so + // simply dropping the stale result would leave an + // already-open popup empty with nothing left to trigger a + // reload. Re-enumerating here is what that turned-away + // caller is relying on. + while (true) { + var repoPath = CanonicalRepoPath; + if (string.IsNullOrWhiteSpace(repoPath)) return; + + var enumerate = () => _enumerator.Enumerate(repoPath!); + var result = _enumerateRunner is not null + ? await _enumerateRunner(enumerate).ConfigureAwait(true) + : await Task.Run(enumerate).ConfigureAwait(true); + + if (!string.Equals(CanonicalRepoPath, repoPath, StringComparison.Ordinal)) + { + continue; + } + + _worktrees = result; + IsLoaded = true; + RaiseListDerivedChanged(); return; } - - _worktrees = result; - IsLoaded = true; - OnPropertyChanged(nameof(Worktrees)); - OnPropertyChanged(nameof(HasAlternativeWorktrees)); } finally { @@ -120,6 +143,13 @@ public async Task EnsureLoadedAsync() } } + private void RaiseListDerivedChanged() + { + OnPropertyChanged(nameof(Worktrees)); + OnPropertyChanged(nameof(HasAlternativeWorktrees)); + OnPropertyChanged(nameof(ShowsEmptyState)); + } + /// /// Write the chosen worktree's working directory back into the /// form's repo path. Worktrees whose directory is gone are rejected diff --git a/DiffViewer/Views/WorktreePicker.xaml b/DiffViewer/Views/WorktreePicker.xaml index 2c06ebf..0c61a06 100644 --- a/DiffViewer/Views/WorktreePicker.xaml +++ b/DiffViewer/Views/WorktreePicker.xaml @@ -9,7 +9,6 @@ x:Name="Self"> - @@ -52,12 +51,25 @@ - - + + + + + + + + - + @@ -93,16 +105,19 @@ - - + - + Visibility="{Binding ShowsEmptyState, Converter={StaticResource BoolToVisibility}}"/> + From 51e83ee3f93e15425062e30bd68a8cbf08f56549 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 19:36:52 -0700 Subject: [PATCH 6/7] Move the recents worktree-label probe off the UI thread RecordLaunchAsync captured worktree labels synchronously. The coordinator awaits it from the UI thread during a context swap, and _gate.WaitAsync completes synchronously when uncontended, so everything up to the first real suspension ran on the dispatcher - including GitWorktreeLayout.Describe, which stats the repo path. A repo on an offline or slow network share would block that stat until SMB timed out and freeze the window mid-launch, violating the rule in AGENTS.md section 9 that filesystem IO must not block WPF's dispatcher. The probe now runs through a runner that defaults to Task.Run, awaited before the gate is taken - which both guarantees the method yields before touching the file system and shortens the critical section. The runner is an internal seam mirroring IRootScanRunner in LocalRepoLocator so tests can observe which thread the probe lands on; two tests cover that it runs off the caller and that no probing happens synchronously. Found by Copilot code review on PR #24. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- .../RecentContextsServiceTests.cs | 50 +++++++++++++++++++ DiffViewer/Services/RecentContextsService.cs | 41 ++++++++++++--- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs b/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs index 1d6c4c5..d11e6ac 100644 --- a/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs +++ b/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs @@ -129,6 +129,56 @@ await RecentsStore.ReadAndMutateAsync(_path, _ => RecentsDoc.From(new[] svc.Current[0].WorktreeName.Should().Be("feature-x"); } + [Fact] + public async Task RecordLaunchAsync_RunsTheLabelProbeOffTheCallingThread() + { + // The coordinator awaits this from the UI thread during a context + // swap, and the gate completes synchronously when uncontended, so + // a synchronous probe would stat the repo path on the dispatcher. + // A repo on an offline share would then freeze the window. + var repoPath = CreateSyntheticCheckout("DiffViewer", worktreeName: "feature-x"); + var callingThreadId = Environment.CurrentManagedThreadId; + var probeThreadId = callingThreadId; + + var svc = new RecentContextsService(_path, labelRunner: probe => Task.Run(() => + { + probeThreadId = Environment.CurrentManagedThreadId; + return probe(); + })); + + await svc.RecordLaunchAsync( + ContextIdentityFactory.Create(repoPath, Left, Right), Left, Right); + + probeThreadId.Should().NotBe(callingThreadId); + // ...and the labels still land, so the move didn't cost behaviour. + svc.Current[0].RepositoryName.Should().Be("DiffViewer"); + svc.Current[0].WorktreeName.Should().Be("feature-x"); + } + + [Fact] + public async Task RecordLaunchAsync_YieldsBeforeProbingTheFileSystem() + { + // Guards the specific trap: `_gate.WaitAsync` completes + // synchronously when uncontended, so anything before the first + // real await stays on the caller's thread. + var repoPath = CreateSyntheticCheckout("DiffViewer"); + var probed = false; + + var svc = new RecentContextsService(_path, labelRunner: async probe => + { + await Task.Yield(); + probed = true; + return probe(); + }); + + var pending = svc.RecordLaunchAsync( + ContextIdentityFactory.Create(repoPath, Left, Right), Left, Right); + + probed.Should().BeFalse("the probe must not have run synchronously on the caller"); + await pending; + probed.Should().BeTrue(); + } + [Fact] public async Task RecordLaunchAsync_ForAPathThatIsNotARepository_LeavesTheLabelsNull() { diff --git a/DiffViewer/Services/RecentContextsService.cs b/DiffViewer/Services/RecentContextsService.cs index 089ba12..92acedd 100644 --- a/DiffViewer/Services/RecentContextsService.cs +++ b/DiffViewer/Services/RecentContextsService.cs @@ -36,6 +36,7 @@ public sealed class RecentContextsService : IRecentContextsService public const int MaxEntries = 10; private readonly string _filePath; + private readonly Func, Task>? _labelRunner; private readonly SemaphoreSlim _gate = new(1, 1); private IReadOnlyList _current = Array.Empty(); @@ -52,8 +53,22 @@ public sealed class RecentContextsService : IRecentContextsService public RecentContextsService() : this(DefaultFilePath) { } public RecentContextsService(string filePath) + : this(filePath, labelRunner: null) + { + } + + /// Seam controlling how the worktree-label + /// probe is dispatched. Production leaves this null and gets + /// ; tests substitute a + /// runner so they can observe which thread the probe lands on. + /// Mirrors in + /// . + internal RecentContextsService( + string filePath, + Func, Task>? labelRunner) { _filePath = filePath ?? throw new ArgumentNullException(nameof(filePath)); + _labelRunner = labelRunner; } /// @@ -82,15 +97,29 @@ public async Task RecordLaunchAsync( ArgumentNullException.ThrowIfNull(leftDisplay); ArgumentNullException.ThrowIfNull(rightDisplay); + // Probe off the calling thread, and before taking the gate. + // + // The coordinator awaits this from the UI thread during a context + // swap. `_gate.WaitAsync` completes synchronously when + // uncontended — which is the normal case — so everything up to + // the first real suspension runs on the dispatcher. Describe() + // stats the repo path, and a repo on an offline or slow network + // share blocks that stat until SMB times out, freezing the + // window mid-launch. Awaiting a runner here guarantees the method + // yields before touching the file system, per AGENTS.md §9. + var repoPath = identity.CanonicalRepoPath; + var probe = () => GitWorktreeLayout.Describe(repoPath); + var labels = _labelRunner is not null + ? await _labelRunner(probe).ConfigureAwait(false) + : await Task.Run(probe, ct).ConfigureAwait(false); + await _gate.WaitAsync(ct).ConfigureAwait(false); try { - // Worktree labels are captured here rather than persisted by - // the caller because this is the moment the row is minted, - // and MergeAndCap replaces any existing row wholesale — so - // rows written by older binaries heal on next launch. - var labels = GitWorktreeLayout.Describe(identity.CanonicalRepoPath); - + // Labels are captured per launch rather than persisted by the + // caller because this is the moment the row is minted, and + // MergeAndCap replaces any existing row wholesale — so rows + // written by older binaries heal on next launch. var fresh = new RecentLaunchContext( identity, leftDisplay, rightDisplay, DateTimeOffset.UtcNow, review, labels.RepositoryName, labels.WorktreeName); From 96ca1097a262d58588a3b206635dcf005c7a1402 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 19:48:21 -0700 Subject: [PATCH 7/7] Coalesce stale stash enumeration in the View stash form Same defect the worktree picker had, in the other view-model that loads on a repo-path change. EnumerateStashesAsync turned away any caller arriving while IsLoading was true, then dropped its own result when the path had moved - so nothing loaded the new checkout and the stash list stayed empty until the user edited the path again. It was latent before: changing the repo path meant typing. The worktree picker makes it a single click, which is what makes it worth fixing here rather than leaving as pre-existing. Now loops on a stale result, mirroring WorktreePickerViewModel.EnsureLoadedAsync. Covered by a test that re-points the form from inside the enumerate runner and asserts both paths were enumerated and the new checkout's stashes landed. Also replaces Task.Yield with a TaskCompletionSource gate in RecordLaunchAsync_YieldsBeforeProbingTheFileSystem. Task.Yield only schedules the continuation, so whether the probe had run by the time the assertion executed was a race; the test failed on its second run. The gate makes it deterministic - the probe provably cannot have run. Found by Copilot code review on PR #24. AI-Local-Session: 80032277-bc2c-4613-a0d3-72ad20d3621b AI-Cloud-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6d4fbf2-3954-4138-a5fb-aa9eaecbff64 --- .../RecentContextsServiceTests.cs | 8 ++- .../ViewModels/ViewStashFormViewModelTests.cs | 56 ++++++++++++++++++- .../ViewModels/ViewStashFormViewModel.cs | 51 ++++++++++------- 3 files changed, 91 insertions(+), 24 deletions(-) diff --git a/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs b/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs index d11e6ac..16503e9 100644 --- a/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs +++ b/DiffViewer.Tests/RecentContexts/RecentContextsServiceTests.cs @@ -160,13 +160,16 @@ public async Task RecordLaunchAsync_YieldsBeforeProbingTheFileSystem() { // Guards the specific trap: `_gate.WaitAsync` completes // synchronously when uncontended, so anything before the first - // real await stays on the caller's thread. + // real await stays on the caller's thread. Gated on a TCS rather + // than Task.Yield so the assertion is deterministic — the probe + // provably cannot have run when we check. var repoPath = CreateSyntheticCheckout("DiffViewer"); + var release = new TaskCompletionSource(); var probed = false; var svc = new RecentContextsService(_path, labelRunner: async probe => { - await Task.Yield(); + await release.Task; probed = true; return probe(); }); @@ -175,6 +178,7 @@ public async Task RecordLaunchAsync_YieldsBeforeProbingTheFileSystem() ContextIdentityFactory.Create(repoPath, Left, Right), Left, Right); probed.Should().BeFalse("the probe must not have run synchronously on the caller"); + release.SetResult(); await pending; probed.Should().BeTrue(); } diff --git a/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs b/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs index fcdbbb8..3296141 100644 --- a/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs +++ b/DiffViewer.Tests/ViewModels/ViewStashFormViewModelTests.cs @@ -114,6 +114,52 @@ public async Task ChangingRepoPath_ClearsSelection() // ---- helpers ------------------------------------------------------- + [Fact] + public async Task SwitchingWorktreeMidEnumeration_StillLoadsTheNewCheckoutsStashes() + { + // The worktree picker rewrites RepoPath in one click. If that + // lands while the first enumeration is in flight, the second + // request is turned away by the IsLoading guard and the first + // drops its result as stale - leaving the new checkout + // permanently unloaded unless the load loops. + var stashes = new[] { MakeStash(0, "WIP on feature-x") }; + var enumerator = new FakeStashEnumerator(stashes); + var validator = new FakeValidator(repoValid: true); + var deps = new FormDependencies( + validator, enumerator, new StubWorktreeEnumerator(), + new NullRecentContextsService(), PrefilledRepoPath: null); + + ViewStashFormViewModel? form = null; + var repointed = false; + form = new ViewStashFormViewModel(deps, enumerateRunner: work => + { + var result = work(); + if (!repointed) + { + repointed = true; + form!.RepoPath = @"C:\worktrees\second"; + } + return Task.FromResult(result); + }); + + // Set the path after construction so the form reference exists + // when the runner re-points it mid-flight. + form.RepoPath = @"C:\repos\first"; + + form.IsLoaded.Should().BeTrue("the loop must re-enumerate the new path"); + form.Stashes.Should().HaveCount(1); + form.HasStashes.Should().BeTrue(); + enumerator.EnumeratedPaths.Should().Equal(@"C:\repos\first", @"C:\worktrees\second"); + } + + private static StashEntry MakeStash(int index, string subject) => new( + Index: index, + SymbolicName: $"stash@{{{index}}}", + Subject: subject, + CreatedAt: DateTimeOffset.UtcNow, + TipSha: new string('a', 40), + TipShortSha: "aaaaaaa"); + private static ViewStashFormViewModel CreateForm( bool repoValid = true, string repoPath = "", @@ -149,12 +195,18 @@ private sealed class FakeStashEnumerator : IGitRefEnumerator public FakeStashEnumerator(IReadOnlyList stashes) => _stashes = stashes; - public RefEnumerationResult Enumerate(string canonicalRepoPath) => - new RefEnumerationResult( + /// Paths this fake was asked about, in call order. + public List EnumeratedPaths { get; } = new(); + + public RefEnumerationResult Enumerate(string canonicalRepoPath) + { + EnumeratedPaths.Add(canonicalRepoPath); + return new RefEnumerationResult( Array.Empty(), Array.Empty(), Array.Empty(), _stashes); + } public string? TryComputeMergeBase(string canonicalRepoPath, string refA, string refB) => null; public string? TryGetDefaultRemoteBranch(string canonicalRepoPath) => null; diff --git a/DiffViewer/ViewModels/ViewStashFormViewModel.cs b/DiffViewer/ViewModels/ViewStashFormViewModel.cs index 72aad8a..713d78d 100644 --- a/DiffViewer/ViewModels/ViewStashFormViewModel.cs +++ b/DiffViewer/ViewModels/ViewStashFormViewModel.cs @@ -98,33 +98,44 @@ internal async Task EnumerateStashesAsync() { if (IsLoading) return; if (string.IsNullOrWhiteSpace(RepoPath)) return; + if (Validator.ValidateRepoPath(RepoPath) is not RepoPathValidation.Valid) return; - var repoResult = Validator.ValidateRepoPath(RepoPath); - if (repoResult is not RepoPathValidation.Valid valid) return; - - var repoPath = valid.CanonicalPath; IsLoading = true; try { - var enumerate = () => _enumerator.Enumerate(repoPath); - var result = _enumerateRunner is not null - ? await _enumerateRunner(enumerate).ConfigureAwait(true) - : await Task.Run(enumerate).ConfigureAwait(true); - - // Drop stale results if the repo path changed mid-flight. - var currentValidation = Validator.ValidateRepoPath(RepoPath); - if (currentValidation is not RepoPathValidation.Valid currentValid - || !string.Equals(currentValid.CanonicalPath, repoPath, StringComparison.Ordinal)) + // Loop rather than drop a stale result. A caller arriving + // during a load is turned away by the IsLoading guard above, + // so discarding the stale result would leave the newly + // selected repo permanently unloaded — nothing else retries. + // The worktree picker makes that a one-click path change, so + // this is readily reachable. Mirrors + // WorktreePickerViewModel.EnsureLoadedAsync. + while (true) { + if (string.IsNullOrWhiteSpace(RepoPath)) return; + if (Validator.ValidateRepoPath(RepoPath) is not RepoPathValidation.Valid valid) return; + + var repoPath = valid.CanonicalPath; + var enumerate = () => _enumerator.Enumerate(repoPath); + var result = _enumerateRunner is not null + ? await _enumerateRunner(enumerate).ConfigureAwait(true) + : await Task.Run(enumerate).ConfigureAwait(true); + + var currentValidation = Validator.ValidateRepoPath(RepoPath); + if (currentValidation is not RepoPathValidation.Valid currentValid + || !string.Equals(currentValid.CanonicalPath, repoPath, StringComparison.Ordinal)) + { + continue; + } + + _stashes = result.Stashes; + IsLoaded = true; + OnPropertyChanged(nameof(Stashes)); + OnPropertyChanged(nameof(HasStashes)); + OnPropertyChanged(nameof(IsEmpty)); + Validate(); return; } - - _stashes = result.Stashes; - IsLoaded = true; - OnPropertyChanged(nameof(Stashes)); - OnPropertyChanged(nameof(HasStashes)); - OnPropertyChanged(nameof(IsEmpty)); - Validate(); } finally {