git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Christian Holtje <docwhat@gmail.com>
Cc: Ian Hilt <Ian.Hilt@gmx.com>, git@vger.kernel.org
Subject: Re: [PATCH] pre-commit hook should ignore carriage returns at EOL
Date: Tue, 24 Jun 2008 15:31:20 -0700	[thread overview]
Message-ID: <7v3an2bh3b.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: 39C2861E-F800-40AE-8C15-4FC3BB51EF16@gmail.com

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.

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.

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.

  reply	other threads:[~2008-06-24 22:32 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-06-24 16:23 [PATCH] pre-commit hook should ignore carriage returns at EOL Christian Holtje
2008-06-24 18:22 ` Alf Clement
2008-06-24 18:26 ` Ian Hilt
2008-06-24 19:05   ` Jakub Narebski
2008-06-24 19:54     ` Ian Hilt
2008-06-24 20:09       ` Jakub Narebski
2008-06-24 20:36         ` Ian Hilt
2008-06-24 19:16   ` Christian Holtje
2008-06-24 22:31     ` Junio C Hamano [this message]
2008-06-24 23:25       ` Christian Holtje
2008-06-24 23:34         ` Jakub Narebski
2008-06-24 23:39           ` Junio C Hamano
2008-06-25  0:19             ` Christian Holtje
2008-06-24 23:59         ` Junio C Hamano
2008-06-25  2:09           ` [PATCH] Ship sample hooks with .sample suffix Junio C Hamano
2008-06-25  2:48             ` Junio C Hamano
2008-06-25  6:51               ` Johannes Sixt
2008-06-25  8:09                 ` Junio C Hamano
2008-06-26  7:19               ` Johannes Sixt
2008-06-26  7:28                 ` Junio C Hamano
2008-06-25  5:18             ` Peter Baumann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7v3an2bh3b.fsf@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=Ian.Hilt@gmx.com \
    --cc=docwhat@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).