Re: [PATCH] Add support for a 'pre-push' hook

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Aske Olsson <askeolsson@gmail.com> writes:

> If the script .git/hooks/pre-push exists and is executable it will be
> called before a `git push` command, and when the script exits with a
> non-zero status the push will be aborted.

That sounds like a sane thing to do.

>  Documentation/git-push.txt |  11 +++-
>  Documentation/githooks.txt |  12 +++++
>  builtin/push.c             |   6 +++
>  t/t5542-pre-push-hook.sh   | 132 +++++++++++++++++++++++++++++++++++++++++++++

It would be nice to provide a sample hook in the default template. See
the template/ directory in Git's source code.

> +--no-verify::
> + This option bypasses the pre-push hook.
> + See also linkgit:githooks[5].
> +
>  -q::
>  --quiet::
>   Suppress all output, including the listing of updated refs,

Here, and below: you seem to have whitespace damage. Somebody replaced
tabs with spaces I guess. Using git send-email helps avoiding this.

> +D=`pwd`

Unused variable, left from previous hacking I guess.

> +# hook that always succeeds
> +mk_hook_exec () {
> +cat > "$HOOK" <<EOF
> +#!/bin/sh
> +exit 0
> +EOF
> +chmod +x "$HOOK"
> +}
> +
> +# hook that fails
> +mk_hook_fail () {
> +cat > "$HOOK" <<EOF
> +#!/bin/sh
> +exit 1
> +EOF
> +chmod +x "$HOOK"
> +}

I'd add a "touch hook-ran" in the script, a "rm -f hook-ran" before
launching git-push, and test the file existance after the hook to make
sure it was ran.

> +test_expect_success 'push with no pre-push hook' '
> + mk_repo_pair &&
> + (
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + git push --mirror up
> + )
> +'
> +
> +test_expect_success 'push --no-verify with no pre-push hook' '
> + mk_repo_pair &&

I don't think you need to re-create the repos for each tests. Tests are
good, but they're better when they're fast so avoiding useless
operations is good.

We try to write tests so that one test failure does not imply failures
of the next tests (i.e. stopping in the middle should not not leave
garbage that would prevent the next test from running), but do not
necessarily write 100% self-contained tests.

> + echo one >foo && git add foo && git commit -m one &&

test_commit ?

> +test_expect_success 'push with failing pre-push hook' '
> + mk_repo_pair &&
> + (
> + mk_hook_fail &&
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + test_must_fail git push --mirror up
> + )
> +'

It would be cool to actually check that the push was not performed
(verify that the target repo is still pointing to the old revision).
Otherwise, an implementation that would call run_hook after pushing
would pass the tests.

> +test_expect_success 'push with non-executable pre-push hook' '
> + mk_repo_pair &&
> + (
> + mk_hook_no_exec &&
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + git push --mirror up
> + )
> +'
> +
> +test_expect_success 'push --no-verify with non-executable pre-push hook' '
> + mk_repo_pair &&
> + (
> + mk_hook_no_exec &&
> + cd master &&
> + echo one >foo && git add foo && git commit -m one &&
> + git push --no-verify --mirror up
> + )
> +'

These two tests are not testing much. The hook is not executable, so it
shouldn't be ran, but you do not check whether the hook is ran or not.
At least make the "exit 0" an "exit 1" in the hook.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/