threads / patch / 47791

patchcolor.h: document and modernize header

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

## tl;dr

16 messages between Feb 8, 2018 and Feb 14, 2018. Diffs are folded; open one to read it.

replies: 15people: 4as markdown or json

Stefan Beller· Feb 8, 2018, 20:15 UTC · lore

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 This used to be part of  sb/blame-color, but I realized this is
 not strictly needed for that series, so I am sending it out alone.
 
 I would think I have addressed all concerns raised in 
 https://public-inbox.org/git/xmqqr2r088p3.fsf@gitster.mtv.corp.google.com/
 specifically the NEEDSWORK was dropped and this one function was just
 put into grep.c
 
 Thanks,
 Stefan
 color.c |  7 -------
 color.h | 52 ++++++++++++++++++++++++++++++++++++++--------------
 grep.c  |  5 +++++
 3 files changed, 43 insertions(+), 21 deletions(-)
Show changes to 3 files +43 −21

color.c, color.h, grep.c

diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
 	return color_parse_mem(value, strlen(value), dst);
 }
 
-void color_set(char *dst, const char *color_bytes)
-{
-	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 	return r;
 }
 
-
-
 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);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
  */
-void color_set(char *dst, const char *color_bytes);
+extern int git_config_colorbool(const char *var, const char *value);
 
-int git_config_colorbool(const char *var, const char *value);
-int want_color(int var);
-int color_parse(const char *value, char *dst);
-int color_parse_mem(const char *value, int len, char *dst);
+/*
+ * Resolve the constants as returned by git_config_colorbool()
+ * (specifically "auto") to a boolean answer.
+ */
+extern int want_color(int var);
+
+/*
+ * 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);
+
+/*
+ * 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.
+ */
 __attribute__((format (printf, 3, 4)))
-int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+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 caller needs to replace the color with the actual desired color.
+ */
+extern int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
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);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
-- 
2.15.1.433.g936d1b9894.dirty
Jeff King· Feb 8, 2018, 20:43 UTC · re: Stefan Beller · lore

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

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
Stefan Beller· Feb 8, 2018, 21:04 UTC · re: Jeff King · lore

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

On Thu, Feb 8, 2018 at 12:43 PM, Jeff King <peff@peff.net> wrote:
Show 23 quoted lines
> On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:
>
>>  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
...
> 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).
A funny and sad rant about why clear communication matters:

[Once upon a time, maybe 2 years ago] I had the impression that the old code is nicely written and was consistently marked extern in header files. (which btw is consistent with variable declarations, they need the extern). All the new code doesn't make use of extern, so I had this on my low prio todo list, that eventually all code converges to have 'extern' functions in headers.

C.f. the following commits, found via
  git log -p --author=Beller -S extern
  5ec8274b84 (xdiff-interface: export comparing and hashing strings,
  2017-10-25) adding new externs
  1ecbf31d02 (hashmap: migrate documentation from Documentation/technical
  into header, 2017-06-30), a cleanup, which doesn't touch externs
  a6d7eb2c7a (pull: optionally rebase submodules (remote submodule
  changes only), 2017-06-23) new code using externs
  bd26756112 (submodule.h: add extern keyword to functions, 2016-12-20)
  (The commit message is as accurate as it gets)

You may sense a pattern here: I currently have the very firm understanding we use the extern keyword in our codebase.

And I can also attest that this was not always the case, as back in the day I remember writing patches without the extern keyword only to be told: (A) be similar to the function in the next lines (B) the standard is to use extern and I was convinced it was a bad decision to prefix declarations with the extern keyword, but followed along as I don't want to have style in the way of writing features.

  $ cat Documentation/CodingGuidelines |grep extern
  $ # oh no it's empty!
Care to add a section to our coding guidelines?

Thanks, Stefan

Jeff King· Feb 8, 2018, 21:38 UTC · re: Stefan Beller · lore

[PATCH] CodingGuidelines: mention "static" and "extern"

On Thu, Feb 08, 2018 at 01:04:08PM -0800, Stefan Beller wrote:
Show 10 quoted lines
> You may sense a pattern here: I currently have the very firm understanding
> we use the extern keyword in our codebase.
> 
> And I can also attest that this was not always the case, as back in the
> day I remember writing patches without the extern keyword only to be told:
> (A) be similar to the function in the next lines
> (B) the standard is to use extern
> and I was convinced it was a bad decision to prefix declarations with
> the extern keyword, but followed along as I don't want to have style
> in the way of writing features.

It definitely was the case that people used to suggest "extern". I think this was a Linus-ism from the early days, and I have been hating it for almost 12 years now. ;)

