Record submodule paths with untracked changes as FileMode.GITLINK

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

org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffSubmoduleTest.java

@@ -43,11 +43,13 @@
 
 package org.eclipse.jgit.lib;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Set;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.GitAPIException;
@@ -118,6 +120,31 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase {
 		assertTrue(indexDiff.diff());
 	}
 
+	private void assertDiff(IndexDiff indexDiff, IgnoreSubmoduleMode mode,
+			IgnoreSubmoduleMode... expectedEmptyModes) throws IOException {
+		boolean diffResult = indexDiff.diff();
+		Set<String> submodulePaths = indexDiff
+				.getPathsWithIndexMode(FileMode.GITLINK);
+		boolean emptyExpected = false;
+		for (IgnoreSubmoduleMode empty : expectedEmptyModes) {
+			if (mode.equals(empty)) {
+				emptyExpected = true;
+				break;
+			}
+		}
+		if (emptyExpected) {
+			assertFalse("diff should be false with mode=" + mode,
+					diffResult);
+			assertEquals("should have no paths with FileMode.GITLINK", 0,
+					submodulePaths.size());
+		} else {
+			assertTrue("diff should be true with mode=" + mode,
+					diffResult);
+			assertTrue("submodule path should have FileMode.GITLINK",
+					submodulePaths.contains("modules/submodule"));
+		}
+	}
+
 	@Theory
 	public void testDirtySubmoduleWorktree(IgnoreSubmoduleMode mode)
 			throws IOException {
@@ -125,13 +152,8 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase {
 		IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD,
 				new FileTreeIterator(db));
 		indexDiff.setIgnoreSubmoduleMode(mode);
-		if (mode.equals(IgnoreSubmoduleMode.ALL)
+		assertDiff(indexDiff, mode, IgnoreSubmoduleMode.ALL,
-				|| mode.equals(IgnoreSubmoduleMode.DIRTY))
+				IgnoreSubmoduleMode.DIRTY);
-			assertFalse("diff should be false with mode=" + mode,
-					indexDiff.diff());
-		else
-			assertTrue("diff should be true with mode=" + mode,
-					indexDiff.diff());
 	}
 
 	@Theory
@@ -145,12 +167,7 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase {
 		IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD,
 				new FileTreeIterator(db));
 		indexDiff.setIgnoreSubmoduleMode(mode);
-		if (mode.equals(IgnoreSubmoduleMode.ALL))
+		assertDiff(indexDiff, mode, IgnoreSubmoduleMode.ALL);
-			assertFalse("diff should be false with mode=" + mode,
-					indexDiff.diff());
-		else
-			assertTrue("diff should be true with mode=" + mode,
-					indexDiff.diff());
 	}
 
 	@Theory
@@ -163,13 +180,8 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase {
 		IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD,
 				new FileTreeIterator(db));
 		indexDiff.setIgnoreSubmoduleMode(mode);
-		if (mode.equals(IgnoreSubmoduleMode.ALL)
+		assertDiff(indexDiff, mode, IgnoreSubmoduleMode.ALL,
-				|| mode.equals(IgnoreSubmoduleMode.DIRTY))
+				IgnoreSubmoduleMode.DIRTY);
-			assertFalse("diff should be false with mode=" + mode,
-					indexDiff.diff());
-		else
-			assertTrue("diff should be true with mode=" + mode,
-					indexDiff.diff());
 	}
 
 	@Theory
@@ -183,13 +195,8 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase {
 		IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD,
 				new FileTreeIterator(db));
 		indexDiff.setIgnoreSubmoduleMode(mode);
-		if (mode.equals(IgnoreSubmoduleMode.ALL)
+		assertDiff(indexDiff, mode, IgnoreSubmoduleMode.ALL,
-				|| mode.equals(IgnoreSubmoduleMode.DIRTY))
+				IgnoreSubmoduleMode.DIRTY);
-			assertFalse("diff should be false with mode=" + mode,
-					indexDiff.diff());
-		else
-			assertTrue("diff should be true with mode=" + mode,
-					indexDiff.diff());
 	}
 
 	@Theory
