Revert "Update gclient to use git config caching"

This reverts commit 3edda8d185.

Reason for revert: Breaks rebase-update; crbug.com/324358728

Original change's description:
> Update gclient to use git config caching
>
> This change updates all the modules used by gclient to use `scm.GIT` for git config calls over directly invoking the subprocess.
>
> This change currently doesn't modify git_cache since the config reads and writes within it are done on bare repository. A follow-up CL will update git_cache.
>
> A follow-up CL will also update git_cl and git_map_branches since they have shown performance improvements too: https://crrev.com/c/4697786.
>
> Benchmarking
> ============
> With chromium/src as the baseline super project, this change reduces about 380 git config calls out of 507 total calls on cache hits during no-op. The below numbers are benchmarked with `update_depot_tools` turned off.
>
> Windows Benchmark
> =================
> Baseline (gpaste/6360045736951808): ~1min 12 sec.
> With Caching (gpaste/6480065209040896): ~1min 3sec.
> ~12.5% decrease in gclient sync noop runtime.
>
> Linux Benchmark
> ===============
> Baseline (gpaste/4730436763254784): ~3.739 sec.
> With Caching (gpaste/4849870978940928): ~3.534 sec.
> ~5.5% decrease in gclient sync noop runtime.
>
> Bug: 1501984
> Change-Id: Ib48df2d26a0c742a9b555a1e2ed6366221c7db17
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5252498
> Commit-Queue: Aravind Vasudevan <aravindvasudev@google.com>
> Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

Bug: 1501984
Change-Id: I4a603238d9ed43edafc8e574493800670520a1d9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5279198
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Aravind Vasudevan <aravindvasudev@google.com>
changes/98/5279198/2
Aravind Vasudevan 1 year ago committed by LUCI CQ
parent e98e458a9f
commit 3569608028

