From dab680c7231cc2f0169d847ed087dc602e58e781 Mon Sep 17 00:00:00 2001 From: Luis Hector Chavez Date: Wed, 21 Dec 2016 13:57:09 -0800 Subject: Add support for automatically applying fixes This change adds support for hooks to provide a mechanism to automatically fix any errors found in the hook's execution. This will currently only be attempted if the commit in question is the topmost in the list of commits to be sent (i.e. if it is HEAD) and there is a single fix available (since multiple fixes might interact badly with each other). This change also adds automatic fix support for the clang-format hook. Bug: 32570902 Test: clang-format can now auto-fix formatting Change-Id: I9214b7eb88548481feb639d8f8e70b3f57913dc0 --- pre-upload.py | 39 ++++++++++++++++++++++++++++++++++++ rh/hooks.py | 31 +++++++++++++++++++++++------ rh/results.py | 22 +++++++++++++++++--- rh/terminal.py | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+), 9 deletions(-) diff --git a/pre-upload.py b/pre-upload.py index 447b8bf..fe4cdd8 100755 --- a/pre-upload.py +++ b/pre-upload.py @@ -161,6 +161,37 @@ def _get_project_config(): return config +def _attempt_fixes(fixup_func_list, commit_list): + """Attempts to run |fixup_func_list| given |commit_list|.""" + if len(fixup_func_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] + + if commit != commit_list[0]: + # If the commit is not at the top of the stack, git operations might be + # needed and might leave the working directory in a tricky state if the + # fix is attempted to run automatically (e.g. it might require manual + # merge conflict resolution). Refuse to run the fix in those cases. + return + + prompt = ('An automatic fix can be attempted for the "%s" hook. ' + 'Do you want to run it?' % hook_name) + if not rh.terminal.boolean_prompt(prompt): + return + + result = fixup_func() + if result: + print('Attempt to fix "%s" for commit "%s" failed: %s' % + (hook_name, commit, result), + file=sys.stderr) + else: + print('Fix successfully applied. Amend the current commit before ' + 'attempting to upload again.\n', file=sys.stderr) + + def _run_project_hooks(project_name, proj_dir=None, commit_list=None): """For each project run its project specific hook from the hooks dictionary. @@ -226,6 +257,7 @@ def _run_project_hooks(project_name, proj_dir=None, ignore_merged_commits=config.ignore_merged_commits) ret = True + fixup_func_list = [] for commit in commit_list: # Mix in some settings for our hooks. @@ -244,6 +276,13 @@ def _run_project_hooks(project_name, proj_dir=None, if error: ret = False output.hook_error(name, error) + for result in hook_results: + if result.fixup_func: + fixup_func_list.append((name, commit, + result.fixup_func)) + + if fixup_func_list: + _attempt_fixes(fixup_func_list, commit_list) output.finish() os.chdir(pwd) diff --git a/rh/hooks.py b/rh/hooks.py index 2c0dc1d..47926bc 100644 --- a/rh/hooks.py +++ b/rh/hooks.py @@ -250,10 +250,26 @@ def _get_build_os_name(): return 'linux-x86' -def _check_cmd(hook_name, project, commit, cmd, **kwargs): +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_command(cmd, **kwargs) + if result.returncode not in (None, 0): + return result.output + return None + return wrapper + + +def _check_cmd(hook_name, project, commit, cmd, fixup_func=None, **kwargs): """Runs |cmd| and returns its result as a HookCommandResult.""" return [rh.results.HookCommandResult(hook_name, project, commit, - _run_command(cmd, **kwargs))] + _run_command(cmd, **kwargs), + fixup_func=fixup_func)] # Where helper programs exist. @@ -284,10 +300,13 @@ def check_clang_format(project, commit, _desc, diff, options=None): tool = get_helper_path('clang-format.py') clang_format = options.tool_path('clang-format') git_clang_format = options.tool_path('git-clang-format') - cmd = ([tool, '--clang-format', clang_format, '--git-clang-format', - git_clang_format] + - options.args(('--style', 'file', '--commit', commit), diff)) - return _check_cmd('clang-format', project, commit, cmd) + tool_args = (['--clang-format', clang_format, '--git-clang-format', + git_clang_format] + + options.args(('--style', 'file', '--commit', commit), diff)) + cmd = [tool] + tool_args + fixup_func = _fixup_func_caller([tool, '--fix'] + tool_args) + return _check_cmd('clang-format', project, commit, cmd, + fixup_func=fixup_func) def check_google_java_format(project, commit, _desc, _diff, options=None): diff --git a/rh/results.py b/rh/results.py index ecf7f50..8fa956f 100644 --- a/rh/results.py +++ b/rh/results.py @@ -29,12 +29,27 @@ del _path class HookResult(object): """A single hook result.""" - def __init__(self, hook, project, commit, error, files=()): + def __init__(self, hook, project, commit, error, files=(), fixup_func=None): + """Initialize. + + Args: + hook: The name of the hook. + project: The name of the project. + commit: The git commit sha. + 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. + """ self.hook = hook self.project = project self.commit = commit self.error = error self.files = files + self.fixup_func = fixup_func def __bool__(self): return bool(self.error) @@ -47,10 +62,11 @@ class HookResult(object): class HookCommandResult(HookResult): """A single hook result based on a CommandResult.""" - def __init__(self, hook, project, commit, result, files=()): + def __init__(self, hook, project, commit, result, files=(), + fixup_func=None): HookResult.__init__(self, hook, project, commit, result.error if result.error else result.output, - files=files) + files=files, fixup_func=fixup_func) self.result = result def __bool__(self): diff --git a/rh/terminal.py b/rh/terminal.py index 49da992..9eda0ec 100644 --- a/rh/terminal.py +++ b/rh/terminal.py @@ -136,3 +136,66 @@ def print_status_line(line, print_newline=False): sys.stderr.write(output) sys.stderr.flush() + + +def get_input(prompt): + """Python 2/3 glue for raw_input/input differences.""" + try: + return raw_input(prompt) + except NameError: + # Python 3 renamed raw_input() to input(), which is safe to call since + # it does not evaluate the input. + # pylint: disable=bad-builtin + return input(prompt) + + +def boolean_prompt(prompt='Do you want to continue?', default=True, + true_value='yes', false_value='no', prolog=None): + """Helper function for processing boolean choice prompts. + + Args: + prompt: The question to present to the user. + default: Boolean to return if the user just presses enter. + true_value: The text to display that represents a True returned. + false_value: The text to display that represents a False returned. + prolog: The text to display before prompt. + + Returns: + True or False. + """ + true_value, false_value = true_value.lower(), false_value.lower() + true_text, false_text = true_value, false_value + if true_value == false_value: + raise ValueError('true_value and false_value must differ: got %r' + % true_value) + + if default: + true_text = true_text[0].upper() + true_text[1:] + else: + false_text = false_text[0].upper() + false_text[1:] + + prompt = ('\n%s (%s/%s)? ' % (prompt, true_text, false_text)) + + if prolog: + prompt = ('\n%s\n%s' % (prolog, prompt)) + + while True: + try: + response = get_input(prompt).lower() + except EOFError: + # If the user hits CTRL+D, or stdin is disabled, use the default. + print() + response = None + except KeyboardInterrupt: + # If the user hits CTRL+C, just exit the process. + print() + raise + + if not response: + return default + if true_value.startswith(response): + if not false_value.startswith(response): + return True + # common prefix between the two... + elif false_value.startswith(response): + return False -- cgit v1.2.3