All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] add hook pre-p4-submit
@ 2018-07-23 11:27 Chen Bin
  2018-07-25  8:37 ` Luke Diamand
  0 siblings, 1 reply; 27+ messages in thread
From: Chen Bin @ 2018-07-23 11:27 UTC (permalink / raw)
  To: git; +Cc: 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.

Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
---
 git-p4.py               |  6 ++++++
 t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/git-p4.py b/git-p4.py
index b449db1cc..69ee9ce41 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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.
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 4849edc4e..48b768fa7 100755
--- 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
+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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-23 11:27 [PATCH 1/1] add hook pre-p4-submit Chen Bin
@ 2018-07-25  8:37 ` Luke Diamand
  2018-07-25  9:14   ` Ævar Arnfjörð Bjarmason
  2018-07-25 13:43   ` Chen Bin
  0 siblings, 2 replies; 27+ messages in thread
From: Luke Diamand @ 2018-07-25  8:37 UTC (permalink / raw)
  To: Chen Bin; +Cc: Git Users

On 23 July 2018 at 12:27, Chen Bin <chenbin.sh@gmail.com> wrote:
> 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


>
> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
> ---
>  git-p4.py               |  6 ++++++
>  t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/git-p4.py b/git-p4.py
> index b449db1cc..69ee9ce41 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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.
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 4849edc4e..48b768fa7 100755
> --- 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
> +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
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-25  8:37 ` Luke Diamand
@ 2018-07-25  9:14   ` Ævar Arnfjörð Bjarmason
  2018-07-25 11:10     ` chen bin
  2018-07-25 13:43   ` Chen Bin
  1 sibling, 1 reply; 27+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-25  9:14 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Chen Bin, Git ML


On Wed, Jul 25 2018, Luke Diamand wrote:

> On 23 July 2018 at 12:27, Chen Bin <chenbin.sh@gmail.com> wrote:
>> 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?

>>
>> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
>> ---
>>  git-p4.py               |  6 ++++++
>>  t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index b449db1cc..69ee9ce41 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -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.
>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>> index 4849edc4e..48b768fa7 100755
>> --- 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
>> +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
>>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-25  9:14   ` Ævar Arnfjörð Bjarmason
@ 2018-07-25 11:10     ` chen bin
  0 siblings, 0 replies; 27+ messages in thread
From: chen bin @ 2018-07-25 11:10 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Luke Diamand, Git ML

Thanks for you review. I will update documentation asap.

On Wed, Jul 25, 2018 at 7:14 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Jul 25 2018, Luke Diamand wrote:
>
>> On 23 July 2018 at 12:27, Chen Bin <chenbin.sh@gmail.com> wrote:
>>> 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?
>
>>>
>>> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
>>> ---
>>>  git-p4.py               |  6 ++++++
>>>  t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++
>>>  2 files changed, 28 insertions(+)
>>>
>>> diff --git a/git-p4.py b/git-p4.py
>>> index b449db1cc..69ee9ce41 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -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.
>>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>>> index 4849edc4e..48b768fa7 100755
>>> --- 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
>>> +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
>>>



-- 
help me, help you.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/1] add hook pre-p4-submit
  2018-07-25  8:37 ` Luke Diamand
  2018-07-25  9:14   ` Ævar Arnfjörð Bjarmason
