refactor(git): Address more review comments

* Move `static_tool_capture` out of `SubprocessRunner` class
* Remove `SHA1Hash` class, instead using normal `str`s and validating in `Commit.__post_init__` instead
* Tweak args used for diffing in `Git._capture_diff_lines`
* Tweak args used for getting untracked files in `Git.get_working_tree_changes`
* Use `...` diffing in `Git.get_changes_with_base`
* Convert `Commit` to a `Struct` instead of a dataclass
* Fix tests

Author: Ishirui

src/dda/config/model/user.py

@@ -10,24 +10,24 @@ def _get_name_from_git() -> str:
     from os import environ
 
     from dda.tools.git import Git
-    from dda.utils.process import SubprocessRunner
+    from dda.utils.process import static_capture
 
     if name := environ.get(Git.AUTHOR_NAME_ENV_VAR):
         return name
 
-    return SubprocessRunner.static_capture(["git", "config", "--global", "--get", "user.name"]).strip()
+    return static_capture(["git", "config", "--global", "--get", "user.name"]).strip()
 
 
 def _get_emails_from_git() -> list[str]:
     from os import environ
 
     from dda.tools.git import Git
-    from dda.utils.process import SubprocessRunner
+    from dda.utils.process import static_capture
 
     if email := environ.get(Git.AUTHOR_EMAIL_ENV_VAR):
         return [email]
 
-    return [SubprocessRunner.static_capture(["git", "config", "--global", "--get", "user.email"]).strip()]
+    return [static_capture(["git", "config", "--global", "--get", "user.email"]).strip()]
 
 
 class UserConfig(Struct, frozen=True):

src/dda/tools/git.py

@@ -4,10 +4,13 @@
 from __future__ import annotations
 
 from functools import cached_property
+from typing import TYPE_CHECKING
 
 from dda.tools.base import Tool
 from dda.utils.git.changeset import ChangeSet
-from dda.utils.git.commit import Commit, CommitDetails, SHA1Hash
+
+if TYPE_CHECKING:
+    from dda.utils.git.commit import Commit, CommitDetails
 
 
 class Git(Tool):
@@ -95,22 +98,21 @@ def get_head_commit(self) -> Commit:
         """
         Get the current HEAD commit of the Git repository in the current working directory.
         """
-        from dda.utils.git.commit import Commit, SHA1Hash
+        from dda.utils.git.commit import Commit
 
-        sha1_str = self.capture(["rev-parse", "HEAD"]).strip()
-        sha1 = SHA1Hash(sha1_str)
+        sha1 = self.capture(["rev-parse", "HEAD"]).strip()
 
         # Get the org/repo from the remote URL
         org, repo, _ = self.get_remote_details()
         return Commit(org=org, repo=repo, sha1=sha1)
 
-    def get_commit_details(self, sha1: SHA1Hash) -> CommitDetails:
+    def get_commit_details(self, sha1: str) -> CommitDetails:
         """
         Get the details of the given commit in the Git repository in the current working directory.
         """
         from datetime import datetime
 
-        from dda.utils.git.commit import CommitDetails, SHA1Hash
+        from dda.utils.git.commit import CommitDetails
 
         raw_details = self.capture([
             "show",
@@ -119,7 +121,7 @@ def get_commit_details(self, sha1: SHA1Hash) -> CommitDetails:
             # fmt: author name, author email, author date, parent SHAs, commit message body
             "--format=%an%n%ae%n%ad%n%P%n%B",
             "--date=iso-strict",
-            str(sha1),
+            sha1,
         ])
         author_name, author_email, date_str, parents_str, *message_lines = raw_details.splitlines()
 
@@ -128,31 +130,37 @@ def get_commit_details(self, sha1: SHA1Hash) -> CommitDetails:
             author_email=author_email,
             datetime=datetime.fromisoformat(date_str),
             message="\n".join(message_lines).strip().strip('"'),
-            parent_shas=[SHA1Hash(parent_sha) for parent_sha in parents_str.split()],
+            parent_shas=list(parents_str.split()),
         )
 
     def _capture_diff_lines(self, *args: str) -> list[str]:
-        diff_args = ["diff", "-U0", "--no-color", "--no-prefix", "--no-renames"]
+        diff_args = [
+            "-c",
+            "core.quotepath=false",
+            "diff",
+            "-U0",
+            "--no-color",
+            "--no-prefix",
+            "--no-renames",
+            "--no-ext-diff",
+            # "-z",
+        ]
         return self.capture([*diff_args, *args], check=False).strip().splitlines()
 
     def _compare_refs(self, ref1: str, ref2: str) -> ChangeSet:
