Repository: VirtusLab/git-machete

Issue:
Check first if given pull request is closed in `github checkout-pr`
Github Api returns all opened pull requests after `GET` on [pulls endpoint](https://docs.github.com/en/rest/reference/pulls).

During `github checkout-pr` on pull request which is already closed, git-machete ends up with message:
```
$ git machete github checkout-pr 204

PR number 204 is not found in repository git-machete
```
Scope of this issue is to make distinction in the error message when pull request is closed or never created.

Discussion:

Patch:
diff --git a/git_machete/client.py b/git_machete/client.py
index 146e994..7cdabcf 100644
--- a/git_machete/client.py
+++ b/git_machete/client.py
@@ -16,10 +16,10 @@ from git_machete.constants import (
 from git_machete.exceptions import MacheteException, StopInteraction
 from git_machete.git_operations import GitContext
 from git_machete.github import (
-    GitHubPullRequest, derive_pull_requests,
-    derive_pull_request_by_head, set_base_of_pull_request, get_parsed_github_remote_url, is_github_remote_url,
-    create_pull_request, set_milestone_of_pull_request,
-    add_assignees_to_pull_request, add_reviewers_to_pull_request)
+    add_assignees_to_pull_request, add_reviewers_to_pull_request,
+    create_pull_request, derive_pull_request_by_head, derive_pull_requests,
+    get_parsed_github_remote_url, get_pull_request_by_number_or_none, GitHubPullRequest,
+    is_github_remote_url, set_base_of_pull_request, set_milestone_of_pull_request)
 from git_machete.utils import (
     get_pretty_choices, flat_map, excluding, fmt, tupled, warn, debug, bold,
     colored, underline, dim, get_second)
@@ -1682,21 +1682,23 @@ class MacheteClient:
         remote: str
         remote, (org, repo) = self.__derive_remote_and_github_org_and_repo()
         debug('checkout_github_pr()', f'organization is {org}, repository is {repo}')
+        print(f"Fetching {remote}...")
+        self.__git.fetch_remote(remote)
+        self.flush_caches()
 
-        all_prs: List[GitHubPullRequest] = derive_pull_requests(org, repo)
-        pr = utils.find_or_none(lambda pr: pr.number == pr_no, all_prs)
-
+        pr = get_pull_request_by_number_or_none(pr_no, org, repo)
         if not pr:
             raise MacheteException(f"PR #{pr_no} is not found in repository `{org}/{repo}`")
+        if '/'.join([remote, pr.head]) not in self.__git.get_remote_branches():
+            raise MacheteException(f"Could not check out PR #{pr_no} because its head branch `{pr.head}` is already deleted from `{remote}`.")
+        if pr.state == 'closed':
+            warn(f'Pull request #{pr_no} is already closed.')
         debug('checkout_github_pr()', f'found {pr}')
 
-        print(f"Fetching {remote}...")
-        self.__git.fetch_remote(remote)
-        if self.__git.remotes():
-            self.flush_caches()
+        all_open_prs: List[GitHubPullRequest] = derive_pull_requests(org, repo)
 
         self.__cli_opts.opt_yes = True  # TODO (#161): pass only needed options to methods
-        path: List[str] = self.__get_path_from_pr_chain(pr, all_prs)
+        path: List[str] = self.__get_path_from_pr_chain(pr, all_open_prs)
         reversed_path: List[str] = path[::-1]  # need to add from root downwards
         for index, branch in enumerate(reversed_path):
             if branch not in self.managed_branches:
@@ -1712,16 +1714,16 @@ class MacheteClient:
         current_user: Optional[str] = git_machete.github.derive_current_user_login()
         debug('checkout_github_pr()',
               'Current GitHub user is ' + (current_user or '<none>'))
-        self.__sync_annotations_to_definition_file(all_prs, current_user)
+        self.__sync_annotations_to_definition_file(all_open_prs, current_user)
 
         self.__git.checkout(pr.head)
         print(fmt(f"Pull request `#{pr.number}` checked out at local branch `{pr.head}`"))
 
-    def __get_path_from_pr_chain(self, current_pr: GitHubPullRequest, all_prs: List[GitHubPullRequest]) -> List[str]:
+    def __get_path_from_pr_chain(self, current_pr: GitHubPullRequest, all_open_prs: List[GitHubPullRequest]) -> List[str]:
         path: List[str] = [current_pr.head]
         while current_pr:
             path.append(current_pr.base)
-            current_pr = utils.find_or_none(lambda x: x.head == current_pr.base, all_prs)
+            current_pr = utils.find_or_none(lambda x: x.head == current_pr.base, all_open_prs)
         return path
 
     def retarget_github_pr(self, head: str) -> None:
diff --git a/git_machete/github.py b/git_machete/github.py
index d322d39..6f98654 100644
--- a/git_machete/github.py
+++ b/git_machete/github.py
@@ -29,19 +29,20 @@ GITHUB_REMOTE_PATTERNS = [
 
 
 class GitHubPullRequest(object):
-    def __init__(self, number: int, user: str, base: str, head: str, html_url: str):
+    def __init__(self, number: int, user: str, base: str, head: str, html_url: str, state: str) -> None:
         self.number = number
         self.user = user
         self.base = base
         self.head = head
         self.html_url = html_url
+        self.state = state
 
     def __repr__(self) -> str:
         return f"PR #{self.number} by {self.user}: {self.head} -> {self.base}"
 
 
 def __parse_pr_json(pr_json: Any) -> GitHubPullRequest:
-    return GitHubPullRequest(int(pr_json['number']), pr_json['user']['login'], pr_json['base']['ref'], pr_json['head']['ref'], pr_json['html_url'])
+    return GitHubPullRequest(int(pr_json['number']), pr_json['user']['login'], pr_json['base']['ref'], pr_json['head']['ref'], pr_json['html_url'], pr_json['state'])
 
 
 def __get_github_token() -> Optional[str]:
@@ -126,7 +127,7 @@ def __fire_github_api_request(method: str, path: str, token: Optional[str], requ
     except HTTPError as err:
         if err.code == http.HTTPStatus.UNPROCESSABLE_ENTITY:
             raise UnprocessableEntityHTTPError(str(err.reason))
-        elif err.code in (http.HTTPStatus.UNAUTHORIZED, http.HTTPStatus.FORBIDDEN, http.HTTPStatus.NOT_FOUND):
+        elif err.code in (http.HTTPStatus.UNAUTHORIZED, http.HTTPStatus.FORBIDDEN):
             first_line = f'GitHub API returned {err.code} HTTP status with error message: `{err.reason}`\n'
             if token:
                 raise MacheteException(first_line + f'Make sure that the token provided in `gh auth status` or `~/.config/hub`'
@@ -136,6 +137,9 @@ def __fire_github_api_request(method: str, path: str, token: Optional[str], requ
                     first_line + f'You might not have the required permissions for this repository. '
                                  f'Provide a GitHub API token with `repo` access via <b>{GITHUB_TOKEN_ENV_VAR}</b> env var or `gh` or `hub`.\n'
                                  'Visit `https://github.com/settings/tokens` to generate a new one.')
+        elif err.code == http.HTTPStatus.NOT_FOUND:
+            raise MacheteException(
+                f'Given endpoint: {url}, not found in Github')  # TODO (#164): make dedicated exception here
         else:
             first_line = fmt(f'GitHub API returned {err.code} HTTP status with error message: `{err.reason}`\n')
             raise MacheteException(first_line + "Please open an issue regarding this topic under link: https://github.com/VirtusLab/git-machete/issues/new")
@@ -225,3 +229,12 @@ def get_parsed_github_remote_url(url: str) -> Optional[Tuple[str, str]]:
         if match:
             return match.group(1), match.group(2)
     return None
+
+
+def get_pull_request_by_number_or_none(number: int, org: str, repo: str) -> Optional[GitHubPullRequest]:
+    token: Optional[str] = __get_github_token()
+    try:
+        pr_json: Dict[str, Any] = __fire_github_api_request('GET', f'/repos/{org}/{repo}/pulls/{number}', token)
+        return __parse_pr_json(pr_json)
+    except MacheteException:
+        return None


Test patch:
diff --git a/git_machete/tests/functional/test_machete.py b/git_machete/tests/functional/test_machete.py
index afd239b..dd0d6e8 100644
--- a/git_machete/tests/functional/test_machete.py
+++ b/git_machete/tests/functional/test_machete.py
@@ -8,11 +8,12 @@ import textwrap
 import time
 import unittest
 import subprocess
-from contextlib import redirect_stdout
+from contextlib import redirect_stdout, redirect_stderr
 from http import HTTPStatus
 from typing import Any, Dict, Iterable, List, Optional, Union
 from unittest import mock
 from urllib.parse import urlparse, ParseResult, parse_qs
+from urllib.error import HTTPError
 
 from git_machete import cli
 from git_machete.client import MacheteClient
@@ -45,10 +46,10 @@ class FakeCommandLineOptions(CommandLineOptions):
 
 
 class MockGithubAPIState:
-    def __init__(self, pulls: List[Dict[str, Any]]) -> None:
+    def __init__(self, pulls: List[Dict[str, Any]], issues: List[Dict[str, Any]] = None) -> None:
         self.pulls: List[Dict[str, Any]] = pulls
         self.user: Dict[str, str] = {'login': 'other_user', 'type': 'User', 'company': 'VirtusLab'}  # login must be different from the one used in pull requests, otherwise pull request author will not be annotated
-        self.issues: List[Dict[str, Any]] = []
+        self.issues: List[Dict[str, Any]] = issues or []
 
     def new_request(self) -> "MockGithubAPIRequest":
         return MockGithubAPIRequest(self)
@@ -98,9 +99,15 @@ class MockGithubAPIRequest:
             return self.make_response_object(HTTPStatus.METHOD_NOT_ALLOWED, [])
 
     def handle_get(self) -> "MockGithubAPIResponse":
-        if self.parsed_url.path.endswith('pulls'):
+        if 'pulls' in self.parsed_url.path:
             full_head_name: Optional[List[str]] = self.parsed_query.get('head')
-            if full_head_name:
+            number: Optional[str] = self.find_number(self.parsed_url.path, 'pulls')
+            if number:
+                for pr in self.github_api_state.pulls:
+                    if pr['number'] == number:
+                        return self.make_response_object(HTTPStatus.OK, pr)
+                return self.make_response_object(HTTPStatus.NOT_FOUND, [])
+            elif full_head_name:
                 head: str = full_head_name[0].split(':')[1]
                 for pr in self.github_api_state.pulls:
                     if pr['head']['ref'] == head:
@@ -123,9 +130,9 @@ class MockGithubAPIRequest:
 
     def handle_post(self) -> "MockGithubAPIResponse":
         assert not self.parsed_query
-        if self.parsed_url.path.endswith('issues'):
+        if 'issues' in self.parsed_url.path:
             return self.update_issue()
-        elif self.parsed_url.path.endswith('pulls'):
+        elif 'pulls' in self.parsed_url.path:
             return self.update_pull_request()
         else:
             return self.make_response_object(HTTPStatus.NOT_FOUND, [])
@@ -140,7 +147,8 @@ class MockGithubAPIRequest:
     def create_pull_request(self) -> "MockGithubAPIResponse":
         pull = {'number': self.get_next_free_number(self.github_api_state.pulls),
                 'user': {'login': 'github_user'},
-                'html_url': 'www.github.com'}
+                'html_url': 'www.github.com',
+                'state': 'open'}
         return self.fill_pull_request_data(json.loads(self.json_data), pull)
 
     def fill_pull_request_data(self, data: Dict[str, Any], pull: Dict[str, Any]) -> "MockGithubAPIResponse":
@@ -172,7 +180,7 @@ class MockGithubAPIRequest:
         index = self.get_index_or_none(issue, self.github_api_state.issues)
         for key in data.keys():
             issue[key] = data[key]
-        if index:
+        if index is not None:
             self.github_api_state.issues[index] = issue
         else:
             self.github_api_state.issues.append(issue)
@@ -203,10 +211,12 @@ class MockGithubAPIRequest:
 
 
 class MockContextManager:
-    def __init__(self, obj: MockGithubAPIRequest) -> None:
+    def __init__(self, obj: MockGithubAPIResponse) -> None:
         self.obj = obj
 
-    def __enter__(self) -> MockGithubAPIRequest:
+    def __enter__(self) -> MockGithubAPIResponse:
+        if self.obj.status_code == HTTPStatus.NOT_FOUND:
+            raise HTTPError(None, 404, 'Not found', None, None)
         return self.obj
 
     def __exit__(self, *args: Any) -> None:
@@ -282,8 +292,9 @@ class MacheteTester(unittest.TestCase):
     def launch_command(*args: str) -> str:
         with io.StringIO() as out:
             with redirect_stdout(out):
-                cli.launch(list(args))
-                git.flush_caches()
+                with redirect_stderr(out):
+                    cli.launch(list(args))
+                    git.flush_caches()
             return out.getvalue()
 
     @staticmethod
@@ -1576,7 +1587,7 @@ class MacheteTester(unittest.TestCase):
 
     git_api_state_for_test_retarget_pr = MockGithubAPIState(
         [{'head': {'ref': 'feature'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'root'}, 'number': '15',
-          'html_url': 'www.github.com'}])
+          'html_url': 'www.github.com', 'state': 'open'}])
 
     @mock.patch('urllib.request.Request', git_api_state_for_test_retarget_pr.new_request())
     @mock.patch('urllib.request.urlopen', MockContextManager)
