Improving CRLF error message; also, enabling autocrlf and safecrlf by default

Improving CRLF error message; also, enabling autocrlf and safecrlf by default

[Jason Spiro]

Hi,

Thanks for writing git.  It's a darn useful tool.  But one thing:

One of the pre-commit hooks detects trailing whitespace:

if (/\s$/) {
bad_line("trailing whitespace", $_);
}

Unfortunately, when I try to check in a file with DOS (CR+LF) line endings, 
this hook triggers on every line.  This happens on Cygwin.  I haven't checked, 
but I bet it happens on other platforms as well, as long as this hook runs.

But the error message "trailing whitespace" doesn't clearly tell me what's 
wrong.

1.  Could you please modify Git so that, when such a problem happens, it 
instead prints an message saying that the file has CR+LF line endings, and that 
Git does not allow this?

2.  In addition, could you please enable the core.autocrlf and core.safecrlf 
options by default in the next version of Git?

P.S.  I hereby release the contents of this e-mail message to the public 
domain.

Thanks in advance,
--
Jason Spiro: software/web developer, packager, trainer, IT consultant.
I support Linux, UNIX, Windows, and more. Contact me to discuss your needs.
+1 (416) 992-3445 / www.jspiro.com

Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default

[Jeff King]

On Mon, Feb 16, 2009 at 02:45:43AM +0000, Jason Spiro wrote:

> One of the pre-commit hooks detects trailing whitespace:
> 
> if (/\s$/) {
> bad_line("trailing whitespace", $_);
> }

Not since 03e2b63 (Update sample pre-commit hook to use "diff --check",
2008-06-26), when that line was removed.

I'm happy you want to improve git; but please, if you want to report
problems, check what the status is in a more recent version (or at least
tell us your version, which can help).

> Unfortunately, when I try to check in a file with DOS (CR+LF) line
> endings, this hook triggers on every line.  This happens on Cygwin.  I
> haven't checked, but I bet it happens on other platforms as well, as
> long as this hook runs.

Yes, I believe carriage returns are considered trailing whitespace. I
think (and I am not 100% sure here, because I have the good fortune not
to have to deal with line-ending conversions on any of my platforms)
that the general philosophy is that the "canonical" form in the
repository should be LF-only, and that conversions can optionally make
the worktree version CRLF (or whatever your platform desires it). But
it's important that the canonical version be the same across platforms
so that the blob sha-1's (and therefore the tree and commit sha-1's) all
match.

IOW, setting up core.autocrlf properly should make this go away.

> But the error message "trailing whitespace" doesn't clearly tell me
> what's wrong.

Modern versions use "diff --check", which should look like this (on my
LF-only box, at least):

  $ mkdir repo && cd repo && git init
  $ touch file && git add file && git commit -m one
  $ printf 'foo\r\n' >file
  $ git diff --check
  file:1: trailing whitespace.
  +foo^M

and if you use "git diff --color --check", the problem is highlighted.

> 1.  Could you please modify Git so that, when such a problem happens,
> it instead prints an message saying that the file has CR+LF line
> endings, and that Git does not allow this?

It might be worth splitting the trailing whitespace detection into
"spaces and tabs at the end" and "CRLF", and providing different
messages (though it is hopefully also obvious with the new output that
it is a CRLF issue).

> 2.  In addition, could you please enable the core.autocrlf and core.safecrlf 
> options by default in the next version of Git?

I think that is up to your platform packaging, I think. I think msysgit
is shipping with core.autocrlf on by default these days. But again, I
don't know very much about that area.

-Peff

Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default

[Junio C Hamano]

Jason Spiro <jasonspiro4@gmail.com> writes:

> One of the pre-commit hooks detects trailing whitespace:

All sample hooks are shipped disabled by default, so it shouldn't be
triggering unless you enabled it yourself.  The only known exception is
the binary packaged one for Cygwin, which we do not have much control over
here.

> if (/\s$/) {
> bad_line("trailing whitespace", $_);
> }
>
> Unfortunately, when I try to check in a file with DOS (CR+LF) line endings, 
> this hook triggers on every line.  This happens on Cygwin.  I haven't checked, 
> but I bet it happens on other platforms as well, as long as this hook runs.
>
> But the error message "trailing whitespace" doesn't clearly tell me what's 
> wrong.

