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

Ben Keene <seraphire@xxxxxxxxx> · Mon, 10 Feb 2020 11:21:04 -0500

On 2/6/2020 2:42 PM, Junio C Hamano wrote:
"Ben Keene via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:

From: Ben Keene <seraphire@xxxxxxxxx>

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@xxxxxxxxx>
---
  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.
Thank you and and I'll resubmit the commits with these additional
suggestions.

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.
The print statement has been changed to:
print("Patch succeesed this time with RCS keywords cleaned")
and will be submitted as a separate commit so that it can be discussed
on its own. However, the rationale for it is that the current flow
reports to the user that the patch has failed and that it will attempt
to re-run the patch after cleaning up the RCS keywords. Since the
program told the user that it failed, I felt they should also be told
of the success at the same level of verbosity.

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






