diff --git a/README.md b/README.md index d3e8a84d..31e784dc 100644 --- a/README.md +++ b/README.md @@ -137,17 +137,14 @@ py setup-project.py --org ``` `python setup-project.py --org ` is the same command on a -machine with no `py`: Python from the Microsoft Store installs `python` and no -launcher at all. - +machine with no `py`: Microsoft Store Python installs `python`, no launcher. Two commands rather than one pipe, on purpose. Windows PowerShell 5.1 — still the default shell on a stock install — re-encodes piped text as ASCII by default (that is its `$OutputEncoding`), and writes UTF-16 when `>` redirects to a file, so no byte above 0x7F survives either route and a script sent down -one is not the script that arrives; and a script arriving on stdin has an -exhausted stdin, so it cannot ask for the one `yes` before three repositories -exist — it refuses instead. Downloading the file first leaves stdin attached -to the terminal you are typing at. +one is not the script that arrives; and a script arriving on stdin leaves it +exhausted, so it cannot ask for the one `yes` before three repositories exist +and refuses. Downloading the file first leaves stdin on your terminal. `setup-project.py` is the flow itself, and `setup.sh` a shim over it: the same preflight, the same naming check, the same plan, the same one `yes`, the same @@ -155,7 +152,10 @@ self-bootstrap outside a checkout. It never needs `make` and never looks for a `python3` — it runs the interpreter that started it. That two-liner IS the Windows way in; the `openRepoShape` command in step 3 below is a macOS, Linux and WSL2 convenience with no Windows twin, because on Windows there is nothing -to install. +to install. Printed commands are quoted for a POSIX shell, so paste them into +Git Bash. Only `shape-doctor.py`'s lines, `setup-project.py`'s family lines +and `family.py`'s `NEXT`, `switch -c`, `add` and `git rm -r --cached` lines +are quoted for PowerShell, on Windows. diff --git a/docs/handbook.html b/docs/handbook.html index 93663e37..a41dfb6d 100644 --- a/docs/handbook.html +++ b/docs/handbook.html @@ -472,7 +472,7 @@ - + @@ -609,7 +609,7 @@

The one thing to read before electing it

- +

Setup

Start here

@@ -722,7 +722,12 @@

The tool offers the rest

for git: a run refuses a missing git before its preflight starts. There is no openRepoShape command to install here: that two-liner IS the Windows way in, and the openRepoShape command is a macOS, Linux and WSL2 - convenience with no Windows twin, because on Windows there is nothing to install.

+ convenience with no Windows twin, because on Windows there is nothing to install. + Printed commands are quoted for a POSIX shell, so paste them into Git Bash. Only + shape-doctor.py’s lines, setup-project.py’s family + lines and family.py’s NEXT, switch -c, + add and git rm -r --cached lines are quoted for PowerShell, on + Windows.

@@ -1022,7 +1027,7 @@

What you have afterwards — the same on every platform

- +

Setup

If it fails part-way

@@ -1057,7 +1062,7 @@

If it fails part-way

- + @@ -1115,7 +1120,7 @@

Reading private legs in CI

- + @@ -1351,7 +1356,7 @@

The InkRouter example

- + @@ -1648,7 +1653,7 @@

Is this repository compliant? openRepoShape --doctor

- + diff --git a/scripts/bump-leg.py b/scripts/bump-leg.py index cd9028f2..59e1f5f5 100755 --- a/scripts/bump-leg.py +++ b/scripts/bump-leg.py @@ -1120,6 +1120,25 @@ def _ensure_root_clean(root: Path) -> None: "exists to keep readable.") +def _switch_command(root: Path, role: str, commit: str) -> str: + """`git -C switch -c bump/-` as a line a person + pastes, every word quoted for a POSIX shell (#532). + + THE ONE PLACE THE THREE `switch -c` LINES ARE SPELLED: the two refusals + below and the dry run's closing hint (#477) all name the same branch, and + the dry run's rule is that it names the branch the real run's refusal + names. Two of them concatenated `str(root)` bare, so a root under `My + Projects/` printed a line that ran in the wrong directory or did not + parse, and one under `it's here/` a quotation nothing closes; a Windows + root, which always holds a backslash, prints in single quotes, as + `shlex` spells it. They are lines for a POSIX shell (Git Bash, or WSL2 + on Windows), as `update-shape.py`'s are, and README's "Windows, + natively" says so. + """ + return shlex.join(["git", "-C", str(root), "switch", "-c", + f"bump/{role}-{commit[:12]}"]) + + def _branch_to_bump_from(root: Path, project: "Project", role: str, commit: str, *, dry_run: bool = False) -> str: """The branch on which the bump commits, refused if there is none or @@ -1130,7 +1149,8 @@ def _branch_to_bump_from(root: Path, project: "Project", role: str, next step would be a push to the default branch. A `--dry-run` makes no commit, so on the tracking branch it is let through and ends by naming the branch the real run needs (#477); a root on no branch at all is - refused either way. + refused either way. Both refusals name that branch's `switch -c` + through `_switch_command`, quoted (#532). """ branch = current_branch(root) if branch is None: @@ -1138,8 +1158,8 @@ def _branch_to_bump_from(root: Path, project: "Project", role: str, "bump-leg-root-detached", f"{root} is not on a branch, so there is no branch to push and " "open a pull request from", - "Remediation: `git -C " + str(root) + " switch -c bump/" - + f"{role}-{commit[:12]}` and re-run.") + f"Remediation: `{_switch_command(root, role, commit)}` and " + "re-run.") if branch == project.tracking and not dry_run: raise Refusal( "bump-leg-on-tracking-branch", @@ -1147,8 +1167,8 @@ def _branch_to_bump_from(root: Path, project: "Project", role: str, "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 git -C " + str(root) - + f" switch -c bump/{role}-{commit[:12]}\nand re-run.") + "default branch. Run:\n " + f"{_switch_command(root, role, commit)}\nand re-run.") return branch @@ -1357,10 +1377,12 @@ def _restore_leg_if_moved(submodule: Path, placement: str | None, cwd=submodule).lower() == was.lower()): restore_leg(submodule, placement, was) if _leg_status(submodule) != status: + # QUOTED (#532): the leg's path is a root's, and a person pastes it. + where = shlex.join(["git", "-C", str(submodule), "status"]) raise LegNotAsFound( "its `git status` no longer reads as it did before the bump, " "which is what a checkout cut off part way leaves behind. Read " - f"`git -C {submodule} status` before anything else, and remove " + f"`{where}` before anything else, and remove " "only what you did not put there yourself.") @@ -1639,8 +1661,7 @@ def cmd_bump(args) -> int: # 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: " - + shlex.join(["git", "-C", str(root), "switch", "-c", - f"bump/{role}-{asked[:12]}"])) + + _switch_command(root, role, asked)) return 0 # ---- the rewrite and the commit, remembered byte for byte so a failure @@ -1669,7 +1690,11 @@ def cmd_bump(args) -> int: print(f" {path}/ is left DETACHED at {commit[:12]}; `make bootstrap` in " "the root puts it on its tracking branch AT the new pin, or says " "why not") - print(f"\nNEXT git -C {root} push -u origin {branch}") + # QUOTED WHOLE (#532), the root and the branch: a root under `My + # Projects/` pasted as two arguments, and the line is the one this run + # ends on, the one a person runs next. + print("\nNEXT " + shlex.join(["git", "-C", str(root), "push", "-u", + "origin", branch])) print(f" then open a pull request against {project.tracking}; never " "push the bump to the default branch directly") return 0 diff --git a/shape-doctor.py b/shape-doctor.py index f140b38f..b54dd35f 100755 --- a/shape-doctor.py +++ b/shape-doctor.py @@ -701,6 +701,19 @@ def quote_arg(value, platform: str | None = None, *, itself is still this function's alone -- one place that knows the apostrophe is doubled, rather than a second copy of the rule beside the one that needs it. + + A WORD THAT BEGINS WITH `=` IS QUOTED IN `sh`'S DIALECT (#532). zsh, the + login shell macOS ships, expands a bare `=word` to the path of the + command `word` (its EQUALS option, on by default): `zsh -f -c 'echo =ls'` + prints `/usr/bin/ls`, and `=x` stops the line with `x not found`. bash + and dash read it as itself, and `=` anywhere but the front is itself to + every shell, so only a leading one is quoted. It stood in + `UNQUOTED_POSIX` as an ordinary character, and reached a line only as a + value that starts with one (a branch named `=x`), which is the gap #460 + named. `repo_shape.shell_quoted` quotes it already (#486, #507), and + `tests/test_family_printed_quoting.py` holds the two quoters to one + answer. PowerShell's dialect is NOT changed: it expands no `=word`, so + `UNQUOTED_NT` keeps its bare `=`, as in any other place in a word. """ # ASCII first, so the alphabet check and the quoting below it both see # what `Row.__init__` will (Copilot, PR #102); see the docstring above. @@ -708,9 +721,9 @@ def quote_arg(value, platform: str | None = None, *, windows = (os.name if platform is None else platform) == "nt" # `.fullmatch`, not `.match`: a pattern ending in `$` still matches just # before a trailing newline, so `.match` alone would call "Atlas\n" bare - # (Copilot, PR #102). + # (Copilot, PR #102). And a leading `=` is never bare for `sh` (#532). if not always and (UNQUOTED_NT if windows else UNQUOTED_POSIX).fullmatch( - text): + text) and (windows or not text.startswith("=")): return text if windows: # PowerShell's verbatim string: nothing inside is expanded, and the diff --git a/tests/test_bump_leg_dry_run_tracking.py b/tests/test_bump_leg_dry_run_tracking.py index 196d567f..5b8a168a 100644 --- a/tests/test_bump_leg_dry_run_tracking.py +++ b/tests/test_bump_leg_dry_run_tracking.py @@ -186,7 +186,9 @@ def test_the_real_run_on_the_tracking_branch_is_refused_byte_for_byte( project, scaffolded, advanced): """The refusal's every byte, spelled out, and nothing on stdout: this is the text the run had before #477, and a change to it is a change to what - a lane that scripts around it reads.""" + a lane that scripts around it reads. The `switch -c` on its last line is + spelled as the dry run's hint is, by `shlex` (#532): a plain root prints + the bytes it always did, and one that needs quoting prints quoted.""" root = project.resolve() before = snapshot(project) @@ -198,8 +200,8 @@ def test_the_real_run_on_the_tracking_branch_is_refused_byte_for_byte( "project.yaml declares as this project's tracking branch\n" "Remediation: these organisations are pull-request only, so this " "will not make a commit whose only next step is a push to the " - f"default branch. Run:\n git -C {root} switch -c " - f"bump/spec-{advanced[:12]}\nand re-run.\n") + f"default branch. Run:\n {switch_command(project, advanced)}\n" + "and re-run.\n") assert_nothing_changed(project, before) diff --git a/tests/test_bump_leg_paste_lines.py b/tests/test_bump_leg_paste_lines.py new file mode 100644 index 00000000..5763c3b4 --- /dev/null +++ b/tests/test_bump_leg_paste_lines.py @@ -0,0 +1,408 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The three paste lines `scripts/bump-leg.py` printed with the root bare keep +a root with a space, a quote or a `$` in it whole (#532). + +`_branch_to_bump_from` handed a person `git -C switch -c bump/...` in +both of its refusals, and the real run ended on `NEXT git -C push -u +origin `, each with `str(root)` concatenated in. Under `sp ace/` the +line pasted as `git -C .../sp` and an argument nobody wrote, under `it's/` it +was a quotation nothing closes, and under `d$x/` the shell expanded `$x` +before git saw it. Only #484's dry-run hint had been quoted, with `shlex`. The fourth is the +`git -C status` that `_restore_leg_if_moved`'s `LegNotAsFound` tells a +person to read: the leg's path is a root's, and it was bare too. + +THE CLAIM IS MADE THE WAY A SHELL WOULD CHECK IT. Each printed line is read +back by `shlex.split`, the word-splitting a POSIX shell performs, and must +come back as the argv it was printed to mean; then, wherever the host has `sh`, +the line is PASTED into one and what it does is read from git: the branch it +makes is the branch the re-run commits on, and the push it is lands the commit +in the root's remote. Neither is `str in str`. + +WHAT IS NOT CLAIMED. These lines are for a POSIX shell (Git Bash, or WSL2, on +Windows), as README's "Windows, natively" says: a Windows root holds a +backslash, which `shlex` quotes, so every exact-string expectation below is +built with `shlex` too, and the pastes into `sh` are POSIX-only. + +NO NETWORK, NO `gh`: the bare remotes are the session's own, and the root's +own remote is a private copy, so the push a pasted line makes reaches nothing +another test reads. +""" + +from __future__ import annotations + +import os +import re +import shlex +import shutil +import subprocess +from pathlib import Path + +import pytest + +from conftest import PROJECT, WINDOWS_SKIP, cloned_just_pushed, git +# A fixture is used by being named in this module's namespace, which is what +# the import of `bump_leg_module` is for. +from test_bump_leg import ( # noqa: F401 + WORKING_BRANCH, branched, bump, bump_leg_module, +) + +#: Where the advanced spec commit is pushed. Not another module's branch: the +#: bare remotes are session-scoped, and a second, different commit pushed to +#: a name already taken is refused as non-fast-forward. +ADVANCE_BRANCH = "bump-leg-paste-lines-advance" + +#: A directory name each way a path breaks a line pasted bare: a space (two +#: arguments), an apostrophe (a quotation nothing closes), a `$` (an +#: expansion). +ROOTS = [ + pytest.param("sp ace", id="space"), + pytest.param("it's", id="apostrophe"), + pytest.param("d$x", id="dollar"), +] + + +@pytest.fixture(scope="module") +def advanced(scaffolded, tmp_path_factory) -> str: + """A real new commit on the spec leg's bare remote; returns its sha.""" + work = cloned_just_pushed( + scaffolded["remotes"] / f"{PROJECT}-spec.git", + tmp_path_factory.mktemp("spec-advance-paste-lines") + / f"{PROJECT}-spec") + (work / "PASTE-LINES.md").write_text("# a contract gained\n") + git("add", "--", "PASTE-LINES.md", cwd=work) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", + "Advance the spec leg for the paste-line tests", cwd=work) + git("push", "-q", "origin", f"HEAD:refs/heads/{ADVANCE_BRANCH}", cwd=work) + return git("rev-parse", "HEAD", cwd=work).stdout.strip() + + +@pytest.fixture +def make_root(scaffolded, tmp_path): + """`make_root(name)`: a private copy of the assembly root at + `//Atlas`, pushing to a private copy of its bare remote, so a + pasted `git push` reaches nothing the session shares. + + A COPY rather than a scaffold into such a directory: the point under test + is what the tool PRINTS about a root, and a submodule's `.git` is a file + holding a RELATIVE path into `../.git/modules/`, which keeps the + copy honest as well as cheap. The legs are still fetched from the + session's remotes, through `--local-remote-dir`, as every test here does. + """ + made: list[Path] = [] + + def make(name: str) -> tuple[Path, Path]: + remote = tmp_path / f"remote-{len(made)}" / f"{PROJECT}.git" + shutil.copytree(scaffolded["remotes"] / f"{PROJECT}.git", remote, + symlinks=True) + target = tmp_path / name / PROJECT + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree(scaffolded["clone"], target, symlinks=True) + git("remote", "set-url", "origin", str(remote), cwd=target) + made.append(target) + return target, remote + + return make + + +def words(line: str) -> list[str]: + """The words a POSIX shell reads out of `line`, or a failure that says what + the shell said: a line no shell can read is the defect, and its own + assertion.""" + try: + return shlex.split(line) + except ValueError as exc: + raise AssertionError(f"no shell can read this line -- {exc}\n" + f" {line}") from exc + + +def paste(line: str, cwd: Path) -> subprocess.CompletedProcess: + """`line` typed into a POSIX `sh`, from a directory that is not the + root's, so only what the line itself says can reach it.""" + return subprocess.run(["sh", "-c", line], cwd=str(cwd), + capture_output=True, text=True, check=False) + + +def switch_argv(root: Path, to: str) -> list[str]: + """What the `switch -c` lines mean: the root as `Project.open` resolves + it, and the branch `bump/-`.""" + return ["git", "-C", str(root.resolve()), "switch", "-c", + f"bump/spec-{to[:12]}"] + + +def detached_remedy(stderr: str) -> str: + """The command between the backticks of `bump-leg-root-detached`'s + remediation.""" + found = re.search(r"^Remediation: `(.*)` and re-run\.$", stderr, re.M) + assert found, stderr + return found.group(1) + + +def tracking_remedy(stderr: str) -> str: + """The command `bump-leg-on-tracking-branch` puts on a line of its own, + after `Run:`.""" + found = re.search(r"default branch\. Run:\n (.*)\nand re-run\.$", + stderr, re.M) + assert found, stderr + return found.group(1) + + +def next_line(stdout: str) -> str: + """The one `NEXT` line a real run ends on, whole, without its label.""" + lines = [line for line in stdout.splitlines() + if line.startswith("NEXT")] + assert len(lines) == 1, stdout + assert lines[0].startswith("NEXT "), lines[0] + return lines[0][len("NEXT "):] + + +def current_branch(root: Path): + return git("symbolic-ref", "--short", "-q", "HEAD", cwd=root, + check=False).stdout.strip() + + +# --- the two `switch -c` remedies -------------------------------------------- + + +@pytest.mark.parametrize("name", ROOTS) +def test_the_root_detached_remedy_is_one_command( + make_root, scaffolded, advanced, name): + """`bump-leg-root-detached`: the root is on no branch, and the remedy's + `switch -c` is split into the argv it means.""" + root, _ = make_root(name) + git("checkout", "-q", "--detach", cwd=root) + + result = bump(root, scaffolded, advanced) + assert result.returncode == 2, result.stderr + result.stdout + assert result.stdout == "" + assert "REFUSED bump-leg-root-detached" in result.stderr, result.stderr + assert words(detached_remedy(result.stderr)) == switch_argv(root, advanced) + + +@pytest.mark.parametrize("name", ROOTS) +def test_the_tracking_branch_remedy_is_one_command( + make_root, scaffolded, advanced, name): + """`bump-leg-on-tracking-branch`: the root stands on the project's + tracking branch, and the `switch -c` on a line of its own is the argv it + means.""" + root, _ = make_root(name) + + result = bump(root, scaffolded, advanced) + assert result.returncode == 2, result.stderr + result.stdout + assert result.stdout == "" + assert "REFUSED bump-leg-on-tracking-branch" in result.stderr + assert words(tracking_remedy(result.stderr)) == switch_argv(root, + advanced) + + +def test_the_two_remedies_and_the_dry_run_hint_are_one_spelling( + make_root, scaffolded, advanced): + """The dry run's rule (#477) is that it names the branch the real run's + refusal names; all three lines come from one function, so one spelling, + quoting included.""" + root, _ = make_root("it's") + hint = bump(root, scaffolded, advanced, "--dry-run") + assert hint.returncode == 0, hint.stderr + hint.stdout + needle = "Create one first with: " + assert hint.stdout.count(needle) == 1, hint.stdout + from_the_dry_run = hint.stdout.split(needle)[1].splitlines()[0] + assert words(from_the_dry_run) == switch_argv(root, advanced) + + refused = bump(root, scaffolded, advanced) + assert tracking_remedy(refused.stderr) == from_the_dry_run + + git("checkout", "-q", "--detach", cwd=root) + detached = bump(root, scaffolded, advanced) + assert detached_remedy(detached.stderr) == from_the_dry_run + + +def test_an_exact_remedy_is_the_line_shlex_spells(make_root, scaffolded, + advanced): + """The byte test, built with `shlex` so it holds on a Windows root too + (every one holds a backslash, which `shlex` quotes): the whole remediation + of the tracking-branch refusal, and the root-detached one.""" + root, _ = make_root("sp ace") + line = shlex.join(switch_argv(root, advanced)) + tracking = bump(root, scaffolded, advanced) + assert tracking.stderr.endswith( + "will not make a commit whose only next step is a push to the " + f"default branch. Run:\n {line}\nand re-run.\n"), tracking.stderr + + git("checkout", "-q", "--detach", cwd=root) + detached = bump(root, scaffolded, advanced) + assert detached.stderr.endswith(f"Remediation: `{line}` and re-run.\n"), \ + detached.stderr + + +@WINDOWS_SKIP +@pytest.mark.parametrize("name", ROOTS) +def test_pasting_the_tracking_branch_remedy_makes_the_branch_the_rerun_uses( + make_root, scaffolded, advanced, name): + """THE LINE, RUN: pasted into `sh` it makes `bump/spec-` in THAT root + and nowhere else, and the run it refused then goes through, on it.""" + root, _ = make_root(name) + refused = bump(root, scaffolded, advanced) + assert refused.returncode == 2, refused.stderr + refused.stdout + assert current_branch(root) == "main" + + pasted = paste(tracking_remedy(refused.stderr), root.parent) + assert pasted.returncode == 0, pasted.stderr + pasted.stdout + assert current_branch(root) == f"bump/spec-{advanced[:12]}" + + again = bump(root, scaffolded, advanced) + assert again.returncode == 0, again.stderr + again.stdout + + +@WINDOWS_SKIP +@pytest.mark.parametrize("name", ROOTS) +def test_pasting_the_detached_remedy_puts_the_root_on_a_branch( + make_root, scaffolded, advanced, name): + root, _ = make_root(name) + git("checkout", "-q", "--detach", cwd=root) + refused = bump(root, scaffolded, advanced) + assert refused.returncode == 2, refused.stderr + refused.stdout + assert current_branch(root) == "" + + pasted = paste(detached_remedy(refused.stderr), root.parent) + assert pasted.returncode == 0, pasted.stderr + pasted.stdout + assert current_branch(root) == f"bump/spec-{advanced[:12]}" + + again = bump(root, scaffolded, advanced) + assert again.returncode == 0, again.stderr + again.stdout + + +# --- the closing NEXT -------------------------------------------------------- + + +@pytest.mark.parametrize("name", ROOTS) +def test_the_closing_next_is_one_command(make_root, scaffolded, advanced, + name): + """`NEXT git -C push -u origin `, split into the argv it + means, and spelled exactly as `shlex` spells it.""" + root, _ = make_root(name) + branched(root) + result = bump(root, scaffolded, advanced) + assert result.returncode == 0, result.stderr + result.stdout + + line = next_line(result.stdout) + argv = ["git", "-C", str(root.resolve()), "push", "-u", "origin", + WORKING_BRANCH] + assert words(line) == argv + assert line == shlex.join(argv) + assert (" then open a pull request against main; never push the " + "bump to the default branch directly") in result.stdout + + +@pytest.mark.skipif(os.name == "nt", reason="a Windows root holds a " + "backslash, which `shlex` quotes") +def test_an_ordinary_root_prints_the_line_it_always_did(make_root, scaffolded, + advanced): + """Nothing is quoted that needs no quoting: a root whose name is plain + prints the same bytes as before #532.""" + root, _ = make_root("plain") + branched(root) + result = bump(root, scaffolded, advanced) + assert result.returncode == 0, result.stderr + result.stdout + assert next_line(result.stdout) == ( + f"git -C {root.resolve()} push -u origin {WORKING_BRANCH}") + + +@WINDOWS_SKIP +@pytest.mark.parametrize("name", ROOTS) +def test_pasting_the_closing_next_pushes_the_commit_the_run_made( + make_root, scaffolded, advanced, name): + """THE LINE, RUN: pasted into `sh` it pushes THIS root's branch, and the + root's own remote then holds it at the commit the run wrote.""" + root, remote = make_root(name) + branched(root) + result = bump(root, scaffolded, advanced) + assert result.returncode == 0, result.stderr + result.stdout + committed = git("rev-parse", "HEAD", cwd=root).stdout.strip() + + pasted = paste(next_line(result.stdout), root.parent) + assert pasted.returncode == 0, pasted.stderr + pasted.stdout + assert git("rev-parse", f"refs/heads/{WORKING_BRANCH}", + cwd=remote).stdout.strip() == committed + + +@pytest.mark.parametrize("branch", ["bump/it's", "wip$x", "fix/a;b"]) +def test_the_closing_next_carries_the_branch_whole(make_root, scaffolded, + advanced, branch): + """The BRANCH is a word of the line too, and git accepts an apostrophe, a + `$` and a `;` in one: bare, the last word of the line would be a + quotation nothing closes, an expansion, or the end of the command.""" + root, _ = make_root("sp ace") + branched(root, branch) + result = bump(root, scaffolded, advanced) + assert result.returncode == 0, result.stderr + result.stdout + + line = next_line(result.stdout) + assert words(line) == ["git", "-C", str(root.resolve()), "push", "-u", + "origin", branch] + assert words(line)[-1] == branch + assert line == shlex.join(["git", "-C", str(root.resolve()), "push", "-u", + "origin", branch]) + + +@WINDOWS_SKIP +@pytest.mark.parametrize("branch", ["bump/it's", "wip$x", "fix/a;b"]) +def test_pasting_the_closing_next_pushes_a_branch_with_a_quote_in_its_name( + make_root, scaffolded, advanced, branch): + """THE LINE, RUN: the branch that lands in the root's remote is the one + the root stood on, whatever it is called.""" + root, remote = make_root("it's") + branched(root, branch) + result = bump(root, scaffolded, advanced) + assert result.returncode == 0, result.stderr + result.stdout + committed = git("rev-parse", "HEAD", cwd=root).stdout.strip() + + pasted = paste(next_line(result.stdout), root.parent) + assert pasted.returncode == 0, pasted.stderr + pasted.stdout + assert git("rev-parse", f"refs/heads/{branch}", + cwd=remote).stdout.strip() == committed + + +# --- the status line of a leg that is not as it was found -------------------- + + +def status_remedy(message: str) -> str: + """The command between the backticks after `Read`, in + `LegNotAsFound`'s message.""" + found = re.search(r"Read `(.*)` before anything else", message) + assert found, message + return found.group(1) + + +@pytest.mark.parametrize("name", ROOTS) +def test_the_status_line_of_a_leg_not_as_found_is_one_command( + make_root, bump_leg_module, name): + """`_restore_leg_if_moved` reads the leg's `git status` against what it + was before the bump and, where it differs, sends the person to read it. + The leg's path is a root's: `git -C status` pasted bare under `sp + ace/` was `git -C .../sp` and an argument nobody wrote.""" + root, _ = make_root(name) + leg = root.resolve() / "spec" + placement = bump_leg_module.leg_placement(leg) + was = git("rev-parse", "HEAD", cwd=leg).stdout.strip() + + with pytest.raises(bump_leg_module.LegNotAsFound) as raised: + bump_leg_module._restore_leg_if_moved(leg, placement, was, + b"not its status") + line = status_remedy(str(raised.value)) + assert words(line) == ["git", "-C", str(leg), "status"] + assert line == shlex.join(["git", "-C", str(leg), "status"]) + + +@WINDOWS_SKIP +@pytest.mark.parametrize("name", ROOTS) +def test_pasting_the_status_line_reads_that_leg(make_root, bump_leg_module, + name): + root, _ = make_root(name) + leg = root.resolve() / "spec" + was = git("rev-parse", "HEAD", cwd=leg).stdout.strip() + with pytest.raises(bump_leg_module.LegNotAsFound) as raised: + bump_leg_module._restore_leg_if_moved( + leg, bump_leg_module.leg_placement(leg), was, b"not its status") + + pasted = paste(status_remedy(str(raised.value)), root.parent) + assert pasted.returncode == 0, pasted.stderr + pasted.stdout + assert pasted.stderr == "" diff --git a/tests/test_bump_leg_rollback.py b/tests/test_bump_leg_rollback.py index 94e42b11..55c1bdd8 100644 --- a/tests/test_bump_leg_rollback.py +++ b/tests/test_bump_leg_rollback.py @@ -45,6 +45,7 @@ import errno import io import os +import shlex import stat import sys from pathlib import Path @@ -463,7 +464,9 @@ def cut_off(args, *rest, **kwargs): assert ROLLED_BACK not in err, "nothing says the tree is exactly as it was" assert "the checkout of spec/ could not be put back: its `git status`" \ in err, "the step is named" - assert "spec status` before anything else" in err, err + where = shlex.join(["git", "-C", str((project / "spec").resolve()), + "status"]) + assert f"`{where}` before anything else" in err, err after = snapshot(project) for key in ("head", "pin", "workflows", "gitlink", "leg_head", "leg_branch"): diff --git a/tests/test_family_printed_quoting.py b/tests/test_family_printed_quoting.py index 059dc87b..db852475 100644 --- a/tests/test_family_printed_quoting.py +++ b/tests/test_family_printed_quoting.py @@ -223,12 +223,16 @@ def test_it_is_the_doctors_rule_on_any_ascii_value(doctor, platform): `quote_arg` flattens its value to ASCII first, because its report is pure ASCII, and is otherwise this function: the same two alphabets and the same two escapes. On an ASCII value the two must agree, but for a - leading `=`, which this function quotes and the doctor does not yet.""" - values = [*ORDINARY, *(value for value in AWKWARD if value.isascii()), + leading `=` in PowerShell's dialect: this function quotes it in both, + because zsh expands a bare `=word`, and the doctor quotes it for `sh` + only (#532), because PowerShell expands nothing there.""" + values = [*ORDINARY, *LEADING_EQUALS, + *(value for value in AWKWARD if value.isascii()), r"C:\work,old\Atlas", *EVERY_ASCII] differ = {value: (shell_quoted(value, platform), doctor.quote_arg(value, platform)) - for value in values if not value.startswith("=") + for value in values + if not (platform == "nt" and value.startswith("=")) and shell_quoted(value, platform) != doctor.quote_arg(value, platform)} assert differ == {} @@ -238,6 +242,53 @@ def test_it_is_the_doctors_rule_on_any_ascii_value(doctor, platform): assert reader(quoted) == [value], quoted +@pytest.mark.parametrize("value", LEADING_EQUALS) +def test_the_doctor_quotes_a_word_that_begins_with_equals_for_sh(doctor, + value): + """#532. `quote_arg` left a leading `=` bare, so a NEXT line holding one + (a branch named `=x`) ran in zsh, macOS's login shell, with `=x` read as + the path of a command `x`. Quoted, `sh` reads the value that went in.""" + quoted = doctor.quote_arg(value, "posix") + assert quoted == f"'{value}'" + assert shlex.split(quoted) == [value] + + +@pytest.mark.parametrize("value", ["KEY=value", "--root=/srv/work", "a=", + "x==y"]) +def test_an_equals_sign_anywhere_but_the_front_stays_bare(doctor, value): + """Only a LEADING `=` is expanded, so the doctor's lines that carry + `KEY=value` words print as they always did.""" + assert doctor.quote_arg(value, "posix") == value + + +@pytest.mark.parametrize("value", LEADING_EQUALS) +def test_the_doctors_powershell_dialect_leaves_a_leading_equals_alone(doctor, + value): + """PowerShell expands no `=word`, so the dialect that quotes for it has no + such gap and is not changed (#532): the doctor's lines on Windows print + as they did.""" + assert doctor.quote_arg(value, "nt") == value + + +@pytest.mark.skipif(os.name == "nt", reason="a POSIX shell's own reading") +@pytest.mark.parametrize("shell", ["sh", "bash", "zsh"]) +def test_a_real_shell_hands_back_each_word_the_doctor_quoted(doctor, shell): + """The shells themselves, where the host has them: each value, quoted by + the doctor for `sh`, comes back from `printf` byte for byte. `zsh -f` is + the one that matters here, and the one that expands a bare `=ls`.""" + if shutil.which(shell) is None: + pytest.skip(f"no {shell} on this host") + values = [*LEADING_EQUALS, "KEY=value", + *(value for value in AWKWARD if value.isascii()), + *(chr(code) for code in range(0x21, 0x7f))] + line = "printf '%s\\0' " + " ".join(doctor.quote_arg(value, "posix") + for value in values) + argv = [shell, "-f", "-c", line] if shell == "zsh" else \ + [shell, "-c", line] + printed = subprocess.run(argv, capture_output=True, check=True).stdout + assert printed.decode("utf-8").split("\0")[:-1] == values + + def test_the_two_alphabets_are_the_doctors(doctor): """The same two character sets, spelled as the same two patterns: the doctor anchors its with `^...$`, and these are asked with `fullmatch`.""" diff --git a/tests/test_readme_windows_quoting_sentence.py b/tests/test_readme_windows_quoting_sentence.py new file mode 100644 index 00000000..2812a7c7 --- /dev/null +++ b/tests/test_readme_windows_quoting_sentence.py @@ -0,0 +1,120 @@ +# SPDX-License-Identifier: Apache-2.0 +"""README's sentence about which printed commands are quoted for PowerShell, +and what keeps it true (#532). + +"Windows, natively" tells a person where to paste what the tools print: into +Git Bash, because the tools quote for a POSIX shell, except for the lines of +the tools that quote for the shell they run in, which on Windows is +PowerShell. A person who follows the sentence and pastes a PowerShell-quoted +line into Git Bash gets a different command (`git -C C:\\work\\Orbit` reads as +`C:workOrbit`), so the sentence is a promise, and two things hold it: + + * the handbook's Windows panel says the same words as README does, so the + page cannot drift from the file it quotes (#519 holds each section's + HASH of the README, not what the two say); + * the tools that call a quoter that follows the host (`quote_arg`, + `shell_quoted`, `shell_line`, `shell_word`) are exactly the three the + sentence names, so a fourth is a fault here until the sentence names it. + +Nothing runs a tool and nothing touches git: the first reads two files, the +second reads every shipped `.py` file with `ast`. +""" + +from __future__ import annotations + +import ast +import html +import re + +from conftest import REPO + +#: The sentence, cut out of README by its first and last words. +SENTENCE = re.compile(r"Printed commands are quoted for a POSIX shell.*?, " + r"on Windows\.") + +#: The quoters whose answer follows the host: PowerShell's dialect where +#: `os.name` is `nt`, `sh`'s elsewhere. +HOST_QUOTERS = {"quote_arg", "shell_quoted", "shell_line", "shell_word"} + +#: The tools the sentence names, which is every shipped file that calls one. +NAMED = {"shape-doctor.py", "scripts/family.py", "setup-project.py"} + + +def squashed(text: str) -> str: + """`text` with every run of whitespace one space: README wraps a line + where the handbook wraps another.""" + return " ".join(text.split()) + + +def readme_section() -> str: + """README's "Windows, natively" section, without its Markdown.""" + text = (REPO / "README.md").read_text(encoding="utf-8") + start = text.index("Windows, natively") + section = text[start:text.index("", start)] + return squashed(section.replace("`", "")) + + +def handbook_panel() -> str: + """The handbook's Windows panel as text: its tags gone, its entities + read, and the typographic apostrophe the page writes (`’`) the + ASCII one README writes.""" + text = (REPO / "docs" / "handbook.html").read_text(encoding="utf-8") + start = text.index('id="panel-windows"') + panel = text[start:text.index("", start)] + plain = html.unescape(re.sub(r"<[^>]*>", "", panel)) + return squashed(plain.replace("\u2019", "'")) + + +def test_the_sentence_is_in_readmes_windows_section_once(): + found = SENTENCE.findall(readme_section()) + assert len(found) == 1, found + + +def test_the_handbooks_windows_panel_says_the_same_words(): + sentence = SENTENCE.findall(readme_section())[0] + panel = handbook_panel() + assert panel.count(sentence) == 1, ( + "docs/handbook.html's Windows panel (`panel-windows`) must say, word " + "for word once its tags are gone and its whitespace is one space, " + f"what README's \"Windows, natively\" says:\n {sentence}") + + +def host_calls(path) -> list[int]: + """The lines of every call in `path` to a quoter that follows the host: + not one that forces `sh`'s dialect with a `'posix'` argument or a + `posix=True` keyword.""" + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + lines = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + name = func.id if isinstance(func, ast.Name) else getattr( + func, "attr", None) + if name not in HOST_QUOTERS: + continue + forced = any(isinstance(arg, ast.Constant) and arg.value == "posix" + for arg in node.args[1:]) or any( + isinstance(kw.value, ast.Constant) + and (kw.value.value == "posix" or kw.value.value is True) + for kw in node.keywords if kw.arg in ("platform", "posix")) + if not forced: + lines.append(node.lineno) + return lines + + +def test_the_tools_that_quote_for_the_host_are_the_ones_the_sentence_names(): + quoting = {} + for path in REPO.rglob("*.py"): + relative = path.relative_to(REPO) + if (relative.parts[0] == "tests" or relative.parts[0].startswith(".") + or relative.as_posix() == "scripts/repo_shape.py"): + continue # the quoters' own home is not a tool + lines = host_calls(path) + if lines: + quoting[relative.as_posix()] = lines + assert set(quoting) == NAMED, ( + "README's \"Windows, natively\" and the handbook's Windows panel name " + "the tools whose printed lines are quoted for PowerShell on Windows. " + f"The files that call a host-dialect quoter are {sorted(quoting)}: " + "change the sentence in both places, then NAMED here.") diff --git a/tests/test_update_shape_paste_lines.py b/tests/test_update_shape_paste_lines.py new file mode 100644 index 00000000..891aa917 --- /dev/null +++ b/tests/test_update_shape_paste_lines.py @@ -0,0 +1,392 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The paste lines `update-shape.py` printed with a root or a path bare keep a +space, a quote or a `$` in it whole (#532). + +#325 quoted the NEXT lines a run prints before anything has gone wrong, and +#202 and #204 the rollback's hand fixes. Six places still concatenated a root +or a path in, each handing a person a line that ran in the wrong directory, +did not parse, or expanded: + + * `check`'s ADDED UPSTREAM list, `--add ` for each path; + * `apply`'s `update-branch-exists` remedy, `git -C checkout + `; + * `apply`'s `update-local-drift` remedy, `--accept-local ` for each; + * `apply`'s note for an `upstream-added` path it was not told to take, `pass + --add `; + * the two sentences `check` appends to a `both` and a `locally-modified` + row, each naming `apply --accept-local ` (display only: the NEXT + line leaves the flag out, but a person types it). + +Measured on 45e01f4, an added path `notes/shape notes.md` pasted as printed +was refused `unrecognized arguments: notes.md`. + +THE CLAIM IS MADE THE WAY A SHELL WOULD CHECK IT. Each printed word is read +back by `shlex.split`, the word-splitting a POSIX shell performs, and must +come back as the argv it was printed to mean, for a name with a space, an +apostrophe and a `$`; then, wherever the host has `sh`, it is PASTED into one +and what it does is read from the tool or from git. The `--add` and the +`checkout` are run as printed, and the paths a standard could not hold (an +apostrophe and a `$` are outside the alphabet it accepts for a copied file) are +read by a real `sh` through `printf`. + +WHAT IS NOT CLAIMED. These lines are for a POSIX shell (Git Bash, or WSL2, on +Windows), as `_by_hand` says of its own and README's "Windows, natively" says +of all of them: a Windows root holds a backslash, which `shlex` quotes, so +every exact-string expectation is built with `shlex` too, and the pastes into +`sh` are POSIX-only. + +NO NETWORK: the upstream is a clone of this repository on disk, passed with +`--upstream `, and the project is a private copy of one scaffolded from +it. +""" + +from __future__ import annotations + +import importlib.util +import os +import re +import shlex +import shutil +import sys +from pathlib import Path + +import pytest + +from conftest import REPO, WINDOWS_SKIP, git, run_script +#: The one reading of a printed line, and the one `sh` a line is pasted into, +#: and the three directory names a root is put under: the bump tests' (#532). +from test_bump_leg_paste_lines import ROOTS, paste, words +from test_update_shape import PROJECT, scaffold_upstream_and_project +from test_update_shape_add import MATERIALIZER, commit_all, name_in_list + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import Refusal, load_yaml # noqa: E402 + +UPDATE = REPO / "update-shape.py" + +#: A Windows root holds a backslash, which `shlex` quotes, and a Windows host +#: has no `sh` to paste into. +ON_WINDOWS = os.name == "nt" + +#: A file the standard gains after the project was cut, whose name holds a +#: space: the one an ADDED UPSTREAM line has to carry whole. A name with an +#: apostrophe or a `$` is not a name the standard may have (`SAFE_PATH_RE`), +#: so those two are read in-process, below. +ADDED = "AGENTS shape added.md" +ADDED_BYTES = b"# A file the standard gained, named with a space.\n" + +#: Each way a PATH breaks a line pasted bare: a space, an apostrophe, a `$`. +PATHS = [ + pytest.param("notes/sp ace.md", id="space"), + pytest.param("notes/it's.md", id="apostrophe"), + pytest.param("notes/d$x.md", id="dollar"), +] + + +@pytest.fixture(scope="module") +def standard(tmp_path_factory) -> dict: + """A clone of this repository at A, a project scaffolded from it, and B: + a commit that adds `ADDED` to the assembly root's copy list.""" + base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-paste-lines") + (upstream / "templates" / "assembly-root" / ADDED).write_bytes(ADDED_BYTES) + source = upstream / MATERIALIZER + source.write_bytes(name_in_list(source.read_bytes().decode("utf-8"), + "COPIED_VERBATIM", ADDED).encode("utf-8")) + commit_b = commit_all(upstream, "Add a file whose name holds a space", + [MATERIALIZER, f"templates/assembly-root/{ADDED}"]) + assert commit_a != commit_b + return {"upstream": upstream, "clone": clone, "a": commit_a, + "b": commit_b} + + +@pytest.fixture +def make_root(standard, tmp_path): + """`make_root(name)`: a private copy of the project at `//Atlas`. + + A COPY rather than a scaffold into such a directory: the point under test + is what the tool PRINTS about a root, and a submodule's `.git` is a file + holding a RELATIVE path into `../.git/modules/`, which keeps the + copy honest as well as cheap.""" + def make(name: str) -> Path: + target = tmp_path / name / PROJECT + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree(standard["clone"], target, symlinks=True) + return target + + return make + + +@pytest.fixture(scope="module") +def update_shape(): + """`update-shape.py` as a module, so a line is built in this process.""" + spec = importlib.util.spec_from_file_location("update_shape_paste_lines", + UPDATE) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def check(standard, root, *extra): + return run_script(UPDATE, "check", "--root", str(root), "--upstream", + str(standard["upstream"]), "--at", standard["b"], + *extra) + + +def apply(standard, root, *extra): + return run_script(UPDATE, "apply", "--root", str(root), "--yes", + "--upstream", str(standard["upstream"]), "--at", + standard["b"], *extra) + + +def sh_reads(fragment: str, cwd: Path) -> list[str]: + """The words a real `sh` makes of `fragment`: `printf` prints each + argument on a line of its own.""" + pasted = paste("printf '%s\\n' " + fragment, cwd) + assert pasted.returncode == 0, pasted.stderr + return pasted.stdout.splitlines() + + +def apply_line(standard, root: Path, tail: str) -> str: + """An `apply` a person could paste, every word quoted, ending in `tail`.""" + head = [sys.executable, str(UPDATE), "apply", "--root", str(root), + "--yes", "--upstream", str(standard["upstream"]), "--at", + standard["b"]] + return f"{shlex.join(head)} {tail}" + + +def pin_paths(root: Path) -> list[str]: + return [row["path"] for row in + load_yaml(root / "contracts" / "shape-pin.yaml")["files"]] + + +# --- check: the ADDED UPSTREAM list ------------------------------------------ + + +def added_tail(stdout: str) -> str: + """The `--add ` words `check` ends its ADDED UPSTREAM sentence on.""" + found = re.search(r"one path at a time — (.*)$", stdout, re.M) + assert found, stdout + return found.group(1) + + +@pytest.mark.parametrize("name", ROOTS) +def test_check_offers_an_added_path_as_one_argument(standard, make_root, name): + """`--add AGENTS shape added.md` pasted bare is `--add AGENTS` and two + words `apply` refuses; quoted, it is the one path.""" + root = make_root(name) + result = check(standard, root) + assert result.returncode == 1, result.stdout + result.stderr + tail = added_tail(result.stdout) + assert words(tail) == ["--add", ADDED] + assert tail == f"--add {shlex.quote(ADDED)}" + + +@WINDOWS_SKIP +def test_pasting_the_added_upstream_words_into_apply_takes_the_file( + standard, make_root, tmp_path): + """THE WORDS, RUN: an `apply` whose last words are the ones `check` + printed takes the one file the standard gained, at the path it has.""" + root = make_root("it's") + tail = added_tail(check(standard, root).stdout) + assert ADDED not in pin_paths(root) + + pasted = paste(apply_line(standard, root, tail), tmp_path) + assert pasted.returncode == 0, pasted.stdout + pasted.stderr + assert (root / ADDED).read_bytes() == ADDED_BYTES + assert ADDED in pin_paths(root) + + +# --- apply: the note for a path it was not told to take ---------------------- + + +@pytest.mark.parametrize("name", ROOTS) +def test_apply_without_add_names_the_path_to_pass_as_one_argument( + standard, make_root, name): + root = make_root(name) + result = apply(standard, root) + assert result.returncode == 0, result.stdout + result.stderr + noted = [line for line in result.stdout.splitlines() + if line.startswith(" note ") and ADDED in line] + assert len(noted) == 1, result.stdout + took = noted[0].split("; not added; ")[1] + assert took == f"pass --add {shlex.quote(ADDED)}" + assert words(took) == ["pass", "--add", ADDED] + assert not (root / ADDED).exists(), "no --add was passed" + + +# --- apply: the update-branch-exists remedy ---------------------------------- + + +def checkout_remedy(stderr: str) -> str: + """The command between the backticks of `update-branch-exists`'s + remediation.""" + found = re.search(r"check out that branch first — `(.*)` — if that is", + stderr) + assert found, stderr + return found.group(1) + + +@pytest.mark.parametrize("name", ROOTS) +def test_the_branch_exists_remedy_is_one_command(standard, make_root, name): + """`git -C checkout ` bare was `git -C .../sp` and an + argument nobody wrote under `sp ace/` (exit 128), a quotation nothing + closes under `it's/`.""" + root = make_root(name) + git("branch", "shape/someone-elses", cwd=root) + + result = apply(standard, root, "--branch", "shape/someone-elses") + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED update-branch-exists" in result.stderr, result.stderr + line = checkout_remedy(result.stderr) + argv = ["git", "-C", str(root.resolve()), "checkout", + "shape/someone-elses"] + assert words(line) == argv + assert line == shlex.join(argv) + + +@WINDOWS_SKIP +@pytest.mark.parametrize("name", ROOTS) +def test_pasting_the_branch_exists_remedy_checks_the_branch_out( + standard, make_root, tmp_path, name): + """THE LINE, RUN: pasted into `sh` it checks the existing branch out in + THAT root, which is what the remedy says the hand merge needs first.""" + root = make_root(name) + git("branch", "shape/someone-elses", cwd=root) + refused = apply(standard, root, "--branch", "shape/someone-elses") + assert refused.returncode == 2, refused.stdout + refused.stderr + assert git("symbolic-ref", "--short", "HEAD", + cwd=root).stdout.strip() == "main" + + pasted = paste(checkout_remedy(refused.stderr), tmp_path) + assert pasted.returncode == 0, pasted.stderr + pasted.stdout + assert git("symbolic-ref", "--short", "HEAD", + cwd=root).stdout.strip() == "shape/someone-elses" + + +@pytest.mark.skipif(ON_WINDOWS, reason="a Windows root holds a backslash, " + "which `shlex` quotes") +def test_an_ordinary_root_prints_the_remedy_it_always_did(standard, make_root): + """Nothing is quoted that needs no quoting.""" + root = make_root("plain") + git("branch", "shape/someone-elses", cwd=root) + result = apply(standard, root, "--branch", "shape/someone-elses") + assert checkout_remedy(result.stderr) == ( + f"git -C {root.resolve()} checkout shape/someone-elses") + + +# --- the paths in the drift refusal and the two sentences `check` appends ---- +# +# A path a pin can carry is one the standard's copy lists name, so none holds +# an apostrophe or a `$` in real use; these are read in-process, with the +# printing functions handed the rows directly, so the quoting is held to every +# path a shell reads differently, and a real `sh` reads the quoted words back. + + +def local_edit(update_shape, path: str): + """A row for `path` as `classify` leaves a file the project edited.""" + row = update_shape.Row(path, "a" * 64) + row.state = update_shape.LOCALLY_MODIFIED + return row + + +def addition(update_shape, path: str): + """An `upstream-added` path the root has nothing at.""" + copy = update_shape.Copy(path, f"templates/assembly-root/{path}", False) + return update_shape.Addition(copy, False, b"added\n") + + +@pytest.mark.parametrize("path", PATHS) +def test_the_drift_refusal_names_each_path_as_one_argument(update_shape, path, + tmp_path): + other = "docs/other file.md" + rows = [local_edit(update_shape, path), local_edit(update_shape, other)] + with pytest.raises(Refusal) as raised: + update_shape._refuse_local_drift(rows, set()) + said = raised.value + assert said.code == "update-local-drift" + tail = said.remediation.split("re-run with ")[1] + assert words(tail) == ["--accept-local", path, "--accept-local", other] + # The unquoted paths are still listed, one a line, for the person to read. + assert f" {path}\n" in said.detail + "\n" + if ON_WINDOWS: + return + assert sh_reads(tail, tmp_path) == ["--accept-local", path, + "--accept-local", other] + + +def test_an_accepted_path_is_not_in_the_refusal(update_shape): + """Control: the paths the refusal asks about are the unaccepted ones.""" + rows = [local_edit(update_shape, "a b.md"), + local_edit(update_shape, "c d.md")] + with pytest.raises(Refusal) as raised: + update_shape._refuse_local_drift(rows, {"a b.md"}) + assert words(raised.value.remediation.split("re-run with ")[1]) == [ + "--accept-local", "c d.md"] + + +def backticked(sentence: str) -> str: + """The command the one pair of backticks in `sentence` holds.""" + found = re.findall(r"`([^`]*)`", sentence) + assert len(found) == 1, sentence + return found[0] + + +@pytest.mark.parametrize("path", PATHS) +def test_the_both_sentence_names_the_path_as_one_argument(update_shape, path, + tmp_path): + sentence = update_shape.accept_local_exit(path) + command = backticked(sentence) + assert words(command) == ["apply", "--accept-local", path] + assert command == f"apply --accept-local {shlex.quote(path)}" + if not ON_WINDOWS: + assert sh_reads(command, tmp_path) == ["apply", "--accept-local", + path] + + +@pytest.mark.parametrize("path", PATHS) +def test_the_locally_modified_sentence_names_the_path_as_one_argument( + update_shape, path, tmp_path): + sentence = update_shape.keep_local_exits(path) + command = backticked(sentence) + assert words(command) == ["apply", "--accept-local", path] + assert command == f"apply --accept-local {shlex.quote(path)}" + if not ON_WINDOWS: + assert sh_reads(command, tmp_path) == ["apply", "--accept-local", + path] + + +@pytest.mark.parametrize("path", PATHS) +def test_the_untaken_addition_note_names_the_path_as_one_argument( + update_shape, path, tmp_path, capsys): + update_shape._note_untaken_additions([addition(update_shape, path)], []) + printed = capsys.readouterr().out.splitlines() + assert len(printed) == 1, printed + said = printed[0] + assert said.startswith(f" note {path} is new upstream; not added; ") + took = said.split("; not added; ")[1] + assert words(took) == ["pass", "--add", path] + assert took == f"pass --add {shlex.quote(path)}" + if not ON_WINDOWS: + assert sh_reads(took, tmp_path) == ["pass", "--add", path] + + +def test_a_path_that_needs_no_quoting_prints_as_it_always_did(update_shape, + capsys): + """Nothing is quoted that needs no quoting, so every existing line a + plain path prints is byte for byte what it was.""" + path = "scripts/new-check.py" + assert update_shape.accept_local_exit(path).endswith( + "`apply --accept-local scripts/new-check.py` so the row is " + "recomputed from what you merged") + assert update_shape.keep_local_exits(path).endswith( + "on their word, `apply --accept-local scripts/new-check.py`") + update_shape._note_untaken_additions([addition(update_shape, path)], []) + assert capsys.readouterr().out == ( + " note scripts/new-check.py is new upstream; not added; " + "pass --add scripts/new-check.py\n") + rows = [local_edit(update_shape, path)] + with pytest.raises(Refusal) as raised: + update_shape._refuse_local_drift(rows, set()) + assert raised.value.remediation.endswith( + "re-run with --accept-local scripts/new-check.py") diff --git a/update-shape.py b/update-shape.py index 9fe07057..fb4473fb 100755 --- a/update-shape.py +++ b/update-shape.py @@ -1939,9 +1939,14 @@ def accept_local_exit(path: str) -> str: A DISPLAY-ONLY ADDITION, not folded into `row.detail` itself: the same field also renders inside `apply`'s `update-conflict` refusal, whose wording an unaccepted `both` must keep exactly as it was. + + THE PATH IS QUOTED (#532), though nothing pastes it for the person: it is + the flag they type next, and a path with a space is one argument only + when it is quoted, as the NEXT's every word is. """ return (f". Merge it by hand, commit it, then `apply --accept-local " - f"{path}` so the row is recomputed from what you merged") + f"{shlex.quote(path)}` so the row is recomputed from what you " + "merged") def keep_local_exits(path: str) -> str: @@ -1954,11 +1959,12 @@ def keep_local_exits(path: str) -> str: and the refusal that asks the human never fired. Brett Heap ruled on 2026-10-13, "Stop pre-filling it": the NEXT leaves the flag out, and the row's detail may still name it, as `accept_local_exit` does for `both`. - Display only, for the reason that function gives. + Display only, for the reason that function gives, and QUOTED like it + (#532). """ return (". Whether the project keeps the edit is the human's decision: " "revert it, carry it upstream, or, on their word, `apply " - f"--accept-local {path}`") + f"--accept-local {shlex.quote(path)}`") def report(root: Path, upstream: Upstream, pinned: str, target: str, @@ -2133,11 +2139,14 @@ def cmd_check(args) -> int: # Spelled out, but deliberately NOT folded into the NEXT command: # a file the standard has added lands in a project because a human # named it, and a command line that names it for them is the tool - # deciding. + # deciding. Each path is QUOTED (#532): `--add notes/shape + # notes.md` pasted as printed reaches `apply` as `--add + # notes/shape` and an argument argparse refuses. print("ADDED UPSTREAM: the standard has gained these since this " "pin, and `apply` writes none of them unless it is told to, " "one path at a time — " - + " ".join(f"--add {add.path}" for add in offered)) + + " ".join(f"--add {shlex.quote(add.path)}" + for add in offered)) # QUOTED WHOLE, THE LANE'S TRAILER INCLUDED, AND NEVER AN # `--accept-local` (#501): see `apply_command`. print("NEXT " + apply_command(root, upstream.label, target)) @@ -2467,10 +2476,12 @@ def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, "update-branch-exists", f"--branch {branch} already exists and is not the current " f"branch ({checked_out})", - "Remediation: check out that branch first — `git -C " - f"{root} checkout {branch}` — if that is where the hand " - "merge AGENTS.md describes was already committed, or pass " - "a different --branch to create a new one.") + # THE ROOT IS QUOTED (#532), by `_by_hand`: bare, a root under + # `my projects/` pasted as `git -C .../my`, exit 128. + "Remediation: check out that branch first — " + f"`{_by_hand(root, 'checkout', branch)}` — if that is where " + "the hand merge AGENTS.md describes was already committed, " + "or pass a different --branch to create a new one.") ledger.step = f"creating the branch {branch}" ledger.new_branch(branch) if added: @@ -3569,6 +3580,8 @@ def _refuse_local_drift(rows: list[Row], accepted: set) -> None: """ unaccepted = [row for row in rows if row.state == LOCALLY_MODIFIED and row.path not in accepted] + # Each `--accept-local` path is QUOTED (#532), so the flag a person types + # next carries a path with a space whole. if unaccepted: raise Refusal( "update-local-drift", @@ -3579,7 +3592,8 @@ def _refuse_local_drift(rows: list[Row], accepted: set) -> None: "which is the fork recorded as the standard.", "Remediation: revert each edit, or carry it upstream, or — " "having decided the project keeps it — re-run with " - + " ".join(f"--accept-local {row.path}" for row in unaccepted)) + + " ".join(f"--accept-local {shlex.quote(row.path)}" + for row in unaccepted)) def _note_untaken_additions(additions: list[Addition], @@ -3592,11 +3606,13 @@ def _note_untaken_additions(additions: list[Addition], the human which files the standard has gained and what to pass to take one. An addition the root already HAS a file at is silent here for the reason `Addition` gives, and one under a file the root keeps says what - is in the way rather than name an `--add` that is refused (#285). + is in the way rather than name an `--add` that is refused (#285). The + path it tells a person to pass is QUOTED (#532). """ for add in additions: if not add.present and add not in taking: - take = (f"pass --add {add.path}" if add.blocked is None else + take = (f"pass --add {shlex.quote(add.path)}" + if add.blocked is None else f"{add.blocked} in this root is not a directory, so " f"--add refuses it until {add.blocked} is moved") dead = f" ({add.left_dead})" if add.left_dead else ""