@ -134,8 +134,11 @@ class SCMWrapper(object):
@staticmethod @staticmethod
def _get_first_remote_url(checkout_path): def _get_first_remote_url(checkout_path):
log = scm.GIT.YieldConfigRegexp(checkout_path, r'remote.*.url') log = scm.GIT.Capture(
return next(log)[1] ['config', '--local', '--get-regexp', r'remote.*.url'],
cwd=checkout_path)
# Get the second token of the first line of the log.
return log.splitlines()[0].split(' ', 1)[1]
def GetCacheMirror(self): def GetCacheMirror(self):
if getattr(self, 'cache_dir', None): if getattr(self, 'cache_dir', None):
@ -589,35 +592,26 @@ class GitWrapper(SCMWrapper):
def wrapper(*args): def wrapper(*args):
return_val = f(*args) return_val = f(*args)
if os.path.exists(os.path.join(args[0].checkout_path, '.git')): if os.path.exists(os.path.join(args[0].checkout_path, '.git')):
# The config updates to the project are stored in this list # If diff.ignoreSubmodules is not already set, set it to `all`.
# and updated consecutively after the reads. The updates config = subprocess2.capture(['git', 'config', '-l'],
# are done this way because `scm.GIT.GetConfig` caches cwd=args[0].checkout_path).decode(
# the config file and `scm.GIT.SetConfig` evicts the cache. 'utf-8').strip().splitlines()
# This ensures we don't interleave reads and writes causing if 'diff.ignoresubmodules=dirty' not in config:
# the cache to set and unset consecutively. subprocess2.capture(
config_updates = [] ['git', 'config', 'diff.ignoreSubmodules', 'dirty'],
cwd=args[0].checkout_path)
if scm.GIT.GetConfig(args[0].checkout_path, if 'fetch.recursesubmodules=off' not in config:
'diff.ignoresubmodules') != 'dirty': subprocess2.capture(
# If diff.ignoreSubmodules is not already set, set it to `all`. ['git', 'config', 'fetch.recurseSubmodules', 'off'],
config_updates.append(('diff.ignoreSubmodules', 'dirty')) cwd=args[0].checkout_path)
if 'push.recursesubmodules=off' not in config:
if scm.GIT.GetConfig(args[0].checkout_path,
'fetch.recursesubmodules') != 'off':
config_updates.append(('fetch.recurseSubmodules', 'off'))
if scm.GIT.GetConfig(args[0].checkout_path,
'push.recursesubmodules') != 'off':
# The default is off, but if user sets submodules.recurse to # The default is off, but if user sets submodules.recurse to
# on, this becomes on too. We never want to push submodules # on, this becomes on too. We never want to push submodules
# for gclient managed repositories. Push, even if a no-op, # for gclient managed repositories. Push, even if a no-op,
# will increase `git cl upload` latency. # will increase `git cl upload` latency.
config_updates.append(('push.recurseSubmodules', 'off')) subprocess2.capture(
['git', 'config', 'push.recurseSubmodules', 'off'],
for update in config_updates: cwd=args[0].checkout_path)
scm.GIT.SetConfig(args[0].checkout_path, update[0],
update[1])
return return_val return return_val
return wrapper return wrapper
@ -745,8 +739,7 @@ class GitWrapper(SCMWrapper):
# See if the url has changed (the unittests use git://foo for the url, # See if the url has changed (the unittests use git://foo for the url,
# let that through). # let that through).
current_url = scm.GIT.GetConfig(self.checkout_path, current_url = self._Capture(['config', 'remote.%s.url' % self.remote])
f'remote.{self.remote}.url')
return_early = False return_early = False
# TODO(maruel): Delete url != 'git://foo' since it's just to make the # TODO(maruel): Delete url != 'git://foo' since it's just to make the
# unit test pass. (and update the comment above) # unit test pass. (and update the comment above)
@ -757,9 +750,12 @@ class GitWrapper(SCMWrapper):
strp_current_url = current_url[:-4] if current_url.endswith( strp_current_url = current_url[:-4] if current_url.endswith(
'.git') else current_url '.git') else current_url
if (strp_current_url.rstrip('/') != strp_url.rstrip('/') if (strp_current_url.rstrip('/') != strp_url.rstrip('/')
and url != 'git://foo' and scm.GIT.GetConfigBool( and url != 'git://foo' and
self.checkout_path, subprocess2.capture([
f'remote.{self.remote}.gclient-auto-fix-url')): 'git', 'config',
'remote.%s.gclient-auto-fix-url' % self.remote
],
cwd=self.checkout_path).strip() != 'False'):
self.Print('_____ switching %s from %s to new upstream %s' % self.Print('_____ switching %s from %s to new upstream %s' %
(self.relpath, current_url, url)) (self.relpath, current_url, url))
if not (options.force or options.reset): if not (options.force or options.reset):
@ -1557,30 +1553,34 @@ class GitWrapper(SCMWrapper):
if requested.""" if requested."""
if options.force or options.reset: if options.force or options.reset:
try: try:
scm.GIT.SetConfig(self.checkout_path, self._Run(
f'remote.{self.remote}.fetch', ['config', '--unset-all',
modify_all=True) 'remote.%s.fetch' % self.remote], options)
scm.GIT.SetConfig( self._Run([
self.checkout_path, f'remote.{self.remote}.fetch', 'config',
f'+refs/heads/*:refs/remotes/{self.remote}/*') 'remote.%s.fetch' % self.remote,
'+refs/heads/*:refs/remotes/%s/*' % self.remote
], options)
except subprocess2.CalledProcessError as e: except subprocess2.CalledProcessError as e:
# If exit code was 5, it means we attempted to unset a config # If exit code was 5, it means we attempted to unset a config
# that didn't exist. Ignore it. # that didn't exist. Ignore it.
if e.returncode != 5: if e.returncode != 5:
raise raise
if hasattr(options, 'with_branch_heads') and options.with_branch_heads: if hasattr(options, 'with_branch_heads') and options.with_branch_heads:
scm.GIT.SetConfig( config_cmd = [
self.checkout_path, 'config',
f'remote.{self.remote}.fetch', 'remote.%s.fetch' % self.remote,
'+refs/branch-heads/*:refs/remotes/branch-heads/*', '+refs/branch-heads/*:refs/remotes/branch-heads/*',
value_pattern='^\\+refs/branch-heads/\\*:.*$', '^\\+refs/branch-heads/\\*:.*$'
modify_all=True) ]
self._Run(config_cmd, options)
if hasattr(options, 'with_tags') and options.with_tags: if hasattr(options, 'with_tags') and options.with_tags:
scm.GIT.SetConfig(self.checkout_path, config_cmd = [
f'remote.{self.remote}.fetch', 'config',
'+refs/tags/*:refs/tags/*', 'remote.%s.fetch' % self.remote, '+refs/tags/*:refs/tags/*',
value_pattern='^\\+refs/tags/\\*:.*$', '^\\+refs/tags/\\*:.*$'
modify_all=True) ]
self._Run(config_cmd, options)
def _AutoFetchRef(self, options, revision, depth=None): def _AutoFetchRef(self, options, revision, depth=None):
"""Attempts to fetch |revision| if not available in local repo. """Attempts to fetch |revision| if not available in local repo.

