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.

[PATCH] apply --whitespace fixes and enhancements.

[Junio C Hamano]

In addition to fixing obvious command line parsing bugs in the
previous round, this changes the following:

 * Adds "--whitespace=strip".  This applies after stripping the
   new trailing whitespaces introduced to the patch.

 * The output error message format is changed to say
   "patch-filename:linenumber:contents of the line".  This makes
   it similar to typical compiler error message format, and
   helps C-x ` (next-error) in Emacs compilation buffer.

 * --whitespace=error and --whitespace=warn do not stop at the
   first error.  We might want to limit the output to say first
   20 such lines to prevent cluttering, but on the other hand if
   you are willing to hand-fix after inspecting them, getting
   everything with a single run might be easier to work with.
   After all, somebody has to do the clean-up work somewhere.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 Junio C Hamano <junkio@cox.net> writes:

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

 apply.c |   77 ++++++++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 54 insertions(+), 23 deletions(-)

e0af70a72d4115c32a1f9b91f1cf4556bbd014b6
diff --git a/apply.c b/apply.c
index e7b3dca..7dbbeb4 100644
--- a/apply.c
+++ b/apply.c
@@ -37,8 +37,11 @@ static const char apply_usage[] =
 static enum whitespace_eol {
 	nowarn,
 	warn_on_whitespace,
-	error_on_whitespace
+	error_on_whitespace,
+	strip_and_apply,
 } new_whitespace = nowarn;
+static int whitespace_error = 0;
+static const char *patch_input_file = NULL;
 
 /*
  * For "diff-stat" like behaviour, we keep track of the biggest change
@@ -823,19 +826,17 @@ static int parse_fragment(char *line, un
 		case '+':
 			/*
 			 * We know len is at least two, since we have a '+' and
-			 * we checked that the last character was a '\n' above
+			 * we checked that the last character was a '\n' above.
+			 * That is, an addition of an empty line would check
+			 * the '+' here.  Sneaky...
 			 */
-			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);
-				}
+			if ((new_whitespace != nowarn) &&
+			    isspace(line[len-2])) {
+				fprintf(stderr, "Added whitespace\n");
+				fprintf(stderr, "%s:%d:%.*s\n",
+					patch_input_file,
+					linenr, len-2, line+1);
+				whitespace_error = 1;
 			}
 			added++;
 			newlines--;
@@ -1114,6 +1115,27 @@ struct buffer_desc {
 	unsigned long alloc;
 };
 
+static int apply_line(char *output, const char *patch, int plen)
+{
+	/* plen is number of bytes to be copied from patch,
+	 * starting at patch+1 (patch[0] is '+').  Typically
+	 * patch[plen] is '\n'.
+	 */
+	int add_nl_to_tail = 0;
+	if ((new_whitespace == strip_and_apply) &&
+	    1 < plen && isspace(patch[plen-1])) {
+		if (patch[plen] == '\n')
+			add_nl_to_tail = 1;
+		plen--;
+		while (0 < plen && isspace(patch[plen]))
+			plen--;
+	}
+	memcpy(output, patch + 1, plen);
+	if (add_nl_to_tail)
+		output[plen++] = '\n';
+	return plen;
+}
+
 static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag)
 {
 	char *buf = desc->buffer;
@@ -1149,10 +1171,9 @@ static int apply_one_fragment(struct buf
 				break;
 		/* Fall-through for ' ' */
 		case '+':
-			if (*patch != '+' || !no_add) {
-				memcpy(new + newsize, patch + 1, plen);
-				newsize += plen;
-			}
+			if (*patch != '+' || !no_add)
+				newsize += apply_line(new + newsize, patch,
+						      plen);
 			break;
 		case '@': case '\\':
 			/* Ignore it, we already handled it */
@@ -1721,7 +1742,7 @@ static int use_patch(struct patch *p)
 	return 1;
 }
 
-static int apply_patch(int fd)
+static int apply_patch(int fd, const char *filename)
 {
 	int newfd;
 	unsigned long offset, size;
@@ -1729,6 +1750,7 @@ static int apply_patch(int fd)
 	struct patch *list = NULL, **listp = &list;
 	int skipped_patch = 0;
 
+	patch_input_file = filename;
 	if (!buffer)
 		return -1;
 	offset = 0;
@@ -1755,6 +1777,9 @@ static int apply_patch(int fd)
 	}
 
 	newfd = -1;
+	if (whitespace_error && (new_whitespace == error_on_whitespace))
+		apply = 0;
+
 	write_index = check_index && apply;
 	if (write_index)
 		newfd = hold_index_file_for_update(&cache_file, get_index_file());
@@ -1801,7 +1826,7 @@ int main(int argc, char **argv)
 		int fd;
 
 		if (!strcmp(arg, "-")) {
-			apply_patch(0);
+			apply_patch(0, "<stdin>");
 			read_stdin = 0;
 			continue;
 		}
@@ -1862,14 +1887,18 @@ int main(int argc, char **argv)
 			continue;
 		}
 		if (!strncmp(arg, "--whitespace=", 13)) {
-			if (strcmp(arg+13, "warn")) {
+			if (!strcmp(arg+13, "warn")) {
 				new_whitespace = warn_on_whitespace;
 				continue;
 			}
-			if (strcmp(arg+13, "error")) {
+			if (!strcmp(arg+13, "error")) {
 				new_whitespace = error_on_whitespace;
 				continue;
 			}
+			if (!strcmp(arg+13, "strip")) {
+				new_whitespace = strip_and_apply;
+				continue;
+			}
 			die("unrecognixed whitespace option '%s'", arg+13);
 		}
 
@@ -1885,10 +1914,12 @@ int main(int argc, char **argv)
 		if (fd < 0)
 			usage(apply_usage);
 		read_stdin = 0;
-		apply_patch(fd);
+		apply_patch(fd, arg);
 		close(fd);
 	}
 	if (read_stdin)
-		apply_patch(0);
+		apply_patch(0, "<stdin>");
+	if (whitespace_error && new_whitespace == error_on_whitespace)
+		return 1;
 	return 0;
 }
-- 
1.2.3.gac5f

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.

[PATCH 1/3] apply: squelch excessive errors and --whitespace=error-all

[Junio C Hamano]

This by default makes --whitespace=warn, error, and strip to
warn only the first 5 additions of trailing whitespaces.  A new
option --whitespace=error-all can be used to view all of them
before applying.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 * This is already in "next".

 apply.c |   53 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 45 insertions(+), 8 deletions(-)

fc96b7c9ba5034a408d508c663a96a15b8f8729c
diff --git a/apply.c b/apply.c
index 7dbbeb4..8139d83 100644
--- a/apply.c
+++ b/apply.c
@@ -41,6 +41,8 @@ static enum whitespace_eol {
 	strip_and_apply,
 } new_whitespace = nowarn;
 static int whitespace_error = 0;
+static int squelch_whitespace_errors = 5;
+static int applied_after_stripping = 0;
 static const char *patch_input_file = NULL;
 
 /*
@@ -832,11 +834,16 @@ static int parse_fragment(char *line, un
 			 */
 			if ((new_whitespace != nowarn) &&
 			    isspace(line[len-2])) {
-				fprintf(stderr, "Added whitespace\n");
-				fprintf(stderr, "%s:%d:%.*s\n",
-					patch_input_file,
-					linenr, len-2, line+1);
-				whitespace_error = 1;
+				whitespace_error++;
+				if (squelch_whitespace_errors &&
+				    squelch_whitespace_errors <
+				    whitespace_error)
+					;
+				else {
+					fprintf(stderr, "Adds trailing whitespace.\n%s:%d:%.*s\n",
+						patch_input_file,
+						linenr, len-2, line+1);
+				}
 			}
 			added++;
 			newlines--;
@@ -1129,6 +1136,7 @@ static int apply_line(char *output, cons
 		plen--;
 		while (0 < plen && isspace(patch[plen]))
 			plen--;
+		applied_after_stripping++;
 	}
 	memcpy(output, patch + 1, plen);
 	if (add_nl_to_tail)
@@ -1895,11 +1903,16 @@ int main(int argc, char **argv)
 				new_whitespace = error_on_whitespace;
 				continue;
 			}
+			if (!strcmp(arg+13, "error-all")) {
+				new_whitespace = error_on_whitespace;
+				squelch_whitespace_errors = 0;
+				continue;
+			}
 			if (!strcmp(arg+13, "strip")) {
 				new_whitespace = strip_and_apply;
 				continue;
 			}
-			die("unrecognixed whitespace option '%s'", arg+13);
+			die("unrecognized whitespace option '%s'", arg+13);
 		}
 
 		if (check_index && prefix_length < 0) {
@@ -1919,7 +1932,31 @@ int main(int argc, char **argv)
 	}
 	if (read_stdin)
 		apply_patch(0, "<stdin>");
-	if (whitespace_error && new_whitespace == error_on_whitespace)
-		return 1;
+	if (whitespace_error) {
+		if (squelch_whitespace_errors &&
+		    squelch_whitespace_errors < whitespace_error) {
+			int squelched =
+				whitespace_error - squelch_whitespace_errors;
+			fprintf(stderr, "warning: squelched %d whitespace error%s\n",
+				squelched,
+				squelched == 1 ? "" : "s");
+		}
+		if (new_whitespace == error_on_whitespace)
+			die("%d line%s add%s trailing whitespaces.",
+			    whitespace_error,
+			    whitespace_error == 1 ? "" : "s",
+			    whitespace_error == 1 ? "s" : "");
+		if (applied_after_stripping)
+			fprintf(stderr, "warning: %d line%s applied after"
+				" stripping trailing whitespaces.\n",
+				applied_after_stripping,
+				applied_after_stripping == 1 ? "" : "s");
+		else if (whitespace_error)
+			fprintf(stderr, "warning: %d line%s add%s trailing"
+				" whitespaces.\n",
+				whitespace_error,
+				whitespace_error == 1 ? "" : "s",
+				whitespace_error == 1 ? "s" : "");
+	}
 	return 0;
 }
-- 
1.2.3.gbfea

[PATCH 2/3] apply --whitespace: configuration option.

[Junio C Hamano]

The new configuration option apply.whitespace can take one of
"warn", "error", "error-all", or "strip".  When git-apply is run
to apply the patch to the index, they are used as the default
value if there is no command line --whitespace option.

Andrew can now tell people who feed him git trees to update to
this version and say:

	git repo-config apply.whitespace error

Signed-off-by: Junio C Hamano <junkio@cox.net>

---
 * Already in "next".

 apply.c       |   72 ++++++++++++++++++++++++++++++++++++++-------------------
 cache.h       |    2 ++
 environment.c |    1 +
 3 files changed, 51 insertions(+), 24 deletions(-)

2ae1c53b51ff78b13cc8abf8e9798a12140b7638
diff --git a/apply.c b/apply.c
index 8139d83..a5cdd8e 100644
--- a/apply.c
+++ b/apply.c
@@ -35,16 +35,42 @@ 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,
+	nowarn_whitespace,
 	warn_on_whitespace,
 	error_on_whitespace,
-	strip_and_apply,
-} new_whitespace = nowarn;
+	strip_whitespace,
+} new_whitespace = nowarn_whitespace;
 static int whitespace_error = 0;
 static int squelch_whitespace_errors = 5;
 static int applied_after_stripping = 0;
 static const char *patch_input_file = NULL;
 
+static void parse_whitespace_option(const char *option)
+{
+	if (!option) {
+		new_whitespace = nowarn_whitespace;
+		return;
+	}
+	if (!strcmp(option, "warn")) {
+		new_whitespace = warn_on_whitespace;
+		return;
+	}
+	if (!strcmp(option, "error")) {
+		new_whitespace = error_on_whitespace;
+		return;
+	}
+	if (!strcmp(option, "error-all")) {
+		new_whitespace = error_on_whitespace;
+		squelch_whitespace_errors = 0;
+		return;
+	}
+	if (!strcmp(option, "strip")) {
+		new_whitespace = strip_whitespace;
+		return;
+	}
+	die("unrecognized whitespace option '%s'", option);
+}
+
 /*
  * 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
@@ -832,7 +858,7 @@ static int parse_fragment(char *line, un
 			 * That is, an addition of an empty line would check
 			 * the '+' here.  Sneaky...
 			 */
-			if ((new_whitespace != nowarn) &&
+			if ((new_whitespace != nowarn_whitespace) &&
 			    isspace(line[len-2])) {
 				whitespace_error++;
 				if (squelch_whitespace_errors &&
@@ -1129,7 +1155,7 @@ static int apply_line(char *output, cons
 	 * patch[plen] is '\n'.
 	 */
 	int add_nl_to_tail = 0;
-	if ((new_whitespace == strip_and_apply) &&
+	if ((new_whitespace == strip_whitespace) &&
 	    1 < plen && isspace(patch[plen-1])) {
 		if (patch[plen] == '\n')
 			add_nl_to_tail = 1;
@@ -1824,10 +1850,21 @@ static int apply_patch(int fd, const cha
 	return 0;
 }
 
+static int git_apply_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "apply.whitespace")) {
+		apply_default_whitespace = strdup(value);
+		return 0;
+	}
+	return git_default_config(var, value);
+}
+
+
 int main(int argc, char **argv)
 {
 	int i;
 	int read_stdin = 1;
+	const char *whitespace_option = NULL;
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
@@ -1895,30 +1932,17 @@ int main(int argc, char **argv)
 			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;
-			}
-			if (!strcmp(arg+13, "error-all")) {
-				new_whitespace = error_on_whitespace;
-				squelch_whitespace_errors = 0;
-				continue;
-			}
-			if (!strcmp(arg+13, "strip")) {
-				new_whitespace = strip_and_apply;
-				continue;
-			}
-			die("unrecognized whitespace option '%s'", arg+13);
+			whitespace_option = arg + 13;
+			parse_whitespace_option(arg + 13);
+			continue;
 		}
 
 		if (check_index && prefix_length < 0) {
 			prefix = setup_git_directory();
 			prefix_length = prefix ? strlen(prefix) : 0;
-			git_config(git_default_config);
+			git_config(git_apply_config);
+			if (!whitespace_option && apply_default_whitespace)
+				parse_whitespace_option(apply_default_whitespace);
 		}
 		if (0 < prefix_length)
 			arg = prefix_filename(prefix, prefix_length, arg);
diff --git a/cache.h b/cache.h
index 58eec00..0d3b244 100644
--- a/cache.h
+++ b/cache.h
@@ -161,11 +161,13 @@ extern int hold_index_file_for_update(st
 extern int commit_index_file(struct cache_file *);
 extern void rollback_index_file(struct cache_file *);
 
+/* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int assume_unchanged;
 extern int only_use_symrefs;
 extern int diff_rename_limit_default;
 extern int shared_repository;
+extern const char *apply_default_whitespace;
 
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
diff --git a/environment.c b/environment.c
index 251e53c..16c08f0 100644
--- a/environment.c
+++ b/environment.c
@@ -17,6 +17,7 @@ int only_use_symrefs = 0;
 int repository_format_version = 0;
 char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8";
 int shared_repository = 0;
+const char *apply_default_whitespace = NULL;
 
 static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir,
 	*git_graft_file;
-- 
1.2.3.gbfea

[PATCH 3/3] git-apply --whitespace=nowarn

[Junio C Hamano]

Andrew insists --whitespace=warn should be the default, and I
tend to agree.  This introduces --whitespace=warn, so if your
project policy is more lenient, you can squelch them by having
apply.whitespace=nowarn in your configuration file.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---
 * Not in "next" but will be shortly.

 apply.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

114b085dd7b82c3ca74760c896e86c425127cf76
diff --git a/apply.c b/apply.c
index a5cdd8e..d5cb5b1 100644
--- a/apply.c
+++ b/apply.c
@@ -39,7 +39,7 @@ static enum whitespace_eol {
 	warn_on_whitespace,
 	error_on_whitespace,
 	strip_whitespace,
-} new_whitespace = nowarn_whitespace;
+} new_whitespace = warn_on_whitespace;
 static int whitespace_error = 0;
 static int squelch_whitespace_errors = 5;
 static int applied_after_stripping = 0;
@@ -55,6 +55,10 @@ static void parse_whitespace_option(cons
 		new_whitespace = warn_on_whitespace;
 		return;
 	}
+	if (!strcmp(option, "nowarn")) {
+		new_whitespace = nowarn_whitespace;
+		return;
+	}
 	if (!strcmp(option, "error")) {
 		new_whitespace = error_on_whitespace;
 		return;
-- 
1.2.3.gbfea

Re: [PATCH 3/3] git-apply --whitespace=nowarn

[A Large Angry SCM]

Junio C Hamano wrote:
> Andrew insists --whitespace=warn should be the default, and I
> tend to agree.  This introduces --whitespace=warn, so if your
> project policy is more lenient, you can squelch them by having
> apply.whitespace=nowarn in your configuration file.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

I think this is wrong. The default policy of, non-security, tools SHOULD 
  be the least restrictive and most flexible.

Re: [PATCH 3/3] git-apply --whitespace=nowarn

[Junio C Hamano]

A Large Angry SCM <gitzilla@gmail.com> writes:

> Junio C Hamano wrote:
>> Andrew insists --whitespace=warn should be the default, and I
>> tend to agree.  This introduces --whitespace=warn, so if your
>> project policy is more lenient, you can squelch them by having
>> apply.whitespace=nowarn in your configuration file.
>> Signed-off-by: Junio C Hamano <junkio@cox.net>
>
> I think this is wrong. The default policy of, non-security, tools
> SHOULD be the least restrictive and most flexible.

The reason is that --whitespace=warn does not refuse to apply
but spits out some warning messages.  Admittedly, going from
zero, any amount of new warning message makes it infinitely more
chatty, but then people can squelch it by having the
configuration option "apply.whitespace = nowarn".

[PATCH] git-apply: war on whitespace -- finishing touches.

[Junio C Hamano]

Andrew Morton <akpm@osdl.org> writes:

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

I've done this and will be pushing it out to "master" branch on
my next git day (Wednesday, west coast US); "maint" branch will
have the same for v1.2.4 sometime by the end of this week.

There is one thing.  By making --whitespace=warn the default,
the diffstat output people would see after "git pull" would also
show the warning message.  I personally do not think this is a
problem (you will know how dirty a tree you are merging into
your tree), but it might not be a bad idea to explicitly squelch
it by making it not to warn when we are not applying.

-- >8 --

This changes the default --whitespace policy to nowarn when we
are only getting --stat, --summary etc. IOW when not applying
the patch.  When applying the patch, the default is warn (spit
out warning message but apply the patch).

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 apply.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

f21d6726150ec4219e94ea605f27a4cd58eb3d99
diff --git a/apply.c b/apply.c
index c4ff418..9deb206 100644
--- a/apply.c
+++ b/apply.c
@@ -75,6 +75,15 @@ static void parse_whitespace_option(cons
 	die("unrecognized whitespace option '%s'", option);
 }
 
+static void set_default_whitespace_mode(const char *whitespace_option)
+{
+	if (!whitespace_option && !apply_default_whitespace) {
+		new_whitespace = (apply
+				  ? warn_on_whitespace
+				  : nowarn_whitespace);
+	}
+}
+
 /*
  * 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
@@ -1955,9 +1964,11 @@ int main(int argc, char **argv)
 		if (fd < 0)
 			usage(apply_usage);
 		read_stdin = 0;
+		set_default_whitespace_mode(whitespace_option);
 		apply_patch(fd, arg);
 		close(fd);
 	}
+	set_default_whitespace_mode(whitespace_option);
 	if (read_stdin)
 		apply_patch(0, "<stdin>");
 	if (whitespace_error) {
-- 
1.2.3.g1da2

Re: [PATCH 2/3] apply --whitespace: configuration option.

[Andreas Ericsson]

Junio C Hamano wrote:
> The new configuration option apply.whitespace can take one of
> "warn", "error", "error-all", or "strip".  When git-apply is run
> to apply the patch to the index, they are used as the default
> value if there is no command line --whitespace option.
> 

I would think "warn-all" would be the logical thing, since "error" 
either breaks out early or prints all warnings before denying the patch 
anyway.

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

Re: [PATCH 2/3] apply --whitespace: configuration option.

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Junio C Hamano wrote:
>> The new configuration option apply.whitespace can take one of
>> "warn", "error", "error-all", or "strip".  When git-apply is run
>> to apply the patch to the index, they are used as the default
>> value if there is no command line --whitespace option.
>
> I would think "warn-all" would be the logical thing, since "error"
> either breaks out early or prints all warnings before denying the
> patch anyway.

Actually there is some thinking behind why I did not do warn-all.
I did consider it at first but rejected.

 * If you are a busy top echelon person but cares about tree
   cleanliness, --whitespace=error is good enough.  The patch is
   rejected on WS basis whether it introduces one such trailing
   WS or hundreds.  The patch is returned to the submitter and
   the tree remains clean.

 * --whitespace=warn-all, if existed, would apply the patch
   _anyway_, so if you notice you got warnings, and if that
   bothers you enough that you would want to do something about
   it, you will have to rewind the HEAD, fix up .dotest/patch
   and reapply.  This means you are willing to clean up other
   peoples' patches.

 * But if you are that kind of person, --whitespace=error-all is
   a better choice for you.  Your tree stays clean and you do
   not have to rewind.  Instead, you get all the errors you can
   go through with your editor (e.g. Emacs users can use C-x `;
   I hope vim users have similar macros) and fix things.

 * --whitespace=warn would show some, but not all, so that you
   can continue while making a mental note to scold the patch
   submitter to be careful the next time.  You chose "warn" to
   apply the patch anyway, so there is no point showing the full
   extent of damage -- the damage is already done to your tree.

 * --whitespace=strip is for people who care about cleanliness,
   who wants to be nice to the submitters, but not nice enough
   to educate them.  They do not want to fix things by hand.
   Instead they have the tool to do the fixing for them.

