Re: [PATCH v3 3/5] git-p4: add --no-verify option

[Junio C Hamano <gitster@pobox.com>]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
>
> Add new command line option --no-verify:
>
> Add a new command line option "--no-verify" to the Submit command of
> git-p4.py.  This option will function in the spirit of the existing
> --no-verify command line option found in git commit. It will cause the
> P4 Submit function to ignore the existing p4-pre-submit.
>
> Change the execution of the existing trigger p4-pre-submit to honor the
> --no-verify option. Before exiting on failure of this hook, display
> text to the user explaining which hook has failed and the impact
> of using the --no-verify option.
>
> Change the call of the p4-pre-submit hook to use the new run_git_hook
> function. This is in preparation of additional hooks to be added.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> ---
>  Documentation/git-p4.txt   | 10 ++++++++--
>  Documentation/githooks.txt |  5 ++++-
>  git-p4.py                  | 30 +++++++++++++++++++-----------
>  3 files changed, 31 insertions(+), 14 deletions(-)

Nicely done.  If your strategy is to "add a feature and use it in
the same patch as the feature is added", which is what is done for
the new "no-verify" feature that is applied to existing p4-pre-submit
hook, then the code that runs p4-pre-submit in the original should
be changed to use run_git_hook() in the previous step, which added
the new run_git_hook() feature.

I see new print() that is not protected with "if verbose:"; is it
debugging cruft added during development, or is it a useful addition
for end-users to see under --verbose mode?  I suspect it is the
latter.

Thanks.

> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 3494a1db3e..362b50eb21 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -374,14 +374,20 @@ These options can be used to modify 'git p4 submit' behavior.
>      been submitted. Implies --disable-rebase. Can also be set with
>      git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.
>  
> -Hook for submit
> -~~~~~~~~~~~~~~~
> +Hooks for submit
> +----------------
> +
> +p4-pre-submit
> +~~~~~~~~~~~~~
> +
>  The `p4-pre-submit` hook is executed if it exists and is executable.
>  The hook takes no parameters and nothing from standard input. Exiting with
>  non-zero status from this script prevents `git-p4 submit` from launching.
> +It can be bypassed with the `--no-verify` command line option.
>  
>  One usage scenario is to run unit tests in the hook.
>  
> +
>  Rebase options
>  ~~~~~~~~~~~~~~
>  These options can be used to modify 'git p4 rebase' behavior.
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 50365f2914..8cf6b08b55 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -520,7 +520,10 @@ p4-pre-submit
>  
>  This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
>  from standard input. Exiting with non-zero status from this script prevent
> -`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
> +`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
> +command line option. Run `git-p4 submit --help` for details.
> +
> +
>  
>  post-index-change
>  ~~~~~~~~~~~~~~~~~
> diff --git a/git-p4.py b/git-p4.py
> index d4c39f112b..b377484464 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1583,13 +1583,17 @@ def __init__(self):
>                                       "work from a local git branch that is not master"),
>                  optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
>                                       help="Skip Perforce sync of p4/master after submit or shelve"),
> +                optparse.make_option("--no-verify", dest="no_verify", action="store_true",
> +                                     help="Bypass p4-pre-submit"),
>          ]
>          self.description = """Submit changes from git to the perforce depot.\n
> -    The `p4-pre-submit` hook is executed if it exists and is executable.
> -    The hook takes no parameters and nothing from standard input. Exiting with
> -    non-zero status from this script prevents `git-p4 submit` from launching.
> +    The `p4-pre-submit` hook is executed if it exists and is executable. It
> +    can be bypassed with the `--no-verify` command line option. The hook takes
> +    no parameters and nothing from standard input. Exiting with a non-zero status
> +    from this script prevents `git-p4 submit` from launching.
>  
> -    One usage scenario is to run unit tests in the hook."""
> +    One usage scenario is to run unit tests in the hook.
> +    """
>  
>          self.usage += " [name of git branch to submit into perforce depot]"
>          self.origin = ""
> @@ -1607,6 +1611,7 @@ def __init__(self):
>          self.exportLabels = False
>          self.p4HasMoveCommand = p4_has_move_command()
>          self.branch = None
> +        self.no_verify = False
>  
>          if gitConfig('git-p4.largeFileSystem'):
>              die("Large file system not supported for git-p4 submit command. Please remove it from config.")
> @@ -1993,6 +1998,9 @@ def applyCommit(self, id):
>          applyPatchCmd = patchcmd + "--check --apply -"
>          patch_succeeded = True
>  
> +        if verbose:
> +            print("TryPatch: %s" % tryPatchCmd)
> +
>          if os.system(tryPatchCmd) != 0:
>              fixed_rcs_keywords = False
>              patch_succeeded = False
> @@ -2032,6 +2040,7 @@ def applyCommit(self, id):
>                  print("Retrying the patch with RCS keywords cleaned up")
>                  if os.system(tryPatchCmd) == 0:
>                      patch_succeeded = True
> +                    print("Patch succeesed this time")
>  
>          if not patch_succeeded:
>              for f in editedFiles:
> @@ -2400,13 +2409,12 @@ def run(self, args):
>              sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
>                       (len(commits), num_shelves))
>  
> -        hooks_path = gitConfig("core.hooksPath")
> -        if len(hooks_path) <= 0:
> -            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
> -
> -        hook_file = os.path.join(hooks_path, "p4-pre-submit")
> -        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
> -            sys.exit(1)
> +        if not self.no_verify:
> +            if not run_git_hook("p4-pre-submit"):
> +                print("\nThe p4-pre-submit hook failed, aborting the submit.\n\nYou can skip " \
> +                    "this pre-submission check by adding\nthe command line option '--no-verify', " \
> +                    "however,\nthis will also skip the p4-changelist hook as well.")
> +                sys.exit(1)
>  
>          #
>          # Apply the commits, one at a time.  On failure, ask if should