Refactor "git cl format" formatters to use a common interface

No behavior change. This is to make it easier to add new formatters.

Bug: 1462204
Change-Id: Ifc9c46ad60fe5024f5dfb0cf781ff468b2cc1044
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4863139
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>

git_cl.py

@@ -6058,10 +6058,6 @@ def BuildGitDiffCmd(diff_type, upstream_commit, args, allow_prefix=False):
 
 def _RunClangFormatDiff(opts, clang_diff_files, top_dir, upstream_commit):
     """Runs clang-format-diff and sets a return value if necessary."""
-
-    if not clang_diff_files:
-        return 0
-
     # Set to 2 to signal to CheckPatchFormatted() that this patch isn't
     # formatted. This is used to block during the presubmit.
     return_value = 0
@@ -6123,10 +6119,6 @@ def _RunClangFormatDiff(opts, clang_diff_files, top_dir, upstream_commit):
 def _RunRustFmt(opts, rust_diff_files, top_dir, upstream_commit):
     """Runs rustfmt.  Just like _RunClangFormatDiff returns 2 to indicate that
   presubmit checks have failed (and returns 0 otherwise)."""
-
-    if not rust_diff_files:
-        return 0
-
     # Locate the rustfmt binary.
     try:
         rustfmt_tool = rustfmt.FindRustfmtToolInChromiumTree()
@@ -6150,10 +6142,8 @@ def _RunRustFmt(opts, rust_diff_files, top_dir, upstream_commit):
 def _RunSwiftFormat(opts, swift_diff_files, top_dir, upstream_commit):
     """Runs swift-format.  Just like _RunClangFormatDiff returns 2 to indicate
   that presubmit checks have failed (and returns 0 otherwise)."""
-
+    if sys.platform != 'darwin':
-    if not swift_diff_files:
+        DieWithError('swift-format is only supported on macOS.')
-        return 0
-
     # Locate the swift-format binary.
     try:
         swift_format_tool = swift_format.FindSwiftFormatToolInChromiumTree()
@@ -6174,6 +6164,150 @@ def _RunSwiftFormat(opts, swift_diff_files, top_dir, upstream_commit):
     return 0
 
 
