From 1ae409e2b76dff36dd48e96a703557e610a5d4b7 Mon Sep 17 00:00:00 2001 From: Erik Date: Sun, 27 Sep 2020 15:08:52 +0300 Subject: [PATCH] Replace DiffCommand with IndexDiff DiffCommand suffered from not taking smudge/clean filters into account --- .../diffplug/spotless/extra/GitRatchet.java | 14 +++-- .../spotless/maven/AbstractSpotlessMojo.java | 3 - .../spotless/maven/GitRatchetMaven.java | 56 ++++++++++++------- 3 files changed, 45 insertions(+), 28 deletions(-) diff --git a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java b/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java index 77568e2ba5..8dc953aa48 100644 --- a/lib-extra/src/main/java/com/diffplug/spotless/extra/GitRatchet.java +++ b/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 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 relativePath) 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(relativePath), 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 " + relativePath + " against treeSha " + treeSha + " is invalid."); } else { // they are all unique // we have to check manually diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java index 9e6f5ca9cf..ef265b1126 100644 --- a/plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java +++ b/plugin-maven/src/main/java/com/diffplug/spotless/maven/AbstractSpotlessMojo.java @@ -45,7 +45,6 @@ import org.eclipse.aether.RepositorySystem; import org.eclipse.aether.RepositorySystemSession; import org.eclipse.aether.repository.RemoteRepository; -import org.eclipse.jgit.api.errors.GitAPIException; import com.diffplug.spotless.Formatter; import com.diffplug.spotless.LineEnding; @@ -190,8 +189,6 @@ private List collectFilesFromGit(FormatterFactory formatterFactory, String .instance().getDirtyFiles(baseDir, ratchetFrom); } catch (IOException e) { throw new MojoExecutionException("Unable to scan file tree rooted at " + baseDir, e); - } catch (GitAPIException e) { - throw new MojoExecutionException("Error getting diff against 'ratchetFrom' setting '" + ratchetFrom + "'", e); } List result = new ArrayList<>(); diff --git a/plugin-maven/src/main/java/com/diffplug/spotless/maven/GitRatchetMaven.java b/plugin-maven/src/main/java/com/diffplug/spotless/maven/GitRatchetMaven.java index db6330087f..f68ac295f6 100644 --- a/plugin-maven/src/main/java/com/diffplug/spotless/maven/GitRatchetMaven.java +++ b/plugin-maven/src/main/java/com/diffplug/spotless/maven/GitRatchetMaven.java @@ -19,16 +19,14 @@ import java.io.IOException; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.List; +import java.util.HashSet; +import java.util.Set; import java.util.stream.Collectors; -import org.eclipse.jgit.api.Git; -import org.eclipse.jgit.api.errors.GitAPIException; -import org.eclipse.jgit.diff.DiffEntry; +import org.eclipse.jgit.lib.IndexDiff; import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectReader; import org.eclipse.jgit.lib.Repository; -import org.eclipse.jgit.treewalk.CanonicalTreeParser; +import org.eclipse.jgit.treewalk.FileTreeIterator; import com.diffplug.spotless.extra.GitRatchet; @@ -58,28 +56,44 @@ static GitRatchetMaven instance() { return instance; } - Iterable getDirtyFiles(File baseDir, String ratchetFrom) - throws IOException, GitAPIException { + Iterable getDirtyFiles(File baseDir, String ratchetFrom) throws IOException { Repository repository = repositoryFor(baseDir); ObjectId sha = rootTreeShaOf(baseDir, ratchetFrom); - ObjectReader oldReader = repository.newObjectReader(); - CanonicalTreeParser oldTree = new CanonicalTreeParser(); - oldTree.reset(oldReader, sha); - - Git git = new Git(repository); - List diffs = git.diff() - .setShowNameAndStatusOnly(true) - .setOldTree(oldTree) - .call(); + IndexDiff indexDiff = new IndexDiff(repository, sha, new FileTreeIterator(repository)); + indexDiff.diff(); String workTreePath = repository.getWorkTree().getPath(); Path baseDirPath = Paths.get(baseDir.getPath()); - return diffs.stream() - .map(DiffEntry::getNewPath) - .map(path -> Paths.get(workTreePath, path)) - .map(path -> baseDirPath.relativize(path).toString()) + Set 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()); } }