From f523f21e599f47a09dacb9bd93b2c9f979faeb61 Mon Sep 17 00:00:00 2001
From: Jonathan Nieder <jrn@google.com>
Date: Tue, 3 Nov 2015 18:54:25 -0800
Subject: [PATCH] Deprecate BitmapBuilder.add and introduce simpler addObject
 method

The BitmapIndex.BitmapBuilder.add API is subtle:

	/**
	 * Adds the id and the existing bitmap for the id, if one
	 * exists, to the bitmap.
	 *
	 * @return true if the value was not contained or able to be
	 * loaded.
	 */
	boolean add(AnyObjectId objectId, int type);

Reading the name of the method does not make it obvious what it will
do.  Does it add the named object to the bitmap, or all objects
reachable from it?  It depends on whether the BitmapIndex owns an
existing bitmap for that object.  I did not notice this subtlety when
skimming the javadoc, either.  This resulted in enough confusion to
subtly break the bitmap building code (see change
I30844134bfde0cbabdfaab884c84b9809dd8bdb8 for details).

So discourage use of the add() API by deprecating it.

To replace it, provide a addObject() method that adds a single object.
This way, callers can decide whether to use addObject() or or() based
on the context.

For example,

	if (bitmap.add(c, OBJ_COMMIT)) {
		for (RevCommit p : c.getParents()) {
			rememberToAlsoHandle(p);
		}
	}

can be replaced with

	if (bitmap.contains(c)) {
		// already included
	} else if (index.getBitmap(c) != null) {
		bitmap.or(index.getBitmap(c));
	} else {
		bitmap.addObject(c, OBJ_COMMIT);
		for (RevCommit p : c.getParents()) {
			rememberToAlsoHandle(p);
		}
	}

which is more verbose but makes it clearer that the behavior
depends on the content of index.getBitmaps().

Change-Id: Ib745645f187e1b1483f8587e99501dfcb7b867a5
---
 .../internal/storage/file/BitmapIndexImpl.java     | 14 ++++++++++----
 .../src/org/eclipse/jgit/lib/BitmapIndex.java      | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/BitmapIndexImpl.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/BitmapIndexImpl.java
index 184a9c1dd6..a8c8aaebc1 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/BitmapIndexImpl.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/BitmapIndexImpl.java
@@ -119,10 +119,10 @@ public class BitmapIndexImpl implements BitmapIndex {
 		return position;
 	}
 
-	int addObject(AnyObjectId objectId, int type) {
+	int findOrInsert(AnyObjectId objectId, int type) {
 		int position = findPosition(objectId);
 		if (position < 0) {
-			position = mutableIndex.addObject(objectId, type);
+			position = mutableIndex.findOrInsert(objectId, type);
 			position += indexObjectCount;
 		}
 		return position;
@@ -215,7 +215,7 @@ public class BitmapIndexImpl implements BitmapIndex {
 
 		@Override
 		public boolean add(AnyObjectId objectId, int type) {
-			int position = addObject(objectId, type);
+			int position = findOrInsert(objectId, type);
 			if (bitset.contains(position))
 				return false;
 
@@ -235,6 +235,12 @@ public class BitmapIndexImpl implements BitmapIndex {
 			return 0 <= position && bitset.contains(position);
 		}
 
+		@Override
+		public BitmapBuilder addObject(AnyObjectId objectId, int type) {
+			bitset.set(findOrInsert(objectId, type));
+			return this;
+		}
+
 		@Override
 		public void remove(AnyObjectId objectId) {
 			int position = findPosition(objectId);
@@ -446,7 +452,7 @@ public class BitmapIndexImpl implements BitmapIndex {
 			}
 		}
 
-		int addObject(AnyObjectId objectId, int type) {
+		int findOrInsert(AnyObjectId objectId, int type) {
 			MutableEntry entry = new MutableEntry(
 					objectId, type, revList.size());
 			revList.add(entry);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BitmapIndex.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BitmapIndex.java
index 3c0e2c128e..a4b4b6ef25 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BitmapIndex.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BitmapIndex.java
@@ -125,7 +125,9 @@ public interface BitmapIndex {
 		 * @param type
 		 *            the Git object type. See {@link Constants}.
 		 * @return true if the value was not contained or able to be loaded.
+		 * @deprecated use {@link #or} or {@link #addObject} instead.
 		 */
+		@Deprecated
 		boolean add(AnyObjectId objectId, int type);
 
 		/**
@@ -137,6 +139,18 @@ public interface BitmapIndex {
 		 */
 		boolean contains(AnyObjectId objectId);
 
+		/**
+		 * Adds the id to the bitmap.
+		 *
+		 * @param objectId
+		 *            the object ID
+		 * @param type
+		 *            the Git object type. See {@link Constants}.
+		 * @return the current builder.
+		 * @since 4.2
+		 */
+		BitmapBuilder addObject(AnyObjectId objectId, int type);
+
 		/**
 		 * Remove the id from the bitmap.
 		 *
-- 
2.39.5