Re: [RFC] Colorization of log --graph

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2009, 20:13 UTC
Message-ID: <7vbprwgdgc.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <b2e43f8f0903201213o396de6c0sb52149ed1d889d1@mail.gmail.com>

Allan Caffee <allan.caffee@gmail.com> writes:

Show 22 quoted lines

> On Thu, Mar 19, 2009 at 5:48 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
>> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>
>>> I'd start like this:
>>>
>>>       enum color_name {
>>>               COLOR_RESET,
>>>               COLOR_RED,
>>>               COLOR_GREEN,
>>>               COLOR_YELLOW,
>>>               COLOR_BLUE,
>>>               COLOR_MAGENTA,
>>>               COLOR_CYAN,
>>>               COLOR_WHITE
>>>       };
>>
>> Looking for "COLOR_RED" in the archive gives:
>>
>>  http://article.gmane.org/gmane.comp.version-control.git/109676
>>
>
> Duly noted.  Perhaps those #defines should be relocated to color.h?

Heh, I did not even realize the above 109676 was referring to what I wrote sometime ago.

Show 43 quoted lines

> If we still wanted to provide a color_name type we could use
> GIT_COLOR_NAME_RESET et al.  That would give us something like:
>
> #define GIT_COLOR_NORMAL	""
> #define GIT_COLOR_RESET		"\033[m"
> #define GIT_COLOR_BOLD		"\033[1m"
> #define GIT_COLOR_RED		"\033[31m"
> #define GIT_COLOR_GREEN		"\033[32m"
> #define GIT_COLOR_YELLOW	"\033[33m"
> #define GIT_COLOR_BLUE		"\033[34m"
> #define GIT_COLOR_CYAN		"\033[36m"
> #define GIT_COLOR_BG_RED	"\033[41m"
>
> enum color_name {
> 	GIT_COLOR_NAME_NORMAL
> 	GIT_COLOR_NAME_RESET,
> 	GIT_COLOR_NAME_RED,
> 	GIT_COLOR_NAME_GREEN,
> 	GIT_COLOR_NAME_YELLOW,
> 	GIT_COLOR_NAME_BLUE,
> 	GIT_COLOR_NAME_MAGENTA,
> 	GIT_COLOR_NAME_CYAN,
> 	GIT_COLOR_NAME_WHITE
> 	GIT_COLOR_NAME_BG_RED
> };
>
> /*
>  * Map names to ANSI escape sequences.  Consider putting this in color.c
>  * and providing color_name_get_ansi_code(enum color_name).
>  */
> const char* git_color_codes[] {
> 	GIT_COLOR_RESET,
> 	GIT_COLOR_BOLD,
> 	GIT_COLOR_RED,
> 	GIT_COLOR_GREEN,
> 	GIT_COLOR_YELLOW,
> 	GIT_COLOR_BLUE,
> 	GIT_COLOR_CYAN,
> 	GIT_COLOR_BG_RED,
> };
>
> That conveniently offers clients access to both the raw escape codes and
> a clear type for storing/handling colors.

Is git_color_codes[GIT_COLOR_NAME_FOO] supposed to give you GIT_COLOR_FOO?

Are you consolidating various pieces of physical color definition to one place? That sounds sensible.

The corrent code does:

diff.c::
	user says "meta" is "purple"
        -> parse_diff_color_slot() says "meta" is slot 2
        -> git_diff_basic_config() asks color_parse() to place the ANSI
           representation of the "purple" in slot 2
	-> code uses diff_get_color() to grab "meta" color from the slot
           and sends it to the terminal

builtin-branch.c duplicates the exact same logic with a separate tables and a set of slots. builtin-grep.c cheats and does not give the end user any customizability, which needs to be fixed.

The "slots" are defined in terms of what the color is used for, the meaning, e.g. "a line from the file before the change (DIFF_FILE_OLD)"; we cannot avoid having application specific set of slots, but the parsing should be able to share the code.

Once the slot number is known, we ask color_parse() to put the final physical string (suitable for the terminal's consumption) to fill the slot. But for that, I do not think git_color_codes[] nor GIT_COLOR_FOO need to be exposed to the applications (i.e. "diff", "branch", "grep"). It is an implementation detail that color_parse() always uses ANSI escape sequences right now, but we could encapsulate that in color.c and later perhaps start looking up from the terminfo database, for example.

But that leaves the question of initialization. I think it would give a better abstraction if we changed the type of values stored in a color-table like diff.c::diff_colors[] from physical string sent to the terminal to a color name (your enum color_name). Then the application code can initialize their own color-table for each application-specific slots with GIT_COLOR_NAME_RED, let the configuration mechanism to customize it for the user. The codepath that currently assume the color table contains strings that can be sent to the terminal need to be modified to ask color_code_to_terminal_string(GIT_COLOR_NAME_YELLOW) or something. Which means:

(1) Physical color representation should be known only to color.c.  I.e.

	#define GIT_COLOR_BOLD "\033[1m"

    does not belong to color.h (public header for application consumption)
    nor diff.c (application);

(2) Logical color name and the ways to convert it for terminal consumption
    belongs to color.h.  I.e.

	enum color_name {
        	GIT_COLOR_NAME_YELLOW,
                ...
	}

    should go to color.h;

    color_fprintf() should be changed to take "enum color_name color"
    instead of "const char *color";

    We would need strbuf variant for callers that prepare the string in
    core before giving it to fprintf().

(3) "static const char *git_color_codes[]" would be an implementation
    detail of the current "ANSI-only" one, hidden inside color.c, for
    color_fprintf() and its strbuf cousin to look at.