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>

gclient_scm.py

@@ -677,9 +677,27 @@ class GitWrapper(SCMWrapper):
                     config_updates.append(
                         ('blame.ignoreRevsFile', '.git-blame-ignore-revs'))
 
-                if scm.GIT.GetConfig(args[0].checkout_path,
-                                     'diff.ignoresubmodules') != 'dirty':
+                ignore_submodules = scm.GIT.GetConfig(args[0].checkout_path,
+                                                      'diff.ignoresubmodules',
+                                                      None, 'local')
+
+                if not ignore_submodules:
                     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,17 +235,30 @@ class CachedGitConfigState(object):
 
     def GetConfig(self,
                   key: str,
-                  default: Optional[str] = None) -> Optional[str]:
+                  default: Optional[str] = None,
+                  scope: 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)
-        values = self._maybe_load_config().get(key, None)
-        if not values:
+        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)
+
+        if not values:
+            return default
         return values[-1]
 
     def GetConfigBool(self, key: str) -> bool:
@@ -259,7 +272,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(key, ()))
+        return list(self._maybe_load_config().get('default', {}).get(key, ()))
 
     def YieldConfigRegexp(self,
                           pattern: Optional[str] = None
@@ -278,7 +291,8 @@ class CachedGitConfigState(object):
             pred = lambda _: True
         else:
             pred = re.compile(pattern).match
-        for key, values in sorted(self._maybe_load_config().items()):
+        for key, values in sorted(self._maybe_load_config().get('default',
+                                                                {}).items()):
             if pred(key):
                 for value in values:
                     yield key, value
@@ -373,19 +387,36 @@ class GitConfigStateReal(GitConfigStateBase):
     def load_config(self) -> GitFlatConfigData:
         # NOTE: `git config --list` already canonicalizes keys.
         try:
-            rawConfig = GIT.Capture(['config', '--list', '-z'],
+            rawConfig = GIT.Capture(['config', '--list', '-z', '--show-scope'],
                                     cwd=self.root,
                                     strip_out=False)
         except subprocess2.CalledProcessError:
             return {}
 
         assert isinstance(rawConfig, str)
-        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)
+        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
 
         return cfg
 
@@ -504,18 +535,20 @@ class GitConfigStateTest(GitConfigStateBase):
             raise GitConfigUnknownScope(scope)
 
     def load_config(self) -> GitFlatConfigData:
-        ret = {k: list(v) for k, v in self.system_state.items()}
-        for scope in GitScopeOrder:
-            if scope == 'system':
+        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':
                 continue
-            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
+            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
 
     def set_config(self, key: str, value: str, *, append: bool,
                    scope: GitConfigScope):
@@ -644,7 +677,8 @@ class GIT(object):
             state = {}
             for key, val in cls._CONFIG_CACHE.items():
                 if val is not None:
-                    state[str(key)] = val._maybe_load_config()
+                    state[str(key)] = val._maybe_load_config().get(
+                        'default', {})
         return state
 
     @staticmethod
@@ -714,13 +748,14 @@ class GIT(object):
     @staticmethod
     def GetConfig(cwd: str,
                   key: str,
-                  default: Optional[str] = None) -> Optional[str]:
+                  default: Optional[str] = None,
+                  scope: 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)
+        return GIT._get_config_state(cwd).GetConfig(key, default, scope)
 
     @staticmethod
     def GetConfigBool(cwd: str, key: str) -> bool:

tests/gclient_git_smoketest.py

@@ -30,6 +30,7 @@ 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,26 +396,38 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
         if not self.enabled:
             return
         options = self.Options()
-        expected_file_list = [
-            join(self.base_path, x) for x in ['a', 'b', 'submodule']
-        ]
+        expected_file_list = [join(self.base_path, x) for x in ['a', 'b']]
         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']),
-            'dirty')
+            'all')
         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,6 +11,7 @@ import os
 import sys
 import tempfile
 import threading
+from collections import defaultdict
 import unittest
 from unittest import mock
 
@@ -449,28 +450,82 @@ class GitConfigStateTestTest(unittest.TestCase):
         self.assertDictEqual(m.load_config(), {})
 
         gs['section.key'] = ['override']
-        self.assertDictEqual(m.load_config(), {'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
 
     def test_construction_global(self):
-        m, gs = self._make(global_state={'section.key': ['global']})
-        self.assertDictEqual(gs, {'section.key': ['global']})
-        self.assertDictEqual(m.load_config(), {'section.key': ['global']})
+
+        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']
+                },
+            })
 
         gs['section.key'] = ['override']
-        self.assertDictEqual(m.load_config(), {'section.key': ['override']})
+        self.assertDictEqual(
+            self.defaultdict_to_dict(m.load_config()), {
+                "global": {
+                    'section.key': ['override']
+                },
+                "default": {
+                    'section.key': ['override']
+                },
+            })
 
     def test_construction_system(self):
         m, gs = self._make(
             global_state={'section.key': ['global']},
             system_state={'section.key': ['system']},
         )
-        self.assertDictEqual(gs, {'section.key': ['global']})
-        self.assertDictEqual(m.load_config(),
-                             {'section.key': ['system', 'global']})
+        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']
+                }
+            })
 
         gs['section.key'] = ['override']
