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
This commit is contained in:
Jonathan Nieder 2015-11-03 18:54:25 -08:00
parent 424aa22b56
commit f523f21e59
2 changed files with 24 additions and 4 deletions

View File

@ -119,10 +119,10 @@ int findPosition(AnyObjectId objectId) {
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 @@ private final class CompressedBitmapBuilder implements BitmapBuilder {
@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 boolean contains(AnyObjectId objectId) {
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 @@ MutableEntry getObject(int position) {
}
}
int addObject(AnyObjectId objectId, int type) {
int findOrInsert(AnyObjectId objectId, int type) {
MutableEntry entry = new MutableEntry(
objectId, type, revList.size());
revList.add(entry);

View File

@ -125,7 +125,9 @@ public interface BitmapBuilder extends Bitmap {
* @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 BitmapBuilder extends Bitmap {
*/
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.
*