Re: [PATCH 04/10] branch: roll show_detached HEAD into regular ref_list

[Karthik Nayak <karthik.188@gmail.com>]

On Tue, Aug 11, 2015 at 8:11 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Remove show_detached() and make detached HEAD to be rolled into
>> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
>> supporting the same in append_ref(). This eliminates the need for an
>> extra function and helps in easier porting of branch.c to use
>> ref-filter APIs.
>>
>> Before show_detached() used to check if the HEAD branch satisfies the
>> '--contains' option, now that is taken care by append_ref().
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 65f6d0d..81815c9 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -535,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>         int color;
>>         struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>>         const char *prefix = "";
>> +       const char *desc = item->name;
>>
>>         if (item->ignore)
>>                 return;
>> @@ -547,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>                 color = BRANCH_COLOR_REMOTE;
>>                 prefix = remote_prefix;
>>                 break;
>> +       case REF_DETACHED_HEAD:
>> +               color = BRANCH_COLOR_CURRENT;
>> +               desc = get_head_description();
>> +               break;
>>         default:
>>                 color = BRANCH_COLOR_PLAIN;
>>                 break;
>> @@ -558,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>                 color = BRANCH_COLOR_CURRENT;
>>         }
>>
>> -       strbuf_addf(&name, "%s%s", prefix, item->name);
>> +       strbuf_addf(&name, "%s%s", prefix, desc);
>>         if (verbose) {
>>                 int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
>>                 strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
>> @@ -581,6 +591,8 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>         }
>>         strbuf_release(&name);
>>         strbuf_release(&out);
>> +       if (item->kind == REF_DETACHED_HEAD)
>> +               free((void *)desc);
>
> This would be cleaner, and more easily extended to other cases if you
> used a 'to_free' variable. For instance, something like this:
>
>     const char *desc = item->name;
>     char *to_free = NULL;
>     ...
>     case REF_DETACHED_HEAD:
>         ...
>         desc = to_free = get_head_description();
>         break;
>     ...
>     strbuf_addf(&name, "%s%s", prefix, desc);
>     ...
>     free(to_free);
>

This looks neater!

> Note that it's safe to free 'to_free' when it's NULL, so you don't
> need to protect the free() with that ugly specialized 'if' which
> checks for REF_DETACHED_HEAD. You can also do away with the "cast to
> non-const" when freeing.
>

Yea makes sense will change to what you suggested.

>>  }
>> @@ -642,7 +638,14 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>>         cb.ref_list = &ref_list;
>>         cb.pattern = pattern;
>>         cb.ret = 0;
>> +       /*
>> +        * First we obtain all regular branch refs and then if the
>
> s/then//
>

Thanks

-- 
Regards,
Karthik Nayak