Skip to content

Add check target and provide pre-commit hook for 'make check' #8021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 26, 2025

Conversation

mattkretz
Copy link
Member

I tend to forget running check-source.sh before pushing. This might help.

Comment on lines 5 to 12
precommit="$(git rev-parse --git-dir)/hooks/pre-commit"
test -f "$precommit" && exit
cat <<EOF > "$precommit"
#!/bin/sh
cd \$(git rev-parse --show-toplevel)/source
make check
EOF
chmod +x "$precommit"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this change do?

Copy link
Member Author

@mattkretz mattkretz Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no pre-commit script exists, it installs one that runs make check in the source directory before every commit. That way one can't commit unless check-source passes.
Obviously, that's opt-in via calling this script first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean even locally? That seems a bit draconian, doesn't it? What if I just want to commit some random noise locally?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I at least don't want to commit noise that would obviously fail CI.
But yes, it might not be a feature for everybody. I could change the setup script to ask whether it should install the hook?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I, on the other hand, as a major user of this repository, would like to commit whatever I like under whatever conditions I like...

Could you just install this script locally in your own checkout?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it an opt-in given...

If no pre-commit script exists, it installs one that runs make check in the source directory before every commit.

That sounded like we'd always install the hook? Do you mean "auto opt-in"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to run the tools/gitconfig.sh script manually to install the hook. If you then dislike the hook you do rm .git/hooks/pre-commit. But like I said, I can also make the script ask explicitly, because it should be recommended to run the script.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed an improved script that asks for confirmation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, even with the hook installed, git commit --no-verify allows to commit failing noise :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see, I didn't realize that this is just an optional tool. Seems fine.

As a drive-by fix, add all other phony targets to .PHONY.
@mattkretz mattkretz force-pushed the pre-commit-check-source branch from bb70301 to 40037b8 Compare June 26, 2025 12:23
@tkoeppe tkoeppe requested a review from jensmaurer June 26, 2025 14:31
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tools/gitconfig.sh is a totally optional "whoever wants to use it" thing, and installing the hook seems totally in line with the exiting other contents.

@tkoeppe tkoeppe merged commit 47d9c36 into cplusplus:main Jun 26, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants