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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 12, 2018, 22:14 UTC
Message-ID: <CAPig+cRUr=dJgaG2-aRArswQXXZEExQah4k17+HkiB+sZHORYQ@mail.gmail.com>
In-Reply-To: <20180212201922.74051-1-sbeller@google.com>

On Mon, Feb 12, 2018 at 3:19 PM, Stefan Beller <sbeller@google.com> 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.

Show 10 quoted lines

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 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[1] 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.

[1]: https://public-inbox.org/git/20180208222851.GA8850@sigill.intra.peff.net/

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.
> + */

Perhaps prefix the last sentence with "BUG:" since we don't want people relying on this NUL bug/misfeature.