PlotCommitList: Refactor lane creation and position allocation

This also properly removes the newly determinded lane position from the
freePositions set in handleBlockedLanes(). closeLane() does only recycle
active lanes, to avoid recycling lanes twice.

Change-Id: Icd019fcf7974441ed05686bb61d6de4e8bf4ab7c
Signed-off-by: Konrad Kügler <swamblumat-eclipsebugs@yahoo.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

1 files changed, 39 insertions, 20 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java b/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java
index 9175e14f2d..9686e4568a 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java
@@ -121,7 +121,6 @@ public class PlotCommitList<L extends PlotLane> extends
 		final int nChildren = currCommit.getChildCount();
 		if (nChildren == 0) {
 			currCommit.lane = nextFreeLane();
-			activeLanes.add(currCommit.lane);
 			closeLane(currCommit.lane);
 			return;
 		}
@@ -135,7 +134,6 @@ public class PlotCommitList<L extends PlotLane> extends
 				// Hmmph. This child must be the first along this lane.
 				//
 				c.lane = nextFreeLane();
-				activeLanes.add(c.lane);
 			}
 			for (int r = index - 1; r >= 0; r--) {
 				final PlotCommit rObj = get(r);
@@ -175,7 +173,6 @@ public class PlotCommitList<L extends PlotLane> extends
 				// not already positioned.
 				if (c.lane == null) {
 					c.lane = nextFreeLane();
-					activeLanes.add(c.lane);
 					if (reservedLane != null)
 						closeLane(c.lane);
 					else
@@ -192,7 +189,6 @@ public class PlotCommitList<L extends PlotLane> extends
 				closeLane(reservedLane);
 
 			currCommit.lane = nextFreeLane();
-			activeLanes.add(currCommit.lane);
 
 			handleBlockedLanes(index, currCommit, nChildren);
 		}
@@ -218,32 +214,33 @@ public class PlotCommitList<L extends PlotLane> extends
 				if (--remaining == 0)
 					break;
 			}
+			addBlockedPosition(blockedPositions, rObj);
 			if (rObj != null) {
-				PlotLane lane = rObj.getLane();
-				if (lane != null)
-					blockedPositions.set(lane.getPosition());
 				rObj.addPassingLane(commit.lane);
 			}
 		}
 		// Now let's check whether we have to reposition the lane
 		if (blockedPositions.get(commit.lane.getPosition())) {
-			int newPos = -1;
-			for (Integer pos : freePositions)
-				if (!blockedPositions.get(pos.intValue())) {
-					newPos = pos.intValue();
-					break;
-				}
-			if (newPos == -1)
-				newPos = positionsAllocated++;
+			int newPos = getFreePosition(blockedPositions);
 			freePositions.add(Integer.valueOf(commit.lane.getPosition()));
 			commit.lane.position = newPos;
 			activeLanes.add(commit.lane);
 		}
 	}
 
+	private static void addBlockedPosition(BitSet blockedPositions,
+			final PlotCommit rObj) {
+		if (rObj != null) {
+			PlotLane lane = rObj.getLane();
+			// Positions may be blocked by a commit on a lane.
+			if (lane != null)
+				blockedPositions.set(lane.getPosition());
+		}
+	}
+
 	private void closeLane(PlotLane lane) {
-		recycleLane((L) lane);
 		if (activeLanes.remove(lane)) {
+			recycleLane((L) lane);
 			freePositions.add(Integer.valueOf(lane.getPosition()));
 		}
 	}
@@ -255,15 +252,37 @@ public class PlotCommitList<L extends PlotLane> extends
 	}
 
 	private PlotLane nextFreeLane() {
+		return nextFreeLane(null);
+	}
+
+	private PlotLane nextFreeLane(BitSet blockedPositions) {
 		final PlotLane p = createLane();
-		if (freePositions.isEmpty()) {
-			p.position = positionsAllocated++;
+		p.position = getFreePosition(blockedPositions);
+		activeLanes.add(p);
+		return p;
+	}
+
+	/**
+	 * @param blockedPositions
+	 *            may be null
+	 * @return a free lane position
+	 */
+	private int getFreePosition(BitSet blockedPositions) {
+		if (freePositions.isEmpty())
+			return positionsAllocated++;
+
+		if (blockedPositions != null) {
+			for (Integer pos : freePositions)
+				if (!blockedPositions.get(pos.intValue())) {
+					freePositions.remove(pos);
+					return pos.intValue();
+				}
+			return positionsAllocated++;
 		} else {
 			final Integer min = freePositions.first();
-			p.position = min.intValue();
 			freePositions.remove(min);
+			return min.intValue();
 		}
-		return p;
 	}
 
 	/**