Closes #1466: Fix error when snapshotting non-repo config, and improve config documentation

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 <Config> 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 <Config> 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 <Config> 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)
 
 

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).
         """

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'),
+    )