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

["Shawn O. Pearce" <spearce@spearce.org>]

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.