feat: rewrite annotated refs keeping their prose

scripts/profile_sync.py

@@ -56,6 +56,7 @@ class RefPart:
     path: str
     start: int | None
     end: int | None
+    raw: str = ""
 
 
 @dataclass(frozen=True)
@@ -122,24 +123,43 @@ def split_source_ref(ref: str) -> list[RefPart]:
     across 50 profiles carry them.
     """
     parts: list[RefPart] = []
-    # Annotations come out first: they may contain the separators themselves,
-    # and splitting inside one would cut it into meaningless fragments.
-    cleaned = _ANNOTATION_RE.sub("", str(ref or ""))
-    for raw in _SPLIT_RE.split(cleaned):
-        chunk = _trim_prose(raw.strip())
+    for raw in split_outside_parentheses(str(ref or "")):
+        chunk = _trim_prose(_ANNOTATION_RE.sub("", raw).strip())
         if not chunk:
             continue
         if _BARE_RANGE_RE.match(chunk) and parts:
             start, _, end = chunk.partition("-")
             parts.append(
-                RefPart(parts[-1].path, int(start), int(end or start))
+                RefPart(parts[-1].path, int(start), int(end or start), raw)
             )
             continue
         path, start_line, end_line = parse_source_ref(chunk)
-        parts.append(RefPart(path, start_line, end_line))
+        parts.append(RefPart(path, start_line, end_line, raw))
     return parts
 
 
+def split_outside_parentheses(text: str) -> list[str]:
+    """Split on commas and semicolons, ignoring those inside an annotation.
+
+    Each chunk keeps its annotation, so a rewrite can put the prose back.
+    """
+    chunks: list[str] = []
+    current: list[str] = []
+    depth = 0
+    for char in text:
+        if char == "(":
+            depth += 1
+        elif char == ")":
+            depth = max(0, depth - 1)
+        if char in ",;" and depth == 0:
+            chunks.append("".join(current))
+            current = []
+        else:
+            current.append(char)
+    chunks.append("".join(current))
+    return [c for c in (chunk.strip() for chunk in chunks) if c]
+
+
 def _trim_prose(chunk: str) -> str:
     """Drop a trailing comment written without parentheses.
 
@@ -734,11 +754,22 @@ def _render_span(path: str, start: int | None, end: int | None) -> str:
 
 
 def _original_part(part: PartResult) -> str:
-    return _render_span(part.part.path, part.part.start, part.part.end)
+    """The part exactly as the profile writes it today."""
+    return part.part.raw or _render_span(
+        part.part.path, part.part.start, part.part.end
+    )
 
 
 def _rendered_part(part: PartResult) -> str:
-    return _render_span(part.new_path or part.part.path, part.start, part.end)
+    """The part rewritten onto its new location, annotation preserved."""
+    span = _render_span(part.new_path or part.part.path, part.start, part.end)
+    raw = part.part.raw
+    if not raw:
+        return span
+    located = _trim_prose(_ANNOTATION_RE.sub("", raw).strip())
+    if located and located in raw:
+        return raw.replace(located, span, 1)
+    return span
 
 
 def _part_line(part: PartResult) -> str:
@@ -1088,11 +1119,18 @@ def backfill_commit(path: Path, sha: str) -> bool:
 
 
 def _is_rewritable(ref: str) -> bool:
-    """A ref carrying prose cannot be regenerated from its parsed parts."""
-    return "(" not in ref and ";" not in ref
+    """Every ref whose parts can be put back with their prose intact."""
+    parts = split_source_ref(ref)
+    if not parts:
+        return False
+    return ", ".join(p.raw for p in parts) == ", ".join(
+        chunk for chunk in split_outside_parentheses(ref)
+    )
 
 
-def rebase_refs(path: Path, report: ProfileReport) -> list[str]:
+def rebase_refs(
+    path: Path, report: ProfileReport, accept_changed: bool = False
+) -> list[str]:
     """Recale the line ranges of parts whose content is unchanged.
 
     All or nothing per profile. `source_commit` names the revision the refs
@@ -1101,7 +1139,9 @@ def rebase_refs(path: Path, report: ProfileReport) -> list[str]:
     and the next comparison would read the pinned revision at line numbers
     that only make sense at HEAD.
     """
