Re: [PATCH v9 19/20] branch: use ref-filter printing APIs
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Dec 28, 2016, 09:40 UTC
- Message-ID
- <CAOLa=ZTPMbhzv_9mLv63UvPu__JqwQrpd2d8ogXuz5wNKb70Dw@mail.gmail.com>
- In-Reply-To
- <xmqqinq56phu.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 28, 2016 at 2:47 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 43 quoted lines
> 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