Skip to content

Commit

Permalink
Fix push and fetch missing data caused by LogOp bug.
Browse files Browse the repository at this point in the history 
The topological log iterator was stopping as soon as it reached the
"have" commit, causing other branching paths to not be included in
the transfer.

Co-authored-by: dblasby <[email protected]>
Signed-off-by: Johnathan Garrett <[email protected]>
  • Loading branch information
@jdgarrett @DBlasby @groldan
2 people authored and groldan committed Oct 18, 2017
1 parent 33c5911 commit 33941d1
Show file tree
Hide file tree
Showing 6 changed files with 438 additions and 31 deletions.
33 changes: 27 additions & 6 deletions src/core/src/main/java/org/locationtech/geogig/porcelain/LogOp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.locationtech.geogig.model.Ref;
import org.locationtech.geogig.model.RevCommit;
import org.locationtech.geogig.model.RevTree;
import org.locationtech.geogig.plumbing.FindCommonAncestor;
import org.locationtech.geogig.plumbing.FindTreeChild;
import org.locationtech.geogig.plumbing.RevParse;
import org.locationtech.geogig.repository.AbstractGeoGigOp;
Expand Down Expand Up @@ -262,7 +263,8 @@ protected Iterator<RevCommit> _call() {
commits.add(newestCommitId);
}
if (topo) {
history = new TopologicalHistoryIterator(commits, repository(), graphDatabase());
history = new TopologicalHistoryIterator(commits, repository(), graphDatabase(),
oldestCommitId);
} else {
history = new ChronologicalHistoryIterator(commits, repository());
}
Expand Down Expand Up @@ -381,6 +383,8 @@ private static class TopologicalHistoryIterator extends AbstractIterator<RevComm

private GraphDatabase graphDb;

private ObjectId oldestCommitId;

