2 files changed, 128 insertions, 81 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
-
-    # 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)
+    # 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
+
+    passed = True
+    try:
+      self._CheckHook()
+
+      # 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)
+
+    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
 
-    # Make sure the user is OK with running the hook.
-    if (not user_allows_all_hooks) and (not self._CheckForHookApproval()):
-      return
+    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)
 
-    # Run the hook with the same version of python we're using.
-    self._ExecuteHook(**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)