Revert "Avoid silently overwriting diff.ignoreSubmodules config."

This reverts commit acd68a9a5dde2545a28c0c29c058a5070ede076f.

Reason for revert: Doesn't get values for non scoped config.

Original change's description:
> Avoid silently overwriting diff.ignoreSubmodules config.
>
> Setting this to `dirty` is generally desirable. If the user has
> explicitly set it to something else on their chromium checkout,
> warn rather than silently overwriting.
>
> Bug: 362787698
> Change-Id: I56587d501aafc5de78c82bc46e29f926519d25fa
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5832742
> Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
> Commit-Queue: Helmut Januschka <helmut@januschka.com>

Bug: 362787698
Change-Id: I3e5d1e44e3707dda53dc398f83c43259868d343b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5842067
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Joey Scarr <jsca@google.com>
Owners-Override: Joey Scarr <jsca@google.com>
Commit-Queue: Dan Le Febvre <dlf@google.com>

gclient_scm.py

@@ -677,27 +677,9 @@ class GitWrapper(SCMWrapper):
                     config_updates.append(
                         ('blame.ignoreRevsFile', '.git-blame-ignore-revs'))
 
-                ignore_submodules = scm.GIT.GetConfig(args[0].checkout_path,
-                                                      'diff.ignoresubmodules',
-                                                      None, 'local')
-
-                if not ignore_submodules:
+                if scm.GIT.GetConfig(args[0].checkout_path,
+                                     'diff.ignoresubmodules') != 'dirty':
                     config_updates.append(('diff.ignoreSubmodules', 'dirty'))
-                elif ignore_submodules != 'dirty':
-                    warning_message = (
-                        "diff.ignoreSubmodules is not set to 'dirty' "
-                        "for this repository.\n"
-                        "This may cause unexpected behavior with submodules; "
-                        "see //docs/git_submodules.md\n"
-                        "Consider setting the config:\n"
-                        "\tgit config diff.ignoreSubmodule dirty\n"
-                        "or disable this warning by setting the "
-                        "GCLIENT_SUPPRESS_SUBMODULE_WARNING environment "
-                        "variable to 1.")
-                    if os.environ.get(
-                            'GCLIENT_SUPPRESS_SUBMODULE_WARNING') != '1':
-                        gclient_utils.AddWarning(warning_message)
-
 
                 if scm.GIT.GetConfig(args[0].checkout_path,
                                      'fetch.recursesubmodules') != 'off':

scm.py

@@ -235,30 +235,17 @@ class CachedGitConfigState(object):
 
     def GetConfig(self,
                   key: str,
-                  default: Optional[str] = None,
-                  scope: Optional[str] = None) -> Optional[str]:
+                  default: Optional[str] = None) -> Optional[str]:
         """Lazily loads all configration observable for this CachedGitConfigState,
         then returns the last value for `key` as a string.
 
         If `key` is missing, returns default.
         """
-
         key = canonicalize_git_config_key(key)
-        if not scope:
-            scope = "local"
-
-        scoped_config = self._maybe_load_config()
-        if not scoped_config:
-            return default
-
-        scoped_config = scoped_config.get(scope, None)
-        if not scoped_config:
-            return default
-
-        values = scoped_config.get(key, None)
-
+        values = self._maybe_load_config().get(key, None)
         if not values:
             return default
+
         return values[-1]
 
     def GetConfigBool(self, key: str) -> bool:
@@ -272,7 +259,7 @@ class CachedGitConfigState(object):
     def GetConfigList(self, key: str) -> list[str]:
         """Returns all values of `key` as a list of strings."""
         key = canonicalize_git_config_key(key)
