{"thread":{"id":"48938","subject":"[PATCH 1/1] add hook pre-p4-submit","startedAt":"2018-07-23T11:27:47Z","lastAt":"2018-08-01T17:41:58Z","messageCount":27,"participants":["Chen Bin","Luke Diamand","Ævar Arnfjörð Bjarmason","chen bin","Eric Sunshine","Junio C Hamano","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"353330","messageId":"20180723112736.15088-1-chenbin.sh@gmail.com","threadId":"48938","inReplyTo":null,"subject":"[PATCH 1/1] add hook pre-p4-submit","fromName":"Chen Bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-23T11:27:36Z","receivedAt":"2018-07-23T11:27:47Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"Hook pre-p4-submit is executed before git-p4 actually submits code.\nIf the hook exits with non-zero value, submit process will abort.\n\nSigned-off-by: Chen Bin \n---\n git-p4.py | 6 ++++++\n t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/git-p4.py b/git-p4.py\nindex b449db1cc..69ee9ce41 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -2303,6 +2303,12 @@ def run(self, args):\n sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n (len(commits), num_shelves))\n \n+ # locate hook at `.git/hooks/pre-p4-submit`\n+ hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n+ # Execute hook. If it exit with non-zero value, do NOT continue.\n+ if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n+ sys.exit(1)\n+\n #\n # Apply the commits, one at a time. On failure, ask if should\n # continue to try the rest of the patches, or quit.\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 4849edc4e..48b768fa7 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n \t)\n '\n \n+# Test following scenarios:\n+# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n+# - With hook returning 0, submit should continue\n+# - With hook returning 1, submit should abort\n+test_expect_success 'run hook pre-p4-submit before submit' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\techo \"hello world\" >hello.txt &&\n+\t\tgit add hello.txt &&\n+\t\tgit commit -m \"add hello.txt\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t: >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" &&\n+\t\techo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n+\t)\n+'\n+\n test_expect_success 'submit from detached head' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.18.0\n\n"},{"id":"353554","messageId":"CAE5ih79ndEbnEeV_muQyZwG+01_8Kg0J74rZtOoK1_V40E0z7g@mail.gmail.com","threadId":"48938","inReplyTo":"20180723112736.15088-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-07-25T08:37:43Z","receivedAt":"2018-07-25T08:37:46Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 23 July 2018 at 12:27, Chen Bin wrote:\n> Hook pre-p4-submit is executed before git-p4 actually submits code.\n> If the hook exits with non-zero value, submit process will abort.\n\n\nLooks good to me - could you add some documentation please\n(Documentation/git-p4.txt).\n\nThanks!\nLuke\n\n\n>\n> Signed-off-by: Chen Bin \n> ---\n> git-p4.py | 6 ++++++\n> t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++\n> 2 files changed, 28 insertions(+)\n>\n> diff --git a/git-p4.py b/git-p4.py\n> index b449db1cc..69ee9ce41 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -2303,6 +2303,12 @@ def run(self, args):\n> sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n> (len(commits), num_shelves))\n>\n> + # locate hook at `.git/hooks/pre-p4-submit`\n> + hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n> + # Execute hook. If it exit with non-zero value, do NOT continue.\n> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n> + sys.exit(1)\n> +\n> #\n> # Apply the commits, one at a time. On failure, ask if should\n> # continue to try the rest of the patches, or quit.\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 4849edc4e..48b768fa7 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n> )\n> '\n>\n> +# Test following scenarios:\n> +# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n> +# - With hook returning 0, submit should continue\n> +# - With hook returning 1, submit should abort\n> +test_expect_success 'run hook pre-p4-submit before submit' '\n> + test_when_finished cleanup_git &&\n> + git p4 clone --dest=\"$git\" //depot &&\n> + (\n> + cd \"$git\" &&\n> + echo \"hello world\" >hello.txt &&\n> + git add hello.txt &&\n> + git commit -m \"add hello.txt\" &&\n> + git config git-p4.skipSubmitEdit true &&\n> + git p4 submit --dry-run | grep \"Would apply\" &&\n> + mkdir -p .git/hooks &&\n> + : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n> + git p4 submit --dry-run | grep \"Would apply\" &&\n> + echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n> + git p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n> + )\n> +'\n> +\n> test_expect_success 'submit from detached head' '\n> test_when_finished cleanup_git &&\n> git p4 clone --dest=\"$git\" //depot &&\n> --\n> 2.18.0\n>\n"},{"id":"353555","messageId":"87tvonbrso.fsf@evledraar.gmail.com","threadId":"48938","inReplyTo":"CAE5ih79ndEbnEeV_muQyZwG+01_8Kg0J74rZtOoK1_V40E0z7g@mail.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-25T09:14:47Z","receivedAt":"2018-07-25T09:14:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 25 2018, Luke Diamand wrote:\n\n> On 23 July 2018 at 12:27, Chen Bin wrote:\n>> Hook pre-p4-submit is executed before git-p4 actually submits code.\n>> If the hook exits with non-zero value, submit process will abort.\n>\n>\n> Looks good to me - could you add some documentation please\n> (Documentation/git-p4.txt).\n>\n> Thanks!\n> Luke\n\nThis looks correct (and you'd know better), but I was surprised that we\nwouldn't just document this in githooks(5), but looking at git-p4 I see\nthat we have its config documented there, not in git-config(1) (ditto\nsome git-svn config stuff).\n\nShouldn't we at least update githooks & git-config to say that the\nconfig / hooks documentation for these utilities can be found there?\n\n>>\n>> Signed-off-by: Chen Bin \n>> ---\n>> git-p4.py | 6 ++++++\n>> t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++\n>> 2 files changed, 28 insertions(+)\n>>\n>> diff --git a/git-p4.py b/git-p4.py\n>> index b449db1cc..69ee9ce41 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -2303,6 +2303,12 @@ def run(self, args):\n>> sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n>> (len(commits), num_shelves))\n>>\n>> + # locate hook at `.git/hooks/pre-p4-submit`\n>> + hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n>> + # Execute hook. If it exit with non-zero value, do NOT continue.\n>> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n>> + sys.exit(1)\n>> +\n>> #\n>> # Apply the commits, one at a time. On failure, ask if should\n>> # continue to try the rest of the patches, or quit.\n>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n>> index 4849edc4e..48b768fa7 100755\n>> --- a/t/t9800-git-p4-basic.sh\n>> +++ b/t/t9800-git-p4-basic.sh\n>> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n>> )\n>> '\n>>\n>> +# Test following scenarios:\n>> +# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n>> +# - With hook returning 0, submit should continue\n>> +# - With hook returning 1, submit should abort\n>> +test_expect_success 'run hook pre-p4-submit before submit' '\n>> + test_when_finished cleanup_git &&\n>> + git p4 clone --dest=\"$git\" //depot &&\n>> + (\n>> + cd \"$git\" &&\n>> + echo \"hello world\" >hello.txt &&\n>> + git add hello.txt &&\n>> + git commit -m \"add hello.txt\" &&\n>> + git config git-p4.skipSubmitEdit true &&\n>> + git p4 submit --dry-run | grep \"Would apply\" &&\n>> + mkdir -p .git/hooks &&\n>> + : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n>> + git p4 submit --dry-run | grep \"Would apply\" &&\n>> + echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n>> + git p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n>> + )\n>> +'\n>> +\n>> test_expect_success 'submit from detached head' '\n>> test_when_finished cleanup_git &&\n>> git p4 clone --dest=\"$git\" //depot &&\n>> --\n>> 2.18.0\n>>\n"},{"id":"353559","messageId":"CAAE-R+9sUUz4eYySDqc-B4RZhVZ_4NoZkvVHsc0T3RSePFsvKw@mail.gmail.com","threadId":"48938","inReplyTo":"87tvonbrso.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"chen bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-25T11:10:24Z","receivedAt":"2018-07-25T11:10:28Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"Thanks for you review. I will update documentation asap.\n\nOn Wed, Jul 25, 2018 at 7:14 PM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Wed, Jul 25 2018, Luke Diamand wrote:\n>\n>> On 23 July 2018 at 12:27, Chen Bin wrote:\n>>> Hook pre-p4-submit is executed before git-p4 actually submits code.\n>>> If the hook exits with non-zero value, submit process will abort.\n>>\n>>\n>> Looks good to me - could you add some documentation please\n>> (Documentation/git-p4.txt).\n>>\n>> Thanks!\n>> Luke\n>\n> This looks correct (and you'd know better), but I was surprised that we\n> wouldn't just document this in githooks(5), but looking at git-p4 I see\n> that we have its config documented there, not in git-config(1) (ditto\n> some git-svn config stuff).\n>\n> Shouldn't we at least update githooks & git-config to say that the\n> config / hooks documentation for these utilities can be found there?\n>\n>>>\n>>> Signed-off-by: Chen Bin \n>>> ---\n>>> git-p4.py | 6 ++++++\n>>> t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++\n>>> 2 files changed, 28 insertions(+)\n>>>\n>>> diff --git a/git-p4.py b/git-p4.py\n>>> index b449db1cc..69ee9ce41 100755\n>>> --- a/git-p4.py\n>>> +++ b/git-p4.py\n>>> @@ -2303,6 +2303,12 @@ def run(self, args):\n>>> sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n>>> (len(commits), num_shelves))\n>>>\n>>> + # locate hook at `.git/hooks/pre-p4-submit`\n>>> + hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n>>> + # Execute hook. If it exit with non-zero value, do NOT continue.\n>>> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n>>> + sys.exit(1)\n>>> +\n>>> #\n>>> # Apply the commits, one at a time. On failure, ask if should\n>>> # continue to try the rest of the patches, or quit.\n>>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n>>> index 4849edc4e..48b768fa7 100755\n>>> --- a/t/t9800-git-p4-basic.sh\n>>> +++ b/t/t9800-git-p4-basic.sh\n>>> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n>>> )\n>>> '\n>>>\n>>> +# Test following scenarios:\n>>> +# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n>>> +# - With hook returning 0, submit should continue\n>>> +# - With hook returning 1, submit should abort\n>>> +test_expect_success 'run hook pre-p4-submit before submit' '\n>>> + test_when_finished cleanup_git &&\n>>> + git p4 clone --dest=\"$git\" //depot &&\n>>> + (\n>>> + cd \"$git\" &&\n>>> + echo \"hello world\" >hello.txt &&\n>>> + git add hello.txt &&\n>>> + git commit -m \"add hello.txt\" &&\n>>> + git config git-p4.skipSubmitEdit true &&\n>>> + git p4 submit --dry-run | grep \"Would apply\" &&\n>>> + mkdir -p .git/hooks &&\n>>> + : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n>>> + git p4 submit --dry-run | grep \"Would apply\" &&\n>>> + echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n>>> + git p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n>>> + )\n>>> +'\n>>> +\n>>> test_expect_success 'submit from detached head' '\n>>> test_when_finished cleanup_git &&\n>>> git p4 clone --dest=\"$git\" //depot &&\n>>> --\n>>> 2.18.0\n>>>\n\n\n\n-- \nhelp me, help you.\n"},{"id":"353566","messageId":"20180725134345.8631-1-chenbin.sh@gmail.com","threadId":"48938","inReplyTo":"CAE5ih79ndEbnEeV_muQyZwG+01_8Kg0J74rZtOoK1_V40E0z7g@mail.gmail.com","subject":"[PATCH 1/1] add hook pre-p4-submit","fromName":"Chen Bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-25T13:43:45Z","receivedAt":"2018-07-25T13:45:10Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"Hook pre-p4-submit is executed before git-p4 actually submits code.\nIf the hook exits with non-zero value, submit process will abort.\n\nSigned-off-by: Chen Bin \n---\n Documentation/git-p4.txt | 7 +++++++\n Documentation/githooks.txt | 7 +++++++\n git-p4.py | 18 +++++++++++++++++-\n t/t9800-git-p4-basic.sh | 22 ++++++++++++++++++++++\n 4 files changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f0de3b891..73f7a2691 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -374,6 +374,13 @@ These options can be used to modify 'git p4 submit' behavior.\n been submitted. Implies --disable-rebase. Can also be set with\n git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.\n \n+Hook for submit\n+~~~~~~~~~~~~~~~\n+Hook `pre-p4-submit` is executed if it exists and is executable.\n+Exiting with non-zero status from this script cause `git-p4 submit` to abort.\n+By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n+See githooks(5) manpage for details about hooks.\n+\n Rebase options\n ~~~~~~~~~~~~~~\n These options can be used to modify 'git p4 rebase' behavior.\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex e3c283a17..5148627b4 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n hook to limit its search. On error, it will fall back to verifying\n all files and folders.\n \n+pre-p4-submit\n+~~~~~~~~~~~~~\n+\n+This hook is invoked by `git-p4 submit`. It takes no parameter, and\n+exiting with non-zero status from this script causes `git-p4 submit`\n+to abort. Run `git-p4 submit --help` for details.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/git-p4.py b/git-p4.py\nindex b449db1cc..bbb0d987e 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1494,7 +1494,12 @@ def __init__(self):\n optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n ]\n- self.description = \"Submit changes from git to the perforce depot.\"\n+ self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n+ Hook `pre-p4-submit` is executed if it exists and is executable.\n+ Exiting with non-zero status from this script cause `git-p4 submit` to abort.\n+ By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n+ See githooks(5) manpage for details about hooks.\"\"\"\n+\n self.usage += \" [name of git branch to submit into perforce depot]\"\n self.origin = \"\"\n self.detectRenames = False\n@@ -2303,6 +2308,17 @@ def run(self, args):\n sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n (len(commits), num_shelves))\n \n+ # locate hook\n+ hooks_path = gitConfig(\"core.hooksPath\")\n+ if len(hooks_path) > 0:\n+ hook_file = os.path.join(hooks_path, \"pre-p4-submit\")\n+ else:\n+ hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n+\n+ # Execute hook. If it exits with non-zero value, do NOT continue.\n+ if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n+ sys.exit(1)\n+\n #\n # Apply the commits, one at a time. On failure, ask if should\n # continue to try the rest of the patches, or quit.\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 4849edc4e..48b768fa7 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n \t)\n '\n \n+# Test following scenarios:\n+# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n+# - With hook returning 0, submit should continue\n+# - With hook returning 1, submit should abort\n+test_expect_success 'run hook pre-p4-submit before submit' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\techo \"hello world\" >hello.txt &&\n+\t\tgit add hello.txt &&\n+\t\tgit commit -m \"add hello.txt\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t: >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" &&\n+\t\techo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n+\t)\n+'\n+\n test_expect_success 'submit from detached head' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.18.0\n\n"},{"id":"353587","messageId":"CAPig+cR2gYEwOTVBMRde35rn9oVsixeerbm5iJV+FmnOiBWxqQ@mail.gmail.com","threadId":"48938","inReplyTo":"20180725134345.8631-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-25T20:00:23Z","receivedAt":"2018-07-25T20:00:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 25, 2018 at 9:45 AM Chen Bin wrote:\n> Hook pre-p4-submit is executed before git-p4 actually submits code.\n> If the hook exits with non-zero value, submit process will abort.\n\nBikeshedding: Should this be named 'p4-pre-submit'? That way, if\ngit-p4 ever grows additional hooks, they can all be grouped under the\n\"p4-\" prefix.\n\nMore below...\n\n> Signed-off-by: Chen Bin \n> ---\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> @@ -374,6 +374,13 @@ These options can be used to modify 'git p4 submit' behavior.\n> +Hook for submit\n> +~~~~~~~~~~~~~~~\n> +Hook `pre-p4-submit` is executed if it exists and is executable.\n> +Exiting with non-zero status from this script cause `git-p4 submit` to abort.\n> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n\nDoes the hook receive any arguments? Does it receive anything on its\nstandard input? Those questions ought to be answered by the\ndocumentation.\n\nAlso, how is the hook supposed to determine whether the \"submit\"\nshould proceed? How does it know what is being submitted? Perhaps the\ndocumentation could provide some explanation of how the hook should\nglean such information (especially if the hook does not itself receive\nany input).\n\n> diff --git a/git-p4.py b/git-p4.py\n> + # locate hook\n> + hooks_path = gitConfig(\"core.hooksPath\")\n> + if len(hooks_path) > 0:\n> + hook_file = os.path.join(hooks_path, \"pre-p4-submit\")\n> + else:\n> + hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n> +\n> + # Execute hook. If it exits with non-zero value, do NOT continue.\n> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n> + sys.exit(1)\n\nNit: The two #comments merely repeat what the code itself already says\nclearly enough, thus don't add value (so can be dropped).\n\nShould this be emitting some sort of message letting the user know\nthat the operation was aborted due to the hook's \"rejection\"? That\ncould be especially important if the hook itself is silent (or buggy).\n\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n> +# Test following scenarios:\n> +# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n> +# - With hook returning 0, submit should continue\n> +# - With hook returning 1, submit should abort\n\nTesting three separate cases would normally be done with three\nseparate tests, making it easier to figure out which case failed. But,\nperhaps it doesn't matter here.\n\n> +test_expect_success 'run hook pre-p4-submit before submit' '\n> + test_when_finished cleanup_git &&\n> + git p4 clone --dest=\"$git\" //depot &&\n> + (\n> + cd \"$git\" &&\n> + echo \"hello world\" >hello.txt &&\n> + git add hello.txt &&\n> + git commit -m \"add hello.txt\" &&\n> + git config git-p4.skipSubmitEdit true &&\n\nThe wholesale removal of the $git directory by cleanup_git() when the\ntest finishes is undoing this config setting. Ditto regarding the\nhooks created below. Okay.\n\n> + git p4 submit --dry-run | grep \"Would apply\" &&\n\nWe normally don't use a git command upstream in a pipe since the exit\ncode of that command gets lost (so, if it crashes, we don't know about\nit). Instead:\n\n git p4 submit --dry-run >out &&\n grep \"Would apply\" out &&\n\nAlso, if \"Would apply\" is localized, we'd use test_i18ngrep (but you\ndon't need it in this case).\n\n> + mkdir -p .git/hooks &&\n> + : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n\nIt might be clearer to readers of this test to use an explicit \"exit\n0\" here (complementing the \"exit 1\" below). The current asymmetry\nrequired extra cognitive effort to realize that this is the \"returning\n0\" case mentioned in the comment above the test.\n\n> + git p4 submit --dry-run | grep \"Would apply\" &&\n> + echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n\nUse write_script() for this, which takes care of the #!/bin/sh line\nand doing chmod:\n\n write_script >.git/hooks/pre-p4-submit <<-\\EOF &&\n exit 1\n EOF\n\n> + git p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n\nMore idiomatic in this test suite:\n\n git p4 submit --dry-run >out &&\n ! grep \"Would apply\" out\n\n> + )\n> +'\n"},{"id":"353588","messageId":"xmqqin53ukvc.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"CAPig+cR2gYEwOTVBMRde35rn9oVsixeerbm5iJV+FmnOiBWxqQ@mail.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-25T20:21:43Z","receivedAt":"2018-07-25T20:21:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Bikeshedding: Should this be named 'p4-pre-submit'? That way, if\n> git-p4 ever grows additional hooks, they can all be grouped under the\n> \"p4-\" prefix.\n\nAn excellent suggestion.\n\n> Does the hook receive any arguments? Does it receive anything on its\n> standard input? Those questions ought to be answered by the\n> documentation.\n>\n> Also, how is the hook supposed to determine whether the \"submit\"\n> should proceed? How does it know what is being submitted? Perhaps the\n> documentation could provide some explanation of how the hook should\n> glean such information (especially if the hook does not itself receive\n> any input).\n\nYes, such information in the documentation, or lack thereof, would\nmean the distinction between a useful feature and an unusable one.\n\nAll other review comments looked quite sensible and an update should\naddress all of them.\n\nThanks for being a careful reviewer, as always.\n"},{"id":"353605","messageId":"CAAE-R+_BWJAB3zsZUZ+=Zn6SiSkzrWG0EYwBF_OcX_V+2idCww@mail.gmail.com","threadId":"48938","inReplyTo":"CAPig+cR2gYEwOTVBMRde35rn9oVsixeerbm5iJV+FmnOiBWxqQ@mail.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"chen bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-26T02:08:35Z","receivedAt":"2018-07-26T02:08:40Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"Thanks for the review.\n\nThe hook does not receive any information or input from git. The\noriginal requirement\ncomes from my colleague. He want to run unit test automatically before\nsubmitting code\nto remote repository. Or else CI server will send the blame mail to the manager.\n\nThe hook actually stops the submit process from start instead of abort\nsubmit in midway.\nSo nothing is touched when hook exits with status 1.\n\nI'm not sure whether `git-p4` should print some \"hook rejection\" message.\nCurrent implementation is same as other hooks (`pre-commit`, for example).\nOnly hook itself is responsible to print error messages.\n\nPersonally I don't have opinion whether we should print out hook\nrelated message inside\n`git-p4.py`. I just try to following existing convention of git.\n\n\nWhat you guys think?\n\nRegards,\nChen\n\nOn Thu, Jul 26, 2018 at 6:00 AM, Eric Sunshine wrote:\n> On Wed, Jul 25, 2018 at 9:45 AM Chen Bin wrote:\n>> Hook pre-p4-submit is executed before git-p4 actually submits code.\n>> If the hook exits with non-zero value, submit process will abort.\n>\n> Bikeshedding: Should this be named 'p4-pre-submit'? That way, if\n> git-p4 ever grows additional hooks, they can all be grouped under the\n> \"p4-\" prefix.\n>\n> More below...\n>\n>> Signed-off-by: Chen Bin \n>> ---\n>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n>> @@ -374,6 +374,13 @@ These options can be used to modify 'git p4 submit' behavior.\n>> +Hook for submit\n>> +~~~~~~~~~~~~~~~\n>> +Hook `pre-p4-submit` is executed if it exists and is executable.\n>> +Exiting with non-zero status from this script cause `git-p4 submit` to abort.\n>> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n>\n> Does the hook receive any arguments? Does it receive anything on its\n> standard input? Those questions ought to be answered by the\n> documentation.\n>\n> Also, how is the hook supposed to determine whether the \"submit\"\n> should proceed? How does it know what is being submitted? Perhaps the\n> documentation could provide some explanation of how the hook should\n> glean such information (especially if the hook does not itself receive\n> any input).\n>\n>> diff --git a/git-p4.py b/git-p4.py\n>> + # locate hook\n>> + hooks_path = gitConfig(\"core.hooksPath\")\n>> + if len(hooks_path) > 0:\n>> + hook_file = os.path.join(hooks_path, \"pre-p4-submit\")\n>> + else:\n>> + hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"pre-p4-submit\")\n>> +\n>> + # Execute hook. If it exits with non-zero value, do NOT continue.\n>> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n>> + sys.exit(1)\n>\n> Nit: The two #comments merely repeat what the code itself already says\n> clearly enough, thus don't add value (so can be dropped).\n>\n> Should this be emitting some sort of message letting the user know\n> that the operation was aborted due to the hook's \"rejection\"? That\n> could be especially important if the hook itself is silent (or buggy).\n>\n>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n>> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n>> +# Test following scenarios:\n>> +# - Without hook \".git/hooks/pre-p4-submit\", submit should continue\n>> +# - With hook returning 0, submit should continue\n>> +# - With hook returning 1, submit should abort\n>\n> Testing three separate cases would normally be done with three\n> separate tests, making it easier to figure out which case failed. But,\n> perhaps it doesn't matter here.\n>\n>> +test_expect_success 'run hook pre-p4-submit before submit' '\n>> + test_when_finished cleanup_git &&\n>> + git p4 clone --dest=\"$git\" //depot &&\n>> + (\n>> + cd \"$git\" &&\n>> + echo \"hello world\" >hello.txt &&\n>> + git add hello.txt &&\n>> + git commit -m \"add hello.txt\" &&\n>> + git config git-p4.skipSubmitEdit true &&\n>\n> The wholesale removal of the $git directory by cleanup_git() when the\n> test finishes is undoing this config setting. Ditto regarding the\n> hooks created below. Okay.\n>\n>> + git p4 submit --dry-run | grep \"Would apply\" &&\n>\n> We normally don't use a git command upstream in a pipe since the exit\n> code of that command gets lost (so, if it crashes, we don't know about\n> it). Instead:\n>\n> git p4 submit --dry-run >out &&\n> grep \"Would apply\" out &&\n>\n> Also, if \"Would apply\" is localized, we'd use test_i18ngrep (but you\n> don't need it in this case).\n>\n>> + mkdir -p .git/hooks &&\n>> + : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&\n>\n> It might be clearer to readers of this test to use an explicit \"exit\n> 0\" here (complementing the \"exit 1\" below). The current asymmetry\n> required extra cognitive effort to realize that this is the \"returning\n> 0\" case mentioned in the comment above the test.\n>\n>> + git p4 submit --dry-run | grep \"Would apply\" &&\n>> + echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&\n>\n> Use write_script() for this, which takes care of the #!/bin/sh line\n> and doing chmod:\n>\n> write_script >.git/hooks/pre-p4-submit <<-\\EOF &&\n> exit 1\n> EOF\n>\n>> + git p4 submit --dry-run | grep \"Would apply\" || echo \"Abort submit\"\n>\n> More idiomatic in this test suite:\n>\n> git p4 submit --dry-run >out &&\n> ! grep \"Would apply\" out\n>\n>> + )\n>> +'\n\n\n\n-- \nhelp me, help you.\n"},{"id":"353623","messageId":"CAPig+cQ80DrYJjYgJEvfcpeNEscgNBvj7ydwxMMVRf6kBfpaqQ@mail.gmail.com","threadId":"48938","inReplyTo":"CAAE-R+_BWJAB3zsZUZ+=Zn6SiSkzrWG0EYwBF_OcX_V+2idCww@mail.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-26T09:21:28Z","receivedAt":"2018-07-26T09:21:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 25, 2018 at 10:08 PM chen bin wrote:\n> The hook does not receive any information or input from git. The\n> original requirement\n> comes from my colleague. He want to run unit test automatically before\n> submitting code\n> to remote repository. Or else CI server will send the blame mail to the manager.\n\nOkay, that seems in line with a hook such as pre-commit. Please do\nupdate the documentation to mention that the hook takes no arguments\nand nothing on standard input, and perhaps describe in the\ndocumentation an example use-case (as you did here).\n\nI'm not a p4 or git-p4 user, but, out of curiosity, would there be any\ninformation which could be supplied to the hook as arguments or\nstandard input (or both) which would help the hook author implement\nthe hook more easily? Perhaps such information would be fodder for a\nfuture enhancement (not necessarily needed for this patch).\n\n> The hook actually stops the submit process from start instead of abort\n> submit in midway.\n> So nothing is touched when hook exits with status 1.\n\nThis might be a good thing to add to the documentation, as well.\n\n> I'm not sure whether `git-p4` should print some \"hook rejection\" message.\n> Current implementation is same as other hooks (`pre-commit`, for example).\n> Only hook itself is responsible to print error messages.\n>\n> Personally I don't have opinion whether we should print out hook\n> related message inside\n> `git-p4.py`. I just try to following existing convention of git.\n>\n> What you guys think?\n\nFollowing existing practice makes sense. It can always be revisited\nlater if needed.\n\nThanks.\n"},{"id":"353643","messageId":"20180726134138.12183-1-chenbin.sh@gmail.com","threadId":"48938","inReplyTo":"20180725134345.8631-1-chenbin.sh@gmail.com","subject":"[PATCH 1/1] add hook pre-p4-submit","fromName":"Chen Bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-26T13:41:38Z","receivedAt":"2018-07-26T13:41:56Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"Hook pre-p4-submit is executed before git-p4 actually submits code.\nIf the hook exits with non-zero value, submit process will abort.\n\nSigned-off-by: Chen Bin \n---\n Documentation/git-p4.txt | 12 ++++++++++++\n Documentation/githooks.txt | 7 +++++++\n git-p4.py | 21 ++++++++++++++++++++-\n t/t9800-git-p4-basic.sh | 26 ++++++++++++++++++++++++++\n 4 files changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f0de3b891..f5d428ddf 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -374,6 +374,18 @@ These options can be used to modify 'git p4 submit' behavior.\n been submitted. Implies --disable-rebase. Can also be set with\n git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.\n \n+Hook for submit\n+~~~~~~~~~~~~~~~\n+Hook `p4-pre-submit` is executed if it exists and is executable.\n+The hook takes no parameter and nothing from standard input. Exiting with\n+non-zero status from this script prevents `git-p4 submit` from launching.\n+So nothing is touched when it exits with non-zero status.\n+\n+One usage scenario is to run unit tests in the hook.\n+\n+By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n+See githooks(5) manpage for details about hooks.\n+\n Rebase options\n ~~~~~~~~~~~~~~\n These options can be used to modify 'git p4 rebase' behavior.\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex e3c283a17..22fcabbe2 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n hook to limit its search. On error, it will fall back to verifying\n all files and folders.\n \n+p4-pre-submit\n+~~~~~~~~~~~~~\n+\n+This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n+from standard input. Exiting with non-zero status from this script prevent\n+`git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/git-p4.py b/git-p4.py\nindex b449db1cc..f147a2d2f 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1494,7 +1494,17 @@ def __init__(self):\n optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n ]\n- self.description = \"Submit changes from git to the perforce depot.\"\n+ self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n+ Hook `p4-pre-submit` is executed if it exists and is executable.\n+ The hook takes no parameter and nothing from standard input. Exiting with\n+ non-zero status from this script prevents `git-p4 submit` from launching.\n+ So nothing is touched when it exits with non-zero status.\n+\n+ One usage scenario is to run unit tests in the hook.\n+\n+ By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n+ See githooks(5) manpage for details about hooks.\"\"\"\n+\n self.usage += \" [name of git branch to submit into perforce depot]\"\n self.origin = \"\"\n self.detectRenames = False\n@@ -2303,6 +2313,15 @@ def run(self, args):\n sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n (len(commits), num_shelves))\n \n+ hooks_path = gitConfig(\"core.hooksPath\")\n+ if len(hooks_path) > 0:\n+ hook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n+ else:\n+ hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"p4-pre-submit\")\n+\n+ if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n+ sys.exit(1)\n+\n #\n # Apply the commits, one at a time. On failure, ask if should\n # continue to try the rest of the patches, or quit.\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 4849edc4e..8457ff617 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -261,6 +261,32 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n \t)\n '\n \n+# Test following scenarios:\n+# - Without hook \".git/hooks/p4-pre-submit\", submit should continue\n+# - With hook returning 0, submit should continue\n+# - With hook returning 1, submit should abort\n+test_expect_success 'run hook p4-pre-submit before submit' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\techo \"hello world\" >hello.txt &&\n+\t\tgit add hello.txt &&\n+\t\tgit commit -m \"add hello.txt\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit --dry-run | grep \"Would apply\" &&\n+\t\tmkdir -p .git/hooks &&\n+\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n+\t\texit 0\n+\t\tEOF\n+\t\tgit p4 submit --dry-run >out && grep \"Would apply\" out &&\n+\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n+\t\texit 1\n+\t\tEOF\n+\t\tgit p4 submit --dry-run >out && grep \"Would apply\" out || echo \"Abort submit\"\n+\t)\n+'\n+\n test_expect_success 'submit from detached head' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.18.0\n\n"},{"id":"353657","messageId":"xmqq36w6rlh7.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"20180726134138.12183-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-26T16:49:08Z","receivedAt":"2018-07-26T16:49:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chen Bin writes:\n\n> +Hook for submit\n> +~~~~~~~~~~~~~~~\n> +Hook `p4-pre-submit` is executed if it exists and is executable.\n\nJust a lang nit, but \"git grep pre-commit\" shows me lines like\n\n Documentation/git-commit.txt: This option bypasses the pre-commit and commit-msg hooks.\n\nto tell me that the above is better phrased like\n\n\tThe `p4-pre-submit` hook is executed...\n\nThe same comment applies to the patch title, the first sentence in\nthe log message and in-code \".description\" field in git-py.py.\n\n> +The hook takes no parameter and nothing from standard input. Exiting with\n> +non-zero status from this script prevents `git-p4 submit` from launching.\n> +So nothing is touched when it exits with non-zero status.\n\nIsn't the last sentence redundant, saying only what the second\nsentence already said?\n\n> +\n> +One usage scenario is to run unit tests in the hook.\n\nGood thing to mention, I guess.\n\n> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n> +See githooks(5) manpage for details about hooks.\n\nI do not think this is particularly a good change; it forces us to\nmaintain this sentence copied from githooks.txt every time the\noriginal has to change, and documentation for no other command\nduplicates it.\n\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index e3c283a17..22fcabbe2 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n> hook to limit its search. On error, it will fall back to verifying\n> all files and folders.\n> \n> +p4-pre-submit\n> +~~~~~~~~~~~~~\n> +\n> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n> +from standard input. Exiting with non-zero status from this script prevent\n> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n> +\n\nHmmm, OK.\n\n> GIT\n> ---\n> Part of the linkgit:git[1] suite\n> diff --git a/git-p4.py b/git-p4.py\n> index b449db1cc..f147a2d2f 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1494,7 +1494,17 @@ def __init__(self):\n> optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n> help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n> ]\n> - self.description = \"Submit changes from git to the perforce depot.\"\n> + self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n> + Hook `p4-pre-submit` is executed if it exists and is executable.\n> + The hook takes no parameter and nothing from standard input. Exiting with\n> + non-zero status from this script prevents `git-p4 submit` from launching.\n> + So nothing is touched when it exits with non-zero status.\n> +\n> + One usage scenario is to run unit tests in the hook.\n> +\n> + By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.\n> + See githooks(5) manpage for details about hooks.\"\"\"\n\nHow do you plan to keep this in sync with the documentation?\nPerhaps the last sentence of the new paragraph you added to\nDocumentation/githooks.txt should read\n\n\tSee `git p4 --help` for details.\n\nThen this hunk can be eliminated, I would think.\n\n> @@ -2303,6 +2313,15 @@ def run(self, args):\n> sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n> (len(commits), num_shelves))\n> \n> + hooks_path = gitConfig(\"core.hooksPath\")\n> + if len(hooks_path) > 0:\n> + hook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n> + else:\n> + hook_file = os.path.join(read_pipe(\"git rev-parse --git-dir\").strip(), \"hooks\", \"p4-pre-submit\")\n> +\n\nIsn't the GIT_DIR available as self.gitdir at this point in the\ncode, prepared by main() before this method is run?\n\nIt may be a style thing, but I somehow would find it easier to\nunderstand if you wrote it like so:\n\n\thooks_path = gitConfig(\"...\")\n\tif len(hooks_path) <= 0:\n\t\thooks_path = os.path.join(self.gitdir, \"hooks\")\n\thook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n\nThe point being hooks_path always point at the directory that holds\nhooks with or without configuration, and hook_file is set based on\nthat to the same \"p4-pre-submit\" without any potential of typo\ngetting in to make the two cases diverge.\n\n> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n> + sys.exit(1)\n> +\n> #\n> # Apply the commits, one at a time. On failure, ask if should\n> # continue to try the rest of the patches, or quit.\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 4849edc4e..8457ff617 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -261,6 +261,32 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n> \t)\n> '\n> \n> +# Test following scenarios:\n> +# - Without hook \".git/hooks/p4-pre-submit\", submit should continue\n> +# - With hook returning 0, submit should continue\n> +# - With hook returning 1, submit should abort\n> +test_expect_success 'run hook p4-pre-submit before submit' '\n> +\ttest_when_finished cleanup_git &&\n> +\tgit p4 clone --dest=\"$git\" //depot &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\techo \"hello world\" >hello.txt &&\n> +\t\tgit add hello.txt &&\n> +\t\tgit commit -m \"add hello.txt\" &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n> +\t\tgit p4 submit --dry-run | grep \"Would apply\" &&\n\n\"git p4\" on the upstream side of a pipe?\n\n> +\t\tmkdir -p .git/hooks &&\n> +\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> +\t\texit 0\n> +\t\tEOF\n> +\t\tgit p4 submit --dry-run >out && grep \"Would apply\" out &&\n\nHave each of these two on its own line, i.e.\n\n\tgit p4 submit --dry-run >out &&\n\tgrep \"Would apply\" out &&\n\n> +\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> +\t\texit 1\n> +\t\tEOF\n> +\t\tgit p4 submit --dry-run >out && grep \"Would apply\" out || echo \"Abort submit\"\n\nWhat is this last \"|| echo I always succeed\" about? \n\nDo you want to make sure \"git p4 submit\" exit with non-zero exit\nstatus *and* its output does not say \"Would apply\"? The way to\nwrite that would be\n\n\t\ttest_must_fail git p4 submit --dry-run >out &&\n\t\t! grep \"Would apply\" out\n\nThen \"git p4 submit --dry-run\" that exits with 0 even though its\npresubmit hook exits with 1 would fail to meet the expectation that\nthe command must fail and would be caught as an error. If it exits\nwith non-zero status, but still write \"Would apply\" to its standard\noutput stream, it would also be caught as an error as \"grep\" would\nsucceed.\n\n> +\t)\n> +'\n> +\n> test_expect_success 'submit from detached head' '\n> \ttest_when_finished cleanup_git &&\n> \tgit p4 clone --dest=\"$git\" //depot &&\n\n"},{"id":"353678","messageId":"CAE5ih7-DatojdaOYZSEKfgG+FeR9+CWiEcf0veHfz89W7Vhx=Q@mail.gmail.com","threadId":"48938","inReplyTo":"CAPig+cQ80DrYJjYgJEvfcpeNEscgNBvj7ydwxMMVRf6kBfpaqQ@mail.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-07-26T21:09:13Z","receivedAt":"2018-07-26T21:09:16Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 26 July 2018 at 10:21, Eric Sunshine wrote:\n> On Wed, Jul 25, 2018 at 10:08 PM chen bin wrote:\n>> The hook does not receive any information or input from git. The\n>> original requirement\n>> comes from my colleague. He want to run unit test automatically before\n>> submitting code\n>> to remote repository. Or else CI server will send the blame mail to the manager.\n>\n> Okay, that seems in line with a hook such as pre-commit. Please do\n> update the documentation to mention that the hook takes no arguments\n> and nothing on standard input, and perhaps describe in the\n> documentation an example use-case (as you did here).\n>\n> I'm not a p4 or git-p4 user, but, out of curiosity, would there be any\n> information which could be supplied to the hook as arguments or\n> standard input (or both) which would help the hook author implement\n> the hook more easily? Perhaps such information would be fodder for a\n> future enhancement (not necessarily needed for this patch).\n\n\nI tried to think of a use-case for a hook requiring any more\ninformation, but I can't think of any. You're already chdir()'d to the\nP4 shadow repo which is what you really need.\n\nAnything where you just need the commit hash (e.g. checking the commit\nmessage) can already be done with one of the existing git hooks; I\ndon't think git-p4 needs to duplicate that.\n\nAnd we can't write a commit hook that can know about the Perforce\nchangelist, because we don't know what it is yet.\n\nHowever, looking at the code, it runs the hook at the point just\n*before* the changes are applied to the P4 shadow repo. Would it make\nmore sense to run the hook *after* they have been applied (but before\nbeing P4 submitted) ?\n\nThat way you can run your tests on the checked-out P4 shadow directory\nwith your changes - as it stands, you can only run them on your git\nrepo at this point, which might not be in sync with Perforce (and\ncould be quite a long way out in fact).\n\nLuke\n\n\n>\n>> The hook actually stops the submit process from start instead of abort\n>> submit in midway.\n>> So nothing is touched when hook exits with status 1.\n>\n> This might be a good thing to add to the documentation, as well.\n>\n>> I'm not sure whether `git-p4` should print some \"hook rejection\" message.\n>> Current implementation is same as other hooks (`pre-commit`, for example).\n>> Only hook itself is responsible to print error messages.\n>>\n>> Personally I don't have opinion whether we should print out hook\n>> related message inside\n>> `git-p4.py`. I just try to following existing convention of git.\n>>\n>> What you guys think?\n>\n> Following existing practice makes sense. It can always be revisited\n> later if needed.\n>\n> Thanks.\n"},{"id":"353679","messageId":"CAPig+cQQmOpEpUNSjw58b-_+Ep3v0XA+vuqqqgHVOaFgtKt3fA@mail.gmail.com","threadId":"48938","inReplyTo":"xmqq36w6rlh7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-26T21:20:17Z","receivedAt":"2018-07-26T21:20:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jul 26, 2018 at 12:49 PM Junio C Hamano wrote:\n> Chen Bin writes:\n> > + git p4 submit --dry-run >out && grep \"Would apply\" out || echo \"Abort submit\"\n>\n> What is this last \"|| echo I always succeed\" about?\n>\n> Do you want to make sure \"git p4 submit\" exit with non-zero exit\n> status *and* its output does not say \"Would apply\"? The way to\n> write that would be\n>\n> test_must_fail git p4 submit --dry-run >out &&\n> ! grep \"Would apply\" out\n\nI missed the 'test_must_fail' when I suggested this same rewrite[1],\nwhich may explain why the suggestion wasn't used in the re-roll.\nSorry.\n\n[1]: https://public-inbox.org/git/CAPig+cR2gYEwOTVBMRde35rn9oVsixeerbm5iJV+FmnOiBWxqQ@mail.gmail.com/\n"},{"id":"353684","messageId":"CAAE-R+_04wHwtKYEYn-iQz5r1hNp0_GCg0ba7LhjSsayQWSuAQ@mail.gmail.com","threadId":"48938","inReplyTo":"CAE5ih7-DatojdaOYZSEKfgG+FeR9+CWiEcf0veHfz89W7Vhx=Q@mail.gmail.com","subject":"Re: [PATCH 1/1] add hook pre-p4-submit","fromName":"chen bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-27T01:31:58Z","receivedAt":"2018-07-27T01:32:04Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"Hi, Luke,\nRunning the hook after applying changes to P4 shadow repo might not be\na good idea.\n\nWhen the hook is running *before* the changes are applied, I'm 100% sure it's\n*only* my code's problem if the hook fails.\n\nOne reason we need this hook is sometimes developer is over confident\nwhen applying\nsome quick one line fix.\n\nThe quick fix could be required by business guy before demo to senior\nmanagers or customers.\nSo we might not want our fix being blocked by upstream commits.\n\nNot everyone is our team is perforce/git expert. Someone only use\n`git-p4 submit` and never use\n `git-p4 rebase`. If unit test fails and he could not submit code, he\nwould come to me for help. But I\nwant to reduce my workload.\n\n\nRegards,\nChen\n\n\nOn Fri, Jul 27, 2018 at 7:09 AM, Luke Diamand wrote:\n> On 26 July 2018 at 10:21, Eric Sunshine wrote:\n>> On Wed, Jul 25, 2018 at 10:08 PM chen bin wrote:\n>>> The hook does not receive any information or input from git. The\n>>> original requirement\n>>> comes from my colleague. He want to run unit test automatically before\n>>> submitting code\n>>> to remote repository. Or else CI server will send the blame mail to the manager.\n>>\n>> Okay, that seems in line with a hook such as pre-commit. Please do\n>> update the documentation to mention that the hook takes no arguments\n>> and nothing on standard input, and perhaps describe in the\n>> documentation an example use-case (as you did here).\n>>\n>> I'm not a p4 or git-p4 user, but, out of curiosity, would there be any\n>> information which could be supplied to the hook as arguments or\n>> standard input (or both) which would help the hook author implement\n>> the hook more easily? Perhaps such information would be fodder for a\n>> future enhancement (not necessarily needed for this patch).\n>\n>\n> I tried to think of a use-case for a hook requiring any more\n> information, but I can't think of any. You're already chdir()'d to the\n> P4 shadow repo which is what you really need.\n>\n> Anything where you just need the commit hash (e.g. checking the commit\n> message) can already be done with one of the existing git hooks; I\n> don't think git-p4 needs to duplicate that.\n>\n> And we can't write a commit hook that can know about the Perforce\n> changelist, because we don't know what it is yet.\n>\n> However, looking at the code, it runs the hook at the point just\n> *before* the changes are applied to the P4 shadow repo. Would it make\n> more sense to run the hook *after* they have been applied (but before\n> being P4 submitted) ?\n>\n> That way you can run your tests on the checked-out P4 shadow directory\n> with your changes - as it stands, you can only run them on your git\n> repo at this point, which might not be in sync with Perforce (and\n> could be quite a long way out in fact).\n>\n> Luke\n>\n>\n>>\n>>> The hook actually stops the submit process from start instead of abort\n>>> submit in midway.\n>>> So nothing is touched when hook exits with status 1.\n>>\n>> This might be a good thing to add to the documentation, as well.\n>>\n>>> I'm not sure whether `git-p4` should print some \"hook rejection\" message.\n>>> Current implementation is same as other hooks (`pre-commit`, for example).\n>>> Only hook itself is responsible to print error messages.\n>>>\n>>> Personally I don't have opinion whether we should print out hook\n>>> related message inside\n>>> `git-p4.py`. I just try to following existing convention of git.\n>>>\n>>> What you guys think?\n>>\n>> Following existing practice makes sense. It can always be revisited\n>> later if needed.\n>>\n>> Thanks.\n\n\n\n-- \nhelp me, help you.\n"},{"id":"353697","messageId":"20180727112222.19061-1-chenbin.sh@gmail.com","threadId":"48938","inReplyTo":"xmqq36w6rlh7.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Chen Bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-07-27T11:22:22Z","receivedAt":"2018-07-27T11:22:33Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"The `p4-pre-submit` hook is executed before git-p4 submits code.\nIf the hook exits with non-zero value, submit process not start.\n\nSigned-off-by: Chen Bin \n---\n Documentation/git-p4.txt | 8 ++++++++\n Documentation/githooks.txt | 7 +++++++\n git-p4.py | 16 +++++++++++++++-\n t/t9800-git-p4-basic.sh | 29 +++++++++++++++++++++++++++++\n 4 files changed, 59 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f0de3b891..a7aac1b92 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit' behavior.\n been submitted. Implies --disable-rebase. Can also be set with\n git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.\n \n+Hook for submit\n+~~~~~~~~~~~~~~~\n+The `p4-pre-submit` hook is executed if it exists and is executable.\n+The hook takes no parameter and nothing from standard input. Exiting with\n+non-zero status from this script prevents `git-p4 submit` from launching.\n+\n+One usage scenario is to run unit tests in the hook.\n+\n Rebase options\n ~~~~~~~~~~~~~~\n These options can be used to modify 'git p4 rebase' behavior.\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex e3c283a17..22fcabbe2 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n hook to limit its search. On error, it will fall back to verifying\n all files and folders.\n \n+p4-pre-submit\n+~~~~~~~~~~~~~\n+\n+This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n+from standard input. Exiting with non-zero status from this script prevent\n+`git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/git-p4.py b/git-p4.py\nindex b449db1cc..879abfd2b 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1494,7 +1494,13 @@ def __init__(self):\n optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n ]\n- self.description = \"Submit changes from git to the perforce depot.\"\n+ self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n+ The `p4-pre-submit` hook is executed if it exists and is executable.\n+ The hook takes no parameter and nothing from standard input. Exiting with\n+ non-zero status from this script prevents `git-p4 submit` from launching.\n+\n+ One usage scenario is to run unit tests in the hook.\"\"\"\n+\n self.usage += \" [name of git branch to submit into perforce depot]\"\n self.origin = \"\"\n self.detectRenames = False\n@@ -2303,6 +2309,14 @@ def run(self, args):\n sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n (len(commits), num_shelves))\n \n+ hooks_path = gitConfig(\"core.hooksPath\")\n+ if len(hooks_path) <= 0:\n+ hooks_path = os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), \"hooks\")\n+\n+ hook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n+ if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n+ sys.exit(1)\n+\n #\n # Apply the commits, one at a time. On failure, ask if should\n # continue to try the rest of the patches, or quit.\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 4849edc4e..2b7baa95d 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n \t)\n '\n \n+# Test following scenarios:\n+# - Without \".git/hooks/p4-pre-submit\" , submit should continue\n+# - With the hook returning 0, submit should continue\n+# - With the hook returning 1, submit should abort\n+test_expect_success 'run hook p4-pre-submit before submit' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\techo \"hello world\" >hello.txt &&\n+\t\tgit add hello.txt &&\n+\t\tgit commit -m \"add hello.txt\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit-p4 submit --dry-run >out &&\n+\t\tgrep \"Would apply\" out &&\n+\t\tmkdir -p .git/hooks &&\n+\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n+\t\texit 0\n+\t\tEOF\n+\t\tgit-p4 submit --dry-run >out &&\n+\t\tgrep \"Would apply\" out &&\n+\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n+\t\texit 1\n+\t\tEOF\n+\t\ttest_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n+\t\t! grep \"Would apply\" err\n+\t)\n+'\n+\n test_expect_success 'submit from detached head' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.18.0\n\n"},{"id":"353741","messageId":"CAPig+cSOQHV-F9GfB+73VAY-jyFdBSU93kdLa9jiRqLUvPoJ4w@mail.gmail.com","threadId":"48938","inReplyTo":"20180727112222.19061-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-27T18:15:21Z","receivedAt":"2018-07-27T18:15:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 27, 2018 at 7:22 AM Chen Bin wrote:\n> The `p4-pre-submit` hook is executed before git-p4 submits code.\n> If the hook exits with non-zero value, submit process not start.\n>\n> Signed-off-by: Chen Bin \n> ---\n> diff --git a/git-p4.py b/git-p4.py\n> @@ -1494,7 +1494,13 @@ def __init__(self):\n> - self.description = \"Submit changes from git to the perforce depot.\"\n> + self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n\nIt seemed odd to have an explicit \\n within a \"\"\"...\"\"\" string as\nopposed to simply using a blank line, but I see there is existing\nprecedence in git-p4.py when setting (at least one) self.description.\nOkay.\n\n> + The `p4-pre-submit` hook is executed if it exists and is executable.\n> + The hook takes no parameter and nothing from standard input. Exiting with\n> + non-zero status from this script prevents `git-p4 submit` from launching.\n> +\n> + One usage scenario is to run unit tests in the hook.\"\"\"\n"},{"id":"353924","messageId":"xmqqlg9sfvgr.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"20180727112222.19061-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-30T18:07:48Z","receivedAt":"2018-07-30T18:07:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chen Bin writes:\n\n> The `p4-pre-submit` hook is executed before git-p4 submits code.\n> If the hook exits with non-zero value, submit process not start.\n>\n> Signed-off-by: Chen Bin \n> ---\n\nLuke, does this version look good to you?\n\nI still think the addition to self.description is a bit too much but\nother than that the incremental since the last one looked sensible\nto my untrained eyes ;-)\n\nThanks, both.\n\n> Documentation/git-p4.txt | 8 ++++++++\n> Documentation/githooks.txt | 7 +++++++\n> git-p4.py | 16 +++++++++++++++-\n> t/t9800-git-p4-basic.sh | 29 +++++++++++++++++++++++++++++\n> 4 files changed, 59 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n> index f0de3b891..a7aac1b92 100644\n> --- a/Documentation/git-p4.txt\n> +++ b/Documentation/git-p4.txt\n> @@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit' behavior.\n> been submitted. Implies --disable-rebase. Can also be set with\n> git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.\n> \n> +Hook for submit\n> +~~~~~~~~~~~~~~~\n> +The `p4-pre-submit` hook is executed if it exists and is executable.\n> +The hook takes no parameter and nothing from standard input. Exiting with\n> +non-zero status from this script prevents `git-p4 submit` from launching.\n> +\n> +One usage scenario is to run unit tests in the hook.\n> +\n> Rebase options\n> ~~~~~~~~~~~~~~\n> These options can be used to modify 'git p4 rebase' behavior.\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index e3c283a17..22fcabbe2 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n> hook to limit its search. On error, it will fall back to verifying\n> all files and folders.\n> \n> +p4-pre-submit\n> +~~~~~~~~~~~~~\n> +\n> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n> +from standard input. Exiting with non-zero status from this script prevent\n> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n> +\n> GIT\n> ---\n> Part of the linkgit:git[1] suite\n> diff --git a/git-p4.py b/git-p4.py\n> index b449db1cc..879abfd2b 100755\n> --- a/git-p4.py\n> +++ b/git-p4.py\n> @@ -1494,7 +1494,13 @@ def __init__(self):\n> optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n> help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n> ]\n> - self.description = \"Submit changes from git to the perforce depot.\"\n> + self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n> + The `p4-pre-submit` hook is executed if it exists and is executable.\n> + The hook takes no parameter and nothing from standard input. Exiting with\n> + non-zero status from this script prevents `git-p4 submit` from launching.\n> +\n> + One usage scenario is to run unit tests in the hook.\"\"\"\n> +\n> self.usage += \" [name of git branch to submit into perforce depot]\"\n> self.origin = \"\"\n> self.detectRenames = False\n> @@ -2303,6 +2309,14 @@ def run(self, args):\n> sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n> (len(commits), num_shelves))\n> \n> + hooks_path = gitConfig(\"core.hooksPath\")\n> + if len(hooks_path) <= 0:\n> + hooks_path = os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), \"hooks\")\n> +\n> + hook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n> + sys.exit(1)\n> +\n> #\n> # Apply the commits, one at a time. On failure, ask if should\n> # continue to try the rest of the patches, or quit.\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 4849edc4e..2b7baa95d 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n> \t)\n> '\n> \n> +# Test following scenarios:\n> +# - Without \".git/hooks/p4-pre-submit\" , submit should continue\n> +# - With the hook returning 0, submit should continue\n> +# - With the hook returning 1, submit should abort\n> +test_expect_success 'run hook p4-pre-submit before submit' '\n> +\ttest_when_finished cleanup_git &&\n> +\tgit p4 clone --dest=\"$git\" //depot &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\techo \"hello world\" >hello.txt &&\n> +\t\tgit add hello.txt &&\n> +\t\tgit commit -m \"add hello.txt\" &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n> +\t\tgit-p4 submit --dry-run >out &&\n> +\t\tgrep \"Would apply\" out &&\n> +\t\tmkdir -p .git/hooks &&\n> +\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> +\t\texit 0\n> +\t\tEOF\n> +\t\tgit-p4 submit --dry-run >out &&\n> +\t\tgrep \"Would apply\" out &&\n> +\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> +\t\texit 1\n> +\t\tEOF\n> +\t\ttest_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n> +\t\t! grep \"Would apply\" err\n> +\t)\n> +'\n> +\n> test_expect_success 'submit from detached head' '\n> \ttest_when_finished cleanup_git &&\n> \tgit p4 clone --dest=\"$git\" //depot &&\n"},{"id":"353933","messageId":"CAE5ih7_-Bd6fUhTL-Yxopc-kaUq0jY9ydx_COqDdN02aXz69Sg@mail.gmail.com","threadId":"48938","inReplyTo":"xmqqlg9sfvgr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-07-30T18:48:48Z","receivedAt":"2018-07-30T18:48:52Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 30 July 2018 at 20:07, Junio C Hamano wrote:\n> Chen Bin writes:\n>\n>> The `p4-pre-submit` hook is executed before git-p4 submits code.\n>> If the hook exits with non-zero value, submit process not start.\n>>\n>> Signed-off-by: Chen Bin \n>> ---\n>\n> Luke, does this version look good to you?\n>\n\nYes, I think so, We might find in the future that we do need an\nadditional hook *after* the change has been applied, but as per Chen's\ncomment, it sounds like that's not what is needed here; it can easily\nbe added in the future if necessary.\n\nAnd there's no directly equivalent existing git-hook which could be\nused instead, so this seems like a useful addition.\n\nPossibly it should say \"it takes no parameters\" rather than \"it takes\nno parameter\"; it is confusing that in English, zero takes the plural\nrather than the singular. There's a PhD in linguistics there for\nsomeone!\n\nLuke\n\n\n> I still think the addition to self.description is a bit too much but\n> other than that the incremental since the last one looked sensible\n> to my untrained eyes ;-)\n>\n> Thanks, both.\n>\n>> Documentation/git-p4.txt | 8 ++++++++\n>> Documentation/githooks.txt | 7 +++++++\n>> git-p4.py | 16 +++++++++++++++-\n>> t/t9800-git-p4-basic.sh | 29 +++++++++++++++++++++++++++++\n>> 4 files changed, 59 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\n>> index f0de3b891..a7aac1b92 100644\n>> --- a/Documentation/git-p4.txt\n>> +++ b/Documentation/git-p4.txt\n>> @@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit' behavior.\n>> been submitted. Implies --disable-rebase. Can also be set with\n>> git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.\n>>\n>> +Hook for submit\n>> +~~~~~~~~~~~~~~~\n>> +The `p4-pre-submit` hook is executed if it exists and is executable.\n>> +The hook takes no parameter and nothing from standard input. Exiting with\n>> +non-zero status from this script prevents `git-p4 submit` from launching.\n>> +\n>> +One usage scenario is to run unit tests in the hook.\n>> +\n>> Rebase options\n>> ~~~~~~~~~~~~~~\n>> These options can be used to modify 'git p4 rebase' behavior.\n>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n>> index e3c283a17..22fcabbe2 100644\n>> --- a/Documentation/githooks.txt\n>> +++ b/Documentation/githooks.txt\n>> @@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n>> hook to limit its search. On error, it will fall back to verifying\n>> all files and folders.\n>>\n>> +p4-pre-submit\n>> +~~~~~~~~~~~~~\n>> +\n>> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n>> +from standard input. Exiting with non-zero status from this script prevent\n>> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n>> +\n>> GIT\n>> ---\n>> Part of the linkgit:git[1] suite\n>> diff --git a/git-p4.py b/git-p4.py\n>> index b449db1cc..879abfd2b 100755\n>> --- a/git-p4.py\n>> +++ b/git-p4.py\n>> @@ -1494,7 +1494,13 @@ def __init__(self):\n>> optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n>> help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n>> ]\n>> - self.description = \"Submit changes from git to the perforce depot.\"\n>> + self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n>> + The `p4-pre-submit` hook is executed if it exists and is executable.\n>> + The hook takes no parameter and nothing from standard input. Exiting with\n>> + non-zero status from this script prevents `git-p4 submit` from launching.\n>> +\n>> + One usage scenario is to run unit tests in the hook.\"\"\"\n>> +\n>> self.usage += \" [name of git branch to submit into perforce depot]\"\n>> self.origin = \"\"\n>> self.detectRenames = False\n>> @@ -2303,6 +2309,14 @@ def run(self, args):\n>> sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n>> (len(commits), num_shelves))\n>>\n>> + hooks_path = gitConfig(\"core.hooksPath\")\n>> + if len(hooks_path) <= 0:\n>> + hooks_path = os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), \"hooks\")\n>> +\n>> + hook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n>> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n>> + sys.exit(1)\n>> +\n>> #\n>> # Apply the commits, one at a time. On failure, ask if should\n>> # continue to try the rest of the patches, or quit.\n>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n>> index 4849edc4e..2b7baa95d 100755\n>> --- a/t/t9800-git-p4-basic.sh\n>> +++ b/t/t9800-git-p4-basic.sh\n>> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n>> )\n>> '\n>>\n>> +# Test following scenarios:\n>> +# - Without \".git/hooks/p4-pre-submit\" , submit should continue\n>> +# - With the hook returning 0, submit should continue\n>> +# - With the hook returning 1, submit should abort\n>> +test_expect_success 'run hook p4-pre-submit before submit' '\n>> + test_when_finished cleanup_git &&\n>> + git p4 clone --dest=\"$git\" //depot &&\n>> + (\n>> + cd \"$git\" &&\n>> + echo \"hello world\" >hello.txt &&\n>> + git add hello.txt &&\n>> + git commit -m \"add hello.txt\" &&\n>> + git config git-p4.skipSubmitEdit true &&\n>> + git-p4 submit --dry-run >out &&\n>> + grep \"Would apply\" out &&\n>> + mkdir -p .git/hooks &&\n>> + write_script .git/hooks/p4-pre-submit <<-\\EOF &&\n>> + exit 0\n>> + EOF\n>> + git-p4 submit --dry-run >out &&\n>> + grep \"Would apply\" out &&\n>> + write_script .git/hooks/p4-pre-submit <<-\\EOF &&\n>> + exit 1\n>> + EOF\n>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n>> + ! grep \"Would apply\" err\n>> + )\n>> +'\n>> +\n>> test_expect_success 'submit from detached head' '\n>> test_when_finished cleanup_git &&\n>> git p4 clone --dest=\"$git\" //depot &&\n"},{"id":"353959","messageId":"xmqq1sbkfneo.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"CAE5ih7_-Bd6fUhTL-Yxopc-kaUq0jY9ydx_COqDdN02aXz69Sg@mail.gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-30T21:01:51Z","receivedAt":"2018-07-30T21:01:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> Possibly it should say \"it takes no parameters\" rather than \"it takes\n> no parameter\"; it is confusing that in English, zero takes the plural\n> rather than the singular. There's a PhD in linguistics there for\n> someone!\n\nI count three instances. Will squash in the following while queuing\nwith your Reviewed-by.\n\nThanks.\n\n Documentation/git-p4.txt | 2 +-\n Documentation/githooks.txt | 2 +-\n git-p4.py | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex a7aac1b920..41780a5aa9 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -377,7 +377,7 @@ These options can be used to modify 'git p4 submit' behavior.\n Hook for submit\n ~~~~~~~~~~~~~~~\n The `p4-pre-submit` hook is executed if it exists and is executable.\n-The hook takes no parameter and nothing from standard input. Exiting with\n+The hook takes no parameters and nothing from standard input. Exiting with\n non-zero status from this script prevents `git-p4 submit` from launching.\n \n One usage scenario is to run unit tests in the hook.\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 22fcabbe21..959044347e 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -488,7 +488,7 @@ all files and folders.\n p4-pre-submit\n ~~~~~~~~~~~~~\n \n-This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n+This hook is invoked by `git-p4 submit`. It takes no parameters and nothing\n from standard input. Exiting with non-zero status from this script prevent\n `git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n \ndiff --git a/git-p4.py b/git-p4.py\nindex 879abfd2b1..7fab255584 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1496,7 +1496,7 @@ def __init__(self):\n ]\n self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n The `p4-pre-submit` hook is executed if it exists and is executable.\n- The hook takes no parameter and nothing from standard input. Exiting with\n+ The hook takes no parameters and nothing from standard input. Exiting with\n non-zero status from this script prevents `git-p4 submit` from launching.\n \n One usage scenario is to run unit tests in the hook.\"\"\"\n"},{"id":"354002","messageId":"20180731084638.18793-1-szeder.dev@gmail.com","threadId":"48938","inReplyTo":"20180727112222.19061-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-31T08:46:38Z","receivedAt":"2018-07-31T08:46:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 4849edc4e..2b7baa95d 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n> \t)\n> '\n> \n> +# Test following scenarios:\n> +# - Without \".git/hooks/p4-pre-submit\" , submit should continue\n> +# - With the hook returning 0, submit should continue\n> +# - With the hook returning 1, submit should abort\n> +test_expect_success 'run hook p4-pre-submit before submit' '\n> +\ttest_when_finished cleanup_git &&\n> +\tgit p4 clone --dest=\"$git\" //depot &&\n> +\t(\n> +\t\tcd \"$git\" &&\n> +\t\techo \"hello world\" >hello.txt &&\n> +\t\tgit add hello.txt &&\n> +\t\tgit commit -m \"add hello.txt\" &&\n> +\t\tgit config git-p4.skipSubmitEdit true &&\n> +\t\tgit-p4 submit --dry-run >out &&\n\nThis must be 'git p4 ...', i.e. without dash.\n\nThe dashed form causes the test to fail:\n\n <...>\n +git config git-p4.skipSubmitEdit true\n +git-p4 submit --dry-run\n t9800-git-p4-basic.sh: 12: eval: git-p4: not found\n error: last command exited with $?=127\n not ok 19 - run hook p4-pre-submit before submit\n\n\n> +\t\tgrep \"Would apply\" out &&\n> +\t\tmkdir -p .git/hooks &&\n> +\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> +\t\texit 0\n> +\t\tEOF\n> +\t\tgit-p4 submit --dry-run >out &&\n\nLikewise.\n\n> +\t\tgrep \"Would apply\" out &&\n> +\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> +\t\texit 1\n> +\t\tEOF\n> +\t\ttest_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n\nLikewise.\n\n> +\t\t! grep \"Would apply\" err\n> +\t)\n> +'\n> +\n> test_expect_success 'submit from detached head' '\n> \ttest_when_finished cleanup_git &&\n> \tgit p4 clone --dest=\"$git\" //depot &&\n> -- \n> 2.18.0\n> \n> \n"},{"id":"354033","messageId":"CAE5ih7_jh55mLVBhiu12gmc0j3mGkm4s0VU2Stx5iGf1tK6yzA@mail.gmail.com","threadId":"48938","inReplyTo":"20180731084638.18793-1-szeder.dev@gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-07-31T13:40:05Z","receivedAt":"2018-07-31T13:40:08Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"I think there is an error in the test harness.\n\nOn 31 July 2018 at 10:46, SZEDER Gábor wrote:\n>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>\n>> + ! grep \"Would apply\" err\n\nIt writes to the file \"errs\" but then looks for the message in \"err\".\n\nLuke\n"},{"id":"354037","messageId":"xmqqh8kfeae3.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"CAE5ih7_jh55mLVBhiu12gmc0j3mGkm4s0VU2Stx5iGf1tK6yzA@mail.gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T14:40:36Z","receivedAt":"2018-07-31T14:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Luke Diamand writes:\n\n> I think there is an error in the test harness.\n>\n> On 31 July 2018 at 10:46, SZEDER Gábor wrote:\n>>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>\n>>> + ! grep \"Would apply\" err\n>\n> It writes to the file \"errs\" but then looks for the message in \"err\".\n>\n> Luke\n\nSigh. Thanks for spotting, both of you.\n\nHere is what I'd locally squash in. If there is anything else, I'd\nrather see a refreshed final one sent by the author.\n\nThanks.\n\n t/t9800-git-p4-basic.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 2b7baa95d2..729cd25770 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before submit' '\n \t\tgit add hello.txt &&\n \t\tgit commit -m \"add hello.txt\" &&\n \t\tgit config git-p4.skipSubmitEdit true &&\n-\t\tgit-p4 submit --dry-run >out &&\n+\t\tgit p4 submit --dry-run >out &&\n \t\tgrep \"Would apply\" out &&\n \t\tmkdir -p .git/hooks &&\n \t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n \t\texit 0\n \t\tEOF\n-\t\tgit-p4 submit --dry-run >out &&\n+\t\tgit p4 submit --dry-run >out &&\n \t\tgrep \"Would apply\" out &&\n \t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n \t\texit 1\n \t\tEOF\n-\t\ttest_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n-\t\t! grep \"Would apply\" err\n+\t\ttest_must_fail git p4 submit --dry-run >errs 2>&1 &&\n+\t\t! grep \"Would apply\" errs\n \t)\n '\n \n"},{"id":"354112","messageId":"CAE5ih7-pLAW8xrY_F2z6gDxaMuA4w+nuUQHeLvGdSyn0GXebWA@mail.gmail.com","threadId":"48938","inReplyTo":"xmqqh8kfeae3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Luke Diamand","fromEmail":"luke@diamand.org","sentAt":"2018-07-31T19:54:05Z","receivedAt":"2018-07-31T19:54:08Z","isPatch":true,"sender":{"key":"luke@diamand.org","avatar":"https://avatars.githubusercontent.com/u/5330967?v=4"},"body":"On 31 July 2018 at 16:40, Junio C Hamano wrote:\n> Luke Diamand writes:\n>\n>> I think there is an error in the test harness.\n>>\n>> On 31 July 2018 at 10:46, SZEDER Gábor wrote:\n>>>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>\n>>>> + ! grep \"Would apply\" err\n>>\n>> It writes to the file \"errs\" but then looks for the message in \"err\".\n>>\n>> Luke\n>\n> Sigh. Thanks for spotting, both of you.\n>\n> Here is what I'd locally squash in. If there is anything else, I'd\n> rather see a refreshed final one sent by the author.\n>\n> Thanks.\n>\n> t/t9800-git-p4-basic.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n> index 2b7baa95d2..729cd25770 100755\n> --- a/t/t9800-git-p4-basic.sh\n> +++ b/t/t9800-git-p4-basic.sh\n> @@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before submit' '\n> git add hello.txt &&\n> git commit -m \"add hello.txt\" &&\n> git config git-p4.skipSubmitEdit true &&\n> - git-p4 submit --dry-run >out &&\n> + git p4 submit --dry-run >out &&\n> grep \"Would apply\" out &&\n> mkdir -p .git/hooks &&\n> write_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> exit 0\n> EOF\n> - git-p4 submit --dry-run >out &&\n> + git p4 submit --dry-run >out &&\n> grep \"Would apply\" out &&\n> write_script .git/hooks/p4-pre-submit <<-\\EOF &&\n> exit 1\n> EOF\n> - test_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n> - ! grep \"Would apply\" err\n> + test_must_fail git p4 submit --dry-run >errs 2>&1 &&\n> + ! grep \"Would apply\" errs\n> )\n> '\n\nThat set of deltas works for me.\n\nSorry, I should have run the tests myself earlier and I would have\npicked up on this.\n"},{"id":"354154","messageId":"CAAE-R+8UA5S3Y4iWw=njtVVn8cu+7s8E7L9fS6UY=Q_a5-JwwQ@mail.gmail.com","threadId":"48938","inReplyTo":"CAE5ih7-pLAW8xrY_F2z6gDxaMuA4w+nuUQHeLvGdSyn0GXebWA@mail.gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"chen bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-08-01T14:57:19Z","receivedAt":"2018-08-01T14:57:23Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"I updated the patch. But for some reason the test keep failing at this line,\n`test_must_fail git p4 submit --dry-run >errs 2>&1 &&`.\n\nIf I change this line to `test_must_fail git-p4 submit --dry-run >errs\n2>&1 &&` the test will pass.\n\nCould you help me to resolve this issue? I'm really confused. I've\nalready added latest `p4d` and `p4` to $PATH.\n\nThe commit is at\nhttps://github.com/redguardtoo/git/commit/b88c38b9ce6cfb1c1fefef15527adfa92d78daf2\n\n\nOn Wed, Aug 1, 2018 at 5:54 AM, Luke Diamand wrote:\n> On 31 July 2018 at 16:40, Junio C Hamano wrote:\n>> Luke Diamand writes:\n>>\n>>> I think there is an error in the test harness.\n>>>\n>>> On 31 July 2018 at 10:46, SZEDER Gábor wrote:\n>>>>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>\n>>>>> + ! grep \"Would apply\" err\n>>>\n>>> It writes to the file \"errs\" but then looks for the message in \"err\".\n>>>\n>>> Luke\n>>\n>> Sigh. Thanks for spotting, both of you.\n>>\n>> Here is what I'd locally squash in. If there is anything else, I'd\n>> rather see a refreshed final one sent by the author.\n>>\n>> Thanks.\n>>\n>> t/t9800-git-p4-basic.sh | 8 ++++----\n>> 1 file changed, 4 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\n>> index 2b7baa95d2..729cd25770 100755\n>> --- a/t/t9800-git-p4-basic.sh\n>> +++ b/t/t9800-git-p4-basic.sh\n>> @@ -274,19 +274,19 @@ test_expect_success 'run hook p4-pre-submit before submit' '\n>> git add hello.txt &&\n>> git commit -m \"add hello.txt\" &&\n>> git config git-p4.skipSubmitEdit true &&\n>> - git-p4 submit --dry-run >out &&\n>> + git p4 submit --dry-run >out &&\n>> grep \"Would apply\" out &&\n>> mkdir -p .git/hooks &&\n>> write_script .git/hooks/p4-pre-submit <<-\\EOF &&\n>> exit 0\n>> EOF\n>> - git-p4 submit --dry-run >out &&\n>> + git p4 submit --dry-run >out &&\n>> grep \"Would apply\" out &&\n>> write_script .git/hooks/p4-pre-submit <<-\\EOF &&\n>> exit 1\n>> EOF\n>> - test_must_fail git-p4 submit --dry-run >errs 2>&1 &&\n>> - ! grep \"Would apply\" err\n>> + test_must_fail git p4 submit --dry-run >errs 2>&1 &&\n>> + ! grep \"Would apply\" errs\n>> )\n>> '\n>\n> That set of deltas works for me.\n>\n> Sorry, I should have run the tests myself earlier and I would have\n> picked up on this.\n\n\n\n-- \nhelp me, help you.\n"},{"id":"354155","messageId":"xmqqmuu686mp.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"CAAE-R+8UA5S3Y4iWw=njtVVn8cu+7s8E7L9fS6UY=Q_a5-JwwQ@mail.gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T15:10:38Z","receivedAt":"2018-08-01T15:10:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"chen bin writes:\n\n> I updated the patch. But for some reason the test keep failing at this line,\n> `test_must_fail git p4 submit --dry-run >errs 2>&1 &&`.\n>\n> If I change this line to `test_must_fail git-p4 submit --dry-run >errs\n> 2>&1 &&` the test will pass.\n\nHmph. I somehow suspect that the test also will pass if you changed\nit like this:\n\n\ttest_must_fail false >errs 2>&1 &&\n\nIOW, my suspicion is that the shell fails to find \"git-p4\" [*1*] and\nthat is why your `test_must_fail git-p4 whatever` lets your test\npass, which is different from the way you _want_ your test_must_fail\nsucceed, i.e. \"git p4 submit\" is run with the \"--dry-run\" option and\nexit with non-zero status.\n\nOf course, if the shell cannot find \"git-p4\", `errs` would not have\nthe string \"Would apply\" in it, so the next test also would pass.\n\n>>>> On 31 July 2018 at 10:46, SZEDER Gábor wrote:\n>>>>>> + test_must_fail git-p4 submit --dry-run >errs 2>&1 &&>\n>>>>>> + ! grep \"Would apply\" err\n\n\n\n[Footnote]\n\n *1* As I do not use (nor install) p4, I cannot test \"'git p4' works\n but 'git-p4' should not work\" myself, but by running t0001 with\n a trial patch like the following, you can see that we do not\n find the dashed form `git-init` on $PATH and the test indeed\n fails.\n\n t/t0001-init.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0001-init.sh b/t/t0001-init.sh\nindex 0681300707..8c598a0d84 100755\n--- a/t/t0001-init.sh\n+++ b/t/t0001-init.sh\n@@ -31,7 +31,7 @@ check_config () {\n }\n \n test_expect_success 'plain' '\n-\tgit init plain &&\n+\tgit-init plain &&\n \tcheck_config plain/.git false unset\n '\n \n"},{"id":"354166","messageId":"20180801155432.4041-1-chenbin.sh@gmail.com","threadId":"48938","inReplyTo":"xmqqmuu686mp.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Chen Bin","fromEmail":"chenbin.sh@gmail.com","sentAt":"2018-08-01T15:54:32Z","receivedAt":"2018-08-01T15:54:49Z","isPatch":true,"sender":{"key":"chenbin.sh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/184553?v=4"},"body":"The `p4-pre-submit` hook is executed before git-p4 submits code.\nIf the hook exits with non-zero value, submit process won't start.\n\nSigned-off-by: Chen Bin \n---\n Documentation/git-p4.txt | 8 ++++++++\n Documentation/githooks.txt | 7 +++++++\n git-p4.py | 16 +++++++++++++++-\n t/t9800-git-p4-basic.sh | 29 +++++++++++++++++++++++++++++\n 4 files changed, 59 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt\nindex f0de3b891..a7aac1b92 100644\n--- a/Documentation/git-p4.txt\n+++ b/Documentation/git-p4.txt\n@@ -374,6 +374,14 @@ These options can be used to modify 'git p4 submit' behavior.\n been submitted. Implies --disable-rebase. Can also be set with\n git-p4.disableP4Sync. Sync with origin/master still goes ahead if possible.\n \n+Hook for submit\n+~~~~~~~~~~~~~~~\n+The `p4-pre-submit` hook is executed if it exists and is executable.\n+The hook takes no parameter and nothing from standard input. Exiting with\n+non-zero status from this script prevents `git-p4 submit` from launching.\n+\n+One usage scenario is to run unit tests in the hook.\n+\n Rebase options\n ~~~~~~~~~~~~~~\n These options can be used to modify 'git p4 rebase' behavior.\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex e3c283a17..22fcabbe2 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the\n hook to limit its search. On error, it will fall back to verifying\n all files and folders.\n \n+p4-pre-submit\n+~~~~~~~~~~~~~\n+\n+This hook is invoked by `git-p4 submit`. It takes no parameter and nothing\n+from standard input. Exiting with non-zero status from this script prevent\n+`git-p4 submit` from launching. Run `git-p4 submit --help` for details.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/git-p4.py b/git-p4.py\nindex b449db1cc..879abfd2b 100755\n--- a/git-p4.py\n+++ b/git-p4.py\n@@ -1494,7 +1494,13 @@ def __init__(self):\n optparse.make_option(\"--disable-p4sync\", dest=\"disable_p4sync\", action=\"store_true\",\n help=\"Skip Perforce sync of p4/master after submit or shelve\"),\n ]\n- self.description = \"Submit changes from git to the perforce depot.\"\n+ self.description = \"\"\"Submit changes from git to the perforce depot.\\n\n+ The `p4-pre-submit` hook is executed if it exists and is executable.\n+ The hook takes no parameter and nothing from standard input. Exiting with\n+ non-zero status from this script prevents `git-p4 submit` from launching.\n+\n+ One usage scenario is to run unit tests in the hook.\"\"\"\n+\n self.usage += \" [name of git branch to submit into perforce depot]\"\n self.origin = \"\"\n self.detectRenames = False\n@@ -2303,6 +2309,14 @@ def run(self, args):\n sys.exit(\"number of commits (%d) must match number of shelved changelist (%d)\" %\n (len(commits), num_shelves))\n \n+ hooks_path = gitConfig(\"core.hooksPath\")\n+ if len(hooks_path) <= 0:\n+ hooks_path = os.path.join(os.environ.get(\"GIT_DIR\", \".git\"), \"hooks\")\n+\n+ hook_file = os.path.join(hooks_path, \"p4-pre-submit\")\n+ if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:\n+ sys.exit(1)\n+\n #\n # Apply the commits, one at a time. On failure, ask if should\n # continue to try the rest of the patches, or quit.\ndiff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh\nindex 4849edc4e..729cd2577 100755\n--- a/t/t9800-git-p4-basic.sh\n+++ b/t/t9800-git-p4-basic.sh\n@@ -261,6 +261,35 @@ test_expect_success 'unresolvable host in P4PORT should display error' '\n \t)\n '\n \n+# Test following scenarios:\n+# - Without \".git/hooks/p4-pre-submit\" , submit should continue\n+# - With the hook returning 0, submit should continue\n+# - With the hook returning 1, submit should abort\n+test_expect_success 'run hook p4-pre-submit before submit' '\n+\ttest_when_finished cleanup_git &&\n+\tgit p4 clone --dest=\"$git\" //depot &&\n+\t(\n+\t\tcd \"$git\" &&\n+\t\techo \"hello world\" >hello.txt &&\n+\t\tgit add hello.txt &&\n+\t\tgit commit -m \"add hello.txt\" &&\n+\t\tgit config git-p4.skipSubmitEdit true &&\n+\t\tgit p4 submit --dry-run >out &&\n+\t\tgrep \"Would apply\" out &&\n+\t\tmkdir -p .git/hooks &&\n+\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n+\t\texit 0\n+\t\tEOF\n+\t\tgit p4 submit --dry-run >out &&\n+\t\tgrep \"Would apply\" out &&\n+\t\twrite_script .git/hooks/p4-pre-submit <<-\\EOF &&\n+\t\texit 1\n+\t\tEOF\n+\t\ttest_must_fail git p4 submit --dry-run >errs 2>&1 &&\n+\t\t! grep \"Would apply\" errs\n+\t)\n+'\n+\n test_expect_success 'submit from detached head' '\n \ttest_when_finished cleanup_git &&\n \tgit p4 clone --dest=\"$git\" //depot &&\n-- \n2.18.0\n\n"},{"id":"354172","messageId":"xmqqzhy66l25.fsf@gitster-ct.c.googlers.com","threadId":"48938","inReplyTo":"20180801155432.4041-1-chenbin.sh@gmail.com","subject":"Re: [PATCH 1/1] Add the `p4-pre-submit` hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T17:41:54Z","receivedAt":"2018-08-01T17:41:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chen Bin writes:\n\n> The `p4-pre-submit` hook is executed before git-p4 submits code.\n> If the hook exits with non-zero value, submit process won't start.\n>\n> Signed-off-by: Chen Bin \n> ---\n\nI see that the only difference between this and what has been queued\non 'pu', i.e. \n\n https://github.com/gitster/git/commit/fb78b040c5dc5b80a093d028d13f8cd32ade17cd\n\nis that this is missing the update in\n\n https://public-inbox.org/git/xmqq1sbkfneo.fsf@gitster-ct.c.googlers.com/\n\nand also Luke's \"Reviewed-by:\".\n\nI recall that you had trouble getting \"git p4\" in the test (not\n\"git-p4\") working for some reason. Has that been resolved (iow\nhave you figured out why it was failing?)\n\nThanks.\n"}]}