Improve layout of branches in the commit graph

The aim of this change is to place all commits of a branch on the same
lane and commits of other (side) branches on different lanes.

The algorithm treats first parents of a commit specially by placing them
on the same lane as the commit itself. When a commit is the first parent
of multiple children it could be placed on any of these children's
lanes. In this case it is placed on the longest child lane, as this is
usually the lane of the branch the commit actually was made on.

Other (non-first) parents are placed on new lanes. This creates a layout
that should make it easier to see branches and merges and follow linear
branch histories.
This differs from the previous approach, which sometimes plotted the
commits of a side branch on the same lane as the base branch commits and
further commits on the base branch appeared on a different lane.
This made the base branch appear as if it was the side branch and
the side branch appears to be the base branch.

In addition to lane assignment, also the plotting code changed to start
drawing a branch lane from the commit where it forks out. Previously it
started only when the first commit on the branch appeared.

Active lanes are continued with every commit that is processed.
Previously lanes were only continued when the next commit on the lane
was encountered. This could produce (temporarily) dangling commits if
the next commit on the lane was not processed yet.

CQ: 8299
Bug: 419359
Bug: 434945
Change-Id: Ibe547aa24b5948ae264f7d0f56a492a4ef335608
Signed-off-by: Konrad Kügler <swamblumat-eclipsebugs@yahoo.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010, Christian Halstrick <christian.halstrick@sap.com>
+ * Copyright (C) 2010, 2014 Christian Halstrick <christian.halstrick@sap.com>
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -42,9 +42,14 @@
  */
 package org.eclipse.jgit.revplot;
 
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 
+import java.util.HashSet;
+import java.util.Set;
+
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalkTestCase;
 import org.junit.Test;
@@ -75,6 +80,25 @@ public class PlotCommitListTest extends RevWalkTestCase {
 			return this;
 		}
 
+		public int getLanePos() {
+			return current.getLane().position;
+		}
+
+		/**
+		 * Checks that the current position is valid and consumes this position.
+		 *
+		 * @param allowedPositions
+		 * @return this
+		 */
+		public CommitListAssert lanePos(Set<Integer> allowedPositions) {
+			PlotLane lane = current.getLane();
+			@SuppressWarnings("boxing")
+			boolean found = allowedPositions.remove(lane.getPosition());
+			assertTrue("Position of lane of commit #" + (nextIndex - 1)
+					+ " not as expected. Expecting one of: " + allowedPositions + " Actual: "+ lane.getPosition(), found);
+			return this;
+		}
+
 		public CommitListAssert nrOfPassingLanes(int lanes) {
 			assertEquals("Number of passing lanes of commit #"
 					+ (nextIndex - 1)
@@ -98,6 +122,13 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		}
 	}
 
+	private static Set<Integer> asSet(int... numbers) {
+		Set<Integer> result = new HashSet<Integer>();
+		for (int n : numbers)
+			result.add(Integer.valueOf(n));
+		return result;
+	}
+
 	@Test
 	public void testLinear() throws Exception {
 		final RevCommit a = commit();
@@ -134,8 +165,8 @@ public class PlotCommitListTest extends RevWalkTestCase {
 
 		CommitListAssert test = new CommitListAssert(pcl);
 		test.commit(d).lanePos(0).parents(b, c);
-		test.commit(c).lanePos(0).parents(a);
-		test.commit(b).lanePos(1).parents(a);
+		test.commit(c).lanePos(1).parents(a);
+		test.commit(b).lanePos(0).parents(a);
 		test.commit(a).lanePos(0).parents();
 		test.noMoreCommits();
 	}
@@ -154,10 +185,11 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		pcl.source(pw);
 		pcl.fillTo(Integer.MAX_VALUE);
 
+		Set<Integer> childPositions = asSet(0, 1);
 		CommitListAssert test = new CommitListAssert(pcl);
-		test.commit(c).lanePos(0).parents(a);
-		test.commit(b).lanePos(0).parents(a);
-		test.commit(a).lanePos(1).parents();
+		test.commit(c).lanePos(childPositions).parents(a);
+		test.commit(b).lanePos(childPositions).parents(a);
+		test.commit(a).lanePos(0).parents();
 		test.noMoreCommits();
 	}
 
@@ -177,11 +209,12 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		pcl.source(pw);
 		pcl.fillTo(Integer.MAX_VALUE);
 
+		Set<Integer> childPositions = asSet(0, 1, 2);
 		CommitListAssert test = new CommitListAssert(pcl);
-		test.commit(d).lanePos(0).parents(a);
-		test.commit(c).lanePos(0).parents(a);
-		test.commit(b).lanePos(0).parents(a);
-		test.commit(a).lanePos(1).parents();
+		test.commit(d).lanePos(childPositions).parents(a);
+		test.commit(c).lanePos(childPositions).parents(a);
+		test.commit(b).lanePos(childPositions).parents(a);
+		test.commit(a).lanePos(0).parents();
 		test.noMoreCommits();
 	}
 
@@ -211,14 +244,17 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		pcl.source(pw);
 		pcl.fillTo(Integer.MAX_VALUE);
 
