Re: [PATCH] builtin-branch: improve output when displaying remote branches

From: Jay Soffian <jaysoffian@gmail.com>
Date: Feb 12, 2009, 04:30 UTC
Message-ID: <76718490902112030x46bddd84r561705a6b4f9d4b4@mail.gmail.com>
In-Reply-To: <7vskmkqpp4.fsf@gitster.siamese.dyndns.org>

On Wed, Feb 11, 2009 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> $ git branch -rv
>>   origin/HEAD -> master
>>   origin/html           6116912 Autogenerated HTML docs for v1.6.2-rc0-10-gf6b9
>
> Doesn't the misalignment between the above two bother you?

This comment makes me sad. In fact, a previous iteration looked like this:

$ git branch -rv
 origin/HEAD -> master
 origin/html   6116912 Autogenerated HTML docs for v1.6.2-rc0-10-gf6b9
 origin/maint  7e1100e gitweb: add $prevent_xss option to prevent XSS
by repository content
 origin/man    67cb1a7 Autogenerated manpages for v1.6.2-rc0-10-gf6b9
 origin/master f6b98e4 git-web--browse: Fix check for /bin/start
 origin/next   417ce12 Merge branch 'master' into next
 origin/pu     9d798e7 Merge branch 'db/foreign-scm' into pu
 origin/todo   5ed7079 What's in update

IOW, align based on the width of the branch name, completely ignoring the width of " -> ...". But I found that ugly. It was actually more work to get it the way it is.

Show 18 quoted lines

>> diff --git a/builtin-branch.c b/builtin-branch.c
>> index 56a1971..03ad757 100644
>> --- a/builtin-branch.c
>> +++ b/builtin-branch.c
>> @@ -181,7 +181,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
>> +static char *resolve_remote_head_symref(const char *head_name) {
>> +     unsigned char sha1[20];
>> +     int flag;
>> +     const char *refname;
>> +     refname = resolve_ref(head_name, sha1, 0, &flag);
>> +     if (refname && (flag & REF_ISSYMREF) &&
>> +         !prefixcmp(refname, "refs/remotes/"))
>> +             return xstrdup(refname + strlen(head_name) - 4);
>
> Here, head_name is like "refs/remotes/frotz/HEAD", and you are assuming
> that resolved refname begins with "refs/remotes/frotz/" without checking
> the "frotz" part. It may point at "refs/remotes/x/y" in a misconfigured
> repository and your xstrdup() just ran past the end of the string.

Indeed. Now I'm doubly-sad, that my code sucks so bad. :-(

Show 26 quoted lines

> If the ref you feed to this function turns out not to be a symbolic ref,
> the caller does do the right thing.  It makes wonder if your caller should
> always call this, so that you would still work sensibly even if the tracking
> hierarchy has a funny symref refs/remotes/origin/TAIL that is not HEAD.
>
> The caller is currently this dense code.
>
>> +     newitem->len = strlen(newitem->name);
>> +     newitem->dest = (newitem->kind == REF_REMOTE_BRANCH &&
>> +                      newitem->len > 5 &&
>> +                      !strcmp(newitem->name + newitem->len - 5, "/HEAD"))
>> +                     ? resolve_remote_head_symref(refname - 13) : NULL;
>> +     /* adjust for " -> " */
>> +     if (newitem->dest)
>> +             newitem->len += strlen(newitem->dest) + 4;
>
> It can become something like:
>
>        if (newitem->kind == REF_REMOTE_BRANCH)
>                newitem->dest = resolve_remote_symref(refname - 13);
>        else
>                newitem->dest = NULL;
>        if (newitem->dest)
>                ...
>
> no?

Yes indeed. I'll re-roll to to clean this up, but I'm keeping the visual output the same unless you really don't like it.

> free(NULL) is Ok; omit the extra check.

Got it. I think I did something similar in the builtin-remote patch I sent you earlier, so I'll make sure to fix that there too when I re-roll that one.

Thanks for the review.

j.