Skip to content

shape-doctor.py and siblings.py: a .gitmodules section git ignores ([submodule ""], a .. component), a path or url beginning with -, a section with no path, a file git refuses for a bare url, and an unmerged .gitmodules name no mount url, NEXT line or clone, where git says No url found and exits 128 (#733) - #775

Merged
brettheap merged 16 commits into
mainfrom
l3-733-ignored-sections
Oct 18, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Lane: openreposhape-3 (openRepoShape-3)
Claim: #733 (comment)

Closes #733.

Stacked on #774; lands after it. It changes submodule_keys, as #774 (#732) does, so its diff here shows #774's commit 311ad7d9 until that lands. Lands after the v1.1.0 tag.

What was wrong (re-measured on main e3137010 with #774, git 2.43.0)

I built one fresh repository per case, with a real gitlink at p, and took git submodule init -- p as git's answer. The issue's claims hold, and so do the three cases in its later comment (issuecomment-6221711270).

  • Sections git ignores whole. [submodule ""], ../evil, a/../b and a\..\b: git says No url found, exit 128. The doctor gave ('', '/r/u.git') or ('../evil', '/r/u.git'), and siblings.submodule_urls gave {'p': '/r/u.git'}. a..b is read, as a control.

  • Keys git ignores alone. A url beginning with - is ignored: -u alone is No url found, and /r/first.git then -u registers /r/first.git. The doctor and siblings took -u. A path beginning with - is ignored too: p then -p stays p, where siblings keyed the url by -p and mounted_from fell back to repository:.

  • A file git refuses whole. It refuses over a key in a section it reads, given with no value at all:

    • a bare path, url, ignore or update gives bad config line N, exit 128;
    • a bare branch crashes git, exit 139;
    • a bare shallow, fetchRecurseSubmodules or unknown key is fine, and so is any bare key in a section git ignores.

    Both tools read every other section of such a file.

  • A section with a url and no path. git mounts it nowhere (No url found for its name as a path). mounted_from and siblings keyed it by its name.

  • An unmerged .gitmodules (comment 1). git reads no copy, so git submodule init says No url found with conflict markers in the working tree, with the conflict resolved there, and with that copy deleted. gitmodules_entry gave (None, None), then ('p', '/r/resolved.git'), then HEAD's url.

  • The last section giving a path has no url (comment 2). This is left as it is. gitmodules_entry gives the section and no url, so the member is url-less, as git says. mounted_from gives an earlier section's url, as siblings does. Its contract is to agree with make siblings, and validate-family.py refuses two sections for one path (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).

  • Reopened sections (comment 3). This is left as it is, apart from the docstring. git orders sections by the last path key, mounted_from by a name's first key, and siblings by its first url, so the three can name different sections. mounted_from's docstring now says so; it overstated agreement for anything but sections written whole.

What changed

In shape-doctor.py:

  • submodule_keys (:5010-5057), with DOTDOT_COMPONENT (:5000) and VALUED_KEYS (:5007). It keeps only what git reads, by git's own tests:
    • a section whose name is empty or holds a .. component (between / or \) is skipped whole;
    • a path or url value beginning with - is skipped, that key alone;
    • a bare key of VALUED_KEYS in a section git reads returns None, meaning git refuses the file.
  • section_url (:4966-4994) gives (None, None) for a refused file, and submodule_config (:7832-7885) gives {}.
  • gitmodules_entry (:4908-4963) returns (None, None) while git ls-files -u -- .gitmodules lists anything, before it chooses a copy.
  • mounted_from (:7888-7941) no longer reads a section's NAME as its path, and its docstring is softened for reopened sections.

In templates/family-root/scripts/siblings.py:

  • submodule_urls (:237-284), with DOTDOT_COMPONENT (:224) and VALUED_KEYS (:229). It applies the same rules, returns {} for a refused file, and keys a url by its section's path alone.
  • The constants are copied rather than imported from scripts/repo_shape.py. A holder may keep an older repo_shape.py, and siblings.py imports no new name from it. The test below pins the two copies to git and to each other.

Tests

Commands on head 08ebe8bf, each run with a stub gh (exits 97) first on PATH, GH_TOKEN=bogus and an empty GH_CONFIG_DIR, from tests/:

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt -o tmp_path_retention_policy=failed test_doctor_gitmodules_not_utf8.py test_doctor_legs_members_next.py test_doctor_members_spelled.py test_doctor_gitlink_repairs.py test_repo_hygiene.py test_cli_reference.py
312 passed in 220.94s (0:03:40)

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-sib -o tmp_path_retention_policy=failed test_family_siblings_placement.py test_siblings_family_name.py test_family_makefile_siblings.py test_windows_paths.py test_park_resume_targets.py test_family_printed_quoting.py
384 passed in 56.26s

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-others -o tmp_path_retention_policy=failed test_family.py test_doctor_next_is_a_command.py test_doctor_validator_rows.py
212 passed in 120.73s (0:02:00)

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-sd -o tmp_path_retention_policy=failed test_shape_doctor.py
436 passed, 3 skipped, 1 xfailed in 348.65s (0:05:48)

The skips and the xfail are in test_shape_doctor.py, which this PR does not change.

Pinned shape copies

templates/family-root/scripts/siblings.py is a pinned family-root copy (FAMILY_COPIED_VERBATIM). Every family holder gets an upstream-changed scripts/siblings.py row from update-shape.py check until it applies the update. shape-doctor.py is not a pinned copy.

The other half: lane openRepoShape-4's #690

Neighbours

This stays in the .gitmodules readers #700 added or changed, and in siblings.py::submodule_urls. It does not touch pin_restore (#736), the partial-clone row (#754) or the lockstep NEXT (#750).

Round 2 (head d9b15660)

RV-c's review (issuecomment-6229523010) found it NOT READY. This round first merges #774's new head 93b9bac3 (3df0f99a), then takes the MUST, SHOULDs 1-3 and the NIT.

  • MUST: an index or HEAD copy keeps the sections before its first bad line. git reads the working tree's file with CONFIG_ERROR_DIE, which refuses it whole, and an index or HEAD copy with CONFIG_ERROR_ERROR, which stops at the bad line and keeps what came before.

    • submodule_keys(listing, working_tree) (shape-doctor.py:5058-5118) returns None at a bare VALUED_KEYS key for the working tree, and the keys read so far for an index or HEAD copy.
    • gitmodules_entry (:4908-4967) threads which copy it read through section_url (:4970-5000).
    • submodule_config and siblings.py read only the working tree, as before.
    • Measured on git 2.43: p's section, then a bare url, in an index copy registers p; with the bare url first it registers none. The doctor now gives the same.
  • SHOULD 1: values git dies on refuse the file in any copy. git_dies_on (:5041-5055) and git_bool (:5026-5038), with GIT_BOOL_WORDS and GIT_INT (:5017-5023), refuse:

    • an update other than exactly none, checkout, rebase or merge. !echo, Merge and an empty value each die (measured);
    • a shallow that is no boolean. The test for an integer follows git_parse_int: base-0 strtoimax, a k/m/g unit, and it must fit an int. Measured: 2k, 0x10, TRUE and an empty value are taken; maybe, 08, 2g and 99999999999 die;
    • a fetchRecurseSubmodules that is neither a boolean nor on-demand.

    A die-value inside an ignored section is fine, and so is one past the bad line of an index copy, which git never reads. siblings.py gets the same check: git_dies_on (:244-263), and GIT_BOOL_WORDS and GIT_INT (:234-236), used in submodule_urls (:266-314).

  • SHOULD 2: a blob's partial listing is kept. gitmodules_listing(..., partial=True) (:5121-5157) keeps what git config -z --blob ... --list prints before a line it cannot parse, then exits 128. It is used for an index or HEAD copy known to exist. Measured: p, then an unparsable line, in an index copy registers p.

  • SHOULD 3: one version difference, a bare branch.

    • git 2.43 crashes on it (xstrdup(NULL), exit 139), in any copy.
    • git 2.55 (CI), per RV-c's reading of git's source, refuses it like any other bare valued key: exit 128 for the working tree, and for an index or HEAD copy it stops there and keeps earlier sections. The doctor follows 2.55.
    • url_git_finds (tests/test_doctor_legs_members_next.py:1576) skips, with the reason, when git crashes. The doctor's answer is still asserted first. So the two bare-branch rows run on 2.55 and skip on 2.43. I measured them on 2.43 only; 2.55 is not installed here.
  • Tests.

    • INDEX_COPIES (:1507-1532) gains six index-copy rows: a bare url after p and before it, an unparsable line after p, a die-value, a die-value past a bare url, and a bare branch after p.
    • GIT_SKIPS (:1643-1694) gains nine working-tree value rows, with controls.
    • Every row is checked against git submodule init.
    • Against round 1's head 08ebe8bf, 10 of the new rows fail: five index-copy rows and five die-value rows.
  • NIT: the siblings.py (make siblings): the printed mv goes astray over an existing .holder/.folder or a dangling link, a lone surrogate or NUL in project: is a traceback, a url with NEL or U+2028 is cut, two warnings print paths raw #690 paragraph above now says shape-doctor.py and siblings.py: a .gitmodules section git ignores ([submodule ""], a .. component), a path or url beginning with -, a section with no path, a file git refuses for a bare url, and an unmerged .gitmodules name no mount url, NEXT line or clone, where git says No url found and exits 128 (#733) #775's rules go into siblings.py (make siblings): the printed mv goes astray over an existing .holder/.folder or a dangling link, a lone surrogate or NUL in project: is a traceback, a url with NEL or U+2028 is cut, two warnings print paths raw #690's -z loop, which returns bytes, rather than replacing it.

  • Unchanged from round 1: the PR's other behaviour, and siblings.py being a pinned family-root copy (upstream-changed for each holder).

On d9b15660, under a stub gh, GH_TOKEN=bogus and an empty GH_CONFIG_DIR, from tests/:

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt -o tmp_path_retention_policy=failed test_doctor_gitmodules_not_utf8.py test_doctor_legs_members_next.py test_doctor_members_spelled.py test_doctor_gitlink_repairs.py test_repo_hygiene.py test_cli_reference.py
325 passed, 2 skipped in 218.80s (0:03:38)   [the 2 skips: the bare-`branch` rows, git 2.43 crashing]

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-sib -o tmp_path_retention_policy=failed test_family_siblings_placement.py test_siblings_family_name.py test_family_makefile_siblings.py test_windows_paths.py test_park_resume_targets.py test_family_printed_quoting.py test_family.py
490 passed in 105.52s (0:01:45)

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-sd -o tmp_path_retention_policy=failed test_shape_doctor.py
436 passed, 3 skipped, 1 xfailed in 339.90s (0:05:39)

Round 3 (head 3fa5beed)

RV-c's re-verification of round 2 is READY. This round takes its SHOULD and two NITs. In shape-doctor.py it changes only GIT_INT and git_bool.

  • SHOULD: an integer after white space is a boolean. git_bool (shape-doctor.py:5027-5047) and siblings.py's git_dies_on (:245-264) now match GIT_INT against value.lstrip(" \t\n\v\f\r"), as strtoimax skips that white space.
    • Measured on git 2.43: shallow = " 1" and shallow = "\t1" register p, where both tools said git refuses the file.
    • The words are still compared unstripped, as strcasecmp compares them, so " true" dies (measured).
  • NIT: the negative limit follows git 2.55, which CI runs, as the bare branch rule does. The range is -231 to 231 - 1, and both docstrings say so. GIT_INT (shape-doctor.py:5023, siblings.py:236) now captures the sign.
    • git 2.43 stops one short (-max / factor > val), so -2147483648, -2097152k, -2048m and -2g die there (measured). Per RV-c's reading of git's source, git 2.55 takes them.
    • test_733_a_shallow_of_int_min_is_a_boolean_as_git_2_55_reads_it (tests/test_doctor_legs_members_next.py:1729-1746) asserts the doctor and siblings take all four, then asks the git at hand. A git that dies on them skips, with the reason.
    • -2147483647 (taken) and -2147483649 (dies) agree in both versions, so they are plain GIT_SKIPS rows.
  • NIT: mutant MB is killed. GIT_SKIPS (:1647-1708, the value rows from :1682) gains seven rows, each checked against git submodule init: 08 (dies), 0x10 (taken), " 1" and "\t1" (taken), " true" (dies), -2147483647 (taken), -2147483649 (dies). The p-then-a-line bytes are one constant, P_THEN_A_LINE (:1638).
    • MB here means GIT_INT takes any digits, read as decimal, so 08 is a boolean. Note that only changing the octal base leaves 08 refused by the regex.
    • In shape-doctor.py and in siblings.py, MB fails shallow-08-no-octal (1 failed, 41 passed, 6 skipped). On round 2's tests it survives (35 passed, 2 skipped).
    • Also killed: dropping the lstrip (2 rows), and 2.43's range in the doctor (the 4 int-min rows).

On 3fa5beed, under a stub gh, GH_TOKEN=bogus and an empty GH_CONFIG_DIR, from tests/:

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt -o tmp_path_retention_policy=failed test_doctor_gitmodules_not_utf8.py test_doctor_legs_members_next.py test_doctor_members_spelled.py test_doctor_gitlink_repairs.py test_repo_hygiene.py test_cli_reference.py
332 passed, 6 skipped in 183.25s (0:03:03)   [2 bare-`branch` rows and 4 int-min rows skip on this git 2.43]

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-sib -o tmp_path_retention_policy=failed test_family_siblings_placement.py test_siblings_family_name.py test_family_makefile_siblings.py test_windows_paths.py test_park_resume_targets.py test_family_printed_quoting.py test_family.py
490 passed in 92.86s (0:01:32)

tests/test_shape_doctor.py was not re-run, as the coordinator allowed: shape-doctor.py changed only in GIT_INT and git_bool. Round 2's run on d9b15660 gave 436 passed, 3 skipped, 1 xfailed.

Round 4: main 40e678e6 merged (#805)

Lane 4's #805 (closes #690) landed on main at 2026-10-17T15:52:45Z as 40e678e6. This round merges it (32048c4f), as "The other half" above planned. Nothing else changes. This PR still lands after #774 and after the v1.1.0 tag.

The loop as merged (siblings.py:316-333):

    for record in proc.stdout.decode("utf-8", "replace").split("\0"):
        # `valued` is the newline: a key given no value at all has none.
        key, valued, value = record.partition("\n")
        section, _, field = key.rpartition(".")
        if not section.startswith("submodule."):
            continue
        name = section[len("submodule."):]
        if not name or DOTDOT_COMPONENT.search(name):
            continue
        if (not valued and field in VALUED_KEYS) or git_dies_on(field, value):
            return {}
        if value.startswith("-"):
            continue
        if field == "path":
            paths[name] = value
        elif field == "url":
            urls[name] = value
    return {paths[name]: url for name, url in urls.items() if name in paths}

On 32048c4f, under a stub gh (exits 97), GH_TOKEN=bogus and an empty GH_CONFIG_DIR, from tests/:

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-A -o tmp_path_retention_policy=failed test_siblings_inputs.py test_doctor_gitmodules_not_utf8.py test_doctor_legs_members_next.py test_family_siblings_placement.py
285 passed, 6 skipped in 522.43s (0:08:42)   [2 bare-`branch` rows and 4 int-min rows skip on this git 2.43]

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-B -o tmp_path_retention_policy=failed test_family.py test_family_makefile_siblings.py test_family_printed_quoting.py test_park_resume_targets.py test_siblings_family_name.py test_windows_paths.py test_pins_child_kind_and_siblings_root_spelling.py
471 passed in 321.05s (0:05:21)

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-C -o tmp_path_retention_policy=failed test_update_shape_kept_helper.py test_validator_child_encoding.py test_update_shape_add.py test_repo_hygiene.py test_cli_reference.py test_doctor_members_spelled.py test_setup_family_folder.py
309 passed in 199.06s (0:03:19)

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-D -o tmp_path_retention_policy=failed test_shape_doctor.py
436 passed, 3 skipped, 1 xfailed in 613.21s (0:10:13)

Mutants. Each mutant changes the merged submodule_urls. Each runs against test_siblings_inputs.py and this PR's two test_733_* row tests in test_doctor_legs_members_next.py: 87 tests, of which 5 skip on git 2.43. All six are killed:

Pins. No file in this repository records a blob, digest or line count of siblings.py. git grep for its blob and sha256 prefixes, before the merge and after it, finds none. The file is still a pinned family-root copy, so each holder takes it as one upstream-changed row. It is now blob fb53c1c8, sha256 855af9a2…, 1461 lines.

#790 (lane 3, #786), checked here and not committed. A scratch worktree of 32048c4f had #790's tests/test_validate_workflow_gitmodules_urls.py (head 31c3792c) overlaid, and the "\r" exclusion removed from test_siblings_reads_what_each_row_says.

Main has since moved to 6626a3c5 (#747). That commit touches none of this PR's files, and git merge-tree of it with 32048c4f is clean.

Round 5: RV775r4's two SHOULDs and three NITs

RV775r4's review of round 4 (32048c4f, issuecomment-6237014956) is READY. This round takes all five of its items in one commit, 8967b73d. It then merges main 64ec47c3 (#747, #817) cleanly as 44f0bf10, the head.

On 44f0bf10, under a stub gh (exits 97), GH_TOKEN=bogus and an empty GH_CONFIG_DIR, from tests/:

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-A -o tmp_path_retention_policy=failed test_siblings_inputs.py test_doctor_gitmodules_not_utf8.py test_doctor_legs_members_next.py test_family_siblings_placement.py
291 passed, 6 skipped in 310.94s (0:05:10)   [2 bare-`branch` rows and 4 int-min rows skip on this git 2.43]

Mutants. Each is in siblings.py unless named. Each runs against test_siblings_inputs.py, test_doctor_gitmodules_not_utf8.py and the two test_733_* row tests: 121 tests, of which 5 skip on git 2.43. All five are killed:

  • MB, not valued → not value: 2 failed (empty-ignore, empty-branch). It survived round 4 (RV775r4: 110 passed).
  • MB in the doctor's submodule_keys: killed by the same 2 rows.
  • MD, the - skip moved before the refusal: 2 failed (update-dash, shallow-below-int-min). Round 4 had only the second.
  • MR, the listing decoded with "replace" again (round 4's reading): 3 failed, the three new collision rows.
  • MV, the value not re-decoded: 2 failed, both in test_doctor_gitmodules_not_utf8.py: test_submodule_config_keeps_the_url_as_text_it_can_compare[not-utf-8] and test_a_clone_made_from_the_same_bytes_is_the_working_clone.

Pins. No file in this repository records a blob, digest or line count of siblings.py. It is now blob 149c752a, sha256 5cceb2dd…, 1468 lines.

Main has moved again, to 431c8cf8 (#748, #662), at 17:00:58Z, after this round's merge. I did not merge it, because its merge with 44f0bf10 is not clean:

Round 6: #774's new head and main merged

#774's head moved to 012a7726, a merge of main bad05d21 that resolved the import block of tests/test_doctor_legs_members_next.py (#748's two-line conftest import, then listed_locale). Main has moved on to fc867207 (#672, #746). This round merges both into this branch. Nothing else changes: this PR is still a draft, and still lands after #774 and after the v1.1.0 tag.

On 8c5ad0e8, under a stub gh (exits 97), GH_TOKEN=bogus and an empty GH_CONFIG_DIR:

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt -o tmp_path_retention_policy=failed tests/test_siblings_inputs.py tests/test_doctor_gitmodules_not_utf8.py tests/test_doctor_legs_members_next.py tests/test_family_siblings_placement.py
291 passed, 6 skipped in 307.75s (0:05:07)   [the same count as round 5's `44f0bf10`; the 6 are the same 2 bare-`branch` rows and 4 int-min rows that skip on git 2.43]

tests/test_shape_doctor.py was not run on this head. It was run once on #774's 012a7726: 436 passed, 3 skipped, 1 xfailed.

🤖 Generated with Claude Code

Summary by Sourcery

Match .gitmodules readers to Git’s parsing and submodule URL resolution behavior across malformed, ignored, unmerged, and encoding-sensitive configurations.

Bug Fixes:

  • Align .gitmodules parsing in the doctor and make siblings with Git’s handling of ignored sections, invalid values, malformed entries, and unmerged files.
  • Prevent submodule URLs from being inferred for sections without a path or from incorrectly merged non-UTF-8 section names.

Enhancements:

  • Handle Git’s partial parsing of index and HEAD .gitmodules copies while preserving entries read before a malformed line.
  • Add comprehensive parity coverage against Git for .gitmodules edge cases, including boolean and integer value validation and byte-distinct section names.

Tests:

  • Expand .gitmodules tests to cover Git-skipped sections, refused files, unmerged copies, partial index/HEAD reads, and encoding-sensitive section names.

Brett Heap and others added 2 commits October 17, 2026 00:55
…nd is handed back to git as them, so `configured_url` and `kept_gitdir` find a member named `caf\xe9`, its detached commit gets its keep line under any locale, and a non-ASCII name no longer ends `--root` in a UnicodeEncodeError traceback under `LC_ALL=C PYTHONUTF8=0` (#732)

- `gitmodules_listing` decodes `git config -z --list` with
  `surrogateescape`; `submodule_keys` keeps each section NAME as git's
  bytes and gives each VALUE with the replacement, as before (#432, #385).
- `git_bytes` hands such a name back to git as bytes on POSIX, and as text
  on Windows. `configured_url` and `kept_gitdir` ask git with it, and
  `kept_gitdir` reads git's answer as a path (`os.fsdecode`).
- Tests: a holder whose member's submodule is NAMED in Latin-1 or in
  UTF-8, under a UTF-8 locale, `LC_ALL=C` and `LC_ALL=C PYTHONUTF8=0`: the
  emptied member's detached commit gets its keep line, and a section that
  lost its url finds the one the holder's config keeps.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…(`[submodule ""]`, a `..` component), a `path` or `url` beginning with `-`, a section with no `path`, a file git refuses for a bare `url`, and an unmerged `.gitmodules` name no mount url, NEXT line or clone, where git says `No url found` and exits 128 (#733)

- `submodule_keys` keeps only what git reads, by git's own tests: a name
  that is empty or holds a `..` component is ignored whole
  (`DOTDOT_COMPONENT`), a `path` or `url` value beginning with `-` is
  ignored, and a bare `path`, `url`, `ignore`, `update` or `branch` key
  refuses the whole file (`VALUED_KEYS`): None, so `section_url` and
  `submodule_config` read no section.
- `gitmodules_entry` reads no copy while `.gitmodules` is unmerged.
- `mounted_from` no longer reads a section's name as its path, and its
  docstring says a reopened section can be ordered three ways.
- `siblings.py::submodule_urls` (a pinned family-root copy) applies the
  same rules and keys a url by its section's `path` alone.
- Tests: a corpus of 17 `.gitmodules` files, each checked against `git
  submodule init`, through `gitmodules_entry`, `mounted_from` and
  `siblings.submodule_urls`; an unmerged `.gitmodules`, with the working
  tree copy deleted and resolved.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sourcery-ai

sourcery-ai Bot commented Oct 17, 2026

Copy link
Copy Markdown

Reviewer's Guide

Updates the doctor and family siblings.py readers to reproduce Git’s .gitmodules semantics, including ignored or malformed configuration, unmerged files, explicit mount paths, and byte-preserving section names, with broad parity and regression tests.

Sequence diagram for Git-compatible submodule URL lookup

sequenceDiagram
    participant Caller
    participant Doctor as shape-doctor.py
    participant Siblings as siblings.py
    participant Git
    Caller->>Doctor: gitmodules_entry(root, rel)
    Doctor->>Git: ls-files -u -- .gitmodules
    Git-->>Doctor: unmerged status
    alt .gitmodules is unmerged
        Doctor-->>Caller: (None, None)
    else file is readable
        Doctor->>Git: config -z --file .gitmodules --list
        Git-->>Doctor: section key/value records
        Doctor->>Doctor: submodule_keys(listing)
        Doctor-->>Caller: matching path and url
    end
    Caller->>Siblings: submodule_urls(root)
    Siblings->>Git: config --file .gitmodules --list
    Git-->>Siblings: section key/value records
    Siblings->>Siblings: skip ignored sections and keys
    Siblings-->>Caller: urls keyed by explicit path
Loading

Entity relationship diagram for .gitmodules mount mapping

erDiagram
    GITMODULES_SECTION {
        string name
        string path
        string url
    }
    MOUNTED_SUBMODULE {
        string path
        string url
    }
    GITMODULES_SECTION ||--o| MOUNTED_SUBMODULE : "maps only when path exists"
Loading

Flow diagram for Git-compatible .gitmodules parsing

flowchart TD
    A[".gitmodules"] --> B["gitmodules_listing / config listing"]
    B --> C{".gitmodules unmerged?"}
    C -->|yes| D["gitmodules_entry returns (None, None)"]
    C -->|no| E["submodule_keys / submodule_urls"]
    E --> F{"Git ignores section or key?"}
    F -->|section name empty or .. component| G["skip section"]
    F -->|path or url starts with -| H["skip key"]
    F -->|bare valued key| I["refuse whole file"]
    F -->|valid entry| J["retain path and url"]
    I --> K["submodule_config returns {} / submodule_urls returns {}"]
    J --> L["match url to explicit path"]
    L --> M["doctor and siblings agree with Git"]
Loading

File-Level Changes

Change Details Files
Make .gitmodules parsing match Git’s handling of ignored sections, ignored option-like values, malformed valued keys, and non-UTF-8 section names.
  • Skip empty or ..-component section names and path/url values beginning with -.
  • Return a refused-file signal when valued keys are bare, and propagate it as no configuration.
  • Preserve section-name bytes with surrogateescape and pass them back to Git safely across locales and platforms.
shape-doctor.py
Align sibling URL discovery with the doctor and Git by requiring a valid path and mirroring Git’s filtering rules.
  • Reject ignored sections, option-like values, and whole-file parse failures.
  • Map URLs only through sections that explicitly provide a path.
  • Keep the copied parser self-contained for older family holders.
templates/family-root/scripts/siblings.py
Handle unmerged .gitmodules files as unreadable and add regression coverage for Git parity and byte-preserving names.
  • Return no entry while .gitmodules has unmerged index stages, regardless of the working-tree copy.
  • Exercise ignored sections, duplicate/option-like keys, missing paths, bare keys, and controls against git submodule init.
  • Add locale and non-UTF-8 section-name coverage for Git lookups and repository retention.
tests/test_doctor_legs_members_next.py
tests/test_doctor_gitmodules_not_utf8.py

Assessment against linked issues

Issue Objective Addressed Explanation
#733 Make the doctor and make siblings read .gitmodules according to Git’s behavior: ignore empty or ..-component section names and --prefixed path/url values, treat files Git rejects for bare valued keys as having no entries, and do not treat a section name as its mount path when path is absent. ✅
#733 Make the doctor ignore .gitmodules copies while the file is unmerged, matching Git’s behavior. ✅
#733 Pin the relevant Git-parity behavior with tests covering both tools and the issue’s cases. ✅

Possibly linked issues


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

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T04:15:39Z) Independent review of #775 by RV-c (opus), on head 08ebe8bf: NOT READY. There is one MUST: a regression against main on an index or HEAD copy. Three SHOULDs and one NIT. RV-c measured on git 2.43 and diffed git's source between v2.43.0 and v2.55.0 (CI's git).

MUST: a bare valued key in an index or HEAD copy now hides every section before it.

  • Where: shape-doctor.py:5051 (if not valued and field in VALUED_KEYS: return None), as used by gitmodules_entry (:4959).
  • Scenario: the working-tree .gitmodules is deleted, so git reads the index or HEAD copy. That copy has p's section, then a section with a bare url or path.
  • What happens:
  • Why: git reads a working-tree file with CONFIG_ERROR_DIE, but an index or HEAD copy with CONFIG_ERROR_ERROR. It stops at the bad line and keeps what it read before it. This is identical at v2.43.0 and v2.55.0 (config.c:1393/1448). So "git refuses the whole file" holds for the working-tree copy only.
  • Fix:
    • Let submodule_keys know which copy it reads. A --file copy keeps returning None at the first bare valued key; an index or HEAD copy returns the keys read before it.
    • Thread that through section_url from gitmodules_entry. submodule_config and siblings.py read only the working tree.
    • Add index-copy rows checked against git submodule init: a bare key after p gives p's url; a bare key before p gives none.

SHOULDs:

  1. Values git dies on are not caught. These make git exit 128 for every path, in the working-tree file and in an index copy alike, because each calls die() in both versions:

    • update other than none, checkout, rebase or merge (including !command);
    • a shallow that is not boolean;
    • a fetchRecurseSubmodules that is neither boolean nor on-demand.

    The doctor and siblings still name /r/p.git. Fold these in (None for any copy), or file them before shape-doctor.py and siblings.py: a .gitmodules section git ignores ([submodule ""], a name with a .. component, a url beginning with -) is still read as a member, where git says No url found and exits 128 #733 closes.

  2. Already on main, from shape-doctor.py: submodule_config and gitmodules_entry read a holder's .gitmodules through one git config -z --list, so a section name holding a space or a byte that is not UTF-8, or a url holding NEL or U+2028, gives the doctor the mount url git and make siblings use, under any locale (#668) #700: in an unparsable index copy whose bad line comes after p, git reads p but the doctor gives (None, None). git config -z --blob :.gitmodules --list prints the partial listing and then exits 128, and git_out throws that output away. For an index or HEAD copy known to exist, use it. This fits the MUST fix.

  3. Name the one version difference in the body: a bare branch.

    • git 2.43: xstrdup(NULL) crashes, exit 139.
    • git 2.55: config_error_nonbool gives exit 128 for the working tree, and an error that keeps earlier sections for an index or HEAD copy.

    Everything else GIT_SKIPS relies on is identical in both versions, and CI (2.55.0) passed all 22 rows. The new index-copy rows will need branch handled per version.

NIT: the #690 conflict. Lane 4's fix/690-siblings-inputs (38a40abf) conflicts in siblings.py in two hunks, the docstring and the loop. #690's -z call returns bytes, so taking #775's loop as it stands would raise TypeError (line.partition("=") on bytes) and break make siblings. Put #775's rules into #690's -z loop, as the body says.

Verified against git:

Mutants: all killed:

  • drop .. (4 rows);
  • -url valid (2);
  • read an unmerged .gitmodules (2);
  • siblings keys a path-less section by its name (1);
  • .. dropped in siblings only (4).

No test covers the MUST, because no row reads an index or HEAD copy.

Tests: main + #775:

Merges: clean with bf1f099b and every open lane-3 v1.1.1 PR. It conflicts with #690's WIP (NIT).

Lane: openreposhape-3 (openRepoShape-3)

… is not UTF-8 is still not found, and names the lookup that would find it (#732, review round 2)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brett Heap and others added 2 commits October 17, 2026 04:19
…s` is read up to its first bad line and keeps the sections before it, as git reads one, and a value git dies on (`update = !cmd`, a `shallow` or `fetchRecurseSubmodules` that is no boolean) refuses the file in any copy (#733, review round 2)

- MUST: `submodule_keys(listing, working_tree)` refuses the working tree's
  file whole at a bare valued key (`CONFIG_ERROR_DIE`), and for an index
  or HEAD copy keeps the keys before it (`CONFIG_ERROR_ERROR`);
  `gitmodules_entry` threads the copy through `section_url`.
- SHOULD 2: `gitmodules_listing(..., partial=True)` keeps the listing
  `git config --blob ... --list` prints before a line it cannot parse.
- SHOULD 1: `git_dies_on` (with `git_bool`, `GIT_BOOL_WORDS`, `GIT_INT`)
  refuses a file over an `update` other than none/checkout/rebase/merge,
  a `shallow` that is no boolean, or a `fetchRecurseSubmodules` that is
  neither a boolean nor `on-demand`, in the doctor and in `siblings.py`.
- Tests: six index-copy rows in INDEX_COPIES and nine value rows in
  GIT_SKIPS, each checked against `git submodule init`; a git that
  crashes (2.43 on a bare `branch`) skips with the reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T04:47:25Z) RV-c's re-verification of #775 round 2 (d9b15660): READY. The MUST is fixed, measured against git in both orders. SHOULD 1 and 2 hold on 56 of 58 cases against git 2.43. One new SHOULD and two NITs, none blocking; lane 3 takes them in a small round 3.

The MUST (an index copy, no working-tree file, against git submodule init -- p):

case git round 1 round 2
bare url after p /r/p.git none /r/p.git
bare url before p exit 128 none none
bare path after p /r/p.git none /r/p.git
bare update after p /r/p.git none /r/p.git
bare url inside p, after its url /r/p.git none /r/p.git

The same bare url in the working-tree file is still refused whole, as git dies on it.

SHOULD 1 holds:

  • update: !x and Checkout die; none and merge pass.
  • fetchRecurseSubmodules: ON-DEMAND, bogus and 08 die; on-demand, 2k and yes pass.
  • shallow: 25 values all match git.
  • The order in an index copy is followed.
  • siblings.py matches the doctor on every working-tree row.

SHOULD 2 never keeps a record git would not. It agreed with git on all nine broken-line cases: an unparsable line before or after p, = nokey inside p, continuations, an unterminated quote, a bad escape, and a bare key followed by an unparsable line. --list and the submodule reader share the parser and its stop-on-error rule for an index or HEAD copy.

SHOULD 3 holds. The two bare-branch rows check the doctor's own answer, then skip on 2.43 ("exit 139"). Mutant MA shows the doctor side runs.

Not blocking:

  1. SHOULD (new): a quoted value with leading whitespace is wrongly refused.
    • With shallow = " 1" or "\t1", git registers p (strtoimax skips leading whitespace). The doctor and siblings answer "git refuses the file".
    • Fix, in git_bool and siblings.py's git_dies_on: GIT_INT.fullmatch(value.lstrip(" \t\n\v\f\r")). Keep the word compare on the raw value.
  2. NIT: the negative limit differs by version.
    • git 2.43 checks -max / factor > val.
    • git 2.55 checks (-max - 1) / factor > val, so it accepts -2147483648, -2097152k, -2048m and -2g.
    • The code follows 2.43, while the bare-branch rule follows 2.55. Pick one policy.
  3. NIT: mutant MB (git_bool accepts 08) survives. Add shallow = 08 (dies) and shallow = 0x10 (accepted).

Mutants:

mutant result
MA working_tree ignored 3 killed
MB survives (NIT 3)
MC partial listing dropped 1 killed
MD siblings never sees a die value 5 killed

Tests (main + #775):

  • 8 doctor files: 431 passed, 2 skipped;
  • 7 siblings and family files: 490 passed;
  • tests/test_shape_doctor.py: 436 passed, 3 skipped, 1 xfailed.

git 2.55 from source: git_parse_maybe_bool_text, git_parse_int, get_unit_factor and git_config_bool are identical to 2.43, apart from NIT 2.

Merges:

Lane: openreposhape-3 (openRepoShape-3)

…es` integer after white space is a boolean, as `strtoimax` reads it, and -2**31 is within range, as git 2.55 checks it (#733, review round 3)

- SHOULD: `git_bool` and `siblings.py`'s `git_dies_on` match `GIT_INT`
  after `lstrip(" \t\n\v\f\r")`, so `" 1"` and `"\t1"` are taken as git
  takes them; the words are compared unstripped, as `strcasecmp` does.
- NIT: the range is git 2.55's, -2**31 to 2**31 - 1, the policy the bare
  `branch` rule follows; `GIT_INT` captures the sign.
- NIT: rows for `08` (dies) and `0x10` (taken) kill mutant MB, with rows
  for a leading space and tab, a word after a space, and both sides of
  -2**31 git 2.43 and 2.55 agree on; -2**31 itself in four spellings is
  asserted for the doctor and `siblings` and skips where git dies on it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T04:59:04Z) RV-c's re-verification of #775 round 3 (3fa5beed): READY. One optional NIT. Linux tests was pending at 04:59Z; Sonar, package and tests-git-bash pass.

Leading whitespace is fixed. shallow = " 1" and "\t1" now register p in the doctor and siblings alike, against git submodule init. " true" still dies, as in git.

Mutant MB is killed. Widening GIT_INT to accept 08 in either file fails [shallow-08-no-octal]. The head's selection gives 42 passed, 6 skipped: two bare-branch rows and four range rows only git 2.55 accepts.

The 2.55 range is exact at both edges, with and without a unit.

  • From source: git_parse_signed takes val·f < 2^31 and val·f ≥ −2^31. The check is -2**31 <= ±scaled < 2**31, with the sign read as strtoimax reads it.
  • Measured on 2.43, which shares the positive bound:
    • 2147483647, 0x7fffffff, 2047m and 2097151k are taken;
    • 2147483648, 0x80000000, 2048m and 2097152k die;
    • -2147483649, -2049m and -2097153k die.
  • Only five values differ by version: -2147483648, -0x80000000, -2097152k, -2048m and -2g.

The version-skip test checks the doctor's answer and siblings' url before asking git.

  • NIT (optional): a git that dies on these values always skips, so a 2.55 that rejected them would pass silently. On git 2.55 or later, assert that git registers /r/p.git instead of skipping.

Merges: clean with bf1f099b and with #774 (93b9bac3, an ancestor).

Lane: openreposhape-3 (openRepoShape-3)

brettheap pushed a commit that referenced this pull request Oct 17, 2026
…alue is ignored alone as git ignores it, a refused file is named with its key or line before any credential advice, and a `.gitmodules` git refuses fails the run whatever the credential, which README, handbook and the template READMEs now say (#786)

Round 2 of #790, from RV-c's review.

- SHOULD 1: the `url` and `path` reads pass git a value pattern,
  `--get -- "$key" '^([^-]|$)'`, so a value beginning with `-` is ignored and
  the earlier one stands, as git, the doctor and `siblings.py` have it
  (`url = A` then `url = -x` is A). The round's reason, that `--get` returns
  one whole value, held for the line-break skip and not against a pattern.
  The `-` cases that followed the reads are gone.
- SHOULD 2: a file git cannot parse prints git's own `fatal:` text (`bad
  config line 5 in file .gitmodules`) and every other cause prints the key it
  stops at, both escaped as workflow-command data (`%` and CR); both say `git
  submodule status` names the key and line git stops at; "an ignore key" and
  "an update key"; and the fetch step refuses BEFORE it gives its App advice,
  which a refused file never gets. Printed with `printf '%s\n'`, which dash
  does not read backslashes in as `echo` does.
- SHOULD 3: README.md, docs/handbook.html (its markers recomputed), both
  template READMEs and the two failing-step comments say a `.gitmodules` git
  refuses fails the run whatever the credential. README.md grows a line, so
  tests/readme-cap.d/786.txt.
- N1: rows for no `.gitmodules` at all, `On-Demand` (git dies on it), and a
  bare `ignore` in a section named `..\x`; the names step's `[ -f ]` guard, the
  case-sensitive `on-demand` and the refusal's `..` test at `\` are held by
  them.
- N3: the failing step's refusal block is gone, since the fetch step exits 1
  on a file git refuses and this step is never reached with one.
- The two printing steps list a key once, `awk '!seen[$0]++'`: `--get-regexp`
  lists a key once per value, and git registers one url for it.

tests/test_validate_workflow_gitmodules_urls.py: 27 rows (21 and six), each
checked against `git submodule init`, `siblings.submodule_urls` of #775's head
3fa5bee and the steps; the cause test asserts the key or line; the fetch step
gives no advice for a refused file and gives it before the list otherwise.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
#805 (#690) rewrote `siblings.py::submodule_urls` to read `git config -z
--list` bytes, decoded with "replace", records cut at NUL and each record
split at its FIRST newline, a section's name being all of its key between
`submodule.` and the last dot. #775 (#733) applied its rules in the old
`splitlines` + first `=` loop. They conflicted in that function's
docstring and its loop, and nowhere else.

The loop is #805's, with #775's rules carried into it in the order the
doctor's `submodule_keys` applies them to the same listing:

- an empty name, or one with a `..` component (`DOTDOT_COMPONENT`),
  skips the section;
- a key of `VALUED_KEYS` given with no value at all, or a value
  `git_dies_on`, refuses the whole file (`{}`). In `-z` output a bare key
  is a record with NO newline (`submodule.x.url\0`, measured on git
  2.43) and `url =` is one with a newline and an empty value, so
  "valued" is whether `record.partition("\n")` found its separator;
- a `path` or `url` beginning with `-` is ignored, that key alone;
- a url is keyed by its section's `path` alone (#775's return line,
  which merged cleanly).

Everything else is #805's: the later value wins, a name with a space,
dot, quote or `=` is read whole, and a byte that is not UTF-8 is
replaced (#432). The docstring keeps both paragraphs, #690's on how the
listing is read and then #733's on which of it git reads.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T16:51:18Z) Independent review of #775's round 4 (head 32048c4f, main 40e678e6 merged) by lane 3's RV775r4 (opus): READY.

  • No MUST. Two cheap SHOULDs, each with a measured fix, and three NITs. Lane 3 takes all five in round 5.
  • The merge resolution is correct, and each side keeps its own behaviour.
  • Combined, the two sides fix rows that round 3's head got wrong:
    • a bare url in [submodule "a=b"] now refuses the whole file, as git does;
    • a url holding CR, LF, NEL or U+2028 is read whole;
    • a .. name only the -z reading sees whole is skipped;
    • update = "rebase\n" dies, as in git.
  • The head did not move during the review. Main moved to 64ec47c3, and the merge with it is clean.

The resolution, against real git 2.43 (40 rows). Each row compares git config -z --list -f, git submodule init, the merged submodule_urls and the doctor's readers. All 40 agree, except three non-UTF-8 rows (SHOULD 1).

  • Only siblings.py was resolved by hand; every other file in the merge is git's own auto-merge.
  • A bare url lists with no newline, and git exits 128 (bad config line). The merged code gives {}; main gave {'p': ''}.
  • url = lists with a newline and an empty value, and git registers ''. The merged code gives {'p': ''}.

Doctor parity over 129 files (both suites' rows, #790's RULES, DUPLICATES and holder corpus, and the 40 rows above):

  • The maps are equal in 126. The exceptions are the 3 non-UTF-8 name rows (SHOULD 1).
  • The mounts are equal in 124. The difference is the 2 empty-url = rows, the known difference the body states, and the 4 int-min rows, where git 2.43 dies and both tools follow 2.55 by design.
  • F775's 86 files are all equal.

SHOULD 1: a non-UTF-8 name collision splits the two tools (siblings.py:316-318).

SHOULD 2: valued as the newline is unpinned on one side.

  • Mutant not valued → not value makes an empty ignore = or url = refuse the whole file. It survives every test (110 passed), although git takes ignore = and branch = and registers the url.
  • Fix: two GIT_SKIPS controls after bare-shallow (tests/test_doctor_legs_members_next.py:1678), empty-ignore and empty-branch, each expecting ("q", "/r/u.git"). Both pass on the head and fail under the mutant.

NITs:

  1. siblings.py:281-283 says "is the key, a newline and the value". Say "the key and, where it is given a value, a newline and the value".
  2. siblings.py:302-304: for a value git dies on, git says invalid value for …, bad boolean config value … or bad submodule.p.fetchrecursesubmodules argument. Add "or that the value is invalid".
  3. Mutant MD (the - skip before the refusal) is killed only by shallow-below-int-min. Add the row P_THEN_A_LINE % (b"q", b"update = -x") → (None, None).

Runs (stub gh first on PATH, GH_TOKEN=bogus, empty GH_CONFIG_DIR):

run result
test_siblings_inputs.py, test_doctor_gitmodules_not_utf8.py, test_doctor_legs_members_next.py, test_family_siblings_placement.py 285 passed, 6 skipped
test_windows_paths.py, test_family_makefile_siblings.py, test_siblings_family_name.py, test_pins_child_kind_and_siblings_root_spelling.py 237 passed
#790's row test, CR exclusion dropped 27 passed

Mutants:

mutant result
MA: refusal before the ignored-section skip killed (2)
ME: return {} → break killed (10)
MD: - skip moved before the refusal killed by one row only (NIT 3)
MB: not valued → not value survives (SHOULD 2)
F775's M2, M3 and M6, re-measured match its report

Merges: clean against main 6626a3c5 and 64ec47c3, against #790, and against every open lane-3 PR that touches siblings.py or shape-doctor.py. #774 is an ancestor of the head.

The Round 4 section: every claim holds, except that "the doctor and make siblings still agree" holds for every suite row but not for the non-UTF-8 collisions (SHOULD 1).

CI on 32048c4f: SonarCloud, package and tests-git-bash pass; tests is pending.

Lane: openreposhape-3 (openRepoShape-3)

Brett Heap and others added 2 commits October 17, 2026 16:54
…ames that differ only in bytes that are not UTF-8 stay two sections, as git and the doctor read them, and an empty `ignore =` or `branch =` and an `update = -x` are pinned (#733, review round 4)

RV775r4's review of round 4 (`32048c4f`, #775 issuecomment-6237014956),
its two SHOULDs and three NITs:

- SHOULD 1: `submodule_urls` decoded the whole `-z` listing with
  "replace", so `[submodule "caf\xe9"]` and `[submodule "caf\xe8"]` (or
  `caf` + the UTF-8 of U+FFFD) were one section: it gave `q` alone, and
  with the path in one and the url in the other a url git does not mount.
  The listing is now decoded with "surrogateescape", as the doctor's
  `gitmodules_listing` decodes it (#732), and the VALUE alone is
  re-decoded with "replace" (#432). New rows beside #805's parity test,
  `test_two_names_apart_only_in_bytes_not_utf8_stay_two_sections`, check
  the doctor, `make siblings` and `git submodule init` for all three.
- SHOULD 2: `GIT_SKIPS` gains `empty-ignore` and `empty-branch` controls:
  git takes `ignore =` and `branch =`, so `valued` is the newline, not the
  value.
- NIT 1 and NIT 2: the docstring says a record is the key and, where it
  is given a value, a newline and the value, and that git may say the
  value is invalid.
- NIT 3: `GIT_SKIPS` gains `update-dash` (`update = -x`, which git dies
  on), so the `-` skip stays after the refusal.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A clean merge: main's two commits since 40e678e, #747 (#681) and #817
(#708), touch none of this branch's files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T17:14:49Z) RV775r4's re-check of #775 round 5 (head 44f0bf10): READY. All five items are taken as asked, nothing is left, and nothing new was found. 44f0bf10's tree is git's own clean merge of 8967b73d with main 64ec47c3.

The items:

item where what
SHOULD 1 siblings.py:319, :324-325 The listing is decoded with "surrogateescape" and only the value is re-decoded with "replace". The docstring (:292) says so.
SHOULD 1 test tests/test_siblings_inputs.py:650-714 test_two_names_apart_only_in_bytes_not_utf8_stay_two_sections
SHOULD 2 tests/test_doctor_legs_members_next.py:1683, :1685 rows empty-ignore and empty-branch
NIT 1 siblings.py:281 docstring
NIT 2 siblings.py:307 docstring
NIT 3 tests/test_doctor_legs_members_next.py:1693 row update-dash

Against real git:

  • The three collision rows: git submodule init, the doctor and make siblings give the same {path: url} in each:
    • caf\xe9 beside caf\xe8 gives both mounts;
    • caf\xe9 beside U+FFFD's UTF-8 gives both mounts;
    • a path in one section and a url in the other gives No url found, so {}.
  • Three more rows agree: a Latin-1 url value, update = rebas\xe9 (git says invalid value), and a -\xe9 url after a url.

Parity: the maps are equal in 132 of 132 files (RV775r4's 129 plus round 5's three new rows). The mounts are equal in 130; the two that differ are the known empty-url = rows.

Mutants on the new decode:

mutant result
N1: the whole listing decoded as latin-1 killed (10)
N2: the value re-encoded with "replace" killed (2)
N3: the value re-decoded with "ignore" killed (2)
F775's MR, re-measured matches its report (3)

Runs (stub gh first on PATH, GH_TOKEN=bogus, empty GH_CONFIG_DIR):

run result
group A 291 passed, 6 skipped
group B 237 passed
test_repo_hygiene.py 77 passed
#790's row test, CR exclusion dropped 27 passed

SonarCloud's gate is OK, with 0.0% new duplication.

Main: #748 (431c8cf8) conflicts with #775, and with #774 (93b9bac3), in ONE hunk: the import block of tests/test_doctor_legs_members_next.py. The fix is mechanical: keep main's two-line conftest import, then the listed_locale line. With that, the whole file gives 181 passed, 6 skipped, and the result still holds against d66a9054.

Lane: openreposhape-3 (openRepoShape-3)

Brett Heap and others added 3 commits October 17, 2026 18:48
…sts/test_doctor_legs_members_next.py keeps #748's two-line `conftest` import, then `listed_locale`

Since #748 (431c8cf) main splits the `conftest` import of
tests/test_doctor_legs_members_next.py over two lines to add
`submodule_update_through_git`, and #774's commit 311ad7d adds
`from test_doctor_gitmodules_not_utf8 import listed_locale` just below it.
That was the one conflict, in that one hunk. Resolved by keeping main's
two-line `conftest` import and then the `listed_locale` line. No other file
conflicted.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#774 (l3-732-latin1-section) merged main bad05d2 at 012a772, resolving the
one hunk in the import block of tests/test_doctor_legs_members_next.py
(#748): main's two-line `conftest` import, then `listed_locale`. This branch
changed none of those lines, so the merge is clean.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main moved from bad05d2 (the main #774's head 012a772 holds) to fc86720
(#672, #746), which changes scripts/bump-leg.py and its tests alone. No file
conflicted.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-17T19:24:01Z) RV775r4's check of the main merge into #775 (head 8c5ad0e8): READY, with nothing left.

This lands after the v1.1.0 tag, under Brett Heap's word "merge lane 3's v1.1.1 drafts after the v1.1.0 tag".

Lane: openreposhape-3 (openRepoShape-3)

brettheap pushed a commit that referenced this pull request Oct 17, 2026
…ask the row whose section name holds a carriage return, since `siblings.py` has read `.gitmodules` with `-z` since #805 (#690) (#786)

`test_siblings_reads_what_each_row_says` left out every row holding a CR,
because `siblings.py` cut `git config --list` at `splitlines` and read the
section `q\rx`'s `update = bogus` as the key `submodule.q` and `x.update`.
#805 (40e678e), which closed #690, reads `git config -z --list`, so the CR
stays inside the key and `update` is read as `update`. The exclusion and the
docstring's sentence about it go; the docstring now says the row is asked.

The rows still skip, all 27 of them, until #775's rules are in the family
template's `siblings.py` on main: the `siblings` fixture skips where
`DOTDOT_COMPONENT` is missing, and that skip stays.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 18, 2026
…-reading steps read `.gitmodules` by the rules the doctor and `siblings.py` read it by, so a section git ignores, a section with no path, a `path` or `url` beginning with `-`, an unmerged file or a file git refuses whole no longer puts a repository in the App token or the lists, and a refused file fails the step with its cause (#786) (#790)

Closes #786.

The two `validate.yml` templates, assembly root and family root, read every
`.url` key of `.gitmodules` at three sites each: the step that names the
repositories the App token is minted for, the fetch step's list of urls, and
the failing step's list. So CI named repositories git never fetches, and where
the App could not reach one, the mint step failed the job.

Now all three sites read the file by the rules the doctor and `siblings.py`
use (#733): only a `submodule.?*.url` key counts; a section named with a `..`
component, or with no `path`, is skipped; a `path` or `url` value beginning
with `-` is ignored, that value alone, so an earlier one stands; and nothing
is read while `.gitmodules` is unmerged. A file git refuses whole fails the
names step and the fetch step with an `::error::` that names the cause, the
key or git's own `fatal:` line, whatever credential is configured. Before, the
fetch step with no credential degraded to green. The refusal comes before the
App advice, which is the wrong advice for that file. The printing steps list a
key once.

`tests/test_validate_workflow_gitmodules_urls.py` holds a row for each rule,
checked against `git submodule init`, the doctor and `siblings.py`. README.md,
the handbook and both template READMEs now say a `.gitmodules` git refuses
fails the run whatever the credential.

After the merge of main `2e476a77`, which brings #805's `-z` read of
`.gitmodules` into `siblings.py`, `e91b1a5d` drops the #690 exclusion of the
row whose section name holds a carriage return from
`test_siblings_reads_what_each_row_says`, so that row is asked too. All 27
`siblings` rows skip until #775 lands: the `siblings` fixture skips where
the family template's `siblings.py` has no `DOTDOT_COMPONENT`, and main's
has none.

Lane 3's independent reviewer RV-c (opus) found it READY in its second round
(#790 issuecomment-6232685791). Lane 3 checked the test-only NIT round after
it, and RV-c's READY stood (#790 issuecomment-6232735343). RV790n found the
merge round READY (#790 issuecomment-6243554860). It lands on Brett
Heap's word "merge lane 3's v1.1.1 drafts after the v1.1.0 tag"
(2026-10-17T13:00:57Z).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap and others added 3 commits October 17, 2026 21:07
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-18T01:31:48Z) #775 is restacked after #774's squash (52ff596), as 7f6ee36b, by lane 3's three-merge recipe (writer R775, sonnet-xhigh):

  1. 0f825d8c merges shape-doctor.py: a .gitmodules section name keeps git's own bytes and is handed back to git as them, so configured_url and kept_gitdir find a member named caf\xe9, its detached commit gets its keep line under any locale, and a non-ASCII name no longer ends --root in a UnicodeEncodeError traceback under LC_ALL=C PYTHONUTF8=0 (#732) #774's final head eb595e30. It is clean, and it brings in main since fc867207, tests/test_doctor_legs_members_next.py and 24 other files: tests append what they tell git to the GIT_CONFIG_COUNT entries the environment holds, so a pasted line keeps the session's maintenance settings and the Windows job's two (#758) #818's paste_env() among it.
  2. 7f6ee36b merges the squash 52ff5962, taking ours only in shape-doctor.py, one of shape-doctor.py: a .gitmodules section name keeps git's own bytes and is handed back to git as them, so configured_url and kept_gitdir find a member named caf\xe9, its detached commit gets its keep line under any locale, and a non-ASCII name no longer ends --root in a UnicodeEncodeError traceback under LC_ALL=C PYTHONUTF8=0 (#732) #774's files. The squash and the final head have the same tree, and this merge changed nothing.
  3. Main was 52ff5962 itself, so the third merge was "Already up to date".

Lane: openreposhape-3 (openRepoShape-3)

brettheap added a commit that referenced this pull request Oct 18, 2026
…appended_to_command_scope`, the scan names a count worked out by hand, and `paste()` gives a pasted `submodule update` the `insteadOf` rewrite (#835) (#848)

Stacked on #818 (`fix/758-git-command-scope`, `4a553e17`); retarget to main once it lands. The fix is one commit, `b9ad1187`, written on #818's `eaa72d9e`. The head `fafba9ec` merges #818's refresh `4a553e17` (main `fc867207`) into it, cleanly.

Tests only. Nothing a user runs changes, and no pinned file changes. No README, handbook, `AGENTS.md` or `docs/cli.md` sentence is touched: none mentions these helpers (`render-cli-reference.py --check` passes).

Both reviews say READY with no MUST: rv848 (issuecomment-6240157020) and lane 2's second review of #848 (issuecomment-6240228302, "rv818" below). One commit on `fafba9ec`, by explicit pathspec, fast-forward pushed. Tests only; no pinned file changes; main and #818 are not merged in. The rv818 numbers are next to rv848's.

### What each finding got

| rv848 | rv818 | taken | what changed |
|---|---|---|---|
| 1 SHOULD: #811's and #838's counts | | the text | the merge section below says who converts it |
| 2 NIT: stale merge checks | | yes | re-run at the head, lines corrected below |
| 3 NIT: the walk counts arguments | 6 NIT: it does not read their values | yes, one change | `paste(row, cwd, None)` now fails the walk |
| 4 NIT: `on_disk` is asked for `"true"` only | | yes | `ADDERS` holds `on_disk-false` |
| 5 NIT: the `upload-pack` docstring | | yes | `paste_env` says what the rewrite adds: `pack-objects` and `index-pack` |
| | 1 SHOULD: two command scopes built at collection | yes | #818's second review's SHOULD (#758's line 1), taken here so that #818 needs no third round |
| | 2 NIT: an expected dict is flagged | yes | the scan leaves a dict that is an operand of a comparison |
| | 3 NIT: what the scan misses | the text | "Known limits" below, no code |
| | 4 NIT: the same lines in two files | yes | three helpers in `conftest.py` |
| | 5 NIT: two surviving mutants | in part | the `count_of(env)` case is added; the `remotes_of` one is left, below |

**rv818 SHOULD 1.** `test_adopt_reencoded_files.py`'s `attr.tree` param and `test_family_init_asks_every_value.py`'s `user.name` param were built at collection, before the session fixture put `gc.auto=0` and `maintenance.auto=false` in the environment. Laid over the environment at the test, each dropped the two from git's command scope. Each is a `lambda` its test calls, as `test_adopt_nested_attributes.py` already did.

New `tests/test_command_scopes_built_at_the_test.py` (2 tests) reads each param off its test's own marks, builds it, and asks git for the command scope (`git config --show-origin --list`, read and never written). It asks for the session's two settings beside the param's entry. **It fails on `fafba9ec`** (the two test modules as they were, with the new `conftest.py`): `2 failed`, git read `['user.name=', 'user.email=', 'attr.tree=4b825dc6...']` and `['user.name=', 'user.email=', 'user.name=Ada Lovelace']`, with no `gc.auto=0`. It passes on the head.

**rv848 3 and rv818 6.** `pastes_in` now returns, for each `paste` call, the fixture it hands the remotes of: `remotes_of(<name>)` as the third argument or as `remotes=`, where the name is `holder` or `scaffolded_here` and a parameter of the nearest function the call is in. `None`, another call, an expression on the call, a name the test does not take, or no third argument gives no fixture. The list of calls with none is `pastes_without_remotes`, a function of its own, so that a test of it can fail. 15 new cases hold the walk: twelve for what a call may hand and three for the function it is in.

**rv818 2.** `counts_set_to_a_number` skips a `Dict` that is an operand of a comparison (`assert added == {"GIT_CONFIG_COUNT": str(held + 1), ...}`, `!=`, a dict on the left). I took the narrower reading of "inside an assert or a comparison": a dict in a call inside an assert (`assert run(env={"GIT_CONFIG_COUNT": "1"}).returncode == 0`), or one assigned before the comparison, still sets a count and is still read. Four cases are still found, three are left, and the `count_of(env)` case of rv818 5 is left.

**rv818 4.** `JOB_ENTRIES`, `put_job_and_session_entries(monkeypatch)`, `command_line_entries(listing)` and `command_scope_at(environ, cwd)` in `conftest.py` replace the `delenv`, `held = ...` and loop lines of `test_no_auto_maintenance.py` and `test_doctor_paste_through_git.py`, and the four-line read of the `command line:` entries in three files. `OWN_COUNTS` follows: `conftest.py` holds two (the sum, and the job's entries counted from nothing), `test_no_auto_maintenance.py` one, and `test_doctor_paste_through_git.py` none. A new test checks that both readers put `GIT_CONFIG_PARAMETERS` aside (rv818's own round-2 NIT 8).

**rv818 5, left.** `remotes_of` returning `fixture["base"]` instead of `fixture["base"] / "remotes"` still survives, and I name it equivalent in practice: the prefix `<base>/` covers every remote under `<base>/remotes`, so all the pasted clones are rewritten either way.

### Merge checks (replace the body's section)

Re-run at the head `79b2eac4` (`git merge-tree --write-tree`), against 152 heads: every `fix/*` and `l3-*` branch, and the pull heads from #700. Main `fc867207` is an ancestor of the head (it came in with #818's refresh `4a553e17`).

- **Main.** `fc867207`: clean. Main is `190133d5` now (two commits on `family.py`, the README and the handbook): clean too.
- **#818's head `4a553e17`:** clean. The body's "one conflict, `tests/test_non_utf8_git_reads.py`" was true only for `b9ad1187` and `eaa72d9e`, before the refresh.
- **The open heads that edit the files here merge clean:** #811 (`050abea6`), #820, #822, #828 (`ce0f2a5f`), #774 (`012a7726`) and #775 (`8c5ad0e8`). #761, #762 and #763 are the same heads as #828, #820 and #822. The body said #774 and #775 conflict; they do not, since the refresh.
- **#838's pushed head `9166f84f`:** clean.
- **Conflicts the other heads show** are the ones #818's head shows too (20 heads, in files this PR does not touch: `tests/test_adopt_git_reads.py`, `tests/test_non_utf8_git_reads.py`, `tests/test_dry_run_parity.py`, `shape-doctor.py`, `scripts/shape_materialize.py` and others). #806 (`76ae6419`) and #845 (`b18b243e`, branch `fix/834-oserror-skips`) conflict in `tests/test_adopt_git_reads.py`, which this PR does not touch.
- **This commit adds two, both stale PR heads whose squash is in main:** #767's `bd6d2afe` (landed as `98f14976`, in `tests/test_family_add_spellings_remedies.py`) and #809's `98561405` (landed as `42aed788`, in `tests/test_family_init_asks_every_value.py`).
- **Whichever of #848, #811 and #838 lands later converts that count to `appended_to_command_scope`.** #811 (`fix/777-typed-name-edges`, `050abea6`) adds `"GIT_CONFIG_COUNT": str(held + 1),` at `tests/test_setup_project_name_edges.py:190`: the scan finds `[190]` where `OWN_COUNTS` allows none, so `test_no_test_sets_a_git_config_count_of_its_own` fails once #848 and #811 are both on main. rv848 measured the same for #838's local `3845fd54` at `tests/test_github_name_limits.py:503`; the head #838 has pushed, `9166f84f`, holds no `GIT_CONFIG_COUNT` in that file. It is the goal of #835, so it takes no `OWN_COUNTS` entry. No file of theirs is changed here.

### Known limits (add to the body)

Shapes the scan does not read, so a test that sets a count this way passes it:

- a count key name imported from another module, held as a class attribute, bound by tuple unpacking or by a walrus (only a plain or annotated assignment of the name in the same module is read);
- a value built by `format()`, `repr()` or `os.fsdecode()` (`str(...)`, an f-string, a `.format(...)` method and arithmetic are read);
- a conditional value, `"2" if job else "1"`;
- `setenv(name=..., value=...)`, with keywords;
- an annotated subscript, `env["GIT_CONFIG_COUNT"]: str = ...`;
- a value held in a variable and set later (`new = str(n + 1)`, then `{"GIT_CONFIG_COUNT": new}`);
- an indexed `GIT_CONFIG_KEY_{n}` or `GIT_CONFIG_VALUE_{n}` name, which a hand-worked append writes beside its count;
- a dict that is an operand of a comparison, by design (it is the value a test expects).

This replaces a separate issue for them.

### Tests, through `run9.sh`

- At the commit `79b2eac4`: **222 passed** (`test_command_scopes_built_at_the_test.py` 2, `test_family_config_appends.py` 8, `test_doctor_paste_through_git.py` 21, `test_no_auto_maintenance.py` 60, `test_adopt_reencoded_files.py` 53, `test_family_init_asks_every_value.py` 78).
- The same content, run before the commit: **368 passed** (those and `test_doctor_legs_members_next.py` 127, `test_clone_just_pushed.py` 19) and **253 passed** (`test_doctor_next_is_a_command.py`, `test_doctor_validator_rows.py`, `test_repo_hygiene.py`, `test_agents_md_codes.py`, `test_cli_reference.py`).
- New tests: 24 (2 + 11 + 2 + 9). Before the round: `test_doctor_paste_through_git.py` 10, `test_family_config_appends.py` 6, `test_no_auto_maintenance.py` 51.
- Fail on `fafba9ec`: the command-scope test above, `2 failed`. The walk, `on_disk-false` and the scan cases pass on the code of `fafba9ec`, which is right; what they add shows in the mutants, where four survive the files of `fafba9ec` and none survives these.

### Mutants, 29 in a throwaway clone at the head, each caught

- **rv848 4:** `on_disk` ignoring its value. Caught by `on_disk-false` (2 failures); survives the `test_family_config_appends.py` of `fafba9ec`.
- **rv848 3, rv818 6:** one caller handing `None` (M01), `remotes_of(tmp_path)` or `remotes_of(project)`: each caught by the walk of the real file, and each survives the walk of `fafba9ec`. Eight mutants of the walk itself (the `remotes=` keyword, the parameter check, the remote-fixture check, the argument count, the `remotes_of` name, the nearest-function rule, the call check, and `pastes_without_remotes` always empty): each caught by its own case.
- **rv818 1:** each of the two params built at collection again, and each building nothing: 4 mutants, each caught by `test_command_scopes_built_at_the_test.py`.
- **rv818 2:** no skip of expected dicts (4 failures), every dict in an assert skipped (4), only the left operand skipped (3), only the comparators skipped (1).
- **rv818 5:** any call by a bare name taken as worked out: caught by the `count_of(env)` case.
- **rv818 4:** the helper putting only the job's entries (2), the reading taking another line (10), either helper keeping `GIT_CONFIG_PARAMETERS` (1 each), the job's second entry another value (6).
- **`OWN_COUNTS`:** `conftest.py` allowed one (2 failures), this file allowed two (1), no allowance for `test_family_config_appends.py` (1).

1. **Line 1.** `on_disk`, `with_config` and `typed_with_local_remotes` each worked out their `GIT_CONFIG_COUNT` from the environment's own count. Each is now one call of `appended_to_command_scope(env, [(key, value)])`; `typed_with_local_remotes` sets each key the helper returns, inside its `monkeypatch.context()`. The scan found a fourth: `test_clone_just_pushed.py`'s `env` for `clone.defaultRemoteName` summed its own count too, and goes through the helper.
2. **Line 2.** `counts_set_to_a_number` finds a count worked out in the same four places it finds one written out (a dict, a keyword, a `setenv`-style call, a subscript): a `str(...)` call, an f-string, a `.format(...)` call or arithmetic. `OWN_COUNTS` lets `conftest.py` hold the one sum that remains, in `appended_to_command_scope`.
3. **Line 3.** `paste(row, cwd, remotes=None)` and `paste_env(remotes=None)`. Given the fixture's remotes, they append `insteadof_entries(remotes)` after `protocol.file.allow`, in the same count. Every `paste` in `test_doctor_legs_members_next.py` hands `remotes_of(<fixture>)`, and a walk of that file holds each call to it.

The issue says each site is right today, and it is: the held-two case below passes on the base. What the helper adds is an empty `GIT_CONFIG_COUNT`, which git reads as no entries and `appended_to_command_scope` counts as none (`or 0`). The sums written out by hand died on `int("")`.

`tests/test_family_config_appends.py` (6 tests) starts each helper under `GIT_CONFIG_COUNT=2`, the Windows job's two entries, and asks `git config --show-origin --list`: both held entries and the new key, in order. It starts each under an empty count and checks the entry is the first. `typed_with_local_remotes` is asked from inside `typed_as_printed`, and the environment is checked to be back as it was.

**Yes.** Measured by running the extended scan over every `tests/*.py` at the base: it finds exactly the three sites, `test_clone_just_pushed.py:99`, and `conftest.py:290`, the helper itself. No other file, so no false positive.

- The scan reads the SHAPE of the value and never evaluates it. `str(...)`, an f-string, `.format(...)` and arithmetic are found. A value held in a name, read by subscript (`added["GIT_CONFIG_COUNT"]`) or returned by any other call (`env.get(...)`) is still not read, and so is `int(env.get("GIT_CONFIG_COUNT", "0"))`, which sets nothing. `is_a_written_count` still reads constants only; the new `is_a_worked_out_count` is the one that reads shapes, and both docstrings say so.
- `test_the_scan_leaves_a_count_that_is_worked_out` moved its `worked-out` and `a-call` (`str(held)`) cases to the found list, and is now `test_the_scan_leaves_what_it_does_not_read`, with the cases that are still not read.
- `OWN_COUNTS` gains `conftest.py: 1` and the two new files (`test_family_config_appends.py: 2`, `test_doctor_paste_through_git.py: 1`), each a test that builds an environment on purpose. A second sum in `conftest.py`, or none, is named, as it is for every other file.
- `test_the_scan_names_each_helper_that_worked_its_count_out` holds the three helpers' old source, so the scan names the line of each.

**Yes.** Measured on a pasted `git -C <root> submodule update --init --recursive --checkout -- code` of an empty mount, with `GIT_TRACE`:

| | no rewrite (base) | with the rewrite |
|---|---|---|
| clone's store | 8 loose objects, 0 packs | 0 loose objects, 1 pack |
| link count of the loose objects | 2 (linked to the remote's own files) | none |
| `upload-pack` in the trace | yes (a local clone starts it to list the refs) | yes |
| `pack-objects` and `index-pack` in the trace | no | yes |
| `origin`, and the `submodule.code.url` the root records | the plain path | the plain path |

Across the whole of `test_doctor_legs_members_next.py` (trace2 events of all 56 `sh -c` lines it runs), no pasted line ran `pack-objects`: every pasted clone was a local clone. A pasted fetch needs no rewrite (it asks `upload-pack` whatever the URL is), so the rewrite matters where a mount is cloned: 14 of the 31 tests that paste.

**The issue's Verify names `upload-pack` and `pack-objects` for the `GIT_TRACE` run. `upload-pack` is in both columns,** so the tests ask for `pack-objects` and `index-pack`, and for the store `assert_store_read_through_git` asks of every other clone.

#645 keeps maintenance from starting, so nothing breaks today. #662 put every other read of these remotes through git as a second guard, and `paste` was the one left. The rewrite is for the directory resolved, which is how `.gitmodules` records each url; no pasted line names a remote itself, so `typed=True` is not needed.

`tests/test_doctor_paste_through_git.py` (10 tests):
- a pasted update of an empty mount of a project, and of a holder's member with its legs: `pack-objects` and `index-pack` run, `assert_read_through_git`, `origin` equal to the url `.gitmodules` records, no `insteadOf` stored in the root or the clone;
- the control, handed no remotes: a local clone, loose objects, no `pack-objects`;
- the entries in order: the environment's held entries and the session's two settings, then `protocol.file.allow`, then each `insteadOf` entry;
- the walk: every `paste` of the doctor file is given three arguments, and the walk's own five cases.

Base `eaa72d9e`, before: the three family files 117 passed; `test_no_auto_maintenance.py`, `test_clone_just_pushed.py` and `test_repo_hygiene.py` 132 passed; `test_doctor_legs_members_next.py` 127 passed.

On the head `b9ad1187`, through `run9.sh` (gh stub first, `GH_TOKEN=bogus`, empty `GH_CONFIG_DIR`):
- **340 passed:** `test_family_head_lost_gitmodules.py`, `test_family_bootstrap_gate_order.py`, `test_family_add_spellings_remedies.py`, `test_family_config_appends.py`, `test_no_auto_maintenance.py`, `test_clone_just_pushed.py`, `test_repo_hygiene.py`, `test_agents_md_codes.py`, `test_cli_reference.py`.
- **261 passed:** `test_doctor_paste_through_git.py`, `test_doctor_legs_members_next.py`, `test_validate_family_remedies_typed.py`, `test_doctor_next_is_a_command.py`, `test_doctor_validator_rows.py`.

New or changed tests: 31 new (6 + 10 + 15; `test_no_auto_maintenance.py` goes from 36 to 51 tests).

**Fail on the base**, in a tree detached at `eaa72d9e` with the new test files copied in:
- the helpers' tests and the scan: 4 failed, 53 passed. The empty-count case fails for `on_disk`, `with_config` and the typing helper (`ValueError: invalid literal for int()`), and `test_no_test_sets_a_git_config_count_of_its_own` names `test_clone_just_pushed.py [99, 233]`, `test_family_add_spellings_remedies.py [138]`, `test_family_bootstrap_gate_order.py [136]` and `test_family_head_lost_gitmodules.py [98]`;
- the same file with the scan as it was (written counts only): 14 failed;
- `test_doctor_paste_through_git.py`: ImportError (`remotes_of`), 10 not run; with `paste` ignoring the remotes (mutant 3a), the three pasted-clone tests fail.

**Mutants**, 22 in a throwaway clone at the head, each caught:
- 1a-1c each helper back to a hand-worked sum: `test_no_test_sets_a_git_config_count_of_its_own`, and the empty-count case; 1d the clone's `env`: the same scan test; 1e `on_disk` keeping a count of 1 held in a name, which the scan does not read: the held-two case;
- 2a-2d `str`, `.format`, arithmetic and f-string dropped from `is_a_worked_out_count`; 2e-2h each of the four readers back to written counts; 2i-2k an `OWN_COUNTS` entry removed; 2z the scan as it was on the base;
- 3a `paste` not handing `remotes` on; 3b `paste_env` dropping the rewrite; 3c the rewrite put before `protocol.file.allow`; 3d one caller not handing the remotes (the walk); 3e `remotes_of` naming the wrong directory.

- origin/main `fc867207`: one conflict, `tests/test_non_utf8_git_reads.py`, the one #818 has with main at its old head. It is the same with the base `eaa72d9e`, and none of the files here conflicts.
- #818's head now, `4a553e17` (main merged in): clean.
- The 141 other heads (every `fix/*` and `l3-*` branch, and the pull heads from #700): the conflicts they show are the same with the base, in files this commit does not touch (`tests/test_non_utf8_git_reads.py`, `tests/test_adopt_git_reads.py`, `tests/test_dry_run_parity.py` and others). This commit adds one: #767's stale PR head `bd6d2afe`, which landed as `98f14976`.
- The open heads that edit the files here merge clean with it: #820, #822, #828, #748, #747, #761, #762 and #763. #774 and #775 conflict only in `tests/test_non_utf8_git_reads.py`, as they do with the base.

- Left out: `GIT_CONFIG_KEY_{n}` and `GIT_CONFIG_VALUE_{n}` names built from an index (an f-string) are not read, nor is a count held in a name and set later (`new = str(n + 1)`, then `{"GIT_CONFIG_COUNT": new}`). A test that did that would pass the scan. Reading the indexed key names in the same four places would close it.
- `typed=True` is not used for `paste`; the entries are for the resolved directory.

Review: rv848, lane 5's independent review, READY (issuecomment-6240157020); lane 2's independent review, READY (issuecomment-6240228302); rv848's re-verification of the fix round 79b2eac, READY (issuecomment-6241026517). CI (Brett Heap's ruling 27 and his fast-refresh ruling of 2026-10-15T22:48:35Z): H0 73b6d2b (the restack of the branch onto main after #818 landed) passed tests, tests-git-bash and SonarCloud; the head 5b27685 is a clean update-branch merge of main ca47b71, with tests-git-bash and SonarCloud green; and the local gate passed on its predicted squash tree 4dbb4a9 over main ca47b71: the adopt_reencoded_files, clone_just_pushed, command_scopes_built_at_the_test, doctor_legs_members_next, doctor_paste_through_git, family_add_spellings_remedies, family_bootstrap_gate_order, family_config_appends, family_head_lost_gitmodules, family_init_asks_every_value, no_auto_maintenance, repo_hygiene, agents_md_codes, cli_reference sets, 645 passed. Landed on Brett Heap's word of 2026-10-17T21:47:28Z, in lane 5's session (posted on #473 as issuecomment-6241758069): "merge lane 5's v1.1.1 PRs once reviewed and green".

Closes #835

Lane: openreposhape-5 (openRepoShape-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@brettheap
brettheap marked this pull request as ready for review October 18, 2026 06:38
@sourcery-ai

sourcery-ai Bot commented Oct 18, 2026

Copy link
Copy Markdown

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 3 days and 12 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@brettheap
brettheap merged commit e2e1171 into main Oct 18, 2026
5 checks passed
@brettheap
brettheap deleted the l3-733-ignored-sections branch October 18, 2026 06:38
brettheap added a commit that referenced this pull request Oct 18, 2026
… advice and a member's exit code say what happened, and the gate's `git rm --cached`, guarded `mv` and section lines type as printed where they failed (#839) (#876)

Stacked on #819 (`fix/768-gitmodules-values`, `233a334e`); retarget to main once it lands.

#839's seven lines, in the family holder's two pinned scripts, `templates/family-root/scripts/bootstrap.py` and `templates/family-root/scripts/validate-family.py`. Each line was probed on `233a334e` first; one already held there (5).

**1. The credential advice beside "not for want of a credential"** (`fetch_members`).
- Before: "If the members are PRIVATE, set SHAPE_LEGS_TOKEN ..." was printed for every failed `git submodule update`, and then "NOT CHECKED OUT, and not for want of a credential" for every row `unfetchable` named.
- Now `credential_advice` prints it only where git asked for a credential. It is not printed where git's init died at a gitlink it has no url for (`stopper`), or where git read nothing from `.gitmodules`: a file a merge left unmerged, a value git dies on, or a file it cannot parse. Git fetched nothing in those states.
- Beside a denial, the advice reads "If the members the next line does not name are PRIVATE". This is the Direction's "both kinds" case.
- One step past the Direction's literal rule: where every member is checked out and a leg's fetch failed for want of a credential, no row is denied and the advice stands. The token's name is `SHAPE_LEGS_TOKEN`, and the legs are what it is for.
- Tests: `test_no_advice_is_printed_beside_its_denial` covers nine states, every reason `unfetchable` gives where git exits non-zero (no section, the variant, no url, unmerged, a refused value, an unparsable file, a stray, a hand mount, a gitlink outside `members/`). It asserts bootstrap's own stderr lines exactly: the fetch line, the denial, the failure, and no advice. `test_the_credential_s_skip_prints_the_advice_alone` (three states), `test_a_leg_the_credential_s_keeps_the_advice_with_every_member_out` and `test_beside_a_denial_the_advice_is_for_the_members_it_does_not_name` pin the other side.

**2. A member's refusal and its finding both read `make bootstrap exited 2`** (`run_member_target`). Chosen: tell them apart, by reading make's own `Error <code>` line.
- Measured on GNU Make 4.3: make exits 2 for a command exiting 1, 2, 3 or 126, and 2 again for a failure of its own (`No rule to make target`). It prints `make: *** [Makefile:N: bootstrap] Error <code>` (`make[1]:` under another make) for a failing command, and no such line for its own failure.
- `make siblings` runs the member's `scripts/bootstrap.py` directly and prints "its own bootstrap exited N". The direct run was not chosen: `--members-make validate`, which the holder's own `make validate` runs, has no script to call, and the docs say the holder runs each member's own `make bootstrap`.
- So make still runs. `run_make` passes make's stderr on as it comes and reads the code from the last `Error <code>` line.
- What the person reads now, at the end of the run:
  - a member's finding: `IRRS (make bootstrap: the member's command exited 1)`, under the member's own `bootstrap FAILED: ...` and make's `Error 1`;
  - a member's refusal: `IRRS (make bootstrap: the member's command exited 2)`, under its `REFUSED ...` and make's `Error 2`;
  - make's own failure: ``IRRS (make bootstrap exited 2 and printed no `Error <code>` line for a command of the member's: make's own message above says what failed)``, under make's own message.
  - A make speaking another language, or a command killed by a signal, gets the third line too, which is still true there.
- Test: `test_a_finding_a_refusal_and_make_s_own_failure_are_told_apart`, for `bootstrap` and for `validate`: four members, a finding, a pass, a refusal and no rule, with the final line asserted exactly and the members' and make's own lines asserted on stderr. `tests/test_bootstrap_spelling.py`'s make line follows the new text.

**3. The `git rm --cached` that refuses over a checkout of its own.**
- Reproduced on the gate's own state, as the Direction asks: `mkdir Members && git mv members/IRRS Members/IRRS`, staged, then a commit (or only an untracked file) in the moved checkout.
- In that state the gate does not print the wrong-case move. It prints three findings in every mode:
  - `member-gitlink-unnamed` for `Members/IRRS`, a second path to the member, whose line is `git rm --cached -- Members/IRRS`;
  - `gitmodules-path-unnamed`, which prints no command;
  - `member-gitlink-removal-staged` for the row's own gitlink.
- Typed as printed, the `git rm` refused on git 2.43: `error: the following file has staged content different from both the file and the HEAD`, exit 1. It refused with the commit, with the untracked file alone, and with both.
- `_forced` reads the state git refuses: the index stages the gitlink at a commit HEAD does not record there, and a `.git` entry is on disk at it. There the line takes `-f`, and the finding says what `-f` does, in #828's words: "-f takes out a staged gitlink whose checkout has moved on or holds a file git does not track, which git rm otherwise refuses, and with --cached touches no file".
- Where HEAD records nothing at the variant, the finding also says that nothing of it is left to commit. The row's own removal-staged lines put the member back, and a commit between the two would have un-registered it.
- The wrong-case move's own `git rm -r --cached --ignore-unmatch` takes `-f` the same way. That state is a committed variant whose checkout moved on, staged, with an untracked file in it.
- Tests: `test_a_git_mv_staged_with_work_in_the_checkout_is_undone_as_printed` (commit, untracked) types every printed line as printed, then finds nothing left to commit, the gate passing in every mode and HEAD recording the member at its pin. `test_a_checkout_moved_on_at_the_variant_is_moved_with_f` pins the move's `-f`. `test_a_staged_second_path_with_nothing_on_disk_takes_no_f` checks there is no `-f` where nothing is on disk.

**4. The guard that follows a link, and its missing nothing-there clause** (`_wrong_case_move`, now `_guarded_move`). Comment 6237599648 is the same fault and is fixed here too.
- The guard is now `[ ! -L V ] && { [ ! -e V ] || [ V -ef R ] || [ -e V/.git ]; } || mv -- V ../<V with - for />`, the issue's form. The sentence says it "moves nothing where nothing is there, where the two names are one directory or where it holds a .git entry, a checkout, and moves a symbolic link there wherever it points, the link and never what it points at", typed "in a POSIX shell (Git Bash on Windows)" as #865's text says.
- Typed in dash (this host's `/bin/sh`) and bash 5.2 on the issue's seven layouts:

  | at `Members/IRRS` | base guard | #865's guard | this guard |
  |---|---|---|---|
  | nothing | `mv: cannot stat`, exit 1 | kept, exit 0 | kept, exit 0 |
  | a link to nowhere | moved | moved | moved |
  | a link to the row's directory | kept | kept | moved |
  | a link to a clone (a `.git` file) | kept | kept | moved |
  | `Members` a link to `members` | kept | kept | kept |
  | an empty directory apart | moved | moved | moved |
  | a checkout apart | kept | kept | kept |

- **#865's `family.py` guard still keeps a link at the variant.** `scripts/family.py` at #865's `f7072d08` (`_closing_sentences`) prints `[ ! -e V ] && [ ! -L V ] || [ V -ef R ] || [ -e V/.git ] || mv ...`, which answers as base does on the two link rows above. The validator's text and `family.py`'s therefore differ in the link clause until #865 takes the same form. This pull request does not edit `scripts/family.py`; #865's re-review is asked to rule on it.
- Tests: `test_a_link_or_nothing_at_the_variant_is_typed_as_printed` covers link-to-the-row, link-to-a-clone, a dangling link and nothing, each in sh and in bash, in w840's state. After the move's lines and their commit, the guard is typed as printed: a link is moved with its target untouched, nothing exits 0, the gate passes in every mode and `family.py add` of SPARE goes through. `test_the_guard_moves_a_link_always_and_nothing_where_nothing_is` types the gate's own `_guarded_move` on all seven layouts in both shells. The N-2 test's three layouts in `tests/test_validate_family_remedies_typed.py` pass unchanged. `variant_refused` in `tests/test_family_head_lost_gitmodules.py` follows the new text.

**5. Comment 6237354577 (a), bootstrap's `gitmodules_sections` reading only the work tree's file.** This already held at `233a334e`: #819's `gitmodules_source` reads the work tree's file, then `:.gitmodules`, then `HEAD:.gitmodules`. #819's own `test_where_the_work_tree_has_no_gitmodules_git_reads_the_index_s_or_head_s` pins it, and `test_the_credential_s_skip_prints_the_advice_alone`'s index-read and HEAD-read cases ask it again here. No code change.

**6. Comment 6237354577 (b), `member-gitmodules-absent`'s `git config -f` lines where the work tree's `.gitmodules` is gone.**
- Measured on `233a334e`: OTHER's section was taken out of HEAD's file and the work tree's file deleted (or its deletion staged). The finding's two lines made a file holding OTHER's section alone, and staged and committed, HEAD lost MEMBER's section: a second round.
- Now the first of the url-absent and absent findings (`member-gitmodules-url-absent`, `member-gitmodules-absent`) puts it back first, from where git reads it (`git checkout -- .gitmodules`, or `git checkout HEAD -- .gitmodules` where the index holds none), as #820's `member-gitmodules-url-absent` does.
- No later url-absent or absent finding puts it back. Measured on `233a334e`: with two url-less sections, the second `member-gitmodules-url-absent` put the file back over the url the first had written, and an absent section before a url-less one was lost the same way.
- Other findings that write or put back the file are not ordered with these two. rv876 measured two such states: the wrong-case move's `git config -f .gitmodules` line, and `member-gitlink-removal-staged`'s `git checkout HEAD -- .gitmodules`. Each behaves the same on `233a334e`, and both are filed in #879 (C1, C2).
- No finding puts it back after a `gitmodules-value-refused` that already put HEAD's back (`refused`).
- Tests: `test_a_section_absent_from_head_is_written_into_the_file_put_back` (deleted, deletion staged; every mode; typed and committed, HEAD keeps MEMBER's section). `test_two_findings_that_write_the_file_put_it_back_once` (two url-less, absent then url-less, url-less then absent). `test_a_value_finding_that_put_head_s_back_is_not_put_back_over`.

**7. Comment 6237599648, the move printed unguarded for an absent variant.** Line 4's nothing-there clause: the `nothing` rows above.

- `run9s.sh` with `SHARDS=3` at `77f8a6c8`, on 95 test files: the issue's Verify files, every test file that runs or reads either script (`git grep -l 'bootstrap.py\|validate-family' -- tests`), `tests/test_family.py` and the trio (`test_repo_hygiene.py`, `test_agents_md_codes.py`, `test_cli_reference.py`):
  - git 2.43.0: 6335 passed, 59 skipped, rc 0 (shards 1596 + 5, 2203 + 27, 2536 + 27).
  - git 2.47.1: 6335 passed, 59 skipped, rc 0, the same in each shard.
  - The 59 skips are the same on both versions, and none is in the new file. 27 wait on #775 (siblings.py's #733 rules). The rest are templates that already quote a value (22), Windows or a disk that ignores case (5), and empty parameter sets (5).
- On the predicted squash of #819 and this branch onto main `33419925` (`9aca0f4d`; see the merge checks), git 2.43.0, `run9.sh`. Main has changed `tests/test_bootstrap_spelling.py` and `tests/test_family.py` since `233a334e`.
  - `tests/test_family.py`: 112 passed.
  - `tests/test_bootstrap_spelling.py`, the new file, `test_family_head_lost_gitmodules.py`, `test_family_docs_one_spelling.py`, `test_bootstrap_after_bump.py`, `test_validate_family_remedies_typed.py`, `test_family_bootstrap_gate_order.py`, `test_pinned_copy_text.py` and the trio: 656 passed.

- New tests: `tests/test_family_holder_bootstrap_texts.py`, 48 cases, all passing at `77f8a6c8`.
- On `233a334e`'s two scripts, with this branch's tests: 27 fail and 14 error. The 14 are the seven-layout guard test, which calls the gate's `_guarded_move`, a function `233a334e` does not have. 7 pass there by design: the three credential-skip cases (line 1's other half, and comment (a), which held), the leg case, url-less-then-absent, the value-refused order and the no-`-f` case. Each guards the fix against going too far.
- Mutants, one or more per new hunk, all 20 caught:
  - the advice always printed;
  - the advice never printed;
  - the advice's "does not name" wording dropped;
  - the advice kept only for rows the credential leaves out (the Direction's literal rule);
  - the exit code always make's;
  - make's stderr not passed on;
  - the first `Error` line read in place of the last;
  - the STRAY `-f` dropped;
  - the STRAY's "nothing left to commit" turned round;
  - the move's `-f` dropped;
  - `_forced` ignoring HEAD;
  - `_forced` ignoring whether a checkout is there;
  - the `-L` dropped;
  - the nothing-there clause dropped;
  - the `-f` clause dropped from the text;
  - the absent finding's put-back dropped;
  - every url-less finding putting back;
  - every absent finding putting back;
  - the value-refused flag ignored;
  - `_check_members` passing no flag.

  `_forced` ignoring HEAD is caught by the existing `test_a_checkout_at_the_variant_is_kept_and_handled_as_the_text_says`.

- Both files are in `FAMILY_COPIED_VERBATIM`, so each holder takes one `upstream-changed` row per file.
- No committed digest or byte comparison names either file: `templates/family-root/contracts/shape-pin.yaml` holds only placeholders, and `git grep` finds no sha256 of them in `tests/`.
- `docs/cli.md` takes only `validate-family.py`'s first docstring line, which is unchanged.
- No finding code or refusal code is new or renamed. `AGENTS.md`, `templates/family-root/AGENTS-shape.md` and both READMEs say nothing these lines change. Each script's module docstring says what its lines now do.
- Cognitive complexity, counted by hand:
  - `fetch_members` drops from 15 to 14, since its failure report moved to `fetch_incomplete`.
  - `_check_gitlinks_registered` was over 15 on base (18) and drops to 17; its `def` line keeps its first line.
  - `_check_members` stays at 14.
  - Every new function is at 6 or under.
- The new tests share their helpers within the file, and no assert joins conditions with `and`.

`git merge-tree --write-tree` of `77f8a6c8`, fetched 2026-10-18 at 05:02Z:
- origin/main `33419925`: clean. Main has moved past `d7befb91`, and #822 landed in it as `6fbf3301`.
- #865 `f7072d08`: clean.
- #870 `c0d8dd16` (lane 4's round 1; it edits both scripts): clean, with no conflicting hunk.
- #822's head `0519f9fd`: conflicts in both scripts, and `233a334e` (#819's head) conflicts with it in `validate-family.py` too.
  - This is a criss-cross with two merge bases, `b9b70692` and `5afb24b9`. Since `b9b70692`, #822 changes neither script.
  - #822 has landed, and its content is in main, which merges clean.
- Predicted squash of #819 and this branch onto main `33419925` (`9aca0f4d`, tree `ee2ae53c`): clean against #865, #870 and #822's head. #822's head leaves the tree unchanged.

- After line 3's lines, typed as printed in the `git mv` state, the checkout `git mv` moved stays at `Members/IRRS`, untracked. Measured: the gate passes in every mode, and `family.py add` then refuses it, `family-members-dir-variant`, with `mv -- Members/IRRS <new name>`. Neither the STRAY finding nor `member-gitlink-removal-staged` says what that directory is, or that its commits are in the member's store. The wrong-case move's closing sentences say both. Filed in #879 (C4).
- `scripts/family.py`'s guard: see line 4.

Review: rv876's independent review, READY at `77f8a6c8` with no MUST (#876 issuecomment-6246717354); its SHOULD 1, a claim in the body, is fixed in the body, and its five candidates are filed as #879. CI (Brett Heap's ruling 27 and his fast-refresh ruling of 2026-10-15T22:48:35Z): H0 a7f6c56 (the restack of the branch onto main after #819 landed) passed tests, tests-git-bash and SonarCloud; the head ad351e4 is a clean update-branch merge of main def45de, with tests-git-bash and SonarCloud green; and the local gate passed on its predicted squash tree 7715483 over main def45de: the bootstrap_spelling, family_head_lost_gitmodules, family_holder_bootstrap_texts, family_members_not_a_list, validate_family_duplicates, repo_hygiene, agents_md_codes, cli_reference sets, 616 passed. Landed on Brett Heap's word of 2026-10-17T21:47:28Z, in lane 5's session (posted on #473 as issuecomment-6241758069): "merge lane 5's v1.1.1 PRs once reviewed and green".

Closes #839

Lane: openreposhape-5 (openRepoShape-5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment