Make Git hook configuration idempotent to avoid config.lock churn#3972
Open
hadamrd wants to merge 1 commit into
Open
Make Git hook configuration idempotent to avoid config.lock churn#3972hadamrd wants to merge 1 commit into
hadamrd wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses build serialization and checkout failures caused by repeated .git/config writes when disabling Git hooks. It makes hook disabling idempotent by skipping StoredConfig.save() when core.hooksPath is already set to the disabled sentinel value, preventing unnecessary creation of .git/config.lock (JENKINS-71349 / JENKINS-65277).
Changes:
- Update
DisableHooksto only write/savecore.hooksPathwhen it differs from the disabled value. - Add regression tests covering stale
config.lockbehavior, repeated disables as a no-op, and overwriting a non-disabled hooks path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/jenkins/plugins/git/DisableHooks.java | Adds an idempotency guard to avoid unnecessary .git/config saves/locks when hooks are already disabled. |
| src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java | Adds regression tests to validate the no-op behavior and ensure non-disabled values are still overwritten. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // every checkout of a reused workspace serializes builds on that lock and, if a | ||
| // build is interrupted between lock creation and the rename, leaves behind a stale | ||
| // config.lock that blocks all subsequent checkouts of that workspace (JENKINS-71349). | ||
| // The sibling UnsetHooks already guards its save() the same way. |
Author
There was a problem hiding this comment.
Reworded to drop the UnsetHooks reference. You're right that UnsetHooks wasn't idempotent either — it called unset()+save() on every checkout when core.hooksPath was already absent, taking the same config.lock. Added an early return there and a test for that path.
DisableHooks and UnsetHooks rewrote core.hooksPath on every checkout, and StoredConfig.save() creates a .git/config.lock each time. On shared-library and pipeline-definition checkouts that reuse one controller-side .git across builds this serializes builds on that lock, and a build interrupted between the lock creation and the rename leaves an orphaned config.lock that blocks every later checkout of that workspace until it is removed by hand (JENKINS-71349, JENKINS-65277). DisableHooks now writes only when core.hooksPath differs from the disabled value. UnsetHooks returns early when it is already absent instead of calling unset() + save(). A value set to something else is still overwritten by DisableHooks or left intact with the existing warning by UnsetHooks.
hadamrd
force-pushed
the
fix/disablehooks-idempotent-config-lock
branch
from
43ca656 to
52b2613
Compare
hadamrd
changed the title
core.hooksPath rewrite when hooks are already disabled
MarkEWaite
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JENKINS-71349
DisableHooksandUnsetHooksrewritecore.hooksPathon every checkout, andStoredConfig.save()creates.git/config.lockeach time. On shared-library and pipeline-definition checkouts that reuse one controller-side.gitacross builds, this serializes builds on that lock; a build interrupted between the lock creation and the rename leaves an orphanedconfig.lockthat blocks every later checkout of that workspace until it is removed by hand. Same report in JENKINS-65277.DisableHooks: writecore.hooksPathonly when it differs from the disabled value.UnsetHooks: return early whencore.hooksPathis already absent instead of callingunset()+save().A value set to something else is still overwritten (
DisableHooks) or left intact with the existing warning (UnsetHooks).Testing done
GitHooksConfigurationTest: a staleconfig.lockplanted on an already-disabled repo is ignored (errors withLockFailedExceptionbefore this change); the allow path is a no-op whenhooksPathis already absent; repeated disable is a no-op; a non-disabled value is still overwritten. 17 tests green locally (Linux, JDK 21).