[patch] Unstage with reset so it works before the first commit

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<RelativeFilePath>());
 
-		IReadOnlyList<string> 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<RelativeFilePath>());
-
-		_ = 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<RelativeFilePath>());
 
 		_ = 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!;

GitIntegration.Test/Integration/GitRoundTripTests.cs

@@ -153,6 +153,52 @@
 		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<RelativeFilePath>()).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<GitCommitMessage>()).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		temporary.WriteFile("f.txt", "two\n");
+		_ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		_ = await repository.Unstage("f.txt".As<RelativeFilePath>()).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()
 	{

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,108 +19,46 @@ public interface IGitRestoreBuilder : IGitCommandBuilder<GitCompleted>
 }
 
 /// <summary>
-/// Builds <c>git restore --staged</c>, with a fallback to <c>git reset HEAD</c> on a git older than
-/// the one that introduced <c>restore</c>.
+/// Builds <c>git reset -q -- &lt;path&gt;</c>, which unstages the path.
 /// </summary>
 /// <remarks>
-/// <c>git restore</c> arrived in git 2.23. Below that, unstaging a path goes through <c>git reset
-/// HEAD -- &lt;path&gt;</c> instead, which every supported git understands. The choice follows the
-/// same shape as <see cref="GitFetchBuilder"/>: a version probe runs in <see cref="ExecuteAsync"/>
-/// and <see cref="TryExecuteAsync"/> before the vector is built, because <c>BuildArguments</c> is
-/// documented as a pure computation with no I/O and so cannot probe for itself.
+/// <c>git restore --staged</c> would read as the modern spelling, but with no <c>--source</c> it
+/// restores the index from <c>HEAD</c>, 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.
+/// <c>git reset -- &lt;path&gt;</c> copies the path's entry from <c>HEAD</c> 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.
 /// </remarks>
 /// <param name="runner">Runs the assembled command.</param>
 /// <param name="repositoryPath">The repository to scope the command to.</param>
 /// <param name="path">The path, relative to the repository root, to unstage.</param>
 internal sealed class GitRestoreBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath repositoryPath, RelativeFilePath path)
 	: GitCommandBuilder<GitCompleted>(runner, repositoryPath), IGitRestoreBuilder
 {
-	/// <summary>The first git release whose <c>restore</c> command exists.</summary>
-	private const int RestoreMajor = 2;
-	private const int RestoreMinor = 23;
-
 	private readonly RelativeFilePath _path = Ensure.NotNull(path);
 
-	/// <summary>
-	/// Gets or sets a value indicating whether the installed git is new enough for <c>restore</c>.
-	/// </summary>
-	/// <remarks>
-	/// Defaults true so <c>BuildArguments</c> emits the modern form until an execution path tells it
-	/// otherwise, matching <see cref="GitFetchBuilder"/>'s own default.
-	/// </remarks>
-	private bool RestoreSupportedByVersion { get; set; } = true;
-
 	/// <summary>
 	/// Appends the verb and the path, separating the two with a bare <c>--</c> rather than through
 	/// <c>AppendOperands</c>.
 	/// </summary>
 	/// <remarks>
-	/// The one builder in this library that does not use <c>AppendOperands</c>, and deliberately so.
-	/// <c>AppendOperands</c> writes <c>--end-of-options</c>, which git gained in 2.24, one release
-	/// after <c>restore</c> itself. This builder exists to serve a git older than 2.23, and on those
-	/// versions <c>--end-of-options</c> 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 <c>--</c> has separated options from pathspecs for git's whole history and is
-	/// what both <c>restore</c> and <c>reset</c> want here, so it gives the same protection against
-	/// a dash-leading path on every version this builder can run against.
+	/// A bare <c>--</c> has separated options from pathspecs for git's whole history, so it gives
+	/// the same protection against a dash-leading path as the <c>--end-of-options</c> that
+	/// <c>AppendOperands</c> writes, without needing git 2.24. <c>-q</c> suppresses the list of
+	/// paths that still have unstaged changes, which <c>reset</c> otherwise prints.
 	/// </remarks>
 	/// <param name="arguments">The vector being assembled.</param>
 	protected override void AppendVerbArguments(ICollection<string> 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);
 	}
 
 	/// <inheritdoc />
 	protected override GitCompleted ParseResult(GitProcessResult result) =>
 		new() { Arguments = Ensure.NotNull(result).Arguments };
-
-	/// <inheritdoc />
-	public override async Task<GitCompleted> ExecuteAsync(CancellationToken cancellationToken = default)
-	{
-		await ProbeVersionAsync(cancellationToken).ConfigureAwait(false);
-
-		return await base.ExecuteAsync(cancellationToken).ConfigureAwait(false);
-	}
-
-	/// <inheritdoc />
-	public override async Task<GitResult<GitCompleted>> TryExecuteAsync(CancellationToken cancellationToken = default)
-	{
-		await ProbeVersionAsync(cancellationToken).ConfigureAwait(false);
-
-		return await base.TryExecuteAsync(cancellationToken).ConfigureAwait(false);
-	}
-
-	/// <summary>
-	/// Asks the installed git what version it is, so the vector can be built to suit.
-	/// </summary>
-	/// <remarks>
-	/// Goes through <see cref="IGitCommandBuilder{TResult}.TryExecuteAsync"/> rather than
-	/// <see cref="IGitCommandBuilder{TResult}.ExecuteAsync"/>, 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 <c>reset</c>, which every supported git
-	/// understands, is the safer default. Mirroring each caller's own strictness would make
-	/// <see cref="ExecuteAsync"/> throw a version exception for what is really a restore problem.
-	/// </remarks>
-	/// <param name="cancellationToken">A token to observe while probing.</param>
-	private async Task ProbeVersionAsync(CancellationToken cancellationToken)
-	{
-		GitResult<GitVersion> probe = await new GitVersionBuilder(Runner)
-			.TryExecuteAsync(cancellationToken).ConfigureAwait(false);
-
-		RestoreSupportedByVersion = probe.Success && probe.Value!.AtLeast(RestoreMajor, RestoreMinor);
-	}
 }