From 06eb23efbd06c6942bdf21e7d5786621143e076a Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 01:27:30 +0000 Subject: [PATCH 1/8] shape-doctor.py: a conflicted shape pin's NEXT takes the side whose pin reads, and names no line where no side's pin reads, so run as printed it no longer leaves the doctor naming the same NEXT again (#752) `pin_restore` took `--ours` wherever ours held a pin, read or not, and `--theirs` only where ours held none. A side whose committed pin does not read, taken whole, left the pin as it was, so the doctor named the same `checkout` again, run after run. - `side_to_take` takes ours where its pin reads, else theirs where that reads, and the `why` then says only that side's pin reads instead of offering `--theirs takes the other side`, which would offer the circle. - Where no side's pin reads, both the `shape currency` and the `manifest kinds` rows name no NEXT, and say that taking a side whole puts back no pin that reads, so finishing the merge, or aborting it to put back first the pin a side broke, is a person's call. `unreadable_pin_row` keeps its `update-shape.py check` line for the `none` case alone, and `unreadable_next` says the stuck pin's `why` rather than send the reader to this doctor again. - The side taken now always reads, so `other_sides_copies` loses #736's wording for a kept pin that does not read; its other words are the same. Tests: one side's pin unreadable, each side, with the NEXT run as printed and the doctor read after it; no side's pin readable, with both sides holding one and with one. Each fails on the doctor before this commit. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 159 ++++++++++++++++-------- tests/test_doctor_pin_restore_merges.py | 127 ++++++++++++++----- 2 files changed, 205 insertions(+), 81 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index d80471da..4e687dd1 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3771,10 +3771,15 @@ def pin_restore(ctx: Context, detail: dict | None = None, these that applies decides, as `detail["restore"]`: unmerged a merge left the pin conflicted. `checkout --ours`, or - `--theirs` where only that side holds a pin, of THE PIN, - which only a tool writes, so one side taken whole is one - tool's output, and of no copy but one PROVED to be tool - output on both sides (#547). + `--theirs` where only that side holds a pin that reads, + of THE PIN, which only a tool writes, so one side taken + whole is one tool's output, and of no copy but one + PROVED to be tool output on both sides (#547). A side + whose pin does not read is never taken, as taken whole + it left the pin as it was and this doctor named the same + line again; where no side's pin reads, no line is named, + `(restore, None, why)`, and the `why` says the merge is a + person's call (`side_to_take`, #752). A copy either side may have edited is never named with it (#485), though a merge of two `update-shape.py apply` runs conflicts in the two together: a copy can hold an @@ -3829,12 +3834,10 @@ def pin_restore(ctx: Context, detail: dict | None = None, `merge` row lists it unmerged all the same. So the `why` says that the other side's pin calls it a copy and the pin taken here does not, and that a person merges it by - hand, keeping the hunks git merged. Where the pin taken - here does not read, it says that instead, and nothing of - whose file the copy becomes; where one side deleted the - copy, it says a person keeps or deletes it, as there are - no hunks to keep (`other_sides_copies`, review of PR - #736). The `--accept-local` + hand, keeping the hunks git merged. Where one side + deleted the copy, it says a person keeps or deletes it, + as there are no hunks to keep (`other_sides_copies`, + review of PR #736). The `--accept-local` sentence is not for it, and is said only where a copy the kept pin has a row for is left: `apply` refuses the flag on a path the pin has no row for. In the `manifest kinds` @@ -3878,8 +3881,9 @@ def pin_restore(ctx: Context, detail: dict | None = None, Nothing git can name fixes it, and the caller says so: `(restore, None, None)`. - EVERY PIN THE COMMAND PUTS BACK IS READ FIRST, by `pin_reads_at`, except - the side of a merge, which is one tool's output either way. + EVERY PIN THE COMMAND PUTS BACK IS READ FIRST, by `pin_reads_at`, the + side of a merge included (#752): one tool's output either way, but a + commit on that side can hold a pin a slip broke. THE STATUS IS TWO FIXED-WIDTH LETTERS, READ AS BYTES: `git_out`'s text form strips its output, and the leading space of ` M` with it. And @@ -3950,24 +3954,17 @@ def pin_restore(ctx: Context, detail: dict | None = None, if path: stages.setdefault(path.decode("utf-8", "replace"), {})[ meta.rsplit(b" ", 1)[-1]] = meta.split(b" ", 1)[0] - pin_stages = stages.get(pin, {}) - side, stage = (("--ours", b"2") if b"2" in pin_stages else - ("--theirs", b"3") if b"3" in pin_stages else - (None, None)) + side, stage, note = side_to_take(ctx, pin, stages.get(pin, {}), what) + if note and not side: + return "unmerged", None, note if side: - note = (f"--theirs takes the other side. Take one side of the " - f"{what} whole" if b"3" in pin_stages and side == "--ours" - else f"only that side of the merge holds a {what}. Take " - "it whole") - lines, copies, other_side, taken = [], [], [], [] - partial, kept_reads = False, True + lines, copies, other_side, taken, partial = [], [], [], [], False if pin == SHAPE_PIN: lines = sorted(name for name in (kind.manifest, us.README) if name in stages) # Both pins say what is a copy, and the kept one what can be # taken: a copy only the other side's is never taken (#687). - held = pin_at(ctx, f":{stage.decode()}") - kept_reads, kept = held is not None, set(held or ()) + kept = set(pin_at(ctx, f":{stage.decode()}") or ()) other = set(pin_at(ctx, ":3" if stage == b"2" else ":2") or ()) copies = sorted(name for name in kept if name in stages) @@ -4026,7 +4023,7 @@ def pin_restore(ctx: Context, detail: dict | None = None, "check calls it both, as update-conflict, and " "either refusal asks for --accept-local on that " "path") - said += other_sides_copies(other_side, stages, kept_reads) + said += other_sides_copies(other_side, stages) if said: then = ". " + ". ".join(said) + ". Then finish" return "unmerged", ( @@ -4085,18 +4082,16 @@ def pin_restore(ctx: Context, detail: dict | None = None, return "none", None, None -def other_sides_copies(names: list, stages: dict, kept_reads: bool) -> list: +def other_sides_copies(names: list, stages: dict) -> list: """The sentences of a conflicted shape pin's `why` for `names`, the files conflicted beside it that only the side NOT taken has `files:` rows for (#687): never taken, and a person's to resolve. `stages` is - `{path: {stage: mode}}`, as `pin_restore` reads `ls-files -u`, and - `kept_reads` whether the pin taken here reads as one. + `{path: {stage: mode}}`, as `pin_restore` reads `ls-files -u`. - WHAT IS KNOWN, AND NOTHING ELSE (review of PR #736). Where the pin - taken here reads, it has no row for them, so once the merge is finished - each is the project's own file, and the sentence says so. Where it does - not read, nothing says whose file each becomes, so the sentence says - only that it does not read. + WHAT IS KNOWN, AND NOTHING ELSE (review of PR #736). The pin taken here + reads, since `side_to_take` takes no side whose pin does not (#752), + and it has no row for them, so once the merge is finished each is the + project's own file, and the sentence says so. ONE SENTENCE FOR THE FILES BOTH SIDES HOLD, ANOTHER FOR THOSE ONE SIDE DELETED. Both sides' stages are there for a content conflict, an @@ -4112,25 +4107,73 @@ def other_sides_copies(names: list, stages: dict, kept_reads: bool) -> list: continue many = len(group) > 1 each = "each" if many else "it" - sentence = ( + said.append( f"{' and '.join(group)} {'are' if many else 'is'} conflicted " "too, and the other side's pin calls " + ("them copies" if many else "it a copy") - + (" where the pin taken here does not" if kept_reads - else " while the pin taken here does not read as a pin") + + " where the pin taken here does not" + (f", and one side deleted {each}" if deleted else "") - + ", so neither side is taken whole: ") - if kept_reads: - sentence += (f"once the merge is finished {each} is the " - "project's own file" - + (" if kept" if deleted else "") + ", and ") - said.append(sentence + ( - f"a person keeps or deletes {each} by hand" if deleted - else f"a person merges {each} by hand, keeping the hunks git " - "merged")) + + ", so neither side is taken whole: once the merge is finished " + f"{each} is the project's own file" + + (f" if kept, and a person keeps or deletes {each} by hand" + if deleted else f", and a person merges {each} by hand, " + "keeping the hunks git merged")) return said +#: The flag of `git checkout` that takes each side's stage of a merge. +SIDE_OF_STAGE = {b"2": "--ours", b"3": "--theirs"} + + +def side_to_take(ctx: Context, pin: str, pin_stages: dict, + what: str) -> tuple: + """`(side, stage, note)` for `pin`, which a merge left conflicted with + `pin_stages` (`{stage: mode}`): the side `pin_restore` takes whole, as + a `checkout` flag and an index stage, and the start of its `why`, that + says why that side. `(None, None, why)` where no side's pin reads, the + `why` then whole, and `(None, None, None)` where neither side holds one. + + THE SIDE TAKEN IS ONE WHOSE PIN READS (#752). `--ours` was taken + wherever ours held a pin, read or not: one tool's output either way, + but a commit can hold a pin a slip broke, and taken whole it left the + pin as it was, so this doctor named the same line again. So ours where + it reads, else theirs where that reads, and the `why` says that only + that side's pin reads; offering the other side, as `--theirs takes the + other side` does where both read, would offer that circle. + + NO LINE WHERE NO SIDE'S PIN READS. Taking either side whole leaves a + pin that does not read, and a pin is never mended by hand, so no + command is named, and the `why` says that finishing the merge, or + aborting it to put back first the pin a side broke, is a person's call. + """ + held = [number for number in (b"2", b"3") if number in pin_stages] + if not held: + return None, None, None + reads = [number for number in held + if pin_reads_at(ctx, f":{number.decode()}", pin)] + if not reads: + whose = (f"neither side's {what} reads as one" if len(held) > 1 + else f"the one side of it that holds a {what} holds one " + "that does not read") + return None, None, ( + f"a merge left the {what} conflicted, and {whose}, so no side " + f"taken whole puts back a {what} that reads, and this doctor " + f"would name the same line again. A {what} is never mended by " + "hand, so no line is named here: whether to finish the merge, " + f"or to abort it and first put back the {what} a side broke, is " + "a person's call") + if len(reads) > 1: + note = (f"--theirs takes the other side. Take one side of the " + f"{what} whole") + elif len(held) > 1: + note = (f"only that side's {what} reads as one, and the other side " + f"taken whole would leave a {what} that does not read. Take " + "it whole") + else: + note = f"only that side of the merge holds a {what}. Take it whole" + return SIDE_OF_STAGE[reads[0]], reads[0], note + + def taken_with_the_pin(ctx: Context, kind, copies: list, stages: dict) -> list: """Which of `copies`, conflicted beside the shape pin, `pin_restore` @@ -4464,7 +4507,7 @@ def tool_written(ctx: Context, rel: str) -> bool: def unreadable_next(ctx: Context, unreadable: list, detail: dict | None = None - ) -> tuple[dict, str, str | None]: + ) -> tuple[dict, str | None, str | None]: """`(restore, command, why)` for the `manifest kinds` row's files that do not load (#291): each file's case, the one command the row prints, and the sentence that goes with that command -- the row's `why`, apart @@ -4477,7 +4520,11 @@ def unreadable_next(ctx: Context, unreadable: list, prints one command, and the next run names the next file. A copy or a neutral product pin, which a person may have edited, gets the read-only command `pin_restore` picks for it instead, a `git diff` - that shows what breaks it, and nothing is put back. + that shows what breaks it, and nothing is put back. A pin a merge left + conflicted where no side's pin reads gets no command (#752): where no + other file has one, the row names none either, and says `pin_restore`'s + `why`, as the `shape currency` row does, rather than send the reader to + this doctor again, which would say the same. A FILE OF THE PROJECT'S OWN IS NOT, whatever it is called, and its case is `project`, with git not asked. `tool_written` reads what the root @@ -4509,6 +4556,9 @@ def unreadable_next(ctx: Context, unreadable: list, restore: dict[str, str] = {} #: `(command, why)` for each file with a command, in the row's order. commands: dict[str, tuple[str, str | None]] = {} + #: The `why` of each file a tool wrote that git was asked about and no + #: line puts back: a merge where no side's pin reads (#752). + stuck: dict[str, str] = {} held: dict = {} for rel in unreadable: if tool_written(ctx, rel): @@ -4516,6 +4566,8 @@ def unreadable_next(ctx: Context, unreadable: list, ctx, held if rel == SHAPE_PIN else None, pin=rel) if found: commands[rel] = (found, why) + elif why: + stuck[rel] = why else: restore[rel] = "project" left = held.get("left_for_a_person", []) @@ -4529,7 +4581,9 @@ def unreadable_next(ctx: Context, unreadable: list, detail["taken_with_the_pin"] = taken command, why = (commands.get(SHAPE_PIN) or next(iter(commands.values()), (None, None))) - if command is None: + if command is None and stuck: + why = stuck.get(SHAPE_PIN) or next(iter(stuck.values())) + elif command is None: command = (f"{PYTHON} {quote_arg(ctx.shape / 'shape-doctor.py')} " f"--root {quote_arg(ctx.root)}") why = UNREADABLE_BY_HAND @@ -4571,14 +4625,17 @@ def unreadable_pin_row(ctx: Context, exc: Refusal) -> Row: same `read_pin`, refuses it the same way, and its remediation is about submodules, adoption or a hand edit, none of which fits. That command stays only where git can name nothing, and its `why` says the rest is a - person's call. Every sentence is the row's `why`, off the command's line, - so a pasted line is one command in any shell (#277). + person's call. Where a merge left the pin conflicted and no side's pin + reads, the row names no command at all, and `pin_restore`'s `why` says + the merge is a person's call (#752). Every sentence is the row's `why`, + off the command's line, so a pasted line is one command in any shell + (#277). """ detail = {"error": exc.code, "pin": SHAPE_PIN, "refusal": {"code": exc.code, "detail": exc.detail}} restore, command, why = pin_restore(ctx, detail) detail["restore"] = restore - if command is None: + if restore == "none": command = (f"{PYTHON} {quote_arg(ctx.shape / 'update-shape.py')} " f"check --root {quote_arg(ctx.root)} --upstream " f"{quote_arg(ctx.shape)}") diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index baa7e9ed..dce11c6d 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -19,9 +19,11 @@ A copy only the other side's pin has a row for -- a row the kept pin lost, or an add/add where one side's `apply --add` took a file the standard added and the other wrote its own -- is never taken either, and the `why` and -`left_for_a_person` name it for a person (#687). Beside a kept pin that -does not read, or where one side deleted it, the `why` says only what is -known: no owner, and no hunks to keep (review of PR #736). +`left_for_a_person` name it for a person (#687). Where one side deleted +it, the `why` says a person keeps or deletes it, as there are no hunks to +keep (review of PR #736). A side whose pin does not read is never taken: +the NEXT takes the side whose pin reads, and where no side's pin reads, +neither row names a NEXT (#752). L2-9. A slip amended into `bump-leg.py`'s own commit leaves the commit before it holding the pin for the OLD leg commit. Putting that pin back is @@ -785,42 +787,107 @@ def test_a_copy_only_the_other_sides_pin_has_a_row_for_is_named(both_pure): assert (root / MAKEFILE).read_bytes() == held -#: What the `why` says of the two copies, by what the kept side's pin says -#: of them: no rows for either, or nothing, as it does not read (review of -#: PR #736), where nothing says whose file each becomes. -TOGETHER = { - "rows-gone": "where the pin taken here does not, so neither side is " - "taken whole: once the merge is finished each is the " - "project's own file, and a person merges each by hand, " - "keeping the hunks git merged. Then finish", - "unreadable": "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. Then finish", -} +#: Both copies conflicted beside the pin, by the conflict each is. +BOTH_COPIES = [MAKEFILE, "project.yaml", BOOTSTRAP] -@pytest.mark.parametrize("kept", sorted(TOGETHER)) def test_copies_only_the_other_sides_pin_has_rows_for_are_named_together( - both_pure, kept): - """The mirror with both copies' rows gone from the kept side's pin, or - with that pin restaged as `files: [`, which does not read, while the - other side's reads: nothing is taken with the pin, and the `why` names - the two in one sentence, which says whose file each becomes only where - the pin taken here reads.""" + both_pure): + """The mirror with both copies' rows gone from the kept side's pin: + nothing is taken with the pin, and the `why` names the two in one + sentence.""" root = both_pure["root"] - if kept == "unreadable": - a_pin_restaged(root, 2, lambda _blob: b"files: [\n") - else: - for rel in (MAKEFILE, BOOTSTRAP): - a_pin_without_a_row(root, 2, rel) + for rel in (MAKEFILE, BOOTSTRAP): + a_pin_without_a_row(root, 2, rel) _verdict, rows = rows_from(both_pure["moved"], root) currency = assert_both_rows_name_it(rows) assert pasted(currency["next"])[3:] == [ "checkout", "--ours", "--", SHAPE_PIN], currency["next"] - assert currency["detail"]["left_for_a_person"] == [ - MAKEFILE, "project.yaml", BOOTSTRAP], currency["detail"] + assert currency["detail"]["left_for_a_person"] == BOTH_COPIES, \ + currency["detail"] assert f"{MAKEFILE} and {BOOTSTRAP} are conflicted too, and the other " \ - f"side's pin calls them copies {TOGETHER[kept]}" in currency["why"], \ + "side's pin calls them copies where the pin taken here does not, " \ + "so neither side is taken whole: once the merge is finished each " \ + "is the project's own file, and a person merges each by hand, " \ + "keeping the hunks git merged. Then finish" in currency["why"], \ + currency["why"] + + +# --- a side whose pin does not read is never taken (#752) ------------------- + +#: Bytes no pin reads as: a flow sequence nothing closes, RV-b's state. +UNREADABLE_PIN = b"files: [\n" + + +@pytest.mark.parametrize("broken, side", [ + pytest.param(2, "--theirs", id="ours-does-not-read"), + pytest.param(3, "--ours", id="theirs-does-not-read"), +]) +def test_the_side_taken_is_one_whose_pin_reads(both_pure, broken, side): + """(a)'s merge with one side's pin restaged as bytes that do not read. + `--ours` was taken wherever ours held a pin, and taken whole a pin that + does not read left the pin as it was, so the doctor named the same + line again (#752). Now the NEXT takes the side whose pin reads, and its + `why` says only that side's pin reads, never offering the other. No + copy is taken: the proof reads both sides' pins. Run as printed, the + pin in the tree reads, and neither row names it unreadable again.""" + root, moved = both_pure["root"], both_pure["moved"] + a_pin_restaged(root, broken, lambda _blob: UNREADABLE_PIN) + _verdict, rows = rows_from(moved, root) + currency = assert_both_rows_name_it(rows) + assert pasted(currency["next"])[3:] == [ + "checkout", side, "--", SHAPE_PIN], currency["next"] + assert "taken_with_the_pin" not in currency["detail"], currency + assert currency["detail"]["left_for_a_person"] == BOTH_COPIES, currency + assert currency["why"].startswith( + "only that side's pin reads as one, and the other side taken whole " + "would leave a pin that does not read. Take it whole and never a " + "hand-merged pin. "), currency["why"] + run_pasted(currency["next"]) + _verdict, after = rows_from(moved, root) + assert "error" not in after["shape-currency"]["detail"], \ + after["shape-currency"] + assert SHAPE_PIN not in after["manifest-kinds"]["detail"].get( + "restore", {}), after["manifest-kinds"] + + +#: What the `why` says where no side's pin reads, by how many sides hold a +#: pin: no line puts back one that reads (#752). +NO_SIDE_READS = { + "both": "neither side's pin reads as one", + "one": "the one side of it that holds a pin holds one that does not " + "read", +} + + +@pytest.mark.parametrize("held", sorted(NO_SIDE_READS)) +def test_no_line_is_named_where_no_sides_pin_reads(both_pure, held): + """(a)'s merge with both sides' pins restaged as bytes that do not + read, or ours removed and theirs not reading. Either side taken whole + leaves a pin that does not read, so both rows name no NEXT, and the + same `why` says the merge is a person's call (#752). The doctor said + `checkout --ours` before, again after it ran.""" + root = both_pure["root"] + a_pin_restaged(root, 3, lambda _blob: UNREADABLE_PIN) + if held == "both": + a_pin_restaged(root, 2, lambda _blob: UNREADABLE_PIN) + else: + stages = index_stages(root, SHAPE_PIN) + del stages[2] + restaged(root, SHAPE_PIN, stages) + _verdict, rows = rows_from(both_pure["moved"], root) + currency, kinds = rows["shape-currency"], rows["manifest-kinds"] + assert currency["next"] is None, currency + assert kinds["next"] is None, kinds + assert kinds["why"] == currency["why"], (kinds, currency) + assert currency["detail"]["restore"] == "unmerged", currency["detail"] + assert kinds["detail"]["restore"] == {SHAPE_PIN: "unmerged"}, kinds + assert currency["why"] == ( + f"a merge left the pin conflicted, and {NO_SIDE_READS[held]}, so no " + "side taken whole puts back a pin that reads, and this doctor would " + "name the same line again. A pin is never mended by hand, so no line " + "is named here: whether to finish the merge, or to abort it and " + "first put back the pin a side broke, is a person's call"), \ currency["why"] From 23f3879c7a23b8481c6137151d7d8f0f3f7c38c4 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 01:34:48 +0000 Subject: [PATCH 2/8] shape-doctor.py: a shape copy renamed on one side of a merge, conflicted under a name no pin has a row for, is named in a conflicted shape pin's `why` and `left_for_a_person` as renamed from that copy, for a person to settle, and never taken (#753) Where one side renamed the Makefile to build.mk and the other edited it, git carries the conflict to build.mk, which no pin's row lists, so the `why` and `left_for_a_person` said nothing of it and only the `merge` row listed the path. - `pin_restore` keeps each conflicted path's stage 1, the merge base's blob, from the `ls-files -u` it already reads. - `copies_renamed` calls a path no pin lists a rename of a copy where the merge base (`merge-base HEAD MERGE_HEAD`) held nothing at that path and its stage 1 is, object for object, the blob the base holds at that copy's path, and one copy alone matches. git gives a path the base did not hold a stage 1 only where it paired it with one the base did. A path the base held is no rename, though its bytes be a copy's, as the source's own file beside a collision copy can be (the collision-layout test in test_shape_doctor.py holds that). No similarity index is guessed at, and any other kind of conflict names nothing, as before. - `renamed_copies` gives the sentence: "build.mk, renamed from the copy Makefile, is conflicted too, and no pin has a row for it under its new name, so neither side is taken whole: a person merges it by hand, keeping the hunks git merged", or, where one side alone holds it (a rename the other side deleted), that a person keeps or deletes it. - `by_conflict` is the split `other_sides_copies` made inline, shared. Tests: the rename on the side `--ours` takes, on the other side, and on one side with the copy deleted on the other. Each fails on the doctor before this commit. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 124 +++++++++++++++++++++--- tests/test_doctor_pin_restore_merges.py | 68 ++++++++++++- 2 files changed, 176 insertions(+), 16 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 4e687dd1..50635bc2 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3844,6 +3844,14 @@ def pin_restore(ctx: Context, detail: dict | None = None, row such a copy under `contracts/` is `unmerged` while the pin is conflicted, as every file the `why` gives a person is, and `project` once the pin taken here reads. + A COPY RENAMED ON ONE SIDE (#753) conflicts under its + new name, as git carries the other side's edit, or + deletion, there, and no pin has a row for that name. Its + stage 1 is the merge base's blob of the copy, so + `copies_renamed` finds the copy by that blob, and the + `why` names the path as renamed from the copy and gives + it to a person (`renamed_copies`). It is never taken: + neither side's pin has a row to prove it by. The manifest and README.md are never named either, though `apply` writes lines of them: every other line is the project's own, and one side taken whole drops the other @@ -3854,8 +3862,10 @@ def pin_restore(ctx: Context, detail: dict | None = None, gives a person. A file of the project's own is never named: its merge is theirs. Both pins decide which file that is, not the pin taken here alone: one NEITHER - side's pin has a row for. A file only the other side's - pin calls a copy is named, as above (#687). + side's pin has a row for, under its name or under the + one the merge base held it at. A file only the other + side's pin calls a copy is named, as above (#687), and + so is a copy's rename (#753). uncommitted HEAD holds a pin that reads, and the tree or the index differs from it. `checkout HEAD`, which drops a change no tool of this standard leaves uncommitted. @@ -3946,19 +3956,25 @@ def pin_restore(ctx: Context, detail: dict | None = None, us = ctx.update_shape() kind = us.root_kind(root) # ` SP SP TAB `, one record per stage: - # `{path: {stage: mode}}`, the mode kept for `taken_with_the_pin`. + # `{path: {stage: mode}}`, the mode kept for `taken_with_the_pin`, + # and `{path: object}` of stage 1, the merge base's, which + # `copies_renamed` reads (#753). stages: dict[str, dict] = {} + bases: dict[str, bytes] = {} listing = git_answer(["ls-files", "-u", "-z"], root, binary=True) for record in (listing or b"").split(b"\0"): meta, _tab, path = record.partition(b"\t") if path: stages.setdefault(path.decode("utf-8", "replace"), {})[ meta.rsplit(b" ", 1)[-1]] = meta.split(b" ", 1)[0] + if meta.endswith(b" 1"): + bases[path.decode("utf-8", "replace")] = meta.split()[1] side, stage, note = side_to_take(ctx, pin, stages.get(pin, {}), what) if note and not side: return "unmerged", None, note if side: lines, copies, other_side, taken, partial = [], [], [], [], False + renamed: dict[str, str] = {} if pin == SHAPE_PIN: lines = sorted(name for name in (kind.manifest, us.README) if name in stages) @@ -3970,6 +3986,9 @@ def pin_restore(ctx: Context, detail: dict | None = None, copies = sorted(name for name in kept if name in stages) other_side = sorted(name for name in other - kept if name in stages) + renamed = copies_renamed(ctx, kept | other, { + name: base for name, base in bases.items() + if name not in kept | other | {pin, *lines}}) # A read of a partial clone can fetch: prove nothing there. partial = bool(copies) and partial_clone(ctx.shape) if not partial: @@ -3989,10 +4008,10 @@ def pin_restore(ctx: Context, detail: dict | None = None, + ("them" if len(taken) > 1 else "it") + " at the " "commit that side's pin names, so neither side holds an " "edit of the project") - if lines or copies or other_side: + if lines or copies or other_side or renamed: if detail is not None: - detail["left_for_a_person"] = sorted(lines + copies - + other_side) + detail["left_for_a_person"] = sorted( + lines + copies + other_side + list(renamed)) if lines: said.append( f"{' and '.join(lines)} " @@ -4024,6 +4043,7 @@ def pin_restore(ctx: Context, detail: dict | None = None, "either refusal asks for --accept-local on that " "path") said += other_sides_copies(other_side, stages) + said += renamed_copies(renamed, stages) if said: then = ". " + ". ".join(said) + ". Then finish" return "unmerged", ( @@ -4094,17 +4114,11 @@ def other_sides_copies(names: list, stages: dict) -> list: project's own file, and the sentence says so. ONE SENTENCE FOR THE FILES BOTH SIDES HOLD, ANOTHER FOR THOSE ONE SIDE - DELETED. Both sides' stages are there for a content conflict, an - add/add included, and a person merges the file by hand, keeping the - hunks git merged. A delete/modify conflict holds one of them alone, and - has no hunks to keep: a person keeps the file or deletes it. + DELETED (`by_conflict`). A person merges the first by hand, keeping + the hunks git merged, and keeps or deletes the second. """ said = [] - for deleted in (False, True): - group = [name for name in names - if deleted != ({b"2", b"3"} <= set(stages.get(name, ())))] - if not group: - continue + for group, deleted in by_conflict(names, stages): many = len(group) > 1 each = "each" if many else "it" said.append( @@ -4121,6 +4135,86 @@ def other_sides_copies(names: list, stages: dict) -> list: return said +def by_conflict(names: list, stages: dict) -> list: + """`names` as `(group, deleted)` pairs: those both sides of the merge + hold first, then those only one side holds, each pair only where its + group has a name. `stages` is `{path: {stage: mode}}`, as `pin_restore` + reads `ls-files -u`. Both sides' stages are there for a content + conflict, an add/add included, whose hunks a person keeps; one of them + alone is a delete/modify conflict, or a rename the other side did not + make, which has no hunks to keep (review of PR #736).""" + pairs = [] + for deleted in (False, True): + group = [name for name in names + if deleted != ({b"2", b"3"} <= set(stages.get(name, ())))] + if group: + pairs.append((group, deleted)) + return pairs + + +def copies_renamed(ctx: Context, copies: set, bases: dict) -> dict: + """`{path: copy}` for each path conflicted beside the shape pin whose + stages came from a rename of `copy`, a path a side's pin has a `files:` + row for (#753). `bases` is `{path: object}`, stage 1 of each conflicted + path no pin has a row for, the merge base's blob. + + THE MERGE BASE'S OWN BLOB, AT A NAME THE BASE DID NOT HOLD. A rename on + one side and an edit on the other conflict under the new name, and git + gives that path the base's bytes of the old one as its stage 1: a path + the base did not hold has a stage 1 only where git paired it with one + the base did. So a path is a copy's rename where the base holds + nothing at it and its stage 1 is, object for object, the blob the base + holds at the copy's path, and one copy alone matches. A path the base + held is no rename, though its bytes be a copy's, as the source's own + file beside a collision copy under `shape/` can be; no similarity index + is guessed at, and a blob two copies share names neither. The base is + `merge-base HEAD MERGE_HEAD`, read as git keeps it for a merge; for + any other kind of conflict, or where git cannot say, no path is called + a rename, as before. + """ + if not bases or not copies: + return {} + base = git_answer(["merge-base", "HEAD", "MERGE_HEAD"], ctx.root) + listing = base and git_answer( + ["--literal-pathspecs", "ls-tree", "-z", base, "--", + *sorted(copies | set(bases))], ctx.root, binary=True) + held: dict[bytes, list] = {} + present = set() + for record in (listing or b"").split(b"\0"): + meta, _tab, path = record.partition(b"\t") + if path and len(meta.split()) == 3: + name = path.decode("utf-8", "replace") + present.add(name) + if name in copies: + held.setdefault(meta.split()[2], []).append(name) + return {name: held[blob][0] for name, blob in sorted(bases.items()) + if name not in present and len(held.get(blob, ())) == 1} + + +def renamed_copies(renamed: dict, stages: dict) -> list: + """The sentences of a conflicted shape pin's `why` for `renamed`, + `{path: copy}` from `copies_renamed`: paths conflicted beside the pin + under a name no pin has a row for, each a rename of a copy (#753). + Never taken, as a copy only the other side's pin lists is never taken + (`other_sides_copies`), and a person's to resolve, its old name said so + the reader knows it for a copy.""" + said = [] + for group, deleted in by_conflict(sorted(renamed), stages): + many = len(group) > 1 + each = "each" if many else "it" + said.append( + " and ".join(f"{name}, renamed from the copy {renamed[name]}," + for name in group) + + f" {'are' if many else 'is'} conflicted too, and no pin has a " + f"row for {'them' if many else 'it'} under " + f"{'their new names' if many else 'its new name'}" + + (f", and one side alone holds {each}" if deleted else "") + + ", so neither side is taken whole: a person " + + (f"keeps or deletes {each} by hand" if deleted + else f"merges {each} by hand, keeping the hunks git merged")) + return said + + #: The flag of `git checkout` that takes each side's stage of a merge. SIDE_OF_STAGE = {b"2": "--ours", b"3": "--theirs"} diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index dce11c6d..4372082a 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -23,7 +23,9 @@ it, the `why` says a person keeps or deletes it, as there are no hunks to keep (review of PR #736). A side whose pin does not read is never taken: the NEXT takes the side whose pin reads, and where no side's pin reads, -neither row names a NEXT (#752). +neither row names a NEXT (#752). A copy renamed on one side conflicts +under a name no pin has a row for; the `why` names it as renamed from that +copy, by the merge base's blob, and gives it to a person (#753). L2-9. A slip amended into `bump-leg.py`'s own commit leaves the commit before it holding the pin for the OLD leg commit. Putting that pin back is @@ -1020,3 +1022,67 @@ def missing() -> set: "nothing was read from it to prove a copy unedited") \ in currency["why"], currency["why"] assert left_out <= missing(), "the doctor fetched a blob" + + +# --- a copy renamed on one side (#753) -------------------------------------- + +#: The name one side gives its Makefile, which no pin has a row for. +RENAMED = "build.mk" + +#: Each rename's merge: the side that renames the Makefile, the side that +#: deletes it (none where the other side keeps it), the stages the merge +#: leaves at the new name, and what the `why` then says of it. +RENAMES = { + "renamed-on-the-side-ours-takes": ( + "shape/b", None, [1, 2, 3], + "so neither side is taken whole: a person merges it by hand, " + "keeping the hunks git merged"), + "renamed-on-the-other-side": ( + "shape/a", None, [1, 2, 3], + "so neither side is taken whole: a person merges it by hand, " + "keeping the hunks git merged"), + "renamed-on-one-side-deleted-on-the-other": ( + "shape/b", "shape/a", [1, 2], + "and one side alone holds it, so neither side is taken whole: a " + "person keeps or deletes it by hand"), +} + + +@pytest.mark.parametrize("merge", sorted(RENAMES)) +def test_a_copy_renamed_on_one_side_is_named_for_a_person( + standard, project, tmp_path, merge): + """Both copies re-copied by `apply` on both sides, and on ONE side the + project renames its Makefile to build.mk, while the other edits it, or + deletes it. git takes the conflict to the new name, a path no pin has + a row for, so the `why` and `left_for_a_person` said nothing of it, and + only the `merge` row listed it (#753). Its stage 1 is the merge base's + own Makefile, so the `why` now names build.mk as renamed from that copy + and gives it to a person, and the NEXT never takes it; bootstrap.py is + still taken. Run, the NEXT leaves build.mk as the merge left it.""" + renamer, deleter, held, tail = RENAMES[merge] + root = under_an_awkward_name(project, tmp_path) + moved, c2, c3, _frozen = the_standard_moved( + standard, tmp_path, UPSTREAM_TARGET, BOOTSTRAP_SOURCE) + for commit, branch in ((c2, "shape/a"), (c3, "shape/b")): + apply_on(moved, root, commit, branch) + if branch == renamer: + git("mv", "--", MAKEFILE, RENAMED, cwd=root) + commit_in(root, f"the project's Makefile renamed to {RENAMED}") + if branch == deleter: + git("rm", "-q", "--", MAKEFILE, cwd=root) + commit_in(root, "the project's Makefile removed") + merge_a_into_b(root) + assert sorted(index_stages(root, RENAMED)) == held + _verdict, rows = rows_from(moved, root) + currency = assert_both_rows_name_it(rows) + assert pasted(currency["next"])[3:] == [ + "checkout", "--ours", "--", SHAPE_PIN, BOOTSTRAP], currency["next"] + assert currency["detail"]["left_for_a_person"] == [ + RENAMED, "project.yaml"], currency["detail"] + assert f"{RENAMED}, renamed from the copy {MAKEFILE}, is conflicted " \ + f"too, and no pin has a row for it under its new name, {tail}. " \ + "Then finish" in currency["why"], currency["why"] + before = (root / RENAMED).read_bytes() + run_pasted(currency["next"]) + assert sorted(index_stages(root, RENAMED)) == held + assert (root / RENAMED).read_bytes() == before From 06e1de88a5378c5466bcc9e8472852579cabbbcf Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 04:15:54 +0000 Subject: [PATCH 3/8] shape-doctor.py: a conflicted path is a copy's rename only where one path in the whole merge base holds its stage-1 blob and that path is a copy, so a file of the project's own with a copy's bytes is never named; the `why` says the copy is reported missing once the merge is finished, where the pin taken here still lists it (review of #781) RV-b's review of #781 at 23f3879c: - S1. `copies_renamed` listed only the copies' paths in the base, so a file of the project's own holding a copy's bytes (tools/bootstrap.py, byte for byte scripts/bootstrap.py), renamed on one side and edited on the other, was named "tools/boot.py, renamed from the copy scripts/bootstrap.py" and given to a person, though a file of the project's own is never named. It now reads the base's whole tree, one `ls-tree -r -z`, and calls a path a rename only where exactly one path holds its stage-1 blob and that path is a copy. Not git's `diff -M` record: a similarity guess of its own, made apart from the merge's. What that costs: a copy renamed beside a file with its exact bytes is not named, as before #753. - N1. Where the pin taken here still lists the old name, the `why` adds "The pin taken here still lists Makefile, so this doctor reports it missing once the merge is finished, unless it is renamed back". Tests: the own file renamed, and the copy renamed beside its twin, name nothing (both were named before; dropping the uniqueness guard now fails them); each rename case finishes the merge with build.mk kept and reads `DRIFTED (1 copy-missing)`. The rename setup is shared (`merged_after`). Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 70 ++++++++---- tests/test_doctor_pin_restore_merges.py | 142 ++++++++++++++++++++---- 2 files changed, 168 insertions(+), 44 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 50635bc2..d5ff4374 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3848,10 +3848,11 @@ def pin_restore(ctx: Context, detail: dict | None = None, new name, as git carries the other side's edit, or deletion, there, and no pin has a row for that name. Its stage 1 is the merge base's blob of the copy, so - `copies_renamed` finds the copy by that blob, and the - `why` names the path as renamed from the copy and gives - it to a person (`renamed_copies`). It is never taken: - neither side's pin has a row to prove it by. + `copies_renamed` finds the copy by that blob, where no + other path of the base holds it, and the `why` names the + path as renamed from the copy and gives it to a person + (`renamed_copies`). It is never taken: neither side's + pin has a row to prove it by. The manifest and README.md are never named either, though `apply` writes lines of them: every other line is the project's own, and one side taken whole drops the other @@ -3975,6 +3976,7 @@ def pin_restore(ctx: Context, detail: dict | None = None, if side: lines, copies, other_side, taken, partial = [], [], [], [], False renamed: dict[str, str] = {} + kept: set = set() if pin == SHAPE_PIN: lines = sorted(name for name in (kind.manifest, us.README) if name in stages) @@ -4043,7 +4045,7 @@ def pin_restore(ctx: Context, detail: dict | None = None, "either refusal asks for --accept-local on that " "path") said += other_sides_copies(other_side, stages) - said += renamed_copies(renamed, stages) + said += renamed_copies(renamed, stages, kept) if said: then = ". " + ". ".join(said) + ". Then finish" return "unmerged", ( @@ -4164,40 +4166,54 @@ def copies_renamed(ctx: Context, copies: set, bases: dict) -> dict: the base did not hold has a stage 1 only where git paired it with one the base did. So a path is a copy's rename where the base holds nothing at it and its stage 1 is, object for object, the blob the base - holds at the copy's path, and one copy alone matches. A path the base - held is no rename, though its bytes be a copy's, as the source's own - file beside a collision copy under `shape/` can be; no similarity index - is guessed at, and a blob two copies share names neither. The base is - `merge-base HEAD MERGE_HEAD`, read as git keeps it for a merge; for - any other kind of conflict, or where git cannot say, no path is called - a rename, as before. + holds at the copy's path. A path the base held is no rename, though its + bytes be a copy's, as the source's own file beside a collision copy + under `shape/` can be. The base is `merge-base HEAD MERGE_HEAD`, read + as git keeps it for a merge; for any other kind of conflict, or where + git cannot say, no path is called a rename, as before. + + ONE PATH IN THE WHOLE BASE TREE HOLDS THAT BLOB, AND IT IS A COPY + (review of PR #781). Reading the copies' paths alone, a file of the + project's own whose bytes are a copy's, renamed, was named a rename of + that copy, though such a file is never named. So the base's whole tree + is read, one `ls-tree -r`, and a blob two paths hold names nothing, a + copy's or not. Not git's own rename record (`diff -M` from the base to + each side): that is a similarity guess of its own, made apart from the + merge's, and nothing here guesses. What that costs is a copy renamed + whose exact bytes the base also holds at another path, which is named + no more than it was before #753; the `merge` row still lists it. """ if not bases or not copies: return {} base = git_answer(["merge-base", "HEAD", "MERGE_HEAD"], ctx.root) - listing = base and git_answer( - ["--literal-pathspecs", "ls-tree", "-z", base, "--", - *sorted(copies | set(bases))], ctx.root, binary=True) + listing = base and git_answer(["ls-tree", "-r", "-z", base], ctx.root, + binary=True) held: dict[bytes, list] = {} - present = set() for record in (listing or b"").split(b"\0"): meta, _tab, path = record.partition(b"\t") if path and len(meta.split()) == 3: - name = path.decode("utf-8", "replace") - present.add(name) - if name in copies: - held.setdefault(meta.split()[2], []).append(name) + held.setdefault(meta.split()[2], []).append( + path.decode("utf-8", "replace")) + present = {name for names in held.values() for name in names} return {name: held[blob][0] for name, blob in sorted(bases.items()) - if name not in present and len(held.get(blob, ())) == 1} + if name not in present and len(held.get(blob, ())) == 1 + and held[blob][0] in copies} -def renamed_copies(renamed: dict, stages: dict) -> list: +def renamed_copies(renamed: dict, stages: dict, kept: set) -> list: """The sentences of a conflicted shape pin's `why` for `renamed`, `{path: copy}` from `copies_renamed`: paths conflicted beside the pin under a name no pin has a row for, each a rename of a copy (#753). Never taken, as a copy only the other side's pin lists is never taken (`other_sides_copies`), and a person's to resolve, its old name said so - the reader knows it for a copy.""" + the reader knows it for a copy. + + WHAT THE DOCTOR SAYS ONCE THE MERGE IS FINISHED, where `kept`, the rows + of the pin taken here, still lists the old name (review of PR #781): + no file is there unless it is renamed back, so this doctor reports the + copy missing. Said only of such a copy: where the pin taken here has no + row for it, nothing is missing. + """ said = [] for group, deleted in by_conflict(sorted(renamed), stages): many = len(group) > 1 @@ -4212,6 +4228,14 @@ def renamed_copies(renamed: dict, stages: dict) -> list: + ", so neither side is taken whole: a person " + (f"keeps or deletes {each} by hand" if deleted else f"merges {each} by hand, keeping the hunks git merged")) + listed = sorted({copy for copy in renamed.values() if copy in kept}) + if listed: + many = len(listed) > 1 + said.append( + f"The pin taken here still lists {' and '.join(listed)}, so this " + f"doctor reports {'them' if many else 'it'} missing once the " + f"merge is finished, unless {'they are' if many else 'it is'} " + "renamed back") return said diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index 4372082a..d6024c34 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -1029,24 +1029,89 @@ def missing() -> set: #: The name one side gives its Makefile, which no pin has a row for. RENAMED = "build.mk" -#: Each rename's merge: the side that renames the Makefile, the side that -#: deletes it (none where the other side keeps it), the stages the merge -#: leaves at the new name, and what the `why` then says of it. + +def renamed(old: str, new: str, line: bytes = b""): + """An act on a branch: `old` renamed to `new`, `line` appended there, + committed.""" + def act(root: Path) -> None: + git("mv", "--", old, new, cwd=root) + if line: + (root / new).write_bytes((root / new).read_bytes() + line) + git("add", "--", new, cwd=root) + commit_in(root, f"{old} renamed to {new}") + return act + + +def deleted(old: str): + """An act on a branch: `old` removed, committed.""" + def act(root: Path) -> None: + git("rm", "-q", "--", old, cwd=root) + commit_in(root, f"{old} removed") + return act + + +def edited(path: str, line: bytes): + """An act on a branch: `line` appended to `path`, committed.""" + def act(root: Path) -> None: + (root / path).write_bytes((root / path).read_bytes() + line) + git("add", "--", path, cwd=root) + commit_in(root, f"a line in {path}") + return act + + +def merged_after(standard: Path, project: Path, tmp_path: Path, acts: dict, + twin: tuple | None = None) -> tuple: + """`(root, the standard moved on)`: (a)'s two applies, each followed + on its branch by `acts[branch]`, then shape/b merging shape/a. `twin`, + `(path, copy)`, is a file of the project's own that main gains first, + the copy's committed blob byte for byte.""" + root = under_an_awkward_name(project, tmp_path) + moved, c2, c3, _frozen = the_standard_moved( + standard, tmp_path, UPSTREAM_TARGET, BOOTSTRAP_SOURCE) + if twin: + path, copy = twin + (root / path).parent.mkdir(parents=True, exist_ok=True) + (root / path).write_bytes(blob_at(root, f"HEAD:{copy}")) + git("add", "--", path, cwd=root) + commit_in(root, f"the project's own {path}, with {copy}'s bytes") + for commit, branch in ((c2, "shape/a"), (c3, "shape/b")): + apply_on(moved, root, commit, branch) + if branch in acts: + acts[branch](root) + merge_a_into_b(root) + return root, moved + + +def blob_at(root: Path, spec: str) -> bytes: + """The blob `git cat-file` reads at `spec`, as bytes.""" + return subprocess.run(["git", "cat-file", "blob", spec], cwd=str(root), + capture_output=True, check=True).stdout + + +#: Each rename's merge: what each side does after its `apply`, the stages +#: the merge leaves at the new name, and what the `why` then says of it. RENAMES = { "renamed-on-the-side-ours-takes": ( - "shape/b", None, [1, 2, 3], + {"shape/b": renamed(MAKEFILE, RENAMED)}, [1, 2, 3], "so neither side is taken whole: a person merges it by hand, " "keeping the hunks git merged"), "renamed-on-the-other-side": ( - "shape/a", None, [1, 2, 3], + {"shape/a": renamed(MAKEFILE, RENAMED)}, [1, 2, 3], "so neither side is taken whole: a person merges it by hand, " "keeping the hunks git merged"), "renamed-on-one-side-deleted-on-the-other": ( - "shape/b", "shape/a", [1, 2], + {"shape/b": renamed(MAKEFILE, RENAMED), "shape/a": deleted(MAKEFILE)}, + [1, 2], "and one side alone holds it, so neither side is taken whole: a " "person keeps or deletes it by hand"), } +#: What the `why` adds where the pin taken here lists the old name, and +#: what the doctor says once the merge is finished (review of PR #781). +STILL_LISTED = ("The pin taken here still lists Makefile, so this doctor " + "reports it missing once the merge is finished, unless it is " + "renamed back") + @pytest.mark.parametrize("merge", sorted(RENAMES)) def test_a_copy_renamed_on_one_side_is_named_for_a_person( @@ -1058,20 +1123,13 @@ def test_a_copy_renamed_on_one_side_is_named_for_a_person( only the `merge` row listed it (#753). Its stage 1 is the merge base's own Makefile, so the `why` now names build.mk as renamed from that copy and gives it to a person, and the NEXT never takes it; bootstrap.py is - still taken. Run, the NEXT leaves build.mk as the merge left it.""" - renamer, deleter, held, tail = RENAMES[merge] - root = under_an_awkward_name(project, tmp_path) - moved, c2, c3, _frozen = the_standard_moved( - standard, tmp_path, UPSTREAM_TARGET, BOOTSTRAP_SOURCE) - for commit, branch in ((c2, "shape/a"), (c3, "shape/b")): - apply_on(moved, root, commit, branch) - if branch == renamer: - git("mv", "--", MAKEFILE, RENAMED, cwd=root) - commit_in(root, f"the project's Makefile renamed to {RENAMED}") - if branch == deleter: - git("rm", "-q", "--", MAKEFILE, cwd=root) - commit_in(root, "the project's Makefile removed") - merge_a_into_b(root) + still taken. Run, the NEXT leaves build.mk as the merge left it. + + The pin taken here still lists the Makefile, and the `why` says what + the doctor says once the merge is finished with build.mk kept: the + Makefile missing (review of PR #781).""" + acts, held, tail = RENAMES[merge] + root, moved = merged_after(standard, project, tmp_path, acts) assert sorted(index_stages(root, RENAMED)) == held _verdict, rows = rows_from(moved, root) currency = assert_both_rows_name_it(rows) @@ -1081,8 +1139,50 @@ def test_a_copy_renamed_on_one_side_is_named_for_a_person( RENAMED, "project.yaml"], currency["detail"] assert f"{RENAMED}, renamed from the copy {MAKEFILE}, is conflicted " \ f"too, and no pin has a row for it under its new name, {tail}. " \ - "Then finish" in currency["why"], currency["why"] + f"{STILL_LISTED}. Then finish" in currency["why"], currency["why"] before = (root / RENAMED).read_bytes() run_pasted(currency["next"]) assert sorted(index_stages(root, RENAMED)) == held assert (root / RENAMED).read_bytes() == before + (root / RENAMED).write_bytes(stage_of(root, 2, RENAMED)) + merge_by_ours(root, tmp_path, "project.yaml") + git("add", "--", SHAPE_PIN, BOOTSTRAP, RENAMED, "project.yaml", cwd=root) + commit_in(root, "merge shape/a, build.mk kept") + verdict, _rows = rows_from(moved, root) + assert verdict == "DRIFTED (1 copy-missing)", verdict + + +#: A file of the project's own that holds a copy's bytes in the merge base, +#: the path the merge then conflicts at, and what each side does (review +#: of PR #781): the own file renamed, or the copy renamed beside it. +TWINS = { + "the-projects-own-file-renamed": ( + ("tools/bootstrap.py", BOOTSTRAP), "tools/boot.py", + {"shape/a": edited("tools/bootstrap.py", C2_LINE), + "shape/b": renamed("tools/bootstrap.py", "tools/boot.py", + C3_LINE)}), + "the-copy-renamed-beside-its-twin": ( + ("tools/Makefile.orig", MAKEFILE), RENAMED, + {"shape/b": renamed(MAKEFILE, RENAMED)}), +} + + +@pytest.mark.parametrize("merge", sorted(TWINS)) +def test_a_blob_two_paths_of_the_base_hold_names_no_rename( + standard, project, tmp_path, merge): + """The merge base holds a file of the project's own byte for byte as a + copy. Renamed on one side and edited on the other, that file conflicts + under its new name with the copy's blob as stage 1, and #781's first + head named it a rename of the copy, though a file of the project's own + is never named. The copy renamed beside such a twin holds a blob two + paths share as well. One blob, two paths: neither is called a rename, + and the new name is not named, as before #753 (review of PR #781).""" + twin, new, acts = TWINS[merge] + root, moved = merged_after(standard, project, tmp_path, acts, twin) + assert sorted(index_stages(root, new)) == [1, 2, 3] + assert stage_of(root, 1, new) == blob_at(root, f"main:{twin[1]}") + _verdict, rows = rows_from(moved, root) + currency = assert_both_rows_name_it(rows) + assert new not in currency["detail"].get("left_for_a_person", []), \ + currency["detail"] + assert new not in currency["why"], currency["why"] From 0a9afe3a734b553bcf975063b68f02fc04955e09 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 04:31:51 +0000 Subject: [PATCH 4/8] tests/test_doctor_pin_restore_merges.py: a project's own file with bytes no copy holds, renamed, is named no copy's rename, and a rename the kept pin has no row for says nothing is missing, so both guards of #753's rename rule are held (review of #781, round 3) RV-b's re-verification of #781 at 06e1de88 found two guards of `copies_renamed` and `renamed_copies` that no test held: - "the one path holding the blob is a copy": dropped, a project's own docs/notes.md, renamed to docs/n.md on one side and edited on the other, was named "renamed from the copy docs/notes.md". Now `test_a_rename_of_no_copy_names_nothing[the-projects-own-unique-file- renamed]` holds it, beside round 2's two twin cases (the table and the test renamed to cover all three). - "the pin taken here lists the old name": dropped, a merge that ends COMPLIANT was told the Makefile would be reported missing. Now `test_a_rename_the_kept_pin_has_no_row_for_says_nothing_is_missing` restages the kept pin without the Makefile's row, finds no "still lists" sentence, and finishes the merge to a verdict that is COMPLIANT and not copy-missing. `merged_after` takes an own file's bytes as well as a copy's, and `finished_keeping` finishes a rename's merge for both tests. In shape-doctor.py, `copies_renamed`'s docstring calls "the base held nothing at the new name" belt and braces since round 2: alone it acts only on a rename onto a path the base held (rename/add). Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 7 +- tests/test_doctor_pin_restore_merges.py | 102 +++++++++++++++++------- 2 files changed, 78 insertions(+), 31 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index d5ff4374..28b49641 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -4168,7 +4168,12 @@ def copies_renamed(ctx: Context, copies: set, bases: dict) -> dict: nothing at it and its stage 1 is, object for object, the blob the base holds at the copy's path. A path the base held is no rename, though its bytes be a copy's, as the source's own file beside a collision copy - under `shape/` can be. The base is `merge-base HEAD MERGE_HEAD`, read + under `shape/` can be. Since the whole base is read (below), that check + is belt and braces: such a path's stage 1 is the blob the base holds at + that very path, which no pin lists, so the count or the copy check + below refuses it already. Alone it changes the answer only for a rename + onto a path the base held (rename/add), which stays unnamed (round 3 of + the review of PR #781). The base is `merge-base HEAD MERGE_HEAD`, read as git keeps it for a merge; for any other kind of conflict, or where git cannot say, no path is called a rename, as before. diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index d6024c34..eb4f84fd 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -1060,20 +1060,22 @@ def act(root: Path) -> None: def merged_after(standard: Path, project: Path, tmp_path: Path, acts: dict, - twin: tuple | None = None) -> tuple: + own: tuple | None = None) -> tuple: """`(root, the standard moved on)`: (a)'s two applies, each followed - on its branch by `acts[branch]`, then shape/b merging shape/a. `twin`, - `(path, copy)`, is a file of the project's own that main gains first, - the copy's committed blob byte for byte.""" + on its branch by `acts[branch]`, then shape/b merging shape/a. `own`, + `(path, content)`, is a file of the project's own that main gains + first: `content` where it is bytes, else the committed blob of the + copy it names, byte for byte.""" root = under_an_awkward_name(project, tmp_path) moved, c2, c3, _frozen = the_standard_moved( standard, tmp_path, UPSTREAM_TARGET, BOOTSTRAP_SOURCE) - if twin: - path, copy = twin + if own: + path, content = own (root / path).parent.mkdir(parents=True, exist_ok=True) - (root / path).write_bytes(blob_at(root, f"HEAD:{copy}")) + (root / path).write_bytes(content if isinstance(content, bytes) + else blob_at(root, f"HEAD:{content}")) git("add", "--", path, cwd=root) - commit_in(root, f"the project's own {path}, with {copy}'s bytes") + commit_in(root, f"the project's own {path}") for commit, branch in ((c2, "shape/a"), (c3, "shape/b")): apply_on(moved, root, commit, branch) if branch in acts: @@ -1144,18 +1146,53 @@ def test_a_copy_renamed_on_one_side_is_named_for_a_person( run_pasted(currency["next"]) assert sorted(index_stages(root, RENAMED)) == held assert (root / RENAMED).read_bytes() == before - (root / RENAMED).write_bytes(stage_of(root, 2, RENAMED)) + assert finished_keeping(root, tmp_path, moved, RENAMED) \ + == "DRIFTED (1 copy-missing)" + + +def finished_keeping(root: Path, tmp_path: Path, moved: Path, + name: str) -> str: + """The merge, its NEXT run, finished by a person who keeps `name` as + the side taken holds it, merges `project.yaml` and commits; the + doctor's verdict then, out of the standard moved on.""" + (root / name).write_bytes(stage_of(root, 2, name)) merge_by_ours(root, tmp_path, "project.yaml") - git("add", "--", SHAPE_PIN, BOOTSTRAP, RENAMED, "project.yaml", cwd=root) - commit_in(root, "merge shape/a, build.mk kept") - verdict, _rows = rows_from(moved, root) - assert verdict == "DRIFTED (1 copy-missing)", verdict + git("add", "--", SHAPE_PIN, BOOTSTRAP, name, "project.yaml", cwd=root) + commit_in(root, f"merge shape/a, {name} kept") + return json.loads(doctor(moved, root, "--json").stdout)["verdict"] + + +def test_a_rename_the_kept_pin_has_no_row_for_says_nothing_is_missing( + standard, project, tmp_path): + """The Makefile renamed on the side `--ours` takes, and that side's pin + restaged without the Makefile's row: only the other side's pin calls + it a copy. build.mk is still named as renamed from it, but the pin + taken here does not list it, so the `why` says nothing of it being + reported missing; finished with build.mk kept, the merge is not + `copy-missing` (round 3 of the review of PR #781).""" + root, moved = merged_after(standard, project, tmp_path, + {"shape/b": renamed(MAKEFILE, RENAMED)}) + a_pin_without_a_row(root, 2, MAKEFILE) + _verdict, rows = rows_from(moved, root) + currency = assert_both_rows_name_it(rows) + assert f"{RENAMED}, renamed from the copy {MAKEFILE}, is conflicted " \ + "too" in currency["why"], currency["why"] + assert "still lists" not in currency["why"], currency["why"] + run_pasted(currency["next"]) + verdict = finished_keeping(root, tmp_path, moved, RENAMED) + assert "copy-missing" not in verdict, verdict + assert verdict.startswith("COMPLIANT"), verdict + +#: Bytes of a file of the project's own that no other path holds. +NOTES = b"# Notes this project keeps, in no copy of the standard.\n" -#: A file of the project's own that holds a copy's bytes in the merge base, -#: the path the merge then conflicts at, and what each side does (review -#: of PR #781): the own file renamed, or the copy renamed beside it. -TWINS = { +#: Renames the rule never calls a copy's (review of PR #781): a file of +#: the project's own, `(path, content)` as `merged_after` takes it, in +#: the merge base; the path the merge then conflicts at; and what each +#: side does. The own file renamed, with a copy's bytes or bytes of its +#: own, or the copy renamed beside an own file holding its bytes. +NOT_A_COPYS_RENAME = { "the-projects-own-file-renamed": ( ("tools/bootstrap.py", BOOTSTRAP), "tools/boot.py", {"shape/a": edited("tools/bootstrap.py", C2_LINE), @@ -1164,23 +1201,28 @@ def test_a_copy_renamed_on_one_side_is_named_for_a_person( "the-copy-renamed-beside-its-twin": ( ("tools/Makefile.orig", MAKEFILE), RENAMED, {"shape/b": renamed(MAKEFILE, RENAMED)}), + "the-projects-own-unique-file-renamed": ( + ("docs/notes.md", NOTES), "docs/n.md", + {"shape/a": edited("docs/notes.md", C2_LINE), + "shape/b": renamed("docs/notes.md", "docs/n.md", C3_LINE)}), } -@pytest.mark.parametrize("merge", sorted(TWINS)) -def test_a_blob_two_paths_of_the_base_hold_names_no_rename( - standard, project, tmp_path, merge): - """The merge base holds a file of the project's own byte for byte as a - copy. Renamed on one side and edited on the other, that file conflicts - under its new name with the copy's blob as stage 1, and #781's first - head named it a rename of the copy, though a file of the project's own - is never named. The copy renamed beside such a twin holds a blob two - paths share as well. One blob, two paths: neither is called a rename, - and the new name is not named, as before #753 (review of PR #781).""" - twin, new, acts = TWINS[merge] - root, moved = merged_after(standard, project, tmp_path, acts, twin) +@pytest.mark.parametrize("merge", sorted(NOT_A_COPYS_RENAME)) +def test_a_rename_of_no_copy_names_nothing(standard, project, tmp_path, + merge): + """A file of the project's own, renamed on one side and edited on the + other, conflicts under its new name with the base's blob of it as + stage 1. Where that blob is a copy's too, #781's first head named the + file a rename of the copy, though a file of the project's own is never + named; the copy renamed beside such a twin holds a blob two paths share + as well. One blob, two paths: neither is called a rename. And where no + copy holds the blob, the one path holding it is no copy: nothing is + called a rename either (review of PR #781, rounds 1 and 3).""" + own, new, acts = NOT_A_COPYS_RENAME[merge] + root, moved = merged_after(standard, project, tmp_path, acts, own) assert sorted(index_stages(root, new)) == [1, 2, 3] - assert stage_of(root, 1, new) == blob_at(root, f"main:{twin[1]}") + assert stage_of(root, 1, new) == blob_at(root, f"main:{own[0]}") _verdict, rows = rows_from(moved, root) currency = assert_both_rows_name_it(rows) assert new not in currency["detail"].get("left_for_a_person", []), \ From 3dd7ffb8397861201966ad85af1dd6b41d203362 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 09:03:51 +0000 Subject: [PATCH 5/8] shape-doctor.py: a shape copy renamed on both sides of a merge, `DD` at its old name, is named gone on both sides with each new name a side's own commit holds the base's blob of it at, never as a merge of hunks git did not merge, and the pin taken here's row for it is said to go missing (#783) Ours renames the copy Makefile to a.mk and theirs to b.mk. git leaves `DD Makefile` (stage 1 alone), `AU a.mk` and `UA b.mk`, neither with a stage 1, so #781's rename rule saw neither new name, and the Makefile, a conflicted copy, got the copies' sentence: "a person merges it by hand, keeping the hunks git merged", where git merged none. - `pin_restore` takes a copy at stage 1 alone out of `copies` and `other_side`: never taken, and never the hunks sentence. - `copies_renamed_twice` names a path one side alone added as that side's new name for the copy where the commit that side records (`HEAD` for stage 2, `MERGE_HEAD` for stage 3) holds the merge base's blob of the copy there, and no other path of the base holds that blob. git writes its merge of the copy's three versions to both new names, so where a side edited the copy no index stage holds the base's blob; that side's own commit does where it renamed without an edit. A side whose commit holds an edit is not proved: no similarity index, and git's merged content, the same at both names, is not taken for a pairing. - `twice_renamed_copies` says: "Makefile is gone on both sides of the merge, renamed to a.mk on the side taken here and to b.mk on the other side, and no pin lists the new names, so nothing is taken whole: a person keeps one name, or both, by hand"; where one side's name is not proved, that it is not; where neither is, that a person settles where it goes. - `still_listed` is #781's "The pin taken here still lists ..." sentence, moved out of `renamed_copies`, said of a copy renamed on one side or both that the pin taken here still lists. - `base_holders` is the base's whole tree by blob, read once for both rename rules; `copies_renamed` takes it. Tests: a rename on both sides; with an edit on ours; beside an own file holding the Makefile's bytes. Each finishes the merge keeping a.mk and reads `DRIFTED (1 copy-missing)`. Each fails on #781's doctor; four mutants (the `DD` test, the blob test, the one-path test, the stage's commit) each fail at least one. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 191 +++++++++++++++++++----- tests/test_doctor_pin_restore_merges.py | 102 +++++++++++-- 2 files changed, 248 insertions(+), 45 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 28b49641..7ae09449 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3853,6 +3853,17 @@ def pin_restore(ctx: Context, detail: dict | None = None, path as renamed from the copy and gives it to a person (`renamed_copies`). It is never taken: neither side's pin has a row to prove it by. + A COPY RENAMED ON BOTH SIDES (#783) is `DD` at its old + name, stage 1 alone, and each new name is added by one + side alone, with no stage 1. It is no copy to merge by + hunks, as git merged none there, so it leaves `copies`, + and the `why` says it is gone on both sides and names + each new name a side's own commit holds the base's blob + of it at, saying where a side's is not proved + (`copies_renamed_twice`, `twice_renamed_copies`). Where + the pin taken here still lists a copy renamed on one side + or both, the `why` says this doctor reports it missing + once the merge is finished (`still_listed`). The manifest and README.md are never named either, though `apply` writes lines of them: every other line is the project's own, and one side taken whole drops the other @@ -3866,7 +3877,7 @@ def pin_restore(ctx: Context, detail: dict | None = None, side's pin has a row for, under its name or under the one the merge base held it at. A file only the other side's pin calls a copy is named, as above (#687), and - so is a copy's rename (#753). + so is a copy's rename, on one side or both (#753, #783). uncommitted HEAD holds a pin that reads, and the tree or the index differs from it. `checkout HEAD`, which drops a change no tool of this standard leaves uncommitted. @@ -3976,6 +3987,7 @@ def pin_restore(ctx: Context, detail: dict | None = None, if side: lines, copies, other_side, taken, partial = [], [], [], [], False renamed: dict[str, str] = {} + twice: dict[str, dict] = {} kept: set = set() if pin == SHAPE_PIN: lines = sorted(name for name in (kind.manifest, us.README) @@ -3988,9 +4000,19 @@ def pin_restore(ctx: Context, detail: dict | None = None, copies = sorted(name for name in kept if name in stages) other_side = sorted(name for name in other - kept if name in stages) - renamed = copies_renamed(ctx, kept | other, { - name: base for name, base in bases.items() - if name not in kept | other | {pin, *lines}}) + named = kept | other | {pin, *lines} + # A copy both sides renamed is `DD`, stage 1 alone (#783): + # never taken, and never a merge of hunks there are not. + gone = {name: bases[name] for name in copies + other_side + if set(stages[name]) == {b"1"}} + copies = [name for name in copies if name not in gone] + other_side = [name for name in other_side + if name not in gone] + unnamed = {name: base for name, base in bases.items() + if name not in named} + held = base_holders(ctx) if gone or unnamed else {} + renamed = copies_renamed(kept | other, unnamed, held) + twice = copies_renamed_twice(ctx, gone, stages, held) # A read of a partial clone can fetch: prove nothing there. partial = bool(copies) and partial_clone(ctx.shape) if not partial: @@ -4010,10 +4032,13 @@ def pin_restore(ctx: Context, detail: dict | None = None, + ("them" if len(taken) > 1 else "it") + " at the " "commit that side's pin names, so neither side holds an " "edit of the project") - if lines or copies or other_side or renamed: + if lines or copies or other_side or renamed or twice: if detail is not None: detail["left_for_a_person"] = sorted( - lines + copies + other_side + list(renamed)) + lines + copies + other_side + list(renamed) + + list(twice) + [path for went in twice.values() + for paths in went.values() + for path in paths]) if lines: said.append( f"{' and '.join(lines)} " @@ -4045,7 +4070,9 @@ def pin_restore(ctx: Context, detail: dict | None = None, "either refusal asks for --accept-local on that " "path") said += other_sides_copies(other_side, stages) - said += renamed_copies(renamed, stages, kept) + said += renamed_copies(renamed, stages) + said += twice_renamed_copies(twice, stage) + said += still_listed(set(renamed.values()) | set(twice), kept) if said: then = ". " + ". ".join(said) + ". Then finish" return "unmerged", ( @@ -4154,11 +4181,30 @@ def by_conflict(names: list, stages: dict) -> list: return pairs -def copies_renamed(ctx: Context, copies: set, bases: dict) -> dict: +def base_holders(ctx: Context) -> dict: + """`{blob: [paths]}`, every path of the merge base's whole tree under + the blob it holds, the base being `merge-base HEAD MERGE_HEAD`, as + `copies_renamed` (#753) and `copies_renamed_twice` (#783) weigh a blob; + `{}` where git cannot name the base, as for any conflict but a merge's. + """ + base = git_answer(["merge-base", "HEAD", "MERGE_HEAD"], ctx.root) + listing = base and git_answer(["ls-tree", "-r", "-z", base], ctx.root, + binary=True) + held: dict[bytes, list] = {} + for record in (listing or b"").split(b"\0"): + meta, _tab, path = record.partition(b"\t") + if path and len(meta.split()) == 3: + held.setdefault(meta.split()[2], []).append( + path.decode("utf-8", "replace")) + return held + + +def copies_renamed(copies: set, bases: dict, held: dict) -> dict: """`{path: copy}` for each path conflicted beside the shape pin whose stages came from a rename of `copy`, a path a side's pin has a `files:` row for (#753). `bases` is `{path: object}`, stage 1 of each conflicted - path no pin has a row for, the merge base's blob. + path no pin has a row for, the merge base's blob, and `held` the base's + whole tree by blob (`base_holders`). THE MERGE BASE'S OWN BLOB, AT A NAME THE BASE DID NOT HOLD. A rename on one side and an edit on the other conflict under the new name, and git @@ -4188,36 +4234,117 @@ def copies_renamed(ctx: Context, copies: set, bases: dict) -> dict: whose exact bytes the base also holds at another path, which is named no more than it was before #753; the `merge` row still lists it. """ - if not bases or not copies: - return {} - base = git_answer(["merge-base", "HEAD", "MERGE_HEAD"], ctx.root) - listing = base and git_answer(["ls-tree", "-r", "-z", base], ctx.root, - binary=True) - held: dict[bytes, list] = {} - for record in (listing or b"").split(b"\0"): - meta, _tab, path = record.partition(b"\t") - if path and len(meta.split()) == 3: - held.setdefault(meta.split()[2], []).append( - path.decode("utf-8", "replace")) present = {name for names in held.values() for name in names} return {name: held[blob][0] for name, blob in sorted(bases.items()) if name not in present and len(held.get(blob, ())) == 1 and held[blob][0] in copies} -def renamed_copies(renamed: dict, stages: dict, kept: set) -> list: +#: The commit each side of a merge records its own tree in, by the index +#: stage git gives that side's version of a path. +COMMIT_OF_STAGE = {b"2": "HEAD", b"3": "MERGE_HEAD"} + + +def copies_renamed_twice(ctx: Context, gone: dict, stages: dict, + held: dict) -> dict: + """`{copy: {stage: [paths]}}` for each copy both sides of the merge + renamed (#783): `gone` is `{copy: blob}`, the copies conflicted beside + the shape pin at stage 1 alone (`DD`) with the merge base's blob of + each, and each path is a new name of the copy on the side git gives + `stage`, b"2" for the side checked out, b"3" for the side merged in. + + BOTH SIDES RENAMED IT. git leaves the old name `DD` and each new one + added by one side alone (`AU`, `UA`), with no stage 1, so #781's rule, + which keys on a path's stage 1, sees neither. And it writes its merge + of the copy's three versions to both new names, so where a side edited + the copy no stage holds the base's blob. + + THE BLOB THE SIDE ITSELF COMMITTED IS THE EVIDENCE. A path added by + one side alone is that side's new name for the copy where the commit + that side records (`HEAD`, `MERGE_HEAD`) holds at that path the merge + base's blob of the copy, object for object, and no other path of the + base holds that blob (`base_holders`, as #781 weighs one). A side whose + commit holds an edit there is not proved: no similarity index is + guessed at, and git's merged content, the same at both names, is not + taken for a pairing. Where git cannot name the base, nothing is proved. + """ + added = sorted(name for name, held_at in stages.items() + if set(held_at) in ({b"2"}, {b"3"})) + twice: dict[str, dict] = {} + for copy, blob in sorted(gone.items()): + found: dict[bytes, list] = {} + if held.get(blob) == [copy]: + for path in added: + number = next(iter(stages[path])) + committed = git_answer( + ["rev-parse", "-q", "--verify", + f"{COMMIT_OF_STAGE[number]}:./{path}"], ctx.root) + if committed == blob.decode("ascii"): + found.setdefault(number, []).append(path) + twice[copy] = found + return twice + + +def twice_renamed_copies(twice: dict, stage: bytes) -> list: + """The sentences of a conflicted shape pin's `why` for `twice`, from + `copies_renamed_twice`, `stage` being the side taken here: one for each + copy both sides renamed (#783), naming the new names proved on each + side, and saying where a side's is not proved. Never the hunks git + merged, as git merged none at the old name, and never taken: a person + keeps one name, or both, or settles where the copy goes.""" + sides = {number: ("the side taken here" if number == stage + else "the other side") for number in (b"2", b"3")} + said = [] + for copy, found in sorted(twice.items()): + start = f"{copy} is gone on both sides of the merge" + if not found: + said.append(f"{start}, and where either side put it is not " + "proved here, so nothing is taken whole: a person " + "settles where it goes, by hand") + continue + renames = " and ".join( + f"to {' and '.join(found[number])} on {sides[number]}" + for number in sorted(found, key=lambda number: number != stage)) + new = ("name" if sum(len(paths) for paths in found.values()) == 1 + else "names") + if len(found) > 1: + said.append(f"{start}, renamed {renames}, and no pin lists the " + f"new {new}, so nothing is taken whole: a person " + "keeps one name, or both, by hand") + continue + unproved = next(number for number in sides if number not in found) + said.append(f"{start}, renamed {renames}, and no pin lists the new " + f"{new}; where {sides[unproved]} put it is not proved " + "here, so nothing is taken whole: a person settles where " + "it goes, by hand") + return said + + +def still_listed(gone: set, kept: set) -> list: + """The sentence a conflicted shape pin's `why` adds for the copies in + `gone`, renamed on one side or both (#753, #783), that `kept`, the rows + of the pin taken here, still lists (review of PR #781): no file is at + that name once the merge is finished unless it is renamed back, so + this doctor reports the copy missing. Said only of such a copy: where + the pin taken here has no row for it, nothing is missing.""" + listed = sorted(copy for copy in gone if copy in kept) + if not listed: + return [] + many = len(listed) > 1 + return [f"The pin taken here still lists {' and '.join(listed)}, so this " + f"doctor reports {'them' if many else 'it'} missing once the " + f"merge is finished, unless {'they are' if many else 'it is'} " + "renamed back"] + + +def renamed_copies(renamed: dict, stages: dict) -> list: """The sentences of a conflicted shape pin's `why` for `renamed`, `{path: copy}` from `copies_renamed`: paths conflicted beside the pin under a name no pin has a row for, each a rename of a copy (#753). Never taken, as a copy only the other side's pin lists is never taken (`other_sides_copies`), and a person's to resolve, its old name said so - the reader knows it for a copy. - - WHAT THE DOCTOR SAYS ONCE THE MERGE IS FINISHED, where `kept`, the rows - of the pin taken here, still lists the old name (review of PR #781): - no file is there unless it is renamed back, so this doctor reports the - copy missing. Said only of such a copy: where the pin taken here has no - row for it, nothing is missing. + the reader knows it for a copy; `still_listed` says what the pin taken + here still lists. """ said = [] for group, deleted in by_conflict(sorted(renamed), stages): @@ -4233,14 +4360,6 @@ def renamed_copies(renamed: dict, stages: dict, kept: set) -> list: + ", so neither side is taken whole: a person " + (f"keeps or deletes {each} by hand" if deleted else f"merges {each} by hand, keeping the hunks git merged")) - listed = sorted({copy for copy in renamed.values() if copy in kept}) - if listed: - many = len(listed) > 1 - said.append( - f"The pin taken here still lists {' and '.join(listed)}, so this " - f"doctor reports {'them' if many else 'it'} missing once the " - f"merge is finished, unless {'they are' if many else 'it is'} " - "renamed back") return said diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index eb4f84fd..9f6dcfb3 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -25,7 +25,10 @@ the NEXT takes the side whose pin reads, and where no side's pin reads, neither row names a NEXT (#752). A copy renamed on one side conflicts under a name no pin has a row for; the `why` names it as renamed from that -copy, by the merge base's blob, and gives it to a person (#753). +copy, by the merge base's blob, and gives it to a person (#753). A copy +renamed on both sides is `DD` at its old name, never a merge of hunks: +the `why` names each new name a side's own commit holds the base's blob +at, and says where a side's is not proved (#783). L2-9. A slip amended into `bump-leg.py`'s own commit leaves the commit before it holding the pin for the OLD leg commit. Putting that pin back is @@ -1060,15 +1063,18 @@ def act(root: Path) -> None: def merged_after(standard: Path, project: Path, tmp_path: Path, acts: dict, - own: tuple | None = None) -> tuple: + own: tuple | None = None, + sources: tuple = (UPSTREAM_TARGET, BOOTSTRAP_SOURCE) + ) -> tuple: """`(root, the standard moved on)`: (a)'s two applies, each followed on its branch by `acts[branch]`, then shape/b merging shape/a. `own`, `(path, content)`, is a file of the project's own that main gains first: `content` where it is bytes, else the committed blob of the - copy it names, byte for byte.""" + copy it names, byte for byte. `sources` are the standard's files C2 + and C3 move on.""" root = under_an_awkward_name(project, tmp_path) - moved, c2, c3, _frozen = the_standard_moved( - standard, tmp_path, UPSTREAM_TARGET, BOOTSTRAP_SOURCE) + moved, c2, c3, _frozen = the_standard_moved(standard, tmp_path, + *sources) if own: path, content = own (root / path).parent.mkdir(parents=True, exist_ok=True) @@ -1150,12 +1156,15 @@ def test_a_copy_renamed_on_one_side_is_named_for_a_person( == "DRIFTED (1 copy-missing)" -def finished_keeping(root: Path, tmp_path: Path, moved: Path, - name: str) -> str: +def finished_keeping(root: Path, tmp_path: Path, moved: Path, name: str, + dropped: tuple = ()) -> str: """The merge, its NEXT run, finished by a person who keeps `name` as - the side taken holds it, merges `project.yaml` and commits; the - doctor's verdict then, out of the standard moved on.""" + the side taken holds it, removes each of `dropped`, merges + `project.yaml` and commits; the doctor's verdict then, out of the + standard moved on.""" (root / name).write_bytes(stage_of(root, 2, name)) + if dropped: + git("rm", "-q", "--", *dropped, cwd=root) merge_by_ours(root, tmp_path, "project.yaml") git("add", "--", SHAPE_PIN, BOOTSTRAP, name, "project.yaml", cwd=root) commit_in(root, f"merge shape/a, {name} kept") @@ -1228,3 +1237,78 @@ def test_a_rename_of_no_copy_names_nothing(standard, project, tmp_path, assert new not in currency["detail"].get("left_for_a_person", []), \ currency["detail"] assert new not in currency["why"], currency["why"] + + +# --- a copy renamed on both sides (#783) ------------------------------------- + +#: The names each side gives the Makefile: the side `--ours` takes, and the +#: other side. +OURS_NAME, THEIRS_NAME = "a.mk", "b.mk" + +#: Both sides rename the Makefile, after `apply` runs that leave it as the +#: merge base holds it. +BOTH_RENAME = {"shape/a": renamed(MAKEFILE, THEIRS_NAME), + "shape/b": renamed(MAKEFILE, OURS_NAME)} + +#: Each merge in which both sides rename the Makefile: what each side does, +#: the file of the project's own main gains first (none, or the Makefile's +#: twin), what the `why` says of the Makefile, and what is left to a person. +TWICE = { + "renamed-on-both-sides": ( + BOTH_RENAME, None, + f"renamed to {OURS_NAME} on the side taken here and to " + f"{THEIRS_NAME} on the other side, and no pin lists the new names, " + "so nothing is taken whole: a person keeps one name, or both, by " + "hand", + [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"]), + "renamed-on-both-sides-edited-on-ours": ( + {**BOTH_RENAME, "shape/b": renamed(MAKEFILE, OURS_NAME, OWN_LINE)}, + None, + f"renamed to {THEIRS_NAME} on the other side, and no pin lists the " + "new name; where the side taken here put it is not proved here, so " + "nothing is taken whole: a person settles where it goes, by hand", + [MAKEFILE, THEIRS_NAME, "project.yaml"]), + "renamed-on-both-sides-beside-a-twin": ( + BOTH_RENAME, ("tools/Makefile.orig", MAKEFILE), + "and where either side put it is not proved here, so nothing is " + "taken whole: a person settles where it goes, by hand", + [MAKEFILE, "project.yaml"]), +} + + +@pytest.mark.parametrize("merge", sorted(TWICE)) +def test_a_copy_renamed_on_both_sides_is_named_for_a_person( + standard, project, tmp_path, merge): + """Both sides rename the Makefile, to a.mk on the side `--ours` takes + and to b.mk on the other. git leaves the Makefile `DD`, stage 1 alone, + and each new name added by one side alone, with no stage 1, so #781's + rule saw neither, and the Makefile got the copies' sentence, keeping + "the hunks git merged" where git merged none (#783). + + Now the Makefile is gone on both sides, and each new name is named + where the commit of the side that made it holds the merge base's blob + of the Makefile there, and no other path of the base holds that blob. + Edited on ours, git writes its merge to both names and ours' commit + holds the edit, so only b.mk is proved, and the `why` says ours' is + not; beside a twin of the Makefile neither is. The pin taken here + still lists the Makefile, and finished keeping a.mk, the merge is + `copy-missing`, as the `why` says.""" + acts, own, said, left = TWICE[merge] + root, moved = merged_after(standard, project, tmp_path, acts, own, + sources=(BOOTSTRAP_SOURCE,)) + assert [sorted(index_stages(root, name)) + for name in (MAKEFILE, OURS_NAME, THEIRS_NAME)] \ + == [[1], [2], [3]] + _verdict, rows = rows_from(moved, root) + currency = assert_both_rows_name_it(rows) + assert pasted(currency["next"])[3:] == [ + "checkout", "--ours", "--", SHAPE_PIN, BOOTSTRAP], currency["next"] + assert currency["detail"]["left_for_a_person"] == left, \ + currency["detail"] + assert f"{MAKEFILE} is gone on both sides of the merge, {said}. " \ + f"{STILL_LISTED}. Then finish" in currency["why"], currency["why"] + assert "hunks" not in currency["why"], currency["why"] + run_pasted(currency["next"]) + assert finished_keeping(root, tmp_path, moved, OURS_NAME, + (MAKEFILE, THEIRS_NAME)) \ + == "DRIFTED (1 copy-missing)" From 37da4e52eb167fb2460dd3249a0f6e6130ce1075 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 09:50:59 +0000 Subject: [PATCH 6/8] shape-doctor.py: a copy renamed on both sides is proved at a side's new name by the bytes that side's own pin row records too, so a move of the standard no longer hides both names; a path a pin has a row for is never also named a new name, and the sentences say "neither name is taken" (review of #789) - S1: `pin_restore` passes `copies_renamed_twice` only the conflicted paths no pin has a row for. Theirs renaming the Makefile to b.mk and moving its row to `- path: b.mk` in the same commit named b.mk twice, as the other side's copy and as a new name "no pin lists", and `left_for_a_person` held it twice. `left_for_a_person` is built from a set. - S3: `side_moved_it` also proves a new name where the side's own commit (`HEAD`, `MERGE_HEAD`) holds there the bytes whose sha256 that commit's own shape pin records in its row for the copy (`pinned_sha256`). Where the standard moved the copy between the two pins, neither commit holds the merge base's blob, and neither name was proved. Exact, no similarity. Those bytes must be held at no path of the merge base, as the base's blob must be held at the copy alone, and with no merge base nothing is proved. - NIT: "so nothing is taken whole" is "so neither name is taken" in the three sentences. Tests: `TWICE` is a table of `Twice` cases, eight new: theirs moving its row (S1); `--theirs` taken, plain and edited on ours, where ours' pin does not read; the kept pin without the Makefile's row, finished COMPLIANT (S2); the standard moving the Makefile, beside a twin of its old bytes, with the base holding ours' new bytes, and with no MERGE_HEAD (S3). Each case also checks every other name is said once, and "hunks" only for a copy left to a person. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 103 ++++++++--- tests/test_doctor_pin_restore_merges.py | 223 +++++++++++++++++++----- 2 files changed, 254 insertions(+), 72 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index 7ae09449..e2eb4065 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3859,7 +3859,8 @@ def pin_restore(ctx: Context, detail: dict | None = None, hunks, as git merged none there, so it leaves `copies`, and the `why` says it is gone on both sides and names each new name a side's own commit holds the base's blob - of it at, saying where a side's is not proved + of it at, or the bytes that commit's own pin row for it + records, saying where a side's is not proved (`copies_renamed_twice`, `twice_renamed_copies`). Where the pin taken here still lists a copy renamed on one side or both, the `why` says this doctor reports it missing @@ -4012,7 +4013,12 @@ def pin_restore(ctx: Context, detail: dict | None = None, if name not in named} held = base_holders(ctx) if gone or unnamed else {} renamed = copies_renamed(kept | other, unnamed, held) - twice = copies_renamed_twice(ctx, gone, stages, held) + # A path a pin has a row for is named as that, never as a + # copy's new name too (review of PR #789). + twice = copies_renamed_twice( + ctx, gone, {name: held_at for name, held_at + in stages.items() if name not in named}, + held) # A read of a partial clone can fetch: prove nothing there. partial = bool(copies) and partial_clone(ctx.shape) if not partial: @@ -4035,10 +4041,9 @@ def pin_restore(ctx: Context, detail: dict | None = None, if lines or copies or other_side or renamed or twice: if detail is not None: detail["left_for_a_person"] = sorted( - lines + copies + other_side + list(renamed) - + list(twice) + [path for went in twice.values() - for paths in went.values() - for path in paths]) + {*lines, *copies, *other_side, *renamed, *twice, + *(path for went in twice.values() + for paths in went.values() for path in paths)}) if lines: said.append( f"{' and '.join(lines)} " @@ -4259,32 +4264,78 @@ def copies_renamed_twice(ctx: Context, gone: dict, stages: dict, of the copy's three versions to both new names, so where a side edited the copy no stage holds the base's blob. - THE BLOB THE SIDE ITSELF COMMITTED IS THE EVIDENCE. A path added by - one side alone is that side's new name for the copy where the commit - that side records (`HEAD`, `MERGE_HEAD`) holds at that path the merge - base's blob of the copy, object for object, and no other path of the - base holds that blob (`base_holders`, as #781 weighs one). A side whose - commit holds an edit there is not proved: no similarity index is - guessed at, and git's merged content, the same at both names, is not - taken for a pairing. Where git cannot name the base, nothing is proved. - """ + THE BYTES THE SIDE ITSELF COMMITTED ARE THE EVIDENCE (`side_moved_it`). + A path added by one side alone is that side's new name for the copy + where the commit that side records (`HEAD`, `MERGE_HEAD`) holds at that + path the merge base's blob of the copy, object for object, or the bytes + whose sha256 that same commit's own shape pin records in its row for + the copy (review of PR #789): where the standard moved the copy between + the two pins, each side's `apply` rewrote it, and neither commit holds + the base's blob. A side whose commit holds an edit there is not proved: + no similarity index is guessed at, and git's merged content, the same + at both names, is not taken for a pairing. + + NO OTHER PATH OF THE BASE HOLDS THOSE BYTES, as #781 weighs a blob + (`base_holders`): the base's blob of the copy is held there by the + copy alone, and bytes the base did not hold at the copy are held there + by no path at all. So a path of the base holding the very bytes a side + holds at its new name proves that name nothing, while a twin of the + copy's old bytes does not stand in the way of bytes the standard moved + it to. Where git cannot name the base, nothing is proved. + + Only a path no pin has a row for is a candidate: `pin_restore` names + the others as what their rows say, never as a new name too (review of + PR #789). Several proved paths on one side were not reached in that + review: git pairs one new name per side with the rename, and another + path a side copies the copy to is added cleanly, never conflicted. The + `[paths]` keeps them all, should git ever give more. + """ + if not gone: + return {} added = sorted(name for name, held_at in stages.items() if set(held_at) in ({b"2"}, {b"3"})) + pinned = {number: pinned_sha256(ctx, commit) + for number, commit in COMMIT_OF_STAGE.items()} twice: dict[str, dict] = {} for copy, blob in sorted(gone.items()): found: dict[bytes, list] = {} - if held.get(blob) == [copy]: - for path in added: - number = next(iter(stages[path])) - committed = git_answer( - ["rev-parse", "-q", "--verify", - f"{COMMIT_OF_STAGE[number]}:./{path}"], ctx.root) - if committed == blob.decode("ascii"): - found.setdefault(number, []).append(path) + for path in added: + number = next(iter(stages[path])) + if held and side_moved_it( + ctx, f"{COMMIT_OF_STAGE[number]}:./{path}", copy, blob, + pinned[number].get(copy), held): + found.setdefault(number, []).append(path) twice[copy] = found return twice +def pinned_sha256(ctx: Context, rev: str) -> dict: + """`{path: sha256}` of the `files:` rows of the shape pin `rev` holds, + read by `pin_at` through `read_pin`'s checks, or `{}` where it does not + read; a row with no `sha256:` maps to ''.""" + read = pin_at(ctx, rev, whole=True) + return {row.path: row.recorded for row in read[1]} if read else {} + + +def side_moved_it(ctx: Context, spec: str, copy: str, blob: bytes, + recorded: str | None, held: dict) -> bool: + """Whether `spec`, `:./` of one side of the merge, holds + the copy `copy` moved there, by `copies_renamed_twice`'s evidence: the + merge base's blob of the copy, `blob`, which the base `held` holds at + the copy alone, or bytes the base holds at no path whose sha256 is + `recorded`, that side's pin row for the copy.""" + committed = git_answer(["rev-parse", "-q", "--verify", spec], ctx.root) + if committed is None: + return False + if committed.encode("ascii") == blob: + return held.get(blob) == [copy] + if not recorded or committed.encode("ascii") in held: + return False + data = git_answer(["cat-file", "blob", spec], ctx.root, binary=True) + return (data is not None + and ctx.update_shape().file_bytes_sha256(data) == recorded) + + def twice_renamed_copies(twice: dict, stage: bytes) -> list: """The sentences of a conflicted shape pin's `why` for `twice`, from `copies_renamed_twice`, `stage` being the side taken here: one for each @@ -4299,7 +4350,7 @@ def twice_renamed_copies(twice: dict, stage: bytes) -> list: start = f"{copy} is gone on both sides of the merge" if not found: said.append(f"{start}, and where either side put it is not " - "proved here, so nothing is taken whole: a person " + "proved here, so neither name is taken: a person " "settles where it goes, by hand") continue renames = " and ".join( @@ -4309,13 +4360,13 @@ def twice_renamed_copies(twice: dict, stage: bytes) -> list: else "names") if len(found) > 1: said.append(f"{start}, renamed {renames}, and no pin lists the " - f"new {new}, so nothing is taken whole: a person " + f"new {new}, so neither name is taken: a person " "keeps one name, or both, by hand") continue unproved = next(number for number in sides if number not in found) said.append(f"{start}, renamed {renames}, and no pin lists the new " f"{new}; where {sides[unproved]} put it is not proved " - "here, so nothing is taken whole: a person settles where " + "here, so neither name is taken: a person settles where " "it goes, by hand") return said diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index 9f6dcfb3..b4097cdd 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -28,7 +28,8 @@ copy, by the merge base's blob, and gives it to a person (#753). A copy renamed on both sides is `DD` at its old name, never a merge of hunks: the `why` names each new name a side's own commit holds the base's blob -at, and says where a side's is not proved (#783). +at, or the bytes that commit's own pin row for the copy records, and says +where a side's is not proved (#783, review of PR #789). L2-9. A slip amended into `bump-leg.py`'s own commit leaves the commit before it holding the pin for the OLD leg commit. Putting that pin back is @@ -50,7 +51,9 @@ import re import shutil import subprocess +from collections.abc import Callable from pathlib import Path +from typing import NamedTuple import pytest @@ -1033,14 +1036,22 @@ def missing() -> set: RENAMED = "build.mk" -def renamed(old: str, new: str, line: bytes = b""): +def renamed(old: str, new: str, line: bytes = b"", row: bool = False): """An act on a branch: `old` renamed to `new`, `line` appended there, - committed.""" + and with `row` the shape pin's `files:` row for `old` moved to `new`, + its sha256 kept, all in one commit.""" def act(root: Path) -> None: git("mv", "--", old, new, cwd=root) if line: (root / new).write_bytes((root / new).read_bytes() + line) git("add", "--", new, cwd=root) + if row: + held = (root / SHAPE_PIN).read_bytes() + at = f" - path: {old}\n".encode() + assert held.count(at) == 1, held + (root / SHAPE_PIN).write_bytes( + held.replace(at, f" - path: {new}\n".encode())) + git("add", "--", SHAPE_PIN, cwd=root) commit_in(root, f"{old} renamed to {new}") return act @@ -1068,18 +1079,19 @@ def merged_after(standard: Path, project: Path, tmp_path: Path, acts: dict, ) -> tuple: """`(root, the standard moved on)`: (a)'s two applies, each followed on its branch by `acts[branch]`, then shape/b merging shape/a. `own`, - `(path, content)`, is a file of the project's own that main gains - first: `content` where it is bytes, else the committed blob of the - copy it names, byte for byte. `sources` are the standard's files C2 - and C3 move on.""" + `(path, content, *lines)`, is a file of the project's own that main + gains first: `content` where it is bytes, else the committed blob of + the copy it names, byte for byte, then `lines`. `sources` are the + standard's files C2 and C3 move on.""" root = under_an_awkward_name(project, tmp_path) moved, c2, c3, _frozen = the_standard_moved(standard, tmp_path, *sources) if own: - path, content = own + path, content, *lines = own (root / path).parent.mkdir(parents=True, exist_ok=True) - (root / path).write_bytes(content if isinstance(content, bytes) - else blob_at(root, f"HEAD:{content}")) + (root / path).write_bytes( + (content if isinstance(content, bytes) + else blob_at(root, f"HEAD:{content}")) + b"".join(lines)) git("add", "--", path, cwd=root) commit_in(root, f"the project's own {path}") for commit, branch in ((c2, "shape/a"), (c3, "shape/b")): @@ -1246,33 +1258,130 @@ def test_a_rename_of_no_copy_names_nothing(standard, project, tmp_path, OURS_NAME, THEIRS_NAME = "a.mk", "b.mk" #: Both sides rename the Makefile, after `apply` runs that leave it as the -#: merge base holds it. +#: merge base holds it, unless the standard moves it too. BOTH_RENAME = {"shape/a": renamed(MAKEFILE, THEIRS_NAME), "shape/b": renamed(MAKEFILE, OURS_NAME)} -#: Each merge in which both sides rename the Makefile: what each side does, -#: the file of the project's own main gains first (none, or the Makefile's -#: twin), what the `why` says of the Makefile, and what is left to a person. +#: Both, the Makefile edited on ours. +EDITED_ON_OURS = {**BOTH_RENAME, + "shape/b": renamed(MAKEFILE, OURS_NAME, OWN_LINE)} + +#: The standard's files C2 and C3 move on where the Makefile moves too, so +#: neither side's commit holds the merge base's blob of it. +THE_MAKEFILE_MOVES = (UPSTREAM_TARGET, BOOTSTRAP_SOURCE) + + +def both_proved(taken_here: str, other: str) -> str: + """What the `why` says of the Makefile where each side's new name is + proved, the side taken here's first.""" + return (f"renamed to {taken_here} on the side taken here and to {other} " + "on the other side, and no pin lists the new names, so neither " + "name is taken: a person keeps one name, or both, by hand") + + +def one_proved(name: str, on: str, unproved: str) -> str: + """What the `why` says of the Makefile where one side's new name, + `name`, is proved, on the side `on`, and `unproved`'s is not.""" + return (f"renamed to {name} on {on}, and no pin lists the new name; " + f"where {unproved} put it is not proved here, so neither name is " + "taken: a person settles where it goes, by hand") + + +#: What the `why` says of the Makefile where neither side's name is proved. +NONE_PROVED = ("and where either side put it is not proved here, so neither " + "name is taken: a person settles where it goes, by hand") +TAKEN_HERE, OTHER_SIDE = "the side taken here", "the other side" + + +def ours_does_not_read(root: Path) -> None: + """Ours' stage of the shape pin as bytes that do not read, so the NEXT + takes `--theirs` (#752).""" + a_pin_restaged(root, 2, lambda _blob: UNREADABLE_PIN) + + +def the_kept_pin_lacks_its_row(root: Path) -> None: + """Ours' stage of the shape pin without the Makefile's row: only the + other side's pin calls it a copy (#687).""" + a_pin_without_a_row(root, 2, MAKEFILE) + + +def no_merge_head(root: Path) -> None: + """MERGE_HEAD gone, as a cherry-pick or a rebase leaves its conflicts: + git names no merge base.""" + git("update-ref", "-d", "MERGE_HEAD", cwd=root) + + +class Twice(NamedTuple): + """A merge in which both sides rename the Makefile.""" + #: What each side does after its `apply`. + acts: dict + #: What the `why` says of the Makefile after "gone on both sides of the + #: merge, ". + said: str + #: What is left to a person. + left: list + #: A file of the project's own main gains first, as `merged_after` + #: takes it. + own: tuple | None = None + #: The standard's files C2 and C3 move on. + sources: tuple = (BOOTSTRAP_SOURCE,) + #: What is done to the merge before the doctor reads it. + then: Callable | None = None + #: The side the NEXT takes. + side: str = "--ours" + #: Whether the `why` says the pin taken here still lists the Makefile. + listed: bool = True + #: The verdict once the merge is finished keeping a.mk, where asked. + finished: str | None = "DRIFTED (1 copy-missing)" + + +#: Each merge in which both sides rename the Makefile, by what proves each +#: new name: the merge base's blob of it, or the bytes the side's own pin +#: row records (review of PR #789), at the side's own commit. TWICE = { - "renamed-on-both-sides": ( - BOTH_RENAME, None, - f"renamed to {OURS_NAME} on the side taken here and to " - f"{THEIRS_NAME} on the other side, and no pin lists the new names, " - "so nothing is taken whole: a person keeps one name, or both, by " - "hand", + "renamed-on-both-sides": Twice( + BOTH_RENAME, both_proved(OURS_NAME, THEIRS_NAME), [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"]), - "renamed-on-both-sides-edited-on-ours": ( - {**BOTH_RENAME, "shape/b": renamed(MAKEFILE, OURS_NAME, OWN_LINE)}, - None, - f"renamed to {THEIRS_NAME} on the other side, and no pin lists the " - "new name; where the side taken here put it is not proved here, so " - "nothing is taken whole: a person settles where it goes, by hand", + "renamed-on-both-sides-edited-on-ours": Twice( + EDITED_ON_OURS, one_proved(THEIRS_NAME, OTHER_SIDE, TAKEN_HERE), [MAKEFILE, THEIRS_NAME, "project.yaml"]), - "renamed-on-both-sides-beside-a-twin": ( - BOTH_RENAME, ("tools/Makefile.orig", MAKEFILE), - "and where either side put it is not proved here, so nothing is " - "taken whole: a person settles where it goes, by hand", - [MAKEFILE, "project.yaml"]), + "renamed-on-both-sides-beside-a-twin": Twice( + BOTH_RENAME, NONE_PROVED, [MAKEFILE, "project.yaml"], + own=("tools/Makefile.orig", MAKEFILE)), + "renamed-on-both-sides-theirs-moving-its-row": Twice( + {**BOTH_RENAME, "shape/a": renamed(MAKEFILE, THEIRS_NAME, row=True)}, + one_proved(OURS_NAME, TAKEN_HERE, OTHER_SIDE), + [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"]), + "renamed-on-both-sides-theirs-taken": Twice( + BOTH_RENAME, both_proved(THEIRS_NAME, OURS_NAME), + [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml", BOOTSTRAP], + then=ours_does_not_read, side="--theirs", finished=None), + "renamed-on-both-sides-theirs-taken-edited-on-ours": Twice( + EDITED_ON_OURS, one_proved(THEIRS_NAME, TAKEN_HERE, OTHER_SIDE), + [MAKEFILE, THEIRS_NAME, "project.yaml", BOOTSTRAP], + then=ours_does_not_read, side="--theirs", finished=None), + "renamed-on-both-sides-the-kept-pin-without-its-row": Twice( + BOTH_RENAME, both_proved(OURS_NAME, THEIRS_NAME), + [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"], + then=the_kept_pin_lacks_its_row, listed=False, + finished="COMPLIANT, SHAPE BEHIND (0 upstream-changed, " + "1 upstream-added)"), + "renamed-on-both-sides-the-standard-moved": Twice( + BOTH_RENAME, both_proved(OURS_NAME, THEIRS_NAME), + [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"], + sources=THE_MAKEFILE_MOVES), + "renamed-on-both-sides-the-standard-moved-beside-a-twin": Twice( + BOTH_RENAME, both_proved(OURS_NAME, THEIRS_NAME), + [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"], + own=("tools/Makefile.orig", MAKEFILE), sources=THE_MAKEFILE_MOVES), + "renamed-on-both-sides-the-standard-moved-the-base-holding-ours": Twice( + BOTH_RENAME, one_proved(THEIRS_NAME, OTHER_SIDE, TAKEN_HERE), + [MAKEFILE, THEIRS_NAME, "project.yaml"], + own=("tools/Makefile.next", MAKEFILE, C2_LINE, C3_LINE), + sources=THE_MAKEFILE_MOVES), + "renamed-on-both-sides-the-standard-moved-no-merge-head": Twice( + BOTH_RENAME, NONE_PROVED, [MAKEFILE, "project.yaml"], + sources=THE_MAKEFILE_MOVES, then=no_merge_head, finished=None), } @@ -1286,29 +1395,51 @@ def test_a_copy_renamed_on_both_sides_is_named_for_a_person( "the hunks git merged" where git merged none (#783). Now the Makefile is gone on both sides, and each new name is named - where the commit of the side that made it holds the merge base's blob - of the Makefile there, and no other path of the base holds that blob. + where the commit of the side that made it holds there the merge base's + blob of the Makefile, or the bytes that commit's own pin row for the + Makefile records, and the base holds those bytes at no other path. Edited on ours, git writes its merge to both names and ours' commit holds the edit, so only b.mk is proved, and the `why` says ours' is - not; beside a twin of the Makefile neither is. The pin taken here - still lists the Makefile, and finished keeping a.mk, the merge is - `copy-missing`, as the `why` says.""" - acts, own, said, left = TWICE[merge] - root, moved = merged_after(standard, project, tmp_path, acts, own, - sources=(BOOTSTRAP_SOURCE,)) + not; beside a twin of the Makefile neither is. Where the standard + moved the Makefile, neither commit holds the base's blob, and each + side's pin row proves its name, a twin of the base's bytes beside it + or not; a base holding ours' bytes at another path proves ours' name + nothing, and with no MERGE_HEAD no base is named, so nothing is proved + (review of PR #789). + + A name a pin has a row for is named by that row alone: theirs, moving + its row to b.mk, gets b.mk named once, as the other side's copy, and + ours' a.mk proved. Taken `--theirs`, b.mk is the side taken here's. + Where the kept pin has no row for the Makefile, both names are still + named, nothing is said to go missing, and the merge finished keeping + a.mk is no `copy-missing`; where it still lists the Makefile, the + `why` says so, and the finished merge is `copy-missing`. Every name + is said once, and only a copy left to a person is a merge of hunks.""" + case = TWICE[merge] + root, moved = merged_after(standard, project, tmp_path, case.acts, + case.own, case.sources) + if case.then: + case.then(root) assert [sorted(index_stages(root, name)) for name in (MAKEFILE, OURS_NAME, THEIRS_NAME)] \ == [[1], [2], [3]] _verdict, rows = rows_from(moved, root) currency = assert_both_rows_name_it(rows) + why, left = currency["why"], case.left + left_whole = BOOTSTRAP in left assert pasted(currency["next"])[3:] == [ - "checkout", "--ours", "--", SHAPE_PIN, BOOTSTRAP], currency["next"] + "checkout", case.side, "--", SHAPE_PIN, + *([] if left_whole else [BOOTSTRAP])], currency["next"] assert currency["detail"]["left_for_a_person"] == left, \ currency["detail"] - assert f"{MAKEFILE} is gone on both sides of the merge, {said}. " \ - f"{STILL_LISTED}. Then finish" in currency["why"], currency["why"] - assert "hunks" not in currency["why"], currency["why"] + listed = f". {STILL_LISTED}" if case.listed else "" + assert f"{MAKEFILE} is gone on both sides of the merge, {case.said}" \ + f"{listed}. Then finish" in why, why + assert ("still lists" in why) == case.listed, why + assert [why.count(name) for name in left if name != MAKEFILE] \ + == [1] * (len(left) - 1), why + assert why.count("hunks") == int(left_whole), why run_pasted(currency["next"]) - assert finished_keeping(root, tmp_path, moved, OURS_NAME, - (MAKEFILE, THEIRS_NAME)) \ - == "DRIFTED (1 copy-missing)" + if case.finished: + assert finished_keeping(root, tmp_path, moved, OURS_NAME, + (MAKEFILE, THEIRS_NAME)) == case.finished From abcad3997cfb82c297f5a62ddba6244106b214b9 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 10:12:11 +0000 Subject: [PATCH 7/8] shape-doctor.py: a copy renamed on both sides is proved at a side's new name only where that side's own commit holds the proving bytes at that one path, so a project's own twin kept in step with the copy and renamed too is never named the copy's new name (review of #789, round 3) RV-b's S4: the base holds the Makefile and the project's own twin of it, `tools/Makefile.orig`. Ours keeps the twin in step with a.mk, and each side renames the twin too. Ours' commit then holds the bytes its pin row records for the Makefile at a.mk and at tools/x.mk, which git paired with the twin, and the `why` said "renamed to a.mk and tools/x.mk on the side taken here": a person following it could delete their own file. - `side_moved_it` proves a name only where the side's own commit holds its bytes at that path alone (`tree_holders`, `ls-tree -r -z` of HEAD or MERGE_HEAD): #781's one-path rule, applied to the side's tree. It closes the same gap for the base's blob written at two paths. - `base_holders` reads the base through `tree_holders`. Tests: two `TWICE` cases, the twin kept in step with the standard moving (pin-row proof) and without (base-blob proof). Each names only b.mk and leaves `[Makefile, b.mk, project.yaml]`; both fail without the one-path check. Co-Authored-By: Claude Opus 5.5 --- shape-doctor.py | 76 ++++++++++++++++--------- tests/test_doctor_pin_restore_merges.py | 54 ++++++++++++++++-- 2 files changed, 100 insertions(+), 30 deletions(-) diff --git a/shape-doctor.py b/shape-doctor.py index e2eb4065..9e2193a3 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3860,11 +3860,12 @@ def pin_restore(ctx: Context, detail: dict | None = None, and the `why` says it is gone on both sides and names each new name a side's own commit holds the base's blob of it at, or the bytes that commit's own pin row for it - records, saying where a side's is not proved - (`copies_renamed_twice`, `twice_renamed_copies`). Where - the pin taken here still lists a copy renamed on one side - or both, the `why` says this doctor reports it missing - once the merge is finished (`still_listed`). + records, at that one path of the commit, saying where a + side's is not proved (`copies_renamed_twice`, + `twice_renamed_copies`). Where the pin taken here still + lists a copy renamed on one side or both, the `why` says + this doctor reports it missing once the merge is + finished (`still_listed`). The manifest and README.md are never named either, though `apply` writes lines of them: every other line is the project's own, and one side taken whole drops the other @@ -4188,13 +4189,22 @@ def by_conflict(names: list, stages: dict) -> list: def base_holders(ctx: Context) -> dict: """`{blob: [paths]}`, every path of the merge base's whole tree under - the blob it holds, the base being `merge-base HEAD MERGE_HEAD`, as - `copies_renamed` (#753) and `copies_renamed_twice` (#783) weigh a blob; - `{}` where git cannot name the base, as for any conflict but a merge's. + the blob it holds (`tree_holders`), the base being `merge-base HEAD + MERGE_HEAD`, as `copies_renamed` (#753) and `copies_renamed_twice` + (#783) weigh a blob; `{}` where git cannot name the base, as for any + conflict but a merge's. """ base = git_answer(["merge-base", "HEAD", "MERGE_HEAD"], ctx.root) - listing = base and git_answer(["ls-tree", "-r", "-z", base], ctx.root, - binary=True) + return tree_holders(ctx, base) if base else {} + + +def tree_holders(ctx: Context, rev: str) -> dict: + """`{blob: [paths]}`, every path of the whole tree `rev` names under + the blob it holds, read from the root as `ls-tree -r` lists it, so each + path is named as `ls-files` names it there; `{}` where git cannot read + it. One read of the merge base (`base_holders`), and of each side's own + commit, for `copies_renamed_twice` (review of PR #789, round 3).""" + listing = git_answer(["ls-tree", "-r", "-z", rev], ctx.root, binary=True) held: dict[bytes, list] = {} for record in (listing or b"").split(b"\0"): meta, _tab, path = record.partition(b"\t") @@ -4283,27 +4293,38 @@ def copies_renamed_twice(ctx: Context, gone: dict, stages: dict, copy's old bytes does not stand in the way of bytes the standard moved it to. Where git cannot name the base, nothing is proved. + AND NO OTHER PATH OF THE SIDE'S OWN COMMIT HOLDS THEM (`tree_holders`, + review of PR #789, round 3): #781's one-path rule, applied to the + side's tree. A project that keeps its own twin of the copy in step + with it, and renames the twin too, holds the copy's bytes at two new + names, and git pairs one of them with the twin: neither is proved, as + either may be the project's own file. The same holds for the base's + blob written at two paths. + Only a path no pin has a row for is a candidate: `pin_restore` names the others as what their rows say, never as a new name too (review of PR #789). Several proved paths on one side were not reached in that - review: git pairs one new name per side with the rename, and another - path a side copies the copy to is added cleanly, never conflicted. The - `[paths]` keeps them all, should git ever give more. + review: git pairs one new name per side with the rename, another path + a side copies the copy to is added cleanly, never conflicted, and two + proved paths would have to hold different bytes, the base's blob at + one and the bytes of the side's pin row at the other. The `[paths]` + keeps them all, should git ever give more. """ if not gone: return {} added = sorted(name for name, held_at in stages.items() if set(held_at) in ({b"2"}, {b"3"})) - pinned = {number: pinned_sha256(ctx, commit) - for number, commit in COMMIT_OF_STAGE.items()} + sides = {number: (commit, pinned_sha256(ctx, commit), + tree_holders(ctx, commit)) + for number, commit in COMMIT_OF_STAGE.items()} twice: dict[str, dict] = {} for copy, blob in sorted(gone.items()): found: dict[bytes, list] = {} for path in added: number = next(iter(stages[path])) - if held and side_moved_it( - ctx, f"{COMMIT_OF_STAGE[number]}:./{path}", copy, blob, - pinned[number].get(copy), held): + commit, pinned, tree = sides[number] + if held and side_moved_it(ctx, commit, path, copy, blob, + pinned.get(copy), tree, held): found.setdefault(number, []).append(path) twice[copy] = found return twice @@ -4317,15 +4338,18 @@ def pinned_sha256(ctx: Context, rev: str) -> dict: return {row.path: row.recorded for row in read[1]} if read else {} -def side_moved_it(ctx: Context, spec: str, copy: str, blob: bytes, - recorded: str | None, held: dict) -> bool: - """Whether `spec`, `:./` of one side of the merge, holds - the copy `copy` moved there, by `copies_renamed_twice`'s evidence: the - merge base's blob of the copy, `blob`, which the base `held` holds at - the copy alone, or bytes the base holds at no path whose sha256 is - `recorded`, that side's pin row for the copy.""" +def side_moved_it(ctx: Context, commit: str, path: str, copy: str, + blob: bytes, recorded: str | None, tree: dict, + held: dict) -> bool: + """Whether `path` of `commit`, one side of the merge, holds the copy + `copy` moved there, by `copies_renamed_twice`'s evidence: bytes that + `tree`, that commit's whole tree by blob, holds at `path` alone, and + that are either the merge base's blob of the copy, `blob`, which the + base `held` holds at the copy alone, or bytes the base holds at no + path whose sha256 is `recorded`, that side's pin row for the copy.""" + spec = f"{commit}:./{path}" committed = git_answer(["rev-parse", "-q", "--verify", spec], ctx.root) - if committed is None: + if committed is None or tree.get(committed.encode("ascii")) != [path]: return False if committed.encode("ascii") == blob: return held.get(blob) == [copy] diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index b4097cdd..66068212 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -1270,6 +1270,40 @@ def test_a_rename_of_no_copy_names_nothing(standard, project, tmp_path, #: neither side's commit holds the merge base's blob of it. THE_MAKEFILE_MOVES = (UPSTREAM_TARGET, BOOTSTRAP_SOURCE) +#: A file of the project's own beside the Makefile: its twin, where main +#: gains it with the Makefile's bytes. +TWIN = "tools/Makefile.orig" + + +def in_step(path: str, like: str): + """An act on a branch: `path` given `like`'s bytes, committed, as a + project keeps its own twin of a copy in step with the copy.""" + def act(root: Path) -> None: + (root / path).write_bytes((root / like).read_bytes()) + git("add", "--", path, cwd=root) + commit_in(root, f"{path} in step with {like}") + return act + + +def in_turn(*acts): + """One act on a branch: each of `acts` in turn.""" + def act(root: Path) -> None: + for each in acts: + each(root) + return act + + +#: Both sides rename the Makefile and the project's own twin of it, ours +#: keeping the twin in step with its a.mk first, so ours' commit holds +#: a.mk's bytes at two new names (review of PR #789, round 3). +TWIN_KEPT_IN_STEP = { + "shape/a": in_turn(renamed(MAKEFILE, THEIRS_NAME), + renamed(TWIN, "tools/y.mk")), + "shape/b": in_turn(renamed(MAKEFILE, OURS_NAME), + in_step(TWIN, OURS_NAME), + renamed(TWIN, "tools/x.mk")), +} + def both_proved(taken_here: str, other: str) -> str: """What the `why` says of the Makefile where each side's new name is @@ -1337,7 +1371,8 @@ class Twice(NamedTuple): #: Each merge in which both sides rename the Makefile, by what proves each #: new name: the merge base's blob of it, or the bytes the side's own pin -#: row records (review of PR #789), at the side's own commit. +#: row records (review of PR #789), at the side's own commit, and at one +#: path of it (round 3). TWICE = { "renamed-on-both-sides": Twice( BOTH_RENAME, both_proved(OURS_NAME, THEIRS_NAME), @@ -1347,7 +1382,11 @@ class Twice(NamedTuple): [MAKEFILE, THEIRS_NAME, "project.yaml"]), "renamed-on-both-sides-beside-a-twin": Twice( BOTH_RENAME, NONE_PROVED, [MAKEFILE, "project.yaml"], - own=("tools/Makefile.orig", MAKEFILE)), + own=(TWIN, MAKEFILE)), + "renamed-on-both-sides-a-twin-kept-in-step": Twice( + TWIN_KEPT_IN_STEP, one_proved(THEIRS_NAME, OTHER_SIDE, TAKEN_HERE), + [MAKEFILE, THEIRS_NAME, "project.yaml"], + own=(TWIN, MAKEFILE, OWN_LINE), finished=None), "renamed-on-both-sides-theirs-moving-its-row": Twice( {**BOTH_RENAME, "shape/a": renamed(MAKEFILE, THEIRS_NAME, row=True)}, one_proved(OURS_NAME, TAKEN_HERE, OTHER_SIDE), @@ -1373,7 +1412,11 @@ class Twice(NamedTuple): "renamed-on-both-sides-the-standard-moved-beside-a-twin": Twice( BOTH_RENAME, both_proved(OURS_NAME, THEIRS_NAME), [MAKEFILE, OURS_NAME, THEIRS_NAME, "project.yaml"], - own=("tools/Makefile.orig", MAKEFILE), sources=THE_MAKEFILE_MOVES), + own=(TWIN, MAKEFILE), sources=THE_MAKEFILE_MOVES), + "renamed-on-both-sides-the-standard-moved-a-twin-kept-in-step": Twice( + TWIN_KEPT_IN_STEP, one_proved(THEIRS_NAME, OTHER_SIDE, TAKEN_HERE), + [MAKEFILE, THEIRS_NAME, "project.yaml"], + own=(TWIN, MAKEFILE), sources=THE_MAKEFILE_MOVES, finished=None), "renamed-on-both-sides-the-standard-moved-the-base-holding-ours": Twice( BOTH_RENAME, one_proved(THEIRS_NAME, OTHER_SIDE, TAKEN_HERE), [MAKEFILE, THEIRS_NAME, "project.yaml"], @@ -1405,7 +1448,10 @@ def test_a_copy_renamed_on_both_sides_is_named_for_a_person( side's pin row proves its name, a twin of the base's bytes beside it or not; a base holding ours' bytes at another path proves ours' name nothing, and with no MERGE_HEAD no base is named, so nothing is proved - (review of PR #789). + (review of PR #789). Where ours keeps its own twin of the Makefile in + step with a.mk and both sides rename the twin too, ours' commit holds + a.mk's bytes at two new names, and git pairs one with the twin: ours' + name is not proved, the standard moved or not (round 3). A name a pin has a row for is named by that row alone: theirs, moving its row to b.mk, gets b.mk named once, as the other side's copy, and From 3cfd34bb521ad5887223223250c27a99cb68d1ef Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 20:15:18 +0000 Subject: [PATCH 8/8] tests: the no-merge-head helper deletes MERGE_HEAD's file, because git 2.46 and later refuse `git update-ref -d` on a pseudoref (#783) CI's git 2.55.0 answered `refusing to update pseudoref 'MERGE_HEAD'` to the helper's `update-ref -d MERGE_HEAD`, which the container's git 2.43.0 allows. The helper now unlinks the file at the path `git rev-parse --git-path MERGE_HEAD` gives, a linked worktree's own included, and asserts it is gone; MERGE_HEAD stays a file under both ref backends, so it holds on every git. A missing file is an error: the helper follows a real merge. Co-Authored-By: Claude Opus 5.5 --- tests/test_doctor_pin_restore_merges.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/test_doctor_pin_restore_merges.py b/tests/test_doctor_pin_restore_merges.py index 66068212..9a122986 100644 --- a/tests/test_doctor_pin_restore_merges.py +++ b/tests/test_doctor_pin_restore_merges.py @@ -1341,8 +1341,20 @@ def the_kept_pin_lacks_its_row(root: Path) -> None: def no_merge_head(root: Path) -> None: """MERGE_HEAD gone, as a cherry-pick or a rebase leaves its conflicts: - git names no merge base.""" - git("update-ref", "-d", "MERGE_HEAD", cwd=root) + git names no merge base. + + The FILE is deleted, at the path git itself gives, which is also a + linked worktree's own. `git update-ref -d MERGE_HEAD` is refused from + git 2.46 on ("refusing to update pseudoref"), and MERGE_HEAD stays a + file under both ref backends (#783, review of PR #789). It must be + there to go: this follows a real merge, so a missing file is a fixture + that stopped being one, never a tolerated state.""" + merge_head = Path(git("rev-parse", "--git-path", "MERGE_HEAD", + cwd=root).stdout.strip()) + if not merge_head.is_absolute(): + merge_head = root / merge_head + merge_head.unlink() + assert not merge_head.exists(), merge_head class Twice(NamedTuple):