From: Shawn O. Pearce <spearce@spearce.org>
Date: Mon, 06 Oct 2008 08:15:54 GMT
Subject: Re: [EGIT PATCH 3/6] Add a method to get refs by object Id
Message-ID: <20081006081554.GD27516@spearce.org>
In-Reply-To: <1223249802-9959-4-git-send-email-robin.rosenberg@dewire.com>

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?

> +		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.

> +			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);
		}
	}

The trick is that most targets have exactly one Ref, so the first
ret.put will return null and we'll never worry about the expansion
cases.  In the rare cases where more than one Ref points at the
same object we'll degrade into doing two hash lookups per target,
which is no worse than what you have right now.

-- 
Shawn.