fix: collapse an unmerged submodule's merge stages into one entry [patch]

CLAUDE.md

@@ -301,7 +301,25 @@ Non-obvious, load-bearing design points:
     submodule has moved: `Sha` is the recorded gitlink, `CheckedOutSha` is what the working directory
     holds. `CheckedOutSha` is null when uninitialised, because git prints the recorded gitlink again
     on that line and reporting it verbatim would make an uninitialised submodule indistinguishable
-    from a synchronised one. Listing does not recurse: `ls-files` enumerates only the superproject's
+    from a synchronised one.
+
+    **The stage field is the third thing `ls-files` says, and it decides how many records a path
+    gets.** A merged path carries stage `0` once; an unmerged one — a submodule both sides of a merge
+    moved to divergent commits — carries no stage `0` at all and appears once per stage present, each
+    naming a different commit, while `submodule status` reports that path once with a `U` marker and
+    the *null object id*. Filtering on the mode alone therefore reported one submodule three times
+    with three contradictory `Sha` values, each carrying forty zeroes as its `CheckedOutSha`.
+    `ParseGitlinks` collapses to one entry per path — stage `0` when present, otherwise `2` ("ours")
+    then `3` then `1`, since a submodule deleted on one side produces `1` and `3` with no `2` — and
+    `Apply` treats an all-zero object id the way it already treats `Uninitialised`. Two details worth
+    keeping: the mode filter runs *before* the stage is validated, so an unmerged blob (most of any
+    real conflict) is skipped rather than held to a gitlink's expectations; and the stages are a
+    closed set where a fifth throws, unlike `submodule status`'s marker characters, because
+    `ls-files` is plumbing and an unexpected stage means the record was misread rather than that git
+    grew a state. The all-zero test is by digit rather than against a constant, since the id is 64
+    characters under `--object-format=sha256`.
+
+    Listing does not recurse: `ls-files` enumerates only the superproject's
     index, so nested paths could only come from the wrapper. A caller recurses by composition, opening
     each submodule as its own `GitRepository`.
 

GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs

@@ -193,6 +193,92 @@
 		Assert.AreEqual("libs/nested/sub".As<RelativeDirectoryPath>(), resolved[0].Path);
 	}
 
+	[TestMethod]
+	public async Task CollapsesAnUnmergedSubmoduleIntoOneEntryAsync()
+	{
+		// Captured from git 2.43: a superproject merge where both sides moved the same gitlink to
+		// divergent commits. ls-files emits no stage 0 for such a path and one record per stage
+		// instead, so filtering on the mode alone reports one submodule three times, with three
+		// contradictory ids, for one directory on disk. The surrounding gitlinks are here so the
+		// collapse is seen to keep git's ordering rather than sorting or appending.
+		ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner()
+			.Then(standardOutput:
+				"160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/before\0" +
+				"160000 941c54aba3ecbbf714e1aa40a2aa1a1e4dfe7ff0 1\tlibs/sub\0" +
+				"160000 7182602cb7a28003eb333e162df18908e81a7866 2\tlibs/sub\0" +
+				"160000 d62f3736e0a7fef1a8dc59dfff3a2b84ffd8c095 3\tlibs/sub\0" +
+				"160000 120669ec6b336c651886335053ac0644d7821e09 0\tlibs/after\0")
+			.Then(standardOutput:
+				" 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 libs/before (heads/master)\n" +
+				"U0000000000000000000000000000000000000000 libs/sub\n" +
+				" 120669ec6b336c651886335053ac0644d7821e09 libs/after (heads/master)\n");
+		GitSubmoduleListBuilder builder = new(runner, TestPaths.Root);
+
+		IReadOnlyList<GitSubmodule> submodules =
+			await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false);
+
+		// The collapse keeps git's own ordering rather than sorting or appending: the unmerged path
+		// stays where git first listed it, between the two merged ones.
+		Assert.AreEqual(3, submodules.Count);
+		Assert.AreEqual("libs/before".As<RelativeDirectoryPath>(), submodules[0].Path);
+		Assert.AreEqual("libs/sub".As<RelativeDirectoryPath>(), submodules[1].Path);
+		Assert.AreEqual("libs/after".As<RelativeDirectoryPath>(), submodules[2].Path);
+
+		GitSubmodule conflicted = submodules[1];
+
+		Assert.AreEqual(GitSubmoduleState.Conflicted, conflicted.State);
+
+		// Stage 2 — "ours", the commit the branch being merged into records. Not stage 1's merge base
+		// (which git listed first, so a first-wins collapse would pick it) and not stage 3's "theirs"
+		// (which git listed last, so a last-wins collapse would pick that).
+		Assert.AreEqual("7182602cb7a28003eb333e162df18908e81a7866".As<GitCommitSha>(), conflicted.Sha);
+
+		// git prints its null object id on the status line, and it is well-formed enough that nothing
+		// downstream would question forty zeroes presented as a commit.
+		Assert.IsNull(conflicted.CheckedOutSha);
+	}
+
+	[TestMethod]
+	public void FallsBackToTheirStageWhenOursIsAbsent()
+	{
+		// A submodule deleted on the side being merged into: git emits the merge base and "theirs"
+		// with no stage 2 at all. Preferring stage 2 must degrade rather than drop the path, since a
+		// gitlink still in the index is exactly what a caller deciding whether a directory is safe to
+		// delete needs to see.
+		IReadOnlyList<GitSubmodule> gitlinks = GitSubmoduleParser.ParseGitlinks(
+			"160000 941c54aba3ecbbf714e1aa40a2aa1a1e4dfe7ff0 1\tlibs/sub\0" +
+			"160000 d62f3736e0a7fef1a8dc59dfff3a2b84ffd8c095 3\tlibs/sub\0");
+
+		Assert.AreEqual(1, gitlinks.Count);
+		Assert.AreEqual("d62f3736e0a7fef1a8dc59dfff3a2b84ffd8c095".As<GitCommitSha>(), gitlinks[0].Sha);
+	}
+
+	[TestMethod]
+	public void SkipsAnUnmergedBlobWithoutReadingItsStage()
+	{
+		// Most of any real conflict is unmerged *blobs*, which carry the same per-stage records. The
+		// mode filter has to run first, so a blob is skipped exactly as it always was rather than being
+		// held to a gitlink's expectations of its stage field.
+		IReadOnlyList<GitSubmodule> gitlinks = GitSubmoduleParser.ParseGitlinks(
+			"100644 abc1234000000000000000000000000000000000 1\tREADME.md\0" +
+			"100644 def5678000000000000000000000000000000000 2\tREADME.md\0" +
+			"160000 7182602cb7a28003eb333e162df18908e81a7866 0\tlibs/sub\0");
+
+		Assert.AreEqual(1, gitlinks.Count);
+		Assert.AreEqual("libs/sub".As<RelativeDirectoryPath>(), gitlinks[0].Path);
+	}
+
+	[TestMethod]
+	public void ThrowsForAStageGitDoesNotDefine()
+	{
+		// Unlike submodule status's marker characters, the stages are a closed set: ls-files is
+		// plumbing and git defines exactly 0 through 3. A fifth means the record was misread, and
+		// ranking it anyway would pick one of an unmerged path's commit ids at random.
+		_ = Assert.ThrowsExactly<GitParseException>(
+			() => _ = GitSubmoduleParser.ParseGitlinks(
+				"160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 4\tlibs/sub\0"));
+	}
+
 	[TestMethod]
 	public void ThrowsForAMalformedGitlinkRecord()
 	{

GitIntegration.Test/Integration/GitSubmoduleTests.cs

@@ -227,10 +227,129 @@
 		Assert.AreEqual(after[0].Sha, after[0].CheckedOutSha);
 	}
 