-    if report.needs_review():
+    statuses = REBASE_STATUSES + (("CHANGED",) if accept_changed else ())
+    blocking = [s for s in REVIEW_STATUSES if s not in statuses]
+    if any((report.counts or {}).get(s) for s in blocking):
         return []
     text = path.read_text(encoding="utf-8")
     document = yaml.safe_load(text)
@@ -1121,7 +1161,7 @@ def rebase_refs(path: Path, report: ProfileReport) -> list[str]:
         rendered = []
         touched = False
         for part in entry.parts:
-            if part.status in REBASE_STATUSES and part.start is not None:
+            if part.status in statuses and part.start is not None:
                 recaled = _rendered_part(part)
                 if recaled != _original_part(part):
                     touched = True
@@ -1146,9 +1186,40 @@ def rebase_refs(path: Path, report: ProfileReport) -> list[str]:
     return applied
 
 
-def bump_commit(path: Path, report: ProfileReport) -> bool:
+def pending_recale(report: ProfileReport, accept_changed: bool = False) -> int:
+    """Parts that ought to move but sit in a ref the writer will not touch.
+
+    An annotated ref cannot be regenerated without losing its prose, so its
+    parts stay on the pinned line numbers. Advancing the pin while they do
+    would leave the profile describing two revisions at once.
+    """
+    movable = REBASE_STATUSES + (("CHANGED",) if accept_changed else ())
+    pending = 0
+    for entry in report.entries or []:
+        if _is_rewritable(entry.source_ref) and not entry.name.endswith("]"):
+            continue
+        pending += sum(
+            1
+            for part in entry.parts
+            if part.status in movable
+            and part.start is not None
+            and _rendered_part(part) != _original_part(part)
+        )
+    return pending
+
+
+def bump_commit(
+    path: Path, report: ProfileReport, accept_changed: bool = False
+) -> bool:
     """Advance source_commit to HEAD when nothing needs a read again."""
-    if report.skipped or report.needs_review() or not report.head:
+    if report.skipped or not report.head:
+        return False
+    blocking = REVIEW_STATUSES if not accept_changed else (
+        s for s in REVIEW_STATUSES if s != "CHANGED"
+    )
+    if any((report.counts or {}).get(s) for s in blocking):
+        return False
+    if pending_recale(report, accept_changed):
         return False
     text = path.read_text(encoding="utf-8")
     document = yaml.safe_load(text)
@@ -1205,6 +1276,11 @@ def build_parser() -> argparse.ArgumentParser:
     parser.add_argument("--backfill-commits", action="store_true")
     parser.add_argument("--rebase-refs", action="store_true")
     parser.add_argument("--bump-commit", action="store_true")
+    parser.add_argument(
+        "--accept-changed",
+        action="store_true",
+        help="recale CHANGED refs too, after their diff has been read",
+    )
     parser.add_argument("--dry-run", action="store_true")
     parser.add_argument("--force", action="store_true")
     parser.add_argument("--offline", action="store_true")
@@ -1261,9 +1337,13 @@ def _apply_writes(args, name: str, profile: dict, report: ProfileReport) -> None
         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):
+        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):
+    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]}")
 
 
@@ -1396,6 +1476,13 @@ def _print_triage(args, selected: dict, reports: list[ProfileReport]) -> None:
 def main() -> None:
     """Build one report per selected profile, then apply writes and output."""
     args = build_parser().parse_args()
+    if args.accept_changed and not args.emulator:
+        print(
+            "--accept-changed applies to one profile at a time: its diff has "
+            "to be read before its CHANGED refs can be recaled.",
+            file=sys.stderr,
+        )
+        raise SystemExit(1)
     profiles = load_emulator_profiles(args.emulators_dir, skip_aliases=False)
     selected = select_profiles(profiles, args)
     _check_quota(len(selected), args.offline)

tests/test_profile_sync.py

@@ -1222,12 +1222,38 @@ class TestRebaseRefs(unittest.TestCase):
         self.assertEqual(rebase_refs(self.path, report), [])
         self.assertIn('source_ref: "a.c:10-12"', self.path.read_text())
 
