[depot_tools] add file parameter support in git cl lint

Currently, git cl lint runs git commands to retrieve a list of
the affected files and runs the lint tool against the files.

This CL updates the command line argument interface such that,
if file paths are given in positional arguments, git cl will
just run the lint tool against the given files w/o executing any
git commands.

The intent of this CL is to make git cl lint runnable out of Git
checkouts.

Bug: 324595694
Change-Id: I0a68e33931f84c2441da53bf880a2d18a5526ae4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5454166
Commit-Queue: Scott Lee <ddoman@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

git_cl.py

@@ -4581,21 +4581,72 @@ def CMDdescription(parser, args):
     return 0
 
 
+def FindFilesForLint(options, args):
+    """Returns the base folder and a list of files to lint."""
+    files = []
+    cwd = os.getcwd()
+    if len(args) > 0:
+        # If file paths are given in positional args, run the lint tools
+        # against them without using git commands. This allows git_cl.py
+        # runnable against any files even out of git checkouts.
+        for fn in args:
+            if os.path.isfile(fn):
+                files.append(fn)
+            else:
+                print('%s is not a file' % fn)
+                return None, None
+    else:
+        # If file names are omitted, use the git APIs to find the files to lint.
+        include_regex = re.compile(settings.GetLintRegex())
+        ignore_regex = re.compile(settings.GetLintIgnoreRegex())
+        cl = Changelist()
+        cwd = settings.GetRoot()
+        affectedFiles = cl.GetAffectedFiles(cl.GetCommonAncestorWithUpstream())
+        if not affectedFiles:
+            print('Cannot lint an empty CL')
+            return None, None
+
+        for fn in affectedFiles:
+            if not include_regex.match(fn):
+                print('Skipping file %s' % fn)
+            elif ignore_regex.match(fn):
+                print('Ignoring file %s' % fn)
+            else:
+                files.append(fn)
+
+    return cwd, files
+
+
+@subcommand.usage('[files ...]')
 @metrics.collector.collect_metrics('git cl lint')
 def CMDlint(parser, args):
-    """Runs cpplint on the current changelist."""
+    """Runs cpplint on the current changelist or given files.
+
+    positional arguments:
+      files           Files to lint. If omitted, it will run cpplint against
+                      all the affected files in the current git checkout.
+    """
     parser.add_option(
         '--filter',
         action='append',
         metavar='-x,+y',
         help='Comma-separated list of cpplint\'s category-filters')
     options, args = parser.parse_args(args)
+    root_path, files = FindFilesForLint(options, args)
+    if files is None:
+        return 1
 
     # Access to a protected member _XX of a client class
     # pylint: disable=protected-access
     try:
         import cpplint
         import cpplint_chromium
