Skip to content

git pre push hook gradle config cache fix + parallel execution handling #2570

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+172 −48
Open

git pre push hook gradle config cache fix + parallel execution handling #2570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9ee283c
git pre push hook gradle config cache fix + parallel execution handling
lazer-dev Jul 22, 2025
4967afd
do not execute for non root
lazer-dev Jul 22, 2025
c9d3047
Update the root changelog.
Jul 24, 2025
87be0ca
Rephrase the project-specific changelogs.
Jul 24, 2025
58c2683
test fix
lazer-dev Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 45 additions & 1 deletion lib/src/main/java/com/diffplug/spotless/GitPrePushHookInstaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
*/
Expand All @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used the simplest strategy: double-checked locking with synchronized. Since this task is meant to be run manually by the user, it’s totally OK. It’s a simple and robust solution that doesn’t affect performance, so in my opinion this is a good approach.
If you disagree or have a better idea, I’m happy to hear it!

One more corner case related to parallel execution: the current solution prevents parallel file modification within a single JVM process.
However, if a user runs Gradle in multiple consoles at the same time, we could end up with parallel creation/editing of the same file with a few Gradle processes...
That could be handled using a file lock on the OS level with Java's FileChannel, but I’m not sure we need to worry about this case — it’s very unlikely to happen in practice.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not worth the complexity, managing within a single process is good enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nedtwigg
If there are no other concerns or comments, can we go ahead and merge this?

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.
*
Expand All @@ -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()) {
Expand Down
2 changes: 2 additions & 0 deletions plugin-gradle/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtensionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 -> {
Expand Down
23 changes: 21 additions & 2 deletions plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -30,7 +34,16 @@
* <p>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<File> getRootDir();

/**
* Determines whether this task is being executed from the root project.
*/
@Internal
abstract Property<Boolean> getIsRootExecution();

/**
* Executes the task to install the Git pre-push hook.
Expand All @@ -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) {
Expand All @@ -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();
}
}
4 changes: 2 additions & 2 deletions ...gradle/src/test/java/com/diffplug/gradle/spotless/SpotlessInstallPrePushHookTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down
1 change: 1 addition & 0 deletions plugin-maven/CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
19 changes: 10 additions & 9 deletions plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessInstallPrePushHookMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand All @@ -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) {
Expand All @@ -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);
Expand Down
Loading
Loading