[fix] NPE due to workingTreeIterator being null for git ignored files. #911

- #911

Signed-off-by: Vincent Potucek <[email protected]>

Author: Vincent Potucek

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

@@ -56,14 +56,15 @@
  */
 public abstract class GitRatchet<Project> implements AutoCloseable {
 
+	private final Map<Repository, DirCache> dirCaches = new HashMap<>();
+
 	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);
+		return isClean(
+				project,
+				treeSha,
+				FileSignature.pathNativeToUnix(repositoryFor(project).getWorkTree().toPath().relativize(file.toPath()).toString()));
 	}
 
-	private final Map<Repository, DirCache> dirCaches = new HashMap<>();
-
 	/**
 	 * 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
@@ -73,16 +74,22 @@ public boolean isClean(Project project, ObjectId treeSha, File file) throws IOEx
 	public boolean isClean(Project project, ObjectId treeSha, String relativePathUnix) throws IOException {
 		Repository repo = repositoryFor(project);
 
-		DirCacheIterator dirCacheIteratorInit;
+		DirCacheIterator dirCacheIterator;
 		synchronized (this) {
 			// each DirCache is thread-safe, and we compute them one-to-one based on `repositoryFor`
 			DirCache dirCache = dirCaches.computeIfAbsent(repo, Errors.rethrow().wrap(Repository::readDirCache));
-			dirCacheIteratorInit = new DirCacheIterator(dirCache);
+			dirCacheIterator = new DirCacheIterator(dirCache);
 		}
+
+		return checkFileCleanliness(treeSha, relativePathUnix, repo, dirCacheIterator);
+	}
+
+	private static boolean checkFileCleanliness(final ObjectId treeSha, final String relativePathUnix,
+			final Repository repo, final DirCacheIterator dirCacheIterator) throws IOException {
 		try (TreeWalk treeWalk = new TreeWalk(repo)) {
 			treeWalk.setRecursive(true);
 			treeWalk.addTree(treeSha);
-			treeWalk.addTree(dirCacheIteratorInit);
+			treeWalk.addTree(dirCacheIterator);
 			treeWalk.addTree(new FileTreeIterator(repo));
 			treeWalk.setFilter(AndTreeFilter.create(
 					PathFilter.create(relativePathUnix),
@@ -93,31 +100,17 @@ public boolean isClean(Project project, ObjectId treeSha, String relativePathUni
 				return true;
 			} else {
 				AbstractTreeIterator treeIterator = treeWalk.getTree(TREE, AbstractTreeIterator.class);
-				DirCacheIterator dirCacheIterator = treeWalk.getTree(INDEX, DirCacheIterator.class);
-				WorkingTreeIterator workingTreeIterator = treeWalk.getTree(WORKDIR, WorkingTreeIterator.class);
-
-				boolean hasTree = treeIterator != null;
-				boolean hasDirCache = dirCacheIterator != null;
-
-				if (!hasTree) {
+				if (treeIterator == null) {
 					// it's not in the tree, so it was added
 					return false;
 				} else {
-					if (hasDirCache) {
-						boolean treeEqualsIndex = treeIterator.idEqual(dirCacheIterator) && treeIterator.getEntryRawMode() == dirCacheIterator.getEntryRawMode();
-						boolean indexEqualsWC = !workingTreeIterator.isModified(dirCacheIterator.getDirCacheEntry(), true, treeWalk.getObjectReader());
-						if (treeEqualsIndex != indexEqualsWC) {
-							// if one is equal and the other isn't, then it has definitely changed
-							return false;
-						} 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 " + relativePathUnix + " against treeSha " + treeSha + " is invalid.");
-						} else {
-							// they are all unique
-							// we have to check manually
-							return worktreeIsCleanCheckout(treeWalk);
-						}
+					DirCacheIterator indexIterator = treeWalk.getTree(INDEX, DirCacheIterator.class);
+					if (indexIterator != null) {
+						boolean treeEqualsIndex = treeIterator.idEqual(indexIterator) &&
+								treeIterator.getEntryRawMode() == indexIterator.getEntryRawMode();
+						WorkingTreeIterator workingTreeIterator = treeWalk.getTree(WORKDIR, WorkingTreeIterator.class);
+						return compareTreeIndexAndWorktreeStates(treeSha, relativePathUnix, treeEqualsIndex,
+								workingTreeIterator, indexIterator, treeWalk);
 					} else {
 						// no dirCache, so we will compare the tree to the workdir manually
 						return worktreeIsCleanCheckout(treeWalk);
@@ -127,6 +120,27 @@ public boolean isClean(Project project, ObjectId treeSha, String relativePathUni
 		}
 	}
 
+	/**
+	 * Compares the three states (tree, index, worktree) to determine file cleanliness.
+	 * Handles gitignored files by treating them as not clean.
+	 */
+	private static boolean compareTreeIndexAndWorktreeStates(final ObjectId treeSha, final String relativePathUnix,
+			final boolean treeEqualsIndex, final WorkingTreeIterator workingTreeIterator,
+			final DirCacheIterator indexIterator, final TreeWalk treeWalk) throws IOException {
+		if (treeEqualsIndex == workingTreeIterator.isModified(indexIterator.getDirCacheEntry(), true, treeWalk.getObjectReader())) {
+			// if one is equal and the other isn't, then it has definitely changed
+			return false;
+		} 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 " + relativePathUnix + " against treeSha " + treeSha + " is invalid.");
+		} else {
+			// they are all unique
+			// we have to check manually
+			return worktreeIsCleanCheckout(treeWalk);
+		}
+	}
+
 	/** Returns true if the worktree file is a clean checkout of head (possibly smudged). */
 	private static boolean worktreeIsCleanCheckout(TreeWalk treeWalk) {
 		return treeWalk.idEqual(TREE, WORKDIR);
@@ -178,16 +192,7 @@ public synchronized ObjectId rootTreeShaOf(Project project, String reference) {
 					if (commitSha == null) {
 						throw new IllegalArgumentException("No such reference '" + reference + "'");
 					}
-
-					RevCommit ratchetFrom = revWalk.parseCommit(commitSha);
-					RevCommit head = revWalk.parseCommit(repo.resolve(Constants.HEAD));
-
-					revWalk.setRevFilter(RevFilter.MERGE_BASE);
-					revWalk.markStart(ratchetFrom);
-					revWalk.markStart(head);
-
-					RevCommit mergeBase = revWalk.next();
-					treeSha = Optional.ofNullable(mergeBase).orElse(ratchetFrom).getTree();
+					treeSha = getMergeBaseTreeSha(revWalk, repo, revWalk.parseCommit(commitSha));
 				}
 				rootTreeShaCache.put(repo, reference, treeSha.copy());
 			}
@@ -197,6 +202,13 @@ public synchronized ObjectId rootTreeShaOf(Project project, String reference) {
 		}
 	}
 
+	private static ObjectId getMergeBaseTreeSha(final RevWalk revWalk, final Repository repo, final RevCommit ratchetFrom) throws IOException {
+		revWalk.setRevFilter(RevFilter.MERGE_BASE);
+		revWalk.markStart(ratchetFrom);
+		revWalk.markStart(revWalk.parseCommit(repo.resolve(Constants.HEAD)));
+		return Optional.ofNullable(revWalk.next()).orElse(ratchetFrom).getTree();
+	}
+
 	/**
 	 * Returns the sha of the git subtree which represents the root of the given project, or {@link ObjectId#zeroId()}
 	 * if there is no git subtree at the project root.
@@ -224,7 +236,8 @@ public synchronized ObjectId subtreeShaOf(Project project, ObjectId rootTreeSha)
 
 	@Override
 	public void close() {
-		gitRoots.values().stream()
+		gitRoots.values()
+				.stream()
 				.distinct()
 				.forEach(Repository::close);
 	}