Re: [PATCH 1/1] add hook pre-p4-submit
Chen Bin <chenbin.sh@gmail.com> writes:
> +Hook for submit
> +~~~~~~~~~~~~~~~
> +Hook `p4-pre-submit` is executed if it exists and is executable.
Just a lang nit, but "git grep pre-commit" shows me lines like
Documentation/git-commit.txt: This option bypasses the pre-commit and commit-msg hooks.
to tell me that the above is better phrased like
The `p4-pre-submit` hook is executed...
The same comment applies to the patch title, the first sentence in the log message and in-code ".description" field in git-py.py.
> +The hook takes no parameter and nothing from standard input. Exiting with
> +non-zero status from this script prevents `git-p4 submit` from launching.
> +So nothing is touched when it exits with non-zero status.
Isn't the last sentence redundant, saying only what the second sentence already said?
> +
> +One usage scenario is to run unit tests in the hook.
Good thing to mention, I guess.
> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
> +See githooks(5) manpage for details about hooks.
I do not think this is particularly a good change; it forces us to maintain this sentence copied from githooks.txt every time the original has to change, and documentation for no other command duplicates it.
Show 15 quoted lines
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index e3c283a17..22fcabbe2 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -485,6 +485,13 @@ The exit status determines whether git will use the data from the
> hook to limit its search. On error, it will fall back to verifying
> all files and folders.
>
> +p4-pre-submit
> +~~~~~~~~~~~~~
> +
> +This hook is invoked by `git-p4 submit`. It takes no parameter and nothing
> +from standard input. Exiting with non-zero status from this script prevent
> +`git-p4 submit` from launching. Run `git-p4 submit --help` for details.
> +
Show 22 quoted lines
> GIT
> ---
> Part of the linkgit:git[1] suite
> diff --git a/git-p4.py b/git-p4.py
> index b449db1cc..f147a2d2f 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1494,7 +1494,17 @@ def __init__(self):
> optparse.make_option("--disable-p4sync", dest="disable_p4sync", action="store_true",
> help="Skip Perforce sync of p4/master after submit or shelve"),
> ]
> - self.description = "Submit changes from git to the perforce depot."
> + self.description = """Submit changes from git to the perforce depot.\n
> + Hook `p4-pre-submit` is executed if it exists and is executable.
> + The hook takes no parameter and nothing from standard input. Exiting with
> + non-zero status from this script prevents `git-p4 submit` from launching.
> + So nothing is touched when it exits with non-zero status.
> +
> + One usage scenario is to run unit tests in the hook.
> +
> + By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.
> + See githooks(5) manpage for details about hooks."""How do you plan to keep this in sync with the documentation? Perhaps the last sentence of the new paragraph you added to Documentation/githooks.txt should read
See `git p4 --help` for details.
Then this hunk can be eliminated, I would think.
Show 10 quoted lines
> @@ -2303,6 +2313,15 @@ def run(self, args):
> sys.exit("number of commits (%d) must match number of shelved changelist (%d)" %
> (len(commits), num_shelves))
>
> + hooks_path = gitConfig("core.hooksPath")
> + if len(hooks_path) > 0:
> + hook_file = os.path.join(hooks_path, "p4-pre-submit")
> + else:
> + hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "p4-pre-submit")
> +Isn't the GIT_DIR available as self.gitdir at this point in the code, prepared by main() before this method is run?
It may be a style thing, but I somehow would find it easier to understand if you wrote it like so:
hooks_path = gitConfig("...")
if len(hooks_path) <= 0:
hooks_path = os.path.join(self.gitdir, "hooks")
hook_file = os.path.join(hooks_path, "p4-pre-submit")The point being hooks_path always point at the directory that holds hooks with or without configuration, and hook_file is set based on that to the same "p4-pre-submit" without any potential of typo getting in to make the two cases diverge.
Show 28 quoted lines
> + if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
> + sys.exit(1)
> +
> #
> # Apply the commits, one at a time. On failure, ask if should
> # continue to try the rest of the patches, or quit.
> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> index 4849edc4e..8457ff617 100755
> --- a/t/t9800-git-p4-basic.sh
> +++ b/t/t9800-git-p4-basic.sh
> @@ -261,6 +261,32 @@ test_expect_success 'unresolvable host in P4PORT should display error' '
> )
> '
>
> +# Test following scenarios:
> +# - Without hook ".git/hooks/p4-pre-submit", submit should continue
> +# - With hook returning 0, submit should continue
> +# - With hook returning 1, submit should abort
> +test_expect_success 'run hook p4-pre-submit before submit' '
> + test_when_finished cleanup_git &&
> + git p4 clone --dest="$git" //depot &&
> + (
> + cd "$git" &&
> + echo "hello world" >hello.txt &&
> + git add hello.txt &&
> + git commit -m "add hello.txt" &&
> + git config git-p4.skipSubmitEdit true &&
> + git p4 submit --dry-run | grep "Would apply" &&
"git p4" on the upstream side of a pipe?
Show 5 quoted lines
> + mkdir -p .git/hooks &&
> + write_script .git/hooks/p4-pre-submit <<-\EOF &&
> + exit 0
> + EOF
> + git p4 submit --dry-run >out && grep "Would apply" out &&
Have each of these two on its own line, i.e.
git p4 submit --dry-run >out &&
grep "Would apply" out &&
> + write_script .git/hooks/p4-pre-submit <<-\EOF &&
> + exit 1
> + EOF
> + git p4 submit --dry-run >out && grep "Would apply" out || echo "Abort submit"
What is this last "|| echo I always succeed" about?
Do you want to make sure "git p4 submit" exit with non-zero exit status *and* its output does not say "Would apply"? The way to write that would be
test_must_fail git p4 submit --dry-run >out &&
! grep "Would apply" out
Then "git p4 submit --dry-run" that exits with 0 even though its presubmit hook exits with 1 would fail to meet the expectation that the command must fail and would be caught as an error. If it exits with non-zero status, but still write "Would apply" to its standard output stream, it would also be caught as an error as "grep" would succeed.
Show 6 quoted lines
> + )
> +'
> +
> test_expect_success 'submit from detached head' '
> test_when_finished cleanup_git &&
> git p4 clone --dest="$git" //depot &&
Chen Bin
