Paths.pathCompare: Utility to sort paths from byte[]

Consolidate copies of this function into one location.

Add some unit tests to prevent bugs that were accidentally
introduced while trying to make this refactoring.

Change-Id: I82f64bbb8601ca2d8316ca57ae8119df32bb5c08

org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java

@@ -214,6 +214,23 @@ public class DirCachePathEditTest {
 		assertEquals("e", dc.getEntry(4).getPathString());
 	}
 
+	@Test
+	public void testDuplicateFiles() throws Exception {
+		DirCache dc = DirCache.newInCore();
+		DirCacheEditor editor = dc.editor();
+		editor.add(new AddEdit("a"));
+		editor.add(new AddEdit("a"));
+
+		try {
+			editor.finish();
+			fail("Expected DirCacheNameConflictException to be thrown");
+		} catch (DirCacheNameConflictException e) {
+			assertEquals("a a", e.getMessage());
+			assertEquals("a", e.getPath1());
+			assertEquals("a", e.getPath2());
+		}
+	}
+
 	@Test
 	public void testFileOverlapsTree() throws Exception {
 		DirCache dc = DirCache.newInCore();

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

@@ -1327,7 +1327,8 @@ public class ObjectCheckerTest {
 	}
 
 	@Test
-	public void testInvalidTreeDuplicateNames1() throws CorruptObjectException {
+	public void testInvalidTreeDuplicateNames1_File()
+			throws CorruptObjectException {
 		StringBuilder b = new StringBuilder();
 		entry(b, "100644 a");
 		entry(b, "100644 a");
@@ -1338,6 +1339,19 @@ public class ObjectCheckerTest {
 		checker.checkTree(data);
 	}
 
+	@Test
+	public void testInvalidTreeDuplicateNames1_Tree()
+			throws CorruptObjectException {
+		StringBuilder b = new StringBuilder();
+		entry(b, "40000 a");
+		entry(b, "40000 a");
+		byte[] data = encodeASCII(b.toString());
+		assertCorrupt("duplicate entry names", OBJ_TREE, data);
+		assertSkipListAccepts(OBJ_TREE, data);
+		checker.setIgnore(DUPLICATE_ENTRIES, true);
+		checker.checkTree(data);
+	}
+
 	@Test
 	public void testInvalidTreeDuplicateNames2() throws CorruptObjectException {
 		StringBuilder b = new StringBuilder();

org.eclipse.jgit.test/tst/org/eclipse/jgit/util/PathsTest.java

@@ -43,9 +43,13 @@
 
 package org.eclipse.jgit.util;
 
+import static org.eclipse.jgit.util.Paths.compare;
+import static org.eclipse.jgit.util.Paths.compareSameName;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.FileMode;
 import org.junit.Test;
 
 public class PathsTest {
@@ -59,4 +63,56 @@ public class PathsTest {
 		assertEquals("a/boo", Paths.stripTrailingSeparator("a/boo//"));
 		assertEquals("a/boo", Paths.stripTrailingSeparator("a/boo///"));
 	}
+
+	@Test
+	public void testPathCompare() {
+		byte[] a = Constants.encode("afoo/bar.c");
+		byte[] b = Constants.encode("bfoo/bar.c");
+
+		assertEquals(0, compare(a, 1, a.length, 0, b, 1, b.length, 0));
+		assertEquals(-1, compare(a, 0, a.length, 0, b, 0, b.length, 0));
+		assertEquals(1, compare(b, 0, b.length, 0, a, 0, a.length, 0));
+
+		a = Constants.encode("a");
+		b = Constants.encode("aa");
+		assertEquals(-97, compare(a, 0, a.length, 0, b, 0, b.length, 0));
+		assertEquals(0, compare(a, 0, a.length, 0, b, 0, 1, 0));
+		assertEquals(0, compare(a, 0, a.length, 0, b, 1, 2, 0));
+		assertEquals(0, compareSameName(a, 0, a.length, b, 1, b.length, 0));
+		assertEquals(0, compareSameName(a, 0, a.length, b, 0, 1, 0));
+		assertEquals(-50, compareSameName(a, 0, a.length, b, 0, b.length, 0));
+		assertEquals(97, compareSameName(b, 0, b.length, a, 0, a.length, 0));
+
+		a = Constants.encode("a");
+		b = Constants.encode("a");
+		assertEquals(0, compare(
+				a, 0, a.length, FileMode.TREE.getBits(),
+				b, 0, b.length, FileMode.TREE.getBits()));
+		assertEquals(0, compare(
+				a, 0, a.length, FileMode.REGULAR_FILE.getBits(),
+				b, 0, b.length, FileMode.REGULAR_FILE.getBits()));
+		assertEquals(-47, compare(
+				a, 0, a.length, FileMode.REGULAR_FILE.getBits(),
+				b, 0, b.length, FileMode.TREE.getBits()));
+		assertEquals(47, compare(
+				a, 0, a.length, FileMode.TREE.getBits(),
+				b, 0, b.length, FileMode.REGULAR_FILE.getBits()));
+
+		assertEquals(0, compareSameName(
+				a, 0, a.length,
+				b, 0, b.length, FileMode.TREE.getBits()));
+		assertEquals(0, compareSameName(
+				a, 0, a.length,
+				b, 0, b.length, FileMode.REGULAR_FILE.getBits()));
+
+		a = Constants.encode("a.c");
+		b = Constants.encode("a");
+		byte[] c = Constants.encode("a0c");
+		assertEquals(-1, compare(
+				a, 0, a.length, FileMode.REGULAR_FILE.getBits(),
+				b, 0, b.length, FileMode.TREE.getBits()));
+		assertEquals(-1, compare(
+				b, 0, b.length, FileMode.TREE.getBits(),
+				c, 0, c.length, FileMode.REGULAR_FILE.getBits()));
+	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java

@@ -44,8 +44,8 @@
 
 package org.eclipse.jgit.dircache;
 
-import static org.eclipse.jgit.lib.FileMode.TREE;
 import static org.eclipse.jgit.lib.FileMode.TYPE_TREE;
+import static org.eclipse.jgit.util.Paths.compareSameName;
 
 import java.io.IOException;
 
@@ -207,8 +207,8 @@ abstract class BaseDirCacheEditor {
 
 				int s = nextSlash(nPath, prefixLen);
 				int m = s < nPath.length ? TYPE_TREE : n.getRawMode();
-				int cmp = pathCompare(
-						ePath, prefixLen, ePath.length, TYPE_TREE,
+				int cmp = compareSameName(
+						ePath, prefixLen, ePath.length,
 						nPath, prefixLen, s, m);
 				if (cmp < 0) {
 					break;
@@ -252,28 +252,6 @@ abstract class BaseDirCacheEditor {
 		return true;
 	}
 
-	static int pathCompare(byte[] aPath, int aPos, int aEnd, int aMode,
-			byte[] bPath, int bPos, int bEnd, int bMode) {
-		while (aPos < aEnd && bPos < bEnd) {
-			int cmp = (aPath[aPos++] & 0xff) - (bPath[bPos++] & 0xff);
-			if (cmp != 0) {
-				return cmp;
-			}
-		}
-
-		if (aPos < aEnd) {
-			return (aPath[aPos] & 0xff) - lastPathChar(bMode);
-		}
-		if (bPos < bEnd) {
-			return lastPathChar(aMode) - (bPath[bPos] & 0xff);
-		}
-		return 0;
-	}
-
-	private static int lastPathChar(int mode) {
-		return TREE.equals(mode) ? '/' : '\0';
-	}
-
 	/**
 	 * Finish, write, commit this change, and release the index lock.
 	 * <p>

org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEditor.java

@@ -57,6 +57,7 @@ import java.util.List;
 
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.util.Paths;
 
 /**
  * Updates a {@link DirCache} by supplying discrete edit commands.
@@ -215,7 +216,7 @@ public class DirCacheEditor extends BaseDirCacheEditor {
 		}
 
 		DirCacheEntry next = cache.getEntry(eIdx);
-		if (pathCompare(next.path, 0, next.path.length, 0,
+		if (Paths.compare(next.path, 0, next.path.length, 0,
 				entPath, 0, entLen, TYPE_TREE) < 0) {
 			// Next DirCacheEntry sorts before new entry as tree. Defer a
 			// DeleteTree command to delete any entries if they exist. This

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

@@ -77,6 +77,8 @@ import static org.eclipse.jgit.lib.ObjectChecker.ErrorType.TREE_NOT_SORTED;
 import static org.eclipse.jgit.lib.ObjectChecker.ErrorType.UNKNOWN_TYPE;
 import static org.eclipse.jgit.lib.ObjectChecker.ErrorType.WIN32_BAD_NAME;
 import static org.eclipse.jgit.lib.ObjectChecker.ErrorType.ZERO_PADDED_FILEMODE;
+import static org.eclipse.jgit.util.Paths.compare;
+import static org.eclipse.jgit.util.Paths.compareSameName;
 import static org.eclipse.jgit.util.RawParseUtils.nextLF;
 import static org.eclipse.jgit.util.RawParseUtils.parseBase10;
 
@@ -541,25 +543,6 @@ public class ObjectChecker {
 		}
 	}
 
-	private static int lastPathChar(final int mode) {
-		return FileMode.TREE.equals(mode) ? '/' : '\0';
-	}
-
-	private static int pathCompare(final byte[] raw, int aPos, final int aEnd,
-			final int aMode, int bPos, final int bEnd, final int bMode) {
-		while (aPos < aEnd && bPos < bEnd) {
-			final int cmp = (raw[aPos++] & 0xff) - (raw[bPos++] & 0xff);
-			if (cmp != 0)
-				return cmp;
-		}
-
-		if (aPos < aEnd)
-			return (raw[aPos] & 0xff) - lastPathChar(bMode);
-		if (bPos < bEnd)
-			return lastPathChar(aMode) - (raw[bPos] & 0xff);
-		return 0;
-	}
-
 	private static boolean duplicateName(final byte[] raw,
 			final int thisNamePos, final int thisNameEnd) {
 		final int sz = raw.length;
@@ -587,8 +570,9 @@ public class ObjectChecker {
 			if (nextNamePos + 1 == nextPtr)
 				return false;
 
-			final int cmp = pathCompare(raw, thisNamePos, thisNameEnd,
-					FileMode.TREE.getBits(), nextNamePos, nextPtr - 1, nextMode);
+			int cmp = compareSameName(
+					raw, thisNamePos, thisNameEnd,
+					raw, nextNamePos, nextPtr - 1, nextMode);
 			if (cmp < 0)
 				return false;
 			else if (cmp == 0)
@@ -676,8 +660,9 @@ public class ObjectChecker {
 			}
 
 			if (lastNameB != 0) {
-				final int cmp = pathCompare(raw, lastNameB, lastNameE,
-						lastMode, thisNameB, ptr, thisMode);
+				int cmp = compare(
+						raw, lastNameB, lastNameE, lastMode,
+						raw, thisNameB, ptr, thisMode);
 				if (cmp > 0) {
 					report(TREE_NOT_SORTED, id,
 							JGitText.get().corruptObjectIncorrectSorting);

org.eclipse.jgit/src/org/eclipse/jgit/notes/NonNoteEntry.java

@@ -47,6 +47,7 @@ import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.FileMode;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.TreeFormatter;
+import org.eclipse.jgit.util.Paths;
 
 /** A tree entry found in a note branch that isn't a valid note. */
 class NonNoteEntry extends ObjectId {
@@ -74,27 +75,8 @@ class NonNoteEntry extends ObjectId {
 	}
 
 	int pathCompare(byte[] bBuf, int bPos, int bLen, FileMode bMode) {
-		return pathCompare(name, 0, name.length, mode, //
-				bBuf, bPos, bLen, bMode);
-	}
-
-	private static int pathCompare(final byte[] aBuf, int aPos, final int aEnd,
-			final FileMode aMode, final byte[] bBuf, int bPos, final int bEnd,
-			final FileMode bMode) {
-		while (aPos < aEnd && bPos < bEnd) {
-			int cmp = (aBuf[aPos++] & 0xff) - (bBuf[bPos++] & 0xff);
-			if (cmp != 0)
-				return cmp;
-		}
-
-		if (aPos < aEnd)
-			return (aBuf[aPos] & 0xff) - lastPathChar(bMode);
-		if (bPos < bEnd)
-			return lastPathChar(aMode) - (bBuf[bPos] & 0xff);
-		return 0;
-	}
-
-	private static int lastPathChar(final FileMode mode) {
-		return FileMode.TREE.equals(mode.getBits()) ? '/' : '\0';
+		return Paths.compare(
+				name, 0, name.length, mode.getBits(),
+				bBuf, bPos, bLen, bMode.getBits());
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java

@@ -59,6 +59,7 @@ import org.eclipse.jgit.lib.MutableObjectId;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.treewalk.filter.TreeFilter;
+import org.eclipse.jgit.util.Paths;
 
 /**
  * Walks a Git tree (directory) in Git sort order.
@@ -382,20 +383,9 @@ public abstract class AbstractTreeIterator {
 	}
 
 	private int pathCompare(byte[] b, int bPos, int bEnd, int bMode, int aPos) {
-		final byte[] a = path;
-		final int aEnd = pathLen;
-
-		for (; aPos < aEnd && bPos < bEnd; aPos++, bPos++) {
-			final int cmp = (a[aPos] & 0xff) - (b[bPos] & 0xff);
-			if (cmp != 0)
-				return cmp;
-		}
-
-		if (aPos < aEnd)
-			return (a[aPos] & 0xff) - lastPathChar(bMode);
-		if (bPos < bEnd)
-			return lastPathChar(mode) - (b[bPos] & 0xff);
-		return lastPathChar(mode) - lastPathChar(bMode);
+		return Paths.compare(
+				path, aPos, pathLen, mode,
+				b, bPos, bEnd, bMode);
 	}
 
 	private static int alreadyMatch(AbstractTreeIterator a,
@@ -412,10 +402,6 @@ public abstract class AbstractTreeIterator {
 		}
 	}
 
-	private static int lastPathChar(final int mode) {
-		return FileMode.TREE.equals(mode) ? '/' : '\0';
-	}
-
 	/**
 	 * Check if the current entry of both iterators has the same id.
 	 * <p>

org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java

@@ -89,6 +89,7 @@ import org.eclipse.jgit.submodule.SubmoduleWalk;
 import org.eclipse.jgit.util.FS;
 import org.eclipse.jgit.util.FS.ExecutionResult;
 import org.eclipse.jgit.util.IO;
+import org.eclipse.jgit.util.Paths;
 import org.eclipse.jgit.util.RawParseUtils;
 import org.eclipse.jgit.util.io.EolCanonicalizingInputStream;
 
@@ -692,31 +693,13 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator {
 	}
 
 	private static final Comparator<Entry> ENTRY_CMP = new Comparator<Entry>() {
-		public int compare(final Entry o1, final Entry o2) {
-			final byte[] a = o1.encodedName;
-			final byte[] b = o2.encodedName;
-			final int aLen = o1.encodedNameLen;
-			final int bLen = o2.encodedNameLen;
-			int cPos;
-
-			for (cPos = 0; cPos < aLen && cPos < bLen; cPos++) {
-				final int cmp = (a[cPos] & 0xff) - (b[cPos] & 0xff);
-				if (cmp != 0)
-					return cmp;
-			}
-
-			if (cPos < aLen)
-				return (a[cPos] & 0xff) - lastPathChar(o2);
-			if (cPos < bLen)
-				return lastPathChar(o1) - (b[cPos] & 0xff);
-			return lastPathChar(o1) - lastPathChar(o2);
+		public int compare(Entry a, Entry b) {
+			return Paths.compare(
+					a.encodedName, 0, a.encodedNameLen, a.getMode().getBits(),
+					b.encodedName, 0, b.encodedNameLen, b.getMode().getBits());
 		}
 	};
 
-	static int lastPathChar(final Entry e) {
-		return e.getMode() == FileMode.TREE ? '/' : '\0';
-	}
-
 	/**
 	 * Constructor helper.
 	 *

org.eclipse.jgit/src/org/eclipse/jgit/util/Paths.java

@@ -43,6 +43,9 @@
 
 package org.eclipse.jgit.util;
 
+import static org.eclipse.jgit.lib.FileMode.TYPE_MASK;
+import static org.eclipse.jgit.lib.FileMode.TYPE_TREE;
+
 /**
  * Utility functions for paths inside of a Git repository.
  *
@@ -72,6 +75,114 @@ public class Paths {
 		return path.substring(0, i);
 	}
 
+	/**
+	 * Compare two paths according to Git path sort ordering rules.
+	 *
+	 * @param aPath
+	 *            first path buffer. The range {@code [aPos, aEnd)} is used.
+	 * @param aPos
+	 *            index into {@code aPath} where the first path starts.
+	 * @param aEnd
+	 *            1 past last index of {@code aPath}.
+	 * @param aMode
+	 *            mode of the first file. Trees are sorted as though
+	 *            {@code aPath[aEnd] == '/'}, even if aEnd does not exist.
+	 * @param bPath
+	 *            second path buffer. The range {@code [bPos, bEnd)} is used.
+	 * @param bPos
+	 *            index into {@code bPath} where the second path starts.
+	 * @param bEnd
+	 *            1 past last index of {@code bPath}.
+	 * @param bMode
+	 *            mode of the second file. Trees are sorted as though
+	 *            {@code bPath[bEnd] == '/'}, even if bEnd does not exist.
+	 * @return <0 if {@code aPath} sorts before {@code bPath};
+	 *         0 if the paths are the same;
+	 *         >0 if {@code aPath} sorts after {@code bPath}.
+	 */
+	public static int compare(byte[] aPath, int aPos, int aEnd, int aMode,
+			byte[] bPath, int bPos, int bEnd, int bMode) {
+		int cmp = coreCompare(
+				aPath, aPos, aEnd, aMode,
+				bPath, bPos, bEnd, bMode);
+		if (cmp == 0) {
+			cmp = lastPathChar(aMode) - lastPathChar(bMode);
+		}
+		return cmp;
+	}
+
+	/**
+	 * Compare two paths, checking for identical name.
+	 * <p>
+	 * Unlike {@code compare} this method returns {@code 0} when the paths have
+	 * the same characters in their names, even if the mode differs. It is
+	 * intended for use in validation routines detecting duplicate entries.
+	 * <p>
+	 * Returns {@code 0} if the names are identical and a conflict exists
+	 * between {@code aPath} and {@code bPath}, as they share the same name.
+	 * <p>
+	 * Returns {@code <0} if all possibles occurrences of {@code aPath} sort
+	 * before {@code bPath} and no conflict can happen. In a properly sorted
+	 * tree there are no other occurrences of {@code aPath} and therefore there
+	 * are no duplicate names.
+	 * <p>
+	 * Returns {@code >0} when it is possible for a duplicate occurrence of
+	 * {@code aPath} to appear later, after {@code bPath}. Callers should
+	 * continue to examine candidates for {@code bPath} until the method returns
+	 * one of the other return values.
+	 *
+	 * @param aPath
+	 *            first path buffer. The range {@code [aPos, aEnd)} is used.
+	 * @param aPos
+	 *            index into {@code aPath} where the first path starts.
+	 * @param aEnd
+	 *            1 past last index of {@code aPath}.
+	 * @param bPath
+	 *            second path buffer. The range {@code [bPos, bEnd)} is used.
+	 * @param bPos
+	 *            index into {@code bPath} where the second path starts.
+	 * @param bEnd
+	 *            1 past last index of {@code bPath}.
+	 * @param bMode
+	 *            mode of the second file. Trees are sorted as though
+	 *            {@code bPath[bEnd] == '/'}, even if bEnd does not exist.
+	 * @return &lt;0 if no duplicate name could exist;
+	 *         0 if the paths have the same name;
+	 *         &gt;0 other {@code bPath} should still be checked by caller.
+	 */
+	public static int compareSameName(
+			byte[] aPath, int aPos, int aEnd,
+			byte[] bPath, int bPos, int bEnd, int bMode) {
+		return coreCompare(
+				aPath, aPos, aEnd, TYPE_TREE,
+				bPath, bPos, bEnd, bMode);
+	}
+
+	private static int coreCompare(
+			byte[] aPath, int aPos, int aEnd, int aMode,
+			byte[] bPath, int bPos, int bEnd, int bMode) {
+		while (aPos < aEnd && bPos < bEnd) {
+			int cmp = (aPath[aPos++] & 0xff) - (bPath[bPos++] & 0xff);
+			if (cmp != 0) {
+				return cmp;
+			}
+		}
+		if (aPos < aEnd) {
+			return (aPath[aPos] & 0xff) - lastPathChar(bMode);
+		}
+		if (bPos < bEnd) {
+			return lastPathChar(aMode) - (bPath[bPos] & 0xff);
+		}
+		return 0;
+	}
+
+	private static int lastPathChar(int mode) {
+		if ((mode & TYPE_MASK) == TYPE_TREE) {
+			return '/';
+		}
+		return 0;
+	}
+
 	private Paths() {
 	}
 }