diff --git a/tests/test_update_shape_apply_lines_and_headers.py b/tests/test_update_shape_apply_lines_and_headers.py new file mode 100644 index 00000000..07743e86 --- /dev/null +++ b/tests/test_update_shape_apply_lines_and_headers.py @@ -0,0 +1,586 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`update-shape.py apply` prints no line that fails as printed, and finds the +pin's `files:` and the manifest's `shape:` headers wherever the reader finds +them (#674). + +FOUR CASES, each measured on main before the fix: + + * THE ROLLBACK LINE. A validator that deleted `contracts/shape-pin.yaml` + and left `contracts/` at 0555 was handed the bare `git cat-file blob + > `, and pasted, that fails `Permission denied`: the directory the + redirect creates the file in will not take it. The line now says `so make + writable, then:` first, naming `_nearest_directory`'s + answer, which is the root itself where `contracts/` went too. A directory + that exists over an absent file is the case #560's tests left out (its + `os.path.lexists` choice, #458's 6192496098). + * THE `committed on :` LINE named `paths`, which always holds the + pin and the manifest, so a run that drops a merged-away row, or keeps a + copy with `--accept-local` at the pinned commit, said `project.yaml` of a + commit without it. It names what git says the commit holds now. + * A BRANCH NAMED LIKE A TAG. In a root tagged `v1.0.0`, `--branch v1.0.0 + --push` committed and died at the push, `src refspec v1.0.0 matches more + than one`, and both printed push lines failed the same way pasted. They + name the branch in full now, `refs/heads/:refs/heads/`. And + `current_branch` read `heads/v1.0.0` there (`--short`), so a root ON that + branch was refused `update-branch-exists`, and a `--pr` re-run on it said + "nothing to do" and opened nothing. + * A QUOTED HEADER. `"files":`, `'files':` and `files :` are the key `files` + to the reader and to both validators, so `check` sent the person to an + `apply` that refused `update-pin-unrecognised`, and `"shape":` got + `update-manifest-no-shape`, "has no `shape:` block". The headers are found + with the reader's own `_split_key` now, and written back as they were. + +THE HARNESSES ARE THE NEIGHBOURS'. +`tests/test_update_shape_partial_rollback.py` gives the upstream (A, B above +it, C above that), the project cut at A into local bare remotes, and the +ledger run in process; `tests/test_pin_rewrite_layout.py` the pins the +scaffold renders; `tests/test_update_shape_landing.py` its `gh` stub; and +`tests/test_update_shape_merged_away.py` the adoption that makes the issue's +own merged-away row. No network is used and no real `gh` +is ever run. Every push goes to a bare remote of the test's own, so no test +moves another's, and every pasted push is run as the argv `sh` would read, +without a shell, so that half runs on every platform. +""" + +from __future__ import annotations + +import errno +import os +import re +import shlex +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import REPO, WINDOWS_SKIP, git, run_script +from test_pin_rewrite_layout import MANIFEST as MANIFEST_TEXT +from test_pin_rewrite_layout import (NEW_COMMIT, NEW_ROWS, NEW_TREE, + OLD_COMMIT, OLD_ROWS, OLD_TREE, make_pin, + read_back) +from test_shape_doctor import standard as doctor_standard # noqa: F401 +from test_update_shape import next_line_of +from test_update_shape_landing import URL, gh, listing # noqa: F401 +from test_update_shape_merged_away import (MERGED, SOURCE_TREE, + adopt_and_clone, merge_away) +from test_update_shape_partial_rollback import ( # noqa: F401 + AGENTS, BRANCH, CHANGED, DENIED, MANIFEST, PIN, apply_args, + assert_reported_as_red, held, left, put_back, refused_by, root, standard, + update_shape, +) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import Refusal, parse_yaml # noqa: E402 + +UPDATE = REPO / "update-shape.py" + +#: The branch the issue names, and the tag beside it that takes its name. +TAGGED = "v1.0.0" +#: That branch as all three push lines now name it. +FULL = f"refs/heads/{TAGGED}:refs/heads/{TAGGED}" + + +def apply(standard, root: Path, at: str, *extra: str, env=None): + """`apply --yes` at `standard[at]`, through the real tool.""" + return run_script(UPDATE, *apply_args(standard, root, at, *extra), + env=env) + + +def head_of(path: Path) -> str: + return git("rev-parse", "HEAD", cwd=path).stdout.strip() + + +# --------------------------------------------------------------------------- +# A header the reader takes is one the rewrite finds +# --------------------------------------------------------------------------- + +#: Each spelling of a header the reader reads as the bare key, `{key}` for +#: the key: quoted either way, a space before the colon, both, and a quoted +#: one with a trailing comment (#437's half of the same rule). +SPELLINGS = { + "double-quoted": '"{key}":', + "single-quoted": "'{key}':", + "space-before-colon": "{key} :", + "quoted-and-spaced": '"{key}" :', + "quoted-with-comment": '"{key}": # one row per copied file', +} + + +def with_header(text: str, key: str, spelling: str) -> str: + """`text` with its one column-0 `:` header spelled `spelling`.""" + marker = f"\n{key}:\n" + assert text.count(marker) == 1, f"fixture: one `{key}:` header" + return text.replace(marker, "\n" + spelling.format(key=key) + "\n") + + +def manifest_text(spelling: str, commit: str, tree: str) -> str: + """`tests/test_pin_rewrite_layout.py`'s manifest, its `shape:` header + spelled `spelling`: a leg's own `commit:` above the block, and a key + after it, so a rewrite that ran past the block would show.""" + return with_header(MANIFEST_TEXT.format(member="e" * 40, commit=commit, + tree=tree), "shape", spelling) + + +@pytest.mark.parametrize("kind", ["assembly shape pin", "family shape pin"]) +@pytest.mark.parametrize("spelling", SPELLINGS.values(), ids=SPELLINGS) +def test_a_files_header_the_reader_takes_is_rewritten_as_it_was( + update_shape, kind, spelling): + """THE ISSUE'S PIN CASE. The reader gives each spelling a `files` key, so + the rewrite finds it: the result is the scaffold's own rendering of the + target with the header spelled as it was, and the reader takes it + back, rows and all.""" + before = with_header(make_pin(kind, OLD_COMMIT, OLD_TREE, OLD_ROWS), + "files", spelling) + assert read_back(before) == (OLD_COMMIT, OLD_TREE) + assert [row["path"] for row in parse_yaml(before)["files"]] == [ + path for path, _ in OLD_ROWS], "fixture: the reader takes the key" + + new = update_shape.rewrite_shape_pin( + before, NEW_COMMIT, NEW_TREE, + [update_shape.Row(path, digest) for path, digest in NEW_ROWS]) + + assert new == with_header(make_pin(kind, NEW_COMMIT, NEW_TREE, NEW_ROWS), + "files", spelling) + assert read_back(new) == (NEW_COMMIT, NEW_TREE) + assert [row["path"] for row in parse_yaml(new)["files"]] == [ + path for path, _ in NEW_ROWS] + + +@pytest.mark.parametrize("kind_name", ["PROJECT_KIND", "FAMILY_KIND"]) +@pytest.mark.parametrize("spelling", SPELLINGS.values(), ids=SPELLINGS) +def test_a_shape_header_the_reader_takes_is_found_and_kept( + update_shape, kind_name, spelling): + """THE ISSUE'S MANIFEST CASE, for `project.yaml` and for a holder's + `family.yaml`, which goes through the same function. It was refused + `update-manifest-no-shape`, saying the manifest "has no `shape:` block" + of one the reader gives a `shape` key. Only the block's two values + move; the header stays as it was spelled.""" + before = manifest_text(spelling, OLD_COMMIT, OLD_TREE) + assert parse_yaml(before)["shape"]["commit"] == OLD_COMMIT, ( + "fixture: the reader takes the key") + + new = update_shape.rewrite_manifest(before, NEW_COMMIT, NEW_TREE, + getattr(update_shape, kind_name)) + + assert new == manifest_text(spelling, NEW_COMMIT, NEW_TREE) + shape = parse_yaml(new)["shape"] + assert (shape["commit"], shape["digests"]["tree_sha256"]) == ( + NEW_COMMIT, NEW_TREE) + + +@pytest.mark.parametrize("quoted_first", [True, False], + ids=["quoted-first", "quoted-last"]) +def test_a_quoted_shape_header_beside_a_bare_one_is_the_key_written_twice( + update_shape, quoted_first): + """The reader reads `"shape":` and `shape:` as ONE key written twice and + keeps the last block, so this finder, asking the reader's question, sees + two headers and refuses by name, as #558 refuses a bare `shape:` twice. + The bare-only finder saw one: with the bare header first it rewrote a + block nobody reads, and the read-back blamed "this tool's line + rewrite".""" + extra = 'shape:\n commit: "{0}"\n digests:\n tree_sha256: "{1}"\n' + text = (manifest_text("shape:", OLD_COMMIT, OLD_TREE) + + extra.format(OLD_COMMIT, OLD_TREE)) + lines = text.splitlines() + first, last = [index for index, line in enumerate(lines) + if line == "shape:"] + quoted = first if quoted_first else last + lines[quoted] = '"shape":' + text = "\n".join(lines) + "\n" + assert parse_yaml(text)["shape"]["commit"] == OLD_COMMIT + + with pytest.raises(Refusal) as caught: + update_shape.rewrite_manifest(text, NEW_COMMIT, NEW_TREE, + update_shape.PROJECT_KIND) + + assert caught.value.code == "update-manifest-unrecognised" + assert caught.value.detail.startswith( + f"project.yaml holds the `shape:` key twice, at lines {first + 1} " + f"and {last + 1}, where exactly one was expected: the reader keeps " + "the last block"), caught.value.detail + assert "line rewrite" not in caught.value.detail + caught.value.remediation + + +@pytest.mark.parametrize("header", [ + pytest.param(' "files":', id="indented"), + pytest.param('"files":#rows', id="comment-glued-on"), + pytest.param('"files" rows:', id="quoted-key-then-text"), + pytest.param("files[0]:", id="bracket-in-key"), +]) +def test_a_line_that_is_no_files_header_to_the_reader_is_still_counted_none( + update_shape, header): + """The column and the rest of the line are the reader's too. Indented, + or with a comment glued on, the line is no top-level `files` key, and + the last two are lines `_split_key` refuses, as the reader refuses the + file holding one: here each is no header, never a traceback, and the + rewrite's own count refuses, before any byte is written (it only + returns text).""" + before = with_header(make_pin("assembly shape pin", OLD_COMMIT, OLD_TREE, + OLD_ROWS), "files", header) + + rows = [update_shape.Row(path, digest) for path, digest in NEW_ROWS] + + with pytest.raises(Refusal) as caught: + update_shape.rewrite_shape_pin(before, NEW_COMMIT, NEW_TREE, rows) + + assert caught.value.code == "update-pin-unrecognised" + assert "has 0 `files` line(s) where exactly one was expected" in \ + caught.value.detail + + +def respell_the_header(path: Path, key: str, spelling: str) -> None: + """`path`'s column-0 `:` line spelled `spelling`, in BYTES, keeping a + CR where the file has one, so no newline translation is compared.""" + data, count = re.subn( + b"(?m)^" + key.encode() + b":(\\r?)$", + lambda found: spelling.format(key=key).encode() + found.group(1), + path.read_bytes(), count=1) + assert count == 1, f"fixture: one `{key}:` header in {path}" + path.write_bytes(data) + + +@pytest.mark.parametrize("key, path", [("files", PIN), ("shape", MANIFEST)], + ids=["pin", "manifest"]) +def test_check_and_apply_agree_on_a_quoted_header(standard, root, key, path): + """THE ISSUE'S RUN, end to end. Both validators pass the quoted header + and `check` sends the person to `apply`, which now takes the update: + exit 0, the header written back quoted, the pin moved to B, and the + project's own validators green.""" + respell_the_header(root / path, key, '"{key}":') + for validator in ("validate-pins.py", "validate-manifest.py"): + green = run_script(root / "scripts" / validator, cwd=root) + assert green.returncode == 0, green.stdout + green.stderr + checked = run_script(UPDATE, "check", "--root", str(root), "--upstream", + str(standard["upstream"]), "--at", standard["b"]) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert "update-shape.py apply " in next_line_of(checked.stdout) + + result = apply(standard, root, "b") + + assert result.returncode == 0, result.stdout + result.stderr + assert "update-pin-unrecognised" not in result.stderr + assert "update-manifest-no-shape" not in result.stderr + assert f'\n"{key}":\n' in (root / path).read_text(encoding="utf-8") + assert parse_yaml((root / PIN).read_text(encoding="utf-8"))[ + "commit"].lower() == standard["b"] + for validator in ("validate-pins.py", "validate-manifest.py"): + green = run_script(root / "scripts" / validator, cwd=root) + assert green.returncode == 0, green.stdout + green.stderr + + +# --------------------------------------------------------------------------- +# `committed on :` names what the commit holds +# --------------------------------------------------------------------------- + + +def assert_said_what_it_committed(result, root: Path, branch: str, + holds: list[str]) -> None: + """The run exits 0, its commit holds exactly `holds`, as `git show` + lists them, and its `committed on` line names those and no others.""" + assert result.returncode == 0, result.stdout + result.stderr + assert git("show", "--name-only", "--format=", "HEAD", + cwd=root).stdout.split() == holds + said = [line.strip() for line in result.stdout.splitlines() + if line.strip().startswith("committed on ")] + assert said == [f"committed on {branch}: " + ", ".join(holds)], said + + +def test_a_manifest_rewritten_with_its_own_bytes_is_not_said_committed( + standard, root): + """`--accept-local` AT THE COMMIT THE PIN NAMES: the kept copy's row is + recomputed from the project's bytes, so the pin changes, and the + manifest's `shape:` block is rewritten with the bytes it had. The line + said `contracts/shape-pin.yaml, project.yaml`; the commit holds the pin + alone, and the line says so.""" + local = root / "scripts" / "bootstrap.py" + local.write_bytes(local.read_bytes() + b"# A local edit, kept.\n") + git("commit", "-q", "-m", "A local edit", "--", "scripts/bootstrap.py", + cwd=root) + + result = apply(standard, root, "a", "--accept-local", + "scripts/bootstrap.py", "--branch", BRANCH) + + assert_said_what_it_committed(result, root, BRANCH, [PIN]) + + +@pytest.fixture(scope="module") +def adopted(doctor_standard, tmp_path_factory) -> dict: + """The issue's own input: the source + `tests/test_update_shape_merged_away.py` adopts, with a `.gitignore` and + a `Makefile` of its own, so the shape's copies of both are pinned under + `shape/`.""" + return adopt_and_clone(doctor_standard, + tmp_path_factory.mktemp("apply-lines-merged"), + SOURCE_TREE) + + +@pytest.mark.skipif( + shutil.which("git-filter-repo") is None, + reason="git filter-repo is not installed: `pip install git-filter-repo`") +def test_a_run_that_only_drops_merged_away_rows_says_it_committed_the_pin( + doctor_standard, adopted, tmp_path): + """THE ISSUE'S MEASUREMENT (AT-scaffold N9, #458's 6172776355): both + `shape/` copies merged into the files they collided with, and `apply + --branch` at the commit the pin already names drops their rows. The + manifest is rewritten with the bytes it had; the line said + `contracts/shape-pin.yaml, project.yaml`.""" + root = tmp_path / "Medx" + shutil.copytree(adopted["clone"], root, symlinks=True) + merge_away(root, *MERGED.values()) + branch = "shape/update-merged-away" + + result = run_script(UPDATE, "apply", "--root", str(root), "--upstream", + str(doctor_standard), "--at", + head_of(doctor_standard), "--yes", "--branch", + branch) + + for path in MERGED: + assert f" dropped {path} " in result.stdout, result.stdout + assert_said_what_it_committed(result, root, branch, [PIN]) + + +# --------------------------------------------------------------------------- +# A branch that shares its name with a tag +# --------------------------------------------------------------------------- + + +def tagged(root: Path, tmp_path: Path) -> Path: + """`root` with a tag `v1.0.0` at HEAD, pushing to an EMPTY bare remote of + this test's own: the bare remote.""" + git("tag", TAGGED, cwd=root) + remote = tmp_path / "own-remote.git" + git("init", "-q", "--bare", str(remote), cwd=tmp_path) + git("remote", "set-url", "origin", str(remote), cwd=root) + return remote + + +def on_the_remote(remote: Path) -> str | None: + """What `remote` holds at the BRANCH `v1.0.0`, or None.""" + proc = git("rev-parse", "--verify", "--quiet", f"refs/heads/{TAGGED}", + cwd=remote, check=False) + return proc.stdout.strip() or None + + +def push_half(line: str) -> list[str]: + """The `git ... push` a NEXT line opens with, as the argv `sh` reads.""" + words = shlex.split(line.removeprefix("NEXT ")) + return words[:words.index("&&")] + + +def test_a_branch_named_like_a_tag_is_pushed_beside_it(standard, root, + tmp_path): + """THE ISSUE'S RUN: `--branch v1.0.0 --push` in a root tagged `v1.0.0`. + It refused `pushing v1.0.0`, exit 2, after the commit. The branch is + pushed now, the tag stays out of it, and `-u` still sets the + upstream.""" + remote = tagged(root, tmp_path) + + result = apply(standard, root, "b", "--branch", TAGGED, "--push") + + assert result.returncode == 0, result.stdout + result.stderr + assert f"pushed {TAGGED} to origin" in result.stdout + assert on_the_remote(remote) == head_of(root) + assert git("for-each-ref", "refs/tags", cwd=remote).stdout == "" + assert git("config", f"branch.{TAGGED}.merge", + cwd=root).stdout.strip() == f"refs/heads/{TAGGED}" + + +def test_the_branch_next_line_pushes_a_branch_named_like_a_tag(standard, root, + tmp_path): + """`--branch` alone ends on the push and the pull request. The push + named the branch bare, and pasted it failed as the run's own did. It + names it in full, and run as printed it pushes.""" + remote = tagged(root, tmp_path) + + result = apply(standard, root, "b", "--branch", TAGGED) + + assert result.returncode == 0, result.stdout + result.stderr + push = push_half(next_line_of(result.stdout)) + assert push == ["git", "-C", str(root), "push", "-u", "origin", FULL] + pasted = subprocess.run(push, capture_output=True, text=True, check=False) + assert pasted.returncode == 0, pasted.stdout + pasted.stderr + assert on_the_remote(remote) == head_of(root) + + +def test_a_failed_push_of_a_branch_named_like_a_tag_prints_one_that_works( + standard, root, tmp_path): + """`landing_failed`'s NEXT line, the other printed push: after a push + origin refused, it pushes before it opens the pull request. Named in + full there too, it pushes once origin will take it.""" + remote = tagged(root, tmp_path) + git("remote", "set-url", "--push", "origin", + str(tmp_path / "no-such-remote.git"), cwd=root) + + result = apply(standard, root, "b", "--branch", TAGGED, "--push") + + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED pushing {TAGGED}" in result.stderr + assert f"git push -q -u -- origin {FULL}" in result.stderr + push = push_half(next_line_of(result.stdout)) + assert push == ["git", "-C", str(root), "push", "-u", "--", "origin", + FULL] + git("remote", "set-url", "--push", "origin", str(remote), cwd=root) + pasted = subprocess.run(push, capture_output=True, text=True, check=False) + assert pasted.returncode == 0, pasted.stdout + pasted.stderr + assert on_the_remote(remote) == head_of(root) + + +def test_apply_on_a_branch_named_like_a_tag_commits_onto_it(standard, root, + tmp_path): + """`--branch` naming THE CURRENT BRANCH commits onto it: the hand-merge + posture AGENTS.md describes. On `v1.0.0` beside a tag `v1.0.0` it was + refused `update-branch-exists`, "('heads/v1.0.0' is checked out)", + because `--short` spelled the branch so.""" + tagged(root, tmp_path) + git("switch", "-q", "-c", TAGGED, cwd=root) + before = head_of(root) + + result = apply(standard, root, "b", "--branch", TAGGED) + + assert result.returncode == 0, result.stdout + result.stderr + assert "update-branch-exists" not in result.stderr + assert git("symbolic-ref", "HEAD", + cwd=root).stdout.strip() == f"refs/heads/{TAGGED}" + assert git("rev-parse", "HEAD~1", cwd=root).stdout.strip() == before + + +def test_current_branch_is_the_branch_git_has_checked_out(update_shape, root, + tmp_path): + """In process: the branch beside a tag of its name, a branch whose name + merely looks like a full refname (`refs/heads/x`, which git nests), and + a detached HEAD, which is on no branch.""" + tagged(root, tmp_path) + git("switch", "-q", "-c", TAGGED, cwd=root) + assert update_shape.current_branch(root) == TAGGED + git("switch", "-q", "-c", "refs/heads/x", cwd=root) + assert update_shape.current_branch(root) == "refs/heads/x" + git("switch", "-q", "--detach", cwd=root) + assert update_shape.current_branch(root) is None + + +def test_a_pr_rerun_on_a_branch_named_like_a_tag_opens_the_missing_one( + standard, root, tmp_path, gh): + """#204's re-run, on `v1.0.0` beside its tag: the first run commits and + pushes, and its `gh pr create` fails; the same command again finds + nothing to write and opens that pull request. `_awaits_its_pull_request` + compared `--short`'s `heads/v1.0.0` with the branch, so the re-run said + "nothing to do" and opened nothing.""" + remote = tagged(root, tmp_path) + flags = ("--branch", TAGGED, "--push", "--pr") + gh.failing(True) + first = apply(standard, root, "b", *flags, env=gh.env) + assert first.returncode == 2, first.stdout + first.stderr + assert f"REFUSED opening the pull request for {TAGGED}" in first.stderr + create = gh.calls()[0] + assert on_the_remote(remote) == head_of(root) + + gh.failing(False) + gh.forget() + second = apply(standard, root, "b", *flags, env=gh.env) + + assert second.returncode == 0, second.stdout + second.stderr + assert "nothing to do" not in second.stdout + assert f"pull request {URL}" in second.stdout + assert gh.calls() == [listing(TAGGED), create] + + +# --------------------------------------------------------------------------- +# A rollback line for a file a validator deleted under a shut directory +# --------------------------------------------------------------------------- + + +def red_after_removing(update_shape, monkeypatch, gone: Path, + shut: Path) -> None: + """The project's validators go red, having removed `gone` (a file, or a + directory with what is in it) and left `shut` at 0555.""" + def red(_root, _script): + if gone.is_dir(): + shutil.rmtree(gone) + elif gone.exists(): + gone.unlink() + shut.chmod(0o555) + return 1 + + monkeypatch.setattr(update_shape, "run_validator", red) + + +def blob_of(root: Path, data: bytes) -> str: + """The id `data` has in `root`'s object store.""" + return subprocess.run(["git", "hash-object", "--stdin"], cwd=str(root), + input=data, capture_output=True, + check=True).stdout.decode("ascii").strip() + + +def follow(line: str, said: str, opened: Path, root: Path, + original: bytes) -> None: + """`line` is exactly `said` and then the command, which fails while + `opened` is shut — the reason the line names it — and, once `opened` is + made writable as the line says, puts `original` back at the pin.""" + assert line.startswith(said), line + command = line[len(said):] + shut = subprocess.run(["sh", "-c", command], capture_output=True, + check=False) + assert shut.returncode != 0, "the line's command ran with it shut" + opened.chmod(0o755) + subprocess.run(["sh", "-c", command], check=True, capture_output=True) + assert (root / PIN).read_bytes() == original + + +@WINDOWS_SKIP +def test_a_pin_deleted_under_a_shut_directory_is_told_to_open_it_first( + update_shape, root, standard, monkeypatch): + """THE ISSUE'S CASE (R560v's probe d7): the pin deleted, `contracts/` + left at 0555. A checkout would unlink and create, so the line is the + `cat-file` redirect, and a redirect creates the file in a directory that + will not take it: the line now says to make `contracts/` writable first, + by the path it has, and never "make it writable" of a file that is not + there. Followed, it puts the pin's bytes back.""" + contracts = root / "contracts" + original = held(root)[PIN] + red_after_removing(update_shape, monkeypatch, root / PIN, contracts) + try: + refusal = refused_by(update_shape, standard, root, "b") + if f"restored {PIN}" in refusal.detail: # pragma: no cover + pytest.skip("this user is not stopped by permission bits") + assert_reported_as_red(refusal) + assert put_back(refusal.detail) == [ + f"restored {path}" for path in (CHANGED, AGENTS, MANIFEST)] + (line,) = left(refusal.detail) + kept = blob_of(root, original) + follow(line, f"file {PIN} was not put back ({DENIED}); its bytes " + f"before this run are kept as blob {kept[:12]} in this " + f"repository, so make {contracts} writable, then: ", + contracts, root, original) + assert "make it writable" not in line + finally: + contracts.chmod(0o755) + + +@WINDOWS_SKIP +def test_a_directory_deleted_under_a_shut_root_names_the_root( + update_shape, root, standard, monkeypatch): + """THE DIRECTORY NAMED IS THE NEAREST ONE THAT EXISTS: a validator that + deleted `contracts/` and left the ROOT at 0555 gets the `mkdir -p` the + redirect needs, and the line says to make the root writable, which is + where `mkdir` writes, not `contracts/`, which is not there.""" + contracts = root / "contracts" + original = held(root)[PIN] + red_after_removing(update_shape, monkeypatch, contracts, root) + try: + refusal = refused_by(update_shape, standard, root, "b") + if f"restored {PIN}" in refusal.detail: # pragma: no cover + pytest.skip("this user is not stopped by permission bits") + assert_reported_as_red(refusal) + (line,) = left(refusal.detail) + kept = blob_of(root, original) + said = (f"file {PIN} was not put back " + f"({os.strerror(errno.ENOENT)}); its bytes before this run " + f"are kept as blob {kept[:12]} in this repository, so make " + f"{root} writable, then: ") + assert line[len(said):].startswith( + f"mkdir -p {shlex.quote(str(contracts))} && "), line + follow(line, said, root, root, original) + finally: + root.chmod(0o755) diff --git a/tests/test_update_shape_header_comments.py b/tests/test_update_shape_header_comments.py index 3b027e52..61200ba0 100644 --- a/tests/test_update_shape_header_comments.py +++ b/tests/test_update_shape_header_comments.py @@ -13,9 +13,10 @@ would have given the manifest a second `shape:` key. Both headers are now found with the reader's own `_strip_comment`, at column -0, and the line is written back as it was. That is the whole claim: the -reader's rule for a comment, and nothing about how else a key may be spelled -(a quoted `"files":` or `files :` is not found, as it was not before). +0, and the line is written back as it was. That is this file's claim: the +reader's rule for a comment. The reader's rule for a key came after, in #674: +a quoted `"files":` or `files :` is found too, which +`tests/test_update_shape_apply_lines_and_headers.py` holds. TWO LEVELS, as the issue's reproduction asks for the second and #380's tests for the first: the rewrites called in-process on the text the scaffold diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index a273212d..60133a19 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -61,6 +61,9 @@ UPDATE = REPO / "update-shape.py" BRANCH = "shape/update-landing" +#: How every push names BRANCH, the run's and both NEXT lines': in full +#: on both sides, so a tag of its name cannot take it (#674). +PUSHED = f"refs/heads/{BRANCH}:refs/heads/{BRANCH}" PIN = "contracts/shape-pin.yaml" #: The copied file upstream commit C changes, and only C (#333). PINS = "scripts/validate-pins.py" @@ -291,7 +294,7 @@ def test_a_failed_pull_request_on_an_unpushed_branch_pushes_first( assert len(calls) == 1, calls assert calls[0][:2] == ["pr", "create"], calls push = shlex.join(["git", "-C", str(root.path), "push", "-u", "--", - "origin", BRANCH]) + "origin", PUSHED]) assert next_line_of(result.stdout) == ( f"NEXT {push} && " + shlex.join(["gh", *calls[0]])) the_commit_stands(root, project) @@ -404,8 +407,8 @@ def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, result = apply(root, project, "--branch", BRANCH, *flags) assert result.returncode == 2, result.stdout + result.stderr assert f"REFUSED pushing {BRANCH}" in result.stderr - assert f"git push -q -u -- origin {BRANCH}" in result.stderr, ( - "`--` ends git's options before the branch") + assert f"git push -q -u -- origin {PUSHED}" in result.stderr, ( + "`--` ends git's options before the branch, named in full") assert "committing the re-synced shape" not in result.stderr assert "origin does not have it yet" in result.stderr assert "nothing was rolled back" in result.stderr @@ -414,7 +417,7 @@ def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, line = next_line_of(result.stdout) push = shlex.join(["git", "-C", str(root.path), "push", "-u", "--", - "origin", BRANCH]) + "origin", PUSHED]) assert line.startswith(f"NEXT {push} && gh pr create "), line if "--pr" in flags: assert f"--repo testorg/{PROJECT} --base main --head {BRANCH} " \ diff --git a/tests/test_update_shape_next_quoting.py b/tests/test_update_shape_next_quoting.py index 84b7fe7f..422c9eaf 100644 --- a/tests/test_update_shape_next_quoting.py +++ b/tests/test_update_shape_next_quoting.py @@ -261,7 +261,8 @@ def test_the_branch_next_line_is_one_argv(project, make_root, name): assert result.returncode == 0, result.stdout + result.stderr title, body = pr_words(project) assert argv_of(next_line_of(result.stdout)) == [ - "git", "-C", str(root.resolve()), "push", "-u", "origin", branch, + "git", "-C", str(root.resolve()), "push", "-u", "origin", + f"refs/heads/{branch}:refs/heads/{branch}", "&&", "gh", "pr", "create", "--repo", f"{ORG}/{PROJECT}", "--base", "main", "--head", branch, "--title", title, "--body", body] @@ -337,7 +338,8 @@ def test_the_branch_is_quoted_even_where_its_alphabet_makes_that_a_no_op( words = argv_of(capsys.readouterr().out.strip()) assert words[words.index("--head") + 1] == branch, words if not push: - assert words[words.index("push") + 3] == branch, words + assert words[words.index("push") + 3] == ( + f"refs/heads/{branch}:refs/heads/{branch}"), words # --- a plain root prints what it always printed ----------------------------- @@ -351,8 +353,10 @@ def test_the_branch_is_quoted_even_where_its_alphabet_makes_that_a_no_op( def test_a_plain_root_prints_the_bytes_it_always_printed(project, make_root): """THE LINES BEFORE #325, spelled out by hand: for a root and an upstream that need no quoting, not one byte of any of the three lines moved. The - one change since is on purpose: the `gh pr create` the branch line ends - with carries `--title` and `--body` (#535), after everything it had.""" + two changes since are on purpose: the `gh pr create` the branch line ends + with carries `--title` and `--body` (#535), after everything it had, and + its push names the branch in full, `refs/heads/:refs/heads/`, so a + tag of the same name cannot take it (#674).""" root = make_root("plain") for path in (root.resolve(), project["upstream"].resolve()): assert shlex.quote(str(path)) == str(path), ( @@ -373,7 +377,8 @@ def test_a_plain_root_prints_the_bytes_it_always_printed(project, make_root): assert branched.returncode == 0, branched.stdout + branched.stderr title, body = pr_words(project) assert next_line_of(branched.stdout) == ( - f"NEXT git -C {pushing.resolve()} push -u origin {branch} && " + f"NEXT git -C {pushing.resolve()} push -u origin " + f"refs/heads/{branch}:refs/heads/{branch} && " f"gh pr create --repo {ORG}/{PROJECT} --base main --head {branch} " f"--title {shlex.quote(title)} --body {shlex.quote(body)}") @@ -454,7 +459,8 @@ def test_the_commands_apply_prints_run_when_they_are_pasted(project, make_root, @pytest.mark.parametrize("name", PASTED) def test_the_branch_line_pushes_and_opens_when_it_is_pasted(project, make_root, tmp_path, name): - """`git -C push -u origin && gh pr create ...`: the push + """`git -C push -u origin refs/heads/:refs/heads/ + && gh pr create ...` (#674 names the branch in full): the push has to reach the root's own remote for the `&&` to let `gh` run, so a `gh` that ran at all is the proof that the root arrived whole.""" root = make_root(name) diff --git a/update-shape.py b/update-shape.py index bc6eb703..63154d3e 100755 --- a/update-shape.py +++ b/update-shape.py @@ -168,8 +168,9 @@ # The reader's own rule for where a comment starts, by its private name on # purpose: `shape_materialize` imports it the same way and says why (#380). A # block header is found with it, so a trailing comment is one here wherever it -# is one to the reader (#437). -from repo_shape import _strip_comment # noqa: E402 +# is one to the reader (#437), and with the reader's rule for a key, so a +# quoted key or a space before the colon is one here too (#674). +from repo_shape import _split_key, _strip_comment # noqa: E402 from shape_materialize import ( # noqa: E402 ATTRIBUTES, COPIED_FROM_SHAPE, COPIED_VERBATIM, FAMILY_COPIED_FROM_SHAPE, FAMILY_COPIED_VERBATIM, SHAPE_REPOSITORY, CommandFailed, check_program, @@ -1456,8 +1457,26 @@ def _is_block_header(line: str, key: str) -> bool: `# comment` aside? Asked the reader's way, `_strip_comment`, so a comment that a quoted scalar holds is not one, and the `#` a comment starts at is one only after a space or a tab (#437). A header indented - at all is not this one: the reader's own top-level keys start at 0.""" - return _strip_comment(line) == f"{key}:" + at all is not this one: the reader's own top-level keys start at 0. + + AND THE KEY IS READ THE READER'S WAY TOO (#674), by `_split_key`, so + `"files":`, `'files':` and `files :` are the header `files`, as + `load_yaml` and both validators read them. This asked for the exact + text `files:`, so `check` passed such a pin, sent the person to + `apply`, and `apply` refused it `update-pin-unrecognised` ("0 `files` + line(s)"), or a manifest `update-manifest-no-shape`, saying it has no + `shape:` block when the reader gives it one. The column is the + reader's: no space in front, and no tab among the blanks in front, + which the reader refuses outright; the text is `.strip()`ped first, + as `_tokenise` strips it. A line `_split_key` refuses is one the + reader refuses as well, so it is no header here rather than a + traceback, and the count that follows says so.""" + if line.startswith(" ") or "\t" in line[:len(line) - len(line.lstrip())]: + return False + try: + return _split_key(_strip_comment(line.strip())) == (key, "") + except YamlError: + return False def _header_as_it_was(line: str) -> str: @@ -1494,10 +1513,11 @@ def rewrite_shape_pin(text: str, commit: str, tree: str, trailing `# comment`. It was `line.rstrip() == "files:"`, so a header that carried a comment, which the reader and both validators accept, was counted as no `files` line and refused as `update-pin-unrecognised`, - after `check` had sent the person to this `apply`. That is ALL this - claims: a header spelled with a quoted key or a space before the colon - is read as the key by the reader and is not found here, as before. The - header is written back AS THE LINE WAS, comment and the spacing in front + after `check` had sent the person to this `apply`. AND WITH THE READER'S + RULE FOR A KEY (#674): a header spelled with a quoted key or a space + before the colon, `"files":` or `files :`, is read as the key by the + reader, and is found here now too (`_is_block_header`). The header is + written back AS THE LINE WAS, quotes, comment and the spacing in front of it included, like the two lines above; only the rows under it are written whole. @@ -1644,9 +1664,12 @@ def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: was `line.rstrip() == "shape:"`, so `shape: # mirrors the pin`, which both validators accept, was refused as having NO `shape:` block, and the remedy, "add it", would have given the manifest a second `shape:` key. - That is ALL this claims: a quoted key or a space before the colon is - read as the key by the reader and is not found here, as before. The - header line itself is never rewritten, so its comment stays. + AND WITH THE READER'S RULE FOR A KEY (#674): a quoted key or a space + before the colon, `"shape":` or `shape :`, is read as the key by the + reader and is found here too, where it was refused as having no + `shape:` block. The header line itself is never rewritten, so its + comment and its quotes stay. So `"shape":` beside `shape:` is the key + written twice, as the reader reads it, and refused by name below. A HEADER THAT APPEARS TWICE IS REFUSED BY NAME (review of PR #558). The reader keeps the last block, so a rewrite of the first would move a pin @@ -2302,11 +2325,31 @@ def current_branch(root: Path) -> str | None: Named the same as `bump-leg.py`'s own helper for the same one-line read: two different tools reading two different checkouts, so a shared import would be the wrong coupling, but no reason for the name to drift. + + READ IN FULL, NEVER `--short` (#674). Beside a tag of the same name, + `--short` prints `heads/`, so on a branch `v1.0.0` in a root tagged + `v1.0.0` this read `heads/v1.0.0`: `commit_on_branch` refused `--branch + v1.0.0` as a branch that "is not the current branch ('heads/v1.0.0' is + checked out)", and `_awaits_its_pull_request` never knew its re-run. So + the ref is read whole and `refs/heads/` taken off by `_branch_of`, as + `Rollback.new_branch` takes it off; a HEAD attached anywhere else is on + no branch, as a detached one is. """ - proc = subprocess.run(["git", "symbolic-ref", "--quiet", "--short", "HEAD"], + proc = subprocess.run(["git", "symbolic-ref", "--quiet", "HEAD"], cwd=str(root), capture_output=True, text=True, encoding="utf-8", errors="replace", check=False) - return proc.stdout.strip() or None + return _branch_of(proc.stdout.strip()) + + +def _branch_of(ref: str | None) -> str | None: + """The branch the full ref `ref` names, `refs/heads/` taken off, or None + where it names none: a detached HEAD, or one attached outside + `refs/heads/`. One spelling for `current_branch` and + `Rollback.new_branch` (#674).""" + prefix = "refs/heads/" + if not ref or not ref.startswith(prefix): + return None + return ref[len(prefix):] or None BRANCH_REMEDIATION = ( @@ -2806,9 +2849,7 @@ def new_branch(self, branch: str) -> None: that is `heads/main`, and `git checkout heads/main` DETACHES HEAD rather than returning to the branch. """ - head = _symbolic_head(self.root) - start = (head[len("refs/heads/"):] - if head and head.startswith("refs/heads/") else None) + start = _branch_of(_symbolic_head(self.root)) commit = run(["git", "rev-parse", "HEAD"], cwd=self.root) self.branch = (branch, start, commit) run(["git", "checkout", "-q", "-b", branch], cwd=self.root) @@ -2923,9 +2964,21 @@ def _restore(self, path: Path, original: bytes, done: list[str], # "MAKE IT WRITABLE" IS SAID OF A FILE THAT IS THERE (review of # PR #560, round 2). A directory a validator deleted took the # file with it, and a file that is not there cannot be made - # writable: the line is then the command alone, which creates it. - how = (f"{where}, so make it writable, then: {command}" - if os.path.lexists(path) else f"{where}, then: {command}") + # writable: the line is then the command, which creates it. + # UNLESS THE DIRECTORY IT IS CREATED IN WILL NOT TAKE IT (#674): + # a validator that deleted the pin and left `contracts/` at 0555 + # was handed the bare `cat-file` redirect, and pasted, that + # failed `Permission denied`. The directory named is the one the + # command writes into, `_nearest_directory`'s, by its full path, + # since it need not be inside the root at all. + shut = (None if os.path.lexists(path) + else self._nearest_directory(path.parent)) + if shut is None: + how = f"{where}, so make it writable, then: {command}" + elif os.access(shut, os.W_OK): + how = f"{where}, then: {command}" + else: + how = f"{where}, so make {shut} writable, then: {command}" left.append(f"file {name} was not put back " f"({exc.strerror or exc}); " + (how if command else where)) @@ -4481,7 +4534,19 @@ def _land(root: Path, args, paths: list[str], added: list[str], target: str, commit_on_branch(root, args.branch, paths, target, upstream, count, kind, added, args.trailer, ledger=ledger, dropped=dropped, accepted=accepted) - print(f"\n committed on {args.branch}: " + ", ".join(paths)) + # WHAT THE COMMIT HOLDS, AS GIT LISTS IT (#674), not `paths`, which is + # what it was asked to commit: `cmd_apply` always names the pin and the + # manifest, and a run that only drops a merged-away row, or keeps a copy + # with `--accept-local` at the pinned commit, rewrites the manifest with + # the bytes it had, so this said `project.yaml` of a commit without it. + # Asked of the plumbing, `-z` so no name comes back quoted, `--root` so + # a first commit lists its files too. Should git not answer, or list + # nothing, the line keeps the paths it was asked to commit, as it always + # printed. + held = _git_answer(root, ["diff-tree", "--root", "--no-commit-id", "-r", + "--name-only", "--no-renames", "-z", "HEAD"]) + named = [path for path in (held or "").split("\0") if path] + print(f"\n committed on {args.branch}: " + ", ".join(named or paths)) if args.push: push_branch(root, args, create) if args.pr: @@ -4655,8 +4720,11 @@ def landing_failed(root: Path, args, exc: CommandFailed, act: str, + f": that commit stands and nothing was rolled back. What is left " f"is {left}{again}.", file=sys.stderr) sys.stderr.flush() + # The branch IN FULL (#674): bare, a tag of its name made the pasted + # push fail as the run's own push had (`_full_refspec`). push = ("" if pushed else shlex.join(["git", "-C", str(root), "push", "-u", - "--", "origin", branch]) + " && ") + "--", "origin", + _full_refspec(branch)]) + " && ") print() print(f"NEXT {push}{shlex.join(create)}") return LandingIncomplete(act) @@ -4680,14 +4748,33 @@ def _landing_run(command: list[str], root: Path) -> str: f"({exc.strerror or exc}): is it installed and on PATH?") from exc +def _full_refspec(branch: str) -> str: + """`refs/heads/:refs/heads/`: the branch named in full on + both sides of the push, for `push_branch` and for the two NEXT lines + that push by hand (`landing_failed`, `next_line`), so the three are one. + + A BARE NAME IS A REFSPEC THAT A TAG CAN TAKE (#674). In a root tagged + `v1.0.0`, `--branch v1.0.0 --push` committed and then died at `git push + -u -- origin v1.0.0`, `error: src refspec v1.0.0 matches more than one`, + and the NEXT line it printed failed the same way pasted. Named in full, + the source is the branch and nothing else, the destination is a branch, + and `-u` still sets the upstream (measured with git 2.43). It is the + form #689 and #672 ask of `family.py`'s and `bump-leg.py`'s push lines, + for the same reason, so the three tools can print one form; and + `checked_branch`'s alphabet already keeps a leading `+`, the other way + a bare name bends, out of this tool.""" + return f"refs/heads/{branch}:refs/heads/{branch}" + + def push_branch(root: Path, args, create: list[str]) -> None: - """`git push -u -- origin `, for the first run and for the re-run - alike, `--` ending git's options before the two positionals. A refusal - from origin is `landing_failed`'s (#204): the commit it was pushing - stands, so the step is named and nothing is rolled back.""" + """`git push -u -- origin refs/heads/:refs/heads/`, for the first + run and for the re-run alike, `--` ending git's options before the two + positionals and the branch named in full (`_full_refspec`, #674). A + refusal from origin is `landing_failed`'s (#204): the commit it was + pushing stands, so the step is named and nothing is rolled back.""" try: - _landing_run(["git", "push", "-q", "-u", "--", "origin", args.branch], - root) + _landing_run(["git", "push", "-q", "-u", "--", "origin", + _full_refspec(args.branch)], root) except CommandFailed as exc: raise landing_failed(root, args, exc, f"pushing {args.branch}", create) from exc @@ -5290,9 +5377,11 @@ def next_line(root: Path, args, paths: list[str], target: str, # THE ROOT, QUOTED (#325): `git -C ` is the one place a # path with a space, a quote or a `$` was printed bare in front of # `&&`, so the push the line starts with went to the wrong place - # or not at all and the `gh pr create` after it never ran. + # or not at all and the `gh pr create` after it never ran. AND + # THE BRANCH IN FULL (#674): bare, a tag of the same name made + # the push fail, `src refspec ... matches more than one`. push = shlex.join(["git", "-C", str(root), "push", "-u", "origin", - args.branch]) + _full_refspec(args.branch)]) print(f"NEXT {push} && {create}") return # NO COMMIT LINE OVER CHANGES THIS RUN DID NOT WRITE (#436). The line