update-shape.py: a --pr re-run finishes the landing only when the working tree holds what HEAD commits at the pin, the manifest and every pinned copy, and is refused as update-rerun-uncommitted otherwise, so an in-place apply --at C left uncommitted on a branch that re-pins B no longer gets B pushed under a pull request titled C (#333)

tests/test_update_shape_landing.py

@@ -61,6 +61,8 @@
 UPDATE = REPO / "update-shape.py"
 BRANCH = "shape/update-landing"
 PIN = "contracts/shape-pin.yaml"
+#: The copied file upstream commit C changes, and only C (#333).
+PINS = "scripts/validate-pins.py"
 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"
 
@@ -128,14 +130,19 @@ def __init__(self, args, *rest, **kwargs):
 
 @pytest.fixture(scope="module")
 def project(tmp_path_factory) -> dict:
-    """A project at A, its bare remotes, and an upstream moved to B."""
+    """A project at A, its bare remotes, and an upstream moved to B, then
+    to C, which changes `PINS` and nothing else."""
     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})
+    commit_c = commit_upstream_changes(
+        upstream, "Fix the pins validator",
+        {f"templates/assembly-root/{PINS}": UPSTREAM_FIX_LINE})
     return {"upstream": upstream, "clone": clone, "a": commit_a,
-            "b": commit_b, "remote": base / "remotes" / f"{PROJECT}.git"}
+            "b": commit_b, "c": commit_c,
+            "remote": base / "remotes" / f"{PROJECT}.git"}
 
 
 @pytest.fixture
@@ -831,3 +838,200 @@ def run(command, *args, **kwargs):
     assert calls[1] == ["gh", *listing()]
     assert next_line_of(out) == "NEXT  " + shlex.join(calls[0])
     assert commits_on_the_branch(root) == 1
