the war on trailing whitespace

the war on trailing whitespace

[Andrew Morton]

It's invariably pointless to add lines which have trailing whitespace. 
Nobody cares much, but my scripts spam me when it happens, so I've become
obsessive.    Looking at Dave Miller's current net devel tree:

bix:/usr/src/25> grep '^+.*[    ]$' patches/git-net.patch | wc -l
    170

Note that this is purely _added_ trailing whitespace.  I'm not proposing
that the revision control system should care about pre-existing trailing
whitespace.

I got the quilt guys to generate warnings when patches add trailing
whitespace, and to provide the tools to strip it.  And I believe Larry made
similar changes to bk.

I realise that we cannot do this when doing git fetches, but when importing
patches and mboxes, git ought to whine loudly about input which matches the
above regexp, and it should offer an option to tidy it up.  Perhaps by
default.

Of course, this means that the person who sent the patch will find that his
as-yet-unsent patches don't apply to the upstream tree any more.  Well,
that's tough luck - perhaps it'll motivate him to stop adding trailing
whitespace.  The patches will still apply with `patch -l'.

Thanks for listening ;)

Re: the war on trailing whitespace

[Junio C Hamano]

Andrew Morton <akpm@osdl.org> writes:

> It's invariably pointless to add lines which have trailing whitespace. 
> Nobody cares much, but my scripts spam me when it happens, so I've become
> obsessive....

I do not call me obsessive, but I do enable pre-commit and
pre-applypatch hooks I ship with git myself.

> I realise that we cannot do this when doing git fetches, but when importing
> patches and mboxes, git ought to whine loudly about input which matches the
> above regexp, and it should offer an option to tidy it up.  Perhaps by
> default.

I stole the policy the sample hook scripts use from you; it is
not enabled by default, and as the tool manufacturer I am a bit
reluctant to do so.

However, as a kernel project maintainer high in the foodchain,
I'd imagine your plea to your fellow maintainers who apply
patches using git tools would be heard well.

Re: the war on trailing whitespace

[Andrew Morton]

Junio C Hamano <junkio@cox.net> wrote:
>
> Andrew Morton <akpm@osdl.org> writes:
> 
> > It's invariably pointless to add lines which have trailing whitespace. 
> > Nobody cares much, but my scripts spam me when it happens, so I've become
> > obsessive....
> 
> I do not call me obsessive, but I do enable pre-commit and
> pre-applypatch hooks I ship with git myself.

It's apparent that few others do this.

> > I realise that we cannot do this when doing git fetches, but when importing
> > patches and mboxes, git ought to whine loudly about input which matches the
> > above regexp, and it should offer an option to tidy it up.  Perhaps by
> > default.
> 
> I stole the policy the sample hook scripts use from you; it is
> not enabled by default, and as the tool manufacturer I am a bit
> reluctant to do so.
> 

It's not strong enough.  I mean, if you ask a developer "do you wish to add
new trialing whitespace to the kernel" then obviously their answer would be
"no".   So how do we help them in this?

I'd suggest a) git will simply refuse to apply such a patch unless given a
special `forcing' flag, b) even when thus forced, it will still warn and c)
with a different flag, it will strip-then-apply, without generating a
warning.

Re: the war on trailing whitespace

[Linus Torvalds]