@ -28,7 +28,6 @@ import auth
import gclient_utils import gclient_utils
import metrics import metrics
import metrics_utils import metrics_utils
import scm
import subprocess2 import subprocess2
import http.cookiejar import http.cookiejar
@ -217,10 +216,12 @@ class CookiesAuthenticator(Authenticator):
def get_gitcookies_path(cls): def get_gitcookies_path(cls):
if os.getenv('GIT_COOKIES_PATH'): if os.getenv('GIT_COOKIES_PATH'):
return os.getenv('GIT_COOKIES_PATH') return os.getenv('GIT_COOKIES_PATH')
try:
return scm.GIT.GetConfig( path = subprocess2.check_output(
os.getcwd(), 'http.cookiefile', ['git', 'config', '--path', 'http.cookiefile'])
os.path.expanduser(os.path.join('~', '.gitcookies'))) return path.decode('utf-8', 'ignore').strip()
except subprocess2.CalledProcessError:
return os.path.expanduser(os.path.join('~', '.gitcookies'))
@classmethod @classmethod
def _get_gitcookies(cls): def _get_gitcookies(cls):

@ -38,7 +38,6 @@ import signal
import tempfile import tempfile
import textwrap import textwrap
import scm
import subprocess2 import subprocess2
from io import BytesIO from io import BytesIO
@ -350,10 +349,8 @@ def branch_config_map(option):
"""Return {branch: <|option| value>} for all branches.""" """Return {branch: <|option| value>} for all branches."""
try: try:
reg = re.compile(r'^branch\.(.*)\.%s$' % option) reg = re.compile(r'^branch\.(.*)\.%s$' % option)
return { lines = get_config_regexp(reg.pattern)
reg.match(k).group(1): v return {reg.match(k).group(1): v for k, v in (l.split() for l in lines)}
for k, v in get_config_regexp(reg.pattern)
}
except subprocess2.CalledProcessError: except subprocess2.CalledProcessError:
return {} return {}
@ -387,7 +384,11 @@ def branches(use_limit=True, *args):
def get_config(option, default=None): def get_config(option, default=None):
return scm.GIT.GetConfig(os.getcwd(), option, default) try:
return run('config', '--get', option) or default
except subprocess2.CalledProcessError:
return default
def get_config_int(option, default=0): def get_config_int(option, default=0):
assert isinstance(default, int) assert isinstance(default, int)
@ -398,11 +399,19 @@ def get_config_int(option, default=0):
def get_config_list(option): def get_config_list(option):
return scm.GIT.GetConfigList(os.getcwd(), option) try:
return run('config', '--get-all', option).split()
except subprocess2.CalledProcessError:
return []
def get_config_regexp(pattern): def get_config_regexp(pattern):
return scm.GIT.YieldConfigRegexp(os.getcwd(), pattern) if IS_WIN: # pragma: no cover
# this madness is because we call git.bat which calls git.exe which
# calls bash.exe (or something to that effect). Each layer divides the
# number of ^'s by 2.
pattern = pattern.replace('^', '^' * 8)
return run('config', '--get-regexp', pattern).splitlines()
def is_fsmonitor_enabled(): def is_fsmonitor_enabled():
@ -446,7 +455,7 @@ def del_branch_config(branch, option, scope='local'):
def del_config(option, scope='local'): def del_config(option, scope='local'):
try: try:
scm.GIT.SetConfig(os.getcwd(), option, scope=scope) run('config', '--' + scope, '--unset', option)
except subprocess2.CalledProcessError: except subprocess2.CalledProcessError:
pass pass
@ -888,7 +897,7 @@ def set_branch_config(branch, option, value, scope='local'):
def set_config(option, value, scope='local'): def set_config(option, value, scope='local'):
scm.GIT.SetConfig(os.getcwd(), option, value, scope=scope) run('config', '--' + scope, option, value)
def get_dirty_files(): def get_dirty_files():
@ -1107,7 +1116,10 @@ def tree(treeref, recurse=False):
def get_remote_url(remote='origin'): def get_remote_url(remote='origin'):
return scm.GIT.GetConfig(os.getcwd(), 'remote.%s.url' % remote) try:
return run('config', 'remote.%s.url' % remote)
except subprocess2.CalledProcessError:
return None
def upstream(branch): def upstream(branch):

