add hook pre-p4-submit

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

Signed-off-by Chen Bin <[email protected]>

---
 git-p4.py               |  6 ++++++
 t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

git-p4.py +6 -0

index b449db1cc..69ee9ce41 100755
@@ -2303,6 +2303,12 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
+        # locate hook at `.git/hooks/pre-p4-submit`
+        hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "pre-p4-submit")
+        # Execute hook. If it exit 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)
+
         #
         # Apply the commits, one at a time.  On failure, ask if should
         # continue to try the rest of the patches, or quit.

t/t9800-git-p4-basic.sh +22 -1

index 4849edc4e..48b768fa7 100755
@@ -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
+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 &&
+               git p4 submit --dry-run | grep "Would apply" &&
+               mkdir -p .git/hooks &&
+               : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&
+               git p4 submit --dry-run | grep "Would apply" &&
+               echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&
+               git p4 submit --dry-run | grep "Would apply" || echo "Abort submit"
+       )
+'
+
 test_expect_success 'submit from detached head' '
        test_when_finished cleanup_git &&
        git p4 clone --dest="$git" //depot &&
--

2.18.0

Quoted 2 lines · Chen Bin

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

Looks good to me - could you add some documentation please (Documentation/git-p4.txt).

Thanks!
Luke

Quoted 60 lines

Signed-off-by Chen Bin <[email protected]>

---
 git-p4.py               |  6 ++++++
 t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

git-p4.py +6 -0

index b449db1cc..69ee9ce41 100755
@@ -2303,6 +2303,12 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
+        # locate hook at `.git/hooks/pre-p4-submit`
+        hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "pre-p4-submit")
+        # Execute hook. If it exit 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)
+
         #
         # Apply the commits, one at a time.  On failure, ask if should
         # continue to try the rest of the patches, or quit.

t/t9800-git-p4-basic.sh +22 -1

index 4849edc4e..48b768fa7 100755
@@ -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
+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 &&
+               git p4 submit --dry-run | grep "Would apply" &&
+               mkdir -p .git/hooks &&
+               : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&
+               git p4 submit --dry-run | grep "Would apply" &&
+               echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&
+               git p4 submit --dry-run | grep "Would apply" || echo "Abort submit"
+       )
+'
+
 test_expect_success 'submit from detached head' '
        test_when_finished cleanup_git &&
        git p4 clone --dest="$git" //depot &&
--

2.18.0

Quoted 10 lines · Luke Diamand

Quoted 2 lines · Chen Bin

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

Looks good to me - could you add some documentation please (Documentation/git-p4.txt).

Thanks!
Luke

This looks correct (and you'd know better), but I was surprised that we wouldn't just document this in githooks(5), but looking at git-p4 I see that we have its config documented there, not in git-config(1) (ditto some git-svn config stuff).

Shouldn't we at least update githooks & git-config to say that the config / hooks documentation for these utilities can be found there?

Quoted 60 lines

Quoted 60 lines

Signed-off-by Chen Bin <[email protected]>

---
 git-p4.py               |  6 ++++++
 t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

git-p4.py +6 -0

index b449db1cc..69ee9ce41 100755
@@ -2303,6 +2303,12 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
+        # locate hook at `.git/hooks/pre-p4-submit`
+        hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "pre-p4-submit")
+        # Execute hook. If it exit 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)
+
         #
         # Apply the commits, one at a time.  On failure, ask if should
         # continue to try the rest of the patches, or quit.

t/t9800-git-p4-basic.sh +22 -1

index 4849edc4e..48b768fa7 100755
@@ -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
+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 &&
+               git p4 submit --dry-run | grep "Would apply" &&
+               mkdir -p .git/hooks &&
+               : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&
+               git p4 submit --dry-run | grep "Would apply" &&
+               echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&
+               git p4 submit --dry-run | grep "Would apply" || echo "Abort submit"
+       )
+'
+
 test_expect_success 'submit from detached head' '
        test_when_finished cleanup_git &&
        git p4 clone --dest="$git" //depot &&
--

2.18.0

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

t/t9800-git-p4-basic.sh +2 -0

+       )
+'

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.

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 2 lines · Chen Bin

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...

Documentation/git-p4.txt These options can be used to modify 'git p4 submit' behavior. · line 378

Quoted 9 lines

Signed-off-by Chen Bin <[email protected]>

---

Documentation/git-p4.txt +5 -0

@@ -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).

git-p4.py def run(self, args): · line 2320