>   $ cat Documentation/CodingGuidelines |grep extern
>   $ # oh no it's empty!
> 
> Care to add a section to our coding guidelines?
Here's a patch.
-- >8 --
Subject: [PATCH] CodingGuidelines: mention "static" and "extern"

It perhaps goes without saying that file-local stuff should be marked static, but it does not hurt to remind people.

Less obvious is that we are settling on "do not include extern in function declarations". It is already the default unless the function was previously declared static (but if you are following a static declaration with an unmarked one, you should think about why you are declaring the thing twice). And so it just becomes an extra noise-word in our header files.

We used to give the opposite advice, so there are quite a few "extern" markers in early Git code. But this at least makes a concrete suggestion that we can follow going forward.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/CodingGuidelines | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to Documentation/CodingGuidelines +5 −0
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c4cb5ff0d4..48aa4edfbd 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -386,6 +386,11 @@ For C programs:
  - Use Git's gettext wrappers to make the user interface
    translatable. See "Marking strings for translation" in po/README.
 
+ - Variables and functions local to a given source file should be marked
+   with "static". Variables that are visible to other source files
+   must be declared with "extern" in header files. However, function
+   declarations should not use "extern", as that is already the default.
+
 For Perl programs:
 
  - Most of the C guidelines above apply.
-- 
2.16.1.365.g89f5777adf
Stefan Beller· Feb 8, 2018, 21:43 UTC · re: Jeff King · lore

Re: [PATCH] CodingGuidelines: mention "static" and "extern"

On Thu, Feb 8, 2018 at 1:38 PM, Jeff King <peff@peff.net> wrote:
Show 44 quoted lines
> On Thu, Feb 08, 2018 at 01:04:08PM -0800, Stefan Beller wrote:
>
>> You may sense a pattern here: I currently have the very firm understanding
>> we use the extern keyword in our codebase.
>>
>> And I can also attest that this was not always the case, as back in the
>> day I remember writing patches without the extern keyword only to be told:
>> (A) be similar to the function in the next lines
>> (B) the standard is to use extern
>> and I was convinced it was a bad decision to prefix declarations with
>> the extern keyword, but followed along as I don't want to have style
>> in the way of writing features.
>
> It definitely was the case that people used to suggest "extern". I think
> this was a Linus-ism from the early days, and I have been hating it for
> almost 12 years now. ;)
>
>>   $ cat Documentation/CodingGuidelines |grep extern
>>   $ # oh no it's empty!
>>
>> Care to add a section to our coding guidelines?
>
> Here's a patch.
>
> -- >8 --
> Subject: [PATCH] CodingGuidelines: mention "static" and "extern"
>
> It perhaps goes without saying that file-local stuff should
> be marked static, but it does not hurt to remind people.
>
> Less obvious is that we are settling on "do not include
> extern in function declarations". It is already the default
> unless the function was previously declared static (but if
> you are following a static declaration with an unmarked one,
> you should think about why you are declaring the thing
> twice). And so it just becomes an extra noise-word in our
> header files.
>
> We used to give the opposite advice, so there are quite a
> few "extern" markers in early Git code. But this at least
> makes a concrete suggestion that we can follow going
> forward.
>
> Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Stefan Beller <sbeller@google.com>

... and now I can resend that patch, after fixing it to follow our style. :)

Show 23 quoted lines
> ---
>  Documentation/CodingGuidelines | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index c4cb5ff0d4..48aa4edfbd 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -386,6 +386,11 @@ For C programs:
>   - Use Git's gettext wrappers to make the user interface
>     translatable. See "Marking strings for translation" in po/README.
>
> + - Variables and functions local to a given source file should be marked
> +   with "static". Variables that are visible to other source files
> +   must be declared with "extern" in header files. However, function
> +   declarations should not use "extern", as that is already the default.
> +
>  For Perl programs:
>
>   - Most of the C guidelines above apply.
> --
> 2.16.1.365.g89f5777adf
>
Eric Sunshine· Feb 8, 2018, 23:14 UTC · re: Jeff King · lore

Re: [PATCH] CodingGuidelines: mention "static" and "extern"

On Thu, Feb 8, 2018 at 4:38 PM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> Subject: [PATCH] CodingGuidelines: mention "static" and "extern"
> [...]
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -386,6 +386,11 @@ For C programs:
> + - Variables and functions local to a given source file should be marked
> +   with "static". Variables that are visible to other source files
> +   must be declared with "extern" in header files. However, function
> +   declarations should not use "extern", as that is already the default.
Perhaps:
    ... as that is already the default, unless declarations in the
    header are already "extern", in which case consistency
    may favor mirroring existing usage.
or something.
Jonathan Tan· Feb 9, 2018, 19:07 UTC · re: Eric Sunshine · lore

Re: [PATCH] CodingGuidelines: mention "static" and "extern"

