-
Notifications
You must be signed in to change notification settings - Fork 17
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.
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
wait
builtin 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 -p
some of the checks might be already failed and finished and it won't show on the job list.Proof of Work
Checklist
skip-changelog
label if not needed