On Sat, 25 Feb 2006, Andrew Morton wrote:
> 
> I'd suggest a) git will simply refuse to apply such a patch unless given a
> special `forcing' flag, b) even when thus forced, it will still warn and c)
> with a different flag, it will strip-then-apply, without generating a
> warning.

This doesn't do the "strip-then-apply" thing, but it allows you to make 
git-apply generate a warning or error on extraneous whitespace.

Use --whitespace=warn to warn, and (surprise, surprise) --whitespace=error 
to make it a fatal error to have whitespace at the end.

Totally untested, of course. But it compiles, so it must be fine.

HOWEVER! Note that this literally will check every single patch-line with 
"+" at the beginning. Which means that if you fix a simple typo, and the 
line had a space at the end before, and you didn't remove it, that's still 
considered a "new line with whitespace at the end", even though obviously 
the line wasn't really new.

I assume this is what you wanted, and there isn't really any sane 
alternatives (you could make the warning activate only for _pure_ 
additions with no deletions at all in that hunk, but that sounds a bit 
insane).

		Linus

---
diff --git a/apply.c b/apply.c
index 244718c..e7b3dca 100644
--- a/apply.c
+++ b/apply.c
@@ -34,6 +34,12 @@ static int line_termination = '\n';
 static const char apply_usage[] =
 "git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [-z] [-pNUM] <patch>...";
 
+static enum whitespace_eol {
+	nowarn,
+	warn_on_whitespace,
+	error_on_whitespace
+} new_whitespace = nowarn;
+
 /*
  * For "diff-stat" like behaviour, we keep track of the biggest change
  * we've seen, and the longest filename. That allows us to do simple
@@ -815,6 +821,22 @@ static int parse_fragment(char *line, un
 			oldlines--;
 			break;
 		case '+':
+			/*
+			 * We know len is at least two, since we have a '+' and
+			 * we checked that the last character was a '\n' above
+			 */
+			if (isspace(line[len-2])) {
+				switch (new_whitespace) {
+				case nowarn:
+					break;
+				case warn_on_whitespace:
+					new_whitespace = nowarn;	/* Just once */
+					error("Added whitespace at end of line at line %d", linenr);
+					break;
+				case error_on_whitespace:
+					die("Added whitespace at end of line at line %d", linenr);
+				}
+			}
 			added++;
 			newlines--;
 			break;
@@ -1839,6 +1861,17 @@ int main(int argc, char **argv)
 			line_termination = 0;
 			continue;
 		}
+		if (!strncmp(arg, "--whitespace=", 13)) {
+			if (strcmp(arg+13, "warn")) {
+				new_whitespace = warn_on_whitespace;
+				continue;
+			}
+			if (strcmp(arg+13, "error")) {
+				new_whitespace = error_on_whitespace;
+				continue;
+			}
+			die("unrecognixed whitespace option '%s'", arg+13);
+		}
 
 		if (check_index && prefix_length < 0) {
 			prefix = setup_git_directory();

Re: the war on trailing whitespace

[Andrew Morton]

Linus Torvalds <torvalds@osdl.org> wrote:
>
> 
> 
> On Sat, 25 Feb 2006, Andrew Morton wrote:
> > 
> > I'd suggest a) git will simply refuse to apply such a patch unless given a
> > special `forcing' flag, b) even when thus forced, it will still warn and c)
> > with a different flag, it will strip-then-apply, without generating a
> > warning.
> 
> This doesn't do the "strip-then-apply" thing, but it allows you to make 
> git-apply generate a warning or error on extraneous whitespace.
> 
> Use --whitespace=warn to warn, and (surprise, surprise) --whitespace=error 
> to make it a fatal error to have whitespace at the end.

Thanks.  But it defaults to nowarn.  Nobody will turn it on and nothing
improves.

> Totally untested, of course. But it compiles, so it must be fine.

Who cares, as long as the patch doesn't add trailing whitespace? ) ;)

> HOWEVER! Note that this literally will check every single patch-line with 
> "+" at the beginning. Which means that if you fix a simple typo, and the 
> line had a space at the end before, and you didn't remove it, that's still 
> considered a "new line with whitespace at the end", even though obviously 
> the line wasn't really new.
> 
> I assume this is what you wanted, and there isn't really any sane 
> alternatives (you could make the warning activate only for _pure_ 
> additions with no deletions at all in that hunk, but that sounds a bit 
> insane).

Yup.  So by the time we've patched every line in the kernel, it's
trailing-whitespace-free.

Re: the war on trailing whitespace

[Sam Ravnborg]

