From: Wincent Colaiuta <win@wincent.com>
Date: Fri, 14 Dec 2007 07:36:09 GMT
Subject: Re: [PATCH 3/5] Unify whitespace checking
Message-ID: <85B8B116-78F6-4D37-A9DB-30EECAB97956@wincent.com>
In-Reply-To: <7vd4taywfw.fsf@gitster.siamese.dyndns.org>

El 14/12/2007, a las 1:03, Junio C Hamano escribió:

>> -	git diff --check | grep "space before tab"
>> +	git diff --check | grep "Space in indent is followed by a tab"
>>
> Hmph.  I think with the multiple detection this rewording would make  
> the
> error message very long to read.  Was the rewording really necessary?

Well, that wording came from builtin-apply.c so it wasn't really a "re- 
wording" but an extraction. The other string comes from "diff.c". So I  
had to pick one of them in unifying them and basically the one from  
builtin-apply.c won because it was the site where I extracted from  
first. So basically it was arbitrary.

If I were to actually *re-word* the message I think something like  
this would be the compromise between clarity and conciseness:

"space before tab in indent"

And in the same spirit, the other two strings extracted from builtin- 
apply.c into the new whitespace_error_string() function:

"Adds trailing whitespace"
"Indent more than 8 places with spaces"

Could be shortened to:

"trailing whitespace" (diff.c says "white space at end")
"indent using spaces"

But I personally have no strong conviction about this, so I'm happy  
for it to be whatever you want.

>> +/* If stream is non-NULL, emits the line after checking. */
>> +unsigned check_and_emit_line(const char *line, int len, unsigned  
>> ws_rule,
>> +                             FILE *stream, const char *set,
>> +                             const char *reset, const char *ws)
>> +{
>
> Honestly, I regretted suggesting this, fearing that it might make the
> checking too costly, but the code is clean, readable, and does not  
> look
> costly at all.  Nice job.

Thanks. The check_and_emit_line function itself is fairly clean  
internally (apart from the somewhat ugly parameter list, but that was  
mostly inherited from the emit_line_with_ws function which it  
replaces). It's the sites where check_and_emit_line is called that  
look a bit ugly, but those can hopefully be cleaned up at some point  
in the future.

Cheers,
Wincent