@ 2018-07-25 13:43   ` Chen Bin
  2018-07-25 20:00     ` Eric Sunshine
  2018-07-26 13:41     ` Chen Bin
  1 sibling, 2 replies; 27+ messages in thread
From: Chen Bin @ 2018-07-25 13:43 UTC (permalink / raw)
  To: git; +Cc: 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.

Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
---
 Documentation/git-p4.txt   |  7 +++++++
 Documentation/githooks.txt |  7 +++++++
 git-p4.py                  | 18 +++++++++++++++++-
 t/t9800-git-p4-basic.sh    | 22 ++++++++++++++++++++++
 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index f0de3b891..73f7a2691 100644
--- 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.
     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
+~~~~~~~~~~~~~~~
+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.
+See githooks(5) manpage for details about hooks.
+
 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..5148627b4 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.
 
+pre-p4-submit
+~~~~~~~~~~~~~
+
+This hook is invoked by `git-p4 submit`. It takes no parameter, and
+exiting with non-zero status from this script causes `git-p4 submit`
+to abort. 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..bbb0d987e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1494,7 +1494,12 @@ 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
+    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.
+    See githooks(5) manpage for details about hooks."""
+
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
         self.detectRenames = False
@@ -2303,6 +2308,17 @@ def run(self, args):
             sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
                      (len(commits), num_shelves))
 
+        # 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)
+
         #
         # 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..48b768fa7 100755
--- 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
+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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-25 13:43   ` Chen Bin
@ 2018-07-25 20:00     ` Eric Sunshine
  2018-07-25 20:21       ` Junio C Hamano
  2018-07-26  2:08       ` chen bin
  2018-07-26 13:41     ` Chen Bin
  1 sibling, 2 replies; 27+ messages in thread
From: Eric Sunshine @ 2018-07-25 20:00 UTC (permalink / raw)
  To: chenbin.sh; +Cc: Git List

On Wed, Jul 25, 2018 at 9:45 AM Chen Bin <chenbin.sh@gmail.com> wrote:
> 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...

> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
> ---
> 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).

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

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

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

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

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

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

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

> +       )
> +'

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-25 20:00     ` Eric Sunshine
@ 2018-07-25 20:21       ` Junio C Hamano
  2018-07-26  2:08       ` chen bin
  1 sibling, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2018-07-25 20:21 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: chenbin.sh, Git List

Eric Sunshine <sunshine@sunshineco.com> writes:

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

An excellent suggestion.

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

Yes, such information in the documentation, or lack thereof, would
mean the distinction between a useful feature and an unusable one.

All other review comments looked quite sensible and an update should
address all of them.

Thanks for being a careful reviewer, as always.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-25 20:00     ` Eric Sunshine
  2018-07-25 20:21       ` Junio C Hamano
@ 2018-07-26  2:08       ` chen bin
  2018-07-26  9:21         ` Eric Sunshine
  1 sibling, 1 reply; 27+ messages in thread
From: chen bin @ 2018-07-26  2:08 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Git List

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 <sunshine@sunshineco.com> wrote:
> On Wed, Jul 25, 2018 at 9:45 AM Chen Bin <chenbin.sh@gmail.com> wrote:
>> 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...
>
>> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
>> ---
>> 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).
>
>> 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).
>
>> 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.
>
>> +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.
>
>> +               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).
>
>> +               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.
>
>> +               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
>
>> +               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
>
>> +       )
>> +'



-- 
help me, help you.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-26  2:08       ` chen bin
@ 2018-07-26  9:21         ` Eric Sunshine
  2018-07-26 21:09           ` Luke Diamand
  0 siblings, 1 reply; 27+ messages in thread
From: Eric Sunshine @ 2018-07-26  9:21 UTC (permalink / raw)
  To: bin chen; +Cc: Git List

On Wed, Jul 25, 2018 at 10:08 PM chen bin <chenbin.sh@gmail.com> wrote:
> 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).

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

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/1] add hook pre-p4-submit
  2018-07-25 13:43   ` Chen Bin
  2018-07-25 20:00     ` Eric Sunshine
@ 2018-07-26 13:41     ` Chen Bin
  2018-07-26 16:49       ` Junio C Hamano
  1 sibling, 1 reply; 27+ messages in thread
