Close the open issue backlog (17 issues)

GitIntegration.Test/Builders/GitCheckoutBuilderTests.cs

@@ -25,8 +25,8 @@ public void BuildsTheDefaultCheckoutVector()
 			"-c", "core.quotepath=false",
 			"-c", "color.ui=false",
 			"checkout",
-			"--end-of-options",
 			"main",
+			"--",
 		];
 		CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray());
 	}
@@ -50,30 +50,80 @@ public void MapsTheOptionFlags()
 	}
 
 	[TestMethod]
-	public void KeepsFlagsBeforeTheEndOfOptionsMarker()
+	public void KeepsFlagsBeforeTheTargetAndTerminatesWithADoubleDash()
 	{
-		// Anything after --end-of-options is an operand, so a flag emitted there would be handed to
-		// git as a ref name.
+		// The target must be the last thing before the "--" terminator, with every flag ahead of it:
+		// a flag emitted after the target would be handed to git as a pathspec.
 		RecordingGitProcessRunner runner = new();
 		GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main);
 
 		_ = builder.CreatingBranch().Force();
 
 		string[] arguments = [.. builder.BuildArguments()];
-		int marker = Array.IndexOf(arguments, "--end-of-options");
+		int target = Array.IndexOf(arguments, "main");
 
-		Assert.IsTrue(Array.IndexOf(arguments, "-b") < marker);
-		Assert.IsTrue(Array.IndexOf(arguments, "--force") < marker);
-		Assert.AreEqual("main", arguments[marker + 1]);
+		Assert.IsTrue(Array.IndexOf(arguments, "-b") < target);
+		Assert.IsTrue(Array.IndexOf(arguments, "--force") < target);
+		Assert.AreEqual("--", arguments[target + 1]);
+		Assert.AreEqual(arguments.Length - 1, target + 1);
+	}
+
+	[TestMethod]
+	public void DoesNotEmitTheEndOfOptionsMarker()
+	{
+		// git <= 2.43 leaves --end-of-options in checkout's own operand list, because checkout sets
+		// PARSE_OPT_KEEP_DASHDASH and that release only stripped the marker when the flag was unset.
+		// The marker then reaches git as a pathspec: "error: pathspec '--end-of-options' did not
+		// match any file(s) known to git". git 2.44 changed the condition, but emitting the marker
+		// would make Checkout unusable on every git before it — including Ubuntu 24.04 LTS's stock
+		// 2.43. GitRefName's NotAnOptionAttribute is what keeps a dash-leading target out of the
+		// vector instead.
+		RecordingGitProcessRunner runner = new();
+		GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main);
+
+		CollectionAssert.DoesNotContain(builder.BuildArguments().ToArray(), "--end-of-options");
 	}
 
 	[TestMethod]
 	public void ConfigurationMethodsReturnTheSameBuilderForChaining()
 	{
+		// Deliberately not chaining CreatingBranch and Detach together: that combination is one git
+		// refuses, and BuildArguments rejects it. Chaining it here to check a fluent return value
+		// would read as an endorsement of a vector that can never run.
 		RecordingGitProcessRunner runner = new();
 		GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main);
 
-		Assert.AreSame(builder, builder.CreatingBranch().Force().Detach());
+		Assert.AreSame(builder, builder.CreatingBranch().Force());
+		Assert.AreSame(builder, builder.Detach());
+	}
+
+	[TestMethod]
+	public void RejectsAskingForBothCreatingBranchAndDetach()
+	{
+		// Real git refuses "-b <name> --detach <target>" with
+		// "fatal: '--detach' cannot be used with '-b/-B/--orphan'" (verified against git 2.43), so
+		// without this guard the contradiction surfaces only as an opaque GitCommandException from a
+		// process that was already spawned. Fetch and pull reject their own equivalent
+		// contradictions before spawning, and checkout should be no less consistent.
+		RecordingGitProcessRunner runner = new();
+		GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main);
+
+		_ = builder.CreatingBranch().Detach();
+
+		Assert.ThrowsExactly<InvalidOperationException>(() => _ = builder.BuildArguments());
+	}
+
+	[TestMethod]
+	public void RejectsTheContradictionRegardlessOfTheOrderItWasConfiguredIn()
+	{
+		// A caller may set either first, so only the finished configuration can detect the
+		// contradiction — the reason the guard lives in BuildArguments rather than in each setter.
+		RecordingGitProcessRunner runner = new();
+		GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main);
+
+		_ = builder.Detach().CreatingBranch();
+
+		Assert.ThrowsExactly<InvalidOperationException>(() => _ = builder.BuildArguments());
 	}
 
 	[TestMethod]

