{"thread":{"id":"17813","subject":"Improving CRLF error message; also, enabling autocrlf and safecrlf by default","startedAt":"2009-02-16T02:45:43Z","lastAt":"2009-02-16T03:43:11Z","messageCount":7,"participants":["Jason Spiro","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"104882","messageId":"loom.20090216T022524-78@post.gmane.org","threadId":"17813","inReplyTo":null,"subject":"Improving CRLF error message; also, enabling autocrlf and safecrlf by default","fromName":"Jason Spiro","fromEmail":"jasonspiro4@gmail.com","sentAt":"2009-02-16T02:45:43Z","receivedAt":"2009-02-16T02:45:43Z","isPatch":false,"sender":{"key":"jasonspiro4@gmail.com","avatar":null},"body":"Hi,\n\nThanks for writing git.  It's a darn useful tool.  But one thing:\n\nOne of the pre-commit hooks detects trailing whitespace:\n\nif (/\\s$/) {\nbad_line(\"trailing whitespace\", $_);\n}\n\nUnfortunately, when I try to check in a file with DOS (CR+LF) line endings, \nthis hook triggers on every line.  This happens on Cygwin.  I haven't checked, \nbut I bet it happens on other platforms as well, as long as this hook runs.\n\nBut the error message \"trailing whitespace\" doesn't clearly tell me what's \nwrong.\n\n1.  Could you please modify Git so that, when such a problem happens, it \ninstead prints an message saying that the file has CR+LF line endings, and that \nGit does not allow this?\n\n2.  In addition, could you please enable the core.autocrlf and core.safecrlf \noptions by default in the next version of Git?\n\nP.S.  I hereby release the contents of this e-mail message to the public \ndomain.\n\nThanks in advance,\n--\nJason Spiro: software/web developer, packager, trainer, IT consultant.\nI support Linux, UNIX, Windows, and more. Contact me to discuss your needs.\n+1 (416) 992-3445 / www.jspiro.com\n"},{"id":"104886","messageId":"20090216030446.GC18780@sigill.intra.peff.net","threadId":"17813","inReplyTo":"loom.20090216T022524-78@post.gmane.org","subject":"Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-16T03:04:46Z","receivedAt":"2009-02-16T03:04:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 16, 2009 at 02:45:43AM +0000, Jason Spiro wrote:\n\n> One of the pre-commit hooks detects trailing whitespace:\n> \n> if (/\\s$/) {\n> bad_line(\"trailing whitespace\", $_);\n> }\n\nNot since 03e2b63 (Update sample pre-commit hook to use \"diff --check\",\n2008-06-26), when that line was removed.\n\nI'm happy you want to improve git; but please, if you want to report\nproblems, check what the status is in a more recent version (or at least\ntell us your version, which can help).\n\n> Unfortunately, when I try to check in a file with DOS (CR+LF) line\n> endings, this hook triggers on every line.  This happens on Cygwin.  I\n> haven't checked, but I bet it happens on other platforms as well, as\n> long as this hook runs.\n\nYes, I believe carriage returns are considered trailing whitespace. I\nthink (and I am not 100% sure here, because I have the good fortune not\nto have to deal with line-ending conversions on any of my platforms)\nthat the general philosophy is that the \"canonical\" form in the\nrepository should be LF-only, and that conversions can optionally make\nthe worktree version CRLF (or whatever your platform desires it). But\nit's important that the canonical version be the same across platforms\nso that the blob sha-1's (and therefore the tree and commit sha-1's) all\nmatch.\n\nIOW, setting up core.autocrlf properly should make this go away.\n\n> But the error message \"trailing whitespace\" doesn't clearly tell me\n> what's wrong.\n\nModern versions use \"diff --check\", which should look like this (on my\nLF-only box, at least):\n\n  $ mkdir repo && cd repo && git init\n  $ touch file && git add file && git commit -m one\n  $ printf 'foo\\r\\n' >file\n  $ git diff --check\n  file:1: trailing whitespace.\n  +foo^M\n\nand if you use \"git diff --color --check\", the problem is highlighted.\n\n> 1.  Could you please modify Git so that, when such a problem happens,\n> it instead prints an message saying that the file has CR+LF line\n> endings, and that Git does not allow this?\n\nIt might be worth splitting the trailing whitespace detection into\n\"spaces and tabs at the end\" and \"CRLF\", and providing different\nmessages (though it is hopefully also obvious with the new output that\nit is a CRLF issue).\n\n> 2.  In addition, could you please enable the core.autocrlf and core.safecrlf \n> options by default in the next version of Git?\n\nI think that is up to your platform packaging, I think. I think msysgit\nis shipping with core.autocrlf on by default these days. But again, I\ndon't know very much about that area.\n\n-Peff\n"},{"id":"104887","messageId":"7vljs7f58a.fsf@gitster.siamese.dyndns.org","threadId":"17813","inReplyTo":"loom.20090216T022524-78@post.gmane.org","subject":"Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-16T03:08:53Z","receivedAt":"2009-02-16T03:08:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jason Spiro <jasonspiro4@gmail.com> writes:\n\n> One of the pre-commit hooks detects trailing whitespace:\n\nAll sample hooks are shipped disabled by default, so it shouldn't be\ntriggering unless you enabled it yourself.  The only known exception is\nthe binary packaged one for Cygwin, which we do not have much control over\nhere.\n\n> if (/\\s$/) {\n> bad_line(\"trailing whitespace\", $_);\n> }\n>\n> Unfortunately, when I try to check in a file with DOS (CR+LF) line endings, \n> this hook triggers on every line.  This happens on Cygwin.  I haven't checked, \n> but I bet it happens on other platforms as well, as long as this hook runs.\n>\n> But the error message \"trailing whitespace\" doesn't clearly tell me what's \n> wrong.\n\nI and other people agreed with your analysis above wholeheartedly several\nmonths ago, and as a result, v1.6.0 and later version of git use a\ndifferent implementation for this check in the sample hook.  It does know\nyour CRLF line endings and therefore it should behave much better.\n\nThe fix to your situation might be just the matter of taking a copy of\ntemplates/hooks--pre-commit.sample from the current git source code and\nreplacing .git/hooks/pre-commit in your repository.\n\nThe sample hook looks like the attached one these days.  It relies on an\nenhancement 346245a (hard-code the empty tree object, 2008-02-13) that\nappeared first in v1.5.5 so it may not work if your copy of git is older\nthan that version.\n\n-- >8 -- cut here -- >8 --\n#!/bin/sh\n#\n# An example hook script to verify what is about to be committed.\n# Called by git-commit with no arguments.  The hook should\n# exit with non-zero status after issuing an appropriate message if\n# it wants to stop the commit.\n#\n# To enable this hook, rename this file to \"pre-commit\".\n\nif git-rev-parse --verify HEAD 2>/dev/null\nthen\n\tagainst=HEAD\nelse\n\t# Initial commit: diff against an empty tree object\n\tagainst=4b825dc642cb6eb9a060e54bf8d69288fbee4904\nfi\n\nexec git diff-index --check --cached $against --\n"},{"id":"104888","messageId":"7vhc2vf4yx.fsf@gitster.siamese.dyndns.org","threadId":"17813","inReplyTo":"20090216030446.GC18780@sigill.intra.peff.net","subject":"Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-16T03:14:30Z","receivedAt":"2009-02-16T03:14:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm happy you want to improve git; but please, if you want to report\n> problems, check what the status is in a more recent version (or at least\n> tell us your version, which can help).\n> ...\n> It might be worth splitting the trailing whitespace detection into\n> \"spaces and tabs at the end\" and \"CRLF\", and providing different\n> messages (though it is hopefully also obvious with the new output that\n> it is a CRLF issue).\n\nI think the status on this in a more recent version can be found by\nrunning \"git grep cr-at-eol\" ;-)\n"},{"id":"104889","messageId":"20090216031849.GA12348@coredump.intra.peff.net","threadId":"17813","inReplyTo":"7vhc2vf4yx.fsf@gitster.siamese.dyndns.org","subject":"Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-16T03:18:49Z","receivedAt":"2009-02-16T03:18:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 15, 2009 at 07:14:30PM -0800, Junio C Hamano wrote:\n\n> > I'm happy you want to improve git; but please, if you want to report\n> > problems, check what the status is in a more recent version (or at least\n> > tell us your version, which can help).\n> > ...\n> > It might be worth splitting the trailing whitespace detection into\n> > \"spaces and tabs at the end\" and \"CRLF\", and providing different\n> > messages (though it is hopefully also obvious with the new output that\n> > it is a CRLF issue).\n> \n> I think the status on this in a more recent version can be found by\n> running \"git grep cr-at-eol\" ;-)\n\nHeh. You didn't quote the part where I already claimed to be clueless.\n;)\n\nBut seriously, might it not be useful for users seeing --check warnings\nfor the first time to print:\n\n  file:1: trailing whitespace (cr-at-eol).\n  +foo^M\n\nThat is, there are different rules for trailing whitespace, but we don't\ncurrently tell you which one triggered. Giving the user \"cr-at-eol\"\ngives them something to grep before.\n\n-Peff\n"},{"id":"104891","messageId":"loom.20090216T032551-612@post.gmane.org","threadId":"17813","inReplyTo":"20090216030446.GC18780@sigill.intra.peff.net","subject":"Re: Improving CRLF error message; also, enabling autocrlf and safecrlf by default","fromName":"Jason Spiro","fromEmail":"jasonspiro4@gmail.com","sentAt":"2009-02-16T03:29:23Z","receivedAt":"2009-02-16T03:29:23Z","isPatch":false,"sender":{"key":"jasonspiro4@gmail.com","avatar":null},"body":"Jeff King <peff <at> peff.net> writes:\n> \n> On Mon, Feb 16, 2009 at 02:45:43AM +0000, Jason Spiro wrote:\n> \n> > One of the pre-commit hooks detects trailing whitespace:\n> > \n> > if (/\\s$/) {\n> > bad_line(\"trailing whitespace\", $_);\n> > }\n> \n> Not since 03e2b63 (Update sample pre-commit hook to use \"diff --check\",\n> 2008-06-26), when that line was removed.\n> \n> I'm happy you want to improve git; but please, if you want to report\n> problems, check what the status is in a more recent version (or at least\n> tell us your version, which can help).\n\nSorry.  Will do.\n\n...\n> > 2.  In addition, could you please enable the core.autocrlf and \ncore.safecrlf \n> > options by default in the next version of Git?\n> \n> I think that is up to your platform packaging, I think. I think msysgit\n> is shipping with core.autocrlf on by default these days. But again, I\n> don't know very much about that area.\n\nAre you saying that only my platform's packager can decide what options are \nenabled by default, and that you upstream folks have no influence at all?  :)\n"},{"id":"104893","messageId":"20090216034311.GA12616@coredump.intra.peff.net","threadId":"17813","inReplyTo":"loom.20090216T032551-612@post.gmane.org","subject":"Re: Improving CRLF error message; also, enabling autocrlf and?safecrlf by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-02-16T03:43:11Z","receivedAt":"2009-02-16T03:43:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 16, 2009 at 03:29:23AM +0000, Jason Spiro wrote:\n\n> > > 2.  In addition, could you please enable the core.autocrlf and \n> core.safecrlf \n> > > options by default in the next version of Git?\n> > \n> > I think that is up to your platform packaging, I think. I think msysgit\n> > is shipping with core.autocrlf on by default these days. But again, I\n> > don't know very much about that area.\n> \n> Are you saying that only my platform's packager can decide what options are \n> enabled by default, and that you upstream folks have no influence at all?  :)\n\nNot necessarily. But I don't think we want core.autocrlf on by default\nfor all platforms. So the decision needs to be made on a platform by\nplatform basis. The cleanest way to do that (in my opinion) is through a\nsystem-level configuration file. But git built from src does not\ndistribute such a configuration file at all; that seems to be in the\nscope of package distributors.\n\n-Peff\n"}]}