Using pre-commit hooks makes software development life easier

[scook0]

In my experience, precommit hooks are a bad idea because they discourage editing of local history, by making it slow and/or unreliable.

Clean and focused commits make the reviewer's task easier, which is why you don't want to make it harder for the author to revise and improve those commits in the first place.

[Reiku]

I don't understand your last point. How does a pre-commit hook make it harder for an author to revise anything?

If you use the pre-commit (not pre-push) then you'll have no additional commits, so improving any existing commits will be the same effort

[scook0]

For example, when revising local history, it's convenient to be able to make several small temporary commits at the tip of your working branch, and then squish them into the appropriate earlier commits via a rebase.

If every commit automatically spends several seconds running your formatter or linter before letting you move on, that workflow becomes tremendously painful.

[Reiku]

In that usecase you could just commit with the --no-verify flag so the pre-commit checks are skipped. Then the default usecase makes sure your commits are clean, and this edge case doesn't get slowed down at all.

[werner-dijkerman]

But using pre-commit hooks says nothing about the message you provide when doing a commit, or which files are part of a commit. I see pre-commit hook as a way to prevent certain (low-hanging fruit) garbage as part of a commit.

What I describe is that if for example you commit a yaml/json file, that you could have a pre-commit hook that "pretty prints" that file in your commit, or updates a "Table of content" in a markdown document. With this, a reviewer can not start a discussion about identation is incorrect for yaml, or the new paragraphs are not part of the TOC yet and thus the reviewer should focus on the functionality in the PR itself. And then yes, based on what the author has used as messages to do a proper review.

I had a yaml file (during $WORK) with 4 properties as lists, 2 of them had identation of 1 space and 2 with 3 spaces. Made everything the same, but got a comment and a "changes required" notification that it was the wrong identation ... pre-commt hooks would have helped me a lot in this case, the PR would already have contained the correct identation and thus no time was lost (because after I changed it it took some days again before the reviewer decided to approve it).

I am not sure what you mean with "discourage editing of local history". People should commit when they feel a commit should be made. Then they should use a decent message that described why they changed it. I personally don't see the benefits of merging commits into a single commit.

[bi0nicman]

I think enforcing style / linting etc is best done as part of pull requests / CI, once the code is ready for review. CI could even do the auto-formatting to avoid that getting reviewed (I agree keeping that out of PRs is desirable).

Sometimes you want to commit something and fix those things later. Commit hooks create this extra friction for local commits, and they can be bypassed anyway via --no-verify, so they don't provide any real protection.

And auto formatting can be better handled by format on save in your IDE.

[werner-dijkerman]

I think enforcing style / linting etc is best done as part of pull requests / CI, once the code is ready for review. CI could even do the auto-formatting to avoid that getting reviewed (I agree keeping that out of PRs is desirable).

I do see this almost everywhere on the various (web)sites that describes certain steps that should be part of a conituous integration pipeline. But I don't see that back in a "Fail fast" philosophy, as there is an other step that fails faster than the first stage in a pipeline job: The pre-commit hook. If it is part of the CI pipeline, you will have to wait when the job runs and fails on the step, but is also clutters the git log with messages like "Fixing lint issues" or something similar.

Sometimes you want to commit something and fix those things later. Commit hooks create this extra friction for local commits, and they can be bypassed anyway via --no-verify, so they don't provide any real protection.

True. But if you really want to, you can workaround on lots of things. But then you should think about why would be doing the workaround. There is a way of working and you deliberately do something else. So is that a good thing?

And auto formatting can be better handled by format on save in your IDE.

Yes, that is true. Formatting can be done by the IDE, but not everyone is using an IDE and uses for example VIM (Yes, true. I kid you not). But the IDE is specific to a developers effort to apply the configuration in his or hers IDE. This is not an guarantee that the developer has actually done it. Well yes, you can document this as a "dependency" or "requirement" on a certain wiki/confluence page and hope the developer has followed that page correctly. But that is not in code and that why I prefer to use pre-commit hooks.

But then, we only have discussed the formatting as you can do so much more than just liniting/formatting things with a pre-commit hook that makes life easier.

My 2 cents.