Detect windows using os.name rather than line.separator.

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

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;
 		}

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<File> testFiles() {
+		try {
+			List<File> 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<String> 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");
+	}
 }

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<File> validateInputFiles(List<File> files) {

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;
 	}

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);

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

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";
 	}
 }

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.";
 

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) {

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

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<String> 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);