{"thread":{"id":"20238","subject":"[PATCH] New whitespace checking category 'trailing-blank-line'","startedAt":"2009-07-26T09:45:37Z","lastAt":"2009-07-30T23:47:23Z","messageCount":4,"participants":["Bruno Haible","Junio C Hamano","Thell"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"118823","messageId":"200907261145.38449.bruno@clisp.org","threadId":"20238","inReplyTo":null,"subject":"[PATCH] New whitespace checking category 'trailing-blank-line'","fromName":"Bruno Haible","fromEmail":"bruno@clisp.org","sentAt":"2009-07-26T09:45:37Z","receivedAt":"2009-07-26T09:45:37Z","isPatch":true,"sender":{"key":"bruno@clisp.org","avatar":null},"body":"Hi,\n\nIn some GNU projects, there are file types for which trailing spaces in a line\nare undesired, but for which trailing blank lines are normal. Such file types\nare:\n - ChangeLog files,\n - modules descriptions in Gnulib,\n - also the README files in 20% of the projects.\n\nCurrently the user has to turn off the 'trailing-space' whitespace attribute\nin order for 'git diff --check' to not complain about such files. This has\nthe drawback that trailing spaces are not detected.\n\nHere is a proposed patch, to allow people to turn the check against trailing\nblank lines independently from the whitespace-in-a-line checking. The default\nbehavior is not changed.\n\n\n>From 049db23a38c92c734aae13788a5a9478ed587cfd Mon Sep 17 00:00:00 2001\nFrom: Bruno Haible \nDate: Sun, 26 Jul 2009 11:08:41 +0200\nSubject: [PATCH] New whitespace checking category 'trailing-blank-line'.\n\n---\n Documentation/RelNotes-1.6.4.txt | 6 ++++++\n Documentation/config.txt | 2 ++\n cache.h | 3 ++-\n diff.c | 2 +-\n ws.c | 6 ++++++\n 5 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.6.4.txt b/Documentation/RelNotes-1.6.4.txt\nindex b3c0346..9ebcc3a 100644\n--- a/Documentation/RelNotes-1.6.4.txt\n+++ b/Documentation/RelNotes-1.6.4.txt\n@@ -64,6 +64,12 @@ Updates since v1.6.3\n to avoid testing a commit that is too close to a commit that is\n already known to be untestable.\n \n+ * In the configuration variable core.whitespace and in a 'whitespace'\n+ attribute specified in .git/info/attributes or .gitattributes, a new\n+ category of whitespace checking is recognized: \"trailing-blank-line\".\n+ Previously this checking was part of \"trailing-space\"; now it can be\n+ turned on or off separately.\n+\n * \"git cvsexportcommit\" learned -k option to stop CVS keywords expansion\n \n * \"git grep\" learned -p option to show the location of the match using the\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6857d2f..e9221ba 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -411,6 +411,8 @@ core.whitespace::\n part of the line terminator, i.e. with it, `trailing-space`\n does not trigger if the character before such a carriage-return\n is not a whitespace (not enabled by default).\n+* `trailing-blank-line` treats blank lines at the end of the file as\n+ an error (enabled by default).\n \n core.fsyncobjectfiles::\n \tThis boolean will enable 'fsync()' when writing object files.\ndiff --git a/cache.h b/cache.h\nindex e6c7f33..24ae981 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -968,7 +968,8 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i\n #define WS_SPACE_BEFORE_TAB\t02\n #define WS_INDENT_WITH_NON_TAB\t04\n #define WS_CR_AT_EOL 010\n-#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)\n+#define WS_TRAILING_BLANK_LINE 020\n+#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|WS_TRAILING_BLANK_LINE)\n extern unsigned whitespace_rule_cfg;\n extern unsigned whitespace_rule(const char *);\n extern unsigned parse_whitespace_rule(const char *);\ndiff --git a/diff.c b/diff.c\nindex cd35e0c..6d1b07b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1704,7 +1704,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\txdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n \t\t\t &xpp, &xecfg, &ecb);\n \n-\t\tif ((data.ws_rule & WS_TRAILING_SPACE) &&\n+\t\tif ((data.ws_rule & WS_TRAILING_BLANK_LINE) &&\n \t\t data.trailing_blanks_start) {\n \t\t\tfprintf(o->file, \"%s:%d: ends with blank lines.\\n\",\n \t\t\t\tdata.filename, data.trailing_blanks_start);\ndiff --git a/ws.c b/ws.c\nindex 59d0883..5f5a930 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -16,6 +16,7 @@ static struct whitespace_rule {\n \t{ \"space-before-tab\", WS_SPACE_BEFORE_TAB, 0 },\n \t{ \"indent-with-non-tab\", WS_INDENT_WITH_NON_TAB, 0 },\n \t{ \"cr-at-eol\", WS_CR_AT_EOL, 1 },\n+\t{ \"trailing-blank-line\", WS_TRAILING_BLANK_LINE, 0 },\n };\n \n unsigned parse_whitespace_rule(const char *string)\n@@ -114,6 +115,11 @@ char *whitespace_error_string(unsigned ws)\n \t\t\tstrbuf_addstr(&err, \", \");\n \t\tstrbuf_addstr(&err, \"indent with spaces\");\n \t}\n+\tif (ws & WS_TRAILING_BLANK_LINE) {\n+\t\tif (err.len)\n+\t\t\tstrbuf_addstr(&err, \", \");\n+\t\tstrbuf_addstr(&err, \"trailing blank line\");\n+\t}\n \treturn strbuf_detach(&err, NULL);\n }\n \n-- \n1.6.3.2\n"},{"id":"118848","messageId":"7vbpn7p1mw.fsf@alter.siamese.dyndns.org","threadId":"20238","inReplyTo":"200907261145.38449.bruno@clisp.org","subject":"Re: [PATCH] New whitespace checking category 'trailing-blank-line'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-26T21:36:55Z","receivedAt":"2009-07-26T21:36:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bruno Haible writes:\n\n> In some GNU projects, there are file types for which trailing spaces in a line\n> ...\n> Currently the user has to turn off the 'trailing-space' whitespace attribute\n> in order for 'git diff --check' to not complain about such files. This has\n> the drawback that trailing spaces are not detected.\n\nVery good problem description. Thanks.\n\n> Here is a proposed patch, to allow people to turn the check against trailing\n> blank lines independently from the whitespace-in-a-line checking. The default\n> behavior is not changed.\n\nThe \"default\" for people who do not have configuration may not change, but\npeople who explicitly asked git to check the trailing blank lines by\nspecifying \"whitespace=trail\" attribute, and have been relying on it, will\nnow be unprotected. Such a regression/incompatibility should be noted here.\n\nBetter yet would be not to introduce such a regression, of course.\n\n> From: Bruno Haible \n> Date: Sun, 26 Jul 2009 11:08:41 +0200\n> Subject: [PATCH] New whitespace checking category 'trailing-blank-line'.\n\nHave these three lines _before_ the proposed commit log message above, not\nafter it, if you want to set these differently from what your MUA gives to\nyour message; in this particular case I do not think they are necessary,\nthough.\n\n> ---\n\nAnd please sign-off your patch before the three-dash line.\n\n> diff --git a/Documentation/RelNotes-1.6.4.txt b/Documentation/RelNotes-1.6.4.txt\n> index b3c0346..9ebcc3a 100644\n> --- a/Documentation/RelNotes-1.6.4.txt\n> +++ b/Documentation/RelNotes-1.6.4.txt\n> @@ -64,6 +64,12 @@ Updates since v1.6.3\n> ...\n> + * In the configuration variable core.whitespace and in a 'whitespace'\n> + attribute specified in .git/info/attributes or .gitattributes, a new\n> + category of whitespace checking is recognized: \"trailing-blank-line\".\n> + Previously this checking was part of \"trailing-space\"; now it can be\n> + turned on or off separately.\n> +\n\nI appreciate a hunk to update the release notes like this (the series\ndefinitely is a post 1.6.4 material, though).\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 6857d2f..e9221ba 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -411,6 +411,8 @@ core.whitespace::\n> part of the line terminator, i.e. with it, `trailing-space`\n> does not trigger if the character before such a carriage-return\n> is not a whitespace (not enabled by default).\n> +* `trailing-blank-line` treats blank lines at the end of the file as\n> + an error (enabled by default).\n\nI suspect this should be done in a similar way as cr-at-eol. Keep the\nbehaviour for people who says trailing-space unchanged, but the error\ncheck can be loosened if you include the new string in the config or\nattribute value.\n\nAnd name it not to begin with \"trail\", e.g. \"blank-lines-at-eof\", to avoid\nbreaking people who have abbrevated \"trailing-space\" to \"trail\" in their\nconfig.\n\nAnother idea would be to:\n\n - introduce two new settings:\n\n blank-at-eol\t\t: SP/HT at EOL is an error\n empty-lines-at-eof\t: empty lines at the end of file is an error\n\n - make existing \"trailing-space\" a mere short-hand for \"blank-at-eol\"\n and \"empty-lines-at-eof\".\n\nI think the way we handled cr-at-eol was suboptimal. We should be able to\nlink this to crlf attribute (and core.autocrlf configuration) and pretend\nas if cr-at-eol was given if a file is subject to the crlf conversion\n(iow,. cr-at-eol should be deprecated/removed as a mistake).\n\nThe \"git diff\" part looked reasonable from a quick glance, but I do not\nthink I saw anything that affects \"git apply --whitespace=fix\" in the\npatch.\n"},{"id":"118851","messageId":"7v7hxvov55.fsf@alter.siamese.dyndns.org","threadId":"20238","inReplyTo":"7vbpn7p1mw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] New whitespace checking category 'trailing-blank-line'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-26T23:57:10Z","receivedAt":"2009-07-26T23:57:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think the way we handled cr-at-eol was suboptimal. We should be able to\n> link this to crlf attribute (and core.autocrlf configuration) and pretend\n> as if cr-at-eol was given if a file is subject to the crlf conversion\n> (iow,. cr-at-eol should be deprecated/removed as a mistake).\n\nI was a moron. I think cr-at-eol was really the best we could do, as it\nis to allow CR at the end of the line _in the tracked contents_; iow,\npeople who would want to use this would not be using crlf attribute at all.\n\nSo please strike this part out, but everything else stays the same.\n"},{"id":"119198","messageId":"loom.20090730T230729-582@post.gmane.org","threadId":"20238","inReplyTo":"7vbpn7p1mw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] New whitespace checking category 'trailing-blank-line'","fromName":"Thell","fromEmail":"tbfowler4@gmail.com","sentAt":"2009-07-30T23:47:23Z","receivedAt":"2009-07-30T23:47:23Z","isPatch":true,"sender":{"key":"tbfowler4@gmail.com","avatar":"https://gravatar.com/avatar/1b038b543b3facd3ae8cbfcc05ae06547e9f2015b5e661efbec68d662955a826?d=mp&s=160"},"body":"Junio C Hamano pobox.com> writes:\n\n> \n> Bruno Haible clisp.org> writes:\n> \n...\n> > Here is a proposed patch, to allow people to turn the check against trailing\n> > blank lines independently from the whitespace-in-a-line checking. The default\n> > behavior is not changed.\n> \n> The \"default\" for people who do not have configuration may not change, but\n> people who explicitly asked git to check the trailing blank lines by\n> specifying \"whitespace=trail\" attribute, and have been relying on it, will\n> now be unprotected. Such a regression/incompatibility should be noted here.\n> \n> Better yet would be not to introduce such a regression, of course.\n> \n> \n> \n... \n> Another idea would be to:\n> \n> - introduce two new settings:\n> \n> blank-at-eol\t\t: SP/HT at EOL is an error\n> empty-lines-at-eof\t: empty lines at the end of file is an error\n> \n> - make existing \"trailing-space\" a mere short-hand for \"blank-at-eol\"\n> and \"empty-lines-at-eof\".\n> \n> \n> The \"git diff\" part looked reasonable from a quick glance, but I do not\n> think I saw anything that affects \"git apply --whitespace=fix\" in the\n> patch.\n> \n\nHi all! How appropriate that this was posted so recently. Seems to be a fairly\npopular issue lately with the recent commits 735c674 and the correction 422a82f.\n\nI was thinking of a fix for the disappearing EOL @ EOF and was hoping for some\ninput and direction, yet wasn't really sure if a new RFH thread would be better\nthan here or not. Until otherwise noted how about just diving in...\n\n\nMy initial posting was to get input on which approach would be best received to\ngive a capability similar to what Junio's idea above states. The problem is\nwhich way to best go about it. Obviously if someone is doing whitespace=fix\nwith trailing-space set then they indeed want it fixed, but having the blank\nlines removed... ?\n\nCurrently the section in builtin-apply.c (v1.6.4) that is the culprit for\nremoving these lines is:\n\n1999 if (added_blank_line)\n2000 new_blank_lines_at_end++;\n2001 else\n2002 new_blank_lines_at_end = 0;\n\nHaving a --keep-new-blank-lines argument for apply, and am seems like a winner\nthat then doesn't effect regular operations.\n\n1999 if (added_blank_line && !keep_new_blank_lines_at_end)\n\nI've tested having the added_blank_line reset to 0 during testing of whitespace\nfixing from crlf to lf and it seems good to go for that purpose.\n\nIs that something that I should try for? Ie: having a new arg for am and apply,\nor is a new core whitespace option the better route?\n\nSincerely,\nThell (almostautomated on freenode)\n"}]}