Re: [PATCH] color.h: document and modernize header
- Date: Mon, 12 Feb 2018 17:14:12 -0500
- From: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
- Subject: Re: [PATCH] color.h: document and modernize header
On Mon, Feb 12, 2018 at 3:19 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> Add documentation explaining the functions in color.h.
> While at it, mark them extern and migrate the function `color_set`
> into grep.c, where the only callers are.
This re-roll no longer marks functions as 'extern', so the commit
message saying that it does seems rather odd.
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> diff --git a/color.h b/color.h
> @@ -76,22 +76,47 @@ int git_color_config(const char *var, const char *value, void *cb);
> + * Return a boolean whether to use color,
> + * resolving ambigious settings such as GIT_COLOR_AUTO, which is returned
> + * by git_config_colorbool().
> + */
> int want_color(int var);
I'm probably being dense, but (if I hadn't had Peff's explanation
to fall back on), based upon the comment block alone, I'd still have
no idea what I'm supposed to pass in as 'var'. Partly this is due to
the comment block not mentioning 'var' explicitly; it talks about
GIT_COLOR_AUTO and git_config_colorbool() abstractly, and, as a
reader, I can't tell if those are supposed to be passed in as 'var' or
if the function somehow grabs them out of the environment. Partly it's
due to the name "var" not conveying any useful meaning. Perhaps take
Peff's hint and state explicitly that the passed-in value is one of
GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO,
then further explain that that value comes from
git_config_colorbool(), possibly modified by local option processing,
such as --color.
> + * 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.
> + */
Perhaps prefix the last sentence with "BUG:" since we don't want
people relying on this NUL bug/misfeature.