New whitespace checking category 'trailing-blank-line'
PATCH 1/1 New whitespace checking category 'trailing-blank-line'
Hi,
In some GNU projects, there are file types for which trailing spaces in a line are undesired, but for which trailing blank lines are normal. Such file types are:
- ChangeLog files,
- modules descriptions in Gnulib,
- also the README files in 20% of the projects.
Currently the user has to turn off the 'trailing-space' whitespace attribute in order for 'git diff --check' to not complain about such files. This has the drawback that trailing spaces are not detected.
Here is a proposed patch, to allow people to turn the check against trailing blank lines independently from the whitespace-in-a-line checking. The default behavior is not changed.
Quoted 1 line
From
049db23a38c92c734aae13788a5a9478ed587cfdMon Sep 17 00:00:00 2001
From: Bruno Haible <[email protected]>
Date: Sun, 26 Jul 2009 11:08:41 +0200
Subject: [PATCH] New whitespace checking category 'trailing-blank-line'.
--- Documentation/RelNotes-1.6.4.txt | 6 ++++++ Documentation/config.txt | 2 ++ cache.h | 3 ++- diff.c | 2 +- ws.c | 6 ++++++ 5 files changed, 17 insertions(+), 2 deletions(-)
Documentation/RelNotes-1.6.4.txt +6 -0
@@ -64,6 +64,12 @@ Updates since v1.6.3 to avoid testing a commit that is too close to a commit that is already known to be untestable. + * In the configuration variable core.whitespace and in a 'whitespace' + attribute specified in .git/info/attributes or .gitattributes, a new + category of whitespace checking is recognized: "trailing-blank-line". + Previously this checking was part of "trailing-space"; now it can be + turned on or off separately. + * "git cvsexportcommit" learned -k option to stop CVS keywords expansion * "git grep" learned -p option to show the location of the match using the
Documentation/config.txt +2 -0
@@ -411,6 +411,8 @@ core.whitespace:: part of the line terminator, i.e. with it, `trailing-space` does not trigger if the character before such a carriage-return is not a whitespace (not enabled by default). +* `trailing-blank-line` treats blank lines at the end of the file as + an error (enabled by default). core.fsyncobjectfiles:: This boolean will enable 'fsync()' when writing object files.
cache.h +2 -1
@@ -968,7 +968,8 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i#define WS_SPACE_BEFORE_TAB 02#define WS_INDENT_WITH_NON_TAB 04#define WS_CR_AT_EOL 010 -#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB) +#define WS_TRAILING_BLANK_LINE 020 +#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|WS_TRAILING_BLANK_LINE)extern unsigned whitespace_rule_cfg;extern unsigned whitespace_rule (const char *);extern unsigned parse_whitespace_rule (const char *);
diff.c +1 -1
@@ -1704,7 +1704,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,xdi_diff_outf (&mf1, &mf2, checkdiff_consume, &data, &xpp, &xecfg, &ecb); -if ((data.ws_rule &WS_TRAILING_SPACE ) && +if ((data.ws_rule &WS_TRAILING_BLANK_LINE ) && data.trailing_blanks_start) {fprintf (o->file,"%s:%d: ends with blank lines.\n" , data.filename, data.trailing_blanks_start);
ws.c +6 -0
@@ -16,6 +16,7 @@ static struct whitespace_rule { { "space-before-tab", WS_SPACE_BEFORE_TAB, 0 }, { "indent-with-non-tab", WS_INDENT_WITH_NON_TAB, 0 }, { "cr-at-eol", WS_CR_AT_EOL, 1 }, + { "trailing-blank-line", WS_TRAILING_BLANK_LINE, 0 }, };unsigned parse_whitespace_rule (const char *string)
@@ -114,6 +115,11 @@ char *whitespace_error_string(unsigned ws)strbuf_addstr (&err,", " );strbuf_addstr (&err,"indent with spaces" ); } +if (ws &WS_TRAILING_BLANK_LINE ) { +if (err.len) +strbuf_addstr (&err,", " ); +strbuf_addstr (&err,"trailing blank line" ); + }return strbuf_detach (&err,NULL ); }
Signature · 1 line
1.6.3.2
Quoted 5 lines · Bruno Haible
In some GNU projects, there are file types for which trailing spaces in a line
...
Currently the user has to turn off the 'trailing-space' whitespace attribute in order for 'git diff --check' to not complain about such files. This has the drawback that trailing spaces are not detected.
Very good problem description. Thanks.
Quoted 3 lines
Here is a proposed patch, to allow people to turn the check against trailing blank lines independently from the whitespace-in-a-line checking. The default behavior is not changed.
The "default" for people who do not have configuration may not change, but people who explicitly asked git to check the trailing blank lines by specifying "whitespace=trail" attribute, and have been relying on it, will now be unprotected. Such a regression/incompatibility should be noted here.
Better yet would be not to introduce such a regression, of course.
Quoted 3 lines
From: Bruno Haible <[email protected]>
Date: Sun, 26 Jul 2009 11:08:41 +0200
Subject: [PATCH] New whitespace checking category 'trailing-blank-line'.
Have these three lines _before_ the proposed commit log message above, not after it, if you want to set these differently from what your MUA gives to your message; in this particular case I do not think they are necessary, though.
Quoted 1 line
---
And please sign-off your patch before the three-dash line.
Documentation/RelNotes-1.6.4.txt Updates since v1.6.3 · line 72
Quoted 12 lines
Documentation/RelNotes-1.6.4.txt +6
-0@@ -64,6 +64,12 @@ Updates since v1.6.3 + * In the configuration variable core.whitespace and in a 'whitespace' + attribute specified in .git/info/attributes or .gitattributes, a new + category of whitespace checking is recognized: "trailing-blank-line". + Previously this checking was part of "trailing-space"; now it can be + turned on or off separately. +
I appreciate a hunk to update the release notes like this (the series definitely is a post 1.6.4 material, though).
Documentation/config.txt core.whitespace:: · line 415
Quoted 10 lines
Documentation/config.txt +2
-0@@ -411,6 +411,8 @@ core.whitespace:: part of the line terminator, i.e. with it, `trailing-space` does not trigger if the character before such a carriage-return is not a whitespace (not enabled by default). +* `trailing-blank-line` treats blank lines at the end of the file as + an error (enabled by default).
I suspect this should be done in a similar way as cr-at-eol. Keep the behaviour for people who says trailing-space unchanged, but the error check can be loosened if you include the new string in the config or attribute value.
And name it not to begin with "trail", e.g. "blank-lines-at-eof", to avoid breaking people who have abbrevated "trailing-space" to "trail" in their config.
Another idea would be to:
- introduce two new settings:
blank-at-eol : SP/HT at EOL is an error
empty-lines-at-eof : empty lines at the end of file is an error
- make existing "trailing-space" a mere short-hand for "blank-at-eol" and "empty-lines-at-eof".
I think the way we handled cr-at-eol was suboptimal. We should be able to link this to crlf attribute (and core.autocrlf configuration) and pretend as if cr-at-eol was given if a file is subject to the crlf conversion (iow,. cr-at-eol should be deprecated/removed as a mistake).
The "git diff" part looked reasonable from a quick glance, but I do not think I saw anything that affects "git apply --whitespace=fix" in the patch.
Quoted 4 lines · Junio C Hamano
I think the way we handled cr-at-eol was suboptimal. We should be able to link this to crlf attribute (and core.autocrlf configuration) and pretend as if cr-at-eol was given if a file is subject to the crlf conversion (iow,. cr-at-eol should be deprecated/removed as a mistake).
I was a moron. I think cr-at-eol was really the best we could do, as it is to allow CR at the end of the line _in the tracked contents_; iow, people who would want to use this would not be using crlf attribute at all.
So please strike this part out, but everything else stays the same.
Quoted 3 lines · Junio C Hamano <gitster <at> pobox.com> writes:
Bruno Haible <bruno <at> clisp.org> writes:
...
Quoted 13 lines
Quoted 3 lines
Here is a proposed patch, to allow people to turn the check against trailing blank lines independently from the whitespace-in-a-line checking. The default behavior is not changed.
The "default" for people who do not have configuration may not change, but people who explicitly asked git to check the trailing blank lines by specifying "whitespace=trail" attribute, and have been relying on it, will now be unprotected. Such a regression/incompatibility should be noted here.
Better yet would be not to introduce such a regression, of course.
...
Quoted 7 lines
Another idea would be to:
- introduce two new settings:
blank-at-eol : SP/HT at EOL is an error
empty-lines-at-eof : empty lines at the end of file is an error
Quoted 8 lines
- make existing "trailing-space" a mere short-hand for "blank-at-eol" and "empty-lines-at-eof".
The "git diff" part looked reasonable from a quick glance, but I do not think I saw anything that affects "git apply --whitespace=fix" in the patch.
Hi all! How appropriate that this was posted so recently. Seems to be a fairly popular issue lately with the recent commits 735c674 and the correction 422a82f.
I was thinking of a fix for the disappearing EOL @ EOF and was hoping for some input and direction, yet wasn't really sure if a new RFH thread would be better than here or not. Until otherwise noted how about just diving in...
My initial posting was to get input on which approach would be best received to give a capability similar to what Junio's idea above states. The problem is which way to best go about it. Obviously if someone is doing whitespace=fix with trailing-space set then they indeed want it fixed, but having the blank lines removed... ?
Currently the section in builtin-apply.c (v1.6.4) that is the culprit for removing these lines is:
1999 if (added_blank_line)
2000 new_blank_lines_at_end++;
2001 else
2002 new_blank_lines_at_end = 0;
Having a --keep-new-blank-lines argument for apply, and am seems like a winner that then doesn't effect regular operations.
1999 if (added_blank_line && !keep_new_blank_lines_at_end)
I've tested having the added_blank_line reset to 0 during testing of whitespace fixing from crlf to lf and it seems good to go for that purpose.
Is that something that I should try for? Ie: having a new arg for am and apply, or is a new core whitespace option the better route?
Sincerely,
Thell (almostautomated on freenode)