From 1c81122f695a406fa7d113f5c67c8bde7ba3c873 Mon Sep 17 00:00:00 2001 From: Scott Lee Date: Fri, 19 Jul 2024 01:45:13 +0000 Subject: [PATCH] [depot_tools] retry git config if it lock-fails If git config set is executed, it writes the new content into a temp file (but called lock-file) and then replace .gitconfig with it. However, if it cannot create the lock file, it returns an error. With this CL, if git config fails with the lock file error, depot_tools will retry it at most 5 times with 0.2s interval. It's found that there are applications, such as vscode extensions, executes `git config set` frequently, and those could often cause unexpected interruptions to ongoing `git rebase-updates` by the lock failure. Bug: 351950514 Change-Id: I985af0d8b7458dbf47cd6baa857dc5adccf15031 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5705561 Reviewed-by: Takuto Ikuta Reviewed-by: Josip Sokcevic Commit-Queue: Scott Lee --- git_common.py | 35 +++++++++++++++++- tests/git_common_test.py | 79 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/git_common.py b/git_common.py index d3e5b143f..e1e6c93fc 100644 --- a/git_common.py +++ b/git_common.py @@ -30,12 +30,14 @@ import contextlib import functools import logging import os +import random import re import setup_color import shutil import signal import tempfile import textwrap +import time import scm import subprocess2 @@ -856,10 +858,41 @@ def run_stream_with_retcode(*cmd, **kwargs): raise subprocess2.CalledProcessError(retcode, cmd, os.getcwd(), b'', b'') - def run_with_stderr(*cmd, **kwargs): """Runs a git command. + Returns (stdout, stderr) as a pair of strings. + If the command is `config` and the execution fails due to a lock failure, + retry the execution at most 5 times with 0.2 interval. + + kwargs + autostrip (bool) - Strip the output. Defaults to True. + indata (str) - Specifies stdin data for the process. + retry_lock (bool) - If true and the command is `config`, + retry on lock failures. Defaults to True. + """ + retry_cnt = 0 + if kwargs.pop('retry_lock', True) and len(cmd) > 0 and cmd[0] == 'config': + retry_cnt = 5 + + while True: + try: + return _run_with_stderr(*cmd, **kwargs) + except subprocess2.CalledProcessError as ex: + lock_err = 'could not lock config file .git/config: File exists' + if retry_cnt > 0 and lock_err in str(ex): + logging.error(ex) + jitter = random.uniform(0, 0.2) + time.sleep(0.1 + jitter) + retry_cnt -= 1 + continue + + raise ex + + +def _run_with_stderr(*cmd, **kwargs): + """Runs a git command. + Returns (stdout, stderr) as a pair of strings. kwargs diff --git a/tests/git_common_test.py b/tests/git_common_test.py index 786010031..03a826172 100755 --- a/tests/git_common_test.py +++ b/tests/git_common_test.py @@ -22,6 +22,7 @@ from unittest import mock DEPOT_TOOLS_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, DEPOT_TOOLS_ROOT) +import subprocess2 from testing_support import coverage_utils from testing_support import git_test_utils @@ -258,7 +259,6 @@ class GitReadOnlyFunctionsTest(git_test_utils.GitRepoReadOnlyTestBase, self.repo.run(testfn) def testStreamWithRetcodeException(self): - import subprocess2 with self.assertRaises(subprocess2.CalledProcessError): with self.gc.run_stream_with_retcode('checkout', 'unknown-branch'): pass @@ -1173,6 +1173,83 @@ class WarnSubmoduleTest(unittest.TestCase): sys.stdout.getvalue()) +@mock.patch('time.sleep') +@mock.patch('git_common._run_with_stderr') +class RunWithStderr(GitCommonTestBase): + + def setUp(self): + super(RunWithStderr, self).setUp() + msg = 'error: could not lock config file .git/config: File exists' + self.lock_failure = self.calledProcessError(msg) + msg = 'error: wrong number of arguments, should be 2' + self.wrong_param = self.calledProcessError(msg) + + def calledProcessError(self, stderr): + return subprocess2.CalledProcessError( + 2, + cmd=['a', 'b', 'c'], # doesn't matter + cwd='/', + stdout='', + stderr=stderr.encode('utf-8'), + ) + + def runGitCheckout(self, ex, retry_lock): + with self.assertRaises(type(ex)): + self.gc.run_with_stderr('checkout', 'a', retry_lock=retry_lock) + + def runGitConfig(self, ex, retry_lock): + with self.assertRaises(type(ex)): + self.gc.run_with_stderr('config', 'set', retry_lock=retry_lock) + + def test_config_with_non_lock_failure(self, run_mock, _): + """Tests git-config with a non-lock-failure.""" + ex = self.wrong_param + run_mock.side_effect = ex + # retry_lock == True + self.runGitConfig(ex, retry_lock=True) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + # retry_lock == False + run_mock.reset_mock() + self.runGitConfig(ex, retry_lock=False) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + + def test_config_with_lock_failure(self, run_mock, _): + """Tests git-config with a lock-failure.""" + ex = self.lock_failure + run_mock.side_effect = ex + # retry_lock == True + self.runGitConfig(ex, retry_lock=True) + self.assertEqual(run_mock.call_count, 6) # 1 + 5 (retry) + # retry_lock == False + run_mock.reset_mock() + self.runGitConfig(ex, retry_lock=False) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + + def test_checkout_with_non_lock_failure(self, run_mock, _): + """Tests git-checkout with a non-lock-failure.""" + ex = self.wrong_param + run_mock.side_effect = ex + # retry_lock == True + self.runGitCheckout(ex, retry_lock=True) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + # retry_lock == False + run_mock.reset_mock() + self.runGitCheckout(ex, retry_lock=False) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + + def test_checkout_with_lock_failure(self, run_mock, _): + """Tests git-checkout with a lock-failure.""" + ex = self.lock_failure + run_mock.side_effect = ex + # retry_lock == True + self.runGitCheckout(ex, retry_lock=True) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + # retry_lock == False + run_mock.reset_mock() + self.runGitCheckout(ex, retry_lock=False) + self.assertEqual(run_mock.call_count, 1) # 1 + 0 (retry) + + if __name__ == '__main__': sys.exit( coverage_utils.covered_main(