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

On 26 July 2018 at 10:21, Eric Sunshine [off-list ref] wrote:

On Wed, Jul 25, 2018 at 10:08 PM chen bin [off-list ref] wrote:

quoted

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

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

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

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

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

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

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

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

Luke

quoted

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

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

quoted

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

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

What you guys think?

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

Thanks.