Re: [PATCH 1/1] add hook pre-p4-submit

Thanks for the review.

The hook does not receive any information or input from git. The
original requirement
comes from my colleague. He want to run unit test automatically before
submitting code
to remote repository. Or else CI server will send the blame mail to the manager.

The hook actually stops the submit process from start instead of abort
submit in midway.
So nothing is touched when hook exits with status 1.

I'm not sure whether `git-p4` should print some "hook rejection" message.
Current implementation is same as other hooks (`pre-commit`, for example).
Only hook itself is responsible to print error messages.

Personally I don't have opinion whether we should print out hook
related message inside
`git-p4.py`. I just try to following existing convention of git.


What you guys think?

Regards,
Chen

On Thu, Jul 26, 2018 at 6:00 AM, Eric Sunshine [off-list ref] wrote:

On Wed, Jul 25, 2018 at 9:45 AM Chen Bin [off-list ref] wrote:

quoted

Hook pre-p4-submit is executed before git-p4 actually submits code.
If the hook exits with non-zero value, submit process will abort.

Bikeshedding: Should this be named 'p4-pre-submit'? That way, if
git-p4 ever grows additional hooks, they can all be grouped under the
"p4-" prefix.

More below...

quoted

Signed-off-by: Chen Bin <redacted>
---

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt

@@ -374,6 +374,13 @@ These options can be used to modify 'git p4 submit' behavior.
+Hook for submit
+~~~~~~~~~~~~~~~
+Hook `pre-p4-submit` is executed if it exists and is executable.
+Exiting with non-zero status from this script cause `git-p4 submit` to abort.
+By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.

Does the hook receive any arguments? Does it receive anything on its
standard input? Those questions ought to be answered by the
documentation.

Also, how is the hook supposed to determine whether the "submit"
should proceed? How does it know what is being submitted? Perhaps the
documentation could provide some explanation of how the hook should
glean such information (especially if the hook does not itself receive
any input).

quoted

diff --git a/git-p4.py b/git-p4.py
+        # locate hook
+        hooks_path = gitConfig("core.hooksPath")
+        if len(hooks_path) > 0:
+            hook_file = os.path.join(hooks_path, "pre-p4-submit")
+        else:
+            hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "pre-p4-submit")
+
+        # Execute hook. If it exits with non-zero value, do NOT continue.
+        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
+            sys.exit(1)

Nit: The two #comments merely repeat what the code itself already says
clearly enough, thus don't add value (so can be dropped).

Should this be emitting some sort of message letting the user know
that the operation was aborted due to the hook's "rejection"? That
could be especially important if the hook itself is silent (or buggy).

quoted

diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh

@@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '
+# Test following scenarios:
+#   - Without hook ".git/hooks/pre-p4-submit", submit should continue
+#   - With hook returning 0, submit should continue
+#   - With hook returning 1, submit should abort

Testing three separate cases would normally be done with three
separate tests, making it easier to figure out which case failed. But,
perhaps it doesn't matter here.

quoted

+test_expect_success 'run hook pre-p4-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 &&

The wholesale removal of the $git directory by cleanup_git() when the
test finishes is undoing this config setting. Ditto regarding the
hooks created below. Okay.

quoted

+               git p4 submit --dry-run | grep "Would apply" &&

We normally don't use a git command upstream in a pipe since the exit
code of that command gets lost (so, if it crashes, we don't know about
it). Instead:

    git p4 submit --dry-run >out &&
    grep "Would apply" out &&

Also, if "Would apply" is localized, we'd use test_i18ngrep (but you
don't need it in this case).

quoted

+               mkdir -p .git/hooks &&
+               : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&

It might be clearer to readers of this test to use an explicit "exit
0" here (complementing the "exit 1" below). The current asymmetry
required extra cognitive effort to realize that this is the "returning
0" case mentioned in the comment above the test.

quoted

+               git p4 submit --dry-run | grep "Would apply" &&
+               echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&

Use write_script() for this, which takes care of the #!/bin/sh line
and doing chmod:

    write_script >.git/hooks/pre-p4-submit <<-\EOF &&
    exit 1
    EOF

quoted

+               git p4 submit --dry-run | grep "Would apply" || echo "Abort submit"

More idiomatic in this test suite:

    git p4 submit --dry-run >out &&
    ! grep "Would apply" out

quoted

+       )
+'

-- 
help me, help you.