fix: collapse an unmerged submodule's merge stages into one entry [patch] - #111
Merged
Merged
Conversation
…tch]
`ls-files --stage` emits one record per merge stage for an unmerged path, and
`GitSubmoduleParser` filtered those records on the mode field alone, discarding
the stage. A submodule both sides of a merge moved to divergent commits was
therefore reported three times, sharing one `Path` and carrying three
contradictory `Sha` values, for a directory that exists once on disk.
`submodule status` reports that same path once, with a `U` marker and git's
null object id. `ApplyStatus` matched all three entries against that one line,
and `Apply` suppressed the object id only for `Uninitialised` — so each entry
also reported forty zeroes as its `CheckedOutSha`, well-formed enough that
nothing downstream would question it as a commit.
Read the stage and emit 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. The mode filter still runs first, so an unmerged blob is skipped
rather than held to a gitlink's expectations of its stage field. A stage
outside 0-3 throws: `ls-files` is plumbing with a closed set, so an unexpected
one means the record was misread, and ranking it anyway would pick one of the
path's commit ids at random.
`Apply` now treats an all-zero object id the way it already treats
`Uninitialised`. The test is by digit rather than against a constant, because
the id is 64 characters under `--object-format=sha256`.
Covered at both tiers: captured output in the builder tests, and an integration
test that builds a real unmerged index by merging two superproject branches
whose gitlinks diverge.
Fixes #103
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y1rCxPMXL6tjQaqr1noz3f
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Fixes #103
What was wrong
GitSubmoduleParser.ParseGitlinksfilteredls-files --stagerecords on the mode field alone and discarded the stage field. During an unfinished merge git emits one record per stage for an unmerged path, so a submodule both sides moved to divergent commits came back as threeGitSubmoduleentries sharing onePath, each with a differentSha, for a directory that exists once on disk.submodule statusreports that same path once, with aUmarker and git's null object id.ApplyStatusmatched all three entries against that single line, andApplysuppressed the object id only forUninitialised— so each entry also reported0000000000000000000000000000000000000000as itsCheckedOutSha. That value is well-formed enough that nothing downstream would question it as a commit.Verified against git 2.43, which is what the issue reported:
The change
ParseGitlinksreads the stage and emits one entry per path. Stage0wins when present; among the unmerged stages the order is2("ours" — the commit the branch being merged into records), then3, then1. The fallbacks are not theoretical: a submodule deleted on one side produces stages1and3with no2at all. Git's own ordering is preserved — a later stage revises the path's entry in place rather than moving it.Two details worth calling out:
0–3throwsGitParseException, unlikesubmodule status's marker characters which degrade toUnknown.ls-filesis plumbing with a closed set: a fifth stage means the record was misread, and ranking it anyway would pick one of the path's commit ids at random.Applytreats an all-zero object id the way it already treatsUninitialised, soCheckedOutShaisnullrather than the null OID. The test is by digit rather than against a constant, because the id is 64 characters under--object-format=sha256.GitSubmodule's own docs and CLAUDE.md's submodule design note record both behaviours.Tests
Covered at both tiers, as the issue's "Missing coverage" section asks.
Unit (
GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs), from captured git 2.43 output:CollapsesAnUnmergedSubmoduleIntoOneEntryAsync— three stages become one entry carrying stage 2's id, withConflictedstate and a nullCheckedOutSha. Merged gitlinks either side of it assert the collapse keeps git's ordering, and stage 1 being listed first / stage 3 last means neither a first-wins nor a last-wins collapse would pass.FallsBackToTheirStageWhenOursIsAbsent— stages 1 and 3 with no 2.SkipsAnUnmergedBlobWithoutReadingItsStage— the mode-filter-first ordering.ThrowsForAStageGitDoesNotDefine.Integration (
GitIntegration.Test/Integration/GitSubmoduleTests.cs), which needs a real unmerged index:ReportsAConflictedSubmoduleOnceWithNoCheckedOutCommitAsync— builds two superproject branches whose gitlinks point at divergent submodule commits and merges them. Both branches are cut from the submodule's single commit before either moves, because 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.mergeis out of scope for this library, so the fixture runs it directly and asserts it fails.Verification
dotnet buildanddotnet build -c Release: 0 warnings, 0 errors.KTSU_GIT_INTEGRATION_TESTS_REQUIRED=1 GIT_CONFIG_NOSYSTEM=1 dotnet test: 593 passed, 0 failed, integration tier included rather than skipped.ParseGitlinksfails 4 of the 5 new tests (SkipsAnUnmergedBlobWithoutReadingItsStagepasses either way — it is a regression guard on behaviour this change must not disturb).IsNullObjectId's result fails the twoCheckedOutShaassertions. Deleting the call instead fails the build withIDE0051: IsNullObjectId is unused— exactly the case CLAUDE.md warns about, which is why the mutation is a substitution.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y1rCxPMXL6tjQaqr1noz3f
Generated by Claude Code