diff --git a/README.md b/README.md index 994114d..407ac45 100644 --- a/README.md +++ b/README.md @@ -101,7 +101,7 @@ pr-split split feature-branch --base main --dry-run pr-split split feature-branch --base main --stack ``` -A group with a single parent has its branch cut from the parent group's branch, carries the parent's hunks for shared files, and targets the parent's branch, with or without `--stack`. Its PR shows only its own diff, compiles standalone, and GitHub retargets it automatically as the parent merges. A group with several parents is cut from the merge base, carries every ancestor's changes, and targets the base branch, so its diff also includes those ancestors' changes (its PR description lists them). Groups with no dependencies are cut from the merge base and target the base branch. `--stack` additionally registers the chains as native GitHub stacks. +A group with a single parent has its branch cut from the parent group's branch, carries the parent's hunks for shared files, and targets the parent's branch, with or without `--stack`. Its PR shows only its own diff and compiles standalone. When `pr-split merge` reaches such a child it retargets the PR at the base branch (`gh pr edit --base`) right before merging it, since GitHub only does that itself for native stacks or when the parent's head branch is deleted (which `gh pr merge --auto` skips). A group with several parents is cut from the merge base, carries every ancestor's changes, and targets the base branch, so its diff also includes those ancestors' changes (its PR description lists them). Groups with no dependencies are cut from the merge base and target the base branch. `--stack` additionally registers the chains as native GitHub stacks. A new file larger than `--max-loc` cannot fit in any one sub-PR. With `--stack` it is cut into pieces between top-level definitions: Python files at top-level statements, and other languages at unindented lines after a blank line, outside brackets, strings and comments. Each piece is appended by a PR stacked on the one holding the piece before it. Every layer holds a valid prefix of the file, and the top of the chain holds the whole file. Without `--stack` a new file is never cut. @@ -121,7 +121,7 @@ Shows a table with each sub-PR's ID, title, branch, PR number, live state (OPEN/ pr-split merge ``` -Walks the dependency DAG and merges each PR in topological order. Skips already-merged, closed, draft, review-required, or changes-requested PRs, and every PR whose dependency was not merged in this run (independent subtrees still proceed). Stops if a merge fails. Exits 1 whenever a merge failed, any PR was left blocked, or a PR's state could not be fetched from GitHub (check `gh auth status`), so re-run once the cause is resolved. +Walks the dependency DAG and merges each PR in topological order. Each child PR that targets its parent's branch is retargeted at the base branch right before it is merged (native-stack PRs are left to GitHub). Skips already-merged, closed, draft, review-required, or changes-requested PRs, and every PR whose dependency was not merged in this run (independent subtrees still proceed). Stops if a merge fails. Exits 1 whenever a merge failed, any PR was left blocked, or a PR's state could not be fetched from GitHub (check `gh auth status`), so re-run once the cause is resolved. Use `--auto` to queue merges behind CI checks (uses `gh pr merge --auto`): diff --git a/pr_split/cli.py b/pr_split/cli.py index 92ba8dd..20e75ba 100644 --- a/pr_split/cli.py +++ b/pr_split/cli.py @@ -77,6 +77,7 @@ get_pr_state, link_stack, merge_pr, + retarget_pr, set_pr_base, stack_numbers_for, unstack, @@ -2112,6 +2113,10 @@ def merge_all( plan = plan_file.plan git_state = plan_file.git_state pr_map = {r.group_id: r for r in git_state.prs} + # Stacked children were opened against their parent's split branch. + stacked_children = { + r.group_id for r in git_state.branches if r.base_branch != plan.base_branch + } if not pr_map: console.print("[yellow]No PRs found in plan. Nothing to merge.[/yellow]") @@ -2202,6 +2207,13 @@ def merge_all( continue try: + if group_id in stacked_children: + # Its parent has merged by now (iter_ready + the guard + # above), but the PR still targets the parent's branch. + # Merging there -- which `--auto` does silently, since gh + # skips deleting the head branch in auto mode -- would + # never reach the base branch. + retarget_pr(pr_record.pr_number, plan.base_branch) merge_pr(pr_record.pr_number, auto=auto) if not auto: merged.append(group_id) diff --git a/pr_split/git_ops/prs.py b/pr_split/git_ops/prs.py index f784173..f59d90d 100644 --- a/pr_split/git_ops/prs.py +++ b/pr_split/git_ops/prs.py @@ -148,6 +148,26 @@ def merge_pr(pr_number: int, *, auto: bool = False) -> None: logger.info(f"{'Queued' if auto else 'Merged'} PR #{pr_number}") +_STACKED_PR_EDIT_REFUSED = "part of a stack" + + +def retarget_pr(pr_number: int, base: str) -> bool: + """Point a stacked child PR at ``base`` once its parent has merged. + + Returns False when GitHub refuses because the PR is in a native stack; + GitHub retargets those itself when the parent PR merges. + """ + try: + set_pr_base(pr_number, base) + except GitOperationError as exc: + if _STACKED_PR_EDIT_REFUSED in str(exc).lower(): + logger.info(logs.PR_RETARGET_NATIVE_STACK.format(number=pr_number)) + return False + raise + logger.info(logs.PR_RETARGETED.format(number=pr_number, base=base)) + return True + + def close_pr(pr_number: int) -> None: _run_gh("pr", "close", str(pr_number)) logger.info(logs.PR_CLOSED.format(number=pr_number)) diff --git a/pr_split/logs.py b/pr_split/logs.py index a338046..1824fdb 100644 --- a/pr_split/logs.py +++ b/pr_split/logs.py @@ -162,3 +162,7 @@ " its sub-PR will not compile or import on its own" ) ADOPTED_REMOTE_BRANCH = "Created local branch {branch} from {remote_ref}" +PR_RETARGETED = "Retargeted PR #{number} at {base} before merging" +PR_RETARGET_NATIVE_STACK = ( + "PR #{number} is in a native stack; GitHub retargets it when its parent merges" +) diff --git a/tests/test_cli_new_features.py b/tests/test_cli_new_features.py index 2de312a..547c715 100644 --- a/tests/test_cli_new_features.py +++ b/tests/test_cli_new_features.py @@ -1023,6 +1023,100 @@ def test_closed_leaf_during_auto_wait_exits_nonzero( assert payload["merged"] == [] +class TestStackedMergeRetargetsBase: + def _stacked_plan(self) -> MagicMock: + groups = [ + _group("pr-1", "root", files=["a.py"]), + _group("pr-2", "child", depends_on=["pr-1"], files=["b.py"]), + ] + plan_file = _plan_with_prs(groups) + plan_file.plan.base_branch = "main" + plan_file.git_state.branches = [ + BranchRecord(group_id="pr-1", branch_name="pr-split/ns/pr-1", base_branch="main"), + BranchRecord( + group_id="pr-2", branch_name="pr-split/ns/pr-2", base_branch="pr-split/ns/pr-1" + ), + ] + return plan_file + + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_child_is_retargeted_at_the_plan_base_before_merging( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + ) -> None: + mock_load.return_value = self._stacked_plan() + mock_state.return_value = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + calls: list[str] = [] + mock_retarget.side_effect = lambda n, base: calls.append(f"retarget {n} -> {base}") + mock_merge.side_effect = lambda n, *, auto=False: calls.append(f"merge {n}") + + result = runner.invoke(app, ["merge"]) + + assert result.exit_code == 0, result.output + assert calls == ["merge 1", "retarget 2 -> main", "merge 2"] + + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_unstacked_prs_are_not_retargeted( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + ) -> None: + plan_file = self._stacked_plan() + plan_file.git_state.branches[1] = BranchRecord( + group_id="pr-2", branch_name="pr-split/ns/pr-2", base_branch="main" + ) + mock_load.return_value = plan_file + mock_state.return_value = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + + result = runner.invoke(app, ["merge"]) + + assert result.exit_code == 0, result.output + mock_retarget.assert_not_called() + assert mock_merge.call_count == 2 + + @patch("pr_split.cli.time.sleep") + @patch("pr_split.cli.retarget_pr") + @patch("pr_split.cli.merge_pr") + @patch("pr_split.cli.get_pr_state") + @patch("pr_split.cli.load_plan") + @patch("pr_split.cli.plan_exists", return_value=True) + def test_auto_mode_retargets_before_queueing( + self, + mock_exists: MagicMock, + mock_load: MagicMock, + mock_state: MagicMock, + mock_merge: MagicMock, + mock_retarget: MagicMock, + mock_sleep: MagicMock, + ) -> None: + mock_load.return_value = self._stacked_plan() + open_state = {"state": "OPEN", "isDraft": False, "reviewDecision": None} + mock_state.side_effect = [open_state, {"state": "MERGED"}, open_state, {"state": "MERGED"}] + calls: list[str] = [] + mock_retarget.side_effect = lambda n, base: calls.append(f"retarget {n}") + mock_merge.side_effect = lambda n, *, auto=False: calls.append(f"queue {n}") + + result = runner.invoke(app, ["merge", "--auto"]) + + assert result.exit_code == 0, result.output + assert calls == ["queue 1", "retarget 2", "queue 2"] + + class TestMergeRejectsMalformedSavedPlan: def _plan_file(self, groups: list[Group]) -> PlanFile: from pr_split.constants import Priority diff --git a/tests/test_git_prs.py b/tests/test_git_prs.py index 2043ef9..f682a13 100644 --- a/tests/test_git_prs.py +++ b/tests/test_git_prs.py @@ -160,3 +160,30 @@ def test_ready_by_default(self, mock_gh: MagicMock) -> None: mock_gh.return_value = "https://github.com/org/repo/pull/7" create_pr("head", "main", "Title", "Body") assert "--draft" not in mock_gh.call_args.args + + +class TestRetargetPr: + @patch("pr_split.git_ops.prs._run_gh", return_value="") + def test_edits_the_base(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + assert retarget_pr(7, "main") is True + mock_gh.assert_called_once_with("pr", "edit", "7", "--base", "main") + + @patch( + "pr_split.git_ops.prs._run_gh", + side_effect=GitOperationError( + "Cannot change the base branch because the pull request is part of a stack" + ), + ) + def test_native_stack_refusal_is_tolerated(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + assert retarget_pr(7, "main") is False + + @patch("pr_split.git_ops.prs._run_gh", side_effect=GitOperationError("rate limited")) + def test_other_failures_raise(self, mock_gh: MagicMock) -> None: + from pr_split.git_ops.prs import retarget_pr + + with pytest.raises(GitOperationError, match="rate limited"): + retarget_pr(7, "main")