Quoted 11 lines

git-p4.py +10 -0

+        # 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).

t/t9800-git-p4-basic.sh test_expect_success 'unresolvable host in P4PORT should display error' ' · line 264

Quoted 6 lines

t/t9800-git-p4-basic.sh +4 -0

@@ -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.

t/t9800-git-p4-basic.sh test_expect_success 'unresolvable host in P4PORT should display error' ' · line 276

Quoted 9 lines

t/t9800-git-p4-basic.sh +9 -0

+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.

t/t9800-git-p4-basic.sh test_expect_success 'unresolvable host in P4PORT should display error' ' · line 274

Quoted 1 line

t/t9800-git-p4-basic.sh +1 -0

+               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).

t/t9800-git-p4-basic.sh test_expect_success 'unresolvable host in P4PORT should display error' ' · line 279

Quoted 2 lines

t/t9800-git-p4-basic.sh +2 -0

+               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.

t/t9800-git-p4-basic.sh test_expect_success 'unresolvable host in P4PORT should display error' ' · line 278

Quoted 2 lines

t/t9800-git-p4-basic.sh +2 -0

+               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

t/t9800-git-p4-basic.sh test_expect_success 'unresolvable host in P4PORT should display error' ' · line 282

Quoted 1 line

t/t9800-git-p4-basic.sh +1 -0

+               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 2 lines

t/t9800-git-p4-basic.sh +2 -0

+       )
+'

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?

Quoted 5 lines · chen bin

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.

Okay, that seems in line with a hook such as pre-commit. Please do update the documentation to mention that the hook takes no arguments and nothing on standard input, and perhaps describe in the documentation an example use-case (as you did here).

I'm not a p4 or git-p4 user, but, out of curiosity, would there be any information which could be supplied to the hook as arguments or standard input (or both) which would help the hook author implement the hook more easily? Perhaps such information would be fodder for a future enhancement (not necessarily needed for this patch).

Quoted 4 lines

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.

This might be a good thing to add to the documentation, as well.

Quoted 9 lines

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?

Following existing practice makes sense. It can always be revisited later if needed.

Thanks.

Quoted 12 lines · Eric Sunshine

Quoted 5 lines · chen bin

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.

Okay, that seems in line with a hook such as pre-commit. Please do update the documentation to mention that the hook takes no arguments and nothing on standard input, and perhaps describe in the documentation an example use-case (as you did here).

Quoted 5 lines

I'm not a p4 or git-p4 user, but, out of curiosity, would there be any information which could be supplied to the hook as arguments or standard input (or both) which would help the hook author implement the hook more easily? Perhaps such information would be fodder for a future enhancement (not necessarily needed for this patch).

I tried to think of a use-case for a hook requiring any more information, but I can't think of any. You're already chdir()'d to the P4 shadow repo which is what you really need.

Anything where you just need the commit hash (e.g. checking the commit message) can already be done with one of the existing git hooks; I don't think git-p4 needs to duplicate that.

And we can't write a commit hook that can know about the Perforce changelist, because we don't know what it is yet.

However, looking at the code, it runs the hook at the point just *before* the changes are applied to the P4 shadow repo. Would it make more sense to run the hook *after* they have been applied (but before being P4 submitted) ?

That way you can run your tests on the checked-out P4 shadow directory with your changes - as it stands, you can only run them on your git repo at this point, which might not be in sync with Perforce (and could be quite a long way out in fact).

Luke

Quoted 21 lines

Quoted 4 lines

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.

This might be a good thing to add to the documentation, as well.

Quoted 9 lines

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?

Following existing practice makes sense. It can always be revisited later if needed.

Thanks.

Documentation/githooks.txt +0 -0

index e3c283a17..22fcabbe2 100644

Documentation/githooks.txt +0 -0

 GIT
 ---
 Part of the linkgit:git[1] suite

git-p4.py +0 -1

index b449db1cc..f147a2d2f 100755
@@ -1494,7 +1494,17 @@ 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."

git-p4.py +3 -0

+        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.

t/t9800-git-p4-basic.sh +4 -0

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

t/t9800-git-p4-basic.sh +3 -0

+	)
+'
+
 test_expect_success 'submit from detached head' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&

Quoted 1 line · Chen Bin

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

What is this last "|| echo I always succeed" about?

Do you want to make sure "git p4 submit" exit with non-zero exit status *and* its output does not say "Would apply"? The way to write that would be

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

git-p4.py +5 -0