GitIntegration.Test/Builders/GitDiffBuilderTests.cs

@@ -4,6 +4,7 @@ namespace ktsu.GitIntegration.Test;
 
 using System;
 using System.Collections.Generic;
+using System.Threading.Tasks;
 
 using ktsu.Semantics.Paths;
 using ktsu.Semantics.Strings;
@@ -120,4 +121,81 @@ public void RejectsNullArguments()
 		Assert.ThrowsExactly<ArgumentNullException>(() => _ = builder.Between("main".As<GitRefName>(), null!));
 		Assert.ThrowsExactly<ArgumentNullException>(() => _ = builder.ForPath(null!));
 	}
+
+	[TestMethod]
+	public void SwitchesToRawAndNumstatWhenLineCountsAreAskedFor()
+	{
+		// --name-status and --numstat are both display formats and git lets the last one win, so
+		// asking for both would silently produce only one section. --raw is the form that combines.
+		RecordingGitProcessRunner runner = new();
+		GitDiffBuilder builder = new(runner, TestPaths.Root);
+
+		_ = builder.WithLineCounts();
+
+		string[] arguments = [.. builder.BuildArguments()];
+
+		CollectionAssert.Contains(arguments, "--raw");
+		CollectionAssert.Contains(arguments, "--numstat");
+		CollectionAssert.DoesNotContain(arguments, "--name-status");
+		CollectionAssert.Contains(arguments, "-z");
+	}
+
+	[TestMethod]
+	public void LeavesTheDefaultVectorUnchangedWhenLineCountsAreNotAskedFor()
+	{
+		// The option is opt-in precisely so the command and its output volume stay as they were for
+		// callers that only want the path list.
+		RecordingGitProcessRunner runner = new();
+		GitDiffBuilder builder = new(runner, TestPaths.Root);
+
+		string[] arguments = [.. builder.BuildArguments()];
+
+		CollectionAssert.Contains(arguments, "--name-status");
+		CollectionAssert.DoesNotContain(arguments, "--raw");
+		CollectionAssert.DoesNotContain(arguments, "--numstat");
+	}
+
+	[TestMethod]
+	public async Task ReportsNoLineCountsUnlessTheyWereAskedForAsync()
+	{
+		// The default parser reads --name-status output, which carries no counts at all, so both
+		// fields stay null rather than defaulting to zero.
+		RecordingGitProcessRunner runner = new() { StandardOutput = "M\0a.txt\0" };
+		GitDiffBuilder builder = new(runner, TestPaths.Root);
+
+		IReadOnlyList<GitDiffEntry> entries =
+			await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false);
+
+		Assert.IsNull(entries[0].Insertions);
+		Assert.IsNull(entries[0].Deletions);
+	}
+
+	[TestMethod]
+	public async Task ReportsLineCountsWhenTheyWereAskedForAsync()
+	{
+		RecordingGitProcessRunner runner = new()
+		{
+			StandardOutput = ":100644 100644 366fd40 fbeb5f4 M\0a.txt\0" + "3\t2\ta.txt\0",
+		};
+		GitDiffBuilder builder = new(runner, TestPaths.Root);
+
+		_ = builder.WithLineCounts();
+
+		IReadOnlyList<GitDiffEntry> entries =
+			await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false);
+
+		Assert.AreEqual(3, entries[0].Insertions);
+		Assert.AreEqual(2, entries[0].Deletions);
+	}
+
+	[TestMethod]
+	public void WithLineCountsReturnsTheSameBuilderForChaining()
+	{
+		RecordingGitProcessRunner runner = new();
+		GitDiffBuilder builder = new(runner, TestPaths.Root);
+
+		Assert.AreSame(builder, builder.WithLineCounts());
+	}
+
+	public TestContext TestContext { get; set; } = null!;
 }