-        return list(self._maybe_load_config().get('default', {}).get(key, ()))
+        return list(self._maybe_load_config().get(key, ()))
 
     def YieldConfigRegexp(self,
                           pattern: Optional[str] = None
@@ -291,8 +278,7 @@ class CachedGitConfigState(object):
             pred = lambda _: True
         else:
             pred = re.compile(pattern).match
-        for key, values in sorted(self._maybe_load_config().get('default',
-                                                                {}).items()):
+        for key, values in sorted(self._maybe_load_config().items()):
             if pred(key):
                 for value in values:
                     yield key, value
@@ -387,36 +373,19 @@ class GitConfigStateReal(GitConfigStateBase):
     def load_config(self) -> GitFlatConfigData:
         # NOTE: `git config --list` already canonicalizes keys.
         try:
-            rawConfig = GIT.Capture(['config', '--list', '-z', '--show-scope'],
+            rawConfig = GIT.Capture(['config', '--list', '-z'],
                                     cwd=self.root,
                                     strip_out=False)
         except subprocess2.CalledProcessError:
             return {}
 
         assert isinstance(rawConfig, str)
-        cfg: Dict[str, Dict[str,
-                            List[str]]] = defaultdict(lambda: defaultdict(list))
-
-        entries = rawConfig.split('\x00')[:-1]
-
-        def process_entry(entry: str, scope: str) -> None:
-            parts = entry.split('\n', 1)
-            key, value = parts if len(parts) == 2 else (parts[0], '')
-            key, value = key.strip(), value.strip()
-            cfg[scope][key].append(value)
-            if scope != "default":
-                cfg["default"][key].append(value)
-
-        i = 0
-        while i < len(entries):
-            if entries[i] in ['local', 'global', 'system']:
-                scope = entries[i]
-                i += 1
-                if i < len(entries):
-                    process_entry(entries[i], scope)
-            else:
-                process_entry(entries[i], "default")
-            i += 1
+        cfg: Dict[str, list[str]] = defaultdict(list)
+
+        # Splitting by '\x00' gets an additional empty string at the end.
+        for line in rawConfig.split('\x00')[:-1]:
+            key, value = map(str.strip, line.split('\n', 1))
+            cfg[key].append(value)
 
         return cfg
 
@@ -535,20 +504,18 @@ class GitConfigStateTest(GitConfigStateBase):
             raise GitConfigUnknownScope(scope)
 
     def load_config(self) -> GitFlatConfigData:
-        cfg: Dict[str, Dict[str,
-                            List[str]]] = defaultdict(lambda: defaultdict(list))
-
-        for key, values in self.system_state.items():
-            cfg['system'][key].extend(values)
-            cfg['default'][key].extend(values)
-        for ordered_scope in GitScopeOrder:
-            if ordered_scope == 'system':
+        ret = {k: list(v) for k, v in self.system_state.items()}
+        for scope in GitScopeOrder:
+            if scope == 'system':
                 continue
-            with self._editable_scope(ordered_scope) as scope_cfg:
-                for key, values in scope_cfg.items():
-                    cfg[ordered_scope][key].extend(values)
-                    cfg['default'][key].extend(values)
-        return cfg
+            with self._editable_scope(scope) as cfg:
+                for key, value in cfg.items():
+                    curvals = ret.get(key, None)
+                    if curvals is None:
+                        curvals = []
+                        ret[key] = curvals
+                    curvals.extend(value)
+        return ret
 
     def set_config(self, key: str, value: str, *, append: bool,
                    scope: GitConfigScope):
@@ -677,8 +644,7 @@ class GIT(object):
             state = {}
             for key, val in cls._CONFIG_CACHE.items():
                 if val is not None:
-                    state[str(key)] = val._maybe_load_config().get(
-                        'default', {})
+                    state[str(key)] = val._maybe_load_config()
         return state
 
     @staticmethod
@@ -748,14 +714,13 @@ class GIT(object):
     @staticmethod
     def GetConfig(cwd: str,
                   key: str,
-                  default: Optional[str] = None,
-                  scope: Optional[str] = None) -> Optional[str]:
+                  default: Optional[str] = None) -> Optional[str]:
         """Lazily loads all configration observable for this CachedGitConfigState,
         then returns the last value for `key` as a string.
 
         If `key` is missing, returns default.
         """
-        return GIT._get_config_state(cwd).GetConfig(key, default, scope)
+        return GIT._get_config_state(cwd).GetConfig(key, default)
 
     @staticmethod
     def GetConfigBool(cwd: str, key: str) -> bool:

tests/gclient_git_smoketest.py

@@ -30,7 +30,6 @@ class GClientSmokeGIT(gclient_smoketest_base.GClientSmokeBase):
         super(GClientSmokeGIT, self).setUp()
         self.env['PATH'] = (os.path.join(ROOT_DIR, 'testing_support') +
                             os.pathsep + self.env['PATH'])
-        self.env['GCLIENT_SUPPRESS_SUBMODULE_WARNING'] = '1'
         self.enabled = self.FAKE_REPOS.set_up_git()
         if not self.enabled:
             self.skipTest('git fake repos not available')

tests/gclient_scm_test.py

@@ -396,38 +396,26 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         if not self.enabled:
             return
         options = self.Options()
-        expected_file_list = [join(self.base_path, x) for x in ['a', 'b']]
+        expected_file_list = [
+            join(self.base_path, x) for x in ['a', 'b', 'submodule']
+        ]
         git_wrapper = gclient_scm.GitWrapper(self.url, self.root_dir,
                                              self.relpath)
         file_list = []
 
-        # Set diff.ignoreSubmodules to something other than 'dirty'
-        git_wrapper._Run(['config', 'diff.ignoreSubmodules', 'all'], options)
         git_wrapper.update(options, (), file_list)
-        expected_warning = "diff.ignoreSubmodules is not set to 'dirty'"
-        self.assertTrue(
-            any(expected_warning in w for w in gclient_utils._WARNINGS),
-            f"Expected warning not found. "
-            f"New warnings: {gclient_utils._WARNINGS}")
         self.assertEqual(file_list, expected_file_list)
         self.assertEqual(git_wrapper.revinfo(options, (), None),
                          '4091c7d010ca99d0f2dd416d4b70b758ae432187')
         self.assertEqual(
             git_wrapper._Capture(['config', '--get', 'diff.ignoreSubmodules']),
-            'all')
+            'dirty')
         self.assertEqual(
             git_wrapper._Capture(['config', '--get',
                                   'fetch.recurseSubmodules']), 'off')
         self.assertEqual(
             git_wrapper._Capture(['config', '--get', 'push.recurseSubmodules']),
             'off')
