Re: [PATCH 2/4] Extract and improve whitespace check from "git apply"

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 12, 2007, 19:39 UTC
Message-ID: <7vsl277ldc.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1197476582-18956-3-git-send-email-win@wincent.com>

Wincent Colaiuta <win@wincent.com> writes:

Show 13 quoted lines

> diff --git a/ws.c b/ws.c
> index 52c10ca..884d373 100644
> --- a/ws.c
> +++ b/ws.c
> +unsigned check_whitespace(const char *line, int len, unsigned ws_rule)
> +{
> + ...
> +	if (ws_rule & WS_TRAILING_SPACE) {
> +		/* Lines start with "+" or "-" so length is at least 1 */
> +		if (line[len - 1] == '\n') {
> +			if (isspace(line[len - 2]))
> +				result |= WS_TRAILING_SPACE;
> +		}

I like the direction, but I think it would make much more sense if you make check_whitespace() not about "a line in a patch that adds a line", but about "here is a line, check if that is acceptable". IOW, make line variable zero-based (and len = strlen(line)). The change would mean that existing callers need to be modified to do something like:

	if (line[0] == '+')
        	check_whitespace(line+1, len-1, ...);

but at the same time we could conceivably teach "git show" to show whitespace errors in a blob, i.e. "git show --show-ws-error HEAD:ws.c" by using such a check_whitespace().

The highlighting code may need similar changes. I was actually hoping you would consolidate the logic there that decides which segment of the string to highlight, and the logic in check_whitespace() to decide if there is an error to begin with. Conceptually, if emit_line_with_ws() decides there is nothing to highlight with DIFF_WHITESPACE color, that means there is no whitespace error on the line and vice-versa, no?