From: Chen Bin @ 2018-07-26 13:41 UTC (permalink / raw)
  To: git; +Cc: 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.

Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
---
 Documentation/git-p4.txt   | 12 ++++++++++++
 Documentation/githooks.txt |  7 +++++++
 git-p4.py                  | 21 ++++++++++++++++++++-
 t/t9800-git-p4-basic.sh    | 26 ++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index f0de3b891..f5d428ddf 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -374,6 +374,18 @@ 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
+~~~~~~~~~~~~~~~
+Hook `p4-pre-submit` 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.
+So nothing is touched when it exits with non-zero status.
+
+One usage scenario is to run unit tests in the hook.
+
+By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
+See githooks(5) manpage for details about hooks.
+
 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..f147a2d2f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -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."
+        self.description = """Submit changes from git to the perforce depot.\n
+    Hook `p4-pre-submit` 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.
+    So nothing is touched when it exits with non-zero status.
+
+    One usage scenario is to run unit tests in the hook.
+
+    By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
+    See githooks(5) manpage for details about hooks."""
+
         self.usage += " [name of git branch to submit into perforce depot]"
         self.origin = ""
         self.detectRenames = False
@@ -2303,6 +2313,15 @@ 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:
+            hook_file = os.path.join(hooks_path, "p4-pre-submit")
+        else:
+            hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "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..8457ff617 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -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
+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 | grep "Would apply" &&
+		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
+		git p4 submit --dry-run >out && grep "Would apply" out || echo "Abort submit"
+	)
+'
+
 test_expect_success 'submit from detached head' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.18.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-26 13:41     ` Chen Bin
@ 2018-07-26 16:49       ` Junio C Hamano
  2018-07-26 21:20         ` Eric Sunshine
  2018-07-27 11:22         ` [PATCH 1/1] Add the `p4-pre-submit` hook Chen Bin
  0 siblings, 2 replies; 27+ messages in thread
From: Junio C Hamano @ 2018-07-26 16:49 UTC (permalink / raw)
  To: Chen Bin; +Cc: git

Chen Bin <chenbin.sh@gmail.com> writes:

> +Hook for submit
> +~~~~~~~~~~~~~~~
> +Hook `p4-pre-submit` is executed if it exists and is executable.

Just a lang nit, but "git grep pre-commit" shows me lines like

  Documentation/git-commit.txt:   This option bypasses the pre-commit and commit-msg hooks.

to tell me that the above is better phrased like

	The `p4-pre-submit` hook is executed...

The same comment applies to the patch title, the first sentence in
the log message and in-code ".description" field in git-py.py.

> +The hook takes no parameter and nothing from standard input. Exiting with
> +non-zero status from this script prevents `git-p4 submit` from launching.
> +So nothing is touched when it exits with non-zero status.

Isn't the last sentence redundant, saying only what the second
sentence already said?

> +
> +One usage scenario is to run unit tests in the hook.

Good thing to mention, I guess.

> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
> +See githooks(5) manpage for details about hooks.

I do not think this is particularly a good change; it forces us to
maintain this sentence copied from githooks.txt every time the
original has to change, and documentation for no other command
duplicates it.

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

Hmmm, OK.

>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/git-p4.py b/git-p4.py
> index b449db1cc..f147a2d2f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -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."
> +        self.description = """Submit changes from git to the perforce depot.\n
> +    Hook `p4-pre-submit` 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.
> +    So nothing is touched when it exits with non-zero status.
> +
> +    One usage scenario is to run unit tests in the hook.
> +
> +    By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
> +    See githooks(5) manpage for details about hooks."""

How do you plan to keep this in sync with the documentation?
Perhaps the last sentence of the new paragraph you added to
Documentation/githooks.txt should read

	See `git p4 --help` for details.

Then this hunk can be eliminated, I would think.

> @@ -2303,6 +2313,15 @@ 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:
> +            hook_file = os.path.join(hooks_path, "p4-pre-submit")
> +        else:
> +            hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "p4-pre-submit")
> +

Isn't the GIT_DIR available as self.gitdir at this point in the
code, prepared by main() before this method is run?

