From: Terry Parker <tparker@google.com>
Date: Tue, 26 Jun 2018 23:44:01 +0000 (-0700)
Subject: Correctly handle initialization of shallow commits
X-Git-Tag: v5.1.0.201808281540-m3~68
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=115a740e2fa65d3ab70f150691b045ecfdb146bb;p=jgit.git

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 6df36e78f1..37243e1fa7 100644
--- 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 b67f934f6e..3de442afb6 100644
--- 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 a986cfd8ff..4d555d2178 100644
--- 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;
+			}
+		}
 	}
 }