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

[Robin Rosenberg <robin.rosenberg.lists@dewire.com>]

tisdag 11 november 2008 19:23:58 skrev Shawn O. Pearce:
> 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?
Means we haven't tried to peel it yet. I could use an extra flags instead. We may
have thousands of refs but perhaps not zillions.

> 
> 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.
No, if it is a loose ref we did not yet attempt to peel it. null means we have tried
zeroId means we haven't tried (see above).

> 
> > +		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:
ok..
>         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.
>         }
> 
That gives a different behavior, but which is also ok...

> >  	/**
> > +	 * 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? 
...it's wrong, and thus irrelevant.

> > +	 * 
> > +	 * @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.
Ack. 
> 
> 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).

I pick the copy.

-- robin