Re: [EGIT PATCH 4/7] Handle peeling of loose refs.

["Shawn O. Pearce" <spearce@spearce.org>]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> @@ -238,10 +235,19 @@ public ObjectId getObjectId() {
>  	 *         refer to an annotated tag.
>  	 */
>  	public ObjectId getPeeledObjectId() {
> +		if (peeledObjectId == ObjectId.zeroId())
> +			return null;
>  		return peeledObjectId;
>  	}
>  
>  	/**
> +	 * @return whether the Ref represents a peeled tag
> +	 */
> +	public boolean isPeeled() {
> +		return peeledObjectId != null && peeledObjectId != ObjectId.zeroId();
> +	}

Huh.  So peeledObjectId == ObjectId.zeroId() means what, exactly?
That we tried to peel this but we cannot because its not a tag?

What does a packed-refs record which has no peel data but which
is in a packed-refs with-peeled file have for peeledObjectId?
null or ObjectId.zeroId()?

> @@ -288,6 +289,28 @@ private void readOneLooseRef(final Map<String, Ref> avail,
>  		}
>  	}
>  
> +	Ref peel(final Ref ref) {
> +		if (ref.isPeeled())
> +			return ref;

I think you mean something more like:

	if (ref.peeledObjectId != null)
		return ref;

as the ref is already peeled, or at least attempted.

> +		try {
> +			Object tt = db.mapObject(ref.getObjectId(), ref.getName());
> +			if (tt != null && tt instanceof Tag) {
> +				Tag t = (Tag)tt;
> +				while (t != null && t.getType().equals(Constants.TYPE_TAG))
> +					t = db.mapTag(t.getTag(), t.getObjId());
> +				if (t != null)
> +					ref.setPeeledObjectId(t.getObjId());
> +				else
> +					ref.setPeeledObjectId(null);
> +			} else
> +				ref.setPeeledObjectId(ref.getObjectId());
> +		} catch (IOException e) {
> +			// Serious error. Caller knows a ref should never be null
> +			ref.setPeeledObjectId(null);
> +		}
> +		return ref;

This whole block is simpler to write as:

	try {
		Object target = db.mapObject(ref.getObjectId(), ref.getName());
		while (target instanceof Tag) {
			final Tag tag = (Tag)target;
			ref.setPeeledObjectId(tag.getObjId());
			if (Constants.TYPE_TAG.equals(tag.getType()))
				target = db.mapObject(tag.getObjId(), ref.getName());
			else
				break;
		}
	} catch (IOException e) {
		// Ignore a read error.  Callers will also get the same error
		// if they try to use the result of getPeeledObjectId.
	}

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> index 26748e2..4d6e6fd 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> @@ -939,6 +939,19 @@ public String getBranch() throws IOException {
>  	}
>  
>  	/**
> +	 * Peel a possibly unpeeled ref and updates it. If the ref cannot be peeled
> +	 * the peeled id is set to {@link ObjectId#zeroId()}

But applications never see ObjectId.zeroId() because you earlier defined
getPeeledObjectId() to return null in that case.  So why is this part of
the documentation relevant? 

> +	 * 
> +	 * @param ref
> +	 *            The ref to peel
> +	 * @return The same, an updated ref with peeled info or a new instance with
> +	 *         more information
> +	 */
> +	public Ref peel(final Ref ref) {
> +		return refs.peel(ref);

Can we tighten this contract?  Are we always going to update the
current ref in-place, or are we going to return the caller a new
Ref instance?  I'd like it to be consistent one way or the other.

If we are updating in-place then the caller needs to realize there
may be some cache synchronization issues when using multiple threads
to access the same Ref instances and peeling them.  I.e. they may
need to go through their own synchornization barrier to ensure the
processor caches are coherent.

Given that almost everywhere else we treat the Ref as immutable
I'm tempted to say this should always return a new Ref object if we
peeled; but return the parameter if the parameter is already peeled
(or is known to be unpeelable, e.g. a branch head).

-- 
Shawn.