[PATCH] New whitespace checking category 'trailing-blank-line'

[PATCH] New whitespace checking category 'trailing-blank-line'

[Bruno Haible]

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.


>From 049db23a38c92c734aae13788a5a9478ed587cfd Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
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(-)

diff --git a/Documentation/RelNotes-1.6.4.txt b/Documentation/RelNotes-1.6.4.txt
index b3c0346..9ebcc3a 100644
--- a/Documentation/RelNotes-1.6.4.txt
+++ b/Documentation/RelNotes-1.6.4.txt
@@ -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
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6857d2f..e9221ba 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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.
diff --git a/cache.h b/cache.h
index e6c7f33..24ae981 100644
--- a/cache.h
+++ b/cache.h
@@ -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 --git a/diff.c b/diff.c
index cd35e0c..6d1b07b 100644
--- a/diff.c
+++ b/diff.c
@@ -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);
diff --git a/ws.c b/ws.c
index 59d0883..5f5a930 100644
--- a/ws.c
+++ b/ws.c
@@ -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);
 }
 
-- 
1.6.3.2

Re: [PATCH] New whitespace checking category 'trailing-blank-line'

[Junio C Hamano]

Bruno Haible <bruno@clisp.org> writes:

> 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.

> 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.

> From: Bruno Haible <bruno@clisp.org>
> 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.

> ---

And please sign-off your patch before the three-dash line.

> diff --git a/Documentation/RelNotes-1.6.4.txt b/Documentation/RelNotes-1.6.4.txt
> index b3c0346..9ebcc3a 100644
> --- a/Documentation/RelNotes-1.6.4.txt
> +++ b/Documentation/RelNotes-1.6.4.txt
> @@ -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).

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 6857d2f..e9221ba 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -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.

Re: [PATCH] New whitespace checking category 'trailing-blank-line'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

Re: [PATCH] New whitespace checking category 'trailing-blank-line'

[Thell]

Junio C Hamano <gitster <at> pobox.com> writes:

> 
> Bruno Haible <bruno <at> clisp.org> writes:
> 
...
> > 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.
> 
> 
> 
... 
> 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".
> 
> 
> 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)