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 <brucedawson@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>

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.setdefault('env', env)
-                                       **kwargs).decode('utf-8')
+        ret = git_common.run(*args, **kwargs)
         if strip:
             ret = ret.strip()
         self.Print('Finished running: %s %s' % ('git', ' '.join(args)))

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)
+        kwargs.setdefault('env', GIT.ApplyEnvVars(kwargs))
-        output = subprocess2.check_output(['git'] + args,
+        kwargs.setdefault('cwd', cwd)
-                                          cwd=cwd,
+        kwargs.setdefault('autostrip', strip_out)
-                                          stderr=subprocess2.PIPE,
+        return git_common.run(*args, **kwargs)
-                                          env=env,
-                                          **kwargs)
-        output = output.decode('utf-8', 'replace')
-        return output.strip() if strip_out else output
 
     @staticmethod
     def CaptureStatus(cwd,

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', 'HEAD^1']), rev)
-        self.assertEqual(scm._Capture(['rev-parse', parent + '1']), rev)
+        self.assertEqual(scm._Capture(['rev-parse', 'HEAD^2']),
-        self.assertEqual(scm._Capture(['rev-parse', parent + '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', 'HEAD^1']),
-        self.assertEqual(scm._Capture(['rev-parse', parent + '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')
+    @mock.patch('git_common.run')
-    def testUpdateNoDotGit(self, mockCheckOutput, mockExists, mockIsdir,
+    def testUpdateNoDotGit(self, mockRun, mockExists, mockIsdir, mockClone):
-                           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, [
+        self.assertEqual(mockRun.mock_calls, [
-            mock.call(['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'],
+            mock.call('symbolic-ref',
+                      'refs/remotes/origin/HEAD',
+                      autostrip=True,
+                      cwd=self.base_path,
+                      env=env),
+            mock.call('-c',
+                      'core.quotePath=false',
+                      'ls-files',
                       cwd=self.base_path,
                       env=env,
                       stderr=-1),
-            mock.call(['git', '-c', 'core.quotePath=false', 'ls-files'],
+            mock.call('rev-parse',
-                      cwd=self.base_path,
+                      '--verify',
-                      env=env,
+                      'HEAD',
-                      stderr=-1),
-            mock.call(['git', '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')
+    @mock.patch('git_common.run')
-    def testUpdateConflict(self, mockCheckOutput, mockExists, mockIsdir,
+    def testUpdateConflict(self, mockRun, mockExists, mockIsdir, mockClone):
-                           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,
+            git_common.subprocess2.CalledProcessError(None, 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, [
+        self.assertEqual(mockRun.mock_calls, [
-            mock.call(['git', 'symbolic-ref', 'refs/remotes/origin/HEAD'],
+            mock.call('symbolic-ref',
+                      'refs/remotes/origin/HEAD',
+                      autostrip=True,
+                      cwd=self.base_path,
+                      env=env),
+            mock.call('-c',
+                      'core.quotePath=false',
+                      'ls-files',
                       cwd=self.base_path,
                       env=env,
                       stderr=-1),
-            mock.call(['git', '-c', 'core.quotePath=false', 'ls-files'],
+            mock.call('rev-parse',
-                      cwd=self.base_path,
+                      '--verify',
-                      env=env,
+                      'HEAD',
-                      stderr=-1),
-            mock.call(['git', '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

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 = [
+        git_common.run.side_effect = ['http.cookiefile\nhttp.cookiefile\x00']
-            b'http.cookiefile\nhttp.cookiefile\x00'
-        ]
         self.assertEqual(
             'http.cookiefile',
             gerrit_util.CookiesAuthenticator().get_gitcookies_path())
-        subprocess2.check_output.assert_called_with(
+        git_common.run.assert_called_with('config',
-            ['git', 'config', '--list', '-z'],
+                                          '--list',
-            cwd=os.getcwd(),
+                                          '-z',
-            env=mock.ANY,
+                                          autostrip=False,
-            stderr=mock.ANY)
+                                          cwd=os.getcwd(),
+                                          env=mock.ANY)
 
         os.getenv.return_value = 'git-cookies-path'
         self.assertEqual(