Skip to content

shape-doctor.py: a conflicted shape pin's NEXT reads both sides' pins, so a copy conflicted beside it that only the side not taken has a files: row for is named in its why and left_for_a_person for a person to merge, and never taken (#687) - #736

Merged
brettheap merged 3 commits into
mainfrom
l3-687-pin-restore-other-side
Oct 17, 2026

Conversation

@brettheap

@brettheap brettheap commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

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

Closes #687.

Head: 3dfe2fde9b6922f796afaa9cc73bd236105ab5a0. That is #687's commit b4c883f1, a merge of main 66b230ed (clean), and one commit for RV-b's review round. Line numbers below are at the head.

What changed and why

shape-doctor.py's pin_restore built the copies of a conflicted shape pin's merge from the TAKEN side's pin alone. So a file conflicted beside the pin that only the side NOT taken has a files: row for was neither taken nor named. The why and detail.left_for_a_person said nothing of it. Only the merge row listed it unmerged.

Reproduced on main de926142 before any change, as the mirror of test_a_copy_the_other_sides_pin_has_no_row_for_is_left_to_a_person: both_pure's merge, with stage 2 of contracts/shape-pin.yaml restaged without the Makefile's row. The doctor said INVALID (merge), and:

  • NEXT: checkout --ours -- contracts/shape-pin.yaml scripts/bootstrap.py;
  • taken_with_the_pin ['scripts/bootstrap.py'], left_for_a_person ['project.yaml'];
  • the why named scripts/bootstrap.py and project.yaml, and never the Makefile;
  • the merge row said 4 path(s) unmerged in the index ...: Makefile, contracts/shape-pin.yaml, project.yaml, scripts/bootstrap.py.

Every claim in the issue held on de926142.

The change, all in pin_restore and a helper of it (shape-doctor.py):

  • Both pins are read (:3826-3839). kept is the taken stage's rows and other the other stage's. kept_reads says whether the taken stage's pin reads at all. copies stays the kept pin's conflicted copies, which are proved and taken, or left to a person, exactly as before. other_side is the conflicted paths only the other pin has a row for.
  • Never taken. other_side never reaches taken_with_the_pin. as_the_standard_wrote_it (:4099) already proves nothing of a side whose pin has no row for the copy, and that stays.
  • Named (:3859-3862, :3893, and the helper other_sides_copies at :3952-3995). The paths join left_for_a_person, and the why says what is known of them:
    • The usual case (the pin taken here reads, and both sides hold the file): "<path> is conflicted too, and the other side's pin calls it a copy where the pin taken here does not, so neither side is taken whole: once the merge is finished it is the project's own file, and a person merges it by hand, keeping the hunks git merged".
    • The pin taken here does not read: "... while the pin taken here does not read as a pin, so neither side is taken whole: a person merges it by hand, keeping the hunks git merged". It makes no claim about whose file it becomes.
    • One side deleted it (a delete/modify conflict; one of stages 2 and 3 is missing): "..., and one side deleted it, so neither side is taken whole: once the merge is finished it is the project's own file if kept, and a person keeps or deletes it by hand". The second clause is said only where the pin taken here reads.
    • Plural forms apply, and a mix gives one sentence per kind.
  • No --accept-local sentence for it. That sentence is still said only where a copy the kept pin has a row for is left. apply refuses --accept-local on a path the pin has no row for (update-accept-local-unknown, update-shape.py:3273-3288), so saying it here would send the reader to a refusal.
  • The manifest kinds row follows. It reaches the same helper, so it prints the same NEXT and why, and unreadable_next copies left_for_a_person as it did. A copy under contracts/ that only the other side pins is unmerged there while the pin is conflicted, as every file the why gives a person is. Once the kept pin reads, with no row for it, it is project. unreadable_next's docstring now says so (:4311-4325).
  • The docstring says which pin decides:
    • At :3648-3653, the copies are the files EITHER side's pin has rows for, and only the taken side's can be taken.
    • A new paragraph at :3685-3707 covers the other side's copy, a kept pin that does not read, and a deletion.
    • The rule at :3714-3719 now says a file of the project's own is one NEITHER side's pin has a row for.
    • pin_at's THE ROWS paragraph (:3603-3607) is updated to match.

Why naming it is right, and today's silence was not a deliberate rule

A reviewer could read the old behaviour as the rule "a file of the project's own is never named: its merge is theirs". It was not that rule at work. It came from which pin the code happened to read:

Review round (RV-b at b4c883f1: one SHOULD, two NITs)

Tests

In tests/test_doctor_pin_restore_merges.py:

  • test_a_copy_only_the_other_sides_pin_has_a_row_for_is_named is the issue's mirror. The kept side's pin is restaged without the Makefile's row. Then:
    • the Makefile is in left_for_a_person and named in the why, and is not in the NEXT;
    • scripts/bootstrap.py is still taken;
    • no --accept-local sentence is said;
    • both rows say the same;
    • the NEXT run pasted leaves the Makefile at stages 1, 2 and 3 with its bytes unchanged.
  • test_copies_only_the_other_sides_pin_has_rows_for_are_named_together[rows-gone|unreadable]: the kept pin has no row for either copy, or does not read. Nothing is taken, and one plural sentence names both, with no claim about whose file each becomes when the pin does not read.
  • test_a_copy_one_side_deleted_is_left_to_a_person[...-its-row]: the delete/modify state, with the kept pin holding the row (unchanged) or not (keeps or deletes).
  • test_a_naming_copy_only_the_other_sides_pin_has_a_row_for_is_named covers contracts/repository-naming.yaml. It is unmerged in the manifest kinds row while the pin is conflicted, and project once the kept pin reads.
  • test_an_add_add_only_one_sides_pin_calls_a_copy_is_named is the natural flow:
    • at C2 the standard adds SECURITY.md to COPIED_VERBATIM, and at C3 its Makefile gains a line;
    • shape/a runs apply --at C2 --add SECURITY.md;
    • shape/b runs apply --at C3 and writes its own SECURITY.md;
    • the merge leaves AA SECURITY.md beside UU contracts/shape-pin.yaml;
    • SECURITY.md is named and not taken, and the NEXT leaves it unmerged;
    • a person keeps the project's own, merges project.yaml, commits, and the doctor says COMPLIANT.
  • Each new case failed before its fix. shape-doctor.py pin_restore: a copy conflicted beside the shape pin that only the side NOT taken has a files: row for is neither taken nor named, so the conflicted pin's why and left_for_a_person say nothing of a file the merge still holds unmerged #687's cases were run against main's shape-doctor.py, and this round's two against b4c883f1's. Each failed on the missing name or the wrong sentence.
  • :676-699's test still passes with its assertions unchanged. Its restage block is now the shared helpers a_pin_restaged and a_pin_without_a_row, so the new tests do not copy it (Sonar's new-duplication gate). assert_only_bootstrap_is_taken takes the sentence it expects of the Makefile.
  • assert_both_rows_name_it also holds the two rows' left_for_a_person equal.