@@ -1602,9 +1613,9 @@ class MacheteTester(unittest.TestCase):
         self.assert_command(['github', 'retarget-pr'], 'The base branch of PR #15 is already `branch-1`\n', strip_indentation=False)
 
     git_api_state_for_test_anno_prs = MockGithubAPIState([
-        {'head': {'ref': 'ignore-trailing'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'hotfix/add-trigger'}, 'number': '3', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'allow-ownership-link'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '7', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'call-ws'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '31', 'html_url': 'www.github.com'}
+        {'head': {'ref': 'ignore-trailing'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'hotfix/add-trigger'}, 'number': '3', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'allow-ownership-link'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '7', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'call-ws'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '31', 'html_url': 'www.github.com', 'state': 'open'}
     ])
 
     @mock.patch('git_machete.github.derive_current_user_login', mock_derive_current_user_login)
@@ -1674,7 +1685,8 @@ class MacheteTester(unittest.TestCase):
             """,
         )
 
-    git_api_state_for_test_create_pr = MockGithubAPIState([{'head': {'ref': 'ignore-trailing'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'hotfix/add-trigger'}, 'number': '3', 'html_url': 'www.github.com'}])
+    git_api_state_for_test_create_pr = MockGithubAPIState([{'head': {'ref': 'ignore-trailing'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'hotfix/add-trigger'}, 'number': '3', 'html_url': 'www.github.com', 'state': 'open'}],
+                                                          issues=[{'number': '4'}, {'number': '5'}, {'number': '6'}])
 
     @mock.patch('git_machete.options.CommandLineOptions', FakeCommandLineOptions)
     @mock.patch('urllib.request.urlopen', MockContextManager)
@@ -1841,14 +1853,15 @@ class MacheteTester(unittest.TestCase):
                              'Verify that expected error message has appeared when creating PR from root branch.')
 
     git_api_state_for_test_checkout_prs = MockGithubAPIState([
-        {'head': {'ref': 'chore/redundant_checks'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'restrict_access'}, 'number': '18', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'restrict_access'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'allow-ownership-link'}, 'number': '17', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'allow-ownership-link'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'bugfix/feature'}, 'number': '12', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'bugfix/feature'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'enhance/feature'}, 'number': '6', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'enhance/add_user'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '19', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'testing/add_user'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'bugfix/add_user'}, 'number': '22', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'chore/comments'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'testing/add_user'}, 'number': '24', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'ignore-trailing'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'hotfix/add-trigger'}, 'number': '3', 'html_url': 'www.github.com'}
+        {'head': {'ref': 'chore/redundant_checks'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'restrict_access'}, 'number': '18', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'restrict_access'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'allow-ownership-link'}, 'number': '17', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'allow-ownership-link'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'bugfix/feature'}, 'number': '12', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'bugfix/feature'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'enhance/feature'}, 'number': '6', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'enhance/add_user'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '19', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'testing/add_user'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'bugfix/add_user'}, 'number': '22', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'chore/comments'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'testing/add_user'}, 'number': '24', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'ignore-trailing'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'hotfix/add-trigger'}, 'number': '3', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'bugfix/remove-n-option'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'develop'}, 'number': '5', 'html_url': 'www.github.com', 'state': 'closed'}
     ])
 
     # We need to mock GITHUB_REMOTE_PATTERNS in the tests for `test_checkout_prs` due to `git fetch` executed by `checkout-prs` subcommand.
@@ -2017,10 +2030,21 @@ class MacheteTester(unittest.TestCase):
             self.assertEqual(e.exception.parameter, expected_error_message,
                              'Verify that expected error message has appeared when given pull request to checkout does not exists.')
 
+        # Check against closed pull request with head branch deleted from remote
+        machete_client = MacheteClient(cli_opts, git)
+        machete_client.read_definition_file()
+        expected_error_message = "Could not check out PR #5 because its head branch `bugfix/remove-n-option` is already deleted from `origin`."
+        with self.assertRaises(MacheteException) as e:
+            machete_client.checkout_github_prs(5)
+        if e:
+            self.assertEqual(e.exception.parameter, expected_error_message,
+                             'Verify that expected error message has appeared when given pull request to checkout have already deleted branch from remote.')
+
     git_api_state_for_test_checkout_prs_fresh_repo = MockGithubAPIState([
-        {'head': {'ref': 'comments/add_docstrings'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'improve/refactor'}, 'number': '2', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'restrict_access'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'allow-ownership-link'}, 'number': '17', 'html_url': 'www.github.com'},
-        {'head': {'ref': 'improve/refactor'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'chore/sync_to_docs'}, 'number': '1', 'html_url': 'www.github.com'},
+        {'head': {'ref': 'comments/add_docstrings'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'improve/refactor'}, 'number': '2', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'restrict_access'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'allow-ownership-link'}, 'number': '17', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'improve/refactor'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'chore/sync_to_docs'}, 'number': '1', 'html_url': 'www.github.com', 'state': 'open'},
+        {'head': {'ref': 'sphinx_export'}, 'user': {'login': 'github_user'}, 'base': {'ref': 'comments/add_docstrings'}, 'number': '23', 'html_url': 'www.github.com', 'state': 'closed'}
     ])
 
     # We need to mock GITHUB_REMOTE_PATTERNS in the tests for `test_checkout_prs_freshly_cloned` due to `git fetch` executed by `checkout-prs` subcommand.
@@ -2044,6 +2068,9 @@ class MacheteTester(unittest.TestCase):
             .new_branch("comments/add_docstrings")
             .commit("docstring added")
             .push()
+            .new_branch("sphinx_export")
+            .commit("export docs to html")
+            .push()
             .check_out("root")
             .new_branch("master")
             .commit("Master commit")
@@ -2092,3 +2119,31 @@ class MacheteTester(unittest.TestCase):
               o-comments/add_docstrings *  PR #2 (github_user)
             """
         )
+
+        # Check against closed pull request
+        expected_msg = ("Fetching origin...\n"
+                        "Warn: Pull request #23 is already closed.\n"
+                        "A local branch `sphinx_export` does not exist, but a remote branch `origin/sphinx_export` exists.\n"
+                        "Checking out `sphinx_export` locally...\n"
+                        "Added branch `sphinx_export` onto `comments/add_docstrings`\n"
+                        "Annotating sphinx_export as `PR #23 (github_user)`\n"
+                        "Pull request `#23` checked out at local branch `sphinx_export`\n")
+        self.assert_command(
+            ['github', 'checkout-prs', '23'],
+            expected_msg,
+            strip_indentation=False
+        )
+        self.assert_command(
+            ["status"],
+            """
+            master
+
+            chore/sync_to_docs
+            |
+            o-improve/refactor  PR #1 (github_user)
+              |
+              o-comments/add_docstrings  PR #2 (github_user)
+                |
+                o-sphinx_export *  PR #23 (github_user)
+            """
+        )