+	[TestMethod]
+	public async Task ReportsAConflictedSubmoduleOnceWithNoCheckedOutCommitAsync()
+	{
+		CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token;
+		await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false);
+
+		using TemporaryRepository subDirectory = new();
+		using TemporaryRepository superDirectory = new();
+
+		GitRepository sub = await CreateRepositoryAsync(subDirectory, "s.txt", cancellationToken).ConfigureAwait(false);
+		GitRepository super = await CreateRepositoryAsync(superDirectory, "m.txt", cancellationToken).ConfigureAwait(false);
+
+		await AddSubmoduleAsync(super, sub.LocalPath!, "libs/sub", cancellationToken).ConfigureAwait(false);
+
+		// The submodule's own working copy, as a repository in its own right — the same composition a
+		// caller uses to recurse, and the only way to move the gitlink somewhere the superproject can
+		// then record.
+		GitRepository checkout = new()
+		{
+			LocalPath = System.IO.Path.Join(superDirectory.RootPath, "libs", "sub").As<AbsoluteDirectoryPath>(),
+			ProcessRunner = super.ProcessRunner,
+		};
+
+		await IntegrationGitFixture.ConfigureIdentityAsync(checkout, AuthorName, AuthorEmail, cancellationToken)
+			.ConfigureAwait(false);
+
+		// Both branches are cut from the submodule's single commit before either moves, so the two
+		// commits below genuinely diverge. git resolves a submodule merge itself when one side is an
+		// ancestor of the other, and a merge it can resolve produces no conflict to read.
+		GitBranchName ours = "ours".As<GitBranchName>();
+		GitBranchName theirs = "theirs".As<GitBranchName>();
+
+		_ = await checkout.CreateBranch(ours).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+		_ = await checkout.CreateBranch(theirs).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		GitCommitSha oursSha = await CommitInSubmoduleAsync(
+			checkout, superDirectory, ours, "ours\n", cancellationToken).ConfigureAwait(false);
+		GitCommitSha theirsSha = await CommitInSubmoduleAsync(
+			checkout, superDirectory, theirs, "theirs\n", cancellationToken).ConfigureAwait(false);
+
+		// A branch of the superproject per side, each recording its own gitlink. "other" is the branch
+		// the merge runs on, so its gitlink is the one git stages as stage 2.
+		GitBranchName other = "other".As<GitBranchName>();
+		_ = await super.CreateBranch(other).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+		_ = await super.Checkout("other".As<GitRefName>()).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		await RecordGitlinkAsync(super, checkout, ours, cancellationToken).ConfigureAwait(false);
+
+		_ = await super.Checkout("main".As<GitRefName>()).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+		await RecordGitlinkAsync(super, checkout, theirs, cancellationToken).ConfigureAwait(false);
+
+		_ = await super.Checkout("other".As<GitRefName>()).ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		// merge is out of scope for this library, so the fixture runs it directly. It is expected to
+		// fail: "Recursive merging with submodules currently only supports trivial cases", which is
+		// precisely the unmerged index this test needs.
+		GitProcessResult merged = await super.ProcessRunner!.RunAsync(
+			new GitProcessRequest
+			{
+				Arguments = ["-C", superDirectory.RootPath, "merge", "--no-edit", "main"],
+			},
+			cancellationToken).ConfigureAwait(false);
+
+		Assert.IsFalse(merged.Success, "the submodule merge was expected to conflict but succeeded");
+
+		IReadOnlyList<GitSubmodule> submodules =
+			await super.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		// One entry, not one per merge stage: ls-files emits three records for this path, and the
+		// directory they all describe exists once.
+		Assert.AreEqual(1, submodules.Count);
+		Assert.AreEqual("libs/sub".As<RelativeDirectoryPath>(), submodules[0].Path);
+		Assert.AreEqual(GitSubmoduleState.Conflicted, submodules[0].State);
+
+		// Stage 2 — what the branch being merged into records — rather than the merge base or theirs.
+		Assert.AreEqual(oursSha, submodules[0].Sha);
+		Assert.AreNotEqual(theirsSha, submodules[0].Sha);
+
+		// git prints its null object id here. Reporting it verbatim would present forty zeroes as a
+		// commit, which is well-formed enough that nothing downstream would question it.
+		Assert.IsNull(submodules[0].CheckedOutSha);
+	}
+
+	/// <summary>Commits a change on one of the submodule's branches, and reports the commit.</summary>
+	private static async Task<GitCommitSha> CommitInSubmoduleAsync(
+		GitRepository checkout,
+		TemporaryRepository superDirectory,
+		GitBranchName branch,
+		string contents,
+		CancellationToken cancellationToken)
+	{
+		_ = await checkout.Checkout(branch.WeakString.As<GitRefName>())
+			.ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		superDirectory.WriteFile("libs/sub/s.txt", contents);
+
+		_ = await checkout.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		GitCommit commit = await checkout.Commit(branch.WeakString.As<GitCommitMessage>())
+			.ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		return commit.Sha;
+	}
+
+	/// <summary>Checks a branch out in the submodule and records the result as the superproject's gitlink.</summary>
+	private static async Task RecordGitlinkAsync(
+		GitRepository super,
+		GitRepository checkout,
+		GitBranchName branch,
+		CancellationToken cancellationToken)
+	{
+		_ = await checkout.Checkout(branch.WeakString.As<GitRefName>())
+			.ExecuteAsync(cancellationToken).ConfigureAwait(false);
+
+		_ = await super.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false);
+		_ = await super.Commit($"record {branch.WeakString}".As<GitCommitMessage>())
+			.ExecuteAsync(cancellationToken).ConfigureAwait(false);
+	}
+
 	[TestMethod]
 	public async Task ReportsNoSubmodulesForARepositoryWithNoneAsync()
 	{
 		CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token;
 		await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false);
 
 		using TemporaryRepository temporary = new();