-        os.environ['GCLIENT_SUPPRESS_SUBMODULE_WARNING'] = '1'
-        gclient_utils._WARNINGS.clear()
-        git_wrapper.update(options, (), file_list)
-        self.assertEqual(len(gclient_utils._WARNINGS), 0,
-                         "Warning was added despite being suppressed")
-        # Clean up
-        del os.environ['GCLIENT_SUPPRESS_SUBMODULE_WARNING']
         sys.stdout.close()
 
     def testUpdateMerge(self):

tests/scm_unittest.py

@@ -11,7 +11,6 @@ import os
 import sys
 import tempfile
 import threading
-from collections import defaultdict
 import unittest
 from unittest import mock
 
@@ -450,82 +449,28 @@ class GitConfigStateTestTest(unittest.TestCase):
         self.assertDictEqual(m.load_config(), {})
 
         gs['section.key'] = ['override']
-        self.assertDictEqual(
-            m.load_config(), {
-                "global": {
-                    'section.key': ['override']
-                },
-                "default": {
-                    'section.key': ['override']
-                },
-            })
-
-    def defaultdict_to_dict(self, d):
-        if isinstance(d, defaultdict):
-            return {k: self.defaultdict_to_dict(v) for k, v in d.items()}
-        return d
+        self.assertDictEqual(m.load_config(), {'section.key': ['override']})
 
     def test_construction_global(self):
-
-        m, gs = self._make(global_state={
-            'section.key': ['global'],
-        })
-        self.assertDictEqual(self.defaultdict_to_dict(gs),
-                             {'section.key': ['global']})
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                "global": {
-                    'section.key': ['global']
-                },
-                "default": {
-                    'section.key': ['global']
-                },
-            })
+        m, gs = self._make(global_state={'section.key': ['global']})
+        self.assertDictEqual(gs, {'section.key': ['global']})
+        self.assertDictEqual(m.load_config(), {'section.key': ['global']})
 
         gs['section.key'] = ['override']
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                "global": {
-                    'section.key': ['override']
-                },
-                "default": {
-                    'section.key': ['override']
-                },
-            })
+        self.assertDictEqual(m.load_config(), {'section.key': ['override']})
 
     def test_construction_system(self):
         m, gs = self._make(
             global_state={'section.key': ['global']},
             system_state={'section.key': ['system']},
         )
-        self.assertDictEqual(self.defaultdict_to_dict(gs),
-                             {'section.key': ['global']})
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                'default': {
-                    'section.key': ['system', 'global']
-                },
-                "global": {
-                    'section.key': ['global']
-                },
-                "system": {
-                    'section.key': ['system']
-                }
-            })
+        self.assertDictEqual(gs, {'section.key': ['global']})
+        self.assertDictEqual(m.load_config(),
+                             {'section.key': ['system', 'global']})
 
         gs['section.key'] = ['override']
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                "global": {
-                    'section.key': ['override']
-                },
-                "system": {
-                    'section.key': ['system']
-                },
-                'default': {
-                    'section.key': ['system', 'override']
-                }
-            })
+        self.assertDictEqual(m.load_config(),
+                             {'section.key': ['system', 'override']})
 
     def test_set_config_system(self):
         m, _ = self._make()
@@ -551,15 +496,9 @@ class GitConfigStateTestTest(unittest.TestCase):
         self.assertDictEqual(m.load_config(), {})
 
         m.set_config('section.key', 'new_global', append=True, scope='global')
-        self.assertDictEqual(
-            m.load_config(), {
-                "default": {
-                    'section.key': ['new_global']
-                },
-                "global": {
-                    'section.key': ['new_global']
-                }
-            })
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['new_global'],
+        })
 
     def test_set_config_global(self):
         m, gs = self._make()