On Thu, 8 Feb 2018 18:14:06 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 20 quoted lines
> On Thu, Feb 8, 2018 at 4:38 PM, Jeff King <peff@peff.net> wrote:
> > Subject: [PATCH] CodingGuidelines: mention "static" and "extern"
> > [...]
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> > @@ -386,6 +386,11 @@ For C programs:
> > + - Variables and functions local to a given source file should be marked
> > +   with "static". Variables that are visible to other source files
> > +   must be declared with "extern" in header files. However, function
> > +   declarations should not use "extern", as that is already the default.
> 
> Perhaps:
> 
>     ... as that is already the default, unless declarations in the
>     header are already "extern", in which case consistency
>     may favor mirroring existing usage.
> 
> or something.

I would prefer not mirroring existing usage in this case - I think it's better if the code becomes eventually consistent in not using extern.

Jeff King· Feb 9, 2018, 19:33 UTC · re: Jonathan Tan · lore

Re: [PATCH] CodingGuidelines: mention "static" and "extern"

On Fri, Feb 09, 2018 at 11:07:38AM -0800, Jonathan Tan wrote:
Show 26 quoted lines
> On Thu, 8 Feb 2018 18:14:06 -0500
> Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> > On Thu, Feb 8, 2018 at 4:38 PM, Jeff King <peff@peff.net> wrote:
> > > Subject: [PATCH] CodingGuidelines: mention "static" and "extern"
> > > [...]
> > >
> > > Signed-off-by: Jeff King <peff@peff.net>
> > > ---
> > > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> > > @@ -386,6 +386,11 @@ For C programs:
> > > + - Variables and functions local to a given source file should be marked
> > > +   with "static". Variables that are visible to other source files
> > > +   must be declared with "extern" in header files. However, function
> > > +   declarations should not use "extern", as that is already the default.
> > 
> > Perhaps:
> > 
> >     ... as that is already the default, unless declarations in the
> >     header are already "extern", in which case consistency
> >     may favor mirroring existing usage.
> > 
> > or something.
> 
> I would prefer not mirroring existing usage in this case - I think it's
> better if the code becomes eventually consistent in not using extern.

Agreed. I think individual bullets in CodingGuidelines should be definitive where possible, describing the end-game we strive for.

-Peff
Eric Sunshine· Feb 8, 2018, 22:26 UTC · re: Jeff King · lore

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

On Thu, Feb 8, 2018 at 3:43 PM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:
>> +/*
>> + * 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. ;)

Agreed, this still fails to (directly) answer the question I asked in [1] about what 'var' is.

Show 7 quoted lines
> 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.

Rather than talking about plural "constants" (which makes it more confusing), it would likely be helpful for it to say (explicitly) that the caller passes in the result of git_config_colorbool() as 'var'.

Or something like that.
Show 11 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?

I (parenthetically) suggested[1] the same about fixing the bug/misbehavior, though doing so is outside the scope of this particular patch.

[1]: https://public-inbox.org/git/CAPig+cQVGsQk3tj43V6f3rFTD8smDxqWvug_u4__EWxOQG90xA@mail.gmail.com/
Jeff King· Feb 8, 2018, 22:28 UTC · re: Eric Sunshine · lore

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

On Thu, Feb 08, 2018 at 05:26:33PM -0500, Eric Sunshine wrote:
Show 13 quoted lines
> > 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.
> 
> Rather than talking about plural "constants" (which makes it more
> confusing), it would likely be helpful for it to say (explicitly) that
> the caller passes in the result of git_config_colorbool() as 'var'.
> 
> Or something like that.

That's not quite it, though. "var" is really the current program's idea of whether color has been requested. So it's git_config_colorbool(), as modified by things like "--color".

I think the best explanation is that it resolves an "enum color_bool" into a single 0/1 boolean. Except that "enum color_bool" doesn't exist, so we have no name for it.

-Peff
Stefan Beller· Feb 12, 2018, 20:19 UTC · re: Jeff King · lore

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.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
* removed the extern keyword
* reworded the docs for want_color once again.
 color.c |  7 -------
 color.h | 35 ++++++++++++++++++++++++++++++-----
 grep.c  |  5 +++++
 3 files changed, 35 insertions(+), 12 deletions(-)
Show changes to 3 files +35 −12

color.c, color.h, grep.c

diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
 	return color_parse_mem(value, strlen(value), dst);
 }
 
-void color_set(char *dst, const char *color_bytes)
-{
-	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 	return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
 	va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..40a40f31a4 100644
--- a/color.h
+++ b/color.h
@@ -76,22 +76,47 @@ 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);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
  */
-void color_set(char *dst, const char *color_bytes);
-
 int git_config_colorbool(const char *var, const char *value);