On Sun, Feb 26, 2006 at 09:29:00AM -0800, Linus Torvalds wrote:
> 
> 
> On Sat, 25 Feb 2006, Andrew Morton wrote:
> > 
> > I'd suggest a) git will simply refuse to apply such a patch unless given a
> > special `forcing' flag, b) even when thus forced, it will still warn and c)
> > with a different flag, it will strip-then-apply, without generating a
> > warning.
> 
> This doesn't do the "strip-then-apply" thing, but it allows you to make 
> git-apply generate a warning or error on extraneous whitespace.

Can this somehow be done in a way so everyone that clones your tree
will inherit the warn/error on whitespace setting?
In this way we make sure it gets enabled automagically in many trees
and I do not have to remember yet another options.

Alternatively something that is enabled for a tree so I only have to do
something once - a trigger maybe?

	Sam

Re: the war on trailing whitespace

[Linus Torvalds]

On Sun, 26 Feb 2006, Andrew Morton wrote:
> 
> Thanks.  But it defaults to nowarn.  Nobody will turn it on and nothing
> improves.

Few enough people run "git-apply" on its own. Most people (certainly me) 
end up using it through some email-applicator script or other. So the plan 
was that the --whitespace=warn/error flag would go there, and that 
git-apply by default would work more like "patch".

But hey, I have no strong preferences, and it's easy enough to make the 
default be warn (and add a "--whitespace=ok" flag to turn it off).

Personally, I don't mind whitespace that much. In particular, I _suspect_ 
I often have empty lines like

	int i;
	
	i = 10;

where the "empty" line actually has the same indentation as the lines 
around it. Is that wrong? Perhaps.

		Linus

Re: the war on trailing whitespace

[Dave Jones]

On Sun, Feb 26, 2006 at 12:16:25PM -0800, Linus Torvalds wrote:

 > Few enough people run "git-apply" on its own. Most people (certainly me)
 > end up using it through some email-applicator script or other. So the plan
 > was that the --whitespace=warn/error flag would go there, and that
 > git-apply by default would work more like "patch".
 >
 > But hey, I have no strong preferences, and it's easy enough to make the
 > default be warn (and add a "--whitespace=ok" flag to turn it off).
 >
 > Personally, I don't mind whitespace that much. In particular, I _suspect_
 > I often have empty lines like
 >
 >	int i;
 >
 >	i = 10;
 >
 > where the "empty" line actually has the same indentation as the lines
 > around it. Is that wrong? Perhaps.

I think I have the same anal-retentive problem Andrew has, because I have ..

highlight RedundantSpaces term=standout ctermbg=red guibg=red
match RedundantSpaces /\s\+$\| \+\ze\t/

in my .vimrc, which highlights this (and other trailing whitespace) as
a big red blob.  I do this in part for the same reason Andrew does,
so that when someone sends me a diff with a zillion spaces at the EOL,
it screams at me, I spot them, and chop them out.

		Dave

Re: the war on trailing whitespace

[Junio C Hamano]

Andrew Morton <akpm@osdl.org> writes:

> Thanks.  But it defaults to nowarn.  Nobody will turn it on and nothing
> improves.

This WS is clearly a policy, and while I personally agree that
it is a _good_ policy, I am a bit hesitant to hardcode this
stricter policy as the default to lower level tools.

I have a feeling that Linus is saying that pre-applypatch hook
is good enough, and you have to educate people who feed things
to you ;-)

Re: the war on trailing whitespace

[Dave Jones]

On Sun, Feb 26, 2006 at 03:26:17PM -0500, Dave Jones wrote:

 > in my .vimrc, which highlights this (and other trailing whitespace) as
 > a big red blob.  I do this in part for the same reason Andrew does,
 > so that when someone sends me a diff with a zillion spaces at the EOL,
 > it screams at me, I spot them, and chop them out.

(seconds later, I find my .vimrc on master.k.o hasn't had this turned
 on so I find a billion instances of this mess in agp/cpufreq -- Bah).

		Dave

Re: the war on trailing whitespace

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Personally, I don't mind whitespace that much. In particular, I _suspect_ 
> I often have empty lines like
>
> 	int i;
> 	
> 	i = 10;
>
> where the "empty" line actually has the same indentation as the lines 
> around it. Is that wrong? Perhaps.

Yes, you do, and I hand-fixed one a couple of minutes ago ;-).

Regarding git-apply change, I suspect warn_on_whitespace should
not squelch itself after the first one, and error_on_whitespace
should not die instantly.  The sample pre-applypatch hook (it
was missing code to figure out where GIT_DIR was so it never
worked as shipped; corrected in "master") shows line numbers of
suspicious lines from the files being patched.  They can be
manually fixed up, and then "git am --resolved", if the
integrator is in a better mood.

The error messages from pre-commit/pre-applypatch hook mimic the
way compiler errors are spit out, so that it works well in Emacs
compilation buffer -- doing C-x ` (next-error) takes you the
line the error appears and lets you edit it.

