Update bitmap selection throttling to fully span active branches.

Replace the “bitmapCommitRange” parameter that was recently introduced
with two new parameters: “bitmapExcessiveBranchCount” and
“bitmapInactiveBranchAgeInDays”. If the count of branches does not
exceed “bitmapExcessiveBranchCount”, then the current algorithm is kept
for all branches.

If the branch count is excessive, then the commit time for the tip
commit for each branch is used to determine if a branch is “inactive”.
"Active" branches get full commit selection using the existing
algorithm. "Inactive" branches get fewer bitmaps near the branch tips.

Introduce a "contiguousCommitCount" parameter that always enforces that
the N most recent commits in a branch are selected for bitmaps. The
previous nextSelectionDistance() algorithm created anywhere from 1-100
contiguous bitmaps at branch tips.

For example, consider a branch with commits numbering 0-300, with 0
being the most recent commit. If the most recent 200 commits are not
merge commits and the 200th commit was the last one selected,
nextSelectionDistance() returned 100, causing commits 200-101 to be
ignored. Then a window of size 100 was evaluated, searching for merge
commits. Since no merge commits are found, the next commit (commit 0)
was selected, for a total of 1 commit in the topmost 100 commits.

If instead the 250th commit was selected, then by the same logic
commit 50 is selected. At that point nextSelectionDistance() switches to
selecting consecutive commits, so commits 0-50 in the topmost 100
commits are selected. The "contiguousCommitCount" parameter provides
more determinism by always selecting a constant number or topmost
commits.

Add an optimization to break out of the inner loop of selectCommits() if
all of the commits for the current branch have already been found.

When reusing bitmaps from an existing pack, remove unnecessary
populating and clearing of the writeBitmaps/PackBitmapIndexBuilder.

Add comments to PackWriterBitmapPreparer, rename methods and variables
for readability.

Add tests for bitmap selection with and without merge commits and with
excessive branch pruning triggered.

Note: I will follow up with an additional change that exposes the new
parameters through PackConfig.

Change-Id: I5ccbb96c8849f331c302d9f7840e05f9650c4608
Signed-off-by: Terry Parker <tparker@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcBasicPackingTest.java

@@ -44,12 +44,15 @@
 package org.eclipse.jgit.internal.storage.file;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import java.io.File;
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Date;
 import java.util.Iterator;
+import java.util.List;
 
 import org.eclipse.jgit.junit.TestRepository.BranchBuilder;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -132,7 +135,8 @@ public class GcBasicPackingTest extends GcTestCase {
 	}
 
 	@Theory
