[brettheap]

Lane: openreposhape-4 (openRepoShape-4)
Claim: #300 (comment)

What and why

Four commits on main c0ed615, none rewritten:

• 03ddadd, the change;
• e0cc351, the fix round on its review (MERGEABLE WITH NITS, no MUST);
• 9bb6f83, the second round on the re-check of e0cc351 (MERGEABLE WITH NITS, no MUST, no SHOULD, one nit);
• c23e2b0, a test fix after tests-windows failed on 9bb6f83: the same-url validator row wrote the member's real url into .gitmodules raw, and on Windows that url is a path whose backslashes git config reads as escapes (\a is none), so git refused the file and the validator rightly said gitmodules-unreadable; the row now has git config write the section, which escapes it. No product code changed.

family.py add asked the disk, the index, git's dry run and the NAMES in .gitmodules (#242's member-name-taken), but never whether a section under ANOTHER name already gave the mount path. git writes the member's own section beside such a "ghost" without a word. Reproduced on c0ed615 with git 2.43.0, local bare remotes and a gh shim that was never called:

• A committed [submodule "ghost"] path = members/Atlas, url = /nowhere/ghost.git, then add --member testorg/Atlas: exit 0, a commit whose .gitmodules has two sections for members/Atlas, and validate-family.py says family ok: Fam (fam), 1 member(s). An unstaged hand edit does the same.
• The issue's clone run: a recursive clone follows the section LAST in the file, so with the ghost second it dies fetching the ghost's url.

A ghost is what a half-removed member, a hand edit or a merge that kept both sides leaves behind. It is the mirror of member-name-taken.

The rulings

1. Refuse before the mount. _checked_mount_path has a new block after member-name-taken's, and the lines around it are not reflowed. Any section whose path is the mount path and whose name is not the mount path is refused as member-path-registered:
   • It runs before the member is read or anything is mounted.
   • It names each such section and its url, or says the section has no url.
   • The remediation removes each section by every key it holds, in a POSIX shell: one git config -f .gitmodules --unset-all 'submodule.<name>.<key>' per key (path, url, and any branch, shallow, update or other key), then git add .gitmodules. Or family.py remove, if the section was a member.
     • Not --remove-section, because on git 2.43 it cannot address a section whose name holds ], nor one whose header a hand spelled [Submodule "..."] (no such section for both).
     • A key reaches every section git config --list names, and --unset-all takes a key given twice.
     • Every key the section holds is unset, so git drops the header with the last one, for every section. Only a comment line inside the section keeps its header, and it keeps no key.
     • Why every key: a key left behind is inherited by a member later given that name, because git writes its path and url into the same section. The re-check of e0cc351 showed add --member testorg/Zed committing a ghost's branch = dev and shallow = true into Zed's section, after which git submodule update --remote members/Zed failed.
     • Proved on 2.43 by pasting the printed commands into sh for both cases.
   • Only when the working tree has no .gitmodules does it also name the git checkout that puts back the copy git read.
   • The section is never dropped or rewritten as part of the add.
   • The refusal reads the file through the reader the name check already uses. _submodule_paths is extended in place to return {name: (path, url)}, so it reads the copy git reads: the working tree's (an unstaged edit included), else the index's, else HEAD's. No second reader was added.
   • Names and paths are compared exactly, as git compares them:
     • Members/Atlas is another name than members/Atlas, so a section with that name for the mount path is a ghost.
     • A section for members/Atlas/ is not one for the mount path.
     • A section whose name IS the mount path is not a ghost: remove leaves one when a ghost followed the member, and git rewrites it in place.
2. The validator (templates/family-root/scripts/validate-family.py) gains gitmodules-path-twice: two or more sections of the holder's .gitmodules with one path, whatever their urls. Its remediation is to unset every key of each extra section.
   • It reads the working-tree file with git config -z --file .gitmodules --list, as family.py does, with no parser of its own. CI's checkout equals HEAD.
   • It runs in every mode, --pins and --no-members included, as family.py add compares --member with the existing rows case-sensitively, so --member inkrouter/irrs on a family that already pins InkRouter/IRRS mounts the same repository again as members/irrs, both gates pass a family that pins one project twice, and bump --member irss's refusal recommends exactly that add #206's member-duplicate-id does, because it needs nothing but that file. That matters: a ghost placed last breaks CI's member fetch, and CI then falls back to --no-members.
   • A missing .gitmodules (a holder with no members) is no finding.
   • Two cases are the refusal gitmodules-unreadable (exit 2), never a pass and never exit 1:
     • a .gitmodules git cannot read;
     • no git to run. Before family.py: add never reads .gitmodules, so a section under ANOTHER name whose path is already members/<P> (a "ghost" section) does not stop it: it exits 0 and commits two sections for one path, validate-family.py says family ok, and a recursive clone takes the LATER section, so with the ghost second it dies fetching the ghost's URL #300, --no-members ran no git at all.
   • The validator is a copied file, so holders get the new check through update-shape.py. update-shape.py check --root <a holder cut from c0ed615> --upstream <this branch> reports upstream-changed scripts/validate-family.py, with the other nine copies unchanged.
   • No test or fixture pins the template's bytes or digest; they are all computed at run time. --help is unchanged, so docs/cli.md does not move.