+		Set<Integer> childPositions = asSet(0, 1, 2, 3, 4);
 		CommitListAssert test = new CommitListAssert(pcl);
-		test.commit(g).lanePos(0).parents(f);
-		test.commit(f).lanePos(0).parents(a);
-		test.commit(e).lanePos(0).parents(a);
-		test.commit(d).lanePos(0).parents(a);
-		test.commit(c).lanePos(0).parents(a);
-		test.commit(b).lanePos(0).parents(a);
-		test.commit(a).lanePos(1).parents();
+		int posG = test.commit(g).lanePos(childPositions).parents(f)
+				.getLanePos();
+		test.commit(f).lanePos(posG).parents(a);
+
+		test.commit(e).lanePos(childPositions).parents(a);
+		test.commit(d).lanePos(childPositions).parents(a);
+		test.commit(c).lanePos(childPositions).parents(a);
+		test.commit(b).lanePos(childPositions).parents(a);
+		test.commit(a).lanePos(0).parents();
 		test.noMoreCommits();
 	}
 
@@ -241,16 +277,18 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
 		pcl.source(pw);
 		pcl.fillTo(Integer.MAX_VALUE);
+		Set<Integer> childPositions = asSet(0, 1);
 		CommitListAssert test = new CommitListAssert(pcl);
-		test.commit(i).lanePos(1).parents(h);
-		test.commit(h).lanePos(1).parents(f);
-		test.commit(g).lanePos(0).parents(a);
-		test.commit(f).lanePos(1).parents(e, d);
-		test.commit(e).lanePos(0).parents(c);
-		test.commit(d).lanePos(1).parents(b);
-		test.commit(c).lanePos(0).parents(b);
-		test.commit(b).lanePos(1).parents(a);
-		test.commit(a).lanePos(2).parents();
+		int posI = test.commit(i).lanePos(childPositions).parents(h)
+				.getLanePos();
+		test.commit(h).lanePos(posI).parents(f);
+		test.commit(g).lanePos(childPositions).parents(a);
+		test.commit(f).lanePos(posI).parents(e, d);
+		test.commit(e).lanePos(posI).parents(c);
+		test.commit(d).lanePos(2).parents(b);
+		test.commit(c).lanePos(posI).parents(b);
+		test.commit(b).lanePos(posI).parents(a);
+		test.commit(a).lanePos(0).parents();
 	}
 
 	// test the history of the egit project between 9fdaf3c1 and e76ad9170f
@@ -276,8 +314,7 @@ public class PlotCommitListTest extends RevWalkTestCase {
 				disable_source);
 		final RevCommit merge_changeset_implementation = commit(
 				merge_disable_source, changeset_implementation);
-		final RevCommit clone_operation = commit(merge_disable_source,
-				merge_changeset_implementation);
+		final RevCommit clone_operation = commit(merge_changeset_implementation);
 		final RevCommit update_eclipse = commit(add_Maven);
 		final RevCommit merge_resolve_handler = commit(clone_operation,
 				resolve_handler);
