From 2852b6a07d78086e155c2f658ff9b5b235d1fb4e Mon Sep 17 00:00:00 2001 From: Robin Stocker Date: Tue, 14 Jan 2014 16:56:01 +0100 Subject: [PATCH] Fix RevWalkUtils.findBranchesReachableFrom not finding some branches The "cut off" optimization causes it to not include branches that contain the specified commit but happen to share commits with a branch that does not contain the commit. An example: -B foo \ -A---C master findBranchesReachableFrom for commit A with both branches as input may not return master (depending on the order of the input). The reason is that A is not contained in foo, and therefore the old code would put B in the cutOff set. When then walking the master commits and B is checked, it is found in the cutOff set and the walk is aborted, causing master not to be returned even though it should. Bug: 425674 Change-Id: I2c0c406ce5fcc9a03538b483473af930d4895d30 Signed-off-by: Robin Stocker Signed-off-by: Matthias Sohn --- .../revwalk/RevWalkUtilsReachableTest.java | 128 ++++++++++++++++++ .../eclipse/jgit/revwalk/RevWalkUtils.java | 31 +---- 2 files changed, 132 insertions(+), 27 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java new file mode 100644 index 000000000..855bb8056 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkUtilsReachableTest.java @@ -0,0 +1,128 @@ +/* + * Copyright (C) 2014, Robin Stocker + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.revwalk; + +import static java.util.Arrays.asList; +import static org.junit.Assert.assertEquals; + +import java.util.Collection; +import java.util.List; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefComparator; +import org.junit.Test; + +public class RevWalkUtilsReachableTest extends RevWalkTestCase { + + @Test + public void oneCommit() throws Exception { + RevCommit a = commit(); + Ref branchA = branch("a", a); + + assertContains(a, asList(branchA)); + } + + @Test + public void twoCommits() throws Exception { + RevCommit a = commit(); + RevCommit b = commit(a); + branch("a", a); + Ref branchB = branch("b", b); + + assertContains(b, asList(branchB)); + } + + @Test + public void multipleBranches() throws Exception { + RevCommit a = commit(); + RevCommit b = commit(a); + branch("a", a); + Ref branchB = branch("b", b); + Ref branchB2 = branch("b2", b); + + assertContains(b, asList(branchB, branchB2)); + } + + @Test + public void withMerge() throws Exception { + RevCommit a = commit(); + RevCommit b = commit(); + RevCommit c = commit(a, b); + Ref branchA = branch("a", a); + Ref branchB = branch("b", b); + Ref branchC = branch("c", c); + + assertContains(a, asList(branchA, branchC)); + assertContains(b, asList(branchB, branchC)); + } + + @Test + public void withCommitLoadedByDifferentRevWalk() throws Exception { + RevCommit a = commit(); + Ref branchA = branch("a", a); + + RevWalk walk = new RevWalk(db); + try { + RevCommit parsedCommit = walk.parseCommit(a.getId()); + assertContains(parsedCommit, asList(branchA)); + } finally { + walk.release(); + } + } + + private Ref branch(final String name, final RevCommit dst) throws Exception { + return Git.wrap(db).branchCreate().setName(name) + .setStartPoint(dst.name()).call(); + } + + private void assertContains(RevCommit commit, Collection refsThatShouldContainCommit) throws Exception { + Collection allRefs = db.getAllRefs().values(); + Collection sortedRefs = RefComparator.sort(allRefs); + List actual = RevWalkUtils.findBranchesReachableFrom(commit, + rw, sortedRefs); + assertEquals(refsThatShouldContainCommit, actual); + } + +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalkUtils.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalkUtils.java index f223dea44..11fec31e9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalkUtils.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalkUtils.java @@ -45,15 +45,11 @@ package org.eclipse.jgit.revwalk; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.List; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; -import org.eclipse.jgit.lib.AnyObjectId; -import org.eclipse.jgit.lib.ObjectId; -import org.eclipse.jgit.lib.ObjectIdSubclassMap; import org.eclipse.jgit.lib.Ref; /** @@ -156,11 +152,10 @@ public final class RevWalkUtils { throws MissingObjectException, IncorrectObjectTypeException, IOException { + // Make sure commit is from the same RevWalk + commit = revWalk.parseCommit(commit.getId()); + revWalk.reset(); List result = new ArrayList(); - // searches from branches can be cut off early if any parent of the - // search-for commit is found. This is quite likely, so optimize for this. - revWalk.markStart(Arrays.asList(commit.getParents())); - ObjectIdSubclassMap cutOff = new ObjectIdSubclassMap(); final int SKEW = 24*3600; // one day clock skew @@ -176,26 +171,8 @@ public final class RevWalkUtils { if (headCommit.getCommitTime() + SKEW < commit.getCommitTime()) continue; - List maybeCutOff = new ArrayList(cutOff.size()); // guess rough size - revWalk.resetRetain(); - revWalk.markStart(headCommit); - RevCommit current; - Ref found = null; - while ((current = revWalk.next()) != null) { - if (AnyObjectId.equals(current, commit)) { - found = ref; - break; - } - if (cutOff.contains(current)) - break; - maybeCutOff.add(current.toObjectId()); - } - if (found != null) + if (revWalk.isMergedInto(commit, headCommit)) result.add(ref); - else - for (ObjectId id : maybeCutOff) - cutOff.addIfAbsent(id); - } return result; }