From 46438168941632949131bb8c5824e0d4cf6fbc08 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 12:41:13 +0000 Subject: [PATCH 1/3] [minor] Implement Commit and Push Commit and Push were disabled placeholders. They now work. Commit stages everything, untracked files included, so the prompt lists the paths it is about to sweep up before asking for a message -- that list is the only thing standing between the user and committing something they did not mean to. The list is capped at 20 paths, because a repository mid-rebuild can have thousands and a prompt taller than the display cannot be dismissed. If staging fails the commit is skipped rather than recording a subset of what the user was shown. Push is a plain `git push`, no refspec and no force, with the result piped through QueueGitLog so a rejection appears in the log panel along with git's own explanation instead of being swallowed. GitCli.ListPendingChanges parses `status --porcelain -z` by consuming a rename's trailing source entry rather than by testing each entry for a status prefix. A source path such as "ab cd.txt" has a space in the third position and is indistinguishable from a record by inspection, so a prefix test would list a file that no longer exists. Tests: 11 new cases covering what git reports as pending (clean tree, modified tracked file, untracked file, a path with a space, a rename, a rename whose source looks like a record, a non-repository) and how that is described (singular, short list, exactly the cap, past the cap). Verified load-bearing by substitution: disabling rename consumption fails 1, shifting the summary threshold fails 2, shifting the plural rule fails 1, ignoring the cap fails 1. Fixes #392 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- CLAUDE.md | 3 + ProjectDirector.Test/CommitTests.cs | 332 ++++++++++++++++++++++++++++ ProjectDirector/GitCli.cs | 50 +++++ ProjectDirector/ProjectDirector.cs | 137 +++++++++++- 4 files changed, 510 insertions(+), 12 deletions(-) create mode 100644 ProjectDirector.Test/CommitTests.cs diff --git a/CLAUDE.md b/CLAUDE.md index 05e47a2..25ab530 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,6 +24,8 @@ dotnet publish --configuration Release --output ./staging Tests live in `ProjectDirector.Test` (MSTest, via `MSTest.Sdk` + `ktsu.Sdk`). The app exposes its internals to the test project through `InternalsVisibleTo` in `ProjectDirector/AssemblyInfo.cs`. `GitCliTests` drives `GitCli` against throwaway repositories under the temp directory; the ImGui layer is not unit-tested. +That last point is why the repository actions are shaped the way they are: the part of each with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display. `ProjectDirector.DecidePull` decides whether pulling interrupts the user first (`PullDecisionTests`), and `GitCli.ListPendingChanges` plus `ProjectDirector.DescribePendingChanges` decide what a commit will sweep up and how that is shown (`CommitTests`). Anything genuinely worth testing that is still tangled up with drawing is usually worth extracting the same way. + ```powershell dotnet test --configuration Release ``` @@ -52,6 +54,7 @@ dotnet test --configuration Release - Arguments are passed as a list rather than as a command string, so paths containing spaces need no quoting - `RunIn` uses `git -C `, which never touches the process working directory and so stays safe while repositories are fetched concurrently - Queries answer from git's exit code rather than by searching its output for "fatal" +- `ListPendingChanges` parses `status --porcelain -z` by *consuming* a rename's trailing source entry rather than by testing each entry for a status prefix. A source path such as `ab cd.txt` has a space in the third position and is indistinguishable from a record by inspection, so a prefix test would list a file that no longer exists ### Why the git command line rather than a library diff --git a/ProjectDirector.Test/CommitTests.cs b/ProjectDirector.Test/CommitTests.cs new file mode 100644 index 0000000..3f27cb3 --- /dev/null +++ b/ProjectDirector.Test/CommitTests.cs @@ -0,0 +1,332 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System; +using System.Collections.ObjectModel; +using System.IO; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests the two pieces of the Commit path that can be tested: what git reports as pending, and +/// how that list is described to the user before they agree to commit it. +/// +/// +/// Commit stages everything, untracked files included, so the list shown in the prompt is the only +/// thing standing between the user and committing something they did not mean to. Both halves are +/// separated from the ImGui drawing around them for exactly that reason -- the drawing needs a live +/// context and a display, and these do not. +/// +[TestClass] +public sealed class CommitTests +{ + private static string CreateCommittedRepository() + { + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_commit_{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; + } + + private static void Cleanup(string root) + { + try + { + Directory.Delete(root, recursive: true); + } + catch (IOException) + { + // A leaked temp directory is not worth failing an otherwise passing test over. + } + catch (UnauthorizedAccessException) + { + // Same. + } + } + + /// + /// A clean tree has nothing pending, which is what makes the "nothing to commit" path reachable + /// instead of opening an empty prompt. + /// + [TestMethod] + public void ACleanWorkingTreeHasNoPendingChanges() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(0, changes.Count); + } + finally + { + Cleanup(root); + } + } + + /// + /// A modified tracked file is pending. + /// + [TestMethod] + public void AModifiedTrackedFileIsPending() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Join(root, "tracked.txt"), "modified\n"); + + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(1, changes.Count); + Assert.AreEqual("tracked.txt", changes[0]); + } + finally + { + Cleanup(root); + } + } + + /// + /// An untracked file is pending too, because staging is add --all. This is the case the + /// prompt's file list exists for: it is the only thing that tells the user a file they never + /// added is about to be committed. + /// + [TestMethod] + public void AnUntrackedFileIsPending() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Join(root, "untracked.txt"), "new\n"); + + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(1, changes.Count); + Assert.AreEqual("untracked.txt", changes[0]); + } + finally + { + Cleanup(root); + } + } + + /// + /// A path containing a space survives intact, which is what -z buys: without it git quotes such + /// a path and the quoting would be shown to the user as part of the name. + /// + [TestMethod] + public void APathContainingASpaceIsReportedIntact() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Join(root, "with space.txt"), "new\n"); + + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(1, changes.Count); + Assert.AreEqual("with space.txt", changes[0]); + } + finally + { + Cleanup(root); + } + } + + /// + /// A rename reports only its destination. git emits the source path as a bare following entry + /// with no status prefix, and listing that too would show the user a file that no longer exists. + /// + [TestMethod] + public void ARenameReportsOnlyItsDestination() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + Assert.IsTrue(GitCli.RunIn(root, "mv", "tracked.txt", "renamed.txt").Succeeded, "git mv failed."); + + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(1, changes.Count); + Assert.AreEqual("renamed.txt", changes[0]); + } + finally + { + Cleanup(root); + } + } + + /// + /// A rename whose source path has a space in its third position is the case that a + /// prefix-shaped test cannot tell apart from a real record. Consuming the entry is what gets + /// this right; inspecting it cannot. + /// + [TestMethod] + public void ARenameWhoseSourceLooksLikeARecordIsStillNotListed() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + // "ab cd.txt" has a space at index 2, exactly where a status record's separator sits. + File.WriteAllText(Path.Join(root, "ab cd.txt"), "original\n"); + Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded); + Assert.IsTrue(GitCli.RunIn(root, "commit", "-m", "second").Succeeded); + + Assert.IsTrue(GitCli.RunIn(root, "mv", "ab cd.txt", "renamed.txt").Succeeded, "git mv failed."); + + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(1, changes.Count); + Assert.AreEqual("renamed.txt", changes[0]); + } + finally + { + Cleanup(root); + } + } + + /// + /// A path that is not a repository at all reports nothing rather than throwing, so the button + /// degrades to "nothing to commit" instead of taking the application down. + /// + [TestMethod] + public void APathThatIsNotARepositoryReportsNothing() + { + // Arrange + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_commit_{Guid.NewGuid():N}"); + _ = Directory.CreateDirectory(root); + + try + { + // Act + Collection changes = GitCli.ListPendingChanges(root); + + // Assert + Assert.AreEqual(0, changes.Count); + } + finally + { + Cleanup(root); + } + } + + /// + /// A single change reads as one file, not "1 files". + /// + [TestMethod] + public void OneChangeIsDescribedInTheSingular() + { + // Act + string description = ProjectDirector.DescribePendingChanges(["only.txt"]); + + // Assert + StringAssert.StartsWith(description, "1 file will be committed:", StringComparison.Ordinal); + StringAssert.Contains(description, "only.txt", StringComparison.Ordinal); + } + + /// + /// Every path is listed while the list is short enough to show in full. + /// + [TestMethod] + public void EveryPathIsListedWhileTheListIsShort() + { + // Arrange + Collection changes = ["a.txt", "b.txt", "c.txt"]; + + // Act + string description = ProjectDirector.DescribePendingChanges(changes); + + // Assert + StringAssert.StartsWith(description, "3 files will be committed:", StringComparison.Ordinal); + foreach (string change in changes) + { + StringAssert.Contains(description, change, StringComparison.Ordinal); + } + + Assert.IsFalse(description.Contains("more", StringComparison.Ordinal)); + } + + /// + /// Exactly as many paths as the cap allows are all shown, with no summary line -- the summary + /// must not appear claiming that zero further files exist. + /// + [TestMethod] + public void ExactlyTheCapIsListedInFull() + { + // Arrange + Collection changes = []; + for (int i = 0; i < 20; ++i) + { + changes.Add($"file{i}.txt"); + } + + // Act + string description = ProjectDirector.DescribePendingChanges(changes); + + // Assert + StringAssert.Contains(description, "file19.txt", StringComparison.Ordinal); + Assert.IsFalse(description.Contains("more", StringComparison.Ordinal)); + } + + /// + /// Past the cap the remainder is summarised rather than listed, because a prompt taller than + /// the display cannot be dismissed. + /// + [TestMethod] + public void PastTheCapTheRemainderIsSummarised() + { + // Arrange + Collection changes = []; + for (int i = 0; i < 25; ++i) + { + changes.Add($"file{i}.txt"); + } + + // Act + string description = ProjectDirector.DescribePendingChanges(changes); + + // Assert + StringAssert.StartsWith(description, "25 files will be committed:", StringComparison.Ordinal); + StringAssert.Contains(description, "file19.txt", StringComparison.Ordinal); + Assert.IsFalse(description.Contains("file20.txt", StringComparison.Ordinal)); + StringAssert.Contains(description, "... and 5 more", StringComparison.Ordinal); + } +} diff --git a/ProjectDirector/GitCli.cs b/ProjectDirector/GitCli.cs index f8d4409..9bc86ef 100644 --- a/ProjectDirector/GitCli.cs +++ b/ProjectDirector/GitCli.cs @@ -163,6 +163,56 @@ internal static Collection ListTrackedFiles(string repositoryPath) return files; } + /// + /// Lists the repository-relative paths of every file that git commit would include + /// after git add --all, tracked or otherwise. + /// + /// The working tree to query. + /// The pending paths, or an empty collection when the path is not a repository. + /// + /// Asks the same status --porcelain that does, but + /// with -z so entries are NUL-separated and git applies none of the quoting it otherwise uses + /// for paths holding unusual characters -- the names arrive exactly as recorded. + /// + /// Each record is two status characters, a space, then the path. A rename or copy additionally + /// emits its source path as a bare following entry with no status prefix, which is why + /// the loop consumes that entry rather than testing every entry for a prefix: a source path + /// such as ab cd.txt has a space in the third position and so is indistinguishable from + /// a record by inspection alone. Reporting it would list a file that no longer exists. + /// + internal static Collection ListPendingChanges(string repositoryPath) + { + GitResult result = RunIn(repositoryPath, "status", "--porcelain", "-z"); + + Collection changes = []; + if (!result.Succeeded) + { + return changes; + } + + string[] entries = result.Output.Split('\0'); + for (int i = 0; i < entries.Length; ++i) + { + string entry = entries[i]; + + // A record needs a status pair, its separator, and at least one character of path. + if (entry.Length < 4 || entry[2] != ' ') + { + continue; + } + + changes.Add(entry[3..]); + + // A rename or copy is recorded against the index, in the first status character. + if (entry[0] is 'R' or 'C') + { + ++i; + } + } + + return changes; + } + /// /// Determines whether the working tree has any uncommitted change, tracked or otherwise. /// diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index f42e038..4aa4675 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -8,6 +8,7 @@ namespace ktsu.ProjectDirector; using System.Diagnostics; using System.IO; using System.Numerics; +using System.Text; using DiffPlex; using DiffPlex.Model; using Hexa.NET.ImGui; @@ -37,6 +38,7 @@ internal sealed class ProjectDirector private ImGuiPopups.InputString PopupSetDevDirectory { get; } = new(); private ImGuiPopups.InputString PopupAddNewGitHubOwner { get; } = new(); private ImGuiPopups.Prompt PopupConfirmPull { get; } = new(); + private ImGuiPopups.InputString PopupCommitMessage { get; } = new(); private Collection BrowserContentsBase { get; set; } = []; private Collection BrowserContentsCompare { get; set; } = []; private PopupPropagateFile PopupPropagateFile { get; } = new(); @@ -280,6 +282,121 @@ private void PullRepo(GitRepository repo) task.Start(); } + /// + /// The number of pending paths the commit prompt lists before summarising the rest. + /// + private const int MaxListedPendingChanges = 20; + + /// + /// Builds the text shown above the commit message box: the paths that will be committed. + /// + /// The pending paths, as reports them. + /// A count line followed by one indented path per line, capped. + /// + /// Committing stages everything, untracked files included, so the user has to be able to see + /// what that sweeps up before agreeing to it. The cap exists because a repository mid-rebuild + /// can have thousands of pending paths, and a prompt taller than the display cannot be + /// dismissed. Pure, so ProjectDirectorCommitTests can drive it without an ImGui context. + /// + internal static string DescribePendingChanges(Collection changes) + { + Ensure.NotNull(changes); + + StringBuilder builder = new(); + _ = builder.Append(changes.Count == 1 + ? "1 file will be committed:" + : $"{changes.Count} files will be committed:"); + + int listed = Math.Min(changes.Count, MaxListedPendingChanges); + for (int i = 0; i < listed; ++i) + { + _ = builder.Append("\n ").Append(changes[i]); + } + + int remaining = changes.Count - listed; + if (remaining > 0) + { + _ = builder.Append($"\n ... and {remaining} more"); + } + + return builder.ToString(); + } + + /// + /// Asks for a commit message, showing what will be committed, then commits on confirmation. + /// + /// The repository to commit. + /// + /// The pending paths are gathered before the prompt opens rather than while it is up, so the + /// list the user agreed to is the one they were shown. It is still advisory: the working tree + /// can change while the prompt is open, and git is the authority on what actually gets staged. + /// + private void CommitRepoAfterConfirmation(GitRepository repo) + { + Collection changes = GitCli.ListPendingChanges(repo.LocalPath); + if (changes.Count == 0) + { + QueueLog($"[{DateTimeOffset.Now}] {repo.LocalPath} has nothing to commit"); + return; + } + + PopupCommitMessage.Open( + "Commit Message?", + DescribePendingChanges(changes), + string.Empty, + message => + { + if (!string.IsNullOrWhiteSpace(message)) + { + CommitRepo(repo, message); + } + }); + } + + /// + /// Stages everything and commits it, reporting both steps in the log panel. + /// + /// The repository to commit. + /// The commit message. + /// + /// The commit is skipped when staging fails, because committing after a failed add + /// would record a subset of what the user was shown without saying so. + /// + private void CommitRepo(GitRepository repo, string message) + { + FullyQualifiedLocalRepoPath repoPath = repo.LocalPath; + Task task = new(() => + { + GitResult staged = GitCli.RunIn(repoPath, "add", "--all"); + if (!staged.Succeeded) + { + QueueGitLog($"Staging {repo.LocalPath}", staged); + return; + } + + QueueGitLog($"Committing {repo.LocalPath}", GitCli.RunIn(repoPath, "commit", "-m", message)); + }); + + task.Start(); + } + + /// + /// Pushes the current branch, reporting the result in the log panel. + /// + /// The repository to push. + /// + /// A plain push with no refspec and no force. A rejection is a non-zero exit that + /// surfaces along with git's own explanation, rather than being + /// swallowed. Credentials come from the platform credential helper, as everywhere else here. + /// + private void PushRepo(GitRepository repo) + { + FullyQualifiedLocalRepoPath repoPath = repo.LocalPath; + Task task = new(() => QueueGitLog($"Pushing {repo.RemotePath}", GitCli.RunIn(repoPath, "push"))); + + task.Start(); + } + private void SwitchPage(FullyQualifiedGitHubRepoName baseRepo) { if (Options.Repos.TryGetValue(baseRepo, out GitRepository? repo)) @@ -316,6 +433,7 @@ private void Tick(float dt) _ = PopupSetDevDirectory.ShowIfOpen(); _ = PopupAddNewGitHubOwner.ShowIfOpen(); _ = PopupConfirmPull.ShowIfOpen(); + _ = PopupCommitMessage.ShowIfOpen(); FetchAllReposIfStale(); SaveOptionsIfRequired(); @@ -386,21 +504,16 @@ private void ShowTopPanel(float dt) ImGui.SameLine(); - // 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); + if (ImGui.Button("Commit", new Vector2(FieldWidth, 0))) + { + CommitRepoAfterConfirmation(repo); + } + ImGui.SameLine(); - _ = ImGui.Button("Push", new Vector2(FieldWidth, 0)); - bool pushHovered = ImGui.IsItemHovered(ImGuiHoveredFlags.AllowWhenDisabled); - ImGui.EndDisabled(); - if (commitHovered || pushHovered) + if (ImGui.Button("Push", new Vector2(FieldWidth, 0))) { - ImGui.SetTooltip("Not implemented yet."); + PushRepo(repo); } } }); From ed5fc9ebdf9b42c0bb915e8f2cd5702a8c0939c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 13:08:52 +0000 Subject: [PATCH 2/3] Move the commit and push git work into GitCli so it can be tested SonarCloud flagged 42.3% coverage on new code. The uncovered half was the part that actually matters: whether Commit commits the right things and whether Push sends them. Both were inline in a Task inside an ImGui handler, so nothing could reach them. GitCli.StageAllAndCommit and GitCli.Push now hold that work, and CommitRepo/PushRepo are the thin wrappers that pipe results to the log panel. The sequencing rule moves with them: a null commit result means staging failed and the commit was never attempted, which is what distinguishes that from git refusing a clean tree. Six new tests against real throwaway repositories: a modified file ends up in a commit with the right message and a clean tree after; add --all sweeps up an untracked file; a clean tree fails the commit without moving HEAD; a failed stage yields no commit result at all; pushing lands the commit in a bare repository standing in for a remote (real push negotiation, no network, no credentials); and a branch with no upstream fails carrying git's own explanation. Verified load-bearing by substitution: inverting the stage/commit condition fails 5, weakening add --all to add --update fails 1, and turning Push into fetch fails 2. Suite 23 -> 29. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- CLAUDE.md | 2 + ProjectDirector.Test/CommitTests.cs | 181 ++++++++++++++++++++++++++++ ProjectDirector/GitCli.cs | 44 +++++++ ProjectDirector/ProjectDirector.cs | 10 +- 4 files changed, 232 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 25ab530..465e273 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,6 +54,8 @@ dotnet test --configuration Release - Arguments are passed as a list rather than as a command string, so paths containing spaces need no quoting - `RunIn` uses `git -C `, which never touches the process working directory and so stays safe while repositories are fetched concurrently - Queries answer from git's exit code rather than by searching its output for "fatal" +- `StageAllAndCommit` returns a null commit result when staging failed, so a caller can tell "never attempted" from "git refused". A clean tree is the second kind: git exits non-zero and that refusal is passed through rather than being pre-empted here +- `Push` sends no refspec and no force, so a branch with no upstream and a diverged branch are both refused by git, with its own explanation, rather than guessed at or overwritten - `ListPendingChanges` parses `status --porcelain -z` by *consuming* a rename's trailing source entry rather than by testing each entry for a status prefix. A source path such as `ab cd.txt` has a space in the third position and is indistinguishable from a record by inspection, so a prefix test would list a file that no longer exists ### Why the git command line rather than a library diff --git a/ProjectDirector.Test/CommitTests.cs b/ProjectDirector.Test/CommitTests.cs index 3f27cb3..a1c92ca 100644 --- a/ProjectDirector.Test/CommitTests.cs +++ b/ProjectDirector.Test/CommitTests.cs @@ -248,6 +248,187 @@ public void APathThatIsNotARepositoryReportsNothing() } } + /// + /// The whole point of the button: an edit ends up in a commit. + /// + [TestMethod] + public void StagingAndCommittingRecordsAModifiedFile() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Join(root, "tracked.txt"), "modified\n"); + + // Act + GitCommitOutcome outcome = GitCli.StageAllAndCommit(root, "the message"); + + // Assert + Assert.IsTrue(outcome.Staged.Succeeded, $"git add failed: {outcome.Staged.FailureText}"); + Assert.IsNotNull(outcome.Committed); + Assert.IsTrue(outcome.Committed.Succeeded, $"git commit failed: {outcome.Committed.FailureText}"); + + Assert.IsFalse(GitCli.HasUncommittedChanges(root), "The tree is still dirty after committing."); + Assert.AreEqual("the message", GitCli.RunIn(root, "log", "-1", "--format=%s").OutputText); + } + finally + { + Cleanup(root); + } + } + + /// + /// Staging is add --all, so an untracked file is committed too. This is the behaviour + /// the prompt's file list warns about, and it has to actually be the behaviour. + /// + [TestMethod] + public void StagingAndCommittingSweepsUpAnUntrackedFile() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + File.WriteAllText(Path.Join(root, "untracked.txt"), "new\n"); + + // Act + GitCommitOutcome outcome = GitCli.StageAllAndCommit(root, "sweep"); + + // Assert + Assert.IsNotNull(outcome.Committed); + Assert.IsTrue(outcome.Committed.Succeeded, $"git commit failed: {outcome.Committed.FailureText}"); + + Collection tracked = GitCli.ListTrackedFiles(root); + Assert.IsTrue(tracked.Contains("untracked.txt"), "The untracked file was not committed."); + } + finally + { + Cleanup(root); + } + } + + /// + /// A clean tree is git's refusal to report, not this code's to invent. The commit result comes + /// back non-null and failed so the log panel can show git's own wording, and HEAD must not move. + /// + [TestMethod] + public void CommittingACleanTreeFailsWithoutMovingHead() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + string before = GitCli.RunIn(root, "rev-parse", "HEAD").OutputText; + + // Act + GitCommitOutcome outcome = GitCli.StageAllAndCommit(root, "nothing here"); + + // Assert + Assert.IsTrue(outcome.Staged.Succeeded); + Assert.IsNotNull(outcome.Committed, "A clean tree is a failed commit, not a failed stage."); + Assert.IsFalse(outcome.Committed.Succeeded, "git accepted an empty commit."); + Assert.AreEqual(before, GitCli.RunIn(root, "rev-parse", "HEAD").OutputText, "HEAD moved."); + } + finally + { + Cleanup(root); + } + } + + /// + /// When staging fails there is no commit result at all, because the commit was never attempted. + /// That distinction is what stops a partial index being recorded as if it were the whole change. + /// + [TestMethod] + public void AFailedStageSkipsTheCommitEntirely() + { + // Arrange + string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_commit_{Guid.NewGuid():N}"); + _ = Directory.CreateDirectory(root); + + try + { + // Act + GitCommitOutcome outcome = GitCli.StageAllAndCommit(root, "never runs"); + + // Assert + Assert.IsFalse(outcome.Staged.Succeeded, "git add succeeded outside a repository."); + Assert.IsNull(outcome.Committed, "The commit was attempted after staging failed."); + } + finally + { + Cleanup(root); + } + } + + /// + /// Pushing lands the commit in the remote. A bare repository on the local filesystem stands in + /// for one, so this exercises real push negotiation with no network and no credentials. + /// + [TestMethod] + public void PushingSendsTheCommitToTheRemote() + { + // Arrange + string root = CreateCommittedRepository(); + string remote = Path.Join(Path.GetTempPath(), $"ktsu_pd_remote_{Guid.NewGuid():N}"); + + try + { + Assert.IsTrue(GitCli.Run("init", "--bare", remote).Succeeded, "git init --bare failed."); + Assert.IsTrue(GitCli.RunIn(root, "remote", "add", "origin", remote).Succeeded); + + string branch = GitCli.RunIn(root, "rev-parse", "--abbrev-ref", "HEAD").OutputText; + GitResult upstream = GitCli.RunIn(root, "push", "--set-upstream", "origin", branch); + Assert.IsTrue(upstream.Succeeded, $"establishing the upstream failed: {upstream.FailureText}"); + + File.WriteAllText(Path.Join(root, "tracked.txt"), "modified\n"); + GitCommitOutcome outcome = GitCli.StageAllAndCommit(root, "to push"); + Assert.IsNotNull(outcome.Committed); + Assert.IsTrue(outcome.Committed.Succeeded); + + string local = GitCli.RunIn(root, "rev-parse", "HEAD").OutputText; + + // Act + GitResult pushed = GitCli.Push(root); + + // Assert + Assert.IsTrue(pushed.Succeeded, $"git push failed: {pushed.FailureText}"); + Assert.AreEqual(local, GitCli.RunIn(remote, "rev-parse", branch).OutputText, "The remote did not receive the commit."); + } + finally + { + Cleanup(remote); + Cleanup(root); + } + } + + /// + /// A branch with no upstream is refused by git rather than guessed at here, and the refusal + /// carries git's own explanation for the log panel instead of being swallowed. + /// + [TestMethod] + public void PushingWithNoUpstreamFailsAndSaysWhy() + { + // Arrange + string root = CreateCommittedRepository(); + + try + { + // Act + GitResult pushed = GitCli.Push(root); + + // Assert + Assert.IsFalse(pushed.Succeeded, "git push succeeded with no remote configured."); + Assert.AreNotEqual(0, pushed.FailureText.Length, "git said nothing about why the push failed."); + } + finally + { + Cleanup(root); + } + } + /// /// A single change reads as one file, not "1 files". /// diff --git a/ProjectDirector/GitCli.cs b/ProjectDirector/GitCli.cs index 9bc86ef..7ea1846 100644 --- a/ProjectDirector/GitCli.cs +++ b/ProjectDirector/GitCli.cs @@ -61,6 +61,16 @@ internal Collection AllLines } } +/// +/// The outcome of staging and committing: what each step reported. +/// +/// The result of git add --all. +/// +/// The result of git commit, or when staging failed and the commit +/// was therefore never attempted. +/// +internal sealed record GitCommitOutcome(GitResult Staged, GitResult? Committed); + /// /// Runs the git command line. /// @@ -213,6 +223,40 @@ internal static Collection ListPendingChanges(string repositoryPath) return changes; } + /// + /// Stages every change, tracked or otherwise, then commits them. + /// + /// The working tree to commit. + /// The commit message. + /// What each step reported, with a null commit result when staging failed. + /// + /// The commit is skipped when staging fails, rather than committing whatever happened to make + /// it into the index: recording a subset of what the user agreed to, without saying so, is + /// worse than recording nothing. A clean tree is not a failure of this method -- git itself + /// refuses with a non-zero exit, and that refusal is returned as the commit result so the + /// caller can report git's own wording. + /// + internal static GitCommitOutcome StageAllAndCommit(string repositoryPath, string message) + { + GitResult staged = RunIn(repositoryPath, "add", "--all"); + + return staged.Succeeded + ? new GitCommitOutcome(staged, RunIn(repositoryPath, "commit", "-m", message)) + : new GitCommitOutcome(staged, null); + } + + /// + /// Pushes the current branch to its configured upstream. + /// + /// The working tree to push from. + /// The exit code and captured output. + /// + /// No refspec and no force, so a branch with no upstream is refused by git rather than guessed + /// at here, and a diverged branch is refused rather than overwritten. Both refusals come back + /// as a non-zero exit carrying git's own explanation, which is what the log panel shows. + /// + internal static GitResult Push(string repositoryPath) => RunIn(repositoryPath, "push"); + /// /// Determines whether the working tree has any uncommitted change, tracked or otherwise. /// diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 4aa4675..bd2b882 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -367,14 +367,14 @@ private void CommitRepo(GitRepository repo, string message) FullyQualifiedLocalRepoPath repoPath = repo.LocalPath; Task task = new(() => { - GitResult staged = GitCli.RunIn(repoPath, "add", "--all"); - if (!staged.Succeeded) + GitCommitOutcome outcome = GitCli.StageAllAndCommit(repoPath, message); + if (outcome.Committed is null) { - QueueGitLog($"Staging {repo.LocalPath}", staged); + QueueGitLog($"Staging {repo.LocalPath}", outcome.Staged); return; } - QueueGitLog($"Committing {repo.LocalPath}", GitCli.RunIn(repoPath, "commit", "-m", message)); + QueueGitLog($"Committing {repo.LocalPath}", outcome.Committed); }); task.Start(); @@ -392,7 +392,7 @@ private void CommitRepo(GitRepository repo, string message) private void PushRepo(GitRepository repo) { FullyQualifiedLocalRepoPath repoPath = repo.LocalPath; - Task task = new(() => QueueGitLog($"Pushing {repo.RemotePath}", GitCli.RunIn(repoPath, "push"))); + Task task = new(() => QueueGitLog($"Pushing {repo.RemotePath}", GitCli.Push(repoPath))); task.Start(); } From d67727bf023719a463fe8a2c4813ef4befeedfff Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 13:18:10 +0000 Subject: [PATCH 3/3] Report a failed stage as staging, not as committing Found while reading back the uncovered half of this diff. The Task body chose between "Staging X failed" and "Committing X failed" inline, so nothing could test it, and getting it backwards is not cosmetic: a user whose git add failed would be told the commit failed and go looking at the wrong step. DescribeCommitOutcome now holds that choice and two tests pin it. Substituting the description text fails 1; inverting the null test does not compile at all, because the nullable commit result cannot flow into the non-nullable tuple -- the compiler already forbids the more serious half of this mistake. Suite 29 -> 31. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DTHNXgSNEHUSQ5KMLgivno --- ProjectDirector.Test/CommitTests.cs | 36 +++++++++++++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 31 ++++++++++++++++++------- 2 files changed, 59 insertions(+), 8 deletions(-) diff --git a/ProjectDirector.Test/CommitTests.cs b/ProjectDirector.Test/CommitTests.cs index a1c92ca..1c17242 100644 --- a/ProjectDirector.Test/CommitTests.cs +++ b/ProjectDirector.Test/CommitTests.cs @@ -429,6 +429,42 @@ public void PushingWithNoUpstreamFailsAndSaysWhy() } } + /// + /// A commit that ran is reported as a commit. + /// + [TestMethod] + public void AnAttemptedCommitIsReportedAsACommit() + { + // Arrange + GitResult staged = new(0, string.Empty, string.Empty); + GitResult committed = new(0, "done", string.Empty); + + // Act + (string description, GitResult result) = ProjectDirector.DescribeCommitOutcome("repo", new GitCommitOutcome(staged, committed)); + + // Assert + Assert.AreEqual("Committing repo", description); + Assert.AreSame(committed, result); + } + + /// + /// A commit that never ran is reported as the staging failure it actually was. Labelling it as + /// a commit would send the user looking at the wrong step. + /// + [TestMethod] + public void ACommitThatNeverRanIsReportedAsStaging() + { + // Arrange + GitResult staged = new(128, string.Empty, "not a git repository"); + + // Act + (string description, GitResult result) = ProjectDirector.DescribeCommitOutcome("repo", new GitCommitOutcome(staged, null)); + + // Assert + Assert.AreEqual("Staging repo", description); + Assert.AreSame(staged, result); + } + /// /// A single change reads as one file, not "1 files". /// diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index bd2b882..7985cac 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -367,19 +367,34 @@ private void CommitRepo(GitRepository repo, string message) FullyQualifiedLocalRepoPath repoPath = repo.LocalPath; Task task = new(() => { - GitCommitOutcome outcome = GitCli.StageAllAndCommit(repoPath, message); - if (outcome.Committed is null) - { - QueueGitLog($"Staging {repo.LocalPath}", outcome.Staged); - return; - } - - QueueGitLog($"Committing {repo.LocalPath}", outcome.Committed); + (string description, GitResult result) = DescribeCommitOutcome(repoPath, GitCli.StageAllAndCommit(repoPath, message)); + QueueGitLog(description, result); }); task.Start(); } + /// + /// Chooses which step of a commit the log panel reports, and under which description. + /// + /// How to name the repository in the description. + /// What staging and committing reported. + /// The description to log and the result it describes. + /// + /// A null commit result means staging failed and the commit was never attempted, so the staging + /// failure is what there is to report. Getting this backwards is not cosmetic: a user whose + /// git add failed would be told "Committing ... failed" and go looking at the wrong step. + /// Pure, so it can be tested without an ImGui context. + /// + internal static (string Description, GitResult Result) DescribeCommitOutcome(string repositoryLabel, GitCommitOutcome outcome) + { + Ensure.NotNull(outcome); + + return outcome.Committed is null + ? ($"Staging {repositoryLabel}", outcome.Staged) + : ($"Committing {repositoryLabel}", outcome.Committed); + } + /// /// Pushes the current branch, reporting the result in the log panel. ///