From 3f18f1611ea388ae4e6ad5a90c425ef0b703b5ba Mon Sep 17 00:00:00 2001 From: Pawel Lipski Date: Wed, 8 Jul 2026 18:35:17 +0200 Subject: [PATCH] Infer base repository from parent branch in retarget/restack In a fork workflow the PR/MR is hosted by the base (upstream) repository, not the head (fork) repository that holds the branches. retarget-pr/restack-pr (and their GitLab retarget-mr/restack-mr counterparts) now resolve the code hosting client against the base repository inferred from the parent branch's tracking remote - exactly like create-pr already does - so they query the repository that actually hosts the PR even when no machete.{github,gitlab}.base* config keys are set. Resolution is best-effort: if the base repository cannot be determined unambiguously and no base* keys are set, it falls back to the head repository, preserving the previous behavior. --- git_machete/client/with_code_hosting.py | 49 +++++++++++++++++++++---- tests/mockers_github.py | 23 ++++++++++-- tests/mockers_gitlab.py | 25 ++++++++++++- tests/test_github_restack_pr.py | 46 ++++++++++++++++++++++- tests/test_github_retarget_pr.py | 43 ++++++++++++++++++++++ tests/test_gitlab_retarget_mr.py | 43 ++++++++++++++++++++++ 6 files changed, 215 insertions(+), 14 deletions(-) diff --git a/git_machete/client/with_code_hosting.py b/git_machete/client/with_code_hosting.py index 1364d4ad9..4e8c50540 100644 --- a/git_machete/client/with_code_hosting.py +++ b/git_machete/client/with_code_hosting.py @@ -433,7 +433,8 @@ def create_pull_request( def restack_pull_request(self, *, opt_update_related_descriptions: bool) -> None: spec = self.code_hosting_spec head = self._git.get_current_branch() - _, org_repo_remote = self._init_code_hosting_client(branch_used_for_tracking_data=head) + _, org_repo_remote = self._init_code_hosting_client( + branch_used_for_tracking_data=head, base_branch_used_for_tracking_data=self.parent_of(head)) pr: Optional[PullRequest] = self.__get_sole_pull_request_for_head(head, ignore_if_missing=False) assert pr is not None @@ -538,7 +539,8 @@ def retarget_pull_request(self, *, opt_branch: Optional[LocalBranchShortName], head: ManagedBranchName = self.expect_in_managed_branches(opt_branch or self._git.get_current_branch()) spec = self.code_hosting_spec if self.__code_hosting_client is None: - self._init_code_hosting_client(branch_used_for_tracking_data=head) + self._init_code_hosting_client( + branch_used_for_tracking_data=head, base_branch_used_for_tracking_data=self.parent_of(head)) pr: Optional[PullRequest] = self.__get_sole_pull_request_for_head( head, ignore_if_missing=opt_ignore_if_missing) @@ -599,16 +601,23 @@ def __derive_org_repo_and_remote( domain: str, branch_used_for_tracking_data: Optional[LocalBranchShortName] = None, *, - is_base: bool = False + is_base: bool = False, + fall_back_org_repo_to_head: bool = False ) -> OrganizationAndRepositoryAndRemote: spec = self.code_hosting_spec keys = spec.git_config_keys if is_base: remote_key, org_key, repo_key = keys.base_remote, keys.base_organization, keys.base_repository - # The base remote falls back to the (non-base) remote; the base organization/repository have no such fallback. + # The base remote falls back to the (non-base) remote; the base organization/repository have no such fallback + # by default (`create_pull_request` relies on that to detect a fork base). PR-reading/-modifying commands, however, + # opt into the fallback via `fall_back_org_repo_to_head`, so that a setup which pins only the head + # `organization`/`repository` (and no base* keys) keeps working when the client is resolved against the base. remote_from_config = self._config.code_hosting_base_remote(keys) or self._config.code_hosting_remote(keys) org_from_config = self._config.code_hosting_base_organization(keys) repo_from_config = self._config.code_hosting_base_repository(keys) + if fall_back_org_repo_to_head: + org_from_config = org_from_config or self._config.code_hosting_organization(keys) + repo_from_config = repo_from_config or self._config.code_hosting_repository(keys) else: remote_key, org_key, repo_key = keys.remote, keys.organization, keys.repository remote_from_config = self._config.code_hosting_remote(keys) @@ -691,16 +700,40 @@ def __derive_org_repo_and_remote( f'{spec.git_config_keys.for_locating_repo_message()}\n') def _init_code_hosting_client(self, - branch_used_for_tracking_data: Optional[LocalBranchShortName] = None + branch_used_for_tracking_data: Optional[LocalBranchShortName] = None, + base_branch_used_for_tracking_data: Optional[LocalBranchShortName] = None ) -> Tuple[str, OrganizationAndRepositoryAndRemote]: if self.__code_hosting_client is not None: raise UnexpectedMacheteException("Code hosting client has already been initialized.") domain = self.__derive_code_hosting_domain() - org_repo_remote = self.__derive_org_repo_and_remote( + # The returned org/repo/remote identifies the *head* repository (the one that holds the branches), which callers use + # to fetch/push. The code hosting client, however, must talk to the repository that *hosts* the PRs, i.e. the base + # repository - in a fork workflow the base (upstream) repository differs from the head (fork) one. When a base branch + # is given, the base repository is inferred from that branch's tracking remote (exactly like `create_pull_request`), + # so retarget/restack query the right repository even with no base* config keys. When no base branch is given, the base + # repository resolves to the head one, so this is a no-op for the common (non-fork) case. + head_org_repo_remote = self.__derive_org_repo_and_remote( domain=domain, branch_used_for_tracking_data=branch_used_for_tracking_data) + base_org_repo_remote = head_org_repo_remote + if base_branch_used_for_tracking_data is not None: + keys = self.code_hosting_spec.git_config_keys + base_config_present = ( + self._config.code_hosting_base_remote(keys) is not None or + self._config.code_hosting_base_organization(keys) is not None or + self._config.code_hosting_base_repository(keys) is not None) + try: + base_org_repo_remote = self.__derive_org_repo_and_remote( + domain=domain, branch_used_for_tracking_data=base_branch_used_for_tracking_data, + is_base=True, fall_back_org_repo_to_head=True) + except MacheteException: + # The base repository could not be resolved unambiguously (for example the base branch has no tracking data + # among several candidate remotes). With no explicit base* config to honor, fall back to the head repository, + # preserving the pre-inference behavior; with base* keys set, the misconfiguration should surface. + if base_config_present: + raise self.code_hosting_client = self.code_hosting_spec.create_client( - domain=domain, organization=org_repo_remote.organization, repository=org_repo_remote.repository) - return domain, org_repo_remote + domain=domain, organization=base_org_repo_remote.organization, repository=base_org_repo_remote.repository) + return domain, head_org_repo_remote START_GIT_MACHETE_GENERATED_COMMENT = '' END_GIT_MACHETE_GENERATED_COMMENT = '' diff --git a/tests/mockers_github.py b/tests/mockers_github.py index a161f948a..7497a63b3 100644 --- a/tests/mockers_github.py +++ b/tests/mockers_github.py @@ -14,6 +14,7 @@ def mock_pr_json(head: str, base: str, number: int, repo_id: int = 1, + base_repo_id: Optional[int] = None, user: str = 'some_other_user', html_url: str = 'www.github.com', body: Optional[str] = '# Summary', @@ -23,7 +24,9 @@ def mock_pr_json(head: str, base: str, number: int, return { 'head': {'ref': head, 'repo': {'id': repo_id}}, 'user': {'login': user}, - 'base': {'ref': base}, + # `base.repo.id` records which repository hosts the PR; `None` (the default) means "served for any repository", + # so the vast majority of tests that don't care about fork/base targeting keep working unchanged. + 'base': {'ref': base, 'repo': {'id': base_repo_id}}, 'number': str(number), 'html_url': html_url, 'title': 'PR title', @@ -113,6 +116,12 @@ def url_with_query_params(**new_params: Any) -> str: new_query_string: str = urlencode({**query_params, **new_params}) return parsed_url._replace(query=new_query_string).geturl() + def find_repo_id_by_org_and_repo(org: str, repo: str) -> Optional[int]: + for repo_id, repo_data in github_api_state.repositories.items(): + if repo_data['owner']['login'] == org and repo_data['name'] == repo: + return repo_id + return None + def handle_get() -> "MockAPIResponse": if url_path_matches('/repositories/[0-9]+'): repo_no = int(url_segments[-1]) @@ -120,14 +129,22 @@ def handle_get() -> "MockAPIResponse": return MockAPIResponse(HTTPStatus.OK, github_api_state.repositories[repo_no]) raise error_404() elif url_path_matches('/repos/*/*/pulls'): + # The `/repos/{org}/{repo}/pulls` endpoint lists PRs hosted by the base repository `{org}/{repo}`, + # so filter out PRs whose base repository (if declared) differs from the one being queried. + requested_base_repo_id: Optional[int] = find_repo_id_by_org_and_repo(url_segments[-3], url_segments[-2]) + + def hosted_by_requested_repo(pull: Dict[str, Any]) -> bool: + base_repo_id = pull['base'].get('repo', {}).get('id') + return base_repo_id is None or base_repo_id == requested_base_repo_id + full_head_name: Optional[str] = query_params.get('head') if full_head_name: head: str = full_head_name.split(':')[1] - prs = github_api_state.get_open_pulls_by_head(head) + prs = [pull for pull in github_api_state.get_open_pulls_by_head(head) if hosted_by_requested_repo(pull)] # If no matching PRs are found, the real GitHub returns 200 OK with an empty JSON array - not 404. return MockAPIResponse(HTTPStatus.OK, prs) else: - pulls = github_api_state.get_open_pulls() + pulls = [pull for pull in github_api_state.get_open_pulls() if hosted_by_requested_repo(pull)] page_str = query_params.get('page') page = int(page_str) if page_str else 1 per_page = int(query_params['per_page']) diff --git a/tests/mockers_gitlab.py b/tests/mockers_gitlab.py index 07c0a83c9..255128451 100644 --- a/tests/mockers_gitlab.py +++ b/tests/mockers_gitlab.py @@ -21,6 +21,7 @@ def mock_mr_json(head: str, base: str, number: int, repo_id: int = 1, + base_repo_id: Optional[int] = None, user: str = 'some_other_user', html_url: str = 'www.gitlab.com', body: Optional[str] = '# Summary', @@ -30,6 +31,9 @@ def mock_mr_json(head: str, base: str, number: int, return { 'source_branch': head, 'source_project_id': repo_id, + # `project_id` records the target project that hosts the MR; `None` (the default) means "served for any project", + # so the vast majority of tests that don't care about fork/target targeting keep working unchanged. + 'project_id': base_repo_id, 'target_branch': base, 'author': {'username': user}, 'iid': str(number), @@ -127,6 +131,15 @@ def url_with_query_params(**new_params: Any) -> str: new_query_string: str = urlencode({**query_params, **new_params}) return parsed_url._replace(query=new_query_string).geturl() + def find_project_id(id_or_path: str) -> Optional[int]: + if id_or_path.isdigit(): + return int(id_or_path) + full_path = urllib.parse.unquote(id_or_path) + for project_id, project in gitlab_api_state.projects.items(): + if project['namespace']['full_path'] == full_path: + return project_id + return None + def handle_get() -> "MockAPIResponse": if url_path_matches('/projects/[0-9]+'): repo_no = int(url_segments[-1]) @@ -140,13 +153,21 @@ def handle_get() -> "MockAPIResponse": return MockAPIResponse(HTTPStatus.OK, project) raise error_404() elif url_path_matches('/projects/*/merge_requests'): + # The `/projects/{id}/merge_requests` endpoint lists MRs hosted by the target project `{id}`, + # so filter out MRs whose target project (if declared) differs from the one being queried. + requested_target_project_id: Optional[int] = find_project_id(url_segments[-2]) + + def hosted_by_requested_project(mr: Dict[str, Any]) -> bool: + target_project_id = mr.get('project_id') + return target_project_id is None or target_project_id == requested_target_project_id + head: Optional[str] = query_params.get('source_branch') if head: - mrs = gitlab_api_state.get_open_mrs_by_head(head) + mrs = [mr for mr in gitlab_api_state.get_open_mrs_by_head(head) if hosted_by_requested_project(mr)] # If no matching MRs are found, the real GitLab returns 200 OK with an empty JSON array - not 404. return MockAPIResponse(HTTPStatus.OK, mrs) else: - mrs = gitlab_api_state.get_open_mrs() + mrs = [mr for mr in gitlab_api_state.get_open_mrs() if hosted_by_requested_project(mr)] page_str = query_params.get('page') page = int(page_str) if page_str else 1 per_page_str = query_params.get('per_page') diff --git a/tests/test_github_restack_pr.py b/tests/test_github_restack_pr.py index aea59c2ab..970a42151 100644 --- a/tests/test_github_restack_pr.py +++ b/tests/test_github_restack_pr.py @@ -4,7 +4,8 @@ from tests.base_test import BaseTest from tests.cli_runner import assert_failure, assert_success, rewrite_branch_layout_file -from tests.git_repository import amend_commit, commit, create_repo_with_remote, new_branch, push, reset_to, set_git_config_key +from tests.git_repository import (add_remote, amend_commit, commit, create_repo, create_repo_with_remote, new_branch, push, reset_to, + set_git_config_key) from tests.mockers import fixed_author_and_committer_date_in_past from tests.mockers_code_hosting import mock_from_url from tests.mockers_github import MockGitHubAPIState, mock_github_token_for_domain_fake, mock_pr_json, mock_urlopen @@ -331,3 +332,46 @@ def test_github_restack_pr_branch_diverged_and_older(self, mocker: MockerFixture assert pr is not None assert pr['draft'] is False assert pr['base']['ref'] == 'master' + + def test_github_restack_pr_infers_base_repo_from_parent_tracking(self, mocker: MockerFixture) -> None: + # In a fork workflow the PR is hosted by the base (upstream) repository, not the head (fork) repository. + # Even with no base* config keys, restack-pr infers the base repository from the parent branch's tracking remote + # (just like create-pr does), so it queries the repository that actually hosts the PR. + self.patch_symbol(mocker, 'git_machete.github.GitHubToken.for_domain', mock_github_token_for_domain_fake) + self.patch_symbol(mocker, 'git_machete.code_hosting.OrganizationAndRepository.from_url', mock_from_url) + repositories = { + 1: {'owner': {'login': 'example-org'}, 'name': 'example-repo', + 'clone_url': 'https://github.com/example-org/example-repo.git'}, + 2: {'owner': {'login': 'example-org'}, 'name': 'example-repo-1', + 'clone_url': 'https://github.com/example-org/example-repo-1.git'}, + } + github_api_state = MockGitHubAPIState( + repositories, + mock_pr_json(number=1, head='feature', base='master', repo_id=1, base_repo_id=2, user='github_user')) + self.patch_symbol(mocker, 'urllib.request.urlopen', mock_urlopen(github_api_state)) + + # `origin` (-> example-org/example-repo) is the head/fork remote holding the branches, while + # `upstream` (-> example-org/example-repo-1) is the base remote that hosts the PR. The parent branch (develop) + # tracks `upstream`, so the base repository is inferred from it - no machete.github.base* key is set. + create_repo_with_remote() + upstream_path = create_repo("remote-1", bare=True, switch_dir_to_new_repo=False) + add_remote("upstream", upstream_path) + + new_branch("master") + commit() + push() + new_branch("develop") + commit() + push(remote="upstream") + new_branch("feature") + commit() + push() + rewrite_branch_layout_file("master\n\tdevelop\n\t\tfeature") + + assert_success( + ['github', 'restack-pr'], + "Switching base branch of PR #1 to develop... OK\n" + ) + pr = github_api_state.get_pull_by_number(1) + assert pr is not None + assert pr['base']['ref'] == 'develop' diff --git a/tests/test_github_retarget_pr.py b/tests/test_github_retarget_pr.py index bb61d75ee..0b5cb8a71 100644 --- a/tests/test_github_retarget_pr.py +++ b/tests/test_github_retarget_pr.py @@ -487,3 +487,46 @@ def test_github_retarget_pr_root_branch(self, mocker: MockerFixture) -> None: "Branch master does not have a parent branch (it is a root) even though there is an open PR #15 to root.\n" "Consider modifying the branch layout file (git machete edit) so that master is a child of root." ) + + def test_github_retarget_pr_infers_base_repo_from_parent_tracking(self, mocker: MockerFixture) -> None: + # In a fork workflow the PR is hosted by the base (upstream) repository, not the head (fork) repository. + # Even with no base* config keys, retarget-pr infers the base repository from the parent branch's tracking remote + # (just like create-pr does), so it queries the repository that actually hosts the PR. + self.patch_symbol(mocker, 'git_machete.code_hosting.OrganizationAndRepository.from_url', mock_from_url) + self.patch_symbol(mocker, 'git_machete.github.GitHubToken.for_domain', mock_github_token_for_domain_fake) + repositories = { + 1: {'owner': {'login': 'example-org'}, 'name': 'example-repo', + 'clone_url': 'https://github.com/example-org/example-repo.git'}, + 2: {'owner': {'login': 'example-org'}, 'name': 'example-repo-1', + 'clone_url': 'https://github.com/example-org/example-repo-1.git'}, + } + github_api_state = MockGitHubAPIState( + repositories, + mock_pr_json(number=1, head='feature', base='master', repo_id=1, base_repo_id=2, user='github_user')) + self.patch_symbol(mocker, 'urllib.request.urlopen', mock_urlopen(github_api_state)) + + # `origin` (-> example-org/example-repo) is the head/fork remote holding the branches, while + # `upstream` (-> example-org/example-repo-1) is the base remote that hosts the PR. The parent branch (develop) + # tracks `upstream`, so the base repository is inferred from it - no machete.github.base* key is set. + create_repo_with_remote() + upstream_path = create_repo("remote-1", bare=True, switch_dir_to_new_repo=False) + add_remote("upstream", upstream_path) + + new_branch("master") + commit() + push() + new_branch("develop") + commit() + push(remote="upstream") + new_branch("feature") + commit() + push() + rewrite_branch_layout_file("master\n\tdevelop\n\t\tfeature") + + assert_success( + ['github', 'retarget-pr'], + "Switching base branch of PR #1 to develop... OK\n" + ) + pr1 = github_api_state.get_pull_by_number(1) + assert pr1 is not None + assert pr1['base']['ref'] == 'develop' diff --git a/tests/test_gitlab_retarget_mr.py b/tests/test_gitlab_retarget_mr.py index 17d56e451..cb9030c24 100644 --- a/tests/test_gitlab_retarget_mr.py +++ b/tests/test_gitlab_retarget_mr.py @@ -505,3 +505,46 @@ def test_gitlab_retarget_mr_root_branch(self, mocker: MockerFixture) -> None: "Branch master does not have a parent branch (it is a root) even though there is an open MR !15 to root.\n" "Consider modifying the branch layout file (git machete edit) so that master is a child of root." ) + + def test_gitlab_retarget_mr_infers_target_project_from_parent_tracking(self, mocker: MockerFixture) -> None: + # In a fork workflow the MR is hosted by the target (upstream) project, not the source (fork) project. + # Even with no base* config keys, retarget-mr infers the target project from the parent branch's tracking remote + # (just like create-mr does), so it queries the project that actually hosts the MR. + self.patch_symbol(mocker, 'git_machete.code_hosting.OrganizationAndRepository.from_url', mock_from_url) + self.patch_symbol(mocker, 'git_machete.gitlab.GitLabToken.for_domain', mock_gitlab_token_for_domain_fake) + projects = { + 2: {'id': 2, 'namespace': {'full_path': 'example-org/example-repo'}, 'name': 'example-repo', + 'http_url_to_repo': 'https://gitlab.com/example-org/example-repo.git'}, + 3: {'id': 3, 'namespace': {'full_path': 'example-org/example-repo-1'}, 'name': 'example-repo-1', + 'http_url_to_repo': 'https://gitlab.com/example-org/example-repo-1.git'}, + } + gitlab_api_state = MockGitLabAPIState( + projects, + mock_mr_json(number=1, head='feature', base='master', repo_id=2, base_repo_id=3, user='gitlab_user')) + self.patch_symbol(mocker, 'urllib.request.urlopen', mock_urlopen(gitlab_api_state)) + + # `origin` (-> example-org/example-repo) is the source/fork remote holding the branches, while + # `upstream` (-> example-org/example-repo-1) is the base remote that hosts the MR. The parent branch (develop) + # tracks `upstream`, so the target project is inferred from it - no machete.gitlab.base* key is set. + create_repo_with_remote() + upstream_path = create_repo("remote-1", bare=True, switch_dir_to_new_repo=False) + add_remote("upstream", upstream_path) + + new_branch("master") + commit() + push() + new_branch("develop") + commit() + push(remote="upstream") + new_branch("feature") + commit() + push() + rewrite_branch_layout_file("master\n\tdevelop\n\t\tfeature") + + assert_success( + ['gitlab', 'retarget-mr'], + "Switching target branch of MR !1 to develop... OK\n" + ) + mr = gitlab_api_state.get_mr_by_number(1) + assert mr is not None + assert mr['target_branch'] == 'develop'