(unknown),

(unknown),

[Robin Rosenberg]

This series decorates the graphical and text (jgit log) history listings
with tags. For the text command it is optional.

Reviewers may want to pay special attention to the changes in the Ref class.

-- robin

[EGIT PATCH 1/6] Keep original ref name when reading refs

[Robin Rosenberg]

We want to know the original name of refs, not just the target name.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java |   63 +++++++++++++++++++-
 .../src/org/spearce/jgit/lib/RefDatabase.java      |   45 +++++++++-----
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   14 ++---
 3 files changed, 94 insertions(+), 28 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
index db94875..2f102af 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Ref.java
@@ -43,6 +43,11 @@
  * A ref in Git is (more or less) a variable that holds a single object
  * identifier. The object identifier can be any valid Git object (blob, tree,
  * commit, annotated tag, ...).
+ * <p>
+ * The ref name has the attributes of the ref that was asked for as well as
+ * the ref it was resolved to for symbolic refs plus the object id it points
+ * to and (for tags) the peeled target object id, i.e. the tag resolved
+ * recursively until a non-tag object is referenced.
  */
 public class Ref {
 	/** Location where a {@link Ref} is stored. */
@@ -119,19 +124,24 @@ public boolean isPacked() {
 
 	private ObjectId peeledObjectId;
 
+	private final String origName;
+
 	/**
 	 * Create a new ref pairing.
 	 * 
 	 * @param st
 	 *            method used to store this ref.
+	 * @param origName
+	 *            The name used to resolve this ref
 	 * @param refName
 	 *            name of this ref.
 	 * @param id
 	 *            current value of the ref. May be null to indicate a ref that
 	 *            does not exist yet.
 	 */
-	public Ref(final Storage st, final String refName, final ObjectId id) {
+	public Ref(final Storage st, final String origName, final String refName, final ObjectId id) {
 		storage = st;
+		this.origName = origName;
 		name = refName;
 		objectId = id;
 	}
@@ -146,19 +156,56 @@ public Ref(final Storage st, final String refName, final ObjectId id) {
 	 * @param id
 	 *            current value of the ref. May be null to indicate a ref that
 	 *            does not exist yet.
+	 */
+	public Ref(final Storage st, final String refName, final ObjectId id) {
+		this(st, refName, refName, id);
+	}
+
+	/**
+	 * Create a new ref pairing.
+	 * 
+	 * @param st
+	 *            method used to store this ref.
+	 * @param origName
+	 *            The name used to resolve this ref
+	 * @param refName
+	 *            name of this ref.
+	 * @param id
+	 *            current value of the ref. May be null to indicate a ref that
+	 *            does not exist yet.
 	 * @param peel
 	 *            peeled value of the ref's tag. May be null if this is not a
 	 *            tag or the peeled value is not known.
 	 */
-	public Ref(final Storage st, final String refName, final ObjectId id,
+	public Ref(final Storage st, final String origName, final String refName, final ObjectId id,
 			final ObjectId peel) {
 		storage = st;
+		this.origName = origName;
 		name = refName;
 		objectId = id;
 		peeledObjectId = peel;
 	}
 
 	/**
+	 * Create a new ref pairing.
+	 * 
+	 * @param st
+	 *            method used to store this ref.
+	 * @param refName
+	 *            name of this ref.
+	 * @param id
+	 *            current value of the ref. May be null to indicate a ref that
+	 *            does not exist yet.
+	 * @param peel
+	 *            peeled value of the ref's tag. May be null if this is not a
+	 *            tag or the peeled value is not known.
+	 */
+	public Ref(final Storage st, final String refName, final ObjectId id,
+			final ObjectId peel) {
+		this(st, refName, refName, id, peel);
+	}
+
+	/**
 	 * What this ref is called within the repository.
 	 * 
 	 * @return name of this ref.
@@ -168,6 +215,13 @@ public String getName() {
 	}
 
 	/**
+	 * @return the originally resolved name
+	 */
+	public String getOrigName() {
+		return origName;
+	}
+
+	/**
 	 * Cached value of this ref.
 	 * 
 	 * @return the value of this ref at the last time we read it.
@@ -200,6 +254,9 @@ public Storage getStorage() {
 	}
 
 	public String toString() {
-		return "Ref[" + name + "=" + ObjectId.toString(getObjectId()) + "]";
+		String o = "";
+		if (!origName.equals(name))
+			o = "(" + origName + ")";
+		return "Ref[" + o + name + "=" + ObjectId.toString(getObjectId()) + "]";
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 5c1f060..5a1b85f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -51,6 +51,7 @@
 import java.util.Map;
 
 import org.spearce.jgit.errors.ObjectWritingException;
+import org.spearce.jgit.lib.Ref.Storage;
 import org.spearce.jgit.util.FS;
 import org.spearce.jgit.util.NB;
 
@@ -135,8 +136,8 @@ RefUpdate newUpdate(final String name) throws IOException {
 		return new RefUpdate(this, r, fileForRef(r.getName()));
 	}
 
-	void stored(final String name, final ObjectId id, final long time) {
-		looseRefs.put(name, new Ref(Ref.Storage.LOOSE, name, id));
+	void stored(final String origName, final String name, final ObjectId id, final long time) {
+		looseRefs.put(name, new Ref(Ref.Storage.LOOSE, origName, name, id));
 		looseRefsMTime.put(name, time);
 		setModified();
 		db.fireRefsMaybeChanged();
@@ -222,12 +223,12 @@ private void readLooseRefs(final Map<String, Ref> avail,
 			final String entName = ent.getName();
 			if (".".equals(entName) || "..".equals(entName))
 				continue;
-			readOneLooseRef(avail, prefix + entName, ent);
+			readOneLooseRef(avail, prefix + entName, prefix + entName, ent);
 		}
 	}
 
 	private void readOneLooseRef(final Map<String, Ref> avail,
-			final String refName, final File ent) {
+			final String origName, final String refName, final File ent) {
 		// Unchanged and cached? Don't read it again.
 		//
 		Ref ref = looseRefs.get(refName);
@@ -270,7 +271,7 @@ private void readOneLooseRef(final Map<String, Ref> avail,
 					return;
 				}
 
-				ref = new Ref(Ref.Storage.LOOSE, refName, id);
+				ref = new Ref(Ref.Storage.LOOSE, origName, refName, id);
 				looseRefs.put(ref.getName(), ref);
 				looseRefsMTime.put(ref.getName(), ent.lastModified());
 				avail.put(ref.getName(), ref);
@@ -293,27 +294,35 @@ private File fileForRef(final String name) {
 		return new File(gitDir, name);
 	}
 
-	private Ref readRefBasic(final String name, final int depth)
+	private Ref readRefBasic(final String name, final int depth) throws IOException {
+		return readRefBasic(name, name, depth);
+	}
+
+	private Ref readRefBasic(final String origName, final String name, final int depth)
 			throws IOException {
 		// Prefer loose ref to packed ref as the loose
 		// file can be more up-to-date than a packed one.
 		//
-		Ref ref = looseRefs.get(name);
+		Ref ref = looseRefs.get(origName);
 		final File loose = fileForRef(name);
 		final long mtime = loose.lastModified();
 		if (ref != null) {
 			Long cachedlastModified = looseRefsMTime.get(name);
 			if (cachedlastModified != null && cachedlastModified == mtime)
 				return ref;
-			looseRefs.remove(name);
-			looseRefsMTime.remove(name);
+			looseRefs.remove(origName);
+			looseRefsMTime.remove(origName);
 		}
 
 		if (mtime == 0) {
 			// If last modified is 0 the file does not exist.
 			// Try packed cache.
 			//
-			return packedRefs.get(name);
+			ref = packedRefs.get(name);
+			if (ref != null)
+				if (!ref.getOrigName().equals(origName))
+					ref = new Ref(Storage.LOOSE_PACKED, origName, name, ref.getObjectId());
+			return ref;
 		}
 
 		final String line;
@@ -324,7 +333,7 @@ private Ref readRefBasic(final String name, final int depth)
 		}
 
 		if (line == null || line.length() == 0)
-			return new Ref(Ref.Storage.LOOSE, name, null);
+			return new Ref(Ref.Storage.LOOSE, origName, name, null);
 
 		if (line.startsWith("ref: ")) {
 			if (depth >= 5) {
@@ -333,12 +342,16 @@ private Ref readRefBasic(final String name, final int depth)
 			}
 
 			final String target = line.substring("ref: ".length());
-			final Ref r = readRefBasic(target, depth + 1);
+			Ref r = readRefBasic(target, target, depth + 1);
 			Long cachedMtime = looseRefsMTime.get(name);
 			if (cachedMtime != null && cachedMtime != mtime)
 				setModified();
 			looseRefsMTime.put(name, mtime);
-			return r != null ? r : new Ref(Ref.Storage.LOOSE, target, null);
+			if (r == null)
+				return new Ref(Ref.Storage.LOOSE, origName, target, null);
+			if (!origName.equals(r.getName()))
+				r = new Ref(Ref.Storage.LOOSE_PACKED, origName, r.getName(), r.getObjectId(), r.getPeeledObjectId());
+			return r; 
 		}
 
 		setModified();
@@ -350,7 +363,7 @@ private Ref readRefBasic(final String name, final int depth)
 			throw new IOException("Not a ref: " + name + ": " + line);
 		}
 
-		ref = new Ref(Ref.Storage.LOOSE, name, id);
+		ref = new Ref(Ref.Storage.LOOSE, origName, name, id);
 		looseRefs.put(name, ref);
 		looseRefsMTime.put(name, mtime);
 		return ref;
@@ -384,7 +397,7 @@ private void refreshPackedRefs() {
 
 						final ObjectId id = ObjectId.fromString(p.substring(1));
 						last = new Ref(Ref.Storage.PACKED, last.getName(), last
-								.getObjectId(), id);
+								.getName(), last.getObjectId(), id);
 						newPackedRefs.put(last.getName(), last);
 						continue;
 					}
@@ -392,7 +405,7 @@ private void refreshPackedRefs() {
 					final int sp = p.indexOf(' ');
 					final ObjectId id = ObjectId.fromString(p.substring(0, sp));
 					final String name = new String(p.substring(sp + 1));
-					last = new Ref(Ref.Storage.PACKED, name, id);
+					last = new Ref(Ref.Storage.PACKED, name, name, id);
 					newPackedRefs.put(last.getName(), last);
 				}
 			} finally {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index 86b44c5..235c2fd 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -131,9 +131,6 @@
 	/** Repository the ref is stored in. */
 	private final RefDatabase db;
 
-	/** Name of the ref. */
-	private final String name;
-
 	/** Location of the loose file holding the value of this ref. */
 	private final File looseFile;
 
@@ -160,7 +157,6 @@
 	RefUpdate(final RefDatabase r, final Ref ref, final File f) {
 		db = r;
 		this.ref = ref;
-		name = ref.getName();
 		oldValue = ref.getObjectId();
 		looseFile = f;
 	}
@@ -171,7 +167,7 @@ RefUpdate(final RefDatabase r, final Ref ref, final File f) {
 	 * @return name of this ref.
 	 */
 	public String getName() {
-		return name;
+		return ref.getName();
 	}
 
 	/**
@@ -349,9 +345,9 @@ public Result delete() throws IOException {
 	 * @throws IOException
 	 */
 	public Result delete(final RevWalk walk) throws IOException {
-		if (name.startsWith(Constants.R_HEADS)) {
+		if (getName().startsWith(Constants.R_HEADS)) {
 			final Ref head = db.readRef(Constants.HEAD);
-			if (head != null && name.equals(head.getName()))
+			if (head != null && getName().equals(head.getName()))
 				return Result.REJECTED_CURRENT_BRANCH;
 		}
 
@@ -373,7 +369,7 @@ private Result updateImpl(final RevWalk walk, final Store store)
 		if (!lock.lock())
 			return Result.LOCK_FAILURE;
 		try {
-			oldValue = db.idOf(name);
+			oldValue = db.idOf(getName());
 			if (oldValue == null)
 				return store.store(lock, Result.NEW);
 
@@ -428,7 +424,7 @@ else if (status == Result.NEW)
 				getName());
 		if (!lock.commit())
 			return Result.LOCK_FAILURE;
-		db.stored(name, newValue, lock.getCommitLastModified());
+		db.stored(this.ref.getOrigName(),  ref.getName(), newValue, lock.getCommitLastModified());
 		return status;
 	}
 
-- 
1.6.0.1.310.gf789d0.dirty

[EGIT PATCH 2/6] Peel annotated tags when getting all refs

[Robin Rosenberg]

For packed refs we got this automatically from packed-refs,
but for loose tags we have to follow the tags and get the leaf
object in order to comply with the documentation.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/lib/RefDatabase.java      |   24 ++++++++++++++++++-
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 5a1b85f..1ee70d9 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -271,7 +271,16 @@ private void readOneLooseRef(final Map<String, Ref> avail,
 					return;
 				}
 
-				ref = new Ref(Ref.Storage.LOOSE, origName, refName, id);
+				Object tt = db.mapObject(id, refName);
+				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 = new Ref(Ref.Storage.LOOSE, origName, refName, id, t.getObjId());
+				} else
+					ref = new Ref(Ref.Storage.LOOSE, origName, refName, id);
+
 				looseRefs.put(ref.getName(), ref);
 				looseRefsMTime.put(ref.getName(), ent.lastModified());
 				avail.put(ref.getName(), ref);
@@ -363,7 +372,18 @@ private Ref readRefBasic(final String origName, final String name, final int dep
 			throw new IOException("Not a ref: " + name + ": " + line);
 		}
 
-		ref = new Ref(Ref.Storage.LOOSE, origName, name, id);
+		Object tt = db.mapObject(id, origName);
+		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 = new Ref(Ref.Storage.LOOSE, origName, name, id, t.getObjId());
+		} else
+			ref = new Ref(Ref.Storage.LOOSE, origName, name, id);
+
+		looseRefs.put(origName, ref);
+		ref = new Ref(Ref.Storage.LOOSE, origName, id);
 		looseRefs.put(name, ref);
 		looseRefsMTime.put(name, mtime);
 		return ref;
-- 
1.6.0.1.310.gf789d0.dirty

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

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/lib/Repository.java       |   28 ++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

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
@@ -47,6 +47,7 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
@@ -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() {
+		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();
+			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);
+		}
+		return ret;
+	}
+
+	/**
 	 * @return true if HEAD points to a StGit patch.
 	 */
 	public boolean isStGitMode() {
-- 
1.6.0.1.310.gf789d0.dirty

[EGIT PATCH 4/6] Add tags to the graphical history display.

[Robin Rosenberg]

Both the SWT (Eclipse) drawing and Swing versions are updated.
The coloring and shapes are intentionally not the same as for gitk.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/core/internal/storage/KidWalk.java        |    3 +-
 .../egit/ui/internal/history/SWTCommit.java        |    5 +-
 .../egit/ui/internal/history/SWTPlotRenderer.java  |   64 +++++++++++++++++++-
 .../spearce/egit/ui/internal/history/SWTWalk.java  |    5 +-
 .../org/spearce/jgit/awtui/AWTPlotRenderer.java    |   46 ++++++++++++++
 .../spearce/jgit/revplot/AbstractPlotRenderer.java |   18 +++++-
 .../src/org/spearce/jgit/revplot/PlotCommit.java   |    8 ++-
 .../src/org/spearce/jgit/revplot/PlotWalk.java     |   56 ++++++++++++++++-
 .../src/org/spearce/jgit/revwalk/RevWalk.java      |   19 +++++-
 9 files changed, 209 insertions(+), 15 deletions(-)

diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/internal/storage/KidWalk.java b/org.spearce.egit.core/src/org/spearce/egit/core/internal/storage/KidWalk.java
index 6b8f468..b337efe 100644
--- a/org.spearce.egit.core/src/org/spearce/egit/core/internal/storage/KidWalk.java
+++ b/org.spearce.egit.core/src/org/spearce/egit/core/internal/storage/KidWalk.java
@@ -11,6 +11,7 @@
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revwalk.RevWalk;
 
 class KidWalk extends RevWalk {
@@ -19,7 +20,7 @@ KidWalk(final Repository repo) {
 	}
 
 	@Override
-	protected RevCommit createCommit(final AnyObjectId id) {
+	protected RevCommit createCommit(final AnyObjectId id, final Ref[] tags) {
 		return new KidCommit(id);
 	}
 }
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java
index fa0d25d..2341fbd 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTCommit.java
@@ -10,12 +10,13 @@
 import org.eclipse.swt.widgets.Widget;
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.revplot.PlotCommit;
+import org.spearce.jgit.lib.Ref;
 
 class SWTCommit extends PlotCommit<SWTCommitList.SWTLane> {
 	Widget widget;
 
-	SWTCommit(final AnyObjectId id) {
-		super(id);
+	SWTCommit(final AnyObjectId id, final Ref[] tags) {
+		super(id, tags);
 	}
 
 	@Override
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
index 23ec255..56d5842 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTPlotRenderer.java
@@ -15,7 +15,10 @@
 import org.eclipse.swt.widgets.Display;
 import org.eclipse.swt.widgets.Event;
 import org.eclipse.swt.widgets.TableItem;
+import org.eclipse.ui.themes.ColorUtil;
 import org.spearce.egit.ui.internal.history.SWTCommitList.SWTLane;
+import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revplot.AbstractPlotRenderer;
 import org.spearce.jgit.revplot.PlotCommit;
 
@@ -26,7 +29,13 @@
 
 	private final Color sys_gray;
 
-	private Color sys_darkblue;
+	private final Color sys_darkblue;
+
+	private final Color sys_yellow;
+
+	private final Color sys_green;
+
+	private final Color sys_white;
 
 	GC g;
 
@@ -43,6 +52,9 @@ SWTPlotRenderer(final Display d) {
 		sys_black = d.getSystemColor(SWT.COLOR_BLACK);
 		sys_gray = d.getSystemColor(SWT.COLOR_GRAY);
 		sys_darkblue = d.getSystemColor(SWT.COLOR_DARK_BLUE);
+		sys_yellow = d.getSystemColor(SWT.COLOR_YELLOW);
+		sys_green = d.getSystemColor(SWT.COLOR_GREEN);
+		sys_white = d.getSystemColor(SWT.COLOR_WHITE);
 	}
 
 	void paint(final Event event) {
@@ -92,7 +104,57 @@ protected void drawText(final String msg, final int x, final int y) {
 		g.drawString(msg, cellX + x, cellY + texty);
 	}
 
+	@Override
+	protected int drawLabel(int x, int y, Ref ref) {
+		String txt;
+		String name = ref.getOrigName();
+		if (name.startsWith(Constants.R_HEADS)) {
+			g.setBackground(sys_green);
+			txt = name.substring(Constants.R_HEADS.length());
+		} else if (name.startsWith(Constants.R_REMOTES)){
+			g.setBackground(sys_gray);
+			txt = name.substring(Constants.R_REMOTES.length());
+		} else if (name.startsWith(Constants.R_TAGS)){
+			g.setBackground(sys_yellow);
+			txt = name.substring(Constants.R_TAGS.length());
+		} else {
+			// Whatever this would be
+			g.setBackground(sys_white);
+			if (name.startsWith(Constants.R_REFS))
+				txt = name.substring(Constants.R_REFS.length());
+			else
+				txt = name; // HEAD and such
+		}
+		Color peeledColor = null;
+		if (ref.getPeeledObjectId() != null) {
+			peeledColor = new Color(g.getDevice(), ColorUtil.blend(g.getBackground().getRGB(), sys_white.getRGB()));
+			g.setBackground(peeledColor);
+		}
+		if (txt.length() > 12)
+			txt = txt.substring(0,11) + "\u2026"; // ellipsis "…" (in UTF-8)
+
+		Point testsz = g.stringExtent(txt);
+		final int texty = (y * 2 - testsz.y) / 2;
+		g.setForeground(sys_black);
+		g.drawString(txt, x + 2, cellY + texty);
+		g.setLineWidth(2);
+		Color blend1 = new Color(g.getDevice(), ColorUtil.blend(g.getBackground().getRGB(), sys_gray.getRGB()));
+		g.setForeground(blend1);
+		g.drawRoundRectangle(x, cellY + texty -2, testsz.x + 3, testsz.y + 3, testsz.y/4, testsz.y/4);
+		g.setLineWidth(2);
+		Color blend2 = new Color(g.getDevice(), ColorUtil.blend(g.getBackground().getRGB(), sys_black.getRGB()));
+		g.setForeground(blend2);
+		g.drawRoundRectangle(x + 1, cellY + texty -1, testsz.x + 1, testsz.y + 1, testsz.y/4, testsz.y/4);
+
+		blend1.dispose();
+		blend2.dispose();
+		if (peeledColor != null)
+			peeledColor.dispose();
+		return 8 + testsz.x;
+	}
+
 	protected Color laneColor(final SWTLane myLane) {
 		return myLane != null ? myLane.color : sys_black;
 	}
+
 }
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java
index 527d284..bc347db 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/SWTWalk.java
@@ -11,6 +11,7 @@
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.revplot.PlotWalk;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.lib.Ref;
 
 class SWTWalk extends PlotWalk {
 	SWTWalk(final Repository repo) {
@@ -18,7 +19,7 @@ SWTWalk(final Repository repo) {
 	}
 
 	@Override
-	protected RevCommit createCommit(final AnyObjectId id) {
-		return new SWTCommit(id);
+	protected RevCommit createCommit(final AnyObjectId id, final Ref[] tags) {
+		return new SWTCommit(id, tags);
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java b/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java
index b6b715c..5dcddf5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/awtui/AWTPlotRenderer.java
@@ -44,6 +44,8 @@
 
 import org.spearce.jgit.awtui.CommitGraphPane.GraphCellRender;
 import org.spearce.jgit.awtui.SwingCommitList.SwingLane;
+import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revplot.AbstractPlotRenderer;
 import org.spearce.jgit.revplot.PlotCommit;
 
@@ -134,4 +136,48 @@ void paintTriangleDown(final int cx, final int y, final int h) {
 		g.drawPolygon(triangle);
 	}
 
+	@Override
+	protected int drawLabel(int x, int y, Ref ref) {
+		String txt;
+		String name = ref.getOrigName();
+		if (name.startsWith(Constants.R_HEADS)) {
+			g.setBackground(Color.GREEN);
+			txt = name.substring(Constants.R_HEADS.length());
+		} else if (name.startsWith(Constants.R_REMOTES)){
+			g.setBackground(Color.LIGHT_GRAY);
+			txt = name.substring(Constants.R_REMOTES.length());
+		} else if (name.startsWith(Constants.R_TAGS)){
+			g.setBackground(Color.YELLOW);
+			txt = name.substring(Constants.R_TAGS.length());
+		} else {
+			// Whatever this would be
+			g.setBackground(Color.WHITE);
+			if (name.startsWith(Constants.R_REFS))
+				txt = name.substring(Constants.R_REFS.length());
+			else
+				txt = name; // HEAD and such
+		}
+		if (ref.getPeeledObjectId() != null) {
+			float[] colorComponents = g.getBackground().getRGBColorComponents(null);
+			colorComponents[0] *= 0.9;
+			colorComponents[1] *= 0.9;
+			colorComponents[2] *= 0.9;
+			g.setBackground(new Color(colorComponents[0],colorComponents[1],colorComponents[2]));
+		}
+		if (txt.length() > 12)
+			txt = txt.substring(0,11) + "\u2026"; // ellipsis "…" (in UTF-8)
+
+		final int texth = g.getFontMetrics().getHeight();
+		int textw = g.getFontMetrics().stringWidth(txt);
+		g.setColor(g.getBackground());
+		int arcHeight = texth/4;
+		int y0 = y - texth/2 + (cell.getHeight() - texth)/2;
+		g.fillRoundRect(x , y0, textw + arcHeight*2, texth -1, arcHeight, arcHeight);
+		g.setColor(g.getColor().darker());
+		g.drawRoundRect(x, y0, textw + arcHeight*2, texth -1 , arcHeight, arcHeight);
+		g.setColor(Color.BLACK);
+		g.drawString(txt, x + arcHeight, y0 + texth - g.getFontMetrics().getDescent());
+
+		return arcHeight * 3 + textw;
+	}
 }
\ No newline at end of file
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java
index f175c9d..603547b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/AbstractPlotRenderer.java
@@ -37,6 +37,7 @@
 
 package org.spearce.jgit.revplot;
 
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revwalk.RevFlag;
 
 /**
@@ -140,11 +141,24 @@ protected void paintCommit(final PlotCommit<TLane> commit, final int h) {
 		else
 			drawCommitDot(dotX, dotY, dotSize, dotSize);
 
+		int textx = Math.max(maxCenter + LANE_WIDTH / 2, dotX + dotSize) + 8;
+		int n = commit.refs == null ? 0 : commit.refs.length;
+		for (int i = 0; i < n; ++i) {
+			textx += drawLabel(textx + dotSize, h/2, commit.refs[i]);
+		}
+
 		final String msg = commit.getShortMessage();
-		final int textx = Math.max(maxCenter + LANE_WIDTH / 2, dotX + dotSize) + 8;
-		drawText(msg, textx, h / 2);
+		drawText(msg, textx + dotSize + n*2, h / 2);
 	}
 
+	/** FIXME: supply text 
+	 * @param x 
+	 * @param y 
+	 * @param ref TODO
+	 * @return TODO
+	 */
+	protected abstract int drawLabel(int x, int y, Ref ref);
+
 	private int computeDotSize(final int h) {
 		int d = (int) (Math.min(h, LANE_WIDTH) * 0.50f);
 		d += (d & 1);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
index 5a5ef1e..fac89f5 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
@@ -39,6 +39,7 @@
 
 import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.lib.Ref;
 
 /**
  * A commit reference to a commit in the DAG.
@@ -58,14 +59,19 @@
 
 	PlotCommit[] children;
 
+	Ref[] refs;
+
 	/**
 	 * Create a new commit.
 	 * 
 	 * @param id
 	 *            the identity of this commit.
+	 * @param tags
+	 *            the tags associated with this commit, null for no tags
 	 */
-	protected PlotCommit(final AnyObjectId id) {
+	protected PlotCommit(final AnyObjectId id, final Ref[] tags) {
 		super(id);
+		this.refs = tags;
 		passingLanes = NO_LANES;
 		children = NO_CHILDREN;
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java
index e5e8aba..6e253f4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotWalk.java
@@ -37,14 +37,26 @@
 
 package org.spearce.jgit.revplot;
 
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Map;
+
 import org.spearce.jgit.lib.AnyObjectId;
+import org.spearce.jgit.lib.Commit;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
+import org.spearce.jgit.lib.Tag;
 import org.spearce.jgit.revwalk.RevCommit;
 import org.spearce.jgit.revwalk.RevSort;
 import org.spearce.jgit.revwalk.RevWalk;
 
 /** Specialized RevWalk for visualization of a commit graph. */
 public class PlotWalk extends RevWalk {
+
+	Map<AnyObjectId, List<Ref>> reverseRefMap;
+
 	/**
 	 * Create a new revision walker for a given repository.
 	 * 
@@ -54,6 +66,7 @@
 	public PlotWalk(final Repository repo) {
 		super(repo);
 		super.sort(RevSort.TOPO, true);
+		reverseRefMap = repo.getAllRefsByPeeledObjectId();
 	}
 
 	@Override
@@ -64,7 +77,46 @@ public void sort(final RevSort s, final boolean use) {
 	}
 
 	@Override
-	protected RevCommit createCommit(final AnyObjectId id) {
-		return new PlotCommit(id);
+	protected RevCommit createCommit(final AnyObjectId id, final Ref[] tags) {
+		return new PlotCommit(id, tags);
+	}
+
+	@Override
+	protected Ref[] getTags(final AnyObjectId commitId) {
+		List<Ref> list = reverseRefMap.get(commitId);
+		Ref[] tags;
+		if (list == null)
+			tags = null;
+		else {
+			if (list != null && list.size() > 1) {
+				Collections.sort(list, new Comparator<Ref>() {
+					public int compare(Ref o1, Ref o2) {
+						try {
+							Object obj1 = getRepository().mapObject(o1.getObjectId(), o1.getName());
+							Object obj2 = getRepository().mapObject(o2.getObjectId(), o2.getName());
+							long t1 = timeof(obj1);
+							long t2 = timeof(obj2);
+							if (t1 > t2)
+								return -1;
+							if (t1 < t2)
+								return 1;
+							return 0;
+						} catch (IOException e) {
+							// ignore
+							return 0;
+						}
+					}
+					long timeof(Object o) {
+						if (o instanceof Commit)
+							return ((Commit)o).getCommitter().getWhen().getTime();
+						if (o instanceof Tag)
+							return ((Tag)o).getTagger().getWhen().getTime();
+						return 0;
+					}
+				});
+			}
+			tags = list.toArray(new Ref[list.size()]);
+		}
+		return tags;
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
index d7e4c58..41d57c6 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
@@ -53,6 +53,7 @@
 import org.spearce.jgit.lib.ObjectId;
 import org.spearce.jgit.lib.ObjectIdSubclassMap;
 import org.spearce.jgit.lib.ObjectLoader;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.lib.WindowCursor;
 import org.spearce.jgit.revwalk.filter.RevFilter;
@@ -541,7 +542,7 @@ public RevTree lookupTree(final AnyObjectId id) {
 	public RevCommit lookupCommit(final AnyObjectId id) {
 		RevCommit c = (RevCommit) objects.get(id);
 		if (c == null) {
-			c = createCommit(id);
+			c = createCommit(id, getTags(id));
 			objects.add(c);
 		}
 		return c;
@@ -564,7 +565,7 @@ public RevObject lookupAny(final AnyObjectId id, final int type) {
 		if (r == null) {
 			switch (type) {
 			case Constants.OBJ_COMMIT:
-				r = createCommit(id);
+				r = createCommit(id, getTags(id));
 				break;
 			case Constants.OBJ_TREE:
 				r = new RevTree(id);
@@ -687,7 +688,7 @@ public RevObject parseAny(final AnyObjectId id)
 			final int type = ldr.getType();
 			switch (type) {
 			case Constants.OBJ_COMMIT: {
-				final RevCommit c = createCommit(ldr.getId());
+				final RevCommit c = createCommit(ldr.getId(), getTags(ldr.getId()));
 				c.parseCanonical(this, data);
 				r = c;
 				break;
@@ -718,6 +719,14 @@ public RevObject parseAny(final AnyObjectId id)
 	}
 
 	/**
+	 * @param commitId
+	 * @return the list of refs associated with a commit, possibly filtered
+	 */
+	protected Ref[] getTags(final AnyObjectId commitId) {
+		return null; // Don't get tags in the basic case
+	}
+
+	/**
 	 * Ensure the object's content has been parsed.
 	 * <p>
 	 * This method only returns successfully if the object exists and was parsed
@@ -1008,9 +1017,11 @@ private boolean isNotStarted() {
 	 * 
 	 * @param id
 	 *            the object this walker requires a commit reference for.
+	 * @param tags
+	 *            tags attached to the commit
 	 * @return a new unparsed reference for the object.
 	 */
-	protected RevCommit createCommit(final AnyObjectId id) {
+	protected RevCommit createCommit(final AnyObjectId id, final Ref[] tags) {
 		return new RevCommit(id);
 	}
 
-- 
1.6.0.1.310.gf789d0.dirty

[EGIT PATCH 5/6] Add decorate option to log program

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/pgm/Log.java              |   32 ++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java
index e16387b..378d9e0 100644
--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Log.java
@@ -40,10 +40,17 @@
 
 import java.text.DateFormat;
 import java.text.SimpleDateFormat;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
 import java.util.TimeZone;
 
+import org.kohsuke.args4j.Option;
+import org.spearce.jgit.lib.AnyObjectId;
 import org.spearce.jgit.lib.PersonIdent;
+import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.revwalk.RevWalk;
 
 @Command(common = true, usage = "View commit history")
 class Log extends RevWalkTextBuiltin {
@@ -51,14 +58,39 @@
 
 	private final DateFormat fmt;
 
+	private Map<AnyObjectId, List<Ref>> allRefsByPeeledObjectId;
+
+	@Option(name="--decorate", usage="Show ref names matching commits")
+	private boolean decorate;
+
 	Log() {
 		fmt = new SimpleDateFormat("EEE MMM dd HH:mm:ss yyyy ZZZZZ");
 	}
 
 	@Override
+	protected org.spearce.jgit.revwalk.RevWalk createWalk() {
+		RevWalk ret = super.createWalk();
+		if (decorate)
+			allRefsByPeeledObjectId = getRepository().getAllRefsByPeeledObjectId();
+		return ret;
+	}
+
+	@Override
 	protected void show(final RevCommit c) throws Exception {
 		out.print("commit ");
 		c.getId().copyTo(outbuffer, out);
+		if (decorate) {
+			List<Ref> list = allRefsByPeeledObjectId.get(c.copy());
+			if (list != null) {
+				out.print(" (");
+				for (Iterator<Ref> i = list.iterator(); i.hasNext(); ) {
+					out.print(i.next().getOrigName());
+					if (i.hasNext())
+						out.print(" ");
+				}
+				out.print(")");
+			}
+		}
 		out.println();
 
 		final PersonIdent author = c.getAuthorIdent();
-- 
1.6.0.1.310.gf789d0.dirty

[EGIT PATCH 6/6] Comment the getId method and hint for copy to actually get an ObjectId

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/revwalk/RevObject.java    |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
index 451205c..2209c04 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
@@ -67,6 +67,7 @@ abstract void parse(RevWalk walk) throws MissingObjectException,
 
 	/**
 	 * Get the name of this object.
+	 * See {@link #copy()} to really get a copy
 	 * 
 	 * @return unique hash of this object.
 	 */
-- 
1.6.0.1.310.gf789d0.dirty

Re: [EGIT PATCH 4/6] Add tags to the graphical history display.

[Shawn O. Pearce]

FYI, my comments don't fully cover this patch yet.

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
> index 5a5ef1e..fac89f5 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/revplot/PlotCommit.java
> @@ -58,14 +59,19 @@
>  
>  	PlotCommit[] children;
>  
> +	Ref[] refs;
> +
>  	/**
>  	 * Create a new commit.
>  	 * 
>  	 * @param id
>  	 *            the identity of this commit.
> +	 * @param tags
> +	 *            the tags associated with this commit, null for no tags
>  	 */
> -	protected PlotCommit(final AnyObjectId id) {
> +	protected PlotCommit(final AnyObjectId id, final Ref[] tags) {
>  		super(id);
> +		this.refs = tags;

this.refs isn't final.  There is no reason to be adding it to the
constructor and having this ripple effect all the way up into the
RevWalk class.

Maybe PlotWalk should override next() and only set PlotCommit.refs on
things returned from super.next()?  This way we only do tag lookup
on commits that the filtering rules have said should be shown in
to the application, but the refs should still be inserted prior to
the application seeing the RevCommit.

> @@ -54,6 +66,7 @@
>  	public PlotWalk(final Repository repo) {
>  		super(repo);
>  		super.sort(RevSort.TOPO, true);
> +		reverseRefMap = repo.getAllRefsByPeeledObjectId();

I wonder if this shouldn't be done as part of the StartGenerator
(or something like it but specialized for PlotWalk).  I only say
that because a reused PlotWalk may want to make sure its ref map
is current with the repository its about to walk against.

> +	@Override
> +	protected Ref[] getTags(final AnyObjectId commitId) {
> +		List<Ref> list = reverseRefMap.get(commitId);
> +		Ref[] tags;
> +		if (list == null)
> +			tags = null;
> +		else {
> +			if (list != null && list.size() > 1) {
> +				Collections.sort(list, new Comparator<Ref>() {
> +					public int compare(Ref o1, Ref o2) {
> +						try {
> +							Object obj1 = getRepository().mapObject(o1.getObjectId(), o1.getName());
> +							Object obj2 = getRepository().mapObject(o2.getObjectId(), o2.getName());
> +							long t1 = timeof(obj1);
> +							long t2 = timeof(obj2);
> +							if (t1 > t2)
> +								return -1;
> +							if (t1 < t2)
> +								return 1;
> +							return 0;
> +						} catch (IOException e) {
> +							// ignore
> +							return 0;
> +						}
> +					}
> +					long timeof(Object o) {
> +						if (o instanceof Commit)
> +							return ((Commit)o).getCommitter().getWhen().getTime();
> +						if (o instanceof Tag)
> +							return ((Tag)o).getTagger().getWhen().getTime();
> +						return 0;

You are inside of a PlotWalk, which extends RevWalk, which has very
aggressive caching of RevCommit and RevTag data instances, and very
fast parsers.  Much faster than the legacy Commit and Tag classes.

I think we should use the RevCommit and RevTag classes to handle
sorting here.  RevCommit already has the committer time cached
and stored in an int.  RevCommit probably should learn to do the
same for its "tagger" field.  The tiny extra bloat (1 word) that
adds to a RevTag instance is worth it when we consider implementing
something like this and/or git-describe where sorting tags by their
dates is useful.


> +			tags = list.toArray(new Ref[list.size()]);

I wonder if using a Ref[] here even makes sense given that the data
is stored in a List<Ref>.  I use RevCommit[] inside of RevCommit
generally because the number of entries in the array is 1 or 2 and
the array is smaller than say an ArrayList.

In hindsight those RevCommit[] probably should be a List<RevCommit>
with different list implementations based on the number of parents
needed:

	0 parents:  Collections.emptyList()
	1 parent:   Collections.singletonList()
	2 parents:  some especialized AbstractList subclass
	3 parents:  ArrayList

I think it would actually use less memory per instance, which is
a huge bonus, but we'd pay a downcasting penalty on each access.
HotSpot is supposed to be really good at removing the downcast
penalty from say java.util.List, but I don't if it can beat a typed
array access.

Sorry I got off on a bit of a tangent here.  I'm just trying to
point out that the primary reason I've usd an array before is
probably moot here since the data is already in a List.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> index d7e4c58..41d57c6 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> @@ -53,6 +53,7 @@

IMHO this class shouldn't need to be modified.

> @@ -541,7 +542,7 @@ public RevTree lookupTree(final AnyObjectId id) {
>  	public RevCommit lookupCommit(final AnyObjectId id) {
>  		RevCommit c = (RevCommit) objects.get(id);
>  		if (c == null) {
> -			c = createCommit(id);
> +			c = createCommit(id, getTags(id));

This code is performance critical to commit parsing.  Trying to
lookup tags for every commit we evaluate, especially ones that we
will never show in the UI (because they are uninteresting) but that
we still need to parse in order to derive the merge base is just
a huge waste of time.

Same applies for the lookupAny and parseAny methods.

-- 
Shawn.

Re: [EGIT PATCH 4/6] Add tags to the graphical history display.

[Robin Rosenberg]

måndagen den 6 oktober 2008 10.08.34 skrev Shawn O. Pearce:
> FYI, my comments don't fully cover this patch yet.
> 
> > -	protected PlotCommit(final AnyObjectId id) {
> > +	protected PlotCommit(final AnyObjectId id, final Ref[] tags) {
> >  		super(id);
> > +		this.refs = tags;
> 
> this.refs isn't final.  There is no reason to be adding it to the
> constructor and having this ripple effect all the way up into the
> RevWalk class.
Eh, it doesn't, ecept I added a methos to RevWalk than can return tags,
which it doesn't unless the information is attached.

> Maybe PlotWalk should override next() and only set PlotCommit.refs on
> things returned from super.next()?  This way we only do tag lookup
> on commits that the filtering rules have said should be shown in
> to the application, but the refs should still be inserted prior to
> the application seeing the RevCommit.

I missed something here I think. Thanks. Maybe I thought... doesn't matter.
I'll rewrite the changes in and related to this completely.

> > @@ -54,6 +66,7 @@
> >  	public PlotWalk(final Repository repo) {
> >  		super(repo);
> >  		super.sort(RevSort.TOPO, true);
> > +		reverseRefMap = repo.getAllRefsByPeeledObjectId();
> 
> I wonder if this shouldn't be done as part of the StartGenerator
> (or something like it but specialized for PlotWalk).  I only say
> that because a reused PlotWalk may want to make sure its ref map
> is current with the repository its about to walk against.
noted.

> You are inside of a PlotWalk, which extends RevWalk, which has very
> aggressive caching of RevCommit and RevTag data instances, and very
> fast parsers.  Much faster than the legacy Commit and Tag classes.
Maybe we should rid us of them completely and make new commit from
these classes of possible more lightweight ones. The old classes were
straightforward but are on overtime now.

> I think we should use the RevCommit and RevTag classes to handle
> sorting here.  RevCommit already has the committer time cached
> and stored in an int.  RevCommit probably should learn to do the
> same for its "tagger" field.  The tiny extra bloat (1 word) that
> adds to a RevTag instance is worth it when we consider implementing
> something like this and/or git-describe where sorting tags by their
> dates is useful.

As you noted earlier we usually have at most one ref per commit to sort.
Not much to optimize for speed here with current repos. For a one-element 
list the compare routine wil not even be invoked.

> 
> > +			tags = list.toArray(new Ref[list.size()]);
> 
> I wonder if using a Ref[] here even makes sense given that the data
> is stored in a List<Ref>.  I use RevCommit[] inside of RevCommit
> generally because the number of entries in the array is 1 or 2 and
> the array is smaller than say an ArrayList.
> 
> In hindsight those RevCommit[] probably should be a List<RevCommit>
> with different list implementations based on the number of parents
> needed:
> 
> 	0 parents:  Collections.emptyList()
> 	1 parent:   Collections.singletonList()
> 	2 parents:  some especialized AbstractList subclass
> 	3 parents:  ArrayList
> 
> I think it would actually use less memory per instance, which is
> a huge bonus, but we'd pay a downcasting penalty on each access.
> HotSpot is supposed to be really good at removing the downcast
> penalty from say java.util.List, but I don't if it can beat a typed
> array access.
I guess measuring is the right way. For these small arrays, my bet
is that List is way faster because we do not need any bounds checking.
Hotspot is reasonably good at optimizing those away, but that won't
help for much small arrays.

> Sorry I got off on a bit of a tangent here.  I'm just trying to
> point out that the primary reason I've usd an array before is
> probably moot here since the data is already in a List.
Could it be that arrays are often better then lists.

> > diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> > index d7e4c58..41d57c6 100644
> > --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> > +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
> > @@ -53,6 +53,7 @@
> 
> IMHO this class shouldn't need to be modified.
> 
> > @@ -541,7 +542,7 @@ public RevTree lookupTree(final AnyObjectId id) {
> >  	public RevCommit lookupCommit(final AnyObjectId id) {
> >  		RevCommit c = (RevCommit) objects.get(id);
> >  		if (c == null) {
> > -			c = createCommit(id);
> > +			c = createCommit(id, getTags(id));
> 
> This code is performance critical to commit parsing.  Trying to
> lookup tags for every commit we evaluate, especially ones that we
> will never show in the UI (because they are uninteresting) but that
> we still need to parse in order to derive the merge base is just
> a huge waste of time.
> 
> Same applies for the lookupAny and parseAny methods.
> 
Ack.

-- robin

Re: [EGIT PATCH 4/6] Add tags to the graphical history display.

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> måndagen den 6 oktober 2008 10.08.34 skrev Shawn O. Pearce:
> > Maybe PlotWalk should override next() [...]
> 
> I missed something here I think. Thanks. Maybe I thought... doesn't matter.
> I'll rewrite the changes in and related to this completely.

OK, looking forward to the changes.
 
> > > @@ -54,6 +66,7 @@
> > >  	public PlotWalk(final Repository repo) {
> > >  		super(repo);
> > >  		super.sort(RevSort.TOPO, true);
> > > +		reverseRefMap = repo.getAllRefsByPeeledObjectId();
> > 
> > I wonder if this shouldn't be done as part of the StartGenerator
> > (or something like it but specialized for PlotWalk).  I only say
> > that because a reused PlotWalk may want to make sure its ref map
> > is current with the repository its about to walk against.
>
> noted.

Be careful.  StartGenerator is package access only and I think
PlotWalk is in a different package.

IMHO binding into the StartGenerator (by subclassing it and replacing
it) feels like the right thing to me, but it means making a bunch
of changes to RevWalk and StartGenerator to make that type public
and allow PlotWalk to inject a different subclass of StartGenerator
for itself.

The StartGenerator trick is very useful though; it allows the RevWalk
to perform init activity only on the first invocation of next()
and then removes the init code entirely from the call path.  It is a
virtual dispatch, but I figured its a virtual dispatch anyway due to
the way the other generators are chained together based the filters
so we at least avoid a "if (first) {...}" test on every call to next.
 
> > You are inside of a PlotWalk, which extends RevWalk, which has very
> > aggressive caching of RevCommit and RevTag data instances, and very
> > fast parsers.  Much faster than the legacy Commit and Tag classes.
>
> Maybe we should rid us of them completely and make new commit from
> these classes of possible more lightweight ones. The old classes were
> straightforward but are on overtime now.

I'm trying to go in that direction.  However there are three reasons
why we aren't ready yet:

- Commits can only be made by the old-style Commit class.
- Tags can only be made by the old-style Tag class.
- Trees can only be made by the old-style Tree class.

I have some experimental code in my merge branch to solve the last
one by teaching DirCache how to write out the index and update the
cache trees with the ObjectIds of the newly created trees, but I
have not had a chance to clean the patches up with test cases and
docs to submit to the git ML yet.

So I'm actively working on fixing the last item.

Oh, and of course existing callers have to be converted.  But I am
trying to avoid introducing new callers.
 
> As you noted earlier we usually have at most one ref per commit to sort.
> Not much to optimize for speed here with current repos. For a one-element 
> list the compare routine wil not even be invoked.

Oh, good point.
 
> > In hindsight those RevCommit[] probably should be a List<RevCommit>
> > with different list implementations based on the number of parents
> > needed:
> > 
> > 	0 parents:  Collections.emptyList()
> > 	1 parent:   Collections.singletonList()
> > 	2 parents:  some especialized AbstractList subclass
> > 	3 parents:  ArrayList
>
> I guess measuring is the right way. For these small arrays, my bet
> is that List is way faster because we do not need any bounds checking.
> Hotspot is reasonably good at optimizing those away, but that won't
> help for much small arrays.

Yup.  Its something to explore.  But I lack the time right now so
I'm going going to be making any changes and measuring it anytime
soon.  ;-)

Maybe someone who wants to get involved can look at this.  Or any
other number of issues we still have open.

But without measurements to back up the code either way I won't be
making changes to what we have right now.
 
-- 
Shawn.

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

[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?

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

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

[Robin Rosenberg]

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

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

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> måndagen den 6 oktober 2008 10.15.54 skrev Shawn O. Pearce:
> > Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > >  
> > >  	/**
> > > +	 * @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.

java.util.Set?  java.util.HashSet?

What am I missing?
 
> > > +		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.

I wasn't concerned about performance, I was going for readability.
This loop is probably not performance critical as its done once
early when the PlotWalk starts.  I just found that grabbing the
entrySet when all you care about is the values was odd.
 
> > 	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. 

I'm probably too used to this pattern of "put, then test" because
I use it a lot when the odds of put returning null are very high.
Its an odd idiom.  I think most developers wouldn't write it.
So I guess a comment of some sort is probably a good idea if you
chose to use this mess.  ;-)

-- 
Shawn.

Re: [EGIT PATCH 2/6] Peel annotated tags when getting all refs

[Shawn O. Pearce]

FWIW I'm still going through the series.  But this jumped out at me.

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> For packed refs we got this automatically from packed-refs,
> but for loose tags we have to follow the tags and get the leaf
> object in order to comply with the documentation.
...
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> index 5a1b85f..1ee70d9 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> @@ -271,7 +271,16 @@ private void readOneLooseRef(final Map<String, Ref> avail,
>  					return;
>  				}
>  
> -				ref = new Ref(Ref.Storage.LOOSE, origName, refName, id);
> +				Object tt = db.mapObject(id, refName);
> +				if (tt != null && tt instanceof Tag) {
> +					Tag t = (Tag)tt;

Owwww.

I don't want to be doing peeling of loose annotated tags anytime
we loop through the loose objects.  That is just painful and very
expensive.  I'd rather the peeling be caused on demand by callers
who need it, but if its done through a method on Repository then
we can push the peeled ObjectId back into the RefDatabase cache.

I'm thinking more like:

	Repository db = ...;
	...
	ObjectId p = ref.getPeeledObjectId()
	if (p == null)
		p = db.peel(ref)

-- 
Shawn.