Git hook to prevent unformatted code from being committed

[Apeiros-46B]

Description

This PR adds a Git pre-commit hook that tracks the last time the formatter was run, and cancels the commit and informs the user if any staged code or documentation files are more recent than the last time of format. This ensures that code style guidelines are being followed before files are committed.

Existing developers need to run git config core.hooksPath scripts/githooks from repo root; new developers will have this done automatically by setup_software.sh.

Resolved Issues

Resolves #3173

Review Checklist

(No edits to actual code)

It is the reviewers responsibility to also make sure every item here has been covered

• Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
• Remove all commented out code
• Remove extra print statements: for example, those just used for testing
• Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

[Apeiros-46B]

Did the software tests fail because of the erroneous removal of the requirements_lock.txt by the auto fix? I'm not familiar with how the CI works yet

[williamckha]

Did the software tests fail because of the erroneous removal of the requirements_lock.txt by the auto fix? I'm not familiar with how the CI works yet

Looks like it, I'm not sure why it did that. Try regenerating the file by running scripts/compile_pip_requirements.sh

[williamckha]

This looks good, awesome work! However, I'm wondering if we can reuse our existing pre-commit setup so that we don't have to handroll our own git hooks.

Software/.pre-commit-config.yaml

Lines 1 to 17 in 549869f

	repos:
	- repo: local
	hooks:
	- id: compile_pip_requirements
	name: compile pip requirements
	language: system
	entry: scripts/compile_pip_requirements.sh
	- id: lint_and_format
	name: fix common trivial code errors
	language: system
	entry: scripts/lint_and_format.sh
	- id: generate_fsm_diagrams
	name: generate new FSM diagrams
	language: python
	entry: scripts/fsm_diagram_generator.py

Right now, we only run these pre-commit hooks in CI, but we could run them locally during development too. setup_software.sh could install the pre-commit package manager and then install the hooks so that they run automatically on git commit. We should probably only run lint_and_format on commit and configure the other hooks to be manual so that they are only run in CI (since they might take a while).

[Apeiros-46B]

With pre-commit, would it be possible to preserve the behavior of just stopping a commit and notifying the developer instead of automatically running the whole formatter? I used this this approach because of how long the formatting script takes, which might be annoying when running git commit.

Considering ergonomics, maybe it might be better to have this as a pre-push hook instead? This way developers can work on all their commits and then just format once before pushing instead of at every commit, which would be a lot more frictionless.

I personally prefer the second option, but I'm willing to work on either of these. Please let me know what you think.

[williamckha]

I see your point now about why we might not want to automatically run the formatting script, and instead just give a warning to the user. Actually, it reminds me of a discussion I had with previous leads about whether we should even be installing git hooks locally. Especially during competition when we're making a lot of changes and don't have a lot of time, I can see this pre-commit/push hook becoming very annoying if it keeps nagging at us to run the formatter. That's sort of why we opted to use pre-commit CI (to ensure that code merged into master is properly formatted and linted, without being a huge inconvenience).

Also, the issue of wasting CI time mentioned in #3173 is not really relevant because afaik github actions is free without limits for public repos.

Considering all this, I think we should use your git hook and change it so that it

• is pre-push instead of pre-commit, and
• prompts the user y/n on whether to continue with the push anyway, even if code is unformatted

[Apeiros-46B]

EDIT: I've solved the major issues with my pre-push implementation, but there's still one issue:
If you commit changes that are considered unformatted, then do git reset --soft HEAD^ to "uncommit", even if the previous state was fine, the push hook complains about it being unformatted. I'm not quite sure how to solve this, but I don't think it's a big deal because you can tell it to push anyways.

[Andrewyx]

Is there some way to commit forcefully regardless of formatting (e.g. via some flag)? It is not uncommon that, during mid-prototyping, a developer would want to be able to commit their work either to save or share their work via remote. It is also common that merge errors may arise, and some nasty commits may have to be made to prevent lost work.

[Apeiros-46B]

Yes, the current version only warns you on commit (doesn't stop you completely), doing the formatting check on push only. You can still bypass it anyways at a y/n prompt

[Andrewyx]

@Apeiros-46B Is this ready for a re-review?

[Apeiros-46B]

Yes

[itsarune]

Nice work, left some comments

[nycrat]

Quick note, once we have mac support from #3496, the stat command has slightly different syntax on mac. So it would be stat -f %m instead of stat -c %Y in the pre-commit script. Might need to have either a different script for mac or just a condition that checks which os.

Either way not that relevant right now though