diff --git a/scripts/family.py b/scripts/family.py index 74c63469..f0862275 100755 --- a/scripts/family.py +++ b/scripts/family.py @@ -53,7 +53,10 @@ subcommand runs that file rather than a second copy of its logic. EXIT CODES: 0 done · 1 nothing (a dry run prints and exits 0) · 2 a refusal. -`siblings` exits with the template script's own code, unchanged. +`siblings` exits with the template script's own code, unchanged. A `bump` whose +commit would record nothing — the member is already pinned at `--to`, in the +gitlink and in `family.yaml` alike — is `nothing`: it says `nothing to do`, as +`bump-leg.py` does for a leg, and writes no commit. """ from __future__ import annotations @@ -875,6 +878,33 @@ def cmd_add(args) -> int: # --------------------------------------------------------------------------- +def _nothing_to_record(root: Path, paths: list[str]) -> bool: + """Whether `git commit -- ` has nothing to record: the gitlink the + member stands at and the manifest as rewritten are what `HEAD` already has. + + ASKED OF GIT, AFTER THE SAME REWRITE THE COMMIT WOULD CARRY (#210), and + not read off the pin alone. A row that names `--to` is not by itself a bump + with nothing in it: `validate-family.py` names `bump --to ` as the + repair for a gitlink that disagrees with the pin and for a digest that does + not recompute, and both leave the row's commit equal to `--to`; and so does + a bump an earlier run wrote and staged but never committed. In each of + them `git diff` finds something, and the commit goes ahead. + `--ignore-submodules=dirty` is what `git commit` itself ignores: files the + member holds untracked or modified are not a thing this commit records, + its HEAD is. The caller has staged the gitlink first, as `_commit_one` + would, so a stray one in the index is not a difference. Exit 1 is git's + "there is a difference"; any other failure is a real one and propagates. + """ + try: + run(["git", "diff", "--quiet", "--ignore-submodules=dirty", "HEAD", + "--", *paths], cwd=root) + except CommandFailed as exc: + if exc.code == 1: + return False + raise + return True + + def cmd_bump(args) -> int: family = Family.open(args.family_root) project = checked_value("--member", repo_basename(args.member)) @@ -921,7 +951,30 @@ def cmd_bump(args) -> int: rows = [(_member_row(project, project_id, str(row["repository"]), commit, digest) if str(r.get("project")) == project else r) for r in family.members] + original = family.path.read_bytes() family.write_members(rows) + # THE GITLINK IS RE-STAGED BEFORE GIT IS ASKED, as `_commit_one` does it + # first anyway. An index still holding a stray gitlink somebody staged and + # never committed makes `git diff HEAD` find a difference the commit then + # does not find, and a bump to the pinned commit ended in the refusal #210 + # is about. `validate-family.py` reads the index first, reports + # `member-gitlink-mismatch` and names this very command as the repair. The + # checkout above already moved the member to `--to`, so this stages that + # and nothing else: a submodule path stages its gitlink, never the + # member's files, and the manifest's own index state is left as found. + run(["git", "add", "--", path], cwd=family.root) + if _nothing_to_record(family.root, [path, MANIFEST]): + # The same sentence `bump-leg.py` answers the same re-run with, and + # exit 1, which is what this file's docstring says `nothing` is. The + # manifest goes back byte for byte as it was found: the rewrite + # recorded nothing, and an uncommitted hand edit it happened to + # overwrite is not this run's to discard. + family.path.write_bytes(original) + print(f" {project:<16} {was[:12]} -> {commit[:12]} tree " + f"{digest[:12]}…") + print(f"\nnothing to do: {row['repository']} is already pinned at " + f"{commit[:12]} and every fact agrees.") + return 1 head = _commit_one( family.root, diff --git a/tests/test_family.py b/tests/test_family.py index 421f5105..05bea68d 100644 --- a/tests/test_family.py +++ b/tests/test_family.py @@ -636,6 +636,8 @@ def test_bump_moves_the_gitlink_and_the_pin_together(family, tmp_path): result = run_script(FAMILY, "bump", "--family-root", str(holder), "--member", "IRRS", "--to", moved) assert result.returncode == 0, result.stderr + result.stdout + assert "nothing to do" not in result.stdout, ( + "a bump that moves the pin is not the one #210 made say so") row = {r["project"]: r for r in manifest(holder)["members"]}["IRRS"] assert row["pin"]["commit"] == moved != was["pin"]["commit"] @@ -665,6 +667,229 @@ def test_bump_refuses_a_tag(holder): assert "member-to-not-a-commit" in result.stderr +# --- bump to the commit already pinned (#210) ------------------------------- + +def nothing_to_do_line(project: str, commit: str) -> str: + """The sentence `bump-leg.py` answers the same re-run with, repository + first, so a lane or script reads one wording from either tool.""" + return (f"nothing to do: {ORG}/{project} is already pinned at " + f"{commit[:12]} and every fact agrees.") + + +def commit_in_member(member, message: str = "The member moved again") -> str: + """An EMPTY commit on top of the member's HEAD: a new commit whose tree is + the old one's, so `pin.tree_sha256` still recomputes and the gitlink is the + only fact that moves. Returns its sha.""" + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", + "--allow-empty", "-m", message, cwd=member) + return git("rev-parse", "HEAD", cwd=member).stdout.strip() + + +@pytest.mark.parametrize("extra", [ + (), ("--trailer", TRAILERS[0], "--trailer", TRAILERS[1])], + ids=["plain", "with-trailers"]) +def test_bump_to_the_commit_already_pinned_is_nothing_to_do(holder, extra): + """A re-run of a bump that already landed. It used to check the member out + at the commit, rewrite the same row and hand `git commit` nothing, which + exited 1 and reached `main` as "REFUSED a git or gh command failed", exit + 2 — a refusal, where `family.py` documents exit 1 for nothing and + `bump-leg.py` answers the same re-run with `nothing to do` (#210). + + NOTHING IS WRITTEN: no commit, no staged path, not one byte of + `family.yaml`. A lane re-runs a bump with its `Lane:` and `Co-Authored-By:` + trailers, so the run with them is the one that has to read the same. + """ + pinned = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + row = {r["project"]: r for r in manifest(holder)["members"]}["IRRS"] + assert row["pin"]["commit"] == pinned, "fixture drift: gitlink != pin" + head = git("rev-parse", "HEAD", cwd=holder).stdout.strip() + status = git("status", "--porcelain", cwd=holder).stdout + before = (holder / "family.yaml").read_bytes() + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", pinned, *extra) + + assert result.returncode == 1, result.stderr + result.stdout + assert nothing_to_do_line("IRRS", pinned) in result.stdout + assert result.stderr == "", ( + "nothing to do is not a refusal, and says nothing on stderr") + assert "REFUSED" not in result.stdout + result.stderr + assert "committed" not in result.stdout + assert git("rev-parse", "HEAD", cwd=holder).stdout.strip() == head, ( + "no commit, not even an empty one") + assert git("status", "--porcelain", cwd=holder).stdout == status == "" + assert (holder / "family.yaml").read_bytes() == before + assert validate(holder).returncode == 0 + + +def test_bump_to_the_commit_already_pinned_ignores_what_the_member_holds( + holder): + """The member's own untracked and modified files are not something the + holder's commit can record: `git commit` finds nothing to commit with them + there, so a bump that asked `git diff` without `--ignore-submodules=dirty` + would have gone on to commit and been refused again (#210).""" + member = holder / "members" / "IRRS" + pinned = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + (member / "UNTRACKED.md").write_text("left lying in the mount\n") + (member / "NOTES.md").write_text("edited in place\n") + head = git("rev-parse", "HEAD", cwd=holder).stdout.strip() + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", pinned) + + assert result.returncode == 1, result.stderr + result.stdout + assert nothing_to_do_line("IRRS", pinned) in result.stdout + assert git("rev-parse", "HEAD", cwd=holder).stdout.strip() == head + assert (member / "UNTRACKED.md").is_file(), "the member's files stay" + assert (member / "NOTES.md").read_text() == "edited in place\n" + + +def test_bump_to_the_commit_already_pinned_leaves_an_uncommitted_edit_alone( + holder): + """The rewrite equals `HEAD`, so there is nothing to record, and what the + working copy held before is put back: an uncommitted edit to the `members:` + block is not this run's to discard, however wrong it is. `git status` says + the same before and after.""" + pinned = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + manifest_path = holder / "family.yaml" + manifest_path.write_text(manifest_path.read_text().replace( + " id: irrs\n", " id: something-else\n")) + edited = manifest_path.read_bytes() + status = git("status", "--porcelain", cwd=holder).stdout + assert status.strip() == "M family.yaml", status + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", pinned) + + assert result.returncode == 1, result.stderr + result.stdout + assert nothing_to_do_line("IRRS", pinned) in result.stdout + assert manifest_path.read_bytes() == edited + assert git("status", "--porcelain", cwd=holder).stdout == status + + +def test_bump_to_the_pinned_commit_unstages_a_stray_gitlink_and_says_nothing( + holder): + """THE VALIDATOR'S OWN REMEDY, followed with the pinned commit. Somebody + moved the member and ran `git add members/IRRS` without committing: + `validate-family.py` reads the INDEX first, reports + `member-gitlink-mismatch` and names `family.py bump --member --to + ` as the repair. `bump` checks the member out at the pinned commit, + which is HEAD's gitlink, but the index still differs from HEAD, so + `git diff HEAD` found something and `git commit` then did not: the refusal + #210 is about, exit 2, reached through the remedy. The gitlink is + re-staged before git is asked, the index agrees with HEAD again, and there + is nothing to record (#210).""" + member = holder / "members" / "IRRS" + pinned = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + stray = commit_in_member(member, "A stray move, staged and never committed") + git("add", "--", "members/IRRS", cwd=holder) + assert git("ls-files", "-s", "--", "members/IRRS", + cwd=holder).stdout.split()[1] == stray, "fixture drift" + broken = validate(holder) + assert broken.returncode == 1 + assert "member-gitlink-mismatch" in broken.stderr + head = git("rev-parse", "HEAD", cwd=holder).stdout.strip() + before = (holder / "family.yaml").read_bytes() + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", pinned) + + assert result.returncode == 1, result.stderr + result.stdout + assert nothing_to_do_line("IRRS", pinned) in result.stdout + assert "REFUSED" not in result.stdout + result.stderr + assert git("rev-parse", "HEAD", cwd=holder).stdout.strip() == head + assert git("ls-files", "-s", "--", "members/IRRS", + cwd=holder).stdout.split()[1] == pinned + assert git("status", "--porcelain", cwd=holder).stdout == "" + assert (holder / "family.yaml").read_bytes() == before + assert validate(holder).returncode == 0 + + +def test_bump_finishes_a_bump_an_earlier_run_staged_and_did_not_commit(holder): + """THE MANIFEST ALREADY NAMES `--to`, and there is still something to + commit: a run that wrote the row and staged both paths and then died + before `git commit` — a hook that refused, a lock — leaves exactly this. + Reading `was == --to` as "nothing to do" would call the half-finished bump + done and leave the commit for somebody to make by hand (#210).""" + member = holder / "members" / "IRRS" + before = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + after = commit_in_member(member) + manifest_path = holder / "family.yaml" + text = manifest_path.read_text() + assert text.count(before) == 1, "fixture drift: IRRS's pin is not unique" + manifest_path.write_text(text.replace(before, after, 1)) + git("add", "--", "members/IRRS", "family.yaml", cwd=holder) + head = git("rev-parse", "HEAD", cwd=holder).stdout.strip() + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", after) + + assert result.returncode == 0, result.stderr + result.stdout + assert "nothing to do" not in result.stdout + assert git("rev-parse", "HEAD", cwd=holder).stdout.strip() != head + assert git("rev-parse", "HEAD:members/IRRS", + cwd=holder).stdout.strip() == after + assert set(git("show", "--name-only", "--format=", "HEAD", + cwd=holder).stdout.split()) == {"members/IRRS", + "family.yaml"} + assert validate(holder).returncode == 0 + + +def test_bump_to_the_pinned_commit_repairs_a_gitlink_that_moved_alone(holder): + """`validate-family.py`'s own remedy for `member-gitlink-mismatch` is + `family.py bump --member --to `, and when the commit it + is pointed at is the one the row already names the manifest agrees with + `--to` and the gitlink does not. That is a bump with something to commit + (#210).""" + member = holder / "members" / "IRRS" + pinned = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + commit_in_member(member) + git("add", "--", "members/IRRS", cwd=holder) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Move the gitlink and not the pin", cwd=holder) + broken = validate(holder) + assert broken.returncode == 1 + assert "member-gitlink-mismatch" in broken.stderr + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", pinned) + + assert result.returncode == 0, result.stderr + result.stdout + assert "nothing to do" not in result.stdout + assert git("rev-parse", "HEAD:members/IRRS", + cwd=holder).stdout.strip() == pinned + assert validate(holder).returncode == 0, "and the lockstep rule holds" + + +def test_bump_to_the_pinned_commit_repairs_a_digest_that_does_not_recompute( + holder): + """The other half of the same remedy: a row whose `tree_sha256` is wrong + names the commit `--to` names, and the bump rewrites the digest from the + member's own objects. Comparing the commit and not the digest would have + called it done.""" + member = holder / "members" / "IRRS" + pinned = git("rev-parse", "HEAD:members/IRRS", cwd=holder).stdout.strip() + row = {r["project"]: r for r in manifest(holder)["members"]}["IRRS"] + manifest_path = holder / "family.yaml" + manifest_path.write_text(manifest_path.read_text().replace( + row["pin"]["tree_sha256"], "b" * 64)) + git("add", "--", "family.yaml", cwd=holder) + git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-q", "-m", + "Hand-edit a digest", cwd=holder) + broken = validate(holder) + assert broken.returncode == 1 + assert "member-digest-mismatch" in broken.stderr + + result = run_script(FAMILY, "bump", "--family-root", str(holder), + "--member", "IRRS", "--to", pinned) + + assert result.returncode == 0, result.stderr + result.stdout + assert "nothing to do" not in result.stdout + repaired = {r["project"]: r for r in manifest(holder)["members"]}["IRRS"] + assert repaired["pin"]["tree_sha256"] == tree_digest(member, pinned) + assert validate(holder).returncode == 0 + + # --- --trailer: the lines a bump's commit ends with (#111) ------------------ def test_bump_with_two_trailers_ends_the_commit_message_in_the_order_given(