Correctly handle initialization of shallow commits

In a new RevWalk, if the first object parsed is one of the
shallow commits, the following happens:
1) RevCommit.parseCanonical() is called on a new "r1" RevCommit.
2) RevCommit.parseCanonical() immediately calls
   RevWalk.initializeShallowCommits().
3) RevWalk.initializeShallowCommits() calls lookupCommit(id),
   creating and adding a new "r2" version of this same object and
   marking its parents empty.
4) RevCommit.parseCanonical() initializes the "r1" RevCommit's
   fields, including the parents.
5) RevCommit.parseCanonical()'s caller uses the "r1" commit that
   has parents, losing the fact that it is a shallow commit.

This change passes the current RevCommit as an argument to
RevWalk.initializeShallowCommits() so that method can set its
parents empty rather than creating the duplicate "r2" commit.

Change-Id: I67b79aa2927dd71ac7b0d8f8917f423dcaf08c8a
Signed-off-by: Terry Parker <tparker@google.com>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkShallowTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkShallowTest.java
index 6df36e78f13b2f3364f4ee0ce364b343ce777fe0..37243e1fa7b28f0b7d4811d9eda9b84d6fa27150 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkShallowTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkShallowTest.java
@@ -184,6 +184,22 @@ public class RevWalkShallowTest extends RevWalkTestCase {
                assertNull(rw.next());
        }
 
+       @Test
+       public void testShallowCommitParse() throws Exception {
+               RevCommit a = commit();
+               RevCommit b = commit(a);
+
+               createShallowFile(b);
+
+               rw.close();
+               rw = createRevWalk();
+               b = rw.parseCommit(b);
+
+               markStart(b);
+               assertCommit(b, rw.next());
+               assertNull(rw.next());
+       }
+
        private void createShallowFile(ObjectId... shallowCommits)
                        throws IOException {
                final StringBuilder builder = new StringBuilder();

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
index b67f934f6ede7719a17c030e598bade8537f7c83..3de442afb62f446cd7c0204a36ab665d885d69d7 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java
@@ -168,10 +168,10 @@ public class RevCommit extends RevObject {
                }
        }
 
-       void parseCanonical(RevWalk walk, byte[] raw)
-                       throws IOException {
-               if (!walk.shallowCommitsInitialized)
-                       walk.initializeShallowCommits();
+       void parseCanonical(RevWalk walk, byte[] raw) throws IOException {
+               if (!walk.shallowCommitsInitialized) {
+                       walk.initializeShallowCommits(this);
+               }
 
                final MutableObjectId idBuffer = walk.idBuffer;
                idBuffer.fromString(raw, 5);
@@ -182,13 +182,14 @@ public class RevCommit extends RevObject {
                        RevCommit[] pList = new RevCommit[1];
                        int nParents = 0;
                        for (;;) {
-                               if (raw[ptr] != 'p')
+                               if (raw[ptr] != 'p') {
                                        break;
+                               }
                                idBuffer.fromString(raw, ptr + 7);
                                final RevCommit p = walk.lookupCommit(idBuffer);
-                               if (nParents == 0)
+                               if (nParents == 0) {
                                        pList[nParents++] = p;
-                               else if (nParents == 1) {
+                               } else if (nParents == 1) {
                                        pList = new RevCommit[] { pList[0], p };
                                        nParents = 2;
                                } else {
@@ -218,8 +219,9 @@ public class RevCommit extends RevObject {
                        commitTime = RawParseUtils.parseBase10(raw, ptr, null);
                }
 
-               if (walk.isRetainBody())
+               if (walk.isRetainBody()) {
                        buffer = raw;
+               }
                flags |= PARSED;
        }
 

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
index a986cfd8ff8a2878f84b71e1d20aaf2bf754090f..4d555d21789013e3cecce9119f360b7806ca2ffa 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
@@ -1460,17 +1460,44 @@ public class RevWalk implements Iterable<RevCommit>, AutoCloseable {
                        lookupCommit(id).parents = RevCommit.NO_PARENTS;
        }
 
-       void initializeShallowCommits() throws IOException {
-               if (shallowCommitsInitialized)
+       /**
+        * Reads the "shallow" file and applies it by setting the parents of shallow
+        * commits to an empty array.
+        * <p>
+        * There is a sequencing problem if the first commit being parsed is a
+        * shallow commit, since {@link RevCommit#parseCanonical(RevWalk, byte[])}
+        * calls this method before its callers add the new commit to the
+        * {@link RevWalk#objects} map. That means a call from this method to
+        * {@link #lookupCommit(AnyObjectId)} fails to find that commit and creates
+        * a new one, which is promptly discarded.
+        * <p>
+        * To avoid that, {@link RevCommit#parseCanonical(RevWalk, byte[])} passes
+        * its commit to this method, so that this method can apply the shallow
+        * state to it directly and avoid creating the duplicate commit object.
+        *
+        * @param rc
+        *            the initial commit being parsed
+        * @throws IOException
+        *             if the shallow commits file can't be read
+        */
+       void initializeShallowCommits(RevCommit rc) throws IOException {
+               if (shallowCommitsInitialized) {
                        throw new IllegalStateException(
                                        JGitText.get().shallowCommitsAlreadyInitialized);
+               }
 
                shallowCommitsInitialized = true;
 
-               if (reader == null)
+               if (reader == null) {
                        return;
+               }
 
-               for (ObjectId id : reader.getShallowCommits())
-                       lookupCommit(id).parents = RevCommit.NO_PARENTS;
+               for (ObjectId id : reader.getShallowCommits()) {
+                       if (id.equals(rc.getId())) {
+                               rc.parents = RevCommit.NO_PARENTS;
+                       } else {
+                               lookupCommit(id).parents = RevCommit.NO_PARENTS;
+                       }
+               }
        }
 }