Re: [PATCH 1/1] Add the `p4-pre-submit` hook

[Luke Diamand <luke@diamand.org>]

On 30 July 2018 at 20:07, Junio C Hamano <gitster@pobox.com> wrote:
> Chen Bin <chenbin.sh@gmail.com> writes:
>
>> The `p4-pre-submit` hook is executed before git-p4 submits code.
>> If the hook exits with non-zero value, submit process not start.
>>
>> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
>> ---
>
> Luke, does this version look good to you?
>

Yes, I think so, We might find in the future that we do need an
additional hook *after* the change has been applied, but as per Chen's
comment, it sounds like that's not what is needed here; it can easily
be added in the future if necessary.

And there's no directly equivalent existing git-hook which could be
used instead, so this seems like a useful addition.

Possibly it should say "it takes no parameters" rather than "it takes
no parameter"; it is confusing that in English, zero takes the plural
rather than the singular. There's a PhD in linguistics there for
someone!

Luke


> I still think the addition to self.description is a bit too much but
> other than that the incremental since the last one looked sensible
> to my untrained eyes ;-)
>
> Thanks, both.
>
>>  Documentation/git-p4.txt   |  8 ++++++++
>>  Documentation/githooks.txt |  7 +++++++
>>  git-p4.py                  | 16 +++++++++++++++-
>>  t/t9800-git-p4-basic.sh    | 29 +++++++++++++++++++++++++++++
>>  4 files changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index f0de3b891..a7aac1b92 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -374,6 +374,14 @@ 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
>> +~~~~~~~~~~~~~~~
>> +The `p4-pre-submit` hook is executed if it exists and is executable.
>> +The hook takes no parameter and nothing from standard input. Exiting with
>> +non-zero status from this script prevents `git-p4 submit` from launching.
>> +
>> +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 e3c283a17..22fcabbe2 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the
>>  hook to limit its search.  On error, it will fall back to verifying
>>  all files and folders.
>>
>> +p4-pre-submit
>> +~~~~~~~~~~~~~
>> +
>> +This hook is invoked by `git-p4 submit`. It takes no parameter 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
>>  ---
>>  Part of the linkgit:git[1] suite
>> diff --git a/git-p4.py b/git-p4.py
>> index b449db1cc..879abfd2b 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1494,7 +1494,13 @@ def __init__(self):
>>                  optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
>>                                       help="Skip Perforce sync of p4/master after submit or shelve"),
>>          ]
>> -        self.description = "Submit changes from git to the perforce depot."
>> +        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 parameter and nothing from standard input. Exiting with
>> +    non-zero status from this script prevents `git-p4 submit` from launching.
>> +
>> +    One usage scenario is to run unit tests in the hook."""
>> +
>>          self.usage += " [name of git branch to submit into perforce depot]"
>>          self.origin = ""
>>          self.detectRenames = False
>> @@ -2303,6 +2309,14 @@ 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)
>> +
>>          #
>>          # Apply the commits, one at a time.  On failure, ask if should
>>          # continue to try the rest of the patches, or quit.
>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>> index 4849edc4e..2b7baa95d 100755
>> --- a/t/t9800-git-p4-basic.sh
>> +++ b/t/t9800-git-p4-basic.sh
>> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should display error' '
>>       )
>>  '
>>
>> +# Test following scenarios:
>> +#   - Without ".git/hooks/p4-pre-submit" , submit should continue
>> +#   - With the hook returning 0, submit should continue
>> +#   - With the hook returning 1, submit should abort
>> +test_expect_success 'run hook p4-pre-submit before submit' '
>> +     test_when_finished cleanup_git &&
>> +     git p4 clone --dest="$git" //depot &&
>> +     (
>> +             cd "$git" &&
>> +             echo "hello world" >hello.txt &&
>> +             git add hello.txt &&
>> +             git commit -m "add hello.txt" &&
>> +             git config git-p4.skipSubmitEdit true &&
>> +             git-p4 submit --dry-run >out &&
>> +             grep "Would apply" out &&
>> +             mkdir -p .git/hooks &&
>> +             write_script .git/hooks/p4-pre-submit <<-\EOF &&
>> +             exit 0
>> +             EOF
>> +             git-p4 submit --dry-run >out &&
>> +             grep "Would apply" out &&
>> +             write_script .git/hooks/p4-pre-submit <<-\EOF &&
>> +             exit 1
>> +             EOF
>> +             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&
>> +             ! grep "Would apply" err
>> +     )
>> +'
>> +
>>  test_expect_success 'submit from detached head' '
>>       test_when_finished cleanup_git &&
>>       git p4 clone --dest="$git" //depot &&