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

[Jay Soffian <jaysoffian@gmail.com>]

On Fri, Feb 13, 2009 at 1:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> +     if (prefix && !prefixcmp(dst, prefix))
>> +             return xstrdup(skip_prefix(dst, prefix));
>> +     else
>> +             return xstrdup(dst);
>> +}
>
> I wonder modern compilers are clever enough to optimze the above to
> something more like:
>
>        pfxlen = prefix ? strlen(prefix) : 0;
>        if (pfxlen && !strncmp(dst, prefix, pfxlen))
>                return xstrdup(dst + pfxlen);
>        else
>                return xstrdup(dst);
>
> given that skip_prefix is an inline function but prefixcmp is not
> (anymore), perhaps not.
>
>>  static int append_ref(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
>>  {
>>       struct ref_list *ref_list = (struct ref_list*)(cb_data);
>>       struct ref_item *newitem;
>>       struct commit *commit;
>>       int kind;
>> -     int len;
>> +     const char *prefix, *orig_refname = refname;
>>
>>       /* Detect kind */
>>       if (!prefixcmp(refname, "refs/heads/")) {
>>               kind = REF_LOCAL_BRANCH;
>>               refname += 11;
>> +             prefix = "refs/heads/";
>>       } else if (!prefixcmp(refname, "refs/remotes/")) {
>>               kind = REF_REMOTE_BRANCH;
>>               refname += 13;
>> +             prefix = "refs/remotes/";
>>       } else
>>               return 0;
>
> Once you start making each case arm do more things, it might make sense to
> rewrite the above unrolled loop into something like this:
>
>        static struct {
>                int kind;
>                const char *prefix;
>                int pfxlen;
>        } ref_kind[] = {
>                { REF_LOCAL_BRANCH, "refs/heads/", 11 },
>                { REF_REMOTE_BRANCH, "refs/remotes/", 13 },
>        };
>
>        for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
>                prefix = ref_kind[i].prefix;
>                if (strncmp(refname, prefix, ref_kind[i].pfxlen))
>                        continue;
>                kind = ref_kind[i].kind;
>                refname += ref_kind[i].pfxlen;
>                break;
>        }
>        if (ARRAY_SIZE(ref_kind) <= i)
>                return 0;
>
> Then we can later add new elements more easily, e.g.
>
>                { REF_TOPGIT_BASE, "refs/top-base/", 14 },
> ;-)

This strikes me as premature optimization. We're just emitting a few
branch names here. I'm beginning to lose my motivation to keep working
on this patch. I just wanted to improve the UI slightly. :-(

j.