Merge branch 'utils-git' - close_batch_command(), docstrings

* utils-git:
  utils.git: Document GitRepo attributes
  utils.git: Add close_batch_command() method to GitRepo class
  utils.git: Add missing param to filter_valid_commits() docstring

Author: jnareb

src/diffannotator/utils/git.py

@@ -23,6 +23,7 @@
 import os
 import re
 import subprocess
+import weakref
 from collections import defaultdict
 from contextlib import contextmanager
 from enum import Enum
@@ -506,8 +507,39 @@ def decode_c_quoted_str(text: str) -> str:
     return text
 
 
+def maybe_close_subprocess(process: Optional[subprocess.Popen]) -> None:
+    """Closes a subprocess safely to avoid resource warnings and resource starvation
+
+    This function ensures the safe termination of a subprocess by properly closing its
+    standard input and output streams, waiting for it to exit, and forcefully
+    killing it if necessary. It is designed to handle `git cat-file --batch-command`
+    and similar persistent subprocesses.
+
+    It is designed to be used as a ` weakref.finalize ` callback.
+
+    Parameters
+    ----------
+    process
+        The subprocess instance to close. If None, the function does nothing.
+    """
+    if process is None:
+        return
+
+    # closing stdin and stdout should end the persistent `git cat-file` process
+    process.stdout.close()  # to avoid ResourceWarning: unclosed file <_io.BufferedReader name=3>
+    process.stdin.close()  # just in case, see above
+    process.wait()  # to avoid ResourceWarning: subprocess NNN is still running
+    process.kill()  # just in case
+
+
 class GitRepo:
-    """Class representing Git repository, for performing operations on"""
+    """Class representing Git repository, for performing operations on
+
+    Attributes
+    ----------
+    repo : Path
+        stores Path to the Git repository
+    """
     path_encoding = 'utf8'
     default_file_encoding = 'utf8'
     log_encoding = 'utf8'
@@ -529,6 +561,8 @@ def __init__(self, repo_dir: PathLike):
         # TODO: check that `git_directory` is a path to git repository
         # TODO: remember absolute path (it is safer)
         self.repo = Path(repo_dir)
+        self._cat_file: Optional[subprocess.Popen] = None
+        self._finalizer = weakref.finalize(self, maybe_close_subprocess, self._cat_file)
 
     def __repr__(self):
         class_name = type(self).__name__
@@ -673,7 +707,7 @@ def batch_command(self) -> subprocess.Popen:
             in the `--batch-command` mode, buffered (because of `--buffer`),
             see https://git-scm.com/docs/git-cat-file
         """
-        return subprocess.Popen(
+        self._cat_file = subprocess.Popen(
             [
                 'git', '-C', str(self.repo),
                 'cat-file', '--batch-command', '--buffer',
@@ -684,6 +718,18 @@ def batch_command(self) -> subprocess.Popen:
             text=True,
             bufsize=1,  # line buffered
         )
+        return self._cat_file
+
+    def close_batch_command(self) -> None:
+        """Close persistent connection to `git cat-file --batch-command --buffer`
+
+        This should be done only when you are finished with using it, because
+        at least with the current implementation calling this method would make
+        `.are_valid_objects()` and `.filter_valid_commits()` methods fail;
+        they rely on persistence of the cached `.batch_command` property.
+        """
+        self._finalizer()
+        self._cat_file = None
 
     def format_patch(self,
                      output_dir: Optional[PathLike] = None,
@@ -1476,6 +1522,9 @@ def filter_valid_commits(self, commits: Iterable[str], to_oid: bool = False) ->
         ----------
         commits
              A list of commit identifiers to check
+        to_oid
+            Whether to convert elements in `commits` to SHA-1 object identifiers,
+            for example, "HEAD" to "3a27ee24b37a3e9572a0acc0aaecd22cc9c10bc7"
 
         Yields
         ------

tests/test_utils_git.py

@@ -2,11 +2,13 @@
 """Test cases for 'src/diffannotator/utils/git.py' module"""
 import subprocess
 import textwrap
+from typing import Optional
 
 import pytest
 from unidiff import PatchSet
 
-from diffannotator.utils.git import decode_c_quoted_str, GitRepo, DiffSide, AuthorStat, parse_shortlog_count, ChangeSet
+from diffannotator.utils.git import decode_c_quoted_str, GitRepo, DiffSide, AuthorStat, parse_shortlog_count, ChangeSet, \
+    maybe_close_subprocess
 from tests.conftest import default_branch, example_repo, example_repo_utf8
 
 
@@ -229,6 +231,45 @@ def test_is_valid_commit(example_repo):
     assert not example_repo.is_valid_commit("HEAD~20"), "HEAD~20 is invalid"
 
 
+def test_batch_command(example_repo):
+    """Test that the GitRepo.batch_command property behaves sanely"""
+    assert example_repo._cat_file is None, "the property is not initialized yet"
+
+    maybe_close_subprocess(example_repo._cat_file)  # no error
+
+    proc_1 = example_repo.batch_command
+    proc_2 = example_repo.batch_command
+    assert proc_1 is proc_2, ".batch_command property is cached"
+
+    proc: Optional[subprocess.Popen] = example_repo._cat_file
+    assert proc is not None, "process was generated by .batch_command, and is not None"
+    assert isinstance(proc, subprocess.Popen), "process is a Popen object"
+    assert proc.returncode is None, "the `git cat-file` didn't return (process is live)"
+
+    actual = example_repo.are_valid_objects(["HEAD"])
+    expected = [True]
+    assert actual == expected, ".are_valid_object('HEAD') returns True"
+
+    maybe_close_subprocess(example_repo._cat_file)  # no error
+
+    proc = example_repo.batch_command
+    assert isinstance(proc, subprocess.Popen), ".batch_command returns a Popen object"
+    assert proc.returncode == 0, "the `git cat-file` returns success (process ended)"
+
+    with pytest.raises(ValueError) as exc_info:
+        example_repo.are_valid_objects(["HEAD"])
+    assert "closed file" in str(exc_info.value), "the `git cat-file` process is closed"
+
+    example_repo.close_batch_command()  # no errors
+
+    #print(f"{example_repo._finalizer=}")
+    #print(f"{example_repo._finalizer.alive=}")
+    #print(f"{example_repo._finalizer.peek()=}")
+    #print(f"{example_repo._finalizer()=}")
+    #print(f"{example_repo._finalizer.detach()=}")
+    #print(f"{example_repo._finalizer.alive=}")
+
+
 def test_are_valid_objects(example_repo):
     """Test that GitRepo.are_valid_objects returns the correct answer"""
     actual = example_repo.are_valid_objects(['HEAD', 'v1', 'v2'], object_type='commit')