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

On Wed, Dec 28, 2016 at 2:47 AM, Junio C Hamano [off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

 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