Re: the war on trailing whitespace

[MIke Galbraith]

On Sun, 2006-02-26 at 15:26 -0500, Dave Jones wrote:

> I think I have the same anal-retentive problem Andrew has, because I have ..
> 
> highlight RedundantSpaces term=standout ctermbg=red guibg=red
> match RedundantSpaces /\s\+$\| \+\ze\t/
> 
> in my .vimrc, which highlights this (and other trailing whitespace) as
> a big red blob.  I do this in part for the same reason Andrew does,
> so that when someone sends me a diff with a zillion spaces at the EOL,
> it screams at me, I spot them, and chop them out.

Dang, I'm _not_ perfect after all ;-).  I just tried this on a patch of
mine, and up popped a few angry red blobs even though I try to be
careful.  Perhaps someone should add more pet peeve thingies to it and
post it to lkml, or put it some place people can be pointed at.

	-Mike

Re: the war on trailing whitespace

[Johannes Schindelin]

Hi,

there is a good reason not to enable the no-whitespace-at-eol checking in 
pre-commit by default (at least for *all* files) for git development:

	Python.

Just do a "/ $" in git-merge-recursive.py. These whitespaces are not an 
error, but a syntactic *requirement*.

Hth,
Dscho

Re: the war on trailing whitespace

[Andrew Morton]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> Hi,
> 
> there is a good reason not to enable the no-whitespace-at-eol checking in 
> pre-commit by default (at least for *all* files) for git development:
> 
> 	Python.

That's not a good reason.  People will discover that git has started
shouting at them and they'll work out how to make it stop.

The probem is getting C users to turn the check on, not in getting python
users to turn it off.

Re: the war on trailing whitespace

[Adrien Beau]

On 2/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>
> there is a good reason not to enable the no-whitespace-at-eol checking in
> pre-commit by default (at least for *all* files) for git development:
>
>         Python.
>
> Just do a "/ $" in git-merge-recursive.py. These whitespaces are not an
> error, but a syntactic *requirement*.

No, they aren't.

A logical line that contains only spaces and tabs is ignored by
Python. (All the "dirty" lines in git-merge-recursive.py are such
lines.)

Besides, spaces, tabs and newlines can be used interchangeably to
separate tokens, so trailing whitespace is never *required*.

Hope this helps,

Adrien

Re: the war on trailing whitespace

[Andreas Ericsson]

Adrien Beau wrote:
> On 2/27/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
>>there is a good reason not to enable the no-whitespace-at-eol checking in
>>pre-commit by default (at least for *all* files) for git development:
>>
>>        Python.
>>
>>Just do a "/ $" in git-merge-recursive.py. These whitespaces are not an
>>error, but a syntactic *requirement*.
> 
> 
> No, they aren't.
> 
> A logical line that contains only spaces and tabs is ignored by
> Python. (All the "dirty" lines in git-merge-recursive.py are such
> lines.)
> 

I think the question is whether completely empty lines are also ignored 
by Python, or if they start a new block of code. Whatever the case, it 
must hold true for both 2.3 and 2.4.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: the war on trailing whitespace

