From 6ed4e7000f8f8e89d091483569c4cdc3d5688137 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 18:55:02 +0000 Subject: [PATCH 1/5] update-shape.py: a push or `gh pr create` that fails after `apply --branch` committed is named as that step, keeps the commit and ends on a NEXT line, and a re-run with `--pr` on the branch opens the missing pull request, so the tool no longer refuses "committing the re-synced shape" and then answers "nothing to do" (#204) `_land` commits, pushes and opens the pull request. A `CommandFailed` from the push or from `gh pr create` used to reach `cmd_apply`'s one `except CommandFailed` arm, which named every failure "committing the re-synced shape", printed no NEXT line and did not flush stdout first. The run after it found the pin already at the target, said "nothing to do" and exited 0, so the tool never opened the pull request `--pr` asked for. The commit is now where rolling back stops: - `push_branch` and `open_pull_request` catch their own failure and hand it to `landing_failed`. The refusal names the step (`REFUSED pushing `, `REFUSED opening the pull request for `), says the commit stands and nothing was rolled back, and ends on a NEXT line. The exit stays 2. - The NEXT line is the `gh pr create` that `--pr` runs, title and body included, shell-quoted so it pastes as the same argv. `pull_request_line` builds it before the commit. When this clone's `origin/` is not HEAD, the line pushes first: `git -C push -u origin && gh pr create ...`. Under `--push` without `--pr` it ends on the bare `gh pr create --base --head `. - stdout is flushed before the refusal, so a combined log reads commit, push, REFUSED, NEXT, in that order. - `LandingIncomplete` carries the failure to `main` as exit 2. It is neither a `Refusal` (that arm rolls the working tree back under a commit holding those bytes) nor a `CommandFailed` (that arm names the commit). - A re-run with `--pr` and nothing to write finishes the landing when the checkout is on `--branch`, that branch is not the tracking branch, and its `contracts/shape-pin.yaml` differs from `origin/`'s. `_finish_landing` asks `gh pr list --state all` before anything else: - an open pull request is reported, and no second one is opened; - a merged or closed one is reported, and nothing is pushed, so a merged branch the remote deleted stays deleted; - with none, `--push` pushes again and the same `gh pr create` runs. There is no second commit. - The refusal promises that re-run only under `--pr`, and names `apply --push --pr` when origin does not have the branch. - The `--pr` help says a later run on the branch opens a pull request that is still missing. docs/cli.md is regenerated. Unchanged, and measured byte-identical to main a5b09c3 in stdout, stderr, exit code and the `gh` argv log: - `check`; - `apply --branch`, and its re-run; - `apply --branch --push`, and its re-run; - `apply --branch --push --pr` with `gh` succeeding. A re-run without `--pr`, from a checkout not on `--branch`, with `--branch` naming the tracking branch, or on a branch whose pin is `origin/`'s still prints "nothing to do" and exits 0, asking `gh` nothing and pushing nothing. Left out: the `--base main` that `next_line` prints after a successful `--branch` or `--branch --push` is #209's; a failure inside the commit, staging an `--add` path, is #202's; the adopt-side unflushed log is #188's. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 4 +- tests/test_update_shape_landing.py | 536 +++++++++++++++++++++++++++++ update-shape.py | 254 +++++++++++++- 3 files changed, 776 insertions(+), 18 deletions(-) create mode 100644 tests/test_update_shape_landing.py diff --git a/docs/cli.md b/docs/cli.md index 5c50a686..cd32a193 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -486,7 +486,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_landing.py b/tests/test_update_shape_landing.py new file mode 100644 index 00000000..650b8a45 --- /dev/null +++ b/tests/test_update_shape_landing.py @@ -0,0 +1,536 @@ +# 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 and 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 and 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 "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 --base main --head {BRANCH}") + 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 + + +# --- 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" + + +# --- 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 diff --git a/update-shape.py b/update-shape.py index 553c1508..4411b3bb 100755 --- a/update-shape.py +++ b/update-shape.py @@ -80,7 +80,9 @@ 1 updates are available, or drift a human must resolve 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 (rolled back) + 2 a REFUSAL, including a validator that went red (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. """ @@ -90,8 +92,10 @@ import argparse import ast import hashlib +import json import os import re +import shlex import shutil import subprocess import sys @@ -1867,17 +1871,25 @@ def _land(root: Path, args, paths: list[str], added: list[str], target: str, Nested the way the flags are: `--push` and `--pr` have nothing to act on without `--branch`, which `main` refuses before this is ever reached. + + 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) 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: @@ -1893,6 +1905,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") @@ -1928,8 +1942,178 @@ def cmd_apply(args) -> int: upstream.close() -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, and 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() + remote = (_git_answer(root, ["rev-parse", "--verify", "--quiet", + f"refs/remotes/origin/{branch}"]) or "").strip() + pushed = bool(head) and remote == 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 push_branch(root: Path, args, create: list[str]) -> None: + """`git push -u origin `, for the first run and for the re-run + alike. 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: + run(["git", "push", "-q", "-u", "origin", args.branch], cwd=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 = run(listing, cwd=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) if kind is FAMILY_KIND: @@ -1950,16 +2134,46 @@ def open_pull_request(root: Path, branch: str, target: str, "repository to open a pull request against") 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 `--base`/`--head` form the + plain `--branch` NEXT line prints, against this root's own tracking + branch.""" + if not args.pr: + base = tracking_branch_of(load_yaml(root / kind.manifest)) + return ["gh", "pr", "create", "--base", base, "--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 = run(create, cwd=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}") @@ -2034,7 +2248,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 @@ -2070,6 +2286,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__": From 8705b27a329da1cbaf9a461ee73c0f824098acc1 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:13:10 +0000 Subject: [PATCH 2/5] update-shape.py: `--branch` is checked once, in `main`, against a conservative alphabet and then `git check-ref-format --branch`, before anything is read, written or run, so a value that reads as an option never reaches `git`, `gh` or a NEXT line (#204) Fix round 1 for SonarCloud pythonsecurity:S8705, which flagged two places on PR #227 where `--branch` reaches a command with no validation it recognises: - `_git_answer`'s `subprocess.run`, reached from `landing_failed` on the re-run path, which never passes through the commit's own `checked_value`; - the printed NEXT line's `git -C push -u origin `, which an agent pastes and runs. The fix: - `checked_branch` runs in `main`, before `cmd_apply`, whenever `--branch` is given. It refuses as `REFUSED update-branch-invalid`, with a remediation line, any value that: - `BRANCH_RE` does not fullmatch. That is the alphabet `checked_value` allowed, less `~`, with a letter or digit first, so there is no leading `-`, `/` or `.`; - `git check-ref-format --branch` then refuses, or would expand rather than echo. Git is asked only once the alphabet has passed. - `landing_failed` asks git which remote-tracking refs point at HEAD (`for-each-ref --points-at HEAD refs/remotes/origin/`) and looks for the branch in that answer, so the name is no longer a `git` argument there. - `--` ends git's options before the positionals: in `git push -q -u -- origin `, and in the NEXT line's `git -C push -u -- origin `. - The two composite assertions Sonar also flagged (S9073) are split. Measured with the fixture from the first commit (Atlas from a5b09c3 plus upstream commit B), running `apply --branch= --push --pr`: - On main a5b09c3 and on 6ed4e70, `-shape`, `--upload-pack=touch`, `shape/a b` and `shape/x@{1}` are refused `unsafe-value` only after the 12 lines of the rewrite. - `shape/a..b` and `shape/x.lock` fail as "REFUSED committing the re-synced shape" at `git checkout -b`, with the rewrite left modified in the tree. - `-shape` with nothing to do says "nothing to do" and exits 0. - On this head every one of them is refused `update-branch-invalid` with nothing on stdout and the tree untouched. - `shape/update-y` lands byte-identical to main: stdout, stderr, exit code, the `gh` argv log and the resulting refs. `--help` is unchanged, so docs/cli.md is too. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- tests/test_update_shape_landing.py | 66 +++++++++++++++++++++--- update-shape.py | 83 ++++++++++++++++++++++++++---- 2 files changed, 134 insertions(+), 15 deletions(-) diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index 650b8a45..0d0b86a0 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -194,7 +194,8 @@ def test_a_failed_pull_request_is_named_and_its_next_line_is_the_command( assert f"pushed {BRANCH} to origin" in result.stdout calls = gh.calls() - assert len(calls) == 1 and calls[0][:2] == ["pr", "create"], 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) @@ -222,9 +223,10 @@ def test_a_failed_pull_request_on_an_unpushed_branch_pushes_first( assert f"pushed {BRANCH}" not in result.stdout calls = gh.calls() - assert len(calls) == 1 and calls[0][:2] == ["pr", "create"], calls - push = shlex.join(["git", "-C", str(root.path), "push", "-u", "origin", - BRANCH]) + 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) @@ -340,6 +342,8 @@ def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, result = apply(root, project, "--branch", BRANCH, *flags) assert result.returncode == 2, result.stdout + result.stderr assert f"REFUSED pushing {BRANCH}" in result.stderr + assert f"git push -q -u -- origin {BRANCH}" in result.stderr, ( + "`--` ends git's options before the branch") assert "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 @@ -347,8 +351,8 @@ def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, 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]) + 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} " \ @@ -477,6 +481,56 @@ def test_a_rerun_naming_the_tracking_branch_pushes_nothing(root, project, 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() == [] + + +@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 + + # --- the same two runs in-process, on every platform ------------------------- @pytest.fixture diff --git a/update-shape.py b/update-shape.py index 4411b3bb..1fac14ff 100755 --- a/update-shape.py +++ b/update-shape.py @@ -118,6 +118,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; @@ -1447,6 +1454,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( @@ -1963,7 +2018,9 @@ def landing_failed(root: Path, args, exc: CommandFailed, act: str, 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, and a NEXT line that cannot vouch for the push carries it in + 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 @@ -1981,9 +2038,10 @@ def landing_failed(root: Path, args, exc: CommandFailed, act: str, """ branch = args.branch head = (_git_answer(root, ["rev-parse", "HEAD"]) or "").strip() - remote = (_git_answer(root, ["rev-parse", "--verify", "--quiet", - f"refs/remotes/origin/{branch}"]) or "").strip() - pushed = bool(head) and remote == head + 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 @@ -2000,18 +2058,20 @@ def landing_failed(root: Path, args, exc: CommandFailed, act: str, f"is {left}{again}.", file=sys.stderr) sys.stderr.flush() push = ("" if pushed else shlex.join(["git", "-C", str(root), "push", "-u", - "origin", branch]) + " && ") + "--", "origin", branch]) + " && ") print() print(f"NEXT {push}{shlex.join(create)}") return LandingIncomplete(act) def push_branch(root: Path, args, create: list[str]) -> None: - """`git push -u origin `, for the first run and for the re-run - alike. A refusal from origin is `landing_failed`'s (#204): the commit it - was pushing stands, so the step is named and nothing is rolled back.""" + """`git push -u -- origin `, 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: - run(["git", "push", "-q", "-u", "origin", args.branch], cwd=root) + run(["git", "push", "-q", "-u", "--", "origin", args.branch], + cwd=root) except CommandFailed as exc: raise landing_failed(root, args, exc, f"pushing {args.branch}", create) from exc @@ -2259,6 +2319,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 From 60038cc225c91e19f69aaa287e01f27b67493e1d Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:33:43 +0000 Subject: [PATCH 3/5] update-shape.py: a `gh` (or `git`) that cannot be started after `apply --branch` committed is named as that step with exit 127 and the NEXT line, on the first run and on the `--pr` re-run, so a machine without `gh` no longer ends in a `FileNotFoundError` traceback, exit 1, after the commit and the push (#204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix round 2, from the independent review (rv-update-shape) of 8705b27. - `_landing_run` wraps `run` for the three steps after the commit: the push, the re-run's `gh pr list` and `gh pr create`. `subprocess` raises `OSError` for a program it cannot start rather than returning an exit code, so it escaped every arm. It now becomes `CommandFailed(command, root, 127, "`gh` could not be started (): is it installed and on PATH?")`, which `landing_failed` names as the step, keeping the commit, and ends on the NEXT line: the `gh pr create` to run once `gh` is installed. - Measured with PATH=/usr/bin:/bin (git and python3, no gh), `apply --branch shape/update-y --push --pr`, then the same command again: - main a5b09c3: exit 1 `FileNotFoundError: ... 'gh'` after "committed on" and "pushed"; the re-run says "nothing to do", exit 0. - 8705b27: the same first run; the re-run is exit 1 with the same traceback. - this head: `REFUSED opening the pull request for shape/update-y`, exit 127 in the block, NEXT `gh pr create --repo testorg/Atlas --base main --head shape/update-y --title …`, exit 2. The re-run is `REFUSED asking gh for shape/update-y's pull request`, with the same NEXT line, exit 2, and still one commit. - Tests: - `test_a_gh_missing_from_path_is_the_step_that_failed_on_both_runs` runs with a PATH of git's and Python's directories, and skips where `gh` lives beside one of them (Linux runners); - `test_a_gh_that_cannot_be_started_in_process` covers both runs on every platform; - `test_a_bad_branch_is_refused_even_with_nothing_to_write` pins fix round 1's one behaviour change. - Removing the `OSError` arm fails the two `gh` tests. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- tests/test_update_shape_landing.py | 104 +++++++++++++++++++++++++++++ update-shape.py | 26 ++++++-- 2 files changed, 126 insertions(+), 4 deletions(-) diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index 0d0b86a0..1a586857 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -523,6 +523,20 @@ def test_a_branch_git_would_not_create_is_refused_before_anything_runs( 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): @@ -531,6 +545,52 @@ def test_a_branch_git_would_create_is_taken_as_written(update_shape, branch): 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 @@ -588,3 +648,47 @@ def run(command, *args, **kwargs): 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/update-shape.py b/update-shape.py index 1fac14ff..ea2f50db 100755 --- a/update-shape.py +++ b/update-shape.py @@ -2064,14 +2064,32 @@ def landing_failed(root: Path, args, exc: CommandFailed, act: str, 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: - run(["git", "push", "-q", "-u", "--", "origin", args.branch], - cwd=root) + _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 @@ -2143,7 +2161,7 @@ def _finish_landing(root: Path, args, upstream: Upstream, target: str, "--state", "all", "--json", "state,url"] asking = f"asking gh for {branch}'s pull request" try: - answer = run(listing, cwd=root) + answer = _landing_run(listing, root) except CommandFailed as exc: raise landing_failed(root, args, exc, asking, create) from exc pulls = _pull_requests(answer) @@ -2229,7 +2247,7 @@ def open_pull_request(root: Path, args, create: list[str]) -> None: refusal names this step, rolls nothing back, and ends on a NEXT line. """ try: - url = run(create, cwd=root) + url = _landing_run(create, root) except CommandFailed as exc: raise landing_failed(root, args, exc, f"opening the pull request for {args.branch}", From c96396be0c47ea692edb50abca7199c0f2108b5f Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:04:52 +0000 Subject: [PATCH 4/5] fix round 3 (#204): a failure before the commit under `--push --pr` is rolled back as #202 rolls it back, and the same command then lands and opens the pull request on one commit After merging main (#233, #202), the commit is where rolling back stops from both sides: #202's ledger puts back a branch, a stage or a commit that fails and closes at the commit, and #204 keeps the commit when a push or `gh pr create` fails after it. The post-commit half was already pinned on the merged tree by the landing tests (`the_commit_stands`: clean tree, HEAD on the branch with the re-pin) and by #202's `test_a_failure_after_the_commit_lands_rolls_nothing_back`. Nothing drove the pre-commit half with the landing flags passed. Two tests do now: - `test_a_failure_before_the_commit_under_push_and_pr_is_rolled_back` (every platform; `gh` is never reached): a pre-commit hook refuses the commit under `--branch --push --pr`. Exit 2, `REFUSED committing the re-synced shape on `, #202's ROLLED BACK block (pin, manifest and copy restored, HEAD switched back, the branch deleted), and nothing of the landing: no push, no `gh` call, no NEXT line, no re-run promised, origin without the branch, the tree and refs as they were. - `test_the_same_command_after_a_rolled_back_commit_lands_and_opens` (shell `gh` stub, WINDOWS_SKIP): with the hook gone the same command has work to write, so it is the first run again, not the `--pr` re-run: one commit, pushed, one `gh pr create` and no `gh pr list`. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- tests/test_update_shape_landing.py | 84 ++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index 1a586857..dc838c6a 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -390,6 +390,90 @@ def test_a_rerun_after_a_failed_push_pushes_and_opens_the_pull_request( 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"]) From c874e4f71058bd6bba94ae100c92603ef8d4a6d1 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Sun, 11 Oct 2026 22:57:48 +0000 Subject: [PATCH 5/5] fix round 4 (#204): the `gh pr create` a failed `--push` prints names the repository with #218's `--repo`, so every `gh pr create` this tool prints or runs says which repository it opens against With #218 (#209) merged, `next_line`'s `--branch` and `--push` lines carry `--repo `, and `--pr` always did. The one left was `pull_request_line`'s bare form, which is the `gh pr create` at the end of the NEXT line a failed push prints under `--push` without `--pr` (`git -C push -u -- origin && gh pr create --base --head `). Pasted from another directory, its `gh` opened against whatever repository the shell was in. - `pull_request_line` without `--pr` reads the manifest once and builds `gh pr create --repo --base --head ` through #218's `pull_request_repository`, the same answer `--pr` and `next_line` use. - A manifest naming no repository leaves `--repo` off rather than refusing: without `--pr` nothing is opened, and `next_line`'s `/` placeholder would be one literal argument in a line quoted to be pasted as an argv. - Tests: the landing `failed_push[push]` case asserts `--repo testorg/Atlas`; #218's repository test checks both `pull_request_line` forms for a project and both family spellings; its no-repository test checks the bare form leaves `--repo` off. Lane: openreposhape-3 (openRepoShape-3) Co-Authored-By: Claude Opus 5.5 --- tests/test_update_shape_landing.py | 3 ++- tests/test_update_shape_pr_base.py | 15 +++++++++++++++ update-shape.py | 23 ++++++++++++++++------- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/tests/test_update_shape_landing.py b/tests/test_update_shape_landing.py index dc838c6a..b5d48d43 100644 --- a/tests/test_update_shape_landing.py +++ b/tests/test_update_shape_landing.py @@ -359,7 +359,8 @@ def test_a_failed_push_is_named_and_its_next_line_pushes_first(root, project, "--title " in line assert f"`apply --push --pr` run again on {BRANCH}" in result.stderr else: - assert line.endswith(f"&& gh pr create --base main --head {BRANCH}") + 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) diff --git a/tests/test_update_shape_pr_base.py b/tests/test_update_shape_pr_base.py index 447fa0f7..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", @@ -312,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 7151d5b6..8c04d0e1 100755 --- a/update-shape.py +++ b/update-shape.py @@ -1636,10 +1636,11 @@ def pull_request_repository(manifest: dict | None, project: its `repository:`, or its `org:` and `name:` joined where there is no `repository:`. An empty answer is the caller's to act on: `pull_request_target` refuses with `why_none` under `--pr`, before the - commit, since there is nothing to open a pull request against, and + 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. + 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: @@ -2717,12 +2718,20 @@ 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 `--base`/`--head` form the - plain `--branch` NEXT line prints, against this root's own tracking - branch.""" + 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: - base = tracking_branch_of(load_yaml(root / kind.manifest)) - return ["gh", "pr", "create", "--base", base, "--head", args.branch] + 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",