-        return ChangeSet.generate_from_diff_output(self._capture_diff_lines(str(ref1), str(ref2)))
+        return ChangeSet.generate_from_diff_output(self._capture_diff_lines(ref1, ref2))
 
-    def get_commit_changes(self, sha1: SHA1Hash) -> ChangeSet:
+    def get_commit_changes(self, sha1: str) -> ChangeSet:
         """
         Get the changes of the given commit in the Git repository in the current working directory.
         """
-        return self._compare_refs(f"{sha1}^", str(sha1))
+        return self._compare_refs(f"{sha1}^", sha1)
 
-    def get_changes_between_commits(self, a: SHA1Hash | Commit, b: SHA1Hash | Commit) -> ChangeSet:
+    def get_changes_between_commits(self, a: str, b: str) -> ChangeSet:
         """
-        Get the changes between two commits.
+        Get the changes between two commits, identified by their SHA-1 hashes.
         """
-        if isinstance(a, Commit):
-            a = a.sha1
-        if isinstance(b, Commit):
-            b = b.sha1
-        return self._compare_refs(str(a), str(b))
+        return self._compare_refs(a, b)
 
     def get_working_tree_changes(self) -> ChangeSet:
         """
@@ -166,26 +174,31 @@ def get_working_tree_changes(self) -> ChangeSet:
         tracked_changes = ChangeSet.generate_from_diff_output(self._capture_diff_lines("HEAD"))
 
         # Capture changes to untracked files
-        untracked_files = self.capture(["ls-files", "--others", "--exclude-standard"]).strip().splitlines()
+        other_files_output = self.capture(["ls-files", "--others", "--exclude-standard", "-z"]).strip()
+        untracked_files = [x.strip() for x in other_files_output.split("\0") if x]  # Remove empty strings
+
+        if not untracked_files:
+            return tracked_changes
+
         diffs = list(chain.from_iterable(self._capture_diff_lines("/dev/null", file) for file in untracked_files))
         untracked_changes = ChangeSet.generate_from_diff_output(diffs)
 
         # Combine the changes
         return tracked_changes | untracked_changes
 
-    def get_merge_base(self, remote_name: str = "origin") -> SHA1Hash | str:
+    def get_merge_base(self, remote_name: str = "origin") -> str:
         """
         Get the merge base of the current branch.
         """
-        res = self.capture(["merge-base", "HEAD", remote_name]).strip()
+        res = self.capture(["merge-base", "HEAD", remote_name], check=False).strip()
         if not res:
             self.app.display_warning("Could not determine merge base for current branch. Using `main` instead.")
             return "main"
-        return SHA1Hash(res)
+        return res
 
     def get_changes_with_base(
         self,
-        base: SHA1Hash | Commit | str | None = None,
+        base: str | None = None,
         *,
         include_working_tree: bool = True,
         remote_name: str = "origin",
@@ -200,12 +213,9 @@ def get_changes_with_base(
         """
         if base is None:
             base = self.get_merge_base(remote_name)
-        if isinstance(base, Commit):
-            base = base.sha1
-        base = str(base)
 
         head = self.get_head_commit()
-        changes = self._compare_refs(base, head.sha1)
+        changes = ChangeSet.generate_from_diff_output(self._capture_diff_lines(base, "...", head.sha1))
         if include_working_tree:
             changes |= self.get_working_tree_changes()
         return changes

src/dda/utils/git/changeset.py

@@ -11,7 +11,6 @@
 from msgspec import Struct, field
 
 from dda.utils.fs import Path
-from dda.utils.git.commit import SHA1Hash
 
 if TYPE_CHECKING:
     from _collections_abc import dict_items, dict_keys, dict_values
