From 7f7acfe9fd93cfd4a697f2bc851d1b8182f6336e Mon Sep 17 00:00:00 2001 From: Remy Bohmer Date: Sat, 1 Aug 2020 18:36:44 +0200 Subject: [PATCH] Concentrate the RepoHook knowledge in the RepoHook class The knowledge about running hooks and all its exception handling is scattered over multiple files. This makes the code harder to read, but also it requires duplication of logic in case other RepoHooks are added to different commands. This refactoring also creates uniform behavior of the hooks across multiple commands and it guarantees the re-use of the same arguments on all of them. Signed-off-by: Remy Bohmer Change-Id: Ia4d90eab429e4af00943306e89faec8db35ba29d Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/277562 Tested-by: Remy Bohmer Reviewed-by: Mike Frysinger --- hooks.py | 143 +++++++++++++++++++++++++++++++++++++--------- subcmds/upload.py | 64 ++++----------------- 2 files changed, 127 insertions(+), 80 deletions(-) diff --git a/hooks.py b/hooks.py index 177bc88b..1abba0c4 100644 --- a/hooks.py +++ b/hooks.py @@ -14,9 +14,11 @@ # See the License for the specific language governing permissions and # limitations under the License. +import errno import json import os import re +import subprocess import sys import traceback @@ -33,6 +35,7 @@ else: urllib.parse = urlparse input = raw_input # noqa: F821 + class RepoHook(object): """A RepoHook contains information about a script to run as a hook. @@ -45,13 +48,29 @@ class RepoHook(object): Hooks are always python. When a hook is run, we will load the hook into the interpreter and execute its main() function. + + Combinations of hook option flags: + - no-verify=False, verify=False (DEFAULT): + If stdout is a tty, can prompt about running hooks if needed. + If user denies running hooks, the action is cancelled. If stdout is + not a tty and we would need to prompt about hooks, action is + cancelled. + - no-verify=False, verify=True: + Always run hooks with no prompt. + - no-verify=True, verify=False: + Never run hooks, but run action anyway (AKA bypass hooks). + - no-verify=True, verify=True: + Invalid """ def __init__(self, hook_type, hooks_project, - topdir, + repo_topdir, manifest_url, + bypass_hooks=False, + allow_all_hooks=False, + ignore_hooks=False, abort_if_user_denies=False): """RepoHook constructor. @@ -59,20 +78,27 @@ class RepoHook(object): hook_type: A string representing the type of hook. This is also used to figure out the name of the file containing the hook. For example: 'pre-upload'. - hooks_project: The project containing the repo hooks. If you have a - manifest, this is manifest.repo_hooks_project. OK if this is None, - which will make the hook a no-op. - topdir: Repo's top directory (the one containing the .repo directory). - Scripts will run with CWD as this directory. If you have a manifest, - this is manifest.topdir + hooks_project: The project containing the repo hooks. + If you have a manifest, this is manifest.repo_hooks_project. + OK if this is None, which will make the hook a no-op. + repo_topdir: The top directory of the repo client checkout. + This is the one containing the .repo directory. Scripts will + run with CWD as this directory. + If you have a manifest, this is manifest.topdir. manifest_url: The URL to the manifest git repo. - abort_if_user_denies: If True, we'll throw a HookError() if the user + bypass_hooks: If True, then 'Do not run the hook'. + allow_all_hooks: If True, then 'Run the hook without prompting'. + ignore_hooks: If True, then 'Do not abort action if hooks fail'. + abort_if_user_denies: If True, we'll abort running the hook if the user doesn't allow us to run the hook. """ self._hook_type = hook_type self._hooks_project = hooks_project + self._repo_topdir = repo_topdir self._manifest_url = manifest_url - self._topdir = topdir + self._bypass_hooks = bypass_hooks + self._allow_all_hooks = allow_all_hooks + self._ignore_hooks = ignore_hooks self._abort_if_user_denies = abort_if_user_denies # Store the full path to the script for convenience. @@ -108,7 +134,7 @@ class RepoHook(object): # NOTE: Local (non-committed) changes will not be factored into this hash. # I think this is OK, since we're really only worried about warning the user # about upstream changes. - return self._hooks_project.work_git.rev_parse('HEAD') + return self._hooks_project.work_git.rev_parse(HEAD) def _GetMustVerb(self): """Return 'must' if the hook is required; 'should' if not.""" @@ -347,7 +373,7 @@ context['main'](**kwargs) try: # Always run hooks with CWD as topdir. - os.chdir(self._topdir) + os.chdir(self._repo_topdir) # Put the hook dir as the first item of sys.path so hooks can do # relative imports. We want to replace the repo dir as [0] so @@ -397,7 +423,12 @@ context['main'](**kwargs) sys.path = orig_syspath os.chdir(orig_path) - def Run(self, user_allows_all_hooks, **kwargs): + def _CheckHook(self): + # Bail with a nice error if we can't find the hook. + if not os.path.isfile(self._script_fullpath): + raise HookError('Couldn\'t find repo hook: %s' % self._script_fullpath) + + def Run(self, **kwargs): """Run the hook. If the hook doesn't exist (because there is no hooks project or because @@ -410,22 +441,80 @@ context['main'](**kwargs) to the hook type. For instance, pre-upload hooks will contain a project_list. - Raises: - HookError: If there was a problem finding the hook or the user declined - to run a required hook (from _CheckForHookApproval). + Returns: + True: On success or ignore hooks by user-request + False: The hook failed. The caller should respond with aborting the action. + Some examples in which False is returned: + * Finding the hook failed while it was enabled, or + * the user declined to run a required hook (from _CheckForHookApproval) + In all these cases the user did not pass the proper arguments to + ignore the result through the option combinations as listed in + AddHookOptionGroup(). """ - # No-op if there is no hooks project or if hook is disabled. - if ((not self._hooks_project) or (self._hook_type not in - self._hooks_project.enabled_repo_hooks)): - return + # Do not do anything in case bypass_hooks is set, or + # no-op if there is no hooks project or if hook is disabled. + if (self._bypass_hooks or + not self._hooks_project or + self._hook_type not in self._hooks_project.enabled_repo_hooks): + return True - # Bail with a nice error if we can't find the hook. - if not os.path.isfile(self._script_fullpath): - raise HookError('Couldn\'t find repo hook: "%s"' % self._script_fullpath) + passed = True + try: + self._CheckHook() - # Make sure the user is OK with running the hook. - if (not user_allows_all_hooks) and (not self._CheckForHookApproval()): - return + # Make sure the user is OK with running the hook. + if self._allow_all_hooks or self._CheckForHookApproval(): + # Run the hook with the same version of python we're using. + self._ExecuteHook(**kwargs) + except SystemExit as e: + passed = False + print('ERROR: %s hooks exited with exit code: %s' % (self._hook_type, str(e)), + file=sys.stderr) + except HookError as e: + passed = False + print('ERROR: %s' % str(e), file=sys.stderr) - # Run the hook with the same version of python we're using. - self._ExecuteHook(**kwargs) + if not passed and self._ignore_hooks: + print('\nWARNING: %s hooks failed, but continuing anyways.' % self._hook_type, + file=sys.stderr) + passed = True + + return passed + + @classmethod + def FromSubcmd(cls, manifest, opt, *args, **kwargs): + """Method to construct the repo hook class + + Args: + manifest: The current active manifest for this command from which we + extract a couple of fields. + opt: Contains the commandline options for the action of this hook. + It should contain the options added by AddHookOptionGroup() in which + we are interested in RepoHook execution. + """ + for key in ('bypass_hooks', 'allow_all_hooks', 'ignore_hooks'): + kwargs.setdefault(key, getattr(opt, key)) + kwargs.update({ + 'hooks_project': manifest.repo_hooks_project, + 'repo_topdir': manifest.topdir, + 'manifest_url': manifest.manifestProject.GetRemote('origin').url, + }) + return cls(*args, **kwargs) + + @staticmethod + def AddOptionGroup(parser, name): + """Help options relating to the various hooks.""" + + # Note that verify and no-verify are NOT opposites of each other, which + # is why they store to different locations. We are using them to match + # 'git commit' syntax. + group = parser.add_option_group(name + ' hooks') + group.add_option('--no-verify', + dest='bypass_hooks', action='store_true', + help='Do not run the %s hook.' % name) + group.add_option('--verify', + dest='allow_all_hooks', action='store_true', + help='Run the %s hook without prompting.' % name) + group.add_option('--ignore-hooks', + action='store_true', + help='Do not abort if %s hooks fail.' % name) diff --git a/subcmds/upload.py b/subcmds/upload.py index f441aae4..6196fe4c 100644 --- a/subcmds/upload.py +++ b/subcmds/upload.py @@ -21,7 +21,7 @@ import sys from command import InteractiveCommand from editor import Editor -from error import HookError, UploadError +from error import UploadError from git_command import GitCommand from git_refs import R_HEADS from hooks import RepoHook @@ -205,33 +205,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/ p.add_option('--no-cert-checks', dest='validate_certs', action='store_false', default=True, help='Disable verifying ssl certs (unsafe).') - - # Options relating to upload hook. Note that verify and no-verify are NOT - # opposites of each other, which is why they store to different locations. - # We are using them to match 'git commit' syntax. - # - # Combinations: - # - no-verify=False, verify=False (DEFAULT): - # If stdout is a tty, can prompt about running upload hooks if needed. - # If user denies running hooks, the upload is cancelled. If stdout is - # not a tty and we would need to prompt about upload hooks, upload is - # cancelled. - # - no-verify=False, verify=True: - # Always run upload hooks with no prompt. - # - no-verify=True, verify=False: - # Never run upload hooks, but upload anyway (AKA bypass hooks). - # - no-verify=True, verify=True: - # Invalid - g = p.add_option_group('Upload hooks') - g.add_option('--no-verify', - dest='bypass_hooks', action='store_true', - help='Do not run the upload hook.') - g.add_option('--verify', - dest='allow_all_hooks', action='store_true', - help='Run the upload hook without prompting.') - g.add_option('--ignore-hooks', - dest='ignore_hooks', action='store_true', - help='Do not abort uploading if upload hooks fail.') + RepoHook.AddOptionGroup(p, 'pre-upload') def _SingleBranch(self, opt, branch, people): project = branch.project @@ -572,31 +546,15 @@ Gerrit Code Review: https://www.gerritcodereview.com/ (branch,), file=sys.stderr) return 1 - if not opt.bypass_hooks: - hook = RepoHook('pre-upload', self.manifest.repo_hooks_project, - self.manifest.topdir, - self.manifest.manifestProject.GetRemote('origin').url, - abort_if_user_denies=True) - pending_proj_names = [project.name for (project, available) in pending] - pending_worktrees = [project.worktree for (project, available) in pending] - passed = True - try: - hook.Run(opt.allow_all_hooks, project_list=pending_proj_names, - worktree_list=pending_worktrees) - except SystemExit: - passed = False - if not opt.ignore_hooks: - raise - except HookError as e: - passed = False - print("ERROR: %s" % str(e), file=sys.stderr) - - if not passed: - if opt.ignore_hooks: - print('\nWARNING: pre-upload hooks failed, but uploading anyways.', - file=sys.stderr) - else: - return 1 + pending_proj_names = [project.name for (project, available) in pending] + pending_worktrees = [project.worktree for (project, available) in pending] + hook = RepoHook.FromSubcmd( + hook_type='pre-upload', manifest=self.manifest, + opt=opt, abort_if_user_denies=True) + if not hook.Run( + project_list=pending_proj_names, + worktree_list=pending_worktrees): + return 1 if opt.reviewers: reviewers = _SplitEmails(opt.reviewers)