diff --git a/scripts/local_review.py b/scripts/local_review.py index 38d2bfe9a..3dd660066 100755 --- a/scripts/local_review.py +++ b/scripts/local_review.py @@ -282,35 +282,68 @@ def ref_exists(ref: str, root: Path) -> bool: return True +def remote_tracking_ref(name: str, root: Path) -> str | None: + """The exact `refs/remotes/` ref, or None where `show-ref --verify` rejects that name. + + `rev-parse` runs its whole resolution list on a full name too, so `refs/remotes/` also + matches a local branch literally named that. `show-ref --verify` matches the name exactly. + Raises CannotRun where the ref it accepts does not resolve to a commit. + """ + ref = f"refs/remotes/{name}" + try: + git("show-ref", "--verify", "--quiet", ref, root=root) + except CannotRun: + return None + if not ref_exists(ref, root): + raise CannotRun( + f"{ref} does not resolve to a commit, so the review scope cannot be determined" + ) + return ref + + def target_ref(target: str, root: Path) -> str: """The ref a target name means, preferring the remote-tracking one. - A target that is already a remote-tracking ref as written, such as `upstream/main`, is used - as written before anything else is tried. Without that check first, the `origin/` - preference below is unconditional and can mis-scope this exact case: if a branch literally - named `upstream/main` also exists on `origin`, `origin/upstream/main` would resolve and win, - silently measuring the caller's explicitly named remote against `origin` instead. + A target that is already a remote-tracking ref as written, such as `upstream/main`, is tried + first, in its qualified form, before anything else. Without that check first, the + `origin/` preference below is unconditional and can mis-scope this exact case: if a + branch literally named `upstream/main` also exists on `origin`, `origin/upstream/main` would + resolve and win, silently measuring the caller's explicitly named remote against `origin` + instead. - Otherwise, `origin/` is tried next and used whenever it resolves, so an ordinary - fleet branch name works and so does one holding a slash. Treating any slash as "already a - full ref", which an earlier version did, silently measured a target such as `release/v1` - against the local branch of that name rather than the remote one, and a local branch that - has moved on then defines the review scope with no error at all. + Otherwise, `origin/` is tried next as a remote-tracking ref, so an ordinary fleet + branch name works and so does one holding a slash. Treating any slash as "already a full ref", + which an earlier version did, silently measured a target such as `release/v1` against the + local branch of that name rather than the remote one, and a local branch that has moved on + then defines the review scope with no error at all. A value that resolves only as written is used as written last, which is what lets a fork-based flow name another remote's branch even when it is not already a remote-tracking ref (for example a local-only branch checked out from that remote). + + A remote-tracking result is matched by its exact name and returned fully qualified, as + `refs/remotes/...`. Git resolves a short name such as `upstream/main` against `refs/heads/` + before `refs/remotes/`, so a local branch literally named that would otherwise define the + review scope in place of the remote-tracking ref this function chose. + + A target already written as `refs/remotes/...` is not also tried under `origin/`, where a + branch literally carrying that name would win. """ - if ref_exists(f"refs/remotes/{target}", root): - return target - remote = f"origin/{target}" - if ref_exists(remote, root): - return remote + prefix = "refs/remotes/" + if target.startswith(prefix): + names: tuple[str, ...] = (target.removeprefix(prefix),) + else: + names = (target, f"origin/{target}") + for name in names: + ref = remote_tracking_ref(name, root) + if ref is not None: + return ref if ref_exists(target, root): return target + tried = " or ".join(f"{prefix}{name}" for name in names) raise CannotRun( - f"neither {remote} nor {target} resolves in this checkout," - " so the review scope cannot be determined" + f"no remote-tracking ref of a commit exists at {tried}, and {target} does not resolve" + " in this checkout, so the review scope cannot be determined" ) @@ -1039,8 +1072,9 @@ def main(argv: list[str] | None = None) -> int: "--target", default=None, help=( - f"target branch (default {DEFAULT_TARGET}). Resolved as origin/ where that" - " exists, else as written, so another remote's branch can be named directly" + f"target branch (default {DEFAULT_TARGET}). Resolved as the remote-tracking ref" + " , then as origin/ unless already starts with" + " refs/remotes/, else as written, so another remote's branch can be named directly" ), ) diff --git a/tests/test_local_review.py b/tests/test_local_review.py index 9d1012a88..f8cdf56d6 100755 --- a/tests/test_local_review.py +++ b/tests/test_local_review.py @@ -228,13 +228,19 @@ def test_a_slashed_target_prefers_the_remote_over_a_local_branch(self) -> None: remote = run(self.tmp, "rev-parse", "origin/release/v1").strip() local = run(self.tmp, "rev-parse", "release/v1").strip() self.assertNotEqual(remote, local, "fixture does not distinguish the two") - self.assertEqual(local_review.target_ref("release/v1", self.tmp), "origin/release/v1") + self.assertEqual( + local_review.target_ref("release/v1", self.tmp), "refs/remotes/origin/release/v1" + ) self.assertEqual(local_review.merge_base("release/v1", self.tmp), remote) - def test_a_target_that_only_exists_on_another_remote_is_used_as_written(self) -> None: + def test_a_target_that_only_exists_on_another_remote_is_qualified_from_that_remote( + self, + ) -> None: """This is what lets a fork-based flow name an upstream branch directly.""" run(self.tmp, "update-ref", "refs/remotes/upstream/main", "HEAD") - self.assertEqual(local_review.target_ref("upstream/main", self.tmp), "upstream/main") + self.assertEqual( + local_review.target_ref("upstream/main", self.tmp), "refs/remotes/upstream/main" + ) def test_an_explicit_remote_tracking_target_wins_over_a_same_named_origin_branch(self) -> None: """An explicitly named remote-tracking ref must not lose to the origin/ preference. @@ -256,7 +262,81 @@ def test_an_explicit_remote_tracking_target_wins_over_a_same_named_origin_branch run(self.tmp, "rev-parse", "upstream/main").strip(), "fixture does not distinguish the two", ) - self.assertEqual(local_review.target_ref("upstream/main", self.tmp), "upstream/main") + self.assertEqual( + local_review.target_ref("upstream/main", self.tmp), "refs/remotes/upstream/main" + ) + + def test_a_same_named_local_branch_cannot_shadow_a_remote_tracking_target(self) -> None: + """Git resolves a short name against refs/heads/ first, so the ref must be qualified. + + The remote-tracking `upstream/main` sits at the base commit, and a local branch of the + same name sits at the task tip. Resolving the short name picks the local branch and + yields the task tip as the merge base, while the qualified ref yields the base commit. + """ + base = run(self.tmp, "rev-parse", "HEAD").strip() + run(self.tmp, "update-ref", "refs/remotes/upstream/main", base) + (self.tmp / "moved.txt").write_text("task moved on\n", encoding="utf-8") + run(self.tmp, "add", "moved.txt") + run(self.tmp, "commit", "-m", "task work") + run(self.tmp, "branch", "upstream/main", "HEAD") + tip = run(self.tmp, "rev-parse", "HEAD").strip() + self.assertNotEqual(base, tip, "fixture does not distinguish the two") + self.assertEqual(local_review.merge_base("upstream/main", self.tmp), base) + + def test_a_local_branch_named_origin_target_cannot_shadow_the_origin_ref(self) -> None: + """The `origin/` step is qualified too, since the fleet default takes it. + + `refs/remotes/origin/feat` sits at the base commit, and a local branch literally named + `origin/feat` sits at the task tip, which the short name would resolve to first. + """ + base = run(self.tmp, "rev-parse", "HEAD").strip() + run(self.tmp, "update-ref", "refs/remotes/origin/feat", base) + (self.tmp / "moved.txt").write_text("task moved on\n", encoding="utf-8") + run(self.tmp, "add", "moved.txt") + run(self.tmp, "commit", "-m", "task work") + run(self.tmp, "branch", "origin/feat", "HEAD") + tip = run(self.tmp, "rev-parse", "HEAD").strip() + self.assertNotEqual(base, tip, "fixture does not distinguish the two") + self.assertEqual(local_review.merge_base("feat", self.tmp), base) + + def test_a_qualified_target_is_not_also_tried_under_origin(self) -> None: + """An origin branch literally named `refs/remotes/origin/main` must not replace the ref. + + The requested `refs/remotes/origin/main` sits at the base commit, and the nested + `refs/remotes/origin/refs/remotes/origin/main` sits at the task tip. + """ + base = run(self.tmp, "rev-parse", "HEAD").strip() + run(self.tmp, "update-ref", "refs/remotes/origin/main", base) + (self.tmp / "moved.txt").write_text("task moved on\n", encoding="utf-8") + run(self.tmp, "add", "moved.txt") + run(self.tmp, "commit", "-m", "task work") + run(self.tmp, "update-ref", "refs/remotes/origin/refs/remotes/origin/main", "HEAD") + tip = run(self.tmp, "rev-parse", "HEAD").strip() + self.assertNotEqual(base, tip, "fixture does not distinguish the two") + self.assertEqual(local_review.merge_base("refs/remotes/origin/main", self.tmp), base) + + def test_a_remote_tracking_ref_naming_a_non_commit_refuses_rather_than_falls_through( + self, + ) -> None: + """Falling through would reach the as-written step, where a same-named branch wins.""" + tree = run(self.tmp, "write-tree").strip() + run(self.tmp, "update-ref", "refs/remotes/upstream/main", tree) + run(self.tmp, "branch", "upstream/main", "HEAD") + with self.assertRaises(local_review.CannotRun): + local_review.target_ref("upstream/main", self.tmp) + + def test_a_local_branch_named_like_a_qualified_remote_ref_is_not_remote_tracking( + self, + ) -> None: + """`rev-parse` also resolves a full name through `refs/heads/`, so the match is exact. + + A local branch literally named `refs/remotes/upstream/main`, with no remote-tracking ref + of that name, must leave the target unresolved rather than be taken as remote-tracking. + """ + run(self.tmp, "branch", "refs/remotes/upstream/main", "HEAD") + self.assertIsNone(local_review.remote_tracking_ref("upstream/main", self.tmp)) + with self.assertRaises(local_review.CannotRun): + local_review.target_ref("upstream/main", self.tmp) def test_a_target_resolving_nowhere_is_a_boundary(self) -> None: with self.assertRaises(local_review.CannotRun): @@ -1128,7 +1208,8 @@ def test_the_backend_is_given_the_merge_base_not_the_target_tip(self) -> None: argv = argv_log.read_text(encoding="utf-8").split("\n") self.assertIn("--agent", argv) self.assertIn(base, argv, f"the backend was not given the merge base: {argv}") - self.assertNotIn("origin/develop", argv, "the backend was given the target tip") + for tip in ("origin/develop", "refs/remotes/origin/develop"): + self.assertNotIn(tip, argv, "the backend was given the target tip") # The flag name matters as much as the value. # The CLI documents --base as taking a branch and --base-commit as taking a commit hash. # A sha handed to --base is the wrong call even though the sha itself is right.