+
+/*
+ * 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);
+
+/*
+ * 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.
+ */
 int color_parse(const char *value, char *dst);
 int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * 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.
+ */
 __attribute__((format (printf, 3, 4)))
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
+/*
+ * 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.
+ */
 int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
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);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
-- 
2.16.1.73.ga2c3e9663f.dirty
Eric Sunshine· Feb 12, 2018, 22:14 UTC · re: Stefan Beller · lore

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

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.

Stefan Beller· Feb 13, 2018, 01:41 UTC · re: Eric Sunshine · lore

Add documentation explaining the functions in color.h. While at it, migrate the function `color_set` into grep.c, where the only callers are.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 * fixed commit message
 * followed Jeffs/Erics advice  and rewrote want_color()s doc once again.
 color.c |  7 -------
 color.h | 34 +++++++++++++++++++++++++++++-----
 grep.c  |  5 +++++
 3 files changed, 34 insertions(+), 12 deletions(-)
Show changes to 3 files +34 −12

color.c, color.h, grep.c

diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
 	return color_parse_mem(value, strlen(value), dst);
 }
 
-void color_set(char *dst, const char *color_bytes)
-{
-	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 /*
  * Write the ANSI color codes for "c" to "out"; the string should
  * already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 	return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
 	va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..763bef857d 100644
--- a/color.h
+++ b/color.h
@@ -76,22 +76,46 @@ 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);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
  */
-void color_set(char *dst, const char *color_bytes);
-
 int git_config_colorbool(const char *var, const char *value);
+
+/*
+ * Return a boolean whether to use color, where the argument 'var' is
+ * one of GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO.
+ */
 int want_color(int var);
+
+/*
+ * 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.
+ */
 int color_parse(const char *value, char *dst);
 int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * 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.  BUG: The `color_print_strbuf` prints the given pre-formatted
+ * strbuf instead, up to its first NUL character.
+ */
 __attribute__((format (printf, 3, 4)))
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
 void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
+/*
+ * 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.
+ */
 int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
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);
+}
+
 /*
  * Initialize the grep_defaults template with hardcoded defaults.
  * We could let the compiler do this, but without C99 initializers
-- 
2.16.1.73.ga2c3e9663f.dirty
Eric Sunshine· Feb 13, 2018, 03:55 UTC · re: Stefan Beller · lore

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

On Mon, Feb 12, 2018 at 8:41 PM, Stefan Beller <sbeller@google.com> wrote:
Show 15 quoted lines
> Add documentation explaining the functions in color.h.
> While at it, migrate the function `color_set` into grep.c,
> where the only callers are.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/color.h b/color.h
> @@ -76,22 +76,46 @@ int git_color_config(const char *var, const char *value, void *cb);
> +/*
> + * 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.  BUG: The `color_print_strbuf` prints the given pre-formatted
> + * strbuf instead, up to its first NUL character.
> + */

"`color_print_strbuf` prints the given pre-formatted strbuf (BUG: but only up to the first NUL character)."

Probably not worth a re-roll is Junio can amend it locally.
Eric Sunshine· Feb 14, 2018, 07:23 UTC · re: Eric Sunshine · lore

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

On Mon, Feb 12, 2018 at 10:55 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 11 quoted lines
> On Mon, Feb 12, 2018 at 8:41 PM, Stefan Beller <sbeller@google.com> wrote:
>> + * 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.  BUG: The `color_print_strbuf` prints the given pre-formatted
>> + * strbuf instead, up to its first NUL character.
>
> "`color_print_strbuf` prints the given pre-formatted strbuf (BUG: but
> only up to the first NUL character)."
>
> Probably not worth a re-roll is Junio can amend it locally.

By the way, thanks for the patience in the face of the nit-picking this patch has undergone.

Stefan Beller· Feb 14, 2018, 17:58 UTC · re: Eric Sunshine · lore

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

On Tue, Feb 13, 2018 at 11:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 15 quoted lines
> On Mon, Feb 12, 2018 at 10:55 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Feb 12, 2018 at 8:41 PM, Stefan Beller <sbeller@google.com> wrote:
>>> + * 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.  BUG: The `color_print_strbuf` prints the given pre-formatted
>>> + * strbuf instead, up to its first NUL character.
>>
>> "`color_print_strbuf` prints the given pre-formatted strbuf (BUG: but
>> only up to the first NUL character)."
>>
>> Probably not worth a re-roll is Junio can amend it locally.
>
> By the way, thanks for the patience in the face of the nit-picking
> this patch has undergone.

In retrospect it is clear why we have so much nitpicking here as it adds documentation to a part of git that is rather non-essential. ;-)

Thanks for bearing with my inability to write perfect code on the first try.

← back to recent threads