diff --git a/docs/changes.rst b/docs/changes.rst index f8497bb36ba2..6df3440e1324 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -114,6 +114,7 @@ Weblate 5.15 * Avoid false positive checks upon committing pending changes. * Performance improvements for file upload. * Show glossary matches for the source language. +* Pull/merge requests are now only created when necessary. .. rubric:: Compatibility diff --git a/weblate/vcs/git.py b/weblate/vcs/git.py index 102590778c6a..e052b71ecad8 100644 --- a/weblate/vcs/git.py +++ b/weblate/vcs/git.py @@ -875,6 +875,56 @@ class GitMergeRequestBase(GitForcePushRepository): REQUIRED_CONFIG: ClassVar[set[str]] = {"username", "token"} OPTIONAL_CONFIG: ClassVar[set[str]] = {"scheme"} + def get_fork_branch_name(self) -> str: + """Get the fork branch name used for pushing.""" + if self.component is not None: + return f"weblate-{self.component.project.slug}-{self.component.slug}" + return f"weblate-{self.branch}" + + def count_outgoing(self, branch: str | None = None) -> int: + """ + Count outgoing commits. + + For merge request workflows, we need to check against both the origin + remote (to see if commits are already merged) and the fork remote (to see + if commits are already pushed but not merged). This prevents creating + duplicate merge requests when commits have already been merged, while also + avoiding unnecessary pushes when commits are already in the fork. + """ + # Check if fork is used for this branch (default and matching branches use fork) + if not self.should_use_fork(branch): + # Fork not used: check specified branch directly on origin + return super().count_outgoing(branch) + + # Fork workflow: check if commits have been merged to origin's pull branch + # Omit branch parameter to check the repository's default pull branch + origin_outgoing = super().count_outgoing() + if origin_outgoing == 0: + # All commits are in origin, nothing to push + return 0 + + # Check if commits are in the fork (already pushed) + # The fork branch name is determined by get_fork_branch_name() + credentials = self.get_credentials() + fork_remote = credentials["username"] + fork_branch_name = self.get_fork_branch_name() + fork_branch = f"{fork_remote}/{fork_branch_name}" + + try: + fork_outgoing = len( + self.log_revisions(self.ref_from_remote.format(fork_branch)) + ) + # If fork has all commits, don't need to push + if fork_outgoing == 0: + return 0 + except RepositoryError: + # Fork branch doesn't exist yet or is not accessible, + # indicating commits haven't been pushed to fork + pass + + # Commits are not in origin or fork, need to push + return origin_outgoing + def merge( self, abort: bool = False, message: str | None = None, no_ff: bool = False ) -> None: @@ -1087,12 +1137,7 @@ def push(self, branch: str) -> None: else: fork_remote = credentials["username"] self.fork(credentials) - if self.component is not None: - fork_branch = ( - f"weblate-{self.component.project.slug}-{self.component.slug}" - ) - else: - fork_branch = f"weblate-{self.branch}" + fork_branch = self.get_fork_branch_name() self.push_to_fork(credentials, self.branch, fork_branch) self.create_pull_request(credentials, self.branch, fork_remote, fork_branch) diff --git a/weblate/vcs/tests/test_vcs.py b/weblate/vcs/tests/test_vcs.py index ce82cc690243..e298369e1191 100644 --- a/weblate/vcs/tests/test_vcs.py +++ b/weblate/vcs/tests/test_vcs.py @@ -513,6 +513,14 @@ class VCSGitForcePushTest(VCSGitTest): class VCSGitUpstreamTest(VCSGitTest): + _repo_override: str = "" + + def setUp(self) -> None: + super().setUp() + # Set repo URL to match configured credentials + if self._repo_override: + self.repo.component.repo = self._repo_override + def add_remote_commit(self, conflict=False, rename=False) -> None: # Use Git to create changed upstream repo backup = self._class @@ -530,6 +538,7 @@ class VCSGiteaTest(VCSGitUpstreamTest): _class = GiteaFakeRepository _vcs = "git" _sets_push = False + _repo_override = "https://try.gitea.io/WeblateOrg/test.git" def mock_responses(self, pr_response, pr_status=200) -> None: """ @@ -623,8 +632,6 @@ def test_api_url_try_gitea(self) -> None: @responses.activate def test_push(self, branch: str = "") -> None: - self.repo.component.repo = "https://try.gitea.io/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -641,8 +648,6 @@ def test_push(self, branch: str = "") -> None: @responses.activate def test_pull_request_error(self, branch: str = "") -> None: - self.repo.component.repo = "https://try.gitea.io/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -660,8 +665,6 @@ def test_pull_request_error(self, branch: str = "") -> None: @responses.activate def test_pull_request_exists(self, branch: str = "") -> None: - self.repo.component.repo = "https://try.gitea.io/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -699,13 +702,10 @@ class VCSAzureDevOpsTest(VCSGitUpstreamTest): _vcs = "git" _sets_push = False _mock_push_to_fork = None + _repo_override = "https://dev.azure.com/organization/WeblateOrg/test.git" def setUp(self) -> None: super().setUp() - self.repo.component.repo = ( - "https://dev.azure.com/organization/WeblateOrg/test.git" - ) - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1059,6 +1059,7 @@ class VCSGitHubTest(VCSGitUpstreamTest): _class = GithubFakeRepository _vcs = "git" _sets_push = False + _repo_override = "https://github.com/WeblateOrg/test.git" def mock_responses(self, pr_response, pr_status=200) -> None: """ @@ -1166,8 +1167,6 @@ def test_api_url_ghes(self) -> None: @responses.activate def test_push(self, branch: str = "") -> None: - self.repo.component.repo = "https://github.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1184,8 +1183,6 @@ def test_push(self, branch: str = "") -> None: @responses.activate def test_pull_request_error(self, branch: str = "") -> None: - self.repo.component.repo = "https://github.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1203,8 +1200,6 @@ def test_pull_request_error(self, branch: str = "") -> None: @responses.activate def test_pull_request_exists(self, branch: str = "") -> None: - self.repo.component.repo = "https://github.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1245,6 +1240,7 @@ class VCSGitLabTest(VCSGitUpstreamTest): _class = GitLabFakeRepository _vcs = "git" _sets_push = False + _repo_override = "https://gitlab.com/WeblateOrg/test.git" def mock_fork_responses(self, get_forks, repo_state=200) -> None: if repo_state == 409: @@ -1469,8 +1465,6 @@ def test_api_url_self_hosted(self) -> None: @responses.activate def test_push(self, branch: str = "") -> None: - self.repo.component.repo = "https://gitlab.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1490,8 +1484,6 @@ def test_push(self, branch: str = "") -> None: @responses.activate def test_push_with_existing_fork(self, branch: str = "") -> None: - self.repo.component.repo = "https://gitlab.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1525,8 +1517,6 @@ def test_push_with_existing_fork(self, branch: str = "") -> None: @responses.activate def test_push_duplicate_repo_name(self, branch: str = "") -> None: - self.repo.component.repo = "https://gitlab.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1554,8 +1544,6 @@ def test_push_duplicate_repo_name(self, branch: str = "") -> None: @responses.activate def test_push_rejected(self, branch: str = "") -> None: - self.repo.component.repo = "https://gitlab.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1584,8 +1572,6 @@ def test_push_rejected(self, branch: str = "") -> None: @responses.activate def test_pull_request_error(self, branch: str = "") -> None: - self.repo.component.repo = "https://gitlab.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1602,8 +1588,6 @@ def test_pull_request_error(self, branch: str = "") -> None: @responses.activate def test_pull_request_exists(self, branch: str = "") -> None: - self.repo.component.repo = "https://gitlab.com/WeblateOrg/test.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1620,6 +1604,56 @@ def test_pull_request_exists(self, branch: str = "") -> None: super().test_push(branch) mock_push_to_fork.stop() + def test_count_outgoing_after_merge(self) -> None: + """Test that count_outgoing correctly detects no pending changes after merge.""" + # Make a commit locally + self.test_commit() + + # Verify there are outgoing commits before any push/merge + initial_count = self.repo.count_outgoing() + self.assertGreater(initial_count, 0) + + # Scenario 1: Simulate the commit being pushed to fork + # The fork branch name is from get_fork_branch_name() method + credentials = self.repo.get_credentials() + fork_branch_name = self.repo.get_fork_branch_name() + fork_ref = f"refs/remotes/{credentials['username']}/{fork_branch_name}" + self.repo.execute(["update-ref", fork_ref, "HEAD"], needs_lock=False) + + # count_outgoing should now be 0 since fork has our commits + # (even though origin doesn't yet - MR is pending) + self.assertEqual(self.repo.count_outgoing(), 0) + + # Scenario 2: Simulate the merge request being merged to origin + # In a real scenario, after a merge request is merged and git fetch is done, + # origin/{branch} would contain the local commits + origin_ref = f"refs/remotes/origin/{self.repo.branch}" + self.repo.execute(["update-ref", origin_ref, "HEAD"], needs_lock=False) + + # count_outgoing should still return 0 since origin has our commits + self.assertEqual(self.repo.count_outgoing(), 0) + + # Verify with explicit branch parameter + self.assertEqual(self.repo.count_outgoing(self.repo.branch), 0) + + def test_count_outgoing_non_default_branch(self) -> None: + """Test count_outgoing with non-default branch doesn't check fork.""" + # Make a commit locally + self.test_commit() + + # When called with a different branch than self.branch, + # should_use_fork() returns False, so fork checking is skipped + # Create a different branch name + different_branch = "develop" if self.repo.branch != "develop" else "feature" + + # Update origin ref for the different branch + origin_ref = f"refs/remotes/origin/{different_branch}" + self.repo.execute(["update-ref", origin_ref, "HEAD"], needs_lock=False) + + # count_outgoing with different branch should return 0 + # (commits are in origin for that branch, fork not checked) + self.assertEqual(self.repo.count_outgoing(different_branch), 0) + @override_settings( PAGURE_CREDENTIALS={"pagure.io": {"username": "test", "token": "token"}} @@ -1628,6 +1662,7 @@ class VCSPagureTest(VCSGitUpstreamTest): _class = PagureFakeRepository _vcs = "git" _sets_push = False + _repo_override = "https://pagure.io/testrepo.git" def mock_responses(self, pr_response: dict, existing_response: dict) -> None: """Mock response helper function.""" @@ -1658,8 +1693,6 @@ def mock_responses(self, pr_response: dict, existing_response: dict) -> None: @responses.activate def test_push(self, branch: str = "") -> None: - self.repo.component.repo = "https://pagure.io/testrepo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1678,8 +1711,6 @@ def test_push(self, branch: str = "") -> None: @responses.activate def test_push_with_existing_fork(self, branch: str = "") -> None: - self.repo.component.repo = "https://pagure.io/testrepo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1707,8 +1738,6 @@ def test_push_with_existing_fork(self, branch: str = "") -> None: @responses.activate def test_push_with_existing_request(self, branch: str = "") -> None: - self.repo.component.repo = "https://pagure.io/testrepo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -1929,6 +1958,7 @@ class VCSBitbucketServerTest(VCSGitUpstreamTest): _sets_push = False _bbhost = "https://api.selfhosted.com" + _repo_override = f"{_bbhost}/bb_pk/bb_repo.git" _bb_api_error_stub: ClassVar[dict] = { "errors": [{"context": "", "message": ""}] } @@ -2099,8 +2129,6 @@ def test_get_headers(self) -> None: @responses.activate def test_default_reviewers_repo_error(self) -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2121,8 +2149,6 @@ def test_default_reviewers_repo_error(self) -> None: @responses.activate def test_default_reviewers_error(self) -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2145,8 +2171,6 @@ def test_default_reviewers_error(self) -> None: @responses.activate def test_push(self, branch: str = "") -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2164,8 +2188,6 @@ def test_push(self, branch: str = "") -> None: @responses.activate def test_push_with_existing_pr(self, branch: str = "") -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2183,8 +2205,6 @@ def test_push_with_existing_pr(self, branch: str = "") -> None: @responses.activate def test_push_pr_error_response(self, branch: str = "") -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2203,8 +2223,6 @@ def test_push_pr_error_response(self, branch: str = "") -> None: @responses.activate def test_push_with_existing_fork(self, branch: str = "") -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2223,8 +2241,6 @@ def test_push_with_existing_fork(self, branch: str = "") -> None: @responses.activate def test_create_fork_unexpected_fail(self, branch: str = "") -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2240,8 +2256,6 @@ def test_create_fork_unexpected_fail(self, branch: str = "") -> None: @responses.activate def test_existing_fork_not_found(self, branch: str = "") -> None: - self.repo.component.repo = f"{self._bbhost}/bb_pk/bb_repo.git" - # Patch push_to_fork() function because we don't want to actually # make a git push request mock_push_to_fork_patcher = patch( @@ -2272,6 +2286,7 @@ class VCSBitbucketCloudTest(VCSGitUpstreamTest): _vcs = "git" _sets_push = False _apihost = "bitbucket.org" + _repo_override = "git@bitbucket.org:WeblateOrg/test.git" def mock_responses(self) -> None: """ @@ -2357,7 +2372,6 @@ def mock_responses(self) -> None: @responses.activate def test_push(self, branch: str = "") -> None: """Test push to bitbucket cloud.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" self.mock_responses() with patch("weblate.vcs.git.GitMergeRequestBase.push_to_fork", return_value=""): super().test_push(branch) @@ -2365,7 +2379,6 @@ def test_push(self, branch: str = "") -> None: @responses.activate def test_push_with_http(self, branch: str = "") -> None: """Test push to bitbucket cloud with HTTP repo link.""" - self.repo.component.repo = "https://bitbucket.org/WeblateOrg/test.git" self.mock_responses() with patch("weblate.vcs.git.GitMergeRequestBase.push_to_fork", return_value=""): super().test_push(branch) @@ -2373,8 +2386,6 @@ def test_push_with_http(self, branch: str = "") -> None: @responses.activate def test_push_with_missing_permission(self, branch: str = "") -> None: """Test push with missing permission for App Password.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" - self.mock_responses() responses.replace( responses.POST, @@ -2400,7 +2411,6 @@ def test_push_with_missing_permission(self, branch: str = "") -> None: @responses.activate def test_default_reviewers_error(self, branch: str = "") -> None: """Test default reviewers error, push expected to be successful.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" self.mock_responses() responses.replace( @@ -2424,7 +2434,6 @@ def test_default_reviewers_error(self, branch: str = "") -> None: @responses.activate def test_paginated_reviewers_list(self, branch: str = "") -> None: """Test the 'build_full_paginated_result' with default reviewers list.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" self.mock_responses() responses.replace( @@ -2475,7 +2484,6 @@ def test_paginated_reviewers_list(self, branch: str = "") -> None: @responses.activate def test_push_nothing_to_merge(self, branch: str = "") -> None: """Test push to bitbucket cloud with no changes to be merged.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" self.mock_responses() responses.replace( @@ -2494,7 +2502,6 @@ def test_push_nothing_to_merge(self, branch: str = "") -> None: @responses.activate def test_fork_already_exists(self, branch: str = "") -> None: """Test push to bitbucket cloud with HTTP repo link.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" self.mock_responses() responses.replace( @@ -2532,7 +2539,6 @@ def test_fork_already_exists(self, branch: str = "") -> None: @responses.activate def test_fork_name_already_taken(self, branch: str = "") -> None: """Test push to bitbucket cloud with HTTP repo link.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" responses.add( responses.POST, "https://api.bitbucket.org/2.0/repositories/WeblateOrg/test/forks", @@ -2554,7 +2560,6 @@ def test_fork_name_already_taken(self, branch: str = "") -> None: @responses.activate def test_fork_error(self, branch: str = "") -> None: """Test push to bitbucket cloud with HTTP repo link.""" - self.repo.component.repo = "git@bitbucket.org:WeblateOrg/test.git" self.mock_responses() responses.replace( responses.POST,