From 52b2613d6a27984a1c4d19dadfbec1fca4203125 Mon Sep 17 00:00:00 2001 From: MAJDOUB Khalid Date: Mon, 15 Jun 2026 20:12:41 +0200 Subject: [PATCH] Make Git hook configuration idempotent to avoid config.lock churn 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. --- .../jenkins/plugins/git/DisableHooks.java | 7 +- .../java/jenkins/plugins/git/UnsetHooks.java | 5 +- .../git/GitHooksConfigurationTest.java | 72 +++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/src/main/java/jenkins/plugins/git/DisableHooks.java b/src/main/java/jenkins/plugins/git/DisableHooks.java index e7c0fc4735..3ae75796b3 100644 --- a/src/main/java/jenkins/plugins/git/DisableHooks.java +++ b/src/main/java/jenkins/plugins/git/DisableHooks.java @@ -23,8 +23,11 @@ class DisableHooks implements RepositoryCallback { public Object invoke(Repository repo, VirtualChannel channel) throws IOException, InterruptedException { final String VAL = Functions.isWindows() ? DISABLED_WIN : DISABLED_NIX; final StoredConfig repoConfig = repo.getConfig(); - repoConfig.setString("core", null, "hooksPath", VAL); - repoConfig.save(); + // Skip the rewrite, and its config.lock, when hooksPath is already disabled. JENKINS-71349 + if (!VAL.equals(repoConfig.getString("core", null, "hooksPath"))) { + repoConfig.setString("core", null, "hooksPath", VAL); + repoConfig.save(); + } return null; } } diff --git a/src/main/java/jenkins/plugins/git/UnsetHooks.java b/src/main/java/jenkins/plugins/git/UnsetHooks.java index 54cebbb89f..c5811e0222 100644 --- a/src/main/java/jenkins/plugins/git/UnsetHooks.java +++ b/src/main/java/jenkins/plugins/git/UnsetHooks.java @@ -21,7 +21,10 @@ class UnsetHooks implements RepositoryCallback { public Object invoke(Repository repo, VirtualChannel channel) throws IOException, InterruptedException { final StoredConfig repoConfig = repo.getConfig(); final String val = repoConfig.getString("core", null, "hooksPath"); - if (val != null && !val.isEmpty() && !DisableHooks.DISABLED_NIX.equals(val) && !DisableHooks.DISABLED_WIN.equals(val)) { + if (val == null) { + return null; // nothing to unset; avoid a redundant config.lock write. JENKINS-71349 + } + if (!val.isEmpty() && !DisableHooks.DISABLED_NIX.equals(val) && !DisableHooks.DISABLED_WIN.equals(val)) { LOGGER.warning(() -> "core.hooksPath explicitly set to %s and will be left intact on %s.".formatted(val, repo.getDirectory())); } else { repoConfig.unset("core", null, "hooksPath"); diff --git a/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java b/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java index 9d12b22c01..a2f996fe7e 100644 --- a/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java +++ b/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java @@ -26,6 +26,8 @@ import hudson.EnvVars; import hudson.model.TaskListener; +import java.io.File; +import java.nio.file.Files; import java.util.Random; import org.eclipse.jgit.lib.StoredConfig; import org.jenkinsci.plugins.gitclient.Git; @@ -203,4 +205,74 @@ void testConfigure_3argsFalse() throws Exception { String hooksPath = getCoreHooksPath(); assertThat(hooksPath, is(NULL_HOOKS_PATH)); } + + private File createConfigLock() throws Exception { + return client.withRepository((repo, channel) -> { + File lock = new File(repo.getDirectory(), "config.lock"); + Files.deleteIfExists(lock.toPath()); + assertTrue(lock.createNewFile()); + return lock; + }); + } + + // JENKINS-71349: re-disabling an already-disabled repo must not save(). A planted stale + // config.lock would otherwise make save() throw LockFailedException. + @Test + void testConfigureDisabledIsNoOpWhenAlreadyDisabled() throws Exception { + GitHooksConfiguration.configure(client, false); + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + + File configLock = createConfigLock(); + try { + GitHooksConfiguration.configure(client, false); + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + assertTrue(configLock.exists(), "DisableHooks must not touch an unrelated stale lock"); + } finally { + Files.deleteIfExists(configLock.toPath()); + } + } + + // The guard must skip only the disabled sentinel, never a different value (SECURITY-2754). + @Test + void testConfigureDisabledOverwritesNonDisabledValue() throws Exception { + setCoreHooksPath(ALTERNATE_HOOKS_PATH); + assertThat(getCoreHooksPath(), is(ALTERNATE_HOOKS_PATH)); + + GitHooksConfiguration.configure(client, false); + + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + } + + @Test + void testConfigureDisabledRepeatedlyIsNoOp() throws Exception { + GitHooksConfiguration.configure(client, false); + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + + File configLock = createConfigLock(); + try { + for (int i = 0; i < 5; i++) { + GitHooksConfiguration.configure(client, false); + } + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + assertTrue(configLock.exists(), "repeated disable must not touch the stale lock"); + } finally { + Files.deleteIfExists(configLock.toPath()); + } + } + + // JENKINS-71349, allow-hooks path: UnsetHooks must not save() when hooksPath is already absent. + @Test + void testConfigureAllowedIsNoOpWhenHooksPathAbsent() throws Exception { + GitHooksConfiguration.configure(client, true); + assertThat(getCoreHooksPath(), is(nullValue())); + + File configLock = createConfigLock(); + try { + GitHooksConfiguration.configure(client, true); + assertThat(getCoreHooksPath(), is(nullValue())); + assertTrue(configLock.exists(), "UnsetHooks must not touch the stale lock when hooksPath is absent"); + } finally { + Files.deleteIfExists(configLock.toPath()); + } + } }