From: Wincent Colaiuta <win@wincent.com>
Date: Wed, 12 Dec 2007 22:50:29 GMT
Subject: Re: [PATCH 2/4] Extract and improve whitespace check from "git apply"
Message-ID: <C0D2E543-2F35-455B-BE58-A0177D087A8D@wincent.com>
In-Reply-To: <7vsl277ldc.fsf@gitster.siamese.dyndns.org>

El 12/12/2007, a las 20:39, Junio C Hamano escribió:

> 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, ...);

Yes, good point. As you say, it can then potentially be used for any  
line, not just lines in patches.

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

You're dead right. As I said in my original message, there is a *lot*  
of diff code and I basically only looked at enough of it to figure out  
what changes would be necessary to implement what I wanted. I hadn't  
looked inside emit_line_with_ws but it is an obvious site of  
duplication and as you point out there is definitely scope for more  
refactoring here; instead of doing the whitespace checking in three  
places (the three that I know about so far) we can do it in just one  
place. I'll see what I can cook up.

Cheers,
Wincent