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

[Junio C Hamano <gitster@pobox.com>]

Wincent Colaiuta <win@wincent.com> writes:

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