-    def test_annotated_ref_is_never_rewritten(self):
+    def test_accept_changed_recales_a_reviewed_diff(self):
+        part = PartResult(RefPart("a.c", 10, 12), "CHANGED", None, 30, 32, [])
+        report = self._report(
+            [EntryReport("a.bin", "a.c:10-12", "CHANGED", [part])]
+        )
+        report.counts = {"CHANGED": 1}
+        self.assertEqual(rebase_refs(self.path, report), [])
+        applied = rebase_refs(self.path, report, accept_changed=True)
+        self.assertEqual(applied, ["a.c:10-12 -> a.c:30-32"])
+
+    def test_accept_changed_does_not_cover_gone(self):
+        part = PartResult(RefPart("a.c", 10, 12), "CHANGED", None, 30, 32, [])
+        report = self._report(
+            [EntryReport("a.bin", "a.c:10-12", "CHANGED", [part])]
+        )
+        report.counts = {"CHANGED": 1, "GONE": 1}
+        self.assertEqual(rebase_refs(self.path, report, accept_changed=True), [])
+
+    def test_accept_changed_is_refused_on_every_profile(self):
+        args = profile_sync.build_parser().parse_args(["--all", "--accept-changed"])
+        self.assertTrue(args.accept_changed)
+        self.assertIsNone(args.emulator)
+
+    def test_annotated_ref_keeps_its_prose_when_rewritten(self):
         self.path.write_text(
             SAMPLE.replace('"a.c:10-12"', '"a.c:10-12 (loads the kernel)"'),
             encoding="utf-8",
         )
-        part = PartResult(RefPart("a.c", 10, 12), "SHIFTED", None, 20, 22, [])
+        part = PartResult(
+            RefPart("a.c", 10, 12, "a.c:10-12 (loads the kernel)"),
+            "SHIFTED", None, 20, 22, [],
+        )
         applied = rebase_refs(
             self.path,
             self._report(
@@ -1238,8 +1264,13 @@ class TestRebaseRefs(unittest.TestCase):
                 ]
             ),
         )
-        self.assertEqual(applied, [])
-        self.assertIn("(loads the kernel)", self.path.read_text())
+        self.assertEqual(len(applied), 1)
+        text = self.path.read_text()
+        self.assertIn('source_ref: "a.c:20-22 (loads the kernel)"', text)
+
+    def test_a_chunk_that_is_only_prose_blocks_the_rewrite(self):
+        self.assertFalse(profile_sync._is_rewritable("a.c:1, (just a note)"))
+        self.assertTrue(profile_sync._is_rewritable("a.c:1 (note), b.c:2"))
 
     def test_mode_keyed_ref_is_never_rewritten(self):
         part = PartResult(RefPart("a.c", 10, 12), "SHIFTED", None, 20, 22, [])
@@ -1277,6 +1308,34 @@ class TestBumpCommit(unittest.TestCase):
     def tearDown(self):
         self.tmp.cleanup()
 
+    def test_refused_when_a_ref_could_not_move(self):
+        part = PartResult(
+            RefPart("a.c", 10, 12, "a.c:10-12"), "SHIFTED", None, 30, 32, []
+        )
+        report = ProfileReport(
+            name="p", repo="o/n", pin="pin", head="newhead",
+            entries=[
+                EntryReport("a.bin", "a.c:10-12, (just a note)", "SHIFTED", [part])
+            ],
+            counts={"SHIFTED": 1},
+        )
+        self.assertEqual(profile_sync.pending_recale(report), 1)
+        self.assertFalse(bump_commit(self.path, report))
+
+    def test_accepted_when_nothing_had_to_move(self):
+        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, (just a note)", "ANCHORED", [part])
+            ],
+            counts={"ANCHORED": 1},
+        )
+        self.assertEqual(profile_sync.pending_recale(report), 0)
+        self.assertTrue(bump_commit(self.path, report))
+
     def test_refused_while_a_changed_remains(self):
         report = ProfileReport(
             name="p", repo="o/n", pin="pin", head="newhead",