From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Mon, 06 Oct 2008 22:37:53 GMT
Subject: Re: [EGIT PATCH 3/6] Add a method to get refs by object Id
Message-ID: <200810070037.53841.robin.rosenberg@dewire.com>
In-Reply-To: <20081006081554.GD27516@spearce.org>

måndagen den 6 oktober 2008 10.15.54 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > 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 dfce1b8..3fc5236 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 +940,33 @@ public String getBranch() throws IOException {
> >  	}
> >  
> >  	/**
> > +	 * @return a map with all objects referenced by a peeled ref.
> > +	 */
> > +	public Map<AnyObjectId, List<Ref>> getAllRefsByPeeledObjectId() {
> 
> Do we really want to promise List here?  Can we make it just
> Collection instead?
Sure. Our promise is actually slightly better, it is Set, but Java doesn't have a suitable class for that.

> 
> > +		Map<String, Ref> allRefs = getAllRefs();
> > +		HashMap<AnyObjectId, List<Ref>> ret = new HashMap<AnyObjectId, List<Ref>>(allRefs.size());
> > +		for (Map.Entry<String,Ref> e : allRefs.entrySet()) {
> > +			Ref ref = e.getValue();
> 
> I think this is cleaner:
> 
> 	for (Ref ref : allRefs.values()) {
> 
> as you never use the key.
Yes. I was thinking it might be less efficient, but the JDK implementation looks quite well optimized in 1.6 at least
so values() is slightly faster.

> > +			AnyObjectId target = ref.getPeeledObjectId();
> > +			if (target == null)
> > +				target = ref.getObjectId();
> > +			List<Ref> list = ret.get(target);
> > +			if (list == null) {
> > +				list = Collections.singletonList(ref);
> > +			} else {
> > +				if (list.size() == 1) {
> > +					ArrayList<Ref> list2 = new ArrayList<Ref>(2);
> > +					list2.add(list.get(0));
> > +					list = list2;
> > +				}
> > +				list.add(ref);
> > +			}
> > +			ret.put(target, list);
> 
> Hmm.  Putting the list every time is pointless.  This is may run
> faster because we (on average) only do one hash lookup per target,
> not 2:
> 
> 	List<Ref> nl = Collections.singletonList(ref);
> 	List<Ref> ol = ret.put(target, nl);
> 	if (ol != null) {
> 		if (ol.size() == 1) {
> 			nl = new ArrayList<Ref>(2);
> 			nl.add(ol.get(0));
> 			nl.add(ref);
> 			ret.put(target, nl);
> 		} else {
> 			ol.add(ref)
> 			ret.put(target, ol);
> 		}
> 	}

ok, I guess one just has has to include the comment on why for the casual reader. 

-- robin