Re: [PATCH 2/4] Extract and improve whitespace check from "git apply"
- From
Wincent Colaiuta <win@wincent.com>
- Date
- Dec 12, 2007, 22:50 UTC
- 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ó:
Show 10 quoted lines
> 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.
Show 11 quoted lines
> 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