From: Junio C Hamano <junkio@cox.net>
Date: Fri, 08 Sep 2006 08:49:20 GMT
Subject: Re: [PATCH] Move color option parsing out of diff.c and into color.[ch]
Message-ID: <7vk64eivzj.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060908080318.GA3771@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> +static int color_vprintf(const char *color, const char *fmt,
> +		va_list args, const char *trail)
> +{
> +	int r = 0;
> +
> +	if (*color)
> +		r += printf("%s", color);
> +	r += vprintf(fmt, args);
> +	if (trail)
> +		r += printf("%s", trail);
> +	if (*color)
> +		r += printf("%s", COLOR_RESET);
> +	return r;
> +}

Hmm,... don't you mean RESET first and then trail (which is often "\n")?

> +int color_printf(const char *color, const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +	va_start(args, fmt);
> +	r = color_vprintf(color, fmt, args, 0);
> +	va_end(args);
> +	return r;
> +}

Please spell NULL not 0 (please do not argue that writing a NULL
pointer as integral constant 0 is perfectly valid C -- we all
know that.  This is not the language-lawyers correctness issue,
but ../linux-2.6/Documentation/CodingStyle thing).  Using
pointer as boolean seems to be Ok style (e.g. "if (trail)" does
not have to be written "if (trail == NULL)").