On Wed, Feb 11, 2009 at 10:49 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jay Soffian <jaysoffian@xxxxxxxxx> 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.
>> 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. :-(
> 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.
--
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