@ -48,7 +48,8 @@ def fetch_remotes(branch_tree):
tag_set = git.tags() tag_set = git.tags()
fetchspec_map = {} fetchspec_map = {}
all_fetchspec_configs = git.get_config_regexp(r'^remote\..*\.fetch') all_fetchspec_configs = git.get_config_regexp(r'^remote\..*\.fetch')
for key, fetchspec in all_fetchspec_configs: for fetchspec_config in all_fetchspec_configs:
key, _, fetchspec = fetchspec_config.partition(' ')
dest_spec = fetchspec.partition(':')[2] dest_spec = fetchspec.partition(':')[2]
remote_name = key.split('.')[1] remote_name = key.split('.')[1]
fetchspec_map[dest_spec] = remote_name fetchspec_map[dest_spec] = remote_name

@ -53,11 +53,11 @@ class BasicTests(unittest.TestCase):
@mock.patch('gclient_scm.scm.GIT.Capture') @mock.patch('gclient_scm.scm.GIT.Capture')
def testGetFirstRemoteUrl(self, mockCapture): def testGetFirstRemoteUrl(self, mockCapture):
REMOTE_STRINGS = [ REMOTE_STRINGS = [
('remote.origin.url = E:\\foo\\bar', 'E:\\foo\\bar'), ('remote.origin.url E:\\foo\\bar', 'E:\\foo\\bar'),
('remote.origin.url = /b/foo/bar', '/b/foo/bar'), ('remote.origin.url /b/foo/bar', '/b/foo/bar'),
('remote.origin.url = https://foo/bar', 'https://foo/bar'), ('remote.origin.url https://foo/bar', 'https://foo/bar'),
('remote.origin.url = E:\\Fo Bar\\bax', 'E:\\Fo Bar\\bax'), ('remote.origin.url E:\\Fo Bar\\bax', 'E:\\Fo Bar\\bax'),
('remote.origin.url = git://what/"do', 'git://what/"do') ('remote.origin.url git://what/"do', 'git://what/"do')
] ]
FAKE_PATH = '/fake/path' FAKE_PATH = '/fake/path'
mockCapture.side_effect = [question for question, _ in REMOTE_STRINGS] mockCapture.side_effect = [question for question, _ in REMOTE_STRINGS]
@ -65,11 +65,10 @@ class BasicTests(unittest.TestCase):
for _, answer in REMOTE_STRINGS: for _, answer in REMOTE_STRINGS:
self.assertEqual( self.assertEqual(
gclient_scm.SCMWrapper._get_first_remote_url(FAKE_PATH), answer) gclient_scm.SCMWrapper._get_first_remote_url(FAKE_PATH), answer)
gclient_scm.scm.GIT._clear_config(FAKE_PATH)
expected_calls = [ expected_calls = [
mock.call(['config', '--list'], cwd=FAKE_PATH, strip_out=False) mock.call(['config', '--local', '--get-regexp', r'remote.*.url'],
for _ in REMOTE_STRINGS cwd=FAKE_PATH) for _ in REMOTE_STRINGS
] ]
self.assertEqual(mockCapture.mock_calls, expected_calls) self.assertEqual(mockCapture.mock_calls, expected_calls)