[Johannes Schindelin]

On Mon, 27 Feb 2006, Adrien Beau wrote:

> A logical line that contains only spaces and tabs is ignored by
> Python. (All the "dirty" lines in git-merge-recursive.py are such
> lines.)
> 
> Hope this helps,

It does.

Thanks,
Dscho

Re: the war on trailing whitespace

[Uwe Zeisberger]

Hello,

Andreas Ericsson wrote:
> I think the question is whether completely empty lines are also ignored 
> by Python, or if they start a new block of code. Whatever the case, it 
> must hold true for both 2.3 and 2.4.
see
	http://www.python.org/doc/2.2.3/ref/blank-lines.html
	http://www.python.org/doc/2.3.5/ref/blank-lines.html
	http://www.python.org/doc/2.4.2/ref/blank-lines.html

Best regards
Uwe

-- 
Uwe Zeisberger

http://www.google.com/search?q=gravity+on+earth%3D

Re: the war on trailing whitespace

[Andreas Ericsson]

Uwe Zeisberger wrote:
> Hello,
> 
> Andreas Ericsson wrote:
> 
>>I think the question is whether completely empty lines are also ignored 
>>by Python, or if they start a new block of code. Whatever the case, it 
>>must hold true for both 2.3 and 2.4.
> 
> see
> 	http://www.python.org/doc/2.2.3/ref/blank-lines.html
> 	http://www.python.org/doc/2.3.5/ref/blank-lines.html
> 	http://www.python.org/doc/2.4.2/ref/blank-lines.html
> 

So in essence, a multi-line statement is closed when a completely empty 
line is found, which means that making git internals recognize and strip 
such lines will result in Python code never being manageable by git.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: the war on trailing whitespace

[Peter Hagervall]

On Mon, Feb 27, 2006 at 03:10:55PM +0100, Andreas Ericsson wrote:
> So in essence, a multi-line statement is closed when a completely empty 
> line is found, which means that making git internals recognize and strip 
> such lines will result in Python code never being manageable by git.
> 

I believe completely empty lines are to be left untouched. The war is on
trailing whitespace.

/ Peter

-- 
Peter Hagervall......................email: hager@cs.umu.se
Department of Computing Science........tel: +46(0)90 786 7018
University of Umeå, SE-901 87 Umeå.....fax: +46(0)90 786 6126

Re: the war on trailing whitespace

[Johannes Schindelin]

Hi,

On Mon, 27 Feb 2006, Peter Hagervall wrote:

> On Mon, Feb 27, 2006 at 03:10:55PM +0100, Andreas Ericsson wrote:
> > So in essence, a multi-line statement is closed when a completely empty 
> > line is found, which means that making git internals recognize and strip 
> > such lines will result in Python code never being manageable by git.
> > 
> 
> I believe completely empty lines are to be left untouched. The war is on
> trailing whitespace.

Exactly. That is what Andreas is saying: if you change a line which 
consists only of trailing whitespace to an empty line, it breaks python 
(or formatting).

Hth,
Dscho

Re: the war on trailing whitespace

[Randal L. Schwartz]

>>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Johannes> Exactly. That is what Andreas is saying: if you change a line which 
Johannes> consists only of trailing whitespace to an empty line, it breaks python 
Johannes> (or formatting).

[insert standard rant about Python treating invisible things significantly,
which is the prime counter-rant to people saying Perl looks like line noise]

[chuckling]

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: the war on trailing whitespace

[Josef Weidendorfer]

On Monday 27 February 2006 15:10, you wrote:
> So in essence, a multi-line statement is closed when a completely empty 
> line is found,

As I read this reference, such handling of completely empty lines is
done only in interactive mode, ie. when python is attached to a terminal...
Git is about storing python scripts, and not "interactive input"; therefore,
this seems to be a non-issue.

I just checked it. The following, with a completely empty line 3, is working
as expected, and not looping on an empty statement:
===================
a = ['cat', 'window', 'defenestrate']
for x in a:

  print x, len(x)
