From 3776b14ab45ca1daf0771fa1a8245bc097ec2e12 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 24 Dec 2015 14:27:50 -0800 Subject: [PATCH 1/3] AddCommand: Use NameConflictTreeWalk to identify file-dir changes Adding a path that already exists but is changing type such as from symlink to subdirectory requires a NameConflictTreeWalk to match up the two different entry types that share the same name. NameConflictTreeWalk needs a bug fix to pop conflicting entries when PathFilterGroup aborts the walk early so that it does not allow DirCacheBuilderIterator to copy conflicting entries into the output cache. Change-Id: I61b49cbe949ca8b4b98f9eb6dbe7b1f82eabb724 --- .../org/eclipse/jgit/api/AddCommandTest.java | 98 ++++++++++++++++++- .../src/org/eclipse/jgit/api/AddCommand.java | 23 ++++- .../jgit/dircache/DirCacheBuildIterator.java | 5 + .../jgit/treewalk/AbstractTreeIterator.java | 8 ++ .../jgit/treewalk/EmptyTreeIterator.java | 5 + .../jgit/treewalk/NameConflictTreeWalk.java | 37 +++++++ .../org/eclipse/jgit/treewalk/TreeWalk.java | 26 ++++- 7 files changed, 192 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java index a5ad18d10..29fc3c3a0 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java @@ -43,6 +43,7 @@ */ package org.eclipse.jgit.api; +import static org.eclipse.jgit.util.FileUtils.RECURSIVE; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -777,12 +778,107 @@ public void testAssumeUnchanged() throws Exception { assertEquals("[a.txt, mode:100644, content:more content," + " assume-unchanged:false][b.txt, mode:100644," - + "" + "" + " content:content, assume-unchanged:true]", indexState(CONTENT | ASSUME_UNCHANGED)); } + @Test + public void testReplaceFileWithDirectory() + throws IOException, NoFilepatternException, GitAPIException { + try (Git git = new Git(db)) { + writeTrashFile("df", "before replacement"); + git.add().addFilepattern("df").call(); + assertEquals("[df, mode:100644, content:before replacement]", + indexState(CONTENT)); + FileUtils.delete(new File(db.getWorkTree(), "df")); + writeTrashFile("df/f", "after replacement"); + git.add().addFilepattern("df").call(); + assertEquals("[df/f, mode:100644, content:after replacement]", + indexState(CONTENT)); + } + } + + @Test + public void testReplaceDirectoryWithFile() + throws IOException, NoFilepatternException, GitAPIException { + try (Git git = new Git(db)) { + writeTrashFile("df/f", "before replacement"); + git.add().addFilepattern("df").call(); + assertEquals("[df/f, mode:100644, content:before replacement]", + indexState(CONTENT)); + FileUtils.delete(new File(db.getWorkTree(), "df"), RECURSIVE); + writeTrashFile("df", "after replacement"); + git.add().addFilepattern("df").call(); + assertEquals("[df, mode:100644, content:after replacement]", + indexState(CONTENT)); + } + } + + @Test + public void testReplaceFileByPartOfDirectory() + throws IOException, NoFilepatternException, GitAPIException { + try (Git git = new Git(db)) { + writeTrashFile("src/main", "df", "before replacement"); + writeTrashFile("src/main", "z", "z"); + writeTrashFile("z", "z2"); + git.add().addFilepattern("src/main/df") + .addFilepattern("src/main/z") + .addFilepattern("z") + .call(); + assertEquals( + "[src/main/df, mode:100644, content:before replacement]" + + "[src/main/z, mode:100644, content:z]" + + "[z, mode:100644, content:z2]", + indexState(CONTENT)); + FileUtils.delete(new File(db.getWorkTree(), "src/main/df")); + writeTrashFile("src/main/df", "a", "after replacement"); + writeTrashFile("src/main/df", "b", "unrelated file"); + git.add().addFilepattern("src/main/df/a").call(); + assertEquals( + "[src/main/df/a, mode:100644, content:after replacement]" + + "[src/main/z, mode:100644, content:z]" + + "[z, mode:100644, content:z2]", + indexState(CONTENT)); + } + } + + @Test + public void testReplaceDirectoryConflictsWithFile() + throws IOException, NoFilepatternException, GitAPIException { + DirCache dc = db.lockDirCache(); + try (ObjectInserter oi = db.newObjectInserter()) { + DirCacheBuilder builder = dc.builder(); + File f = writeTrashFile("a", "df", "content"); + addEntryToBuilder("a", f, oi, builder, 1); + + f = writeTrashFile("a", "df", "other content"); + addEntryToBuilder("a/df", f, oi, builder, 3); + + f = writeTrashFile("a", "df", "our content"); + addEntryToBuilder("a/df", f, oi, builder, 2); + + f = writeTrashFile("z", "z"); + addEntryToBuilder("z", f, oi, builder, 0); + builder.commit(); + } + assertEquals( + "[a, mode:100644, stage:1, content:content]" + + "[a/df, mode:100644, stage:2, content:our content]" + + "[a/df, mode:100644, stage:3, content:other content]" + + "[z, mode:100644, content:z]", + indexState(CONTENT)); + + try (Git git = new Git(db)) { + FileUtils.delete(new File(db.getWorkTree(), "a"), RECURSIVE); + writeTrashFile("a", "merged"); + git.add().addFilepattern("a").call(); + assertEquals("[a, mode:100644, content:merged]" + + "[z, mode:100644, content:z]", + indexState(CONTENT)); + } + } + @Test public void testExecutableRetention() throws Exception { StoredConfig config = db.getConfig(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/AddCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/AddCommand.java index 8a2c08c80..3b94f16f1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/AddCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/AddCommand.java @@ -45,6 +45,7 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.eclipse.jgit.lib.FileMode.GITLINK; +import static org.eclipse.jgit.lib.FileMode.TYPE_TREE; import java.io.IOException; import java.io.InputStream; @@ -66,7 +67,7 @@ import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.treewalk.FileTreeIterator; -import org.eclipse.jgit.treewalk.TreeWalk; +import org.eclipse.jgit.treewalk.NameConflictTreeWalk; import org.eclipse.jgit.treewalk.TreeWalk.OperationType; import org.eclipse.jgit.treewalk.WorkingTreeIterator; import org.eclipse.jgit.treewalk.filter.PathFilterGroup; @@ -141,7 +142,7 @@ public DirCache call() throws GitAPIException, NoFilepatternException { boolean addAll = filepatterns.contains("."); //$NON-NLS-1$ try (ObjectInserter inserter = repo.newObjectInserter(); - final TreeWalk tw = new TreeWalk(repo)) { + NameConflictTreeWalk tw = new NameConflictTreeWalk(repo)) { tw.setOperationType(OperationType.CHECKIN_OP); dc = repo.lockDirCache(); @@ -151,7 +152,6 @@ public DirCache call() throws GitAPIException, NoFilepatternException { workingTreeIterator = new FileTreeIterator(repo); workingTreeIterator.setDirCacheIterator(tw, 0); tw.addTree(workingTreeIterator); - tw.setRecursive(true); if (!addAll) tw.setFilter(PathFilterGroup.createFromStrings(filepatterns)); @@ -180,9 +180,14 @@ public DirCache call() throws GitAPIException, NoFilepatternException { continue; } + if (tw.isSubtree() && !tw.isDirectoryFileConflict()) { + tw.enterSubtree(); + continue; + } + if (f == null) { // working tree file does not exist - if (c != null - && (!update || GITLINK == c.getEntryFileMode())) { + if (entry != null + && (!update || GITLINK == entry.getFileMode())) { builder.add(entry); } continue; @@ -196,6 +201,14 @@ public DirCache call() throws GitAPIException, NoFilepatternException { continue; } + if (f.getEntryRawMode() == TYPE_TREE) { + // Index entry exists and is symlink, gitlink or file, + // otherwise the tree would have been entered above. + // Replace the index entry by diving into tree of files. + tw.enterSubtree(); + continue; + } + byte[] path = tw.getRawPath(); if (entry == null || entry.getStage() > 0) { entry = new DirCacheEntry(path); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheBuildIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheBuildIterator.java index da5530666..c10e41608 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheBuildIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheBuildIterator.java @@ -130,4 +130,9 @@ public void stopWalk() { if (cur < cnt) builder.keep(cur, cnt - cur); } + + @Override + protected boolean needsStopWalk() { + return ptr < cache.getEntryCount(); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java index 5e7188957..27d0f7b58 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java @@ -691,6 +691,14 @@ public void stopWalk() { // Do nothing by default. Most iterators do not care. } + /** + * @return true if the iterator implements {@link #stopWalk()}. + * @since 4.2 + */ + protected boolean needsStopWalk() { + return false; + } + /** * @return the length of the name component of the path for the current entry */ diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/EmptyTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/EmptyTreeIterator.java index 8dbf80e6a..ec4a84eff 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/EmptyTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/EmptyTreeIterator.java @@ -142,4 +142,9 @@ public void stopWalk() { if (parent != null) parent.stopWalk(); } + + @Override + protected boolean needsStopWalk() { + return parent != null && parent.needsStopWalk(); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/NameConflictTreeWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/NameConflictTreeWalk.java index 350f56396..d2195a874 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/NameConflictTreeWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/NameConflictTreeWalk.java @@ -43,6 +43,8 @@ package org.eclipse.jgit.treewalk; +import java.io.IOException; + import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.lib.FileMode; @@ -338,6 +340,41 @@ void skipEntriesEqual() throws CorruptObjectException { dfConflict = null; } + void stopWalk() throws IOException { + if (!needsStopWalk()) { + return; + } + + // Name conflicts make aborting early difficult. Multiple paths may + // exist between the file and directory versions of a name. To ensure + // the directory version is skipped over (as it was previously visited + // during the file version step) requires popping up the stack and + // finishing out each subtree that the walker dove into. Siblings in + // parents do not need to be recursed into, bounding the cost. + for (;;) { + AbstractTreeIterator t = min(); + if (t.eof()) { + if (depth > 0) { + exitSubtree(); + popEntriesEqual(); + continue; + } + return; + } + currentHead = t; + skipEntriesEqual(); + } + } + + private boolean needsStopWalk() { + for (AbstractTreeIterator t : trees) { + if (t.needsStopWalk()) { + return true; + } + } + return false; + } + /** * True if the current entry is covered by a directory/file conflict. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java index 438549520..83fada4f9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java @@ -57,6 +57,7 @@ import org.eclipse.jgit.attributes.AttributesNode; import org.eclipse.jgit.attributes.AttributesNodeProvider; import org.eclipse.jgit.attributes.AttributesProvider; +import org.eclipse.jgit.dircache.DirCacheBuildIterator; import org.eclipse.jgit.dircache.DirCacheIterator; import org.eclipse.jgit.errors.CorruptObjectException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; @@ -256,7 +257,7 @@ public static TreeWalk forPath(final Repository db, final String path, private boolean postOrderTraversal; - private int depth; + int depth; private boolean advance; @@ -665,12 +666,29 @@ public boolean next() throws MissingObjectException, return true; } } catch (StopWalkException stop) { - for (final AbstractTreeIterator t : trees) - t.stopWalk(); + stopWalk(); return false; } } + /** + * Notify iterators the walk is aborting. + *

+ * Primarily to notify {@link DirCacheBuildIterator} the walk is aborting so + * that it can copy any remaining entries. + * + * @throws IOException + * if traversal of remaining entries throws an exception during + * object access. This should never occur as remaining trees + * should already be in memory, however the methods used to + * finish traversal are declared to throw IOException. + */ + void stopWalk() throws IOException { + for (AbstractTreeIterator t : trees) { + t.stopWalk(); + } + } + /** * Obtain the tree iterator for the current entry. *

@@ -1065,7 +1083,7 @@ void skipEntriesEqual() throws CorruptObjectException { } } - private void exitSubtree() { + void exitSubtree() { depth--; for (int i = 0; i < trees.length; i++) trees[i] = trees[i].parent; From b71ba69410db5e50dd10c5c40e96699c9de7e5e8 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Mon, 28 Dec 2015 11:43:07 -0800 Subject: [PATCH 2/3] DirCacheEditor: Replace file-with-tree and tree-with-file If a PathEdit tries to store a file where a subtree was, or a subtree where a file was, replace the entry in the DirCache with the new name(s). This supports switching between file and tree entry types using a DirCacheEditor. Add new unit tests to cover the conditions where these can happen. Change-Id: Ie843d9388825f9e3d918a5666aa04e47cd6306e7 --- .../jgit/dircache/DirCachePathEditTest.java | 58 +++++++ .../jgit/lib/DirCacheCheckoutTest.java | 6 +- .../jgit/dircache/BaseDirCacheEditor.java | 24 +++ .../org/eclipse/jgit/dircache/DirCache.java | 7 +- .../eclipse/jgit/dircache/DirCacheEditor.java | 162 +++++++++++++++++- .../eclipse/jgit/dircache/DirCacheEntry.java | 2 +- 6 files changed, 246 insertions(+), 13 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java index 63ec85861..3988f6a4c 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java @@ -154,6 +154,64 @@ public void testPathEditShouldBeCalledForEachStage() throws Exception { assertEquals(DirCacheEntry.STAGE_3, entries.get(2).getStage()); } + @Test + public void testFileReplacesTree() throws Exception { + DirCache dc = DirCache.newInCore(); + DirCacheEditor editor = dc.editor(); + editor.add(new AddEdit("a")); + editor.add(new AddEdit("b/c")); + editor.add(new AddEdit("b/d")); + editor.add(new AddEdit("e")); + editor.finish(); + + editor = dc.editor(); + editor.add(new AddEdit("b")); + editor.finish(); + + assertEquals(3, dc.getEntryCount()); + assertEquals("a", dc.getEntry(0).getPathString()); + assertEquals("b", dc.getEntry(1).getPathString()); + assertEquals("e", dc.getEntry(2).getPathString()); + + dc.clear(); + editor = dc.editor(); + editor.add(new AddEdit("A.c")); + editor.add(new AddEdit("A/c")); + editor.add(new AddEdit("A0c")); + editor.finish(); + + editor = dc.editor(); + editor.add(new AddEdit("A")); + editor.finish(); + assertEquals(3, dc.getEntryCount()); + assertEquals("A", dc.getEntry(0).getPathString()); + assertEquals("A.c", dc.getEntry(1).getPathString()); + assertEquals("A0c", dc.getEntry(2).getPathString()); + } + + @Test + public void testTreeReplacesFile() throws Exception { + DirCache dc = DirCache.newInCore(); + DirCacheEditor editor = dc.editor(); + editor.add(new AddEdit("a")); + editor.add(new AddEdit("ab")); + editor.add(new AddEdit("b")); + editor.add(new AddEdit("e")); + editor.finish(); + + editor = dc.editor(); + editor.add(new AddEdit("b/c/d/f")); + editor.add(new AddEdit("b/g/h/i")); + editor.finish(); + + assertEquals(5, dc.getEntryCount()); + assertEquals("a", dc.getEntry(0).getPathString()); + assertEquals("ab", dc.getEntry(1).getPathString()); + assertEquals("b/c/d/f", dc.getEntry(2).getPathString()); + assertEquals("b/g/h/i", dc.getEntry(3).getPathString()); + assertEquals("e", dc.getEntry(4).getPathString()); + } + private static DirCacheEntry createEntry(String path, int stage) { DirCacheEntry entry = new DirCacheEntry(path, stage); entry.setFileMode(FileMode.REGULAR_FILE); 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 d768e0fa0..92901f826 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 @@ -1084,7 +1084,7 @@ public void testCheckoutChangeLinkToNonEmptyDirsAndNewIndexEntry() assertWorkDir(mkmap(linkName, "a", fname, "a")); Status st = git.status().call(); - assertFalse(st.isClean()); + assertTrue(st.isClean()); } @Test @@ -1213,9 +1213,7 @@ public void testCheckoutChangeFileToNonEmptyDirsAndNewIndexEntry() assertWorkDir(mkmap(fname, "a")); Status st = git.status().call(); - assertFalse(st.isClean()); - assertEquals(1, st.getAdded().size()); - assertTrue(st.getAdded().contains(fname + "/dir/file1")); + assertTrue(st.isClean()); } @Test diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java index 70f80aeb7..c5c1b0d77 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java @@ -44,6 +44,8 @@ package org.eclipse.jgit.dircache; +import static org.eclipse.jgit.lib.FileMode.TREE; + import java.io.IOException; /** @@ -176,6 +178,28 @@ protected void replace() { cache.replace(entries, entryCnt); } + 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. *

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java index fa0339544..ecdfe823a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java @@ -800,8 +800,11 @@ public int findEntry(final String path) { * information. If < 0 the entry does not exist in the index. * @since 3.4 */ - public int findEntry(final byte[] p, final int pLen) { - int low = 0; + public int findEntry(byte[] p, int pLen) { + return findEntry(0, p, pLen); + } + + int findEntry(int low, byte[] p, int pLen) { int high = entryCnt; while (low < high) { int mid = (low + high) >>> 1; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEditor.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEditor.java index 932ef94db..889e19422 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEditor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEditor.java @@ -44,6 +44,10 @@ package org.eclipse.jgit.dircache; +import static org.eclipse.jgit.dircache.DirCache.cmp; +import static org.eclipse.jgit.dircache.DirCacheTree.peq; +import static org.eclipse.jgit.lib.FileMode.TYPE_TREE; + import java.io.IOException; import java.text.MessageFormat; import java.util.ArrayList; @@ -72,11 +76,12 @@ public class DirCacheEditor extends BaseDirCacheEditor { public int compare(final PathEdit o1, final PathEdit o2) { final byte[] a = o1.path; final byte[] b = o2.path; - return DirCache.cmp(a, a.length, b, b.length); + return cmp(a, a.length, b, b.length); } }; private final List edits; + private int editIdx; /** * Construct a new editor. @@ -126,37 +131,44 @@ public void finish() { private void applyEdits() { Collections.sort(edits, EDIT_CMP); + editIdx = 0; final int maxIdx = cache.getEntryCount(); int lastIdx = 0; - for (final PathEdit e : edits) { - int eIdx = cache.findEntry(e.path, e.path.length); + while (editIdx < edits.size()) { + PathEdit e = edits.get(editIdx++); + int eIdx = cache.findEntry(lastIdx, e.path, e.path.length); final boolean missing = eIdx < 0; if (eIdx < 0) eIdx = -(eIdx + 1); final int cnt = Math.min(eIdx, maxIdx) - lastIdx; if (cnt > 0) fastKeep(lastIdx, cnt); - lastIdx = missing ? eIdx : cache.nextEntry(eIdx); - if (e instanceof DeletePath) + if (e instanceof DeletePath) { + lastIdx = missing ? eIdx : cache.nextEntry(eIdx); continue; + } if (e instanceof DeleteTree) { lastIdx = cache.nextEntry(e.path, e.path.length, eIdx); continue; } if (missing) { - final DirCacheEntry ent = new DirCacheEntry(e.path); + DirCacheEntry ent = new DirCacheEntry(e.path); e.apply(ent); if (ent.getRawMode() == 0) { throw new IllegalArgumentException(MessageFormat.format( JGitText.get().fileModeNotSetForPath, ent.getPathString())); } + lastIdx = e.replace + ? deleteOverlappingSubtree(ent, eIdx) + : eIdx; fastAdd(ent); } else { // Apply to all entries of the current path (different stages) + lastIdx = cache.nextEntry(eIdx); for (int i = eIdx; i < lastIdx; i++) { final DirCacheEntry ent = cache.getEntry(i); e.apply(ent); @@ -170,6 +182,102 @@ private void applyEdits() { fastKeep(lastIdx, cnt); } + private int deleteOverlappingSubtree(DirCacheEntry ent, int eIdx) { + byte[] entPath = ent.path; + int entLen = entPath.length; + + // Delete any file that was previously processed and overlaps + // the parent directory for the new entry. Since the editor + // always processes entries in path order, binary search back + // for the overlap for each parent directory. + for (int p = pdir(entPath, entLen); p > 0; p = pdir(entPath, p)) { + int i = findEntry(entPath, p); + if (i >= 0) { + // A file does overlap, delete the file from the array. + // No other parents can have overlaps as the file should + // have taken care of that itself. + int n = --entryCnt - i; + System.arraycopy(entries, i + 1, entries, i, n); + break; + } + + // If at least one other entry already exists in this parent + // directory there is no need to continue searching up the tree. + i = -(i + 1); + if (i < entryCnt && inDir(entries[i], entPath, p)) { + break; + } + } + + int maxEnt = cache.getEntryCount(); + if (eIdx >= maxEnt) { + return maxEnt; + } + + DirCacheEntry next = cache.getEntry(eIdx); + if (pathCompare(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 + // case only happens for A, A.c, A/c type of conflicts (rare). + insertEdit(new DeleteTree(entPath)); + return eIdx; + } + + // Next entry may be contained by the entry-as-tree, skip if so. + while (eIdx < maxEnt && inDir(cache.getEntry(eIdx), entPath, entLen)) { + eIdx++; + } + return eIdx; + } + + private int findEntry(byte[] p, int pLen) { + int low = 0; + int high = entryCnt; + while (low < high) { + int mid = (low + high) >>> 1; + int cmp = cmp(p, pLen, entries[mid]); + if (cmp < 0) { + high = mid; + } else if (cmp == 0) { + while (mid > 0 && cmp(p, pLen, entries[mid - 1]) == 0) { + mid--; + } + return mid; + } else { + low = mid + 1; + } + } + return -(low + 1); + } + + private void insertEdit(DeleteTree d) { + for (int i = editIdx; i < edits.size(); i++) { + int cmp = EDIT_CMP.compare(d, edits.get(i)); + if (cmp < 0) { + edits.add(i, d); + return; + } else if (cmp == 0) { + return; + } + } + edits.add(d); + } + + private static boolean inDir(DirCacheEntry e, byte[] path, int pLen) { + return e.path.length > pLen && e.path[pLen] == '/' + && peq(path, e.path, pLen); + } + + private static int pdir(byte[] path, int e) { + for (e--; e > 0; e--) { + if (path[e] == '/') { + return e; + } + } + return 0; + } + /** * Any index record update. *

@@ -181,6 +289,7 @@ private void applyEdits() { */ public abstract static class PathEdit { final byte[] path; + boolean replace = true; /** * Create a new update command by path name. @@ -192,6 +301,10 @@ public PathEdit(final String entryPath) { path = Constants.encode(entryPath); } + PathEdit(byte[] path) { + this.path = path; + } + /** * Create a new update command for an existing entry instance. * @@ -203,6 +316,22 @@ public PathEdit(final DirCacheEntry ent) { path = ent.path; } + /** + * Configure if a file can replace a directory (or vice versa). + *

+ * Default is {@code true} as this is usually the desired behavior. + * + * @param ok + * if true a file can replace a directory, or a directory can + * replace a file. + * @return {@code this} + * @since 4.2 + */ + public PathEdit setReplace(boolean ok) { + replace = ok; + return this; + } + /** * Apply the update to a single cache entry matching the path. *

@@ -214,6 +343,12 @@ public PathEdit(final DirCacheEntry ent) { * the path is a new path in the index. */ public abstract void apply(DirCacheEntry ent); + + @Override + public String toString() { + String p = DirCacheEntry.toString(path); + return getClass().getSimpleName() + '[' + p + ']'; + } } /** @@ -281,6 +416,21 @@ public DeleteTree(String entryPath) { : entryPath + '/'); } + DeleteTree(byte[] path) { + super(appendSlash(path)); + } + + private static byte[] appendSlash(byte[] path) { + int n = path.length; + if (n > 0 && path[n - 1] != '/') { + byte[] r = new byte[n + 1]; + System.arraycopy(path, 0, r, 0, n); + r[n] = '/'; + return r; + } + return path; + } + public void apply(final DirCacheEntry ent) { throw new UnsupportedOperationException(JGitText.get().noApplyInDelete); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEntry.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEntry.java index c8bc0960f..5df9e0b4d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEntry.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheEntry.java @@ -745,7 +745,7 @@ private static void checkPath(byte[] path) { } } - private static String toString(final byte[] path) { + static String toString(final byte[] path) { return Constants.CHARSET.decode(ByteBuffer.wrap(path)).toString(); } From 683c41af92d248c818dcddb4d86c1640eba79374 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 18 Dec 2015 14:31:20 -0800 Subject: [PATCH 3/3] DirCache: Do not create duplicate tree entries If a file (e.g. "A") and a subtree file (e.g. "A/foo.c") both appear in the DirCache this cache should not be written out as a tree object. The "A" file and "A" subtree conflict with each other in the same tree and will fail fsck. Detect this condition during DirCacheBuilder and DirCacheEditor finish() so the application can be halted early before it updates a DirCache that might later write an invalid tree structure. Change-Id: I95660787e88df336297949b383f4c5fda52e75f5 --- .../jgit/dircache/DirCachePathEditTest.java | 46 +++++++++++ .../jgit/dircache/BaseDirCacheEditor.java | 74 +++++++++++++++++ .../errors/DirCacheNameConflictException.java | 80 +++++++++++++++++++ 3 files changed, 200 insertions(+) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/errors/DirCacheNameConflictException.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java index 3988f6a4c..39a0cdac5 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/dircache/DirCachePathEditTest.java @@ -43,11 +43,13 @@ package org.eclipse.jgit.dircache; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import java.util.ArrayList; import java.util.List; import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit; +import org.eclipse.jgit.errors.DirCacheNameConflictException; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; @@ -212,6 +214,50 @@ public void testTreeReplacesFile() throws Exception { assertEquals("e", dc.getEntry(4).getPathString()); } + @Test + public void testFileOverlapsTree() throws Exception { + DirCache dc = DirCache.newInCore(); + DirCacheEditor editor = dc.editor(); + editor.add(new AddEdit("a")); + editor.add(new AddEdit("a/b").setReplace(false)); + try { + editor.finish(); + fail("Expected DirCacheNameConflictException to be thrown"); + } catch (DirCacheNameConflictException e) { + assertEquals("a a/b", e.getMessage()); + assertEquals("a", e.getPath1()); + assertEquals("a/b", e.getPath2()); + } + + editor = dc.editor(); + editor.add(new AddEdit("A.c")); + editor.add(new AddEdit("A/c").setReplace(false)); + editor.add(new AddEdit("A0c")); + editor.add(new AddEdit("A")); + try { + editor.finish(); + fail("Expected DirCacheNameConflictException to be thrown"); + } catch (DirCacheNameConflictException e) { + assertEquals("A A/c", e.getMessage()); + assertEquals("A", e.getPath1()); + assertEquals("A/c", e.getPath2()); + } + + editor = dc.editor(); + editor.add(new AddEdit("A.c")); + editor.add(new AddEdit("A/b/c/d").setReplace(false)); + editor.add(new AddEdit("A/b/c")); + editor.add(new AddEdit("A0c")); + try { + editor.finish(); + fail("Expected DirCacheNameConflictException to be thrown"); + } catch (DirCacheNameConflictException e) { + assertEquals("A/b/c A/b/c/d", e.getMessage()); + assertEquals("A/b/c", e.getPath1()); + assertEquals("A/b/c/d", e.getPath2()); + } + } + private static DirCacheEntry createEntry(String path, int stage) { DirCacheEntry entry = new DirCacheEntry(path, stage); entry.setFileMode(FileMode.REGULAR_FILE); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java index c5c1b0d77..fc789b3d1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/BaseDirCacheEditor.java @@ -45,9 +45,12 @@ package org.eclipse.jgit.dircache; import static org.eclipse.jgit.lib.FileMode.TREE; +import static org.eclipse.jgit.lib.FileMode.TYPE_TREE; import java.io.IOException; +import org.eclipse.jgit.errors.DirCacheNameConflictException; + /** * Generic update/editing support for {@link DirCache}. *

@@ -170,6 +173,7 @@ protected void fastKeep(final int pos, int cnt) { * {@link #finish()}, and only after {@link #entries} is sorted. */ protected void replace() { + checkNameConflicts(); if (entryCnt < entries.length / 2) { final DirCacheEntry[] n = new DirCacheEntry[entryCnt]; System.arraycopy(entries, 0, n, 0, entryCnt); @@ -178,6 +182,76 @@ protected void replace() { cache.replace(entries, entryCnt); } + private void checkNameConflicts() { + int end = entryCnt - 1; + for (int eIdx = 0; eIdx < end; eIdx++) { + DirCacheEntry e = entries[eIdx]; + if (e.getStage() != 0) { + continue; + } + + byte[] ePath = e.path; + int prefixLen = lastSlash(ePath) + 1; + + for (int nIdx = eIdx + 1; nIdx < entryCnt; nIdx++) { + DirCacheEntry n = entries[nIdx]; + if (n.getStage() != 0) { + continue; + } + + byte[] nPath = n.path; + if (!startsWith(ePath, nPath, prefixLen)) { + // Different prefix; this entry is in another directory. + break; + } + + int s = nextSlash(nPath, prefixLen); + int m = s < nPath.length ? TYPE_TREE : n.getRawMode(); + int cmp = pathCompare( + ePath, prefixLen, ePath.length, TYPE_TREE, + nPath, prefixLen, s, m); + if (cmp < 0) { + break; + } else if (cmp == 0) { + throw new DirCacheNameConflictException( + e.getPathString(), + n.getPathString()); + } + } + } + } + + private static int lastSlash(byte[] path) { + for (int i = path.length - 1; i >= 0; i--) { + if (path[i] == '/') { + return i; + } + } + return -1; + } + + private static int nextSlash(byte[] b, int p) { + final int n = b.length; + for (; p < n; p++) { + if (b[p] == '/') { + return p; + } + } + return n; + } + + private static boolean startsWith(byte[] a, byte[] b, int n) { + if (b.length < n) { + return false; + } + for (n--; n >= 0; n--) { + if (a[n] != b[n]) { + return false; + } + } + 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) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/DirCacheNameConflictException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/DirCacheNameConflictException.java new file mode 100644 index 000000000..5f67e3439 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/DirCacheNameConflictException.java @@ -0,0 +1,80 @@ +/* + * Copyright (C) 2015, Google Inc. + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.errors; + +/** + * Thrown by DirCache code when entries overlap in impossible way. + * + * @since 4.2 + */ +public class DirCacheNameConflictException extends IllegalStateException { + private static final long serialVersionUID = 1L; + + private final String path1; + private final String path2; + + /** + * Construct an exception for a specific path. + * + * @param path1 + * one path that conflicts. + * @param path2 + * another path that conflicts. + */ + public DirCacheNameConflictException(String path1, String path2) { + super(path1 + ' ' + path2); + this.path1 = path1; + this.path2 = path2; + } + + /** @return one of the paths that has a conflict. */ + public String getPath1() { + return path1; + } + + /** @return another path that has a conflict. */ + public String getPath2() { + return path2; + } +}