From 814fa9d8f4034b6c0d691494b70fe7df73793b35 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:09:34 +0000 Subject: [PATCH 1/3] [patch] Wire up Pull and stop advertising unimplemented Commit and Push The repository detail panel drew three enabled buttons that did nothing when clicked. Because they gave no indication of being unfinished, clicking one looked like a silent failure rather than absent functionality. Pull is now implemented. The TODO asked for a check for uncommitted changes before pulling, and GitCli.HasUncommittedChanges already existed to answer it, so PullRepoConfirmingUncommittedChanges pulls straight away on a clean tree and otherwise asks first, offering "Pull Anyway" and "Cancel". This is a warning rather than a safety mechanism, and deliberately so. PullRepo already passes --ff-only, which refuses to advance a divergent branch, but that protects the history and says nothing about the working tree: a pull across uncommitted changes can still fail partway, or succeed and leave the user unsure which changes were theirs. Proceeding stays available because there are legitimate reasons to pull with a dirty tree. Commit and Push are disabled rather than implemented, with a shared "Not implemented yet." tooltip. They stay visible so the layout is unchanged. Both need policy decisions rather than plumbing -- what Commit stages, whether the user sees the file list first, which remote and branch Push targets, and how a rejected push is surfaced. PullRepo's own comment records that unattended git operations swallowing their failures already caused a bad bug in this file, so guessing at those answers is the wrong move. Raised as #392 with the details. Uses ImGui.BeginDisabled/EndDisabled rather than ImGuiWidgets.ScopedDisable, which is not nested under ImGuiWidgets in the referenced package version. The hover states are captured per-button before EndDisabled, since IsItemHovered only ever refers to the item immediately submitted. Fixes #391 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- ProjectDirector/ProjectDirector.cs | 55 +++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 1991353..66f75ce 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -36,6 +36,7 @@ internal sealed class ProjectDirector private ConcurrentQueue LogQueue { get; } = new(); private ImGuiPopups.InputString PopupSetDevDirectory { get; } = new(); private ImGuiPopups.InputString PopupAddNewGitHubOwner { get; } = new(); + private ImGuiPopups.Prompt PopupConfirmPull { get; } = new(); private Collection BrowserContentsBase { get; set; } = []; private Collection BrowserContentsCompare { get; set; } = []; private PopupPropagateFile PopupPropagateFile { get; } = new(); @@ -218,6 +219,38 @@ private void FetchRepo(GitRepository repo) task.Start(); } + /// + /// Pulls , first asking the user to confirm if the working tree has + /// uncommitted changes. + /// + /// The repository to pull. + /// + /// already passes --ff-only, so a divergent + /// branch is refused rather than merged. That protects the history but says nothing about the + /// working tree: a pull across uncommitted changes can still fail partway, or succeed and + /// leave the user unsure which changes were theirs. Asking first is the point of the + /// confirmation -- it is a warning, not a safety mechanism, so pulling anyway stays available. + /// + private void PullRepoConfirmingUncommittedChanges(GitRepository repo) + { + if (!GitCli.HasUncommittedChanges(repo.LocalPath)) + { + PullRepo(repo); + return; + } + + PopupConfirmPull.Open( + "Uncommitted Changes", + $"{repo.LocalPath} has uncommitted changes.\n\nPulling now may fail partway or leave the working tree in a confusing state.\n\nPull anyway?", + new Dictionary + { + ["Pull Anyway"] = () => PullRepo(repo), + ["Cancel"] = null, + }, + ImGuiPopups.PromptTextLayoutType.Wrapped, + new Vector2(420, 0)); + } + private void PullRepo(GitRepository repo) { FullyQualifiedLocalRepoPath repoPath = repo.LocalPath; @@ -265,6 +298,7 @@ private void Tick(float dt) _ = PopupSetDevDirectory.ShowIfOpen(); _ = PopupAddNewGitHubOwner.ShowIfOpen(); + _ = PopupConfirmPull.ShowIfOpen(); FetchAllReposIfStale(); SaveOptionsIfRequired(); @@ -330,14 +364,27 @@ private void ShowTopPanel(float dt) if (ImGui.Button("Pull", new Vector2(FieldWidth, 0))) { - // TODO: check if there are any uncommitted changes and warn the user before - // calling PullRepo(repo), which is otherwise ready to be wired up here. + PullRepoConfirmingUncommittedChanges(repo); } ImGui.SameLine(); - _ = ImGui.Button("Commit", new Vector2(FieldWidth, 0)); // TODO + + // Commit and Push have no implementation behind them. They stay visible so the + // intended layout is not disturbed, but disabled so the UI does not advertise + // a capability that is not there -- an enabled button that silently does + // nothing reads as a bug rather than as unfinished work. See issue #392. + ImGui.BeginDisabled(); + _ = ImGui.Button("Commit", new Vector2(FieldWidth, 0)); + bool commitHovered = ImGui.IsItemHovered(ImGuiHoveredFlags.AllowWhenDisabled); ImGui.SameLine(); - _ = ImGui.Button("Push", new Vector2(FieldWidth, 0)); // TODO + _ = ImGui.Button("Push", new Vector2(FieldWidth, 0)); + bool pushHovered = ImGui.IsItemHovered(ImGuiHoveredFlags.AllowWhenDisabled); + ImGui.EndDisabled(); + + if (commitHovered || pushHovered) + { + ImGui.SetTooltip("Not implemented yet."); + } } }); From 59957549c28716322aa88742f673fad806159c08 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:31:24 +0000 Subject: [PATCH 2/3] Extract and test the pull confirmation decision SonarCloud failed the quality gate on the previous commit with 0% coverage on new code. That commit was almost entirely ImGui drawing, which needs a live context and a display, but it did contain one rule: whether clicking Pull runs immediately or asks first. Extracts that rule into DecidePull, which returns a PullDecision rather than branching inline, and covers it with PullDecisionTests against real throwaway repositories the way GitCliTests already does. Getting it wrong is user-visible in both directions: nagging on a clean tree makes the button annoying, and staying silent on a dirty one is the case the confirmation exists for. Covered: a clean tree pulls without asking; a modified tracked file, an untracked file and a staged-but-uncommitted change each ask first; and committing the change stops the confirmation, so the decision tracks the tree's current state rather than merely that it was ever dirty. Verified by mutation: inverting the decision fails 5 of the 12 tests. This does not make the whole diff reach the 80% new-code coverage threshold -- the remainder is ImGui drawing with no headless harness in this repository to exercise it. Noted on the pull request rather than left silent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- ProjectDirector.Test/PullDecisionTests.cs | 177 ++++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 19 ++- ProjectDirector/PullDecision.cs | 19 +++ 3 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 ProjectDirector.Test/PullDecisionTests.cs create mode 100644 ProjectDirector/PullDecision.cs diff --git a/ProjectDirector.Test/PullDecisionTests.cs b/ProjectDirector.Test/PullDecisionTests.cs new file mode 100644 index 0000000..3322dfa --- /dev/null +++ b/ProjectDirector.Test/PullDecisionTests.cs @@ -0,0 +1,177 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System; +using System.IO; + +using ktsu.Semantics.Strings; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests the rule that decides whether pulling a repository interrupts the user first. +/// +/// +/// The Pull button used to do nothing at all. Now it either pulls or asks, and which one it does +/// is the only part of that path with a rule in it -- everything around it draws ImGui and needs a +/// live context and a display. exists separately so this +/// rule can be driven against real throwaway repositories, the way does. +/// +/// Getting it wrong in either direction is user-visible: nagging on a clean tree makes the button +/// annoying, and staying silent on a dirty one is the case the confirmation exists for. +/// +[TestClass] +public sealed class PullDecisionTests +{ + private static FullyQualifiedLocalRepoPath CreateCommittedRepository() + { + string root = Path.Combine(Path.GetTempPath(), $"ktsu_pd_pull_{Guid.NewGuid():N}"); + _ = Directory.CreateDirectory(root); + + Assert.IsTrue(GitCli.Run("init", root).Succeeded, "git init failed."); + + // Scope identity to this throwaway repository so the test neither depends on nor disturbs + // whatever global configuration the machine happens to carry. + Assert.IsTrue(GitCli.RunIn(root, "config", "user.name", "ProjectDirector").Succeeded); + Assert.IsTrue(GitCli.RunIn(root, "config", "user.email", "ProjectDirector@ktsu.dev").Succeeded); + + File.WriteAllText(Path.Combine(root, "tracked.txt"), "original\n"); + Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed."); + + GitResult committed = GitCli.RunIn(root, "commit", "-m", "initial"); + Assert.IsTrue(committed.Succeeded, $"git commit failed: {committed.FailureText}"); + + return root.As(); + } + + private static void Cleanup(FullyQualifiedLocalRepoPath root) + { + try + { + Directory.Delete(root.WeakString, recursive: true); + } + catch (IOException) + { + // A leaked temp directory is not worth failing an otherwise passing test over. + } + catch (UnauthorizedAccessException) + { + // Same. + } + } + + /// + /// A clean tree must pull without interrupting the user. + /// + [TestMethod] + public void ACleanWorkingTreePullsWithoutAsking() + { + // Arrange + FullyQualifiedLocalRepoPath root = CreateCommittedRepository(); + + try + { + // Act & Assert + Assert.AreEqual(PullDecision.PullNow, ProjectDirector.DecidePull(root)); + } + finally + { + Cleanup(root); + } + } + + /// + /// A modification to a tracked file must trigger the confirmation. + /// + [TestMethod] + public void AModifiedTrackedFileAsksFirst() + { + // Arrange + FullyQualifiedLocalRepoPath root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Combine(root.WeakString, "tracked.txt"), "modified\n"); + + // Act & Assert + Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); + } + finally + { + Cleanup(root); + } + } + + /// + /// An untracked file counts too: a pull can still clobber it, so the user should be asked. + /// + [TestMethod] + public void AnUntrackedFileAsksFirst() + { + // Arrange + FullyQualifiedLocalRepoPath root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Combine(root.WeakString, "untracked.txt"), "new\n"); + + // Act & Assert + Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); + } + finally + { + Cleanup(root); + } + } + + /// + /// Staging a change does not make it committed, so it must still ask. + /// + [TestMethod] + public void AStagedButUncommittedChangeAsksFirst() + { + // Arrange + FullyQualifiedLocalRepoPath root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Combine(root.WeakString, "staged.txt"), "staged\n"); + Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed."); + + // Act & Assert + Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); + } + finally + { + Cleanup(root); + } + } + + /// + /// Committing the change makes the tree clean again, so the confirmation must stop firing -- + /// the decision has to track the tree's current state, not merely that it was ever dirty. + /// + [TestMethod] + public void CommittingTheChangeStopsTheConfirmation() + { + // Arrange + FullyQualifiedLocalRepoPath root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Combine(root.WeakString, "tracked.txt"), "modified\n"); + Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); + + // Act + Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded); + Assert.IsTrue(GitCli.RunIn(root, "commit", "-m", "second").Succeeded); + + // Assert + Assert.AreEqual(PullDecision.PullNow, ProjectDirector.DecidePull(root)); + } + finally + { + Cleanup(root); + } + } +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 66f75ce..f42e038 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -219,6 +219,23 @@ private void FetchRepo(GitRepository repo) task.Start(); } + /// + /// Decides whether pulling a repository should ask the user first. + /// + /// The working tree that would be pulled into. + /// + /// when the working tree has uncommitted changes, + /// otherwise. + /// + /// + /// Separated from so the decision can be + /// tested. Everything around it draws ImGui and needs a live context and a display; this is + /// the part with a rule in it, and ProjectDirectorPullTests drives it against real + /// throwaway repositories. + /// + internal static PullDecision DecidePull(FullyQualifiedLocalRepoPath repoPath) => + GitCli.HasUncommittedChanges(repoPath) ? PullDecision.Confirm : PullDecision.PullNow; + /// /// Pulls , first asking the user to confirm if the working tree has /// uncommitted changes. @@ -233,7 +250,7 @@ private void FetchRepo(GitRepository repo) /// private void PullRepoConfirmingUncommittedChanges(GitRepository repo) { - if (!GitCli.HasUncommittedChanges(repo.LocalPath)) + if (DecidePull(repo.LocalPath) == PullDecision.PullNow) { PullRepo(repo); return; diff --git a/ProjectDirector/PullDecision.cs b/ProjectDirector/PullDecision.cs new file mode 100644 index 0000000..6464f78 --- /dev/null +++ b/ProjectDirector/PullDecision.cs @@ -0,0 +1,19 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector; + +/// +/// Whether a pull can proceed immediately or should ask the user first. +/// +internal enum PullDecision +{ + /// + /// The working tree is clean, so the pull can run without interrupting the user. + /// + PullNow, + + /// + /// The working tree has uncommitted changes, so the user should be asked before pulling. + /// + Confirm, +} From 3b87cf0c2ca0625ad48061e9e0be2069b5c822b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 05:41:15 +0000 Subject: [PATCH 3/3] Use Path.Join in PullDecisionTests github-code-quality flagged all six Path.Combine calls in the new test file: "Call to 'System.IO.Path.Combine' may silently drop its earlier arguments." Path.Combine returns its later argument verbatim when that argument is rooted, discarding everything before it. Every second argument here is a literal file name or a generated directory name, so none of them can be rooted and the current behaviour is correct -- but Path.Join concatenates unconditionally and so cannot exhibit the pattern at all, which is the safer construct to reach for by default. The results are identical for these inputs; Path.GetTempPath's trailing separator is handled by both. Left the pre-existing Path.Combine calls in GitCliTests alone: they are not part of this change, and widening the diff to satisfy a rule the bot did not raise against them belongs in its own commit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- ProjectDirector.Test/PullDecisionTests.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ProjectDirector.Test/PullDecisionTests.cs b/ProjectDirector.Test/PullDecisionTests.cs index 3322dfa..c7b2fea 100644 --- a/ProjectDirector.Test/PullDecisionTests.cs +++ b/ProjectDirector.Test/PullDecisionTests.cs @@ -25,7 +25,7 @@ public sealed class PullDecisionTests { private static FullyQualifiedLocalRepoPath CreateCommittedRepository() { - string root = Path.Combine(Path.GetTempPath(), $"ktsu_pd_pull_{Guid.NewGuid():N}"); + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_pull_{Guid.NewGuid():N}"); _ = Directory.CreateDirectory(root); Assert.IsTrue(GitCli.Run("init", root).Succeeded, "git init failed."); @@ -35,7 +35,7 @@ private static FullyQualifiedLocalRepoPath CreateCommittedRepository() Assert.IsTrue(GitCli.RunIn(root, "config", "user.name", "ProjectDirector").Succeeded); Assert.IsTrue(GitCli.RunIn(root, "config", "user.email", "ProjectDirector@ktsu.dev").Succeeded); - File.WriteAllText(Path.Combine(root, "tracked.txt"), "original\n"); + File.WriteAllText(Path.Join(root, "tracked.txt"), "original\n"); Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed."); GitResult committed = GitCli.RunIn(root, "commit", "-m", "initial"); @@ -91,7 +91,7 @@ public void AModifiedTrackedFileAsksFirst() try { - File.WriteAllText(Path.Combine(root.WeakString, "tracked.txt"), "modified\n"); + File.WriteAllText(Path.Join(root.WeakString, "tracked.txt"), "modified\n"); // Act & Assert Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); @@ -113,7 +113,7 @@ public void AnUntrackedFileAsksFirst() try { - File.WriteAllText(Path.Combine(root.WeakString, "untracked.txt"), "new\n"); + File.WriteAllText(Path.Join(root.WeakString, "untracked.txt"), "new\n"); // Act & Assert Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); @@ -135,7 +135,7 @@ public void AStagedButUncommittedChangeAsksFirst() try { - File.WriteAllText(Path.Combine(root.WeakString, "staged.txt"), "staged\n"); + File.WriteAllText(Path.Join(root.WeakString, "staged.txt"), "staged\n"); Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed."); // Act & Assert @@ -159,7 +159,7 @@ public void CommittingTheChangeStopsTheConfirmation() try { - File.WriteAllText(Path.Combine(root.WeakString, "tracked.txt"), "modified\n"); + File.WriteAllText(Path.Join(root.WeakString, "tracked.txt"), "modified\n"); Assert.AreEqual(PullDecision.Confirm, ProjectDirector.DecidePull(root)); // Act