===================

Josef

PS: I am not a python programmer...

Re: the war on trailing whitespace

[Adrien Beau]

On 2/27/06, Andreas Ericsson <ae@op5.se> wrote:
>
> So in essence, a multi-line statement is closed when a completely empty
> line is found, which means that making git internals recognize and strip
> such lines will result in Python code never being manageable by git.

Incorrect. This is only the case in the *interactive* interpreter in
the standard implementation. For source code in general, quoting the
Python Reference Manual:

"A logical line that contains only spaces, tabs, formfeeds and
possibly a comment, is ignored (i.e., no NEWLINE token is generated)."

So such lines, whether completely empty or only apparently so (i.e.
dirty), are ignored, and can be safely cleaned-up.

Adrien

Re: the war on trailing whitespace

[Uwe Zeisberger]

Andreas Ericsson wrote:
> Uwe Zeisberger wrote:
> >Hello,
> >
> >Andreas Ericsson wrote:
> >
> >>I think the question is whether completely empty lines are also ignored 
> >>by Python, or if they start a new block of code. Whatever the case, it 
> >>must hold true for both 2.3 and 2.4.
> >
> >see
> >	http://www.python.org/doc/2.2.3/ref/blank-lines.html
> >	http://www.python.org/doc/2.3.5/ref/blank-lines.html
> >	http://www.python.org/doc/2.4.2/ref/blank-lines.html
> >
> 
> So in essence, a multi-line statement is closed when a completely empty 
> line is found,
Wrong.

A logical line that contains only spaces, tabs, formfeeds and possibly a
comment, is ignored (i.e., no NEWLINE token is generated). During
interactive input of statements, handling of a blank line may differ
depending on the implementation of the read-eval-print loop. In the
standard implementation, an entirely blank logical line (i.e. one
containing not even whitespace or a comment) terminates a multi-line
statement.

To translate that to python:

  if not interactive:
    a line only containing whitespace is ignored.
  else:
    if standard implementation:
      empty line terminates multi-line statement
    else:
      dependent on implementation

i.e. In scripts, lines containing only (zero or more) whitespaces are
ignored.

hth
Uwe

-- 
Uwe Zeisberger

Set the I_WANT_A_BROKEN_PS environment variable to force BSD syntax ...
	-- manpage of procps

Re: the war on trailing whitespace

[Andreas Ericsson]

Uwe Zeisberger wrote:
> Andreas Ericsson wrote:
> 
>>Uwe Zeisberger wrote:
>>
>>>Hello,
>>>
>>>Andreas Ericsson wrote:
>>>
>>>
>>>>I think the question is whether completely empty lines are also ignored 
>>>>by Python, or if they start a new block of code. Whatever the case, it 
>>>>must hold true for both 2.3 and 2.4.
>>>
>>>see
>>>	http://www.python.org/doc/2.2.3/ref/blank-lines.html
>>>	http://www.python.org/doc/2.3.5/ref/blank-lines.html
>>>	http://www.python.org/doc/2.4.2/ref/blank-lines.html
>>>
>>
>>So in essence, a multi-line statement is closed when a completely empty 
>>line is found,
> 
> Wrong.
> 
> i.e. In scripts, lines containing only (zero or more) whitespaces are
> ignored.
> 

I stand corrected. Thrice, even. Voi, voi... :)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: the war on trailing whitespace

[Junio C Hamano]

Andrew Morton <akpm@osdl.org> writes:

> That's not a good reason.  People will discover that git has started
> shouting at them and they'll work out how to make it stop.
>
> The problem is getting C users to turn the check on, not in getting python
> users to turn it off.

This whitespace policy should be at least per-project (people
working on both kernel and other things may have legitimate
reason to want trailing whitespace in the other project), so we
would need some configurability; the problem is *both*.