@@ -302,48 +339,57 @@ public class PlotCommitListTest extends RevWalkTestCase {
 
 		CommitListAssert test = new CommitListAssert(pcl);
 
+		// Note: all positions of side branches are rather arbitrary, but some
+		// may not overlap. Testing for the positions yielded by the current
+		// implementation, which was manually checked to not overlap.
+		final int mainPos = 0;
 		test.commit(merge_fixed_logged_npe).parents(sort_roots, fix_logged_npe)
-				.lanePos(0);
+				.lanePos(mainPos);
 		test.commit(fix_logged_npe).parents(merge_changeset_implementation)
-				.lanePos(0);
-		test.commit(sort_roots).parents(merge_update_eclipse).lanePos(1);
-		test.commit(merge_update_eclipse).parents(add_a_clear, update_eclipse)
 				.lanePos(1);
-		test.commit(add_a_clear).parents(fix_broken).lanePos(1);
-		test.commit(fix_broken).parents(merge_disable_comment).lanePos(1);
+		test.commit(sort_roots).parents(merge_update_eclipse).lanePos(mainPos);
+		test.commit(merge_update_eclipse).parents(add_a_clear, update_eclipse)
+				.lanePos(mainPos);
+		test.commit(add_a_clear).parents(fix_broken).lanePos(mainPos);
+		test.commit(fix_broken).parents(merge_disable_comment).lanePos(mainPos);
 		test.commit(merge_disable_comment)
-				.parents(merge_resolve_handler, disable_comment).lanePos(1);
-		test.commit(disable_comment).parents(clone_operation).lanePos(1);
+				.parents(merge_resolve_handler, disable_comment)
+				.lanePos(mainPos);
+		test.commit(disable_comment).parents(clone_operation).lanePos(2);
 		test.commit(merge_resolve_handler)
-				.parents(clone_operation, resolve_handler).lanePos(2);
+				.parents(clone_operation, resolve_handler).lanePos(mainPos);
 		test.commit(update_eclipse).parents(add_Maven).lanePos(3);
-		test.commit(clone_operation)
-				.parents(merge_disable_source, merge_changeset_implementation)
-				.lanePos(1);
+		test.commit(clone_operation).parents(merge_changeset_implementation)
+				.lanePos(mainPos);
 		test.commit(merge_changeset_implementation)
 				.parents(merge_disable_source, changeset_implementation)
-				.lanePos(0);
+				.lanePos(mainPos);
 		test.commit(merge_disable_source)
-				.parents(update_eclipse_iplog2, disable_source).lanePos(1);
-		test.commit(update_eclipse_iplog2).parents(merge_use_remote).lanePos(0);
+				.parents(update_eclipse_iplog2, disable_source)
+				.lanePos(mainPos);
+		test.commit(update_eclipse_iplog2).parents(merge_use_remote)
+				.lanePos(mainPos);
 		test.commit(disable_source).parents(merge_use_remote).lanePos(1);
 		test.commit(merge_use_remote).parents(update_eclipse_iplog, use_remote)
-				.lanePos(0);
+				.lanePos(mainPos);
 		test.commit(changeset_implementation).parents(clear_repositorycache)
 				.lanePos(2);
-		test.commit(update_eclipse_iplog).parents(merge_add_Maven).lanePos(0);
+		test.commit(update_eclipse_iplog).parents(merge_add_Maven)
+				.lanePos(mainPos);
 		test.commit(merge_add_Maven).parents(findToolBar_layout, add_Maven)
-				.lanePos(0);
+				.lanePos(mainPos);
 		test.commit(findToolBar_layout).parents(clear_repositorycache)
-				.lanePos(0);
+				.lanePos(mainPos);
 		test.commit(use_remote).parents(clear_repositorycache).lanePos(1);
 		test.commit(add_Maven).parents(clear_repositorycache).lanePos(3);
-		test.commit(clear_repositorycache).parents(merge_remove).lanePos(2);
+		test.commit(clear_repositorycache).parents(merge_remove)
+				.lanePos(mainPos);
 		test.commit(resolve_handler).parents(merge_fix).lanePos(4);
-		test.commit(merge_remove).parents(add_simple, remove_unused).lanePos(2);
-		test.commit(remove_unused).parents(merge_fix).lanePos(0);
-		test.commit(add_simple).parents(merge_fix).lanePos(1);
-		test.commit(merge_fix).parents().lanePos(3);
+		test.commit(merge_remove).parents(add_simple, remove_unused)
+				.lanePos(mainPos);
+		test.commit(remove_unused).parents(merge_fix).lanePos(1);
+		test.commit(add_simple).parents(merge_fix).lanePos(mainPos);
+		test.commit(merge_fix).parents().lanePos(mainPos);
 		test.noMoreCommits();
 	}
 
@@ -373,4 +419,253 @@ public class PlotCommitListTest extends RevWalkTestCase {
 		test.noMoreCommits();
 	}
 
