diff --git a/scaffold-project.py b/scaffold-project.py index 380c4034..0da39754 100755 --- a/scaffold-project.py +++ b/scaffold-project.py @@ -86,7 +86,8 @@ from repo_shape import ( # noqa: E402 COMMIT_RE, NEUTRAL_PRODUCT_OWNER, PROJECT_ID_RE, TREE_DIGEST_DEFINITION, VISIBILITY_CHOICES, NamingPolicy, Refusal, accepts_role, checked_value, - git_out, link_pins_from_trees, repo_basename, shell_line, tree_digest, + git_out, link_pins_from_trees, repo_basename, shell_line, shown_path, + tree_digest, ) from shape_materialize import ( # noqa: E402 RULESET_HINT, SHAPE_REPOSITORY, CommandFailed, @@ -552,6 +553,11 @@ def _reusable_remotes(names: dict, urls: dict, repositories: dict, always was, of `remote_is_empty` or `local_remote_is_empty`; the seed is asked only of a leg that is not empty, and a leg git cannot read is refused as one with commits, saying so (#542). + + A LOCAL REMOTE IS NAMED AS `shown_path` NAMES A PATH (#766), as + `family.py init` names its holder's: it is a path under + `--local-remote-dir`, where `scaffold-unwritable-value` refuses a + control character first. A GitHub `/` is named as it is. """ reused: dict[str, str | None] = {} with tempfile.TemporaryDirectory(prefix="openreposhape-seed-") as scratch: @@ -573,7 +579,8 @@ def _reusable_remotes(names: dict, urls: dict, repositories: dict, continue raise Refusal( "scaffold-remote-exists", - f"{target} already exists" + ("" if local else " on GitHub") + f"{shown_path(target) if local else target} already exists" + + ("" if local else " on GitHub") + ("" if empty else " and has commits"), _exists_remedy(role, empty, reuse_flag, seed is not None, unread)) diff --git a/scripts/bump-leg.py b/scripts/bump-leg.py index 226e7b0d..f1aa7e28 100755 --- a/scripts/bump-leg.py +++ b/scripts/bump-leg.py @@ -121,7 +121,7 @@ sys.path.insert(0, str(SHAPE_ROOT / "scripts")) from repo_shape import ( # noqa: E402 COMMIT_RE, SAFE_PATH_RE, Refusal, checked_value, git_out, load_yaml, - recorded_gitlink, repo_basename, spelled, tree_digest, + recorded_gitlink, repo_basename, shown_path, spelled, tree_digest, ) from shape_materialize import ( # noqa: E402 CommandFailed, captured_child_environment, check_program, commit_trailers, @@ -1155,11 +1155,13 @@ def _ensure_root_clean(root: Path) -> None: # `spelled`'s to show, and `strip()` took them off the last name and # the leading space off the first line's ` M`, which then read as # `M` (staged). git quotes a name with a control in it or a space at - # its end, so the last newline is all there is to take off. + # its end, so the last newline is all there is to take off. THE ROOT + # ABOVE THEM is named as `shown_path` names a path (#766): its name + # held a C1 control and a bidi override raw on the first line. shown = dirt.decode("utf-8", "backslashreplace").rstrip("\n") raise Refusal( "bump-leg-root-dirty", - f"{root} has uncommitted changes:\n" + f"{shown_path(root)} has uncommitted changes:\n" + "\n".join(f" {spelled(line)}" for line in shown.split("\n")), "Remediation: commit or stash them first. A lockstep commit that " "cannot be read as exactly one change is the thing the rule " @@ -1217,21 +1219,23 @@ def _branch_to_bump_from(root: Path, project: "Project", role: str, the branch the real run needs (#477); a root on no branch at all is refused either way. Both refusals name that branch's `switch -c` through `_switch_command`, quoted (#532), and end with `BRANCH_LEFT`, the - sentence for a root that has that branch already (#647). + sentence for a root that has that branch already (#647). Each detail + names the root as `shown_path` names a path (#766), as the dirty root's + does. """ branch = current_branch(root) if branch is None: raise Refusal( "bump-leg-root-detached", - f"{root} is not on a branch, so there is no branch to push and " - "open a pull request from", + f"{shown_path(root)} is not on a branch, so there is no branch " + "to push and open a pull request from", f"Remediation: `{_switch_command(root, role, commit)}` and " f"re-run.\n{BRANCH_LEFT}") if branch == project.tracking and not dry_run: raise Refusal( "bump-leg-on-tracking-branch", - f"{root} is on {branch!r}, which {MANIFEST} declares as this " - "project's tracking branch", + f"{shown_path(root)} is on {branch!r}, which {MANIFEST} declares " + "as this project's tracking branch", "Remediation: these organisations are pull-request only, so this " "will not make a commit whose only next step is a push to the " "default branch. Run:\n " @@ -1796,9 +1800,10 @@ def cmd_bump(args) -> int: # LAST, after everything a dry run on a branch prints, so that # line and the sentence under it are the whole of the difference # between the two (#477, #647). The command is quoted, so a root - # under `My Projects/` pastes. - print(f"\nthe real run needs a branch: {root} is on {branch!r}, " - "the tracking branch. Create one first with: " + # under `My Projects/` pastes; the root before it is named as + # `shown_path` names a path, as the refusals name it (#766). + print(f"\nthe real run needs a branch: {shown_path(root)} is on " + f"{branch!r}, the tracking branch. Create one first with: " + _switch_command(root, role, asked)) print(BRANCH_LEFT) return 0 diff --git a/scripts/family.py b/scripts/family.py index faa7ba4d..5e9c3f8d 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -832,7 +832,14 @@ def _checked_branch(family: "Family", verb: str, suggested: str) -> str: (#487). Asked by the real run, after the refusals about the arguments and the holder and before the member is read or anything is written; `--dry-run` asks `_branch_said` instead. The remedy's `switch -c` is - followed by `BRANCH_LEFT`, for a holder that has the branch (#607).""" + followed by `BRANCH_LEFT`, for a holder that has the branch (#607). + + THE HOLDER IS NAMED AS `shown_path` NAMES A PATH (#766), as #692 names + the folders `init` makes: a holder whose path held an ESC sent it to the + terminal, and one with a line break began a line of its own choosing. + A printable path is named byte for byte. The remedy's `switch -c` is + a command to paste, and still holds the holder's path raw inside its + shell quotes (#459).""" branch = _holder_branch(family.root) if branch is not None and branch != family.tracking(): return branch @@ -845,16 +852,17 @@ def _checked_branch(family: "Family", verb: str, suggested: str) -> str: if branch is None: raise Refusal( "family-holder-detached", - f"{family.root} is not on a branch, so `{verb}` would write a " - "commit no branch holds, with nothing to push and open a pull " - "request from. No git command that writes was run and nothing " - "was written.", remedy) + f"{shown_path(family.root)} is not on a branch, so `{verb}` " + "would write a commit no branch holds, with nothing to push and " + "open a pull request from. No git command that writes was run " + "and nothing was written.", remedy) raise Refusal( "family-on-tracking-branch", - f"{family.root} is on {branch!r}, which {MANIFEST} declares as the " - f"holder's tracking branch, and `{verb}` will not write a commit " - "whose only next step is a push to the default branch. No git " - "command that writes was run and nothing was written.", remedy) + f"{shown_path(family.root)} is on {branch!r}, which {MANIFEST} " + "declares as the holder's tracking branch, and " + f"`{verb}` will not write a commit whose only next step is a push to " + "the default branch. No git command that writes was run and nothing " + "was written.", remedy) def _branch_said(family: "Family", verb: str, suggested: str) -> list[str]: @@ -1894,6 +1902,12 @@ def _refuse_landing_in_standard(into: Path, holder: Path, checkout the relative path that reaches its parent from there, so the printed flag lands the family beside the checkout wherever the person stands. Asked before anything is created, under `--dry-run` alike. + + THE THREE PATHS ARE NAMED AS `shown_path` NAMES A PATH (#766): the + holder, the checkout and where `--into ..` lands, each under the + directory the person stands in, whose name can hold an ESC or a line + break. `--into {parent}` is `..` and `..`s, or on Windows a path no + control character can be in, and is named as it is. """ checkout = _standard_checkout(into) if checkout is None: @@ -1906,15 +1920,16 @@ def _refuse_landing_in_standard(into: Path, holder: Path, raise Refusal( "family-landing-in-standard", f"with neither --into nor --work-dir the holder lands where you are " - f"standing, at {holder}, which is inside {checkout}, a checkout of " - "openRepoShape itself (it holds scripts/family.py and " - "templates/family-root/). The family would be part of the " - "standard's tree, and `make siblings` would clone every member into " - "it too. Nothing was created, and `--dry-run` refuses the same.", + f"standing, at {shown_path(holder)}, which is inside " + f"{shown_path(checkout)}, a checkout of openRepoShape itself (it " + "holds scripts/family.py and templates/family-root/). The family " + "would be part of the standard's tree, and `make siblings` would " + "clone every member into it too. Nothing was created, and " + "`--dry-run` refuses the same.", f"Remediation: pass `--into {parent}` to land it beside this " - f"checkout, at {beside / family / family}, or `--into ` for any " - "other parent directory; `--work-dir ` is the override " - "(`/`, no family folder). There is no --force.") + f"checkout, at {shown_path(beside / family / family)}, or `--into " + "` for any other parent directory; `--work-dir ` is the " + "override (`/`, no family folder). There is no --force.") def _landing(args, family: str) -> tuple[Path | None, Path]: @@ -1948,12 +1963,15 @@ def _landing(args, family: str) -> tuple[Path | None, Path]: # `os.path.isfile`, which answers False for a holder this user cannot # search: `Path.is_file()` raised `PermissionError` there, a traceback # before the question about the holder (`_refuse_a_taken_holder`) that - # names it (#609's rule: a refusal by name, never a traceback). + # names it (#609's rule: a refusal by name, never a traceback). The two + # paths are named as `shown_path` names a path (#766): `--into`, or the + # directory the person stands in, can hold an ESC or a line break. if into.name == family and os.path.isfile(into / family / MANIFEST): raise Refusal( "family-holder-already-here", - f"{into} is already the {family} family folder and its holder " - f"is at {into / family}, so there is nothing to init", + f"{shown_path(into)} is already the {family} family folder and " + f"its holder is at {shown_path(into / family)}, so there is " + "nothing to init", f"Remediation: `{_add_command(into / family, args.org)}` grows " "it, and `make siblings` in the holder puts each member's " "working clone beside it. There is no --force.") @@ -2504,6 +2522,13 @@ def _print_init_plan(values: dict, repository: str, url: str, and what a real run shows before it does anything. Split from `cmd_init` for #136. + + THE FOLDER AND THE HOLDER ARE NAMED AS `shown_path` NAMES A PATH (#766). + Both lie under `--into`, `--work-dir` or the directory the person + stands in, and the plan is on stdout before any refusal, so a name with + a line break and then `WARNING forged-line: pwned` printed that line as + a line of the plan, on a dry run and on a run refused after it, and an + ESC reached the terminal. A printable path is named byte for byte. """ print(f"family {values['FAMILY_NAME']} ({values['FAMILY_ID']})") print(f"shape {SHAPE_REPOSITORY} @ {shape_commit[:12]} " @@ -2511,10 +2536,13 @@ def _print_init_plan(values: dict, repository: str, url: str, print(f"created by {values['CREATED_BY']} on {values['CREATED_ON']}") print(f" holder {repository:<28} -> {url}") if folder is None: - print(f"landing {holder} (--work-dir: no family folder)") + print(f"landing {shown_path(holder)} (--work-dir: no family " + "folder)") else: - print(f"folder {folder} a plain folder, NOT a repository") - print(f"landing {holder} the holder clone, inside it") + print(f"folder {shown_path(folder)} a plain folder, NOT a " + "repository") + print(f"landing {shown_path(holder)} the holder clone, inside " + "it") print(f"members mounted under {MEMBERS_DIR}/; none yet — " "`family.py add` puts one there") print("remotes " + ("a bare repository on disk (no network)" if local @@ -2610,6 +2638,12 @@ def _checked_reuse(reuse_empty_repo: bool, local: bool, url: str, Refused when the remote already exists WITH commits, or exists empty without `--reuse-empty-repo`. Split from `cmd_init` for #136. + + A LOCAL REMOTE IS NAMED AS `shown_path` NAMES A PATH (#766): it is a + path under `--local-remote-dir`. `_refuse_an_unwritable_clone_url` + refuses a control character there first, so only a character that is + not printable and not refused (a zero-width space) reaches this from + the command line. A GitHub `/` is named as it is. """ # THE ONE REPOSITORY THAT MAY ALREADY EXIST is the holder, and only with # --reuse-empty-repo, and only with ZERO commits. `InkRouter` in the @@ -2622,7 +2656,8 @@ def _checked_reuse(reuse_empty_repo: bool, local: bool, url: str, if not (reuse_empty_repo and empty): raise Refusal( "family-remote-exists", - f"{target} already exists" + ("" if local else " on GitHub") + f"{shown_path(target) if local else target} already exists" + + ("" if local else " on GitHub") + ("" if empty else " and has commits"), ("Remediation: it has ZERO commits, so it is a reserved name " "rather than a live repository — re-run with " @@ -7765,6 +7800,11 @@ def _checked_index_unlocked(root: Path, path: str) -> None: `.git/index.lock`. A lock taken after this check is the rollback's to meet. The dry run does not ask: it writes nothing, and a lock is a fact about the moment. + + THE DETAIL NAMES THE LOCK AS `shown_path` NAMES A PATH (#766): it lies + under the holder, whose path can hold an ESC or a line break. The + remedy's `rm -f` is quoted for a shell, as it was, and still holds + the lock's path raw inside those quotes (#459). """ shown = _git_bytes(["rev-parse", "--git-path", "index.lock"], root).decode("utf-8", "surrogateescape").strip() @@ -7773,11 +7813,11 @@ def _checked_index_unlocked(root: Path, path: str) -> None: return raise Refusal( "family-index-locked", - f"the holder's index is locked: {lock.as_posix()} exists. `git " - f"submodule deinit` takes no such lock, so `remove` would empty " - f"{path} and drop its registration and then fail at `git rm`, its " - "first write to the index; it was refused before either, and " - "nothing was written.", + f"the holder's index is locked: {shown_path(lock.as_posix())} " + "exists. `git submodule deinit` takes no such lock, so `remove` " + f"would empty {path} and drop its registration and then fail at " + "`git rm`, its first write to the index; it was refused before " + "either, and nothing was written.", "Remediation: another git command is running in the holder, or one " "that stopped left the lock behind. Let it finish and run the same " "command again; if no git command is running, delete the lock (`rm " diff --git a/tests/conftest.py b/tests/conftest.py index ea685927..df1e4fde 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2204,3 +2204,97 @@ def assert_step_four_speaks_in_the_plans_words(run, tmp_path: Path, f"({FIRST_LINK_WORDS}). The recorded chain still classifies." ), offered assert not (tmp_path / "remotes").exists() + + +# --- a path a line names, spelled safe to show (#692, #766) ------------------ +# +# A path a person supplied, or one git keeps, can hold an ESC, which a +# terminal acts on, or a line break and then a line that is not the tool's. +# A line that names such a path names it as `repo_shape.shown_path` does: as +# it is, unless a character in it is not printable, and then as Python's +# `repr` spells it. #692 gave the folder refusals that spelling and #766 the +# plan rows and refusals beside them; their tests hand the tools these names +# and read what comes back with these helpers. + +#: Folder names that are not safe to show raw: an ESC, and a line break then +#: a line that is not the tool's. +ESC_NAME = "wd-\x1b[31mRED" +FORGED_NAME = "wd-\nWARNING forged-line: pwned" + +#: A command line and a file name on Windows hold none of these. +POSIX_NAMES = pytest.mark.skipif( + os.name == "nt", + reason="needs a folder name holding a control character, which a " + "Windows file name cannot hold") + +#: The two names, for `parametrize`, by the ids #692's rows use. +UNSAFE_NAMES = [pytest.param(ESC_NAME, id="ESC", marks=POSIX_NAMES), + pytest.param(FORGED_NAME, id="line-break", marks=POSIX_NAMES)] + +#: A name a line names as it is, byte for byte, with no quotes: a space and +#: a letter that is not ASCII are each a character `str.isprintable` +#: accepts. +PRINTABLE_NAME = "wd \u00e9 and a space" + +#: `UNSAFE_NAMES` and `PRINTABLE_NAME`, for a row that holds both kinds +#: (rv766's SHOULD 1: a quoted printable path was caught by no row). +SHOWN_NAMES = [*UNSAFE_NAMES, pytest.param(PRINTABLE_NAME, id="printable")] + + +def spelt(path) -> str: + """A path that holds a control character as a line names it: as + Python's `repr` spells it, quotes and `\\x1b` and `\\n` and all. It is + `repr`, and not `shown_path`, so a change to that function moves no + expectation with it.""" + return repr(str(path)) + + +def named_as(path, name: str) -> str: + """`path`, made under the folder `name`, as a line names it: as it is, + byte for byte, under `PRINTABLE_NAME`, and `spelt` under one of + `UNSAFE_NAMES`.""" + return str(path) if name == PRINTABLE_NAME else spelt(path) + + +def unsafe_folder(parent: Path, name: str) -> Path: + """`parent / name`, made with every folder above it, or the test skipped + where the disk refuses the name.""" + folder = parent / name + try: + folder.mkdir(parents=True, exist_ok=True) + except (OSError, UnicodeError) as exc: + pytest.skip(f"this filesystem cannot hold the name: {exc}") + return folder + + +def copied_under(source: Path, parent: Path, name: str) -> Path: + """A private copy of the repository `source`, under its own name, in a + folder `name` made under `parent` (`unsafe_folder`): a holder or an + assembly root whose path holds that name. A copy keeps a submodule's + `.git` file, a relative path, true.""" + target = unsafe_folder(parent, name) / source.name + shutil.copytree(source, target, symlinks=True) + return target.resolve() + + +def assert_shown_safe(*texts: str) -> None: + """No ESC in any of `texts`, and no line of them that starts `WARNING + forged-line`: no path reached the terminal raw.""" + for text in texts: + assert "\x1b" not in text, ascii(text) + assert not any(line.startswith("WARNING forged-line") + for line in text.splitlines()), text + + +def refusal_detail(stderr: str, code: str) -> list[str]: + """The lines refusal `code` prints before its `Remediation:` line: the + `REFUSED` line, and any line its detail goes on to, such as a list of + file names. A path that broke the detail in two adds a line here.""" + lines = stderr.splitlines() + starts = [at for at, line in enumerate(lines) + if line.startswith(f"REFUSED {code}: ")] + assert len(starts) == 1, stderr + ends = [at for at in range(starts[0], len(lines)) + if lines[at].startswith("Remediation:")] + assert ends, stderr + return lines[starts[0]:ends[0]] diff --git a/tests/test_bump_leg_branch_and_push_line.py b/tests/test_bump_leg_branch_and_push_line.py index 11dc4bdd..52e35ac3 100644 --- a/tests/test_bump_leg_branch_and_push_line.py +++ b/tests/test_bump_leg_branch_and_push_line.py @@ -32,6 +32,11 @@ they already refuse it exits 0 and the real run ends `bump-leg-validators-red`. Its closing paragraph says so. +THE ROOT ABOVE THEM (#766). The three refusals and the dry run's `the real +run needs a branch` line printed the root as it was, so a root under a name +with an ESC, a line break or a bidi override sent it to the terminal raw. +Each names it as `repo_shape.shown_path` does now. + NO NETWORK, NO `gh`: the bare remotes are the session's own, and every root here pushes to a private copy of its remote (`make_root`), so a push reaches nothing another test reads. Every printed line is run as the argv a POSIX @@ -48,6 +53,10 @@ import pytest from conftest import FILE_PROTOCOL, PROJECT, REPO, cloned_just_pushed, git +#: The names no line may print raw, and how a line spells them (#766). +from conftest import (POSIX_NAMES, SHOWN_NAMES, UNSAFE_NAMES, + assert_shown_safe, copied_under, named_as, + refusal_detail, spelt) # A fixture is used by being named in this module's namespace, which is what # the imports of `bump_leg_module` and `make_root` are for. from test_bump_leg import ( # noqa: F401 @@ -518,3 +527,82 @@ def test_a_dry_run_says_the_validators_are_the_real_runs(project, scaffolded, assert "REFUSED bump-leg-validators-red" in real.stderr, real.stderr assert head_of(project) == head assert git("status", "--porcelain", cwd=project).stdout == "" + + +# --- #766: the root, named safe to show ------------------------------------- + +#: The names a root is put under: an ESC, a line break then a forged line, +#: and rv672's measured name, a bidi override and a C1 control. +ROOT_NAMES = [*UNSAFE_NAMES, + pytest.param("r\u202eo\u009bot", id="bidi-and-C1", + marks=POSIX_NAMES)] + + +@pytest.mark.parametrize("extra", [(), ("--dry-run",)], + ids=["bump", "dry-run"]) +@pytest.mark.parametrize("name", ROOT_NAMES) +def test_a_dirty_root_is_named_safe_to_show(scaffolded, advanced, tmp_path, + name, extra): + """#766's third line, measured by rv672 on #672's head: under a root + named `root`, `bump-leg-root-dirty` put both characters + raw into its first line, above the file names #672 spells. The root is + named as `repr` spells it now, by the real run and the dry run alike, + and nothing on either stream holds the name raw.""" + root = branched(copied_under(scaffolded["clone"], tmp_path, name)) + readme = root / "README.md" + readme.write_bytes(readme.read_bytes() + b"dirty\n") + result = bump(root, scaffolded, advanced, *extra) + assert result.returncode == 2, result.stderr + result.stdout + assert refusal_detail(result.stderr, "bump-leg-root-dirty") == [ + f"REFUSED bump-leg-root-dirty: {spelt(root)} has uncommitted " + "changes:", " M README.md"] + lines = (result.stdout + result.stderr).splitlines() + assert [raw_controls(name, line) for line in lines + if raw_controls(name, line)] == [], ascii(result.stderr) + assert_shown_safe(result.stdout, result.stderr) + + +@pytest.mark.parametrize("state", ["tracking-branch", "detached"]) +@pytest.mark.parametrize("name", SHOWN_NAMES) +def test_a_root_refused_for_its_branch_is_named_safe_to_show( + scaffolded, advanced, tmp_path, name, state): + """#766: `bump-leg-on-tracking-branch` and `bump-leg-root-detached` + named the root as it was. The detail names it as `repr` spells it, on + the one line it is, and a printable root as it is, byte for byte, with + no quotes (rv766's P14). The remedy's `switch -c` is a command to + paste, quoted for a shell as it was (`_switch_command`), and is not + this row's.""" + root = copied_under(scaffolded["clone"], tmp_path, name) + if state == "detached": + git("checkout", "-q", "--detach", cwd=root) + result = bump(root, scaffolded, advanced) + assert result.returncode == 2, result.stderr + result.stdout + assert result.stdout == "" + code, said = ( + ("bump-leg-root-detached", + "is not on a branch, so there is no branch to push and open a pull " + "request from") + if state == "detached" else + ("bump-leg-on-tracking-branch", + "is on 'main', which project.yaml declares as this project's " + "tracking branch")) + assert refusal_detail(result.stderr, code) == [ + f"REFUSED {code}: {named_as(root, name)} {said}"] + + +@pytest.mark.parametrize("name", UNSAFE_NAMES) +def test_the_dry_runs_branch_line_names_the_root_safe_to_show( + scaffolded, advanced, tmp_path, name): + """#766: on the tracking branch the dry run ends `the real run needs a + branch: is on 'main'`, and named the root as it was. It names it + as `repr` spells it. The `switch -c` after it is a command to paste, + quoted for a shell as it was, and is not this row's.""" + root = copied_under(scaffolded["clone"], tmp_path, name) + result = bump(root, scaffolded, advanced, "--dry-run") + assert result.returncode == 0, result.stderr + result.stdout + said = [line for line in result.stdout.splitlines() + if line.startswith("the real run needs a branch: ")] + assert len(said) == 1, result.stdout + assert said[0].startswith( + f"the real run needs a branch: {spelt(root)} is on 'main', the " + "tracking branch. Create one first with: git -C "), said diff --git a/tests/test_dry_run_asks_remotes.py b/tests/test_dry_run_asks_remotes.py index 31c87771..52498b84 100644 --- a/tests/test_dry_run_asks_remotes.py +++ b/tests/test_dry_run_asks_remotes.py @@ -37,6 +37,9 @@ import pytest from conftest import ORG, REPO, SCAFFOLD, WINDOWS_SKIP, git, run_script +#: The names no line may print raw, and how a line spells them (#766). +from conftest import (UNSAFE_NAMES, assert_shown_safe, refusal_detail, + spelt, unsafe_folder) from test_scaffold_reuse_seeded_legs import (REPOS, bare, commits, refused_at_the_root, tip) from test_ruleset_hint_tracking_branch import template # noqa: F401 @@ -482,3 +485,51 @@ def test_a_real_run_with_no_gh_is_refused_in_the_dry_runs_words( assert refusal in captured.err, captured.err assert "put `gh` on PATH, logged in" in captured.err assert asked == [first] + + +# --- #766: a local remote that exists, named safe to show ------------------- + +def remote_exists_in_process(tool: str, remote: Path): + """The refusal the tool's own question raises for `remote`, a local + remote with commits, asked in process: `family.py`'s `_checked_reuse` + for its holder, or the scaffold's `_reusable_remotes` for its root.""" + if tool == "family": + module = load("family_766", FAMILY) + with pytest.raises(module.Refusal) as refused: + module._checked_reuse(False, True, str(remote), f"{ORG}/{HOLDER}") + else: + module = load("scaffold_766", SCAFFOLD) + role = "assembly" + with pytest.raises(module.Refusal) as refused: + module._reusable_remotes( + {role: PROJECT}, {role: str(remote)}, + {role: f"{ORG}/{PROJECT}"}, True, False, {}, "main") + return refused.value + + +@pytest.mark.parametrize("name", UNSAFE_NAMES) +@pytest.mark.parametrize("tool", ["family", "scaffold"]) +def test_a_local_remote_that_exists_is_named_safe_to_show(tmp_path, tool, + name): + """#766: `family-remote-exists` and `scaffold-remote-exists` named a + local remote as it was, a path under `--local-remote-dir`. From the + command line a name with an ESC or a line break there never reaches + them: the clone URL is refused first, as `-unwritable-value`, + which names it as `repr` spells it, and nothing on either stream holds + the name raw. So each question is asked in process too, as #692 asks + `make_folder`, and names the remote as `repr` spells it.""" + remotes = unsafe_folder(tmp_path, name).resolve() + remote = live_bare( + remotes / f"{HOLDER if tool == 'family' else PROJECT}.git") + run = family_init if tool == "family" else scaffold + result = run(tmp_path, "--local-remote-dir", str(remotes), local=False) + assert result.returncode == 2, result.stdout + result.stderr + refused = refusal_detail(result.stderr, f"{tool}-unwritable-value") + assert f"own path, {spelt(remote)}, which " in refused[0], refused + assert_shown_safe(result.stdout, result.stderr) + + refusal = remote_exists_in_process(tool, remote) + assert refusal.code == f"{tool}-remote-exists" + assert refusal.detail == ( + f"{spelt(remote)} already exists and has commits") + assert_shown_safe(refusal.detail, refusal.remediation) diff --git a/tests/test_dry_run_parity.py b/tests/test_dry_run_parity.py index 89cad33a..46281ef9 100644 --- a/tests/test_dry_run_parity.py +++ b/tests/test_dry_run_parity.py @@ -59,6 +59,9 @@ import pytest from conftest import ORG, REPO, SCAFFOLD, WINDOWS_SKIP, run_script +#: The names #692's rows hold, and how a line spells them, kept beside the +#: helpers #766's rows in other modules share (#766). +from conftest import ESC_NAME, FORGED_NAME, POSIX_NAMES, spelt from test_dry_run_asks_remotes import (DRY_RUN_DONE, live_bare, load, no_gh_run, scratch_env, without_gh) @@ -72,17 +75,6 @@ SCAFFOLD_FOLDER = "REFUSED scaffold-folder-cannot-be-made" FAMILY_FOLDER = "REFUSED family-folder-cannot-be-made" -#: A command line and a file name on Windows hold none of these. -POSIX_NAMES = pytest.mark.skipif( - os.name == "nt", - reason="needs a folder name holding a control character, which a " - "Windows file name cannot hold") - -#: Folder names that are not safe to show raw (#692): an ESC, which a -#: terminal acts on, and a line break then a line that is not the tool's. -ESC_NAME = "wd-\x1b[31mRED" -FORGED_NAME = "wd-\nWARNING forged-line: pwned" - def touch(path: Path) -> None: """A file at `path`, somebody's, made with every folder above it.""" @@ -205,14 +197,6 @@ def at(tmp_path: Path, relative: str) -> Path: return (tmp_path / relative).resolve() -def spelt(path: Path) -> str: - """A path that holds a control character as a refusal names it (#692): - as Python's `repr` spells it, quotes and `\\x1b` and `\\n` and all. It - is `repr`, and not `shown_source`, so a change to that function moves no - expectation with it.""" - return repr(str(path)) - - def case_params(cases: dict) -> list: """One `pytest.param` per row of `cases`, a row named `ESC-...` or `line-break-...` skipped where no file can be named so (`POSIX_NAMES`).""" diff --git a/tests/test_family_bump_remove_rollback.py b/tests/test_family_bump_remove_rollback.py index 63838823..e71819e2 100644 --- a/tests/test_family_bump_remove_rollback.py +++ b/tests/test_family_bump_remove_rollback.py @@ -78,6 +78,8 @@ BUMP_ORDER, commit_in_mount, isolated, registered_keys, state, ) from test_family_member_path import MEMBER, OTHER, older, raw +#: The names no line may print raw, and how a line spells them (#766). +from conftest import UNSAFE_NAMES, copied_under, refusal_detail, spelt PATH = f"members/{MEMBER}" @@ -426,6 +428,30 @@ def test_a_remove_dry_run_under_a_held_index_lock_is_unchanged( "changed") +@pytest.mark.parametrize("name", UNSAFE_NAMES) +def test_a_held_index_lock_is_named_safe_to_show(family, offline, tmp_path, + name): + """#766: the detail named the lock as it was, under the holder, so a + holder under a name with an ESC sent it to the terminal, and one with a + line break printed a forged line in the detail. It names the lock as + `repr` spells it, on the one line it is. The remedy's `rm -f` is a + command to paste, quoted for a shell as it was, and is not this row's.""" + holder = copied_under(family["root"], tmp_path, name) + env = isolated(offline, tmp_path) + lock = index_lock(holder) + lock.write_bytes(b"") + result = remove(holder, env) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert lock.is_file(), "the lock is left where it was" + assert refusal_detail(result.stderr, "family-index-locked") == [ + "REFUSED family-index-locked: the holder's index is locked: " + f"{spelt(lock.resolve().as_posix())} exists. `git submodule deinit` " + f"takes no such lock, so `remove` would empty {PATH} and drop its " + "registration and then fail at `git rm`, its first write to the " + "index; it was refused before either, and nothing was written."] + + # --- refusals a dry run asks first are asked first (#365) ----------------- def comment_on_members(root: Path, env: dict) -> None: diff --git a/tests/test_family_init_landing.py b/tests/test_family_init_landing.py index 73c2c6be..8656d35b 100644 --- a/tests/test_family_init_landing.py +++ b/tests/test_family_init_landing.py @@ -22,6 +22,12 @@ under the test's own temporary directory (the two paths the detection reads, and nothing else), so a regression writes there and never into the repository under test. NO NETWORK: every remote is `--local-remote-dir`. + +THE PATHS ARE NAMED SAFE TO SHOW (#766). The plan's `folder` and `landing` +rows, `family-landing-in-standard` and `family-holder-already-here` name a +path under the directory the person gave or stands in, and printed it as it +was. Each names it as `repo_shape.shown_path` does now: as it is, or as +`repr` spells it where a character in it is not printable. """ from __future__ import annotations @@ -34,6 +40,10 @@ import pytest from conftest import REPO, run_script +#: The names no line may print raw, and how a line spells them (#766). +from conftest import (PRINTABLE_NAME, SHOWN_NAMES, UNSAFE_NAMES, + assert_shown_safe, named_as, refusal_detail, + spelt, unsafe_folder) sys.path.insert(0, str(REPO / "scripts")) from repo_shape import PYTHON # noqa: E402 @@ -196,3 +206,114 @@ def test_the_printed_next_runs_from_where_init_was_run(tmp_path, where): check=False) assert helped.returncode == 0, helped.stderr assert helped.stdout.startswith("usage: family.py add"), helped.stdout + + +# --- #766: the paths the plan and `_landing`'s refusals name ---------------- + +def planned_rows(stdout: str) -> list[str]: + """The plan's `folder` and `landing` rows, in the order printed.""" + return [line for line in stdout.splitlines() + if line.startswith(("folder ", "landing "))] + + +@pytest.mark.parametrize("name", UNSAFE_NAMES) +@pytest.mark.parametrize("flag", ["--work-dir", "--into"]) +def test_the_plan_names_the_folder_and_the_holder_safe_to_show( + tmp_path, flag, name): + """#766's first line, measured on #692's head: the plan printed the + family folder and the holder as they were, on stdout before any + refusal. A `--work-dir` or `--into` holding a line break and then + `WARNING forged-line: pwned` printed that line as a line of the plan, + on a dry run and on a run refused after the plan, and an ESC reached the + terminal. Each row names its path as `repr` spells it now, in both + runs, and nothing else on either stream holds the name raw.""" + place = unsafe_folder(tmp_path, name).resolve() + if flag == "--work-dir": + holder = place / NAME + rows = [f"landing {spelt(holder)} (--work-dir: no family " + "folder)"] + else: + holder = place / NAME / NAME + rows = [f"folder {spelt(place / NAME)} a plain folder, NOT a " + "repository", + f"landing {spelt(holder)} the holder clone, inside it"] + dry = init(tmp_path, tmp_path, flag, str(place), "--dry-run") + assert dry.returncode == 0, dry.stdout + dry.stderr + # A file where the holder goes: the real run prints the same plan, and + # is refused after it (`family-target-exists`, #692's spelling). + holder.parent.mkdir(parents=True, exist_ok=True) + holder.write_bytes(b"somebody's\n") + real = init(tmp_path, tmp_path, flag, str(place)) + assert real.returncode == 2, real.stdout + real.stderr + assert refusal_detail(real.stderr, "family-target-exists") == [ + f"REFUSED family-target-exists: {spelt(holder)} already exists and " + "is a file, not a folder"] + for result in (dry, real): + assert planned_rows(result.stdout) == rows, result.stdout + assert_shown_safe(result.stdout, result.stderr) + + +def test_the_plan_names_a_printable_folder_byte_for_byte(tmp_path): + """#766 changes no row a person could read before: a space and a letter + that is not ASCII are printable, so the folder and the holder are named + as they are, with no quotes.""" + place = tmp_path / PRINTABLE_NAME + place.mkdir() + place = place.resolve() + result = init(tmp_path, tmp_path, "--into", str(place), "--dry-run") + assert result.returncode == 0, result.stdout + result.stderr + assert planned_rows(result.stdout) == [ + f"folder {place / NAME} a plain folder, NOT a repository", + f"landing {place / NAME / NAME} the holder clone, inside it"] + + +@pytest.mark.parametrize("name", UNSAFE_NAMES) +def test_landing_in_a_checkout_names_its_three_paths_safe_to_show( + tmp_path, name): + """#766: `family-landing-in-standard` named the holder, the checkout and + where `--into ..` lands as they were, each under the directory the + person stands in, so a checkout under a name with a line break printed + a forged line in the detail and in the remedy. Each is named as `repr` + spells it, by the dry run and the real run alike, and the whole refusal + is safe to show: `--into ..` holds no name.""" + checkout = fake_checkout(unsafe_folder(tmp_path, name) / "std").resolve() + for extra in (("--dry-run",), ()): + result = init(checkout, tmp_path, *extra) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert refusal_detail(result.stderr, CODE) == [ + f"REFUSED {CODE}: with neither --into nor --work-dir the holder " + "lands where you are standing, at " + f"{spelt(checkout / NAME / NAME)}, which is inside " + f"{spelt(checkout)}, a checkout of openRepoShape itself (it " + "holds scripts/family.py and templates/family-root/). " + "The family would be part of the standard's tree, and `make " + "siblings` would clone every member into it too. Nothing was " + "created, and `--dry-run` refuses the same."] + assert ("Remediation: pass `--into ..` to land it beside this " + f"checkout, at {spelt(checkout.parent / NAME / NAME)}, or " + "`--into `" in result.stderr), result.stderr + assert_shown_safe(result.stdout, result.stderr) + assert sorted(path.name for path in checkout.iterdir()) == [ + "scripts", "templates"] + + +@pytest.mark.parametrize("name", SHOWN_NAMES) +def test_a_family_folder_already_here_is_named_safe_to_show(tmp_path, name): + """#766: `family-holder-already-here` named `--into` and the holder in + it as they were. The detail names both as `repr` spells them, on the + one line it is, and a printable path as it is, byte for byte, with no + quotes (rv766's P8 and P9). The remedy's `add` line is a command to + paste, quoted for a shell as it was (`_add_command`), and is not this + row's.""" + into = unsafe_folder(tmp_path, name).resolve() / NAME + (into / NAME).mkdir(parents=True) + (into / NAME / "family.yaml").write_bytes(b"name: Orbit\n") + result = init(tmp_path, tmp_path, "--into", str(into), "--dry-run") + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + assert refusal_detail(result.stderr, "family-holder-already-here") == [ + "REFUSED family-holder-already-here: " + f"{named_as(into, name)} is already the {NAME} family folder and " + f"its holder is at {named_as(into / NAME, name)}, so there is " + "nothing to init"] diff --git a/tests/test_family_tracking_branch.py b/tests/test_family_tracking_branch.py index 85b62ee1..54f543f4 100644 --- a/tests/test_family_tracking_branch.py +++ b/tests/test_family_tracking_branch.py @@ -39,6 +39,8 @@ import pytest from conftest import REPO, git, run_script +#: The names no line may print raw, and how a line spells them (#766). +from conftest import UNSAFE_NAMES, copied_under, refusal_detail, spelt #: The family fixture, its names and the work branch it grows the holder on, #: imported from the module that owns them. from test_family import ( # noqa: F401 @@ -330,3 +332,37 @@ def test_each_doc_makes_the_branch_before_the_add(doc): add = text.index("python3 scripts/family.py add ") block = text[text.rindex("```", 0, add):add] assert " switch -c family/add-" in block, block + + +# --- #766: the holder each refusal names, safe to show ----------------------- + +@pytest.mark.parametrize("name", UNSAFE_NAMES) +@pytest.mark.parametrize("detached", [False, True], + ids=["tracking-branch", "detached"]) +def test_a_holder_refused_for_its_branch_is_named_safe_to_show( + family, tmp_path, detached, name): + """#766: `family-on-tracking-branch` and `family-holder-detached` named + the holder as it was, so a holder under a name with an ESC sent it to + the terminal, and one with a line break printed a forged line in the + detail. The detail names it as `repr` spells it, on the one line it is. + The remedy's `switch -c` is a command to paste, quoted for a shell as + it was (`_switch_command`), and is not this row's.""" + holder = copied_under(family["root"], tmp_path, name) + git("switch", "-q", "-C", TRACKING, cwd=holder) + if detached: + git("checkout", "-q", "--detach", cwd=holder) + result = act(family, holder, "remove") + assert result.returncode == 2, result.stdout + result.stderr + assert result.stdout == "" + code, said = ( + ("family-holder-detached", + "is not on a branch, so `remove` would write a commit no branch " + "holds, with nothing to push and open a pull request from") + if detached else + ("family-on-tracking-branch", + f"is on '{TRACKING}', which family.yaml declares as the holder's " + "tracking branch, and `remove` will not write a commit whose only " + "next step is a push to the default branch")) + assert refusal_detail(result.stderr, code) == [ + f"REFUSED {code}: {spelt(holder)} {said}. No git command that writes " + "was run and nothing was written."]