@ -117,16 +117,12 @@ class CookiesAuthenticatorTest(unittest.TestCase):
os.path.expanduser(os.path.join('~', '.gitcookies')), os.path.expanduser(os.path.join('~', '.gitcookies')),
gerrit_util.CookiesAuthenticator().get_gitcookies_path()) gerrit_util.CookiesAuthenticator().get_gitcookies_path())
subprocess2.check_output.side_effect = [ subprocess2.check_output.side_effect = [b'http.cookiefile']
b'http.cookiefile = http.cookiefile'
]
self.assertEqual( self.assertEqual(
'http.cookiefile', 'http.cookiefile',
gerrit_util.CookiesAuthenticator().get_gitcookies_path()) gerrit_util.CookiesAuthenticator().get_gitcookies_path())
subprocess2.check_output.assert_called_with(['git', 'config', '--list'], subprocess2.check_output.assert_called_with(
cwd=os.getcwd(), ['git', 'config', '--path', 'http.cookiefile'])
env=mock.ANY,
stderr=mock.ANY)
os.getenv.return_value = 'git-cookies-path' os.getenv.return_value = 'git-cookies-path'
self.assertEqual( self.assertEqual(

@ -3107,6 +3107,7 @@ class TestGitCl(unittest.TestCase):
mock.patch('git_cl._GitCookiesChecker.get_hosts_with_creds', mock.patch('git_cl._GitCookiesChecker.get_hosts_with_creds',
lambda _, include_netrc=False: []).start() lambda _, include_netrc=False: []).start()
self.calls = [ self.calls = [
((['git', 'config', '--path', 'http.cookiefile'], ), CERR1),
((['git', 'config', '--global', 'http.cookiefile'], ), CERR1), ((['git', 'config', '--global', 'http.cookiefile'], ), CERR1),
(('os.path.exists', os.path.join('~', NETRC_FILENAME)), True), (('os.path.exists', os.path.join('~', NETRC_FILENAME)), True),
(('ask_for_data', 'Press Enter to setup .gitcookies, ' (('ask_for_data', 'Press Enter to setup .gitcookies, '
@ -3129,6 +3130,7 @@ class TestGitCl(unittest.TestCase):
custom_cookie_path = ('C:\\.gitcookies' if sys.platform == 'win32' else custom_cookie_path = ('C:\\.gitcookies' if sys.platform == 'win32' else
'/custom/.gitcookies') '/custom/.gitcookies')
self.calls = [ self.calls = [
((['git', 'config', '--path', 'http.cookiefile'], ), CERR1),
((['git', 'config', '--global', ((['git', 'config', '--global',
'http.cookiefile'], ), custom_cookie_path), 'http.cookiefile'], ), custom_cookie_path),
(('os.path.exists', custom_cookie_path), False), (('os.path.exists', custom_cookie_path), False),

@ -296,8 +296,7 @@ class GitReadOnlyFunctionsTest(git_test_utils.GitRepoReadOnlyTestBase,
def testDormant(self): def testDormant(self):
self.assertFalse(self.repo.run(self.gc.is_dormant, 'main')) self.assertFalse(self.repo.run(self.gc.is_dormant, 'main'))
self.gc.scm.GIT.SetConfig(self.repo.repo_path, 'branch.main.dormant', self.repo.git('config', 'branch.main.dormant', 'true')
'true')
self.assertTrue(self.repo.run(self.gc.is_dormant, 'main')) self.assertTrue(self.repo.run(self.gc.is_dormant, 'main'))
def testBlame(self): def testBlame(self):
@ -458,7 +457,6 @@ class GitMutableFunctionsTest(git_test_utils.GitRepoReadWriteTestBase,
[]) [])
self.repo.git('config', '--add', 'happy.derpies', 'cat') self.repo.git('config', '--add', 'happy.derpies', 'cat')
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual( self.assertEqual(
self.repo.run(self.gc.get_config_list, 'happy.derpies'), self.repo.run(self.gc.get_config_list, 'happy.derpies'),
['food', 'cat']) ['food', 'cat'])
@ -466,7 +464,8 @@ class GitMutableFunctionsTest(git_test_utils.GitRepoReadWriteTestBase,
self.assertEqual('cat', self.assertEqual('cat',
self.repo.run(self.gc.get_config, 'dude.bob', 'cat')) self.repo.run(self.gc.get_config, 'dude.bob', 'cat'))
self.gc.scm.GIT.SetConfig(self.repo.repo_path, 'dude.bob', 'dog') self.repo.run(self.gc.set_config, 'dude.bob', 'dog')
self.assertEqual('dog', self.assertEqual('dog',
self.repo.run(self.gc.get_config, 'dude.bob', 'cat')) self.repo.run(self.gc.get_config, 'dude.bob', 'cat'))
@ -482,19 +481,15 @@ class GitMutableFunctionsTest(git_test_utils.GitRepoReadWriteTestBase,
self.repo.git('config', 'depot-tools.upstream', 'catfood') self.repo.git('config', 'depot-tools.upstream', 'catfood')
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual('catfood', self.repo.run(self.gc.root)) self.assertEqual('catfood', self.repo.run(self.gc.root))
self.repo.git('config', '--add', 'core.fsmonitor', 'true') self.repo.git('config', '--add', 'core.fsmonitor', 'true')
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual(True, self.repo.run(self.gc.is_fsmonitor_enabled)) self.assertEqual(True, self.repo.run(self.gc.is_fsmonitor_enabled))
self.repo.git('config', '--add', 'core.fsmonitor', 't') self.repo.git('config', '--add', 'core.fsmonitor', 't')
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual(False, self.repo.run(self.gc.is_fsmonitor_enabled)) self.assertEqual(False, self.repo.run(self.gc.is_fsmonitor_enabled))
self.repo.git('config', '--add', 'core.fsmonitor', 'false') self.repo.git('config', '--add', 'core.fsmonitor', 'false')
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual(False, self.repo.run(self.gc.is_fsmonitor_enabled)) self.assertEqual(False, self.repo.run(self.gc.is_fsmonitor_enabled))
def testRoot(self): def testRoot(self):
@ -639,8 +634,8 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase,
_, rslt = self.repo.capture_stdio(list, self.gc.branches()) _, rslt = self.repo.capture_stdio(list, self.gc.branches())
self.assertIn('too many branches (39/20)', rslt) self.assertIn('too many branches (39/20)', rslt)
self.gc.scm.GIT.SetConfig(self.repo.repo_path,
'depot-tools.branch-limit', '100') self.repo.git('config', 'depot-tools.branch-limit', '100')
# should not raise # should not raise
# This check fails with git 2.4 (see crbug.com/487172) # This check fails with git 2.4 (see crbug.com/487172)
@ -659,7 +654,6 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase,
self.repo.run(self.gc.get_or_create_merge_base, 'branch_L', self.repo.run(self.gc.get_or_create_merge_base, 'branch_L',
'branch_K')) 'branch_K'))
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual( self.assertEqual(
self.repo['B'], self.repo['B'],
self.repo.run(self.gc.get_config, 'branch.branch_K.base')) self.repo.run(self.gc.get_config, 'branch.branch_K.base'))
@ -680,7 +674,6 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase,
self.repo.run(self.gc.manual_merge_base, 'branch_K', self.repo['I'], self.repo.run(self.gc.manual_merge_base, 'branch_K', self.repo['I'],
'branch_G') 'branch_G')
self.gc.scm.GIT._clear_config(self.repo.repo_path)
self.assertEqual( self.assertEqual(
self.repo['I'], self.repo['I'],
self.repo.run(self.gc.get_or_create_merge_base, 'branch_K', self.repo.run(self.gc.get_or_create_merge_base, 'branch_K',

Loading…
Cancel
Save