It may be a style thing, but I somehow would find it easier to
understand if you wrote it like so:

	hooks_path = gitConfig("...")
	if len(hooks_path) <= 0:
		hooks_path = os.path.join(self.gitdir, "hooks")
	hook_file = os.path.join(hooks_path, "p4-pre-submit")

The point being hooks_path always point at the directory that holds
hooks with or without configuration, and hook_file is set based on
that to the same "p4-pre-submit" without any potential of typo
getting in to make the two cases diverge.

> +        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..8457ff617 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -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
> +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 | grep "Would apply" &&

"git p4" on the upstream side of a pipe?

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

Have each of these two on its own line, i.e.

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

> +		write_script .git/hooks/p4-pre-submit <<-\EOF &&
> +		exit 1
> +		EOF
> +		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

Then "git p4 submit --dry-run" that exits with 0 even though its
presubmit hook exits with 1 would fail to meet the expectation that
the command must fail and would be caught as an error.  If it exits
with non-zero status, but still write "Would apply" to its standard
output stream, it would also be caught as an error as "grep" would
succeed.

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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-26  9:21         ` Eric Sunshine
@ 2018-07-26 21:09           ` Luke Diamand
  2018-07-27  1:31             ` chen bin
  0 siblings, 1 reply; 27+ messages in thread
From: Luke Diamand @ 2018-07-26 21:09 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: bin chen, Git List

On 26 July 2018 at 10:21, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jul 25, 2018 at 10:08 PM chen bin <chenbin.sh@gmail.com> wrote:
>> 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).


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


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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-26 16:49       ` Junio C Hamano
@ 2018-07-26 21:20         ` Eric Sunshine
  2018-07-27 11:22         ` [PATCH 1/1] Add the `p4-pre-submit` hook Chen Bin
  1 sibling, 0 replies; 27+ messages in thread
From: Eric Sunshine @ 2018-07-26 21:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: bin chen, Git List

On Thu, Jul 26, 2018 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote:
> Chen Bin <chenbin.sh@gmail.com> writes:
> > +             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

I missed the 'test_must_fail' when I suggested this same rewrite[1],
which may explain why the suggestion wasn't used in the re-roll.
Sorry.

[1]: https://public-inbox.org/git/CAPig+cR2gYEwOTVBMRde35rn9oVsixeerbm5iJV+FmnOiBWxqQ@mail.gmail.com/

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] add hook pre-p4-submit
  2018-07-26 21:09           ` Luke Diamand
@ 2018-07-27  1:31             ` chen bin
  0 siblings, 0 replies; 27+ messages in thread
From: chen bin @ 2018-07-27  1:31 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Eric Sunshine, Git List

Hi, Luke,
Running the hook after applying changes to P4 shadow repo might not be
a good idea.

When the hook is running *before* the changes are applied, I'm 100% sure it's
*only* my code's problem if the hook fails.

One reason we need this hook is sometimes developer is over confident
when applying
some quick one line fix.

The quick fix could be required by business guy before demo to senior
managers or customers.
So we might not want our fix being blocked by upstream commits.

Not everyone is our team is perforce/git expert. Someone only use
`git-p4 submit` and never use
 `git-p4 rebase`. If unit test fails and he could not submit code, he
would come to me for help. But I
want to reduce my workload.


Regards,
Chen


On Fri, Jul 27, 2018 at 7:09 AM, Luke Diamand <luke@diamand.org> wrote:
> On 26 July 2018 at 10:21, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jul 25, 2018 at 10:08 PM chen bin <chenbin.sh@gmail.com> wrote:
>>> 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).
>
>
> 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
>
>
>>
>>> 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.
>>
>>> 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.



-- 
help me, help you.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-26 16:49       ` Junio C Hamano
  2018-07-26 21:20         ` Eric Sunshine
