refactor(pull): switch repository pull to core.git commands

- Replace raw subprocess git pull with core.git.commands.pull_args
- Remove shell-based command execution
- Add GitPullArgsError wrapper for consistent error handling
- Align unit tests to mock pull_args instead of subprocess.run
- Preserve verification and prompt logic

https://chatgpt.com/share/69414dc9-5b30-800f-88b2-bd27a873580b

src/pkgmgr/actions/repository/pull.py

@@ -1,25 +1,30 @@
-#!/usr/bin/env python3
-# -*- coding: utf-8 -*-
+from __future__ import annotations
 
 import os
-import subprocess
 import sys
+from typing import List, Dict, Any
 
+from pkgmgr.core.git.commands import pull_args, GitPullArgsError
 from pkgmgr.core.repository.dir import get_repo_dir
 from pkgmgr.core.repository.identifier import get_repo_identifier
 from pkgmgr.core.repository.verify import verify_repository
 
+Repository = Dict[str, Any]
+
 
 def pull_with_verification(
-    selected_repos,
-    repositories_base_dir,
-    all_repos,
-    extra_args,
-    no_verification,
+    selected_repos: List[Repository],
+    repositories_base_dir: str,
+    all_repos: List[Repository],
+    extra_args: List[str],
+    no_verification: bool,
     preview: bool,
 ) -> None:
     """
     Execute `git pull` for each repository with verification.
+
+    - If verification fails and verification is enabled, prompt user to continue.
+    - Uses core.git.commands.pull_args() (no raw subprocess usage).
     """
     for repo in selected_repos:
         repo_identifier = get_repo_identifier(repo, all_repos)
@@ -37,12 +42,7 @@ def pull_with_verification(
             no_verification=no_verification,
         )
 
-        if (
-            not preview
-            and not no_verification
-            and verified_info
-            and not verified_ok
-        ):
+        if not preview and not no_verification and verified_info and not verified_ok:
             print(f"Warning: Verification failed for {repo_identifier}:")
             for err in errors:
                 print(f"  - {err}")
