Add missing tests - vol. 6

RELEASE_NOTES.md

@@ -2,7 +2,8 @@
 
 ## New in git-machete 3.17.5
 
-- fixed: `machete-status-branch` hook can now be executed on Windows
+- fixed: `machete-post-slide-out`, `machete-pre-rebase` and `machete-status-branch` hooks can now be executed on Windows
+- fixed: unstable behavior after `edit` option has been selected for interactively sliding out invalid branches
 
 ## New in git-machete 3.17.4
 

git_machete/cli.py

@@ -727,8 +727,6 @@ def strip_remote_name(remote_branch: RemoteBranchShortName) -> LocalBranchShortN
                 fork_point_hash=cli_opts.opt_fork_point, branch=current_branch)
 
         reapply_fork_point = cli_opts.opt_fork_point or machete_client.fork_point(branch=current_branch, use_overrides=True)
-        if not reapply_fork_point:
-            raise MacheteException(f"Fork point not found for branch <b>{current_branch}</b>; use `--fork-point` flag")
 
         git.rebase_onto_ancestor_commit(current_branch, reapply_fork_point, cli_opts.opt_no_interactive_rebase)
     elif cmd == "show":
@@ -770,9 +768,6 @@ def strip_remote_name(remote_branch: RemoteBranchShortName) -> LocalBranchShortN
                 fork_point_hash=cli_opts.opt_fork_point, branch=current_branch)
 
         squash_fork_point = cli_opts.opt_fork_point or machete_client.fork_point(branch=current_branch, use_overrides=True)
-        if not squash_fork_point:
-            raise MacheteException(f"Fork point not found for branch <b>{current_branch}</b>; use `--fork-point` flag")
-
         machete_client.squash(current_branch=current_branch, opt_fork_point=squash_fork_point)
     elif cmd in {"status", alias_by_command["status"]}:
         machete_client.read_definition_file(perform_interactive_slide_out=should_perform_interactive_slide_out)

git_machete/git_operations.py

@@ -279,7 +279,7 @@ def get_git_version(self) -> Tuple[int, ...]:
             # which is irrelevant for our purpose (checking whether certain git CLI features are available/bugs are fixed).
             raw = re.search(r"\d+.\d+.\d+", self._popen_git("version").stdout)
             if not raw:  # unlikely, never observed so far; mostly to satisfy mypy
-                return 0, 0, 0
+                return 0, 0, 0  # pragma: no cover
             self._git_version = tuple(map(int, raw.group(0).split(".")))
         return self._git_version
 
@@ -409,9 +409,9 @@ def push(self, remote: str, branch: LocalBranchShortName, force_with_lease: bool
             opt_force = []
         elif self.get_git_version() >= (2, 30, 0):  # earliest version of git to support 'push --force-with-lease --force-if-includes'
             opt_force = ["--force-with-lease", "--force-if-includes"]
-        elif self.get_git_version() >= (1, 8, 5):  # earliest version of git to support 'push --force-with-lease'
+        elif self.get_git_version() >= (1, 8, 5):  # pragma: no cover; earliest version of git to support 'push --force-with-lease'
             opt_force = ["--force-with-lease"]
-        else:
+        else:  # pragma: no cover
             opt_force = ["--force"]
         args = [remote, branch]
         self._run_git("push", "--set-upstream", *(opt_force + args))
@@ -667,7 +667,7 @@ def get_reflog(self, branch: AnyBranchName) -> List[GitReflogEntry]:
                 self.__load_all_reflogs()
             assert self.__reflogs_cached is not None
             return self.__reflogs_cached.get(branch, [])
-        else:
+        else:  # pragma: no cover
             if self.__reflogs_cached is None:
                 self.__reflogs_cached = {}
             if branch not in self.__reflogs_cached:
@@ -863,7 +863,7 @@ def get_merged_local_branches(self) -> List[LocalBranchShortName]:
                     utils.get_non_empty_lines(
                         self._popen_git("for-each-ref", "--format=%(refname)", "--merged", "HEAD", "refs/heads").stdout)))
         else:
