diff --git a/scripts/profile_sync.py b/scripts/profile_sync.py index 8ce4d9d1..aa9703f6 100644 --- a/scripts/profile_sync.py +++ b/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) diff --git a/tests/test_profile_sync.py b/tests/test_profile_sync.py index aece31fb..7f615fbf 100644 --- a/tests/test_profile_sync.py +++ b/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",