diff --git a/shape-doctor.py b/shape-doctor.py index b7099555..e37ef4d2 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -3779,10 +3779,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 @@ -3837,12 +3842,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` @@ -3886,8 +3889,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 @@ -3958,24 +3962,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) @@ -4034,7 +4031,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", ( @@ -4093,18 +4090,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 @@ -4120,25 +4115,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` @@ -4472,7 +4515,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 @@ -4485,7 +4528,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 @@ -4517,6 +4564,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): @@ -4524,6 +4574,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", []) @@ -4537,7 +4589,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 @@ -4579,14 +4633,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"]