Pathological up-to-date case (fixes #144)

lib/src/main/java/com/diffplug/spotless/PaddedCellBulk.java

@@ -191,6 +191,11 @@ public String getName() {
 
 	/** Performs the typical spotlessApply, but with PaddedCell handling of misbehaving FormatterSteps. */
 	public static void apply(Formatter formatter, File file) throws IOException {
+		applyAnyChanged(formatter, file);
+	}
+
+	/** Performs the typical spotlessApply, but with PaddedCell handling of misbehaving FormatterSteps. */
+	public static boolean applyAnyChanged(Formatter formatter, File file) throws IOException {
 		Objects.requireNonNull(formatter, "formatter");
 		Objects.requireNonNull(file, "file");
 
@@ -206,14 +211,14 @@ public static void apply(Formatter formatter, File file) throws IOException {
 		// if F(input) == input, then the formatter is well-behaving and the input is clean
 		byte[] formattedBytes = formatted.getBytes(formatter.getEncoding());
 		if (Arrays.equals(rawBytes, formattedBytes)) {
-			return;
+			return false;
 		}
 
 		// F(input) != input, so we'll do a padded check
 		PaddedCell cell = PaddedCell.check(formatter, file, rawUnix);
 		if (!cell.isResolvable()) {
 			// nothing we can do, but check will warn and dump out the divergence path
-			return;
+			return false;
 		}
 
 		// get the canonical bytes
@@ -223,6 +228,9 @@ public static void apply(Formatter formatter, File file) throws IOException {
 		if (!Arrays.equals(rawBytes, canonicalBytes)) {
 			// and write them to disk if needed
 			Files.write(file.toPath(), canonicalBytes, StandardOpenOption.TRUNCATE_EXISTING);
+			return true;
+		} else {
+			return false;
 		}
 	}
 

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SerializableMisc.java

@@ -0,0 +1,68 @@
+/*
+ * Copyright 2016 DiffPlug
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.diffplug.gradle.spotless;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.io.OutputStream;
+import java.io.Serializable;
+
+import com.diffplug.common.base.Errors;
+import com.diffplug.common.base.Preconditions;
+import com.diffplug.common.io.Files;
+
+class SerializableMisc {
+	static void toFile(Serializable obj, File file) {
+		try {
+			java.nio.file.Files.createDirectories(file.getParentFile().toPath());
+			try (OutputStream output = Files.asByteSink(file).openBufferedStream()) {
+				toStream(obj, output);
+			}
+		} catch (IOException e) {
+			throw Errors.asRuntime(e);
+		}
+	}
+
+	static <T extends Serializable> T fromFile(Class<T> clazz, File file) {
+		try (InputStream input = Files.asByteSource(file).openBufferedStream()) {
+			return fromStream(clazz, input);
+		} catch (IOException e) {
+			throw Errors.asRuntime(e);
+		}
+	}
+
+	static void toStream(Serializable obj, OutputStream stream) {
+		try (ObjectOutputStream objectOutput = new ObjectOutputStream(stream)) {
+			objectOutput.writeObject(obj);
+		} catch (IOException e) {
+			throw Errors.asRuntime(e);
+		}
+	}
+
+	@SuppressWarnings("unchecked")
+	static <T> T fromStream(Class<T> clazz, InputStream stream) {
+		try (ObjectInputStream objectInput = new ObjectInputStream(stream)) {
+			T object = (T) objectInput.readObject();
+			Preconditions.checkArgument(clazz.isInstance(object), "Requires class %s, was %s", clazz, object);
+			return object;
+		} catch (ClassNotFoundException | IOException e) {
+			throw Errors.asRuntime(e);
+		}
+	}
+}

plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java

@@ -19,6 +19,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.io.Serializable;
 import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.StandardOpenOption;
@@ -32,10 +33,11 @@
 import org.gradle.api.GradleException;
 import org.gradle.api.tasks.Input;
 import org.gradle.api.tasks.InputFiles;
-import org.gradle.api.tasks.SkipWhenEmpty;
 import org.gradle.api.tasks.TaskAction;
 import org.gradle.api.tasks.incremental.IncrementalTaskInputs;
 
+import com.diffplug.common.collect.ImmutableList;
+import com.diffplug.common.collect.Iterables;
 import com.diffplug.spotless.FormatExceptionPolicy;
 import com.diffplug.spotless.FormatExceptionPolicyStrict;
 import com.diffplug.spotless.Formatter;
@@ -46,9 +48,9 @@
 
 public class SpotlessTask extends DefaultTask {
 	// set by SpotlessExtension, but possibly overridden by FormatExtension
-	@Input
 	protected String encoding = "UTF-8";
 
+	@Input
 	public String getEncoding() {
 		return encoding;
 	}
@@ -57,9 +59,9 @@ public void setEncoding(String encoding) {
 		this.encoding = Objects.requireNonNull(encoding);
 	}
 
-	@Input
 	protected LineEnding.Policy lineEndingsPolicy = LineEnding.UNIX.createPolicy();
 
+	@Input
 	public LineEnding.Policy getLineEndingsPolicy() {
 		return lineEndingsPolicy;
 	}
@@ -69,9 +71,9 @@ public void setLineEndingsPolicy(LineEnding.Policy lineEndingsPolicy) {
 	}
 
 	// set by FormatExtension
-	@Input
 	protected boolean paddedCell = false;
 
+	@Input
 	public boolean isPaddedCell() {
 		return paddedCell;
 	}
@@ -80,19 +82,17 @@ public void setPaddedCell(boolean paddedCell) {
 		this.paddedCell = paddedCell;
 	}
 
-	@Input
 	protected FormatExceptionPolicy exceptionPolicy = new FormatExceptionPolicyStrict();
 
 	public void setExceptionPolicy(FormatExceptionPolicy exceptionPolicy) {
 		this.exceptionPolicy = Objects.requireNonNull(exceptionPolicy);
 	}
 
+	@Input
 	public FormatExceptionPolicy getExceptionPolicy() {
 		return exceptionPolicy;
 	}
 
-	@InputFiles
-	@SkipWhenEmpty
 	protected Iterable<File> target;
 
 	public Iterable<File> getTarget() {
@@ -103,9 +103,21 @@ public void setTarget(Iterable<File> target) {
 		this.target = requireElementsNonNull(target);
 	}
 
-	@Input
+	/** Internal use only. */
+	@InputFiles
+	@Deprecated
+	public Iterable<File> getInternalTarget() {
+		// used to combine the special cache file and the real target
+		return Iterables.concat(ImmutableList.of(getCacheFile()), target);
+	}
+
+	private File getCacheFile() {
+		return new File(getProject().getBuildDir(), getName());
+	}
+
 	protected List<FormatterStep> steps = new ArrayList<>();
 
+	@Input
 	public List<FormatterStep> getSteps() {
 		return Collections.unmodifiableList(steps);
 	}
@@ -160,30 +172,64 @@ public void performAction(IncrementalTaskInputs inputs) throws Exception {
 		List<File> outOfDate = new ArrayList<>();
 		inputs.outOfDate(inputDetails -> {
 			File file = inputDetails.getFile();
-			if (file.isFile()) {
+			if (file.isFile() && !file.equals(getCacheFile())) {
 				outOfDate.add(file);
 			}
 		});
+		// load the files that were changed by the last run
+		// because it's possible the user changed them back to their
+		// unformatted form, so we need to treat them as dirty
+		// (see bug #144)
+		if (getCacheFile().exists()) {
+			LastApply lastApply = SerializableMisc.fromFile(LastApply.class, getCacheFile());
+			for (File file : lastApply.changedFiles) {
+				if (!outOfDate.contains(file) && file.exists()) {
+					outOfDate.add(file);
+				}
+			}
+		}
 
 		if (apply) {
-			apply(formatter, outOfDate);
+			List<File> changedFiles = applyAnyChanged(formatter, outOfDate);
+			if (!changedFiles.isEmpty()) {
+				// If any file changed, we need to mark the task as dirty
+				// next time to avoid bug #144.
+				LastApply lastApply = new LastApply();
+				lastApply.timestamp = System.currentTimeMillis();
+				lastApply.changedFiles = changedFiles;
+
+				SerializableMisc.toFile(lastApply, getCacheFile());
+			}
 		}
 		if (check) {
 			check(formatter, outOfDate);
 		}
 	}
 
-	private void apply(Formatter formatter, List<File> outOfDate) throws Exception {
+	static class LastApply implements Serializable {
+		private static final long serialVersionUID = 6245070824310295090L;
+
+		long timestamp;
+		List<File> changedFiles;
+	}
+
+	private List<File> applyAnyChanged(Formatter formatter, List<File> outOfDate) throws Exception {
+		List<File> changed = new ArrayList<>(outOfDate.size());
 		if (isPaddedCell()) {
 			for (File file : outOfDate) {
 				getLogger().debug("Applying format to " + file);
-				PaddedCellBulk.apply(formatter, file);
+				if (PaddedCellBulk.applyAnyChanged(formatter, file)) {
+					changed.add(file);
+				}
 			}
 		} else {
 			boolean anyMisbehave = false;
 			for (File file : outOfDate) {
 				getLogger().debug("Applying format to " + file);
 				String unixResultIfDirty = formatter.applyToAndReturnResultIfDirty(file);
+				if (unixResultIfDirty != null) {
+					changed.add(file);
+				}
 				// because apply will count as up-to-date, it's important
 				// that every call to apply will get a PaddedCell check
 				if (!anyMisbehave && unixResultIfDirty != null) {
@@ -207,6 +253,7 @@ private void apply(Formatter formatter, List<File> outOfDate) throws Exception {
 				throw PaddedCellGradle.youShouldTurnOnPaddedCell(this);
 			}
 		}
+		return changed;
 	}
 
 	private void check(Formatter formatter, List<File> outOfDate) throws Exception {

plugin-gradle/src/test/java/com/diffplug/gradle/spotless/GradleIncrementalResolutionTest.java

@@ -68,11 +68,12 @@ public void failureDoesntTriggerAll() throws IOException {
 		// if we change just one file
 		writeState("Abc");
 		// then check runs against just the changed file
-		checkRanAgainst("a");
+		// and also (because #144) the last files to be changed
+		checkRanAgainst("a", "b");
 		// even after failing, still just the one
-		checkRanAgainst("a");
+		checkRanAgainst("a", "b");
 		// and so does apply
-		applyRanAgainst("a");
+		applyRanAgainst("a", "b");
 		applyRanAgainst("a");
 		// until the issue has been fixed
 		applyRanAgainst("");

plugin-gradle/src/test/java/com/diffplug/gradle/spotless/UpToDateTest.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright 2016 DiffPlug
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.diffplug.gradle.spotless;
+
+import java.io.IOException;
+
+import org.junit.Test;
+
+public class UpToDateTest extends GradleIntegrationTest {
+	/** Requires that README be lowercase. */
+	private void writeBuildFile() throws IOException {
+		write("build.gradle",
+				"plugins {",
+				"    id 'com.diffplug.gradle.spotless'",
+				"}",
+				"spotless {",
+				"    format 'misc', {",
+				"        target file('README.md')",
+				"        customLazyGroovy('lowercase') {",
+				"             return { str -> str.toLowerCase(Locale.ROOT) }",
+				"        }",
+				"        bumpThisNumberIfACustomStepChanges(1)",
+				"    }",
+				"}");
+	}
+
+	@Test
+	public void testNormalCase() throws IOException {
+		writeBuildFile();
+		write("README.md", "ABC");
+		// first time, the task runs as expected
+		applyIsUpToDate(false);
+		assertFile("README.md").hasContent("abc");
+		// because a file was changed (by spotless),
+		// up-to-date is false, even though nothing is
+		// going to change during this run.  This second
+		// run is very fast though, because it will
+		// only run on the few files that were changed.
+		applyIsUpToDate(false);
+		// it's not until the third run that everything
+		// is totally up-to-date
+		applyIsUpToDate(true);
+	}
+
+	@Test
+	public void testNearPathologicalCase() throws IOException {
+		writeBuildFile();
+		write("README.md", "ABC");
+		// first time, up-to-date is false
+		applyIsUpToDate(false);
+		assertFile("README.md").hasContent("abc");
+
+		// now we'll change the file
+		write("README.md", "AB");
+		// as expected, the task will run again
+		applyIsUpToDate(false);
+		assertFile("README.md").hasContent("ab");
+		// and it'll take two more runs to get to up-to-date
+		applyIsUpToDate(false);
+		applyIsUpToDate(true);
+	}
+
+	@Test
+	public void testPathologicalCase() throws IOException {
+		writeBuildFile();
+		write("README.md", "ABC");
+		// first time, up-to-date is false
+		applyIsUpToDate(false);
+		assertFile("README.md").hasContent("abc");
+
+		// now we'll change the file back to EXACTLY its original content
+		write("README.md", "ABC");
+		// the task should run again, but instead the next line will
+		// fail an assertion, because the task is actually reported as up-to-date
+		applyIsUpToDate(false);
+		assertFile("README.md").hasContent("abc");
+		// and it'll take two more runs to get to up-to-date
+		applyIsUpToDate(false);
+		applyIsUpToDate(true);
+	}
+}