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

On Fri, Nov 16, 2012 at 9:25 PM, Matthieu Moy
[off-list ref] wrote:

Aske Olsson [off-list ref] writes:

quoted

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

I had some firewall problems at work, so ended up sending from gmail.
Will fix ;)

quoted

+D=`pwd`

Unused variable, left from previous hacking I guess.

Yep!

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.

Yea' that would probably be a good idea!

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.

Ok, I'll speed it up.

quoted

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

test_commit ?

Cool, I'll use that!

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.

[...]

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.

All very good points, thanks. I'll get back to hacking.

-Aske