On Wed, Dec 28, 2016 at 2:47 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Karthik Nayak <karthik....@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
