A plea to abolish commit hooks

[davidmsibley]

I realize I've expressed being against commit hooks, but I don't think I've explained why.

Committing is always good. I never want to discourage people from committing. I realize there's value in influencing the quality of code, but I believe that investment should be made at the integration layer (CI and PR's). Version control should be about retaining history, so that we can learn how code came about. You can't do that if your version control stops you from storing your work until you've made it presentable.

In the same vein, I want to advocate using pull requests as learning opportunities or teachable moments, rather than customs checkpoints. No one's going to lose their job because they didn't put curlies around an if block. So, the Watchmen of this little world, we ain't. Instead, Committers get to be shepherds and explorers within the world of code. We get to learn from one another and improve our skills as we grow the value of the product. If we just enforce rules, it's difficult to learn why those rules are effective. So again, I'd encourage giving developers access to the tools that improve our codebase, and impressing upon them the reasons we value each tool.

Finally, imperfections within the code (in moderation) are often the best motivation to improve your craft. Refactoring is the most effective method of learning how and why a system works as it does. Teaching developers to spot potential problems encourages them to refactor early and often. Letting imperfect code through the integration process can drive developers to grow, while still keeping a handle on a product's technical debt.

I think the style guides and the linting have definitely given this product a lot of value, but I want to voice concern before we head too far down this path. Every project has a different set of circumstances and requirements. I'd like to make sure everyone who works on this project can navigate us towards the destination, rather than just being able to keep us on the road. I want to encourage committing early and often, using pull requests as learning opportunities, and giving developers the fire to improve. I believe this is the best way to build a community that values the same things, not one that conforms to the style du jour.

[ChristianMurphy]

I hear you @davidmsibley. 👂

I completely agree that uPortal-app-framework should strive to be as easy to contribute to and as welcoming as possible. 💯
I still believe that Git Hooks help enable and empower the contributing process, rather than hurting it.
-0.9 for removing Git Hooks.

---

Version control should be about retaining history, so that we can learn how code came about. You can't do that if your version control stops you from storing your work until you've made it presentable.

That is one way of looking at it.
From another perspective, for git history to have value, you need to be able to see what people really changed, tools like git blame can't tell the difference between style fixes, features, and bug fixes.
Precommit hooks reduce stylistic noise from commits, giving historical tools better data to work with.
📓 Note: Git squash can be used to achieve a similar level of commit cleanliness.
📔 Note: conventional commits works along side this to add meta data that can help distinguish types of changes.

Letting imperfect code through the integration process can drive developers to grow, while still keeping a handle on a product's technical debt.

Unit Testing, Static Analysis, and Code Style are not the gold standard for code quality 🥇 (by themselves), they are baseline metrics. They provide a low cost, low maintenance way to keep quality from dropping into a free fall 📉. While still leaving more abstract code concepts, like usability or good design, open for discussion and teachable moments. 🎓

In the same vein, I want to advocate using pull requests as learning opportunities or teachable moments, rather than customs checkpoints
I'd encourage giving developers access to the tools that improve our codebase, and impressing upon them the reasons we value each tool.
I want to encourage committing early and often, using pull requests as learning opportunities, and giving developers the fire to improve.

All great points, but they are strawman arguments.
Git hooks don't prevent any of these.

I'd like to make sure everyone who works on this project can navigate us towards the destination, rather than just being able to keep us on the road.

It would be helpful to know what that destination is, most of the backlog is still in a private issue tracker that the public can't access. 😉

[davidmsibley]

Then I'll be consise. Committing is always good. There should be no barriers to committing.

Your points above seem to be focused on the code. I'm not worried about the code, code can always change. I want to be able to build trust in developers. There's no amount of process and code quality metrics that will deliver a working product.

[ChristianMurphy]

There should be no barriers to committing.

It isn't a barrier though, it's a suggestion that is easily by passed with --no-verify.

I'm not worried about the code, code can always change.

I am.
Code can't always change, change is limited by the triple constraint (time, resources, scope).
"code can always change" assumes that significant resources and time are being set aside solely for the purpose of code quality.

[apetro]

(Wrote and deleted a few long from attempts at this. Going with this:)

That @davidmsibley would like to try not having the commit hooks convinces me. Let's turn them off and work together to explore how might we achieve similar advantages without actual commit hooks, probably by more attention to the PR stage of contributions.

Worst case, we learn from this why commit hooks are worth it, observing when they would have helped and deciding to turn them on again in the future.

Best case, we achieve similar code quality and process advantages in different ways. Doesn't annoy @davidmsibley so much, still quality, a win all around.