3. remove after a ghost is not changed here. It is family.py: bump and remove take the git pathspec from the manifest row's path: with no check and no --literal-pathspecs, so a hand-edited path: members/* makes remove --member Atlas exit 0 with ONE commit that unmounts every member while dropping only Atlas's row, and path: . makes it git rm -r -f the holder's own files and die on a FileNotFoundError, though validate-family.py already has a rule, member-path, for what path: must equal #299's function this round. Probed on c0ed615:
   • With the ghost SECOND, remove --member Atlas exits 0. Its commit's .gitmodules still holds [submodule "members/Atlas"] with the member's url, the holder's .git/config keeps submodule.members/Atlas.url and .active, and validate-family.py says family ok … 0 member(s).
   • With the ghost FIRST, as add left it, remove exits 0 and its commit's .gitmodules still holds the ghost.
   • Either way git's rm and deinit took out only the section git resolves the path to: the later one.

Not in this change (follow-ups)

• remove with two sections for one path (ruling 3).
• A validator finding for a section whose path no members: row names. After either remove above, the leftover section passes the gate.
• A case variant of the PATH (family.py: MountRollback reads the holder's index with a case-sensitive pathspec (git ls-files -s -- <path>), so with core.ignorecase=true and an index entry Members/Atlas/README a refused add leaves the 19 files git submodule add staged under Members/Atlas/ and still prints "the holder is exactly as it was before this command" #302's ground). Probed on Linux:
  • A section ghost for Members/Atlas is not refused, with or without core.ignorecase=true. The add exits 0 and commits both sections, because the comparison is exact.
  • gitmodules-path-twice does not report the pair either.
  • On a case-insensitive disk the two paths are one directory. Not reproduced on macOS or Windows.
• add over a .gitmodules git cannot parse says git-failed, with a remediation that names validate-pins.py, a file no holder carries. This is unchanged since family.py add runs git submodule add without --force (and _commit_one a plain git add), so under an operator's global excludes naming members or the member's own name it exits 2 at the mount for every member, and the remediation it prints blames access (gh auth status, your SSH key) instead of the ignore rule #242.
• add with a ghost-free .gitmodules present only in the index or in HEAD is refused late, by git, after the member was read. This is unchanged since before family.py: add never reads .gitmodules, so a section under ANOTHER name whose path is already members/<P> (a "ghost" section) does not stop it: it exits 0 and commits two sections for one path, validate-family.py says family ok, and a recursive clone takes the LATER section, so with the ghost second it dies fetching the ghost's URL #300.

Tests

tests/test_family_add_refusals.py, appended in a section of its own. Each test compares a whole-holder snapshot before and after, in the file's style:

• The refusal, four rows. A section named ghost for members/IRQS is kept in one place per row. Rows are built in git's index and objects, never through a checkout:

  • in the working-tree file only, unstaged;
  • committed and in the work tree;
  • only in the index's .gitmodules;
  • only in HEAD's (the work-tree file and the index entry removed).

  In each row, add --member InkRouter/IRQS exits 2 with:

  • the member-path-registered refusal and remediation, word for word;
  • nothing on stdout, so the member was never read;
  • nothing cloned or mounted;
  • the snapshot byte-identical, with the ghost still there;
  • no gh call.

• Another name in another case. A section named Members/IRQS for members/IRQS is refused, named in its own spelling.

• The control, four rows. Each exits 0 with one section for the mount path, the member's url, a clean tree and a green validator:

  • the plain add;
  • an add past a section for members/IRQSX;
  • an add past a section for members/IRQS/;
  • an add past a section named members/IRQS for its own path.

• Two ghosts. One is named old IRQS (with a space) and has no url. The refusal names both. Its three printed git config commands, run exactly as printed (split by shlex.split, run with no shell), take both out, and the same add then succeeds.