We could do one of two things, at least.

 - I modify the git-apply that is in the "next" branch further
   to make --whitespace=error the default, and push it out.  You
   convince people who feed things to you to update to *that*
   version or later.

 - I already have the added whitespace detection hook (a fixed
   one that actually matches what I use) shipped with git.  You
   convince people who feed things to you to update to *that*
   version or later, and to enable that hook.

I think you are arguing for the first one.  I am reluctant to do
so because it would not help by itself *anyway*.  In any case
you need to convince people who feed things to you to do
something to prevent later changes fed to you from being
contaminated with trailing whitespaces.

Having said that, I have a third solution, which consists of two
patches that come on top of what are already in "next" branch:

 - apply: squelch excessive errors and --whitespace=error-all
 - apply --whitespace: configuration option.

With these, git-apply used by git-applymbox and git-am would
refuse to apply a patch that adds trailing whitespaces, when the
per-repository configuration is set like this:

        [apply]
                whitespace = error

(Alternatively,

	$ git repo-config apply.whitespace error

would set these lines there for you).

I think there are three kinds of git users.

 * Linus, you, and the kernel subsystem maintainers.  The
   whitespace policy with this version of git-apply (with the
   configuration option set to apply.whitespace=error) gives
   would help these people by enforcing the SubmittingPatches
   and your "perfect patch" requirements.

 * People who feed patches to the above people.  They are helped
   by enabling the pre-commit hook that comes with git to
   conform to the kernel whitespace policy -- they need to be
   educated to do so.

 * People outside of kernel community, using git in projects to
   which the kernel whitespace policy does not have any
   relevance.

While I do consider the kernel folks a lot more important
customers than other users, I have to take flak from the third
kind of users, and to them, authority by Linus or you does not
weigh as much as the first two classes of people.  Making the
default to --whitespace=error means that you are making me
justify this kernel project policy as something applicable to
projects outside the kernel.  That is simply not fair to me.

You have to convince people you work with to update to at least
to this version anyway, so I do not think it is too much to ask
from you, while you are at it, to tell the higher echelon folks
to do:

	$ git repo-config apply.whitespace error

in their repositories (and/or set that in their templates so new
repositories created with git-init-db would inherit it).

Re: the war on trailing whitespace

[Peter Williams]

Junio C Hamano wrote:
> Andrew Morton <akpm@osdl.org> writes:
> 
> 
>>That's not a good reason.  People will discover that git has started
>>shouting at them and they'll work out how to make it stop.
>>
>>The problem is getting C users to turn the check on, not in getting python
>>users to turn it off.
> 
> 
> This whitespace policy should be at least per-project (people
> working on both kernel and other things may have legitimate
> reason to want trailing whitespace in the other project),

I'd be interested to hear these reasons.  My experience is that people 
don't put trailing white space in deliberately (or even tolerate it if 
they notice it) and it's usually there as a side effect of the way their 
text editor works (and that's also the reason that they don't usually 
notice it).  But if my experience is misleading me and there are valid 
reasons for having trailing white space I'm genuinely interested in 
knowing what they are.

