From 062ecac69f7149d88a467eef91f707f441d62b1d Mon Sep 17 00:00:00 2001 From: Bruce Dawson Date: Thu, 23 May 2024 06:14:38 +0000 Subject: [PATCH] Use git_common to call git In preparation for some Windows optimizations to how git_common calls git it is important to use git_common more widely, specifically from scm.py and gclient_scm.py. This change updates scm.py and gclient_scm.py and updates the associated tests: Test command lines used when updating the tests include: vpython3 tests/gclient_scm_test.py ManagedGitWrapperTestCaseMock.testUpdateConflict vpython3 tests/gclient_scm_test.py GerritChangesTest.testRecoversAfterPatchFailure vpython3 tests/gerrit_util_test.py CookiesAuthenticatorTest.testGetGitcookiesPath Bug: 332982922 Change-Id: I7aacb110b2888c164259815385cd77e26942adc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5478509 Commit-Queue: Bruce Dawson Reviewed-by: Gavin Mak --- gclient_scm.py | 5 ++-- scm.py | 13 ++++---- tests/gclient_scm_test.py | 63 ++++++++++++++++++++++----------------- tests/gerrit_util_test.py | 18 +++++------ 4 files changed, 52 insertions(+), 47 deletions(-) diff --git a/gclient_scm.py b/gclient_scm.py index b56bacc9c..1b881d894 100644 --- a/gclient_scm.py +++ b/gclient_scm.py @@ -22,6 +22,7 @@ import traceback import gclient_utils import gerrit_util import git_cache +import git_common import scm import subprocess2 @@ -1578,8 +1579,8 @@ class GitWrapper(SCMWrapper): env.setdefault( 'GIT_DIR', os.path.abspath(os.path.join(self.checkout_path, '.git'))) - ret = subprocess2.check_output(['git'] + args, env=env, - **kwargs).decode('utf-8') + kwargs.setdefault('env', env) + ret = git_common.run(*args, **kwargs) if strip: ret = ret.strip() self.Print('Finished running: %s %s' % ('git', ' '.join(args))) diff --git a/scm.py b/scm.py index 4e8123056..67f4806ab 100644 --- a/scm.py +++ b/scm.py @@ -10,6 +10,7 @@ import re from typing import Mapping, List import gclient_utils +import git_common import subprocess2 # TODO: Should fix these warnings. @@ -105,14 +106,10 @@ class GIT(object): @staticmethod def Capture(args, cwd=None, strip_out=True, **kwargs): - env = GIT.ApplyEnvVars(kwargs) - output = subprocess2.check_output(['git'] + args, - cwd=cwd, - stderr=subprocess2.PIPE, - env=env, - **kwargs) - output = output.decode('utf-8', 'replace') - return output.strip() if strip_out else output + kwargs.setdefault('env', GIT.ApplyEnvVars(kwargs)) + kwargs.setdefault('cwd', cwd) + kwargs.setdefault('autostrip', strip_out) + return git_common.run(*args, **kwargs) @staticmethod def CaptureStatus(cwd, diff --git a/tests/gclient_scm_test.py b/tests/gclient_scm_test.py index 0eea289fa..207a8fb9c 100755 --- a/tests/gclient_scm_test.py +++ b/tests/gclient_scm_test.py @@ -22,6 +22,7 @@ sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import gclient_scm import gclient_utils import git_cache +import git_common import subprocess2 from testing_support import fake_repos from testing_support import test_case_utils @@ -428,9 +429,8 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase): # and parents instead. self.assertEqual(scm._Capture(['rev-parse', 'HEAD:']), '3a3ba72731fa208d37b06598a129ba93970325df') - parent = 'HEAD^' if sys.platform != 'win32' else 'HEAD^^' - self.assertEqual(scm._Capture(['rev-parse', parent + '1']), rev) - self.assertEqual(scm._Capture(['rev-parse', parent + '2']), + self.assertEqual(scm._Capture(['rev-parse', 'HEAD^1']), rev) + self.assertEqual(scm._Capture(['rev-parse', 'HEAD^2']), scm._Capture(['rev-parse', 'origin/main'])) sys.stdout.close() @@ -456,8 +456,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase): # and parent instead. self.assertEqual(scm._Capture(['rev-parse', 'HEAD:']), '3a3ba72731fa208d37b06598a129ba93970325df') - parent = 'HEAD^' if sys.platform != 'win32' else 'HEAD^^' - self.assertEqual(scm._Capture(['rev-parse', parent + '1']), + self.assertEqual(scm._Capture(['rev-parse', 'HEAD^1']), scm._Capture(['rev-parse', 'origin/main'])) sys.stdout.close() @@ -673,28 +672,32 @@ class ManagedGitWrapperTestCaseMock(unittest.TestCase): @mock.patch('gclient_scm.GitWrapper._Clone') @mock.patch('os.path.isdir') @mock.patch('os.path.exists') - @mock.patch('subprocess2.check_output') - def testUpdateNoDotGit(self, mockCheckOutput, mockExists, mockIsdir, - mockClone): + @mock.patch('git_common.run') + def testUpdateNoDotGit(self, mockRun, mockExists, mockIsdir, mockClone): mockIsdir.side_effect = lambda path: path == self.base_path mockExists.side_effect = lambda path: path == self.base_path - mockCheckOutput.side_effect = [b'refs/remotes/origin/main', b'', b''] + mockRun.side_effect = ['refs/remotes/origin/main', '', ''] options = self.Options() scm = gclient_scm.GitWrapper(self.url, self.root_dir, self.relpath) scm.update(options, None, []) env = gclient_scm.scm.GIT.ApplyEnvVars({}) - self.assertEqual(mockCheckOutput.mock_calls, [ - mock.call(['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'], + self.assertEqual(mockRun.mock_calls, [ + mock.call('symbolic-ref', + 'refs/remotes/origin/HEAD', + autostrip=True, cwd=self.base_path, - env=env, - stderr=-1), - mock.call(['git', '-c', 'core.quotePath=false', 'ls-files'], + env=env), + mock.call('-c', + 'core.quotePath=false', + 'ls-files', cwd=self.base_path, env=env, stderr=-1), - mock.call(['git', 'rev-parse', '--verify', 'HEAD'], + mock.call('rev-parse', + '--verify', + 'HEAD', cwd=self.base_path, env=env, stderr=-1), @@ -706,15 +709,14 @@ class ManagedGitWrapperTestCaseMock(unittest.TestCase): @mock.patch('gclient_scm.GitWrapper._Clone') @mock.patch('os.path.isdir') @mock.patch('os.path.exists') - @mock.patch('subprocess2.check_output') - def testUpdateConflict(self, mockCheckOutput, mockExists, mockIsdir, - mockClone): + @mock.patch('git_common.run') + def testUpdateConflict(self, mockRun, mockExists, mockIsdir, mockClone): mockIsdir.side_effect = lambda path: path == self.base_path mockExists.side_effect = lambda path: path == self.base_path - mockCheckOutput.side_effect = [b'refs/remotes/origin/main', b'', b''] + mockRun.side_effect = ['refs/remotes/origin/main', '', ''] mockClone.side_effect = [ - gclient_scm.subprocess2.CalledProcessError(None, None, None, None, - None), + git_common.subprocess2.CalledProcessError(None, None, None, None, + None), None, ] @@ -723,16 +725,21 @@ class ManagedGitWrapperTestCaseMock(unittest.TestCase): scm.update(options, None, []) env = gclient_scm.scm.GIT.ApplyEnvVars({}) - self.assertEqual(mockCheckOutput.mock_calls, [ - mock.call(['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'], + self.assertEqual(mockRun.mock_calls, [ + mock.call('symbolic-ref', + 'refs/remotes/origin/HEAD', + autostrip=True, cwd=self.base_path, - env=env, - stderr=-1), - mock.call(['git', '-c', 'core.quotePath=false', 'ls-files'], + env=env), + mock.call('-c', + 'core.quotePath=false', + 'ls-files', cwd=self.base_path, env=env, stderr=-1), - mock.call(['git', 'rev-parse', '--verify', 'HEAD'], + mock.call('rev-parse', + '--verify', + 'HEAD', cwd=self.base_path, env=env, stderr=-1), @@ -1502,7 +1509,7 @@ class GerritChangesTest(fake_repos.FakeReposTestBase): with self.assertRaises(subprocess2.CalledProcessError) as cm: scm.apply_patch_ref(self.url, 'refs/changes/36/1236/1', 'refs/heads/main', self.options, file_list) - self.assertEqual(cm.exception.cmd[:2], ['git', 'cherry-pick']) + self.assertEqual(cm.exception.cmd[3], 'cherry-pick') self.assertIn(b'error: could not apply', cm.exception.stderr) # Try to apply 'refs/changes/35/1235/1', which doesn't have a merge diff --git a/tests/gerrit_util_test.py b/tests/gerrit_util_test.py index 0862c5807..3d8c15a28 100755 --- a/tests/gerrit_util_test.py +++ b/tests/gerrit_util_test.py @@ -16,6 +16,7 @@ from unittest import mock sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import gerrit_util +import git_common import metrics import subprocess2 @@ -86,7 +87,7 @@ class CookiesAuthenticatorTest(unittest.TestCase): mock.patch('os.environ', {'HOME': '$HOME'}).start() mock.patch('os.path.exists', return_value=True).start() mock.patch( - 'subprocess2.check_output', + 'git_common.run', side_effect=[ subprocess2.CalledProcessError(1, ['cmd'], 'cwd', 'out', 'err') ], @@ -117,17 +118,16 @@ class CookiesAuthenticatorTest(unittest.TestCase): os.path.expanduser(os.path.join('~', '.gitcookies')), gerrit_util.CookiesAuthenticator().get_gitcookies_path()) - subprocess2.check_output.side_effect = [ - b'http.cookiefile\nhttp.cookiefile\x00' - ] + git_common.run.side_effect = ['http.cookiefile\nhttp.cookiefile\x00'] self.assertEqual( 'http.cookiefile', gerrit_util.CookiesAuthenticator().get_gitcookies_path()) - subprocess2.check_output.assert_called_with( - ['git', 'config', '--list', '-z'], - cwd=os.getcwd(), - env=mock.ANY, - stderr=mock.ANY) + git_common.run.assert_called_with('config', + '--list', + '-z', + autostrip=False, + cwd=os.getcwd(), + env=mock.ANY) os.getenv.return_value = 'git-cookies-path' self.assertEqual(