GitIntegration/Models/GitSubmodule.cs

@@ -24,6 +24,13 @@ public sealed record GitSubmodule
 	/// The gitlink itself, read from the superproject's index. This is what a
 	/// <c>submodule update</c> would check out, and it does not change when someone commits inside
 	/// the submodule's working directory — <see cref="CheckedOutSha"/> is what moves then.
+	/// <para>
+	/// A submodule whose <see cref="State"/> is <see cref="GitSubmoduleState.Conflicted"/> has no
+	/// single recorded gitlink: the index holds one per merge stage. This is the "ours" stage — the
+	/// commit recorded by the branch being merged into — falling back to "theirs" and then to the
+	/// merge base when the side that would carry it deleted the submodule instead. The listing still
+	/// reports one entry per submodule, since one is what exists on disk.
+	/// </para>
 	/// </remarks>
 	public required GitCommitSha Sha { get; init; }
 
@@ -38,6 +45,11 @@ public sealed record GitSubmodule
 	/// submodule, whose working directory holds no checkout to report — git prints the recorded
 	/// gitlink again in that case, which would otherwise make an uninitialised submodule look
 	/// indistinguishable from a synchronised one.
+	/// <para>
+	/// Also <see langword="null"/> for a <see cref="GitSubmoduleState.Conflicted"/> submodule, where
+	/// git prints its null object id rather than a commit. That value is well-formed enough to pass
+	/// for a commit id, so reporting it verbatim would hand a caller forty zeroes to look up.
+	/// </para>
 	/// </remarks>
 	public GitCommitSha? CheckedOutSha { get; init; }