+def _RunYapf(opts, paths, top_dir, upstream_commit):
+    depot_tools_path = os.path.dirname(os.path.abspath(__file__))
+    yapf_tool = os.path.join(depot_tools_path, 'yapf')
+
+    # Used for caching.
+    yapf_configs = {}
+    for p in paths:
+        # Find the yapf style config for the current file, defaults to depot
+        # tools default.
+        _FindYapfConfigFile(p, yapf_configs, top_dir)
+
+    # Turn on python formatting by default if a yapf config is specified.
+    # This breaks in the case of this repo though since the specified
+    # style file is also the global default.
+    if opts.python is None:
+        paths = [
+            p for p in paths
+            if _FindYapfConfigFile(p, yapf_configs, top_dir) is not None
+        ]
+
+    # Note: yapf still seems to fix indentation of the entire file
+    # even if line ranges are specified.
+    # See https://github.com/google/yapf/issues/499
+    if not opts.full and paths:
+        py_line_diffs = _ComputeDiffLineRanges(paths, upstream_commit)
+
+    yapfignore_patterns = _GetYapfIgnorePatterns(top_dir)
+    paths = _FilterYapfIgnoredFiles(paths, yapfignore_patterns)
+
+    return_value = 0
+    for path in paths:
+        yapf_style = _FindYapfConfigFile(path, yapf_configs, top_dir)
+        # Default to pep8 if not .style.yapf is found.
+        if not yapf_style:
+            yapf_style = 'pep8'
+
+        with open(path, 'r') as py_f:
+            if 'python2' in py_f.readline():
+                vpython_script = 'vpython'
+            else:
+                vpython_script = 'vpython3'
+
+        cmd = [vpython_script, yapf_tool, '--style', yapf_style, path]
+
+        has_formattable_lines = False
+        if not opts.full:
+            # Only run yapf over changed line ranges.
+            for diff_start, diff_len in py_line_diffs[path]:
+                diff_end = diff_start + diff_len - 1
+                # Yapf errors out if diff_end < diff_start but this
+                # is a valid line range diff for a removal.
+                if diff_end >= diff_start:
+                    has_formattable_lines = True
+                    cmd += ['-l', '{}-{}'.format(diff_start, diff_end)]
+            # If all line diffs were removals we have nothing to format.
+            if not has_formattable_lines:
+                continue
+
+        if opts.diff or opts.dry_run:
+            cmd += ['--diff']
+            # Will return non-zero exit code if non-empty diff.
+            stdout = RunCommand(cmd,
+                                error_ok=True,
+                                stderr=subprocess2.PIPE,
+                                cwd=top_dir,
+                                shell=sys.platform.startswith('win32'))
+            if opts.diff:
+                sys.stdout.write(stdout)
+            elif len(stdout) > 0:
+                return_value = 2
+        else:
+            cmd += ['-i']
+            RunCommand(cmd, cwd=top_dir, shell=sys.platform.startswith('win32'))
+    return return_value
+
+
+def _RunGnFormat(opts, paths, top_dir, upstream_commit):
+    cmd = ['gn', 'format']
+    if opts.dry_run or opts.diff:
+        cmd.append('--dry-run')
+    return_value = 0
+    for path in paths:
+        gn_ret = subprocess2.call(cmd + [path],
+                                  shell=sys.platform.startswith('win'),
+                                  cwd=top_dir)
+        if opts.dry_run and gn_ret == 2:
+            return_value = 2  # Not formatted.
+        elif opts.diff and gn_ret == 2:
+            # TODO this should compute and print the actual diff.
+            print('This change has GN build file diff for ' + path)
+        elif gn_ret != 0:
+            # For non-dry run cases (and non-2 return values for dry-run), a
+            # nonzero error code indicates a failure, probably because the
+            # file doesn't parse.
+            DieWithError('gn format failed on ' + path +
+                         '\nTry running `gn format` on this file manually.')
+    return return_value
+
+
+def _FormatXml(opts, paths, top_dir, upstream_commit):
+    # Skip the metrics formatting from the global presubmit hook. These files
+    # have a separate presubmit hook that issues an error if the files need
+    # formatting, whereas the top-level presubmit script merely issues a
+    # warning. Formatting these files is somewhat slow, so it's important not to
+    # duplicate the work.
+    if opts.presubmit:
+        return 0
+
+    return_value = 0
+    for path in paths:
+        xml_dir = GetMetricsDir(path)
+        if not xml_dir:
+            continue
+
+        tool_dir = os.path.join(top_dir, xml_dir)
+        pretty_print_tool = os.path.join(tool_dir, 'pretty_print.py')
+        cmd = [shutil.which('vpython3'), pretty_print_tool, '--non-interactive']
+
+        # If the XML file is histograms.xml or enums.xml, add the xml path
+        # to the command as histograms/pretty_print.py now needs a relative
+        # path argument after splitting the histograms into multiple
+        # directories. For example, in tools/metrics/ukm, pretty-print could
+        # be run using: $ python pretty_print.py But in
+        # tools/metrics/histogrmas, pretty-print should be run with an
+        # additional relative path argument, like: $ python pretty_print.py
+        # metadata/UMA/histograms.xml $ python pretty_print.py enums.xml
+        if xml_dir == os.path.join('tools', 'metrics', 'histograms'):
+            if os.path.basename(path) not in ('histograms.xml', 'enums.xml',
+                                              'histogram_suffixes_list.xml'):
+                # Skip this XML file if it's not one of the known types.
+                continue
+            cmd.append(path)
+
+        if opts.dry_run or opts.diff:
+            cmd.append('--diff')
+
+        stdout = RunCommand(cmd, cwd=top_dir)
+        if opts.diff:
+            sys.stdout.write(stdout)
+        if opts.dry_run and stdout:
+            return_value = 2  # Not formatted.
+    return return_value
+
+
 def MatchingFileType(file_name, extensions):
     """Returns True if the file name ends with one of the given extensions."""
     return bool([ext for ext in extensions if file_name.lower().endswith(ext)])
@@ -6183,10 +6317,8 @@ def MatchingFileType(file_name, extensions):
 @metrics.collector.collect_metrics('git cl format')
 def CMDformat(parser, args):
     """Runs auto-formatting tools (clang-format etc.) on the diff."""
-    CLANG_EXTS = ['.cc', '.cpp', '.h', '.m', '.mm', '.proto', '.java']
+    clang_exts = ['.cc', '.cpp', '.h', '.m', '.mm', '.proto', '.java']
     GN_EXTS = ['.gn', '.gni', '.typemap']
-    RUST_EXTS = ['.rs']
-    SWIFT_EXTS = ['.swift']
     parser.add_option('--full',
                       action='store_true',
                       help='Reformat the full content of all touched files')
