tisdag 11 november 2008 19:23:58 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
