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

From: Thell <tbfowler4@gmail.com>
Date: Jul 30, 2009, 23:47 UTC
Message-ID: <loom.20090730T230729-582@post.gmane.org>
In-Reply-To: <7vbpn7p1mw.fsf@alter.siamese.dyndns.org>

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

> 
> Bruno Haible <bruno <at> clisp.org> writes:
>

...

Show 13 quoted 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.
> 
> 
>

...

Show 15 quoted 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
> 
>     - 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)