From c280e4c092c64d5ad4fdc2f35b967ea1e177f689 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 14:46:08 +0000 Subject: [PATCH] [patch] Unstage with reset so it works before the first commit Unstage() built `git restore --staged -- ` on git 2.23 and later. With no --source, restore reads the index back from HEAD, so in a repository with no commits yet it exited 128 with "could not resolve HEAD", and ExecuteAsync threw. That is exactly when a user is most likely to have staged too much. GitRestoreBuilder now always builds `git reset -q -- `. It unstages a committed path and leaves a never-committed one untracked, on every supported git, so the version probe and the reset HEAD fallback go away. Fixes ktsu-dev/GitIntegration#122 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PNmrp6FP3tBU47owLsiovc --- .../Builders/GitRestoreBuilderTests.cs | 46 +++------- .../Integration/GitRoundTripTests.cs | 46 ++++++++++ GitIntegration/Builders/GitRestoreBuilder.cs | 90 +++---------------- 3 files changed, 72 insertions(+), 110 deletions(-) diff --git a/GitIntegration.Test/Builders/GitRestoreBuilderTests.cs b/GitIntegration.Test/Builders/GitRestoreBuilderTests.cs index 7e1fa02..0811621 100644 --- a/GitIntegration.Test/Builders/GitRestoreBuilderTests.cs +++ b/GitIntegration.Test/Builders/GitRestoreBuilderTests.cs @@ -3,7 +3,6 @@ namespace ktsu.GitIntegration.Test; using System; -using System.Collections.Generic; using System.Threading.Tasks; using ktsu.Semantics.Paths; @@ -13,16 +12,17 @@ namespace ktsu.GitIntegration.Test; public class GitRestoreBuilderTests { [TestMethod] - public void BuildsTheRestoreVectorOnAModernGit() + public void BuildsTheResetVector() { + // reset rather than restore --staged: restore reads the index back from HEAD and dies with + // "could not resolve HEAD" before the first commit, where reset leaves the file untracked. RecordingGitProcessRunner runner = new(); GitRestoreBuilder builder = new(runner, TestPaths.Root, "f.txt".As()); - IReadOnlyList arguments = builder.BuildArguments(); + string[] arguments = [.. builder.BuildArguments()]; - Assert.IsTrue(arguments.Contains("restore")); - Assert.IsTrue(arguments.Contains("--staged")); - Assert.IsTrue(arguments.Contains("f.txt")); + Assert.AreSequenceEqual(["reset", "-q", "--", "f.txt"], arguments[^4..]); + Assert.IsFalse(arguments.Contains("restore")); } [TestMethod] @@ -33,13 +33,10 @@ public void SeparatesThePathWithABareDoubleDash() string[] arguments = [.. builder.BuildArguments()]; - Assert.AreSequenceEqual( - ["restore", "--staged", "--", "f.txt"], - arguments[^4..], - "--end-of-options arrived in git 2.24, one release after restore, so the very versions this builder's reset fallback serves would read it as a pathspec and fail."); + Assert.AreEqual("--", arguments[^2]); Assert.IsFalse( arguments.Contains("--end-of-options"), - "--end-of-options arrived in git 2.24, one release after restore, so the very versions this builder's reset fallback serves would read it as a pathspec and fail."); + "A bare -- protects a dash-leading path on every git, while --end-of-options needs 2.24."); } [TestMethod] @@ -52,35 +49,18 @@ public void RefusesANullPath() } [TestMethod] - public async Task FallsBackToResetOnAGitOlderThanRestoreAsync() - { - // git restore arrived in 2.23. Below that, unstaging goes through reset HEAD instead, which - // every supported git understands. - ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() - .Then(standardOutput: "git version 2.22.0\n") - .Then(standardOutput: string.Empty); - GitRestoreBuilder builder = new(runner, TestPaths.Root, "f.txt".As()); - - _ = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - - string[] arguments = [.. runner.Invocations[1]]; - Assert.AreSequenceEqual(["reset", "HEAD", "--", "f.txt"], arguments[^4..]); - } - - [TestMethod] - public async Task TreatsExactlyTwoTwentyThreeAsSupportedAsync() + public async Task RunsOneCommandWithNoVersionProbeAsync() { - // The documented floor, asserted exactly: an off-by-one here silently falls back to reset - // for every user on the first version that supports restore. + // reset behaves the same on every supported git, so nothing needs to ask which one is + // installed first. ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() - .Then(standardOutput: "git version 2.23.0\n") .Then(standardOutput: string.Empty); GitRestoreBuilder builder = new(runner, TestPaths.Root, "f.txt".As()); _ = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - string[] arguments = [.. runner.Invocations[1]]; - Assert.AreSequenceEqual(["restore", "--staged", "--", "f.txt"], arguments[^4..]); + string[] invocation = [.. Assert.ContainsSingle(runner.Invocations)]; + Assert.AreSequenceEqual(["reset", "-q", "--", "f.txt"], invocation[^4..]); } public TestContext TestContext { get; set; } = null!; diff --git a/GitIntegration.Test/Integration/GitRoundTripTests.cs b/GitIntegration.Test/Integration/GitRoundTripTests.cs index 0c60fd3..5492edd 100644 --- a/GitIntegration.Test/Integration/GitRoundTripTests.cs +++ b/GitIntegration.Test/Integration/GitRoundTripTests.cs @@ -153,6 +153,52 @@ public async Task StatusReflectsStagedAndUntrackedWorkAsync() Assert.IsFalse(dirty.IsClean); } + [TestMethod] + public async Task UnstageBeforeTheFirstCommitLeavesTheFileUntrackedAsync() + { + // A freshly initialised repository has no HEAD to restore the index from, which is exactly + // when a user is most likely to have staged too much. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await InitializeAsync(temporary, cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("f.txt", "x\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + _ = await repository.Unstage("f.txt".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + GitStatus status = await repository.Status().ExecuteAsync(cancellationToken).ConfigureAwait(false); + GitStatusEntry entry = status.Entries.Single(); + Assert.AreEqual(GitFileState.Untracked, entry.IndexState); + Assert.AreEqual(GitFileState.Untracked, entry.WorkTreeState); + } + + [TestMethod] + public async Task UnstageAfterACommitRestoresTheIndexAndKeepsTheWorkingTreeAsync() + { + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await InitializeAsync(temporary, cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("f.txt", "one\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c1".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("f.txt", "two\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + _ = await repository.Unstage("f.txt".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + GitStatus status = await repository.Status().ExecuteAsync(cancellationToken).ConfigureAwait(false); + GitStatusEntry entry = status.Entries.Single(); + Assert.AreEqual(GitFileState.Unmodified, entry.IndexState, "The staged change should be gone from the index."); + Assert.AreEqual(GitFileState.Modified, entry.WorkTreeState, "The change itself should still be in the working tree."); + } + [TestMethod] public async Task StatusReportsUntrackedWorkEvenWhereTheHostHidesItAsync() { diff --git a/GitIntegration/Builders/GitRestoreBuilder.cs b/GitIntegration/Builders/GitRestoreBuilder.cs index 90218ce..6b0d967 100644 --- a/GitIntegration/Builders/GitRestoreBuilder.cs +++ b/GitIntegration/Builders/GitRestoreBuilder.cs @@ -3,8 +3,6 @@ namespace ktsu.GitIntegration; using System.Collections.Generic; -using System.Threading; -using System.Threading.Tasks; using ktsu.Semantics.Paths; @@ -21,15 +19,15 @@ public interface IGitRestoreBuilder : IGitCommandBuilder } /// -/// Builds git restore --staged, with a fallback to git reset HEAD on a git older than -/// the one that introduced restore. +/// Builds git reset -q -- <path>, which unstages the path. /// /// -/// git restore arrived in git 2.23. Below that, unstaging a path goes through git reset -/// HEAD -- <path> instead, which every supported git understands. The choice follows the -/// same shape as : a version probe runs in -/// and before the vector is built, because BuildArguments is -/// documented as a pure computation with no I/O and so cannot probe for itself. +/// git restore --staged would read as the modern spelling, but with no --source it +/// restores the index from HEAD, and in a repository with no commits yet it dies with "could +/// not resolve HEAD". That is exactly when a user is most likely to have staged too much. +/// git reset -- <path> copies the path's entry from HEAD into the index, and +/// on an unborn branch it removes the entry, leaving the file untracked. It behaves that way on +/// every supported git, so there is no version to probe for. /// /// Runs the assembled command. /// The repository to scope the command to. @@ -37,51 +35,25 @@ public interface IGitRestoreBuilder : IGitCommandBuilder internal sealed class GitRestoreBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath repositoryPath, RelativeFilePath path) : GitCommandBuilder(runner, repositoryPath), IGitRestoreBuilder { - /// The first git release whose restore command exists. - private const int RestoreMajor = 2; - private const int RestoreMinor = 23; - private readonly RelativeFilePath _path = Ensure.NotNull(path); - /// - /// Gets or sets a value indicating whether the installed git is new enough for restore. - /// - /// - /// Defaults true so BuildArguments emits the modern form until an execution path tells it - /// otherwise, matching 's own default. - /// - private bool RestoreSupportedByVersion { get; set; } = true; - /// /// Appends the verb and the path, separating the two with a bare -- rather than through /// AppendOperands. /// /// - /// The one builder in this library that does not use AppendOperands, and deliberately so. - /// AppendOperands writes --end-of-options, which git gained in 2.24, one release - /// after restore itself. This builder exists to serve a git older than 2.23, and on those - /// versions --end-of-options is not an option at all: git reads it as a pathspec and the - /// command fails, which would make the fallback unreachable and break the restore path on 2.23 - /// exactly. A bare -- has separated options from pathspecs for git's whole history and is - /// what both restore and reset want here, so it gives the same protection against - /// a dash-leading path on every version this builder can run against. + /// A bare -- has separated options from pathspecs for git's whole history, so it gives + /// the same protection against a dash-leading path as the --end-of-options that + /// AppendOperands writes, without needing git 2.24. -q suppresses the list of + /// paths that still have unstaged changes, which reset otherwise prints. /// /// The vector being assembled. protected override void AppendVerbArguments(ICollection arguments) { Ensure.NotNull(arguments); - if (RestoreSupportedByVersion) - { - arguments.Add("restore"); - arguments.Add("--staged"); - } - else - { - arguments.Add("reset"); - arguments.Add("HEAD"); - } - + arguments.Add("reset"); + arguments.Add("-q"); arguments.Add("--"); arguments.Add(_path.WeakString); } @@ -89,40 +61,4 @@ protected override void AppendVerbArguments(ICollection arguments) /// protected override GitCompleted ParseResult(GitProcessResult result) => new() { Arguments = Ensure.NotNull(result).Arguments }; - - /// - public override async Task ExecuteAsync(CancellationToken cancellationToken = default) - { - await ProbeVersionAsync(cancellationToken).ConfigureAwait(false); - - return await base.ExecuteAsync(cancellationToken).ConfigureAwait(false); - } - - /// - public override async Task> TryExecuteAsync(CancellationToken cancellationToken = default) - { - await ProbeVersionAsync(cancellationToken).ConfigureAwait(false); - - return await base.TryExecuteAsync(cancellationToken).ConfigureAwait(false); - } - - /// - /// Asks the installed git what version it is, so the vector can be built to suit. - /// - /// - /// Goes through rather than - /// , and the same way regardless of which - /// of this builder's own two entry points is running: a failed probe means the version genuinely - /// could not be established, and falling back to reset, which every supported git - /// understands, is the safer default. Mirroring each caller's own strictness would make - /// throw a version exception for what is really a restore problem. - /// - /// A token to observe while probing. - private async Task ProbeVersionAsync(CancellationToken cancellationToken) - { - GitResult probe = await new GitVersionBuilder(Runner) - .TryExecuteAsync(cancellationToken).ConfigureAwait(false); - - RestoreSupportedByVersion = probe.Success && probe.Value!.AtLeast(RestoreMajor, RestoreMinor); - } }