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

From

Aske Olsson <askeolsson@gmail.com>

Date

Nov 17, 2012, 09:00 UTC

Message-ID

<CAJwKrPbP--_zT4=xaYKwD1SbkO+bhT73EbgVzgT0oPA2FBCfTw@mail.gmail.com>

In-Reply-To

<vpq625547ne.fsf@grenoble-inp.fr>

> Aske Olsson <askeolsson@gmail.com> writes:
>
>> +--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.

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.

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