From 115a740e2fa65d3ab70f150691b045ecfdb146bb Mon Sep 17 00:00:00 2001 From: Terry Parker Date: Tue, 26 Jun 2018 16:44:01 -0700 Subject: [PATCH] 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 --- .../jgit/revwalk/RevWalkShallowTest.java | 16 ++++++++ .../org/eclipse/jgit/revwalk/RevCommit.java | 18 +++++---- .../src/org/eclipse/jgit/revwalk/RevWalk.java | 37 ++++++++++++++++--- 3 files changed, 58 insertions(+), 13 deletions(-) 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 6df36e78f..37243e1fa 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 b67f934f6..3de442afb 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 a986cfd8f..4d555d217 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, 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. + *

+ * 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. + *

+ * 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; + } + } } }