Re: [PATCH 3/5] Unify whitespace checking

From: Wincent Colaiuta <win@wincent.com>
Date: Dec 14, 2007, 07:36 UTC
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ó:

Show 6 quoted lines

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

Show 11 quoted lines

>> +/* 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