Fix MissingObjectException in from RevisionParser.parse Revision.peel was failing to distinguish between MissingObjectExceptions due to the root object missing and those due to an object pointed to by a tag missing. In RevisionParser, we were sometimes passing nonexistent SHAs that were nonetheless valid according to repo.resolve() to this method. In that case, the root object being missing is ok (it should parse as null), but a tagged object being missing is more serious and should throw. Separate these cases out by requiring callers to validate the SHA before passing to Revision.peel. Change-Id: I1d488a087bd3284f990dcb1839bdebec378a8d18
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java b/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java index 4915ba2..d64a19f 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/LogServlet.java
@@ -149,7 +149,7 @@ try { return GitilesView.log() .copyFrom(view) - .setRevision(Revision.peel(Constants.HEAD, headRef.getObjectId(), walk)) + .setRevision(Revision.peel(Constants.HEAD, walk.parseAny(headRef.getObjectId()), walk)) .build(); } finally { walk.release();
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java b/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java index 05778e0..4b362fa 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/Revision.java
@@ -60,9 +60,8 @@ return peeled(name, null, OBJ_BAD); } - public static Revision peel(String name, ObjectId id, RevWalk walk) + public static Revision peel(String name, RevObject obj, RevWalk walk) throws MissingObjectException, IOException { - RevObject obj = walk.parseAny(id); RevObject peeled = walk.peel(obj); return new Revision(name, obj, obj.getType(), peeled, peeled.getType()); }
diff --git a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java index ffb59ab..2d27e2c 100644 --- a/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java +++ b/gitiles-servlet/src/main/java/com/google/gitiles/RevisionParser.java
@@ -16,19 +16,21 @@ import static com.google.common.base.Preconditions.checkNotNull; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.CharMatcher; -import com.google.common.base.Objects; -import com.google.common.base.Splitter; +import java.io.IOException; -import org.eclipse.jgit.errors.IncorrectObjectTypeException; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.errors.RevisionSyntaxException; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevObject; +import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; -import java.io.IOException; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.CharMatcher; +import com.google.common.base.Objects; +import com.google.common.base.Splitter; /** Object to parse revisions out of Gitiles paths. */ class RevisionParser { @@ -126,7 +128,7 @@ if (!isValidRevision(oldName)) { return null; } else { - ObjectId old = resolve(oldName); + RevObject old = resolve(oldName, walk); if (old == null) { return null; } @@ -143,16 +145,18 @@ if (!isValidRevision(name)) { return null; } - ObjectId id = resolve(name); - if (id == null) { + RevObject obj = resolve(name, walk); + if (obj == null) { return null; } - RevCommit c; - try { - c = walk.parseCommit(id); - } catch (IncorrectObjectTypeException e) { + while (obj instanceof RevTag) { + obj = ((RevTag) obj).getObject(); + walk.parseHeaders(obj); + } + if (!(obj instanceof RevCommit)) { return null; // Not a commit, ^! is invalid. } + RevCommit c = (RevCommit) obj; if (c.getParentCount() > 0) { oldRevision = Revision.peeled(name + "^", c.getParent(0)); } else { @@ -168,8 +172,8 @@ if (!isValidRevision(name)) { return null; } - ObjectId id = resolve(name); - if (id != null) { + RevObject obj = resolve(name, walk); + if (obj != null) { int pathStart; if (oldRevision == null) { pathStart = name.length(); // foo @@ -177,7 +181,7 @@ // foo..bar (foo may be empty) pathStart = oldRevision.getName().length() + 2 + name.length(); } - Result result = new Result(Revision.peel(name, id, walk), oldRevision, pathStart); + Result result = new Result(Revision.peel(name, obj, walk), oldRevision, pathStart); return isVisible(walk, result) ? result : null; } first = false; @@ -188,11 +192,14 @@ } } - private ObjectId resolve(String name) throws IOException { + private RevObject resolve(String name, RevWalk walk) throws IOException { try { - return repo.resolve(name); + ObjectId id = repo.resolve(name); + return id != null ? walk.parseAny(id) : null; } catch (RevisionSyntaxException e) { return null; + } catch (MissingObjectException e) { + return null; } }
diff --git a/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java b/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java index a636a17..9d26504 100644 --- a/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java +++ b/gitiles-servlet/src/test/java/com/google/gitiles/RevisionParserTest.java
@@ -17,6 +17,7 @@ import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.eclipse.jgit.lib.Constants.OBJ_COMMIT; import static org.eclipse.jgit.lib.Constants.OBJ_TAG; + import junit.framework.TestCase; import org.eclipse.jgit.internal.storage.dfs.DfsRepository; @@ -271,4 +272,9 @@ //assertEquals(null, repo.getRepository().resolve("master@{0}")); assertEquals(null, parser.parse("master@{0}")); } + + public void testParseMissingSha() throws Exception { + assertNull(parser.parse("deadbeef")); + assertNull(parser.parse("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + } }