fixups: store command rather than callback in results am: 9cf5981041

Original change: https://android-review.googlesource.com/c/platform/tools/repohooks/+/2617658

Change-Id: I51a706d309d8b2666bc2508c891eb286e94bb84a
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

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