diff --git a/docs/cli.md b/docs/cli.md index aa423b4f..b646ba8d 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -487,7 +487,9 @@ options: given, and needs --branch — there is no commit to put a line on without one --push push the branch to origin (needs --branch) - --pr open a pull request with `gh` (needs --branch) + --pr open a pull request with `gh` (needs --branch); a + later run on that branch with nothing left to write + opens it if it is still missing ``` ### `shape-doctor.py` diff --git a/tests/test_update_shape_add_ignored.py b/tests/test_update_shape_add_ignored.py index 2e1516f4..385e0029 100644 --- a/tests/test_update_shape_add_ignored.py +++ b/tests/test_update_shape_add_ignored.py @@ -517,10 +517,6 @@ def operators_staged_work(root) -> None: "REFUSED creating the branch shape/update/ignored", ("kept HEAD on main, where it started", "absent shape/update/ignored (never created)"), "unstaged"), - ("branch-invalid", lambda root: None, "shape/update..ignored", - "REFUSED creating the branch shape/update..ignored", - ("kept HEAD on main, where it started", - "absent shape/update..ignored (never created)"), "unstaged"), ("branch-exists", the_branch_already_exists, BRANCH, f"REFUSED update-branch-exists: --branch {BRANCH} already exists", (), "deleted"), @@ -593,6 +589,33 @@ def test_a_failure_before_the_commit_puts_the_tree_back_and_names_the_step( assert "nothing to do" not in again.stdout +def test_a_branch_git_cannot_name_is_refused_before_any_write( + root, standard, quiet): + """`shape/update..ignored` WAS this file's branch-creation failure after + the writes: `git checkout -b` refuses `..`, so the copies, the pin and + the README line had to be put back. Since #204 `main` asks `git + check-ref-format --branch` about `--branch` before anything is read or + written, so the same name is refused as `update-branch-invalid`, with + nothing on stdout and nothing to put back, and the operator's staged + work is untouched. `branch-df` above still makes `git checkout -b` fail + after the writes.""" + operators_staged_work(root) + checked = check(root, standard, quiet) + before = snapshot(root, quiet) + + result = apply(root, standard, quiet, *ADD_ARGS, + "--branch", "shape/update..ignored") + + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + "REFUSED update-branch-invalid: --branch 'shape/update..ignored' "), ( + result.stderr) + assert result.stdout == "", "refused before anything was written" + assert "ROLLED BACK" not in result.stderr + assert_same_tree(before, snapshot(root, quiet)) + assert check(root, standard, quiet).stdout == checked.stdout + + def test_a_rollback_git_will_not_finish_says_so_and_names_the_fix( root, standard, quiet): """A PARTIAL ROLLBACK IS SAID TO BE ONE. Every undo is judged by reading diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py new file mode 100644 index 00000000..b5d48d43 --- /dev/null +++ b/tests/test_update_shape_landing.py @@ -0,0 +1,779 @@ +# SPDX-License-Identifier: Apache-2.0 +"""What `update-shape.py apply` says, and does next, when a step AFTER its +commit fails (#204). + +`apply --branch --push --pr` makes three acts after a clean rewrite: the +commit, the push, the pull request. Until #204 a failure in either of the last +two reached `cmd_apply`'s one `except CommandFailed` arm, which called every +failure "committing the re-synced shape", printed no NEXT line and did not +flush stdout first. And the run after a failed `gh pr create` found the pin +already at the target, said "nothing to do" and exited 0, so the tool never +opened the pull request it was asked for. + +THE UPSTREAM IS A CLONE OF THIS REPOSITORY, the project is scaffolded out of +it into bare repositories in a temporary directory, and every test gets its +own copy of the root AND of its bare remote, so one test's push cannot make +another's non-fast-forward. `gh` is a shell script on PATH that logs its argv +and answers from a state directory — the shape `test_free_plan_hint.py` +already uses — so nothing here reaches a network or a real repository. A test +that needs that script carries `WINDOWS_SKIP` (CreateProcess reads no +shebang); the ones that never reach `gh`, and one that stubs it in-process, +run on every platform. +""" + +from __future__ import annotations + +import importlib.util +import json +import os +import shlex +import shutil +import stat +import subprocess +import sys +from types import SimpleNamespace + +import pytest + +from conftest import REPO, WINDOWS_SKIP, blank_unnamed_pin_sources, git, \ + run_script +from test_update_shape import (CHANGED, CHANGED_SOURCE, PROJECT, + UPSTREAM_FIX_LINE, commit_upstream_changes, + next_line_of, scaffold_upstream_and_project) + +sys.path.insert(0, str(REPO / "scripts")) +from repo_shape import load_yaml # noqa: E402 +from shape_materialize import SHAPE_REPOSITORY, CommandFailed # noqa: E402 + +UPDATE = REPO / "update-shape.py" +BRANCH = "shape/update-landing" +PIN = "contracts/shape-pin.yaml" +URL = "https://github.invalid/testorg/Atlas/pull/7" +PR_OPEN_NEXT = "NEXT the pull request is open; ask a human to review and merge it" + +#: The stub. One line per call, each argument ended by a unit separator, so +#: the log keeps argument boundaries (the `--title` and `--body` values have +#: spaces in them) without a shell-quoting round trip. +GH_STUB = """#!/bin/sh +{ printf '%s\\037' "$@"; printf '\\n'; } >> "$GH_LOG" +case "$1 $2" in + "pr list") cat "$GH_STATE/list" 2>/dev/null || echo '[]' ;; + "pr create") + if [ -e "$GH_STATE/create-fails" ]; then + echo "HTTP 422: Validation Failed (stub)" >&2; exit 97 + fi + echo "__URL__" + printf '[{"state":"OPEN","url":"__URL__"}]\\n' > "$GH_STATE/list" ;; + *) echo "stub gh: unexpected $*" >&2; exit 64 ;; +esac +""".replace("__URL__", URL) + + +@pytest.fixture(scope="module") +def project(tmp_path_factory) -> dict: + """A project at A, its bare remotes, and an upstream moved to B.""" + base, upstream, commit_a, clone = scaffold_upstream_and_project( + tmp_path_factory, "update-shape-landing") + commit_b = commit_upstream_changes( + upstream, "Fix the manifest validator", + {CHANGED_SOURCE: UPSTREAM_FIX_LINE}) + return {"upstream": upstream, "clone": clone, "a": commit_a, + "b": commit_b, "remote": base / "remotes" / f"{PROJECT}.git"} + + +@pytest.fixture +def root(project, tmp_path): + """A private copy of the root, pushing to a private copy of its remote. + + `refs/remotes/origin/main` comes with the copy, which is the ref + `_awaits_its_pull_request` compares the branch's pin against.""" + remote = tmp_path / f"{PROJECT}.git" + shutil.copytree(project["remote"], remote, symlinks=True) + target = tmp_path / PROJECT + shutil.copytree(project["clone"], target, symlinks=True) + git("remote", "set-url", "origin", str(remote), cwd=target) + return SimpleNamespace(path=target, remote=remote) + + +@pytest.fixture +def gh(tmp_path): + """The `gh` stub on PATH, and what it was asked.""" + binary = tmp_path / "bin" + binary.mkdir() + script = binary / "gh" + script.write_text(GH_STUB, encoding="utf-8") + script.chmod(script.stat().st_mode | stat.S_IEXEC) + state = tmp_path / "gh-state" + state.mkdir() + log = tmp_path / "gh.log" + + def calls() -> list: + if not log.exists(): + return [] + return [line.split("\x1f")[:-1] + for line in log.read_text(encoding="utf-8").splitlines()] + + def failing(fails: bool) -> None: + marker = state / "create-fails" + if fails: + marker.touch() + elif marker.exists(): + marker.unlink() + + def answer(pulls: list) -> None: + (state / "list").write_text(json.dumps(pulls) + "\n", + encoding="utf-8") + + def forget() -> None: + if log.exists(): + log.unlink() + + env = {"PATH": f"{binary}{os.pathsep}{os.environ['PATH']}", + "GH_LOG": str(log), "GH_STATE": str(state)} + return SimpleNamespace(env=env, calls=calls, failing=failing, + answer=answer, forget=forget) + + +def apply(root, project, *extra, env=None, at=None): + return run_script(UPDATE, "apply", "--root", str(root.path), "--yes", + "--upstream", str(project["upstream"]), + "--at", at or project["b"], *extra, env=env) + + +def listing(branch: str = BRANCH) -> list: + """The `gh pr list` a re-run asks before it pushes or opens anything.""" + return ["pr", "list", "--repo", f"testorg/{PROJECT}", "--head", branch, + "--state", "all", "--json", "state,url"] + + +def the_commit_stands(root, project) -> str: + """The re-pin is COMMITTED on the branch and nothing was rolled back: + the checkout is on the branch, HEAD's pin names B, and the tree holds + exactly HEAD's bytes. Returns HEAD.""" + path = root.path + assert git("rev-parse", "--abbrev-ref", "HEAD", + cwd=path).stdout.strip() == BRANCH + assert load_yaml(path / PIN)["commit"].lower() == project["b"] + assert git("show", "--name-only", "--format=", "HEAD", + cwd=path).stdout.split() == [PIN, "project.yaml", CHANGED] + assert git("status", "--porcelain", cwd=path).stdout == "", ( + "nothing was rolled back under the commit") + return git("rev-parse", "HEAD", cwd=path).stdout.strip() + + +def on_the_remote(root, branch: str = BRANCH) -> str | None: + proc = git("rev-parse", "--verify", "--quiet", f"refs/heads/{branch}", + cwd=root.remote, check=False) + return proc.stdout.strip() or None + + +def commits_on_the_branch(root) -> int: + return int(git("rev-list", "--count", "origin/main..HEAD", + cwd=root.path).stdout.strip()) + + +# --- the pull request fails after the commit and the push ----------------- + +@WINDOWS_SKIP +def test_a_failed_pull_request_is_named_and_its_next_line_is_the_command( + root, project, gh): + """THE ISSUE'S FIRST RUN. The commit and the push landed, so the refusal + names the step that failed, says what stands, and ends on a NEXT line + that is the very `gh pr create` that failed — title and body included, + quoted to survive being pasted.""" + gh.failing(True) + result = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED opening the pull request for {BRANCH}" in result.stderr + assert "committing the re-synced shape" not in result.stderr + assert "origin has it" in result.stderr + assert "nothing was rolled back" in result.stderr + assert f"`apply --pr` run again on {BRANCH}" in result.stderr + assert f"committed on {BRANCH}" in result.stdout + assert f"pushed {BRANCH} to origin" in result.stdout + + calls = gh.calls() + assert len(calls) == 1, calls + assert calls[0][:2] == ["pr", "create"], calls + create = ["gh", *calls[0]] + line = next_line_of(result.stdout) + assert line == "NEXT " + shlex.join(create) + assert shlex.split(line)[1:] == create, "it pastes as the same argv" + assert create[create.index("--head") + 1] == BRANCH + + head = the_commit_stands(root, project) + assert on_the_remote(root) == head + + +@WINDOWS_SKIP +def test_a_failed_pull_request_on_an_unpushed_branch_pushes_first( + root, project, gh): + """`--pr` WITHOUT `--push`: the branch never reached origin, so the NEXT + line pushes before it opens the pull request, and the re-run it promises + is the one that pushes too, `apply --push --pr`, since a re-run with + `--pr` alone would ask `gh` for a pull request from a branch origin + lacks.""" + gh.failing(True) + result = apply(root, project, "--branch", BRANCH, "--pr", env=gh.env) + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED opening the pull request for {BRANCH}" in result.stderr + assert "origin does not have it yet" in result.stderr + assert f"`apply --push --pr` run again on {BRANCH}" in result.stderr + assert f"pushed {BRANCH}" not in result.stdout + + calls = gh.calls() + assert len(calls) == 1, calls + assert calls[0][:2] == ["pr", "create"], calls + push = shlex.join(["git", "-C", str(root.path), "push", "-u", "--", + "origin", BRANCH]) + assert next_line_of(result.stdout) == ( + f"NEXT {push} && " + shlex.join(["gh", *calls[0]])) + the_commit_stands(root, project) + assert on_the_remote(root) is None + + +@WINDOWS_SKIP +def test_the_commit_and_the_push_print_above_the_refusal_in_one_log( + root, project, gh): + """STDOUT IS FLUSHED BEFORE THE REFUSAL. With both streams in one pipe, + main printed the REFUSED block above the "committed on" and "pushed" + lines it was refusing after (#204, measured: line 13 against 21).""" + gh.failing(True) + env = blank_unnamed_pin_sources( + {**os.environ, **gh.env, "LANES_LANE": ""}, gh.env) + proc = subprocess.run( + [sys.executable, str(UPDATE), "apply", "--root", str(root.path), + "--yes", "--upstream", str(project["upstream"]), + "--at", project["b"], "--branch", BRANCH, "--push", "--pr"], + stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, text=True, env=env, check=False) + assert proc.returncode == 2, proc.stdout + lines = proc.stdout.splitlines() + found = {prefix: [i for i, line in enumerate(lines) + if line.lstrip().startswith(prefix)] + for prefix in ("committed on", "pushed", "REFUSED", "NEXT")} + assert all(len(at) == 1 for at in found.values()), (found, proc.stdout) + order = [at[0] for at in found.values()] + assert order == sorted(order), (found, proc.stdout) + + +@WINDOWS_SKIP +def test_a_rerun_on_the_branch_opens_the_missing_pull_request(root, project, + gh): + """THE ISSUE'S SECOND RUN. Nothing is left to write, and the pull request + `--pr` asked for is still missing, so the same command opens it: `gh` is + asked first, the push is a no-op, and the `gh pr create` is the one the + first run tried. No second commit. A third run finds it open and opens + no second one.""" + gh.failing(True) + first = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert first.returncode == 2, first.stdout + first.stderr + head = the_commit_stands(root, project) + create = gh.calls()[0] + + gh.failing(False) + gh.forget() + second = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert second.returncode == 0, second.stdout + second.stderr + assert "nothing to do" not in second.stdout + assert f"nothing to write: every copied file matches {SHAPE_REPOSITORY}" \ + in second.stdout + assert f"pull request {URL}" in second.stdout + assert next_line_of(second.stdout) == PR_OPEN_NEXT + assert gh.calls() == [listing(), create], ( + "asked first, then the very command the first run tried") + assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head + assert commits_on_the_branch(root) == 1, "no second commit" + assert on_the_remote(root) == head + + gh.forget() + third = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert third.returncode == 0, third.stdout + third.stderr + assert f"pull request {URL} is already open" in third.stdout + assert next_line_of(third.stdout) == PR_OPEN_NEXT + assert gh.calls() == [listing()], "no second pull request" + assert commits_on_the_branch(root) == 1 + + +@WINDOWS_SKIP +@pytest.mark.parametrize("state", ["MERGED", "CLOSED"]) +def test_a_branch_whose_pull_request_was_merged_or_closed_gets_no_second_one( + root, project, gh, state): + """`gh` IS ASKED BEFORE ANYTHING IS PUSHED. GitHub deletes a merged + branch; a re-run that pushed first would have put it back. The branch is + deleted on the remote here to show that it stays deleted.""" + gh.failing(True) + first = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert first.returncode == 2, first.stdout + first.stderr + git("update-ref", "-d", f"refs/heads/{BRANCH}", cwd=root.remote) + + gh.failing(False) + gh.forget() + gh.answer([{"state": state, "url": URL}]) + rerun = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert rerun.returncode == 0, rerun.stdout + rerun.stderr + assert f"pull request {URL} for {BRANCH} is {state.lower()}; a re-run " \ + "opens no second one" in rerun.stdout + assert gh.calls() == [listing()] + assert on_the_remote(root) is None, "nothing was pushed" + + +# --- the push fails after the commit ---------------------------------------- + +@pytest.mark.parametrize("flags", [("--push",), ("--push", "--pr")], + ids=["push", "push-pr"]) +def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, + tmp_path, + flags): + """A PUSH ORIGIN REFUSES is the same arm's other half: main called it + "committing the re-synced shape" as well. Named now, the commit kept, + and the NEXT line pushes and then opens the pull request — under `--pr` + with the command `--pr` would have run, without it with the bare form + the `--branch` NEXT line prints. `gh` is never reached, so this runs on + every platform.""" + git("remote", "set-url", "--push", "origin", + str(tmp_path / "no-such-remote.git"), cwd=root.path) + result = apply(root, project, "--branch", BRANCH, *flags) + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED pushing {BRANCH}" in result.stderr + assert f"git push -q -u -- origin {BRANCH}" in result.stderr, ( + "`--` ends git's options before the branch") + assert "committing the re-synced shape" not in result.stderr + assert "origin does not have it yet" in result.stderr + assert "nothing was rolled back" in result.stderr + assert f"committed on {BRANCH}" in result.stdout + assert f"pushed {BRANCH}" not in result.stdout + + line = next_line_of(result.stdout) + push = shlex.join(["git", "-C", str(root.path), "push", "-u", "--", + "origin", BRANCH]) + assert line.startswith(f"NEXT {push} && gh pr create "), line + if "--pr" in flags: + assert f"--repo testorg/{PROJECT} --base main --head {BRANCH} " \ + "--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 "run again" not in result.stderr, ( + "a re-run without --pr finishes nothing, so none is promised") + the_commit_stands(root, project) + assert on_the_remote(root) is None + + +@WINDOWS_SKIP +def test_a_rerun_after_a_failed_push_pushes_and_opens_the_pull_request( + root, project, gh, tmp_path): + git("remote", "set-url", "--push", "origin", + str(tmp_path / "no-such-remote.git"), cwd=root.path) + first = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert first.returncode == 2, first.stdout + first.stderr + assert gh.calls() == [], "the pull request was never attempted" + head = the_commit_stands(root, project) + + git("remote", "set-url", "--push", "origin", str(root.remote), + cwd=root.path) + rerun = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert rerun.returncode == 0, rerun.stdout + rerun.stderr + assert f"pushed {BRANCH} to origin" in rerun.stdout + assert f"pull request {URL}" in rerun.stdout + assert [call[:2] for call in gh.calls()] == [["pr", "list"], + ["pr", "create"]] + assert on_the_remote(root) == head + assert commits_on_the_branch(root) == 1 + + +# --- a failure BEFORE the commit is still rolled back (#202) ---------------- + +def refuse_the_commit(root) -> None: + """A pre-commit hook that says no, as `test_update_shape_add_ignored.py` + injects it: `core.hooksPath` in the repository's own config, and a + shebang script, which Git for Windows runs through its own `sh`.""" + hooks = root.path / ".git" / "refusing-hooks" + hooks.mkdir() + hook = hooks / "pre-commit" + hook.write_bytes(b"#!/bin/sh\necho 'pre-commit: not this commit' >&2\n" + b"exit 1\n") + hook.chmod(0o755) + git("config", "core.hooksPath", hooks.as_posix(), cwd=root.path) + + +def test_a_failure_before_the_commit_under_push_and_pr_is_rolled_back( + root, project, gh): + """THE COMMIT IS WHERE ROLLING BACK STOPS, FROM BOTH SIDES. #202 rolls + back a branch, a stage or a commit that fails; this PR keeps the commit + when a push or a pull request fails after it. With `--push --pr` passed, + a commit a hook refuses is still #202's: the copies, the pin and the + branch are put back and the step is named, and nothing of the landing + happens — no push, no `gh`, no NEXT line, no re-run promised — because + there is no commit for any of it to finish. `gh` is never reached, so + this runs on every platform.""" + refuse_the_commit(root) + head = git("rev-parse", "HEAD", cwd=root.path).stdout + + result = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + + assert result.returncode == 2, result.stdout + result.stderr + assert f"REFUSED committing the re-synced shape on {BRANCH}" \ + in result.stderr, result.stderr + assert ("ROLLED BACK: every byte this command wrote has been put back" + in result.stderr), result.stderr + for rel in (PIN, "project.yaml", CHANGED): + assert f"restored {rel}" in result.stderr, result.stderr + assert "switched HEAD back to main" in result.stderr, result.stderr + assert f"deleted {BRANCH}" in result.stderr, result.stderr + assert "nothing was rolled back" not in result.stderr + assert "run again" not in result.stderr + assert f"committed on {BRANCH}" not in result.stdout + assert "pushed" not in result.stdout + assert "NEXT" not in result.stdout, result.stdout + assert gh.calls() == [] + assert on_the_remote(root) is None + assert git("rev-parse", "HEAD", cwd=root.path).stdout == head + assert git("symbolic-ref", "--short", "HEAD", + cwd=root.path).stdout.strip() == "main" + assert git("status", "--porcelain", cwd=root.path).stdout == "" + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout.split() == ["refs/heads/main"] + + +@WINDOWS_SKIP +def test_the_same_command_after_a_rolled_back_commit_lands_and_opens( + root, project, gh): + """ROLLED BACK, THE NEXT RUN IS THE FIRST RUN AGAIN. With the hook gone + the same `apply --branch --push --pr` has work to write, so it is not + the re-run `_awaits_its_pull_request` recognises: it commits once, + pushes, and opens the pull request with the one `gh pr create` the first + run path makes, asking `gh pr list` nothing.""" + refuse_the_commit(root) + failed = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert failed.returncode == 2, failed.stdout + failed.stderr + git("config", "--unset", "core.hooksPath", cwd=root.path) + + result = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + + assert result.returncode == 0, result.stdout + result.stderr + assert "nothing to" not in result.stdout + assert f"committed on {BRANCH}" in result.stdout + assert f"pushed {BRANCH} to origin" in result.stdout + assert f"pull request {URL}" in result.stdout + assert next_line_of(result.stdout) == PR_OPEN_NEXT + assert [call[:2] for call in gh.calls()] == [["pr", "create"]] + head = the_commit_stands(root, project) + assert commits_on_the_branch(root) == 1 + assert on_the_remote(root) == head + + +# --- what does not change --------------------------------------------------- + +@pytest.mark.parametrize("flags", [(), ("--push",)], ids=["branch", "push"]) +def test_without_pr_a_rerun_on_the_branch_is_still_nothing_to_do( + root, project, gh, flags): + """No `--pr`, no re-run behaviour: the second run prints main's one line + and exits 0, and `gh` is never asked (on Windows the stub is not a + program, which a call would turn into a failure).""" + first = apply(root, project, "--branch", BRANCH, *flags, env=gh.env) + assert first.returncode == 0, first.stdout + first.stderr + again = apply(root, project, "--branch", BRANCH, *flags, env=gh.env) + assert again.returncode == 0, again.stdout + again.stderr + assert again.stdout == ( + f"nothing to do: every copied file matches {SHAPE_REPOSITORY} @ " + f"{project['b'][:12]}, which the pin already names\n") + assert again.stderr == "" + assert gh.calls() == [] + + +def test_a_branch_that_proposes_no_repin_is_nothing_to_do_under_pr( + root, project, gh): + """A BRANCH WITH COMMITS IS NOT A PULL REQUEST OWED. The root is pinned + at A and asked for A, on a branch carrying an unrelated commit: its pin + is the one `origin/main` holds, so there is nothing for a pull request to + propose, and `--pr` does not invent one.""" + git("checkout", "-q", "-b", BRANCH, cwd=root.path) + (root.path / "notes.txt").write_text("somebody's work\n", + encoding="utf-8") + git("add", "--", "notes.txt", cwd=root.path) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Unrelated work", "--", "notes.txt", cwd=root.path) + result = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env, at=project["a"]) + assert result.returncode == 0, result.stdout + result.stderr + assert result.stdout == ( + f"nothing to do: every copied file matches {SHAPE_REPOSITORY} @ " + f"{project['a'][:12]}, which the pin already names\n") + assert gh.calls() == [] + assert on_the_remote(root) is None + + +@WINDOWS_SKIP +def test_a_rerun_from_another_branch_is_still_nothing_to_do(root, project, + gh): + """THE RE-RUN ACTS ONLY ON THE BRANCH CHECKED OUT. A checkout that moved + off `--branch` after the failed run, here onto a second branch at the + same commit, so the pin it holds still differs from `origin/main`'s, gets + main's "nothing to do", and `gh` is never asked: the tool pushes and opens + nothing for a branch it is not standing on.""" + gh.failing(True) + first = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert first.returncode == 2, first.stdout + first.stderr + git("checkout", "-q", "-b", "elsewhere", cwd=root.path) + + gh.failing(False) + gh.forget() + rerun = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=gh.env) + assert rerun.returncode == 0, rerun.stdout + rerun.stderr + assert rerun.stdout == ( + f"nothing to do: every copied file matches {SHAPE_REPOSITORY} @ " + f"{project['b'][:12]}, which the pin already names\n") + assert gh.calls() == [] + + +def test_a_rerun_naming_the_tracking_branch_pushes_nothing(root, project, + gh): + """`--branch ` IS NEVER A PULL REQUEST OWED. Here main itself + carries a local re-pin commit, made from an in-place `apply`, so its pin + is not the one `origin/main` holds. `apply --branch main --push --pr` + still finds nothing to do: it asks `gh` nothing and pushes nothing, + because a pull request from the tracking branch onto itself is not one + to open, and pushing it would be a push to the default branch.""" + in_place = apply(root, project, env=gh.env) + assert in_place.returncode == 0, in_place.stdout + in_place.stderr + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-a", + "-m", "Re-pin in place", cwd=root.path) + landed = git("rev-parse", "refs/heads/main", cwd=root.remote).stdout + + result = apply(root, project, "--branch", "main", "--push", "--pr", + env=gh.env) + assert result.returncode == 0, result.stdout + result.stderr + assert result.stdout == ( + f"nothing to do: every copied file matches {SHAPE_REPOSITORY} @ " + f"{project['b'][:12]}, which the pin already names\n") + assert gh.calls() == [] + assert git("rev-parse", "refs/heads/main", + cwd=root.remote).stdout == landed, "nothing was pushed" + + +# --- `--branch` is checked before anything runs ------------------------------ + +ALPHABET = "must start with a letter or a digit" +GIT_SAYS = "`git check-ref-format --branch` refuses it" + + +@pytest.mark.parametrize("branch, why", [ + ("-shape", ALPHABET), + ("--upload-pack=touch", ALPHABET), + ("shape/a b", ALPHABET), + ("shape/x@{1}", ALPHABET), + ("shape/a..b", GIT_SAYS), + ("shape/x.lock", GIT_SAYS), + ("shape/.x", GIT_SAYS), + ("shape//x", GIT_SAYS), + ("shape/x/", GIT_SAYS), +], ids=["leading-dash", "option", "space", "at-brace", "dot-dot", "dot-lock", + "dot-part", "double-slash", "trailing-slash"]) +def test_a_branch_git_would_not_create_is_refused_before_anything_runs( + root, project, gh, branch, why): + """`--branch` BECOMES `git` AND `gh` ARGUMENTS and a word in a NEXT line + somebody pastes, the re-run's included, so `main` checks it before + anything is read, written or run: the alphabet first, so nothing that + reads as an option reaches git, then `git check-ref-format --branch`. + `--branch=` is how a value starting with `-` gets past argparse.""" + head = git("rev-parse", "HEAD", cwd=root.path).stdout + result = apply(root, project, f"--branch={branch}", "--push", "--pr", + env=gh.env) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + f"REFUSED update-branch-invalid: --branch {branch!r} "), result.stderr + assert why in result.stderr + assert "Remediation: pass a branch name such as `shape/update-`" \ + in result.stderr + assert result.stdout == "", "refused before anything was read or written" + assert git("rev-parse", "HEAD", cwd=root.path).stdout == head + assert git("status", "--porcelain", cwd=root.path).stdout == "" + assert git("for-each-ref", "--format=%(refname)", "refs/heads/", + cwd=root.path).stdout.split() == ["refs/heads/main"] + assert gh.calls() == [] + + +def test_a_bad_branch_is_refused_even_with_nothing_to_write(root, project, gh): + """THE CHECK DOES NOT WAIT FOR WORK. Pinned at A and asked for A, main + answered "nothing to do" and exit 0 whatever `--branch` said; that path + can now push the branch and hand it to `gh`, so a value refused on a run + with work is refused on this one too.""" + result = apply(root, project, "--branch=-shape", "--push", "--pr", + env=gh.env, at=project["a"]) + assert result.returncode == 2, result.stdout + result.stderr + assert result.stderr.startswith( + "REFUSED update-branch-invalid: --branch '-shape' "), result.stderr + assert result.stdout == "" + assert gh.calls() == [] + + +@pytest.mark.parametrize("branch", ["shape/update-landing", "shape/update-v1.2", + "lane-3/shape+fix@eagle", "x"]) +def test_a_branch_git_would_create_is_taken_as_written(update_shape, branch): + """A NAME WITH A `/` IS THE ORDINARY CASE, and so is the rest of the + alphabet `checked_value` always allowed: each is returned unchanged.""" + assert update_shape.checked_branch(branch) == branch + + +# --- `gh` is not installed ---------------------------------------------------- + +GH_MISSING = "`gh` could not be started (" + + +def test_a_gh_missing_from_path_is_the_step_that_failed_on_both_runs( + root, project): + """`gh` NOT INSTALLED is the likeliest way `gh pr create` fails on a + fresh machine, and `subprocess` raises for it instead of exiting: main + and the first cut of #204 both ended in a traceback, exit 1, after the + commit and the push. Now it is the named step, exit 127 in the block, + the NEXT line is the `gh pr create` to run once `gh` is installed, and a + re-run still without it says the same about its own first `gh` call. + + PATH is git's directory and Python's, nothing else. Where `gh` lives + beside one of them (Linux runners put all three in /usr/bin) no PATH + can hide it, and the in-process test after this one covers both runs.""" + git_dir = os.path.dirname(shutil.which("git")) + path = os.pathsep.join(dict.fromkeys( + [git_dir, os.path.dirname(sys.executable)])) + if shutil.which("gh", path=path): + pytest.skip("gh is installed beside git or Python here") + env = {"PATH": path} + first = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=env) + assert first.returncode == 2, first.stdout + first.stderr + assert "Traceback" not in first.stderr, first.stderr + assert f"REFUSED opening the pull request for {BRANCH}" in first.stderr + assert GH_MISSING in first.stderr + assert " exit 127" in first.stderr + line = next_line_of(first.stdout) + assert line.startswith(f"NEXT gh pr create --repo testorg/{PROJECT} " + f"--base main --head {BRANCH} --title "), line + head = the_commit_stands(root, project) + assert on_the_remote(root) == head + + again = apply(root, project, "--branch", BRANCH, "--push", "--pr", + env=env) + assert again.returncode == 2, again.stdout + again.stderr + assert "Traceback" not in again.stderr, again.stderr + assert f"REFUSED asking gh for {BRANCH}'s pull request" in again.stderr + assert GH_MISSING in again.stderr + assert next_line_of(again.stdout) == line + assert commits_on_the_branch(root) == 1 + + +# --- the same two runs in-process, on every platform ------------------------- + +@pytest.fixture +def update_shape(): + spec = importlib.util.spec_from_file_location("update_shape_landing", + UPDATE) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_the_failed_pull_request_and_its_rerun_in_process( + root, project, update_shape, monkeypatch, capsys): + """THE WINDOWS JOB'S COVERAGE of the two runs above: `gh` stubbed at + `update_shape.run`, the one function every command goes through, and + `git` — the commit and the push — run for real.""" + real = update_shape.run + calls: list = [] + fails = [True] + + def run(command, *args, **kwargs): + if command[0] != "gh": + return real(command, *args, **kwargs) + calls.append(list(command)) + if command[1:3] == ["pr", "list"]: + return "[]" + if fails[0]: + raise CommandFailed(command, kwargs.get("cwd"), 97, + "HTTP 422: Validation Failed (stub)") + return URL + + 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) + argv = ["apply", "--root", str(root.path), "--yes", + "--upstream", str(project["upstream"]), "--at", project["b"], + "--branch", BRANCH, "--push", "--pr"] + + assert update_shape.main(argv) == 2 + out, err = capsys.readouterr() + assert f"REFUSED opening the pull request for {BRANCH}" in err + assert len(calls) == 1 + assert next_line_of(out) == "NEXT " + shlex.join(calls[0]) + head = the_commit_stands(root, project) + assert on_the_remote(root) == head + + fails[0] = False + assert update_shape.main(argv) == 0 + out, err = capsys.readouterr() + assert next_line_of(out) == PR_OPEN_NEXT, out + err + assert calls[1] == ["gh", *listing()] + assert calls[2] == calls[0] + assert commits_on_the_branch(root) == 1 + + +def test_a_gh_that_cannot_be_started_in_process(root, project, update_shape, + monkeypatch, capsys): + """THE WINDOWS JOB'S COVERAGE of a missing `gh`: `update_shape.run` + raises for every `gh` call exactly as `subprocess.run` does when the + program is not on PATH, and `git` runs for real. The first run and the + re-run each name their step and end on the same `gh pr create`.""" + real = update_shape.run + calls: list = [] + + def run(command, *args, **kwargs): + if command[0] != "gh": + return real(command, *args, **kwargs) + calls.append(list(command)) + raise FileNotFoundError(2, "No such file or directory", "gh") + + 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) + argv = ["apply", "--root", str(root.path), "--yes", + "--upstream", str(project["upstream"]), "--at", project["b"], + "--branch", BRANCH, "--push", "--pr"] + + assert update_shape.main(argv) == 2 + out, err = capsys.readouterr() + assert f"REFUSED opening the pull request for {BRANCH}" in err + assert GH_MISSING in err + assert " exit 127" in err + assert next_line_of(out) == "NEXT " + shlex.join(calls[0]) + head = the_commit_stands(root, project) + assert on_the_remote(root) == head + + assert update_shape.main(argv) == 2 + out, err = capsys.readouterr() + assert f"REFUSED asking gh for {BRANCH}'s pull request" in err + assert GH_MISSING in err + assert calls[1] == ["gh", *listing()] + assert next_line_of(out) == "NEXT " + shlex.join(calls[0]) + assert commits_on_the_branch(root) == 1 diff --git a/tests/test_update_shape_pr_base.py b/tests/test_update_shape_pr_base.py index e4d6fdb0..90d309d2 100644 --- a/tests/test_update_shape_pr_base.py +++ b/tests/test_update_shape_pr_base.py @@ -274,6 +274,15 @@ def test_the_repository_is_read_where_open_pull_request_reads_it( assert line.startswith("NEXT gh pr create" if push else "NEXT git -C "), line + # And every `gh pr create` `pull_request_line` builds (#204): the one + # `--pr` runs, and the bare one a failed `--push` prints. + for pr in (True, False): + args = SimpleNamespace(branch="shape/x", push=True, pr=pr) + create = update_shape.pull_request_line( + tmp_path, args, "0" * 40, SimpleNamespace(repository="o/r"), kind) + assert create[:9] == ["gh", "pr", "create", "--repo", repository, + "--base", "main", "--head", "shape/x"], create + @pytest.mark.parametrize("kind_name, manifest_text, refusal", [ ("PROJECT_KIND", "legs:\n - role: spec\n repository: acme/AtlasSpec\n", @@ -299,9 +308,7 @@ def test_a_manifest_naming_no_repository_refuses_pr_and_leaves_the_placeholder( update_shape.load_yaml(tmp_path / kind.manifest), kind)[0] == "" with pytest.raises(update_shape.Refusal) as raised: - update_shape.open_pull_request(tmp_path, "shape/x", "0" * 40, - SimpleNamespace(repository="o/r"), - kind) + update_shape.pull_request_target(tmp_path, kind) assert raised.value.code == "update-pr-no-repository" assert refusal in str(raised.value) assert gh_calls == [] @@ -314,6 +321,12 @@ def test_a_manifest_naming_no_repository_refuses_pr_and_leaves_the_placeholder( update_shape.next_line(tmp_path, args, [], "0" * 40, [], kind) assert 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"] def test_nothing_in_the_next_line_names_a_base_when_the_pull_request_is_open( diff --git a/update-shape.py b/update-shape.py index bb8affeb..8c04d0e1 100755 --- a/update-shape.py +++ b/update-shape.py @@ -87,7 +87,9 @@ 2 a REFUSAL: no pin, an unresolvable upstream, an unreadable root apply 0 applied (or there was nothing to do) 2 a REFUSAL, including a validator that went red or a branch, - stage or commit that failed (rolled back) + stage or commit that failed (rolled back); or a push or pull + request failing after the commit, which is KEPT, and the + NEXT line printed then finishes the job (#204) STANDARD LIBRARY ONLY, like everything else shipped here. """ @@ -97,6 +99,7 @@ import argparse import ast import hashlib +import json import os import re import shlex @@ -124,6 +127,13 @@ #: `owner/repo`, the only remote spelling `--upstream` accepts. REPOSITORY_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") +#: How `--branch` may be spelled: the alphabet `checked_value` already +#: allowed it, less the `~` git refuses anyway, and a FIRST character that is +#: a letter or a digit, so no value reads as an option (`--upload-pack=…`) or +#: starts with a `/` or a `.`. `checked_branch` holds it to this, then to +#: git's own rules. +BRANCH_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9._/@+-]{0,254}") + #: Where `adopt-project.py` puts a template file whose name the source #: repository already uses. Spelled here because this tool must map #: `shape/Makefile` back to the template it was cut from; @@ -1538,6 +1548,54 @@ def current_branch(root: Path) -> str | None: return proc.stdout.strip() or None +BRANCH_REMEDIATION = ( + "Remediation: pass a branch name such as `shape/update-`. It starts " + "with a letter or a digit, holds only letters, digits and `. _ / @ + -`, " + "and is one `git check-ref-format --branch ` accepts: no `..`, no " + "`//`, no `@{`, no trailing `/` or `.`, and no part that starts with `.` " + "or ends in `.lock`.") + + +def checked_branch(value: str) -> str: + """`--branch`, CHECKED ONCE, in `main`, before this tool reads, writes or + runs anything (#204). + + The value becomes `git` arguments (the branch `apply` creates and pushes, + and the ref its re-run reads), `gh pr create --head`, and a word in the + NEXT line a human or an agent pastes and runs. A value that starts with + `-` is an option to whichever of those reads it first, so the alphabet + is checked before anything else, and only a value `BRANCH_RE` admits is + 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. + """ + if not BRANCH_RE.fullmatch(value): + raise Refusal( + "update-branch-invalid", + f"--branch {value!r} is not a branch name this tool will put on a " + "`git` or `gh` command line: it must start with a letter or a " + "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) + except OSError as exc: + 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() + 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) + return value + + def branch_exists(root: Path, branch: str) -> bool: """Is `branch` a local branch of `root` — checked out or not?""" proc = subprocess.run( @@ -1548,8 +1606,9 @@ def branch_exists(root: Path, branch: str) -> bool: def tracking_branch_of(manifest: dict | None) -> str: """This root's tracking branch, out of an already-loaded manifest — the - same field `open_pull_request` lands its pull request on, and the one - base `next_line` prints in the `gh pr create` it hands a human (#209). + same field `pull_request_target` lands `--pr`'s pull request on, and the + one base `next_line` prints in the `gh pr create` it hands a human + (#209). `--branch` may never commit onto it directly: these organisations are pull-request only, and a commit whose only next step is a push to the default branch is not one this tool will make. @@ -1567,7 +1626,7 @@ def pull_request_repository(manifest: dict | None, kind: Kind = PROJECT_KIND) -> tuple[str, str]: """`(repository, why_none)`: the repository a shape update's pull request is opened against, out of an already-loaded manifest, and what to say - where it names none (#209). One of the two values `open_pull_request` + where it names none (#209). One of the two values `pull_request_target` and `next_line` both have to agree on, beside `tracking_branch_of`: they used to settle them separately, and the printed line said `main` and `/` whatever the manifest recorded. @@ -1576,10 +1635,12 @@ def pull_request_repository(manifest: dict | None, own, having no legs to read one from, being a holder rather than a project: its `repository:`, or its `org:` and `name:` joined where there is no `repository:`. An empty answer is the caller's to act on: - `open_pull_request` refuses with `why_none`, since there is nothing to - open a pull request against, and `next_line` keeps the placeholder it - always printed, since it runs after the commit and a manifest that names - no repository is no reason to take that back. + `pull_request_target` refuses with `why_none` under `--pr`, before the + commit, since there is nothing to open a pull request against; + `next_line` keeps the placeholder it always printed, since it runs + after the commit and a manifest that names no repository is no reason + to take that back; and `pull_request_line`'s bare form, the one a + failed `--push` prints, leaves `--repo` off (#204). """ manifest = manifest or {} if kind is FAMILY_KIND: @@ -2347,17 +2408,25 @@ def _land(root: Path, args, paths: list[str], added: list[str], target: str, without `--branch`, which `main` refuses before this is ever reached. The commit goes through `ledger` and closes it (#202), so a failure up to and including the commit is rolled back and one after it is not. + + THE COMMIT IS WHERE ROLLING BACK STOPS (#204). A push or a pull request + that fails after it is `landing_failed`'s: named by its own step, the + commit kept, and a NEXT line that finishes the job. The `gh pr create` + that line ends with is built HERE, BEFORE the commit, so a root with no + repository for `--pr` to open against is refused while there is still + nothing committed to keep, and the failure path reads no file at all. """ if not args.branch: return + create = (pull_request_line(root, args, target, upstream, kind) + if args.push or args.pr else []) commit_on_branch(root, args.branch, paths, target, upstream, count, kind, added, args.trailer, ledger=ledger) print(f"\n committed on {args.branch}: " + ", ".join(paths)) if args.push: - run(["git", "push", "-q", "-u", "origin", args.branch], cwd=root) - print(f" pushed {args.branch} to origin") + push_branch(root, args, create) if args.pr: - open_pull_request(root, args.branch, target, upstream, kind) + open_pull_request(root, args, create) def cmd_apply(args) -> int: @@ -2373,6 +2442,8 @@ def cmd_apply(args) -> int: target) _note_untaken_additions(additions, taking) if _nothing_to_do(rows, pinned, target, taking): + if _awaits_its_pull_request(root, args, kind): + return _finish_landing(root, args, upstream, target, kind) print(f"nothing to do: every copied file matches " f"{upstream.repository} @ {target[:12]}, which the pin " "already names") @@ -2417,9 +2488,10 @@ def _rolled_back(exc: Exception, ledger: Rollback) -> int: staging the additions, committing) rather than always "committing". NOTHING TO PUT BACK keeps the answer each exit always gave: a refusal - before the first write is re-raised as it was, and a git command that - fails after the commit landed — a push, a pull request — is #204's, and - prints the line it always printed. + before the first write is re-raised as it was. A push or a pull request + that fails after the commit never reaches here: `landing_failed` names + it and raises `LandingIncomplete`, which is neither of this arm's two + classes, so the commit `ledger.land()` closed on stays (#204). """ undone = ledger.undo() if undone is not None: @@ -2439,23 +2511,254 @@ def _rolled_back(exc: Exception, ledger: Rollback) -> int: return 2 -def open_pull_request(root: Path, branch: str, target: str, - upstream: Upstream, kind: Kind = PROJECT_KIND) -> None: +class LandingIncomplete(Exception): + """The re-pin is COMMITTED on its branch and a step after the commit + failed (#204). Already reported, NEXT line included, by `landing_failed`. + + ITS OWN CLASS, NEITHER A `Refusal` NOR A `CommandFailed`, because + `cmd_apply` has an arm for each and both are wrong here: the `Refusal` + arm rolls the working tree back under a commit that already holds those + bytes, and the `CommandFailed` arm names the commit as the step that + failed and prints no NEXT line, which is how a `gh pr create` that failed + after the commit and the push both landed came out as "REFUSED committing + the re-synced shape". `main` turns it into exit 2 and says nothing more. + """ + + +def landing_failed(root: Path, args, exc: CommandFailed, act: str, + create: list[str]) -> LandingIncomplete: + """Report a step that failed AFTER the commit: the step by name, what + stands, and the one command that finishes the job. + + WHAT STANDS IS READ FROM GIT, not from the flags: `push -u` moves + `refs/remotes/origin/`, so "origin has it" is that ref agreeing + with HEAD. Git is asked which remote-tracking refs point at HEAD and the + branch is looked for in its answer, so the name is never a `git` + argument here. A NEXT line that cannot vouch for the push carries it in + front of the `gh pr create` (a push origin already has is a no-op). The + `gh pr create` is `create` itself, quoted to be pasted: under `--pr` the + very command `--pr` runs, title and body included, so the pull request + opened by hand is the one the tool would have opened. + + A RE-RUN IS PROMISED ONLY UNDER `--pr`, because only then does one finish + the job: `_awaits_its_pull_request` recognises it. A failed `--push` + without `--pr` is finished by the NEXT line alone. The re-run named + carries `--push` whenever origin is not known to have the branch: a + re-run pushes only when asked to, and `gh` opens no pull request from a + branch origin lacks. + + STDOUT IS FLUSHED FIRST, so a combined log reads in the order things + happened: the commit and the push above the refusal, not below it. + """ + branch = args.branch + head = (_git_answer(root, ["rev-parse", "HEAD"]) or "").strip() + at_head = (_git_answer(root, ["for-each-ref", "--format=%(refname)", + "--points-at", "HEAD", "refs/remotes/origin/"]) + or "").splitlines() + pushed = bool(head) and f"refs/remotes/origin/{branch}" in at_head + sys.stdout.flush() + print(exc.loudly(act), file=sys.stderr) + left = ("its pull request: the NEXT line opens it" if pushed + else "the push and the pull request: the NEXT line makes both") + rerun = "apply --pr" if pushed else "apply --push --pr" + again = (f", and so does `{rerun}` run again on {branch} once the " + "step above can succeed, because with nothing left to write it " + "opens the pull request that is still missing" if args.pr + else "") + print(f"{branch} carries the re-pin at {head[:12] or 'HEAD'}" + + (", and origin has it" if pushed + else ", and origin does not have it yet") + + f": that commit stands and nothing was rolled back. What is left " + f"is {left}{again}.", file=sys.stderr) + sys.stderr.flush() + push = ("" if pushed else shlex.join(["git", "-C", str(root), "push", "-u", + "--", "origin", branch]) + " && ") + print() + print(f"NEXT {push}{shlex.join(create)}") + return LandingIncomplete(act) + + +def _landing_run(command: list[str], root: Path) -> str: + """`run`, for the steps after the commit (#204), with a program that + cannot be started reported as the failed step it is. `gh` not installed, + or not on PATH, is the likeliest way `gh` fails on a fresh machine, and + `subprocess` raises `OSError` for it rather than returning an exit code, + so it escaped every arm as a traceback after the commit and the push had + landed. It becomes a `CommandFailed` with the exit 127 a shell gives a + command it cannot find, which `landing_failed` then names, NEXT line + included.""" + try: + return run(command, cwd=root) + except OSError as exc: + raise CommandFailed( + command, root, 127, + f"`{command[0]}` could not be started " + f"({exc.strerror or exc}): is it installed and on PATH?") from exc + + +def push_branch(root: Path, args, create: list[str]) -> None: + """`git push -u -- origin `, for the first run and for the re-run + alike, `--` ending git's options before the two positionals. A refusal + from origin is `landing_failed`'s (#204): the commit it was pushing + stands, so the step is named and nothing is rolled back.""" + try: + _landing_run(["git", "push", "-q", "-u", "--", "origin", args.branch], + root) + except CommandFailed as exc: + raise landing_failed(root, args, exc, f"pushing {args.branch}", + create) from exc + print(f" pushed {args.branch} to origin") + + +def _awaits_its_pull_request(root: Path, args, kind: Kind) -> bool: + """Is this no-write run a RE-RUN on the branch whose pull request `--pr` + still owes (#204)? Without this, the run that follows a failed `gh pr + create` found the pin already at the target, said "nothing to do" and + exited 0, so the tool never opened the pull request it was asked for. + + `--pr` was passed; `--branch` is the branch checked out, which is where + the earlier run left the checkout; it is not the tracking branch, which + has no pull request to open onto itself; and the branch PROPOSES A PIN — + its `contracts/shape-pin.yaml` is not the one `origin/` holds, + as far as this clone has seen. A branch cut from a tracking branch that + already carries the re-pin proposes nothing, however many other commits + it holds, and stays "nothing to do". A clone that has never seen + `origin/` cannot tell, and asks `gh`, which answers for itself. + """ + if not (args.pr and args.branch) or current_branch(root) != args.branch: + return False + base = tracking_branch_of(load_yaml(root / kind.manifest)) + if args.branch == base: + return False + pin = "contracts/shape-pin.yaml" + proposed = _git_answer(root, ["rev-parse", "--verify", "--quiet", + f"HEAD:{pin}"]) + landed = _git_answer(root, ["rev-parse", "--verify", "--quiet", + f"refs/remotes/origin/{base}:{pin}"]) + return proposed is not None and proposed != landed + + +def _pull_requests(answer: str) -> "list[tuple[str, str]] | None": + """`(STATE, url)` per pull request in `gh pr list --json state,url`'s + answer, or None when the answer is not that shape.""" + try: + pulls = json.loads(answer or "[]") + except ValueError: + return None + if not isinstance(pulls, list) or \ + not all(isinstance(pull, dict) for pull in pulls): + return None + return [(str(pull.get("state") or "").upper(), str(pull.get("url") or "")) + for pull in pulls] + + +def _finish_landing(root: Path, args, upstream: Upstream, target: str, + kind: Kind) -> int: + """The re-run `_awaits_its_pull_request` recognised: nothing to write, + and the landing the flags asked for finished instead (#204). + + `gh` IS ASKED FIRST, before anything is pushed or opened: a branch that + already has a pull request — open, merged or closed — gets no second one + and no push, and the run says which it has (pushing first would put back + a merged branch its remote had deleted). Only a branch with none is + 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. + """ + 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) + listing = ["gh", "pr", "list", "--repo", repository, "--head", branch, + "--state", "all", "--json", "state,url"] + asking = f"asking gh for {branch}'s pull request" + try: + answer = _landing_run(listing, root) + except CommandFailed as exc: + raise landing_failed(root, args, exc, asking, create) from exc + pulls = _pull_requests(answer) + if pulls is None: + raise landing_failed(root, args, + CommandFailed(listing, root, 0, answer), + asking + " (the answer is not a JSON list)", + create) + opened = [url for state, url in pulls if state == "OPEN"] + if opened: + print(f" pull request {opened[0]} is already open") + elif pulls: + state, url = pulls[0] + print(f" pull request {url} for {branch} is {state.lower()}; a " + "re-run opens no second one") + return 0 + else: + if args.push: + push_branch(root, args, create) + open_pull_request(root, args, create) + print() + print("NEXT the pull request is open; ask a human to review and merge it") + return 0 + + +def pull_request_target(root: Path, kind: Kind) -> tuple[str, str]: + """`(repository, base)` the pull request is opened against, out of the + kind's manifest. Split out of `open_pull_request` (#204) so the command + is built once, before the commit, and a re-run asks `gh` about the same + pair.""" manifest = load_yaml(root / kind.manifest) base = tracking_branch_of(manifest) repository, missing = pull_request_repository(manifest, kind) if not repository: raise Refusal("update-pr-no-repository", missing) - url = run(["gh", "pr", "create", "--repo", repository, "--base", base, - "--head", 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."], cwd=root) + return repository, base + + +def pull_request_line(root: Path, args, target: str, upstream: Upstream, + kind: Kind) -> 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. + + 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.""" + 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] + 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."] + + +def open_pull_request(root: Path, args, create: list[str]) -> None: + """Run `create`, the `gh pr create` `pull_request_line` built, for + `args.branch`, which already carries the re-pin. + + A FAILED `gh pr create` IS REPORTED HERE, as what it is (#204): the + commit, and the push when there was one, have already landed, so the + refusal names this step, rolls nothing back, and ends on a NEXT line. + """ + try: + url = _landing_run(create, root) + except CommandFailed as exc: + raise landing_failed(root, args, exc, + f"opening the pull request for {args.branch}", + create) from exc print(f" pull request {url}") @@ -2466,7 +2769,8 @@ def next_line(root: Path, args, paths: list[str], target: str, return if args.branch: # The base and the repository the pull request `--pr` would have - # opened lands on, read the way `open_pull_request` reads them (#209). + # opened lands on, read the way `pull_request_target` reads them + # (#209). manifest = load_yaml(root / kind.manifest) base = shlex.quote(tracking_branch_of(manifest)) repository, _ = pull_request_repository(manifest, kind) @@ -2558,7 +2862,9 @@ def build_parser() -> argparse.ArgumentParser: help="push the branch to origin (needs --branch)") apply_parser.add_argument("--pr", action="store_true", help="open a pull request with `gh` (needs " - "--branch)") + "--branch); a later run on that branch " + "with nothing left to write opens it if " + "it is still missing") return parser @@ -2570,6 +2876,11 @@ def main(argv: list[str] | None = None) -> int: try: if args.command == "check": return cmd_check(args) + if args.branch: + # BEFORE ANY USE (#204): every path below may run `git` or `gh` + # with it or print it in a NEXT line, the re-run's included, + # which never reaches the commit's own check. + args.branch = checked_branch(args.branch) # NAMED BY WHAT WAS ACTUALLY PASSED. The three fail for one # reason — there is no commit — and a refusal listing flags nobody # typed reads as a refusal of something else. `--trailer` joined them @@ -2597,6 +2908,10 @@ def main(argv: list[str] | None = None) -> int: except CommandFailed as exc: print(exc.loudly("updating the shape"), file=sys.stderr) return 2 + except LandingIncomplete: + # Reported where it happened, NEXT line included (#204); the exit + # code is all that is left to give. + return 2 if __name__ == "__main__":