From 6f00a3e651e91592e2eb4f239b243a0aa2fe4610 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 1 Sep 2010 12:31:37 -0700 Subject: [PATCH] Fix TreeWalk bug comparing DirCache and WorkingTree with ANY_DIFF When comparing a DirCache and a WorkingTree using ANY_DIFF we sometimes didn't recursive into a subtree of both sides gave us zeroId() back for the identity of a subtree. This happens when the DirCache doesn't have a valid cache tree for the subtree, as then it uses zeroId() for the ObjectId of the subtree, which then appears to be equal to the zeroId() of the WorkingTreeIterator's subtree. We work around this by adding a hasId() method that returns true only if this iterator has a valid ObjectId. The idEquals method on TreeWalk than only performs a compare between two iterators if both iterators have a valid id. Change-Id: I695f7fafbeb452e8c0703a05c02921fae0822d3f Signed-off-by: Shawn O. Pearce --- .../org/eclipse/jgit/dircache/DirCacheIterator.java | 12 ++++++++---- .../eclipse/jgit/treewalk/AbstractTreeIterator.java | 3 +++ .../eclipse/jgit/treewalk/CanonicalTreeParser.java | 5 +++++ .../org/eclipse/jgit/treewalk/EmptyTreeIterator.java | 5 +++++ .../src/org/eclipse/jgit/treewalk/TreeWalk.java | 6 ++++-- .../eclipse/jgit/treewalk/WorkingTreeIterator.java | 7 +++++++ 6 files changed, 32 insertions(+), 6 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheIterator.java index aa8d9fb85..e685e0cad 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCacheIterator.java @@ -45,7 +45,6 @@ package org.eclipse.jgit.dircache; import java.io.IOException; -import java.util.Arrays; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.lib.Constants; @@ -141,10 +140,17 @@ public EmptyTreeIterator createEmptyTreeIterator() { return new EmptyTreeIterator(this, n, pathLen + 1); } + @Override + public boolean hasId() { + if (currentSubtree != null) + return currentSubtree.isValid(); + return currentEntry != null; + } + @Override public byte[] idBuffer() { if (currentSubtree != null) - return subtreeId; + return currentSubtree.isValid() ? subtreeId : zeroid; if (currentEntry != null) return currentEntry.idBuffer(); return zeroid; @@ -218,8 +224,6 @@ private void parseEntry() { if (s.isValid()) s.getObjectId().copyRawTo(subtreeId, 0); - else - Arrays.fill(subtreeId, (byte) 0); mode = FileMode.TREE.getBits(); path = cep; pathLen = pathOffset + s.nameLength(); 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 eee62c63a..79b57d1eb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java @@ -369,6 +369,9 @@ public boolean idEqual(final AbstractTreeIterator otherIterator) { otherIterator.idBuffer(), otherIterator.idOffset()); } + /** @return true if the entry has a valid ObjectId. */ + public abstract boolean hasId(); + /** * Get the object id of the current entry. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java index 01b827425..1e49d380a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java @@ -233,6 +233,11 @@ public CanonicalTreeParser createSubtreeIterator(final ObjectReader reader) return createSubtreeIterator(reader, new MutableObjectId()); } + @Override + public boolean hasId() { + return true; + } + @Override public byte[] idBuffer() { return raw; 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 49d75871e..8dbf80e6a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/EmptyTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/EmptyTreeIterator.java @@ -92,6 +92,11 @@ public AbstractTreeIterator createSubtreeIterator(final ObjectReader reader) return new EmptyTreeIterator(this); } + @Override + public boolean hasId() { + return false; + } + @Override public ObjectId getEntryObjectId() { return ObjectId.zeroId(); 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 16859646b..992928bc4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java @@ -683,8 +683,6 @@ public boolean idEqual(final int nthA, final int nthB) { final AbstractTreeIterator ch = currentHead; final AbstractTreeIterator a = trees[nthA]; final AbstractTreeIterator b = trees[nthB]; - if (a.matches == ch && b.matches == ch) - return a.idEqual(b); if (a.matches != ch && b.matches != ch) { // If neither tree matches the current path node then neither // tree has this entry. In such case the ObjectId is zero(), @@ -692,6 +690,10 @@ public boolean idEqual(final int nthA, final int nthB) { // return true; } + if (!a.hasId() || !b.hasId()) + return false; + if (a.matches == ch && b.matches == ch) + return a.idEqual(b); return false; } 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 51c348369..5cc061bbb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -194,6 +194,13 @@ protected void initRootIterator(Repository repo) { ignoreNode = new RootIgnoreNode(entry, repo); } + @Override + public boolean hasId() { + if (contentIdFromPtr == ptr) + return true; + return (mode & FileMode.TYPE_MASK) == FileMode.TYPE_FILE; + } + @Override public byte[] idBuffer() { if (contentIdFromPtr == ptr)