diff --git a/CHANGES.md b/CHANGES.md index f3f4939af5..2e292b9227 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,10 @@ This document is intended for Spotless developers. We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +### Added +* `FileSignature.machineIsWin()`, to replace the now-deprecated `LineEnding.nativeIsWin()`, because it turns out that `\r\n` is [not a reliable way](https://github.com/diffplug/spotless/issues/559#issuecomment-653739898) to detect the windows OS ([#639](https://github.com/diffplug/spotless/pull/639)). +### Fixed +* `GitAttributesLineEndings` was fatally broken (always returned platform default), and our tests missed it because they tested the part before the broken part ([#639](https://github.com/diffplug/spotless/pull/639)). ## [2.0.0] - 2020-07-02 ### Changed diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java index 630f4bacb4..bd7cbd992c 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java +++ b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitAttributesLineEndings.java @@ -113,16 +113,17 @@ static class CachedEndings implements Serializable { for (File file : toFormat) { String ending = runtime.getEndingFor(file); if (!ending.equals(defaultEnding)) { - String path = FileSignature.pathNativeToUnix(file.getAbsolutePath()); - hasNonDefaultEnding.put(path, ending); + String absPath = FileSignature.pathNativeToUnix(file.getAbsolutePath()); + String subPath = FileSignature.subpath(rootDir, absPath); + hasNonDefaultEnding.put(subPath, ending); } } } /** Returns the line ending appropriate for the given file. */ public String endingFor(File file) { - String path = FileSignature.pathNativeToUnix(file.getAbsolutePath()); - String subpath = FileSignature.subpath(rootDir, path); + String absPath = FileSignature.pathNativeToUnix(file.getAbsolutePath()); + String subpath = FileSignature.subpath(rootDir, absPath); String ending = hasNonDefaultEnding.getValueForExactKey(subpath); return ending == null ? defaultEnding : ending; } diff --git a/lib-extra/src/test/java/com/diffplug/spotless/extra/GitAttributesTest.java b/lib-extra/src/test/java/com/diffplug/spotless/extra/GitAttributesTest.java index ca9d3366c2..baef2a2b71 100644 --- a/lib-extra/src/test/java/com/diffplug/spotless/extra/GitAttributesTest.java +++ b/lib-extra/src/test/java/com/diffplug/spotless/extra/GitAttributesTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2016 DiffPlug + * Copyright 2016-2020 DiffPlug * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,55 +17,74 @@ import java.io.File; import java.io.IOException; -import java.nio.file.Files; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; -import org.junit.Assert; -import org.junit.Rule; +import org.assertj.core.api.Assertions; import org.junit.Test; -import org.junit.rules.TemporaryFolder; -public class GitAttributesTest { - @Rule - public TemporaryFolder folder = new TemporaryFolder(); +import com.diffplug.common.base.Errors; +import com.diffplug.common.base.StringPrinter; +import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.ResourceHarness; - private void write(String path, String... content) throws IOException { - File file = file(path); - file.getParentFile().mkdirs(); - Files.write(file.toPath(), Arrays.asList(content)); +public class GitAttributesTest extends ResourceHarness { + private List testFiles() { + try { + List result = new ArrayList<>(); + for (String path : TEST_PATHS) { + setFile(path).toContent(""); + result.add(newFile(path)); + } + return result; + } catch (IOException e) { + throw Errors.asRuntime(e); + } } - private File file(String path) { - return new File(folder.getRoot(), path); - } + private static List TEST_PATHS = Arrays.asList("someFile", "subfolder/someFile", "MANIFEST.MF", "subfolder/MANIFEST.MF"); @Test public void cacheTest() throws IOException { - write(".gitattributes", "* eol=lf", "*.MF eol=crlf"); + setFile(".gitattributes").toContent(StringPrinter.buildStringFromLines( + "* eol=lf", + "*.MF eol=crlf")); { GitAttributesLineEndings.AttributesCache cache = new GitAttributesLineEndings.AttributesCache(); - Assert.assertEquals("lf", cache.valueFor(file("someFile"), "eol")); - Assert.assertEquals("lf", cache.valueFor(file("subfolder/someFile"), "eol")); - Assert.assertEquals("crlf", cache.valueFor(file("MANIFEST.MF"), "eol")); - Assert.assertEquals("crlf", cache.valueFor(file("subfolder/MANIFEST.MF"), "eol")); + Assertions.assertThat(cache.valueFor(newFile("someFile"), "eol")).isEqualTo("lf"); + Assertions.assertThat(cache.valueFor(newFile("subfolder/someFile"), "eol")).isEqualTo("lf"); + Assertions.assertThat(cache.valueFor(newFile("MANIFEST.MF"), "eol")).isEqualTo("crlf"); + Assertions.assertThat(cache.valueFor(newFile("subfolder/MANIFEST.MF"), "eol")).isEqualTo("crlf"); // write out a .gitattributes for the subfolder - write("subfolder/.gitattributes", "* eol=lf"); + setFile("subfolder/.gitattributes").toContent("* eol=lf"); // it shouldn't change anything, because it's cached - Assert.assertEquals("lf", cache.valueFor(file("someFile"), "eol")); - Assert.assertEquals("lf", cache.valueFor(file("subfolder/someFile"), "eol")); - Assert.assertEquals("crlf", cache.valueFor(file("MANIFEST.MF"), "eol")); - Assert.assertEquals("crlf", cache.valueFor(file("subfolder/MANIFEST.MF"), "eol")); + Assertions.assertThat(cache.valueFor(newFile("someFile"), "eol")).isEqualTo("lf"); + Assertions.assertThat(cache.valueFor(newFile("subfolder/someFile"), "eol")).isEqualTo("lf"); + Assertions.assertThat(cache.valueFor(newFile("MANIFEST.MF"), "eol")).isEqualTo("crlf"); + Assertions.assertThat(cache.valueFor(newFile("subfolder/MANIFEST.MF"), "eol")).isEqualTo("crlf"); } - { // but if we make a new cache, it should change GitAttributesLineEndings.AttributesCache cache = new GitAttributesLineEndings.AttributesCache(); - Assert.assertEquals("lf", cache.valueFor(file("someFile"), "eol")); - Assert.assertEquals("lf", cache.valueFor(file("subfolder/someFile"), "eol")); - Assert.assertEquals("crlf", cache.valueFor(file("MANIFEST.MF"), "eol")); - Assert.assertEquals("lf", cache.valueFor(file("subfolder/MANIFEST.MF"), "eol")); + Assertions.assertThat(cache.valueFor(newFile("someFile"), "eol")).isEqualTo("lf"); + Assertions.assertThat(cache.valueFor(newFile("subfolder/someFile"), "eol")).isEqualTo("lf"); + Assertions.assertThat(cache.valueFor(newFile("MANIFEST.MF"), "eol")).isEqualTo("crlf"); + Assertions.assertThat(cache.valueFor(newFile("subfolder/MANIFEST.MF"), "eol")).isEqualTo("lf"); } } + + @Test + public void policyTest() throws IOException { + setFile(".gitattributes").toContent(StringPrinter.buildStringFromLines( + "* eol=lf", + "*.MF eol=crlf")); + LineEnding.Policy policy = LineEnding.GIT_ATTRIBUTES.createPolicy(rootFolder(), () -> testFiles()); + Assertions.assertThat(policy.getEndingFor(newFile("someFile"))).isEqualTo("\n"); + Assertions.assertThat(policy.getEndingFor(newFile("subfolder/someFile"))).isEqualTo("\n"); + Assertions.assertThat(policy.getEndingFor(newFile("MANIFEST.MF"))).isEqualTo("\r\n"); + Assertions.assertThat(policy.getEndingFor(newFile("subfolder/MANIFEST.MF"))).isEqualTo("\r\n"); + } } diff --git a/lib/src/main/java/com/diffplug/spotless/FileSignature.java b/lib/src/main/java/com/diffplug/spotless/FileSignature.java index 63546df04e..d0a8c2881c 100644 --- a/lib/src/main/java/com/diffplug/spotless/FileSignature.java +++ b/lib/src/main/java/com/diffplug/spotless/FileSignature.java @@ -30,6 +30,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -106,14 +107,21 @@ public File getOnlyFile() { } } + private static boolean machineIsWin = System.getProperty("os.name").toLowerCase(Locale.ROOT).contains("win"); + + /** Returns true if this JVM is running on a windows machine. */ + public static boolean machineIsWin() { + return machineIsWin; + } + /** Transforms a native path to a unix one. */ public static String pathNativeToUnix(String pathNative) { - return LineEnding.nativeIsWin() ? pathNative.replace('\\', '/') : pathNative; + return pathNative.replace(File.separatorChar, '/'); } /** Transforms a unix path to a native one. */ public static String pathUnixToNative(String pathUnix) { - return LineEnding.nativeIsWin() ? pathUnix.replace('/', '\\') : pathUnix; + return pathUnix.replace('/', File.separatorChar); } private static List validateInputFiles(List files) { diff --git a/lib/src/main/java/com/diffplug/spotless/LineEnding.java b/lib/src/main/java/com/diffplug/spotless/LineEnding.java index 1b37b60444..c4af47d3f3 100644 --- a/lib/src/main/java/com/diffplug/spotless/LineEnding.java +++ b/lib/src/main/java/com/diffplug/spotless/LineEnding.java @@ -100,6 +100,13 @@ public String getEndingFor(File file) { private static final Policy _platformNativePolicy = new ConstantLineEndingPolicy(_platformNative); private static final boolean nativeIsWin = _platformNative.equals(WINDOWS.str()); + /** + * @deprecated Using the system-native line endings to detect the windows operating system has turned out + * to be unreliable. Use {@link FileSignature#machineIsWin()} instead. + * + * @see FileSignature#machineIsWin() + */ + @Deprecated public static boolean nativeIsWin() { return nativeIsWin; } diff --git a/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java b/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java index 59077f797e..3066113d3b 100644 --- a/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java +++ b/lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java @@ -31,6 +31,7 @@ import javax.annotation.Nullable; +import com.diffplug.spotless.FileSignature; import com.diffplug.spotless.FormatterFunc; import com.diffplug.spotless.FormatterStep; import com.diffplug.spotless.LineEnding; @@ -288,7 +289,7 @@ private String setLicenseHeaderYearsFromGitHistory(String raw, File file) throws private static String parseYear(String cmd, File file) throws IOException { String fullCmd = cmd + " " + file.getAbsolutePath(); ProcessBuilder builder = new ProcessBuilder().directory(file.getParentFile()); - if (LineEnding.nativeIsWin()) { + if (FileSignature.machineIsWin()) { builder.command("cmd", "/c", fullCmd); } else { builder.command("bash", "-c", fullCmd); diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 1fd229e138..f5e3d72eae 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -5,6 +5,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( **5.x preview:** If you are using Gradle 5.4+, you can preview thew upcoming `com.diffplug.spotless` plugin by adding `-PspotlessModern`, see [CHANGES-5.x-PREVIEW.md](CHANGES-5.x-PREVIEW.md) for details. ## [Unreleased] +### Fixed +* Git-native handling of line endings was broken, now fixed ([#639](https://github.com/diffplug/spotless/pull/639)). ## [4.5.0] - 2020-07-02 ### Added diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java index 45b944a0de..a60232f9f3 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessCheck.java @@ -30,8 +30,8 @@ import org.gradle.api.tasks.PathSensitivity; import org.gradle.api.tasks.TaskAction; +import com.diffplug.spotless.FileSignature; import com.diffplug.spotless.Formatter; -import com.diffplug.spotless.LineEnding; import com.diffplug.spotless.extra.integration.DiffMessageFormatter; public class SpotlessCheck extends DefaultTask { @@ -105,6 +105,6 @@ private String getTaskPathPrefix() { } private static String calculateGradleCommand() { - return LineEnding.nativeIsWin() ? "gradlew.bat" : "./gradlew"; + return FileSignature.machineIsWin() ? "gradlew.bat" : "./gradlew"; } } diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/DiffMessageFormatterTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/DiffMessageFormatterTest.java index 238c43ffe0..c352f95d93 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/DiffMessageFormatterTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/DiffMessageFormatterTest.java @@ -30,6 +30,7 @@ import org.junit.Test; import com.diffplug.common.base.StringPrinter; +import com.diffplug.spotless.FileSignature; import com.diffplug.spotless.FormatterStep; import com.diffplug.spotless.LineEnding; import com.diffplug.spotless.ResourceHarness; @@ -110,7 +111,7 @@ private void assertCheckFailure(Bundle spotless, String... expectedLines) throws Assertions.assertThat(middle).isEqualTo(expectedMessage.substring(0, expectedMessage.length() - 1)); } - static final String EXPECTED_RUN_SPOTLESS_APPLY_SUGGESTION = LineEnding.nativeIsWin() + static final String EXPECTED_RUN_SPOTLESS_APPLY_SUGGESTION = FileSignature.machineIsWin() ? "Run 'gradlew.bat :spotlessApply' to fix these violations." : "Run './gradlew :spotlessApply' to fix these violations."; diff --git a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpecificFilesTest.java b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpecificFilesTest.java index 5e82543f65..75d3077a72 100644 --- a/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpecificFilesTest.java +++ b/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/SpecificFilesTest.java @@ -23,13 +23,13 @@ import org.junit.Test; import org.junit.experimental.categories.Category; -import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.FileSignature; @Category(ExcludeFromPluginGradleModern.class) public class SpecificFilesTest extends GradleIntegrationHarness { private static String regexWinSafe(String input) { - return LineEnding.nativeIsWin() ? input.replace("/", "\\\\") : input; + return FileSignature.machineIsWin() ? input.replace("/", "\\\\") : input; } private String testFilePath(int number) { diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md index 9d8010aab3..aa3236a3ce 100644 --- a/plugin-maven/CHANGES.md +++ b/plugin-maven/CHANGES.md @@ -3,6 +3,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +### Fixed +* Git-native handling of line endings was broken, now fixed ([#639](https://github.com/diffplug/spotless/pull/639)). ## [2.0.0] - 2020-07-02 ### Added diff --git a/plugin-maven/src/test/java/com/diffplug/spotless/maven/MavenRunner.java b/plugin-maven/src/test/java/com/diffplug/spotless/maven/MavenRunner.java index e7fbb88a14..c730fc6e9c 100644 --- a/plugin-maven/src/test/java/com/diffplug/spotless/maven/MavenRunner.java +++ b/plugin-maven/src/test/java/com/diffplug/spotless/maven/MavenRunner.java @@ -28,7 +28,7 @@ import com.diffplug.common.base.Throwables; import com.diffplug.common.io.ByteStreams; -import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.FileSignature; /** * Harness for running a maven build, same idea as the @@ -128,7 +128,7 @@ public String toString() { /** Prepends any arguments necessary to run a console command. */ private static List getPlatformCmds(String cmd) { - if (LineEnding.nativeIsWin()) { + if (FileSignature.machineIsWin()) { return Arrays.asList("cmd", "/c", "mvnw " + cmd); } else { return Arrays.asList("/bin/sh", "-c", "./mvnw " + cmd);