@@ -567,64 +506,44 @@ class GitConfigStateTestTest(unittest.TestCase):
         self.assertDictEqual(m.load_config(), {})
 
         m.set_config('section.key', 'new_global', append=False, scope='global')
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                "global": {
-                    'section.key': ['new_global']
-                },
-                "default": {
-                    'section.key': ['new_global']
-                }
-            })
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['new_global'],
+        })
 
         m.set_config('section.key', 'new_global2', append=True, scope='global')
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                "global": {
-                    'section.key': ['new_global', 'new_global2']
-                },
-                "default": {
-                    'section.key': ['new_global', 'new_global2']
-                },
-            })
-
-        self.assertDictEqual(self.defaultdict_to_dict(gs),
-                             {'section.key': ['new_global', 'new_global2']})
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['new_global', 'new_global2'],
+        })
+
+        self.assertDictEqual(gs, {
+            'section.key': ['new_global', 'new_global2'],
+        })
 
     def test_set_config_multi_global(self):
-        m, gs = self._make(global_state={'section.key': ['1', '2']})
+        m, gs = self._make(global_state={
+            'section.key': ['1', '2'],
+        })
 
         m.set_config_multi('section.key',
                            'new_global',
                            value_pattern=None,
                            scope='global')
-        self.assertDictEqual(
-            self.defaultdict_to_dict(m.load_config()), {
-                "default": {
-                    'section.key': ['new_global']
-                },
-                "global": {
-                    'section.key': ['new_global']
-                }
-            })
-
-        self.assertDictEqual(gs, {'section.key': ['new_global']})
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['new_global'],
+        })
+
+        self.assertDictEqual(gs, {
+            'section.key': ['new_global'],
+        })
 
         m.set_config_multi('othersection.key',
                            'newval',
                            value_pattern=None,
                            scope='global')
-        self.assertDictEqual(
-            m.load_config(), {
-                "global": {
-                    'section.key': ['new_global'],
-                    'othersection.key': ['newval'],
-                },
-                "default": {
-                    'section.key': ['new_global'],
-                    'othersection.key': ['newval'],
-                }
-            })
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['new_global'],
+            'othersection.key': ['newval'],
+        })
 
         self.assertDictEqual(gs, {
             'section.key': ['new_global'],
@@ -640,29 +559,17 @@ class GitConfigStateTestTest(unittest.TestCase):
                            'new_global',
                            value_pattern='2',
                            scope='global')
-        self.assertDictEqual(
-            m.load_config(), {
-                "global": {
-                    'section.key': ['1', '1', 'new_global', '3']
-                },
-                "default": {
-                    'section.key': ['1', '1', 'new_global', '3']
-                }
-            })
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['1', '1', 'new_global', '3'],
+        })
 
         m.set_config_multi('section.key',
                            'additional',
                            value_pattern='narp',
                            scope='global')
-        self.assertDictEqual(
-            m.load_config(), {
-                "default": {
-                    'section.key': ['1', '1', 'new_global', '3', 'additional']
-                },
-                "global": {
-                    'section.key': ['1', '1', 'new_global', '3', 'additional']
-                }
-            })
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['1', '1', 'new_global', '3', 'additional'],
+        })
 
     def test_unset_config_global(self):
         m, _ = self._make(global_state={
@@ -688,34 +595,19 @@ class GitConfigStateTestTest(unittest.TestCase):
 
         m.unset_config('section.key', scope='global', missing_ok=False)
         self.assertDictEqual(m.load_config(), {
-            "global": {
-                'extra': ['another']
-            },
-            "default": {
-                'extra': ['another']
-            }
+            'extra': ['another'],
         })
 
         with self.assertRaises(scm.GitConfigUnsetMissingValue):
             m.unset_config('section.key', scope='global', missing_ok=False)
 
         self.assertDictEqual(m.load_config(), {
-            "global": {
-                'extra': ['another']
-            },
-            "default": {
-                'extra': ['another']
-            }
+            'extra': ['another'],
         })
 
         m.unset_config('section.key', scope='global', missing_ok=True)
         self.assertDictEqual(m.load_config(), {
-            "global": {
-                'extra': ['another']
-            },
-            "default": {
-                'extra': ['another']
-            }
+            'extra': ['another'],
         })
 
     def test_unset_config_global_multi(self):
@@ -752,15 +644,9 @@ class GitConfigStateTestTest(unittest.TestCase):
                              value_pattern='2',
                              scope='global',
                              missing_ok=False)
-        self.assertDictEqual(
-            m.load_config(), {
-                'global': {
-                    'section.key': ['1', '3', '1'],
-                },
-                'default': {
-                    'section.key': ['1', '3', '1'],
-                }
-            })
+        self.assertDictEqual(m.load_config(), {
+            'section.key': ['1', '3', '1'],
+        })
 
 
 class CanonicalizeGitConfigKeyTest(unittest.TestCase):