Repository: VirtusLab/git-machete
Pull request #558 (2022-06-21 11:23:36)

Issue #553: `github retarget-pr` should take branch tracking data into account

codecov-commenter: # [Codecov](https://codecov.io/gh/VirtusLab/git-machete/pull/558?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab) Report
> Merging [#558](https://codecov.io/gh/VirtusLab/git-machete/pull/558?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab) (fe500f4) into [develop](https://codecov.io/gh/VirtusLab/git-machete/commit/372189cdc038d8468d4748d2cd51e40dd4177d75?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab) (372189c) will **increase** coverage by `0.18%`.
> The diff coverage is `100.00%`.

```diff
@@             Coverage Diff             @@
##           develop     #558      +/-   ##
===========================================
+ Coverage    66.90%   67.09%   +0.18%     
===========================================
  Files           10       10              
  Lines         2880     2890      +10     
  Branches       699      703       +4     
===========================================
+ Hits          1927     1939      +12     
+ Misses         690      689       -1     
+ Partials       263      262       -1     
```


| [Impacted Files](https://codecov.io/gh/VirtusLab/git-machete/pull/558?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab) | Coverage Δ | |
|---|---|---|
| [git\_machete/\_\_init\_\_.py](https://codecov.io/gh/VirtusLab/git-machete/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab#diff-Z2l0X21hY2hldGUvX19pbml0X18ucHk=) | `100.00% <100.00%> (ø)` | |
| [git\_machete/client.py](https://codecov.io/gh/VirtusLab/git-machete/pull/558/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab#diff-Z2l0X21hY2hldGUvY2xpZW50LnB5) | `62.40% <100.00%> (+0.42%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/VirtusLab/git-machete/pull/558?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/VirtusLab/git-machete/pull/558?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab). Last update [372189c...fe500f4](https://codecov.io/gh/VirtusLab/git-machete/pull/558?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab).

PawelLipski: 67% coverage threshold passed 😌
PawelLipski: Anything still to be done here? 🤔 as it's still a draft
amalota: > Anything still to be done here? 🤔 as it's still a draft

Not sure yet, tomorrow I will take a look at it with fresh mind and see what's left to implement
PawelLipski:  > Not sure yet, tomorrow I will take a look at it with fresh mind and see what's left to implement

Turned out quite a lot, another Pandora's box opened in the process 😅 



Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 933785d..a23176e 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -2,7 +2,8 @@
 
 ## New in git-machete 3.11.2
 
-- fixed: `git machete` now correctly infers remote for fetching of branch when the branch is found in more than one remote
+- fixed: `git machete` now correctly infers remote for fetching of branch when the branch is associated with more than one remote
+- fixed: `git machete github retarget-pr` now takes branch tracking data into account when finding out where (in what GitHub organization/repository) to create a PR
 
 ## New in git-machete 3.11.1
 
diff --git a/git_machete/client.py b/git_machete/client.py
index 2708312..b823566 100644
--- a/git_machete/client.py
+++ b/git_machete/client.py
@@ -2012,7 +2012,7 @@ class MacheteClient:
     def retarget_github_pr(self, head: LocalBranchShortName) -> None:
         org: str
         repo: str
-        _, org, repo = self.__derive_remote_and_github_org_and_repo()
+        _, org, repo = self.__derive_remote_and_github_org_and_repo(branch_used_for_tracking_data=head)
 
         debug(f'organization is {org}, repository is {repo}')
 
@@ -2035,7 +2035,7 @@ class MacheteClient:
         else:
             print(fmt(f'The base branch of PR #{pr.number} is already `{new_base}`'))
 
-    def __derive_remote_and_github_org_and_repo(self) -> RemoteAndOrganizationAndRepository:
+    def __derive_remote_and_github_org_and_repo(self, branch_used_for_tracking_data: LocalBranchShortName = None) -> RemoteAndOrganizationAndRepository:
         remote_and_organization_and_repository_for_custom_url = self.__get_remote_and_organization_and_repository_name_for_custom_url()
         if all(remote_and_organization_and_repository_for_custom_url):
             return remote_and_organization_and_repository_for_custom_url
@@ -2049,6 +2049,7 @@ class MacheteClient:
         remote_and_github_org_and_repo: Dict[str, RemoteAndOrganizationAndRepository] = {
             remote: get_parsed_github_remote_url(url, remote) for remote, url in url_for_remote.items() if is_github_remote_url(url)
         }
+
         if not remote_and_github_org_and_repo:
             raise MacheteException(
                 fmt('Remotes are defined for this repository, but none of them '
@@ -2057,6 +2058,13 @@ class MacheteClient:
                     'If that is the case, you can provide repository information explicitly, via these 3 git config keys: '
                     '`machete.github.{remote,organization,repository}`\n'))
 
+        if len(remote_and_github_org_and_repo) > 1 and branch_used_for_tracking_data is not None:
+            remote_for_fetching_of_branch = self.__git.get_combined_remote_for_fetching_of_branch(
+                branch=branch_used_for_tracking_data,
+                remotes=list(remote_and_github_org_and_repo.keys()))
+            if remote_for_fetching_of_branch is not None:
+                return remote_and_github_org_and_repo[remote_for_fetching_of_branch]
+
         if len(remote_and_github_org_and_repo) == 1:
             return remote_and_github_org_and_repo[list(remote_and_github_org_and_repo.keys())[0]]
 
@@ -2065,7 +2073,7 @@ class MacheteClient:
 
         raise MacheteException(
             f'Multiple non-origin remotes correspond to GitHub in this repository: '
-            f'{", ".join(remote_and_github_org_and_repo.keys())}, aborting. \n'
+            f'{", ".join(remote_and_github_org_and_repo.keys())} -> aborting. \n'
             f'You can also select the repository by providing 3 git config keys: '
             '`machete.github.{remote,organization,repository}`\n')
 
diff --git a/git_machete/git_operations.py b/git_machete/git_operations.py
index e29be0d..dd9753e 100644
--- a/git_machete/git_operations.py
+++ b/git_machete/git_operations.py
@@ -454,9 +454,9 @@ class GitContext:
         remote = self.get_config_attr_or_none(f"branch.{branch}.remote")
         return remote.rstrip() if remote else None
 
-    def get_combined_remote_for_fetching_of_branch(self, branch: LocalBranchShortName) -> Optional[str]:
+    def get_combined_remote_for_fetching_of_branch(self, branch: LocalBranchShortName, remotes: Optional[List[str]] = None) -> Optional[str]:
         # Since many people don't use '--set-upstream' flag of 'push', we try to infer the remote instead if the tracking data is missing.
-        return self.get_strict_remote_for_fetching_of_branch(branch) or self.get_inferred_remote_for_fetching_of_branch(branch)
+        return self.get_strict_remote_for_fetching_of_branch(branch) or self.get_inferred_remote_for_fetching_of_branch(branch, remotes)
 
     def __get_inferred_counterpart_for_fetching_of_branch(self, branch: LocalBranchShortName) -> Optional[RemoteBranchShortName]:
         for remote in self.get_remotes():


Test patch:
diff --git a/tests/test_github.py b/tests/test_github.py
index 71d0439..9b87880 100644
--- a/tests/test_github.py
+++ b/tests/test_github.py
@@ -1,5 +1,6 @@
 import os
 import subprocess
+from tempfile import mkdtemp
 from typing import Any, Dict, Optional
 from unittest import mock
 
@@ -69,6 +70,24 @@ class TestGithub:
                 'user': {'login': 'github_user'},
                 'base': {'ref': 'root'}, 'number': '15',
                 'html_url': 'www.github.com', 'state': 'open'
+            },
+            {
+                'head': {'ref': 'feature_1', 'repo': mock_repository_info},
+                'user': {'login': 'github_user'},
+                'base': {'ref': 'root'}, 'number': '20',
+                'html_url': 'www.github.com', 'state': 'open'
+            },
+            {
+                'head': {'ref': 'feature_2', 'repo': mock_repository_info},
+                'user': {'login': 'github_user'},
+                'base': {'ref': 'root'}, 'number': '25',
+                'html_url': 'www.github.com', 'state': 'open'
+            },
+            {
+                'head': {'ref': 'feature_3', 'repo': mock_repository_info},
+                'user': {'login': 'github_user'},
+                'base': {'ref': 'root'}, 'number': '35',
+                'html_url': 'www.github.com', 'state': 'open'
             }
         ]
     )
@@ -105,6 +124,110 @@ class TestGithub:
             strip_indentation=False
         )
 
