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 <github@bohmer.net>
Change-Id: Ia4d90eab429e4af00943306e89faec8db35ba29d
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/277562
Tested-by: Remy Bohmer <oss@bohmer.net>
Reviewed-by: Mike Frysinger <vapier@google.com>

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
+      hooks_project: The project containing the repo hooks.
-          manifest, this is manifest.repo_hooks_project.  OK if this is None,
+          If you have a manifest, this is manifest.repo_hooks_project.
-          which will make the hook a no-op.
+          OK if this is None, which will make the hook a no-op.
-      topdir: Repo's top directory (the one containing the .repo directory).
+      repo_topdir: The top directory of the repo client checkout.
-          Scripts will run with CWD as this directory.  If you have a manifest,
+          This is the one containing the .repo directory. Scripts will
-          this is manifest.topdir
+          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:
+    Returns:
-      HookError: If there was a problem finding the hook or the user declined
+      True: On success or ignore hooks by user-request
-          to run a required hook (from _CheckForHookApproval).
+      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.
+    # Do not do anything in case bypass_hooks is set, or
-    if ((not self._hooks_project) or (self._hook_type not in
+    # no-op if there is no hooks project or if hook is disabled.
-                                      self._hooks_project.enabled_repo_hooks)):
+    if (self._bypass_hooks or
-      return
+        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.
+    passed = True
-    if not os.path.isfile(self._script_fullpath):
+    try:
-      raise HookError('Couldn\'t find repo hook: "%s"' % self._script_fullpath)
+      self._CheckHook()
 
-    # Make sure the user is OK with running the hook.
+      # Make sure the user is OK with running the hook.
-    if (not user_allows_all_hooks) and (not self._CheckForHookApproval()):
+      if self._allow_all_hooks or self._CheckForHookApproval():
-      return
+        # 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.
+    if not passed and self._ignore_hooks:
-    self._ExecuteHook(**kwargs)
+      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)

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).')
-
+    RepoHook.AddOptionGroup(p, 'pre-upload')
-    # 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.')
 
   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:
+    pending_proj_names = [project.name for (project, available) in pending]
-      hook = RepoHook('pre-upload', self.manifest.repo_hooks_project,
+    pending_worktrees = [project.worktree for (project, available) in pending]
-                      self.manifest.topdir,
+    hook = RepoHook.FromSubcmd(
-                      self.manifest.manifestProject.GetRemote('origin').url,
+        hook_type='pre-upload', manifest=self.manifest,
-                      abort_if_user_denies=True)
+        opt=opt, abort_if_user_denies=True)
-      pending_proj_names = [project.name for (project, available) in pending]
+    if not hook.Run(
-      pending_worktrees = [project.worktree for (project, available) in pending]
+        project_list=pending_proj_names,
-      passed = True
+        worktree_list=pending_worktrees):
-      try:
+      return 1
-        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
 
     if opt.reviewers:
       reviewers = _SplitEmails(opt.reviewers)