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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Nov 16, 2012, 20:25 UTC
Message-ID: <vpq625547ne.fsf@grenoble-inp.fr>
In-Reply-To: <CAJwKrPYwCE4ExmK09PURMfjYezn6vdCH_BBXU4WCwrnotyV9CA@mail.gmail.com>

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.

Show 7 quoted lines

> +--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.

Show 17 quoted lines

> +# 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.

Show 11 quoted lines

> +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 ?

Show 9 quoted lines

> +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.

Show 19 quoted lines

> +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/