diff --git a/scaffold-project.py b/scaffold-project.py index 8f9c8f8e..df977fd5 100755 --- a/scaffold-project.py +++ b/scaffold-project.py @@ -131,7 +131,7 @@ # not-found answer as the one failure of `gh repo view` that means absent. from shape_materialize import ( # noqa: E402 make_folder, refuse_unless_not_found, refuse_unmakeable_folder, - target_in_the_way, target_remediation, + shown_source, target_in_the_way, target_remediation, ) # How the READMEs' `git clone` line spells the clone URL, one word (#693), # in the quoting `shell_line` gives the NEXT STEPS clone (#486). @@ -1322,7 +1322,7 @@ def _refuse_existing_trees(work_root: Path, names: dict) -> None: if why: raise Refusal( "scaffold-target-exists", - f"{target} already exists and {why}", + f"{shown_source(target)} already exists and {why}", target_remediation( why, "Remediation: choose an empty --work-dir. There is no " diff --git a/scripts/family.py b/scripts/family.py index e4698d3a..2ce4486e 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -176,8 +176,8 @@ commit_trailers, env_commit, escape_what_stdout_cannot_encode, force_stage, inherited_child_environment, make_folder, materialize_family_root, refuse_bad_branch, refuse_unless_not_found, - refuse_unmakeable_folder, run, stage_written, target_in_the_way, - target_remediation, trailer_line, write_lf, + refuse_unmakeable_folder, run, shown_source, stage_written, + target_in_the_way, target_remediation, trailer_line, write_lf, ) # What `init` asks of the holder's own reader and validator before it creates # anything (#307), imported as its own unit and not added to the lists above. @@ -1021,6 +1021,13 @@ def _checked_no_operation_in_progress(root: Path, verb: str) -> None: `load_yaml` makes git run before the refusal of a hand-edited `path:`, which this test holds to no git command at all: `test_family_member_path.py::test_the_refusal_is_made_before_any_git_call`. + + THE MARK'S PATH IS NAMED AS `shown_source` NAMES A PATH (#692): as it is, + and as Python's `repr` spells it when a character in it is not printable. + It is the true path, as git keeps it, so a holder whose path holds a line + break printed the refusal across two lines, with a line of the person's + choosing in it, and an ESC reached the terminal. A printable path is + named byte for byte. """ found = _operation_in_progress(root) if found is None: @@ -1041,7 +1048,8 @@ def _checked_no_operation_in_progress(root: Path, verb: str) -> None: raise Refusal( "family-operation-in-progress", f"{what} is unfinished in the family root: git keeps {mark} at " - f"{where.as_posix()}. `{verb}` makes its commit with `git commit -- " + f"{shown_source(where.as_posix())}. `{verb}` makes its commit with " + "`git commit -- " f"`, and {effect.format(verb=verb)}. No git command that " "writes was run and nothing was written.", end) @@ -1945,7 +1953,8 @@ def _refuse_a_taken_holder(folder: Path | None, holder: Path) -> None: why = target_in_the_way(holder) if why: raise Refusal( - "family-target-exists", f"{holder} already exists and {why}", + "family-target-exists", + f"{shown_source(holder)} already exists and {why}", target_remediation( why, "Remediation: choose another --into (or an empty " diff --git a/scripts/shape_materialize.py b/scripts/shape_materialize.py index 58d62a79..c22decb8 100644 --- a/scripts/shape_materialize.py +++ b/scripts/shape_materialize.py @@ -1360,15 +1360,21 @@ def folder_cannot_be_made(path: Path) -> str | None: write in (the system's own answer, `os.access`, where it has one; it says yes for root, and for any folder on Windows, as `mkdir` would). A `path` nothing stands at is asked of the folder above it. + + THE FOLDER IS NAMED AS `shown_source` NAMES A PATH (#692): as it is, and + as Python's `repr` spells it when a character in it is not printable. + Printed raw, an ESC in a folder's name reached the terminal and a line + break began a line of the person's choosing. A printable path is named + byte for byte, so `why.endswith(CANNOT_WRITE_IN)` still holds. """ for here in (path, *path.parents): if not os.path.lexists(here): continue why = _not_a_folder(here) if why: - return f"{here} {why}" + return f"{shown_source(here)} {why}" if not os.access(here, os.W_OK | os.X_OK): - return f"{here} {CANNOT_WRITE_IN}" + return f"{shown_source(here)} {CANNOT_WRITE_IN}" return None return None @@ -1396,11 +1402,13 @@ def refuse_unmakeable_folder(refusal: str, named: str, path: Path, or one above it) is not told to "move what stands in the way aside": it is told what `_unwritable_remediation` says, naming the flag `choose`, which is `named` unless it is no flag (`the family folder`, met by - `--into`). Every other answer keeps `FOLDER_REMEDIATION`.""" + `--into`). Every other answer keeps `FOLDER_REMEDIATION`. + + THE PATH IS NAMED AS `folder_cannot_be_made` NAMES IT (#692).""" why = folder_cannot_be_made(path) if why: raise Refusal( - refusal, f"{named} {path} cannot be made: {why}", + refusal, f"{named} {shown_source(path)} cannot be made: {why}", _unwritable_remediation(choose or named) if why.endswith(CANNOT_WRITE_IN) else FOLDER_REMEDIATION) @@ -1412,12 +1420,13 @@ def make_folder(refusal: str, named: str, path: Path) -> None: The real run's `mkdir`, after `refuse_unmakeable_folder` has said yes: what still fails here (a file made in the meantime, a permission `os.access` did not foresee) is the same refusal, before any remote is - made.""" + made. The path is named as `refuse_unmakeable_folder` names it (#692).""" try: path.mkdir(parents=True, exist_ok=True) except OSError as exc: - raise Refusal(refusal, f"{named} {path} cannot be made: " - f"{exc.strerror or exc}", MKDIR_REMEDIATION) from exc + raise Refusal(refusal, f"{named} {shown_source(path)} cannot be " + f"made: {exc.strerror or exc}", + MKDIR_REMEDIATION) from exc def target_in_the_way(target: Path) -> str | None: diff --git a/tests/test_dry_run_parity.py b/tests/test_dry_run_parity.py index 31170cb6..89cad33a 100644 --- a/tests/test_dry_run_parity.py +++ b/tests/test_dry_run_parity.py @@ -37,6 +37,15 @@ The GitHub route runs against a stub `gh` on PATH that answers `repo view` as the test says and refuses anything else, and is POSIX only, as every `#!` stub in this suite is. + +A FOLDER'S NAME IS SPELLED SAFE TO SHOW (#692, from #459's L2-77). Each +refusal above names a folder, and printed it as it was, so an ESC in a +path reached the terminal and a line break began a line of the person's +choosing, a forged `WARNING` or `REFUSED` among them. Each is named as +`shape_materialize.shown_source` names a `--pin-source`: as it is, unless a +character in it is not printable, and then as Python's `repr` spells it. The +rows whose names are `ESC-...` and `line-break-...` below hold such a name, +and a name like that is a POSIX file name only (`POSIX_NAMES`). """ from __future__ import annotations @@ -63,6 +72,17 @@ 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.""" @@ -132,6 +152,8 @@ def assert_refused(result, said: str) -> None: """Exit 2 with the refusal `said`, its whole line, and no traceback.""" assert result.returncode == 2, result.stdout + result.stderr assert refused_line(result) == said, result.stderr + assert not any(line.startswith("WARNING forged-line") + for line in result.stderr.splitlines()), result.stderr assert "Traceback" not in result.stderr, result.stderr assert DRY_RUN_DONE not in result.stdout, result.stdout @@ -183,6 +205,22 @@ 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`).""" + return [pytest.param(name, marks=POSIX_NAMES + if name.startswith(("ESC-", "line-break-")) else ()) + for name in cases] + + #: The scaffold's four measured cases and two beside them: `(what the test #: puts in the way, the --work-dir and --local-remote-dir it passes, the #: REFUSED line both runs end on)`. On main 2989b4c each dry run exited 0; @@ -215,10 +253,40 @@ def at(tmp_path: Path, relative: str) -> Path: "up", ("wd", "up/r"), lambda t: f"{SCAFFOLD_FOLDER}: --local-remote-dir {at(t, 'up/r')} " f"cannot be made: {at(t, 'up')} is a file, not a folder"), + # #692: each folder a refusal names is spelled safe to show, whether the + # name is the folder's own or a folder above it: an ESC, and a line + # break then a forged line. There is no row for a --local-remote-dir + # like that: its clone URL is refused first, as `scaffold-unwritable- + # value`, and it names the path as `repr` spells it already. + "ESC-in-the-work-dir-name": ( + ESC_NAME, (ESC_NAME, "r"), + lambda t: f"{SCAFFOLD_FOLDER}: --work-dir {spelt(at(t, ESC_NAME))} " + f"cannot be made: {spelt(at(t, ESC_NAME))} is a file, not " + "a folder"), + "line-break-in-the-work-dir-name": ( + FORGED_NAME, (FORGED_NAME, "r"), + lambda t: f"{SCAFFOLD_FOLDER}: --work-dir " + f"{spelt(at(t, FORGED_NAME))} cannot be made: " + f"{spelt(at(t, FORGED_NAME))} is a file, not a folder"), + "ESC-in-a-folder-name-above-the-work-dir": ( + ESC_NAME, (f"{ESC_NAME}/wd", "r"), + lambda t: f"{SCAFFOLD_FOLDER}: --work-dir " + f"{spelt(at(t, f'{ESC_NAME}/wd'))} cannot be made: " + f"{spelt(at(t, ESC_NAME))} is a file, not a folder"), + "ESC-in-the-work-dir-name-holding-a-file-named-Atlas": ( + f"{ESC_NAME}/Atlas", (ESC_NAME, "r"), + lambda t: "REFUSED scaffold-target-exists: " + f"{spelt(at(t, f'{ESC_NAME}/Atlas'))} already exists and " + "is a file, not a folder"), + "line-break-in-the-work-dir-name-holding-a-file-named-Atlas": ( + f"{FORGED_NAME}/Atlas", (FORGED_NAME, "r"), + lambda t: "REFUSED scaffold-target-exists: " + f"{spelt(at(t, f'{FORGED_NAME}/Atlas'))} already exists " + "and is a file, not a folder"), } -@pytest.mark.parametrize("case", list(SCAFFOLD_CASES)) +@pytest.mark.parametrize("case", case_params(SCAFFOLD_CASES)) def test_a_scaffold_dry_run_refuses_what_its_real_run_refuses(tmp_path, case): """L2-34, the scaffold: on main every dry run here exited 0 and every real run but the non-empty tree's ended in a traceback, exit 1.""" @@ -265,10 +333,37 @@ def test_a_scaffold_dry_run_refuses_what_its_real_run_refuses(tmp_path, case): "r", (), lambda t: f"{FAMILY_FOLDER}: --local-remote-dir {at(t, 'r')} cannot " f"be made: {at(t, 'r')} is a file, not a folder"), + # #692, as the scaffold's rows beside them: an ESC, and a line break then + # a forged line, in a folder a refusal names or in one above it. + "ESC-in-the-work-dir-name": ( + ESC_NAME, ("--work-dir", ESC_NAME), + lambda t: f"{FAMILY_FOLDER}: --work-dir {spelt(at(t, ESC_NAME))} " + f"cannot be made: {spelt(at(t, ESC_NAME))} is a file, not " + "a folder"), + "line-break-in-the-work-dir-name": ( + FORGED_NAME, ("--work-dir", FORGED_NAME), + lambda t: f"{FAMILY_FOLDER}: --work-dir " + f"{spelt(at(t, FORGED_NAME))} cannot be made: " + f"{spelt(at(t, FORGED_NAME))} is a file, not a folder"), + "ESC-in-the-into-name": ( + ESC_NAME, ("--into", ESC_NAME), + lambda t: f"{FAMILY_FOLDER}: the family folder " + f"{spelt(at(t, f'{ESC_NAME}/Orbit'))} cannot be made: " + f"{spelt(at(t, ESC_NAME))} is a file, not a folder"), + "ESC-in-the-work-dir-name-holding-a-file-named-Orbit": ( + f"{ESC_NAME}/Orbit", ("--work-dir", ESC_NAME), + lambda t: "REFUSED family-target-exists: " + f"{spelt(at(t, f'{ESC_NAME}/Orbit'))} already exists and " + "is a file, not a folder"), + "line-break-in-the-work-dir-name-holding-a-file-named-Orbit": ( + f"{FORGED_NAME}/Orbit", ("--work-dir", FORGED_NAME), + lambda t: "REFUSED family-target-exists: " + f"{spelt(at(t, f'{FORGED_NAME}/Orbit'))} already exists " + "and is a file, not a folder"), } -@pytest.mark.parametrize("case", list(FAMILY_CASES)) +@pytest.mark.parametrize("case", case_params(FAMILY_CASES)) def test_an_init_dry_run_refuses_what_its_real_run_refuses(tmp_path, case): """L2-34, `init`: on main every dry run here exited 0, and every real run but the non-empty holder's ended in a traceback, exit 1.""" @@ -355,6 +450,18 @@ def test_a_folder_that_cannot_be_written_in_is_refused_by_both_runs( lambda t: f"{FAMILY_FOLDER}: --local-remote-dir {at(t, 'r')} cannot " f"be made: {at(t, 'r')} {CANNOT_WRITE_TEXT}", "--local-remote-dir"), + # #692: the folder the refusal says this user cannot write in is spelled + # safe to show too. + "ESC-in-the-scaffold-work-dir": ( + ESC_NAME, lambda t: scaffold_in(t, ESC_NAME), + lambda t: f"{SCAFFOLD_FOLDER}: --work-dir {spelt(at(t, ESC_NAME))} " + f"cannot be made: {spelt(at(t, ESC_NAME))} " + f"{CANNOT_WRITE_TEXT}", "--work-dir"), + "ESC-in-the-family-work-dir": ( + ESC_NAME, lambda t: init_in(t, "--work-dir", ESC_NAME), + lambda t: f"{FAMILY_FOLDER}: --work-dir {spelt(at(t, ESC_NAME))} " + f"cannot be made: {spelt(at(t, ESC_NAME))} " + f"{CANNOT_WRITE_TEXT}", "--work-dir"), } #: What every OTHER refusal of a folder says to do, as it always said it. @@ -365,7 +472,7 @@ def test_a_folder_that_cannot_be_written_in_is_refused_by_both_runs( @pytest.mark.parametrize("dry_run", [True, False], ids=["dry-run", "real-run"]) -@pytest.mark.parametrize("case", list(UNWRITABLE_FOLDER_AT)) +@pytest.mark.parametrize("case", case_params(UNWRITABLE_FOLDER_AT)) def test_an_unwritable_folder_a_run_makes_is_told_what_fits( tmp_path, unwritable_folder, case, dry_run): """rv622r's nit: an existing EMPTY `--work-dir`, `--local-remote-dir` or @@ -431,6 +538,60 @@ def test_the_folder_remediation_is_chosen_by_what_stands_in_the_way( assert refused.value.remediation == unwritable_remediation("--named") +# --- #692: a folder's name is spelled safe to show --------------------------- + +@POSIX_NAMES +@pytest.mark.parametrize("name", [ESC_NAME, FORGED_NAME], + ids=["ESC", "line-break"]) +def test_each_folder_question_spells_a_name_that_is_not_printable( + tmp_path, name): + """#692, one call each, beneath the CLI rows: `folder_cannot_be_made` + names the file that stands in the way, and `refuse_unmakeable_folder` + and `make_folder` name the folder they were asked for, as `repr` spells + it. On main each printed the path as it was, ESC or line break and all + (`make_folder` has no CLI row: the real run's own `mkdir` fails only for + what the question did not foresee).""" + stands = tmp_path / name + touch(stands) + below = stands / "sub" + why = f"{spelt(stands)} is a file, not a folder" + assert shape_materialize.folder_cannot_be_made(below) == why + with pytest.raises(Refusal) as refused: + shape_materialize.refuse_unmakeable_folder("x-code", "--named", below) + assert refused.value.detail == ( + f"--named {spelt(below)} cannot be made: {why}") + with pytest.raises(Refusal) as made: + shape_materialize.make_folder("x-code", "--named", below) + assert made.value.detail == ( + f"--named {spelt(below)} cannot be made: {mkdir_says(below)}") + + +@POSIX_NAMES +def test_a_folder_this_user_cannot_write_in_is_spelled_safe_to_show( + unwritable_folder): + """The other answer `folder_cannot_be_made` gives, `CANNOT_WRITE_IN`, + names its folder the same way (#692).""" + folder = unwritable_folder(ESC_NAME, CANNOT_WRITE["no-w"]) + assert (shape_materialize.folder_cannot_be_made(folder / "sub") + == f"{spelt(folder)} {shape_materialize.CANNOT_WRITE_IN}") + + +def test_a_printable_name_is_named_byte_for_byte(tmp_path): + """#692 changes no row a person could read before: a name with a space + or a letter that is not ASCII is printable, and is named as it is, with + no quotes.""" + stands = tmp_path / "wd \u00e9 and a space" + touch(stands) + assert stands.name.isprintable() + why = f"{stands} is a file, not a folder" + assert shape_materialize.folder_cannot_be_made(stands / "sub") == why + with pytest.raises(Refusal) as refused: + shape_materialize.refuse_unmakeable_folder( + "x-code", "--named", stands / "sub") + assert refused.value.detail == ( + f"--named {stands / 'sub'} cannot be made: {why}") + + # --- L2-60: an EMPTY working tree this user cannot write in ------------------ #: A folder this user cannot write in, two ways: with no `w` (nothing may be diff --git a/tests/test_family_refuse_up_front.py b/tests/test_family_refuse_up_front.py index 8258af96..6eb587b4 100644 --- a/tests/test_family_refuse_up_front.py +++ b/tests/test_family_refuse_up_front.py @@ -536,10 +536,13 @@ def test_a_merge_in_a_linked_worktree_is_found_where_git_keeps_it( @pytest.mark.skipif(sys.platform == "win32", - reason="Windows refuses a newline in a file name; Linux " - "and APFS take it") -def test_a_holder_path_with_a_newline_still_shows_its_merge( - family, holder, offline, tmp_path): + reason="Windows refuses a newline and an ESC in a file " + "name; Linux and APFS take them") +@pytest.mark.parametrize("name", ["Ink\nRouter", "Ink\x1b[31mRouter", + "Ink\nWARNING forged-line: pwned"], + ids=["line-break", "ESC", "forged-line"]) +def test_a_holder_path_with_a_control_character_still_shows_its_merge( + family, holder, offline, tmp_path, name): """V-F1 of #612's verification. A linked worktree's `git rev-parse --git-path` answers with an ABSOLUTE path through the holder's own `.git`, so a holder in a directory whose name holds a newline makes every @@ -547,9 +550,15 @@ def test_a_holder_path_with_a_newline_still_shows_its_merge( only that answer's FIRST line, which named a directory that does not exist: no mark was found, `add --dry-run` printed a plan, and the real `add` mounted and failed at the commit, #612's own defect back for that - path. The whole answer, less git's final newline, is the path.""" + path. The whole answer, less git's final newline, is the path. + + THE PATH IS NAMED AS `repr` SPELLS IT (#692, R612's round 2). It is the + true path, as git keeps it, and the refusal printed it as it was: across + two lines for a newline, with a forged line of the person's choosing for + `WARNING forged-line`, and an ESC reached the terminal. It is one line + now, with the path quoted and its control characters spelled out.""" env = isolated(offline, tmp_path) - broken = tmp_path / "Ink\nRouter" + broken = tmp_path / name shutil.copytree(holder, broken, symlinks=True) linked = tmp_path / "linked" raw(broken, env, "worktree", "add", "-q", "-b", "linked-branch", @@ -557,15 +566,18 @@ def test_a_holder_path_with_a_newline_still_shows_its_merge( begin("merge", linked, env, tmp_path) shown = raw(linked, env, "rev-parse", "--git-path", "MERGE_HEAD" ).stdout.decode() - assert shown.count("\n") == 2, f"premise: a newline in the path: {shown!r}" + where = shown[:-1] + assert name in where, f"premise: the name is in the path: {shown!r}" for extra in ((), ("--dry-run",)): ran = add(linked, env, family["remotes"], *extra) assert ran.returncode == 2, (extra, ran.stdout, ran.stderr) assert ran.stdout == "" - assert ran.stderr.startswith( + line, _ = refusal(ran.stderr) + assert line.startswith( "REFUSED family-operation-in-progress: a merge is unfinished in " "the family root: git keeps MERGE_HEAD at "), ran.stderr - assert "Ink\nRouter/.git/worktrees/" in ran.stderr, ran.stderr + assert f"git keeps MERGE_HEAD at {where!r}. `add` makes " in line + assert "\x1b" not in ran.stderr, ran.stderr def test_with_no_act_and_no_sparse_checkout_each_verb_runs_as_it_always_did(