• Every key. A ghost named members/IRQZ also carries branch, shallow and update. The refusal prints one command per key. Run as printed, they leave nothing under submodule.members/IRQZ., and an add of a member called IRQZ then commits a section holding only path and url.

• Sections --remove-section cannot name. A ghost named old]IRQS, and one under a hand-capitalised [Submodule "ghost"] header. The commands printed for each, run as printed, clear the way for the same add.

• The validator. A holder with one section per path is clean. One more section for members/IRRS, under another name and last in the file, is the only finding, gitmodules-path-twice word for word:

  • under no flag, --pins and --no-members;
  • whether the second section's url is another or the same.

• The validator's refusals. An unreadable .gitmodules is gitmodules-unreadable, exit 2. The holder's own validator, run in-process with its git call failing as a missing program does, refuses gitmodules-unreadable (exit 2) instead of crashing. No PATH is changed.

Runs on Linux, git 2.43.0:

• on the head: tests/test_family_add_refusals.py tests/test_family_add_excludes.py tests/test_family.py tests/test_update_shape.py, 253 passed, 0 skipped, 0 failed.

On e0cc351's bytes, before the every-key change:

• the run: /usr/bin/python3 -m pytest tests/test_family_add_refusals.py tests/test_family.py tests/test_family_add_excludes.py tests/test_cli_reference.py tests/test_update_shape_add.py tests/test_update_shape.py tests/test_shape_doctor.py tests/test_update_shape_pr_base.py tests/test_repo_hygiene.py -q;
• the result: 601 passed, 3 skipped, 0 failed. The three skips are the PowerShell rows of test_a_template_validators_next_runs_when_pasted_into_powershell, which run only on a Windows host (tests/test_shape_doctor.py:5330). git-filter-repo 2.47.0 was on PATH, and a gh shim that exits 97 was asked only --version and auth status.

Merges:

