From 22296682d66fb0b2a76e549670d0ec6ef7447ab1 Mon Sep 17 00:00:00 2001 From: Byron Date: Sun, 13 Sep 2026 11:01:15 +0200 Subject: [PATCH] fix: preserve implicit boolean config keys (#2237) This was mostly a rubber-stamp, knowing the the whole implementation is quite a hack that is held together with ductape. Ideally, it will just work well enough at some point, to gain time for v4 to be made. GitConfigParser discarded keys written without an assignment. Reading such an option raised NoOptionError, and editing an unrelated setting silently removed it. Git treats a bare key as true but an explicitly empty value as false, so representing both as an empty string would lose their meaning. Store bare entries as None, following RawConfigParser's allow_no_value representation. Raw get/items access preserves the distinction, while get_value/get_values return an empty string as requested. Convert None to true and an empty string to false in getboolean, retaining the standard boolean spellings. Write None entries without an equals sign and exclude them from string validation and include-path expansion. The existing ordered multi-dict preserves repeated bare and assigned entries together. Update the regression that expected bare color.ui to disappear, and add a Git-backed read-modify-write test covering trailing whitespace, EOF without a newline, quoted and unquoted empty values, repeated keys, and a valueless non-path option in an include section. Both regressions failed before the fix. Compare Git's NUL-delimited listing before and after an unrelated edit and check the resulting repeated values with --type=bool --get-all. Git reference: checkout 1630431f326e15fcde608827b5ff38422528eb59, t/t1300-config.sh tests for novalue.variable and emptyvalue.variable, and parse.c:git_parse_maybe_bool_text. Runtime comparison used Git 2.50.1 (Apple Git-155). Assisted-by: GPT 6.0 Co-authored-by: GPT 6.0 --- git/config.py | 51 ++++++++++++++++++++++++++++++++---------- test/test_config.py | 54 +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/git/config.py b/git/config.py index 0da90bab2..d6f2706f1 100644 --- a/git/config.py +++ b/git/config.py @@ -45,7 +45,7 @@ from git.repo.base import Repo T_ConfigParser = TypeVar("T_ConfigParser", bound="GitConfigParser") -T_OMD_value = TypeVar("T_OMD_value", str, bytes, int, float, bool) +T_OMD_value = TypeVar("T_OMD_value", str, bytes, int, float, bool, None) if sys.version_info[:3] < (3, 7, 2): # typing.Ordereddict not added until Python 3.7.2. @@ -291,6 +291,12 @@ class GitConfigParser(cp.RawConfigParser, metaclass=MetaParserBuilder): :note: If used as a context manager, this will release the locked file. + + :note: + Options without a value are stored as ``None`` and written without ``=``. + :meth:`get_value` and :meth:`get_values` return an empty string for them, + while :meth:`getboolean` returns ``True``. An explicit empty value is + stored as an empty string and reads as ``False`` with :meth:`getboolean`. """ # { Configuration @@ -348,7 +354,7 @@ def __init__( Reference to repository to use if ``[includeIf]`` sections are found in configuration files. """ - cp.RawConfigParser.__init__(self, dict_type=_OMD) + cp.RawConfigParser.__init__(self, dict_type=_OMD, allow_no_value=True) self._dict: Callable[..., _OMD] self._defaults: _OMD self._sections: _OMD @@ -587,8 +593,12 @@ def parse_value(value: str) -> str: # Preserves multiple values for duplicate optnames. cursect.add(optname, optval) else: - # Check if it's an option with no value - it's just ignored by git. - if not self.OPTVALUEONLY.match(line): + # A valueless option is an implicit boolean true, not an empty value. + mo = self.OPTVALUEONLY.match(line) + if mo: + optname = self.optionxform(mo.group("option").rstrip()) + cursect.add(optname, None) + else: if not e: e = cp.ParsingError(fpname) e.append(lineno, repr(line)) @@ -625,6 +635,7 @@ def _all_items(section: str) -> List[Tuple[str, str]]: for key, values in self._sections[section].items_all() if key != "__name__" for value in values + if value is not None ] paths = [] @@ -760,13 +771,16 @@ def _write(self, fp: IO) -> None: def write_section(name: str, section_dict: _OMD) -> None: fp.write(("[%s]\n" % name).encode(defenc)) - values: Sequence[str] # Runtime only gets str in tests, but should be whatever _OMD stores. - v: str + values: List[Any] + v: Any for key, values in section_dict.items_all(): if key == "__name__": continue for v in values: + if v is None: + fp.write(("\t%s\n" % key).encode(defenc)) + continue value = self._value_to_string(v) if any(char in value for char in '\n\t\b\\"#;') or value[:1].isspace() or value[-1:].isspace(): value = value.replace("\\", "\\\\").replace('"', '\\"') @@ -783,11 +797,11 @@ def write_section(name: str, section_dict: _OMD) -> None: for name, value in self._sections.items(): write_section(name, value) - def items(self, section_name: str) -> List[Tuple[str, str]]: # type: ignore[override] + def items(self, section_name: str) -> List[Tuple[str, Union[str, None]]]: # type: ignore[override] """:return: list((option, value), ...) pairs of all items in the given section""" return [(k, v) for k, v in super().items(section_name) if k != "__name__"] - def items_all(self, section_name: str) -> List[Tuple[str, List[str]]]: + def items_all(self, section_name: str) -> List[Tuple[str, List[Union[str, None]]]]: """:return: list((option, [values...]), ...) pairs of all items in the given section""" rv = _OMD(self._defaults) @@ -841,6 +855,8 @@ def write(self) -> None: for key, values in section.items_all(): if key != "__name__": for raw_value in values: + if raw_value is None: + continue if "\r" in self._value_to_string(raw_value) or "\x00" in self._value_to_string(raw_value): raise ValueError("Git config values must not contain CR or NUL") @@ -877,7 +893,6 @@ def read_only(self) -> bool: """:return: ``True`` if this instance may change the configuration file""" return self._read_only - # FIXME: Figure out if default or return type can really include bool. def get_value( self, section: str, @@ -894,7 +909,7 @@ def get_value( did not exist. :return: - A properly typed value, either int, float or string + A properly typed value, either int, float, string or bool :raise TypeError: In case the value could not be understood. @@ -925,7 +940,7 @@ def get_values( in case the option did not exist. :return: - A list of properly typed values, either int, float or string + A list of properly typed values, either int, float, string or bool :raise TypeError: In case the value could not be understood. @@ -941,7 +956,19 @@ def get_values( return [self._string_to_value(valuestr) for valuestr in lst] - def _string_to_value(self, valuestr: str) -> Union[int, float, str, bool]: + def _convert_to_boolean(self, value: Union[str, None]) -> bool: + if value is None: + return True + if value == "": + return False + try: + return self.BOOLEAN_STATES[value.lower()] + except KeyError: + raise ValueError("Not a boolean: %s" % value) from None + + def _string_to_value(self, valuestr: Union[str, None]) -> Union[int, float, str, bool]: + if valuestr is None: + return "" types = (int, float) for numtype in types: try: diff --git a/test/test_config.py b/test/test_config.py index 3031721de..2910f3e6c 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -864,8 +864,58 @@ def test_empty_config_value(self): assert cr.get_value("core", "filemode"), "Should read keys with values" - with self.assertRaises(cp.NoOptionError): - cr.get_value("color", "ui") + self.assertTrue(cr.has_option("color", "ui")) + self.assertIsNone(cr.get("color", "ui")) + self.assertEqual(cr.get_value("color", "ui"), "") + self.assertIs(cr.getboolean("color", "ui"), True) + + @with_rw_directory + def test_implicit_boolean_round_trip(self, rw_dir): + config_path = osp.join(rw_dir, "config") + with open(config_path, "wb") as config_file: + config_file.write( + b"[include]\n" + b"\toptional\n" + b"[flag]\n" + b"\timplicit\n" + b"\ttrailing-space \n" + b"\ttrailing-tab\t\n" + b"\tempty =\n" + b'\tquoted = ""\n' + b"\tmultiple = false\n" + b"\tmultiple\n" + b"\tmultiple =\n" + b"\tmultiple" + ) + git_config = ["git", "config", "--file", config_path] + original = subprocess.check_output(git_config + ["--null", "--list"]) + + with GitConfigParser(config_path, read_only=False) as config: + for option in ("implicit", "trailing-space", "trailing-tab"): + self.assertIsNone(config.get("flag", option)) + self.assertEqual(config.get_value("flag", option), "") + self.assertIs(config.getboolean("flag", option), True) + for option in ("empty", "quoted"): + self.assertEqual(config.get("flag", option), "") + self.assertEqual(config.get_value("flag", option), "") + self.assertIs(config.getboolean("flag", option), False) + self.assertEqual(config.get_values("flag", "multiple"), [False, "", "", ""]) + self.assertIsNone(dict(config.items("flag"))["multiple"]) + self.assertEqual(dict(config.items_all("flag"))["multiple"], ["false", None, "", None]) + config.set_value("other", "value", "updated") + + self.assertEqual( + subprocess.check_output(git_config + ["--null", "--list"]), + original + b"other.value\nupdated\0", + ) + self.assertEqual( + subprocess.check_output(git_config + ["--type=bool", "--get-all", "flag.multiple"]), + b"false\ntrue\nfalse\ntrue\n", + ) + with GitConfigParser(config_path) as config: + self.assertIs(config.getboolean("flag", "implicit"), True) + self.assertIs(config.getboolean("flag", "empty"), False) + self.assertEqual(dict(config.items_all("flag"))["multiple"], ["false", None, "", None]) def test_config_with_quotes(self): cr = GitConfigParser(fixture_path("git_config_with_quotes"), read_only=True)