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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Bruno Haible <bruno@xxxxxxxxx> 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@xxxxxxxxx>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]