Re: [PATCH v2] pre-commit hook should ignore carriage returns at EOL

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

Christian Holtje <docwhat@gmail.com> writes:

> On Jun 25, 2008, at 2:14 PM, Alex Riesen wrote:
>> Christian Holtje, Tue, Jun 24, 2008 21:21:22 +0200:
>>> diff --git a/t/t7503-template-hook--pre-commit.sh b/t/t7503-template-
>>> hook--pre-commit.sh
>>
>> Your patch has long lines wrapped.
>>
>>> diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-
>>> commit
>>> index b25dce6..335ca09 100644
>>> --- a/templates/hooks--pre-commit
>>> +++ b/templates/hooks--pre-commit
>>> @@ -55,8 +55,14 @@ perl -e '
>>> 	if (s/^\+//) {
>>> 	    $lineno++;
>>> 	    chomp;
>>> -	    if (/\s$/) {
>>> -		bad_line("trailing whitespace", $_);
>>> +	    if (/\r$/) {
>>> +		if (/\s\r$/) {
>>> +		    bad_line("trailing whitespace", $_);
>>> +		}
>>> +	    } else {
>>> +		if (/\s$/) {
>>> +		    bad_line("trailing whitespace", $_);
>>> +		}
>>
>> You coud just strip the trailing (cr)lf, instead of chomp:
>>
>>  	if (s/^\+//) {
>>  	    $lineno++;
>> - 	    chomp;
>> +	    s/\r?\n$//so;
>> 	    if (/\s$/) {
>> 		bad_line("trailing whitespace", $_);
>>
>> Makes for a shorter patch and less code.
>
> That's a good idea!  However, this patch is not going anyplace, I
> think.  Junio submitted a different patch to disable the pre-commit
> example.
>
> Junio, do you want me to make this change anyway?  It does make sense.
> The unittests for the pre-commit hook may or may not still be  useful.

"disable" is not an issue.  The intention has always been that these are
samples, and it was an accident that some packaging shipped them enabled
by mistake.  The patch was to make that mistake harder to make.

The issue now is about keeping the example hooks _relevant_.  The one we
have does not work well with projects that want to check in files with
CRLF line endings (iow, without using autocrlf to attempt to make the
project files cross-platform), so it is irrelevant for such projects with
Windows origin.

The "solution" you are proposing to strip out \r makes the check less
useful for projects that want to keep files with LF line endings in the
commited history, because your patch would stop catching such a mistake of
adding an CR before LF.  It is robbing from Peter to pay Paul, and I am
afraid it would make the sample even more irrelevant in the end.  I do not
think we would want to go there.

I suggested using "diff --check" (and possibly teaching "diff --check"
other things the scripted example checks, such as conflict markers),
which would know to honor the line endings specified per path via
gitattributes(5), instead of building on top of the big Perl script, and I
had an impression that you agreed to the approach.