@ 2018-07-27 11:22         ` Chen Bin
  2018-07-27 18:15           ` Eric Sunshine
                             ` (2 more replies)
  1 sibling, 3 replies; 27+ messages in thread
From: Chen Bin @ 2018-07-27 11:22 UTC (permalink / raw)
  To: git; +Cc: 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 <chenbin.sh@gmail.com>
---
 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 &&
-- 
2.18.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-27 11:22         ` [PATCH 1/1] Add the `p4-pre-submit` hook Chen Bin
@ 2018-07-27 18:15           ` Eric Sunshine
  2018-07-30 18:07           ` Junio C Hamano
  2018-07-31  8:46           ` SZEDER Gábor
  2 siblings, 0 replies; 27+ messages in thread
From: Eric Sunshine @ 2018-07-27 18:15 UTC (permalink / raw)
  To: bin chen; +Cc: Git List

On Fri, Jul 27, 2018 at 7:22 AM Chen Bin <chenbin.sh@gmail.com> wrote:
> 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>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -1494,7 +1494,13 @@ def __init__(self):
> -        self.description = "Submit changes from git to the perforce depot."
> +        self.description = """Submit changes from git to the perforce depot.\n

It seemed odd to have an explicit \n within a """...""" string as
opposed to simply using a blank line, but I see there is existing
precedence in git-p4.py when setting (at least one) self.description.
Okay.

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-27 11:22         ` [PATCH 1/1] Add the `p4-pre-submit` hook Chen Bin
  2018-07-27 18:15           ` Eric Sunshine
@ 2018-07-30 18:07           ` Junio C Hamano
  2018-07-30 18:48             ` Luke Diamand
  2018-07-31  8:46           ` SZEDER Gábor
  2 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2018-07-30 18:07 UTC (permalink / raw)
  To: Luke Diamand; +Cc: git, Chen Bin

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?

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-30 18:07           ` Junio C Hamano
@ 2018-07-30 18:48             ` Luke Diamand
  2018-07-30 21:01               ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Luke Diamand @ 2018-07-30 18:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Users, Chen Bin

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-30 18:48             ` Luke Diamand
@ 2018-07-30 21:01               ` Junio C Hamano
  0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2018-07-30 21:01 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Git Users, Chen Bin

Luke Diamand <luke@diamand.org> writes:

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

I count three instances.  Will squash in the following while queuing
with your Reviewed-by.

Thanks.

 Documentation/git-p4.txt   | 2 +-
 Documentation/githooks.txt | 2 +-
 git-p4.py                  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index a7aac1b920..41780a5aa9 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -377,7 +377,7 @@ These options can be used to modify 'git p4 submit' behavior.
 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
+The hook takes no parameters 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.
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 22fcabbe21..959044347e 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -488,7 +488,7 @@ all files and folders.
 p4-pre-submit
 ~~~~~~~~~~~~~
 
-This hook is invoked by `git-p4 submit`. It takes no parameter and nothing
+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.
 
diff --git a/git-p4.py b/git-p4.py
index 879abfd2b1..7fab255584 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1496,7 +1496,7 @@ def __init__(self):
         ]
         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
