Skip to content

Fix pre-commit script not preventing commit - #167

Closed
alitzel-macias wants to merge 2 commits into
mainfrom
advanced-course-fix-ex7-pre-commit-script
Closed

Fix pre-commit script not preventing commit#167
alitzel-macias wants to merge 2 commits into
mainfrom
advanced-course-fix-ex7-pre-commit-script

Conversation

@alitzel-macias

Copy link
Copy Markdown
Contributor

In Exercise 7, in the pre-commit script when the whitespace check script failed but the verify-name-and-email script didn't, then pre-commit exited without error and allowed the commit to go through.

@alitzel-macias
alitzel-macias requested a review from stelliom July 8, 2026 12:06
@stelliom

stelliom commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Nice. thank you! Maybe you could add a comment or note explaining why the status is needed, so the people doing the course don't get confused.

Note

Another option to fix this would be to add set -e at the beginning of the script. This would exit immediately if something fails, i.e. it would not run more pre-commit checks if one has already failed. However, I think in this case your solution is better, since the two pre-commit checks are independent, so if both fail users could fix them in one go.

@mjaehn mjaehn added bug Something isn't working advanced Relevant for Advanced course labels Aug 18, 2026
@mjaehn

mjaehn commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks @alitzel-macias!

This will be fixed in #170 using set -e instead of manual status tracking.

As this covers the same issue you found, so I'll close this PR here.

@mjaehn mjaehn closed this Aug 18, 2026
@mjaehn
mjaehn deleted the advanced-course-fix-ex7-pre-commit-script branch August 19, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

advanced Relevant for Advanced course bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants