RevWalk: Traverse all parents of UNINTERESTING commits

When firstParent is set, RevWalk traverses only the first parent of a
commit, even though that commit is UNINTERESTING. Since we want the
maximal UNINTERESTING set, we shouldn't prune any parents here. This
issue is apparent only when some of the commits being traversed are
unparsed, since walker.carryFlagsImpl() propagates the UNINTERESTING
flag to all parsed ancestors, masking the issue.

Therefore teach RevWalk to traverse all parents when a commit is
UNINTERESTING and not only the first parent. Since this issue is
masked by commit parsing, also test situations when the commits
involved are unparsed.

Signed-off-by: Alex Spradlin <alexaspradlin@google.com>
Change-Id: I95e2ad9ae8f1f50fbecae674367ee7e0855519b1

diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java
index e89cf0fb8336bffa11900a0936c633b854ce58e9..362007a982cfd1b866c972d50bbc9263026f4fc5 100644 (file)
--- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java
+++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java
@@ -360,6 +360,21 @@ public class TestRepository<R extends Repository> implements AutoCloseable {
                return null; // never reached.
        }
 
+       /**
+        * Create a new, unparsed commit.
+        * <p>
+        * See {@link #unparsedCommit(int, RevTree, ObjectId...)}. The tree is the
+        * empty tree (no files or subdirectories).
+        *
+        * @param parents
+        *            zero or more IDs of the commit's parents.
+        * @return the ID of the new commit.
+        * @throws Exception
+        */
+       public ObjectId unparsedCommit(ObjectId... parents) throws Exception {
+               return unparsedCommit(1, tree(), parents);
+       }
+
        /**
         * Create a new commit.
         * <p>
@@ -428,6 +443,28 @@ public class TestRepository<R extends Repository> implements AutoCloseable {
         */
        public RevCommit commit(final int secDelta, final RevTree tree,
                        final RevCommit... parents) throws Exception {
+               ObjectId id = unparsedCommit(secDelta, tree, parents);
+               return pool.parseCommit(id);
+       }
+
+       /**
+        * Create a new, unparsed commit.
+        * <p>
+        * The author and committer identities are stored using the current
+        * timestamp, after being incremented by {@code secDelta}. The message body
+        * is empty.
+        *
+        * @param secDelta
+        *            number of seconds to advance {@link #tick(int)} by.
+        * @param tree
+        *            the root tree for the commit.
+        * @param parents
+        *            zero or more IDs of the commit's parents.
+        * @return the ID of the new commit.
+        * @throws Exception
+        */
+       public ObjectId unparsedCommit(final int secDelta, final RevTree tree,
+                       final ObjectId... parents) throws Exception {
                tick(secDelta);
 
                final org.eclipse.jgit.lib.CommitBuilder c;
@@ -443,7 +480,7 @@ public class TestRepository<R extends Repository> implements AutoCloseable {
                        id = ins.insert(c);
                        ins.flush();
                }
-               return pool.parseCommit(id);
+               return id;
        }
 
        /**

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FirstParentRevWalkTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FirstParentRevWalkTest.java
index 474ff7a7c22d9e7ebe7435494066a07692c27cab..1fc7a55457e39b75088199d992e5a47323d6bd9b 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FirstParentRevWalkTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FirstParentRevWalkTest.java
@@ -46,6 +46,7 @@ package org.eclipse.jgit.revwalk;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.filter.MessageRevFilter;
 import org.eclipse.jgit.revwalk.filter.RevFilter;
 import org.junit.Test;
@@ -267,6 +268,26 @@ public class FirstParentRevWalkTest extends RevWalkTestCase {
                assertNull(rw.next());
        }
 
+       @Test
+       public void testUnparsedFirstParentOfFirstParentMarkedUninteresting()
+                       throws Exception {
+               ObjectId a = unparsedCommit();
+               ObjectId b1 = unparsedCommit(a);
+               ObjectId b2 = unparsedCommit(a);
+               ObjectId c1 = unparsedCommit(b1);
+               ObjectId c2 = unparsedCommit(b2);
+               ObjectId d = unparsedCommit(c1, c2);
+
+               rw.reset();
+               rw.setFirstParent(true);
+               RevCommit parsedD = rw.parseCommit(d);
+               markStart(parsedD);
+               markUninteresting(rw.parseCommit(b1));
+               assertCommit(parsedD, rw.next());
+               assertCommit(rw.parseCommit(c1), rw.next());
+               assertNull(rw.next());
+       }
+
        @Test
        public void testFirstParentMarkedUninteresting() throws Exception {
                RevCommit a = commit();
@@ -282,6 +303,75 @@ public class FirstParentRevWalkTest extends RevWalkTestCase {
                assertNull(rw.next());
        }
 
+       @Test
+       public void testUnparsedFirstParentMarkedUninteresting() throws Exception {
+               ObjectId a = unparsedCommit();
+               ObjectId b1 = unparsedCommit(a);
+               ObjectId b2 = unparsedCommit(a);
+               ObjectId c = unparsedCommit(b1, b2);
+
+               rw.reset();
+               rw.setFirstParent(true);
+               RevCommit parsedC = rw.parseCommit(c);
+               markStart(parsedC);
+               markUninteresting(rw.parseCommit(b1));
+               assertCommit(parsedC, rw.next());
+               assertNull(rw.next());
+       }
+
+       @Test
+       public void testUninterestingCommitWithTwoParents() throws Exception {
+               RevCommit a = commit();
+               RevCommit b = commit(a);
+               RevCommit c1 = commit(b);
+               RevCommit c2 = commit(b);
+               RevCommit d = commit(c1);
+               RevCommit e = commit(c1, c2);
+
+               RevCommit uA = commit(a, b);
+               RevCommit uB1 = commit(uA, c2);
+               RevCommit uB2 = commit(uA, d);
+               RevCommit uninteresting = commit(uB1, uB2);
+
+               rw.reset();
+               rw.setFirstParent(true);
+               markStart(e);
+               markUninteresting(uninteresting);
+
+               assertCommit(e, rw.next());
+               assertNull(rw.next());
+       }
+
+       /**
+        * This fails if we try to propagate flags before parsing commits.
+        *
+        * @throws Exception
+        */
+       @Test
+       public void testUnparsedUninterestingCommitWithTwoParents()
+                       throws Exception {
+               ObjectId a = unparsedCommit();
+               ObjectId b = unparsedCommit(a);
+               ObjectId c1 = unparsedCommit(b);
+               ObjectId c2 = unparsedCommit(b);
+               ObjectId d = unparsedCommit(c1);
+               ObjectId e = unparsedCommit(c1, c2);
+
+               ObjectId uA = unparsedCommit(a, b);
+               ObjectId uB1 = unparsedCommit(uA, c2);
+               ObjectId uB2 = unparsedCommit(uA, d);
+               ObjectId uninteresting = unparsedCommit(uB1, uB2);
+
+               rw.reset();
+               rw.setFirstParent(true);
+               RevCommit parsedE = rw.parseCommit(e);
+               markStart(parsedE);
+               markUninteresting(rw.parseCommit(uninteresting));
+
+               assertCommit(parsedE, rw.next());
+               assertNull(rw.next());
+       }
+
        @Test
        public void testDepthWalk() throws Exception {
                RevCommit a = commit();

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkTestCase.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkTestCase.java
index 544398219ff08188056b06a5a4e50f3612f3b012..a0056ae217974b2facbaa7afd0e9959e6da6dcdc 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkTestCase.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkTestCase.java
@@ -51,6 +51,7 @@ import org.eclipse.jgit.dircache.DirCacheEntry;
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.junit.TestRepository.CommitBuilder;
+import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
 
 /** Support for tests of the {@link RevWalk} class. */
@@ -96,6 +97,10 @@ public abstract class RevWalkTestCase extends RepositoryTestCase {
                return util.get(tree, path);
        }
 
+       protected ObjectId unparsedCommit(ObjectId... parents) throws Exception {
+               return util.unparsedCommit(parents);
+       }
+
        protected RevCommit commit(RevCommit... parents) throws Exception {
                return util.commit(parents);
        }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PendingGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PendingGenerator.java
index 52dd56d129731c5079c9ca74535e3f2b4c32070c..3990dd6d0e93e4ff3909f4fc663a0ae81af4f6ef 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PendingGenerator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/PendingGenerator.java
@@ -143,7 +143,9 @@ class PendingGenerator extends Generator {
 
                                for (int i = 0; i < c.parents.length; i++) {
                                        RevCommit p = c.parents[i];
-                                       if (firstParent && i > 0) {
+                                       // If the commit is uninteresting, don't try to prune
+                                       // parents because we want the maximal uninteresting set.
+                                       if (firstParent && i > 0 && (c.flags & UNINTERESTING) == 0) {
                                                continue;
                                        }
                                        if ((p.flags & SEEN) != 0)