> so we
> would need some configurability; the problem is *both*.
> 
> We could do one of two things, at least.
> 
>  - I modify the git-apply that is in the "next" branch further
>    to make --whitespace=error the default, and push it out.  You
>    convince people who feed things to you to update to *that*
>    version or later.
> 
>  - I already have the added whitespace detection hook (a fixed
>    one that actually matches what I use) shipped with git.  You
>    convince people who feed things to you to update to *that*
>    version or later, and to enable that hook.
> 
> I think you are arguing for the first one.  I am reluctant to do
> so because it would not help by itself *anyway*.  In any case
> you need to convince people who feed things to you to do
> something to prevent later changes fed to you from being
> contaminated with trailing whitespaces.
> 
> Having said that, I have a third solution, which consists of two
> patches that come on top of what are already in "next" branch:
> 
>  - apply: squelch excessive errors and --whitespace=error-all
>  - apply --whitespace: configuration option.
> 
> With these, git-apply used by git-applymbox and git-am would
> refuse to apply a patch that adds trailing whitespaces, when the
> per-repository configuration is set like this:
> 
>         [apply]
>                 whitespace = error
> 
> (Alternatively,
> 
> 	$ git repo-config apply.whitespace error
> 
> would set these lines there for you).
> 
> I think there are three kinds of git users.
> 
>  * Linus, you, and the kernel subsystem maintainers.  The
>    whitespace policy with this version of git-apply (with the
>    configuration option set to apply.whitespace=error) gives
>    would help these people by enforcing the SubmittingPatches
>    and your "perfect patch" requirements.
> 
>  * People who feed patches to the above people.  They are helped
>    by enabling the pre-commit hook that comes with git to
>    conform to the kernel whitespace policy -- they need to be
>    educated to do so.
> 
>  * People outside of kernel community, using git in projects to
>    which the kernel whitespace policy does not have any
>    relevance.
> 
> While I do consider the kernel folks a lot more important
> customers than other users, I have to take flak from the third
> kind of users, and to them, authority by Linus or you does not
> weigh as much as the first two classes of people.  Making the
> default to --whitespace=error means that you are making me
> justify this kernel project policy as something applicable to
> projects outside the kernel.  That is simply not fair to me.
> 
> You have to convince people you work with to update to at least
> to this version anyway, so I do not think it is too much to ask
> from you, while you are at it, to tell the higher echelon folks
> to do:
> 
> 	$ git repo-config apply.whitespace error
> 
> in their repositories (and/or set that in their templates so new
> repositories created with git-init-db would inherit it).
> 
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce

Re: the war on trailing whitespace

[Andrew Morton]

Junio C Hamano <junkio@cox.net> wrote:
>
> tell the higher echelon folks
>  to do:
> 
>  	$ git repo-config apply.whitespace error
> 
>  in their repositories

That might be reasonable, some might object to tiny amounts of extra work..

In my setup, trailing whitespace purely causes warnings.  But with a
quilt-style thing, it's trivial to unapply the patch, strip it, reapply.

git is different - I'd imagine that if warnings were generated while an
mbox was being applied, it's too much hassle to go and unapply the mbox,
fix the diffs up and then apply the mbox again.

I'd suggest that git have options to a) generate trailing-whitespace
warnings, b) generate trailing-whitespace errors and c) strip trailing
whitespace while applying.   And that the as-shipped default be a).

Re: the war on trailing whitespace

[Junio C Hamano]

Peter Williams <pwil3058@bigpond.net.au> writes:

>> This whitespace policy should be at least per-project (people
>> working on both kernel and other things may have legitimate
>> reason to want trailing whitespace in the other project),
>
> I'd be interested to hear these reasons.  My experience is that people
> don't put trailing white space in deliberately (or even tolerate it if
> they notice it) and it's usually there as a side effect of the way
> their text editor works (and that's also the reason that they don't
> usually notice it).  But if my experience is misleading me and there
> are valid reasons for having trailing white space I'm genuinely
> interested in knowing what they are.

For example:

	http://compsoc.dur.ac.uk/whitespace/

Jokes aside, I can imagine people want to keep format=flowed
text messages (i.e. not programming language source code) under
git control.  Maybe pulling and pushing would be the default
mode of operation for those people, so what git-apply does would
not be in the picture for those people, but who knows.

One way to find it out is to push out a strict one and see who
screams ;-), but the point is I am reluctant to make a stricter
policy the default, thinking, but not knowing as a fact, that it
is good enough for everybody.

Re: the war on trailing whitespace

[linux]

The only language I know of where the presence of whitespace on blank
lines matters is make(1).  Even there, it's subtle.  It's okay to have
(using "cat -e" syntax)

foo : bar$
	command$
$
	command$

But there's a difference between

foo : bar$
	$

which specifies an empty command and will therefore not use a default rule, and

foo : bar$
$

which does not specify any command and so will use a default rule if one exists.


(Of course, you can also get [ \t]\n inside an arbitrary binary file, too.)