pre-commit: add hook to ensure Makefiles are indented with tabs #6691
Conversation
Manual hook added which runs a shell script. Can be run locally manually by Linux or Mac users and we have this also running on the GitHub Actions CI. Will fail if Makefiles uses spaces for indentation. Can run the manual hooks with: `pre-commit run check-makefile-indentation --all-files --hook-stage manual`
Summary of ChangesHello @jbampton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new pre-commit hook to ensure Makefiles use tabs for indentation. The implementation is generally correct, but there are a couple of areas for improvement.
In scripts/check_makefiles_for_tabs.sh, the script uses grep -P, which is a GNU extension and will not work on standard macOS systems. I've suggested a more portable and efficient alternative.
In .pre-commit-config.yaml, the file matching pattern is quite restrictive and will only check a Makefile in the root directory. I've left a comment to consider making this more flexible to accommodate future Makefiles in other directories.
| if [[ -f "$makefile" ]]; then | ||
| if grep -P '^\s' "$makefile" | grep -vP '^\t' > /dev/null; then | ||
| echo "Error: File '$makefile' contains spaces at the beginning of lines instead of tabs." | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
This script has a couple of issues:
- Portability: The use of
grep -Pfor Perl-compatible regular expressions is a GNU extension and is not available on all systems, notably default macOS installations. This will cause the hook to fail for Mac users. - Redundancy & Efficiency: The
if [[ -f "$makefile" ]]check is redundant becausepre-commitis configured withtypes: [file], which ensures only existing files are passed. Additionally, using a pipe with twogrepprocesses is less efficient than a singlegrep.
You can address these points by using a single, more portable grep command to check for lines starting with a space. The [ ] syntax for a space is used for clarity and portability.
| if [[ -f "$makefile" ]]; then | |
| if grep -P '^\s' "$makefile" | grep -vP '^\t' > /dev/null; then | |
| echo "Error: File '$makefile' contains spaces at the beginning of lines instead of tabs." | |
| exit 1 | |
| fi | |
| fi | |
| if grep -q '^[ ]' "$makefile"; then | |
| echo "Error: File '$makefile' contains spaces at the beginning of lines instead of tabs." | |
| exit 1 | |
| fi |
| description: ensures that Makefiles are indented with tabs | ||
| entry: ./scripts/check_makefiles_for_tabs.sh | ||
| language: system | ||
| files: "(?i)^makefile$" |
There was a problem hiding this comment.
The regex (?i)^makefile$ will only match a file named Makefile or makefile in the root of the repository. If you ever add Makefiles in subdirectories, they will not be checked. A more flexible pattern would be something like (?i)makefile$, which would match any file ending in makefile (e.g., src/Makefile). Consider if you want to support this for future-proofing.
1 participant
Manual hook added which runs a shell script.
Can be run locally manually by Linux or Mac users and we have this also running on the GitHub Actions CI.
Will fail if Makefiles uses spaces for indentation.
Can run the manual hooks with:
pre-commit run check-makefile-indentation --all-files --hook-stage manual