Marek Zawirski <marek.zawirski@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