The last one is somewhat risky, and the output may need to be
examined carefully depending on the contents (e.g. programming
language) the project is dealing with.

Re: [PATCH 2/3] apply --whitespace: configuration option.

[Andreas Ericsson]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> 
>>Junio C Hamano wrote:
>>
>>>The new configuration option apply.whitespace can take one of
>>>"warn", "error", "error-all", or "strip".  When git-apply is run
>>>to apply the patch to the index, they are used as the default
>>>value if there is no command line --whitespace option.
>>
>>I would think "warn-all" would be the logical thing, since "error"
>>either breaks out early or prints all warnings before denying the
>>patch anyway.
> 
> 
> Actually there is some thinking behind why I did not do warn-all.
> I did consider it at first but rejected.
> 
>  * If you are a busy top echelon person but cares about tree
>    cleanliness, --whitespace=error is good enough.  The patch is
>    rejected on WS basis whether it introduces one such trailing
>    WS or hundreds.  The patch is returned to the submitter and
>    the tree remains clean.
> 
>  * --whitespace=warn-all, if existed, would apply the patch
>    _anyway_, so if you notice you got warnings, and if that
>    bothers you enough that you would want to do something about
>    it, you will have to rewind the HEAD, fix up .dotest/patch
>    and reapply.  This means you are willing to clean up other
>    peoples' patches.
> 
>  * But if you are that kind of person, --whitespace=error-all is
>    a better choice for you.  Your tree stays clean and you do
>    not have to rewind.  Instead, you get all the errors you can
>    go through with your editor (e.g. Emacs users can use C-x `;
>    I hope vim users have similar macros) and fix things.
> 

Good Thinking. Thanks for explaining.

> 
> The last one is somewhat risky, and the output may need to be
> examined carefully depending on the contents (e.g. programming
> language) the project is dealing with.
> 
> 

echo Makefile >> .git/no-ws-strip
echo '*.[ch]' >> .git/ws-strip

Perhaps not viable, and probably stupid as well. Mixed content repos 
would likely just keep the 'warn' policy.

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