+	/**
+	 * The graph shows the problematic original positioning. Due to this some
+	 * lanes are no straight lines here, but they are with the new layout code)
+	 *
+	 * <pre>
+	 * a5
+	 * | \
+	 * |  a4
+	 * | /
+	 * a3
+	 * |
+	 * |  e
+	 * |    \
+	 * |     |
+	 * |  b3 |
+	 * |  |  d
+	 * |  |/
+	 * | /|
+	 * |/ |
+	 * a2 |
+	 * |  b2
+	 * |    \
+	 * |  c |
+	 * | /  /
+	 * |/ b1
+	 * a1
+	 * </pre>
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testBug419359() throws Exception {
+		// this may not be the exact situation of bug 419359 but it shows
+		// similar behavior
+		final RevCommit a1 = commit();
+		final RevCommit b1 = commit();
+		final RevCommit c = commit(a1);
+		final RevCommit b2 = commit(b1);
+		final RevCommit a2 = commit(a1);
+		final RevCommit d = commit(a2);
+		final RevCommit b3 = commit(b2);
+		final RevCommit e = commit(d);
+		final RevCommit a3 = commit(a2);
+		final RevCommit a4 = commit(a3);
+		final RevCommit a5 = commit(a3, a4);
+
+		PlotWalk pw = new PlotWalk(db);
+		pw.markStart(pw.lookupCommit(b3.getId()));
+		pw.markStart(pw.lookupCommit(c.getId()));
+		pw.markStart(pw.lookupCommit(e.getId()));
+		pw.markStart(pw.lookupCommit(a5.getId()));
+
+		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
+		pcl.source(pw);
+		pcl.fillTo(Integer.MAX_VALUE);
+
+		// test that the commits b1, b2 and b3 are on the same position
+		int bPos = pcl.get(9).lane.position; // b1
+		assertEquals("b2 is an a different position", bPos,
+				pcl.get(7).lane.position);
+		assertEquals("b3 is on a different position", bPos,
+				pcl.get(4).lane.position);
+
+		// test that nothing blocks the connections between b1, b2 and b3
+		assertNotEquals("b lane is blocked by c", bPos,
+				pcl.get(8).lane.position);
+		assertNotEquals("b lane is blocked by a2", bPos,
+				pcl.get(6).lane.position);
+		assertNotEquals("b lane is blocked by d", bPos,
+				pcl.get(5).lane.position);
+	}
+
+	/**
+	 * <pre>
+	 *    b3
+	 * a4 |
+	 * | \|
+	 * |  b2
+	 * a3 |
+	 * | \|
+	 * a2 |
+	 * |  b1
+	 * | /
+	 * a1
+	 * </pre>
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testMultipleMerges() throws Exception {
+		final RevCommit a1 = commit();
+		final RevCommit b1 = commit(a1);
+		final RevCommit a2 = commit(a1);
+		final RevCommit a3 = commit(a2, b1);
+		final RevCommit b2 = commit(b1);
+		final RevCommit a4 = commit(a3, b2);
+		final RevCommit b3 = commit(b2);
+
+		PlotWalk pw = new PlotWalk(db);
+		pw.markStart(pw.lookupCommit(a4));
+		pw.markStart(pw.lookupCommit(b3));
+		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
+		pcl.source(pw);
+		pcl.fillTo(Integer.MAX_VALUE);
+
+		Set<Integer> positions = asSet(0, 1);
+		CommitListAssert test = new CommitListAssert(pcl);
+		int posB = test.commit(b3).lanePos(positions).getLanePos();
+		int posA = test.commit(a4).lanePos(positions).getLanePos();
+		test.commit(b2).lanePos(posB);
+		test.commit(a3).lanePos(posA);
+		test.commit(a2).lanePos(posA);
+		test.commit(b1).lanePos(posB);
+		test.commit(a1).lanePos(posA);
+		test.noMoreCommits();
+	}
+
+	/**
+	 * <pre>
+	 * a4
+	 * |   b3
+	 * a3  |
+	 * | \\|
+	 * |   |\\
+	 * |   b2||
+	 * a2  | //
+	 * |   b1
+	 * | /
+	 * a1
+	 * </pre>
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testMergeBlockedBySelf() throws Exception {
+		final RevCommit a1 = commit();
+		final RevCommit b1 = commit(a1);
+		final RevCommit a2 = commit(a1);
+		final RevCommit b2 = commit(b1); // blocks merging arc
+		final RevCommit a3 = commit(a2, b1);
+		final RevCommit b3 = commit(b2);
+		final RevCommit a4 = commit(a3);
+
+		PlotWalk pw = new PlotWalk(db);
+		pw.markStart(pw.lookupCommit(a4));
+		pw.markStart(pw.lookupCommit(b3));
+		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
+		pcl.source(pw);
+		pcl.fillTo(Integer.MAX_VALUE);
+
+		Set<Integer> positions = asSet(0, 1);
+		CommitListAssert test = new CommitListAssert(pcl);
+		int posA = test.commit(a4).lanePos(positions).getLanePos();
+		int posB = test.commit(b3).lanePos(positions).getLanePos();
+		test.commit(a3).lanePos(posA);
+		test.commit(b2).lanePos(posB);
+		test.commit(a2).lanePos(posA);
+		// b1 is not repositioned, uses "detour lane"
+		// (drawn as a double arc in the ascii graph above)
+		test.commit(b1).lanePos(posB);
+		test.commit(a1).lanePos(posA);
+		test.noMoreCommits();
+	}
+
+	/**
+	 * <pre>
+	 *      b2
+	 * a4   |
+	 * |  \ |
+	 * a3  \|
+	 * | \  |
+	 * |  c |
+	 * | /  |
+	 * a2   |
+	 * |    b1
+	 *     /
+	 * |  /
+	 * a1
+	 * </pre>
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testMergeBlockedByOther() throws Exception {
+		final RevCommit a1 = commit();
+		final RevCommit b1 = commit(a1);
+		final RevCommit a2 = commit(a1);
+		final RevCommit c = commit(a2);// blocks merging arc
+		final RevCommit a3 = commit(a2, c);
+		final RevCommit a4 = commit(a3, b1);
+		final RevCommit b2 = commit(b1);
+
+		PlotWalk pw = new PlotWalk(db);
+		pw.markStart(pw.lookupCommit(a4));
+		pw.markStart(pw.lookupCommit(b2));
+		pw.markStart(pw.lookupCommit(c));
+		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
+		pcl.source(pw);
+		pcl.fillTo(Integer.MAX_VALUE);
+
+		Set<Integer> positions = asSet(0, 1, 2);
+		CommitListAssert test = new CommitListAssert(pcl);
+		int posB = test.commit(b2).lanePos(positions).getLanePos();
+		int posA = test.commit(a4).lanePos(positions).getLanePos();
+		test.commit(a3).lanePos(posA);
+		test.commit(c).lanePos(positions);
+		test.commit(a2).lanePos(posA);
+		test.commit(b1).lanePos(posB); // repositioned to go around c
+		test.commit(a1).lanePos(posA);
+		test.noMoreCommits();
+	}
+
+	/**
+	 * <pre>
+	 *     b1
+	 * a3  |
+	 * |   |
+	 * a2  |
+	 * -- processing stops here --
+	 * |  /
+	 * a1
+	 * </pre>
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testDanglingCommitShouldContinueLane() throws Exception {
+		final RevCommit a1 = commit();
+		final RevCommit a2 = commit(a1);
+		final RevCommit a3 = commit(a2);
+		final RevCommit b1 = commit(a1);
+
+		PlotWalk pw = new PlotWalk(db);
+		pw.markStart(pw.lookupCommit(a3));
+		pw.markStart(pw.lookupCommit(b1));
+		PlotCommitList<PlotLane> pcl = new PlotCommitList<PlotLane>();
+		pcl.source(pw);
+		pcl.fillTo(2); // don't process a1
+
+		Set<Integer> positions = asSet(0, 1);
+		CommitListAssert test = new CommitListAssert(pcl);
+		PlotLane laneB = test.commit(b1).lanePos(positions).current.getLane();
+		int posA = test.commit(a3).lanePos(positions).getLanePos();
+		test.commit(a2).lanePos(posA);
+		assertArrayEquals(
+				"Although the parent of b1, a1, is not processed yet, the b lane should still be drawn",
+				new PlotLane[] { laneB }, test.current.passingLanes);
+		test.noMoreCommits();
+	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/revplot/AbstractPlotRenderer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org>
+ * Copyright (C) 2008, 2014 Shawn O. Pearce <spearce@spearce.org>
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -105,42 +105,63 @@ public abstract class AbstractPlotRenderer<TLane extends PlotLane, TColor> {
 			maxCenter = Math.max(maxCenter, cx);
 		}
 
+		final int dotX = myLaneX - dotSize / 2 - 1;
+		final int dotY = (h - dotSize) / 2;
+
 		final int nParent = commit.getParentCount();
-		for (int i = 0; i < nParent; i++) {
-			final PlotCommit<TLane> p;
-			final TLane pLane;
-			final TColor pColor;
-			final int cx;
+		if (nParent > 0) {
+			drawLine(myColor, myLaneX, h, myLaneX, (h + dotSize) / 2,
+					LINE_WIDTH);
 
-			p = (PlotCommit<TLane>) commit.getParent(i);
-			pLane = p.getLane();
-			if (pLane == null)
-				continue;
+			for (int i = 0; i < commit.mergingLanes.length; i++) {
+				@SuppressWarnings("unchecked")
+				final TLane pLane = (TLane) commit.mergingLanes[i];
+				final TColor pColor = laneColor(pLane);
+				final int cx = laneC(pLane);
 
-			pColor = laneColor(pLane);
-			cx = laneC(pLane);
+				if (Math.abs(myLaneX - cx) > LANE_WIDTH) {
+					final int ix;
+					if (myLaneX < cx)
+						ix = cx - LANE_WIDTH / 2;
+					else
+						ix = cx + LANE_WIDTH / 2;
 
-			if (Math.abs(myLaneX - cx) > LANE_WIDTH) {
-				if (myLaneX < cx) {
-					final int ix = cx - LANE_WIDTH / 2;
 					drawLine(pColor, myLaneX, h / 2, ix, h / 2, LINE_WIDTH);
 					drawLine(pColor, ix, h / 2, cx, h, LINE_WIDTH);
-				} else {
-					final int ix = cx + LANE_WIDTH / 2;
-					drawLine(pColor, myLaneX, h / 2, ix, h / 2, LINE_WIDTH);
-					drawLine(pColor, ix, h / 2, cx, h, LINE_WIDTH);
-				}
-			} else {
-				drawLine(pColor, myLaneX, h / 2, cx, h, LINE_WIDTH);
+				} else
+					drawLine(pColor, myLaneX, h / 2, cx, h, LINE_WIDTH);
+
+				maxCenter = Math.max(maxCenter, cx);
 			}
-			maxCenter = Math.max(maxCenter, cx);
 		}
 
-		final int dotX = myLaneX - dotSize / 2 - 1;
-		final int dotY = (h - dotSize) / 2;
 
-		if (commit.getChildCount() > 0)
-			drawLine(myColor, myLaneX, 0, myLaneX, dotY, LINE_WIDTH);
+		if (commit.getChildCount() > 0) {
+			for (int i = 0; i < commit.forkingOffLanes.length; i++) {
+				@SuppressWarnings("unchecked")
+				final TLane childLane = (TLane) commit.forkingOffLanes[i];
+				final TColor cColor = laneColor(childLane);
+				final int cx = laneC(childLane);
+				if (Math.abs(myLaneX - cx) > LANE_WIDTH) {
+					final int ix;
+					if (myLaneX < cx)
+						ix = cx - LANE_WIDTH / 2;
+					else
+						ix = cx + LANE_WIDTH / 2;
+
+					drawLine(cColor, myLaneX, h / 2, ix, h / 2, LINE_WIDTH);
+					drawLine(cColor, ix, h / 2, cx, 0, LINE_WIDTH);
+				} else {
+					drawLine(cColor, myLaneX, h / 2, cx, 0, LINE_WIDTH);
+				}
+				maxCenter = Math.max(maxCenter, cx);
+			}
+
+			int nonForkingChildren = commit.getChildCount()
+					- commit.forkingOffLanes.length;
+			if (nonForkingChildren > 0)
+						drawLine(myColor, myLaneX, 0, myLaneX, dotY, LINE_WIDTH);
+		}
 
 		if (commit.has(RevFlag.UNINTERESTING))
 			drawBoundaryDot(dotX, dotY, dotSize, dotSize);

org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommit.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org>
+ * Copyright (C) 2008, 2014 Shawn O. Pearce <spearce@spearce.org>
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -61,8 +61,12 @@ public class PlotCommit<L extends PlotLane> extends RevCommit {
 
 	static final Ref[] NO_REFS = {};
 
+	PlotLane[] forkingOffLanes;
+
 	PlotLane[] passingLanes;
 
+	PlotLane[] mergingLanes;
+
 	PlotLane lane;
 
 	PlotCommit[] children;
@@ -77,23 +81,38 @@ public class PlotCommit<L extends PlotLane> extends RevCommit {
 	 */
 	protected PlotCommit(final AnyObjectId id) {
 		super(id);
+		forkingOffLanes = NO_LANES;
 		passingLanes = NO_LANES;
+		mergingLanes = NO_LANES;
 		children = NO_CHILDREN;
 		refs = NO_REFS;
 	}
 