/**
* Constructs a new {@code LinearHistoryIterator} with the given parameters.
*
Expand All @@ -389,10 +393,12 @@ private static class TopologicalHistoryIterator extends AbstractIterator<RevComm
* @param graphDb
*/
public TopologicalHistoryIterator(final List<ObjectId> tipsList, final Repository repo,
GraphDatabase graphDb) {
GraphDatabase graphDb, ObjectId oldestCommitId) {
this.graphDb = graphDb;
tips = new Stack<RevCommit>();
stopPoints = Lists.newArrayList();
stopPoints.add(oldestCommitId);
this.oldestCommitId = oldestCommitId;
for (ObjectId tip : tipsList) {
if (!tip.isNull()) {
final RevCommit commit = repo.getCommit(tip);
Expand All @@ -417,6 +423,10 @@ protected RevCommit computeNext() {
Optional<ObjectId> parent = Optional.absent();
int index = 0;
for (ObjectId parentId : lastCommit.getParentIds()) {
if (stopPoints.contains(parentId)){
index++;
continue;
}
if (repo.commitExists(parentId)) {
parent = Optional.of(parentId);
break;
Expand All @@ -429,6 +439,15 @@ protected RevCommit computeNext() {
return endOfData();
} else {
lastCommit = tips.pop();
if (!ObjectId.NULL.equals(oldestCommitId)) {
// Add the common ancestor of oldestCommitId and the new tip to the
// stopPoints to make sure we only hit relevant commits
Optional<ObjectId> ancestor = repo.command(FindCommonAncestor.class)
.setLeftId(lastCommit.getId()).setRightId(oldestCommitId).call();
if (ancestor.isPresent()) {
stopPoints.add(ancestor.get());
}
}
}
} else {
List<ObjectId> parents = lastCommit.getParentIds();
Expand All @@ -439,10 +458,12 @@ protected RevCommit computeNext() {
}
}
lastCommit = repo.getCommit(parent.get());
ImmutableList<ObjectId> children = this.graphDb.getChildren(parent.get());
if (children.size() > 1) {
stopPoints.add(parent.get());
}
}


ImmutableList<ObjectId> children = this.graphDb.getChildren(lastCommit.getId());
if (children.size() > 1) {
stopPoints.add(lastCommit.getId());
}

return lastCommit;
Expand Down
173 changes: 173 additions & 0 deletions src/core/src/test/java/org/locationtech/geogig/test/integration/LogOpTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@
*/
package org.locationtech.geogig.test.integration;

import static com.google.common.collect.Lists.newArrayList;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.stream.Collectors;

import org.geotools.util.Range;
import org.junit.Rule;
Expand All @@ -32,8 +35,12 @@
import org.locationtech.geogig.porcelain.LogOp;
import org.locationtech.geogig.porcelain.MergeOp;
import org.locationtech.geogig.porcelain.MergeOp.MergeReport;
import org.locationtech.geogig.porcelain.ResetOp;
import org.locationtech.geogig.repository.DefaultProgressListener;
import org.locationtech.geogig.repository.ProgressListener;
import org.opengis.feature.Feature;

import com.google.common.base.Suppliers;
import com.google.common.collect.Iterators;
import com.google.common.collect.Lists;

Expand All @@ -49,6 +56,109 @@ protected void setUpInternal() throws Exception {
logOp = geogig.command(LogOp.class);
}

@Test
public void testComplex() throws Exception {
// Commit several features to the remote
List<Feature> features = Arrays.asList(points1, lines1, points2, lines2, points3, lines3);

for (Feature f : features) {
insertAndAdd(f);
}

geogig.command(CommitOp.class).setMessage("initial commit").call();

createBranch("branch1");
checkout("master");

insertAndAdd(points1_modified);
commit("left modify 1");

createBranch("intermediate_left");
checkout("branch1");

insertAndAdd(points2_modified);
commit("right modify 1");

checkout("intermediate_left");

mergeNoFF("branch1", "merge 1", true);

createBranch("intermediate_right");
checkout("master");

insertAndAdd(points3_modified);
commit("left modify 2");

checkout("intermediate_left");

MergeReport merge2_left = mergeNoFF("master", "merge 2 left", true);

checkout("master");
geogig.command(ResetOp.class).setMode(ResetOp.ResetMode.HARD)
.setCommit(Suppliers.ofInstance(merge2_left.getMergeCommit().getId())).call();

checkout("branch1");

insertAndAdd(lines1_modified);
commit("right modify 2");

checkout("intermediate_right");

MergeReport merge2_right = mergeNoFF("branch1", "merge 2 right", true);

checkout("branch1");
geogig.command(ResetOp.class).setMode(ResetOp.ResetMode.HARD)
.setCommit(Suppliers.ofInstance(merge2_right.getMergeCommit().getId())).call();

checkout("master");

mergeNoFF("branch1", "final merge", true);

// both arrays should have 9 elements and contain the same commits (in different order)
List<RevCommit> log_topo = newArrayList(
geogig.command(LogOp.class).setTopoOrder(true).call());

assertEquals(log_topo.size(), 9);

List<RevCommit> log_chrono = newArrayList(
geogig.command(LogOp.class).setTopoOrder(false).call());

assertEquals(log_chrono.size(), 9);

List<ObjectId> log_topo_ids = log_topo.stream().map(c -> c.getId()).sorted().collect(Collectors.toList());
List<ObjectId> log_chrono_ids = log_chrono.stream().map(c -> c.getId()).sorted().collect(Collectors.toList());

assertTrue(log_topo_ids.equals(log_chrono_ids));
}

protected static final ProgressListener SIMPLE_PROGRESS = new DefaultProgressListener() {
public @Override
void setDescription(String msg) {
System.err.println(msg);
}
};

protected void createBranch(String branch) {
geogig.command(BranchCreateOp.class).setAutoCheckout(true).setName(branch)
.setProgressListener(SIMPLE_PROGRESS).call();
}

protected MergeReport mergeNoFF(String branch, String mergeMessage,
boolean mergeOurs) {
Ref branchRef = geogig.command(RefParse.class).setName(branch).call().get();
ObjectId updatesBranchTip = branchRef.getObjectId();
MergeReport mergeReport = geogig.command(MergeOp.class)//
.setMessage(mergeMessage)//
.setNoFastForward(true)//
.addCommit(updatesBranchTip)//
.setOurs(mergeOurs)//
.setTheirs(!mergeOurs)//
.setProgressListener(SIMPLE_PROGRESS)//
.call();
return mergeReport;
}


@Test
public void testEmptyRepo() throws Exception {
Iterator<RevCommit> logs = logOp.call();
Expand Down Expand Up @@ -483,6 +593,69 @@ public void testMergedWithPathFilter() throws Exception {
assertFalse(iterator.hasNext());
}

@Test
public void testTopoWithMergedAndSince() throws Exception {
// Create the following revision graph
// o
// |
// o - Points 1 added
// |\
// | o - branch1 - Points 2 added
// |
// o - Points 3 added
// |
// o - master - HEAD - Lines 1 added
insertAndAdd(points1);
geogig.command(CommitOp.class).setMessage("commit for " + idP1).call();

// create branch1 and checkout
geogig.command(BranchCreateOp.class).setAutoCheckout(true).setName("branch1").call();
insertAndAdd(points2);
final RevCommit points2Added = geogig.command(CommitOp.class)
.setMessage("commit for " + idP2).call();

// checkout master
geogig.command(CheckoutOp.class).setSource("master").call();
insertAndAdd(points3);
RevCommit sinceCommit = geogig.command(CommitOp.class).setMessage("commit for " + idP3)
.call();
insertAndAdd(lines1);
RevCommit lines1Added = geogig.command(CommitOp.class).setMessage("commit for " + idL1)
.call();

// Merge branch1 into master to create the following revision graph
// o
// |
// o - Points 1 added
// |\
// | o - branch1 - Points 2 added
// | |
// o | - Points 3 added ("since" commit)
// | |
// o | - Lines 1 added
// |/
// o - master - HEAD - Merge commit

Ref branch1 = geogig.command(RefParse.class).setName("branch1").call().get();
MergeReport mergeReport = geogig.command(MergeOp.class).addCommit(branch1.getObjectId())
.setMessage("My merge message.").call();

RevCommit mergeCommit = mergeReport.getMergeCommit();

Iterator<RevCommit> iterator = logOp.setTopoOrder(true).setSince(sinceCommit.getId())
.call();

// The log should include Merge commit, Lines 1 added, and Points 2 Added.
assertNotNull(iterator);
assertTrue(iterator.hasNext());
assertEquals(mergeCommit, iterator.next());
assertTrue(iterator.hasNext());
assertEquals(lines1Added, iterator.next());
assertTrue(iterator.hasNext());
assertEquals(points2Added, iterator.next());
assertFalse(iterator.hasNext());
}

@Test
public void testAll() throws Exception {
// Create the following revision graph
Expand Down
17 changes: 17 additions & 0 deletions src/core/src/test/java/org/locationtech/geogig/test/integration/RepositoryTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ public abstract class RepositoryTestCase extends Assert {

protected Feature points3;

protected Feature points2_modified;

protected Feature points3_modified;

public static final String linesNs = "http://geogig.lines";

public static final String linesName = "Lines";
Expand All @@ -134,6 +138,9 @@ public abstract class RepositoryTestCase extends Assert {

public Feature lines1;

Feature lines1_modified;


public Feature lines2;

public Feature lines3;
Expand Down Expand Up @@ -241,6 +248,16 @@ protected final void doSetUp() throws IOException, SchemaException, ParseExcepti
poly3 = feature(polyType, idPG3, "StringProp3_3", new Integer(3000),
"POLYGON ((11 11, 12 12, 13 13, 14 14, 11 11))");


points2_modified = feature(pointsType, idP2, "StringProp1_2a", new Integer(2001),
"POINT(2 3)");

points3_modified = feature(pointsType, idP3, "StringProp1_3a", new Integer(3001),
"POINT(3 4)");

lines1_modified = feature(linesType, idL1, "StringProp2_1a", new Integer(1001),
"LINESTRING (1 2, 2 2)");

setUpInternal();
}

Expand Down
Loading

0 comments on commit 33941d1

Please sign in to comment.