Thanks for the review.
The hook does not receive any information or input from git. The
original requirement
comes from my colleague. He want to run unit test automatically before
submitting code
to remote repository. Or else CI server will send the blame mail to the manager.
The hook actually stops the submit process from start instead of abort
submit in midway.
So nothing is touched when hook exits with status 1.
I'm not sure whether `git-p4` should print some "hook rejection" message.
Current implementation is same as other hooks (`pre-commit`, for example).
Only hook itself is responsible to print error messages.
Personally I don't have opinion whether we should print out hook
related message inside
`git-p4.py`. I just try to following existing convention of git.
What you guys think?
Regards,
Chen
On Thu, Jul 26, 2018 at 6:00 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Jul 25, 2018 at 9:45 AM Chen Bin <chenbin.sh@xxxxxxxxx> wrote:
>> Hook pre-p4-submit is executed before git-p4 actually submits code.
>> If the hook exits with non-zero value, submit process will abort.
>
> Bikeshedding: Should this be named 'p4-pre-submit'? That way, if
> git-p4 ever grows additional hooks, they can all be grouped under the
> "p4-" prefix.
>
> More below...
>
>> Signed-off-by: Chen Bin <chenbin.sh@xxxxxxxxx>
>> ---
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> @@ -374,6 +374,13 @@ These options can be used to modify 'git p4 submit' behavior.
>> +Hook for submit
>> +~~~~~~~~~~~~~~~
>> +Hook `pre-p4-submit` is executed if it exists and is executable.
>> +Exiting with non-zero status from this script cause `git-p4 submit` to abort.
>> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
>
> Does the hook receive any arguments? Does it receive anything on its
> standard input? Those questions ought to be answered by the
> documentation.
>
> Also, how is the hook supposed to determine whether the "submit"
> should proceed? How does it know what is being submitted? Perhaps the
> documentation could provide some explanation of how the hook should
> glean such information (especially if the hook does not itself receive
> any input).
>
>> diff --git a/git-p4.py b/git-p4.py
>> + # locate hook
>> + hooks_path = gitConfig("core.hooksPath")
>> + if len(hooks_path) > 0:
>> + hook_file = os.path.join(hooks_path, "pre-p4-submit")
>> + else:
>> + hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "pre-p4-submit")
>> +
>> + # Execute hook. If it exits with non-zero value, do NOT continue.
>> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
>> + sys.exit(1)
>
> Nit: The two #comments merely repeat what the code itself already says
> clearly enough, thus don't add value (so can be dropped).
>
> Should this be emitting some sort of message letting the user know
> that the operation was aborted due to the hook's "rejection"? That
> could be especially important if the hook itself is silent (or buggy).
>
>> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
>> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '
>> +# Test following scenarios:
>> +# - Without hook ".git/hooks/pre-p4-submit", submit should continue
>> +# - With hook returning 0, submit should continue
>> +# - With hook returning 1, submit should abort
>
> Testing three separate cases would normally be done with three
> separate tests, making it easier to figure out which case failed. But,
> perhaps it doesn't matter here.
>
>> +test_expect_success 'run hook pre-p4-submit before submit' '
>> + test_when_finished cleanup_git &&
>> + git p4 clone --dest="$git" //depot &&
>> + (
>> + cd "$git" &&
>> + echo "hello world" >hello.txt &&
>> + git add hello.txt &&
>> + git commit -m "add hello.txt" &&
>> + git config git-p4.skipSubmitEdit true &&
>
> The wholesale removal of the $git directory by cleanup_git() when the
> test finishes is undoing this config setting. Ditto regarding the
> hooks created below. Okay.
>
>> + git p4 submit --dry-run | grep "Would apply" &&
>
> We normally don't use a git command upstream in a pipe since the exit
> code of that command gets lost (so, if it crashes, we don't know about
> it). Instead:
>
> git p4 submit --dry-run >out &&
> grep "Would apply" out &&
>
> Also, if "Would apply" is localized, we'd use test_i18ngrep (but you
> don't need it in this case).
>
>> + mkdir -p .git/hooks &&
>> + : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&
>
> It might be clearer to readers of this test to use an explicit "exit
> 0" here (complementing the "exit 1" below). The current asymmetry
> required extra cognitive effort to realize that this is the "returning
> 0" case mentioned in the comment above the test.
>
>> + git p4 submit --dry-run | grep "Would apply" &&
>> + echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&
>
> Use write_script() for this, which takes care of the #!/bin/sh line
> and doing chmod:
>
> write_script >.git/hooks/pre-p4-submit <<-\EOF &&
> exit 1
> EOF
>
>> + git p4 submit --dry-run | grep "Would apply" || echo "Abort submit"
>
> More idiomatic in this test suite:
>
> git p4 submit --dry-run >out &&
> ! grep "Would apply" out
>
>> + )
>> +'
--
help me, help you.
