Re: [PATCH] color.h: document and modernize header

From: Jeff King <peff@peff.net>
Date: Feb 8, 2018, 20:43 UTC
Message-ID: <20180208204309.GA4541@sigill.intra.peff.net>
In-Reply-To: <20180208201546.194935-1-sbeller@google.com>

On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:

Show 15 quoted lines

>  int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
>  {
>  	va_list args;
> diff --git a/color.h b/color.h
> index fd2b688dfb..8c7e6c41c2 100644
> --- a/color.h
> +++ b/color.h
> @@ -72,26 +72,50 @@ extern int color_stdout_is_tty;
>   * Use the first one if you need only color config; the second is a convenience
>   * if you are just going to change to git_default_config, too.
>   */
> -int git_color_config(const char *var, const char *value, void *cb);
> -int git_color_default_config(const char *var, const char *value, void *cb);
> +extern int git_color_config(const char *var, const char *value, void *cb);
> +extern int git_color_default_config(const char *var, const char *value, void *cb);

Hmph, I thought we weren't adding "extern" everywhere. See:

  https://public-inbox.org/git/xmqq8tea5hxi.fsf@gitster.mtv.corp.google.com/

Other than that, these changes mostly look like improvements. A few comments:

Show 5 quoted lines

> +/*
> + * Resolve the constants as returned by git_config_colorbool()
> + * (specifically "auto") to a boolean answer.
> + */
> +extern int want_color(int var);

This explanation left me even more confused about what should go in "var", and I think I'm the one who wrote the function. ;)

I think the point is that "var" is a quad-state variable (yes, no, auto, or "unknown") and we are converting to a boolean. This would probably be a lot more clear if GIT_COLOR_* were all enum values and not #defines, and this function took the matching enum type.

So I think that's what you were trying to name with "constants as returned by...", but it definitely took me some thinking to parse it.

Show 8 quoted lines

> +/*
> + * Translate a Git color from 'value' into a string that the terminal can
> + * interpret and store it into 'dst'. The Git color values are of the form
> + * "foreground [background] [attr]" where fore- and background can be a color
> + * name ("red"), a RGB code (#0xFF0000) or a 256-color-mode from the terminal.
> + */
> +extern int color_parse(const char *value, char *dst);
> +extern int color_parse_mem(const char *value, int len, char *dst);

The inputs here are called "value" mostly because we feed them from the var/value pair of config. But maybe "colorspec", or even just "src" would be more clear than "value".

Show 7 quoted lines

> +/*
> + * Output the formatted string in the specified color (and then reset to normal
> + * color so subsequent output is uncolored). Omits the color encapsulation if
> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
> + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
> + * instead, up to its first NUL character.
> + */

It probably doesn't matter much in practice, but the color_print_strbuf behavior sounds like a bug. Shouldn't it print the whole strbuf, even if it has an embedded NUL?

Show 5 quoted lines

> +/*
> + * Check if the given color is GIT_COLOR_NIL that means "no color selected".
> + * The caller needs to replace the color with the actual desired color.
> + */
> +extern int color_is_nil(const char *color);

Is this a parsed color string, or a human-readable source? I think consistent naming of the two variables would help (using a type doesn't work since they're both "const char *").

Show 13 quoted lines

> diff --git a/grep.c b/grep.c
> index 3d7cd0e96f..834b8eb439 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -18,6 +18,11 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size)
>  	fwrite(buf, size, 1, stdout);
>  }
>  
> +static void color_set(char *dst, const char *color_bytes)
> +{
> +	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
> +}
> +

This part seems OK. I think we made color_set() globally available with the notion that other callers might make use of it. But it's pretty horrid as public interfaces go, requiring that the caller has created a buffer of the appropriate size. We'd do better to have a "struct color" with the correctly-sized buffer. But at this point I don't think it's worth overhauling the color code.

Those are all suggestions. Given that there's no documentation currently on most of these, I think even if you don't take any of my suggestions, this would still be a net improvement (modulo the "extern" thing).

-Peff