From 2f89de1ff577e9ee3fa9e74948559d4b75788b8d Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Tue, 16 Dec 2025 13:17:04 +0100 Subject: [PATCH] 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 | 49 ++++---- src/pkgmgr/core/git/commands/__init__.py | 3 + src/pkgmgr/core/git/commands/pull_args.py | 35 ++++++ .../repos/test_pull_with_verification.py | 114 ++++++++++-------- 4 files changed, 121 insertions(+), 80 deletions(-) create mode 100644 src/pkgmgr/core/git/commands/pull_args.py diff --git a/src/pkgmgr/actions/repository/pull.py b/src/pkgmgr/actions/repository/pull.py index fb825d2..cc35a3a 100644 --- a/src/pkgmgr/actions/repository/pull.py +++ b/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) diff --git a/src/pkgmgr/core/git/commands/__init__.py b/src/pkgmgr/core/git/commands/__init__.py index cf8aab4..7cd37aa 100644 --- a/src/pkgmgr/core/git/commands/__init__.py +++ b/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", diff --git a/src/pkgmgr/core/git/commands/pull_args.py b/src/pkgmgr/core/git/commands/pull_args.py new file mode 100644 index 0000000..b09e9fb --- /dev/null +++ b/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 diff --git a/tests/unit/pkgmgr/actions/repos/test_pull_with_verification.py b/tests/unit/pkgmgr/actions/repos/test_pull_with_verification.py index 0ddfb3b..5113fd5 100644 --- a/tests/unit/pkgmgr/actions/repos/test_pull_with_verification.py +++ b/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, + )