refactor(git): code cleanup and better test coverage

Author: bearomorphism

commitizen/git.py

@@ -1,34 +1,14 @@
 from __future__ import annotations
 
 import os
-from enum import Enum
-from os import linesep
 from pathlib import Path
 from tempfile import NamedTemporaryFile
 
 from commitizen import cmd, out
 from commitizen.exceptions import GitCommandError
 
-UNIX_EOL = "\n"
-WINDOWS_EOL = "\r\n"
-
-
-class EOLTypes(Enum):
-    """The EOL type from `git config core.eol`."""
-
-    LF = "lf"
-    CRLF = "crlf"
-    NATIVE = "native"
-
-    def get_eol_for_open(self) -> str:
-        """Get the EOL character for `open()`."""
-        map = {
-            EOLTypes.CRLF: WINDOWS_EOL,
-            EOLTypes.LF: UNIX_EOL,
-            EOLTypes.NATIVE: linesep,
-        }
-
-        return map[self]
+_UNIX_EOL = "\n"
+_WINDOWS_EOL = "\r\n"
 
 
 class GitObject:
@@ -37,9 +17,7 @@ class GitObject:
     date: str
 
     def __eq__(self, other) -> bool:
-        if not hasattr(other, "rev"):
-            return False
-        return self.rev == other.rev  # type: ignore
+        return hasattr(other, "rev") and self.rev == other.rev
 
 
 class GitCommit(GitObject):