+	void addForkingOffLane(final PlotLane f) {
+		forkingOffLanes = addLane(f, forkingOffLanes);
+	}
+
 	void addPassingLane(final PlotLane c) {
-		final int cnt = passingLanes.length;
+		passingLanes = addLane(c, passingLanes);
+	}
+
+	void addMergingLane(final PlotLane m) {
+		mergingLanes = addLane(m, mergingLanes);
+	}
+
+	private static PlotLane[] addLane(final PlotLane l, PlotLane[] lanes) {
+		final int cnt = lanes.length;
 		if (cnt == 0)
-			passingLanes = new PlotLane[] { c };
+			lanes = new PlotLane[] { l };
 		else if (cnt == 1)
-			passingLanes = new PlotLane[] { passingLanes[0], c };
+			lanes = new PlotLane[] { lanes[0], l };
 		else {
 			final PlotLane[] n = new PlotLane[cnt + 1];
-			System.arraycopy(passingLanes, 0, n, 0, cnt);
-			n[cnt] = c;
-			passingLanes = n;
+			System.arraycopy(lanes, 0, n, 0, cnt);
+			n[cnt] = l;
+			lanes = n;
 		}
+		return lanes;
 	}
 
 	void addChild(final PlotCommit c) {
@@ -185,7 +204,9 @@ public class PlotCommit<L extends PlotLane> extends RevCommit {
 
 	@Override
 	public void reset() {
+		forkingOffLanes = NO_LANES;
 		passingLanes = NO_LANES;
+		mergingLanes = NO_LANES;
 		children = NO_CHILDREN;
 		lane = null;
 		super.reset();

org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java

@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org>,
  * Copyright (C) 2010, Christian Halstrick <christian.halstrick@sap.com>
+ * Copyright (C) 2014, Konrad Kügler
  * and other copyright owners as documented in the project's IP log.
  *
  * This program and the accompanying materials are made available
@@ -47,6 +48,7 @@ package org.eclipse.jgit.revplot;
 import java.text.MessageFormat;
 import java.util.BitSet;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.TreeSet;
 
@@ -76,12 +78,17 @@ public class PlotCommitList<L extends PlotLane> extends
 
 	private final HashSet<PlotLane> activeLanes = new HashSet<PlotLane>(32);
 
+	/** number of (child) commits on a lane */
+	private final HashMap<PlotLane, Integer> laneLength = new HashMap<PlotLane, Integer>(
+			32);
+
 	@Override
 	public void clear() {
 		super.clear();
 		positionsAllocated = 0;
 		freePositions.clear();
 		activeLanes.clear();
+		laneLength.clear();
 	}
 
 	@Override
@@ -121,97 +128,201 @@ public class PlotCommitList<L extends PlotLane> extends
 		final int nChildren = currCommit.getChildCount();
 		if (nChildren == 0) {
 			currCommit.lane = nextFreeLane();
-			closeLane(currCommit.lane);
+			continueActiveLanes(currCommit);
 			return;
 		}
 
 		if (nChildren == 1 && currCommit.children[0].getParentCount() < 2) {
 			// Only one child, child has only us as their parent.
 			// Stay in the same lane as the child.
-			//
-			final PlotCommit c = currCommit.children[0];
-			for (int r = index - 1; r >= 0; r--) {
-				final PlotCommit rObj = get(r);
-				if (rObj == c)
-					break;
-				rObj.addPassingLane(c.lane);
-			}
 
+			@SuppressWarnings("unchecked")
+			final PlotCommit<L> c = currCommit.children[0];
 			currCommit.lane = c.lane;
-			handleBlockedLanes(index, currCommit, nChildren);
+			Integer len = laneLength.get(currCommit.lane);
+			len = Integer.valueOf(len.intValue() + 1);
+			laneLength.put(currCommit.lane, len);
 		} else {
 			// More than one child, or our child is a merge.
-			// Use a different lane.
-			//
-
-			// Process all our children. Especially important when there is more
-			// than one child (e.g. a commit is processed where other branches
-			// fork out). For each child the following is done
-			// 1. If no lane was assigned to the child a new lane is created and
-			// assigned
-			// 2. The lane of the child is closed. If this frees a position,
-			// this position will be added freePositions list.
-			// If we have multiple children which where previously not on a lane
-			// each such child will get his own new lane but all those new lanes
-			// will be on the same position. We have to take care that not
-			// multiple newly created (in step 1) lanes occupy that position on
-			// which the
-			// parent's lane will be on. Therefore we delay closing the lane
-			// with the parents position until all children are processed.
-
-			// The lane on that position the current commit will be on
+
+			// We look for the child lane the current commit should continue.
+			// Candidate lanes for this are those with children, that have the
+			// current commit as their first parent.
+			// There can be multiple candidate lanes. In that case the longest
+			// lane is chosen, as this is usually the lane representing the
+			// branch the commit actually was made on.
+
+			// When there are no candidate lanes (i.e. the current commit has
+			// only children whose non-first parent it is) we place the current
+			// commit on a new lane.
+
+			// The lane the current commit will be placed on:
 			PlotLane reservedLane = null;
+			PlotCommit childOnReservedLane = null;
+			int lengthOfReservedLane = -1;
 
 			for (int i = 0; i < nChildren; i++) {
-				final PlotCommit c = currCommit.children[i];
-				if (reservedLane == null && activeLanes.contains(c.lane))
-					reservedLane = c.lane;
-				else
-					closeLane(c.lane);
+				@SuppressWarnings("unchecked")
+				final PlotCommit<L> c = currCommit.children[i];
+				if (c.getParent(0) == currCommit) {
+					Integer len = laneLength.get(c.lane);
+					// we may be the first parent for multiple lines of
+					// development, try to continue the longest one
+					if (len.intValue() > lengthOfReservedLane) {
+						reservedLane = c.lane;
+						childOnReservedLane = c;
+						lengthOfReservedLane = len.intValue();
+					}
+				}
 			}
 
-			// finally all children are processed. We can close the lane on that
-			// position our current commit will be on.
-			if (reservedLane != null)
-				closeLane(reservedLane);
-
-			currCommit.lane = nextFreeLane();
+			if (reservedLane != null) {
+				currCommit.lane = reservedLane;
+				laneLength.put(reservedLane,
+						Integer.valueOf(lengthOfReservedLane + 1));
+				handleBlockedLanes(index, currCommit, childOnReservedLane);
+			} else {
+				currCommit.lane = nextFreeLane();
+				handleBlockedLanes(index, currCommit, null);
+			}
 
-			handleBlockedLanes(index, currCommit, nChildren);
+			// close lanes of children, if there are no first parents that might
+			// want to continue the child lanes
+			for (int i = 0; i < nChildren; i++) {
+				final PlotCommit c = currCommit.children[i];
+				PlotCommit firstParent = (PlotCommit) c.getParent(0);
+				if (firstParent.lane != null && firstParent.lane != c.lane)
+					closeLane(c.lane);
+			}
 		}
+		continueActiveLanes(currCommit);
+	}
+
+	private void continueActiveLanes(final PlotCommit currCommit) {
+		for (PlotLane lane : activeLanes)
+			if (lane != currCommit.lane)
+				currCommit.addPassingLane(lane);
 	}
 
 	/**
-	 * when connecting a plotcommit to the child make sure that you will not be
-	 * located on a lane on which a passed commit is located on. Otherwise we
-	 * would have to draw a line through a commit.
+	 * Sets up fork and merge information in the involved PlotCommits.
+	 * Recognizes and handles blockades that involve forking or merging arcs.
 	 *
 	 * @param index
-	 * @param commit
-	 * @param nChildren
+	 *            the index of <code>currCommit</code> in the list
+	 * @param currCommit
+	 * @param childOnLane
+	 *            the direct child on the same lane as <code>currCommit</code>,
+	 *            may be null if <code>currCommit</code> is the first commit on
+	 *            the lane
 	 */
-	private void handleBlockedLanes(final int index,
-			final PlotCommit<L> commit, final int nChildren) {
-		// take care:
-		int remaining = nChildren;
+	private void handleBlockedLanes(final int index, final PlotCommit currCommit,
+			final PlotCommit childOnLane) {
+		for (PlotCommit child : currCommit.children) {
+			if (child == childOnLane)
+				continue; // simple continuations of lanes are handled by
+							// continueActiveLanes() calls in enter()
+
+			// Is the child a merge or is it forking off?
+			boolean childIsMerge = child.getParent(0) != currCommit;
+			if (childIsMerge) {
+				PlotLane laneToUse = currCommit.lane;
+				laneToUse = handleMerge(index, currCommit, childOnLane, child,
+						laneToUse);
+				child.addMergingLane(laneToUse);
+			} else {
+				// We want to draw a forking arc in the child's lane.
+				// As an active lane, the child lane already continues
+				// (unblocked) up to this commit, we only need to mark it as
+				// forking off from the current commit.
+				PlotLane laneToUse = child.lane;
+				currCommit.addForkingOffLane(laneToUse);
+			}
+		}
+	}
+
+	// Handles the case where currCommit is a non-first parent of the child
+	private PlotLane handleMerge(final int index, final PlotCommit currCommit,
+			final PlotCommit childOnLane, PlotCommit child, PlotLane laneToUse) {
+
+		// find all blocked positions between currCommit and this child
+
+		int childIndex = index; // useless initialization, should
+								// always be set in the loop below
 		BitSet blockedPositions = new BitSet();
 		for (int r = index - 1; r >= 0; r--) {
 			final PlotCommit rObj = get(r);
-			if (commit.isChild(rObj)) {
-				if (--remaining == 0)
-					break;
+			if (rObj == child) {
+				childIndex = r;
+				break;
 			}
 			addBlockedPosition(blockedPositions, rObj);
-			if (rObj != null) {
-				rObj.addPassingLane(commit.lane);
+		}
+
+		// handle blockades
+
+		if (blockedPositions.get(laneToUse.getPosition())) {
+			// We want to draw a merging arc in our lane to the child,
+			// which is on another lane, but our lane is blocked.
+
+			// Check if childOnLane is beetween commit and the child we
+			// are currently processing
+			boolean needDetour = false;
+			if (childOnLane != null) {
+				for (int r = index - 1; r > childIndex; r--) {
+					final PlotCommit rObj = get(r);
+					if (rObj == childOnLane) {
+						needDetour = true;
+						break;
+					}
+				}
+			}
+
+			if (needDetour) {
+				// It is childOnLane which is blocking us. Repositioning
+				// our lane would not help, because this repositions the
+				// child too, keeping the blockade.
+				// Instead, we create a "detour lane" which gets us
+				// around the blockade. That lane has no commits on it.
+				laneToUse = nextFreeLane(blockedPositions);
+				currCommit.addForkingOffLane(laneToUse);
+				closeLane(laneToUse);
+			} else {
+				// The blockade is (only) due to other (already closed)
+				// lanes at the current lane's position. In this case we
+				// reposition the current lane.
+				// We are the first commit on this lane, because
+				// otherwise the child commit on this lane would have
+				// kept other lanes from blocking us. Since we are the
+				// first commit, we can freely reposition.
+				int newPos = getFreePosition(blockedPositions);
+				freePositions.add(Integer.valueOf(laneToUse
+						.getPosition()));
+				laneToUse.position = newPos;
 			}
 		}
-		// Now let's check whether we have to reposition the lane
-		if (blockedPositions.get(commit.lane.getPosition())) {
-			int newPos = getFreePosition(blockedPositions);
-			freePositions.add(Integer.valueOf(commit.lane.getPosition()));
-			commit.lane.position = newPos;
-			activeLanes.add(commit.lane);
+
+		// Actually connect currCommit to the merge child
+		drawLaneToChild(index, child, laneToUse);
+		return laneToUse;
+	}
+
+	/**
+	 * Connects the commit at commitIndex to the child, using the given lane.
+	 * All blockades on the lane must be resolved before calling this method.
+	 *
+	 * @param commitIndex
+	 * @param child
+	 * @param laneToContinue
+	 */
+	private void drawLaneToChild(final int commitIndex, PlotCommit child,
+			PlotLane laneToContinue) {
+		for (int r = commitIndex - 1; r >= 0; r--) {
+			final PlotCommit rObj = get(r);
+			if (rObj == child)
+				break;
+			if (rObj != null)
+				rObj.addPassingLane(laneToContinue);
 		}
 	}
 
@@ -222,12 +333,20 @@ public class PlotCommitList<L extends PlotLane> extends
 			// Positions may be blocked by a commit on a lane.
 			if (lane != null)
 				blockedPositions.set(lane.getPosition());
+			// Positions may also be blocked by forking off and merging lanes.
+			// We don't consider passing lanes, because every passing lane forks
+			// off and merges at it ends.
+			for (PlotLane l : rObj.forkingOffLanes)
+				blockedPositions.set(l.getPosition());
+			for (PlotLane l : rObj.mergingLanes)
+				blockedPositions.set(l.getPosition());
 		}
 	}
 
 	private void closeLane(PlotLane lane) {
 		if (activeLanes.remove(lane)) {
 			recycleLane((L) lane);
+			laneLength.remove(lane);
 			freePositions.add(Integer.valueOf(lane.getPosition()));
 		}
 	}
@@ -246,6 +365,7 @@ public class PlotCommitList<L extends PlotLane> extends
 		final PlotLane p = createLane();
 		p.position = getFreePosition(blockedPositions);
 		activeLanes.add(p);
+		laneLength.put(p, Integer.valueOf(1));
 		return p;
 	}