Here's the thing: @davidmsibley and @ChristianMurphy are both talented, thoughtful committers. I predict real followthrough both on what are we learning from this, and on "how might we achieve similar advantages without relying on the commit hooks"?

Code quality is a challenge, and the burden on maintainers in engaging with problematic contributions may become a more prominent challenge in the future. I don't think it's the biggest present challenge though. The biggest present challenge is attracting more contributors and more contribution. There's at least the colorable argument that commit hooks make things less convenient for contributors earlier in their contribution process and so discourage contributions before they hit the PR stage where we can engage with them. I don't have to think this hypothesis will prevail to believe that reasonable people could have this hypothesis. So let's try it. Off with the commit hooks, experiment to see whether things get worse (too much work or compromises at PR stage) or better (more contributors and contributions), and experiment with, given the constraint of not using commit hooks, what can we come up with that adds similar value.

[vertein]

+0 for removing the git commit hooks.

I do see value of the CI layer enforcing some rules, and with that I'll +1 the discussion about more tightly enforcing the checks.

[ChristianMurphy]

If githooks are being disabled, the conventional commits check will need to adapt.
Currently those only checked via the commitmsg hook, that will need to move to CI.

ref: https://marionebl.github.io/commitlint/#/guides-ci-setup?id=linting-relevant-commits

[ChristianMurphy]

To reiterate, I'm -0.9 on this. Meaning

I don't like this, but I'm not going to stand in the way if everyone else wants to go ahead with it

---

@apetro

how might we achieve similar advantages without relying on the commit hooks

All of the advantages can be achieved with out git hooks, except one.
Git hooks run before code has left a developer's machine, giving feedback sooner, and keeping the focus of PRs higher level.

The biggest present challenge is attracting more contributors and more contribution. There's at least the colorable argument that commit hooks make things less convenient for contributors earlier in their contribution process and so discourage contributions before they hit the PR stage where we can engage with them.

I tested that hypothesis on one of my projects, before bringing git hooks here.
After enabling the hooks, the project continued to get new contributors, and PRs required less rework.

---

@vertein

I do see value of the CI layer enforcing some rules

I'd be more open to moving some of the checks exclusively to CI, while leaving some hooks in place.

I'll +1 the discussion about more tightly enforcing the checks

same

[apetro]

Could we make local git hooks opt-in rather than opt-out?

[ChristianMurphy]

Could we make local git hooks opt-in rather than opt-out?

not easily.
neither githooks nor husky have an on/off switch.
disable functionality would need to be added to the tasks themselves.

[davidmsibley]

How about commit hooks that just run but don't block you from committing if there are errors?

[ChristianMurphy]

@davidmsibley Warn level git hooks would be fine. 👍

[marionebl]

I'm the author of commitlint. Currently I collect some ideas at https://github.com/marionebl/commitlint/issues/94 on how to make commitlint and commit hooks a better contributor experience. Would be awesome if you could leave your thoughts.

[ChristianMurphy]

@davidmsibley and @apetro its also worth noting that many first time contributors use the GitHub site to make changes, git hooks have no impact on that experience.

[ChristianMurphy]

@davidmsibley commitlint now runs on CI not through git hook

[ChristianMurphy]

UW-Madison-DoIT/widget-creator#25 offers a non-blocking approach to handling styling locally.
If it works out, that could hopefully bring this ticket to a close.

[ChristianMurphy]

So I came across a way that might make optional githooks possible.
Husky includes an uninstall script under the hood. https://github.com/typicode/husky/blob/4265d69f577514aea493387706d73f51deceba73/src/uninstall.js

Currently husky installs itself as part of the npm install process.
We could create an npm postinstall hook, that calls husky's uninstall script, directly after it has been initially installed.
Then add a new script add-hooks that would need to manually be run to re-install the hook.

Its round about and a bit hacky (kinda like waiting until after code is committed and pushed to do sanity checks 😜), but it could work.
Thoughts? 💭

edit after some more thought, a new script add-hooks could also be linked to npm install husky --no-save, it would remove the uninstall, re-install hack, but would leave husky as a floating version.

[vertein]

edit after some more thought, a new script add-hooks could also be linked to npm install husky --no-save, it would remove the uninstall, re-install hack, but would leave husky as a floating version.
That could be nice. I've grown to appreciate locally checking my commit messages.

[ChristianMurphy]

The last vestiges of git hooks that are on by default have been removed.
Hooks are now optional, but still highly recommended.