diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index 4a64ce4c..6ef633b9 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -43,6 +43,7 @@ import os import shlex import shutil +import socket import subprocess import sys from types import SimpleNamespace @@ -1035,3 +1036,554 @@ def test_a_crlf_pin_and_manifest_still_hold_what_head_commits(root, project, for path, data in crlf.items(): assert (root.path / path).read_bytes() == data, ( f"{path} was left as it was") + + +# --- a first `--branch` run over a tree HEAD does not commit (#436) ---------- +# +# #333's comparison, asked of every `--branch` run before its first write. The +# verdicts — what to copy, what to re-pin, whether there is anything to do — +# are read from the working tree, and the commit is HEAD plus only the paths +# the run writes. Over an in-place `apply` left uncommitted, main exited 0 on a +# commit its own `validate-pins.py` refused, died on git's "nothing to commit", +# or said "nothing to do" and made no branch; over an accepted local edit or a +# manifest edit left uncommitted, it committed without the one or with the +# other. Each is refused by name now, with nothing written. + +#: A pinned copy neither B nor C changes upstream, for the local edits. +LOCAL = "scripts/bootstrap.py" +#: The branch a first run creates. +FRESH = "shape/update-first" + + +def in_place(root, project, env, at) -> None: + """An `apply` without `--branch`, left uncommitted.""" + result = apply(root, project, env=env, at=project[at]) + assert result.returncode == 0, result.stdout + result.stderr + + +def b_in_place_on_main(root, project, env) -> None: + """THE ISSUE'S STARTING POINT: an in-place `apply --at B` on main.""" + in_place(root, project, env, "b") + + +def c_in_place_over_b_on_branch(root, project, env) -> None: + """THE ISSUE'S CASE (b): B committed on BRANCH and pushed, then an + in-place `apply --at C` there.""" + landed_without_its_pull_request(root, project, env) + in_place(root, project, env, "c") + + +def edit_a_copy(root, project, env) -> None: + """A local edit to a pinned copy, left uncommitted.""" + path = root.path / LOCAL + path.write_bytes(path.read_bytes() + b"\n# edited here, not committed\n") + + +def edit_the_manifest_on_main(root, project, env) -> None: + """An edit to `project.yaml` alone, left uncommitted.""" + manifest = root.path / "project.yaml" + manifest.write_bytes(manifest.read_bytes() + b"# not committed\n") + + +def delete_a_copy(root, project, env) -> None: + """A pinned copy deleted, the deletion left uncommitted.""" + (root.path / LOCAL).unlink() + + +@pytest.mark.parametrize("flags", [(), ("--push", "--pr")], + ids=["branch", "push-pr"]) +@pytest.mark.parametrize("prepare, extra, at, branch, differ, pins", [ + (b_in_place_on_main, (), "c", FRESH, [PIN, "project.yaml", CHANGED], + ("b", "a")), + (c_in_place_over_b_on_branch, (), "b", BRANCH, + [PIN, "project.yaml", PINS], ("c", "b")), + (b_in_place_on_main, (), "b", FRESH, [PIN, "project.yaml", CHANGED], + ("b", "a")), + (edit_a_copy, ("--accept-local", LOCAL), "b", FRESH, [LOCAL], None), + (edit_the_manifest_on_main, (), "b", FRESH, ["project.yaml"], None), + (delete_a_copy, (), "b", FRESH, [LOCAL], None), +], ids=["moves-on", "moves-back", "stays", "accepted-edit", "manifest-edit", + "copy-deleted"]) +def test_a_first_branch_run_over_a_tree_head_does_not_commit_is_refused( + root, project, gh, flags, prepare, extra, at, branch, differ, pins): + """THE ISSUE (#436). Main, on each: `moves-on` (the issue's (a)) exited + 0 over a commit of C's re-pin without B's copy, red under its own + `validate-pins.py`, and left that copy modified; `moves-back` ((b)) said + REFUSED with git's "nothing to commit" after writing and rolling back; + `stays` ((c)) said "nothing to do", exit 0, and made no branch; + `accepted-edit` committed a pin recording a digest of an edit the commit + did not carry; `manifest-edit` committed the uncommitted manifest line + under the re-pin's message; `copy-deleted` called it `update-conflict`. + Now each is refused by name, listing exactly the paths HEAD does not + commit, before anything is written, a branch made, `gh` asked or + anything pushed.""" + prepare(root, project, gh.env) + head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip() + here = git("symbolic-ref", "--short", "HEAD", cwd=root.path).stdout + branches = git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout + remote = on_the_remote(root, branch) + status = git("status", "--porcelain", cwd=root.path).stdout + gh.forget() + + result = apply(root, project, *extra, "--branch", branch, *flags, + env=gh.env, at=project[at]) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED update-branch-uncommitted: --branch {branch} would commit " + f"what HEAD ({head[:12]}) commits plus only the paths this run " + "writes, but the working tree does not hold what HEAD commits at " + f"{', '.join(differ)}. "), result.stderr + if pins: + named, committed = (project[key][:12] for key in pins) + assert (f"there the pin names {named} and HEAD's names {committed}" + in result.stderr), result.stderr + else: + assert "there the pin names" not in result.stderr, result.stderr + assert "commit those changes first" in result.stderr + assert "Nothing was written, committed or pushed." in result.stderr + assert "nothing to commit" not in result.stderr + assert "ROLLED BACK" not in result.stderr + assert result.stdout == "", "nothing copied, re-pinned or said done" + assert gh.calls() == [] + assert on_the_remote(root, branch) == remote, "nothing was pushed" + assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head + assert git("symbolic-ref", "--short", "HEAD", + cwd=root.path).stdout == here + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout == branches, "no branch was made" + assert git("status", "--porcelain", cwd=root.path).stdout == status, ( + "nothing was written and nothing rolled back") + + +def test_a_first_branch_run_reads_no_readme_and_no_untracked_dirt( + root, project, gh): + """WHAT THE REFUSAL DOES NOT TOUCH, on a first run as on #333's re-run: + an uncommitted README edit and an untracked note are read by no + decision, so the run commits B's re-pin as it does on a clean tree — + the three paths it wrote, green — and leaves both where they were.""" + readme = root.path / "README.md" + readme.write_bytes(readme.read_bytes() + b"\nNot committed.\n") + (root.path / "notes.txt").write_bytes(b"untracked\n") + + result = apply(root, project, "--branch", BRANCH, env=gh.env) + + assert result.returncode == 0, result.stdout + result.stderr + assert f"committed on {BRANCH}" in result.stdout + assert git("show", "--name-only", "--format=", "HEAD", + cwd=root.path).stdout.split() == [PIN, "project.yaml", + CHANGED] + assert git("status", "--porcelain", cwd=root.path).stdout.splitlines() \ + == [" M README.md", "?? notes.txt"] + assert gh.calls() == [] + + +def test_a_copy_deleted_and_committed_is_still_a_conflict(root, project, gh): + """NEITHER SIDE HOLDS IT, SO NOTHING DIFFERS. A pinned copy whose + deletion HEAD commits is not a change left uncommitted; it is + `update-conflict`'s `copy-missing`, as it was, and nothing is written.""" + git("rm", "-q", "--", LOCAL, cwd=root.path) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Drop a copy", "--", LOCAL, cwd=root.path) + + result = apply(root, project, "--branch", BRANCH, env=gh.env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith("REFUSED update-conflict: "), \ + result.stderr + assert f"{LOCAL}: pinned but absent from the root" in result.stderr + assert "update-branch-uncommitted" not in result.stderr + assert git("status", "--porcelain", cwd=root.path).stdout == "" + + +#: What goes where a pinned copy was, and a commit id no repository holds. +NOTE = "note.txt" +SUBMODULE = "1" * 40 + + +def a_directory_in_place_of_the_copy(root, *, stage: bool = False) -> None: + """`LOCAL`, a file HEAD commits, replaced by a directory on disk.""" + path = root.path / LOCAL + path.unlink() + path.mkdir() + (path / NOTE).write_bytes(b"a directory where the copy was\n") + if stage: + git("add", "--", LOCAL, cwd=root.path) + + +def a_directory_committed_in_place_of_the_copy(root, project, env) -> None: + a_directory_in_place_of_the_copy(root, stage=True) + git("commit", "-q", "-m", "Replace a copy with a directory", + cwd=root.path) + assert git("cat-file", "-t", f"HEAD:{LOCAL}", + cwd=root.path).stdout.strip() == "tree" + + +def a_gitlink_committed_in_place_of_the_copy(root, project, env) -> None: + """A gitlink checks out as a directory: an empty one, uninitialised.""" + (root.path / LOCAL).unlink() + (root.path / LOCAL).mkdir() + git("rm", "-q", "--cached", "--", LOCAL, cwd=root.path) + git("update-index", "--add", "--cacheinfo", + f"160000,{SUBMODULE},{LOCAL}", cwd=root.path) + git("commit", "-q", "-m", "Replace a copy with a submodule", + cwd=root.path) + assert git("ls-tree", "HEAD", "--", LOCAL, + cwd=root.path).stdout.startswith("160000 commit ") + + +@pytest.mark.parametrize("prepare", [ + a_directory_committed_in_place_of_the_copy, + a_gitlink_committed_in_place_of_the_copy, +], ids=["tree", "gitlink"]) +def test_a_copy_replaced_by_a_directory_and_committed_is_still_a_conflict( + root, project, gh, prepare): + """THE KIND IS HEAD'S, SO NOTHING DIFFERS (#436, review of e7e80c4). + A pinned copy replaced by a directory, the change committed, is not an + edit left uncommitted: HEAD commits a directory and the disk holds one. + `git hash-object` cannot hash it, so the comparison read an empty + answer as a difference, and every `--branch` run said + `update-branch-uncommitted` for good, a refusal no remedy it gives + changes. It is `update-conflict`'s `copy-missing` instead, as on main + and as for a deletion HEAD commits, with nothing written.""" + prepare(root, project, gh.env) + head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip() + branches = git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout + assert git("status", "--porcelain", cwd=root.path).stdout == "" + + result = apply(root, project, "--branch", BRANCH, env=gh.env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith("REFUSED update-conflict: "), \ + result.stderr + assert f"{LOCAL}: pinned but absent from the root" in result.stderr + assert "update-branch-uncommitted" not in result.stderr + assert result.stdout == "", "nothing copied, re-pinned or said done" + assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout == branches, "no branch was made" + assert git("status", "--porcelain", cwd=root.path).stdout == "" + assert gh.calls() == [] + + +def a_directory_uncommitted(root, project, env) -> None: + """HEAD commits the file; the disk holds a directory, untracked.""" + a_directory_in_place_of_the_copy(root) + + +def a_directory_staged(root, project, env) -> None: + """The same, staged: the index is not HEAD.""" + a_directory_in_place_of_the_copy(root, stage=True) + + +def a_file_in_place_of_the_committed_directory(root, project, env) -> None: + """HEAD commits a directory; the disk holds a file again, uncommitted.""" + a_directory_committed_in_place_of_the_copy(root, project, env) + shutil.rmtree(root.path / LOCAL) + (root.path / LOCAL).write_bytes(b"a file again, not committed\n") + + +def a_directory_where_head_commits_nothing(root, project, env) -> None: + """The copy's deletion committed, then a directory there, untracked.""" + git("rm", "-q", "--", LOCAL, cwd=root.path) + git("commit", "-q", "-m", "Drop a copy", cwd=root.path) + (root.path / LOCAL).mkdir() + (root.path / LOCAL / NOTE).write_bytes(b"a directory, not committed\n") + + +@pytest.mark.parametrize("prepare", [ + a_directory_uncommitted, + a_directory_staged, + a_file_in_place_of_the_committed_directory, + a_directory_where_head_commits_nothing, +], ids=["directory", "staged-directory", "file", "untracked-directory"]) +def test_a_type_change_head_does_not_commit_is_still_refused( + root, project, gh, prepare): + """A KIND THAT DISAGREES IS A DIFFERENCE (#436). HEAD commits a file and + the disk holds a directory, or HEAD commits a directory and the disk + holds a file, or HEAD commits nothing and the disk holds a directory: + a type change left uncommitted, which the commit would not carry, so + `update-branch-uncommitted` names the path and nothing is written. Only + a directory over a directory HEAD commits is HEAD's.""" + prepare(root, project, gh.env) + head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip() + branches = git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout + status = git("status", "--porcelain", cwd=root.path).stdout + + result = apply(root, project, "--branch", BRANCH, env=gh.env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED update-branch-uncommitted: --branch {BRANCH} would commit " + f"what HEAD ({head[:12]}) commits plus only the paths this run " + "writes, but the working tree does not hold what HEAD commits at " + f"{LOCAL}. "), result.stderr + assert "Nothing was written, committed or pushed." in result.stderr + assert result.stdout == "", "nothing copied, re-pinned or said done" + assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout == branches, "no branch was made" + assert git("status", "--porcelain", cwd=root.path).stdout == status + assert gh.calls() == [] + + +@pytest.mark.skipif( + os.name == "nt", + reason="a committed symlink is checked out as a plain file on Windows " + "unless core.symlinks is on, and making one needs a privilege " + "the runners do not grant") +def test_a_copy_kept_as_a_committed_link_is_what_head_commits(root, project, + gh): + """GIT COMMITS THE LINK, NOT WHAT IT LEADS TO. `hash-object` on the path + reads through a link, so a pinned copy kept as one that HEAD commits + would read as a change HEAD does not commit, and every `--branch` run + would be refused as that, whatever else was true. Its own text is + compared with HEAD's entry for it instead, so this refusal does not + stand in front of the run. What `apply` then does with a link is not + this test's question (#347 answers it), so what is asserted holds + either way: no `update-branch-uncommitted`, and the link is still the + one HEAD commits.""" + real = "scripts/bootstrap-real.py" + os.replace(root.path / LOCAL, root.path / real) + (root.path / LOCAL).symlink_to("bootstrap-real.py") + git("add", "--", LOCAL, real, cwd=root.path) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Keep the bootstrap copy as a link", cwd=root.path) + link = git("rev-parse", f"HEAD:{LOCAL}", cwd=root.path).stdout + + result = apply(root, project, "--branch", BRANCH, env=gh.env) + + assert "Traceback" not in result.stderr, result.stderr + assert "update-branch-uncommitted" not in result.stderr, result.stderr + assert (root.path / LOCAL).is_symlink() + assert git("rev-parse", f"HEAD:{LOCAL}", cwd=root.path).stdout == link + assert git("status", "--porcelain", cwd=root.path).stdout == "" + + +# --- what the comparison never reads, and what it cannot vouch for (#436) --- +# +# Codex's review of 1af060e. A FIFO where a pinned copy was blocked +# `git hash-object` for good, so every `--branch` run hung where main refused; +# and a link HEAD commits is HEAD's, but the bytes the run reads and writes +# are at its far end, which no commit of the link carries. + +#: Long enough for any refusal `apply` makes before its first write, on the +#: slowest runner. A run still going after it is blocked on a read. +HANG = 120 + + +def release(path) -> None: + """Open the FIFO at `path` for writing and close it again, so a process + left blocked opening it for reading is let go, with nothing to read.""" + try: + os.close(os.open(path, os.O_WRONLY | os.O_NONBLOCK)) + except OSError: + pass + + +def apply_within(root, project, *extra, env): + """`apply` as `apply` above runs it, given `HANG` seconds: a run still + going then is killed and reported as a hang, not waited for. A FIFO at + `LOCAL` is released after the kill, so no `git` it started stays + blocked behind the test.""" + full = blank_unnamed_pin_sources( + {**os.environ, **env, "LANES_LANE": ""}, env) + proc = subprocess.Popen( + [sys.executable, str(UPDATE), "apply", "--root", str(root.path), + "--yes", "--upstream", str(project["upstream"]), + "--at", project["b"], *extra], + stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, env=full) + try: + out, err = proc.communicate(timeout=HANG) + except subprocess.TimeoutExpired: + proc.kill() + release(root.path / LOCAL) + proc.communicate() + pytest.fail(f"apply was still running after {HANG}s: blocked " + f"reading what stands at {LOCAL}") + return subprocess.CompletedProcess(proc.args, proc.returncode, out, err) + + +def a_fifo_in_place_of_the_copy(root) -> None: + (root.path / LOCAL).unlink() + os.mkfifo(root.path / LOCAL) + + +def a_fifo_where_head_commits_nothing(root) -> None: + git("rm", "-q", "--", LOCAL, cwd=root.path) + git("commit", "-q", "-m", "Drop a copy", cwd=root.path) + os.mkfifo(root.path / LOCAL) + + +def a_socket_in_place_of_the_copy(root) -> None: + """Bound by its name inside its directory: a socket's whole path has to + fit in about a hundred bytes, which a temporary directory may not.""" + (root.path / LOCAL).unlink() + here = os.getcwd() + os.chdir(root.path / os.path.dirname(LOCAL)) + try: + with socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) as sock: + sock.bind(os.path.basename(LOCAL)) + finally: + os.chdir(here) + + +@pytest.mark.skipif( + not hasattr(os, "mkfifo"), + reason="no FIFOs or Unix sockets in a Windows working tree: os.mkfifo " + "is POSIX only") +@pytest.mark.parametrize("flags", [(), ("--push", "--pr")], + ids=["branch", "push-pr"]) +@pytest.mark.parametrize("prepare", [ + a_fifo_in_place_of_the_copy, + a_fifo_where_head_commits_nothing, + a_socket_in_place_of_the_copy, +], ids=["fifo", "fifo-head-commits-nothing", "socket"]) +def test_a_copy_replaced_by_what_is_not_a_file_is_not_read( + root, project, gh, prepare, flags): + """A FIFO, OR A SOCKET, IS `classify`'s, AS ON MAIN (#436, codex + review of 1af060e). `classify` calls a pinned copy that is not a file + `copy-missing`, and main refused `update-conflict` before writing. The + comparison handed the path to `git hash-object`, which opens it: over + a FIFO the open waits for a writer that never comes, and `apply` hung; + over a socket the open fails, and the path was called a change HEAD + does not commit, which no remedy changes. It is `lstat`ed first now + and never read, so the run refuses as main does, with nothing written, + no branch made and `gh` not asked.""" + prepare(root) + head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip() + branches = git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout + status = git("status", "--porcelain", cwd=root.path).stdout + + result = apply_within(root, project, "--branch", BRANCH, *flags, + env=gh.env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith("REFUSED update-conflict: "), \ + result.stderr + assert f"{LOCAL}: pinned but absent from the root" in result.stderr + assert "update-branch-uncommitted" not in result.stderr + assert result.stdout == "", "nothing copied, re-pinned or said done" + assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout == branches, "no branch was made" + assert git("status", "--porcelain", cwd=root.path).stdout == status + assert gh.calls() == [] + + +def a_link_in_place_of(root, path: str, end: str) -> str: + """`path` kept as a committed relative link to `end`, beside it, which + holds its bytes. Returns `end`.""" + os.replace(root.path / path, root.path / end) + (root.path / path).symlink_to(os.path.basename(end)) + git("add", "--", path, end, cwd=root.path) + git("commit", "-q", "-m", f"Keep {path} as a link", cwd=root.path) + return end + + +def a_copy_upstream_changes_kept_as_a_link(root) -> str: + """B changes `CHANGED`, which the root keeps as a link.""" + return a_link_in_place_of(root, CHANGED, + "scripts/validate-manifest-real.py") + + +def an_edit_at_the_end_of_a_linked_copy(root) -> str: + """`LOCAL` kept as a link, its far end edited and not committed.""" + end = a_link_in_place_of(root, LOCAL, "scripts/bootstrap-real.py") + (root.path / end).write_bytes((root.path / end).read_bytes() + + b"\n# edited at the link's end\n") + return end + + +def the_pin_kept_as_a_link(root) -> str: + return a_link_in_place_of(root, PIN, "contracts/shape-pin-real.yaml") + + +def the_manifest_kept_as_a_link(root) -> str: + return a_link_in_place_of(root, "project.yaml", "project-real.yaml") + + +@pytest.mark.skipif( + os.name == "nt", + reason="a committed symlink is checked out as a plain file on Windows " + "unless core.symlinks is on, and making one needs a privilege " + "the runners do not grant") +@pytest.mark.parametrize("prepare, extra, linked", [ + (a_copy_upstream_changes_kept_as_a_link, (), CHANGED), + (an_edit_at_the_end_of_a_linked_copy, ("--accept-local", LOCAL), LOCAL), + (the_pin_kept_as_a_link, (), PIN), + (the_manifest_kept_as_a_link, (), "project.yaml"), +], ids=["upstream-changed", "accepted-edit", "pin", "manifest"]) +def test_a_branch_run_that_would_write_through_a_link_is_refused( + root, project, gh, prepare, extra, linked): + """GIT COMMITS THE LINK, SO NOTHING IS WRITTEN OR RE-PINNED THROUGH ONE + (#436, codex review of 1af060e). The comparison holds a committed link + to be HEAD's, which it is; but the run reads, writes and re-pins the + bytes at its far end. Main and 1af060e exited 0 on each of these: a + copy B rewrote through its link, an edit `--accept-local` took at the + link's end, the pin and the manifest each rewritten through one, with + the commit holding the links as HEAD had them and its own + `validate-pins.py` red in a clean checkout. `--branch` refuses them by + name before the first write now, `gh` not asked.""" + end = prepare(root) + kept = (root.path / end).read_bytes() + head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip() + branches = git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout + status = git("status", "--porcelain", cwd=root.path).stdout + + result = apply(root, project, *extra, "--branch", BRANCH, "--push", + "--pr", env=gh.env) + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith("REFUSED update-rewrite-symlink: "), \ + result.stderr + assert linked in result.stderr, result.stderr + assert result.stdout == "", "nothing copied, re-pinned or said done" + assert (root.path / end).read_bytes() == kept, "nothing written there" + assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout == branches, "no branch was made" + assert git("status", "--porcelain", cwd=root.path).stdout == status + assert on_the_remote(root) is None, "nothing was pushed" + assert gh.calls() == [] + + +# --- the same tree without `--branch`: no commit line to get wrong (#436) ----- + +@pytest.mark.parametrize("prepare, extra, at, left", [ + (b_in_place_on_main, (), "c", [CHANGED]), + (edit_a_copy, ("--accept-local", LOCAL), "b", [LOCAL]), +], ids=["in-place-twice", "accepted-edit"]) +def test_an_in_place_run_names_what_its_commit_line_would_leave_out( + root, project, gh, prepare, extra, at, left): + """THE ISSUE'S (a'). In place, the run writes as it always did: the + copies, the pin and the manifest, green. Main then printed `NEXT git + checkout -b … && git commit -- `, which, pasted, + made the red commit `--branch` now refuses: the pin recorded the bytes + of copies the line left out. The NEXT line names those copies now, and + prints no commit to paste.""" + prepare(root, project, gh.env) + + result = apply(root, project, *extra, env=gh.env, at=project[at]) + + assert result.returncode == 0, result.stdout + result.stderr + assert load_yaml(root.path / PIN)["commit"].lower() == project[at] + wrote = [line.split()[-1] for line in result.stdout.splitlines() + if line.startswith(" copied ")] + paths = sorted({*wrote, PIN, "project.yaml"}) + assert next_line_of(result.stdout) == ( + "NEXT no commit line: the working tree does not hold what HEAD " + f"commits at {', '.join(left)}, paths this run did not write and " + "whose bytes the pin it wrote records, so a commit of " + f"{', '.join(paths)} alone would record digests for bytes it does " + "not carry. Commit those changes with them if this re-pin is to " + "carry them, or put both back to HEAD's bytes and run apply again") + assert git("symbolic-ref", "--short", "HEAD", + cwd=root.path).stdout.strip() == "main", "nothing committed" + assert gh.calls() == [] diff --git a/update-shape.py b/update-shape.py index 8643b3f4..4bb06a0a 100755 --- a/update-shape.py +++ b/update-shape.py @@ -45,6 +45,20 @@ appends a `CONTRACTS_DIR` block to an adopted Makefile — is treated as a conflict the moment upstream touches it, because copying the target bytes over it would delete that block without saying so. + * Nothing is committed that was judged from other bytes (#436). Every + verdict — what to copy, what to re-pin, whether there is anything to + do — is read from the WORKING TREE, and `--branch` commits HEAD plus + only the paths this run writes. So before the first write the pin, the + manifest and every pinned copy are compared with HEAD, and a difference + is refused by name (`update-branch-uncommitted`): commit those changes + or put them back. The commit is never widened to take them in. A + directory where HEAD commits one is HEAD's, not a difference, and a + FIFO or a socket is never read: the copy is `update-conflict`'s + `copy-missing`. A copy, the pin or the manifest this run would write or + re-pin THROUGH a symlink is refused too (`update-rewrite-symlink`): git + commits the link, not the bytes it leads to. Without `--branch` the run + writes as it always did, and its NEXT line names those copies instead + of printing a commit that would leave them out. * Nothing is left red: the root's OWN `validate-pins.py` and `validate-manifest.py` run after the rewrite, and every byte is rolled back if either refuses. @@ -2764,17 +2778,25 @@ def cmd_apply(args) -> int: [str(path) for path in (args.add or [])], target) _note_untaken_additions(additions, taking) - if _nothing_to_do(rows, pinned, target, taking): - if _awaits_its_pull_request(root, args, kind): - _refuse_what_head_does_not_commit(root, rows, kind, - args.branch, pinned, target) - return _finish_landing(root, args, upstream, target, kind) + nothing = _nothing_to_do(rows, pinned, target, taking) + rerun = nothing and _awaits_its_pull_request(root, args, kind) + if args.branch: + # BEFORE THE FIRST WRITE, AND BEFORE "NOTHING TO DO" (#436): that + # answer is read from the working tree too, and over a re-pin + # left uncommitted it made no branch where one was asked for. + _refuse_what_head_does_not_commit(root, rows, kind, args.branch, + pinned, target, rerun) + if rerun: + return _finish_landing(root, args, upstream, target, kind) + if nothing: print(f"nothing to do: every copied file matches " f"{upstream.repository} @ {target[:12]}, which the pin " "already names") return 0 _refuse_conflicts(rows, accepted) _refuse_local_drift(rows, accepted) + if args.branch: + _refuse_writes_through_a_link(root, rows, kind, args.branch) confirm(args, rows, taking, target) target_tree = upstream.tree_sha256(target) # READ BEFORE THE COPIES, REWRITTEN AFTER THE PIN. `.gitattributes` @@ -2796,7 +2818,8 @@ def cmd_apply(args) -> int: _land(root, args, paths, added, target, upstream, kind, len(copied), ledger) print() - next_line(root, args, paths, target, added, kind) + next_line(root, args, paths, target, added, kind, + pinned_paths=[row.path for row in rows]) return 0 except (Refusal, CommandFailed) as exc: return _rolled_back(exc, ledger) @@ -2972,26 +2995,36 @@ def _awaits_its_pull_request(root: Path, args, kind: Kind) -> bool: def _refuse_what_head_does_not_commit(root: Path, rows: list[Row], kind: Kind, branch: str, pinned: str, - target: str) -> None: - """The re-run `_awaits_its_pull_request` recognised finishes what HEAD - commits, so the working tree must hold HEAD's bytes wherever this run - read it, or the re-run is refused by name (#333). - - TWO DECISIONS, ONE SOURCE. "Nothing to write" is judged from the - working tree — `read_pin` and `classify` — and the pull request's base - and repository are read from the manifest there too; whether a pull - request is owed is judged from HEAD, and HEAD is what the re-run pushes - and what its pull request carries. Over a tree that IS HEAD's that is - one answer. Over an in-place `apply --at C` left uncommitted on a - branch whose commit re-pins B it was two: the re-run pushed B under a - pull request titled C, and said the pin on the branch named C. + target: str, rerun: bool) -> None: + """Every `--branch` run lands what HEAD commits, so the working tree + must hold HEAD's bytes wherever this run read it, or the run is refused + by name before its first write (#333, #436). + + TWO SOURCES, ONE ANSWER EXPECTED. Everything this run decides is read + from the working tree — `read_pin` and `classify`: what to copy, what to + re-pin, whether there is anything to do at all — and the pull request's + base and repository are read from the manifest there too. What it lands + is HEAD's: the re-run `_awaits_its_pull_request` recognises (`rerun`) + pushes HEAD as it stands, and any other `--branch` run commits HEAD plus + only the paths it writes (`commit_on_branch`). Over a tree that IS + HEAD's that is one answer. Over an in-place `apply` left uncommitted it + was two, each case measured: + + * the re-run (#333) pushed B under a pull request titled C; + * a first run (#436) committed D's re-pin without C's copies, exit 0 + with the commit's own `validate-pins.py` red; or re-pinned back to + what HEAD held and died on git's "nothing to commit"; or found + "nothing to do" in a re-pin HEAD never committed, and made no branch. So the pin, the manifest and every pinned copy are compared with HEAD - BY BYTES (`_not_what_head_commits`). The index is not asked: nothing - here commits, and neither decision reads a staged entry. Nor is any - other path, a README or an untracked note, which neither decision - reads. The refusal comes before `gh` is asked or anything is pushed, - with nothing written, so there is nothing to roll back. + BY BYTES (`_not_what_head_commits`) and a difference is refused, in the + words for the run it is: `update-rerun-uncommitted` for the re-run, + `update-branch-uncommitted` for a run that would commit. The commit's + pathspec is NOT widened to take the difference in: what a person left + uncommitted is theirs to commit or put back. The index is not asked: no + decision here reads a staged entry. Nor is any other path, a README or + an untracked note, which no decision reads. Nothing is written yet, so + there is nothing to roll back. """ pin = "contracts/shape-pin.yaml" differ = _not_what_head_commits( @@ -3001,11 +3034,22 @@ def _refuse_what_head_does_not_commit(root: Path, rows: list[Row], return head = (_git_answer(root, ["rev-parse", "HEAD"]) or "").strip() committed = _committed_pin(root, pin) + if rerun: + raise _rerun_uncommitted(branch, head, target, differ, pinned, + committed) + raise _branch_uncommitted(branch, head, differ, pinned, committed) + + +def _rerun_uncommitted(branch: str, head: str, target: str, + differ: list[str], pinned: str, + committed: str) -> Refusal: + """`update-rerun-uncommitted` (#333): the re-run would push HEAD under a + pull request named for the working tree.""" carried = (f"; there the pin names {pinned[:12]} and HEAD's names " f"{committed[:12]}, so the pull request would name one commit " "and carry another" if committed and committed != pinned else "") - raise Refusal( + return Refusal( "update-rerun-uncommitted", f"--pr would finish the landing of {branch} from what HEAD " f"({head[:12]}) commits, under a pull request naming {target[:12]}, " @@ -3020,6 +3064,92 @@ def _refuse_what_head_does_not_commit(root: Path, rows: list[Row], "was not asked.") +def _branch_uncommitted(branch: str, head: str, differ: list[str], + pinned: str, committed: str) -> Refusal: + """`update-branch-uncommitted` (#436): a `--branch` run that would + commit HEAD plus its own paths over a working tree it judged instead. + + The remedy is the one #333's gives, for a run that has not committed + yet: commit those changes, or put them back. Putting back is the exit + for an earlier `apply` left uncommitted, which this command then makes + again from HEAD; committing is the exit for changes the re-pin is to + carry, and never onto the tracking branch, which `commit_on_branch` + refuses for this tool's own commit too. + """ + moved = (f"; there the pin names {pinned[:12]} and HEAD's names " + f"{committed[:12]}" if committed and committed != pinned + else "") + return Refusal( + "update-branch-uncommitted", + f"--branch {branch} would commit what HEAD " + f"({head[:12] or 'no commit'}) commits plus only the paths this run " + "writes, but the working tree does not hold what HEAD commits at " + + ", ".join(differ) + ". What to copy, what to re-pin and whether " + "there is anything to do at all are judged from the working " + f"tree{moved}, so the commit would not hold what this run judged.", + "Remediation: commit those changes first if this re-pin is to carry " + "them — on a branch, since the tracking branch takes no direct " + "commit — or put them back to HEAD's bytes if it is not, as for an " + "earlier `apply` left uncommitted, which this command then makes " + "again from HEAD. Then run the same command again. Nothing was " + "written, committed or pushed.") + + +def _refuse_writes_through_a_link(root: Path, rows: list[Row], kind: Kind, + branch: str) -> None: + """`update-rewrite-symlink` under `--branch` (#436, codex review of + 1af060e): this run would write, or re-pin from, a file it reaches + THROUGH a symlink, so the commit would not hold what it judged. + + `_not_what_head_commits` compares a link's own text with HEAD, because + that is what git commits there. But `classify` and `file_sha256` read + the bytes the link leads to, and `Rollback.write` writes through it. So + a copy upstream rewrote, a local edit `--accept-local` took at the + link's end, and the pin and the manifest themselves land at the far end + of a link, while the commit holds HEAD's link unchanged: exit 0 over a + commit whose own `validate-pins.py` reports `shape-copy-drift`, or the + pin and the manifest disagreeing, in a clean checkout (measured on main + and on 1af060e alike). Refused here, before the first write. + + ASKED AFTER `update-conflict` AND `update-local-drift`, so every row + still not UNCHANGED is one this run writes or re-pins, and those + refusals keep main's answers. A linked copy the run leaves UNCHANGED is + not refused: the pin keeps HEAD's row for it, so the commit claims + nothing new about it. The name is the rewrite twin of `--add`'s + `update-add-symlink` (#239), for a file this run overwrites rather than + creates. Without `--branch` nothing is committed, and a write through + a link is made as on main (#286). + """ + pin = "contracts/shape-pin.yaml" + paths = [pin, kind.manifest] + [row.path for row in rows + if row.state != UNCHANGED] + linked = [] + for path in paths: + link = symlink_on_the_way(root, path) + if link is not None: + name, points = link + above = "" if name == path else " above it" + linked.append(f" {path}: {name} is a symlink{above}, " + f"pointing at {points!r}") + if not linked: + return + raise Refusal( + "update-rewrite-symlink", + f"--branch {branch} would commit HEAD plus the paths this run " + "writes, but this run would write, or re-pin from, bytes it reaches " + "THROUGH a symlink, and git commits the link, never what it leads " + "to:\n" + "\n".join(linked) + + "\n The commit would hold each link as HEAD does, under a pin " + "recording bytes it does not carry, so its own validate-pins.py " + "would refuse it in a clean checkout.", + "Remediation: replace each link with what it points at — a plain " + "file, or directory, at that path, so git holds the bytes there — " + "commit that, on a branch, since the tracking branch takes no " + "direct commit, and run the same command again. A root that keeps " + "a link takes the update by hand, at the link's end. Nothing was " + "written, committed or pushed.") + + def _not_what_head_commits(root: Path, paths: list[str]) -> list[str]: """Those of `paths` whose bytes on disk are not the blob HEAD commits at that path, or that HEAD does not commit at all, in the order given @@ -3030,15 +3160,93 @@ def _not_what_head_commits(root: Path, paths: list[str]) -> list[str]: runs, so a CRLF checkout of an LF blob is HEAD's, and no index flag hides an edit — `readme_is_committed`'s second question, asked of each path. + + FOUR ANSWERS THAT ARE NOT DIFFERENCES (#436), now that every `--branch` + run asks rather than only #333's re-run, whose rows were all present + and none of them links. A path neither the disk nor HEAD holds — a + pinned copy deleted and committed — is the same on both sides, and is + `update-conflict`'s to name as missing. Where a symlink stands on + the path's way (`symlink_on_the_way`, the path itself or a directory + above it), what git commits is THE LINK, its target as text, never the + bytes it leads to, which `hash-object` on the path would read: so the + link's own text is hashed against HEAD's entry for the link. A pinned + copy kept as a link HEAD commits is then HEAD's, as it is to git; the + bytes it leads to, which this run reads and may write, are + `_refuse_writes_through_a_link`'s question. And a directory + where HEAD commits a directory (`_head_holds_a_directory`) is HEAD's + too: a pinned copy replaced by a directory, that type change committed, + has nothing for `hash-object` to read, so it was named a difference + for good, though no edit is left to commit and no remedy this refusal + gives changes it. It is not a file, so `classify` calls it + `copy-missing` and `update-conflict` names it before anything is + written, as it does for a deletion HEAD commits. Any OTHER pair of + kinds is a difference: a directory over a file HEAD commits, a file + over a directory HEAD commits, or a directory where HEAD commits + nothing, is a type change nobody has committed. + + And a node that is NOT A FILE, A DIRECTORY OR A LINK — a FIFO, a + socket, a device — is never READ (codex review of 1af060e): `lstat` + says what it is first. `hash-object` opens what it is given, and a + FIFO with no writer blocks that open for good, so `apply --branch` + hung over a pinned copy replaced by one, where main refused. git + commits none of them, so there is nothing of HEAD's to compare, and + `classify`, whose `is_file()` says no, already calls it `copy-missing`: + `update-conflict` names it before anything is written, as on main. """ - differ = [] - for path in paths: - disk = (_git_answer(root, ["hash-object", "--", path]) or "").strip() - head = (_git_answer(root, ["rev-parse", "--verify", "--quiet", - f"HEAD:{path}"]) or "").strip() - if not disk or disk != head: - differ.append(path) - return differ + return [path for path in paths if _differs_from_head(root, path)] + + +def _differs_from_head(root: Path, path: str) -> bool: + """`_not_what_head_commits`' question for one path. What stands there + is `lstat`ed before git is asked to read it, so git is handed a regular + file or nothing, and never opens what could block.""" + link = symlink_on_the_way(root, path) + if link is not None: + name, points = link + disk = _blob_of(root, os.fsencode(points)) + return not disk or disk != _head_blob(root, name) + try: + mode = os.lstat(root / path).st_mode + except (OSError, ValueError): + # Nothing on disk: a difference only where HEAD commits something. + return bool(_head_blob(root, path)) + if stat.S_ISDIR(mode): + # Nothing here to hash: `hash-object` on a directory fails. + return not _head_holds_a_directory(root, path) + if not stat.S_ISREG(mode): + # A FIFO, a socket or a device: not read, and `classify`'s. + return False + disk = (_git_answer(root, ["hash-object", "--", path]) or "").strip() + return not disk or disk != _head_blob(root, path) + + +def _head_holds_a_directory(root: Path, path: str) -> bool: + """Does HEAD commit a directory at `path`: a tree, or a gitlink, which + git checks out as one (#436)? True for any entry that is not a blob, + and False where HEAD commits nothing there. + + `cat-file -t` names a tree, and fails for a gitlink, whose commit is + not an object of this repository — so the answer is "is not a blob", + never "is a tree". A symlink is a blob whose text is the target. + """ + entry = _head_blob(root, path) + return bool(entry) and (_git_answer( + root, ["cat-file", "-t", entry]) or "").strip() != "blob" + + +def _head_blob(root: Path, path: str) -> str: + """The object HEAD commits at `path`, or "" where it commits none.""" + return (_git_answer(root, ["rev-parse", "--verify", "--quiet", + f"HEAD:{path}"]) or "").strip() + + +def _blob_of(root: Path, data: bytes) -> str: + """The blob id git gives `data`, as BYTES on stdin (no filter runs on + stdin without `--path`), or "" when git will not say.""" + proc = _git_quietly(["git", "hash-object", "--stdin"], root, data) + if proc is None or proc.returncode != 0: + return "" + return proc.stdout.decode("ascii", "replace").strip() def _committed_pin(root: Path, pin: str) -> str: @@ -3176,8 +3384,29 @@ def open_pull_request(root: Path, args, create: list[str]) -> None: print(f" pull request {url}") +def _unwritten_changes(root: Path, paths: list[str], + pinned_paths: list[str]) -> list[str]: + """The pinned copies this run did NOT write whose bytes HEAD does not + commit (#436): what a branchless run's commit line would leave out. + + THE PIN THIS RUN WROTE RECORDS EVERY ROW FROM THE WORKING TREE, so a + pinned copy that differs from HEAD and is not among `paths` — an + earlier `apply` left uncommitted, a local edit `--accept-local` took — + would be HEAD's bytes in that commit under a digest of the disk's: the + red commit `--branch` now refuses to make. Asked only where HEAD names + a commit: a root with none has no commit line to get wrong, and prints + 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: + return [] + return _not_what_head_commits(root, rest) + + def next_line(root: Path, args, paths: list[str], target: str, - added: list[str] = (), kind: Kind = PROJECT_KIND) -> None: + added: list[str] = (), kind: Kind = PROJECT_KIND, *, + pinned_paths: list[str] = ()) -> None: if args.branch and args.pr: print("NEXT the pull request is open; ask a human to review and merge it") return @@ -3204,6 +3433,21 @@ def next_line(root: Path, args, paths: list[str], target: str, print(f"NEXT git -C {root} push -u origin {args.branch} && " f"{create}") return + # NO COMMIT LINE OVER CHANGES THIS RUN DID NOT WRITE (#436). The line + # below commits HEAD plus only this run's paths, which is the commit + # `update-branch-uncommitted` refuses to make under `--branch`; here the + # copies are already written, so the answer is a NEXT line that names + # what that commit would leave out, rather than one that makes it red. + unwritten = _unwritten_changes(root, paths, pinned_paths) + if unwritten: + print("NEXT no commit line: the working tree does not hold what " + f"HEAD commits at {', '.join(unwritten)}, paths this run did " + "not write and whose bytes the pin it wrote records, so a " + f"commit of {', '.join(paths)} alone would record digests for " + "bytes it does not carry. Commit those changes with them if " + "this re-pin is to carry them, or put both back to HEAD's " + "bytes and run apply again") + 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,