+
+
+# --- a re-run over a tree HEAD does not commit (#333) -------------------------
+#
+# `_awaits_its_pull_request` judges HEAD; "nothing to write" judges the working
+# tree, and so does the manifest read for the pull request's base and
+# repository. Over a tree that is HEAD's those agree. Over an uncommitted
+# change to the pin, the manifest or a pinned copy they did not, and the re-run
+# pushed HEAD under a pull request named for the working tree. Each run is the
+# update as a child with the `gh` fixture's stub in it (#334), so a tool that
+# asked `gh` would be heard asking, on every platform.
+
+def landed_without_its_pull_request(root, project, env) -> None:
+    """B committed on BRANCH and pushed, with no pull request: what a `gh pr
+    create` failing after the push leaves, reached without `gh`."""
+    first = apply(root, project, "--branch", BRANCH, "--push", env=env)
+    assert first.returncode == 0, first.stdout + first.stderr
+    assert on_the_remote(root) == the_commit_stands(root, project)
+
+
+def apply_c_in_place(root, project, env) -> None:
+    """THE ISSUE'S CASE: then an in-place `apply --at C`, left uncommitted."""
+    landed_without_its_pull_request(root, project, env)
+    in_place = apply(root, project, env=env, at=project["c"])
+    assert in_place.returncode == 0, in_place.stdout + in_place.stderr
+
+
+def put_the_repin_back_by_hand(root, project, env) -> None:
+    """Then the branch's re-pin put back to `origin/main`'s, staged too."""
+    landed_without_its_pull_request(root, project, env)
+    git("checkout", "origin/main", "--", PIN, "project.yaml", CHANGED,
+        cwd=root.path)
+
+
+def edit_the_manifest(root, project, env) -> None:
+    """Then an edit to `project.yaml` alone, the pin untouched."""
+    landed_without_its_pull_request(root, project, env)
+    manifest = root.path / "project.yaml"
+    manifest.write_bytes(manifest.read_bytes() + b"# not committed\n")
+
+
+def commit_the_pin_without_its_copy(root, project, env) -> None:
+    """An in-place `apply --at B` committed by hand on BRANCH, the copy
+    left out: HEAD's pin names B over A's validator, and the working tree
+    holds B's."""
+    in_place = apply(root, project, env=env)
+    assert in_place.returncode == 0, in_place.stdout + in_place.stderr
+    git("checkout", "-q", "-b", BRANCH, cwd=root.path)
+    git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m",
+        "Re-pin by hand", "--", PIN, "project.yaml", cwd=root.path)
+
+
+def hidden_from_the_index(flag: str):
+    """THE ISSUE'S CASE, then every path it left uncommitted marked `flag`
+    in the index. `git status` and `git diff HEAD` then report nothing, so
+    a check that asked the index would have taken the re-run; the bytes on
+    disk are still not HEAD's."""
+    def prepare(root, project, env) -> None:
+        apply_c_in_place(root, project, env)
+        git("update-index", flag, "--", PIN, "project.yaml", PINS,
+            cwd=root.path)
+        assert git("status", "--porcelain", cwd=root.path).stdout == ""
+        assert git("diff", "--quiet", "HEAD", "--", PIN, "project.yaml", PINS,
+                   cwd=root.path, check=False).returncode == 0, (
+            f"{flag} hides the in-place apply from `git diff HEAD`")
+    return prepare
+
+
+@pytest.mark.parametrize("prepare, at, differ, two_pins", [
+    (apply_c_in_place, "c", [PIN, "project.yaml", PINS], True),
+    (hidden_from_the_index("--assume-unchanged"), "c",
+     [PIN, "project.yaml", PINS], True),
+    (hidden_from_the_index("--skip-worktree"), "c",
+     [PIN, "project.yaml", PINS], True),
+    (put_the_repin_back_by_hand, "a", [PIN, "project.yaml", CHANGED], True),
+    (edit_the_manifest, "b", ["project.yaml"], False),
+    (commit_the_pin_without_its_copy, "b", [CHANGED], False),
+], ids=["in-place-apply", "assume-unchanged", "skip-worktree",
+        "put-back-by-hand", "manifest-edit", "copy-uncommitted"])
+def test_a_rerun_over_a_tree_head_does_not_commit_is_refused_by_name(
+        root, project, gh, prepare, at, differ, two_pins):
+    """THE ISSUE (#333). `apply --branch --push --pr` on BRANCH finds nothing
+    to write in the working tree while HEAD proposes a pin, so main took the
+    re-run: it pushed HEAD and opened a pull request titled with the
+    working tree's commit (in-place-apply: C over B; put-back-by-hand: A
+    over B), or one whose base and repository came from an uncommitted
+    manifest, or one carrying a copy HEAD never committed. Now each is
+    refused by name, listing exactly the paths HEAD does not commit, before
+    `gh` is asked or anything is pushed, and the tree is left as it was.
+    The comparison is of bytes, so the in-place apply is refused the same
+    with its paths marked `--assume-unchanged` or `--skip-worktree`, which
+    hide it from every question put to the index."""
+    prepare(root, project, gh.env)
+    head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip()
+    remote = on_the_remote(root)
+    status = git("status", "--porcelain", cwd=root.path).stdout
+    gh.forget()
+
+    rerun = apply(root, project, "--branch", BRANCH, "--push", "--pr",
+                  env=gh.env, at=project[at])
+
+    assert rerun.returncode == 2, rerun.stdout + rerun.stderr
+    assert rerun.stderr.startswith(
+        f"REFUSED update-rerun-uncommitted: --pr would finish the landing of "
+        f"{BRANCH} from what HEAD ({head[:12]}) commits, under a pull request "
+        f"naming {project[at][:12]}, but the working tree does not hold what "
+        f"HEAD commits at {', '.join(differ)}. "), rerun.stderr
+    carried = (f"there the pin names {project[at][:12]} and HEAD's names "
+               f"{project['b'][:12]}, so the pull request would name one "
+               "commit and carry another.")
+    assert (carried in rerun.stderr) == two_pins, rerun.stderr
+    assert "Nothing was written or pushed, and gh was not asked." \
+        in rerun.stderr
+    assert rerun.stdout == "", "no `nothing to write`, no push, no NEXT line"
+    assert gh.calls() == []
+    assert on_the_remote(root) == remote, "nothing was pushed"
+    assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head
+    assert git("symbolic-ref", "--short", "HEAD",
+               cwd=root.path).stdout.strip() == BRANCH
+    assert git("status", "--porcelain", cwd=root.path).stdout == status, (
+        "nothing was written and nothing rolled back")
+
+
+def test_dirt_neither_decision_reads_leaves_the_rerun_as_it_was(
+        root, project, gh):
+    """WHAT THE REFUSAL DOES NOT TOUCH. An uncommitted README edit and an
+    untracked note are read by neither decision, so the re-run still asks
+    `gh`, finds HEAD already on origin and opens the pull request for B, as
+    #227's does, and leaves both where they were."""
+    landed_without_its_pull_request(root, project, gh.env)
+    head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip()
+    readme = root.path / "README.md"
+    readme.write_bytes(readme.read_bytes() + b"\nNot committed.\n")
+    (root.path / "notes.txt").write_bytes(b"untracked\n")
+    gh.forget()
+
+    rerun = apply(root, project, "--branch", BRANCH, "--push", "--pr",
+                  env=gh.env)
+
+    assert rerun.returncode == 0, rerun.stdout + rerun.stderr
+    assert f"pull request {URL}" in rerun.stdout
+    assert next_line_of(rerun.stdout) == PR_OPEN_NEXT
+    calls = gh.calls()
+    assert [call[:2] for call in calls] == [["pr", "list"],
+                                            ["pr", "create"]], calls
+    assert calls[0] == listing()
+    assert calls[1][calls[1].index("--title") + 1].endswith(
+        f"@ {project['b'][:12]}")
+    assert on_the_remote(root) == head
+    assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head
+    assert git("status", "--porcelain", cwd=root.path).stdout.splitlines() \
+        == [" M README.md", "?? notes.txt"]
+
+
+def test_a_crlf_pin_and_manifest_still_hold_what_head_commits(root, project,
+                                                              gh):
+    """THE CLEAN FILTER RUNS. The project's `.gitattributes` is `* text=auto
+    eol=lf`, so a pin and a manifest whose lines end CRLF and are otherwise
+    HEAD's are what git would commit there: HEAD's blobs. The re-run is not
+    refused for them and opens the pull request for B, as a clean re-run
+    does; a comparison of the raw bytes would have named both. A pinned
+    copy cannot show this here: CRLF makes it `locally-modified` before the
+    re-run is recognised."""
+    landed_without_its_pull_request(root, project, gh.env)
+    head = git("rev-parse", "HEAD", cwd=root.path).stdout.strip()
+    crlf = {}
+    for path in (PIN, "project.yaml"):
+        file = root.path / path
+        crlf[path] = file.read_bytes().replace(b"\r\n", b"\n").replace(
+            b"\n", b"\r\n")
+        file.write_bytes(crlf[path])
+        committed = git("rev-parse", f"HEAD:{path}",
+                        cwd=root.path).stdout.strip()
+        assert git("hash-object", "--no-filters", "--", path,
+                   cwd=root.path).stdout.strip() != committed, (
+            f"{path}'s raw bytes are not HEAD's blob")
+        assert git("hash-object", "--", path,
+                   cwd=root.path).stdout.strip() == committed, (
+            f"{path}, cleaned as git commits it, is HEAD's blob")
+    gh.forget()
+
+    rerun = apply(root, project, "--branch", BRANCH, "--push", "--pr",
+                  env=gh.env)
+
+    assert rerun.returncode == 0, rerun.stdout + rerun.stderr
+    assert f"pull request {URL}" in rerun.stdout
+    assert next_line_of(rerun.stdout) == PR_OPEN_NEXT
+    calls = gh.calls()
+    assert [call[:2] for call in calls] == [["pr", "list"],
+                                            ["pr", "create"]], calls
+    assert calls[1][calls[1].index("--title") + 1].endswith(
+        f"@ {project['b'][:12]}")
+    assert on_the_remote(root) == head
+    assert git("rev-parse", "HEAD", cwd=root.path).stdout.strip() == head
+    for path, data in crlf.items():
+        assert (root.path / path).read_bytes() == data, (
+            f"{path} was left as it was")

