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.

<!-- agent -->
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 <codex@openai.com>

Author: Byron

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:

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)