On the head, with a stub gh first on PATH that 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_doctor_pin_restore_merges.py tests/test_doctor_next_is_a_command.py tests/test_doctor_recorded_answers.py tests/test_repo_hygiene.py tests/test_cli_reference.py
204 passed in 139.35s (0:02:19)

/usr/bin/python3 -m pytest -q -p no:cacheprovider --basetemp=<scratch>/bt-sd2 -o tmp_path_retention_policy=failed tests/test_shape_doctor.py
428 passed, 3 skipped, 1 xfailed in 395.36s (0:06:35)

These are every test file of the manifest kinds row (grep -l 'manifest-kinds\|manifest kinds\|LABEL_MANIFEST_KINDS' tests/*.py), plus hygiene and the CLI reference.

Pinned shape copies

None. shape-doctor.py is in no copy list (COPIED_VERBATIM, COPIED_FROM_SHAPE, FAMILY_COPIED_VERBATIM, FAMILY_COPIED_FROM_SHAPE), so no project sees an upstream-changed row.

Docs

None changed. README.md, AGENTS.md, docs/cli.md and docs/handbook.html say nothing of a conflicted pin's restore. The one mention is docs/release-notes/v1.0.0.md, a record of v1.0.0.

🤖 Generated with Claude Code

Summary by Sourcery

Update conflicted shape-pin restoration to inspect both pins, identify other-side-only copies for manual merging, and leave them unmerged while taking only files proven by the selected side.

New Features:

  • Name conflicted files as requiring manual merging when only the non-taken shape pin identifies them as copies.

Bug Fixes:

  • Ensure pin restore considers both sides' shape-pin file rows so affected files are not omitted from why and left_for_a_person.

Enhancements:

  • Clarify shape-pin restore rules and ensure files identified only by the other side are never taken or incorrectly offered to --accept-local.
  • Keep manifest-kind diagnostics consistent with the expanded manual-merge handling.

Tests:

  • Add coverage for single and multiple other-side-only copies, naming copies, manifest-kind status transitions, and add/add conflicts.

…, so a copy conflicted beside it that only the side not taken has a `files:` row for is named in its `why` and `left_for_a_person` for a person to merge, and never taken (#687)

`pin_restore` built its copies from the TAKEN side's pin alone, so a
conflicted file only the other side's pin calls a copy was neither taken
nor named: the `why` and `left_for_a_person` said nothing of it, and only
the `merge` row listed it unmerged. It now reads both stages' pins:

- a copy the kept pin has a row for is proved and taken, or left to a
  person, as before;
- a copy only the other side's pin has a row for is never taken, since
  `as_the_standard_wrote_it` proves nothing of a side whose pin has no row
  for it, and the `why` now says the other side's pin calls it a copy and
  the pin taken here does not, so once the merge is finished it is the
  project's own file and a person merges it by hand, keeping the hunks git
  merged. It joins `left_for_a_person`. The `--accept-local` sentence is
  not said for it, since `apply` refuses that flag on a path the pin has
  no row for.

The `manifest kinds` row prints the same NEXT and `why`, through the same
helper. The docstring rules from the review of #358 now say which pin
decides: a file of the project's own is one NEITHER side's pin has a row
for.

Tests: the mirror of the other side's missing row (the kept side's pin
restaged without the Makefile's row), the same with both copies' rows
gone, the naming copy under `contracts/` (`unmerged` in the `manifest
kinds` row while the pin is conflicted, `project` once the kept pin
reads), and the natural flow: an add/add where one side's `apply --add`
took a file the standard added and the other wrote its own, followed to
COMPLIANT.

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

sourcery-ai Bot commented Oct 16, 2026

Copy link
Copy Markdown

Reviewer's Guide

Updates conflicted shape-pin restoration to read both pins: only copies named by the taken pin can be automatically taken, while copies named solely by the other pin are explicitly given to a person for manual merge, with consistent manifest-kind reporting and comprehensive regression tests.

Sequence diagram for conflicted shape-pin restoration

sequenceDiagram
    participant Doctor as shape-doctor.py
    participant KeptPin as Taken side pin
    participant OtherPin as Other side pin
    participant Index as Git index
    participant Person as Person

    Doctor->>KeptPin: pin_at(stage)
    Doctor->>OtherPin: pin_at(other_stage)
    KeptPin-->>Doctor: kept copy paths
    OtherPin-->>Doctor: other-side copy paths
    Doctor->>Index: Restore kept copies
    Note over Doctor,Index: Only copies in the taken pin are eligible for automatic taking
    Doctor->>Person: Name other-side-only copies in why and left_for_a_person
    Person->>Index: Manually merge hunks
Loading

Flow diagram for taken and manual conflicted copies

flowchart TD
    A[Read both conflicted pin stages] --> B{Which pin has the files row?}
    B -->|Taken pin| C[Prove and possibly take copy]
    B -->|Only other pin| D[Leave unmerged]
    D --> E[Name in why and left_for_a_person]
    B -->|Neither pin| F[Treat as project's own file]
    C --> G[Report restoration result]
    E --> G
    F --> G
Loading

File-Level Changes

Change Details Files
Teach conflicted shape-pin restoration to inspect both merge-stage pins while restricting automatic taking to the selected side.
  • Read the kept and opposing pin rows separately.
  • Keep existing proof-and-take behavior limited to copies named by the kept pin.
  • Track paths named only by the opposing pin as person-merged, never automatically accepted.
  • Add singular and plural explanatory why text and include those paths in left_for_a_person without suggesting --accept-local.
  • Update pin_at and pin_restore docstrings to document the two-pin classification and taking rules.
shape-doctor.py
Add regression coverage for opposing-side-only copies across restore output, manifest kinds, and natural add/add conflicts.
  • Refactor pin restaging into a reusable helper and extend apply setup for extra arguments.
  • Assert matching why and left_for_a_person data across shape-currency and manifest-kinds rows.
  • Cover one or multiple opposing-side-only copies, naming copies, and an apply --add add/add scenario.
  • Verify NEXT leaves these files unmerged and preserves bytes, while shared copies remain taken and final compliance still succeeds.
tests/test_doctor_pin_restore_merges.py

Assessment against linked issues

Issue Objective Addressed Explanation
#687 Determine conflicted copies from both sides’ shape pins, while allowing only copies listed by the taken side’s pin to be taken with the pin. ✅
#687 Name copies listed only by the other side’s pin in the restore explanation and left_for_a_person, without including them in the NEXT command. ✅
#687 Document the distinction between copies listed by either pin and copies eligible to be taken, and test the behavior in the merge and manifest-kinds reporting paths. ✅

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-16T23:12:07Z) Lane 3's independent review of #736 at b4c883f1 (RV-b, ruling 8): READY, with no MUST, one SHOULD and two NITs. The SHOULD is a false sentence in a rare state, but it is text this PR adds, so the writer takes it and the NITs in one small round before landing.

Every #687 claim holds on main. In the mirror state, the NEXT takes only scripts/bootstrap.py, the why never names the Makefile, and the status shows UU Makefile.

On the head:

  • The NEXT is byte-identical, and scripts/bootstrap.py is still taken.
  • The Makefile is named in the why and in left_for_a_person. There is no --accept-local sentence.
  • The manifest kinds row agrees.
  • The NEXT, pasted, leaves the Makefile at stages 1, 2 and 3 with its bytes unchanged.

Nothing more is taken. Across 10 probe states, on de926142 and on the merge with 66b230ed, the NEXT is byte-identical to main's.

  • Six states are fully identical: both sides pure, the other pin lacking the row, the other pin unreadable, both unreadable, a rename, and both pins lacking the row.
  • Four only add a name and a sentence: the mirror, the kept pin unreadable, and delete/modify both ways.

SHOULD

  • S1: with the KEPT side's pin unreadable, the new sentence claims what the doctor cannot know. At shape-doctor.py:3576-3581 and :3635-3647, the sentence says "the other side's pin calls them copies where the pin taken here does not … once the merge is finished each is the project's own file", but the pin taken here does not read.
    • Fix: where the kept stage's pin is None, either leave other_side empty (main's behaviour), or say only that the other side's pin calls them copies and the pin taken here does not read.
    • Test: add one; no test covers this branch now.

NITs

Mutants: all six fail the new tests:

  • take the other side's copies in the NEXT;
  • drop the naming;
  • drop the sentence;
  • drop left_for_a_person;
  • treat the other pin's rows as kept;
  • swap the two pins.

Tests (stub gh):

Merges: clean with main, with #734, #737, #740 and #742 (9f24bf31 and the merged 3027937d). #736 changes no line of as_the_standard_wrote_it, so the feared conflict with #742 does not happen.

CI at the head: green. The cross-module test import costs 12 ms.

Lane: openreposhape-3 (openRepoShape-3)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…` names the other side's copies without saying whose file each becomes, and a copy one side deleted is one a person keeps or deletes (review of #736)

RV-b's review of #736 at b4c883f:

- S1. Where the pin taken here does not read, the new sentence said it
  "does not" call the files copies and that each becomes the project's own
  file, and neither is known. `other_sides_copies` now says the pin taken
  here does not read as a pin, and says nothing of whose file each becomes.
  The files are still named: the other side's pin calls them copies, and
  dropping them would bring back #687's silence where the reader knows
  least.
- N2. A delete/modify conflict holds one side's stage alone and has no
  hunks to keep, so for such a file the sentence says a person keeps or
  deletes it. A content conflict, an add/add included, keeps its words,
  "merges it by hand, keeping the hunks git merged".
- N1. `unreadable_next`'s docstring says a `contracts/` copy only the
  other side's pin lists is `project` once the kept pin reads, and
  `unmerged` only while the pin is conflicted.

Tests: the plural case with the kept pin restaged as `files: [`, and the
delete/modify case with only the other side's pin holding the Makefile's
row. Each fails on the doctor before this commit.

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

Copy link
Copy Markdown

@brettheap

Copy link
Copy Markdown
Contributor Author

(lane openRepoShape-3, 2026-10-16T23:29:22Z) Lane 3's independent re-verification of #736 at 3dfe2fde (RV-b, ruling 8): READY. S1, N1 and N2 are fixed and true.

The items

  • S1: with the kept side's pin unreadable, the row now says "the other side's pin calls them copies while the pin taken here does not read as a pin, so neither side is taken whole: a person merges each by hand, keeping the hunks git merged" (other_sides_copies, shape-doctor.py:3952-3995). Every clause is true, and the claim about whose file each becomes is gone. Keeping the names is right: the other side's rows are the only signal, and the NEXT is the same either way.
  • N1: unreadable_next's docstring (:4311-4325) now matches what the naming-copy test measures.
  • N2: both delete/modify directions read "…and one side deleted it, so neither side is taken whole: once the merge is finished it is the project's own file if kept, and a person keeps or deletes it by hand". The content-conflict sentence is byte-identical to round 1.

Probes, 10 states, against main 66b230ed:

  • The NEXT is byte-identical to main's in all 10.
  • Six states are fully identical.
  • Four only add a name and a sentence.

Mutants: all nine fail the tests, including three new ones:

  • the kept pin always read;
  • silence where it does not read;
  • a deletion never told apart.

Tests (stub gh):

Merges: clean with each open lane-3 PR and with all four in sequence.

Pre-existing, filed for v1.1.1:

  • where the kept side's pin does not read, the NEXT still takes that side, and when run it names itself again;
  • a copy renamed on one side conflicts under a name no pin lists, so only the merge row names it.

Lane: openreposhape-3 (openRepoShape-3)

This was referenced Oct 16, 2026
@brettheap
brettheap marked this pull request as ready for review October 17, 2026 01:22
@sourcery-ai

sourcery-ai Bot commented Oct 17, 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 22 hours and 28 minutes by commenting @sourcery-ai review. Upgrade to get a review now.

@brettheap
brettheap merged commit bf1f099 into main Oct 17, 2026
7 of 8 checks passed
@brettheap
brettheap deleted the l3-687-pin-restore-other-side branch October 17, 2026 01:23
brettheap pushed a commit that referenced this pull request Oct 17, 2026
main gained 25 pull requests since de92614: #634, #699, #671, #719,
#701, #717, #700, #702, #722, #720, #725, #721, #728, #705, #727, #718,
#744, #743, #749, #740, #737, #734, #742, #751 and #736. No conflict:
AGENTS.md, adopt-project.py and docs/cli.md merged as text, and
README.md and docs/handbook.html are unchanged by the merge. README.md
is 1473 lines, its cap; AGENTS.md 494, its cap; every handbook marker is
current; `render-cli-reference.py --check` is ok.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap pushed a commit that referenced this pull request Oct 17, 2026
…es every reason THIS checkout cannot answer, and the notes are current to main bf1f099 with the 25 pull requests since de92614 (#563, #683)

- W683 and RV-f's N4 on #742: README's exit-3 list (Keeping a project's
  shape current, :1208-1220) and its handbook twin (`doctor`) named only a
  pin whose commit this checkout does not carry. They now say, as
  shape-doctor.py's EXIT CODES does: a pin's commit it lacks, a partial
  clone missing what is compared (#683), a read that fails
  (`update-upstream-unreadable`), a crashed validator of its own (#278,
  #410), or its own naming policy not loading (#678). +1 line, with
  tests/readme-cap.d/683.txt; README.md is 1474, its cap (1422 + 2 + 5 +
  12 + 22 + 10 + 1). The two `Keeping a project's shape current` markers
  move: `doctor` text and marker, `current` marker only.
- The notes, "What changed since v1.0.1", one entry per pull request from
  `git log --first-parent de92614..bf1f099`, each from its squash commit and
  the issues GitHub says it closes:
  - adopt-project.py: #699, #719, #749, #727, #717, #728 (with #621's
    hint), #743 + #720 + #718, #722, #721;
  - update-shape.py, bump-leg.py and a project's own scripts: #702 (with
    the pinned validators), #634;
  - scaffold, setup, family and the shared readers: #725, #671, #744;
  - shape-doctor.py: #742, #701, #737, #751, #736, #734, #700 + #705,
    #740;
  - install, CI and the docs: the exit-3 sentence above.
- Counts (99 pull requests since bfed143; 109 of 135 milestone issues
  closed at 10:28Z), the "current to main" sha and the open list (26) are
  at main bf1f099, still placeholders for tag time.

Left for round B: lane 5's eight lines (#630 issuecomment-6227129879),
whose pull requests #726, #747, #682, #724, #746, #745 and #738 are open
drafts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap pushed a commit that referenced this pull request Oct 17, 2026
Main at bf1f099 (#736), 29 commits past the branch's base 4ce9045, among them #671 and #728 in scripts/family.py. No conflicts: git merged every file on its own.

Lane: openreposhape-4 (openRepoShape-4)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap pushed a commit that referenced this pull request Oct 17, 2026
…d A3 (three SHOULDs, four nits), and notes entries for the seven pull requests landed since bf1f099, current to main 2000488 (#563, #683)

Round A4, from lane 2's review (#630 issuecomment-6233033833, READY):
- SHOULD 1: the doctor's exit-3 list names "a validator of its own that
  crashed or could not be run", so the case of one that cannot be given
  its `PYTHONPATH` (shape-doctor.py's EXIT CODES, #278) is listed too:
  README.md:1208-1222, its handbook twin (`doctor`) and the notes' docs
  line. The paragraph re-wraps to one more line:
  tests/readme-cap.d/683.txt is +2, and its text now says which causes
  the EXIT CODES block names and that #742 added the partial clone and
  #701 the naming policy. README.md is 1475 lines, its cap (1422 + 2 + 5
  + 12 + 22 + 10 + 2). Both `Keeping a project's shape current` markers
  move to c728c970: `doctor` text and marker, `current` marker only.
- SHOULD 2: #727's entry names the leg description, whose em dash stopped
  every `execute` against GitHub at the first leg.
- SHOULD 3: #749's entry names `source-ambiguous`, for an `org/repo` that
  is also a directory here.
- Nits: the doctor section's three near-duplicate pairs are merged (#636
  with #737, #632 with #700 and #705, #586 with #736); #718 has its own
  bullet; #634's entry names `git_out`'s spelling for every caller and
  #744's the gate's `--link-source-label` and `--link-remedy`; the header
  comment says 2026-10-17 at main 2000488.

Wave B1, `git log --first-parent bf1f099..2000488`: #706 (#664), #703
(#696), #670 (#646), #682 (#648), #726 (#667), #730 (#666) and #724
(#665), each from its squash commit; #726's and #682's from lane 5's lines
(#630 issuecomment-6227129879), #682's with the unmerged `.gitmodules`
its last round added, and #724's with lane 5's note that v1.0.0's
test-hygiene line on the generator's `.pyc` files closes. The counts (106
pull requests; 116 of 135 milestone issues closed at 11:53Z), "current to
main" and the open list (19) are at main 2000488, still placeholders.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brettheap added a commit that referenced this pull request Oct 17, 2026
… in command scope and in a global config that includes the person's own, so a `copytree` of a fixture no longer meets a detached repack deleting its loose objects (#645) (#713)

**What changed.** One session fixture in `tests/conftest.py`, `no_auto_maintenance` (autouse), and one new test file, `tests/test_no_auto_maintenance.py`. No shipped file and no other test file changes. Four commits: the fix and its tests (e2110f8), a comment (accc7d2), and the fix round below (8cfc8eb, and 979bb39, which only shares a test helper).

The fixture sets `gc.auto=0` and `maintenance.auto=false` for every git the session starts, in two scopes:

- **Command scope** (`GIT_CONFIG_COUNT`), appended after the entries the environment already holds (the Windows job holds two). Every git the suite starts, every git a tool under test starts, and every git a `git submodule` starts inherits it. It outranks every config file.
- **A global config file** (`GIT_CONFIG_GLOBAL`, in the basetemp). It includes the global config git would read without it (the file `GIT_CONFIG_GLOBAL` already named, or `$XDG_CONFIG_HOME/git/config` and then `~/.gitconfig`) and sets the two keys after it. This one is for the `receive-pack` a push starts in a local remote: git starts it with `GIT_CONFIG_COUNT` and `GIT_CONFIG_PARAMETERS` removed, so command scope never reaches it. Where `HOME` is unset the `~/` paths are left out, and an empty `GIT_CONFIG_GLOBAL` includes nothing (fix round, below).

A comment beside #440's note in `clone_just_pushed` says why, names both sites, and says what is left.

**Why.** `shutil.copytree` reads a repository's `objects/` one file at a time. The suite copies a repository a fixture has just committed in, fetched into or pushed to in 139 places. Each `git commit`, `merge` and `fetch`, and the `receive-pack` in a remote after a push, runs `git maintenance run --auto` as it finishes. From git's source:

- git 2.46: the auto run stays in the foreground.
- git 2.47: it is started with `--detach`, takes `objects/maintenance.lock` and then detaches, so the command returns while it runs.
- git 2.52 and 2.53: the default strategy is still `gc`, which does nothing below 6700 loose objects.
- git 2.54: the default strategy is `geometric`. Its repack runs when packs are out of geometric order, or once its estimate, taken from `objects/17` alone, is above 256 loose objects, so two loose objects there are enough (`maintenance.geometric-repack.auto`, 100, rounded up). It deletes the loose objects it packed.

So on a current git, a copy taken just after a commit can find a loose object gone. It happened twice on the Linux runner:

- `holder` (`tests/test_family.py:129`), in `test_family_member_name.py` (lane 1's report on #473);
- `the_standard_moved` (`tests/test_doctor_pin_restore_merges.py:99`), a copy of the session's `standard` with its `.git`, in `test_copies_neither_side_edited_are_taken_whole_with_the_pin` on #623's run 38895011713: `shutil.Error ... standard5/openRepoShape/.git/objects/1e ... [Errno 2]` (lane 2's #473 comment 6215422231). The same function copies `moved` at :102 just after a commit in it.

Both are covered by the same session setting: the commits before each copy run under `os.environ` (`commit_all` passes `{**os.environ, ...}`).

A listing of `.git/objects` taken before and after such a repack also differs: lane 3's `objects changed` in `tests/test_family_bump_remove_rollback.py` (#736's Linux run 38981191418), 37 loose objects packed into two. The same setting covers it (i645 on #645).

**Decisions and evidence.**

- *Option A (the suite's environment), not B or C.*
  - B (config in each copied repository) would have to reach repositories the tools under test create themselves, such as the bare remotes `scaffold-project.py --local-remote-dir` makes and pushes into before a test sees them. A config written after that first push is too late.
  - C (a copy that survives a repack) cannot be made safe. A copy can list `objects/pack/` before the new pack lands, and then a loose-object directory after it was emptied. That copy succeeds and is missing objects.
  - A covers all 139 places from one place.
- *Two scopes, not one.* Probe on git 2.43, pushing into a local bare remote with `receive.denyDeletes=true` given each way:
  - with `GIT_CONFIG_COUNT`, the branch was deleted;
  - with `-c`, deleted;
  - through a `file://` URL with `GIT_CONFIG_COUNT`, deleted;
  - with `GIT_CONFIG_GLOBAL`, `remote: error: denying ref deletion` (refused).

  So command scope alone misses the remotes, and ten copies are of bare remotes. A global file alone would be dropped wherever a test gives git a global file of its own (14 test files set `GIT_CONFIG_GLOBAL`, five more through `excluding()`). No test file replaces both scopes and also copies a repository. rv645, the reviewer, repeated the `receive.denyDeletes` probe and got the same answers, and instrumented the git calls of the batches it finished before the checkpoint: the only receive-packs that ran without both settings were in `test_family_add_excludes.py` and `test_adopt_clone_template.py`, neither of which copies a remote.
- *Which verbs start auto maintenance* (git 2.43 trace2): `commit`, `fetch`, `pull` and `submodule update --remote` (its fetch in the submodule) start `git maintenance run --auto`. A push's `receive-pack` starts `git gc --auto --quiet`. `clone`, `submodule add` and `submodule update --init` start none.
- *Behaviour no test checks is unchanged.* The person's own global config is still read: it is included, and it comes first. A test of that is part of the new file. The tests that read or set a global config (`test_git_helper_identity.py`, `test_adopt_git_reads.py`, `test_tracking_branch_ref.py` and others) set their own, which replaces the session's.

**Fix round (8cfc8eb), on rv645's review** (no MUST; two SHOULD, five NIT).

- **SHOULD 1: with `HOME` unset every git was fatal.** The file's include of `~/.config/git/config` and `~/.gitconfig` made git exit 128, `error: could not expand include path '~/.config/git/config'`, in every command the session started. The estate runs with `HOME` set, so nothing failed here; an `env -i` or hermetic runner would have lost the whole suite.
  - `session_global_config` now writes a `~/` path only where `home_is_known()` (`HOME` is set, or `os.name == "nt"`, where Git for Windows finds a home itself), and writes no `[include]` header when nothing is included. An `XDG_CONFIG_HOME` that is set is still included, because it needs no `HOME`.
  - The two `test_a_persons_own_global_config...` cases skip where `HOME` was unset. They skip on the environment and not on the file's bytes, so a fixture that dropped its includes still fails them (probe M5 below).
  - A new test, `test_where_home_is_unset_the_file_is_one_git_can_read` (with and without `XDG_CONFIG_HOME`), reads the file back through `git config --global --includes`. It is skipped on Windows. It and the empty-value test share one helper, `probe_read_from_the_session_file`, so the new tests do not repeat a body for SonarCloud's duplication gate.
  - Measured under `env -u HOME -u XDG_CONFIG_HOME`, `run9.sh`, git 2.43.0:

    | file | before (accc7d2) | after (8cfc8eb) |
    |---|---|---|
    | `tests/test_clone_just_pushed.py` | 1 passed, 6 errors | 7 passed |
    | `tests/test_no_auto_maintenance.py` | 3 failed, 2 passed, 2 errors (7 tests) | 9 passed, 2 skipped (11 tests) |
    | `tests/test_family_member_name.py` | not run | 13 passed |

    The 2 skips are the two `test_a_persons_own_global_config...` cases.
  - Where `HOME` is set and the file is written as before, nothing changes: across 27 combinations of `HOME` (set, unset, empty), `XDG_CONFIG_HOME` (set, unset, empty) and `GIT_CONFIG_GLOBAL` (unset, a file, empty), the file's bytes are the same as accc7d2's in the 12 where `GIT_CONFIG_GLOBAL` names a file or `HOME` is set with `GIT_CONFIG_GLOBAL` unset, and differ in the 15 where `HOME` is not set or `GIT_CONFIG_GLOBAL` is empty.
- **SHOULD 2: nothing guarded the fixture's scope.** At function scope all seven tests passed (rv645's probe), and the session fixtures `scaffolded` and `family` would have been built before the setting existed. A new test, `test_the_settings_are_in_place_before_a_session_fixture_builds_a_repo`, takes a session fixture as its FIRST argument, which reads `GIT_CONFIG_GLOBAL` and `GIT_CONFIG_COUNT` as that fixture sees them, and compares them with what the test sees and with the fixture's file. Pytest builds the higher scope first, and then an autouse fixture, so the fixture is only in place early enough if it is session scope and autouse.
- **NIT 1: the receive-pack test failed under a person's global `core.hooksPath`**, which hid the remote's own `post-receive` hook (`FileNotFoundError` on `read-by-receive-pack`). The test now sets `core.hooksPath` to the remote's own `hooks` directory in the remote's local config, which outranks a global one. Measured with `GIT_CONFIG_GLOBAL` naming a file that holds `core.hooksPath`: before, `1 failed, 4 passed, 2 skipped`; after, `9 passed, 2 skipped` (11 tests).
- **NIT 2: an empty `GIT_CONFIG_GLOBAL=` was read as unset.** Git 2.43 reads it as "no global config" (checked here: with `HOME` holding a `.gitconfig` probe, `GIT_CONFIG_GLOBAL=` does not read it). The fixture now includes nothing for an empty value: `if named is not None: included = [...] if named else []`. A new test, `test_an_empty_GIT_CONFIG_GLOBAL_is_no_global_config_and_includes_nothing`, fails on the old `if named:`.
- **NITs 3 to 5: no code change.**
  - NIT 3: `test_family_dry_run.py`'s own `NO_MAINTENANCE` is redundant now. It lives in another test file that this PR leaves alone, and it is harmless.
  - NIT 4: a `git config --global` read without `--includes` sees only the session's two keys. The only reader is `setup-project.py:1105`, behind `os.name == "nt"`, and no test reads it. `_credential_helper_configured` uses `--get-all` with no scope, so it still sees the person's config. The conftest comment already says so.
  - NIT 5: the version facts in the conftest comment ("2.47 detaches", "2.54 default geometric") are from git's source tags and are not run here: only git 2.43.0 is installed. The comment is unchanged.

**Tests** (`run9.sh`: gh stub first on PATH, identity blanked; git 2.43.0, Python 3.12, pytest 9.1.1).

- On the fix round (8cfc8eb; 979bb39 differs from it in the new test file alone, and the batches that read that file were run on it):
  - `tests/test_no_auto_maintenance.py`: 11 passed (7 from the first commit, 4 new: the `HOME` test twice, the empty value, the scope).
  - `tests/test_no_auto_maintenance.py`, `tests/test_clone_just_pushed.py`, `tests/test_repo_hygiene.py` and `tests/test_git_helper_identity.py` together, on 979bb39: 105 passed. With `tests/test_family_dry_run.py` added, on 8cfc8eb: 166 passed.
  - `tests/test_adopt_git_reads.py`, `tests/test_tracking_branch_ref.py`, `tests/test_adopt_late_failure.py`, `tests/test_adopt_github_exits.py` and `tests/test_family_add_excludes.py` (the tests that read or set a global config or a hooks path): 314 passed.
- On the first two commits (e2110f8 for the first four, accc7d2 for the rest; the second commit changes comment lines only). The fix round changes nothing where `HOME` is set and `GIT_CONFIG_GLOBAL` is not empty (the byte comparison above), so these were not run again:
  - Family, `tests/test_family*.py` (20 files): 823 passed, 19 skipped.
  - Adopt, `tests/test_adopt*.py` (46 files): 3463 passed, 2 skipped.
  - Doctor, `tests/test_shape_doctor.py` and `tests/test_doctor*.py` (12 files): 741 passed, 3 skipped.
  - Scaffold, `tests/test_scaffold*.py`: 634 passed.
  - Update-shape and bump-leg, `tests/test_update_shape*.py` and `tests/test_bump_leg*.py` (24 files): 857 passed, 4 skipped.
  - The rest of the files that copy a repository (`test_bootstrap_after_bump.py`, `test_non_list_project_fields.py`, `test_pin_source_env_collisions.py`, `test_pin_source_unreadable.py`, `test_shape_pin_files_not_a_list.py`, `test_validator_child_encoding.py`, `test_siblings_family_name.py`, `test_bootstrap_spelling.py`, `test_non_utf8_git_reads.py`), with `test_repo_hygiene.py`, `test_git_helper_identity.py`, `test_clone_just_pushed.py` and the new file: 924 passed.
  - On a merge of accc7d2 with main c6122e8: `test_no_auto_maintenance.py`, `test_git_helper_identity.py`, `test_repo_hygiene.py`, `test_clone_just_pushed.py` and `test_family_member_name.py`, 114 passed.

**Reproductions.**

- *The mechanism, on git 2.43.* A repository of 300 files plus two loose objects named `17…`, committed under `gc.auto=1`, then copied at once with `shutil.copytree`. This is what git 2.54 does to such a repository by default. With the base's settings, 10 of 10 copies failed, then 9 of 10 (`[Errno 2]` on a loose object directory or file). With the fix's two entries appended after it, 0 of 10 failed.
- *The emulation of the race* (rv645's recipe, run on 8cfc8eb: a global `gc.auto=1` that includes the person's `~/.gitconfig`, plus a `core.hooksPath` pre-commit hook that leaves two `17…` objects and drops `gc.log`, so git 2.43 repacks in the background after each commit; `GIT_TRACE2_EVENT` over the whole run). Base figures are rv645's, on a524542; head figures are the fix round's own, on 8cfc8eb:

  | file | git | auto-maintenance children | repack runs | result |
  |---|---|---|---|---|
  | `test_doctor_pin_restore_merges.py` | base | 78 `maintenance run --auto`, 77 `gc --auto` | 65 | 1 failed, 14 passed, 1 error |
  | | head | 4 `gc --auto` (from `receive-pack`, inert under `gc.auto=0`) | 0 | 16 passed |
  | `test_family_member_name.py` | base | 19 `maintenance run --auto`, 29 `gc --auto` | 15 | 13 passed |
  | | head | 14 `gc --auto` (inert) | 0 | 13 passed |

  On the base, rv645 reproduced the symptom itself: `shutil.Error ... moved/openRepoShape/.git/objects/pack/tmp_rev_...`, at `the_standard_moved`'s copy of `moved` (`tests/test_doctor_pin_restore_merges.py:102`). On head, 0 maintenance runs and 0 repacks in both files.
- *The first writer's runs on the issue's file* (`test_family_member_name.py`, base then head, 19 maintenance and 15 repacks against 0 and 0) agree.

**Mutation probes.** Each reverts one hunk of the fix in a throwaway clone; the new file then fails. First round (against the first seven tests):

| probe | fails |
|---|---|
| fixture sets nothing | 6 of 7 (all but the append test) |
| no command scope | `test_a_commit_a_fetch_and_a_merge_start_no_maintenance[a-tests-own-global]` |
| no global file | the receive-pack test, both person's-config cases |
| count overwritten, not appended | the append test |
| global file without the includes | both person's-config cases |
| the two keys before the includes | both person's-config cases |
| no `maintenance.auto` | 5 tests |
| no `gc.auto` | the receive-pack test, the append test |

Fix round (against the eleven tests):

| probe | result | what fails |
|---|---|---|
| S1, `no_auto_maintenance` at function scope | 1 failed, 10 passed | the scope test, alone (the first seven passed in rv645's run of the same mutant) |
| S2, not autouse | 4 failed, 7 passed | the two commit/fetch/merge cases, the tool test, the receive-pack test |
| H1, `home_is_known()` always true | 2 failed | `test_where_home_is_unset_the_file_is_one_git_can_read`, both variants |
| H2, the XDG include needs `HOME` | 1 failed | the same test, the `xdg` variant |
| E1, an empty value is read as unset (`if named:`) | 1 failed | the empty-value test |
| M5, the file includes nothing (the first round's probe, again) | 3 failed, 8 passed | both `test_a_persons_own_global_config...` cases, and the `xdg` variant of the `HOME` test |
| K1, the receive-pack test without its own `core.hooksPath`, under a global `core.hooksPath` | 1 failed, 8 passed, 2 skipped | the receive-pack test |

S2 is caught by tests that ask for the setting by itself; the scope test does not catch it, because earlier tests in the file build the fixture on request. K1 only fails where a global `core.hooksPath` is set, as NIT 1 says.

On the base itself the new file does not import (`cannot import name 'NO_AUTO_MAINTENANCE'`).

**Merge checks** (`git merge-tree --write-tree` on 979bb39): clean against origin/main 2fac979 and against lane 5's `fix/662-insteadof-helper` (4149cbb; #662 is stacked on accc7d2 and also edits `tests/conftest.py`: the fix round's hunks are in `session_global_config`, its helper `home_is_known`, and the comment above them). Earlier, on e2110f8 and accc7d2: clean against origin/main d4a7fab and c6122e8, #621, #622, #630, #631, #634, #635, #636, #637, #638 and #639, and the sibling branches for #646, #647, #648 and #649.

**What it leaves.**

- A git whose environment drops both scopes (a `GIT_CONFIG_COUNT` overwritten, not appended to, together with a test's own `GIT_CONFIG_GLOBAL`, or an environment built from nothing), and a push made under a test's own global file. No repository the suite copies is made or pushed to that way today.
- A git older than 2.32 ignores `GIT_CONFIG_GLOBAL`. Its `receive-pack` runs `gc --auto`, which does not detach below 6700 loose objects.
- `clone_just_pushed` keeps reading through git. A test with its own global file still needs it.
- `test_family_dry_run.py`'s own `NO_MAINTENANCE` is now redundant, and is left alone (NIT 3).
- Not run: a real Windows job (none runs the full suite since #597, and the `HOME` test is skipped on Windows), and a git newer than 2.43.

**Copies found** (`git grep -n 'shutil.copytree(' -- tests`, 152 calls):

- 127 copy a repository the suite built (non-bare, many holding `.git/modules`): `conftest.py` 1, `test_family.py` 18, `test_shape_doctor.py` 32, `test_doctor_legs_members_next.py` 8, the `test_update_shape*` files 30, and 38 more in 28 files. Four of them copy a subtree that holds no store: `standard / name` twice, and a member's work tree twice.
- 10 copy a bare remote, or a folder of them: `test_bump_leg_paste_lines.py:95`, `test_doctor_unfinished_merge.py:166`, `test_family_add_member_id.py:239`, `test_family_add_refusals.py:487`, `:788` and `:1983`, `test_family_dry_run.py:1446`, `test_family_tracking_branch.py:279`, `test_update_shape_landing.py:156` and `test_update_shape_next_quoting.py:116`.
- 2 copy a `.git` directory: `test_adopt_clone_template.py:190` and `test_scaffold_mount_ignored.py:381`.
- 13 hold no repository: a template, or this checkout with `.git` ignored.
- `test_adopt_plan.py:449` copies a clone named by `SHAPE_MEDXEHR_CLONE`. It is outside the suite and skipped without the variable.

No shipped script copies a repository.

**Checklist candidates** (rv645's; the coordinator places them):

- The import-time git probe at `tests/test_family_refuse_up_front.py:75-96` runs in a throwaway directory before any session setting. It is harmless today.
- Tests that overwrite `GIT_CONFIG_COUNT` (14 files) lose command scope and rely on the global file alone. That holds today, and nothing guards it.

Review: rv645, lane 5's independent review, found no MUST, two SHOULDs and five NITs on accc7d2; the fix rounds above take them, and its re-verification approved 979bb39. CI (Brett Heap's ruling 27): H0 979bb39 passed tests, tests-git-bash and SonarCloud; the head a9f36ce is a clean update-branch merge of main bf1f099, with tests, tests-git-bash and SonarCloud green; and the local gate passed on its predicted squash tree 760892f over main 2000488: every test file that #706, #703 and lane 5's six round-1 PRs change, run under #713's conftest, with the two copytree-race files and the hygiene trio, 1332 passed, 3 skipped, 1 xfailed. Landed on Brett Heap's word of 2026-10-17T10:59:39Z, relayed by lane 3 on #473: "merge lane 5's approved v1.1.0 drafts".

Closes #645

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

Labels

None yet

Projects

None yet

1 participant