From 471d63ed235e20c6cded64a491f18447ae5fcb57 Mon Sep 17 00:00:00 2001 From: Nick Williams Date: Thu, 25 Jun 2026 19:20:58 -0500 Subject: [PATCH] Fix error when snapshotting non-repo config, and improve config docs - Most of this change is documentation improvement for the `Config` class and some of its methods. - Calling `Config.snapshot()` previously raised an attribute error if the `Config` did not come from a repository (i.e. the system or global config or a config loaded from a file). This is fixed by ensuring that `self._repo` is always set to something (`None` by default). - Passing a `PathLike` into `Config.__init__()` resulted in a value error unneccesarily. This is supported now. - Added tests that confirm this behavior. --- pygit2/config.py | 76 ++++++++++++++++++++++++++++++++++---------- pygit2/repository.py | 2 +- test/test_config.py | 74 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 127 insertions(+), 25 deletions(-) diff --git a/pygit2/config.py b/pygit2/config.py index 5ffb6896..e7522ffe 100644 --- a/pygit2/config.py +++ b/pygit2/config.py @@ -25,8 +25,7 @@ from collections.abc import Callable, Iterator from os import PathLike -from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Optional try: from functools import cached_property @@ -43,7 +42,7 @@ from .repository import BaseRepository -def str_to_bytes(value: str | PathLike[str] | bytes, name: str) -> bytes: +def str_to_bytes(value: str | bytes, name: str) -> bytes: if not isinstance(value, str): raise TypeError(f'{name} must be a string') @@ -79,25 +78,53 @@ def __next__(self) -> str | None: # type: ignore[override] class Config: - """Git configuration management.""" - - _repo: 'BaseRepository' + """Git configuration management. + + This class is for the reading and writing of Git configuration files. + Configuration files are read individually, either by passing a path into + the constructor or by using one of the static methods + :meth:`Config.get_system_config`, :meth:`Config.get_global_config`, or + :meth:`Config.get_xdg_config`. Additional files can be loaded into the + `Config` object using :meth:`Config.add_file`. + + Changes made to the configuration with :meth:`Config.set_multivar` are + immediately persisted to disk. Reads performed with accessor methods like + :meth:`Config.get_multivar` or :meth:`Config.__getitem__` may result in + reading from different versions of the configuration file if this or + another process has modified the file. To avoid this and have all read + operations occur against the same version of the configuration file, use + :meth:`Config.snapshot()` to create a snapshot of the current config. This + is especially important when iterating, as the contents of the config + might change mid-iteration if you don't use a snapshot. + + This class can technically be used to manually read and write a repository's + configuration file by pointing the constructor to the repository's + ``.git/config`` file, but this is not recommended. The resulting ``Config`` + object represents only the configuration directly within ``.git/config``. + It does not represent the total effective configuration for that repository + that includes the combined system, global (user), and global (user) XDG. + Instead, use :meth:`BaseRepository.config` for loading a repository's + configuration. + """ + + _repo: Optional['BaseRepository'] _config: 'GitConfigC' - def __init__(self, path: str | None = None) -> None: + def __init__(self, path: PathLike | str | None = None) -> None: cconfig = ffi.new('git_config **') if not path: err = C.git_config_new(cconfig) else: - path_bytes = str_to_bytes(path, 'path') + path_bytes = to_bytes(path) err = C.git_config_open_ondisk(cconfig, path_bytes) check_error(err, io=True) + self._repo = None self._config = cconfig[0] @classmethod - def from_c(cls, repo: 'BaseRepository', ptr: 'GitConfigC') -> 'Config': + def from_c(cls, repo: Optional['BaseRepository'], ptr: 'GitConfigC') -> 'Config': config = cls.__new__(cls) config._repo = repo config._config = ptr @@ -200,7 +227,8 @@ def set_multivar( self, name: str | bytes, regex: str | bytes, value: str | bytes ) -> None: """Set a multivar ''name'' to ''value''. ''regexp'' is a regular - expression to indicate which values to replace. + expression to indicate which values to replace. Changes are persisted + to the configuration file(s) backing this ``Config``. """ name = str_to_bytes(name, 'name') regex = str_to_bytes(regex, 'regex') @@ -211,7 +239,8 @@ def set_multivar( def delete_multivar(self, name: str | bytes, regex: str | bytes) -> None: """Delete a multivar ''name''. ''regexp'' is a regular expression to - indicate which values to delete. + indicate which values to delete. Changes are persisted to the + configuration file(s) backing this ``Config``. """ name = str_to_bytes(name, 'name') regex = str_to_bytes(regex, 'regex') @@ -249,7 +278,7 @@ def get_int(self, key: bytes | str) -> int: return res[0] - def add_file(self, path: str | Path, level: int = 0, force: int = 0) -> None: + def add_file(self, path: str | PathLike, level: int = 0, force: int = 0) -> None: """Add a config file instance to an existing config.""" err = C.git_config_add_file_ondisk( @@ -258,7 +287,7 @@ def add_file(self, path: str | Path, level: int = 0, force: int = 0) -> None: check_error(err) def snapshot(self) -> 'Config': - """Create a snapshot from this Config object. + """Create a snapshot from this ``Config`` object. This means that looking up multiple values will use the same version of the configuration files. @@ -305,17 +334,32 @@ def _from_found_config(fn: Callable) -> 'Config': @staticmethod def get_system_config() -> 'Config': - """Return a object representing the system configuration file.""" + """Return a ``Config`` object representing the system configuration file. + + The system configuration file is the one found at ``/etc/gitconfig`` or + ``%PROGRAMFILES%\\Git\\etc\\gitconfig``, depending on the operating system. + """ return Config._from_found_config(C.git_config_find_system) @staticmethod def get_global_config() -> 'Config': - """Return a object representing the global configuration file.""" + """Return a ``Config`` object representing the global configuration file. + + The global configuration file is the one found at the standard user config + location for Git, which is ``$HOME/.gitconfig``. This will not find the file + at the XDG-compatible user config file location (for that, see + :meth:`Config.get_xdg_config`). + """ return Config._from_found_config(C.git_config_find_global) @staticmethod def get_xdg_config() -> 'Config': - """Return a object representing the global configuration file.""" + """Return a ``Config`` object representing the XDG-compatible global configuration file. + + The XDG-compatible user config file follows the XDG Base Directory Specification. + This file is located at ``$HOME/.config/git/config``. This will not find the file + at the standard user config location (for that, see :meth:`Config.get_global_config`). + """ return Config._from_found_config(C.git_config_find_xdg) diff --git a/pygit2/repository.py b/pygit2/repository.py index e90ac0ad..f3c34e81 100644 --- a/pygit2/repository.py +++ b/pygit2/repository.py @@ -291,7 +291,7 @@ def __repr__(self) -> str: def config(self) -> Config: """The configuration file for this repository. - If a the configuration hasn't been set yet, the default config for + If the configuration hasn't been set yet, the default config for repository will be returned, including global and system configurations (if they are available). """ diff --git a/test/test_config.py b/test/test_config.py index aacfcf6e..88c7d0c4 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -28,7 +28,7 @@ import pytest -from pygit2 import Config, Repository +from pygit2 import Config, Repository, Settings from . import utils @@ -39,7 +39,7 @@ def config_path(tmp_path: Path) -> Path: @pytest.fixture -def config(testrepo: Repository) -> Generator[object, None, None]: +def config(testrepo: Repository) -> Generator[Config, None, None]: yield testrepo.config @@ -50,17 +50,16 @@ def test_config(config: Config) -> None: def test_global_config() -> None: try: assert Config.get_global_config() is not None - except IOError: - # There is no user config - pass + except IOError as e: + settings = Settings() + pytest.skip(f'Unavailable for testing with home dir = {settings.homedir}: {e}') def test_system_config() -> None: try: assert Config.get_system_config() is not None - except IOError: - # There is no system config - pass + except IOError as e: + pytest.skip(f'Unavailable for testing: {e}') def test_new(config_path: Path) -> None: @@ -219,3 +218,62 @@ def test_parsing() -> None: assert 5 == Config.parse_int('5') assert 1024 == Config.parse_int('1k') + + +def test_repository_config_snapshot(config: Config) -> None: + assert 'core.bare' in config + assert not config.get_bool('core.bare') + assert 'core.editor' in config + assert config['core.editor'] == 'ed' + assert 'core.repositoryformatversion' in config + assert config.get_int('core.repositoryformatversion') == 0 + + snapshot = config.snapshot() + assert 'core.bare' in snapshot + assert not snapshot.get_bool('core.bare') + assert 'core.editor' in snapshot + assert snapshot['core.editor'] == 'ed' + assert 'core.repositoryformatversion' in snapshot + assert snapshot.get_int('core.repositoryformatversion') == 0 + + assert 'core.snapshot1' not in config + assert 'core.snapshot1' not in snapshot + config['core.snapshot1'] = 42 + assert 'core.snapshot1' in config + assert 'core.snapshot1' not in snapshot + assert config.get_int('core.snapshot1') == 42 + utils.assertRaisesWithArg( + KeyError, + 'core.snapshot1', + lambda: snapshot.get_int('core.snapshot1'), + ) + + +def test_non_repository_config_snapshot(config_path: Path) -> None: + with config_path.open('w') as new_file: + new_file.write('[this]\n\tthat = true\n') + new_file.write('[something "other"]\n\there = false') + + config = Config(config_path) + assert 'this.that' in config + assert config.get_bool('this.that') + assert 'something.other.here' in config + assert not config.get_bool('something.other.here') + + snapshot = config.snapshot() + assert 'this.that' in snapshot + assert snapshot.get_bool('this.that') + assert 'something.other.here' in snapshot + assert not snapshot.get_bool('something.other.here') + + assert 'this.snapshot1' not in config + assert 'this.snapshot1' not in snapshot + config['this.snapshot1'] = 42 + assert 'this.snapshot1' in config + assert 'this.snapshot1' not in snapshot + assert config.get_int('this.snapshot1') == 42 + utils.assertRaisesWithArg( + KeyError, + 'this.snapshot1', + lambda: snapshot.get_int('this.snapshot1'), + )