Re: [EGIT PATCH 3/6] Add a method to get refs by object Id

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 6, 2008, 08:15 UTC
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:

Show 11 quoted lines

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

Show 15 quoted lines

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