@@ -6202,12 +6334,11 @@ def CMDformat(parser, args):
         help='Disables formatting of various file types using clang-format.')
     parser.add_option('--python',
                       action='store_true',
-                      default=None,
                       help='Enables python formatting on all python files.')
     parser.add_option(
         '--no-python',
-        action='store_true',
+        action='store_false',
-        default=False,
+        dest='python',
         help='Disables python formatting on all python files. '
         'If neither --python or --no-python are set, python files that have a '
         '.style.yapf file in an ancestor directory will be formatted. '
@@ -6250,11 +6381,6 @@ def CMDformat(parser, args):
 
     opts, args = parser.parse_args(args)
 
-    if opts.python is not None and opts.no_python:
-        raise parser.error('Cannot set both --python and --no-python')
-    if opts.no_python:
-        opts.python = False
-
     # Normalize any remaining args against the current path, so paths relative
     # to the current directory are still resolved as expected.
     args = [os.path.join(os.getcwd(), arg) for arg in args]
@@ -6289,195 +6415,33 @@ def CMDformat(parser, args):
     diff_files = [x for x in diff_files if os.path.isfile(x)]
 
     if opts.js:
-        CLANG_EXTS.extend(['.js', '.ts'])
+        clang_exts.extend(['.js', '.ts'])
 
-    clang_diff_files = []
+    formatters = [
-    if opts.clang_format:
+        (GN_EXTS, _RunGnFormat),
-        clang_diff_files = [
+        (['.xml'], _FormatXml),
-            x for x in diff_files if MatchingFileType(x, CLANG_EXTS)
-        ]
-    python_diff_files = [x for x in diff_files if MatchingFileType(x, ['.py'])]
-    rust_diff_files = [x for x in diff_files if MatchingFileType(x, RUST_EXTS)]
-    swift_diff_files = [
-        x for x in diff_files if MatchingFileType(x, SWIFT_EXTS)
     ]
-    gn_diff_files = [x for x in diff_files if MatchingFileType(x, GN_EXTS)]
+    if opts.clang_format:
+        formatters += [(clang_exts, _RunClangFormatDiff)]
+    if opts.use_rust_fmt:
+        formatters += [(['.rs'], _RunRustFmt)]
+    if opts.use_swift_format:
+        formatters += [(['.swift'], _RunSwiftFormat)]
+    if opts.python is not False:
+        formatters += [(['.py'], _RunYapf)]
 
     top_dir = settings.GetRoot()
-
+    return_value = 0
-    return_value = _RunClangFormatDiff(opts, clang_diff_files, top_dir,
+    for file_types, format_func in formatters:
-                                       upstream_commit)
+        paths = [p for p in diff_files if MatchingFileType(p, file_types)]
-
+        if not paths:
-    if opts.use_rust_fmt:
+            continue
-        rust_fmt_return_value = _RunRustFmt(opts, rust_diff_files, top_dir,
+        ret = format_func(opts, paths, top_dir, upstream_commit)
-                                            upstream_commit)
+        return_value = return_value or ret
-        if rust_fmt_return_value == 2:
-            return_value = 2
-
-    if opts.use_swift_format:
-        if sys.platform != 'darwin':
-            DieWithError('swift-format is only supported on macOS.')
-        swift_format_return_value = _RunSwiftFormat(opts, swift_diff_files,
-                                                    top_dir, upstream_commit)
-        if swift_format_return_value == 2:
-            return_value = 2
-
-    # Similar code to above, but using yapf on .py files rather than
-    # clang-format on C/C++ files
-    py_explicitly_disabled = opts.python is not None and not opts.python
-    if python_diff_files and not py_explicitly_disabled:
-        depot_tools_path = os.path.dirname(os.path.abspath(__file__))
-        yapf_tool = os.path.join(depot_tools_path, 'yapf')
-
-        # Used for caching.
-        yapf_configs = {}
-        for f in python_diff_files:
-            # Find the yapf style config for the current file, defaults to depot
-            # tools default.
-            _FindYapfConfigFile(f, yapf_configs, top_dir)
-
-        # Turn on python formatting by default if a yapf config is specified.
-        # This breaks in the case of this repo though since the specified
-        # style file is also the global default.
-        if opts.python is None:
-            filtered_py_files = []
-            for f in python_diff_files:
-                if _FindYapfConfigFile(f, yapf_configs, top_dir) is not None:
-                    filtered_py_files.append(f)
-        else:
-            filtered_py_files = python_diff_files
-
-        # Note: yapf still seems to fix indentation of the entire file
-        # even if line ranges are specified.
-        # See https://github.com/google/yapf/issues/499
-        if not opts.full and filtered_py_files:
-            py_line_diffs = _ComputeDiffLineRanges(filtered_py_files,
-                                                   upstream_commit)
-
-        yapfignore_patterns = _GetYapfIgnorePatterns(top_dir)
-        filtered_py_files = _FilterYapfIgnoredFiles(filtered_py_files,
-                                                    yapfignore_patterns)
-
-        for f in filtered_py_files:
-            yapf_style = _FindYapfConfigFile(f, yapf_configs, top_dir)
-            # Default to pep8 if not .style.yapf is found.
-            if not yapf_style:
-                yapf_style = 'pep8'
-
-            with open(f, 'r') as py_f:
-                if 'python2' in py_f.readline():
-                    vpython_script = 'vpython'
-                else:
-                    vpython_script = 'vpython3'
-
-            cmd = [vpython_script, yapf_tool, '--style', yapf_style, f]
-
-            has_formattable_lines = False
-            if not opts.full:
-                # Only run yapf over changed line ranges.
-                for diff_start, diff_len in py_line_diffs[f]:
-                    diff_end = diff_start + diff_len - 1
-                    # Yapf errors out if diff_end < diff_start but this
-                    # is a valid line range diff for a removal.
-                    if diff_end >= diff_start:
-                        has_formattable_lines = True
-                        cmd += ['-l', '{}-{}'.format(diff_start, diff_end)]
-                # If all line diffs were removals we have nothing to format.
-                if not has_formattable_lines:
-                    continue
-
-            if opts.diff or opts.dry_run:
-                cmd += ['--diff']
-                # Will return non-zero exit code if non-empty diff.
-                stdout = RunCommand(cmd,
-                                    error_ok=True,
-                                    stderr=subprocess2.PIPE,
-                                    cwd=top_dir,
-                                    shell=sys.platform.startswith('win32'))
-                if opts.diff:
-                    sys.stdout.write(stdout)
-                elif len(stdout) > 0:
-                    return_value = 2
-            else:
-                cmd += ['-i']
-                RunCommand(cmd,
-                           cwd=top_dir,
-                           shell=sys.platform.startswith('win32'))
-
-    # Format GN build files. Always run on full build files for canonical form.
-    if gn_diff_files:
-        cmd = ['gn', 'format']
-        if opts.dry_run or opts.diff:
-            cmd.append('--dry-run')
-        for gn_diff_file in gn_diff_files:
-            gn_ret = subprocess2.call(cmd + [gn_diff_file],
-                                      shell=sys.platform.startswith('win'),
-                                      cwd=top_dir)
-            if opts.dry_run and gn_ret == 2:
-                return_value = 2  # Not formatted.
-            elif opts.diff and gn_ret == 2:
-                # TODO this should compute and print the actual diff.
-                print('This change has GN build file diff for ' + gn_diff_file)
-            elif gn_ret != 0:
-                # For non-dry run cases (and non-2 return values for dry-run), a
-                # nonzero error code indicates a failure, probably because the
-                # file doesn't parse.
-                DieWithError('gn format failed on ' + gn_diff_file +
-                             '\nTry running `gn format` on this file manually.')
-
-    # Skip the metrics formatting from the global presubmit hook. These files
-    # have a separate presubmit hook that issues an error if the files need
-    # formatting, whereas the top-level presubmit script merely issues a
-    # warning. Formatting these files is somewhat slow, so it's important not to
-    # duplicate the work.
-    if not opts.presubmit:
-        for diff_xml in GetDiffXMLs(diff_files):
-            xml_dir = GetMetricsDir(diff_xml)
-            if not xml_dir:
-                continue
-
-            tool_dir = os.path.join(top_dir, xml_dir)
-            pretty_print_tool = os.path.join(tool_dir, 'pretty_print.py')
-            cmd = [
-                shutil.which('vpython3'), pretty_print_tool, '--non-interactive'
-            ]
-
-            # If the XML file is histograms.xml or enums.xml, add the xml path
-            # to the command as histograms/pretty_print.py now needs a relative
-            # path argument after splitting the histograms into multiple
-            # directories. For example, in tools/metrics/ukm, pretty-print could
-            # be run using: $ python pretty_print.py But in
-            # tools/metrics/histogrmas, pretty-print should be run with an
-            # additional relative path argument, like: $ python pretty_print.py
-            # metadata/UMA/histograms.xml $ python pretty_print.py enums.xml
-
-            if xml_dir == os.path.join('tools', 'metrics', 'histograms'):
-                if os.path.basename(diff_xml) not in (
-                        'histograms.xml', 'enums.xml',
-                        'histogram_suffixes_list.xml'):
-                    # Skip this XML file if it's not one of the known types.
-                    continue
-                cmd.append(diff_xml)
-
-            if opts.dry_run or opts.diff:
-                cmd.append('--diff')
-
-            stdout = RunCommand(cmd, cwd=top_dir)
-            if opts.diff:
-                sys.stdout.write(stdout)
-            if opts.dry_run and stdout:
-                return_value = 2  # Not formatted.
 
     return return_value
 
 
-def GetDiffXMLs(diff_files):
-    return [
-        os.path.normpath(x) for x in diff_files
-        if MatchingFileType(x, ['.xml'])
-    ]
-
-
 def GetMetricsDir(diff_xml):
     metrics_xml_dirs = [
         os.path.join('tools', 'metrics', 'actions'),