• family.py: bump and remove refuse member-path when a row's path: is not members/<project> or its project: is not a directory name, and hand git every path literally, so a hand-edited path: members/* or project: . no longer makes remove unmount every member in one commit while dropping one row (#299) #315 at 8f8eb92: the merge is clean with the head (git merge-tree), and on e0cc351 tests/test_family_add_refusals.py tests/test_family_member_path.py tests/test_family_add_excludes.py tests/test_family.py give 218 passed.
• family.py: add asks the holder's index for the mount path in any case, so an entry Members/Atlas/README is refused as member-path-taken before anything is cloned instead of leaving the member's 19 files staged under it, and a refused add says the holder is exactly as it was only when the whole index is as it was (#302) #318 (family.py: MountRollback reads the holder's index with a case-sensitive pathspec (git ls-files -s -- <path>), so with core.ignorecase=true and an index entry Members/Atlas/README a refused add leaves the 19 files git submodule add staged under Members/Atlas/ and still prints "the holder is exactly as it was before this command" #302): the merge conflicts in tests/test_family_add_refusals.py only. It is an append against an append at the end of the file, and keeping both blocks resolves it. scripts/family.py auto-merges.

Mutants, over -k "section or gitmodules or name_taken or no_git" in this file and tests/test_family_add_excludes.py, each in a scratch clone, on 03ddadd and on the head:

mutant	03ddadd	head
the refusal dropped (the double section comes back)	4 failed	8 failed
the reader on the work tree only	4 failed (the index and HEAD rows, and #242's two off-disk member-name-taken rows)	the same 4 failed
the validator check dropped	2 failed	4 failed
the reader takes the index before the work tree	12 passed	3 failed (the work-tree-only row and the two cannot-name rows)
names compared case-insensitively	12 passed	1 failed (Members/IRQS)
a trailing slash stripped from the path	12 passed	1 failed (the members/IRQS/ control)
only differing urls flagged	12 passed	1 failed (the same-url validator row)
the OSError clause dropped	(no clause)	1 failed (the no-git test)
only a ghost's path and url unset (what e0cc351 printed)	(no test; e0cc351's own code fails the every-key test)	1 failed (the every-key test: its text, and with that assertion dropped, the leftover keys and then the keys IRQZ inherits)

Closes #300

🤖 Generated with Claude Code

Summary by Sourcery

Prevent duplicate submodule path registrations from being created or accepted by adding pre-mount refusal and validator checks for conflicting .gitmodules sections.

Bug Fixes:

• Prevent family.py add from creating duplicate .gitmodules sections for a mount path already registered under another name.
• Reject holders with duplicate .gitmodules paths during family validation, including pins and no-members modes.

Enhancements:

• Provide actionable remediation for ghost sections by removing every key associated with the conflicting section.
• Treat unreadable or unavailable .gitmodules data as an explicit validator refusal rather than a pass or crash.

Tests:

• Add comprehensive coverage for conflicting sections across working tree, index, and HEAD, including unusual names, extra keys, duplicate URLs, unreadable files, and missing Git.

[sourcery-ai]

Sorry @brettheap, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 19 hours by commenting @sourcery-ai review. Upgrade to get a review now.

[sourcery-ai]

Reviewer's Guide

The PR closes the duplicate-mount-path gap by making family.py add refuse ghost .gitmodules sections before any member work, adding a validator finding for holders that already contain duplicate paths, and covering source-location, remediation, control, and unreadable-file cases with regression tests.

Sequence diagram for rejecting duplicate submodule mount paths

sequenceDiagram
    participant User
    participant Family as family.py add
    participant GitConfig as git config
    participant Holder as Holder repository

    User->>Family: add --member project/member
    Family->>GitConfig: _submodule_paths(root)
    GitConfig-->>Family: sections with path and url
    Family->>Family: _checked_mount_path(root, project, path)
    alt ghost section uses the mount path
        Family-->>User: Refusal member-path-registered
        User->>GitConfig: --remove-section submodule.name
        User->>Holder: git add .gitmodules
    else no conflicting section
        Family->>Holder: mount member and commit
        Family-->>User: add succeeds
    end

Loading

Flow diagram for detecting duplicate .gitmodules paths

flowchart TD
    Start["validate-family.py"] --> Exists{".gitmodules exists?"}
    Exists -->|No| Pass["No gitmodules finding"]
    Exists -->|Yes| Read["git config --file .gitmodules --list"]
    Read -->|Unreadable| Error["Refusal: gitmodules-unreadable"]
    Read -->|Readable| Group["Group sections by path"]
    Group --> Duplicate{"Any path has multiple sections?"}
    Duplicate -->|No| Clean["Validation continues"]
    Duplicate -->|Yes| Finding["Finding: gitmodules-path-twice"]

Loading

File-Level Changes

Change	Details	Files
Prevent adding a member when another .gitmodules section already owns the exact mount path.	Extend the shared .gitmodules reader to return each section’s path and optional URL, using working tree, index, or HEAD fallback. Check for path collisions under a different section name after the existing name collision check and refuse before reading or mounting the member. Report all conflicting sections, URLs, shell-quoted removal commands, and remediation for restoring an off-disk .gitmodules copy. Preserve the existing behavior for exact-name sections and non-exact paths.	scripts/family.py
Detect duplicate .gitmodules paths during family validation.	Read the working-tree .gitmodules through git config and report duplicate paths as gitmodules-path-twice, including section names and URLs. Run the check in all validator modes, including --pins and --no-members. Treat unreadable .gitmodules files as gitmodules-unreadable with exit status 2, while allowing missing files.	templates/family-root/scripts/validate-family.py
Add regression coverage for path-registration refusals and validation failures.	Test ghosts sourced from the working tree, index, and HEAD, asserting no output, mount, clone, commit, or snapshot changes. Test multiple ghosts, quoted section names, missing URLs, exact-path controls, and successful remediation. Test duplicate-path findings across validator modes and unreadable .gitmodules refusal behavior.	tests/test_family_add_refusals.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#300	Make family.py add refuse before mounting when any .gitmodules section under a different name already registers the requested members/<P> path, including registrations present only in the index or HEAD.	✅
#300	Extend validate-family.py to report holders whose .gitmodules contains multiple sections for the same path, including copied validators and operation modes such as --pins and --no-members.	✅
#300	Make the validator report a .gitmodules section whose path is not named by any family.yaml member row.	❌	The PR explicitly excludes this check as a follow-up; it only detects duplicate .gitmodules paths.

Possibly linked issues

• family.py: add never reads .gitmodules, so a section under ANOTHER name whose path is already members/<P> (a "ghost" section) does not stop it: it exits 0 and commits two sections for one path, validate-family.py says family ok, and a recursive clone takes the LATER section, so with the ghost second it dies fetching the ghost's URL #300: PR directly fixes issue family.py: add never reads .gitmodules, so a section under ANOTHER name whose path is already members/<P> (a "ghost" section) does not stop it: it exits 0 and commits two sections for one path, validate-family.py says family ok, and a recursive clone takes the LATER section, so with the ghost second it dies fetching the ghost's URL #300 by refusing ghost path registrations and reporting duplicate .gitmodules paths.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud