From: Christian Holtje <docwhat@gmail.com>
Date: Tue, 24 Jun 2008 23:25:40 GMT
Subject: Re: [PATCH] pre-commit hook should ignore carriage returns at EOL
Message-ID: <930163B6-4A7E-49C7-B9D5-F7B37699C2A9@gmail.com>
In-Reply-To: <7v3an2bh3b.fsf@gitster.siamese.dyndns.org>


On Jun 24, 2008, at 6:31 PM, Junio C Hamano wrote:

> Christian Holtje <docwhat@gmail.com> writes:
>
>> The code is checking for \r$ and then doing a different space check
>> depending on that, not one after another.
>>
>> Thanks for the feedback. I'll put up v2 in a second.
>
> Please don't.
>
> It's an ancient sample hook that is not be enabled by default.  I do  
> not
> want people to be wasting too much time on the relic

Part of the reason I'm fixing this is because it *is* enabled by  
default in windows.  I don't know why, but cygwin always marks it with  
executable.

> However, if this sample is to be changed at all, please do it right.
>
> If somebody suddenly adds CR at the end of an existing file that  
> ought to
> have LF line endings, we _DO_ want to catch that as a breakage.  So  
> the
> title of the commit "should ignore carriage returns at EOL" is  
> WRONG.  It
> shouldn't, in general.
>
> One thing the hook could and probably should do these days is if the  
> file
> type says you _ought to_ have CRLF line endings, actively make sure  
> your
> lines do end with CRLF (this is a much stronger and better check than
> blindly ignoring CR before LF for such files).  And on the other  
> hand, if
> the file should end with LF, do make sure it does not have CR before  
> it.
>
> The person who did the sample hook you are looking at couldn't do so
> because there weren't autocrlf nor gitattributes(5) facility back  
> then.
> But you can use them now to rewrite this properly.

How can I detect if a file should have CRLF vs. LF?  I didn't do a  
better check because I didn't know how.

> I wonder if "git diff --check" can be used for most if not all of the
> checking, without the big Perl script you are touching in your patch.
> That facility did not exist when the current sample hook was written,
> either.

I like that better.  Ditching the whole perl script in a shell script  
seems better.

I wrote a test case for what you describe above (a crlf file with an  
lf line or a lf file with a crlf) but "git diff --check" doesn't catch  
that.

Based on the information about core.whitespace doesn't git do this  
already?  Maybe we should just delete the pre-commit hook or make it  
empty with a note saying what you can do with it?

Ciao!