+    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."""

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 <[email protected]>

---

 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(-)

Documentation/git-p4.txt +8 -0

index f0de3b891..a7aac1b92 100644
@@ -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.

Documentation/githooks.txt +7 -0

index e3c283a17..22fcabbe2 100644
@@ -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

git-p4.py +15 -1

index b449db1cc..879abfd2b 100755
@@ -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.

t/t9800-git-p4-basic.sh +29 -0

index 4849edc4e..2b7baa95d 100755
@@ -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 &&

Quoted 5 lines · Chen Bin

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 <[email protected]>

---

Luke, does this version look good to you?

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.

Quoted 117 lines

 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(-)

Documentation/git-p4.txt +8 -0

index f0de3b891..a7aac1b92 100644
@@ -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.

Documentation/githooks.txt +7 -0

index e3c283a17..22fcabbe2 100644
@@ -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

git-p4.py +15 -1

index b449db1cc..879abfd2b 100755
@@ -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.

t/t9800-git-p4-basic.sh +29 -0

index 4849edc4e..2b7baa95d 100755
@@ -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 &&

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!

t/t9800-git-p4-basic.sh +4 -0

index 4849edc4e..2b7baa95d 100755
@@ -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

t/t9800-git-p4-basic.sh +4 -0

+		! grep "Would apply" err
+	)
+'
+
 test_expect_success 'submit from detached head' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&

Signature · 1 line

2.18.0

Review

Quoted 2 lines

(quoted diff) +2 -0

+             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
+             ! grep "Would apply" err

I think there is an error in the test harness.

Quoted 2 lines · SZEDER Gábor

Review

Quoted 2 lines

(quoted diff) +2 -0

+             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
+             ! grep "Would apply" err

It writes to the file "errs" but then looks for the message in "err".

Luke

Quoted 9 lines · Luke Diamand

I think there is an error in the test harness.

Quoted 2 lines · SZEDER Gábor

Review

Quoted 2 lines

(quoted diff) +2 -0

+             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
+             ! grep "Would apply" err

It writes to the file "errs" but then looks for the message in "err".

Luke

Sigh. Thanks for spotting, both of you.

Here is what I'd locally squash in. If there is anything else, I'd rather see a refreshed final one sent by the author.

Thanks.

 t/t9800-git-p4-basic.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

t/t9800-git-p4-basic.sh +2 -2

index 2b7baa95d2..729cd25770 100755
@@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before submit' '
                git add hello.txt &&
                git commit -m "add hello.txt" &&
                git config git-p4.skipSubmitEdit true &&
-               git-p4 submit --dry-run >out &&
+               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 &&
+               git p4 submit --dry-run >out &&

Quoted 39 lines · Junio C Hamano

Quoted 9 lines · Luke Diamand

I think there is an error in the test harness.

Quoted 2 lines · SZEDER Gábor

Review

Quoted 2 lines

(quoted diff) +2 -0

+             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
+             ! grep "Would apply" err

It writes to the file "errs" but then looks for the message in "err".

Luke

Sigh. Thanks for spotting, both of you.

Here is what I'd locally squash in. If there is anything else, I'd rather see a refreshed final one sent by the author.

Thanks.

 t/t9800-git-p4-basic.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

t/t9800-git-p4-basic.sh +2 -2

index 2b7baa95d2..729cd25770 100755
@@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before submit' '
                git add hello.txt &&
                git commit -m "add hello.txt" &&
                git config git-p4.skipSubmitEdit true &&
-               git-p4 submit --dry-run >out &&
+               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 &&
+               git p4 submit --dry-run >out &&

t/t9800-git-p4-basic.sh test_expect_success 'run hook p4-pre-submit before submit' ' · line 291

Quoted 10 lines

t/t9800-git-p4-basic.sh +2 -2

                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_must_fail git p4 submit --dry-run >errs 2>&1 &&
+               ! grep "Would apply" errs
        )
 '

That set of deltas works for me.

Sorry, I should have run the tests myself earlier and I would have picked up on this.

I updated the patch. But for some reason the test keep failing at this line, test_must_fail git p4 submit --dry-run >errs 2>&1 &&.

If I change this line to test_must_fail git-p4 submit --dry-run >errs 2>&1 && the test will pass.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines · SZEDER Gábor

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
+             ! grep "Would apply" err

The p4-pre-submit hook is executed before git-p4 submits code. If the hook exits with non-zero value, submit process won't start.

Signed-off-by Chen Bin <[email protected]>

---