-	public void testPackCommitsAndLooseOne(boolean aggressive) throws Exception {
+	public void testPackCommitsAndLooseOne(boolean aggressive)
+			throws Exception {
 		BranchBuilder bb = tr.branch("refs/heads/master");
 		RevCommit first = bb.commit().add("A", "A").add("B", "B").create();
 		bb.commit().add("A", "A2").add("B", "B2").create();
@@ -226,71 +230,139 @@ public class GcBasicPackingTest extends GcTestCase {
 	}
 
 	@Test
-	public void testCommitRangeForBitmaps() throws Exception {
+	public void testBitmapSpansNoMerges() throws Exception {
-		BranchBuilder bb1 = tr.branch("refs/heads/master");
+		/*
-		bb1.commit().message("A1").add("A1", "A1").create();
+		 * Commit counts -> expected bitmap counts for history without merges.
-		bb1.commit().message("B1").add("B1", "B1").create();
+		 * The top 100 contiguous commits should always have bitmaps, and the
-		bb1.commit().message("C1").add("C1", "C1").create();
+		 * "recent" bitmaps beyond that are spaced out every 100-200 commits.
-		BranchBuilder bb2 = tr.branch("refs/heads/working");
+		 * (Starting at 100, the next 100 commits are searched for a merge
-		bb2.commit().message("A2").add("A2", "A2").create();
+		 * commit. Since one is not found, the spacing between commits is 200.
-		bb2.commit().message("B2").add("B2", "B2").create();
+		 */
-		bb2.commit().message("C2").add("C2", "C2").create();
+		int[][] bitmapCounts = { //
-
+				{ 1, 1 }, { 50, 50 }, { 99, 99 }, { 100, 100 }, { 101, 100 },
-		// Consider all commits. Since history isn't deep all commits are
+				{ 200, 100 }, { 201, 100 }, { 299, 100 }, { 300, 101 },
-		// selected.
+				{ 301, 101 }, { 401, 101 }, { 499, 101 }, { 500, 102 }, };
-		configureGcRange(gc, -1);
+		int currentCommits = 0;
-		gc.gc();
+		BranchBuilder bb = tr.branch("refs/heads/main");
-		assertEquals(6, gc.getStatistics().numberOfBitmaps);
+
-
+		for (int[] counts : bitmapCounts) {
-		// Range==0 means don't examine commit history, create bitmaps only for
+			int nextCommitCount = counts[0];
-		// branch tips, C1 & C2.
+			int expectedBitmapCount = counts[1];
-		configureGcRange(gc, 0);
+			assertTrue(nextCommitCount > currentCommits); // programming error
-		gc.gc();
+			for (int i = currentCommits; i < nextCommitCount; i++) {
-		assertEquals(2, gc.getStatistics().numberOfBitmaps);
+				String str = "A" + i;
-
+				bb.commit().message(str).add(str, str).create();
-		// Consider only the most recent commit (C2, which is also a branch
+			}
-		// tip).
+			currentCommits = nextCommitCount;
-		configureGcRange(gc, 1);
-		gc.gc();
-		assertEquals(2, gc.getStatistics().numberOfBitmaps);
 
-		// Consider only the two most recent commits, C2 & B2. C1 gets included
+			gc.setExpireAgeMillis(0); // immediately delete old packs
-		// too since it is a branch tip.
-		configureGcRange(gc, 2);
 			gc.gc();
-		assertEquals(3, gc.getStatistics().numberOfBitmaps);
+			assertEquals(currentCommits * 3, // commit/tree/object
+					gc.getStatistics().numberOfPackedObjects);
+			assertEquals(currentCommits + " commits: ", expectedBitmapCount,
+					gc.getStatistics().numberOfBitmaps);
+		}
+	}
 
-		// Consider C2 & B2 & A2. C1 gets included too since it is a branch tip.
+	@Test
-		configureGcRange(gc, 3);
+	public void testBitmapSpansWithMerges() throws Exception {
-		gc.gc();
+		/*
-		assertEquals(4, gc.getStatistics().numberOfBitmaps);
+		 * Commits that are merged. Since 55 is in the oldest history it is
+		 * never considered. Searching goes from oldest to newest so 115 is the
+		 * first merge commit found. After that the range 116-216 is ignored so
+		 * 175 is never considered.
+		 */
+		List<Integer> merges = Arrays.asList(Integer.valueOf(55),
+				Integer.valueOf(115), Integer.valueOf(175),
+				Integer.valueOf(235));
+		/*
+		 * Commit counts -> expected bitmap counts for history with merges. The
+		 * top 100 contiguous commits should always have bitmaps, and the
+		 * "recent" bitmaps beyond that are spaced out every 100-200 commits.
+		 * Merges in the < 100 range have no effect and merges in the > 100
+		 * range will only be considered for commit counts > 200.
+		 */
+		int[][] bitmapCounts = { //
+				{ 1, 1 }, { 55, 55 }, { 56, 57 }, // +1 bitmap from branch A55
+				{ 99, 100 }, // still +1 branch @55
+				{ 100, 100 }, // 101 commits, only 100 newest
+				{ 116, 100 }, // @55 still in 100 newest bitmaps
+				{ 176, 101 }, // @55 branch tip is not in 100 newest
+				{ 213, 101 }, // 216 commits, @115&@175 in 100 newest
+				{ 214, 102 }, // @55 branch tip, merge @115, @177 in newest
+				{ 236, 102 }, // all 4 merge points in history
+				{ 273, 102 }, // 277 commits, @175&@235 in newest
+				{ 274, 103 }, // @55, @115, merge @175, @235 in newest
+				{ 334, 103 }, // @55,@115,@175, @235 in newest
+				{ 335, 104 }, // @55,@115,@175, merge @235
+				{ 435, 104 }, // @55,@115,@175,@235 tips
+				{ 436, 104 }, // force @236
+		};
+
+		int currentCommits = 0;
+		BranchBuilder bb = tr.branch("refs/heads/main");
+
+		for (int[] counts : bitmapCounts) {
+			int nextCommitCount = counts[0];
+			int expectedBitmapCount = counts[1];
+			assertTrue(nextCommitCount > currentCommits); // programming error
+			for (int i = currentCommits; i < nextCommitCount; i++) {
+				String str = "A" + i;
+				if (!merges.contains(Integer.valueOf(i))) {
+					bb.commit().message(str).add(str, str).create();
+				} else {
+					BranchBuilder bbN = tr.branch("refs/heads/A" + i);
+					bb.commit().message(str).add(str, str)
+							.parent(bbN.commit().create()).create();
+				}
+			}
+			currentCommits = nextCommitCount;
 
-		// Consider C2 & B2 & A2 & C1.
+			gc.setExpireAgeMillis(0); // immediately delete old packs
-		configureGcRange(gc, 4);
 			gc.gc();
-		assertEquals(4, gc.getStatistics().numberOfBitmaps);
+			assertEquals(currentCommits + " commits: ", expectedBitmapCount,
+					gc.getStatistics().numberOfBitmaps);
+		}
+	}
 
-		// Consider C2 & B2 & A2 & C1 & B1.
+	@Test
-		configureGcRange(gc, 5);
+	public void testBitmapsForExcessiveBranches() throws Exception {
-		gc.gc();
+		int oneDayInSeconds = 60 * 60 * 24;
-		assertEquals(5, gc.getStatistics().numberOfBitmaps);
+
+		// All of branch A is committed on day1
+		BranchBuilder bbA = tr.branch("refs/heads/A");
+		for (int i = 0; i < 1001; i++) {
+			String msg = "A" + i;
+			bbA.commit().message(msg).add(msg, msg).create();
+		}
+		// All of in branch B is committed on day91
+		tr.tick(oneDayInSeconds * 90);
+		BranchBuilder bbB = tr.branch("refs/heads/B");
+		for (int i = 0; i < 1001; i++) {
+			String msg = "B" + i;
+			bbB.commit().message(msg).add(msg, msg).create();
+		}
+		// Create 100 other branches with a single commit
+		for (int i = 0; i < 100; i++) {
+			BranchBuilder bb = tr.branch("refs/heads/N" + i);
+			String msg = "singlecommit" + i;
+			bb.commit().message(msg).add(msg, msg).create();
+		}
+		// now is day92
+		tr.tick(oneDayInSeconds);
 
-		// Consider all six commits.
+		// Since there are no merges, commits in recent history are selected
-		configureGcRange(gc, 6);
+		// every 200 commits.
-		gc.gc();
+		final int commitsForSparseBranch = 1 + (1001 / 200);
-		assertEquals(6, gc.getStatistics().numberOfBitmaps);
+		final int commitsForFullBranch = 100 + (901 / 200);
+		final int commitsForShallowBranches = 100;
 
-		// Input is out of range but should be capped to the total number of
+		// Excessive branch history pruning, one old branch.
-		// commits.
-		configureGcRange(gc, 1000);
 		gc.gc();
-		assertEquals(6, gc.getStatistics().numberOfBitmaps);
+		assertEquals(
-	}
+				commitsForSparseBranch + commitsForFullBranch
-
+						+ commitsForShallowBranches,
-	private void configureGcRange(GC myGc, int range) {
+				gc.getStatistics().numberOfBitmaps);
-		PackConfig pconfig = new PackConfig(repo);
-		pconfig.setBitmapCommitRange(range);
-		myGc.setPackConfig(pconfig);
 	}
 
 	private void configureGc(GC myGc, boolean aggressive) {

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcTestCase.java

@@ -52,6 +52,7 @@ import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.junit.TestRepository.CommitBuilder;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
 import org.junit.After;
 import org.junit.Before;
 
@@ -65,7 +66,8 @@ public abstract class GcTestCase extends LocalDiskRepositoryTestCase {
 	public void setUp() throws Exception {
 		super.setUp();
 		repo = createWorkRepository();
-		tr = new TestRepository<FileRepository>((repo));
+		tr = new TestRepository<FileRepository>(repo, new RevWalk(repo),
+				mockSystemReader);
 		gc = new GC(repo);
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java

@@ -2017,11 +2017,8 @@ public class PackWriter implements AutoCloseable {
 		PackWriterBitmapPreparer bitmapPreparer = new PackWriterBitmapPreparer(
 				reader, writeBitmaps, pm, stats.interestingObjects);
 
-		int commitRange = config.getBitmapCommitRange();
-		if (commitRange < 0)
-			commitRange = numCommits; // select from all commits
 		Collection<PackWriterBitmapPreparer.BitmapCommit> selectedCommits =
-				bitmapPreparer.doCommitSelection(commitRange);
+				bitmapPreparer.selectCommits(numCommits);
 
 		beginPhase(PackingPhase.BUILDING_BITMAPS, pm, selectedCommits.size());
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriterBitmapPreparer.java

@@ -55,8 +55,6 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
 
-import com.googlecode.javaewah.EWAHCompressedBitmap;
-
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.internal.JGitText;
@@ -75,14 +73,18 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.BlockList;
+import org.eclipse.jgit.util.SystemReader;
 
-/** Helper class for the PackWriter to select commits for pack index bitmaps. */
+/**
+ * Helper class for the {@link PackWriter} to select commits for which to build
+ * pack index bitmaps.
+ */
 class PackWriterBitmapPreparer {
 
-	private static final Comparator<BitmapBuilder> BUILDER_BY_CARDINALITY_DSC =
+	private static final Comparator<BitmapBuilderEntry> ORDER_BY_DESCENDING_CARDINALITY = new Comparator<BitmapBuilderEntry>() {
-			new Comparator<BitmapBuilder>() {
+		public int compare(BitmapBuilderEntry a, BitmapBuilderEntry b) {
-		public int compare(BitmapBuilder a, BitmapBuilder b) {
+			return Integer.signum(b.getBuilder().cardinality()
-			return Integer.signum(b.cardinality() - a.cardinality());
+					- a.getBuilder().cardinality());
 		}
 	};
 
@@ -93,8 +95,13 @@ class PackWriterBitmapPreparer {
 	private final BitmapIndexImpl commitBitmapIndex;
 	private final PackBitmapIndexRemapper bitmapRemapper;
 	private final BitmapIndexImpl bitmapIndex;
-	private final int minCommits = 100;
+
-	private final int maxCommits = 5000;
+	private final int contiguousCommitCount;
+	private final int recentCommitCount;
+	private final int recentCommitSpan;
+	private final int distantCommitSpan;
+	private final int excessiveBranchCount;
+	private final long inactiveBranchTimestamp;
 
 	PackWriterBitmapPreparer(ObjectReader reader,
 			PackBitmapIndexBuilder writeBitmaps, ProgressMonitor pm,
@@ -107,69 +114,138 @@ class PackWriterBitmapPreparer {
 		this.bitmapRemapper = PackBitmapIndexRemapper.newPackBitmapIndex(
 				reader.getBitmapIndex(), writeBitmaps);
 		this.bitmapIndex = new BitmapIndexImpl(bitmapRemapper);
-	}
+		this.contiguousCommitCount = 100;
-
+		this.recentCommitCount = 20000;
-	Collection<BitmapCommit> doCommitSelection(int commitRange)
+		this.recentCommitSpan = 100;
-			throws MissingObjectException, IncorrectObjectTypeException,
+		this.distantCommitSpan = 5000;
-			IOException {
+		this.excessiveBranchCount = 100;
+		long now = SystemReader.getInstance().getCurrentTime();
+		long ageInSeconds = 90 * 24 * 60 * 60;
+		this.inactiveBranchTimestamp = (now / 1000) - ageInSeconds;
+	}
+
+	/**
+	 * Returns the commit objects for which bitmap indices should be built.
+	 *
+	 * @param expectedCommitCount
+	 *            count of commits in the pack
+	 * @return commit objects for which bitmap indices should be built
+	 * @throws IncorrectObjectTypeException
+	 *             if any of the processed objects is not a commit
+	 * @throws IOException
+	 *             on errors reading pack or index files
+	 * @throws MissingObjectException
+	 *             if an expected object is missing
+	 */
+	Collection<BitmapCommit> selectCommits(int expectedCommitCount)
+			throws IncorrectObjectTypeException, IOException,
+			MissingObjectException {
+		/*
+		 * Thinking of bitmap indices as a cache, if we find bitmaps at or at a
+		 * close ancestor to 'old' and 'new' when calculating old..new, then all
+		 * objects can be calculated with minimal graph walking. A distribution
+		 * that favors creating bitmaps for the most recent commits maximizes
+		 * the cache hits for clients that are close to HEAD, which is the
+		 * majority of calculations performed.
+		 */
 		pm.beginTask(JGitText.get().selectingCommits, ProgressMonitor.UNKNOWN);
 		RevWalk rw = new RevWalk(reader);
 		rw.setRetainBody(false);
-		WalkResult result = findPaths(rw, commitRange);
+		CommitSelectionHelper selectionHelper = setupTipCommitBitmaps(rw,
+				expectedCommitCount);
 		pm.endTask();
 
-		int totCommits = result.commitsByOldest.length - result.commitStartPos;
+		int totCommits = selectionHelper.getCommitCount();
 		BlockList<BitmapCommit> selections = new BlockList<BitmapCommit>(
-				totCommits / minCommits + 1);
+				totCommits / recentCommitSpan + 1);
-		for (BitmapCommit reuse : result.reuse)
+		for (BitmapCommit reuse : selectionHelper.reusedCommits) {
 			selections.add(reuse);
+		}
 
 		if (totCommits == 0) {
-			for (AnyObjectId id : result.peeledWant)
+			for (AnyObjectId id : selectionHelper.peeledWants) {
 				selections.add(new BitmapCommit(id, false, 0));
+			}
 			return selections;
 		}
 
 		pm.beginTask(JGitText.get().selectingCommits, totCommits);
+		int totalWants = selectionHelper.peeledWants.size();
 
-		for (BitmapBuilder bitmapableCommits : result.paths) {
+		for (BitmapBuilderEntry entry : selectionHelper.tipCommitBitmaps) {
-			int cardinality = bitmapableCommits.cardinality();
+			BitmapBuilder bitmap = entry.getBuilder();
+			int cardinality = bitmap.cardinality();
 
 			List<List<BitmapCommit>> running = new ArrayList<
 					List<BitmapCommit>>();
 
+			// Mark the current branch as inactive if its tip commit isn't
+			// recent and there are an excessive number of branches, to
+			// prevent memory bloat of computing too many bitmaps for stale
+			// branches.
+			boolean isActiveBranch = true;
+			if (totalWants > excessiveBranchCount
+					&& !isRecentCommit(entry.getCommit())) {
+				isActiveBranch = false;
+			}
+
 			// Insert bitmaps at the offsets suggested by the
 			// nextSelectionDistance() heuristic.
 			int index = -1;
-			int nextIn = nextSelectionDistance(0, cardinality);
+			int nextIn = nextSpan(cardinality);
-			int nextFlg = nextIn == maxCommits ? PackBitmapIndex.FLAG_REUSE : 0;
+			int nextFlg = nextIn == distantCommitSpan
-			boolean mustPick = nextIn == 0;
+					? PackBitmapIndex.FLAG_REUSE : 0;
-			for (RevCommit c : result) {
+
-				if (!bitmapableCommits.contains(c))
+			// For the current branch, iterate through all commits from oldest
+			// to newest.
+			for (RevCommit c : selectionHelper) {
+				// Optimization: if we have found all the commits for this
+				// branch, stop searching
+				int distanceFromTip = cardinality - index - 1;
+				if (distanceFromTip == 0) {
+					break;
+				}
+
+				// Ignore commits that are not in this branch
+				if (!bitmap.contains(c)) {
 					continue;
+				}
 
 				index++;
 				nextIn--;
 				pm.update(1);
 
-				// Always pick the items in want and prefer merge commits.
+				// Always pick the items in wants, prefer merge commits.
-				if (result.peeledWant.remove(c)) {
+				if (selectionHelper.peeledWants.remove(c)) {
-					if (nextIn > 0)
+					if (nextIn > 0) {
 						nextFlg = 0;
-				} else if (!mustPick && ((nextIn > 0)
+					}
-						|| (c.getParentCount() <= 1 && nextIn > -minCommits))) {
+				} else {
+					boolean stillInSpan = nextIn >= 0;
+					boolean isMergeCommit = c.getParentCount() > 1;
+					// Force selection if:
+					// a) we have exhausted the window looking for merges
+					// b) we are in the top commits of an active branch
+					// c) we are at a branch tip
+					boolean mustPick = (nextIn <= -recentCommitSpan)
+							|| (isActiveBranch
+									&& (distanceFromTip <= contiguousCommitCount))
+							|| (distanceFromTip == 1); // most recent commit
+					if (!mustPick && (stillInSpan || !isMergeCommit)) {
 						continue;
 					}
+				}
 
+				// This commit is selected, calculate the next one.
 				int flags = nextFlg;
-				nextIn = nextSelectionDistance(index, cardinality);
+				nextIn = nextSpan(distanceFromTip);
-				nextFlg = nextIn == maxCommits ? PackBitmapIndex.FLAG_REUSE : 0;
+				nextFlg = nextIn == distantCommitSpan
-				mustPick = nextIn == 0;
+						? PackBitmapIndex.FLAG_REUSE : 0;
 
 				BitmapBuilder fullBitmap = commitBitmapIndex.newBitmapBuilder();
 				rw.reset();
 				rw.markStart(c);
-				for (AnyObjectId objectId : result.reuse)
+				for (AnyObjectId objectId : selectionHelper.reusedCommits)
 					rw.markUninteresting(rw.parseCommit(objectId));
 				rw.setRevFilter(
 						PackWriterBitmapWalker.newRevFilter(null, fullBitmap));
@@ -182,9 +258,10 @@ class PackWriterBitmapPreparer {
 						List<BitmapCommit>>();
 				for (List<BitmapCommit> list : running) {
 					BitmapCommit last = list.get(list.size() - 1);
-					if (fullBitmap.contains(last))
+					if (fullBitmap.contains(last)) {
 						matches.add(list);
 					}
+				}
 
 				List<BitmapCommit> match;
 				if (matches.isEmpty()) {
@@ -194,121 +271,176 @@ class PackWriterBitmapPreparer {
 					match = matches.get(0);
 					// Append to longest
 					for (List<BitmapCommit> list : matches) {
-						if (list.size() > match.size())
+						if (list.size() > match.size()) {
 							match = list;
 						}
 					}
+				}
 				match.add(new BitmapCommit(c, !match.isEmpty(), flags));
 				writeBitmaps.addBitmap(c, fullBitmap, 0);
 			}
 
-			for (List<BitmapCommit> list : running)
+			for (List<BitmapCommit> list : running) {
 				selections.addAll(list);
 			}
+		}
 		writeBitmaps.clearBitmaps(); // Remove the temporary commit bitmaps.
 
 		// Add the remaining peeledWant
-		for (AnyObjectId remainingWant : result.peeledWant)
+		for (AnyObjectId remainingWant : selectionHelper.peeledWants) {
 			selections.add(new BitmapCommit(remainingWant, false, 0));
+		}
 
 		pm.endTask();
 		return selections;
 	}
 
-	private WalkResult findPaths(RevWalk rw, int commitRange)
+	private boolean isRecentCommit(RevCommit revCommit) {
-			throws MissingObjectException, IOException {
+		return revCommit.getCommitTime() > inactiveBranchTimestamp;
-		BitmapBuilder reuseBitmap = commitBitmapIndex.newBitmapBuilder();
+	}
-		List<BitmapCommit> reuse = new ArrayList<BitmapCommit>();
+
+	/**
+	 * For each of the {@code want}s, which represent the tip commit of each
+	 * branch, set up an initial {@link BitmapBuilder}. Reuse previously built
+	 * bitmaps if possible.
+	 *
+	 * @param rw
+	 *            a {@link RevWalk} to find reachable objects in this repository
+	 * @param expectedCommitCount
+	 *            expected count of commits. The actual count may be less due to
+	 *            unreachable garbage.
+	 * @return a {@link CommitSelectionHelper} containing bitmaps for the tip
+	 *         commits
+	 * @throws IncorrectObjectTypeException
+	 *             if any of the processed objects is not a commit
+	 * @throws IOException
+	 *             on errors reading pack or index files
+	 * @throws MissingObjectException
+	 *             if an expected object is missing
+	 */
+	private CommitSelectionHelper setupTipCommitBitmaps(RevWalk rw,
+			int expectedCommitCount) throws IncorrectObjectTypeException,
+					IOException, MissingObjectException {
+		BitmapBuilder reuse = commitBitmapIndex.newBitmapBuilder();
+		List<BitmapCommit> reuseCommits = new ArrayList<BitmapCommit>();
 		for (PackBitmapIndexRemapper.Entry entry : bitmapRemapper) {
-			if ((entry.getFlags() & FLAG_REUSE) != FLAG_REUSE)
+			if ((entry.getFlags() & FLAG_REUSE) != FLAG_REUSE) {
 				continue;
+			}
 
 			RevObject ro = rw.peel(rw.parseAny(entry));
 			if (ro instanceof RevCommit) {
 				RevCommit rc = (RevCommit) ro;
-				reuse.add(new BitmapCommit(rc, false, entry.getFlags()));
+				reuseCommits.add(new BitmapCommit(rc, false, entry.getFlags()));
 				rw.markUninteresting(rc);
-
+				// PackBitmapIndexRemapper.ofObjectType() ties the underlying
-				EWAHCompressedBitmap bitmap = bitmapRemapper.ofObjectType(
+				// bitmap in the old pack into the new bitmap builder.
-						bitmapRemapper.getBitmap(rc), Constants.OBJ_COMMIT);
+				bitmapRemapper.ofObjectType(bitmapRemapper.getBitmap(rc),
-				writeBitmaps.addBitmap(rc, bitmap, 0);
+						Constants.OBJ_COMMIT).trim();
-				reuseBitmap.add(rc, Constants.OBJ_COMMIT);
+				reuse.add(rc, Constants.OBJ_COMMIT);
 			}
 		}
-		writeBitmaps.clearBitmaps(); // Remove temporary bitmaps
 
 		// Do a RevWalk by commit time descending. Keep track of all the paths
 		// from the wants.
-		List<BitmapBuilder> paths = new ArrayList<BitmapBuilder>(want.size());
+		List<BitmapBuilderEntry> tipCommitBitmaps = new ArrayList<BitmapBuilderEntry>(
+				want.size());
 		Set<RevCommit> peeledWant = new HashSet<RevCommit>(want.size());
 		for (AnyObjectId objectId : want) {
 			RevObject ro = rw.peel(rw.parseAny(objectId));
-			if (ro instanceof RevCommit && !reuseBitmap.contains(ro)) {
+			if (ro instanceof RevCommit && !reuse.contains(ro)) {
 				RevCommit rc = (RevCommit) ro;
 				peeledWant.add(rc);
 				rw.markStart(rc);
 
 				BitmapBuilder bitmap = commitBitmapIndex.newBitmapBuilder();
-				bitmap.or(reuseBitmap);
+				bitmap.or(reuse);
 				bitmap.add(rc, Constants.OBJ_COMMIT);
-				paths.add(bitmap);
+				tipCommitBitmaps.add(new BitmapBuilderEntry(rc, bitmap));
 			}
 		}
 
-		// Update the paths from the wants and create a list of commits in
+		// Create a list of commits in reverse order (older to newer).
-		// reverse iteration order for the desired commit range.
+		RevCommit[] commits = new RevCommit[expectedCommitCount];
-		RevCommit[] commits = new RevCommit[commitRange];
 		int pos = commits.length;
 		RevCommit rc;
 		while ((rc = rw.next()) != null && pos > 0) {
 			commits[--pos] = rc;
-			for (BitmapBuilder path : paths) {
+			for (BitmapBuilderEntry entry : tipCommitBitmaps) {
-				if (path.contains(rc)) {
+				BitmapBuilder bitmap = entry.getBuilder();
-					for (RevCommit c : rc.getParents())
+				if (bitmap.contains(rc)) {
-						path.add(c, Constants.OBJ_COMMIT);
+					for (RevCommit c : rc.getParents()) {
+						bitmap.add(c, Constants.OBJ_COMMIT);
+					}
 				}
 			}
-
 			pm.update(1);
 		}
 
-		// Remove the reused bitmaps from the paths
+		// Remove the reused bitmaps from the tip commit bitmaps
-		if (!reuse.isEmpty())
+		if (!reuseCommits.isEmpty()) {
-			for (BitmapBuilder bitmap : paths)
+			for (BitmapBuilderEntry entry : tipCommitBitmaps) {
-				bitmap.andNot(reuseBitmap);
+				entry.getBuilder().andNot(reuse);
+			}
+		}
 
-		// Sort the paths
+		// Sort the tip commit bitmaps. Find the one containing the most
-		List<BitmapBuilder> distinctPaths = new ArrayList<BitmapBuilder>(paths.size());
+		// commits, remove those commits from the remaining bitmaps, resort and
-		while (!paths.isEmpty()) {
+		// repeat.
-			Collections.sort(paths, BUILDER_BY_CARDINALITY_DSC);
+		List<BitmapBuilderEntry> orderedTipCommitBitmaps = new ArrayList<>(
-			BitmapBuilder largest = paths.remove(0);
+				tipCommitBitmaps.size());
-			distinctPaths.add(largest);
+		while (!tipCommitBitmaps.isEmpty()) {
+			Collections.sort(tipCommitBitmaps, ORDER_BY_DESCENDING_CARDINALITY);
+			BitmapBuilderEntry largest = tipCommitBitmaps.remove(0);
+			orderedTipCommitBitmaps.add(largest);
 
 			// Update the remaining paths, by removing the objects from
 			// the path that was just added.
-			for (int i = paths.size() - 1; i >= 0; i--)
+			for (int i = tipCommitBitmaps.size() - 1; i >= 0; i--) {
-				paths.get(i).andNot(largest);
+				tipCommitBitmaps.get(i).getBuilder()
+						.andNot(largest.getBuilder());
+			}
 		}
 
-		return new WalkResult(peeledWant, commits, pos, distinctPaths, reuse);
+		return new CommitSelectionHelper(peeledWant, commits, pos,
+				orderedTipCommitBitmaps, reuseCommits);
 	}
 
-	private int nextSelectionDistance(int idx, int cardinality) {
+	/*-
-		if (idx > cardinality)
+	 * Returns the desired distance to the next bitmap based on the distance
+	 * from the tip commit. Only differentiates recent from distant spans,
+	 * selectCommits() handles the contiguous commits at the tip for active
+	 * or inactive branches.
+	 *
+	 * A graph of this function looks like this, where
+	 * the X axis is the distance from the tip commit and the Y axis is the
+	 * bitmap selection distance.
+	 *
+	 * 5000                ____...
+	 *                    /
+	 *                  /
+	 *                /
+	 *              /
+	 *  100  _____/
+	 *       0  20100  25000
+	 *
+	 * Linear scaling between 20100 and 25000 prevents spans >100 for distances
+	 * <20000 (otherwise, a span of 5000 would be returned for a distance of
+	 * 21000, and the range 16000-20000 would have no selections).
+	 */
+	int nextSpan(int distanceFromTip) {
+		if (distanceFromTip < 0) {
 			throw new IllegalArgumentException();
-		int idxFromStart = cardinality - idx;
+		}
-		int mustRegionEnd = 100;
-		if (idxFromStart <= mustRegionEnd)
-			return 0;
 
 		// Commits more toward the start will have more bitmaps.
-		int minRegionEnd = 20000;
+		if (distanceFromTip <= recentCommitCount) {
-		if (idxFromStart <= minRegionEnd)
+			return recentCommitSpan;
-			return Math.min(idxFromStart - mustRegionEnd, minCommits);
+		}
 
-		// Commits more toward the end will have fewer.
+		int next = Math.min(distanceFromTip - recentCommitCount,
-		int next = Math.min(idxFromStart - minRegionEnd, maxCommits);
+				distantCommitSpan);
-		return Math.max(next, minCommits);
+		return Math.max(next, recentCommitSpan);
 	}
 
 	PackWriterBitmapWalker newBitmapWalker() {
@@ -316,12 +448,14 @@ class PackWriterBitmapPreparer {
 				new ObjectWalk(reader), bitmapIndex, null);
 	}
 
+	/**
+	 * A commit object for which a bitmap index should be built.
+	 */
 	static final class BitmapCommit extends ObjectId {
 		private final boolean reuseWalker;
 		private final int flags;
 
-		private BitmapCommit(
+		BitmapCommit(AnyObjectId objectId, boolean reuseWalker, int flags) {
-				AnyObjectId objectId, boolean reuseWalker, int flags) {
 			super(objectId);
 			this.reuseWalker = reuseWalker;
 			this.flags = flags;
@@ -336,21 +470,55 @@ class PackWriterBitmapPreparer {
 		}
 	}
 
-	private static final class WalkResult implements Iterable<RevCommit> {
+	/**
-		private final Set<? extends ObjectId> peeledWant;
+	 * A POJO representing a Pair<RevCommit, BitmapBuidler>.
+	 */
+	private static final class BitmapBuilderEntry {
+		private final RevCommit commit;
+
+		private final BitmapBuilder builder;
+
+		BitmapBuilderEntry(RevCommit commit, BitmapBuilder builder) {
+			this.commit = commit;
+			this.builder = builder;
+		}
+
+		RevCommit getCommit() {
+			return commit;
+		}
+
+		BitmapBuilder getBuilder() {
+			return builder;
+		}
+	}
+
+	/**
+	 * Container for state used in the first phase of selecting commits, which
+	 * walks all of the reachable commits via the branch tips (
+	 * {@code peeledWants}), stores them in {@code commitsByOldest}, and sets up
+	 * bitmaps for each branch tip ({@code tipCommitBitmaps}).
+	 * {@code commitsByOldest} is initialized with an expected size of all
+	 * commits, but may be smaller if some commits are unreachable, in which
+	 * case {@code commitStartPos} will contain a positive offset to the root
+	 * commit.
+	 */
+	private static final class CommitSelectionHelper implements Iterable<RevCommit> {
+		final Set<? extends ObjectId> peeledWants;
+
+		final List<BitmapBuilderEntry> tipCommitBitmaps;
+		final Iterable<BitmapCommit> reusedCommits;
 		private final RevCommit[] commitsByOldest;
 		private final int commitStartPos;
-		private final List<BitmapBuilder> paths;
-		private final Iterable<BitmapCommit> reuse;
 
-		private WalkResult(Set<? extends ObjectId> peeledWant,
+		CommitSelectionHelper(Set<? extends ObjectId> peeledWant,
 				RevCommit[] commitsByOldest, int commitStartPos,
-				List<BitmapBuilder> paths, Iterable<BitmapCommit> reuse) {
+				List<BitmapBuilderEntry> bitmapEntries,
-			this.peeledWant = peeledWant;
+				Iterable<BitmapCommit> reuse) {
+			this.peeledWants = peeledWant;
 			this.commitsByOldest = commitsByOldest;
 			this.commitStartPos = commitStartPos;
-			this.paths = paths;
+			this.tipCommitBitmaps = bitmapEntries;
-			this.reuse = reuse;
+			this.reusedCommits = reuse;
 		}
 
 		public Iterator<RevCommit> iterator() {
@@ -370,5 +538,9 @@ class PackWriterBitmapPreparer {
 				}
 			};
 		}
+
+		int getCommitCount() {
+			return commitsByOldest.length - commitStartPos;
+		}
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackConfig.java

@@ -138,14 +138,6 @@ public class PackConfig {
 	 */
 	public static final boolean DEFAULT_BUILD_BITMAPS = true;
 
-	/**
-	 * Default range of commits for which to create bitmaps: {@value}
-	 *
-	 * @see #setBitmapCommitRange(int)
-	 * @since 4.2
-	 */
-	public static final int DEFAULT_BITMAP_COMMIT_RANGE = -1;
-
 
 	private int compressionLevel = Deflater.DEFAULT_COMPRESSION;
 
@@ -177,8 +169,6 @@ public class PackConfig {
 
 	private boolean buildBitmaps = DEFAULT_BUILD_BITMAPS;
 
-	private int bitmapCommitRange = DEFAULT_BITMAP_COMMIT_RANGE;
-
 	private boolean cutDeltaChains;
 
 	/** Create a default configuration. */
@@ -232,7 +222,6 @@ public class PackConfig {
 		this.executor = cfg.executor;
 		this.indexVersion = cfg.indexVersion;
 		this.buildBitmaps = cfg.buildBitmaps;
-		this.bitmapCommitRange = cfg.bitmapCommitRange;
 		this.cutDeltaChains = cfg.cutDeltaChains;
 	}
 
@@ -694,39 +683,6 @@ public class PackConfig {
 		this.buildBitmaps = buildBitmaps;
 	}
 
-	/**
-	 * Get the range of commits for which to build bitmaps. The range starts
-	 * from the most recent commit.
-	 *
-	 * A value of 0 creates bitmaps for only branch tips. A value of -1 creates
-	 * bitmaps spaced through the entire history of commits.
-	 *
-	 * Default setting: {@value #DEFAULT_BITMAP_COMMIT_RANGE}
-	 *
-	 * @return the range of commits for which to create bitmaps, starting with
-	 *         the most recent commit
-	 * @see PackIndexWriter
-	 * @since 4.2
-	 */
-	public int getBitmapCommitRange() {
-		return bitmapCommitRange;
-	}
-
-	/**
-	 * Set the range of commits for which to build bitmaps.
-	 *
-	 * Default setting: {@value #DEFAULT_BITMAP_COMMIT_RANGE}
-	 *
-	 * @param range
-	 *            the range of commits for which to create bitmaps, starting
-	 *            with the most recent commit
-	 * @see PackIndexWriter
-	 * @since 4.2
-	 */
-	public void setBitmapCommitRange(final int range) {
-		bitmapCommitRange = range;
-	}
-
 	/**
 	 * Update properties by setting fields from the configuration.
 	 *
@@ -762,8 +718,6 @@ public class PackConfig {
 		setCutDeltaChains(rc.getBoolean(
 				"pack", "cutdeltachains", getCutDeltaChains())); //$NON-NLS-1$ //$NON-NLS-2$
 		setBuildBitmaps(rc.getBoolean("pack", "buildbitmaps", isBuildBitmaps())); //$NON-NLS-1$ //$NON-NLS-2$
-		setBitmapCommitRange(
-				rc.getInt("pack", "bitmapcommitrange", getBitmapCommitRange())); //$NON-NLS-1$ //$NON-NLS-2$
 	}
 
 	public String toString() {
@@ -781,7 +735,6 @@ public class PackConfig {
 		b.append(", reuseObjects=").append(isReuseObjects()); //$NON-NLS-1$
 		b.append(", deltaCompress=").append(isDeltaCompress()); //$NON-NLS-1$
 		b.append(", buildBitmaps=").append(isBuildBitmaps()); //$NON-NLS-1$
-		b.append(", bitmapCommitRange=").append(getBitmapCommitRange()); //$NON-NLS-1$
 		return b.toString();
 	}
 }