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

From: Stefan Beller <sbeller@google.com>
Date: Jan 4, 2018, 19:49 UTC
Message-ID: <CAGZ79kZJq1Y-R_Wx8w6raNFHE4snOgfHtfBh=gbH4pA0uOEF5w@mail.gmail.com>
In-Reply-To: <CAPig+cQVGsQk3tj43V6f3rFTD8smDxqWvug_u4__EWxOQG90xA@mail.gmail.com>

On Thu, Dec 28, 2017 at 2:00 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 21 quoted lines

> 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.

Right. This description is bad.

It's implementation is just

    xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);

Apparently this function is only ever used by grep.c which uses
it to fill in struct grep_opt {
    ...
    char color_context[COLOR_MAXLEN];
    char color_filename[COLOR_MAXLEN];
    char color_function[COLOR_MAXLEN];
    char color_lineno[COLOR_MAXLEN];
    char color_match_context[COLOR_MAXLEN];
    char color_match_selected[COLOR_MAXLEN];
    char color_selected[COLOR_MAXLEN];
    char color_sep[COLOR_MAXLEN];
    ...
}

I guess I'll drop the documentation for color_set, and put a NEEDSWORK comment there as I'd think we don't need to copy around the colors using snprintf, but either can keep pointers or use xmemdup.

Show 10 quoted lines

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

Maybe I should fix the code as well. Currently it only returns one of 0 (=GIT_COLOR_NEVER), 1 (=GIT_COLOR_ALWAYS) and GIT_COLOR_AUTO.

> 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?

done.

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

done

Show 13 quoted lines

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

done

Show 29 quoted lines

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

sounds good.

Show 6 quoted lines

>
>> + * 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?)

added.

Show 14 quoted lines

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

done

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