Re: [PATCH v9 19/20] branch: use ref-filter printing APIs

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

On Wed, Dec 28, 2016 at 2:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>  static char branch_colors[][COLOR_MAXLEN] = {
>> -     GIT_COLOR_RESET,
>> -     GIT_COLOR_NORMAL,       /* PLAIN */
>> -     GIT_COLOR_RED,          /* REMOTE */
>> -     GIT_COLOR_NORMAL,       /* LOCAL */
>> -     GIT_COLOR_GREEN,        /* CURRENT */
>> -     GIT_COLOR_BLUE,         /* UPSTREAM */
>> +     "%(color:reset)",
>> +     "%(color:reset)",       /* PLAIN */
>> +     "%(color:red)",         /* REMOTE */
>> +     "%(color:reset)",       /* LOCAL */
>> +     "%(color:green)",       /* CURRENT */
>> +     "%(color:blue)",        /* UPSTREAM */
>>  };
>
> The contents of this table is no longer tied to COLOR_MAXLEN.  The
> above entries used by default happen to be shorter, but it is
> misleading.
>
>         static const char *branch_colors[] = {
>         "%(color:reset)",
>         ...
>         };
>
> perhaps?
>
> More importantly, does this re-definition of the branch_colors[]
> work with custom colors filled in git_branch_config() by calling
> e.g. color_parse("red", branch_colors[BRANCH_COLOR_REMOTE]), where
> "red" and "remote" come from an existing configuration file?
>
>         [color "branch"]
>                 remote = red
>
> It obviously would not work if you changed the type of branch_colors[]
> because the color_parse() wants the caller to have allocated space
> of COLOR_MAXLEN.
>
> But if filling these ANSI escape sequence into the format works OK,
> then doesn't it tell us that we do not need to have this change to
> use "%(color:reset)" etc. as the new default values?

Good point, this would overwrite the existing configuration based setup
existing in builtin/branch.c.

I think it'd make sense to use the existing branch_colors[] definition without
any changes. That's mean that instead of using %(color:<color>). We hard
code the colors by calling  branch_get_color(). This is ok with me since,
users who which to have their own formats will anyways use --format option.

-- 
Regards,
Karthik Nayak