On Jun 24, 2008, at 6:31 PM, Junio C Hamano wrote:
Christian Holtje <docwhat@xxxxxxxxx> 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!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