@@ -50,17 +50,10 @@ def pull_with_verification(
             if choice != "y":
                 continue
 
-        args_part = " ".join(extra_args) if extra_args else ""
-        full_cmd = f"git pull{(' ' + args_part) if args_part else ''}"
-
-        if preview:
-            print(f"[Preview] In '{repo_dir}': {full_cmd}")
-        else:
-            print(f"Running in '{repo_dir}': {full_cmd}")
-            result = subprocess.run(full_cmd, cwd=repo_dir, shell=True, check=False)
-            if result.returncode != 0:
-                print(
-                    f"'git pull' for {repo_identifier} failed "
-                    f"with exit code {result.returncode}."
-                )
-                sys.exit(result.returncode)
+        try:
+            pull_args(extra_args, cwd=repo_dir, preview=preview)
+        except GitPullArgsError as exc:
+            # Keep behavior consistent with previous implementation:
+            # stop on first failure and propagate return code as generic failure.
+            print(str(exc))
+            sys.exit(1)

src/pkgmgr/core/git/commands/__init__.py

@@ -16,6 +16,7 @@ from .fetch import GitFetchError, fetch
 from .init import GitInitError, init
 from .merge_no_ff import GitMergeError, merge_no_ff
 from .pull import GitPullError, pull
+from .pull_args import GitPullArgsError, pull_args  # <-- add
 from .pull_ff_only import GitPullFfOnlyError, pull_ff_only
 from .push import GitPushError, push
 from .push_upstream import GitPushUpstreamError, push_upstream
@@ -29,6 +30,7 @@ __all__ = [
     "fetch",
     "checkout",
     "pull",
+    "pull_args",  # <-- add
     "pull_ff_only",
     "merge_no_ff",
     "push",
@@ -50,6 +52,7 @@ __all__ = [
     "GitFetchError",
     "GitCheckoutError",
     "GitPullError",
+    "GitPullArgsError",  # <-- add
     "GitPullFfOnlyError",
     "GitMergeError",
     "GitPushError",

src/pkgmgr/core/git/commands/pull_args.py

@@ -0,0 +1,35 @@
+from __future__ import annotations
+
+from typing import List
+
+from ..errors import GitError, GitCommandError
+from ..run import run
+
+
+class GitPullArgsError(GitCommandError):
+    """Raised when `git pull` with arbitrary args fails."""
+
+
+def pull_args(
+    args: List[str] | None = None,
+    *,
+    cwd: str = ".",
+    preview: bool = False,
+) -> None:
+    """
+    Execute `git pull` with caller-provided arguments.
+
+    Examples:
+      []                          -> git pull
+      ["--ff-only"]               -> git pull --ff-only
+      ["--rebase"]                -> git pull --rebase
+      ["origin", "main"]          -> git pull origin main
+    """
+    extra = args or []
+    try:
+        run(["pull", *extra], cwd=cwd, preview=preview)
+    except GitError as exc:
+        raise GitPullArgsError(
+            f"Failed to run `git pull` with args={extra!r}.",
+            cwd=cwd,
+        ) from exc

tests/unit/pkgmgr/actions/repos/test_pull_with_verification.py

@@ -1,6 +1,6 @@
 import io
 import unittest
-from unittest.mock import patch, MagicMock
+from unittest.mock import patch
 
 from pkgmgr.actions.repository.pull import pull_with_verification
 
@@ -12,14 +12,23 @@ class TestPullWithVerification(unittest.TestCase):
     These tests verify:
       - Preview mode behaviour
       - Verification logic (prompting, bypassing, skipping)
-      - subprocess.run invocation
+      - pull_args invocation (instead of subprocess.run)
       - Repository directory existence checks
       - Handling of extra git pull arguments
     """
 
-    def _setup_mocks(self, mock_exists, mock_get_repo_id, mock_get_repo_dir,
-                     mock_verify, exists=True, verified_ok=True,
-                     errors=None, verified_info=True):
+    def _setup_mocks(
+        self,
+        mock_exists,
+        mock_get_repo_id,
+        mock_get_repo_dir,
+        mock_verify,
+        *,
+        exists: bool = True,
+        verified_ok: bool = True,
+        errors=None,
+        verified_info: bool = True,
+    ):
         """Helper to configure repetitive mock behavior."""
         repo = {
             "name": "pkgmgr",
@@ -31,13 +40,13 @@ class TestPullWithVerification(unittest.TestCase):
         mock_verify.return_value = (
             verified_ok,
             errors or [],
-            "deadbeef",    # commit hash
-            "ABCDEF",      # signing key
+            "deadbeef",  # commit hash
+            "ABCDEF",  # signing key
         )
         return repo
 
     # ---------------------------------------------------------------------
-    @patch("pkgmgr.actions.repository.pull.subprocess.run")
+    @patch("pkgmgr.actions.repository.pull.pull_args")
     @patch("pkgmgr.actions.repository.pull.verify_repository")
     @patch("pkgmgr.actions.repository.pull.get_repo_dir")
     @patch("pkgmgr.actions.repository.pull.get_repo_identifier")
@@ -50,11 +59,11 @@ class TestPullWithVerification(unittest.TestCase):
         mock_get_repo_id,
         mock_get_repo_dir,
         mock_verify,
-        mock_subprocess,
+        mock_pull_args,
     ):
         """
-        Preview mode must NEVER request user input and must NEVER execute git.
-        It must only print the preview command.
+        Preview mode must NEVER request user input and must still call pull_args
+        in preview mode (which prints the preview command via core.git.run()).
         """
         repo = self._setup_mocks(
             mock_exists,
@@ -78,17 +87,15 @@ class TestPullWithVerification(unittest.TestCase):
                 preview=True,
             )
 
-        output = buf.getvalue()
-        self.assertIn(
-            "[Preview] In '/fake/base/pkgmgr': git pull --ff-only",
-            output,
+        mock_input.assert_not_called()
+        mock_pull_args.assert_called_once_with(
+            ["--ff-only"],
+            cwd="/fake/base/pkgmgr",
+            preview=True,
         )
 
-        mock_input.assert_not_called()
-        mock_subprocess.assert_not_called()
-
     # ---------------------------------------------------------------------
-    @patch("pkgmgr.actions.repository.pull.subprocess.run")
+    @patch("pkgmgr.actions.repository.pull.pull_args")
     @patch("pkgmgr.actions.repository.pull.verify_repository")
     @patch("pkgmgr.actions.repository.pull.get_repo_dir")
     @patch("pkgmgr.actions.repository.pull.get_repo_identifier")
@@ -101,7 +108,7 @@ class TestPullWithVerification(unittest.TestCase):
         mock_get_repo_id,
         mock_get_repo_dir,
         mock_verify,
-        mock_subprocess,
+        mock_pull_args,
     ):
         """
         If verification fails and preview=False, the user is prompted.
@@ -118,22 +125,20 @@ class TestPullWithVerification(unittest.TestCase):
 
         mock_input.return_value = "n"
 
-        buf = io.StringIO()
-        with patch("sys.stdout", new=buf):
-            pull_with_verification(
-                selected_repos=[repo],
-                repositories_base_dir="/fake/base",
-                all_repos=[repo],
-                extra_args=[],
-                no_verification=False,
-                preview=False,
-            )
+        pull_with_verification(
+            selected_repos=[repo],
+            repositories_base_dir="/fake/base",
+            all_repos=[repo],
+            extra_args=[],
+            no_verification=False,
+            preview=False,
+        )
 
         mock_input.assert_called_once()
-        mock_subprocess.assert_not_called()
+        mock_pull_args.assert_not_called()
 
     # ---------------------------------------------------------------------
-    @patch("pkgmgr.actions.repository.pull.subprocess.run")
+    @patch("pkgmgr.actions.repository.pull.pull_args")
     @patch("pkgmgr.actions.repository.pull.verify_repository")
     @patch("pkgmgr.actions.repository.pull.get_repo_dir")
     @patch("pkgmgr.actions.repository.pull.get_repo_identifier")
@@ -146,11 +151,11 @@ class TestPullWithVerification(unittest.TestCase):
         mock_get_repo_id,
         mock_get_repo_dir,
         mock_verify,
-        mock_subprocess,
+        mock_pull_args,
     ):
         """
         If verification fails and the user accepts ('y'),
-        then the git pull should be executed.
+        then the git pull should be executed via pull_args.
         """
         repo = self._setup_mocks(
             mock_exists,
@@ -162,7 +167,6 @@ class TestPullWithVerification(unittest.TestCase):
         )
 
         mock_input.return_value = "y"
-        mock_subprocess.return_value = MagicMock(returncode=0)
 
         pull_with_verification(
             selected_repos=[repo],
@@ -173,11 +177,15 @@ class TestPullWithVerification(unittest.TestCase):
             preview=False,
         )
 
-        mock_subprocess.assert_called_once()
         mock_input.assert_called_once()
+        mock_pull_args.assert_called_once_with(
+            [],
+            cwd="/fake/base/pkgmgr",
+            preview=False,
+        )
 
     # ---------------------------------------------------------------------
-    @patch("pkgmgr.actions.repository.pull.subprocess.run")
+    @patch("pkgmgr.actions.repository.pull.pull_args")
     @patch("pkgmgr.actions.repository.pull.verify_repository")
     @patch("pkgmgr.actions.repository.pull.get_repo_dir")
     @patch("pkgmgr.actions.repository.pull.get_repo_identifier")
@@ -190,7 +198,7 @@ class TestPullWithVerification(unittest.TestCase):
         mock_get_repo_id,
         mock_get_repo_dir,
         mock_verify,
-        mock_subprocess,
+        mock_pull_args,
     ):
         """
         If verification is successful, the user should NOT be prompted,
@@ -204,8 +212,6 @@ class TestPullWithVerification(unittest.TestCase):
             verified_ok=True,
         )
 
-        mock_subprocess.return_value = MagicMock(returncode=0)
-
         pull_with_verification(
             selected_repos=[repo],
             repositories_base_dir="/fake/base",
@@ -216,12 +222,14 @@ class TestPullWithVerification(unittest.TestCase):
         )
 
         mock_input.assert_not_called()
-        mock_subprocess.assert_called_once()
-        cmd = mock_subprocess.call_args[0][0]
-        self.assertIn("git pull --rebase", cmd)
+        mock_pull_args.assert_called_once_with(
+            ["--rebase"],
+            cwd="/fake/base/pkgmgr",
+            preview=False,
+        )
 
     # ---------------------------------------------------------------------
-    @patch("pkgmgr.actions.repository.pull.subprocess.run")
+    @patch("pkgmgr.actions.repository.pull.pull_args")
     @patch("pkgmgr.actions.repository.pull.verify_repository")
     @patch("pkgmgr.actions.repository.pull.get_repo_dir")
     @patch("pkgmgr.actions.repository.pull.get_repo_identifier")
@@ -234,11 +242,11 @@ class TestPullWithVerification(unittest.TestCase):
         mock_get_repo_id,
         mock_get_repo_dir,
         mock_verify,
-        mock_subprocess,
+        mock_pull_args,
     ):
         """
         If the repository directory does not exist, the repo must be skipped
-        silently and no git command executed.
+        and no git command executed.
         """
         repo = self._setup_mocks(
             mock_exists,
@@ -263,10 +271,10 @@ class TestPullWithVerification(unittest.TestCase):
         self.assertIn("not found", output)
 
         mock_input.assert_not_called()
-        mock_subprocess.assert_not_called()
+        mock_pull_args.assert_not_called()
 
     # ---------------------------------------------------------------------
-    @patch("pkgmgr.actions.repository.pull.subprocess.run")
+    @patch("pkgmgr.actions.repository.pull.pull_args")
     @patch("pkgmgr.actions.repository.pull.verify_repository")
     @patch("pkgmgr.actions.repository.pull.get_repo_dir")
     @patch("pkgmgr.actions.repository.pull.get_repo_identifier")
@@ -279,7 +287,7 @@ class TestPullWithVerification(unittest.TestCase):
         mock_get_repo_id,
         mock_get_repo_dir,
         mock_verify,
-        mock_subprocess,
+        mock_pull_args,
     ):
         """
         If no_verification=True, verification failures must NOT prompt.
@@ -294,8 +302,6 @@ class TestPullWithVerification(unittest.TestCase):
             errors=["invalid"],
         )
 
-        mock_subprocess.return_value = MagicMock(returncode=0)
-
         pull_with_verification(
             selected_repos=[repo],
             repositories_base_dir="/fake/base",
@@ -306,4 +312,8 @@ class TestPullWithVerification(unittest.TestCase):
         )
 
         mock_input.assert_not_called()
-        mock_subprocess.assert_called_once()
+        mock_pull_args.assert_called_once_with(
+            [],
+            cwd="/fake/base/pkgmgr",
+            preview=False,
+        )