fixups: store command rather than callback in results

All of the fixup funcs just run an external command.  This makes it
difficult to harmonize output, and leads to duplicated logic.  Switch
to storing the fixup command that would be run.  This doesn't change
any behavior currently, just refactoring to make it easier to make
followup changes.

Bug: None
Test: unittests
Change-Id: Ib9e56bc46116997862b25362c49a617326b8909b

4 files changed, 44 insertions, 55 deletions

diff --git a/pre-upload.py b/pre-upload.py
index 123eb8d..ee0639e 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -225,14 +225,14 @@ def _get_project_config(from_git=False):
     return rh.config.PreUploadSettings(paths=paths, global_paths=global_paths)
 
 
-def _attempt_fixes(fixup_func_list, commit_list):
-    """Attempts to run |fixup_func_list| given |commit_list|."""
-    if len(fixup_func_list) != 1:
+def _attempt_fixes(fixup_list, commit_list, cwd):
+    """Attempts to run |fixup_list| given |commit_list|."""
+    if len(fixup_list) != 1:
         # Only single fixes will be attempted, since various fixes might
         # interact with each other.
         return
 
-    hook_name, commit, fixup_func = fixup_func_list[0]
+    hook_name, commit, fixup_cmd, fixup_files = fixup_list[0]
 
     if commit != commit_list[0]:
         # If the commit is not at the top of the stack, git operations might be
@@ -246,10 +246,17 @@ def _attempt_fixes(fixup_func_list, commit_list):
     if not rh.terminal.boolean_prompt(prompt):
         return
 
-    result = fixup_func()
-    if result:
+    result = rh.utils.run(
+        fixup_cmd + list(fixup_files),
+        cwd=cwd,
+        combine_stdout_stderr=True,
+        capture_output=True,
+        check=False,
+        input='',
+    )
+    if result.returncode:
         print(f'Attempt to fix "{hook_name}" for commit "{commit}" failed: '
-              f'{result}',
+              f'{result.stdout}',
               file=sys.stderr)
     else:
         print('Fix successfully applied. Amend the current commit before '
@@ -310,7 +317,7 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, co
             ignore_merged_commits=config.ignore_merged_commits)
 
     ret = True
-    fixup_func_list = []
+    fixup_list = []
 
     for commit in commit_list:
         # Mix in some settings for our hooks.
@@ -336,12 +343,12 @@ def _run_project_hooks_in_cwd(project_name, proj_dir, output, from_git=False, co
                     ret = False
                     output.hook_error(error)
                 for result in hook_results:
-                    if result.fixup_func:
-                        fixup_func_list.append((name, commit,
-                                                result.fixup_func))
+                    if result.fixup_cmd:
+                        fixup_list.append(
+                            (name, commit, result.fixup_cmd, result.files))
 
-    if fixup_func_list:
-        _attempt_fixes(fixup_func_list, commit_list)
+    if fixup_list:
+        _attempt_fixes(fixup_list, commit_list, proj_dir)
 
     return ret
 
diff --git a/rh/hooks.py b/rh/hooks.py
index c3e6e79..1d7eb3f 100644
--- a/rh/hooks.py
+++ b/rh/hooks.py
@@ -317,26 +317,11 @@ def _get_build_os_name():
     return 'linux-x86'
 
 
-def _fixup_func_caller(cmd, **kwargs):
-    """Wraps |cmd| around a callable automated fixup.
-
-    For hooks that support automatically fixing errors after running (e.g. code
-    formatters), this function provides a way to run |cmd| as the |fixup_func|
-    parameter in HookCommandResult.
-    """
-    def wrapper():
-        result = _run(cmd, **kwargs)
-        if result.returncode not in (None, 0):
-            return result.stdout
-        return None
-    return wrapper
-
-
-def _check_cmd(hook_name, project, commit, cmd, fixup_func=None, **kwargs):
+def _check_cmd(hook_name, project, commit, cmd, fixup_cmd=None, **kwargs):
     """Runs |cmd| and returns its result as a HookCommandResult."""
     return [rh.results.HookCommandResult(hook_name, project, commit,
                                          _run(cmd, **kwargs),
-                                         fixup_func=fixup_func)]
+                                         fixup_cmd=fixup_cmd)]
 
 
 # Where helper programs exist.
@@ -370,12 +355,11 @@ def check_bpfmt(project, commit, _desc, diff, options=None):
             fixup_cmd = [bpfmt, '-w']
             if '-s' in bpfmt_options:
                 fixup_cmd.append('-s')
-            fixup_cmd.append(os.path.join(project.dir, d.file))
             ret.append(rh.results.HookResult(
                 'bpfmt', project, commit,
                 error=result.stdout,
                 files=(d.file,),
-                fixup_func=_fixup_func_caller(fixup_cmd)))
+                fixup_cmd=fixup_cmd))
     return ret
 
 
@@ -397,9 +381,9 @@ def check_clang_format(project, commit, _desc, diff, options=None):
                   git_clang_format] +
                  options.args(('--style', 'file', '--commit', commit), diff))
     cmd = [tool] + tool_args
-    fixup_func = _fixup_func_caller([tool, '--fix'] + tool_args)
+    fixup_cmd = [tool, '--fix'] + tool_args
     return _check_cmd('clang-format', project, commit, cmd,
-                      fixup_func=fixup_func)
+                      fixup_cmd=fixup_cmd)
 
 
 def check_google_java_format(project, commit, _desc, _diff, options=None):