-            return list(
+            return list(  # pragma: no cover
                 filter(lambda branch: self.is_ancestor_or_equal(branch, AnyRevision.of('HEAD')),
                        map(lambda branch: LocalBranchFullName.of(branch).to_short_name(),
                            utils.get_non_empty_lines(
@@ -904,55 +904,42 @@ def merge_fast_forward_only(self, branch: LocalBranchShortName) -> None:  # refs
         self.flush_caches()
 
     def rebase(self, onto: AnyRevision, from_exclusive: AnyRevision, branch: LocalBranchShortName, opt_no_interactive_rebase: bool) -> None:
-        def do_rebase() -> None:
-            # Let's use `OPTS` suffix for consistency with git's built-in env var `GIT_DIFF_OPTS`
-            git_machete_rebase_opts_var = 'GIT_MACHETE_REBASE_OPTS'
-            rebase_opts = os.environ.get(git_machete_rebase_opts_var, '').split()
-            try:
-                if not opt_no_interactive_rebase:
-                    rebase_opts.append("--interactive")
-                if self.get_git_version() >= (2, 26, 0):
-                    rebase_opts.append("--empty=drop")
-                self._run_git("rebase", *rebase_opts, "--onto", onto, from_exclusive, branch)
-            finally:
-                # https://public-inbox.org/git/317468c6-40cc-9f26-8ee3-3392c3908efb@talktalk.net/T
-                # In our case, this can happen when git version invoked by git-machete to start the rebase
-                # is different than git version used (outside of git-machete) to continue the rebase.
-                # This used to be the case when git-machete was installed via a strict-confinement snap
-                # with its own version of git baked in as a dependency.
-                # Currently we're using classic-confinement snaps which no longer have this problem
-                # (snapped git-machete uses whatever git is available in the host system),
-                # but it still doesn't harm to patch the author script.
-
-                # No need to fix <git-dir>/rebase-apply/author-script,
-                # only <git-dir>/rebase-merge/author-script (i.e. interactive rebases, for the most part) is affected.
-                author_script = self.get_worktree_git_subpath("rebase-merge", "author-script")
-                if os.path.isfile(author_script):
-                    faulty_line_regex = re.compile("[A-Z0-9_]+='[^']*")
-
-                    def fix_if_needed(line: str) -> str:
-                        return f"{line.rstrip()}'\n" if faulty_line_regex.fullmatch(line) else line
-
-                    def get_all_lines_fixed() -> Iterator[str]:
-                        with open(author_script) as f_read:
-                            return map(fix_if_needed, f_read.readlines())
-
-                    fixed_lines = get_all_lines_fixed()  # must happen before we open for writing
-                    # See https://github.com/VirtusLab/git-machete/issues/935 for why author-script needs to be saved in this manner
-                    io.open(author_script, "w", newline="").write("".join(fixed_lines))
-                self.flush_caches()
-
-        hook_path = self.get_hook_path("machete-pre-rebase")
-        if self.check_hook_executable(hook_path):
-            debug(f"running machete-pre-rebase hook ({hook_path})")
-            exit_code = utils.run_cmd(hook_path, onto, from_exclusive, branch, cwd=self.get_root_dir())
-            if exit_code == 0:
-                do_rebase()
-            else:
-                raise MacheteException(
-                    f"The machete-pre-rebase hook refused to rebase. Error code: {exit_code}\n")
-        else:
-            do_rebase()
+        # Let's use `OPTS` suffix for consistency with git's built-in env var `GIT_DIFF_OPTS`
+        git_machete_rebase_opts_var = 'GIT_MACHETE_REBASE_OPTS'
+        rebase_opts = os.environ.get(git_machete_rebase_opts_var, '').split()
+        try:
+            if not opt_no_interactive_rebase:
+                rebase_opts.append("--interactive")
+            if self.get_git_version() >= (2, 26, 0):  # pragma: no branch
+                rebase_opts.append("--empty=drop")
+            self._run_git("rebase", *rebase_opts, "--onto", onto, from_exclusive, branch)
+        finally:
+            # https://public-inbox.org/git/317468c6-40cc-9f26-8ee3-3392c3908efb@talktalk.net/T
+            # In our case, this can happen when git version invoked by git-machete to start the rebase
+            # is different than git version used (outside of git-machete) to continue the rebase.
+            # This used to be the case when git-machete was installed via a strict-confinement snap
+            # with its own version of git baked in as a dependency.
+            # Currently we're using classic-confinement snaps which no longer have this problem
+            # (snapped git-machete uses whatever git is available in the host system),
+            # but it still doesn't harm to patch the author script.
+
+            # No need to fix <git-dir>/rebase-apply/author-script,
+            # only <git-dir>/rebase-merge/author-script (i.e. interactive rebases, for the most part) is affected.
+            author_script = self.get_worktree_git_subpath("rebase-merge", "author-script")
+            if os.path.isfile(author_script):
+                faulty_line_regex = re.compile("[A-Z0-9_]+='[^']*")
+
+                def fix_if_needed(line: str) -> str:
+                    return f"{line.rstrip()}'\n" if faulty_line_regex.fullmatch(line) else line
+
+                def get_all_lines_fixed() -> Iterator[str]:
+                    with open(author_script) as f_read:
+                        return map(fix_if_needed, f_read.readlines())
+
+                fixed_lines = get_all_lines_fixed()  # must happen before we open for writing
+                # See https://github.com/VirtusLab/git-machete/issues/935 for why author-script needs to be saved in this manner
+                io.open(author_script, "w", newline="").write("".join(fixed_lines))
+            self.flush_caches()
 
     def rebase_onto_ancestor_commit(self,
                                     branch: LocalBranchShortName,

tests/base_test.py

@@ -6,12 +6,6 @@
 
 from git_machete.git_operations import GitContext
 
-
-def popen(command: str) -> str:
-    with os.popen(command) as process:
-        return process.read().strip()
-
-
 git: GitContext = GitContext()
 
 
@@ -37,10 +31,18 @@ def __init__(self) -> None:
         self.local_path = mkdtemp()
         self.file_counter = 0
 
+    def popen(self, command: str) -> str:
+        with os.popen(command) as process:
+            return process.read().strip()
+
     def execute(self, command: str) -> "GitRepositorySandbox":
         subprocess.check_call(command, shell=True)
         return self
 
+    def execute_ignoring_exit_code(self, command: str) -> "GitRepositorySandbox":
+        subprocess.call(command, shell=True)
+        return self
+
     def new_repo(self, directory: str, bare: bool, switch_dir_to_new_repo: bool = True) -> "GitRepositorySandbox":
         previous_dir = os.getcwd()
         os.chdir(directory)
@@ -54,7 +56,7 @@ def new_branch(self, branch_name: str) -> "GitRepositorySandbox":
         self.execute(f"git checkout -b {branch_name}")
         return self
 
-    def new_root_branch(self, branch_name: str) -> "GitRepositorySandbox":
+    def new_orphan_branch(self, branch_name: str) -> "GitRepositorySandbox":
         self.execute(f"git checkout --orphan {branch_name}")
         return self
 
@@ -82,7 +84,7 @@ def commit_amend(self, message: str) -> "GitRepositorySandbox":
         return self
 
     def push(self, remote: str = 'origin', set_upstream: bool = True, tracking_branch: Optional[str] = None) -> "GitRepositorySandbox":
-        branch = popen("git symbolic-ref -q --short HEAD")
+        branch = self.popen("git symbolic-ref -q --short HEAD")
         tracking_branch = tracking_branch or branch
         self.execute(f"git push {'--set-upstream' if set_upstream else ''} {remote} {branch}:{tracking_branch}")
         return self
@@ -108,7 +110,7 @@ def remove_remote(self, remote: str = 'origin') -> "GitRepositorySandbox":
         return self
 
     def get_local_branches(self) -> List[str]:
-        return popen('git for-each-ref refs/heads/ "--format=%(refname:short)"').splitlines()
+        return self.popen('git for-each-ref refs/heads/ "--format=%(refname:short)"').splitlines()
 
     def is_ancestor(self, earlier: str, later: str) -> bool:
         return subprocess.call(f'git merge-base --is-ancestor  "{earlier}"  "{later}"', shell=True) == 0
@@ -117,7 +119,7 @@ def get_current_commit_hash(self) -> str:
         return self.get_commit_hash("HEAD")
 
     def get_commit_hash(self, revision: str) -> str:
-        return popen(f"git rev-parse {revision}")
+        return self.popen(f"git rev-parse {revision}")
 
     def set_git_config_key(self, key: str, value: str) -> "GitRepositorySandbox":
         self.execute(f'git config {key} {value}')

tests/mockers_github.py

@@ -1,9 +1,10 @@
+import contextlib
 import json
 import re
 from http import HTTPStatus
 from subprocess import CompletedProcess
 from tempfile import mkdtemp
-from typing import Any, Callable, ContextManager, Dict, List, Optional, Union
+from typing import Any, Callable, Dict, Iterator, List, Optional, Union
 from urllib.error import HTTPError
 from urllib.parse import ParseResult, parse_qs, urlparse
 
@@ -284,24 +285,15 @@ def read(self, n: int = 1) -> bytes:  # noqa: F841
         return json.dumps(self.msg).encode()
 
 
-class MockContextManager(ContextManager[MockGitHubAPIResponse]):
-    def __init__(self, obj: MockGitHubAPIResponse) -> None:
-        self.obj = obj
+@contextlib.contextmanager
+def mock_urlopen(obj: MockGitHubAPIResponse) -> Iterator[MockGitHubAPIResponse]:
+    if obj.status_code == HTTPStatus.NOT_FOUND:
+        raise HTTPError("http://example.org", 404, 'Not found', None, None)  # type: ignore[arg-type]
+    elif obj.status_code == HTTPStatus.UNPROCESSABLE_ENTITY:
+        raise MockHTTPError("http://example.org", 422, obj.response_data, None, None)  # type: ignore[arg-type]
+    yield obj
 
-    def __enter__(self) -> MockGitHubAPIResponse:
-        if self.obj.status_code == HTTPStatus.NOT_FOUND:
-            raise HTTPError("http://example.org", 404, 'Not found', None, None)  # type: ignore[arg-type]
-        elif self.obj.status_code == HTTPStatus.UNPROCESSABLE_ENTITY:
-            raise MockHTTPError("http://example.org", 422, self.obj.response_data, None, None)  # type: ignore[arg-type]
-        return self.obj
 
-    def __exit__(self, *args: Any) -> None:
-        pass
-
-
-class MockContextManagerRaise403(MockContextManager):
-    def __init__(self, obj: MockGitHubAPIResponse) -> None:
-        super().__init__(obj)
-
-    def __enter__(self) -> MockGitHubAPIResponse:
-        raise HTTPError("http://example.org", 403, 'Forbidden', None, None)  # type: ignore[arg-type]
+@contextlib.contextmanager
+def mock_urlopen_raising_403(obj: MockGitHubAPIResponse) -> Iterator[MockGitHubAPIResponse]:
+    raise HTTPError("http://example.org", 403, 'Forbidden', None, None)  # type: ignore[arg-type]

tests/test_add.py

@@ -1,8 +1,9 @@
 from typing import Any
 
 from .base_test import BaseTest
-from .mockers import (assert_failure, assert_success,
-                      mock_run_cmd_and_discard_output, rewrite_definition_file)
+from .mockers import (assert_failure, assert_success, mock_input_returning,
+                      mock_input_returning_y, mock_run_cmd_and_discard_output,
+                      rewrite_definition_file)
 
 
 class TestAdd(BaseTest):
@@ -31,6 +32,11 @@ def test_add(self, mocker: Any) -> None:
         self.repo_sandbox.new_branch("bugfix/feature_fail")
 
         # Test `git machete add` without providing the branch name
+        mocker.patch("builtins.input", mock_input_returning("n"))
+        assert_success(
+            ['add'],
+            'Add bugfix/feature_fail onto the inferred upstream (parent) branch develop? (y, N) \n'
+        )
         assert_success(
             ['add', '-y'],
             'Adding bugfix/feature_fail onto the inferred upstream (parent) branch develop\n'
@@ -76,19 +82,69 @@ def test_add_check_out_remote_branch(self, mocker: Any) -> None:
             .delete_branch("feature/foo")
         )
 
+        mocker.patch("builtins.input", mock_input_returning("n"))
+        assert_success(
+            ['add', 'foo'],
+            'A local branch foo does not exist. Create out of the current HEAD? (y, N) \n'
+        )
+
         assert_success(
             ['add', '-y', 'foo'],
             'A local branch foo does not exist. Creating out of the current HEAD\n'
             'Added branch foo as a new root\n'
         )
 
+        mocker.patch("builtins.input", mock_input_returning("n"))
+        assert_success(
+            ['add', '--as-root', 'feature/foo'],
+            'A local branch feature/foo does not exist, but a remote branch origin/feature/foo exists.\n'
+            'Check out feature/foo locally? (y, N) \n'
+        )
+
         assert_success(
             ['add', '-y', '--as-root', 'feature/foo'],
             'A local branch feature/foo does not exist, but a remote branch origin/feature/foo exists.\n'
             'Checking out feature/foo locally...\n'
             'Added branch feature/foo as a new root\n'
         )
 
+    def test_add_new_branch_onto_managed_current_branch(self, mocker: Any) -> None:
+        (
+            self.repo_sandbox.new_branch("master")
+            .commit()
+        )
+
+        rewrite_definition_file("master")
+
+        mocker.patch("builtins.input", mock_input_returning_y)
+        assert_success(
+            ['add', 'foo'],
+            "A local branch foo does not exist. Create out of the current HEAD? (y, N) \n"
+            "Added branch foo onto master\n"
+        )
+
+    def test_add_new_branch_when_cannot_infer_parent(self, mocker: Any) -> None:
+        (
+            self.repo_sandbox.new_branch("master")
+            .commit()
+            .new_branch("develop")
+            .commit()
+            .check_out("master")
+        )
+
+        rewrite_definition_file("develop")
+
+        mocker.patch("builtins.input", mock_input_returning_y)
+        assert_failure(
+            ['add', 'foo'],
+            """
+            Could not automatically infer upstream (parent) branch for foo.
+            You can either:
+            1) specify the desired upstream branch with --onto or
+            2) pass --as-root to attach foo as a new root or
+            3) edit the definition file manually with git machete edit"""
+        )
+
     def test_add_already_managed_branch(self) -> None:
         (
             self.repo_sandbox.new_branch("master")
@@ -100,3 +156,19 @@ def test_add_already_managed_branch(self) -> None:
         rewrite_definition_file("master\n  develop")
 
         assert_failure(['add', 'develop'], 'Branch develop already exists in the tree of branch dependencies')
+
+    def test_add_onto_non_existent_branch(self) -> None:
+        (
+            self.repo_sandbox.new_branch("master")
+            .commit("master commit.")
+            .new_branch("develop")
+            .commit("develop commit.")
+        )
+
+        rewrite_definition_file("master")
+
+        assert_failure(
+            ['add', 'develop', '--onto', 'foo'],
+            "Branch foo not found in the tree of branch dependencies.\n"
+            "Use git machete add foo or git machete edit."
+        )