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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
> Add documentation explaining the functions in color.h.
> While at it, mark them extern.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/color.h b/color.h
> @@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
>  /*
> - * Set the color buffer (which must be COLOR_MAXLEN bytes)
> - * to the raw color bytes; this is useful for initializing
> + * Set the color buffer `dst` (which must be COLOR_MAXLEN bytes)
> + * to the raw color bytes `color_bytes`; this is useful for initializing
>   * default color variables.
>   */
> -void color_set(char *dst, const char *color_bytes);
> +extern void color_set(char *dst, const char *color_bytes);

I don't see an explanation of what "color bytes" are. From where does
one obtain such bytes? How is this function used? The function comment
does not particularly answer these questions.

> +/*
> + * Parses a config option, which can be a boolean or one of
> + * "never", "auto", "always". Returns the constant for the given setting.
> + */
> +extern int git_config_colorbool(const char *var, const char *value);

I suppose that "constant for the given setting" means one of
GIT_COLOR_NEVER , GIT_COLOR_AUTO, GIT_COLOR_ALWAYS? Perhaps say so
explicitly?

Would it also make sense to say that boolean "true" ("yes", etc.)
results in GIT_COLOR_ALWAYS and "false" ("no", etc.)" results in
GIT_COLOR_NEVER?

Finally, for grammatical consistency with other comments:
    s/Parses/Parse
    s/Returns/Return/

> +/* Is the output supposed to be colored? Resolve and cache the 'auto' setting */
> +extern int want_color(int var);

What is the 'var' argument? How is it interpreted? (...goes and checks
implementation...) I guess this documentation should explain that the
caller would pass in the result of git_config_colorbool().

Also, the meaning of "Resolve and cache 'auto' setting" stumped me for
a while since it's not clear why it's here (or why it's missing the
full stop), but I eventually realized that it's describing an
implementation detail, which probably doesn't belong in API
documentation.

> +/*
> + * Translate the human readable color string from `value` and into
> + * terminal color codes and store them in `dst`
> + */
> +extern int color_parse(const char *value, char *dst);
> +extern int color_parse_mem(const char *value, int len, char *dst);

What does "human readable" mean in this context? Is it talking about
color names or RGB(A) tuples or what?

Also, how does the caller know how large to make 'dst'? At minimum,
you should say something about COLOR_MAXLEN.

Finally, for the 'len' case, what happens if 'dst' is too small? This
should be documented.

And, the return value of these functions should be discussed.

> +/*
> + * Print the format string `fmt`, encapsulated by setting and resetting the
> + * color. Omits the color encapsulation if `color` is NULL.

The "encapsulated by setting and resetting the color" bit is hard to
grok. Perhaps instead say something along the lines of:

    Output the formatted string in the specified color (and
    then reset to normal color so subsequent output is
    uncolored).

> + * The `color_fprintf_ln` prints a new line after resetting the color.
> + * The `color_print_strbuf` prints the given pre-formatted strbuf instead.

Should the strbuf variation warn that it only outputs content up to
the first embedded NUL? (Or should that bug/misfeature be fixed?)

> + */
>  __attribute__((format (printf, 3, 4)))
> +extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
>  __attribute__((format (printf, 3, 4)))
> +extern int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
> +extern void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
>
> -int color_is_nil(const char *color);
> +/*
> + * Check if the given color is GIT_COLOR_NIL that means "no color selected".
> + * The application needs to replace the color with the actual desired color.

Maybe: s/application/caller/

> + */
> +extern int color_is_nil(const char *color);