[patch] Wire up Pull and stop advertising unimplemented Commit and Push

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;
+
+/// <summary>
+/// Tests the rule that decides whether pulling a repository interrupts the user first.
+/// </summary>
+/// <remarks>
+/// 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. <see cref="ProjectDirector.DecidePull"/> exists separately so this
+/// rule can be driven against real throwaway repositories, the way <see cref="GitCliTests"/> 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.
+/// </remarks>
+[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<FullyQualifiedLocalRepoPath>();
+	}
+
+	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.
+		}
+	}
+
+	/// <summary>
+	/// A clean tree must pull without interrupting the user.
+	/// </summary>
+	[TestMethod]
+	public void ACleanWorkingTreePullsWithoutAsking()
+	{
+		// Arrange
+		FullyQualifiedLocalRepoPath root = CreateCommittedRepository();
+
+		try
+		{
+			// Act & Assert
+			Assert.AreEqual(PullDecision.PullNow, ProjectDirector.DecidePull(root));
+		}
+		finally
+		{
+			Cleanup(root);
+		}
+	}
+
+	/// <summary>
+	/// A modification to a tracked file must trigger the confirmation.
+	/// </summary>
+	[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);
+		}
+	}
+
+	/// <summary>
+	/// An untracked file counts too: a pull can still clobber it, so the user should be asked.
+	/// </summary>
+	[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);
+		}
+	}
+
+	/// <summary>
+	/// Staging a change does not make it committed, so it must still ask.
+	/// </summary>
+	[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);
+		}
+	}
+
+	/// <summary>
+	/// 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.
+	/// </summary>
+	[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);
+		}
+	}
+}

ProjectDirector/ProjectDirector.cs

@@ -17,7 +17,7 @@
 using ktsu.ImGui.Widgets;
 using ktsu.ImGui.Styler;
 using Octokit;
 // using OpenAI.Chat;
 using Semantics.Paths;
 
 #pragma warning disable CA1506
@@ -36,11 +36,12 @@
 	private ConcurrentQueue<string> LogQueue { get; } = new();
 	private ImGuiPopups.InputString PopupSetDevDirectory { get; } = new();
 	private ImGuiPopups.InputString PopupAddNewGitHubOwner { get; } = new();
+	private ImGuiPopups.Prompt PopupConfirmPull { get; } = new();
 	private Collection<RelativePath> BrowserContentsBase { get; set; } = [];
 	private Collection<RelativePath> BrowserContentsCompare { get; set; } = [];
 	private PopupPropagateFile PopupPropagateFile { get; } = new();
 
 	// private ChatClient ChatClient { get; init; }
 
 	private static void Main(string[] _)
 	{
@@ -60,7 +61,7 @@
 	{
 		Options = ProjectDirectorOptions.LoadOrCreate();
 		Options.Save();
 		// ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken));
 		DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerCols = new("VerticalDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerRows = new("HorizontalDivider", DividerResized, ImGuiWidgets.DividerLayout.Rows);
@@ -218,6 +219,55 @@
 		task.Start();
 	}
 
+	/// <summary>
+	/// Decides whether pulling a repository should ask the user first.
+	/// </summary>
+	/// <param name="repoPath">The working tree that would be pulled into.</param>
+	/// <returns>
+	/// <see cref="PullDecision.Confirm"/> when the working tree has uncommitted changes,
+	/// <see cref="PullDecision.PullNow"/> otherwise.
+	/// </returns>
+	/// <remarks>
+	/// Separated from <see cref="PullRepoConfirmingUncommittedChanges"/> 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 <c>ProjectDirectorPullTests</c> drives it against real
+	/// throwaway repositories.
+	/// </remarks>
+	internal static PullDecision DecidePull(FullyQualifiedLocalRepoPath repoPath) =>
+		GitCli.HasUncommittedChanges(repoPath) ? PullDecision.Confirm : PullDecision.PullNow;
+
+	/// <summary>
+	/// Pulls <paramref name="repo"/>, first asking the user to confirm if the working tree has
+	/// uncommitted changes.
+	/// </summary>
+	/// <param name="repo">The repository to pull.</param>
+	/// <remarks>
+	/// <see cref="PullRepo(GitRepository)"/> already passes <c>--ff-only</c>, 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.
+	/// </remarks>
+	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<string, Action?>
+			{
+				["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 @@
 
 		_ = PopupSetDevDirectory.ShowIfOpen();
 		_ = PopupAddNewGitHubOwner.ShowIfOpen();
+		_ = PopupConfirmPull.ShowIfOpen();
 
 		FetchAllReposIfStale();
 		SaveOptionsIfRequired();
@@ -317,7 +368,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -330,14 +381,27 @@
 
 					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.");
+					}
 				}
 			});
 
@@ -1422,7 +1486,7 @@
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;
 					//}
@@ -1431,7 +1495,7 @@
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"X"))
 					//{
 					//	//Directory.Delete(Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path));
 					//}
 

ProjectDirector/PullDecision.cs

@@ -0,0 +1,19 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector;
+
+/// <summary>
+/// Whether a pull can proceed immediately or should ask the user first.
+/// </summary>
+internal enum PullDecision
+{
+	/// <summary>
+	/// The working tree is clean, so the pull can run without interrupting the user.
+	/// </summary>
+	PullNow,
+
+	/// <summary>
+	/// The working tree has uncommitted changes, so the user should be asked before pulling.
+	/// </summary>
+	Confirm,
+}