+    The hook takes no parameters 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."""

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-27 11:22         ` [PATCH 1/1] Add the `p4-pre-submit` hook Chen Bin
  2018-07-27 18:15           ` Eric Sunshine
  2018-07-30 18:07           ` Junio C Hamano
@ 2018-07-31  8:46           ` SZEDER Gábor
  2018-07-31 13:40             ` Luke Diamand
  2 siblings, 1 reply; 27+ messages in thread
From: SZEDER Gábor @ 2018-07-31  8:46 UTC (permalink / raw)
  To: Chen Bin; +Cc: SZEDER Gábor, git, Junio C Hamano


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

This must be 'git p4 ...', i.e. without dash.

The dashed form causes the test to fail:

  <...>
  +git config git-p4.skipSubmitEdit true
  +git-p4 submit --dry-run
  t9800-git-p4-basic.sh: 12: eval: git-p4: not found
  error: last command exited with $?=127
  not ok 19 - run hook p4-pre-submit before submit


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

Likewise.

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

Likewise.

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-31  8:46           ` SZEDER Gábor
@ 2018-07-31 13:40             ` Luke Diamand
  2018-07-31 14:40               ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Luke Diamand @ 2018-07-31 13:40 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Chen Bin, Git Users, Junio C Hamano

I think there is an error in the test harness.

On 31 July 2018 at 10:46, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> +             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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-31 13:40             ` Luke Diamand
@ 2018-07-31 14:40               ` Junio C Hamano
  2018-07-31 19:54                 ` Luke Diamand
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2018-07-31 14:40 UTC (permalink / raw)
  To: Luke Diamand; +Cc: SZEDER Gábor, Chen Bin, Git Users

Luke Diamand <luke@diamand.org> writes:

> I think there is an error in the test harness.
>
> On 31 July 2018 at 10:46, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>> +             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(-)

diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
index 2b7baa95d2..729cd25770 100755
--- a/t/t9800-git-p4-basic.sh
+++ b/t/t9800-git-p4-basic.sh
@@ -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 &&
 		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
 	)
 '
 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-31 14:40               ` Junio C Hamano
@ 2018-07-31 19:54                 ` Luke Diamand
  2018-08-01 14:57                   ` chen bin
  0 siblings, 1 reply; 27+ messages in thread
From: Luke Diamand @ 2018-07-31 19:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: SZEDER Gábor, Chen Bin, Git Users

On 31 July 2018 at 16:40, Junio C Hamano <gitster@pobox.com> wrote:
> Luke Diamand <luke@diamand.org> writes:
>
>> I think there is an error in the test harness.
>>
>> On 31 July 2018 at 10:46, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>>> +             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(-)
>
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 2b7baa95d2..729cd25770 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -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 &&
>                 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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-07-31 19:54                 ` Luke Diamand
@ 2018-08-01 14:57                   ` chen bin
  2018-08-01 15:10                     ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: chen bin @ 2018-08-01 14:57 UTC (permalink / raw)
  To: Luke Diamand; +Cc: Junio C Hamano, SZEDER Gábor, Git Users

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.

Could you help me to resolve this issue? I'm really confused. I've
already added latest `p4d` and `p4` to $PATH.

The commit is at
https://github.com/redguardtoo/git/commit/b88c38b9ce6cfb1c1fefef15527adfa92d78daf2


On Wed, Aug 1, 2018 at 5:54 AM, Luke Diamand <luke@diamand.org> wrote:
> On 31 July 2018 at 16:40, Junio C Hamano <gitster@pobox.com> wrote:
>> Luke Diamand <luke@diamand.org> writes:
>>
>>> I think there is an error in the test harness.
>>>
>>> On 31 July 2018 at 10:46, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>>>> +             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(-)
>>
>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>> index 2b7baa95d2..729cd25770 100755
>> --- a/t/t9800-git-p4-basic.sh
>> +++ b/t/t9800-git-p4-basic.sh
>> @@ -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 &&
>>                 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.



-- 
help me, help you.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-08-01 14:57                   ` chen bin
@ 2018-08-01 15:10                     ` Junio C Hamano
  2018-08-01 15:54                       ` Chen Bin
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2018-08-01 15:10 UTC (permalink / raw)
  To: chen bin; +Cc: Luke Diamand, SZEDER Gábor, Git Users

chen bin <chenbin.sh@gmail.com> writes:

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

Hmph.  I somehow suspect that the test also will pass if you changed
it like this:

	test_must_fail false >errs 2>&1 &&

IOW, my suspicion is that the shell fails to find "git-p4" [*1*] and
that is why your `test_must_fail git-p4 whatever` lets your test
pass, which is different from the way you _want_ your test_must_fail
succeed, i.e. "git p4 submit" is run with the "--dry-run" option and
exit with non-zero status.

Of course, if the shell cannot find "git-p4", `errs` would not have
the string "Would apply" in it, so the next test also would pass.

>>>> On 31 July 2018 at 10:46, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>>>>>> +             test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>
>>>>>> +             ! grep "Would apply" err



[Footnote]

 *1* As I do not use (nor install) p4, I cannot test "'git p4' works
     but 'git-p4' should not work" myself, but by running t0001 with
     a trial patch like the following, you can see that we do not
     find the dashed form `git-init` on $PATH and the test indeed
     fails.

 t/t0001-init.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 0681300707..8c598a0d84 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -31,7 +31,7 @@ check_config () {
 }
 
 test_expect_success 'plain' '
-	git init plain &&
+	git-init plain &&
 	check_config plain/.git false unset
 '
 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-08-01 15:10                     ` Junio C Hamano
@ 2018-08-01 15:54                       ` Chen Bin
  2018-08-01 17:41                         ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Chen Bin @ 2018-08-01 15:54 UTC (permalink / raw)
  To: git; +Cc: Chen Bin

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 <chenbin.sh@gmail.com>
---
 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..729cd2577 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" errs
+	)
+'
+
 test_expect_success 'submit from detached head' '
 	test_when_finished cleanup_git &&
 	git p4 clone --dest="$git" //depot &&
-- 
2.18.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] Add the `p4-pre-submit` hook
  2018-08-01 15:54                       ` Chen Bin
@ 2018-08-01 17:41                         ` Junio C Hamano
  0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2018-08-01 17:41 UTC (permalink / raw)
  To: Chen Bin; +Cc: git

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 won't start.
>
> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
> ---

I see that the only difference between this and what has been queued
on 'pu', i.e. 

  https://github.com/gitster/git/commit/fb78b040c5dc5b80a093d028d13f8cd32ade17cd

is that this is missing the update in

  https://public-inbox.org/git/xmqq1sbkfneo.fsf@gitster-ct.c.googlers.com/

and also Luke's "Reviewed-by:".

I recall that you had trouble getting "git p4" in the test (not
"git-p4") working for some reason.  Has that been resolved (iow
have you figured out why it was failing?)

Thanks.

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2018-08-01 17:41 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-23 11:27 [PATCH 1/1] add hook pre-p4-submit Chen Bin
2018-07-25  8:37 ` Luke Diamand
2018-07-25  9:14   ` Ævar Arnfjörð Bjarmason
2018-07-25 11:10     ` chen bin
2018-07-25 13:43   ` Chen Bin
2018-07-25 20:00     ` Eric Sunshine
2018-07-25 20:21       ` Junio C Hamano
2018-07-26  2:08       ` chen bin
2018-07-26  9:21         ` Eric Sunshine
2018-07-26 21:09           ` Luke Diamand
2018-07-27  1:31             ` chen bin
2018-07-26 13:41     ` Chen Bin
2018-07-26 16:49       ` Junio C Hamano
2018-07-26 21:20         ` Eric Sunshine
2018-07-27 11:22         ` [PATCH 1/1] Add the `p4-pre-submit` hook Chen Bin
2018-07-27 18:15           ` Eric Sunshine
2018-07-30 18:07           ` Junio C Hamano
2018-07-30 18:48             ` Luke Diamand
2018-07-30 21:01               ` Junio C Hamano
2018-07-31  8:46           ` SZEDER Gábor
2018-07-31 13:40             ` Luke Diamand
2018-07-31 14:40               ` Junio C Hamano
2018-07-31 19:54                 ` Luke Diamand
2018-08-01 14:57                   ` chen bin
2018-08-01 15:10                     ` Junio C Hamano
2018-08-01 15:54                       ` Chen Bin
2018-08-01 17:41                         ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.