+    @mock.patch('git_machete.cli.exit_script', mock_exit_script)
+    @mock.patch('git_machete.github.GITHUB_REMOTE_PATTERNS', FAKE_GITHUB_REMOTE_PATTERNS)
+    @mock.patch('git_machete.utils.run_cmd', mock_run_cmd)  # to hide git outputs in tests
+    @mock.patch('urllib.request.Request', git_api_state_for_test_retarget_pr.new_request())
+    @mock.patch('urllib.request.urlopen', MockContextManager)
+    def test_github_retarget_pr_multiple_non_origin_remotes(self) -> None:
+        branchs_first_commit_msg = "First commit on branch."
+        branchs_second_commit_msg = "Second commit on branch."
+
+        origin_1_remote_path = mkdtemp()
+        origin_2_remote_path = mkdtemp()
+        self.repo_sandbox.new_repo(origin_1_remote_path)
+        self.repo_sandbox.new_repo(origin_2_remote_path)
+
+        os.chdir(self.repo_sandbox.local_path)
+
+        # branch feature present in each remote, no branch tracking data
+        (
+            self.repo_sandbox.remove_remote(remote='origin')
+                .new_branch("root")
+                .add_remote('origin_1', origin_1_remote_path)
+                .add_remote('origin_2', origin_2_remote_path)
+                .commit("First commit on root.")
+                .push(remote='origin_1')
+                .push(remote='origin_2')
+                .new_branch("branch-1")
+                .commit(branchs_first_commit_msg)
+                .commit(branchs_second_commit_msg)
+                .push(remote='origin_1')
+                .push(remote='origin_2')
+                .new_branch('feature')
+                .commit('introduce feature')
+                .push(remote='origin_1', set_upstream=False)
+                .push(remote='origin_2', set_upstream=False)
+        )
+
+        launch_command("discover", "-y")
+        expected_error_message = (
+            "Multiple non-origin remotes correspond to GitHub in this repository: origin_1, origin_2 -> aborting. \n"
+            "You can also select the repository by providing 3 git config keys: `machete.github.{remote,organization,repository}`\n"
+        )
+        with pytest.raises(MacheteException) as e:
+            launch_command("github", "retarget-pr")
+        if e:
+            assert e.value.args[0] == expected_error_message, \
+                'Verify that expected error message has appeared when given pull request to create is already created.'
+
+        # branch feature_1 present in each remote, tracking data present
+        (
+            self.repo_sandbox.check_out('feature')
+                .new_branch('feature_1')
+                .commit('introduce feature 1')
+                .push(remote='origin_1')
+                .push(remote='origin_2')
+        )
+
+        launch_command("discover", "-y")
+        assert_command(
+            ['github', 'retarget-pr'],
+            'The base branch of PR #20 has been switched to `feature`\n',
+            strip_indentation=False
+        )
+
+        # branch feature_2 is not present in any of the remotes
+        (
+            self.repo_sandbox.check_out('feature')
+                .new_branch('feature_2')
+                .commit('introduce feature 2')
+        )
+
+        launch_command("discover", "-y")
+        with pytest.raises(MacheteException) as e:
+            launch_command("github", "retarget-pr")
+        if e:
+            assert e.value.args[0] == expected_error_message, \
+                'Verify that expected error message has appeared when given pull request to create is already created.'
+
+        # branch feature_2 present in only one remote: origin_1 and there is no tracking data available -> infer the remote
+        (
+            self.repo_sandbox.check_out('feature_2')
+                .push(remote='origin_1', set_upstream=False)
+        )
+
+        assert_command(
+            ['github', 'retarget-pr'],
+            'The base branch of PR #25 has been switched to `feature`\n',
+            strip_indentation=False
+        )
+
+        # branch feature_3 present in only one remote: origin_1 and has tracking data
+        (
+            self.repo_sandbox.check_out('feature_2')
+                .new_branch('feature_3')
+                .commit('introduce feature 3')
+                .push(remote='origin_1')
+        )
+
+        launch_command("discover", "-y")
+        assert_command(
+            ['github', 'retarget-pr'],
+            'The base branch of PR #35 has been switched to `feature_2`\n',
+            strip_indentation=False
+        )
+
     git_api_state_for_test_anno_prs = MockGitHubAPIState(
         [
             {