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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Aug 18, 2008, 14:20 UTC
Message-ID: <20080818142039.GA9572@spearce.org>
In-Reply-To: <48A97C23.8030103@gmail.com>

Marek Zawirski <marek.zawirski@gmail.com> wrote:

Show 18 quoted lines

> 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 + '/').

Show 5 quoted lines

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