update-shape.py

@@ -2598,6 +2598,8 @@
         _note_untaken_additions(additions, taking)
         if _nothing_to_do(rows, pinned, target, taking):
             if _awaits_its_pull_request(root, args, kind):
+                _refuse_what_head_does_not_commit(root, rows, kind,
+                                                  args.branch, pinned, target)
                 return _finish_landing(root, args, upstream, target, kind)
             print(f"nothing to do: every copied file matches "
                   f"{upstream.repository} @ {target[:12]}, which the pin "
@@ -2618,7 +2620,7 @@
         _refuse_red_validators(root, kind)
         added = [add.path for add in taking]
         paths = sorted({row.path for row in copied} | set(added)
                        | {"contracts/shape-pin.yaml", kind.manifest}
                        | ({README} if readme.rewritten else set()))
         _land(root, args, paths, added, target, upstream, kind, len(copied),
               ledger)
@@ -2779,6 +2781,10 @@
     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/<tracking>` cannot tell, and asks `gh`, which answers for itself.
+
+    THIS READS HEAD, AND "NOTHING TO DO" READ THE WORKING TREE (#333). What
+    it recognises is finished only once `_refuse_what_head_does_not_commit`
+    has found the two to be the same bytes wherever either was read.
     """
     if not (args.pr and args.branch) or current_branch(root) != args.branch:
         return False
@@ -2793,6 +2799,88 @@
     return proposed is not None and proposed != landed
 
 
+def _refuse_what_head_does_not_commit(root: Path, rows: list[Row],
+                                      kind: Kind, branch: str, pinned: str,
+                                      target: str) -> None:
+    """The re-run `_awaits_its_pull_request` recognised finishes what HEAD
+    commits, so the working tree must hold HEAD's bytes wherever this run
+    read it, or the re-run is refused by name (#333).
+
+    TWO DECISIONS, ONE SOURCE. "Nothing to write" is judged from the
+    working tree — `read_pin` and `classify` — and the pull request's base
+    and repository are read from the manifest there too; whether a pull
+    request is owed is judged from HEAD, and HEAD is what the re-run pushes
+    and what its pull request carries. Over a tree that IS HEAD's that is
+    one answer. Over an in-place `apply --at C` left uncommitted on a
+    branch whose commit re-pins B it was two: the re-run pushed B under a
+    pull request titled C, and said the pin on the branch named C.
+
+    So the pin, the manifest and every pinned copy are compared with HEAD
+    BY BYTES (`_not_what_head_commits`). The index is not asked: nothing
+    here commits, and neither decision reads a staged entry. Nor is any
+    other path, a README or an untracked note, which neither decision
+    reads. The refusal comes before `gh` is asked or anything is pushed,
+    with nothing written, so there is nothing to roll back.
+    """
+    pin = "contracts/shape-pin.yaml"
+    differ = _not_what_head_commits(
+        root, list(dict.fromkeys([pin, kind.manifest,
+                                  *(row.path for row in rows)])))
+    if not differ:
+        return
+    head = (_git_answer(root, ["rev-parse", "HEAD"]) or "").strip()
+    committed = _committed_pin(root, pin)
+    carried = (f"; there the pin names {pinned[:12]} and HEAD's names "
+               f"{committed[:12]}, so the pull request would name one commit "
+               "and carry another" if committed and committed != pinned
+               else "")
+    raise Refusal(
+        "update-rerun-uncommitted",
+        f"--pr would finish the landing of {branch} from what HEAD "
+        f"({head[:12]}) commits, under a pull request naming {target[:12]}, "
+        "but the working tree does not hold what HEAD commits at "
+        + ", ".join(differ) + ". \"Nothing to write\" was judged from the "
+        "working tree, and the pull request's base and repository are read "
+        f"from it too{carried}.",
+        f"Remediation: a re-run finishes only what {branch} already "
+        "commits. Commit those changes on it if they are the re-pin to "
+        "propose, or put them back to HEAD's bytes if they are not, then "
+        "run the same command again. Nothing was written or pushed, and gh "
+        "was not asked.")
+
+
+def _not_what_head_commits(root: Path, paths: list[str]) -> list[str]:
+    """Those of `paths` whose bytes on disk are not the blob HEAD commits
+    at that path, or that HEAD does not commit at all, in the order given
+    (#333). A path git will not hash is one this cannot vouch for, and is
+    named with them.
+
+    `git hash-object` on the file, against `HEAD:<path>`: the clean filter
+    runs, so a CRLF checkout of an LF blob is HEAD's, and no index flag
+    hides an edit — `readme_is_committed`'s second question, asked of each
+    path.
+    """
+    differ = []
+    for path in paths:
+        disk = (_git_answer(root, ["hash-object", "--", path]) or "").strip()
+        head = (_git_answer(root, ["rev-parse", "--verify", "--quiet",
+                                   f"HEAD:{path}"]) or "").strip()
+        if not disk or disk != head:
+            differ.append(path)
+    return differ
+
+
+def _committed_pin(root: Path, pin: str) -> str:
+    """The `commit:` HEAD's shape pin names, or "" when that pin does not
+    read as a mapping (#333). Said in the refusal, never decided on."""
+    try:
+        parsed = parse_yaml(_git_answer(root, ["show", f"HEAD:{pin}"]) or "")
+    except YamlError:
+        return ""
+    return (str(parsed.get("commit") or "").lower()
+            if isinstance(parsed, dict) else "")
+
+
 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."""