GitIntegration.Test/Builders/GitLogBuilderTests.cs

@@ -133,4 +133,93 @@ public void RejectsANullRevisionOrPath()
 		Assert.ThrowsExactly<ArgumentNullException>(() => _ = builder.ForRevision(null!));
 		Assert.ThrowsExactly<ArgumentNullException>(() => _ = builder.ForPath(null!));
 	}
+
+	[TestMethod]
+	public void EmitsAllRefsBeforeTheNegation()
+	{
+		// Order is the whole correctness question here. git negates everything after a --not, so
+		// "--not --remotes --all" excludes every reference instead of including them — and reports an
+		// empty log rather than failing, so nothing else would catch the mistake. Verified against
+		// git 2.43.
+		RecordingGitProcessRunner runner = new();
+		GitLogBuilder builder = new(runner, TestPaths.Root);
+
+		_ = builder.IncludingAllRefs().ExcludingRemoteTrackingRefs();
+
+		string[] arguments = [.. builder.BuildArguments()];
+		int all = Array.IndexOf(arguments, "--all");
+		int not = Array.IndexOf(arguments, "--not");
+
+		Assert.AreNotEqual(-1, all);
+		Assert.AreNotEqual(-1, not);
+		Assert.IsTrue(all < not, "--all must precede --not or the query means the opposite.");
+	}
+
+	[TestMethod]
+	public void EmitsTheNegationAsAClosedTriple()
+	{
+		// --not reverses every revision specifier that follows it until the next --not, so the pair is
+		// emitted with a closing --not that scopes the negation to --remotes alone. Without it a
+		// revision or pathspec emitted below would be excluded rather than selected.
+		RecordingGitProcessRunner runner = new();
+		GitLogBuilder builder = new(runner, TestPaths.Root);
+
+		_ = builder.ExcludingRemoteTrackingRefs();
+
+		string[] arguments = [.. builder.BuildArguments()];
+		int not = Array.IndexOf(arguments, "--not");
+
+		Assert.AreEqual("--remotes", arguments[not + 1]);
+		Assert.AreEqual("--not", arguments[not + 2]);
+	}
+
+	[TestMethod]
+	public void KeepsARevisionOutsideTheNegation()
+	{
+		// The failure the closing --not prevents: "git log --not --remotes <revision>" asks for
+		// commits in neither, which is a different question that quietly returns nothing. The revision
+		// has to land after the negation has been closed.
+		RecordingGitProcessRunner runner = new();
+		GitLogBuilder builder = new(runner, TestPaths.Root);
+
+		_ = builder.ExcludingRemoteTrackingRefs().ForRevision("HEAD".As<GitRefName>());
+
+		string[] arguments = [.. builder.BuildArguments()];
+		int revision = Array.IndexOf(arguments, "HEAD");
+		int closingNot = Array.LastIndexOf(arguments, "--not");
+
+		Assert.IsTrue(closingNot < revision, "The negation must be closed before the revision.");
+	}
+
+	[TestMethod]
+	public void KeepsTheNegationBeforeEveryNonOptionArgument()
+	{
+		// git refuses --not once a non-option argument has appeared: "git log --end-of-options HEAD
+		// --not --remotes" dies with "fatal: option '--not' must come before non-option arguments".
+		// That is why the negation cannot simply be deferred to the end of the vector instead.
+		RecordingGitProcessRunner runner = new();
+		GitLogBuilder builder = new(runner, TestPaths.Root);
+
+		_ = builder.IncludingAllRefs().ExcludingRemoteTrackingRefs().ForRevision("HEAD".As<GitRefName>());
+
+		string[] arguments = [.. builder.BuildArguments()];
+		int marker = Array.IndexOf(arguments, "--end-of-options");
+
+		Assert.IsTrue(Array.LastIndexOf(arguments, "--not") < marker);
+		Assert.IsTrue(Array.IndexOf(arguments, "--remotes") < marker);
+		Assert.IsTrue(Array.IndexOf(arguments, "--all") < marker);
+	}
+
+	[TestMethod]
+	public void OmitsBothFlagsWhenNeitherWasRequested()
+	{
+		RecordingGitProcessRunner runner = new();
+		GitLogBuilder builder = new(runner, TestPaths.Root);
+
+		string[] arguments = [.. builder.BuildArguments()];
+
+		CollectionAssert.DoesNotContain(arguments, "--all");
+		CollectionAssert.DoesNotContain(arguments, "--not");
+		CollectionAssert.DoesNotContain(arguments, "--remotes");
+	}
 }