-        self.assertDictEqual(m.load_config(),
-                             {'section.key': ['system', 'override']})
+        self.assertDictEqual(
+            self.defaultdict_to_dict(m.load_config()), {
+                "global": {
+                    'section.key': ['override']
+                },
+                "system": {
+                    'section.key': ['system']
+                },
+                'default': {
+                    'section.key': ['system', 'override']
+                }
+            })
 
     def test_set_config_system(self):
         m, _ = self._make()
@@ -496,9 +551,15 @@ 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(), {
-            'section.key': ['new_global'],
-        })
+        self.assertDictEqual(
+            m.load_config(), {
+                "default": {
+                    'section.key': ['new_global']
+                },
+                "global": {
+                    'section.key': ['new_global']
+                }
+            })
 
     def test_set_config_global(self):
         m, gs = self._make()
@@ -506,44 +567,64 @@ class GitConfigStateTestTest(unittest.TestCase):
         self.assertDictEqual(m.load_config(), {})
 
         m.set_config('section.key', 'new_global', append=False, scope='global')
-        self.assertDictEqual(m.load_config(), {
-            'section.key': ['new_global'],
-        })
+        self.assertDictEqual(
+            self.defaultdict_to_dict(m.load_config()), {
+                "global": {
+                    'section.key': ['new_global']
+                },
+                "default": {
+                    'section.key': ['new_global']
+                }
+            })
 
         m.set_config('section.key', 'new_global2', append=True, scope='global')
-        self.assertDictEqual(m.load_config(), {
-            'section.key': ['new_global', 'new_global2'],
-        })
-
-        self.assertDictEqual(gs, {
-            'section.key': ['new_global', 'new_global2'],
-        })
+        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']})
 
     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(m.load_config(), {
-            'section.key': ['new_global'],
-        })
-
-        self.assertDictEqual(gs, {
-            'section.key': ['new_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']})
 
         m.set_config_multi('othersection.key',
                            'newval',
                            value_pattern=None,
                            scope='global')
-        self.assertDictEqual(m.load_config(), {
-            'section.key': ['new_global'],
-            'othersection.key': ['newval'],
-        })
+        self.assertDictEqual(
+            m.load_config(), {
+                "global": {
+                    'section.key': ['new_global'],
+                    'othersection.key': ['newval'],
+                },
+                "default": {
+                    'section.key': ['new_global'],
+                    'othersection.key': ['newval'],
+                }
+            })
 
         self.assertDictEqual(gs, {
             'section.key': ['new_global'],
@@ -559,17 +640,29 @@ class GitConfigStateTestTest(unittest.TestCase):
                            'new_global',
                            value_pattern='2',
                            scope='global')
-        self.assertDictEqual(m.load_config(), {
-            'section.key': ['1', '1', 'new_global', '3'],
-        })
+        self.assertDictEqual(
+            m.load_config(), {
+                "global": {
+                    'section.key': ['1', '1', 'new_global', '3']
+                },
+                "default": {
+                    'section.key': ['1', '1', 'new_global', '3']
+                }
+            })
 
         m.set_config_multi('section.key',
                            'additional',
                            value_pattern='narp',
                            scope='global')
-        self.assertDictEqual(m.load_config(), {
-            'section.key': ['1', '1', 'new_global', '3', 'additional'],
-        })
+        self.assertDictEqual(
+            m.load_config(), {
+                "default": {
+                    'section.key': ['1', '1', 'new_global', '3', 'additional']
+                },
+                "global": {
+                    'section.key': ['1', '1', 'new_global', '3', 'additional']
+                }
+            })
 
     def test_unset_config_global(self):
         m, _ = self._make(global_state={
@@ -595,19 +688,34 @@ class GitConfigStateTestTest(unittest.TestCase):
 
         m.unset_config('section.key', scope='global', missing_ok=False)
         self.assertDictEqual(m.load_config(), {
-            'extra': ['another'],
+            "global": {
+                'extra': ['another']
+            },
+            "default": {
+                'extra': ['another']
+            }
         })
 
         with self.assertRaises(scm.GitConfigUnsetMissingValue):
             m.unset_config('section.key', scope='global', missing_ok=False)
 
         self.assertDictEqual(m.load_config(), {
-            'extra': ['another'],
+            "global": {
+                'extra': ['another']
+            },
+            "default": {
+                'extra': ['another']
+            }
         })
 
         m.unset_config('section.key', scope='global', missing_ok=True)
         self.assertDictEqual(m.load_config(), {
-            'extra': ['another'],
+            "global": {
+                'extra': ['another']
+            },
+            "default": {
+                'extra': ['another']
+            }
         })
 
     def test_unset_config_global_multi(self):
@@ -644,9 +752,15 @@ class GitConfigStateTestTest(unittest.TestCase):
                              value_pattern='2',
                              scope='global',
                              missing_ok=False)
-        self.assertDictEqual(m.load_config(), {
-            'section.key': ['1', '3', '1'],
-        })
+        self.assertDictEqual(
+            m.load_config(), {
+                'global': {
+                    'section.key': ['1', '3', '1'],
+                },
+                'default': {
+                    'section.key': ['1', '3', '1'],
+                }
+            })
 
 
 class CanonicalizeGitConfigKeyTest(unittest.TestCase):