diff --git a/CHANGES.md b/CHANGES.md index 38af879390..a0745a3b99 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -16,6 +16,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( * Adds support for worktrees (fixes [#1765](https://github.com/diffplug/spotless/issues/1765)) * Bump default `google-java-format` version to latest `1.24.0` -> `1.28.0`. ([#2345](https://github.com/diffplug/spotless/pull/2345)) * Bump default `ktlint` version to latest `1.5.0` -> `1.7.1`. ([#2555](https://github.com/diffplug/spotless/pull/2555)) +* `GitPrePushHookInstaller` uses a lock to run gracefully if it is called many times in parallel. ([#2570](https://github.com/diffplug/spotless/pull/2570)) ## [3.3.1] - 2025-07-21 ### Fixed diff --git a/lib/src/main/java/com/diffplug/spotless/GitPrePushHookInstaller.java b/lib/src/main/java/com/diffplug/spotless/GitPrePushHookInstaller.java index 7a627a070c..f2c6ad8018 100644 --- a/lib/src/main/java/com/diffplug/spotless/GitPrePushHookInstaller.java +++ b/lib/src/main/java/com/diffplug/spotless/GitPrePushHookInstaller.java @@ -24,6 +24,8 @@ import java.nio.file.Files; import java.util.Locale; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + /** * Abstract class responsible for installing a Git pre-push hook in a repository. * This class ensures that specific checks and logic are run before a push operation in Git. @@ -35,6 +37,10 @@ public abstract class GitPrePushHookInstaller { private static final String HOOK_HEADER = "##### SPOTLESS HOOK START #####"; private static final String HOOK_FOOTER = "##### SPOTLESS HOOK END #####"; + private static final Object LOCK = new Object(); + + private static volatile boolean installing = false; + /** * Logger for recording informational and error messages during the installation process. */ @@ -56,6 +62,44 @@ public GitPrePushHookInstaller(GitPreHookLogger logger, File root) { this.root = requireNonNull(root, "root file can not be null"); } + /** + * Installs the Git pre-push hook while ensuring thread safety and preventing parallel installations. + * + * The method: + * 1. Uses a thread-safe mechanism to prevent concurrent installations + * 2. If a parallel installation is detected, logs a warning and skips the installation + * 3. Uses a synchronized block with a static lock object to ensure thread safety + * + * The installation process sets a flag during installation and resets it afterwards, + * using the {@link #doInstall()} method to perform the actual installation. + * + * @throws Exception if any error occurs during installation + */ + @SuppressFBWarnings(value = "ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD", justification = "This is a safe usage of a static lock object") + public void install() throws Exception { + if (installing) { + logger.warn("Parallel Spotless Git pre-push hook installation detected, skipping installation"); + return; + } + + // Since hook installation is a manual task triggered by the user, + // using synchronized locking is acceptable. It ensures thread safety + // without affecting overall performance, and provides a simple and reliable solution. + synchronized (LOCK) { + if (installing) { + logger.warn("Parallel Spotless Git pre-push hook installation detected, skipping installation"); + return; + } + + try { + installing = true; + doInstall(); + } finally { + installing = false; + } + } + } + /** * Installs the Git pre-push hook into the repository. * @@ -70,7 +114,7 @@ public GitPrePushHookInstaller(GitPreHookLogger logger, File root) { * * @throws Exception if any error occurs during installation. */ - public void install() throws Exception { + private void doInstall() throws Exception { logger.info("Installing git pre-push hook"); if (!isGitInstalled()) { diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 9e25b1d3d3..91e4547030 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -5,12 +5,14 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( ## [Unreleased] ### Changed * **BREAKING** Bump the required Gradle to `7.3` and required Java to `17`. ([#2375](https://github.com/diffplug/spotless/issues/2375), [#2540](https://github.com/diffplug/spotless/pull/2540)) +* **BREAKING** `spotlessInstallGitPrePushHook` task is now installed only on the root project. ([#2570](https://github.com/diffplug/spotless/pull/2570)) * Bump JGit from `6.10.1` to `7.3.0` ([#2257](https://github.com/diffplug/spotless/pull/2257)) * Adds support for worktrees (fixes [#1765](https://github.com/diffplug/spotless/issues/1765)) * Bump default `google-java-format` version to latest `1.24.0` -> `1.28.0`. ([#2345](https://github.com/diffplug/spotless/pull/2345)) * Bump default `ktlint` version to latest `1.5.0` -> `1.7.1`. ([#2555](https://github.com/diffplug/spotless/pull/2555)) ### Fixed * Respect system gitconfig when performing git operations ([#2404](https://github.com/diffplug/spotless/issues/2404)) +* `spotlessInstallGitPrePushHook` is now compatible with configuration cache. ([#2570](https://github.com/diffplug/spotless/pull/2570)) ## [7.2.1] - 2025-07-21 ### Fixed diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java index 53b7b011a1..2e4e7f1202 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java @@ -41,6 +41,8 @@ public SpotlessExtensionImpl(Project project) { rootInstallPreHook = project.getTasks().register(EXTENSION + INSTALL_GIT_PRE_PUSH_HOOK, SpotlessInstallPrePushHookTask.class, task -> { task.setGroup(BUILD_SETUP_TASK_GROUP); task.setDescription(INSTALL_GIT_PRE_PUSH_HOOK_DESCRIPTION); + task.getRootDir().set(project.getRootDir()); + task.getIsRootExecution().set(project.equals(project.getRootProject())); }); project.afterEvaluate(unused -> { diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTask.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTask.java index 3e59e94b0a..6717f121a8 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTask.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTask.java @@ -15,7 +15,11 @@ */ package com.diffplug.gradle.spotless; +import java.io.File; + import org.gradle.api.DefaultTask; +import org.gradle.api.provider.Property; +import org.gradle.api.tasks.Internal; import org.gradle.api.tasks.TaskAction; import org.gradle.work.DisableCachingByDefault; @@ -30,7 +34,16 @@ *

The task leverages {@link GitPrePushHookInstallerGradle} to implement the installation process. */ @DisableCachingByDefault(because = "not worth caching") -public class SpotlessInstallPrePushHookTask extends DefaultTask { +public abstract class SpotlessInstallPrePushHookTask extends DefaultTask { + + @Internal + abstract Property getRootDir(); + + /** + * Determines whether this task is being executed from the root project. + */ + @Internal + abstract Property getIsRootExecution(); /** * Executes the task to install the Git pre-push hook. @@ -43,6 +56,12 @@ public class SpotlessInstallPrePushHookTask extends DefaultTask { */ @TaskAction public void performAction() throws Exception { + // if is not root project, skip it + if (!getIsRootExecution().get()) { + getLogger().debug("Skipping Spotless pre-push hook installation because it is not being executed from the root project."); + return; + } + final var logger = new GitPreHookLogger() { @Override public void info(String format, Object... arguments) { @@ -60,7 +79,7 @@ public void error(String format, Object... arguments) { } }; - final var installer = new GitPrePushHookInstallerGradle(logger, getProject().getRootDir()); + final var installer = new GitPrePushHookInstallerGradle(logger, getRootDir().get()); installer.install(); } } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTaskTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTaskTest.java index d5ff385b82..f739ab0c63 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTaskTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTaskTest.java @@ -35,7 +35,7 @@ public void should_create_pre_hook_file_when_hook_file_does_not_exists() throws // when var output = gradleRunner() - .withArguments("spotlessInstallGitPrePushHook") + .withArguments("spotlessInstallGitPrePushHook", "--system-prop=org.gradle.configuration-cache=true") .build() .getOutput(); @@ -65,7 +65,7 @@ public void should_append_to_existing_pre_hook_file_when_hook_file_exists() thro // when final var output = gradleRunner() - .withArguments("spotlessInstallGitPrePushHook") + .withArguments("spotlessInstallGitPrePushHook", "--system-prop=org.gradle.configuration-cache=true") .build() .getOutput(); diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md index f6f6bd14fa..4856471f26 100644 --- a/plugin-maven/CHANGES.md +++ b/plugin-maven/CHANGES.md @@ -5,6 +5,7 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( ## [Unreleased] ### Changes * **BREAKING** Bump the required Java to `17`. ([#2375](https://github.com/diffplug/spotless/issues/2375), [#2540](https://github.com/diffplug/spotless/pull/2540)) +* **BREAKING** `spotless:install-git-pre-push-hook` task is now always installed in the root `.git/hooks` directory by resolving the top-level project base directory. ([#2570](https://github.com/diffplug/spotless/pull/2570)) * Bump JGit from `6.10.1` to `7.3.0` ([#2257](https://github.com/diffplug/spotless/pull/2257)) * Adds support for worktrees (fixes [#1765](https://github.com/diffplug/spotless/issues/1765)) * Bump default `google-java-format` version to latest `1.24.0` -> `1.28.0`. ([#2345](https://github.com/diffplug/spotless/pull/2345)) diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessInstallPrePushHookMojo.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessInstallPrePushHookMojo.java index 984724a688..81a9f0403a 100644 --- a/plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessInstallPrePushHookMojo.java +++ b/plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessInstallPrePushHookMojo.java @@ -15,13 +15,12 @@ */ package com.diffplug.spotless.maven; -import java.io.File; - import org.apache.maven.plugin.AbstractMojo; import org.apache.maven.plugin.MojoExecutionException; import org.apache.maven.plugin.MojoFailureException; import org.apache.maven.plugins.annotations.Mojo; import org.apache.maven.plugins.annotations.Parameter; +import org.apache.maven.project.MavenProject; import com.diffplug.spotless.GitPrePushHookInstaller.GitPreHookLogger; import com.diffplug.spotless.GitPrePushHookInstallerMaven; @@ -37,12 +36,8 @@ @Mojo(name = AbstractSpotlessMojo.GOAL_PRE_PUSH_HOOK, threadSafe = true) public class SpotlessInstallPrePushHookMojo extends AbstractMojo { - /** - * The base directory of the Maven project where the Git pre-push hook will be installed. - * This parameter is automatically set to the root directory of the current project. - */ - @Parameter(defaultValue = "${project.basedir}", readonly = true, required = true) - private File baseDir; + @Parameter(defaultValue = "${project}", readonly = true, required = true) + private MavenProject project; /** * Executes the Mojo, installing the Git pre-push hook for the Spotless plugin. @@ -56,6 +51,12 @@ public class SpotlessInstallPrePushHookMojo extends AbstractMojo { */ @Override public void execute() throws MojoExecutionException, MojoFailureException { + // if is not root project, skip it + if (!project.isExecutionRoot()) { + getLog().debug("Skipping Spotless pre-push hook installation for non-root project: " + project.getName()); + return; + } + final var logger = new GitPreHookLogger() { @Override public void info(String format, Object... arguments) { @@ -74,7 +75,7 @@ public void error(String format, Object... arguments) { }; try { - final var installer = new GitPrePushHookInstallerMaven(logger, baseDir); + final var installer = new GitPrePushHookInstallerMaven(logger, project.getBasedir()); installer.install(); } catch (Exception e) { throw new MojoExecutionException("Unable to install pre-push hook", e); diff --git a/testlib/src/test/java/com/diffplug/spotless/GitPrePushHookInstallerTest.java b/testlib/src/test/java/com/diffplug/spotless/GitPrePushHookInstallerTest.java index 0ac3fa2c1c..b45ebfcde0 100644 --- a/testlib/src/test/java/com/diffplug/spotless/GitPrePushHookInstallerTest.java +++ b/testlib/src/test/java/com/diffplug/spotless/GitPrePushHookInstallerTest.java @@ -17,10 +17,12 @@ import static com.diffplug.spotless.GitPrePushHookInstaller.Executor.GRADLE; import static com.diffplug.spotless.GitPrePushHookInstaller.Executor.MAVEN; +import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; -import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.stream.IntStream; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -31,7 +33,7 @@ class GitPrePushHookInstallerTest extends ResourceHarness { private final static String OS = System.getProperty("os.name"); - private final List logs = new ArrayList<>(); + private final List logs = new CopyOnWriteArrayList<>(); private final GitPreHookLogger logger = new GitPreHookLogger() { @Override public void info(String format, Object... arguments) { @@ -52,6 +54,11 @@ public void error(String format, Object... arguments) { @BeforeEach public void beforeEach() { System.setProperty("os.name", "linux"); + + final var hookFile = newFile(".git/hooks/pre-push"); + if (hookFile.exists()) { + hookFile.delete(); + } } @AfterEach @@ -68,9 +75,9 @@ public void should_not_create_pre_hook_file_when_git_is_not_installed() throws E gradle.install(); // then - assertThat(logs).hasSize(2); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git not found in root directory"); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git not found in root directory"); assertThat(newFile(".git/hooks/pre-push")).doesNotExist(); } @@ -84,11 +91,11 @@ public void should_use_global_gradle_when_gradlew_is_not_installed() throws Exce gradle.install(); // then - assertThat(logs).hasSize(4); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook not found, creating it"); - assertThat(logs).element(2).isEqualTo("Local gradle wrapper (gradlew) not found, falling back to global command 'gradle'"); - assertThat(logs).element(3).isEqualTo("Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook not found, creating it", + "Local gradle wrapper (gradlew) not found, falling back to global command 'gradle'", + "Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); final var content = gradleHookContent("git_pre_hook/pre-push.created-tpl", ExecutorType.GLOBAL); assertFile(".git/hooks/pre-push").hasContent(content); @@ -108,10 +115,10 @@ public void should_reinstall_pre_hook_file_when_hook_already_installed() throws gradle.install(); // then - assertThat(logs).hasSize(3); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook already installed, reinstalling it"); - assertThat(logs).element(2).isEqualTo("Git pre-push hook installed successfully to the file " + hookFile.getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook already installed, reinstalling it", + "Git pre-push hook installed successfully to the file " + hookFile.getAbsolutePath()); final var content = gradleHookContent("git_pre_hook/pre-push.existing-installed-end-tpl", ExecutorType.WRAPPER); assertFile(".git/hooks/pre-push").hasContent(content); @@ -131,10 +138,10 @@ public void should_reinstall_pre_hook_file_when_hook_already_installed_in_the_mi gradle.install(); // then - assertThat(logs).hasSize(3); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook already installed, reinstalling it"); - assertThat(logs).element(2).isEqualTo("Git pre-push hook installed successfully to the file " + hookFile.getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook already installed, reinstalling it", + "Git pre-push hook installed successfully to the file " + hookFile.getAbsolutePath()); final var content = gradleHookContent("git_pre_hook/pre-push.existing-reinstalled-middle-tpl", ExecutorType.WRAPPER); assertFile(".git/hooks/pre-push").hasContent(content); @@ -171,10 +178,10 @@ public void should_create_pre_hook_file_when_hook_file_does_not_exists() throws gradle.install(); // then - assertThat(logs).hasSize(3); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook not found, creating it"); - assertThat(logs).element(2).isEqualTo("Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook not found, creating it", + "Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); final var content = gradleHookContent("git_pre_hook/pre-push.created-tpl", ExecutorType.WRAPPER); assertFile(".git/hooks/pre-push").hasContent(content); @@ -192,9 +199,9 @@ public void should_append_to_existing_pre_hook_file_when_hook_file_exists() thro gradle.install(); // then - assertThat(logs).hasSize(2); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); final var content = gradleHookContent("git_pre_hook/pre-push.existing-installed-end-tpl", ExecutorType.WRAPPER); assertFile(".git/hooks/pre-push").hasContent(content); @@ -211,10 +218,10 @@ public void should_create_pre_hook_file_for_maven_when_hook_file_does_not_exists gradle.install(); // then - assertThat(logs).hasSize(3); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook not found, creating it"); - assertThat(logs).element(2).isEqualTo("Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook not found, creating it", + "Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); final var content = mavenHookContent("git_pre_hook/pre-push.created-tpl", ExecutorType.WRAPPER); assertFile(".git/hooks/pre-push").hasContent(content); @@ -230,11 +237,11 @@ public void should_use_global_maven_when_maven_wrapper_is_not_installed() throws gradle.install(); // then - assertThat(logs).hasSize(4); - assertThat(logs).element(0).isEqualTo("Installing git pre-push hook"); - assertThat(logs).element(1).isEqualTo("Git pre-push hook not found, creating it"); - assertThat(logs).element(2).isEqualTo("Local maven wrapper (mvnw) not found, falling back to global command 'mvn'"); - assertThat(logs).element(3).isEqualTo("Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); + assertThat(logs).containsExactly( + "Installing git pre-push hook", + "Git pre-push hook not found, creating it", + "Local maven wrapper (mvnw) not found, falling back to global command 'mvn'", + "Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); final var content = mavenHookContent("git_pre_hook/pre-push.created-tpl", ExecutorType.GLOBAL); assertFile(".git/hooks/pre-push").hasContent(content); @@ -334,6 +341,29 @@ public void should_use_gradle_global_when_bat_and_cmd_files_not_exists_for_windo assertThat(hook).contains("SPOTLESS_EXECUTOR=gradle"); } + @Test + public void should_handle_parallel_installation() { + // given + setFile(".git/config").toContent(""); + + // when + parallelRun(() -> { + final var gradle = new GitPrePushHookInstallerGradle(logger, rootFolder()); + gradle.install(); + }); + + // then + assertThat(logs).contains( + "Installing git pre-push hook", + "Git pre-push hook not found, creating it", + "Parallel Spotless Git pre-push hook installation detected, skipping installation", + "Local gradle wrapper (gradlew) not found, falling back to global command 'gradle'", + "Git pre-push hook installed successfully to the file " + newFile(".git/hooks/pre-push").getAbsolutePath()); + + final var content = gradleHookContent("git_pre_hook/pre-push.created-tpl", ExecutorType.GLOBAL); + assertFile(".git/hooks/pre-push").hasContent(content); + } + private String gradleHookContent(String resourcePath, ExecutorType executorType) { return getTestResource(resourcePath) .replace("${executor}", executorType == ExecutorType.WRAPPER ? "./" + newFile("gradlew").getName() : "gradle") @@ -348,7 +378,31 @@ private String mavenHookContent(String resourcePath, ExecutorType executorType) .replace("${applyCommand}", "spotless:apply"); } + private void parallelRun(ThrowableRun runnable) { + IntStream.range(0, 5) + .mapToObj(i -> new Thread(() -> { + try { + runnable.run(); + } catch (Exception e) { + throw new RuntimeException(e); + } + })) + .peek(Thread::start) + .collect(toList()) + .forEach(t -> { + try { + t.join(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + }); + } + private enum ExecutorType { WRAPPER, GLOBAL } + + private interface ThrowableRun { + void run() throws Exception; + } }