diff --git a/ProjectDirector.Test/PullDecisionTests.cs b/ProjectDirector.Test/PullDecisionTests.cs new file mode 100644 index 0000000..c7b2fea --- /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.Join(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.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"); + 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.Join(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.Join(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.Join(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.Join(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 1991353..f42e038 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,55 @@ 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. + /// + /// 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 (DecidePull(repo.LocalPath) == PullDecision.PullNow) + { + 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 +315,7 @@ private void Tick(float dt) _ = PopupSetDevDirectory.ShowIfOpen(); _ = PopupAddNewGitHubOwner.ShowIfOpen(); + _ = PopupConfirmPull.ShowIfOpen(); FetchAllReposIfStale(); SaveOptionsIfRequired(); @@ -330,14 +381,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."); + } } }); 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, +}