Merge branch 'main' into fix/buf-on-windows

nedtwigg · web-flow · commit 3fa38e50ce24 · 2024-06-04T00:44:54.000-07:00
diff --git a/CHANGES.md b/CHANGES.md
@@ -14,11 +14,13 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 * `FileSignature.Promised` and `JarState.Promised` to facilitate round-trip serialization for the Gradle configuration cache. ([#1945](https://github.com/diffplug/spotless/pull/1945))
 * Respect `.editorconfig` settings for formatting shell via `shfmt` ([#2031](https://github.com/diffplug/spotless/pull/2031))
 ### Fixed
+* Check if ktlint_code_style is set in .editorconfig before overriding it ([#2143](https://github.com/diffplug/spotless/issues/2143))
 * Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))
 * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052))
 * Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096))
 * scalafmt.conf fileOverride section now works correctly ([#1854](https://github.com/diffplug/spotless/pull/1854))
 * Fix stdin pipe is being closed exception on Windows for large .proto files ([#2147](https://github.com/diffplug/spotless/issues/2147))
+* Reworked ShadowCopy (`npmInstallCache`) to use atomic filesystem operations, resolving several race conditions that could arise ([#2151](https://github.com/diffplug/spotless/pull/2151))
 ### Changes
 * Bump default `cleanthat` version to latest `2.16` -> `2.20`. ([#1725](https://github.com/diffplug/spotless/pull/1725))
 * Bump default `gherkin-utils` version to latest `8.0.2` -> `9.0.0`. ([#1703](https://github.com/diffplug/spotless/pull/1703))
diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties
@@ -1,6 +1,6 @@
 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-8.7-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-8.8-bin.zip
 networkTimeout=10000
 validateDistributionUrl=true
 zipStoreBase=GRADLE_USER_HOME
diff --git a/gradlew b/gradlew
@@ -55,7 +55,7 @@
 #       Darwin, MinGW, and NonStop.
 #
 #   (3) This script is generated from the Groovy template
-#       https://github.com/gradle/gradle/blob/HEAD/subprojects/plugins/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt
+#       https://github.com/gradle/gradle/blob/HEAD/platforms/jvm/plugins-application/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt
 #       within the Gradle project.
 #
 #       You can find Gradle at https://github.com/gradle/gradle/.
diff --git a/lib/src/compatKtLint1Dot0Dot0/java/com/diffplug/spotless/glue/ktlint/compat/KtLintCompat1Dot0Dot0Adapter.java b/lib/src/compatKtLint1Dot0Dot0/java/com/diffplug/spotless/glue/ktlint/compat/KtLintCompat1Dot0Dot0Adapter.java
@@ -93,20 +93,21 @@ public String format(
 				.flatMap(loader -> loader.get().getRuleProviders().stream())
 				.collect(Collectors.toUnmodifiableSet());
 
-		EditorConfigOverride editorConfigOverride;
-		if (editorConfigOverrideMap.isEmpty()) {
-			editorConfigOverride = EditorConfigOverride.Companion.getEMPTY_EDITOR_CONFIG_OVERRIDE();
-		} else {
-			editorConfigOverride = createEditorConfigOverride(allRuleProviders.stream().map(
-					RuleProvider::createNewRuleInstance).collect(Collectors.toList()),
-					editorConfigOverrideMap);
-		}
 		EditorConfigDefaults editorConfig;
 		if (editorConfigPath == null || !Files.exists(editorConfigPath)) {
 			editorConfig = EditorConfigDefaults.Companion.getEMPTY_EDITOR_CONFIG_DEFAULTS();
 		} else {
 			editorConfig = EditorConfigDefaults.Companion.load(editorConfigPath, RuleProviderKt.propertyTypes(allRuleProviders));
 		}
+		EditorConfigOverride editorConfigOverride;
+		if (editorConfigOverrideMap.isEmpty()) {
+			editorConfigOverride = EditorConfigOverride.Companion.getEMPTY_EDITOR_CONFIG_OVERRIDE();
+		} else {
+			editorConfigOverride = createEditorConfigOverride(
+					editorConfig,
+					allRuleProviders.stream().map(RuleProvider::createNewRuleInstance).collect(Collectors.toList()),
+					editorConfigOverrideMap);
+		}
 
 		return new KtLintRuleEngine(
 				allRuleProviders,
@@ -120,7 +121,7 @@ public String format(
 	/**
 	 * Create EditorConfigOverride from user provided parameters.
 	 */
-	private static EditorConfigOverride createEditorConfigOverride(final List<Rule> rules, Map<String, Object> editorConfigOverrideMap) {
+	private static EditorConfigOverride createEditorConfigOverride(final EditorConfigDefaults editorConfig, final List<Rule> rules, Map<String, Object> editorConfigOverrideMap) {
 		// Get properties from rules in the rule sets
 		Stream<EditorConfigProperty<?>> ruleProperties = rules.stream()
 				.flatMap(rule -> rule.getUsesEditorConfigProperties().stream());
@@ -132,7 +133,9 @@ private static EditorConfigOverride createEditorConfigOverride(final List<Rule>
 				.collect(Collectors.toMap(EditorConfigProperty::getName, property -> property));
 
 		// The default style had been changed from intellij_idea to ktlint_official in version 1.0.0
-		if (!editorConfigOverrideMap.containsKey("ktlint_code_style")) {
+		boolean isCodeStyleDefinedInEditorConfig = editorConfig.getValue().getSections().stream()
+				.anyMatch(section -> section.getProperties().containsKey("ktlint_code_style"));
+		if (!isCodeStyleDefinedInEditorConfig && !editorConfigOverrideMap.containsKey("ktlint_code_style")) {
 			editorConfigOverrideMap.put("ktlint_code_style", "intellij_idea");
 		}
 
diff --git a/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java b/lib/src/main/java/com/diffplug/spotless/npm/ShadowCopy.java
@@ -17,16 +17,17 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.AtomicMoveNotSupportedException;
+import java.nio.file.DirectoryNotEmptyException;
 import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.FileSystemException;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.SimpleFileVisitor;
+import java.nio.file.StandardCopyOption;
 import java.nio.file.attribute.BasicFileAttributes;
-import java.time.Duration;
-import java.util.concurrent.TimeoutException;
 import java.util.function.Supplier;
 
 import javax.annotation.Nonnull;
@@ -36,6 +37,8 @@
 
 import com.diffplug.spotless.ThrowingEx;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 class ShadowCopy {
 
 	private static final Logger logger = LoggerFactory.getLogger(ShadowCopy.class);
@@ -55,70 +58,67 @@ private File shadowCopyRoot() {
 	}
 
 	public void addEntry(String key, File orig) {
-		// prevent concurrent adding of entry with same key
-		if (!reserveSubFolder(key)) {
-			logger.debug("Shadow copy entry already in progress: {}. Awaiting finalization.", key);
+		File target = entry(key, orig.getName());
+		if (target.exists()) {
+			logger.debug("Shadow copy entry already exists, not overwriting: {}", key);
+		} else {
 			try {
-				NpmResourceHelper.awaitFileDeleted(markerFilePath(key).toFile(), Duration.ofSeconds(120));
-			} catch (TimeoutException e) {
-				throw new RuntimeException(e);
+				storeEntry(key, orig, target);
+			} catch (Throwable ex) {
+				// Log but don't fail
+				logger.warn("Unable to store cache entry for {}", key, ex);
 			}
 		}
+	}
+
+	@SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE")
+	private void storeEntry(String key, File orig, File target) throws IOException {
+		// Create a temp directory in the same directory as target
+		Files.createDirectories(target.toPath().getParent());
+		Path tempDirectory = Files.createTempDirectory(target.toPath().getParent(), key);
+		logger.debug("Will store entry {} to temporary directory {}, which is a sibling of the ultimate target {}", orig, tempDirectory, target);
+
 		try {
-			storeEntry(key, orig);
+			// Copy orig to temp dir
+			Files.walkFileTree(orig.toPath(), new CopyDirectoryRecursively(tempDirectory, orig.toPath()));
+			try {
+				logger.debug("Finished storing entry {}. Atomically moving temporary directory {} into final place {}", key, tempDirectory, target);
+				// Atomically rename the completed cache entry into place
+				Files.move(tempDirectory, target.toPath(), StandardCopyOption.ATOMIC_MOVE);
+			} catch (FileAlreadyExistsException | DirectoryNotEmptyException e) {
+				// Someone already beat us to it
+				logger.debug("Shadow copy entry now exists, not overwriting: {}", key);
+			} catch (AtomicMoveNotSupportedException e) {
+				logger.warn("The filesystem at {} does not support atomic moves. Spotless cannot safely cache on such a system due to race conditions. Caching has been skipped.", target.toPath().getParent(), e);
+			}
 		} finally {
-			cleanupReservation(key);
+			// Best effort to clean up
+			if (Files.exists(tempDirectory)) {
+				try {
+					Files.walkFileTree(tempDirectory, new DeleteDirectoryRecursively());
+				} catch (Throwable ex) {
+					logger.warn("Ignoring error while cleaning up temporary copy", ex);
+				}
+			}
 		}
 	}
 
 	public File getEntry(String key, String fileName) {
 		return entry(key, fileName);
 	}
 
-	private void storeEntry(String key, File orig) {
-		File target = entry(key, orig.getName());
-		if (target.exists()) {
-			logger.debug("Shadow copy entry already exists: {}", key);
-			// delete directory "target" recursively
-			// https://stackoverflow.com/questions/3775694/deleting-folder-from-java
-			ThrowingEx.run(() -> Files.walkFileTree(target.toPath(), new DeleteDirectoryRecursively()));
-		}
-		// copy directory "orig" to "target" using hard links if possible or a plain copy otherwise
-		ThrowingEx.run(() -> Files.walkFileTree(orig.toPath(), new CopyDirectoryRecursively(target, orig)));
-	}
-
-	private void cleanupReservation(String key) {
-		ThrowingEx.run(() -> Files.delete(markerFilePath(key)));
-	}
-
-	private Path markerFilePath(String key) {
-		return Paths.get(shadowCopyRoot().getAbsolutePath(), key + ".marker");
-	}
-
 	private File entry(String key, String origName) {
 		return Paths.get(shadowCopyRoot().getAbsolutePath(), key, origName).toFile();
 	}
 
-	private boolean reserveSubFolder(String key) {
-		// put a marker file named "key".marker in "shadowCopyRoot" to make sure no other process is using it or return false if it already exists
-		try {
-			Files.createFile(Paths.get(shadowCopyRoot().getAbsolutePath(), key + ".marker"));
-			return true;
-		} catch (FileAlreadyExistsException e) {
-			return false;
-		} catch (IOException e) {
-			throw new RuntimeException(e);
-		}
-	}
-
 	public File copyEntryInto(String key, String origName, File targetParentFolder) {
 		File target = Paths.get(targetParentFolder.getAbsolutePath(), origName).toFile();
 		if (target.exists()) {
 			logger.warn("Shadow copy destination already exists, deleting! {}: {}", key, target);
 			ThrowingEx.run(() -> Files.walkFileTree(target.toPath(), new DeleteDirectoryRecursively()));
 		}
 		// copy directory "orig" to "target" using hard links if possible or a plain copy otherwise
-		ThrowingEx.run(() -> Files.walkFileTree(entry(key, origName).toPath(), new CopyDirectoryRecursively(target, entry(key, origName))));
+		ThrowingEx.run(() -> Files.walkFileTree(entry(key, origName).toPath(), new CopyDirectoryRecursively(target.toPath(), entry(key, origName).toPath())));
 		return target;
 	}
 
@@ -127,20 +127,20 @@ public boolean entryExists(String key, String origName) {
 	}
 
 	private static class CopyDirectoryRecursively extends SimpleFileVisitor<Path> {
-		private final File target;
-		private final File orig;
+		private final Path target;
+		private final Path orig;
 
 		private boolean tryHardLink = true;
 
-		public CopyDirectoryRecursively(File target, File orig) {
+		public CopyDirectoryRecursively(Path target, Path orig) {
 			this.target = target;
 			this.orig = orig;
 		}
 
 		@Override
 		public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException {
 			// create directory on target
-			Files.createDirectories(target.toPath().resolve(orig.toPath().relativize(dir)));
+			Files.createDirectories(target.resolve(orig.relativize(dir)));
 			return super.preVisitDirectory(dir, attrs);
 		}
 
@@ -149,7 +149,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
 			// first try to hardlink, if that fails, copy
 			if (tryHardLink) {
 				try {
-					Files.createLink(target.toPath().resolve(orig.toPath().relativize(file)), file);
+					Files.createLink(target.resolve(orig.relativize(file)), file);
 					return super.visitFile(file, attrs);
 				} catch (UnsupportedOperationException | SecurityException | FileSystemException e) {
 					logger.debug("Shadow copy entry does not support hard links: {}. Switching to 'copy'.", file, e);
@@ -160,11 +160,12 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
 				}
 			}
 			// copy file to target
-			Files.copy(file, target.toPath().resolve(orig.toPath().relativize(file)));
+			Files.copy(file, target.resolve(orig.relativize(file)));
 			return super.visitFile(file, attrs);
 		}
 	}
 
+	// https://stackoverflow.com/questions/3775694/deleting-folder-from-java
 	private static class DeleteDirectoryRecursively extends SimpleFileVisitor<Path> {
 		@Override
 		public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md
@@ -9,13 +9,15 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 * Respect `.editorconfig` settings for formatting shell via `shfmt` ([#2031](https://github.com/diffplug/spotless/pull/2031))
 * Add support for formatting and sorting Maven POMs ([#2082](https://github.com/diffplug/spotless/issues/2082))
 ### Fixed
+* Check if ktlint_code_style is set in .editorconfig before overriding it ([#2143](https://github.com/diffplug/spotless/issues/2143))
 * Full no-asterisk support for configuration cache ([#2088](https://github.com/diffplug/spotless/pull/2088) closes [#1274](https://github.com/diffplug/spotless/issues/1274) and [#987](https://github.com/diffplug/spotless/issues/987)).
 * Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))
 * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052))
 * Fixed memory leak introduced in 6.21.0 ([#2067](https://github.com/diffplug/spotless/issues/2067))
 * Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096))
 * scalafmt.conf fileOverride section now works correctly ([#1854](https://github.com/diffplug/spotless/pull/1854))
 * Fix stdin pipe is being closed exception on Windows for large .proto files ([#2147](https://github.com/diffplug/spotless/issues/2147))
+* Reworked ShadowCopy (`npmInstallCache`) to use atomic filesystem operations, resolving several race conditions that could arise ([#2151](https://github.com/diffplug/spotless/pull/2151))
 ### Changes
 * Bump default `cleanthat` version to latest `2.16` -> `2.20`. ([#1725](https://github.com/diffplug/spotless/pull/1725))
 * Bump default `gherkin-utils` version to latest `8.0.2` -> `9.0.0`. ([#1703](https://github.com/diffplug/spotless/pull/1703))
diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/KotlinExtensionTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/KotlinExtensionTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 DiffPlug
+ * Copyright 2016-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -127,6 +127,25 @@ void testReadCodeStyleFromEditorConfigFile() throws IOException {
 		checkKtlintOfficialStyle();
 	}
 
+	@Test
+	void testEditorConfigOverrideWithUnsetCodeStyleDoesNotOverrideEditorConfigCodeStyleWithDefault() throws IOException {
+		setFile(".editorconfig").toResource("kotlin/ktlint/ktlint_official/.editorconfig");
+		setFile("build.gradle").toLines(
+				"plugins {",
+				"    id 'org.jetbrains.kotlin.jvm' version '1.6.21'",
+				"    id 'com.diffplug.spotless'",
+				"}",
+				"repositories { mavenCentral() }",
+				"spotless {",
+				"    kotlin {",
+				"        ktlint().editorConfigOverride([",
+				"	         ktlint_test_key: true,",
+				"        ])",
+				"    }",
+				"}");
+		checkKtlintOfficialStyle();
+	}
+
 	@Test
 	void testSetEditorConfigCanOverrideEditorConfigFile() throws IOException {
 		setFile(".editorconfig").toResource("kotlin/ktlint/intellij_idea/.editorconfig");
diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md
@@ -7,11 +7,14 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
 * Respect `.editorconfig` settings for formatting shell via `shfmt` ([#2031](https://github.com/diffplug/spotless/pull/2031))
 * Skip execution in M2E (incremental) builds by default ([#1814](https://github.com/diffplug/spotless/issues/1814), [#2037](https://github.com/diffplug/spotless/issues/2037))
 ### Fixed
+* Check if ktlint_code_style is set in .editorconfig before overriding it ([#2143](https://github.com/diffplug/spotless/issues/2143))
+* Default EditorConfig path to ".editorconfig" ([#2143](https://github.com/diffplug/spotless/issues/2143))
 * Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))
 * Correctly provide EditorConfig property types for Ktlint ([#2052](https://github.com/diffplug/spotless/issues/2052))
 * Made ShadowCopy (`npmInstallCache`) more robust by re-creating the cache dir if it goes missing ([#1984](https://github.com/diffplug/spotless/issues/1984),[2096](https://github.com/diffplug/spotless/pull/2096))
 * scalafmt.conf fileOverride section now works correctly ([#1854](https://github.com/diffplug/spotless/pull/1854))
 * Fix stdin pipe is being closed exception on Windows for large .proto files ([#2147](https://github.com/diffplug/spotless/issues/2147))
+* Reworked ShadowCopy (`npmInstallCache`) to use atomic filesystem operations, resolving several race conditions that could arise ([#2151](https://github.com/diffplug/spotless/pull/2151))
 ### Changes
 * Bump default `cleanthat` version to latest `2.16` -> `2.20`. ([#1725](https://github.com/diffplug/spotless/pull/1725))
 * Bump default `gherkin-utils` version to latest `8.0.2` -> `9.0.0`. ([#1703](https://github.com/diffplug/spotless/pull/1703))
diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/kotlin/Ktlint.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/kotlin/Ktlint.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 DiffPlug
+ * Copyright 2016-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,6 +15,7 @@
  */
 package com.diffplug.spotless.maven.kotlin;
 
+import java.io.File;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -30,6 +31,8 @@
 import com.diffplug.spotless.maven.FormatterStepFactory;
 
 public class Ktlint implements FormatterStepFactory {
+	private static final File defaultEditorConfig = new File(".editorconfig");
+
 	@Parameter
 	private String version;
 	@Parameter
@@ -43,6 +46,9 @@ public class Ktlint implements FormatterStepFactory {
 	public FormatterStep newFormatterStep(final FormatterStepConfig stepConfig) {
 		String ktlintVersion = version != null ? version : KtLintStep.defaultVersion();
 		FileSignature configPath = null;
+		if (editorConfigPath == null && defaultEditorConfig.exists()) {
+			editorConfigPath = defaultEditorConfig.getPath();
+		}
 		if (editorConfigPath != null) {
 			configPath = ThrowingEx.get(() -> FileSignature.signAsList(stepConfig.getFileLocator().locateFile(editorConfigPath)));
 		}
diff --git a/plugin-maven/src/test/java/com/diffplug/spotless/maven/kotlin/KtlintTest.java b/plugin-maven/src/test/java/com/diffplug/spotless/maven/kotlin/KtlintTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 DiffPlug
+ * Copyright 2016-2024 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -55,6 +55,17 @@ void testReadCodeStyleFromEditorConfigFile() throws Exception {
 		checkKtlintOfficialStyle();
 	}
 
+	@Test
+	void testEditorConfigOverrideWithUnsetCodeStyleDoesNotOverrideEditorConfigCodeStyleWithDefault() throws Exception {
+		setFile(".editorconfig").toResource("kotlin/ktlint/ktlint_official/.editorconfig");
+		writePomWithKotlinSteps("<ktlint>\n" +
+				"  <editorConfigOverride>\n" +
+				"    <ktlint_test_key>true</ktlint_test_key>\n" +
+				"  </editorConfigOverride>\n" +
+				"</ktlint>");
+		checkKtlintOfficialStyle();
+	}
+
 	@Test
 	void testSetEditorConfigCanOverrideEditorConfigFile() throws Exception {
 		setFile(".editorconfig").toResource("kotlin/ktlint/intellij_idea/.editorconfig");
diff --git a/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java b/testlib/src/test/java/com/diffplug/spotless/npm/ShadowCopyTest.java

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@`
`55`	`55`	`# Darwin, MinGW, and NonStop.`
`56`	`56`	`#`
`57`	`57`	`# (3) This script is generated from the Groovy template`
`58`		`-# https://github.com/gradle/gradle/blob/HEAD/subprojects/plugins/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt`
	`58`	`+# https://github.com/gradle/gradle/blob/HEAD/platforms/jvm/plugins-application/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt`
`59`	`59`	`# within the Gradle project.`
`60`	`60`	`#`
`61`	`61`	`# You can find Gradle at https://github.com/gradle/gradle/.`