@@ -200,13 +207,7 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase {
 		IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD,
 				new FileTreeIterator(db));
 		indexDiff.setIgnoreSubmoduleMode(mode);
-		if (mode.equals(IgnoreSubmoduleMode.ALL)
+		assertDiff(indexDiff, mode, IgnoreSubmoduleMode.ALL,
-				|| mode.equals(IgnoreSubmoduleMode.DIRTY)
+				IgnoreSubmoduleMode.DIRTY, IgnoreSubmoduleMode.UNTRACKED);
-				|| mode.equals(IgnoreSubmoduleMode.UNTRACKED))
-			assertFalse("diff should be false with mode=" + mode,
-					indexDiff.diff());
-		else
-			assertTrue("diff should be true with mode=" + mode,
-					indexDiff.diff());
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java

@@ -513,14 +513,10 @@ public class IndexDiff {
 					}
 				}
 
-				for (int i = 0; i < treeWalk.getTreeCount(); i++) {
-					Set<String> values = fileModes.get(treeWalk.getFileMode(i));
 				String path = treeWalk.getPathString();
 				if (path != null) {
-						if (values == null)
+					for (int i = 0; i < treeWalk.getTreeCount(); i++) {
-							values = new HashSet<>();
+						recordFileMode(path, treeWalk.getFileMode(i));
-						values.add(path);
-						fileModes.put(treeWalk.getFileMode(i), values);
 					}
 				}
 			}
@@ -545,19 +541,21 @@ public class IndexDiff {
 				}
 				Repository subRepo = smw.getRepository();
 				if (subRepo != null) {
+					String subRepoPath = smw.getPath();
 					try {
 						ObjectId subHead = subRepo.resolve("HEAD"); //$NON-NLS-1$
 						if (subHead != null
-								&& !subHead.equals(smw.getObjectId()))
+								&& !subHead.equals(smw.getObjectId())) {
-							modified.add(smw.getPath());
+							modified.add(subRepoPath);
-						else if (ignoreSubmoduleMode != IgnoreSubmoduleMode.DIRTY) {
+							recordFileMode(subRepoPath, FileMode.GITLINK);
+						} else if (ignoreSubmoduleMode != IgnoreSubmoduleMode.DIRTY) {
 							IndexDiff smid = submoduleIndexDiffs.get(smw
 									.getPath());
 							if (smid == null) {
 								smid = new IndexDiff(subRepo,
 										smw.getObjectId(),
 										wTreeIt.getWorkingTreeIterator(subRepo));
-								submoduleIndexDiffs.put(smw.getPath(), smid);
+								submoduleIndexDiffs.put(subRepoPath, smid);
 							}
 							if (smid.diff()) {
 								if (ignoreSubmoduleMode == IgnoreSubmoduleMode.UNTRACKED
@@ -569,7 +567,8 @@ public class IndexDiff {
 										&& smid.getRemoved().isEmpty()) {
 									continue;
 								}
-								modified.add(smw.getPath());
+								modified.add(subRepoPath);
+								recordFileMode(subRepoPath, FileMode.GITLINK);
 							}
 						}
 					} finally {
@@ -593,6 +592,17 @@ public class IndexDiff {
 			return true;
 	}
 
+	private void recordFileMode(String path, FileMode mode) {
+		Set<String> values = fileModes.get(mode);
+		if (path != null) {
+			if (values == null) {
+				values = new HashSet<>();
+				fileModes.put(mode, values);
+			}
+			values.add(path);
+		}
+	}
+
 	private boolean isEntryGitLink(AbstractTreeIterator ti) {
 		return ((ti != null) && (ti.getEntryRawMode() == FileMode.GITLINK
 				.getBits()));