(plugin-maven) when ratcheting, check dirty files from git (#706)

Author: nedtwigg

lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java

@@ -56,15 +56,21 @@
  * - If you have up-to-date checking and want the best possible performance, use {@link #subtreeShaOf(Object, ObjectId)} to optimize up-to-date checks on a per-project basis.
  */
 public abstract class GitRatchet<Project> implements AutoCloseable {
+
+	public boolean isClean(Project project, ObjectId treeSha, File file) throws IOException {
+		Repository repo = repositoryFor(project);
+		String relativePath = FileSignature.pathNativeToUnix(repo.getWorkTree().toPath().relativize(file.toPath()).toString());
+		return isClean(project, treeSha, relativePath);
+	}
+
 	/**
 	 * This is the highest-level method, which all the others serve.  Given the sha
 	 * of a git tree (not a commit!), and the file in question, this method returns
 	 * true if that file is clean relative to that tree.  A naive implementation of this
 	 * could be verrrry slow, so the rest of this is about speeding this up.
 	 */
-	public boolean isClean(Project project, ObjectId treeSha, File file) throws IOException {
+	public boolean isClean(Project project, ObjectId treeSha, String relativePathUnix) throws IOException {
 		Repository repo = repositoryFor(project);
-		String path = FileSignature.pathNativeToUnix(repo.getWorkTree().toPath().relativize(file.toPath()).toString());
 
 		// TODO: should be cached-per-repo if it is thread-safe, or per-repo-per-thread if it is not
 		DirCache dirCache = repo.readDirCache();
@@ -75,7 +81,7 @@ public boolean isClean(Project project, ObjectId treeSha, File file) throws IOEx
 			treeWalk.addTree(new DirCacheIterator(dirCache));
 			treeWalk.addTree(new FileTreeIterator(repo));
 			treeWalk.setFilter(AndTreeFilter.create(
-					PathFilter.create(path),
+					PathFilter.create(relativePathUnix),
 					new IndexDiffFilter(INDEX, WORKDIR)));
 
 			if (!treeWalk.next()) {
@@ -102,7 +108,7 @@ public boolean isClean(Project project, ObjectId treeSha, File file) throws IOEx
 						} else if (treeEqualsIndex) {
 							// this means they are all equal to each other, which should never happen
 							// the IndexDiffFilter should keep those out of the TreeWalk entirely
-							throw new IllegalStateException("Index status for " + file + " against treeSha " + treeSha + " is invalid.");
+							throw new IllegalStateException("Index status for " + relativePathUnix + " against treeSha " + treeSha + " is invalid.");
 						} else {
 							// they are all unique
 							// we have to check manually
@@ -135,7 +141,7 @@ private static boolean worktreeIsCleanCheckout(TreeWalk treeWalk) {
 	 * builds and submodules, it's quite possible that a single Gradle project will span across multiple git repositories.
 	 * We cache the Repository for every Project in `gitRoots`, and use dynamic programming to populate it.
 	 */
-	private Repository repositoryFor(Project project) throws IOException {
+	protected Repository repositoryFor(Project project) throws IOException {
 		Repository repo = gitRoots.get(project);
 		if (repo == null) {
 			if (isGitRoot(getDir(project))) {

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
+Improve speed of plugin when using `<ratchetFrom>` ([#701](https://github.com/diffplug/spotless/pull/706)).
 
 ## [2.4.1] - 2020-09-18
 ### Fixed

plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java

@@ -19,6 +19,7 @@
 
 import java.io.File;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
@@ -30,6 +31,7 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
 
 import org.apache.maven.plugin.AbstractMojo;
 import org.apache.maven.plugin.MojoExecutionException;
@@ -38,11 +40,11 @@
 import org.codehaus.plexus.resource.ResourceManager;
 import org.codehaus.plexus.resource.loader.FileResourceLoader;
 import org.codehaus.plexus.util.FileUtils;
+import org.codehaus.plexus.util.MatchPatterns;
 import org.eclipse.aether.RepositorySystem;
 import org.eclipse.aether.RepositorySystemSession;
 import org.eclipse.aether.repository.RemoteRepository;
 
-import com.diffplug.common.collect.Iterables;
 import com.diffplug.spotless.Formatter;
 import com.diffplug.spotless.LineEnding;
 import com.diffplug.spotless.Provisioner;
@@ -137,38 +139,23 @@ public final void execute() throws MojoExecutionException {
 	}
 
 	private void execute(FormatterFactory formatterFactory) throws MojoExecutionException {
-		List<File> files = collectFiles(formatterFactory);
 		FormatterConfig config = getFormatterConfig();
-		Optional<String> ratchetFrom = formatterFactory.ratchetFrom(config);
-		Iterable<File> toFormat;
-		if (!ratchetFrom.isPresent()) {
-			toFormat = files;
-		} else {
-			toFormat = Iterables.filter(files, GitRatchetMaven.instance().isGitDirty(baseDir, ratchetFrom.get()));
-		}
+		List<File> files = collectFiles(formatterFactory, config);
+
 		try (Formatter formatter = formatterFactory.newFormatter(files, config)) {
-			process(toFormat, formatter);
+			process(files, formatter);
 		}
 	}
 
-	private List<File> collectFiles(FormatterFactory formatterFactory) throws MojoExecutionException {
-		Set<String> configuredIncludes = formatterFactory.includes();
-		Set<String> configuredExcludes = formatterFactory.excludes();
-
-		Set<String> includes = configuredIncludes.isEmpty() ? formatterFactory.defaultIncludes() : configuredIncludes;
-		if (includes.isEmpty()) {
-			throw new MojoExecutionException("You must specify some files to include, such as '<includes><include>src/**</include></includes>'");
-		}
-
-		Set<String> excludes = new HashSet<>(FileUtils.getDefaultExcludesAsList());
-		excludes.add(withTrailingSeparator(buildDir.toString()));
-		excludes.addAll(configuredExcludes);
-
-		String includesString = String.join(",", includes);
-		String excludesString = String.join(",", excludes);
-
+	private List<File> collectFiles(FormatterFactory formatterFactory, FormatterConfig config) throws MojoExecutionException {
+		Optional<String> ratchetFrom = formatterFactory.ratchetFrom(config);
 		try {
-			final List<File> files = FileUtils.getFiles(baseDir, includesString, excludesString);
+			final List<File> files;
+			if (ratchetFrom.isPresent()) {
+				files = collectFilesFromGit(formatterFactory, ratchetFrom.get());
+			} else {
+				files = collectFilesFromFormatterFactory(formatterFactory);
+			}
 			if (filePatterns == null || filePatterns.isEmpty()) {
 				return files;
 			}
@@ -189,10 +176,68 @@ private List<File> collectFiles(FormatterFactory formatterFactory) throws MojoEx
 		}
 	}
 
+	private List<File> collectFilesFromGit(FormatterFactory formatterFactory, String ratchetFrom) throws MojoExecutionException {
+		MatchPatterns includePatterns = MatchPatterns.from(
+				withNormalizedFileSeparators(getIncludes(formatterFactory)));
+		MatchPatterns excludePatterns = MatchPatterns.from(
+				withNormalizedFileSeparators(getExcludes(formatterFactory)));
+
+		Iterable<String> dirtyFiles;
+		try {
+			dirtyFiles = GitRatchetMaven
+					.instance().getDirtyFiles(baseDir, ratchetFrom);
+		} catch (IOException e) {
+			throw new MojoExecutionException("Unable to scan file tree rooted at " + baseDir, e);
+		}
+
+		List<File> result = new ArrayList<>();
+		for (String file : withNormalizedFileSeparators(dirtyFiles)) {
+			if (includePatterns.matches(file, true)) {
+				if (!excludePatterns.matches(file, true)) {
+					result.add(new File(baseDir.getPath(), file));
+				}
+			}
+		}
+		return result;
+	}
+
+	private List<File> collectFilesFromFormatterFactory(FormatterFactory formatterFactory)
+			throws MojoExecutionException, IOException {
+		String includesString = String.join(",", getIncludes(formatterFactory));
+		String excludesString = String.join(",", getExcludes(formatterFactory));
+
+		return FileUtils.getFiles(baseDir, includesString, excludesString);
+	}
+
+	private Iterable<String> withNormalizedFileSeparators(Iterable<String> patterns) {
+		return StreamSupport.stream(patterns.spliterator(), true)
+				.map(pattern -> pattern.replace('/', File.separatorChar))
+				.map(pattern -> pattern.replace('\\', File.separatorChar))
+				.collect(Collectors.toSet());
+	}
+
 	private static String withTrailingSeparator(String path) {
 		return path.endsWith(File.separator) ? path : path + File.separator;
 	}
 
+	private Set<String> getIncludes(FormatterFactory formatterFactory) throws MojoExecutionException {
+		Set<String> configuredIncludes = formatterFactory.includes();
+		Set<String> includes = configuredIncludes.isEmpty() ? formatterFactory.defaultIncludes() : configuredIncludes;
+		if (includes.isEmpty()) {
+			throw new MojoExecutionException("You must specify some files to include, such as '<includes><include>src/**</include></includes>'");
+		}
+		return includes;
+	}
+
+	private Set<String> getExcludes(FormatterFactory formatterFactory) {
+		Set<String> configuredExcludes = formatterFactory.excludes();
+
+		Set<String> excludes = new HashSet<>(FileUtils.getDefaultExcludesAsList());
+		excludes.add(withTrailingSeparator(buildDir.toString()));
+		excludes.addAll(configuredExcludes);
+		return excludes;
+	}
+
 	private FormatterConfig getFormatterConfig() {
 		ArtifactResolver resolver = new ArtifactResolver(repositorySystem, repositorySystemSession, repositories, getLog());
 		Provisioner provisioner = MavenProvisioner.create(resolver);

plugin-maven/src/main/java/com/diffplug/spotless/maven/GitRatchetMaven.java

@@ -17,11 +17,17 @@
 
 import java.io.File;
 import java.io.IOException;
-import java.util.function.Predicate;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
 
+import org.eclipse.jgit.lib.IndexDiff;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.treewalk.FileTreeIterator;
 
-import com.diffplug.common.base.Errors;
 import com.diffplug.spotless.extra.GitRatchet;
 
 final class GitRatchetMaven extends GitRatchet<File> {
@@ -50,15 +56,44 @@ static GitRatchetMaven instance() {
 		return instance;
 	}
 
-	/** A predicate which returns only the "git dirty" files. */
-	Predicate<File> isGitDirty(File baseDir, String ratchetFrom) {
+	Iterable<String> getDirtyFiles(File baseDir, String ratchetFrom) throws IOException {
+		Repository repository = repositoryFor(baseDir);
 		ObjectId sha = rootTreeShaOf(baseDir, ratchetFrom);
-		return file -> {
-			try {
-				return !isClean(baseDir, sha, file);
-			} catch (IOException e) {
-				throw Errors.asRuntime(e);
+
+		IndexDiff indexDiff = new IndexDiff(repository, sha, new FileTreeIterator(repository));
+		indexDiff.diff();
+
+		String workTreePath = repository.getWorkTree().getPath();
+		Path baseDirPath = Paths.get(baseDir.getPath());
+
+		Set<String> dirtyPaths = new HashSet<>(indexDiff.getChanged());
+		dirtyPaths.addAll(indexDiff.getAdded());
+		dirtyPaths.addAll(indexDiff.getConflicting());
+		dirtyPaths.addAll(indexDiff.getUntracked());
+
+		for (String path : indexDiff.getModified()) {
+			if (!dirtyPaths.add(path)) {
+				// File differs to index both in working tree and local repository,
+				// which means the working tree and local repository versions may be equal
+				if (isClean(baseDir, sha, path)) {
+					dirtyPaths.remove(path);
+				}
 			}
-		};
+		}
+		for (String path : indexDiff.getRemoved()) {
+			if (dirtyPaths.contains(path)) {
+				// A removed file can also be untracked, if a new file with the same name has been created.
+				// This file may be identical to the one in the local repository.
+				if (isClean(baseDir, sha, path)) {
+					dirtyPaths.remove(path);
+				}
+			}
+		}
+		// A file can be modified in the index but removed in the tree
+		dirtyPaths.removeAll(indexDiff.getMissing());
+
+		return dirtyPaths.stream()
+				.map(path -> baseDirPath.relativize(Paths.get(workTreePath, path)).toString())
+				.collect(Collectors.toList());
 	}
 }