diff --git a/tests/test_update_shape.py b/tests/test_update_shape.py index 36490f49..f85a5ab5 100644 --- a/tests/test_update_shape.py +++ b/tests/test_update_shape.py @@ -1222,8 +1222,8 @@ def test_no_trailer_leaves_the_commit_carrying_none(root, assert result.returncode == 0, result.stderr + result.stdout assert trailers_of(root) == [] assert message_of(root).endswith( - "the copies are not hand-edited and the digests are recomputed, not " - "adjusted.\n") + "the copies re-copied from the upstream are not hand-edited and the " + "digests are recomputed, not adjusted.\n") assert "Lane:" not in message_of(root) diff --git a/tests/test_update_shape_branch_any_directory.py b/tests/test_update_shape_branch_any_directory.py new file mode 100644 index 00000000..17c7f050 --- /dev/null +++ b/tests/test_update_shape_branch_any_directory.py @@ -0,0 +1,374 @@ +# SPDX-License-Identifier: Apache-2.0 +"""`--branch` is judged the same from any directory (#431). + +`checked_branch` asked `git check-ref-format --branch ` with no `cwd` +and no `--git-dir`, so git set up whatever repository surrounded the person's +directory, and read its own failure as the name's fault. From three kinds of +directory a good name was refused as `update-branch-invalid`: a `.git` file +naming a gitdir that is gone (a linked worktree whose main repository was +moved), a `.git/config` with a syntax error, and a directory removed under +the shell. `--root` names the repository `apply` works on and has nothing to +do with where the person stands, which is why the answer must not depend on +it. + +The question is now `shape_materialize._ask_git_branch`'s, the helper #355 +wrote for the scaffold and the family holder: an empty temporary directory is +the `--git-dir` and the working directory, so no repository is in play. THE +VERDICT IS TOLD APART FROM A FAILURE OF GIT'S OWN, as `_branch_verdict` does +it: a git that refuses `main` too could not answer for any name, and says +"could not be checked" with the remedy for that, never "pick another name". + +THE CLAIM IS MADE TWICE. End to end, `apply --branch` against an upstream that +has no change, from each directory, is `nothing to do` and exit 0; and +in-process, `checked_branch` itself, whose arms (a refusal by git, an +expansion, a git that cannot answer, a git that cannot be run) need a git +this suite does not have to break. +""" + +from __future__ import annotations + +import importlib.util +import os +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +from conftest import REPO, WINDOWS_SKIP, git, run_script +from test_update_shape import PROJECT, scaffold_upstream_and_project + +sys.path.insert(0, str(REPO / "scripts")) +import shape_materialize # noqa: E402 +from repo_shape import Refusal # noqa: E402 + +UPDATE = REPO / "update-shape.py" +BRANCH = "shape/update-x" + +#: What git says of a name it refuses, which the refusal carries. +GIT_SAYS = "`git check-ref-format --branch` refuses it" +#: The first words of each remedy: about the NAME, and about GIT. +ABOUT_THE_NAME = "Remediation: pass a branch name such as `shape/update-`" +ABOUT_GIT = "Remediation: the name was not judged" + + +@pytest.fixture(scope="module") +def update_shape(): + """`update-shape.py` as a module, so `checked_branch` is called in-process.""" + spec = importlib.util.spec_from_file_location("branch_any_dir_update", + UPDATE) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def project(tmp_path_factory) -> dict: + """A project at A and an upstream that has not moved from it, so every + `apply` that gets past the branch question says `nothing to do`.""" + _base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-branch-any-directory") + return {"upstream": upstream, "clone": clone, "a": commit_a} + + +@pytest.fixture +def root(project, tmp_path) -> Path: + """A private copy of the project: the repository `--root` names.""" + target = tmp_path / PROJECT + shutil.copytree(project["clone"], target, symlinks=True) + return target + + +# --- the directories git cannot open ------------------------------------------ + + +def ordinary(tmp_path: Path) -> Path: + """The control: a directory no repository surrounds.""" + where = tmp_path / "ordinary" + where.mkdir() + return where + + +def gitfile_to_a_missing_gitdir(tmp_path: Path) -> Path: + """A linked worktree whose main repository was moved or deleted: a `.git` + FILE naming a gitdir that is not there.""" + where = tmp_path / "gitfile" + where.mkdir() + (where / ".git").write_bytes( + b"gitdir: " + os.fsencode(tmp_path / "no-such-gitdir") + b"\n") + return where + + +def malformed_git_config(tmp_path: Path) -> Path: + """A repository whose `.git/config` has a syntax error.""" + where = tmp_path / "badconfig" + where.mkdir() + git("init", "-q", cwd=where) + (where / ".git" / "config").write_bytes(b"[core\n\tthis is not config\n") + return where + + +#: The directories a person may stand in that make a bare `git` die. +BROKEN = {"a-gitfile-to-a-missing-gitdir": gitfile_to_a_missing_gitdir, + "a-malformed-git-config": malformed_git_config} + + +def apply_from(where: Path, root: Path, project: dict, branch: str = BRANCH, + env: dict | None = None): + """`apply --branch ` against the unmoved upstream, run from + `where`. `--branch=` so a value that starts with a dash is still a value.""" + return run_script(UPDATE, "apply", "--root", str(root), "--upstream", + str(project["upstream"]), "--yes", + f"--branch={branch}", cwd=where, env=env) + + +def apply_from_a_removed_directory(tmp_path: Path, root: Path, project: dict, + branch: str = BRANCH): + """The same, from a directory removed under the shell: a shell enters it, + removes it, and runs the tool from where it WAS. POSIX only: a Windows + process holds its directory open, so the removal is refused.""" + gone = tmp_path / "gone" + gone.mkdir() + return subprocess.run( + ["sh", "-c", 'cd "$1" && rmdir "$1" && shift && exec "$@"', "sh", + str(gone), sys.executable, str(UPDATE), "apply", "--root", str(root), + "--upstream", str(project["upstream"]), "--yes", + f"--branch={branch}"], + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + stdin=subprocess.DEVNULL, check=False, + env={**os.environ, "LANES_LANE": ""}) + + +def assert_nothing_to_do(result) -> None: + assert result.returncode == 0, result.stdout + result.stderr + assert "nothing to do" in result.stdout, result.stdout + assert "update-branch-invalid" not in result.stderr, result.stderr + + +def test_the_control_is_nothing_to_do_from_an_ordinary_directory( + project, root, tmp_path): + """THE ISSUE'S CONTROL: the premise of every comparison below.""" + assert_nothing_to_do(apply_from(ordinary(tmp_path), root, project)) + + +@pytest.mark.parametrize("name", BROKEN) +def test_a_good_name_is_taken_from_a_directory_git_cannot_open( + project, root, tmp_path, name): + """The first two of the issue's three: a `.git` file naming a missing + gitdir and a malformed `.git/config`. Each exits as an ordinary directory + does, `nothing to do`, and never says the name was refused.""" + assert_nothing_to_do(apply_from(BROKEN[name](tmp_path), root, project)) + + +@WINDOWS_SKIP +def test_a_good_name_is_taken_from_a_directory_removed_under_the_shell( + project, root, tmp_path): + """The third: git cannot even read the current working directory.""" + assert_nothing_to_do( + apply_from_a_removed_directory(tmp_path, root, project)) + + +# --- a name git refuses is still refused, from there -------------------------- + + +@pytest.mark.parametrize("branch", ["a..b", "x.lock", "a/"]) +@pytest.mark.parametrize("name", [*BROKEN, "ordinary"]) +def test_a_name_git_refuses_is_still_refused_as_the_names_fault( + project, root, tmp_path, name, branch): + """`a..b`, `x.lock` and `a/` are inside the alphabet and refused by git, + which is what the question is for. The code, the line's own prefix and + the remedy about the NAME are what they were; it is not said to be + unchecked.""" + where = ({**BROKEN, "ordinary": ordinary})[name](tmp_path) + + result = apply_from(where, root, project, branch) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED update-branch-invalid: --branch {branch!r} "), result.stderr + assert GIT_SAYS in result.stderr + assert ABOUT_THE_NAME in result.stderr + assert "could not be checked" not in result.stderr + assert result.stdout == "", "refused before anything was read or written" + + +@WINDOWS_SKIP +@pytest.mark.parametrize("branch", ["a..b", "x.lock", "a/"]) +def test_a_name_git_refuses_is_refused_from_a_removed_directory_too( + project, root, tmp_path, branch): + result = apply_from_a_removed_directory(tmp_path, root, project, branch) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED update-branch-invalid: --branch {branch!r} "), result.stderr + assert GIT_SAYS in result.stderr + assert "could not be checked" not in result.stderr + + +def test_an_expansion_is_refused_in_a_repository_with_a_reflog( + project, root, tmp_path): + """`@{-1}` is `the branch checked out before this one` to a git that has + a repository. Run from one whose previous checkout was another branch, it + is still refused, and not by a git that lent it that reflog.""" + where = tmp_path / "with-a-reflog" + where.mkdir() + git("init", "-q", "-b", "main", cwd=where) + (where / "a.txt").write_text("a\n", encoding="utf-8") + git("add", "--", "a.txt", cwd=where) + git("commit", "-q", "-m", "a", cwd=where) + git("checkout", "-q", "-b", "other", cwd=where) + git("checkout", "-q", "main", cwd=where) + assert git("rev-parse", "--abbrev-ref", "@{-1}", + cwd=where).stdout.strip() == "other", "fixture: a reflog" + + result = apply_from(where, root, project, "@{-1}") + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + "REFUSED update-branch-invalid: --branch '@{-1}' "), result.stderr + assert ABOUT_THE_NAME in result.stderr + assert result.stdout == "" + + +# --- a git that could not answer ---------------------------------------------- + + +def a_global_config_git_cannot_read(tmp_path: Path) -> dict: + """An environment under which `git` fails for EVERY name, `main` too: + its global config does not parse.""" + broken = tmp_path / "broken-gitconfig" + broken.write_bytes(b"[core\n\tthis is not config\n") + return {"GIT_CONFIG_GLOBAL": str(broken), "GIT_CONFIG_NOSYSTEM": "1"} + + +@pytest.mark.parametrize("branch", [BRANCH, "a..b"]) +def test_a_git_that_cannot_answer_says_the_name_could_not_be_checked( + project, root, tmp_path, branch): + """A GIT THAT REFUSES `main` TOO JUDGED NOTHING, so it is not the name's + fault, for the good name and for the one git would have refused: the + sentence says "could not be checked", and the remedy is about git.""" + env = a_global_config_git_cannot_read(tmp_path) + + result = apply_from(ordinary(tmp_path), root, project, branch, env=env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED update-branch-invalid: --branch {branch!r} "), result.stderr + assert "could not be checked" in result.stderr + assert "so git could not answer" in result.stderr + assert "bad config" in result.stderr, "git's own words are quoted" + assert ABOUT_GIT in result.stderr + assert ABOUT_THE_NAME not in result.stderr + assert "is not a branch name git will create" not in result.stderr + assert result.stdout == "" + + +# --- `checked_branch`, in-process --------------------------------------------- + + +def test_the_question_is_the_one_helper_shape_materialize_asks_it_with( + update_shape): + """NOT A SECOND COPY: the name `update-shape.py` asks with is the object + `shape_materialize` defines, and so are the control name and the remedy + for a git that could not answer.""" + assert update_shape._ask_git_branch is shape_materialize._ask_git_branch + assert update_shape.BRANCH_CONTROL == shape_materialize.BRANCH_CONTROL + assert (update_shape.BRANCH_UNCHECKED_REMEDIATION + is shape_materialize.BRANCH_UNCHECKED_REMEDIATION) + + +@pytest.mark.parametrize("name", [*BROKEN, "ordinary"]) +def test_checked_branch_takes_a_good_name_from_any_directory( + update_shape, tmp_path, monkeypatch, name): + monkeypatch.chdir(({**BROKEN, "ordinary": ordinary})[name](tmp_path)) + assert update_shape.checked_branch(BRANCH) == BRANCH + + +@WINDOWS_SKIP +def test_checked_branch_takes_a_good_name_from_a_removed_directory( + update_shape, tmp_path, monkeypatch): + gone = tmp_path / "gone" + gone.mkdir() + monkeypatch.chdir(gone) + gone.rmdir() + assert update_shape.checked_branch(BRANCH) == BRANCH + + +def test_a_name_outside_the_alphabet_never_reaches_git(update_shape, + monkeypatch): + """The alphabet check comes first and is kept: nothing that reads as an + option, or holds a space or a brace, is put to git at all.""" + def asked(name): + raise AssertionError(f"git was asked about {name!r}") + + monkeypatch.setattr(update_shape, "_ask_git_branch", asked) + for value in ("-x", "--upload-pack=touch", "a b", "@{-1}", "/a", ".a"): + with pytest.raises(Refusal) as caught: + update_shape.checked_branch(value) + assert caught.value.code == "update-branch-invalid" + assert "will put on a `git` or `gh` command line" in \ + caught.value.detail + assert caught.value.remediation == update_shape.BRANCH_REMEDIATION + + +def test_a_name_git_would_expand_is_refused_as_the_names_fault( + update_shape, monkeypatch): + """A spelling git answers with ANOTHER name is refused too, because + `apply` names the branch it creates: the alphabet admits none today, so a + git that expands one is stood in for.""" + monkeypatch.setattr(update_shape, "_ask_git_branch", + lambda name: "main") + + with pytest.raises(Refusal) as caught: + update_shape.checked_branch("shape/x") + + assert caught.value.code == "update-branch-invalid" + assert caught.value.detail.startswith("--branch 'shape/x' ") + assert "reads it as 'main', not as written" in caught.value.detail + assert caught.value.remediation == update_shape.BRANCH_REMEDIATION + + +def test_a_git_that_cannot_be_run_is_unchecked_and_not_the_names_fault( + update_shape, monkeypatch): + """`git` missing from PATH, or no empty directory to ask it from: the + answer is not git's, so the remedy is the one for a git that could not + answer.""" + def missing(name): + raise FileNotFoundError(2, "No such file or directory", "git") + + monkeypatch.setattr(update_shape, "_ask_git_branch", missing) + + with pytest.raises(Refusal) as caught: + update_shape.checked_branch(BRANCH) + + assert caught.value.code == "update-branch-invalid" + assert caught.value.detail.startswith(f"--branch {BRANCH!r} ") + assert "could not be checked" in caught.value.detail + assert "could not be run" in caught.value.detail + assert caught.value.remediation == update_shape.BRANCH_UNCHECKED_REMEDIATION + + +def test_a_refusal_by_git_is_asked_about_the_control_name_before_it_is_blamed( + update_shape, monkeypatch): + """The control is asked only after git refused the person's name, and a + git that answers it is judging: the name is the fault.""" + asked = [] + + def refuses_everything_but_main(name): + asked.append(name) + if name != update_shape.BRANCH_CONTROL: + raise update_shape.CommandFailed( + ["git", "check-ref-format"], None, 1, "fatal: not valid") + return name + + monkeypatch.setattr(update_shape, "_ask_git_branch", + refuses_everything_but_main) + + with pytest.raises(Refusal) as caught: + update_shape.checked_branch("a..b") + + assert asked == ["a..b", update_shape.BRANCH_CONTROL] + assert GIT_SAYS in caught.value.detail + assert "(fatal: not valid)" in caught.value.detail + assert caught.value.remediation == update_shape.BRANCH_REMEDIATION diff --git a/tests/test_update_shape_header_comments.py b/tests/test_update_shape_header_comments.py new file mode 100644 index 00000000..3b027e52 --- /dev/null +++ b/tests/test_update_shape_header_comments.py @@ -0,0 +1,437 @@ +# SPDX-License-Identifier: Apache-2.0 +"""A `files:` or `shape:` header that carries a trailing comment is rewritten, +and keeps it (#437). + +#380 made the `commit:` and `tree_sha256:` rewrites keep each line's +indentation and comment. The two BLOCK HEADERS were still matched as +`line.rstrip() == "files:"` and `line.rstrip() == "shape:"`, so a header +followed by a comment, which the reader and both validators accept, was +refused after `check` had sent the person to `apply`: `files: # one row per +copied file` as `update-pin-unrecognised` ("0 `files` line(s)"), and +`shape: # mirrors contracts/shape-pin.yaml` as `update-manifest-no-shape`, +which was false (the manifest has the block) and whose remedy, "add it", +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). + +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 +renders (no scaffold, no git), and `apply` run end to end on a scaffolded +project and on a family holder, whose `family.yaml` `shape:` block goes +through the same function. + +FIXTURES ARE BYTES. A comment is put on a header with `write_bytes` and a +regular expression that keeps a carriage return where there is one, so no +Windows `write_text` newline translation reaches a comparison, which is made +against what the same tool wrote for a project with no comment. +""" + +from __future__ import annotations + +import importlib.util +import re +import shutil +import sys +from pathlib import Path + +import pytest + +from conftest import ORG, REPO, git, run_script +from test_family import upstream_clone +from test_pin_rewrite_layout import (MANIFEST, NEW_COMMIT, NEW_ROWS, NEW_TREE, + OLD_COMMIT, OLD_ROWS, OLD_TREE, make_pin, + read_back) +from test_update_shape import (CHANGED_SOURCE, PROJECT, UPSTREAM_FIX_LINE, + apply, commit_upstream_changes, next_line_of, + scaffold_upstream_and_project) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import Refusal, parse_yaml # noqa: E402 + +UPDATE = REPO / "update-shape.py" +PIN = "contracts/shape-pin.yaml" +HOLDER = "Contoso" +FAMILY_SOURCE = "templates/family-root/scripts/validate-family.py" + +#: Every way a comment may follow a header. `_strip_comment` takes a `#` after +#: a space or a tab, and a later `#` or a quote inside the comment is part of +#: it. +COMMENTS = { + "two spaces": " # one row per copied file", + "one space": " # one row per copied file", + "a tab": "\t# one row per copied file", + "a hash inside it": " # rows, then # more", + "quotes inside it": ' # the "files" we copy, and the \'rest\'', + "nothing after the hash": " #", +} + + +@pytest.fixture(scope="module") +def update_shape(): + """`update-shape.py` as a module, so the rewrites are called in-process.""" + spec = importlib.util.spec_from_file_location("header_comments_update", + UPDATE) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def with_comment(text: str, key: str, comment: str) -> str: + """`text` with `comment` after its one column-0 `:` header line.""" + marker = f"\n{key}:\n" + assert text.count(marker) == 1, f"fixture: one `{key}:` header" + return text.replace(marker, f"\n{key}:{comment}\n") + + +def comment_the_header(path: Path, key: str, comment: str) -> None: + """The same, on a file, in BYTES, keeping a CR where the file has one.""" + pattern = b"(?m)^" + key.encode() + b":(\\r?)$" + data, count = re.subn( + pattern, lambda found: key.encode() + b":" + comment.encode() + + found.group(1), path.read_bytes(), count=1) + assert count == 1, f"fixture: one `{key}:` header in {path}" + path.write_bytes(data) + + +def rows_of(update_shape, rows): + return [update_shape.Row(path, digest) for path, digest in rows] + + +# --- the rewrites, in-process ------------------------------------------------- + + +@pytest.mark.parametrize("kind", ["assembly shape pin", "family shape pin"]) +@pytest.mark.parametrize("comment", COMMENTS.values(), ids=COMMENTS) +def test_a_files_header_with_a_comment_is_rewritten_and_keeps_it( + update_shape, kind, comment): + """THE ISSUE'S PIN CASE. The result is the scaffold's own rendering of + the target, with the header line as it was; the reader takes it back.""" + before = with_comment(make_pin(kind, OLD_COMMIT, OLD_TREE, OLD_ROWS), + "files", comment) + expected = with_comment(make_pin(kind, NEW_COMMIT, NEW_TREE, NEW_ROWS), + "files", comment) + assert read_back(before) == (OLD_COMMIT, OLD_TREE), ( + "fixture: the reader accepts the commented header") + + new = update_shape.rewrite_shape_pin(before, NEW_COMMIT, NEW_TREE, + rows_of(update_shape, NEW_ROWS)) + + assert new == expected + assert f"\nfiles:{comment}\n" in new + assert read_back(new) == (NEW_COMMIT, NEW_TREE) + assert [row["path"] for row in parse_yaml(new)["files"]] == [ + path for path, _ in NEW_ROWS], "the rows under it are written whole" + + +@pytest.mark.parametrize("kind", ["assembly shape pin", "family shape pin"]) +def test_a_files_header_with_trailing_blanks_only_is_written_bare( + update_shape, kind): + """A header with no comment loses its trailing whitespace, as every line + `with_scalar` writes does, so a scaffold-shaped pin is still rewritten to + the scaffold's own bytes (#380's identity).""" + before = make_pin(kind, OLD_COMMIT, OLD_TREE, OLD_ROWS).replace( + "\nfiles:\n", "\nfiles: \n", 1) + + new = update_shape.rewrite_shape_pin(before, NEW_COMMIT, NEW_TREE, + rows_of(update_shape, NEW_ROWS)) + + assert new == make_pin(kind, NEW_COMMIT, NEW_TREE, NEW_ROWS) + + +@pytest.mark.parametrize("kind_name", ["PROJECT_KIND", "FAMILY_KIND"]) +@pytest.mark.parametrize("comment", COMMENTS.values(), ids=COMMENTS) +def test_a_shape_header_with_a_comment_is_rewritten_and_keeps_it( + update_shape, kind_name, comment): + """THE ISSUE'S MANIFEST CASE, for a project's `project.yaml` and for a + holder's `family.yaml`, which goes through the same function. Only the + block's two values move: the member row's own `commit:` above it, the + header and the key after the block stay as they were.""" + member = "e" * 40 + before = with_comment( + MANIFEST.format(member=member, commit=OLD_COMMIT, tree=OLD_TREE), + "shape", comment) + + new = update_shape.rewrite_manifest( + before, NEW_COMMIT, NEW_TREE, getattr(update_shape, kind_name)) + + assert new == with_comment( + MANIFEST.format(member=member, commit=NEW_COMMIT, tree=NEW_TREE), + "shape", comment) + shape = parse_yaml(new)["shape"] + assert (shape["commit"], shape["digests"]["tree_sha256"]) == ( + NEW_COMMIT, NEW_TREE) + + +# --- a header that is missing is still refused, and a doubled one is refused +# --- by name --------------------------------------------------------------------- + + +@pytest.mark.parametrize("damage", [ + pytest.param(lambda text: text.replace("\nfiles:\n", "\nrows:\n", 1), + id="absent"), + pytest.param(lambda text: text.replace("\nfiles:\n", "\n files:\n", 1), + id="indented"), + pytest.param(lambda text: text.replace("\nfiles:\n", "\n# files:\n", 1), + id="commented-out"), + pytest.param(lambda text: text.replace("\nfiles:\n", "\nfiles:#rows\n", 1), + id="comment-glued-on"), +]) +def test_a_pin_with_no_files_header_is_still_refused_by_its_count( + update_shape, damage): + """The count is unchanged: no `files:` line at column 0 is `0`, whatever + else on the line looks like one. Nothing is written, since this is the + rewrite's own refusal.""" + before = make_pin("assembly shape pin", OLD_COMMIT, OLD_TREE, OLD_ROWS) + broken = damage(before) + assert broken != before + + with pytest.raises(Refusal) as caught: + update_shape.rewrite_shape_pin(broken, NEW_COMMIT, NEW_TREE, + rows_of(update_shape, NEW_ROWS)) + + assert caught.value.code == "update-pin-unrecognised" + assert "has 0 `files` line(s) where exactly one was expected" in \ + caught.value.detail + + +@pytest.mark.parametrize("kind_name, validator", [ + ("PROJECT_KIND", "validate-manifest.py"), + ("FAMILY_KIND", "validate-family.py")]) +@pytest.mark.parametrize("damage", [ + pytest.param(lambda text: text.replace("\nshape:\n", "\nelected:\n", 1), + id="absent"), + pytest.param(lambda text: text.replace("\nshape:\n", "\n shape:\n", 1), + id="indented"), + pytest.param(lambda text: text.replace("\nshape:\n", "\n# shape:\n", 1), + id="commented-out"), +]) +def test_a_manifest_with_no_shape_header_still_names_this_kinds_validator( + update_shape, kind_name, validator, damage): + """`update-manifest-no-shape` is kept for a manifest that really has no + `shape:` block at column 0, and its remedy still names the validator the + kind carries.""" + before = MANIFEST.format(member="e" * 40, commit=OLD_COMMIT, tree=OLD_TREE) + broken = damage(before) + assert broken != before + + with pytest.raises(Refusal) as caught: + update_shape.rewrite_manifest(broken, NEW_COMMIT, NEW_TREE, + getattr(update_shape, kind_name)) + + assert caught.value.code == "update-manifest-no-shape" + assert validator in caught.value.remediation + + +def doubled_manifest() -> str: + """A manifest whose `shape:` key is written twice, the second block holding + the old values: the reader keeps the LAST block.""" + doubled = (MANIFEST.format(member="e" * 40, commit=OLD_COMMIT, + tree=OLD_TREE) + + f'shape:\n commit: "{OLD_COMMIT}"\n digests:\n' + f' tree_sha256: "{OLD_TREE}"\n') + assert parse_yaml(doubled)["shape"]["commit"] == OLD_COMMIT, ( + "fixture: the reader keeps the last block") + return doubled + + +@pytest.mark.parametrize("kind_name", ["PROJECT_KIND", "FAMILY_KIND"]) +@pytest.mark.parametrize("which", ["neither", "first", "last", "both"]) +def test_a_doubled_shape_header_is_refused_by_name_as_a_key_written_twice( + update_shape, kind_name, which): + """THE READER DOES NOT REFUSE A DOUBLED `shape:` (it keeps the last), and a + rewrite of the first block would move a pin nobody reads. It is refused + BY NAME, in words that are true of a key written twice, whichever header + carries a comment: the read-back's, "this tool's line rewrite", are not. + On main a first header with a comment was skipped by the exact match, so + that manifest was rewritten at its last block and went through (review of + PR #558); it is refused here, as the uncommented one always was.""" + text = doubled_manifest() + first, last = text.index("\nshape:\n"), text.rindex("\nshape:\n") + assert first != last + if which in ("first", "both"): + text = text[:first] + "\nshape: # first\n" + text[first + 8:] + if which in ("last", "both"): + last = text.rindex("\nshape:\n") + text = text[:last] + "\nshape: # last\n" + text[last + 8:] + assert (text != doubled_manifest()) is (which != "neither") + + with pytest.raises(Refusal) as caught: + update_shape.rewrite_manifest(text, NEW_COMMIT, NEW_TREE, + getattr(update_shape, kind_name)) + + kind = getattr(update_shape, kind_name) + assert caught.value.code == "update-manifest-unrecognised" + assert caught.value.detail.startswith( + f"{kind.manifest} holds the `shape:` key twice, at lines 8 and 17, " + "where exactly one was expected: the reader keeps the last block") + assert "Remediation: delete the extra `shape:` block by hand" in \ + caught.value.remediation + assert "line rewrite" not in (caught.value.detail + + caught.value.remediation) + + +# --- `apply`, end to end ------------------------------------------------------ + + +@pytest.fixture(scope="module") +def project(tmp_path_factory) -> dict: + """A project at A, its upstream moved to B, and the line-for-line control + this file compares with: the same project with no comment, applied.""" + base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-header-comments") + commit_b = commit_upstream_changes( + upstream, "Fix the manifest validator", + {CHANGED_SOURCE: UPSTREAM_FIX_LINE}) + assert commit_a != commit_b + return {"upstream": upstream, "clone": clone, "a": commit_a, + "b": commit_b} + + +@pytest.fixture(scope="module") +def controlled(project, tmp_path_factory) -> dict: + """What `apply` writes for a project with NO comment on either header: + the bytes of its pin and its manifest after the update.""" + root = tmp_path_factory.mktemp("header-comments-control") / PROJECT + shutil.copytree(project["clone"], root, symlinks=True) + result = apply(root, project) + assert result.returncode == 0, result.stdout + result.stderr + return {"pin": (root / PIN).read_bytes(), + "manifest": (root / "project.yaml").read_bytes()} + + +@pytest.fixture +def root(project, tmp_path) -> Path: + """A private, mutable copy of the project as it was scaffolded at A.""" + target = tmp_path / PROJECT + shutil.copytree(project["clone"], target, symlinks=True) + return target + + +def expected_bytes(control: bytes, key: str, comment: str) -> bytes: + """`control`, which `apply` wrote with bare headers, with the comment the + project's own header carried put after `key:`.""" + marker = f"\n{key}:".encode() + data, count = re.subn( + re.escape(marker) + b"(\\r?)\n", + lambda found: marker + comment.encode() + found.group(1) + b"\n", + control, count=1) + assert count == 1 + return data + + +@pytest.mark.parametrize("which", ["pin", "manifest", "both"]) +@pytest.mark.parametrize("comment", [COMMENTS["two spaces"], + COMMENTS["a tab"]], + ids=["two-spaces", "tab"]) +def test_apply_takes_the_update_for_a_header_with_a_comment_and_keeps_it( + project, controlled, root, which, comment): + """THE ISSUE'S TWO REPRODUCTIONS, and both at once. `check` sends the + person to `apply`, which now exits 0 with each comment where it was and + every other byte what it is for a project with no comment: it wrote the + same bytes, and its own validators, which `apply` runs, are green.""" + if which in ("pin", "both"): + comment_the_header(root / PIN, "files", comment) + if which in ("manifest", "both"): + comment_the_header(root / "project.yaml", "shape", comment) + checked = run_script(UPDATE, "check", "--root", str(root), "--upstream", + str(project["upstream"]), "--at", project["b"]) + assert checked.returncode == 1, checked.stdout + checked.stderr + assert "apply" in next_line_of(checked.stdout) + + result = apply(root, project) + + 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 + pin_wanted = (expected_bytes(controlled["pin"], "files", comment) + if which in ("pin", "both") else controlled["pin"]) + manifest_wanted = (expected_bytes(controlled["manifest"], "shape", comment) + if which in ("manifest", "both") + else controlled["manifest"]) + assert (root / PIN).read_bytes() == pin_wanted + assert (root / "project.yaml").read_bytes() == manifest_wanted + 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 + + +def test_a_commented_header_lands_on_a_branch_with_the_comment_in_the_commit( + project, root): + """`--branch` commits the pin and the manifest from the working tree, so + the commit holds the commented headers too: what a person reviews is the + two values and the rows, never a header that lost its comment.""" + comment_the_header(root / PIN, "files", COMMENTS["two spaces"]) + comment_the_header(root / "project.yaml", "shape", COMMENTS["two spaces"]) + git("add", "--", PIN, "project.yaml", cwd=root) + git("commit", "-q", "-m", "Comment the two headers", cwd=root) + + result = apply(root, project, "--branch", "shape/update-header-comments") + + assert result.returncode == 0, result.stdout + result.stderr + pin = git("show", f"HEAD:{PIN}", cwd=root).stdout + assert "\nfiles: # one row per copied file\n" in pin + manifest = git("show", "HEAD:project.yaml", cwd=root).stdout + assert "\nshape: # one row per copied file\n" in manifest + assert git("status", "--porcelain", cwd=root).stdout == "" + + +# --- a family holder ---------------------------------------------------------- + + +@pytest.fixture(scope="module") +def holder_project(tmp_path_factory) -> dict: + """A real family holder cut into a local bare repository, an upstream + that has since moved to B, and the control `apply` writes with no + comment.""" + base = tmp_path_factory.mktemp("header-comments-family") + created = run_script( + REPO / "scripts" / "family.py", "init", "--org", ORG, + "--family", HOLDER, "--created-by", "Test Human", + "--created-on", "2026-09-09", + "--local-remote-dir", str(base / "remotes"), + "--work-dir", str(base / "work")) + assert created.returncode == 0, created.stderr + created.stdout + upstream = upstream_clone(base / "openRepoShape") + commit_b = commit_upstream_changes( + upstream, "Fix the family validator", + {FAMILY_SOURCE: "\n# An upstream fix that must reach every family.\n"}) + clone = base / "work" / HOLDER + control = base / "control" / HOLDER + shutil.copytree(clone, control, symlinks=True) + result = apply(control, {"upstream": upstream, "b": commit_b}) + assert result.returncode == 0, result.stdout + result.stderr + return {"upstream": upstream, "clone": clone, "b": commit_b, + "pin": (control / PIN).read_bytes(), + "manifest": (control / "family.yaml").read_bytes()} + + +@pytest.mark.parametrize("which", ["pin", "manifest", "both"]) +def test_apply_takes_the_update_for_a_family_holder_header_with_a_comment( + holder_project, tmp_path, which): + """`family.yaml`'s `shape:` block goes through the same function as + `project.yaml`'s, and a holder's pin through the same `files:` rewrite: + each comment is kept, the holder's own validator stays green, and the + rest is what `apply` wrote for the holder with no comment.""" + holder = tmp_path / HOLDER + shutil.copytree(holder_project["clone"], holder, symlinks=True) + comment = COMMENTS["two spaces"] + if which in ("pin", "both"): + comment_the_header(holder / PIN, "files", comment) + if which in ("manifest", "both"): + comment_the_header(holder / "family.yaml", "shape", comment) + + result = apply(holder, holder_project) + + assert result.returncode == 0, result.stdout + result.stderr + assert "update-manifest-no-shape" not in result.stderr + assert (holder / PIN).read_bytes() == ( + expected_bytes(holder_project["pin"], "files", comment) + if which in ("pin", "both") else holder_project["pin"]) + assert (holder / "family.yaml").read_bytes() == ( + expected_bytes(holder_project["manifest"], "shape", comment) + if which in ("manifest", "both") else holder_project["manifest"]) + green = run_script(holder / "scripts" / "validate-family.py", cwd=holder) + assert green.returncode == 0, green.stdout + green.stderr diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index 6ef633b9..a273212d 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -421,8 +421,11 @@ def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, "--title " in line assert f"`apply --push --pr` run again on {BRANCH}" in result.stderr else: - assert line.endswith(f"&& gh pr create --repo testorg/{PROJECT} " - f"--base main --head {BRANCH}"), line + assert f"&& gh pr create --repo testorg/{PROJECT} --base main " \ + f"--head {BRANCH} --title " in line, line + assert " --body " in line and "--fill" not in line, ( + "a bare `gh pr create` has gh prompt for what a shell with no " + "terminal cannot answer (#535)") assert "run again" not in result.stderr, ( "a re-run without --pr finishes nothing, so none is promised") the_commit_stands(root, project) diff --git a/tests/test_update_shape_messages.py b/tests/test_update_shape_messages.py new file mode 100644 index 00000000..9ee6606a --- /dev/null +++ b/tests/test_update_shape_messages.py @@ -0,0 +1,704 @@ +# SPDX-License-Identifier: Apache-2.0 +"""Four `update-shape.py` messages that said what was not true (#535). + +Each was an ordinary path whose only harm was a misleading or unpasteable +message, found in the v1.0.0 review of `apply` and `check`: + + N1 A branchless `apply` that moved the pin back to what HEAD commits still + printed a commit line, and pasted, git said "nothing to commit". + N3 The `update-conflict` remedy said `git show :` and + nothing, in the refusal or in `check`, said what either was. + N4 The commit `apply --branch` writes said "the copies are not + hand-edited", and so did `--pr`'s body, of every file, including those + `--accept-local` re-pinned from the project's own bytes. + N7 The `gh pr create` that `apply --branch` prints without `--pr` had no + `--title`, `--body` or `--fill`; `gh` prompts for them. + +THE TEXTS ARE READ, NEVER RUN. `gh` is never reached: `--pr` runs in this +process with `update_shape.run` stubbed at the one seam it goes through, as +`test_update_shape_pr_base.py` does, and a printed `gh pr create` is a line of +text, read back by `shlex.split`. What `gh` does without a terminal was not +run, and nothing here says what it does. +""" + +from __future__ import annotations + +import importlib.util +import re +import shlex +import shutil +import sys +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from conftest import ORG, REPO, WINDOWS_SKIP, git, run_script +from test_commit_trailers import message_of +from test_update_shape import (CHANGED, CHANGED_SOURCE, LOCAL, PROJECT, + UPSTREAM_FIX_LINE, commit_upstream_changes, + next_line_of, pin_rows, + scaffold_upstream_and_project) +from test_update_shape_header_comments import ( # noqa: F401 + HOLDER, holder_project, +) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import file_sha256 # noqa: E402 +from shape_materialize import SHAPE_REPOSITORY # noqa: E402 + +UPDATE = REPO / "update-shape.py" +PIN = "contracts/shape-pin.yaml" +MANIFEST = "project.yaml" +LOCAL_SOURCE = f"templates/assembly-root/{LOCAL}" + +#: The sentence the commit message ends on, and the body's: "not +#: hand-edited" of the copies re-copied from the upstream, and of nothing +#: else. +MESSAGE_ENDS = ("the copies re-copied from the upstream are not hand-edited " + "and the digests are recomputed, not adjusted.") +BODY_SAYS = ("No copy re-copied from the upstream was hand-edited and every " + "digest was recomputed.") + + +@pytest.fixture(scope="module") +def update_shape(): + """`update-shape.py` as a module, so `--pr` runs here with `gh` stubbed + and `next_line` and `conflict_remediation` are called directly.""" + spec = importlib.util.spec_from_file_location("update_shape_messages", + UPDATE) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(scope="module") +def project(tmp_path_factory) -> dict: + """A project at A and an upstream that has moved twice. B changes the one + file `CHANGED_SOURCE`. C, on B, changes it and `LOCAL_SOURCE` too, so a + project that edited both has TWO files changed on both sides at C, and one + that edited `LOCAL` alone has a file only IT changed at B.""" + _base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-messages") + commit_b = commit_upstream_changes( + upstream, "Fix the manifest validator", + {CHANGED_SOURCE: UPSTREAM_FIX_LINE}) + commit_c = commit_upstream_changes( + upstream, "Fix both validators", + {CHANGED_SOURCE: "\n# A second fix.\n", LOCAL_SOURCE: "\n# A fix.\n"}) + assert len({commit_a, commit_b, commit_c}) == 3 + return {"upstream": upstream, "clone": clone, "a": commit_a, + "b": commit_b, "c": commit_c} + + +@pytest.fixture +def root(project, tmp_path) -> Path: + """A private, mutable copy of the project as it was scaffolded at A.""" + target = tmp_path / PROJECT + shutil.copytree(project["clone"], target, symlinks=True) + return target + + +def edit(root: Path, path: str, *, commit: bool = False) -> None: + """A local edit to a pinned copy, in bytes, and, where `--branch` is to + take the run, committed: `apply --branch` refuses a root whose pinned + copy is not what HEAD commits.""" + target = root / path + target.write_bytes(target.read_bytes() + b"\n# a local edit\n") + if commit: + git("add", "--", path, cwd=root) + git("commit", "-q", "-m", f"Edit {path}", cwd=root) + + +def run_apply(root: Path, project: dict, at: str, *extra: str): + return run_script(UPDATE, "apply", "--root", str(root), "--yes", + "--upstream", str(project["upstream"]), "--at", + project[at], *extra) + + +def run_check(root: Path, project: dict, at: str): + return run_script(UPDATE, "check", "--root", str(root), "--upstream", + str(project["upstream"]), "--at", project[at]) + + +# --- N3: the source path nobody names ------------------------------------------ + + +def test_check_names_the_upstream_file_of_a_row_changed_on_both_sides( + project, root): + """`check`'s `both` row says which file in the standard it compares with, + which is not the path in the root.""" + edit(root, CHANGED) + + result = run_check(root, project, "b") + + assert result.returncode == 1, result.stdout + result.stderr + assert ("(edited here AND upstream since the pin (upstream's file: " + f"{CHANGED_SOURCE}). Merge it by hand") in result.stdout, \ + result.stdout + + +def test_apply_refuses_a_both_row_with_the_command_that_reads_its_upstream_bytes( + project, root): + """The remedy's placeholders are filled: the full target commit and the + path in the standard, which `git show` reads in a shape clone. The row's + own line in the refusal names the same file.""" + edit(root, CHANGED) + + result = run_apply(root, project, "b") + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith("REFUSED update-conflict: "), result.stderr + assert (f" {CHANGED}: edited here AND upstream since the pin " + f"(upstream's file: {CHANGED_SOURCE})") in result.stderr + assert ("(the upstream bytes are `git show " + f"{project['b']}:{CHANGED_SOURCE}` in a shape clone)" + ) in result.stderr + assert "" not in result.stderr + assert "" not in result.stderr + assert "--accept-local on that path" in result.stderr + + +def test_apply_names_each_both_rows_command_by_the_path_in_the_root( + project, root): + """Two rows changed on both sides are two commands, each for its own + file, so the person reading one never runs the other's.""" + edit(root, CHANGED) + edit(root, LOCAL) + + result = run_apply(root, project, "c") + + assert result.returncode == 2, result.stdout + result.stderr + assert (f"`git show {project['c']}:{LOCAL_SOURCE}` for {LOCAL}" + in result.stderr), result.stderr + assert (f"`git show {project['c']}:{CHANGED_SOURCE}` for {CHANGED}" + in result.stderr), result.stderr + assert "(the upstream bytes, in a shape clone: " in result.stderr + + +def test_a_missing_copy_keeps_its_own_remedy_and_the_others_are_named( + project, root): + """`copy-missing` has an exit of its own and no upstream bytes to name; a + `both` row beside it still gets its command, after `For the others,`.""" + edit(root, CHANGED) + (root / "Makefile").unlink() + + result = run_apply(root, project, "b") + + assert result.returncode == 2, result.stdout + result.stderr + assert "Makefile is pinned and gone from the root" in result.stderr + assert ("For the others, merge each one by hand (the upstream bytes are " + f"`git show {project['b']}:{CHANGED_SOURCE}` in a shape clone)" + ) in result.stderr, result.stderr + + +def test_a_row_with_no_upstream_bytes_is_named_no_command(update_shape): + """`unmapped` has no upstream file and `upstream-removed` is gone at the + target, so `git show` would fail for either: the sentence is what it + always was, with no parenthesis, never a command that cannot run.""" + unmapped = update_shape.Row("scripts/x.py", "0" * 64) + unmapped.state = update_shape.UNMAPPED + removed = update_shape.Row("scripts/y.py", "0" * 64) + removed.state = update_shape.UPSTREAM_REMOVED + removed.source = "templates/assembly-root/scripts/y.py" + removed.target = "f" * 40 + + said = update_shape.conflict_remediation([unmapped, removed]) + + assert said == ("Remediation: merge each one by hand, commit it, then " + "re-run this with --accept-local on that path so the row " + "is recomputed from what you merged.") + assert "git show" not in said + + +# --- N4: "not hand-edited" is not true of everything --------------------------- + + +def test_a_run_that_accepted_nothing_says_not_hand_edited_of_the_re_copied_only( + project, root): + result = run_apply(root, project, "b", "--branch", "shape/update-n4a") + + assert result.returncode == 0, result.stdout + result.stderr + message = message_of(root) + assert message.endswith(f"Written by update-shape.py; {MESSAGE_ENDS}\n") + assert "was not re-copied" not in message + assert "were not re-copied" not in message + assert "--accept-local" not in message + + +def test_the_commit_names_a_path_accept_local_kept_from_the_projects_own_bytes( + project, root): + """`LOCAL` is edited here and unchanged upstream at B, so `--accept-local` + re-pins it from the project's bytes: the history must not call it + not hand-edited.""" + edit(root, LOCAL, commit=True) + + result = run_apply(root, project, "b", "--accept-local", LOCAL, + "--branch", "shape/update-n4b") + + assert result.returncode == 0, result.stdout + result.stderr + message = message_of(root) + assert (f"{LOCAL} was not re-copied: `--accept-local` kept it as the " + "project holds it, so the pin's row for it records a digest of " + "the project's own bytes, not of the upstream's file.\n\n" + f"Written by update-shape.py; {MESSAGE_ENDS}") in message, message + assert message.endswith(MESSAGE_ENDS + "\n") + + +def test_the_commit_names_every_accepted_path_in_one_order(project, root): + """Two rows changed on both sides, both accepted: named together, sorted, + in the plural.""" + edit(root, CHANGED, commit=True) + edit(root, LOCAL, commit=True) + + result = run_apply(root, project, "c", "--accept-local", CHANGED, + "--accept-local", LOCAL, "--branch", + "shape/update-n4c") + + assert result.returncode == 0, result.stdout + result.stderr + assert (f"{LOCAL}, {CHANGED} were not re-copied: `--accept-local` kept " + "each as the project holds it, so the pin's row for each records " + "a digest of the project's own bytes, not of the upstream's " + "file.") in message_of(root) + + +def test_a_path_accept_local_names_but_the_run_re_copied_is_not_called_kept( + project, root): + """`--accept-local` on a copy nobody edited changes nothing: it is + re-copied from the upstream, so it is among the copies the message says + are not hand-edited, and it is not named as kept.""" + result = run_apply(root, project, "b", "--accept-local", CHANGED, + "--branch", "shape/update-n4d") + + assert result.returncode == 0, result.stdout + result.stderr + message = message_of(root) + assert "1 copied file(s) re-copied from the upstream" in message + assert "not re-copied" not in message + + +def stub_gh(update_shape, monkeypatch, answer) -> list: + """`update_shape.run` with `gh` answered by `answer(command, calls)`, which + is never run, and `git` still run for real: a commit under test has to be + a real one. Returns the list of every `gh` command the tool tried.""" + real = update_shape.run + calls: list = [] + + def run(command, *args, **kwargs): + if command[0] == "gh": + calls.append(list(command)) + return answer(command, calls, update_shape) + return real(command, *args, **kwargs) + + monkeypatch.setattr(update_shape, "run", run) + for name, value in (("GIT_AUTHOR_NAME", "openRepoShape tests"), + ("GIT_AUTHOR_EMAIL", "tests@openreposhape.invalid"), + ("GIT_COMMITTER_NAME", "openRepoShape tests"), + ("GIT_COMMITTER_EMAIL", "tests@openreposhape.invalid"), + ("LANES_LANE", "")): + monkeypatch.setenv(name, value) + return calls + + +@pytest.fixture +def gh_calls(update_shape, monkeypatch) -> list: + """Every `gh` command the tool tries to run, none of them run: `gh` is + answered with a pull request URL.""" + return stub_gh(update_shape, monkeypatch, + lambda command, calls, module: + "https://example.invalid/pull/1") + + +def apply_in_process(update_shape, root: Path, project: dict, at: str, + branch: str, *extra: str) -> int: + """`apply --branch --pr` run in this process, whose `gh` is a + stub; the exit code.""" + return update_shape.main(["apply", "--root", str(root), "--yes", + "--upstream", str(project["upstream"]), "--at", + project[at], "--branch", branch, "--pr", *extra]) + + +def opened_by_pr(update_shape, gh_calls: list, root: Path, project: dict, + at: str, branch: str, *extra: str) -> tuple[str, str]: + """`(title, body)` of the one `gh pr create` `--pr` runs.""" + assert apply_in_process(update_shape, root, project, at, branch, + *extra) == 0 + assert len(gh_calls) == 1, gh_calls + command = gh_calls[0] + assert command[:3] == ["gh", "pr", "create"], command + return (command[command.index("--title") + 1], + command[command.index("--body") + 1]) + + +def test_the_pull_request_body_claims_not_hand_edited_of_the_re_copied_only( + update_shape, gh_calls, project, root): + title, body = opened_by_pr(update_shape, gh_calls, root, project, "b", + "shape/update-n4e") + + assert title == ("Re-sync the shape copies to " + f"{SHAPE_REPOSITORY} @ {project['b'][:12]}") + assert BODY_SAYS in body + assert body.startswith("Written by openRepoShape's `update-shape.py`: " + "`contracts/shape-pin.yaml` and `project.yaml` " + "re-pinned onto "), body + assert "No copy was hand-edited" not in body + assert "not re-copied" not in body + + +def test_the_pull_request_body_names_what_accept_local_kept( + update_shape, gh_calls, project, root): + edit(root, LOCAL, commit=True) + + _, body = opened_by_pr(update_shape, gh_calls, root, project, "b", + "shape/update-n4f", "--accept-local", LOCAL) + + assert BODY_SAYS in body + assert body.endswith( + f" {LOCAL} was not re-copied: `--accept-local` kept it as the " + "project holds it, so the pin's row for it records a digest of the " + "project's own bytes, not of the upstream's file."), body + + +# --- N7: a bare `gh pr create` -------------------------------------------------- + + +def create_argv(line: str) -> list[str]: + """The `gh pr create` in a NEXT line, as the shell would split it: the + part after the `&&` of the push, or the whole line after `--push`.""" + words = shlex.split(line.removeprefix("NEXT ")) + return words[words.index("gh"):] + + +@pytest.mark.parametrize("accepted", [False, True], + ids=["plain", "accept-local"]) +def test_the_branch_next_line_is_a_gh_pr_create_with_the_words_pr_uses( + update_shape, gh_calls, project, tmp_path, accepted): + """THE LINE ROUND-TRIPS through `shlex.split` and carries a `--title` and + a `--body`, and they are exactly what `--pr` opens the pull request with + for the same run, the apostrophe and the backticks in the body included. + Never `--fill`, which reads the commits of wherever the line is pasted.""" + plain, opened = (tmp_path / "plain" / PROJECT, tmp_path / "opened" / PROJECT) + for root in (plain, opened): + shutil.copytree(project["clone"], root, symlinks=True) + if accepted: + edit(root, LOCAL, commit=True) + extra = ("--accept-local", LOCAL) if accepted else () + + result = run_apply(plain, project, "b", "--branch", "shape/update-n7", + *extra) + assert result.returncode == 0, result.stdout + result.stderr + argv = create_argv(next_line_of(result.stdout)) + title, body = opened_by_pr(update_shape, gh_calls, opened, project, "b", + "shape/update-n7", *extra) + + assert argv == ["gh", "pr", "create", "--repo", f"{ORG}/{PROJECT}", + "--base", "main", "--head", "shape/update-n7", "--title", + title, "--body", body] + assert "--fill" not in argv + assert "openRepoShape's `update-shape.py`" in body, "quoted whole" + assert (LOCAL in body) is accepted + + +def test_the_line_after_a_push_carries_the_words_too(update_shape, tmp_path, + capsys): + """`--push` has pushed, so the line is the `gh pr create` alone. It is + given the words by the run that made the commit; a caller that came + another way is given the standard's own repository to name.""" + (tmp_path / MANIFEST).write_text( + "tracking_branch: main\nlegs:\n - role: assembly\n" + " repository: acme/Atlas\n", encoding="utf-8") + target = "ab" * 20 + kept = update_shape.pull_request_words("acme/Shape", target, MANIFEST, + [LOCAL]) + fallback = update_shape.pull_request_words(SHAPE_REPOSITORY, target, + MANIFEST) + for stash, words in ((kept, kept), (None, fallback)): + args = SimpleNamespace(branch="shape/x", push=True, pr=False) + if stash: + args.pull_request_words = stash + + update_shape.next_line(tmp_path, args, [], target, []) + + line = capsys.readouterr().out.strip() + assert line.count("\n") == 0 + assert create_argv(line) == [ + "gh", "pr", "create", "--repo", "acme/Atlas", "--base", "main", + "--head", "shape/x", "--title", words[0], "--body", words[1]] + + +def test_the_bare_form_a_failed_push_prints_carries_the_words_too( + update_shape, tmp_path): + """`pull_request_line` builds the `gh pr create` that `landing_failed` + prints when `--push` fails, and it is the plain `--branch` line's twin: a + bare one there would stop in a shell with no terminal for the same + reason. Without `--pr` it leaves `--repo` off where the manifest names + none, and carries the title and the body all the same.""" + (tmp_path / MANIFEST).write_text("tracking_branch: trunk\n", + encoding="utf-8") + args = SimpleNamespace(branch="shape/x", push=True, pr=False) + upstream = SimpleNamespace(repository="o/r") + title, body = update_shape.pull_request_words("o/r", "cd" * 20, MANIFEST, + [LOCAL]) + + create = update_shape.pull_request_line( + tmp_path, args, "cd" * 20, upstream, update_shape.PROJECT_KIND, + [LOCAL]) + + assert create == ["gh", "pr", "create", "--base", "trunk", "--head", + "shape/x", "--title", title, "--body", body] + assert title == f"Re-sync the shape copies to o/r @ {'cd' * 6}" + assert LOCAL in body + + +# --- the words are true in every state (review of PR #558) ------------------- + + +def test_a_pull_request_that_re_copied_nothing_does_not_say_it_did( + update_shape, gh_calls, project, root): + """`CHANGED` is the one file upstream changed at B, and this project + edited it: its row is `both`, accepted, so NOTHING is re-copied. The body + opened with "the copied shape files were re-copied from the upstream" in + every state; it claims no copy was re-copied, and names the one kept.""" + edit(root, CHANGED, commit=True) + + _, body = opened_by_pr(update_shape, gh_calls, root, project, "b", + "shape/update-n3b", "--accept-local", CHANGED) + + assert body.startswith("Written by openRepoShape's `update-shape.py`: " + "`contracts/shape-pin.yaml` and `project.yaml` " + "re-pinned onto "), body + assert "shape files were re-copied" not in body + assert f" {CHANGED} was not re-copied: " in body + + +def test_a_holders_pull_request_words_name_family_yaml_and_not_project_yaml( + update_shape, gh_calls, holder_project, tmp_path): + """A holder has a `family.yaml` and no `project.yaml`, and the body said + `project.yaml` was re-pinned: in what `--pr` opens the pull request + with, in the plain `--branch` NEXT line and in the line a failed + `--push` prints. The words are read whole here, never stripped.""" + printed = tmp_path / "printed" / HOLDER + opened = tmp_path / "opened" / HOLDER + for holder in (printed, opened): + shutil.copytree(holder_project["clone"], holder, symlinks=True) + + result = run_script(UPDATE, "apply", "--root", str(printed), "--yes", + "--upstream", str(holder_project["upstream"]), "--at", + holder_project["b"], "--branch", "shape/update-fam") + assert result.returncode == 0, result.stdout + result.stderr + argv = create_argv(next_line_of(result.stdout)) + printed_body = argv[argv.index("--body") + 1] + _, opened_body = opened_by_pr(update_shape, gh_calls, opened, + holder_project, "b", "shape/update-fam") + + assert printed_body == opened_body + for body in (printed_body, opened_body): + assert ("`contracts/shape-pin.yaml` and `family.yaml` re-pinned onto " + f"{holder_project['b']}") in body, body + assert "project.yaml" not in body, body + args = SimpleNamespace(branch="shape/x", push=True, pr=False) + failed_push = update_shape.pull_request_line( + opened, args, holder_project["b"], + SimpleNamespace(repository=SHAPE_REPOSITORY), update_shape.FAMILY_KIND) + assert "`family.yaml` re-pinned" in failed_push[failed_push.index( + "--body") + 1] + assert "project.yaml" not in " ".join(failed_push) + + +def failing_once(command, calls, module): + """`gh`, as a stub: no pull request exists, and the first one it is asked + to open fails, as it does when `gh` is not logged in.""" + if command[:3] == ["gh", "pr", "list"]: + return "[]" + if sum(1 for each in calls if each[:3] == ["gh", "pr", "create"]) == 1: + raise module.CommandFailed(command, None, 1, "stubbed: gh failed") + return "https://example.invalid/pull/1" + + +def creations(calls: list) -> list[list[str]]: + return [call for call in calls if call[:3] == ["gh", "pr", "create"]] + + +def adopt_the_makefile(root: Path) -> None: + """As an in-place adoption leaves it: the project's own `Makefile` carries + a block the standard's does not, and the pin's row records THOSE bytes. + Committed, so `--branch` takes the run.""" + makefile = root / "Makefile" + makefile.write_bytes(makefile.read_bytes() + b"\n# the adoption's block\n") + pin = root / PIN + old = pin_rows(root)["Makefile"] + text, count = re.subn(old, file_sha256(makefile), + pin.read_bytes().decode("utf-8"), flags=re.I) + assert count == 1, "fixture: one row holds that digest" + pin.write_bytes(text.encode("utf-8")) + git("add", "--", "Makefile", PIN, cwd=root) + git("commit", "-q", "-m", "Adopt in place", cwd=root) + # ... and landed: the tracking branch the clone has seen holds it too. + git("update-ref", "refs/remotes/origin/main", "HEAD", cwd=root) + + +KEPT = (f"{LOCAL} was not re-copied: `--accept-local` kept it as the project " + "holds it, so the pin's row for it records a digest of the project's " + "own bytes, not of the upstream's file.") + + +@pytest.mark.parametrize("adopted", [False, True], + ids=["plain", "adopted-makefile"]) +def test_the_rerun_that_opens_the_missing_pull_request_names_what_was_kept( + update_shape, monkeypatch, project, root, adopted): + """THE RE-RUN (`_finish_landing`). The first run commits, and its `gh pr + create` fails; `apply --pr` run again has nothing to write and opens the + pull request, with no rows to say what `--accept-local` kept. Its body + names the same path, read back out of the pin. An in-place adoption's + `Makefile` row records bytes the standard's file does not hold and was + never accepted: it is not named, and the body says what is true.""" + calls = stub_gh(update_shape, monkeypatch, failing_once) + if adopted: + adopt_the_makefile(root) + edit(root, LOCAL, commit=True) + + first = apply_in_process(update_shape, root, project, "b", + "shape/update-rerun", "--accept-local", LOCAL) + again = apply_in_process(update_shape, root, project, "b", + "shape/update-rerun") + + assert first == 2 and again == 0 + opened_first, opened_again = [call[call.index("--body") + 1] + for call in creations(calls)] + assert opened_first.endswith(KEPT), opened_first + assert opened_again == opened_first, ( + "the re-run opens the very command the first run tried") + assert "Makefile" not in opened_again + + +def test_the_rerun_of_a_run_that_accepted_nothing_names_nothing( + update_shape, monkeypatch, project, root): + calls = stub_gh(update_shape, monkeypatch, failing_once) + adopt_the_makefile(root) + + first = apply_in_process(update_shape, root, project, "b", + "shape/update-rerun-none") + again = apply_in_process(update_shape, root, project, "b", + "shape/update-rerun-none") + + assert first == 2 and again == 0 + body = creations(calls)[1] + assert "not re-copied" not in body[body.index("--body") + 1] + + +# --- N1: a commit line with nothing to commit ----------------------------------- + + +@pytest.fixture +def repository(tmp_path) -> Path: + """A root that is a git repository holding the pin and the manifest, as + HEAD commits them.""" + where = tmp_path / "root" + (where / "contracts").mkdir(parents=True) + (where / PIN).write_bytes(b"commit: aaaa\n") + (where / MANIFEST).write_bytes(b"name: Atlas\n") + git("init", "-q", "-b", "main", cwd=where) + git("add", "--", PIN, MANIFEST, cwd=where) + git("commit", "-q", "-m", "Scaffold", cwd=where) + return where + + +def said_by_next_line(update_shape, root, paths, added=(), capsys=None) -> str: + args = SimpleNamespace(branch=None, push=False, pr=False) + update_shape.next_line(root, args, list(paths), "ab" * 20, list(added)) + return capsys.readouterr().out + + +def test_no_commit_line_when_every_written_path_is_what_head_commits( + update_shape, repository, capsys): + out = said_by_next_line(update_shape, repository, [PIN, MANIFEST], + capsys=capsys) + + assert out.startswith("NEXT nothing to commit: the working tree now " + "holds what HEAD commits at " + f"{PIN}, {MANIFEST}"), out + assert out.count("\n") == 1 + assert "commit -m" not in out and "checkout -b" not in out + + +def test_the_commit_line_stands_when_one_written_path_differs_from_head( + update_shape, repository, capsys): + (repository / PIN).write_bytes(b"commit: bbbb\n") + + out = said_by_next_line(update_shape, repository, [PIN, MANIFEST], + capsys=capsys) + + assert out.startswith("NEXT git -C "), out + assert " commit -m 'Re-sync the shape @ " in out + + +def test_a_run_that_added_a_file_still_gets_its_commit_line( + update_shape, repository, capsys): + """An `--add` file is not in HEAD, so what this run wrote is a change + even when the pin and the manifest are HEAD's bytes.""" + (repository / "docs").mkdir() + (repository / "docs" / "new.md").write_bytes(b"new\n") + + out = said_by_next_line(update_shape, repository, + [PIN, MANIFEST, "docs/new.md"], ["docs/new.md"], + capsys) + + assert "--literal-pathspecs add -f -- docs/new.md" in out, out + assert " commit -m 'Re-sync the shape @ " in out + + +def test_a_root_with_no_commit_gets_the_line_it_always_got( + update_shape, tmp_path, capsys): + """No HEAD, nothing to compare with: even paths that are not on disk, for + which HEAD and the disk agree where there is a HEAD, get the line.""" + where = tmp_path / "fresh" + where.mkdir() + git("init", "-q", "-b", "main", cwd=where) + + out = said_by_next_line(update_shape, where, [PIN, MANIFEST], + capsys=capsys) + + assert out.startswith("NEXT git -C "), out + assert "nothing to commit" not in out + + +@WINDOWS_SKIP +def test_a_written_path_whose_mode_differs_from_head_still_gets_its_commit_line( + update_shape, repository, capsys): + """`_not_what_head_commits` compares blobs, which a mode change leaves + alone, and git commits the change: the file is HEAD's bytes and not + HEAD's mode, so "nothing to commit" would be false (review of PR #558). + POSIX only: Windows runs with `core.fileMode=false`.""" + assert git("config", "core.fileMode", cwd=repository).stdout.strip() \ + == "true", "fixture: modes are tracked" + (repository / PIN).chmod(0o755) + assert git("diff", "--name-only", "HEAD", cwd=repository).stdout.split() \ + == [PIN], "fixture: git sees the mode change" + + out = said_by_next_line(update_shape, repository, [PIN, MANIFEST], + capsys=capsys) + + assert out.startswith("NEXT git -C "), out + assert " commit -m 'Re-sync the shape @ " in out + assert "nothing to commit" not in out + + +def test_an_apply_that_moves_the_pin_back_to_heads_prints_no_commit_line( + project, root): + """THE ISSUE'S RUN, end to end. `apply` at B leaves the update written and + uncommitted, and prints its commit line. `apply` at A then moves the copy, + the pin and the manifest back to the bytes HEAD commits: the tree is clean + and the one NEXT line says there is nothing to commit, where it used to + print a `checkout -b && commit` that git answers with "nothing to + commit".""" + first = run_apply(root, project, "b") + assert first.returncode == 0, first.stdout + first.stderr + assert " commit -m 'Re-sync the shape @ " in next_line_of(first.stdout) + assert git("status", "--porcelain", cwd=root).stdout != "" + + back = run_apply(root, project, "a") + + assert back.returncode == 0, back.stdout + back.stderr + assert git("status", "--porcelain", cwd=root).stdout == "" + line = next_line_of(back.stdout) + assert line.startswith("NEXT nothing to commit: "), line + assert "commit -m" not in back.stdout diff --git a/tests/test_update_shape_next_quoting.py b/tests/test_update_shape_next_quoting.py index 50330ec1..84b7fe7f 100644 --- a/tests/test_update_shape_next_quoting.py +++ b/tests/test_update_shape_next_quoting.py @@ -49,6 +49,7 @@ sys.path.insert(0, str(REPO / "scripts")) from repo_shape import PYTHON # noqa: E402 +from shape_materialize import SHAPE_REPOSITORY # noqa: E402 UPDATE = REPO / "update-shape.py" PIN = "contracts/shape-pin.yaml" @@ -156,6 +157,23 @@ def steps(argv: list[str]) -> list[list[str]]: return out +#: The first words of the body of every pull request this tool opens or tells a +#: person to open (#535). What the rest of it says is +#: `tests/test_update_shape_messages.py`'s. +BODY_OPENS = "Written by openRepoShape's `update-shape.py`: " + + +def pr_words(project: dict) -> tuple[str, str]: + """`(title, body)` of the pull request for the run these tests make, spelled + out by hand: the line a person pastes carries both (#535), because a + `gh pr create` without them has `gh` prompt for them.""" + return (f"Re-sync the shape copies to {SHAPE_REPOSITORY} @ " + f"{project['b'][:12]}", + BODY_OPENS + "`contracts/shape-pin.yaml` and `project.yaml` " + f"re-pinned onto {project['b']}. No copy re-copied from the " + "upstream was hand-edited and every digest was recomputed.") + + def check_run(project, root, *, lane: str = "", upstream=None): result = run_script(UPDATE, "check", "--root", str(root), "--upstream", str(upstream or project["upstream"]), @@ -241,10 +259,11 @@ def test_the_branch_next_line_is_one_argv(project, make_root, name): branch = "shape/next-quoting-branch" result = apply(root, project, "--branch", branch) 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, "&&", "gh", "pr", "create", "--repo", f"{ORG}/{PROJECT}", "--base", - "main", "--head", branch] + "main", "--head", branch, "--title", title, "--body", body] def test_the_branch_and_push_next_line_is_one_argv(project, make_root): @@ -255,9 +274,10 @@ def test_the_branch_and_push_next_line_is_one_argv(project, make_root): branch = "shape/next-quoting-pushed" result = apply(root, project, "--branch", branch, "--push") assert result.returncode == 0, result.stdout + result.stderr + title, body = pr_words(project) assert argv_of(next_line_of(result.stdout)) == [ "gh", "pr", "create", "--repo", f"{ORG}/{PROJECT}", "--base", "main", - "--head", branch] + "--head", branch, "--title", title, "--body", body] @pytest.mark.parametrize("name", AWKWARD) @@ -330,7 +350,9 @@ def test_the_branch_is_quoted_even_where_its_alphabet_makes_that_a_no_op( "bare; `_by_hand` says so of its own lines") 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.""" + 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.""" root = make_root("plain") for path in (root.resolve(), project["upstream"].resolve()): assert shlex.quote(str(path)) == str(path), ( @@ -349,9 +371,11 @@ def test_a_plain_root_prints_the_bytes_it_always_printed(project, make_root): pushing = make_root("plain-too") branched = apply(pushing, project, "--branch", branch) 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"gh pr create --repo {ORG}/{PROJECT} --base main --head {branch}") + f"gh pr create --repo {ORG}/{PROJECT} --base main --head {branch} " + f"--title {shlex.quote(title)} --body {shlex.quote(body)}") bare = make_root("plain-three") unbranched = apply(bare, project) @@ -449,9 +473,10 @@ def test_the_branch_line_pushes_and_opens_when_it_is_pasted(project, make_root, pasted = paste(next_line_of(result.stdout), tmp_path, env) assert pasted.returncode == 0, pasted.stdout + pasted.stderr + title, body = pr_words(project) assert log.read_text(encoding="utf-8").split("\n")[:-1] == [ "pr", "create", "--repo", f"{ORG}/{PROJECT}", "--base", "main", - "--head", branch] + "--head", branch, "--title", title, "--body", body] remote = git("config", "remote.origin.url", cwd=root).stdout.strip() assert git("rev-parse", "--verify", "--quiet", f"refs/heads/{branch}", cwd=Path(remote)).stdout.strip() == git( diff --git a/tests/test_update_shape_pr_base.py b/tests/test_update_shape_pr_base.py index c895b4b1..1b8237ac 100644 --- a/tests/test_update_shape_pr_base.py +++ b/tests/test_update_shape_pr_base.py @@ -41,6 +41,16 @@ UPDATE = REPO / "update-shape.py" +def without_its_words(line: str) -> str: + """A printed `gh pr create` less the `--title --body ` it ends on + (#535): each is checked to be there, and what they say is + `tests/test_update_shape_messages.py`'s. What is left is the part these + tests are about, the repository, the base and the head.""" + assert " --title " in line and " --body " in line, line + assert "--fill" not in line, line + return line.partition(" --title ")[0] + + def project_with_an_upstream_fix(tmp_path_factory, subdir: str, *scaffold_args: str) -> dict: """A project at A, and an upstream that has since moved to B — the shape @@ -156,8 +166,9 @@ def test_the_next_line_names_the_roots_tracking_branch_as_the_base(case, line = next_line_of(result.stdout) assert "gh pr create" in line assert line.count("--base") == 1, line - assert line.endswith(f"gh pr create --repo {ORG}/{PROJECT} --base " - f"{case.base} --head {branch}"), line + assert without_its_words(line).endswith( + f"gh pr create --repo {ORG}/{PROJECT} --base {case.base} --head " + f"{branch}"), line def test_the_next_line_after_a_push_names_the_repository_not_a_placeholder( @@ -170,8 +181,9 @@ def test_the_next_line_after_a_push_names_the_repository_not_a_placeholder( assert result.returncode == 0, result.stdout + result.stderr line = next_line_of(result.stdout) assert "<" not in line and ">" not in line, line - assert line == (f"NEXT gh pr create --repo {ORG}/{PROJECT} --base " - f"{case.base} --head {branch}"), line + assert without_its_words(line) == ( + f"NEXT gh pr create --repo {ORG}/{PROJECT} --base {case.base} " + f"--head {branch}"), line def test_the_pull_request_is_opened_against_the_base_the_next_line_names( @@ -205,11 +217,19 @@ def test_the_pull_request_is_opened_against_the_base_the_next_line_names( assert opened_against == case.base assert opened_in == f"{ORG}/{PROJECT}" plain, branch = printed[()] - assert plain.endswith(f" && gh pr create --repo {opened_in} --base " - f"{opened_against} --head {branch}"), plain + assert without_its_words(plain).endswith( + f" && gh pr create --repo {opened_in} --base {opened_against} " + f"--head {branch}"), plain pushed, branch = printed[("--push",)] - assert pushed == (f"NEXT gh pr create --repo {opened_in} --base " - f"{opened_against} --head {branch}"), pushed + assert without_its_words(pushed) == ( + f"NEXT gh pr create --repo {opened_in} --base {opened_against} " + f"--head {branch}"), pushed + # THE WORDS TOO (#535): what a person pastes opens the pull request with + # the title and the body `--pr` opens it with. + for line in (plain, pushed): + words = shlex.split(line)[-4:] + assert words == ["--title", command[command.index("--title") + 1], + "--body", command[command.index("--body") + 1]], line @pytest.mark.parametrize("kind_name", ["PROJECT_KIND", "FAMILY_KIND"]) @@ -231,7 +251,8 @@ def test_each_kind_of_root_is_asked_in_its_own_manifest( update_shape.next_line(tmp_path, args, [], "0" * 40, [], kind) line = capsys.readouterr().out.strip() assert line.startswith("NEXT"), line - assert line.endswith(f"--base {base} --head shape/x"), line + assert without_its_words(line).endswith( + f"--base {base} --head shape/x"), line #: Manifests a family holder and a project can declare their repository in, @@ -269,8 +290,9 @@ def test_the_repository_is_read_where_open_pull_request_reads_it( args = SimpleNamespace(branch="shape/x", push=push, pr=False) update_shape.next_line(tmp_path, args, [], "0" * 40, [], kind) line = capsys.readouterr().out.strip() - assert line.endswith(f"gh pr create --repo {repository} --base main " - "--head shape/x"), line + assert without_its_words(line).endswith( + f"gh pr create --repo {repository} --base main --head shape/x"), \ + line assert line.startswith("NEXT gh pr create" if push else "NEXT git -C "), line @@ -315,18 +337,20 @@ def test_a_manifest_naming_no_repository_refuses_pr_and_leaves_the_placeholder( args = SimpleNamespace(branch="shape/x", push=True, pr=False) update_shape.next_line(tmp_path, args, [], "0" * 40, [], kind) - assert capsys.readouterr().out.strip() == ( + assert without_its_words(capsys.readouterr().out.strip()) == ( "NEXT gh pr create --repo / --base main --head shape/x") args = SimpleNamespace(branch="shape/x", push=False, pr=False) update_shape.next_line(tmp_path, args, [], "0" * 40, [], kind) - assert capsys.readouterr().out.strip().endswith( + assert without_its_words(capsys.readouterr().out.strip()).endswith( " && gh pr create --base main --head shape/x") # The line a failed `--push` prints (#204) leaves `--repo` off too: a # placeholder would be one literal argument in a line quoted as an argv. args = SimpleNamespace(branch="shape/x", push=True, pr=False) - assert update_shape.pull_request_line( - tmp_path, args, "0" * 40, SimpleNamespace(repository="o/r"), - kind) == ["gh", "pr", "create", "--base", "main", "--head", "shape/x"] + create = update_shape.pull_request_line( + tmp_path, args, "0" * 40, SimpleNamespace(repository="o/r"), kind) + assert create[:7] == ["gh", "pr", "create", "--base", "main", "--head", + "shape/x"], create + assert create[7] == "--title" and create[9] == "--body", create def test_nothing_in_the_next_line_names_a_base_when_the_pull_request_is_open( @@ -356,9 +380,11 @@ def test_a_value_the_manifest_holds_is_quoted_for_the_shell_that_reads_the_line( update_shape.next_line(tmp_path, args, [], "0" * 40, []) line = capsys.readouterr().out.strip() command = line.partition(" && ")[2] if not push else line[len("NEXT "):] - assert shlex.split(command) == [ + words = shlex.split(command) + assert words[:9] == [ "gh", "pr", "create", "--repo", "acme/It's here", "--base", "a&b", "--head", "shape/x"], line + assert words[9] == "--title" and words[11] == "--body", line FAMILY_SOURCE = "templates/family-root/scripts/validate-family.py" @@ -403,8 +429,9 @@ def test_a_family_holder_is_asked_in_its_own_manifest_after_the_commit( assert result.returncode == 0, result.stdout + result.stderr assert f"committed on {branch}" in result.stdout, result.stdout line = next_line_of(result.stdout) - assert line.endswith(f"gh pr create --repo {ORG}/{HOLDER} --base trunk " - f"--head {branch}"), line + assert without_its_words(line).endswith( + f"gh pr create --repo {ORG}/{HOLDER} --base trunk --head {branch}"), \ + line assert line.startswith("NEXT git -C " if not flags else "NEXT gh pr create"), line assert git("status", "--porcelain", cwd=holder).stdout == "" diff --git a/update-shape.py b/update-shape.py index 5ac39186..7b06b0e9 100755 --- a/update-shape.py +++ b/update-shape.py @@ -151,6 +151,11 @@ checked_value, die, file_sha256, git_out, load_yaml, parse_yaml, tree_digest, ) +# 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 from shape_materialize import ( # noqa: E402 ATTRIBUTES, COPIED_FROM_SHAPE, COPIED_VERBATIM, FAMILY_COPIED_FROM_SHAPE, FAMILY_COPIED_VERBATIM, SHAPE_REPOSITORY, CommandFailed, check_program, @@ -160,6 +165,13 @@ is_nested_key, readme_trailing_shape_line, rewrite_fault, run, trailer_line, with_scalar, ) +# What the branch question is asked with (#431): the one helper that asks git +# with no repository in play, and the two words for a git that could not +# answer. Private and imported on purpose, as `_strip_comment` is above: a +# second copy of the question is how two tools come to judge one name apart. +from shape_materialize import ( # noqa: E402 + BRANCH_CONTROL, BRANCH_UNCHECKED_REMEDIATION, _ask_git_branch, +) # Where a root keeps its shape pin. The materializer that writes it names it # once; this module spells the path by that name and never by a literal (#290). from shape_materialize import SHAPE_PIN # noqa: E402 @@ -436,6 +448,10 @@ def __init__(self, path: str, recorded: str): self.path = path self.recorded = (recorded or "").lower() self.source: str | None = None + #: The commit `source` is read at: the run's target, set where the + #: source is, so a remedy can name `git show :` whole + #: (#535). + self.target: str | None = None self.state = UNCHANGED self.detail = "" self.target_bytes: bytes | None = None @@ -678,6 +694,7 @@ def _classify_row(root: Path, row: Row, upstream: Upstream, pinned: str, return source = source_for(row.path, upstream, target, kind) row.source = source + row.target = target if source is None: row.state = UNMAPPED row.detail = ("no file in the upstream tree corresponds to this " @@ -726,7 +743,12 @@ def _row_verdict(row: Row, source: str, here: bytes, return if locally_modified and upstream_changed: row.state = BOTH - row.detail = "edited here AND upstream since the pin" + # THE UPSTREAM'S FILE IS NAMED (#535): the remedy sends the person to + # read its bytes, and a copy's path in the root is not its path in the + # standard (`scripts/validate-manifest.py` is + # `templates/assembly-root/scripts/validate-manifest.py`). + row.detail = ("edited here AND upstream since the pin " + f"(upstream's file: {source})") # Precomputed the same way `locally-modified` precomputes it, # below: IF `--accept-local` later names this path, `cmd_apply` # must re-pin from what is on disk NOW, not from whatever was @@ -1339,6 +1361,24 @@ def resolve_additions(rows: list[Row], additions: list[Addition], # --------------------------------------------------------------------------- +def _is_block_header(line: str, key: str) -> bool: + """Is `line` the column-0 header `:` of a block, a trailing + `# 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}:" + + +def _header_as_it_was(line: str) -> str: + """`line`, a block header, with its comment and the spacing in front of + it kept (#437). A header with no comment loses trailing whitespace, as + `shape_materialize.with_scalar` says of its own lines.""" + body = _strip_comment(line) + tail = line[len(body):] + return body + (tail if tail.strip() else "") + + def rewrite_shape_pin(text: str, commit: str, tree: str, rows: list[Row]) -> str: """`commit:`, `digests.tree_sha256:` and the whole `files:` block. @@ -1359,6 +1399,18 @@ def rewrite_shape_pin(text: str, commit: str, tree: str, four spaces was refused as hand-edited and a comment was dropped. The `files:` block below them is still written whole, as before. + THE `files:` HEADER IS FOUND WITH THE READER'S RULE FOR A COMMENT (#437): + at column 0, once the reader's own `_strip_comment` has dropped a + 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 + of it included, like the two lines above; only the rows under it are + written whole. + WHAT IS RETURNED HAS BEEN READ BACK, as `rewrite_manifest`'s is (`shape_materialize.rewrite_fault`): a layout this line rewrite cannot carry is refused HERE as this tool's failure, before any byte is written. @@ -1375,9 +1427,9 @@ def rewrite_shape_pin(text: str, commit: str, tree: str, seen["tree"] += 1 out.append(with_scalar(line, "tree_sha256", tree)) continue - if line.rstrip() == "files:": + if _is_block_header(line, "files"): seen["files"] += 1 - out.append("files:") + out.append(_header_as_it_was(line)) for row in rows: out.append(f" - path: {row.path}") out.append(f' sha256: "{row.digest}"') @@ -1496,12 +1548,38 @@ def _shape_block_bounds(lines: list[str], kind: Kind) -> tuple[int, int]: next top-level key — or at the end of the file, which is the `shape:` block a scaffold writes last. No `shape:` block at all is the one refusal, raised here because there is nothing to return. + + THE HEADER IS FOUND WITH THE READER'S RULE FOR A COMMENT (#437): at + column 0, once `_strip_comment` has dropped a trailing `# comment`. It + 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. + + 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 + nobody reads. That was left to `rewrite_fault`'s read-back, whose words + blame this tool's line rewrite, which is false of a key written twice. + And where the first header carries a comment, the exact match skipped it + and rewrote the last block, which is what the reader reads, so such a + manifest went through; this finder sees the first header, and would + have refused it in those same false words. """ - start = None - for index, line in enumerate(lines): - if line.rstrip() == "shape:": - start = index - break + starts = [index for index, line in enumerate(lines) + if _is_block_header(line, "shape")] + if len(starts) > 1: + raise Refusal( + "update-manifest-unrecognised", + f"{kind.manifest} holds the `shape:` key twice, at lines " + + " and ".join(str(index + 1) for index in starts[:2]) + + ", where exactly one was expected: the reader keeps the last " + "block, so this tool will not guess which of them is the " + "manifest's", + "Remediation: delete the extra `shape:` block by hand (the " + "validators read the last one) and run the same command again.") + start = starts[0] if starts else None if start is None: raise Refusal( "update-manifest-no-shape", @@ -2151,6 +2229,25 @@ def checked_branch(value: str) -> str: put to `git check-ref-format --branch`, which is the authority on what a branch name is: it is asked, not copied. A spelling it would expand rather than echo is refused too: `apply` names the branch it creates. + + GIT IS ASKED WITH NO REPOSITORY IN PLAY (#431), through + `shape_materialize._ask_git_branch`, the one helper that asks it so for + the scaffold and the family holder too. Asked in the operator's own + directory, `--branch` set up whatever repository surrounded it and died + where git could not open it (a `.git` file naming a gitdir that is gone, + a `.git/config` that does not parse, a directory removed under the + shell), and the refusal below read that failure as the name's fault. A + repository it did open lent `@{-1}` its own reflog. `--root` names the + repository `apply` works on and has nothing to do with where the person + stands, so the answer is the same from every directory. + + A GIT THAT COULD NOT ANSWER IS NOT THE NAME'S FAULT. When git refuses the + name it is asked about `BRANCH_CONTROL`, a name every git creates, as + `shape_materialize._branch_verdict` does: a git that refuses that too + judged nothing, and the refusal says the name "could not be checked" + and sends the person to git (`BRANCH_UNCHECKED_REMEDIATION`), not to + another name. The code, the `--branch '' ` opening of the detail + and `BRANCH_REMEDIATION` for a name git judged are unchanged. """ if not BRANCH_RE.fullmatch(value): raise Refusal( @@ -2160,25 +2257,51 @@ def checked_branch(value: str) -> str: "digit and hold only letters, digits and `. _ / @ + -`", BRANCH_REMEDIATION) try: - verdict = subprocess.run(["git", "check-ref-format", "--branch", value], - capture_output=True, text=True, - encoding="utf-8", errors="replace", - check=False) + answer = _ask_git_branch(value) + except CommandFailed as exc: + said = f" ({exc.output})" if exc.output else "" + if _git_cannot_answer_for_any_name(): + raise Refusal( + "update-branch-invalid", + f"--branch {value!r} could not be checked: `git " + f"check-ref-format --branch` fails for `{BRANCH_CONTROL}` " + f"too, so git could not answer{said}", + BRANCH_UNCHECKED_REMEDIATION) from exc + raise Refusal( + "update-branch-invalid", + f"--branch {value!r} is not a branch name git will create: `git " + f"check-ref-format --branch` refuses it{said}", + BRANCH_REMEDIATION) from exc except OSError as exc: + # `git` is missing, or no empty directory could be made to ask it + # from: the answer is not git's, and the fault is not the name's. raise Refusal( "update-branch-invalid", f"--branch {value!r} could not be checked: `git check-ref-format " - f"--branch` could not be run ({exc})", BRANCH_REMEDIATION) from exc - if verdict.returncode != 0 or verdict.stdout.strip() != value: - said = (verdict.stderr or verdict.stdout).strip() + f"--branch` could not be run ({exc})", + BRANCH_UNCHECKED_REMEDIATION) from exc + if answer != value: raise Refusal( "update-branch-invalid", f"--branch {value!r} is not a branch name git will create: `git " - "check-ref-format --branch` refuses it" - + (f" ({said})" if said else ""), BRANCH_REMEDIATION) + f"check-ref-format --branch` reads it as {answer!r}, not as " + "written", BRANCH_REMEDIATION) return value +def _git_cannot_answer_for_any_name() -> bool: + """Does `git check-ref-format --branch` fail for `BRANCH_CONTROL`, a name + every git creates as a branch? Git's words are localised, so they are + never read: a git that refuses this one too could not answer (a + malformed global config, say), and the name it was first asked about was + never judged (#431).""" + try: + _ask_git_branch(BRANCH_CONTROL) + except (CommandFailed, OSError): + return True + return False + + def branch_exists(root: Path, branch: str) -> bool: """Is `branch` a local branch of `root` — checked out or not?""" proc = subprocess.run( @@ -2245,7 +2368,8 @@ def pull_request_repository(manifest: dict | None, def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, upstream: Upstream, count: int, kind: Kind, added: list[str] = (), trailers: list[str] = (), *, - ledger: Rollback, dropped: list[str] = ()) -> None: + ledger: Rollback, dropped: list[str] = (), + accepted: list[str] = ()) -> None: """A branch and ONE commit, with EXPLICIT PATHSPECS. `git commit -- ` commits the working-tree state of exactly those @@ -2300,6 +2424,14 @@ def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, writes its own commit message can be told any other way. Passed none, the message is exactly the one this function has always written; see `shape_materialize.commit_trailers` for the two ways git is asked. + + THE MESSAGE CLAIMS "NOT HAND-EDITED" ONLY OF WHAT WAS RE-COPIED (#535). + `accepted` are the paths `--accept-local` kept as the project holds them, + which the run did not copy and whose pin rows it recomputed from the + project's own bytes: the sentence it ended on, "the copies are not + hand-edited", was false of exactly those files, in a history nobody + rewrites. It now says it of the copies re-copied from the upstream, and a + run with `accepted` names them in a paragraph of their own. """ checked_value("--branch", branch) manifest = load_yaml(root / kind.manifest) @@ -2345,13 +2477,14 @@ def commit_on_branch(root: Path, branch: str, paths: list[str], target: str, # an unexplained edit to a document the project owns (#148). prose = (f", and {README}'s `Shape:` line now names it too" if README in paths else "") + kept = f"{accepted_local_said(accepted)}\n\n" if accepted else "" message = ( f"Re-sync the shape copies to {upstream.repository} @ {target[:12]}\n\n" f"{count} copied file(s) re-copied from the upstream{gained}; " f"contracts/shape-pin.yaml and {kind.manifest}'s `shape:` block now " - f"record {target}{prose}.\n\nWritten by update-shape.py; the copies " - f"are not hand-edited and the digests are recomputed, not " - f"adjusted.\n") + f"record {target}{prose}.\n\n{kept}Written by update-shape.py; the " + f"copies re-copied from the upstream are not hand-edited and the " + f"digests are recomputed, not adjusted.\n") env = dict(os.environ) for key, fallback in (("GIT_AUTHOR_NAME", "openRepoShape update"), ("GIT_COMMITTER_NAME", "openRepoShape update"), @@ -2910,14 +3043,37 @@ def _refuse_conflicts(rows: list[Row], accepted: set) -> None: conflict_remediation(conflicts)) -#: What `update-conflict` tells a row it can name a merge for: `both`, and -#: the two states that cannot be compared. The words it always printed, so a -#: refusal naming no `copy-missing` row reads exactly as it did. -MERGE_BY_HAND = ( - "merge each one by hand (the upstream bytes are `git show " - ":` in a shape clone), commit it, then re-run this " - "with --accept-local on that path so the row is recomputed from what " - "you merged.") +def merge_by_hand(rows: list[Row]) -> str: + """What `update-conflict` tells the rows it can name a merge for: `both`, + and the two states that cannot be compared. The words it always printed, + with the upstream's bytes NAMED where there are some to name (#535). + + The remedy said `git show :` and nothing in the + refusal or in `check` said what either was. A `both` row holds both, so + the command is spelled whole: the full target commit, which `git show` + reads in a shape clone, and the path the standard keeps the file at, + which is not the path in the root (`scripts/validate-manifest.py` is + `templates/assembly-root/scripts/validate-manifest.py`). One row is the + sentence it always was with its two placeholders filled; several are one + command each, by the path in the root they are for. A row that is not + `both` is named no command, because none reads: `unmapped` has no + upstream file, and an `upstream-removed` one is gone at the target, where + `git show` would fail. + """ + shows = [(row.path, f"git show {row.target}:{row.source}") + for row in rows + if row.state == BOTH and row.source and row.target] + if len(shows) == 1: + where = f" (the upstream bytes are `{shows[0][1]}` in a shape clone)" + elif shows: + where = (" (the upstream bytes, in a shape clone: " + + "; ".join(f"`{command}` for {path}" + for path, command in shows) + ")") + else: + where = "" + return (f"merge each one by hand{where}, commit it, then re-run this " + "with --accept-local on that path so the row is recomputed from " + "what you merged.") def conflict_remediation(conflicts: list[Row]) -> str: @@ -2945,12 +3101,14 @@ def conflict_remediation(conflicts: list[Row]) -> str: row. The sentence says so, because a person who removed the copy without the merge reads it here. - The other three states keep `MERGE_BY_HAND`, after it where a - `copy-missing` row is named too. + The other three states keep `merge_by_hand`'s sentence, after it where a + `copy-missing` row is named too, naming each `both` row's upstream file + (#535). """ missing = [row.path for row in conflicts if row.state == COPY_MISSING] + others = [row for row in conflicts if row.state != COPY_MISSING] if not missing: - return f"Remediation: {MERGE_BY_HAND}" + return f"Remediation: {merge_by_hand(conflicts)}" verb = "is" if len(missing) == 1 else "are" said = (f"Remediation: {', '.join(missing)} {verb} pinned and gone from " "the root, so --accept-local has no bytes to keep, and this " @@ -2963,8 +3121,8 @@ def conflict_remediation(conflicts: list[Row]) -> str: "copy merged into the file it collided with is not refused: " "with that file at the root, `check` reports it merged-away and " "`apply` drops its row.") - if len(missing) < len(conflicts): - said += f" For the others, {MERGE_BY_HAND}" + if others: + said += f" For the others, {merge_by_hand(others)}" return said @@ -3577,13 +3735,27 @@ def _refuse_red_validators(root: Path, kind: Kind) -> None: "somebody else's pull request.") +def _accepted_local(rows: list[Row]) -> list[str]: + """The paths `--accept-local` kept as the project holds them: the rows + still `locally-modified` or `both` when the writes are done, since a row + in either state that nobody accepted was refused before the first one + (#535). Sorted, so a message names them in an order that does not move. + `already-at-target` is not one: the project's bytes ARE the standard's + there.""" + return sorted(row.path for row in rows + if row.state in (LOCALLY_MODIFIED, BOTH)) + + def _land(root: Path, args, paths: list[str], added: list[str], target: str, upstream: Upstream, kind: Kind, count: int, - ledger: Rollback, dropped: list[str] = ()) -> None: + ledger: Rollback, dropped: list[str] = (), + accepted: list[str] = ()) -> None: """The three optional acts after a clean rewrite — commit, push, open a pull request — each done only because a flag asked for it. Split out of `cmd_apply` for #133. `dropped` names the merged-away rows the re-pin - left out (#502), for the commit message. + left out (#502), for the commit message; `accepted` the paths + `--accept-local` kept as the project holds them (#535), which neither the + commit message nor the pull request's body may call "not hand-edited". Nested the way the flags are: `--push` and `--pr` have nothing to act on without `--branch`, which `main` refuses before this is ever reached. @@ -3599,11 +3771,17 @@ def _land(root: Path, args, paths: list[str], added: list[str], target: str, """ if not args.branch: return - create = (pull_request_line(root, args, target, upstream, kind) + # THE WORDS `--pr` OPENS WITH, kept for `next_line`, which runs after this + # and prints the same title and body in the `gh pr create` it hands a + # person (#535): it is not given the upstream or the accepted paths, and + # `cmd_apply`'s call to it does not change. + args.pull_request_words = pull_request_words(upstream.repository, target, + kind.manifest, accepted) + create = (pull_request_line(root, args, target, upstream, kind, accepted) if args.push or args.pr else []) commit_on_branch(root, args.branch, paths, target, upstream, count, kind, added, args.trailer, ledger=ledger, - dropped=dropped) + dropped=dropped, accepted=accepted) print(f"\n committed on {args.branch}: " + ", ".join(paths)) if args.push: push_branch(root, args, create) @@ -3670,7 +3848,7 @@ def cmd_apply(args) -> int: # as re-copied nor asked about as a copy the commit leaves out. dropped = [row.path for row in rows if row.state == MERGED_AWAY] _land(root, args, paths, added, target, upstream, kind, len(copied), - ledger, dropped) + ledger, dropped, accepted=_accepted_local(rows)) print() next_line(root, args, paths, target, added, kind, pinned_paths=[row.path for row in rows @@ -4132,6 +4310,42 @@ def _pull_requests(answer: str) -> "list[tuple[str, str]] | None": for pull in pulls] +def _accepted_in_the_pin(root: Path, upstream: Upstream, target: str, + kind: Kind) -> list[str]: + """The pinned copies a run that has already committed kept as the + project holds them, read back out of the pin it wrote (#535). + + A ROW IS ONE WHEN IT IS TWO THINGS. Its recorded digest is not that of + the upstream's file at `target`, so the project's bytes are what the pin + names there; and it is not the digest the tracking branch's pin already + held for it, so THIS RUN moved it. Without the second, an in-place + adoption's `Makefile`, whose row has always recorded the file with the + adoption's block appended, would be reported as kept by a flag nobody + passed. Where the tracking branch's pin cannot be read (a clone that has + never seen `origin/`) there is nothing to tell the two apart, + and the answer is none: a body that names too few paths is incomplete, + one that names a path nobody accepted is false. Sorted, as + `_accepted_local` is. + """ + base = tracking_branch_of(load_yaml(root / kind.manifest)) + landed = _git_answer(root, ["show", f"refs/remotes/origin/{base}:" + f"{SHAPE_PIN}"]) + try: + held = {str(row["path"]): str(row["sha256"]).lower() + for row in parse_yaml(landed or "")["files"]} + except (YamlError, KeyError, TypeError): + return [] + _, rows = read_pin(root) + kept = [] + for row in rows: + source = source_for(row.path, upstream, target, kind) + blob = upstream.blob(target, source) if source else None + if (blob is not None and file_bytes_sha256(blob) != row.recorded + and held.get(row.path, row.recorded) != row.recorded): + kept.append(row.path) + return sorted(kept) + + def _finish_landing(root: Path, args, upstream: Upstream, target: str, kind: Kind) -> int: """The re-run `_awaits_its_pull_request` recognised: nothing to write, @@ -4144,13 +4358,20 @@ def _finish_landing(root: Path, args, upstream: Upstream, target: str, pushed again under `--push` (a no-op when origin has it) and handed to `open_pull_request` with the same `gh pr create` the first run built. Every failure on the way is `landing_failed`'s, named by its own step. + + THE BODY NAMES WHAT `--accept-local` KEPT, AS THE FIRST RUN'S DID (#535, + review of PR #558). This run has no rows that say so, and the flag is not + repeated by "the same command" necessarily, so the committed pin is + asked (`_accepted_in_the_pin`). """ branch = args.branch print(f"nothing to write: every copied file matches {upstream.repository}" f" @ {target[:12]}, which the pin on {branch} already names; " f"--pr asked for {branch}'s pull request") repository, _ = pull_request_target(root, kind) - create = pull_request_line(root, args, target, upstream, kind) + create = pull_request_line(root, args, target, upstream, kind, + _accepted_in_the_pin(root, upstream, target, + kind)) listing = ["gh", "pr", "list", "--repo", repository, "--head", branch, "--state", "all", "--json", "state,url"] asking = f"asking gh for {branch}'s pull request" @@ -4194,35 +4415,80 @@ def pull_request_target(root: Path, kind: Kind) -> tuple[str, str]: return repository, base +def accepted_local_said(accepted: list[str]) -> str: + """The sentence that names what `--accept-local` kept, for the commit + message and the pull request's body alike (#535): neither may say a copy + was not hand-edited of a file the run re-pinned FROM THE PROJECT'S OWN + BYTES, which is what the flag does.""" + many = len(accepted) > 1 + each = "each" if many else "it" + return (f"{', '.join(accepted)} {'were' if many else 'was'} not " + f"re-copied: `--accept-local` kept {each} as the project holds " + f"it, so the pin's row for {each} records a digest of the " + "project's own bytes, not of the upstream's file.") + + +def pull_request_words(repository: str, target: str, manifest: str, + accepted: list[str] = ()) -> tuple[str, str]: + """`(title, body)` of the pull request that lands `--branch`: what + `--pr` opens it with, and what the `gh pr create` the plain `--branch` + NEXT line hands a person carries (#535). ONE PLACE, so the two cannot + say different things. + + THE BODY NAMES THE MANIFEST THIS ROOT HAS (#535, review of PR #558): + `project.yaml` for a project and `family.yaml` for a holder, as the + commit message names `kind.manifest`. It said `project.yaml` of both. + + "NOT HAND-EDITED" IS CLAIMED OF THE COPIES RE-COPIED FROM THE UPSTREAM + ONLY, as the commit message claims it: the body said "No copy was + hand-edited" of every file, which `--accept-local` makes false of the + paths it kept, and they are named here when there are any. + + NOR DOES IT SAY ANY COPY WAS RE-COPIED. The opening clause said "the + copied shape files were re-copied from the upstream" in every state, + which is false for a run whose only change is a row `--accept-local` + kept. What the body says of the files is the sentence after it, which is + true of none, one or many. A count, as the commit message gives, is left + out on purpose: a re-run that only opens the missing pull request does + not know it, and opens the very command the first run tried + (`_finish_landing`), which the tests hold it to. + """ + title = f"Re-sync the shape copies to {repository} @ {target[:12]}" + body = ("Written by openRepoShape's `update-shape.py`: " + f"`contracts/shape-pin.yaml` and `{manifest}` re-pinned onto " + f"{target}. No copy re-copied from the upstream was hand-edited " + "and every digest was recomputed." + + (f" {accepted_local_said(accepted)}" if accepted else "")) + return title, body + + def pull_request_line(root: Path, args, target: str, upstream: Upstream, - kind: Kind) -> list[str]: + kind: Kind, accepted: list[str] = ()) -> list[str]: """The `gh pr create` that lands `--branch` (#204): under `--pr` the one `--pr` runs, title and body included, so the NEXT line a failure prints - is that same command; without it, the bare form the plain `--branch` - NEXT line prints (#209): `--repo` the manifest's repository, then - `--base`/`--head` against this root's own tracking branch. + is that same command; without it, the form the plain `--branch` NEXT + line prints (#209): `--repo` the manifest's repository, then + `--base`/`--head` against this root's own tracking branch, and the same + `--title` and `--body` (#535: a bare line has gh ask for them, which a + shell with no terminal cannot answer, and `--fill` would read commits + from wherever the line is pasted). WITHOUT `--pr` A MANIFEST NAMING NO REPOSITORY LEAVES `--repo` OFF rather than refusing: nothing is opened, so nothing is wrong yet, and the `/` placeholder `next_line` prints would be one literal argument in a line quoted to be pasted as an argv.""" + title, body = pull_request_words(upstream.repository, target, + kind.manifest, accepted) if not args.pr: manifest = load_yaml(root / kind.manifest) repository, _ = pull_request_repository(manifest, kind) where = ["--repo", repository] if repository else [] return ["gh", "pr", "create", *where, "--base", - tracking_branch_of(manifest), "--head", args.branch] + tracking_branch_of(manifest), "--head", args.branch, + "--title", title, "--body", body] repository, base = pull_request_target(root, kind) return ["gh", "pr", "create", "--repo", repository, "--base", base, - "--head", args.branch, "--title", - f"Re-sync the shape copies to {upstream.repository} @ " - f"{target[:12]}", - "--body", - "Written by openRepoShape's `update-shape.py`: the copied " - "shape files were re-copied from the upstream and " - "`contracts/shape-pin.yaml` and `project.yaml` re-pinned onto " - f"{target}. No copy was hand-edited and every digest was " - "recomputed."] + "--head", args.branch, "--title", title, "--body", body] def open_pull_request(root: Path, args, create: list[str]) -> None: @@ -4256,12 +4522,28 @@ def _unwritten_changes(root: Path, paths: list[str], the one it always printed. """ rest = [path for path in pinned_paths if path not in paths] - if not rest or _git_answer(root, ["rev-parse", "--verify", "--quiet", - "HEAD"]) is None: + if not rest or not _has_head(root): return [] return _not_what_head_commits(root, rest) +def _same_as_head(root: Path, paths: list[str]) -> bool: + """Does `git diff --quiet HEAD` find nothing among `paths`: no content, + and no file mode, that HEAD does not commit? `--literal-pathspecs`, as + the commit line is printed (#202). An answer git does not give is "no", + which is what a root with no HEAD gets: nothing there to be the same as + (#535).""" + return _git_answer(root, ["--literal-pathspecs", "diff", "--quiet", + "HEAD", "--", *paths]) is not None + + +def _has_head(root: Path) -> bool: + """Does `root` have a commit HEAD names? A root with none has nothing for + a commit line to be compared with (#436, #535).""" + return _git_answer(root, ["rev-parse", "--verify", "--quiet", + "HEAD"]) is not None + + def next_line(root: Path, args, paths: list[str], target: str, added: list[str] = (), kind: Kind = PROJECT_KIND, *, pinned_paths: list[str] = ()) -> None: @@ -4288,8 +4570,20 @@ def next_line(root: Path, args, paths: list[str], target: str, # `shlex.quote` leaves alone, so this prints the bytes it always # printed; it is quoted so that widening that alphabet one day cannot # reopen a line that has a whole argument to itself. + # + # WITH A TITLE AND A BODY (#535). Bare, the line has `gh` prompt for + # both, and in a shell with no terminal the pasted line stops there + # (gh's behaviour without one was not run: a real `gh pr create` is + # never run here). `--fill` is not the answer: it reads the commits of + # wherever the line is pasted, and `check`'s NEXT sends people to + # paste from the standard's checkout. The words are `--pr`'s, which + # `_land` kept on `args`; a caller that did not come through it is + # given the standard's own repository to name. + title, body = getattr(args, "pull_request_words", None) or \ + pull_request_words(SHAPE_REPOSITORY, target, kind.manifest) create = (f"gh pr create{where} --base {base} --head " - f"{shlex.quote(args.branch)}") + f"{shlex.quote(args.branch)} --title {shlex.quote(title)} " + f"--body {shlex.quote(body)}") if args.push: print(f"NEXT {create}") else: @@ -4316,6 +4610,25 @@ def next_line(root: Path, args, paths: list[str], target: str, "this re-pin is to carry them, or put both back to HEAD's " "bytes and run apply again") return + # NOTHING TO COMMIT IS NOT A COMMIT LINE (#535). An `apply` that moved the + # pin back to what HEAD commits writes copies, a pin and a manifest that + # are HEAD's bytes again, and the line below, pasted, is git's "nothing to + # commit". Asked of the paths THIS RUN WROTE, which is the commit's whole + # content: an `--add` file is not in HEAD, so a run that added one still + # gets its line. + # + # BY BYTES AND BY MODE (review of PR #558). `_not_what_head_commits` + # compares blobs, so a pinned script that `apply` left executable or + # not, where HEAD commits the other, would read "nothing to commit" over + # a change git does commit. `_same_as_head` asks `git diff --quiet HEAD`, + # which counts a mode change, and which is answered only where HEAD names + # a commit: a root with none has nothing to compare with, and prints the + # line it always printed. + if _same_as_head(root, paths) and not _not_what_head_commits(root, paths): + print("NEXT nothing to commit: the working tree now holds what HEAD " + f"commits at {', '.join(paths)}, so this run left no change to " + "record") + return # A `git add` for the ADDITIONS only. A hint that a human pastes has to # work when they paste it, and `git commit -- ` refuses a path git # has never heard of — which every file `--add` just wrote is. FORCED,