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 <kiehl@google.com>
Commit-Queue: Josiah Kiehl <kiehl@google.com>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>

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

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
 

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()

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)