GitIntegration.Test/Builders/GitPullBuilderTests.cs

@@ -291,6 +291,65 @@ public async Task TryExecuteReportsStandardErrorAloneWithNoTrailingNewlineAsync(
 			result.Error?.StandardError);
 	}
 
+	[TestMethod]
+	public async Task ExecuteReportsANonConflictFailureExplainedOnStandardOutputAsync()
+	{
+		// The two entry points must describe the identical failure identically. This one is not a
+		// conflict, so CreateException hands it to the base implementation — which used to build its
+		// message from standard error alone and produced "git exited with code 1: " with nothing
+		// after the colon, while TryExecuteAsync returned the real explanation.
+		const string Explanation = "You have divergent branches and need to specify how to reconcile them.\n";
+		RecordingGitProcessRunner runner = new()
+		{
+			ExitCode = 128,
+			StandardOutput = Explanation,
+			StandardError = string.Empty,
+		};
+		GitPullBuilder builder = new(runner, TestPaths.Root);
+
+		GitCommandException exception = await Assert.ThrowsExactlyAsync<GitCommandException>(
+			async () => await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false))
+			.ConfigureAwait(false);
+
+		StringAssert.Contains(exception.Message, "divergent branches");
+		StringAssert.Contains(exception.StandardError, "divergent branches");
+	}
+
+	[TestMethod]
+	public async Task ExecuteAndTryExecuteReportTheSameDiagnosticForTheSameFailureAsync()
+	{
+		// The point of routing both entry points through one seam: they cannot drift. Asserting the
+		// two texts against each other pins that directly, rather than pinning each against a
+		// literal that a future change could update in one place only.
+		const string FetchProgress = "From /srv/origin\n * branch main -> FETCH_HEAD\n";
+		const string Explanation = "error: Your local changes would be overwritten by merge.\n";
+
+		RecordingGitProcessRunner throwingRunner = new()
+		{
+			ExitCode = 1,
+			StandardOutput = Explanation,
+			StandardError = FetchProgress,
+		};
+		GitPullBuilder throwing = new(throwingRunner, TestPaths.Root);
+
+		GitCommandException exception = await Assert.ThrowsExactlyAsync<GitCommandException>(
+			async () => await throwing.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false))
+			.ConfigureAwait(false);
+
+		RecordingGitProcessRunner resultRunner = new()
+		{
+			ExitCode = 1,
+			StandardOutput = Explanation,
+			StandardError = FetchProgress,
+		};
+		GitPullBuilder returning = new(resultRunner, TestPaths.Root);
+
+		GitResult<GitCompleted> result =
+			await returning.TryExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false);
+
+		Assert.AreEqual(result.Error?.StandardError, exception.StandardError);
+	}
+
 	[TestMethod]
 	public async Task ForwardsProgressToTheRequestAsync()
 	{