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

[Ben Keene <seraphire@gmail.com>]

On 2/6/2020 2:42 PM, Junio C Hamano wrote:
> "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.
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