@@ -63,6 +41,20 @@ def __init__(
     def message(self):
         return f"{self.title}\n\n{self.body}".strip()
 
+    @classmethod
+    def from_rev_and_commit(cls, rev_and_commit: str) -> GitCommit:
+        rev, parents, title, author, author_email, *body_list = rev_and_commit.split(
+            "\n"
+        )
+        return cls(
+            rev=rev.strip(),
+            title=title.strip(),
+            body="\n".join(body_list).strip(),
+            author=author,
+            author_email=author_email,
+            parents=[p for p in parents.strip().split(" ") if p],
+        )
+
     def __repr__(self):
         return f"{self.title} ({self.rev})"
 
@@ -101,13 +93,11 @@ def tag(
     # according to https://git-scm.com/book/en/v2/Git-Basics-Tagging,
     # we're not able to create lightweight tag with message.
     # by adding message, we make it a annotated tags
-    c = cmd.run(f'git tag {_opt} "{tag if _opt == "" or msg is None else msg}"')
-    return c
+    return cmd.run(f'git tag {_opt} "{tag if _opt == "" or msg is None else msg}"')
 
 
 def add(*args: str) -> cmd.Command:
-    c = cmd.run(f"git add {' '.join(args)}")
-    return c
+    return cmd.run(f"git add {' '.join(args)}")
 
 
 def commit(
@@ -140,24 +130,10 @@ def get_commits(
 ) -> list[GitCommit]:
     """Get the commits between start and end."""
     git_log_entries = _get_log_as_str_list(start, end, args)
-    git_commits = []
-    for rev_and_commit in git_log_entries:
-        if not rev_and_commit:
-            continue
-        rev, parents, title, author, author_email, *body_list = rev_and_commit.split(
-            "\n"
-        )
-        if rev_and_commit:
-            git_commit = GitCommit(
-                rev=rev.strip(),
-                title=title.strip(),
-                body="\n".join(body_list).strip(),
-                author=author,
-                author_email=author_email,
-                parents=[p for p in parents.strip().split(" ") if p],
-            )
-            git_commits.append(git_commit)
-    return git_commits
+    return [
+        GitCommit.from_rev_and_commit(rev_and_commit)
+        for rev_and_commit in filter(None, git_log_entries)
+    ]
 
 
 def get_filenames_in_commit(git_reference: str = ""):
@@ -170,8 +146,7 @@ def get_filenames_in_commit(git_reference: str = ""):
     c = cmd.run(f"git show --name-only --pretty=format: {git_reference}")
     if c.return_code == 0:
         return c.out.strip().split("\n")
-    else:
-        raise GitCommandError(c.err)
+    raise GitCommandError(c.err)
 
 
 def get_tags(
@@ -197,16 +172,11 @@ def get_tags(
     if c.err:
         out.warn(f"Attempting to proceed after: {c.err}")
 
-    if not c.out:
-        return []
-
-    git_tags = [
+    return [
         GitTag.from_line(line=line, inner_delimiter=inner_delimiter)
         for line in c.out.split("\n")[:-1]
     ]
 
-    return git_tags
-
 
 def tag_exist(tag: str) -> bool:
     c = cmd.run(f"git tag --list {tag}")
@@ -231,18 +201,18 @@ def get_tag_message(tag: str) -> str | None:
     return c.out.strip()
 
 
-def get_tag_names() -> list[str | None]:
+def get_tag_names() -> list[str]:
     c = cmd.run("git tag --list")
     if c.err:
         return []
-    return [tag.strip() for tag in c.out.split("\n") if tag.strip()]
+    return list(filter(None, (tag.strip() for tag in c.out.split("\n"))))
 
 
 def find_git_project_root() -> Path | None:
     c = cmd.run("git rev-parse --show-toplevel")
-    if not c.err:
-        return Path(c.out.strip())
-    return None
+    if c.err:
+        return None
+    return Path(c.out.strip())
 
 
 def is_staging_clean() -> bool:
@@ -253,32 +223,19 @@ def is_staging_clean() -> bool:
 
 def is_git_project() -> bool:
     c = cmd.run("git rev-parse --is-inside-work-tree")
-    if c.out.strip() == "true":
-        return True
-    return False
+    return c.out.strip() == "true"
 
 
-def get_eol_style() -> EOLTypes:
+def get_eol_for_open() -> str:
+    # See: https://git-scm.com/docs/git-config#Documentation/git-config.txt-coreeol
     c = cmd.run("git config core.eol")
     eol = c.out.strip().lower()
 
-    # We enumerate the EOL types of the response of
-    # `git config core.eol`, and map it to our enumration EOLTypes.
-    #
-    # It is just like the variant of the "match" syntax.
-    map = {
-        "lf": EOLTypes.LF,
-        "crlf": EOLTypes.CRLF,
-        "native": EOLTypes.NATIVE,
-    }
-
-    # If the response of `git config core.eol` is in the map:
-    if eol in map:
-        return map[eol]
-    else:
-        # The default value is "native".
-        # https://git-scm.com/docs/git-config#Documentation/git-config.txt-coreeol
-        return map["native"]
+    if eol == "lf":
+        return _UNIX_EOL
+    if eol == "crlf":
+        return _WINDOWS_EOL
+    return os.linesep
 
 
 def get_core_editor() -> str | None:
@@ -288,22 +245,18 @@ def get_core_editor() -> str | None:
     return None
 
 
-def smart_open(*args, **kargs):
+def smart_open(*args, **kwargs):
     """Open a file with the EOL style determined from Git."""
-    return open(*args, newline=get_eol_style().get_eol_for_open(), **kargs)
+    return open(*args, newline=get_eol_for_open(), **kwargs)
 
 
 def _get_log_as_str_list(start: str | None, end: str, args: str) -> list[str]:
     """Get string representation of each log entry"""
     delimiter = "----------commit-delimiter----------"
     log_format: str = "%H%n%P%n%s%n%an%n%ae%n%b"
-    git_log_cmd = (
-        f"git -c log.showSignature=False log --pretty={log_format}{delimiter} {args}"
-    )
-    if start:
-        command = f"{git_log_cmd} {start}..{end}"
-    else:
-        command = f"{git_log_cmd} {end}"
+    command_range = f"{start}..{end}" if start else end
+    command = f"git -c log.showSignature=False log --pretty={log_format}{delimiter} {args} {command_range}"
+
     c = cmd.run(command)
     if c.return_code != 0:
         raise GitCommandError(c.err)

tests/test_git.py

@@ -79,8 +79,7 @@ def test_get_reachable_tags_with_commits(
     monkeypatch.setenv("LANGUAGE", f"{locale}.UTF-8")
     monkeypatch.setenv("LC_ALL", f"{locale}.UTF-8")
     with tmp_commitizen_project.as_cwd():
-        tags = git.get_tags(reachable_only=True)
-        assert tags == []
+        assert git.get_tags(reachable_only=True) == []
 
 
 def test_get_tag_names(mocker: MockFixture):
@@ -271,7 +270,7 @@ def test_get_commits_with_signature():
 def test_get_tag_names_has_correct_arrow_annotation():
     arrow_annotation = inspect.getfullargspec(git.get_tag_names).annotations["return"]
 
-    assert arrow_annotation == "list[str | None]"
+    assert arrow_annotation == "list[str]"
 
 
 def test_get_latest_tag_name(tmp_commitizen_project):
@@ -317,24 +316,18 @@ def test_is_staging_clean_when_updating_file(tmp_commitizen_project):
         assert git.is_staging_clean() is False
 
 
-def test_git_eol_style(tmp_commitizen_project):
+def test_get_eol_for_open(tmp_commitizen_project):
     with tmp_commitizen_project.as_cwd():
-        assert git.get_eol_style() == git.EOLTypes.NATIVE
+        assert git.get_eol_for_open() == os.linesep
 
         cmd.run("git config core.eol lf")
-        assert git.get_eol_style() == git.EOLTypes.LF
+        assert git.get_eol_for_open() == "\n"
 
         cmd.run("git config core.eol crlf")
-        assert git.get_eol_style() == git.EOLTypes.CRLF
+        assert git.get_eol_for_open() == "\r\n"
 
         cmd.run("git config core.eol native")
-        assert git.get_eol_style() == git.EOLTypes.NATIVE
-
-
-def test_eoltypes_get_eol_for_open():
-    assert git.EOLTypes.get_eol_for_open(git.EOLTypes.NATIVE) == os.linesep
-    assert git.EOLTypes.get_eol_for_open(git.EOLTypes.LF) == "\n"
-    assert git.EOLTypes.get_eol_for_open(git.EOLTypes.CRLF) == "\r\n"
+        assert git.get_eol_for_open() == os.linesep
 
 
 def test_get_core_editor(mocker):
@@ -401,3 +394,14 @@ def test_commit_with_spaces_in_path(mocker, file_path, expected_cmd):
 
     mock_run.assert_called_once_with(expected_cmd)
     mock_unlink.assert_called_once_with(file_path)
+
+
+def test_get_filenames_in_commit_error(mocker: MockFixture):
+    """Test that GitCommandError is raised when git command fails."""
+    mocker.patch(
+        "commitizen.cmd.run",
+        return_value=FakeCommand(out="", err="fatal: bad object HEAD", return_code=1),
+    )
+    with pytest.raises(exceptions.GitCommandError) as excinfo:
+        git.get_filenames_in_commit()
+    assert str(excinfo.value) == "fatal: bad object HEAD"