-
Notifications
You must be signed in to change notification settings - Fork 30
Fix races in pre-commit checks and improve error handling #365
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
Conversation
MCK 1.3.0 Release NotesBug Fixes
Other Changes
|
9fd019e to
2c3d9f5
Compare
2c3d9f5 to
4b596b6
Compare
.githooks/pre-commit
Outdated
| run_job_in_background() { | ||
| job_name=$1 | ||
| time ${job_name} 2>&1 | prepend "${job_name}" & | ||
| capture_bg_job "${job_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not inline capture_bg_job here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch! It was a leftover from my previous approach where I had capture_bg_job used for every check in pre-commit function. Removed.
nammn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, consider my comment
Summary
Introduction of parallelism in pre-commit hook introduced races or even caused ignoring errors for some checks executed in background jobs.
Changes
waitbuiltin without arguments. While it's OK for just waiting until completion it's ignoring any errors. Now, we're usingwait "${pid}", which fails if the background job return non zero code.jobs -psome of the checks might be already failed and finished and it won't show on the job list.Proof of Work
Checklist
skip-changeloglabel if not needed