Re: [iproute PATCH] lib/color: introduce freely configurable color strings

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 12 Oct 2020 16:50:21 +0200
Jan Engelhardt <jengelh@inai.de> wrote:

> Implement fine-grained control over color codes for iproute, very
> similar to the GCC_COLORS environment variable.
> 
> Signed-off-by: Jan Engelhardt <jengelh@inai.de>
> ---
>  lib/color.c   | 127 ++++++++++++++++++++++++--------------------------
>  man/man8/ip.8 |  25 ++++++++--
>  2 files changed, 81 insertions(+), 71 deletions(-)

I like the idea a lot.

But there are some style issues.
Checkpatch sees:

WARNING: Missing a blank line after declarations
#165: FILE: lib/color.c:46:
+	const char *s = getenv("COLORTERM"), *s2 = getenv("COLORFGBG");
+	color_is_enabled = (s != NULL && strtoul(s, NULL, 0) != 0) ||

ERROR: code indent should use tabs where possible
#166: FILE: lib/color.c:47:
+^I                   (s2 != NULL && *s2 != '\0');$

ERROR: do not use assignment in if condition
#188: FILE: lib/color.c:99:
+	if (p && (p = strrchr(p, ';')) != NULL && (p[1] == '7' && p[2] == '\0'))

WARNING: Missing a blank line after declarations
#197: FILE: lib/color.c:108:
+		const char *code = NULL;
+		for (size_t j = 0; j < ARRAY_SIZE(color_codes); ++j) {

WARNING: Missing a blank line after declarations
#207: FILE: lib/color.c:118:
+		const char *next = strchr(p, ':');
+		if (next == NULL)

WARNING: Missing a blank line after declarations
#228: FILE: lib/color.c:144:
+	const struct color_code *k = &color_codes[attr];
+	if (k->code != NULL && *k->code != '\0')


Also, please don't mix declarations in code like:

+		for (size_t j = 0; j < ARRAY_SIZE(color_codes); ++j) {

Iproute2 tries to stick to the kernel coding style, and loop variables
like this are allowed in that standard.