Correctly assign configuration to origin files with pip config debug (#12201)

This commit extends the pip configuration dictionary to be nested,
allowing the configuration keys to be connected with the origin files.

This fixes a bug where pip config debug couldn't tell which file a
value came from if the files are at the same "level". 

Co-authored-by: Tzu-ping Chung <[email protected]>
Co-authored-by: Pradyun Gedam <[email protected]>
Co-authored-by: Richard Si <[email protected]>

Author: 4 people

news/12099.bugfix.rst

@@ -0,0 +1,2 @@
+``pip config debug`` now correctly separates options as set by the different files
+at the same level.

src/pip/_internal/cli/parser.py

@@ -188,23 +188,24 @@ def _get_ordered_configuration_items(
         section_items: dict[str, list[tuple[str, Any]]] = {
             name: [] for name in override_order
         }
-        for section_key, val in self.config.items():
-            # ignore empty values
-            if not val:
-                logger.debug(
-                    "Ignoring configuration key '%s' as it's value is empty.",
-                    section_key,
-                )
-                continue
 
-            section, key = section_key.split(".", 1)
-            if section in override_order:
-                section_items[section].append((key, val))
+        for _, value in self.config.items():  # noqa: PERF102
+            for section_key, val in value.items():
+                # ignore empty values
+                if not val:
+                    logger.debug(
+                        "Ignoring configuration key '%s' as its value is empty.",
+                        section_key,
+                    )
+                    continue
+
+                section, key = section_key.split(".", 1)
+                if section in override_order:
+                    section_items[section].append((key, val))
 
-        # Yield each group in their override order
-        for section in override_order:
-            for key, val in section_items[section]:
-                yield key, val
+            # Yield each group in their override order
+            for section in override_order:
+                yield from section_items[section]
 
     def _update_defaults(self, defaults: dict[str, Any]) -> dict[str, Any]:
         """Updates the given defaults with values from the config files and

src/pip/_internal/commands/configuration.py

@@ -177,7 +177,8 @@ def list_values(self, options: Values, args: list[str]) -> None:
         self._get_n_args(args, "list", n=0)
 
         for key, value in sorted(self.configuration.items()):
-            write_output("%s=%r", key, value)
+            for key, value in sorted(value.items()):
+                write_output("%s=%r", key, value)
 
     def get_name(self, options: Values, args: list[str]) -> None:
         key = self._get_n_args(args, "get [name]", n=1)
@@ -211,13 +212,15 @@ def list_config_values(self, options: Values, args: list[str]) -> None:
                     file_exists = os.path.exists(fname)
                     write_output("%s, exists: %r", fname, file_exists)
                     if file_exists:
-                        self.print_config_file_values(variant)
+                        self.print_config_file_values(variant, fname)
 
-    def print_config_file_values(self, variant: Kind) -> None:
+    def print_config_file_values(self, variant: Kind, fname: str) -> None:
         """Get key-value pairs from the file of a variant"""
         for name, value in self.configuration.get_values_in_config(variant).items():
             with indent_log():
-                write_output("%s: %s", name, value)
+                if name == fname:
+                    for confname, confvalue in value.items():
+                        write_output("%s: %s", confname, confvalue)
 
     def print_env_var_values(self) -> None:
         """Get key-values pairs present as environment variables"""

src/pip/_internal/configuration.py

@@ -117,7 +117,7 @@ def __init__(self, isolated: bool, load_only: Kind | None = None) -> None:
         self._parsers: dict[Kind, list[tuple[str, RawConfigParser]]] = {
             variant: [] for variant in OVERRIDE_ORDER
         }
-        self._config: dict[Kind, dict[str, Any]] = {
+        self._config: dict[Kind, dict[str, dict[str, Any]]] = {
             variant: {} for variant in OVERRIDE_ORDER
         }
         self._modified_parsers: list[tuple[str, RawConfigParser]] = []
@@ -148,7 +148,10 @@ def get_value(self, key: str) -> Any:
         orig_key = key
         key = _normalize_name(key)
         try:
-            return self._dictionary[key]
+            clean_config: dict[str, Any] = {}
+            for file_values in self._dictionary.values():
+                clean_config.update(file_values)
+            return clean_config[key]
         except KeyError:
             # disassembling triggers a more useful error message than simply
             # "No such key" in the case that the key isn't in the form command.option
@@ -171,7 +174,8 @@ def set_value(self, key: str, value: Any) -> None:
                 parser.add_section(section)
             parser.set(section, name, value)
 
-        self._config[self.load_only][key] = value
+        self._config[self.load_only].setdefault(fname, {})
+        self._config[self.load_only][fname][key] = value
         self._mark_as_modified(fname, parser)
 
     def unset_value(self, key: str) -> None:
@@ -181,11 +185,14 @@ def unset_value(self, key: str) -> None:
         self._ensure_have_load_only()
 
         assert self.load_only
-        if key not in self._config[self.load_only]:
-            raise ConfigurationError(f"No such key - {orig_key}")
-
         fname, parser = self._get_parser_to_modify()
 
+        if (
+            key not in self._config[self.load_only][fname]
+            and key not in self._config[self.load_only]
+        ):
+            raise ConfigurationError(f"No such key - {orig_key}")
+
         if parser is not None:
             section, name = _disassemble_key(key)
             if not (
@@ -200,8 +207,10 @@ def unset_value(self, key: str) -> None:
             if not parser.items(section):
                 parser.remove_section(section)
             self._mark_as_modified(fname, parser)
-
-        del self._config[self.load_only][key]
+        try:
+            del self._config[self.load_only][fname][key]
+        except KeyError:
+            del self._config[self.load_only][key]
 
     def save(self) -> None:
         """Save the current in-memory state."""
@@ -233,7 +242,7 @@ def _ensure_have_load_only(self) -> None:
         logger.debug("Will be working with %s variant only", self.load_only)
 
     @property
-    def _dictionary(self) -> dict[str, Any]:
+    def _dictionary(self) -> dict[str, dict[str, Any]]:
         """A dictionary representing the loaded configuration."""
         # NOTE: Dictionaries are not populated if not loaded. So, conditionals
         #       are not needed here.
@@ -273,7 +282,8 @@ def _load_file(self, variant: Kind, fname: str) -> RawConfigParser:
 
         for section in parser.sections():
             items = parser.items(section)
-            self._config[variant].update(self._normalized_keys(section, items))
+            self._config[variant].setdefault(fname, {})
+            self._config[variant][fname].update(self._normalized_keys(section, items))
 
         return parser
 
@@ -300,7 +310,8 @@ def _construct_parser(self, fname: str) -> RawConfigParser:
 
     def _load_environment_vars(self) -> None:
         """Loads configuration from environment variables"""
-        self._config[kinds.ENV_VAR].update(
+        self._config[kinds.ENV_VAR].setdefault(":env:", {})
+        self._config[kinds.ENV_VAR][":env:"].update(
             self._normalized_keys(":env:", self.get_environ_vars())
         )
 

tests/functional/test_configuration.py

@@ -1,16 +1,43 @@
 """Tests for the config command"""
 
+from __future__ import annotations
+
+import ast
+import os
 import re
+import subprocess
+import sys
 import textwrap
 
 from pip._internal.cli.status_codes import ERROR
-from pip._internal.configuration import CONFIG_BASENAME, get_configuration_files
+from pip._internal.configuration import CONFIG_BASENAME, Kind
+from pip._internal.configuration import get_configuration_files as _get_config_files
+from pip._internal.utils.compat import WINDOWS
 
 from tests.lib import PipTestEnvironment
 from tests.lib.configuration_helpers import ConfigurationMixin, kinds
 from tests.lib.venv import VirtualEnvironment
 
 
+def get_configuration_files() -> dict[Kind, list[str]]:
+    """Wrapper over pip._internal.configuration.get_configuration_files()."""
+    if WINDOWS:
+        # The user configuration directory is updated in the isolate fixture using the
+        # APPDATA environment variable. This will only take effect in new subprocesses,
+        # however. To ensure that get_configuration_files() never returns stale data,
+        # call it in a subprocess on Windows.
+        code = (
+            "from pip._internal.configuration import get_configuration_files; "
+            "print(get_configuration_files())"
+        )
+        proc = subprocess.run(
+            [sys.executable, "-c", code], capture_output=True, encoding="utf-8"
+        )
+        return ast.literal_eval(proc.stdout)
+    else:
+        return _get_config_files()
+
+
 def test_no_options_passed_should_error(script: PipTestEnvironment) -> None:
     result = script.pip("config", expect_error=True)
     assert result.returncode == ERROR
@@ -148,3 +175,40 @@ def test_editor_does_not_exist(self, script: PipTestEnvironment) -> None:
             "config", "edit", "--editor", "notrealeditor", expect_error=True
         )
         assert "notrealeditor" in result.stderr
+
+    def test_config_separated(
+        self, script: PipTestEnvironment, virtualenv: VirtualEnvironment
+    ) -> None:
+        """Test that the pip configuration values in the different config sections
+        are correctly assigned to their origin files.
+        """
+
+        # Use new config file
+        new_config_file = get_configuration_files()[kinds.USER][1]
+
+        # Get legacy config file and touch it for testing purposes
+        legacy_config_file = get_configuration_files()[kinds.USER][0]
+        os.makedirs(os.path.dirname(legacy_config_file))
+        open(legacy_config_file, "a").close()
+
+        # Site config file
+        site_config_file = virtualenv.location / CONFIG_BASENAME
+
+        script.pip("config", "--user", "set", "global.timeout", "60")
+        script.pip("config", "--site", "set", "freeze.timeout", "10")
+
+        result = script.pip("config", "debug")
+
+        assert (
+            f"{site_config_file}, exists: True\n    freeze.timeout: 10" in result.stdout
+        )
+        assert (
+            f"{new_config_file}, exists: True\n    global.timeout: 60" in result.stdout
+        )
+        assert re.search(
+            (
+                rf"{re.escape(legacy_config_file)}, "
+                rf"exists: True\n(  {re.escape(new_config_file)}.+\n)+"
+            ),
+            result.stdout,
+        )

tests/lib/configuration_helpers.py

@@ -28,7 +28,8 @@ def patch_configuration(self, variant: Kind, di: dict[str, Any]) -> None:
         @functools.wraps(old)
         def overridden() -> None:
             # Manual Overload
-            self.configuration._config[variant].update(di)
+            self.configuration._config[variant].setdefault("fakefile", {})
+            self.configuration._config[variant]["fakefile"].update(di)
             # Configuration._parsers has type:
             # Dict[Kind, List[Tuple[str, RawConfigParser]]].
             # As a testing convenience, pass a special value.