From 4ba4e78317b4078a1513f8cb2b01825e79ced75a Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 13:57:08 +0000 Subject: [PATCH 1/4] adopt-project.py: `check` prints the rename-first remedy for a plan in the source's own refused name, as `execute` does, and a plan's recorded `source: repository:` holds its names where the live source names none, so a plan edited to `Atlsa` with `origin` removed is `plan-not-the-source` in `check` and `execute` alike (#716) L4-38. `cmd_check` asked `_check_names` about the plan's names without the source, so a plan in the source's own name that the naming policy refuses (`billing-service`, `Tools-Install`) was `naming-unclassified` or `naming-role-mismatch` with no remedy, where `execute`, which hands the source over, says the source cannot be adopted in place under that name and that renaming it on GitHub comes first. `check` hands the source over as `execute` does, and prints that one remedy once after its findings. Only that remedy: `_check_names` gives a leg's name the `--project takes ...` text, which `check` never printed, and a leg's name is the plan's own to edit. The source is handed over only where the plan does not name another repository: `execute` stops at `plan-not-the-source` and never asks the names then, and a refusal of a name that is not the source's must not say the source cannot be adopted under it (`_own_name_remediation` has said what to do). L4-40. Where the live source names no repository, `_not_the_source` returned None and the plan's own `source: repository:` was never asked: a plan for acme/Atlas edited to `Atlsa`, with `origin` then removed from the source, or run against a copy that has none (`--source`), checked `plan ok`, and `execute` went on to `git ls-remote https://github.com/acme/Atlsa.git`. `_held_repository` is the live source's repository and, where it names none, the plan's record, read as `_github_name` reads a name and never raising: a record that is no text, or names no `owner/repo`, holds the plan to nothing, as a null does. A plan from a source with no `origin` records null and is asked nothing, as before; one whose source names a repository is held to that, never to an older record. The detail says where the repository came from ("the plan records the source as acme/Atlas (its `origin` names no repository)"), and `_own_name_remediation` takes the repository as text, so it no longer reads `str(None)`. L4-48. The comments on #499's `org:` read said any hand edit is caught "whatever `org:` then holds", and the issue said an `org:` that `checked_value` refuses is never compared. It is caught in both commands but not compared by both: `check` asks `checked_value` first (`_manifest_values`) and ends in `unsafe-value`, exit 2; `execute` compares first, as `plan-not-the-source`; and `check` does compare it where the plan's own names are refused and no manifest is built. The block comment and `_not_the_source`'s docstring now say that, and the module docstring names the recorded-repository case. No behaviour changes there. tests/test_adopt_check_plan_name.py pins each line, with the controls: a record that is the plan's names (any case, with `.git`), none recorded, a record that is no repository, and a live `origin` that outranks the record. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 115 +++++++-- tests/test_adopt_check_plan_name.py | 368 ++++++++++++++++++++++++++++ 2 files changed, 457 insertions(+), 26 deletions(-) create mode 100644 tests/test_adopt_check_plan_name.py diff --git a/adopt-project.py b/adopt-project.py index 6e56e5df..8a0b7e76 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -38,9 +38,11 @@ `--project` name that one, compared without case, or `plan` refuses them (`adopt-not-the-source`), and `check` finds and `execute` refuses a plan edited to name another - (`plan-not-the-source`): in place, the source IS the assembly - root, and its own name is renamed first, on GitHub, never - retyped (#499). An `origin` that is a url names the + (`plan-not-the-source`), or, where the source names none, than + the plan recorded for it (#716): in place, the source IS the + assembly root, and its own name is renamed first, on GitHub, + never retyped, which `check` says as `execute` does (#499, #716). + An `origin` that is a url names the `org/repo` it ends in, with or without a trailing `/`; one that is a path, in any separator, names none, and `plan` says it compared nothing (#521). An organisation read from the url and @@ -9564,15 +9566,30 @@ def cmd_check(args) -> int: another = _not_the_source(plan, source, names) if another: findings.append(f"FINDING {PLAN_NOT_THE_SOURCE}: {another}") - remediation.append(_own_name_remediation(source, naming, pins, "")) + remediation.append(_own_name_remediation( + _held_repository(plan, source), naming, pins, "")) + # THE SOURCE IS HANDED TO `_check_names` WHERE `execute` HANDS IT (#716): + # when the plan does not name another repository, so that a refusal of + # the assembly root's own name says to rename it first, as `execute` + # says it on the same plan. Where the plan does name another, + # `execute` stops at `plan-not-the-source` and never asks the names, and + # a refusal of the plan's name is then about a name that is not the + # source's: `_own_name_remediation` has said what to do. + renamed = (_rename_first(source.repository) + if source.repository is not None and not another else None) try: - _check_names(naming, names, pins) + _check_names(naming, names, pins, None if another else source) for role, name in names.items(): found = naming.classify(name, role, pins) print(f" {role:<9} {name:<28} {found.family}" + (f"/{found.role}" if found.role else "")) except Refusal as exc: findings.append(f"FINDING {exc.code}: {exc.detail}") + # Only the rename-first remedy is printed, once after the findings: + # it is the one `check` was missing. `--project takes ...` is not, + # and `check` never printed it (#716). + if renamed is not None and exc.remediation == renamed: + remediation.append(renamed) _print_what_will_happen(plan, source, names, naming.topic_for(plan.project_id), @@ -12741,9 +12758,15 @@ def _check_names(policy: NamingPolicy, names: dict[str, str], # refuses before it asks the naming policy or any remote, a plan whose `org:` # names another with its `legs.assembly:`, which is what `execute` acts on, or # with its `project:`, as `plan-not-the-source`: a hand edit is caught too, -# whatever `org:` then holds, because it is read as `execute` reads it -# (`_plan_org`). A source that names no repository (no `origin`, or one that -# ends in no `org/repo`) is asked nothing, as before. +# because `org:` is read as `execute` reads it (`_plan_org`), as `str()` of +# what the file holds. One that `checked_value` refuses (a list, a mapping, +# an empty value, a space, a leading `-`, a character outside ASCII) is refused +# by `execute` as `plan-not-the-source` all the same, but `check` meets it +# as `unsafe-value` first, and compares it only where the plan's own names +# are refused before that (#716, L4-48). A source that names no repository +# (no `origin`, or one that ends in no `org/repo`) is asked nothing by +# `plan`, as before; `check` and `execute` hold the plan's names to what the +# plan recorded for it, `source: repository:`, where it recorded one (#716). # # AN `origin` THAT IS A PATH NAMES NO REPOSITORY (#521), in any separator # (`url_repository`). It was read as its last two directories, so a clone of @@ -12823,13 +12846,14 @@ def _path_origin_notes(args, source: Source) -> list[str]: "repository on GitHub."] -def _own_name_remediation(source: Source, naming: NamingPolicy, +def _own_name_remediation(repository: str, naming: NamingPolicy, pins: set[str], done: str) -> str: """What `adopt-not-the-source` and `plan-not-the-source` say to do: `plan` again under the source's own name, or, where the naming policy - refuses that name, `_rename_first`. `done` is what the refusing command - left as it was, said first.""" - repository = str(source.repository) + refuses that name, `_rename_first`. `repository` is the one the source + is held to (`_held_repository`), as text: where the live source names + none, the plan's record of it, and `str(None)` is no name (#716). `done` + is what the refusing command left as it was, said first.""" org, _, name = repository.partition("/") try: _check_names(naming, _names(name), pins) @@ -12866,24 +12890,57 @@ def _refuse_another_repository(args, source: Source, naming: NamingPolicy, "an in-place adoption keeps the source's own name, and `execute` " "pushes the split to the repository they name, opens its pull " "request there and sets its topic on it", - _own_name_remediation(source, naming, pins, "no plan was written. ")) + _own_name_remediation(repository, naming, pins, + "no plan was written. ")) + + +def _held_repository(plan: Plan, source: Source) -> str | None: + """The repository an in-place adoption by `plan` is held to, or None + where there is none (#716): the one the live source names + (`source.repository`), and where it names none, the one the plan + recorded for it, `source: repository:`. + + A SOURCE WITH NO `origin` ONCE HELD A PLAN TO NOTHING. `plan` records + `repository: null` for it and compares nothing, which is right; but a + plan for acme/Atlas whose names were edited to `Atlsa`, with `origin` + then removed from the source, or given a copy of it that has none by + `--source`, checked `plan ok`, and `execute` went on to ask GitHub about + `acme/Atlsa`. The record is read as `_github_name` reads a name, and + never raises: one that is no text, or names no `owner/repo`, holds the + plan to nothing, as a null does, and `Plan.open_source` is the reader + that refuses one it must clone (`_recorded_repository`).""" + if source.repository is not None: + return source.repository + recorded = plan.source.get("repository") + return _github_name(recorded) if isinstance(recorded, str) else None def _not_the_source(plan: Plan, source: Source, names: dict[str, str]) -> str | None: """`plan-not-the-source`'s detail, or None: the plan's `org:` with its `legs.assembly:` (`names`, read by `Plan.names`) or with its `project:` - names another repository than the one the source names (#499). - - `org:` is read as `execute` reads it (`_plan_org`), whatever the file - holds. While it was compared only when it was text, a number, a - boolean, a null or a deleted `org:` line checked `plan ok`, and - `execute` acted on `2026/Atlas`, `True/Atlas` or `None/Atlas` (#513's - review, SHOULD-1). Where it is not text, the detail says what the file - holds beside how `execute` spells it. `project:` is read only as text: - a misread one is not this question's to answer, and `execute` acts on - `legs.assembly:`.""" - repository = source.repository + names another repository than the one the source is held to + (`_held_repository`): the one it names (#499), or, where it names none, + the one the plan records for it (#716). + + `org:` is read as `execute` reads it (`_plan_org`), as whatever text + `str()` makes of what the file holds. While it was compared only when + it was text, a number, a boolean, a null or a deleted `org:` line + checked `plan ok`, and `execute` acted on `2026/Atlas`, `True/Atlas` or + `None/Atlas` (#513's review, SHOULD-1). Where it is not text, the detail + says what the file holds beside how `execute` spells it. `project:` is + read only as text: a misread one is not this question's to answer, and + `execute` acts on `legs.assembly:`. + + AN `org:` THAT `checked_value` REFUSES IS CAUGHT, BUT NOT ALWAYS AS + THIS (#716, L4-48): a list, a mapping, an empty value, one with a space, + a leading `-` or a character outside ASCII. `check` asks `checked_value` + of `org:` first, in `_manifest_values`, and ends in `unsafe-value`, exit + 2, before it compares anything; only where the plan's own names are + refused, and no manifest is built, does `check` reach this with it. + `execute` asks this first, so it refuses the same `org:` as + `plan-not-the-source`, before `_repository_urls` meets it.""" + repository = _held_repository(plan, source) if repository is None: return None org, held = _plan_org(plan), plan.get("org") @@ -12900,8 +12957,14 @@ def _not_the_source(plan: Plan, source: Source, keys = " and ".join(f"`{key}: {_shown(value)}`" for key, value in wrong) named = _and_listed(sorted({f"{_shown(org)}/{_shown(value)}" for _, value in wrong})) + # Where the live source names none, the plan's own record is what it is + # held to, and the detail says so: the source did not say it (#716). + is_the = (f"the source is {_shown(repository)}" + if source.repository is not None else + f"the plan records the source as {_shown(repository)} (its " + "`origin` names no repository)") return (f"the plan's {said} with its {keys} names {named}, " - f"and the source is {_shown(repository)}: an in-place adoption " + f"and {is_the}: an in-place adoption " "keeps the source's own name, and `execute` acts on the " "repository `org:` and `legs.assembly:` name") @@ -12919,7 +12982,7 @@ def _refuse_a_plan_for_another_repository( raise Refusal( PLAN_NOT_THE_SOURCE, f"{plan.path}: {detail}", _own_name_remediation( - source, naming, pins, + _held_repository(plan, source), naming, pins, "no leg repository was created and nothing was pushed. ")) diff --git a/tests/test_adopt_check_plan_name.py b/tests/test_adopt_check_plan_name.py new file mode 100644 index 00000000..4f42b815 --- /dev/null +++ b/tests/test_adopt_check_plan_name.py @@ -0,0 +1,368 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`check` says what `execute` says of a plan's name and of its source (#716). + +#499 made an in-place adoption keep the source's own name, and told +`check` and `execute` to say so alike. Two lines of that work were left, +both about a plan edited by hand, both refused by `execute` already, so that +nothing was lost, and `check` is the command that tells a person before any +leg exists: + +- L4-38: `check` asked `_check_names` about the plan's names without the + source, so a plan in the source's own name that the naming policy refuses + (`billing-service`) was `naming-unclassified` with no remedy, where + `execute` says that the source cannot be adopted in place under that name + and that renaming it on GitHub comes first. `check` now hands the source to + `_check_names` where `execute` does, and prints that remedy, once, after + its findings. +- L4-40: where the live source names no repository, the plan's own + `source: repository:` was asked nothing. A plan for acme/Atlas edited to + `Atlsa`, with `origin` then removed from the source, or run against a copy + that has none (`--source`), checked `plan ok`, and `execute` went on to ask + GitHub about `acme/Atlsa`. The plan's record holds it now, in both. +- L4-48: the comments on #499's `org:` read said that a hand edit is caught + "whatever `org:` then holds". An `org:` that `checked_value` refuses is + `unsafe-value` in `check`, which asks it first, and `plan-not-the-source` + in `execute`, which compares first: caught in both, compared only by one. + The last test here pins that, so the comments say what the commands do. + +Nothing here reaches GitHub or runs `gh`: each source's `origin` is an +`https://github.com/` URL that nothing fetches, and `execute` is run +in-process with every `git` and `gh` command it would run replaced by one +that fails the test (`refused_before_any_remote`). +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from conftest import ADOPT, git, run_script +from test_adopt_project_is_the_source import ( # noqa: F401 - a fixture + PLAN, PLAN_NOT_THE_SOURCE, RENAME_FIRST, RETYPE, adopter, check, edited, + findings, plan, refused_before_any_remote, source_of) + +#: The organisation, the source's own name, and a typo of it. +ORG, OWN, TYPO = "acme", "Atlas", "Atlsa" +#: The remedy `plan-not-the-source` gives, up to where it names the source's +#: own name and no more. `{}` is what it says first, as `check` does not. +OWN_NAME = ("Remediation: {}Run `plan` again with `--project Atlas --org " + "acme`, the source's own name,") +#: Why `plan-not-the-source` refuses, after what the plan names and the +#: source is. +WHY = ("an in-place adoption keeps the source's own name, and `execute` acts " + "on the repository `org:` and `legs.assembly:` name") +#: What `check` finds of the plan edited to `Atlsa` where the source's +#: `origin` is gone and the plan's record of it is the repository. +RECORDED = (f"the plan's `org: {ORG}` with its `legs.assembly: {TYPO}` and " + f"`project: {TYPO}` names {ORG}/{TYPO}, and the plan records the " + f"source as {ORG}/{OWN} (its `origin` names no repository): {WHY}") + + +def renamed(plan_path: Path, name: str, was: str = "Northwind") -> None: + """The plan with `name` for its project and the three names that carry + it, as a hand edit makes them.""" + edited(plan_path, *[(f"{key}: {was}{suffix}", f"{key}: {name}{suffix}") + for key, suffix in (("project", ""), + (" assembly", ""), + (" spec", "-spec"), + (" code", "-code"))]) + + +def planned_for(base: Path, project: str, org: str = ORG) -> Path: + """A source with no `origin`, and the plan `plan` writes for it as + `project`, which the plan then records no repository for.""" + source = source_of(base, None) + written = plan(base, source, "--project", project, "--org", org) + assert written.returncode == 0, written.stdout + written.stderr + return source + + +def planned_from_origin(base: Path, repository: str = f"{ORG}/{OWN}") -> Path: + """A source whose `origin` is `repository` and the plan for it, which + records `repository` as the source's. Returns the source.""" + source = source_of(base, repository) + written = plan(base, source, "--project", repository.split("/")[1]) + assert written.returncode == 0, written.stdout + written.stderr + return source + + +# --- L4-38: `check` says what `execute` says of the source's own name ------- + +@pytest.mark.parametrize("repository, code", [ + ("acme/billing-service", "naming-unclassified"), + ("acme/Tools-Install", "naming-role-mismatch"), +], ids=["no-family", "not-a-project-leg"]) +def test_check_and_execute_say_the_same_remedy_of_the_sources_own_name( + adopter, tmp_path, monkeypatch, repository, code): + """A plan whose names are the source's own, in a name the policy + refuses, as `plan` wrote it before the source had an `origin` and a + hand edit made it: `execute` says the source cannot be adopted in place + under that name and that renaming it comes first. `check` said only + that the name matched no family; it says that too, once, and never to + retype `--project`.""" + org, _, name = repository.partition("/") + source = planned_for(tmp_path, "Northwind", org) + renamed(tmp_path / PLAN, name) + git("remote", "add", "origin", f"https://github.com/{repository}.git", + cwd=source) + + checked = check(tmp_path / PLAN) + refusal = refused_before_any_remote(adopter, monkeypatch, + tmp_path / PLAN, tmp_path / "work") + + assert checked.returncode == 1, checked.stdout + checked.stderr + assert [line.split(":")[0] for line in findings(checked.stderr)] == [ + f"FINDING {code}"], checked.stderr + assert refusal.code == code + assert refusal.remediation == f"Remediation: {repository} {RENAME_FIRST}" + # Said as `execute` says it, as a line of its own, after the findings. + assert checked.stderr.splitlines().count(refusal.remediation) == 1, \ + checked.stderr + assert RETYPE not in checked.stderr + + +def test_check_says_nothing_of_renaming_a_leg_whose_name_is_the_plans_own( + tmp_path): + """A leg's name is the plan's own to edit, and `_check_names` keeps the + remedy it gave for one, which `check` never printed: a leg the policy + refuses is a finding with no rename-first text, though the source is + named.""" + planned_from_origin(tmp_path) + edited(tmp_path / PLAN, (f" spec: {OWN}-spec", " spec: bad_name")) + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 1, checked.stdout + checked.stderr + assert findings(checked.stderr) == [ + "FINDING naming-unclassified: 'bad_name' matches no family in the " + "naming policy"], checked.stderr + assert "Remediation:" not in checked.stderr + assert "gh repo rename" not in checked.stderr + + +def test_check_does_not_tell_the_source_to_be_renamed_for_another_name( + adopter, tmp_path, monkeypatch): + """Where the plan names ANOTHER repository, `execute` stops at + `plan-not-the-source` and never asks the naming policy, and the name + the policy would refuse is not the source's. `check` finds both, and its + one remedy is `plan-not-the-source`'s; it does not tell a source named + `Atlas`, a name the policy takes, that it cannot be adopted under it.""" + planned_from_origin(tmp_path) + renamed(tmp_path / PLAN, "bad_name", OWN) + + checked = check(tmp_path / PLAN) + refusal = refused_before_any_remote(adopter, monkeypatch, + tmp_path / PLAN, tmp_path / "work") + + assert checked.returncode == 1, checked.stdout + checked.stderr + assert [line.split(":")[0] for line in findings(checked.stderr)] == [ + f"FINDING {PLAN_NOT_THE_SOURCE}", + "FINDING naming-unclassified"], checked.stderr + remedies = [line for line in checked.stderr.splitlines() + if line.startswith("Remediation:")] + assert len(remedies) == 1, checked.stderr + assert remedies[0].startswith(OWN_NAME.format("")), remedies + assert "cannot be adopted in place" not in checked.stderr + assert refusal.code == PLAN_NOT_THE_SOURCE + + +# --- L4-40: the plan's record holds it where the source names none ---------- + +def unreadable_origin(source: Path) -> None: + """The source as `execute` meets it after `git remote remove origin`: + no repository named.""" + git("remote", "remove", "origin", cwd=source) + + +def test_a_plan_edited_to_a_typo_is_found_where_the_origin_is_gone( + adopter, tmp_path, monkeypatch): + """The issue's own: a plan for acme/Atlas edited to `Atlsa`, then `git + remote remove origin`. On main `check` said `plan ok` and `execute` went + on to ask GitHub about `acme/Atlsa`. Both find `plan-not-the-source`, + naming what the plan records, and neither says to run `plan` with a + project that is not a name.""" + source = planned_from_origin(tmp_path) + unreadable_origin(source) + assert check(tmp_path / PLAN).returncode == 0, "the plan, unedited" + renamed(tmp_path / PLAN, TYPO, OWN) + + checked = check(tmp_path / PLAN) + refusal = refused_before_any_remote(adopter, monkeypatch, + tmp_path / PLAN, tmp_path / "work") + + assert checked.returncode == 1, checked.stdout + checked.stderr + assert findings(checked.stderr) == [ + f"FINDING {PLAN_NOT_THE_SOURCE}: {RECORDED}"], checked.stderr + assert checked.stderr.count(OWN_NAME.format("")) == 1, checked.stderr + assert refusal.code == PLAN_NOT_THE_SOURCE + assert refusal.detail == f"{tmp_path / PLAN}: {RECORDED}" + assert refusal.remediation.startswith(OWN_NAME.format( + "no leg repository was created and nothing was pushed. ")), \ + refusal.remediation + + +def test_a_source_given_by_source_that_has_no_origin_holds_the_plan_too( + adopter, tmp_path, monkeypatch): + """`--source `, which `check` and `execute` both + take: the copy names no repository, so the plan's record does. The + source the plan names has its `origin`, and is not the one read.""" + source = planned_from_origin(tmp_path) + copy = tmp_path / "copy" + git("clone", "-q", str(source), str(copy), cwd=tmp_path) + unreadable_origin(copy) + renamed(tmp_path / PLAN, TYPO, OWN) + + checked = run_script(ADOPT, "check", "--plan", str(tmp_path / PLAN), + "--source", str(copy)) + real = adopter.Plan.open_source + # `refused_before_any_remote` passes no `--source`: every open of the + # source is handed the copy as one. + monkeypatch.setattr(adopter.Plan, "open_source", + lambda self, _, *rest, **keywords: real( + self, str(copy), *rest, **keywords)) + refusal = refused_before_any_remote(adopter, monkeypatch, + tmp_path / PLAN, tmp_path / "work") + + assert checked.returncode == 1, checked.stdout + checked.stderr + assert findings(checked.stderr) == [ + f"FINDING {PLAN_NOT_THE_SOURCE}: {RECORDED}"], checked.stderr + assert (refusal.code, refusal.detail) == ( + PLAN_NOT_THE_SOURCE, f"{tmp_path / PLAN}: {RECORDED}") + + +@pytest.mark.parametrize("held", ["acme/Atlas", "ACME/atlas", + "acme/Atlas.git"], + ids=["exact", "another-case", "dot-git"]) +def test_a_plan_that_names_the_recorded_repository_is_not_refused( + adopter, tmp_path, monkeypatch, held): + """Control: a record that the plan's names are, as GitHub resolves them + (without case, with or without `.git`), holds nothing against it. `check` + says `plan ok`, and `execute` goes on to the first command it runs + against GitHub, which the stand-in turns into the test's own failure.""" + source = planned_from_origin(tmp_path) + unreadable_origin(source) + edited(tmp_path / PLAN, (" repository: acme/Atlas", + f" repository: {held}")) + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 0, checked.stdout + checked.stderr + assert "\nplan ok\n" in checked.stdout + with pytest.raises(AssertionError, match=r"execute ran \['git', " + r"'ls-remote', 'https://github" + r"\.com/acme/Atlas\.git'"): + refused_before_any_remote(adopter, monkeypatch, tmp_path / PLAN, + tmp_path / "work") + + +def test_a_plan_that_recorded_no_repository_holds_nothing( + adopter, tmp_path, monkeypatch): + """Control: a source with no `origin` when `plan` read it has no + repository to record (`repository: null`), so the plan's names are its + own, as ever: edited to another, `check` still says `plan ok`, and + `execute` goes on to ask about the name it was given.""" + planned_for(tmp_path, "Northwind") + renamed(tmp_path / PLAN, "Northwnd") + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 0, checked.stdout + checked.stderr + assert "\nplan ok\n" in checked.stdout + with pytest.raises(AssertionError, match=r"execute ran \['git', " + r"'ls-remote', 'https://github" + r"\.com/acme/Northwnd\.git'"): + refused_before_any_remote(adopter, monkeypatch, tmp_path / PLAN, + tmp_path / "work") + + +@pytest.mark.parametrize("held", ["[acme, Atlas]", "Atlas", "2026", "~", + "acme/a/b"], + ids=["list", "no-owner", "number", "null", + "three-segments"]) +def test_a_record_that_names_no_repository_holds_nothing_and_raises_none( + tmp_path, held): + """A record that is no text, or that names no `owner/repo`, is read as + none, as `_github_name` reads a name: `check` says `plan ok` for the + plan edited to a typo, with no traceback, as it did.""" + source = planned_from_origin(tmp_path) + unreadable_origin(source) + edited(tmp_path / PLAN, (" repository: acme/Atlas", + f" repository: {held}")) + renamed(tmp_path / PLAN, TYPO, OWN) + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 0, checked.stdout + checked.stderr + assert "Traceback" not in checked.stderr + assert "\nplan ok\n" in checked.stdout + + +def test_the_sources_own_origin_is_asked_before_the_plans_record(tmp_path): + """Control: where the source names a repository, that is the one the + plan is held to, as #499 made it; the plan's record of an older one is + never asked. A plan for the source as it is checks ok, though the record + names another.""" + planned_from_origin(tmp_path) + edited(tmp_path / PLAN, (" repository: acme/Atlas", + " repository: acme/Elsewhere")) + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 0, checked.stdout + checked.stderr + assert "\nplan ok\n" in checked.stdout + + +# --- L4-48: what `check` does with an `org:` `checked_value` refuses -------- + +#: The value `execute` spells, and `check` asks `checked_value` about. +UNSAFE_ORGS = [("org: acme labs", "'acme labs'"), + ("org: [acme, labs]", "\"['acme', 'labs']\""), + ('org: ""', "''"), + ("org: -acme", "'-acme'"), + ("org: \u00e1cme", "'\u00e1cme'"), + ("org: {a: b}", "\"{'a': 'b'}\"")] + + +@pytest.mark.parametrize("org, spelled", UNSAFE_ORGS, + ids=["space", "list", "empty", "leading-dash", + "not-ascii", "mapping"]) +def test_an_unsafe_org_is_unsafe_in_check_and_compared_in_execute( + adopter, tmp_path, monkeypatch, org, spelled): + """L4-48: the comments on #499's `org:` read said any `org:` is caught + "whatever the file holds". It is caught in both commands, and not by the + same one: `check` asks `checked_value` first and ends in `unsafe-value`, + exit 2, never comparing it with the repository; `execute` compares it + first and refuses it as `plan-not-the-source`.""" + planned_from_origin(tmp_path) + edited(tmp_path / PLAN, ("org: acme", org)) + + checked = check(tmp_path / PLAN) + refusal = refused_before_any_remote(adopter, monkeypatch, + tmp_path / PLAN, tmp_path / "work") + + assert checked.returncode == 2, checked.stdout + checked.stderr + assert f"REFUSED unsafe-value: org is {spelled}, which is not a value " \ + "this tool will put on a `git` or `gh` command line" in \ + checked.stderr.splitlines() + assert PLAN_NOT_THE_SOURCE not in checked.stderr + assert refusal.code == PLAN_NOT_THE_SOURCE + + +def test_check_compares_an_unsafe_org_where_no_manifest_is_built( + tmp_path): + """The one place `check` reaches the comparison with such an `org:`: + where the plan's own names are refused first, `_manifest_values` builds + no manifest and never asks `checked_value`, so `check` finds the plan + names another repository, and the name, and exits 1.""" + planned_from_origin(tmp_path) + edited(tmp_path / PLAN, ("org: acme", "org: acme labs")) + renamed(tmp_path / PLAN, "bad_name", OWN) + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 1, checked.stdout + checked.stderr + assert [line.split(":")[0] for line in findings(checked.stderr)] == [ + f"FINDING {PLAN_NOT_THE_SOURCE}", + "FINDING naming-unclassified"], checked.stderr + assert "`org: acme labs`" in findings(checked.stderr)[0] From a20dcaa46387ec9a4ab602a70f49f0f2edf49cff Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 16:37:36 +0000 Subject: [PATCH 2/4] adopt-project.py: a plan's record that is a path `origin`'s last two directories holds nothing, so a plan v1.0.x wrote for one stays correct, the L4-48 comments say which command compares an unsafe `org:`, and `plan-not-the-source` says a source has no `origin` where it has none (#716) Fix round 1 for #825, on RV825's review of 4ba4e783. MUST-1. v1.0.0 and v1.0.1 read a path `origin` as its last two directories (#521 is in neither tag), so `plan --org acme --project Atlas` on a clone of `/srv/clones/Atlas.git` was refused as `--org clones`, and a plan written all the same recorded `repository: clones/Atlas`. With `org: acme` put back by hand it is a correct plan, which main checks `plan ok` and `execute` takes on to acme/Atlas. `_held_repository` held it to that record and refused it, with a remedy that brings `--org clones` back. Where the source's `origin` is a path (`_origin_path`), a record that is that path's last two directories (`_last_two_directories`: either separator, one `.git` dropped, compared without case) is the old reader's, not the source's name, and the plan is held to nothing. Any other record is held: a `git clone` of the source, whose `origin` is a path, still holds a plan edited to `Atlsa` to acme/Atlas. SHOULD-1. Both L4-48 comments said an unsafe `org:` is refused by `execute` as `plan-not-the-source`, and that `check` asks `checked_value` first in `_manifest_values`. Neither holds throughout: where the plan is held to nothing the same `org:` is `unsafe-value` in `execute` too, and `_submodule_findings` asks `checked_value` earlier than `_manifest_values` where a kept submodule url is relative. Both comments say that. NIT-1. The module docstring's sentence reads as a sentence, and no longer says `check` says the rename-first remedy "as `execute` does" of the recorded-repository case. NIT-3. A source with no `origin` at all says "it has no `origin`" beside the plan's record; one whose `origin` names no repository (a path, a url that ends in no `org/repo`) keeps "its `origin` names no repository". NIT-2 is a follow-up and changes no code: `_check_names` takes the `Source`, not the held repository, so a plan whose names are the recorded repository's own, in a name the policy refuses, gets no rename-first remedy in `check` or `execute`. tests/test_adopt_check_plan_name.py pins each: the plan v1.0.x wrote, in eight spellings of the path; the typo plan against a source with no `origin`, a path, a one-segment path and a url ending in no `org/repo`, and against a `--source` copy that kept its path `origin`; the unsafe `org:` in `check` and `execute`, held to a repository and held to none; and the relative-submodule corner. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 93 +++++++++---- tests/test_adopt_check_plan_name.py | 196 ++++++++++++++++++++++------ 2 files changed, 228 insertions(+), 61 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 8a0b7e76..0685be97 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -37,14 +37,13 @@ whole (#273). Where the source names a repository, `--org` and `--project` name that one, compared without case, or `plan` refuses them (`adopt-not-the-source`), and `check` finds and - `execute` refuses a plan edited to name another - (`plan-not-the-source`), or, where the source names none, than - the plan recorded for it (#716): in place, the source IS the - assembly root, and its own name is renamed first, on GitHub, - never retyped, which `check` says as `execute` does (#499, #716). - An `origin` that is a url names the - `org/repo` it ends in, with or without a trailing `/`; one that - is a path, in any separator, names none, and `plan` says it + `execute` refuses a plan edited to name another repository than + the source names, or, where it names none, than the plan + recorded for it (`plan-not-the-source`, #716): in place, the + source IS the assembly root, and its own name is renamed first, + on GitHub, never retyped (#499). An `origin` that is a url names + the `org/repo` it ends in, with or without a trailing `/`; one + that is a path, in any separator, names none, and `plan` says it compared nothing (#521). An organisation read from the url and refused is named by where it came from (#427). check validates a plan against the source: each entry path bound for @@ -12760,10 +12759,14 @@ def _check_names(policy: NamingPolicy, names: dict[str, str], # with its `project:`, as `plan-not-the-source`: a hand edit is caught too, # because `org:` is read as `execute` reads it (`_plan_org`), as `str()` of # what the file holds. One that `checked_value` refuses (a list, a mapping, -# an empty value, a space, a leading `-`, a character outside ASCII) is refused -# by `execute` as `plan-not-the-source` all the same, but `check` meets it -# as `unsafe-value` first, and compares it only where the plan's own names -# are refused before that (#716, L4-48). A source that names no repository +# an empty value, a space, a leading `-`, a character outside ASCII) is, in +# `execute`, `plan-not-the-source` where the plan is held to a repository +# (`_held_repository`) and `unsafe-value` where it is held to none. `check` +# asks `checked_value` of `org:` first (`_manifest_values`, and +# `_submodule_findings` where a kept submodule url is relative) and ends in +# `unsafe-value`, exit 2, before it compares anything; only where the plan's +# own names are refused, and no kept url is relative, does it reach the +# comparison with one (#716, L4-48). A source that names no repository # (no `origin`, or one that ends in no `org/repo`) is asked nothing by # `plan`, as before; `check` and `execute` hold the plan's names to what the # plan recorded for it, `source: repository:`, where it recorded one (#716). @@ -12894,6 +12897,18 @@ def _refuse_another_repository(args, source: Source, naming: NamingPolicy, "no plan was written. ")) +def _last_two_directories(path: str) -> str | None: + """`/` for a filesystem path, in either separator, with + one `.git` dropped from the name and the case kept; None where the path + has fewer than two. It is what `url_repository` read of a path `origin` + until #521 (#716).""" + segments = [part for part in re.split(r"[\\/]", path) if part] + if segments and segments[-1].endswith(".git") \ + and len(segments[-1]) > len(".git"): + segments[-1] = segments[-1][:-len(".git")] + return "/".join(segments[-2:]) if len(segments) >= 2 else None + + def _held_repository(plan: Plan, source: Source) -> str | None: """The repository an in-place adoption by `plan` is held to, or None where there is none (#716): the one the live source names @@ -12908,11 +12923,42 @@ def _held_repository(plan: Plan, source: Source) -> str | None: `acme/Atlsa`. The record is read as `_github_name` reads a name, and never raises: one that is no text, or names no `owner/repo`, holds the plan to nothing, as a null does, and `Plan.open_source` is the reader - that refuses one it must clone (`_recorded_repository`).""" + that refuses one it must clone (`_recorded_repository`). + + A RECORD THAT IS THE PATH'S OWN LAST TWO DIRECTORIES HOLDS NOTHING. v1.0.0 + and v1.0.1 read a path `origin` as those directories (#521), so `plan + --org acme --project Atlas` on a clone of `/srv/clones/Atlas.git` was + refused with "`--org clones`" and, where the plan was written all the + same, recorded `repository: clones/Atlas`. A person who put `org: acme` + back by hand holds a correct plan, and holding it to that record + refused it, with a remedy that is the advice #521 took out. Where the + source's `origin` is a path (`_origin_path`), a record that is its last + two directories, as `_last_two_directories` reads them without case, in + either separator, is the old reader's and not the source's name: the + plan is held to nothing. Any other record is held, so a `git clone` of + the source, whose `origin` is a path, still holds a plan edited to + `Atlsa` to `acme/Atlas`.""" if source.repository is not None: return source.repository recorded = plan.source.get("repository") - return _github_name(recorded) if isinstance(recorded, str) else None + held = _github_name(recorded) if isinstance(recorded, str) else None + path = _origin_path(source) if held is not None else None + if path is not None and held.casefold() == ( + _last_two_directories(path) or "").casefold(): + return None + return held + + +def _why_the_source_names_none(source: Source) -> str: + """Why the live source names no repository, as `_not_the_source` says it + beside the plan's record (#716): it has no `origin`, or its `origin` + names none (a path, a url that ends in no `org/repo`, or one that is not + UTF-8: `_remote_repository`).""" + try: + git_out(["remote", "get-url", "origin"], cwd=source.path, binary=True) + except Refusal: + return "it has no `origin`" + return "its `origin` names no repository" def _not_the_source(plan: Plan, source: Source, @@ -12934,12 +12980,15 @@ def _not_the_source(plan: Plan, source: Source, AN `org:` THAT `checked_value` REFUSES IS CAUGHT, BUT NOT ALWAYS AS THIS (#716, L4-48): a list, a mapping, an empty value, one with a space, - a leading `-` or a character outside ASCII. `check` asks `checked_value` - of `org:` first, in `_manifest_values`, and ends in `unsafe-value`, exit - 2, before it compares anything; only where the plan's own names are - refused, and no manifest is built, does `check` reach this with it. - `execute` asks this first, so it refuses the same `org:` as - `plan-not-the-source`, before `_repository_urls` meets it.""" + a leading `-` or a character outside ASCII. `execute` asks this first, + so it refuses such an `org:` as `plan-not-the-source` where the plan is + held to a repository (`_held_repository`), and as `unsafe-value`, from + `_repository_urls`, where it is held to none. `check` asks + `checked_value` of `org:` first (`_manifest_values`, and + `_submodule_findings` where a kept submodule url is relative) and ends + in `unsafe-value`, exit 2, before it compares anything; only where the + plan's own names are refused, and no kept url is relative, does it + reach this with one.""" repository = _held_repository(plan, source) if repository is None: return None @@ -12961,8 +13010,8 @@ def _not_the_source(plan: Plan, source: Source, # held to, and the detail says so: the source did not say it (#716). is_the = (f"the source is {_shown(repository)}" if source.repository is not None else - f"the plan records the source as {_shown(repository)} (its " - "`origin` names no repository)") + f"the plan records the source as {_shown(repository)} " + f"({_why_the_source_names_none(source)})") return (f"the plan's {said} with its {keys} names {named}, " f"and {is_the}: an in-place adoption " "keeps the source's own name, and `execute` acts on the " diff --git a/tests/test_adopt_check_plan_name.py b/tests/test_adopt_check_plan_name.py index 4f42b815..3d9c3d83 100644 --- a/tests/test_adopt_check_plan_name.py +++ b/tests/test_adopt_check_plan_name.py @@ -21,9 +21,18 @@ GitHub about `acme/Atlsa`. The plan's record holds it now, in both. - L4-48: the comments on #499's `org:` read said that a hand edit is caught "whatever `org:` then holds". An `org:` that `checked_value` refuses is - `unsafe-value` in `check`, which asks it first, and `plan-not-the-source` - in `execute`, which compares first: caught in both, compared only by one. - The last test here pins that, so the comments say what the commands do. + `unsafe-value` in `check`, which asks it first, and, in `execute`, + `plan-not-the-source` where the plan is held to a repository and + `unsafe-value` where it is held to none: caught in both, compared only + by one. The last tests here pin that, so the comments say what the + commands do. +- RV825's MUST-1: v1.0.0 and v1.0.1 read a path `origin` as its last two + directories (#521), so a plan they wrote can record `repository: + clones/Atlas` for a source whose `origin` is `/clones/Atlas.git`. + With the right `org:` put back by hand it is a correct plan, and holding + it to that record refused it and brought back the advice #521 took out + (`--org clones`). A record that is the path's own last two directories + holds nothing. Nothing here reaches GitHub or runs `gh`: each source's `origin` is an `https://github.com/` URL that nothing fetches, and `execute` is run @@ -41,6 +50,9 @@ from test_adopt_project_is_the_source import ( # noqa: F401 - a fixture PLAN, PLAN_NOT_THE_SOURCE, RENAME_FIRST, RETYPE, adopter, check, edited, findings, plan, refused_before_any_remote, source_of) +from test_adopt_relative_submodule_url import ( + HTTPS_ORIGIN, SIBLING, THING, planned as planned_with_submodule, + source_with as source_with_submodule) #: The organisation, the source's own name, and a typo of it. ORG, OWN, TYPO = "acme", "Atlas", "Atlsa" @@ -52,11 +64,19 @@ #: source is. WHY = ("an in-place adoption keeps the source's own name, and `execute` acts " "on the repository `org:` and `legs.assembly:` name") -#: What `check` finds of the plan edited to `Atlsa` where the source's -#: `origin` is gone and the plan's record of it is the repository. -RECORDED = (f"the plan's `org: {ORG}` with its `legs.assembly: {TYPO}` and " +#: Why the live source names no repository, as the finding says it: it has +#: no `origin` at all, or its `origin` names none (a path, here). +NO_ORIGIN = "it has no `origin`" +NAMES_NONE = "its `origin` names no repository" + + +def recorded(why: str) -> str: + """What `check` finds of the plan edited to `Atlsa` where the live + source names no repository, for `why`, and the plan's record of it is + the repository.""" + return (f"the plan's `org: {ORG}` with its `legs.assembly: {TYPO}` and " f"`project: {TYPO}` names {ORG}/{TYPO}, and the plan records the " - f"source as {ORG}/{OWN} (its `origin` names no repository): {WHY}") + f"source as {ORG}/{OWN} ({why}): {WHY}") def renamed(plan_path: Path, name: str, was: str = "Northwind") -> None: @@ -169,21 +189,33 @@ def test_check_does_not_tell_the_source_to_be_renamed_for_another_name( # --- L4-40: the plan's record holds it where the source names none ---------- -def unreadable_origin(source: Path) -> None: - """The source as `execute` meets it after `git remote remove origin`: - no repository named.""" - git("remote", "remove", "origin", cwd=source) - - -def test_a_plan_edited_to_a_typo_is_found_where_the_origin_is_gone( - adopter, tmp_path, monkeypatch): +def unreadable_origin(source: Path, origin: str | None = None) -> None: + """The source as `execute` meets it after `git remote remove origin`, or + with `origin` instead: no repository named.""" + if origin is None: + git("remote", "remove", "origin", cwd=source) + else: + git("remote", "set-url", "origin", origin, cwd=source) + + +@pytest.mark.parametrize("origin, why", [ + (None, NO_ORIGIN), + ("/srv/mirrors/Atlas.git", NAMES_NONE), + ("Atlas.git", NAMES_NONE), + ("https://example.invalid/Atlas", NAMES_NONE), +], ids=["no-origin", "path", "one-segment-path", "url-with-no-org"]) +def test_a_plan_edited_to_a_typo_is_found_where_the_origin_names_none( + adopter, tmp_path, monkeypatch, origin, why): """The issue's own: a plan for acme/Atlas edited to `Atlsa`, then `git remote remove origin`. On main `check` said `plan ok` and `execute` went on to ask GitHub about `acme/Atlsa`. Both find `plan-not-the-source`, naming what the plan records, and neither says to run `plan` with a - project that is not a name.""" + project that is not a name. A source with no `origin` says it has none + (RV825's NIT-3); one whose `origin` names no repository, a path or a url + that ends in no `org/repo`, says that. A record that is not the path's + own last two directories is held.""" source = planned_from_origin(tmp_path) - unreadable_origin(source) + unreadable_origin(source, origin) assert check(tmp_path / PLAN).returncode == 0, "the plan, unedited" renamed(tmp_path / PLAN, TYPO, OWN) @@ -193,24 +225,30 @@ def test_a_plan_edited_to_a_typo_is_found_where_the_origin_is_gone( assert checked.returncode == 1, checked.stdout + checked.stderr assert findings(checked.stderr) == [ - f"FINDING {PLAN_NOT_THE_SOURCE}: {RECORDED}"], checked.stderr + f"FINDING {PLAN_NOT_THE_SOURCE}: {recorded(why)}"], checked.stderr assert checked.stderr.count(OWN_NAME.format("")) == 1, checked.stderr assert refusal.code == PLAN_NOT_THE_SOURCE - assert refusal.detail == f"{tmp_path / PLAN}: {RECORDED}" + assert refusal.detail == f"{tmp_path / PLAN}: {recorded(why)}" assert refusal.remediation.startswith(OWN_NAME.format( "no leg repository was created and nothing was pushed. ")), \ refusal.remediation -def test_a_source_given_by_source_that_has_no_origin_holds_the_plan_too( - adopter, tmp_path, monkeypatch): - """`--source `, which `check` and `execute` both - take: the copy names no repository, so the plan's record does. The +@pytest.mark.parametrize("keep_origin, why", [ + (False, NO_ORIGIN), + (True, NAMES_NONE), +], ids=["no-origin", "origin-is-the-path-it-was-cloned-from"]) +def test_a_source_given_by_source_that_names_none_holds_the_plan_too( + adopter, tmp_path, monkeypatch, keep_origin, why): + """`--source `, which `check` and `execute` both take: a copy + with no `origin`, or one whose `origin` is the path it was cloned from + (which names no repository), names none, so the plan's record does. The source the plan names has its `origin`, and is not the one read.""" source = planned_from_origin(tmp_path) copy = tmp_path / "copy" git("clone", "-q", str(source), str(copy), cwd=tmp_path) - unreadable_origin(copy) + if not keep_origin: + unreadable_origin(copy) renamed(tmp_path / PLAN, TYPO, OWN) checked = run_script(ADOPT, "check", "--plan", str(tmp_path / PLAN), @@ -226,9 +264,9 @@ def test_a_source_given_by_source_that_has_no_origin_holds_the_plan_too( assert checked.returncode == 1, checked.stdout + checked.stderr assert findings(checked.stderr) == [ - f"FINDING {PLAN_NOT_THE_SOURCE}: {RECORDED}"], checked.stderr + f"FINDING {PLAN_NOT_THE_SOURCE}: {recorded(why)}"], checked.stderr assert (refusal.code, refusal.detail) == ( - PLAN_NOT_THE_SOURCE, f"{tmp_path / PLAN}: {RECORDED}") + PLAN_NOT_THE_SOURCE, f"{tmp_path / PLAN}: {recorded(why)}") @pytest.mark.parametrize("held", ["acme/Atlas", "ACME/atlas", @@ -313,6 +351,49 @@ def test_the_sources_own_origin_is_asked_before_the_plans_record(tmp_path): assert "\nplan ok\n" in checked.stdout +# --- RV825's MUST-1: a plan v1.0.0 or v1.0.1 wrote for a path `origin` ------- + +@pytest.mark.parametrize("origin", [ + "/srv/clones/Atlas.git", + "/srv/clones/Atlas", + "/srv/clones/Atlas.git/", + "/srv/Clones/ATLAS.git", + "C:\\srv\\clones\\Atlas.git", + "\\\\server\\clones\\Atlas.git", + "file:///srv/clones/Atlas.git", + "clones/Atlas.git", +], ids=["dot-git", "no-dot-git", "trailing-slash", "another-case", + "backslashes", "share", "file-url", "relative"]) +def test_a_plan_that_v1_0_x_wrote_for_a_path_origin_is_still_a_correct_plan( + adopter, tmp_path, monkeypatch, origin): + """v1.0.0 and v1.0.1 read a path `origin` as its last two directories + (#521): `plan --org acme --project Atlas` on a clone of + `/srv/clones/Atlas.git` was refused as `--org clones`, and the plan + written all the same recorded `repository: clones/Atlas`. With `org: + acme` put back by hand it is a correct plan, and `check` said `plan ok` + and `execute` went on to `acme/Atlas`. Held to that record it was + refused, with a remedy that brings `--org clones` back; the record is + the old reader's, not the source's name, so it holds nothing. The plan + here is the one this version writes, with its null edited to what + those versions recorded.""" + source = source_of(tmp_path, None) + git("remote", "add", "origin", origin, cwd=source) + written = plan(tmp_path, source, "--project", OWN, "--org", ORG) + assert written.returncode == 0, written.stdout + written.stderr + edited(tmp_path / PLAN, (" repository: null", + " repository: clones/Atlas")) + + checked = check(tmp_path / PLAN) + + assert checked.returncode == 0, checked.stdout + checked.stderr + assert "\nplan ok\n" in checked.stdout + with pytest.raises(AssertionError, match=r"execute ran \['git', " + r"'ls-remote', 'https://github" + r"\.com/acme/Atlas\.git'"): + refused_before_any_remote(adopter, monkeypatch, tmp_path / PLAN, + tmp_path / "work") + + # --- L4-48: what `check` does with an `org:` `checked_value` refuses -------- #: The value `execute` spells, and `check` asks `checked_value` about. @@ -327,14 +408,23 @@ def test_the_sources_own_origin_is_asked_before_the_plans_record(tmp_path): @pytest.mark.parametrize("org, spelled", UNSAFE_ORGS, ids=["space", "list", "empty", "leading-dash", "not-ascii", "mapping"]) -def test_an_unsafe_org_is_unsafe_in_check_and_compared_in_execute( - adopter, tmp_path, monkeypatch, org, spelled): +@pytest.mark.parametrize("held, code", [ + (True, PLAN_NOT_THE_SOURCE), + (False, "unsafe-value"), +], ids=["held-to-a-repository", "held-to-none"]) +def test_an_unsafe_org_is_unsafe_in_check_and_compared_by_execute_if_held( + adopter, tmp_path, monkeypatch, org, spelled, held, code): """L4-48: the comments on #499's `org:` read said any `org:` is caught "whatever the file holds". It is caught in both commands, and not by the same one: `check` asks `checked_value` first and ends in `unsafe-value`, - exit 2, never comparing it with the repository; `execute` compares it - first and refuses it as `plan-not-the-source`.""" - planned_from_origin(tmp_path) + exit 2, never comparing it with the repository. `execute` compares it + first, so it refuses it as `plan-not-the-source` where the plan is held + to a repository, and as `unsafe-value` where it is held to none (a source + that recorded no repository), as `check` does in both.""" + if held: + planned_from_origin(tmp_path) + else: + planned_for(tmp_path, OWN) edited(tmp_path / PLAN, ("org: acme", org)) checked = check(tmp_path / PLAN) @@ -342,19 +432,22 @@ def test_an_unsafe_org_is_unsafe_in_check_and_compared_in_execute( tmp_path / PLAN, tmp_path / "work") assert checked.returncode == 2, checked.stdout + checked.stderr - assert f"REFUSED unsafe-value: org is {spelled}, which is not a value " \ - "this tool will put on a `git` or `gh` command line" in \ - checked.stderr.splitlines() + unsafe = (f"REFUSED unsafe-value: org is {spelled}, which is not a value " + "this tool will put on a `git` or `gh` command line") + assert unsafe in checked.stderr.splitlines() assert PLAN_NOT_THE_SOURCE not in checked.stderr - assert refusal.code == PLAN_NOT_THE_SOURCE + assert refusal.code == code + if not held: + assert f"{refusal.code}: {refusal.detail}" == unsafe[len("REFUSED "):] def test_check_compares_an_unsafe_org_where_no_manifest_is_built( tmp_path): - """The one place `check` reaches the comparison with such an `org:`: - where the plan's own names are refused first, `_manifest_values` builds - no manifest and never asks `checked_value`, so `check` finds the plan - names another repository, and the name, and exits 1.""" + """Where `check` reaches the comparison with such an `org:`: where the + plan's own names are refused first, `_manifest_values` builds no + manifest and never asks `checked_value` (and no kept submodule url is + relative: the next test), so `check` finds the plan names another + repository, and the name, and exits 1.""" planned_from_origin(tmp_path) edited(tmp_path / PLAN, ("org: acme", "org: acme labs")) renamed(tmp_path / PLAN, "bad_name", OWN) @@ -366,3 +459,28 @@ def test_check_compares_an_unsafe_org_where_no_manifest_is_built( f"FINDING {PLAN_NOT_THE_SOURCE}", "FINDING naming-unclassified"], checked.stderr assert "`org: acme labs`" in findings(checked.stderr)[0] + + +def test_check_asks_an_unsafe_org_first_where_a_kept_submodule_url_is_relative( + adopter, tmp_path, monkeypatch): + """RV825's SHOULD-1(b): `_submodule_findings` asks `checked_value` of + `org:` earlier than `_manifest_values` does, wherever a kept submodule's + url is relative. With the plan's names refused and `org: test org`, + `check` ends in `unsafe-value`, exit 2, and never compares it, as it + does with no relative url (the test above); `execute` says + `plan-not-the-source`.""" + source = source_with_submodule(tmp_path, SIBLING, HTTPS_ORIGIN) + made = planned_with_submodule(source, tmp_path) + edited(made, ("org: testorg", "org: test org")) + renamed(made, "bad_name", THING) + + checked = check(made) + refusal = refused_before_any_remote(adopter, monkeypatch, made, + tmp_path / "work") + + assert checked.returncode == 2, checked.stdout + checked.stderr + assert checked.stderr.splitlines()[0] == ( + "REFUSED unsafe-value: org is 'test org', which is not a value this " + "tool will put on a `git` or `gh` command line") + assert PLAN_NOT_THE_SOURCE not in checked.stderr + assert refusal.code == PLAN_NOT_THE_SOURCE From 1bb91fe364ade95b6e44fbadec324245869f8ee9 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 18:54:31 +0000 Subject: [PATCH 3/4] adopt-project.py: a path `origin`'s record is read as v1.0.1 read it, `:` a separator and a doubled `.git` kept, so `/srv/a:b/Atlas.git` and `/srv/clones/Atlas.git.git` hold a plan to nothing too, and the comments say which `org:` is the held repository's own (#716) Round 2 for #825, on RV825's re-check of a20dcaa4. NIT r1-2. v1.0.1's `_remote_repository` took `[:/]([^/:]+/[^/]+?)(?:\.git)?$` of a path `origin`, so `/srv/a:b/Atlas.git` recorded `b/Atlas` and `/srv/clones/Atlas.git.git` recorded `clones/Atlas.git`, and round 1 still held both and refused them. `_last_two_directories` splits at `/`, `\` and `:`, and `_held_repository` compares `_github_name` of it with the held name, so a record `clones/Atlas.git` (which `_github_name` reads as `clones/Atlas`) is the old reader's too. The test gets both origins, each with the record v1.0.1 writes. NIT r1-1. `_last_two_directories`'s docstring says it is a superset of what the old reader read, and names what it adds: a trailing `/`, a backslash path and a relative one, which held a plan to nothing as main does. NIT r1-3. Both L4-48 comments say `plan-not-the-source` where the plan is held to a repository and the `org:` is not that repository's own: an `org:` that is the repository's own, and one `checked_value` refuses, is `unsafe-value` in both commands. SHOULD r1-1 (SonarCloud S3415). The test's last assert puts the actual value first, as the file's others do. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- adopt-project.py | 52 ++++++++++++++++++----------- tests/test_adopt_check_plan_name.py | 32 ++++++++++-------- 2 files changed, 52 insertions(+), 32 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index 0685be97..7d48908a 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -12761,15 +12761,16 @@ def _check_names(policy: NamingPolicy, names: dict[str, str], # what the file holds. One that `checked_value` refuses (a list, a mapping, # an empty value, a space, a leading `-`, a character outside ASCII) is, in # `execute`, `plan-not-the-source` where the plan is held to a repository -# (`_held_repository`) and `unsafe-value` where it is held to none. `check` -# asks `checked_value` of `org:` first (`_manifest_values`, and -# `_submodule_findings` where a kept submodule url is relative) and ends in -# `unsafe-value`, exit 2, before it compares anything; only where the plan's -# own names are refused, and no kept url is relative, does it reach the -# comparison with one (#716, L4-48). A source that names no repository -# (no `origin`, or one that ends in no `org/repo`) is asked nothing by -# `plan`, as before; `check` and `execute` hold the plan's names to what the -# plan recorded for it, `source: repository:`, where it recorded one (#716). +# (`_held_repository`, and the `org:` is not that repository's own) and +# `unsafe-value` where it is held to none. `check` asks `checked_value` of +# `org:` first (`_manifest_values`, and `_submodule_findings` where a kept +# submodule url is relative) and ends in `unsafe-value`, exit 2, before it +# compares anything; only where the plan's own names are refused, and no +# kept url is relative, does it reach the comparison with one (#716, +# L4-48). A source that names no repository (no `origin`, or one that ends +# in no `org/repo`) is asked nothing by `plan`, as before; `check` and +# `execute` hold the plan's names to what the plan recorded for it, +# `source: repository:`, where it recorded one (#716). # # AN `origin` THAT IS A PATH NAMES NO REPOSITORY (#521), in any separator # (`url_repository`). It was read as its last two directories, so a clone of @@ -12898,11 +12899,19 @@ def _refuse_another_repository(args, source: Source, naming: NamingPolicy, def _last_two_directories(path: str) -> str | None: - """`/` for a filesystem path, in either separator, with - one `.git` dropped from the name and the case kept; None where the path - has fewer than two. It is what `url_repository` read of a path `origin` - until #521 (#716).""" - segments = [part for part in re.split(r"[\\/]", path) if part] + """`/` for a filesystem path, in either separator and + after a `:`, with one `.git` dropped from the name and the case kept; + None where the path has fewer than two (#716). + + A SUPERSET OF WHAT v1.0.0 AND v1.0.1 READ. Their `_remote_repository` + took `[:/]([^/:]+/[^/]+?)(?:\\.git)?$` of a path `origin` (until #521), + which is its last two segments, split at `/` or `:`, one `.git` dropped + from the second: `/srv/a:b/Atlas.git` was `b/Atlas`, and + `/srv/clones/Atlas.git.git` was `clones/Atlas.git`. It read none for a + trailing `/`, a backslash path, or a relative one that starts with no + separator; this reads each as its last two directories too, which only + holds a plan to nothing more often, and that is what main does.""" + segments = [part for part in re.split(r"[\\/:]", path) if part] if segments and segments[-1].endswith(".git") \ and len(segments[-1]) > len(".git"): segments[-1] = segments[-1][:-len(".git")] @@ -12943,9 +12952,13 @@ def _held_repository(plan: Plan, source: Source) -> str | None: recorded = plan.source.get("repository") held = _github_name(recorded) if isinstance(recorded, str) else None path = _origin_path(source) if held is not None else None - if path is not None and held.casefold() == ( - _last_two_directories(path) or "").casefold(): - return None + if path is not None: + # Read as `held` was, so the two spellings are one name: v1.0.1 + # records `clones/Atlas.git` for `/srv/clones/Atlas.git.git`. + old = _last_two_directories(path) + old = None if old is None else _github_name(old) + if old is not None and old.casefold() == held.casefold(): + return None return held @@ -12982,8 +12995,9 @@ def _not_the_source(plan: Plan, source: Source, THIS (#716, L4-48): a list, a mapping, an empty value, one with a space, a leading `-` or a character outside ASCII. `execute` asks this first, so it refuses such an `org:` as `plan-not-the-source` where the plan is - held to a repository (`_held_repository`), and as `unsafe-value`, from - `_repository_urls`, where it is held to none. `check` asks + held to a repository (`_held_repository`, and the `org:` is not that + repository's own), and as `unsafe-value`, from `_repository_urls`, + where it is held to none. `check` asks `checked_value` of `org:` first (`_manifest_values`, and `_submodule_findings` where a kept submodule url is relative) and ends in `unsafe-value`, exit 2, before it compares anything; only where the diff --git a/tests/test_adopt_check_plan_name.py b/tests/test_adopt_check_plan_name.py index 3d9c3d83..fb214301 100644 --- a/tests/test_adopt_check_plan_name.py +++ b/tests/test_adopt_check_plan_name.py @@ -353,19 +353,25 @@ def test_the_sources_own_origin_is_asked_before_the_plans_record(tmp_path): # --- RV825's MUST-1: a plan v1.0.0 or v1.0.1 wrote for a path `origin` ------- -@pytest.mark.parametrize("origin", [ - "/srv/clones/Atlas.git", - "/srv/clones/Atlas", - "/srv/clones/Atlas.git/", - "/srv/Clones/ATLAS.git", - "C:\\srv\\clones\\Atlas.git", - "\\\\server\\clones\\Atlas.git", - "file:///srv/clones/Atlas.git", - "clones/Atlas.git", +@pytest.mark.parametrize("origin, record", [ + ("/srv/clones/Atlas.git", "clones/Atlas"), + ("/srv/clones/Atlas", "clones/Atlas"), + ("/srv/clones/Atlas.git/", "clones/Atlas"), + # What v1.0.1's own regex records for it, `Clones/ATLAS`. + ("/srv/Clones/ATLAS.git", "Clones/ATLAS"), + ("C:\\srv\\clones\\Atlas.git", "clones/Atlas"), + ("\\\\server\\clones\\Atlas.git", "clones/Atlas"), + ("file:///srv/clones/Atlas.git", "clones/Atlas"), + ("clones/Atlas.git", "clones/Atlas"), + # Two that v1.0.1 really records an exotic name for: its regex takes a + # `:` for a separator and drops only the last `.git`. + ("/srv/clones/Atlas.git.git", "clones/Atlas.git"), + ("/srv/a:b/Atlas.git", "b/Atlas"), ], ids=["dot-git", "no-dot-git", "trailing-slash", "another-case", - "backslashes", "share", "file-url", "relative"]) + "backslashes", "share", "file-url", "relative", "two-dot-gits", + "colon"]) def test_a_plan_that_v1_0_x_wrote_for_a_path_origin_is_still_a_correct_plan( - adopter, tmp_path, monkeypatch, origin): + adopter, tmp_path, monkeypatch, origin, record): """v1.0.0 and v1.0.1 read a path `origin` as its last two directories (#521): `plan --org acme --project Atlas` on a clone of `/srv/clones/Atlas.git` was refused as `--org clones`, and the plan @@ -381,7 +387,7 @@ def test_a_plan_that_v1_0_x_wrote_for_a_path_origin_is_still_a_correct_plan( written = plan(tmp_path, source, "--project", OWN, "--org", ORG) assert written.returncode == 0, written.stdout + written.stderr edited(tmp_path / PLAN, (" repository: null", - " repository: clones/Atlas")) + f" repository: {record}")) checked = check(tmp_path / PLAN) @@ -438,7 +444,7 @@ def test_an_unsafe_org_is_unsafe_in_check_and_compared_by_execute_if_held( assert PLAN_NOT_THE_SOURCE not in checked.stderr assert refusal.code == code if not held: - assert f"{refusal.code}: {refusal.detail}" == unsafe[len("REFUSED "):] + assert unsafe[len("REFUSED "):] == f"{refusal.code}: {refusal.detail}" def test_check_compares_an_unsafe_org_where_no_manifest_is_built( From 6d5a448f67ea949219899e52a0d0c9e4edd70303 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 19:25:22 +0000 Subject: [PATCH 4/4] tests/test_adopt_check_plan_name.py: the another-case row keeps a record in another case than the path, beside the one v1.0.1 writes, so a record compared with case now fails a test (#716) Round 2 for #825. The v1.0.1 spelling of the case row (`Clones/ATLAS` for `/srv/Clones/ATLAS.git`) is the same case as the path, so it no longer failed a mutant that compares the record with case (R4). The row with a record a person retypes, `clones/Atlas`, is back beside it. Lane: openreposhape-4 (openRepoShape-4) Co-Authored-By: Claude Sonnet 5.5 --- tests/test_adopt_check_plan_name.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/test_adopt_check_plan_name.py b/tests/test_adopt_check_plan_name.py index fb214301..4ec05aa6 100644 --- a/tests/test_adopt_check_plan_name.py +++ b/tests/test_adopt_check_plan_name.py @@ -357,7 +357,9 @@ def test_the_sources_own_origin_is_asked_before_the_plans_record(tmp_path): ("/srv/clones/Atlas.git", "clones/Atlas"), ("/srv/clones/Atlas", "clones/Atlas"), ("/srv/clones/Atlas.git/", "clones/Atlas"), - # What v1.0.1's own regex records for it, `Clones/ATLAS`. + # The record in another case than the path, as a person may retype it, + # and as v1.0.1's own regex writes it, `Clones/ATLAS`. + ("/srv/Clones/ATLAS.git", "clones/Atlas"), ("/srv/Clones/ATLAS.git", "Clones/ATLAS"), ("C:\\srv\\clones\\Atlas.git", "clones/Atlas"), ("\\\\server\\clones\\Atlas.git", "clones/Atlas"), @@ -368,8 +370,8 @@ def test_the_sources_own_origin_is_asked_before_the_plans_record(tmp_path): ("/srv/clones/Atlas.git.git", "clones/Atlas.git"), ("/srv/a:b/Atlas.git", "b/Atlas"), ], ids=["dot-git", "no-dot-git", "trailing-slash", "another-case", - "backslashes", "share", "file-url", "relative", "two-dot-gits", - "colon"]) + "v1-0-1-case", "backslashes", "share", "file-url", "relative", + "two-dot-gits", "colon"]) def test_a_plan_that_v1_0_x_wrote_for_a_path_origin_is_still_a_correct_plan( adopter, tmp_path, monkeypatch, origin, record): """v1.0.0 and v1.0.1 read a path `origin` as its last two directories