[JGIT PATCH 0/3] jgit: Verbase branch command

[JGIT PATCH 0/3] jgit: Verbase branch command

[Charles O'Farrell]

Verbose branch listing added for the sake of completeness

Charles O'Farrell (3):
  Extract RefComparator to sort collection of Refs
  Cleanup of Branch command ready for verbose mode
  Verbose branch command

 .../src/org/spearce/jgit/pgm/Branch.java           |   60 +++++++++++++----
 .../src/org/spearce/jgit/lib/RefComparator.java    |   72 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/RefWriter.java        |   14 +----
 3 files changed, 120 insertions(+), 26 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/lib/RefComparator.java

[JGIT PATCH 1/3] Extract RefComparator to sort collection of Refs

[Charles O'Farrell]

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/lib/RefComparator.java    |   72 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/RefWriter.java        |   14 +----
 2 files changed, 73 insertions(+), 13 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/lib/RefComparator.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefComparator.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefComparator.java
new file mode 100644
index 0000000..6d994eb
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefComparator.java
@@ -0,0 +1,72 @@
+/**
+ * Copyright (C) 2008, Charles O'Farrell <charleso@charleso.org>
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.lib;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+
+/**
+ * Util for sorting (or comparing) Ref instances by name.
+ * <p>
+ * Useful for command line tools or writing out refs to file.
+ */
+public class RefComparator implements Comparator<Ref> {
+
+	/** Singleton instance of RefComparator */
+	public static final RefComparator INSTANCE = new RefComparator();
+
+	public int compare(Ref o1, Ref o2) {
+		return o1.getName().compareTo(o2.getName());
+	}
+
+	/**
+	 * Sorts the collection of refs, returning a new collection.
+	 * 
+	 * @param refs
+	 *            collection to be sorted
+	 * @return sorted collection of refs
+	 */
+	public static Collection<Ref> sort(final Collection<Ref> refs) {
+		final List<Ref> r = new ArrayList<Ref>(refs);
+		Collections.sort(r, INSTANCE);
+		return r;
+	}
+}
\ No newline at end of file
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefWriter.java
index 9c784d5..2d39713 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefWriter.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefWriter.java
@@ -41,8 +41,6 @@
 import java.io.IOException;
 import java.io.StringWriter;
 import java.util.Collection;
-import java.util.Comparator;
-import java.util.TreeSet;
 
 /**
  * Writes out refs to the {@link Constants#INFO_REFS} and
@@ -61,8 +59,7 @@
 	 *            by applying updates to the advertised refs already discovered.
 	 */
 	public RefWriter(Collection<Ref> refs) {
-		this.refs = new TreeSet<Ref>(RefComparator.INSTANCE);
-		this.refs.addAll(refs);
+		this.refs = RefComparator.sort(refs);
 	}
 
 	/**
@@ -163,13 +160,4 @@ public void writePackedRefs() throws IOException {
 	 */
 	protected abstract void writeFile(String file, byte[] content)
 			throws IOException;
-
-	private static class RefComparator implements Comparator<Ref> {
-
-		private static final RefComparator INSTANCE = new RefComparator();
-
-		public int compare(Ref o1, Ref o2) {
-			return o1.getName().compareTo(o2.getName());
-		}
-	}
 }
-- 
1.6.0.2.g2ebc0

[JGIT PATCH 2/3] Cleanup of Branch command ready for verbose mode

[Charles O'Farrell]

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/pgm/Branch.java           |   30 ++++++++++++++-----
 1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
index c89f510..9141be0 100644
--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
@@ -39,15 +39,17 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.TreeSet;
+import java.util.Map.Entry;
 
 import org.kohsuke.args4j.Argument;
 import org.kohsuke.args4j.Option;
 import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
 import org.spearce.jgit.lib.Ref;
+import org.spearce.jgit.lib.RefComparator;
 import org.spearce.jgit.lib.RefUpdate;
 import org.spearce.jgit.lib.RefUpdate.Result;
 
@@ -72,6 +74,8 @@
 	@Argument
 	private List<String> branches = new ArrayList<String>();
 
+	private Map<String, Ref> printRefs = new LinkedHashMap<String, Ref>();
+
 	@Override
 	protected void run() throws Exception {
 		if (delete || deleteForce)
@@ -87,17 +91,27 @@ private void list() {
 		if (head != null) {
 			String current = head.getName();
 			if (current.equals(Constants.HEAD))
-				printHead("(no branch)", true);
-			for (String ref : new TreeSet<String>(refs.keySet())) {
-				if (isHead(ref))
-					printHead(ref, current.equals(ref));
+				addRef("(no branch)", head);
+			addRefs(refs, Constants.HEADS_PREFIX, !remote);
+			addRefs(refs, Constants.REMOTES_PREFIX, remote);
+			for (Entry<String, Ref> e : printRefs.entrySet()) {
+				printHead(e.getKey(), current.equals(e.getValue().getName()));
+			}
+		}
+	}
+
+	private void addRefs(Map<String, Ref> allRefs, String prefix, boolean add) {
+		if (all || add) {
+			for (Ref ref : RefComparator.sort(allRefs.values())) {
+				String name = ref.getName();
+				if (name.startsWith(prefix))
+					addRef(name, ref);
 			}
 		}
 	}
 
-	private boolean isHead(String key) {
-		return (all || !remote) && key.startsWith(Constants.HEADS_PREFIX)
-				|| (all || remote) && key.startsWith(Constants.REMOTES_PREFIX);
+	private void addRef(String name, Ref ref) {
+		printRefs.put(name, ref);
 	}
 
 	private void printHead(String ref, boolean isCurrent) {
-- 
1.6.0.2.g2ebc0

[JGIT PATCH 3/3] Verbose branch command

[Charles O'Farrell]

Signed-off-by: Charles O'Farrell <charleso@charleso.org>
---
 .../src/org/spearce/jgit/pgm/Branch.java           |   34 +++++++++++++++----
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
index 9141be0..ba8ffde 100644
--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
@@ -52,6 +52,7 @@
 import org.spearce.jgit.lib.RefComparator;
 import org.spearce.jgit.lib.RefUpdate;
 import org.spearce.jgit.lib.RefUpdate.Result;
+import org.spearce.jgit.revwalk.RevWalk;
 
 @Command(common = true, usage = "List, create, or delete branches")
 class Branch extends TextBuiltin {
@@ -76,15 +77,23 @@
 
 	private Map<String, Ref> printRefs = new LinkedHashMap<String, Ref>();
 
+	/** Only set for verbose branch listing at-the-moment */
+	private RevWalk rw;
+
+	private int maxNameLength;
+
 	@Override
 	protected void run() throws Exception {
 		if (delete || deleteForce)
 			delete(deleteForce);
-		else
+		else {
+			if (verbose)
+				rw = new RevWalk(db);
 			list();
+		}
 	}
 
-	private void list() {
+	private void list() throws Exception {
 		Map<String, Ref> refs = db.getAllRefs();
 		Ref head = refs.get(Constants.HEAD);
 		// This can happen if HEAD is stillborn
@@ -95,7 +104,8 @@ private void list() {
 			addRefs(refs, Constants.HEADS_PREFIX, !remote);
 			addRefs(refs, Constants.REMOTES_PREFIX, remote);
 			for (Entry<String, Ref> e : printRefs.entrySet()) {
-				printHead(e.getKey(), current.equals(e.getValue().getName()));
+				Ref ref = e.getValue();
+				printHead(e.getKey(), current.equals(ref.getName()), ref);
 			}
 		}
 	}
@@ -105,20 +115,30 @@ private void addRefs(Map<String, Ref> allRefs, String prefix, boolean add) {
 			for (Ref ref : RefComparator.sort(allRefs.values())) {
 				String name = ref.getName();
 				if (name.startsWith(prefix))
-					addRef(name, ref);
+					addRef(name.substring(name.indexOf('/', 5) + 1), ref);
 			}
 		}
 	}
 
 	private void addRef(String name, Ref ref) {
 		printRefs.put(name, ref);
+		maxNameLength = Math.max(maxNameLength, name.length());
 	}
 
-	private void printHead(String ref, boolean isCurrent) {
+	private void printHead(String ref, boolean isCurrent, Ref refObj)
+			throws Exception {
 		out.print(isCurrent ? '*' : ' ');
 		out.print(' ');
-		ref = ref.substring(ref.indexOf('/', 5) + 1);
-		out.println(ref);
+		out.print(ref);
+		if (verbose) {
+			int spaces = maxNameLength - ref.length() + 1;
+			out.print(String.format("%" + spaces + "s", ""));
+			ObjectId objectId = refObj.getObjectId();
+			out.print(objectId.toString().substring(0, 7));
+			out.print(' ');
+			out.print(rw.parseCommit(objectId).getShortMessage());
+		}
+		out.println();
 	}
 
 	private void delete(boolean force) throws IOException {
-- 
1.6.0.2.g2ebc0

Re: [JGIT PATCH 2/3] Cleanup of Branch command ready for verbose mode

[Marek Zawirski]

Charles O'Farrell wrote:
> diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
(...)
> @@ -87,17 +91,27 @@ private void list() {
>  		if (head != null) {
>  			String current = head.getName();
>  			if (current.equals(Constants.HEAD))
> -				printHead("(no branch)", true);
> -			for (String ref : new TreeSet<String>(refs.keySet())) {
> -				if (isHead(ref))
> -					printHead(ref, current.equals(ref));
> +				addRef("(no branch)", head);
> +			addRefs(refs, Constants.HEADS_PREFIX, !remote);
> +			addRefs(refs, Constants.REMOTES_PREFIX, remote);

We used to use (Constants.HEADS_PREFIX + '/') or 
(Constants.REMOTES_PREFIX + '/') in such places, probably to handle 
correctly jokes like refs named "refs/remotes_will_broke_your_code".

I've seen this expression so many times that I think it's right moment 
to create another Constants.HEADS_PREFIX_SLASHED (same for tags, 
remotes) or similar as this piece of this code is redundant in many 
places. But wait, does anybody use pure ones without slashes? Maybe we 
can just change existing constants.

Beside of that detail, me (being just jgit developer) says that the 
series looks good. Oh, and it's probably good to follow convention of 
marking variables as final when they are final.

-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

Re: [JGIT PATCH 2/3] Cleanup of Branch command ready for verbose mode

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> Charles O'Farrell wrote:
>> diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Branch.java
> (...)
>> @@ -87,17 +91,27 @@ private void list() {
>>  		if (head != null) {
>>  			String current = head.getName();
>>  			if (current.equals(Constants.HEAD))
>> -				printHead("(no branch)", true);
>> -			for (String ref : new TreeSet<String>(refs.keySet())) {
>> -				if (isHead(ref))
>> -					printHead(ref, current.equals(ref));
>> +				addRef("(no branch)", head);
>> +			addRefs(refs, Constants.HEADS_PREFIX, !remote);
>> +			addRefs(refs, Constants.REMOTES_PREFIX, remote);
>
> We used to use (Constants.HEADS_PREFIX + '/') or  
> (Constants.REMOTES_PREFIX + '/') in such places, probably to handle  
> correctly jokes like refs named "refs/remotes_will_broke_your_code".

Yup.  I hate those constants because they are almost useless.
Almost anytime we need them, we really need to use instead
(Constants.FOO_PREFIX + '/').

> I've seen this expression so many times that I think it's right moment  
> to create another Constants.HEADS_PREFIX_SLASHED (same for tags,  
> remotes) or similar as this piece of this code is redundant in many  
> places. But wait, does anybody use pure ones without slashes? Maybe we  
> can just change existing constants.

Right.  It would be a nice cleanup to go through the existing users
and see if we cannot change the meaning of these.  But that's a
lot of code to change because you also have to delete the +'/'
we have everywhere.

> Beside of that detail, me (being just jgit developer) says that the  
> series looks good. Oh, and it's probably good to follow convention of  
> marking variables as final when they are final.

Yea, the series does look nice, except the prefix matching bug.

-- 
Shawn.