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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 16, 2012 at 9:25 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Aske Olsson <askeolsson@xxxxxxxxx> 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.

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

>> +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]