From 4244c70855851f4a6baa0b27c5e411c383227f57 Mon Sep 17 00:00:00 2001 From: Josiah Kiehl Date: Thu, 11 Dec 2025 18:23:44 -0800 Subject: [PATCH] Handle Windows command line limits in CheckForCommitObjects. On Windows, the command line has a character limit of 8191. When checking for commit objects in submodules, the `git ls-tree` command can become very long if many files are affected, potentially exceeding this limit. This change modifies `CheckForCommitObjects` to check the estimated length of the `git ls-tree` command when running on Windows. If the command line with all affected files would exceed the limit, it falls back to using a recursive `ls-tree` (`-r`) instead of listing each file individually. This prevents command line overflow errors on Windows. Change-Id: I6a340baefee57f5933473add0601a42ff1e61bb6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7254474 Auto-Submit: Josiah Kiehl Commit-Queue: Josiah Kiehl Reviewed-by: Yiwei Zhang --- presubmit_canned_checks.py | 19 +++++++--- .../presubmit_canned_checks_test_mocks.py | 4 ++ tests/presubmit_canned_checks_test.py | 38 +++++++++++++++++++ tests/presubmit_unittest.py | 9 +++-- 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 559859a774..7871a03427 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -75,6 +75,10 @@ _CORP_LINK_KEYWORD = '.corp.google' # See https://git-scm.com/docs/git-ls-tree#_output_format _GIT_MODE_SUBMODULE = b'160000' +# Windows command line character limit. See +# https://learn.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/command-line-string-limitation +_WIN_COMMAND_LINE_CHAR_LIMIT = 8191 + ### Description checks @@ -316,7 +320,7 @@ def CheckChangeLintsClean(input_api, # Use VS error format on Windows to make it easier to step through the # results. - if input_api.platform == 'win32': + if input_api.is_windows: cpplint._SetOutputFormat('vs7') if source_file_filter == None: @@ -1329,7 +1333,7 @@ def _FetchAllFiles(input_api, files_to_check, files_to_skip): # can break another unmodified file. # Use code similar to InputApi.FilterSourceFile() def Find(filepath, filters): - if input_api.platform == 'win32': + if input_api.is_windows: filepath = filepath.replace('\\', '/') for item in filters: @@ -1435,7 +1439,7 @@ def GetPylint(input_api, # the command-line, so we pass arguments via a pipe. tool = input_api.os_path.join(_HERE, 'pylint-' + version) kwargs = {'env': env} - if input_api.platform == 'win32': + if input_api.is_windows: # On Windows, scripts on the current directory take precedence over # PATH. When `pylint.bat` calls `vpython3`, it will execute the # `vpython3` of the depot_tools under test instead of the one in the @@ -2085,7 +2089,12 @@ def CheckForCommitObjects(input_api, output_api): input_api.os_path.relpath(f.AbsoluteLocalPath(), repo_root) for f in affected_files ] - cmd.extend(['--'] + files_to_check) + # On Windows, the command line is limited to 8191 characters. + cmd_len = len(' '.join(cmd + ['--'] + files_to_check)) + if (input_api.is_windows and cmd_len > _WIN_COMMAND_LINE_CHAR_LIMIT): + cmd.extend(['-r']) + else: + cmd.extend(['--'] + files_to_check) else: cmd.extend(['-r']) @@ -2844,7 +2853,7 @@ def CheckInclusiveLanguage(input_api, local_dir = input_api.os_path.dirname(affected_file.LocalPath()) # Excluded paths use forward slashes. - if input_api.platform == 'win32': + if input_api.is_windows: local_dir = local_dir.replace('\\', '/') return local_dir in excluded_paths diff --git a/testing_support/presubmit_canned_checks_test_mocks.py b/testing_support/presubmit_canned_checks_test_mocks.py index 4d2010ff1a..83c7a4d89e 100644 --- a/testing_support/presubmit_canned_checks_test_mocks.py +++ b/testing_support/presubmit_canned_checks_test_mocks.py @@ -81,6 +81,10 @@ class MockInputApi(object): self.presubmit_local_path = os.path.dirname(__file__) self.logging = logging.getLogger('PRESUBMIT') + @property + def is_windows(self): + return self.platform == 'win32' + def CreateMockFileInPath(self, f_list): self.os_path.exists = lambda x: x in f_list diff --git a/tests/presubmit_canned_checks_test.py b/tests/presubmit_canned_checks_test.py index 6c2353bd84..a486f5003b 100755 --- a/tests/presubmit_canned_checks_test.py +++ b/tests/presubmit_canned_checks_test.py @@ -910,6 +910,44 @@ class CheckForCommitObjectsTest(unittest.TestCase): self.assertEqual(1, len(results)) self.assertIn('submodule', results[0].items) + def testWindowsCommandLineLimit(self): + # On Windows, if the command line is too long, we should fall back to a + # recursive ls-tree. + self.input_api.platform = 'win32' + self.input_api.files = [ + MockAffectedFile('a' * 100, '') for i in range(100) + ] + self.input_api.subprocess.check_output.return_value = b'' + + presubmit_canned_checks.CheckForCommitObjects( + self.input_api, self.output_api) + + # The first call is to `git show HEAD:DEPS`. + # The second call is to `git ls-tree`. + self.assertEqual(2, self.input_api.subprocess.check_output.call_count) + ls_tree_cmd = self.input_api.subprocess.check_output.call_args_list[1][0][0] + self.assertIn('-r', ls_tree_cmd) + + def testWindowsCommandLineNotTooLong(self): + # On Windows, if the command line is not too long, we should pass the + # file list. + self.input_api.platform = 'win32' + self.input_api.files = [ + MockAffectedFile('foo.txt', '') + ] + self.input_api.subprocess.check_output.return_value = b'' + + presubmit_canned_checks.CheckForCommitObjects( + self.input_api, self.output_api) + + # The first call is to `git show HEAD:DEPS`. + # The second call is to `git ls-tree`. + self.assertEqual(2, self.input_api.subprocess.check_output.call_count) + ls_tree_cmd = self.input_api.subprocess.check_output.call_args_list[1][0][0] + self.assertNotIn('-r', ls_tree_cmd) + self.assertIn('--', ls_tree_cmd) + self.assertIn('foo.txt', ls_tree_cmd) + if __name__ == '__main__': unittest.main() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 9e3ed41748..d4ed8648bc 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -625,7 +625,7 @@ class PresubmitUnittest(PresubmitTestsBase): gerrit_obj=None, verbose=False)) expected = (r'Running post upload checks \.\.\.\n') - self.assertRegexpMatches(sys.stdout.getvalue(), expected) + self.assertRegex(sys.stdout.getvalue(), expected) def testDoPostUploadExecuterWarning(self): path = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') @@ -645,7 +645,7 @@ class PresubmitUnittest(PresubmitTestsBase): '??\n' '\n', sys.stdout.getvalue()) - def testDoPostUploadExecuterWarning(self): + def testDoPostUploadExecuterError(self): path = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') os.path.isfile.side_effect = lambda f: f == path os.listdir.return_value = ['PRESUBMIT.py'] @@ -662,7 +662,7 @@ class PresubmitUnittest(PresubmitTestsBase): '\*\* Post Upload Hook Messages \*\*\n' '!!\n' '\n') - self.assertRegexpMatches(sys.stdout.getvalue(), expected) + self.assertRegex(sys.stdout.getvalue(), expected) def testDoPresubmitChecksNoWarningsOrErrors(self): haspresubmit_path = os.path.join(self.fake_root_dir, 'haspresubmit', @@ -3094,6 +3094,7 @@ the current line as well! pylintrc = os.path.join(_ROOT, 'pylintrc-2.7') env = {str('PYTHONPATH'): str('')} if sys.platform == 'win32': + input_api.is_windows = True pylint += '.bat' results = presubmit_canned_checks.RunPylint(input_api, @@ -3300,7 +3301,7 @@ the current line as well! for result in results: result.handle() if expected_output: - self.assertRegexpMatches(sys.stdout.getvalue(), expected_output) + self.assertRegex(sys.stdout.getvalue(), expected_output) else: self.assertEqual(sys.stdout.getvalue(), expected_output) sys.stdout.truncate(0)