Merge pull request #2238 from gitpython-developers/read-implicit-bool

Preserve implicit boolean config keys

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)