@@ -412,9 +396,9 @@ def check_google_java_format(project, commit, _desc, _diff, options=None):
                  '--google-java-format-diff', google_java_format_diff,
                  '--commit', commit] + options.args()
     cmd = [tool] + tool_args
-    fixup_func = _fixup_func_caller([tool, '--fix'] + tool_args)
+    fixup_cmd = [tool, '--fix'] + tool_args
     return _check_cmd('google-java-format', project, commit, cmd,
-                      fixup_func=fixup_func)
+                      fixup_cmd=fixup_cmd)
 
 
 def check_ktfmt(project, commit, _desc, diff, options=None):
@@ -442,15 +426,14 @@ def check_ktfmt(project, commit, _desc, diff, options=None):
     result = _run(cmd)
     if result.stdout:
         paths = [os.path.join(project.dir, x.file) for x in filtered]
-        fixup_cmd = [ktfmt] + args + paths
+        fixup_cmd = [ktfmt] + args
         error = (
             '\nKotlin files need formatting.\n' +
             'To reformat the kotlin files in this commit:\n' +
-            rh.shell.cmd_to_str(fixup_cmd)
+            rh.shell.cmd_to_str(fixup_cmd + paths)
         )
-        fixup_func = _fixup_func_caller(fixup_cmd)
         return [rh.results.HookResult('ktfmt', project, commit, error=error,
-                                      files=paths, fixup_func=fixup_func)]
+                                      files=paths, fixup_cmd=fixup_cmd)]
     return None
 
 
@@ -886,10 +869,10 @@ def check_gofmt(project, commit, _desc, diff, options=None):
         data = rh.git.get_file_content(commit, d.file)
         result = _run(cmd, input=data)
         if result.stdout:
-            fixup_func = _fixup_func_caller([gofmt, '-w', d.file])
+            fixup_cmd = [gofmt, '-w']
             ret.append(rh.results.HookResult(
                 'gofmt', project, commit, error=result.stdout,
-                files=(d.file,), fixup_func=fixup_func))
+                files=(d.file,), fixup_cmd=fixup_cmd))
     return ret
 
 
@@ -1047,12 +1030,10 @@ def check_aidl_format(project, commit, _desc, diff, options=None):
         data = rh.git.get_file_content(commit, d.file)
         result = _run(diff_cmd, input=data)
         if result.stdout:
-            fix_cmd = [aidl_format, '-w', '--clang-format-path', clang_format,
-                       d.file]
-            fixup_func = _fixup_func_caller(fix_cmd)
+            fixup_cmd = [aidl_format, '-w', '--clang-format-path', clang_format]
             ret.append(rh.results.HookResult(
                 'aidl-format', project, commit, error=result.stdout,
-                files=(d.file,), fixup_func=fixup_func))
+                files=(d.file,), fixup_cmd=fixup_cmd))
     return ret
 
 
diff --git a/rh/hooks_unittest.py b/rh/hooks_unittest.py
index 4f2103a..901eca1 100755
--- a/rh/hooks_unittest.py
+++ b/rh/hooks_unittest.py
@@ -377,7 +377,7 @@ class BuiltinHooksTests(unittest.TestCase):
             self.project, 'commit', 'desc', diff, options=self.options)
         self.assertIsNotNone(ret)
         for result in ret:
-            self.assertIsNotNone(result.fixup_func)
+            self.assertIsNotNone(result.fixup_cmd)
 
     def test_checkpatch(self, mock_check, _mock_run):
         """Verify the checkpatch builtin hook."""
diff --git a/rh/results.py b/rh/results.py
index a7a4b49..a754cef 100644
--- a/rh/results.py
+++ b/rh/results.py
@@ -16,6 +16,7 @@
 
 import os
 import sys
+from typing import List, Optional
 
 _path = os.path.realpath(__file__ + '/../..')
 if sys.path[0] != _path:
@@ -26,7 +27,8 @@ del _path
 class HookResult(object):
     """A single hook result."""
 
-    def __init__(self, hook, project, commit, error, files=(), fixup_func=None):
+    def __init__(self, hook, project, commit, error, files=(),
+                 fixup_cmd: Optional[List[str]] = None):
         """Initialize.
 
         Args:
@@ -36,17 +38,16 @@ class HookResult(object):
           error: A string representation of the hook's result.  Empty on
               success.
           files: The list of files that were involved in the hook execution.
-          fixup_func: A callable that will attempt to automatically fix errors
-              found in the hook's execution.  Returns an non-empty string if
-              this, too, fails.  Can be None if the hook does not support
-              automatically fixing errors.
+          fixup_cmd: A command that can automatically fix errors found in the
+              hook's execution.  Can be None if the hook does not support
+              automatic fixups.
         """
         self.hook = hook
         self.project = project
         self.commit = commit
         self.error = error
         self.files = files
-        self.fixup_func = fixup_func
+        self.fixup_cmd = fixup_cmd
 
     def __bool__(self):
         return bool(self.error)
@@ -59,10 +60,10 @@ class HookCommandResult(HookResult):
     """A single hook result based on a CompletedProcess."""
 
     def __init__(self, hook, project, commit, result, files=(),
-                 fixup_func=None):
+                 fixup_cmd=None):
         HookResult.__init__(self, hook, project, commit,
                             result.stderr if result.stderr else result.stdout,
-                            files=files, fixup_func=fixup_func)
+                            files=files, fixup_cmd=fixup_cmd)
         self.result = result
 
     def __bool__(self):