I and other people agreed with your analysis above wholeheartedly several
months ago, and as a result, v1.6.0 and later version of git use a
different implementation for this check in the sample hook.  It does know
your CRLF line endings and therefore it should behave much better.

The fix to your situation might be just the matter of taking a copy of
templates/hooks--pre-commit.sample from the current git source code and
replacing .git/hooks/pre-commit in your repository.

The sample hook looks like the attached one these days.  It relies on an
enhancement 346245a (hard-code the empty tree object, 2008-02-13) that
appeared first in v1.5.5 so it may not work if your copy of git is older
than that version.

-- >8 -- cut here -- >8 --
#!/bin/sh
#
# An example hook script to verify what is about to be committed.
# Called by git-commit with no arguments.  The hook should
# exit with non-zero status after issuing an appropriate message if
# it wants to stop the commit.
#
# To enable this hook, rename this file to "pre-commit".

if git-rev-parse --verify HEAD 2>/dev/null
then
	against=HEAD
else
	# Initial commit: diff against an empty tree object
	against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi

exec git diff-index --check --cached $against --

Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm happy you want to improve git; but please, if you want to report
> problems, check what the status is in a more recent version (or at least
> tell us your version, which can help).
> ...
> It might be worth splitting the trailing whitespace detection into
> "spaces and tabs at the end" and "CRLF", and providing different
> messages (though it is hopefully also obvious with the new output that
> it is a CRLF issue).

I think the status on this in a more recent version can be found by
running "git grep cr-at-eol" ;-)

Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default

[Jeff King]

On Sun, Feb 15, 2009 at 07:14:30PM -0800, Junio C Hamano wrote:

> > I'm happy you want to improve git; but please, if you want to report
> > problems, check what the status is in a more recent version (or at least
> > tell us your version, which can help).
> > ...
> > It might be worth splitting the trailing whitespace detection into
> > "spaces and tabs at the end" and "CRLF", and providing different
> > messages (though it is hopefully also obvious with the new output that
> > it is a CRLF issue).
> 
> I think the status on this in a more recent version can be found by
> running "git grep cr-at-eol" ;-)

Heh. You didn't quote the part where I already claimed to be clueless.
;)

But seriously, might it not be useful for users seeing --check warnings
for the first time to print:

  file:1: trailing whitespace (cr-at-eol).
  +foo^M

That is, there are different rules for trailing whitespace, but we don't
currently tell you which one triggered. Giving the user "cr-at-eol"
gives them something to grep before.

-Peff

Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default

[Jason Spiro]

Jeff King <peff <at> peff.net> writes:
> 
> On Mon, Feb 16, 2009 at 02:45:43AM +0000, Jason Spiro wrote:
> 
> > One of the pre-commit hooks detects trailing whitespace:
> > 
> > if (/\s$/) {
> > bad_line("trailing whitespace", $_);
> > }
> 
> Not since 03e2b63 (Update sample pre-commit hook to use "diff --check",
> 2008-06-26), when that line was removed.
> 
> I'm happy you want to improve git; but please, if you want to report
> problems, check what the status is in a more recent version (or at least
> tell us your version, which can help).

Sorry.  Will do.

...
> > 2.  In addition, could you please enable the core.autocrlf and 
core.safecrlf 
> > options by default in the next version of Git?
> 
> I think that is up to your platform packaging, I think. I think msysgit
> is shipping with core.autocrlf on by default these days. But again, I
> don't know very much about that area.

Are you saying that only my platform's packager can decide what options are 
enabled by default, and that you upstream folks have no influence at all?  :)

Re: Improving CRLF error message; also, enabling autocrlf and?safecrlf by default

[Jeff King]

On Mon, Feb 16, 2009 at 03:29:23AM +0000, Jason Spiro wrote:

> > > 2.  In addition, could you please enable the core.autocrlf and 
> core.safecrlf 
> > > options by default in the next version of Git?
> > 
> > I think that is up to your platform packaging, I think. I think msysgit
> > is shipping with core.autocrlf on by default these days. But again, I
> > don't know very much about that area.
> 
> Are you saying that only my platform's packager can decide what options are 
> enabled by default, and that you upstream folks have no influence at all?  :)

Not necessarily. But I don't think we want core.autocrlf on by default
for all platforms. So the decision needs to be made on a platform by
platform basis. The cleanest way to do that (in my opinion) is through a
system-level configuration file. But git built from src does not
distribute such a configuration file at all; that seems to be in the
scope of package distributors.

-Peff