diff --git a/CLAUDE.md b/CLAUDE.md index a56a1f2..bf7ee7c 100644 --- a/CLAUDE.md +++ b/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`. diff --git a/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs b/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs index dcc4702..016b875 100644 --- a/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs +++ b/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs @@ -193,6 +193,92 @@ public void MatchesAStatusLineAgainstGitsOwnSpellingOfThePath() Assert.AreEqual("libs/nested/sub".As(), 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 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(), submodules[0].Path); + Assert.AreEqual("libs/sub".As(), submodules[1].Path); + Assert.AreEqual("libs/after".As(), 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(), 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 gitlinks = GitSubmoduleParser.ParseGitlinks( + "160000 941c54aba3ecbbf714e1aa40a2aa1a1e4dfe7ff0 1\tlibs/sub\0" + + "160000 d62f3736e0a7fef1a8dc59dfff3a2b84ffd8c095 3\tlibs/sub\0"); + + Assert.AreEqual(1, gitlinks.Count); + Assert.AreEqual("d62f3736e0a7fef1a8dc59dfff3a2b84ffd8c095".As(), 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 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(), 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( + () => _ = GitSubmoduleParser.ParseGitlinks( + "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 4\tlibs/sub\0")); + } + [TestMethod] public void ThrowsForAMalformedGitlinkRecord() { diff --git a/GitIntegration.Test/Integration/GitSubmoduleTests.cs b/GitIntegration.Test/Integration/GitSubmoduleTests.cs index fb803a5..0d34e91 100644 --- a/GitIntegration.Test/Integration/GitSubmoduleTests.cs +++ b/GitIntegration.Test/Integration/GitSubmoduleTests.cs @@ -227,6 +227,125 @@ .. clone.UpdateSubmodules().Initialise().Recursive().BuildArguments(), 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(), + 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 theirs = "theirs".As(); + + _ = 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(); + _ = await super.CreateBranch(other).ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await super.Checkout("other".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + await RecordGitlinkAsync(super, checkout, ours, cancellationToken).ConfigureAwait(false); + + _ = await super.Checkout("main".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + await RecordGitlinkAsync(super, checkout, theirs, cancellationToken).ConfigureAwait(false); + + _ = await super.Checkout("other".As()).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 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(), 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); + } + + /// Commits a change on one of the submodule's branches, and reports the commit. + private static async Task CommitInSubmoduleAsync( + GitRepository checkout, + TemporaryRepository superDirectory, + GitBranchName branch, + string contents, + CancellationToken cancellationToken) + { + _ = await checkout.Checkout(branch.WeakString.As()) + .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()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + return commit.Sha; + } + + /// Checks a branch out in the submodule and records the result as the superproject's gitlink. + private static async Task RecordGitlinkAsync( + GitRepository super, + GitRepository checkout, + GitBranchName branch, + CancellationToken cancellationToken) + { + _ = await checkout.Checkout(branch.WeakString.As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + _ = await super.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await super.Commit($"record {branch.WeakString}".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + } + [TestMethod] public async Task ReportsNoSubmodulesForARepositoryWithNoneAsync() { diff --git a/GitIntegration/Models/GitSubmodule.cs b/GitIntegration/Models/GitSubmodule.cs index 30499e1..c83ce93 100644 --- a/GitIntegration/Models/GitSubmodule.cs +++ b/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 /// submodule update would check out, and it does not change when someone commits inside /// the submodule's working directory — is what moves then. + /// + /// A submodule whose is 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. + /// /// 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. + /// + /// Also for a 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. + /// /// public GitCommitSha? CheckedOutSha { get; init; } diff --git a/GitIntegration/Parsing/GitSubmoduleParser.cs b/GitIntegration/Parsing/GitSubmoduleParser.cs index 88b8038..cba4dce 100644 --- a/GitIntegration/Parsing/GitSubmoduleParser.cs +++ b/GitIntegration/Parsing/GitSubmoduleParser.cs @@ -48,18 +48,34 @@ internal static class GitSubmoduleParser /// Reads the recorded gitlinks from NUL-terminated ls-files --stage output. /// /// + /// /// Each record is <mode> <object> <stage>\t<path>. Everything that /// is not a gitlink is skipped, which is most of a repository: this command lists the whole index, /// and the mode filter is what turns that into a submodule listing. + /// + /// + /// The stage field is read rather than discarded, because it is the field that says whether a path + /// appears once or several times. A merged path carries stage 0 and appears exactly once; + /// an unmerged one — a submodule both sides of a merge moved to divergent commits — + /// carries no stage 0 record at all and instead appears once per stage present, each naming + /// a different commit. Listing those verbatim would report one submodule three times, with three + /// contradictory values, for a directory that exists once on disk. + /// One entry per path is emitted instead — see for which record wins. + /// /// /// Everything git wrote to standard output. - /// One entry per gitlink, in the order git listed them, with no state resolved yet. + /// One entry per gitlink path, in the order git first listed it, with no state resolved yet. /// A gitlink record did not have the expected shape. internal static IReadOnlyList ParseGitlinks(string output) { Ensure.NotNull(output); - List submodules = []; + // The dictionary carries the winning record per path and the order list carries git's own + // ordering, because the two questions are separate: a later stage of a path already seen + // revises that path's entry without moving it, so an unmerged submodule stays where git put it + // rather than jumping to wherever its highest-precedence stage happened to appear. + Dictionary byPath = []; + List order = []; foreach (string record in output.Split('\0')) { @@ -86,22 +102,69 @@ internal static IReadOnlyList ParseGitlinks(string output) throw new GitParseException($"Malformed ls-files record: '{record}'."); } + // The mode filter runs before the stage is validated, so an unmerged *blob* — which this + // command also emits one record per stage for, and which is most of any real conflict — is + // skipped exactly as it always was rather than being held to a gitlink's expectations. if (!string.Equals(fields[0], GitlinkMode, StringComparison.Ordinal)) { continue; } - submodules.Add(new GitSubmodule + string path = record[(tab + 1)..]; + int precedence = StagePrecedence(fields[2], record); + + bool seen = byPath.TryGetValue(path, out (int Precedence, GitSubmodule Submodule) existing); + + if (seen && existing.Precedence >= precedence) + { + continue; + } + + if (!seen) + { + order.Add(path); + } + + byPath[path] = (precedence, new GitSubmodule { - Path = GitParseValues.ToRelativeDirectoryPath(record[(tab + 1)..]), + Path = GitParseValues.ToRelativeDirectoryPath(path), Sha = GitParseValues.ToSemantic(fields[1], "submodule gitlink object id"), State = GitSubmoduleState.Unknown, }); } - return submodules; + return [.. order.Select(path => byPath[path].Submodule)]; } + /// + /// Ranks an ls-files --stage stage field, so the record that best represents a path wins. + /// + /// + /// Stage 0 outranks everything: it is what a merged path carries, and its presence means + /// there is nothing to collapse. Among the unmerged stages the order is 2 then 3 + /// then 1 — "ours" first, because a caller inspecting a repository mid-merge is standing on + /// the branch that is being merged into and that is the commit its history records; + /// "theirs" next, and the merge base last, since the base is the one commit neither side chose. + /// The fallbacks are not theoretical: a submodule deleted on one side of the merge produces stages + /// 1 and 3 with no 2 at all. + /// + /// The stage field, exactly as git spelled it. + /// The whole record, for the diagnostic. + /// A rank, where higher wins. + /// The stage was not one git defines. + private static int StagePrecedence(string stage, string record) => stage switch + { + "0" => 3, + "2" => 2, + "3" => 1, + "1" => 0, + + // A closed set, unlike submodule status's marker characters: this is plumbing, and git defines + // exactly these four stages. A fifth would mean the record was misread, not that git grew a + // state — and inventing a rank for it would silently pick a commit id at random. + _ => throw new GitParseException($"Malformed ls-files record: '{record}'."), + }; + /// /// Resolves each gitlink's working-directory state from submodule status output. /// @@ -213,12 +276,30 @@ private static GitSubmodule Apply(GitSubmodule submodule, StatusLine status, str // git prints the recorded gitlink again for an uninitialised submodule, which would make it // indistinguishable from a synchronised one. Nothing is checked out there, so nothing is // reported. - CheckedOutSha = status.State == GitSubmoduleState.Uninitialised + // + // The null object id is the same question asked by the other command: git prints it for an + // unmerged submodule, where there is no single checked-out commit to name. It is a well-formed + // object id as far as the semantic type is concerned, so nothing downstream would catch it — + // it would simply read as a commit that happens to be all zeroes. + CheckedOutSha = status.State == GitSubmoduleState.Uninitialised || IsNullObjectId(status.ObjectId) ? null : GitParseValues.ToSemantic(status.ObjectId, "submodule checked-out object id"), Describe = describe, }; + /// + /// Reports whether an object id is git's null id — the absence of an object, not an object. + /// + /// + /// Tested by its digits rather than against a constant, because the id's width is the repository's + /// object format: 40 characters under SHA-1 and 64 under --object-format=sha256. The empty + /// case is excluded explicitly, so "nothing at all" is never mistaken for "all zeroes". + /// + /// The object id git printed. + /// when every character is a zero. + private static bool IsNullObjectId(string objectId) => + objectId.Length > 0 && objectId.AsSpan().IndexOfAnyExcept('0') < 0; + /// /// Spells a path the way git prints it, so a status line can be matched against it. ///