+
+        # Process cpplint arguments, if any.
+        filters = presubmit_canned_checks.GetCppLintFilters(options.filter)
+        command = ['--filter=' + ','.join(filters)]
+        command.extend(files)
+        files_to_lint = cpplint.ParseArguments(command)
     except ImportError:
         print(
             'Your depot_tools is missing cpplint.py and/or cpplint_chromium.py.'
@@ -4605,39 +4656,17 @@ def CMDlint(parser, args):
     # Change the current working directory before calling lint so that it
     # shows the correct base.
     previous_cwd = os.getcwd()
-    os.chdir(settings.GetRoot())
     try:
-        cl = Changelist()
-        files = cl.GetAffectedFiles(cl.GetCommonAncestorWithUpstream())
-        if not files:
-            print('Cannot lint an empty CL')
-            return 1
-
-        # Process cpplint arguments, if any.
-        filters = presubmit_canned_checks.GetCppLintFilters(options.filter)
-        command = ['--filter=' + ','.join(filters)]
-        command.extend(args)
-        command.extend(files)
-        filenames = cpplint.ParseArguments(command)
-
-        include_regex = re.compile(settings.GetLintRegex())
-        ignore_regex = re.compile(settings.GetLintIgnoreRegex())
+        os.chdir(root_path)
         extra_check_functions = [
             cpplint_chromium.CheckPointerDeclarationWhitespace
         ]
-        for filename in filenames:
-            if not include_regex.match(filename):
-                print('Skipping file %s' % filename)
-                continue
-
-            if ignore_regex.match(filename):
-                print('Ignoring file %s' % filename)
-                continue
-
-            cpplint.ProcessFile(filename, cpplint._cpplint_state.verbose_level,
+        for file in files_to_lint:
+            cpplint.ProcessFile(file, cpplint._cpplint_state.verbose_level,
                                 extra_check_functions)
     finally:
         os.chdir(previous_cwd)
+
     print('Total errors found: %d\n' % cpplint._cpplint_state.error_count)
     if cpplint._cpplint_state.error_count != 0:
         return 1

tests/git_cl_test.py

@@ -5,13 +5,14 @@
 # found in the LICENSE file.
 """Unit tests for git_cl.py."""
 
+import codecs
 import datetime
 import json
 import logging
-from io import StringIO
 import multiprocessing
 import optparse
 import os
+import io
 import shutil
 import sys
 import tempfile
@@ -196,8 +197,8 @@ class SystemExitMock(Exception):
 class TestGitClBasic(unittest.TestCase):
     def setUp(self):
         mock.patch('sys.exit', side_effect=SystemExitMock).start()
-        mock.patch('sys.stdout', StringIO()).start()
-        mock.patch('sys.stderr', StringIO()).start()
+        mock.patch('sys.stdout', io.StringIO()).start()
+        mock.patch('sys.stderr', io.StringIO()).start()
         self.addCleanup(mock.patch.stopall)
 
     def test_die_with_error(self):
@@ -524,7 +525,7 @@ class GitCookiesCheckerTest(unittest.TestCase):
         super(GitCookiesCheckerTest, self).setUp()
         self.c = git_cl._GitCookiesChecker()
         self.c._all_hosts = []
-        mock.patch('sys.stdout', StringIO()).start()
+        mock.patch('sys.stdout', io.StringIO()).start()
         self.addCleanup(mock.patch.stopall)
 
     def mock_hosts_creds(self, subhost_identity_pairs):
@@ -613,7 +614,7 @@ class TestGitCl(unittest.TestCase):
         self.calls = []
         self._calls_done = []
         self.failed = False
-        mock.patch('sys.stdout', StringIO()).start()
+        mock.patch('sys.stdout', io.StringIO()).start()
         mock.patch('git_cl.time_time',
                    lambda: self._mocked_call('time.time')).start()
         mock.patch('git_cl.metrics.collector.add_repeated',
@@ -732,15 +733,15 @@ class TestGitCl(unittest.TestCase):
             result = result.encode('utf-8')
         return result
 
-    @mock.patch('sys.stdin', StringIO('blah\nye\n'))
-    @mock.patch('sys.stdout', StringIO())
+    @mock.patch('sys.stdin', io.StringIO('blah\nye\n'))
+    @mock.patch('sys.stdout', io.StringIO())
     def test_ask_for_explicit_yes_true(self):
         self.assertTrue(git_cl.ask_for_explicit_yes('prompt'))
         self.assertEqual('prompt [Yes/No]: Please, type yes or no: ',
                          sys.stdout.getvalue())
 
     def test_LoadCodereviewSettingsFromFile_gerrit(self):
-        codereview_file = StringIO('GERRIT_HOST: true')
+        codereview_file = io.StringIO('GERRIT_HOST: true')
         self.calls = [
             ((['git', 'config', '--unset-all', 'rietveld.cc'], ), CERR1),
             ((['git', 'config', '--unset-all',
@@ -2013,8 +2014,8 @@ class TestGitCl(unittest.TestCase):
 
     @mock.patch('git_cl.RunGit')
     @mock.patch('git_cl.CMDupload')
-    @mock.patch('sys.stdin', StringIO('\n'))
-    @mock.patch('sys.stdout', StringIO())
+    @mock.patch('sys.stdin', io.StringIO('\n'))
+    @mock.patch('sys.stdout', io.StringIO())
     def test_upload_branch_deps(self, *_mocks):
         def mock_run_git(*args, **_kwargs):
             if args[0] == [
@@ -2454,7 +2455,7 @@ class TestGitCl(unittest.TestCase):
             ]), 0)
         self.assertIssueAndPatchset(patchset='1', git_short_host='else')
 
-    @mock.patch('sys.stderr', StringIO())
+    @mock.patch('sys.stderr', io.StringIO())
     def test_patch_gerrit_conflict(self):
         self._patch_common()
         self.calls += [
@@ -2471,7 +2472,7 @@ class TestGitCl(unittest.TestCase):
 
     @mock.patch('gerrit_util.GetChangeDetail',
                 side_effect=gerrit_util.GerritError(404, ''))
-    @mock.patch('sys.stderr', StringIO())
+    @mock.patch('sys.stderr', io.StringIO())
     def test_patch_gerrit_not_exists(self, *_mocks):
         self.mockGit.config['remote.origin.url'] = (
             'https://chromium.googlesource.com/my/repo')
@@ -2525,7 +2526,7 @@ class TestGitCl(unittest.TestCase):
         cl.branchref = 'refs/heads/main'
         return cl
 
-    @mock.patch('sys.stderr', StringIO())
+    @mock.patch('sys.stderr', io.StringIO())
     def test_gerrit_ensure_authenticated_missing(self):
         cl = self._test_gerrit_ensure_authenticated_common(
             auth={
@@ -2672,7 +2673,7 @@ class TestGitCl(unittest.TestCase):
         self.assertEqual(0, git_cl.main(['description', '-d']))
         self.assertEqual('foo\n', sys.stdout.getvalue())
 
-    @mock.patch('sys.stderr', StringIO())
+    @mock.patch('sys.stderr', io.StringIO())
     def test_StatusFieldOverrideIssueMissingArgs(self):
         try:
             self.assertEqual(git_cl.main(['status', '--issue', '1']), 0)
@@ -2723,7 +2724,7 @@ class TestGitCl(unittest.TestCase):
 
     def test_description_set_raw(self):
         mock.patch('git_cl.Changelist', ChangelistMock).start()
-        mock.patch('git_cl.sys.stdin', StringIO('hihi')).start()
+        mock.patch('git_cl.sys.stdin', io.StringIO('hihi')).start()
 
         self.assertEqual(0, git_cl.main(['description', '-n', 'hihi']))
         self.assertEqual('hihi', ChangelistMock.desc)
@@ -2777,7 +2778,7 @@ class TestGitCl(unittest.TestCase):
     def test_description_set_stdin(self):
         mock.patch('git_cl.Changelist', ChangelistMock).start()
         mock.patch('git_cl.sys.stdin',
-                   StringIO('hi \r\n\t there\n\nman')).start()
+                   io.StringIO('hi \r\n\t there\n\nman')).start()
 
         self.assertEqual(0, git_cl.main(['description', '-n', '-']))
         self.assertEqual('hi\n\t there\n\nman', ChangelistMock.desc)
@@ -4266,7 +4267,7 @@ class CMDTestCaseBase(unittest.TestCase):
 
     def setUp(self):
         super(CMDTestCaseBase, self).setUp()
-        mock.patch('git_cl.sys.stdout', StringIO()).start()
+        mock.patch('git_cl.sys.stdout', io.StringIO()).start()
         mock.patch('git_cl.uuid.uuid4', return_value='uuid4').start()
         mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start()
         mock.patch(
@@ -4718,7 +4719,7 @@ class CMDTryTestCase(CMDTestCaseBase):
                                                'cr-buildbucket.appspot.com',
                                                'Batch', expected_request)
 
-    @mock.patch('sys.stderr', StringIO())
+    @mock.patch('sys.stderr', io.StringIO())
     def testScheduleOnBuildbucket_WrongBucket(self):
         with self.assertRaises(SystemExit):
             git_cl.main([
@@ -5278,6 +5279,51 @@ class CMDOwnersTestCase(CMDTestCaseBase):
         self.assertIn('b@example.com', sys.stdout.getvalue())
 
 
+class CMDLintTestCase(CMDTestCaseBase):
+    bad_indent = '\n'.join([
+        '// Copyright 1999 <a@example.com>',
+        'namespace foo {',
+        '  class a;',
+        '}',
+        '',
+    ])
+    filesInCL = ['foo', 'bar']
+
+    def setUp(self):
+        super(CMDLintTestCase, self).setUp()
+        mock.patch('git_cl.sys.stderr', io.StringIO()).start()
+        mock.patch('codecs.open', mock.mock_open()).start()
+        mock.patch('os.path.isfile', return_value=True).start()
+
+    def testLintSingleFile(self, *_mock):
+        codecs.open().read.return_value = self.bad_indent
+        self.assertEqual(1, git_cl.main(['lint', 'pdf.h']))
+        self.assertIn('pdf.h:3:  (cpplint) Do not indent within a namespace',
+                      git_cl.sys.stderr.getvalue())
+
+    def testLintMultiFiles(self, *_mock):
+        codecs.open().read.return_value = self.bad_indent
+        self.assertEqual(1, git_cl.main(['lint', 'pdf.h', 'pdf.cc']))
+        self.assertIn('pdf.h:3:  (cpplint) Do not indent within a namespace',
+                      git_cl.sys.stderr.getvalue())
+        self.assertIn('pdf.cc:3:  (cpplint) Do not indent within a namespace',
+                      git_cl.sys.stderr.getvalue())
+
+    @mock.patch('git_cl.Changelist.GetAffectedFiles',
+                return_value=['chg-1.h', 'chg-2.cc'])
+    @mock.patch('git_cl.Changelist.GetCommonAncestorWithUpstream',
+                return_value='upstream')
+    @mock.patch('git_cl.Settings.GetRoot', return_value='.')
+    @mock.patch('git_cl.FindCodereviewSettingsFile', return_value=None)
+    def testLintChangelist(self, *_mock):
+        codecs.open().read.return_value = self.bad_indent
+        self.assertEqual(1, git_cl.main(['lint']))
+        self.assertIn('chg-1.h:3:  (cpplint) Do not indent within a namespace',
+                      git_cl.sys.stderr.getvalue())
+        self.assertIn('chg-2.cc:3:  (cpplint) Do not indent within a namespace',
+                      git_cl.sys.stderr.getvalue())
+
+
 if __name__ == '__main__':
     logging.basicConfig(
         level=logging.DEBUG if '-v' in sys.argv else logging.ERROR)