@@ -52,10 +51,10 @@ class FileChanges(Struct, frozen=True):
 
     Example:
     ```diff
-    @@ -15,2 +15 @@ if TYPE_CHECKING:
-    -    from dda.utils.git.commit import Commit, CommitDetails
-    -    from dda.utils.git.commit import SHA1Hash
-    +    from dda.utils.git.commit import Commit, CommitDetails, SHA1Hash
+    @@ -15,1 +15 @@ if TYPE_CHECKING:
+    -    from dda.utils.git.commit import Commit
+    -    from dda.utils.git.commit import CommitDetails
+    +    from dda.utils.git.commit import Commit, CommitDetails
     ```
     """
 
@@ -239,7 +238,7 @@ def changed(self) -> set[Path]:
         return set(self.keys())
 
     # == methods == #
-    def digest(self) -> SHA1Hash:
+    def digest(self) -> str:
         """Compute a hash of the changeset."""
         from hashlib import sha1
 
@@ -249,7 +248,7 @@ def digest(self) -> SHA1Hash:
             digester.update(change.type.value.encode())
             digester.update(change.patch.encode())
 
-        return SHA1Hash(digester.hexdigest())
+        return str(digester.hexdigest())
 
     @classmethod
     def from_iter(cls, data: Iterable[FileChanges]) -> Self:

src/dda/utils/git/commit.py

@@ -3,9 +3,10 @@
 # SPDX-License-Identifier: MIT
 from __future__ import annotations
 
-from dataclasses import dataclass, field
 from functools import cached_property
-from typing import TYPE_CHECKING, Self
+from typing import TYPE_CHECKING
+
+from msgspec import Struct, field
 
 if TYPE_CHECKING:
     from datetime import datetime
@@ -14,21 +15,30 @@
     from dda.utils.git.changeset import ChangeSet
 
 
-@dataclass
-class Commit:
+class Commit(Struct, dict=True):
     """
     A Git commit, identified by its SHA-1 hash.
     """
 
     org: str
     repo: str
-    sha1: SHA1Hash
+    sha1: str
 
-    _details: CommitDetails | None = field(default=None, init=False)
+    _details: CommitDetails | None = field(default=None)
     _changes: ChangeSet | None = field(default=None)
 
+    def __post_init__(self) -> None:
+        if len(self.sha1) != 40:  # noqa: PLR2004
+            msg = "SHA-1 hash must be 40 characters long"
+            raise ValueError(msg)
+        for c in self.sha1:
+            code = ord(c)
+            if code not in range(48, 58) and code not in range(97, 103):
+                msg = "SHA-1 hash must contain only hexadecimal characters"
+                raise ValueError(msg)
+
     def __str__(self) -> str:
-        return str(self.sha1)
+        return self.sha1
 
     @property
     def full_repo(self) -> str:
@@ -53,7 +63,7 @@ def compare_to(self, app: Application, other: Commit) -> ChangeSet:
         """
         Compare this commit to another commit.
         """
-        return app.tools.git.get_changes_between_commits(self, other)
+        return app.tools.git.get_changes_between_commits(self.sha1, other.sha1)
 
     def get_details_and_changes_from_github(self) -> tuple[CommitDetails, ChangeSet]:
         """
@@ -88,7 +98,7 @@ def get_details_and_changes_from_github(self) -> tuple[CommitDetails, ChangeSet]
             author_email=data["commit"]["author"]["email"],
             datetime=datetime.fromisoformat(data["commit"]["author"]["date"]),
             message=data["commit"]["message"],
-            parent_shas=[SHA1Hash(parent["sha"]) for parent in data.get("parents", [])],
+            parent_shas=[parent["sha"] for parent in data.get("parents", [])],
         )
 
         return self.details, self.changes
@@ -146,35 +156,13 @@ def message(self) -> str:
         return self.details.message
 
     @property
-    def parent_shas(self) -> list[SHA1Hash]:
+    def parent_shas(self) -> list[str]:
         return self.details.parent_shas
 
 
-@dataclass
-class CommitDetails:
+class CommitDetails(Struct):
     author_name: str
     author_email: str
     datetime: datetime
     message: str
-    parent_shas: list[SHA1Hash]
-
-
-class SHA1Hash(str):
-    """
-    A hexadecimal representation of a SHA-1 hash.
-    """
-
-    LENGTH = 40
-    __slots__ = ()
-
-    def __new__(cls, value: str) -> Self:
-        if len(value) != cls.LENGTH or any(c not in "0123456789abcdef" for c in value.lower()):
-            msg = f"Invalid SHA-1 hash: {value}"
-            raise ValueError(msg)
-        return str.__new__(cls, value)
-
-    def __repr__(self) -> str:
-        return f"{self.__class__.__name__}({super().__repr__()})"
-
-    def __bytes__(self) -> bytes:
-        return bytes.fromhex(self)
+    parent_shas: list[str]