Environment variable to hard-skip specific hooks

[guillaumeaubert]

Per a discussion with @timbunce, we should have a way to skip a hook. An environment variable GITHOOKS_SKIP that takes a comma-separated list of hooks to skip sounds desirable.

Additionally, in the case of the pre-commit hook being skipped this way, we should add a note in the commit message that this path was used (for future reference / review).

[choroba]

Hi, I was assigned App::GitHooks in the CPAN Pull Request Challenge. I guess I could help you with this issue, but I need some guidance, as the module seems pretty abstract.

Where should the check happen? In App::GitHooks::run() or App::GitHooks::Hook::run() (or somewhere else)?

[guillaumeaubert]

Hello @choroba! Thank you for looking at my module as part of the challenge.

App::GitHooks::Hook::run() will get subclassed by various hooks with more specific flows (e.g.: PreCommit) and calling SUPER::run() isn't a requirement for the subclassed run() methods, so I would avoid that spot to keep the subclassing process simple.

But App::GitHooks::run() seems like a good spot for it indeed. When you skip a hook there, I would suggest printing some acknowledgement on STDERR, so that there's a record for the user (in case the output is being logged). It should make writing a test much easier for you as well.

Let me know if you have any questions!

[choroba]

Well, the next question is harder. If we just skip the ->run method of the hook, which would be the simplest solution, we'd still do a lot of unnecessary work: validating arguments, creating an instance, handling the encoding. The other option is to skip at the moment we have the hook name - but it would be a larger change, as we'd need to leave the try block from the middle. What are your opinions?

[guillaumeaubert]

I agree, the second option is going to be cleaner. Fortunately it shouldn't require a major change - the try/catch/finally blocks in Try::Tiny are actually subs, so you should be able to return $HOOK_EXIT_SUCCESS once you have the hook name, to leave the try block and set the correct exit code.

[timbunce]

The original motivation due to very slow commits on large repos containing lots of third-party code. In that case the cost of even loading the App::GitHooks modules hundreds or thousands of times was significant.

So I hacked something like this into the top of the hook.pl script:

BEGIN { exit 0 if $ENV{DISABLE_APP_GITHOOKS} }
use App::GitHooks ...

so App::GitHooks wasn't even loaded.

This doesn't detract from the possible utility of more fine-grained control, but this was simple and sufficient for my needs.

[guillaumeaubert]

Thank you for the context, @timbunce!

As long as we exit before the plugins are run, @choroba's approach should still be fine. The expensive part on very large diffs is the checking of each modified file by each plugin, and instantiating App::GitHooks still gives us an opportunity to warn about git hooks being intentionally disabled for that run.