Skip ignored directories in FileTreeIterator

Make FileTreeIterator not enter ignored directories by default. We
only need to enter ignored directories if we do some operation against
git, and there is at least one tracked file underneath an ignored
directory.

Walking ignored directories should be avoided as much as possible as
it is a potential performance bottleneck. Some projects have a lot of
files or very deep hierarchies in ignored directories; walking those
may be costly (especially so on Windows). See for instance also bug
500106.

Provide a FileTreeIterator.setWalkIgnoredDirectories() operation to
force the iterator to iterate also through otherwise ignored
directories. Useful for tests (IgnoreNodeTest, CGitIgnoreTest), or
to implement things like "git ls-files --ignored".

Add tests in DirCacheCheckoutTest, and amend IndexDiffTest to test a
little bit more.

Bug: 388582
Change-Id: I6ff584a42c55a07120a4369fd308409431bdb94a
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

6 files changed, 195 insertions, 5 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java
index 3b11616fe6..0c6ed0c19e 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java
@@ -141,7 +141,9 @@ public class CGitIgnoreTest extends RepositoryTestCase {
 		// Do a tree walk that does descend into ignored directories and return
 		// a list of all ignored files
 		try (TreeWalk walk = new TreeWalk(db)) {
-			walk.addTree(new FileTreeIterator(db));
+			FileTreeIterator iter = new FileTreeIterator(db);
+			iter.setWalkIgnoredDirectories(true);
+			walk.addTree(iter);
 			walk.setRecursive(true);
 			while (walk.next()) {
 				if (walk.getTree(WorkingTreeIterator.class).isEntryIgnored()) {
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java
index 80595feff5..78d9a82c24 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java
@@ -732,7 +732,9 @@ public class IgnoreNodeTest extends RepositoryTestCase {
 
 	private void beginWalk() {
 		walk = new TreeWalk(db);
-		walk.addTree(new FileTreeIterator(db));
+		FileTreeIterator iter = new FileTreeIterator(db);
+		iter.setWalkIgnoredDirectories(true);
+		walk.addTree(iter);
 	}
 
 	private void endWalk() throws IOException {
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
index 0412242ebf..eb87827805 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/DirCacheCheckoutTest.java
@@ -78,8 +78,10 @@ import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.junit.TestRepository.BranchBuilder;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.treewalk.AbstractTreeIterator;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
 import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.treewalk.WorkingTreeIterator;
 import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.FileUtils;
 import org.junit.Assume;
@@ -1904,6 +1906,117 @@ public class DirCacheCheckoutTest extends RepositoryTestCase {
 		assertUpdated(longFileName);
 	}
 
+	@Test
+	public void testIgnoredDirectory() throws Exception {
+		writeTrashFile(".gitignore", "src/ignored");
+		writeTrashFile("src/ignored/sub/foo.txt", "1");
+		try (Git git = new Git(db)) {
+			git.add().addFilepattern(".").call();
+			RevCommit commit = git.commit().setMessage("adding .gitignore")
+					.call();
+			writeTrashFile("foo.txt", "2");
+			writeTrashFile("zzz.txt", "3");
+			git.add().addFilepattern("foo.txt").call();
+			git.commit().setMessage("add file").call();
+			assertEquals("Should not have entered ignored directory", 1,
+					resetHardAndCount(commit));
+		}
+	}
+
+	@Test
+	public void testIgnoredDirectoryWithTrackedContent() throws Exception {
+		writeTrashFile("src/ignored/sub/foo.txt", "1");
+		try (Git git = new Git(db)) {
+			git.add().addFilepattern(".").call();
+			git.commit().setMessage("adding foo.txt").call();
+			writeTrashFile(".gitignore", "src/ignored");
+			writeTrashFile("src/ignored/sub/foo.txt", "2");
+			writeTrashFile("src/ignored/other/bar.txt", "3");
+			git.add().addFilepattern(".").call();
+			RevCommit commit = git.commit().setMessage("adding .gitignore")
+					.call();
+			writeTrashFile("foo.txt", "2");
+			writeTrashFile("zzz.txt", "3");
+			git.add().addFilepattern("foo.txt").call();
+			git.commit().setMessage("add file").call();
+			File file = writeTrashFile("src/ignored/sub/foo.txt", "3");
+			assertEquals("Should have entered ignored directory", 3,
+					resetHardAndCount(commit));
+			checkFile(file, "2");
+		}
+	}
+
+	@Test
+	public void testResetWithChangeInGitignore() throws Exception {
+		writeTrashFile(".gitignore", "src/ignored");
+		writeTrashFile("src/ignored/sub/foo.txt", "1");
+		try (Git git = new Git(db)) {
+			git.add().addFilepattern(".").call();
+			RevCommit initial = git.commit().setMessage("initial").call();
+			writeTrashFile("src/newignored/foo.txt", "2");
+			writeTrashFile("src/.gitignore", "newignored");
+			git.add().addFilepattern(".").call();
+			RevCommit commit = git.commit().setMessage("newignored").call();
+			assertEquals("Should not have entered src/newignored directory", 1,
+					resetHardAndCount(initial));
+			assertEquals("Should have entered src/newignored directory", 2,
+					resetHardAndCount(commit));
+			deleteTrashFile("src/.gitignore");
+			git.rm().addFilepattern("src/.gitignore").call();
+			RevCommit top = git.commit().setMessage("Unignore newignore")
+					.call();
+			assertEquals("Should have entered src/newignored directory", 2,
+					resetHardAndCount(initial));
+			assertEquals("Should have entered src/newignored directory", 2,
+					resetHardAndCount(commit));
+			assertEquals("Should not have entered src/newignored directory", 1,
+					resetHardAndCount(top));
+
+		}
+	}
+
+	private static class TestFileTreeIterator extends FileTreeIterator {
+
+		// For assertions only
+		private final int[] count;
+
+		public TestFileTreeIterator(Repository repo, int[] count) {
+			super(repo);
+			this.count = count;
+		}
+
+		protected TestFileTreeIterator(final WorkingTreeIterator p,
+				final File root, FS fs, FileModeStrategy fileModeStrategy,
+				int[] count) {
+			super(p, root, fs, fileModeStrategy);
+			this.count = count;
+		}
+
+		@Override
+		protected AbstractTreeIterator enterSubtree() {
+			count[0] += 1;
+			return new TestFileTreeIterator(this,
+					((FileEntry) current()).getFile(), fs, fileModeStrategy,
+					count);
+		}
+	}
+
+	private int resetHardAndCount(RevCommit commit) throws Exception {
+		int[] callCount = { 0 };
+		DirCache cache = db.lockDirCache();
+		FileTreeIterator workingTreeIterator = new TestFileTreeIterator(db,
+				callCount);
+		try {
+			DirCacheCheckout checkout = new DirCacheCheckout(db, null, cache,
+					commit.getTree().getId(), workingTreeIterator);
+			checkout.setFailOnConflict(false);
+			checkout.checkout();
+		} finally {
+			cache.unlock();
+		}
+		return callCount[0];
+	}
+
 	public void assertWorkDir(Map<String, String> i)
 			throws CorruptObjectException,
 			IOException {
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java
index 2cb8f86fc2..580b08b42f 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java
@@ -470,7 +470,8 @@ public class IndexDiffTest extends RepositoryTestCase {
 	}
 
 	/**
-	 * Test that ignored folders aren't listed as untracked
+	 * Test that ignored folders aren't listed as untracked, but are listed as
+	 * ignored.
 	 *
 	 * @throws Exception
 	 */
@@ -499,6 +500,8 @@ public class IndexDiffTest extends RepositoryTestCase {
 			diff.diff();
 			assertEquals(new HashSet<>(Arrays.asList("src")),
 					diff.getUntrackedFolders());
+			assertEquals(new HashSet<>(Arrays.asList("sr", "target")),
+					diff.getIgnoredNotInIndex());
 
 			git.add().addFilepattern("src").call();
 			writeTrashFile("sr/com/X1.java", "");
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java
index ccd5e70912..24b9ac086e 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/FileTreeIterator.java
@@ -52,6 +52,7 @@ import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 
+import org.eclipse.jgit.dircache.DirCacheIterator;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.FileMode;
@@ -67,6 +68,7 @@ import org.eclipse.jgit.util.FS;
  * {@link org.eclipse.jgit.treewalk.TreeWalk}.
  */
 public class FileTreeIterator extends WorkingTreeIterator {
+
 	/**
 	 * the starting directory of this Iterator. All entries are located directly
 	 * in this directory.
@@ -207,7 +209,33 @@ public class FileTreeIterator extends WorkingTreeIterator {
 	@Override
 	public AbstractTreeIterator createSubtreeIterator(ObjectReader reader)
 			throws IncorrectObjectTypeException, IOException {
-		return new FileTreeIterator(this, ((FileEntry) current()).getFile(), fs, fileModeStrategy);
+		if (!walksIgnoredDirectories() && isEntryIgnored()) {
+			DirCacheIterator iterator = getDirCacheIterator();
+			if (iterator == null) {
+				return new EmptyTreeIterator(this);
+			}
+			// Only enter if we have an associated DirCacheIterator that is
+			// at the same entry (which indicates there is some already
+			// tracked file underneath this directory). Otherwise the
+			// directory is indeed ignored and can be skipped entirely.
+		}
+		return enterSubtree();
+	}
+
+
+	/**
+	 * Create a new iterator for the current entry's subtree.
+	 * <p>
+	 * The parent reference of the iterator must be <code>this</code>, otherwise
+	 * the caller would not be able to exit out of the subtree iterator
+	 * correctly and return to continue walking <code>this</code>.
+	 *
+	 * @return a new iterator that walks over the current subtree.
+	 * @since 5.0
+	 */
+	protected AbstractTreeIterator enterSubtree() {
+		return new FileTreeIterator(this, ((FileEntry) current()).getFile(), fs,
+				fileModeStrategy);
 	}
 
 	private Entry[] entries() {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
index 10e9196411..179fd46791 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java
@@ -256,6 +256,45 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 		state.dirCacheTree = treeId;
 	}
 
+	/**
+	 * Retrieves the {@link DirCacheIterator} at the current entry if
+	 * {@link #setDirCacheIterator(TreeWalk, int)} was called.
+	 *
+	 * @return the DirCacheIterator, or {@code null} if not set or not at the
+	 *         current entry
+	 * @since 5.0
+	 */
+	protected DirCacheIterator getDirCacheIterator() {
+		if (state.dirCacheTree >= 0 && state.walk != null) {
+			return state.walk.getTree(state.dirCacheTree,
+					DirCacheIterator.class);
+		}
+		return null;
+	}
+
+	/**
+	 * Defines whether this {@link WorkingTreeIterator} walks ignored
+	 * directories.
+	 *
+	 * @param includeIgnored
+	 *            {@code false} to skip ignored directories, if possible;
+	 *            {@code true} to always include them in the walk
+	 * @since 5.0
+	 */
+	public void setWalkIgnoredDirectories(boolean includeIgnored) {
+		state.walkIgnored = includeIgnored;
+	}
+
+	/**
+	 * Tells whether this {@link WorkingTreeIterator} walks ignored directories.
+	 *
+	 * @return {@code true} if it does, {@code false} otherwise
+	 * @since 5.0
+	 */
+	public boolean walksIgnoredDirectories() {
+		return state.walkIgnored;
+	}
+
 	/** {@inheritDoc} */
 	@Override
 	public boolean hasId() {
@@ -1365,7 +1404,10 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 		TreeWalk walk;
 
 		/** Position of the matching {@link DirCacheIterator}. */
-		int dirCacheTree;
+		int dirCacheTree = -1;
+
+		/** Whether the iterator shall walk ignored directories. */
+		boolean walkIgnored = false;
 
 		final Map<String, Boolean> directoryToIgnored = new HashMap<>();