diff --git a/scripts/profile_sync.py b/scripts/profile_sync.py index 998a4e6d..385ca2a4 100644 --- a/scripts/profile_sync.py +++ b/scripts/profile_sync.py @@ -9,6 +9,7 @@ recalable from what needs the code read again. from __future__ import annotations import argparse +import contextlib import difflib import json import os @@ -16,6 +17,7 @@ import posixpath import re import subprocess import sys +import tempfile from dataclasses import asdict, dataclass from datetime import date from pathlib import Path @@ -2175,6 +2177,10 @@ def bump_commit( if pending_recale(report, accept_changed, text): return False document = yaml.safe_load(text) + if document.get("source_commit") == report.head: + # Rewriting the pin to the value it already holds is not a bump, and + # announcing it buries the profiles that did move. + return False expected = dict(document) expected["source_commit"] = report.head if document.get("source_commit"): @@ -2525,15 +2531,24 @@ def _apply_writes(args, name: str, profile: dict, report: ProfileReport) -> None print(f"{name}: would write source_commit {report.pin[:7]}") elif backfill_commit(path, report.pin): print(f"{name}: source_commit {report.pin[:7]}") - if args.rebase_refs and not args.dry_run and not report.skipped: - for line in rebase_refs(path, report, args.accept_changed): - print(f"{name}: {line}") - if ( - args.bump_commit - and not args.dry_run - and bump_commit(path, report, args.accept_changed) - ): - print(f"{name}: source_commit -> {report.head[:7]}") + if not (args.rebase_refs or args.bump_commit): + return + with contextlib.ExitStack() as stack: + if args.dry_run: + # Plan on a copy through the production write path rather than a + # parallel branch: the bump then reads the text the rebase would + # have left, which is the only text under which it is reachable. + scratch = stack.enter_context(tempfile.TemporaryDirectory()) + target = Path(scratch) / path.name + target.write_bytes(path.read_bytes()) + recale, bumped = "would recale ", "would set source_commit ->" + else: + target, recale, bumped = path, "", "source_commit ->" + if args.rebase_refs and not report.skipped: + for line in rebase_refs(target, report, args.accept_changed): + print(f"{name}: {recale}{line}") + if args.bump_commit and bump_commit(target, report, args.accept_changed): + print(f"{name}: {bumped} {report.head[:7]}") def _cited_paths(report: ProfileReport) -> list[str]: diff --git a/tests/test_profile_sync.py b/tests/test_profile_sync.py index 1c0fcd1e..e69c6079 100644 --- a/tests/test_profile_sync.py +++ b/tests/test_profile_sync.py @@ -2,6 +2,7 @@ from __future__ import annotations +import argparse import contextlib import io import json @@ -1834,6 +1835,128 @@ class TestRebaseRefs(unittest.TestCase): self.assertIn('source_ref: "b.c:5"', text) +class TestWriteDryRun(unittest.TestCase): + """--dry-run has to plan a write, not stay silent about it. + + --backfill-commits and --realign-prose have always reported what they + would write. --rebase-refs and --bump-commit accepted the flag and said + nothing, so the only way to read the plan was to let it happen. + """ + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.dir = Path(self.tmp.name) + self.path = self.dir / "p.yml" + self.path.write_text(SAMPLE, encoding="utf-8") + + def tearDown(self): + self.tmp.cleanup() + + def _args(self, **over): + base = dict( + emulators_dir=str(self.dir), backfill_commits=False, + rebase_refs=False, bump_commit=False, accept_changed=False, + dry_run=True, + ) + base.update(over) + return argparse.Namespace(**base) + + def _run(self, args, report, sample=SAMPLE): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + profile_sync._apply_writes( + args, "p", yaml.safe_load(sample), report + ) + return buffer.getvalue() + + def _shifted(self): + part = PartResult( + RefPart("a.c", 10, 12, "a.c:10-12"), "SHIFTED", None, 30, 32, [] + ) + return ProfileReport( + name="p", repo="o/n", pin="pin", head="newhead", + entries=[EntryReport("a.bin", "a.c:10-12", "SHIFTED", [part])], + counts={"SHIFTED": 1}, + ) + + def test_rebase_states_the_move_and_leaves_the_file(self): + before = self.path.read_text() + output = self._run(self._args(rebase_refs=True), self._shifted()) + self.assertIn("would recale", output) + self.assertIn("a.c:10-12 -> a.c:30-32", output) + self.assertEqual(self.path.read_text(), before) + + def test_a_pin_already_at_head_is_not_announced(self): + """A write that changes nothing must not be reported as one. + + 126 of 232 announced bumps on the corpus rewrote the pin to the + value it already held, so the plan overstated the work by more than + double and no reader could tell which entries meant anything. + """ + part = PartResult( + RefPart("a.c", 10, 12, "a.c:10-12"), "ANCHORED", None, 10, 12, [] + ) + self.path.write_text( + insert_after_line(SAMPLE, "profiled_date", 'source_commit: "newhead"'), + encoding="utf-8", + ) + report = ProfileReport( + name="p", repo="o/n", pin="newhead", head="newhead", + entries=[EntryReport("a.bin", "a.c:10-12", "ANCHORED", [part])], + counts={"ANCHORED": 1}, + ) + self.assertFalse(bump_commit(self.path, report)) + self.assertEqual(self._run(self._args(bump_commit=True), report), "") + + def test_bump_states_the_pin_and_leaves_the_file(self): + before = self.path.read_text() + part = PartResult( + RefPart("a.c", 10, 12, "a.c:10-12"), "ANCHORED", None, 10, 12, [] + ) + report = ProfileReport( + name="p", repo="o/n", pin="pin", head="newhead", + entries=[EntryReport("a.bin", "a.c:10-12", "ANCHORED", [part])], + counts={"ANCHORED": 1}, + ) + output = self._run(self._args(bump_commit=True), report) + self.assertIn("would set source_commit -> newhead", output) + self.assertEqual(self.path.read_text(), before) + + def test_writes_still_happen_without_the_flag(self): + output = self._run( + self._args(rebase_refs=True, dry_run=False), self._shifted() + ) + self.assertNotIn("would", output) + self.assertIn('source_ref: "a.c:30-32"', self.path.read_text()) + + def test_a_planned_bump_reads_the_prose_the_rebase_would_have_left(self): + """The pin is blocked by prose until the rebase moves it. + + Planning the bump against the file on disk would report a refusal + that the real run, where the rebase lands first, never produces. + """ + self.path.write_text(PROSE_SAMPLE, encoding="utf-8") + before = self.path.read_text() + part = PartResult(RefPart("a.c", 10, 10, "10"), "SHIFTED", None, 14, 14, []) + entry = EntryReport( + "notes", "a.c:10", "SHIFTED", [part], "prose", "notes" + ) + report = ProfileReport( + name="p", repo="o/n", pin="pin", head="newhead", + entries=[entry], counts={"SHIFTED": 1}, + ) + self.assertEqual( + profile_sync.pending_recale(report, False, before), 1, + "the prose still describes the pinned revision", + ) + output = self._run( + self._args(rebase_refs=True, bump_commit=True), report, PROSE_SAMPLE + ) + self.assertIn("would recale", output) + self.assertIn("would set source_commit -> newhead", output) + self.assertEqual(self.path.read_text(), before) + + class TestBumpCommit(unittest.TestCase): def setUp(self): self.tmp = tempfile.TemporaryDirectory() diff --git a/wiki/tools.md b/wiki/tools.md index 8a938353..f6a59b5a 100644 --- a/wiki/tools.md +++ b/wiki/tools.md @@ -282,6 +282,21 @@ recaling is all or nothing per profile: a profile that still needs a re-read is left untouched and moves as a whole once resolved. Rewrites preserve the prose of annotated refs, replacing only the location. +Those two writes belong in one pass, because a profile whose refs moved +without its pin describes two revisions at once: + +```bash +python scripts/profile_sync.py --all --rebase-refs --bump-commit --dry-run +python scripts/profile_sync.py --all --rebase-refs --bump-commit +``` + +The first prints the plan and changes nothing: `would recale` per ref and +`would set source_commit` per profile. It reaches that plan by running the +real write path over a throwaway copy, so the planned bump reads the text +the recale would have left rather than the one on disk; a pin held back by +prose that the same pass would have moved is not reported as blocked. Drop +`--dry-run` and the same pass writes, recale before bump on each profile. + `--accept-changed` recales `CHANGED` refs too, for a profile whose diff has been read and judged benign. It applies to one profile at a time and is refused with `--all`.