whitespace-stripping

whitespace-stripping

[J. Bruce Fields]

The following patches fix one (probably rare) bug in

        git apply --whitespace=strip

and then teach it to also complain about initial consecutive spaces that
could be tabs.

The latter is the standard for the kernel, but may not be appropriate
for other projects.  I'd like to make the whitespace code handle the
kernel style completely first, then consider configuration to handle
other styles if people complain.  But maybe the change of behavior would
be an unpleasant surprise for someone with apply.whitespace=strip and a
project that always uses spaces for indents.

--b.

[PATCH 1/3] git-apply: fix whitespace stripping

[J. Bruce Fields]

The algorithm isn't right here: it accumulates any set of 8 spaces into
tabs even if they're separated by tabs, so

	<four spaces><tab><four spaces><tab>

is converted to

	<tab><tab><tab>

when it should be just

	<tab><tab>

So teach git-apply that a tab hides any group of less than 8 previous
spaces in a row.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 builtin-apply.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 976ec77..70359c1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1642,15 +1642,22 @@ static int apply_line(char *output, const char *patch, int plen)
 
 	buf = output;
 	if (need_fix_leading_space) {
+		int consecutive_spaces = 0;
 		/* between patch[1..last_tab_in_indent] strip the
 		 * funny spaces, updating them to tab as needed.
 		 */
 		for (i = 1; i < last_tab_in_indent; i++, plen--) {
 			char ch = patch[i];
-			if (ch != ' ')
+			if (ch != ' ') {
+				consecutive_spaces = 0;
 				*output++ = ch;
-			else if ((i % 8) == 0)
-				*output++ = '\t';
+			} else {
+				consecutive_spaces++;
+				if (consecutive_spaces == 8) {
+					*output++ = '\t';
+					consecutive_spaces = 0;
+				}
+			}
 		}
 		fixed = 1;
 		i = last_tab_in_indent;
-- 
1.5.3.1.42.gfe5df

[PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent

[J. Bruce Fields]

Complain if we find 8 spaces or more in a row as part of the initial
whitespace on a line, and (with --whitespace=stripspace) replace such by
a tab.

Well, linux's checkpatch.pl complains about this sort of thing.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 builtin-apply.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 70359c1..fb63089 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -918,6 +918,7 @@ static void check_whitespace(const char *line, int len)
 {
 	const char *err = "Adds trailing whitespace";
 	int seen_space = 0;
+	int consecutive_spaces = 0;
 	int i;
 
 	/*
@@ -944,6 +945,18 @@ static void check_whitespace(const char *line, int len)
 		else
 			break;
 	}
+
+	err = "Initial indent contains eight or more spaces in a row";
+	for (i = 1; i < len; i++) {
+		if (line[i] == ' ')
+			consecutive_spaces++;
+		else if (line[i] == '\t')
+			consecutive_spaces = 0;
+		else
+			break;
+		if (consecutive_spaces == 8)
+			goto error;
+	}
 	return;
 
  error:
@@ -1607,9 +1620,10 @@ static int apply_line(char *output, const char *patch, int plen)
 	int i;
 	int add_nl_to_tail = 0;
 	int fixed = 0;
-	int last_tab_in_indent = -1;
+	int after_indent = -1;
 	int last_space_in_indent = -1;
 	int need_fix_leading_space = 0;
+	int consecutive_spaces = 0;
 	char *buf;
 
 	if ((new_whitespace != strip_whitespace) || !whitespace_error ||
@@ -1630,23 +1644,27 @@ static int apply_line(char *output, const char *patch, int plen)
 	for (i = 1; i < plen; i++) {
 		char ch = patch[i];
 		if (ch == '\t') {
-			last_tab_in_indent = i;
+			consecutive_spaces = 0;
 			if (0 <= last_space_in_indent)
 				need_fix_leading_space = 1;
 		}
-		else if (ch == ' ')
+		else if (ch == ' ') {
+			consecutive_spaces++;
 			last_space_in_indent = i;
-		else
+		} else
 			break;
+		if (consecutive_spaces == 8)
+			need_fix_leading_space = 1;
 	}
+	after_indent=i;
 
 	buf = output;
 	if (need_fix_leading_space) {
-		int consecutive_spaces = 0;
+		consecutive_spaces = 0;
 		/* between patch[1..last_tab_in_indent] strip the
 		 * funny spaces, updating them to tab as needed.
 		 */
-		for (i = 1; i < last_tab_in_indent; i++, plen--) {
+		for (i = 1; i < after_indent; i++, plen--) {
 			char ch = patch[i];
 			if (ch != ' ') {
 				consecutive_spaces = 0;
@@ -1660,7 +1678,9 @@ static int apply_line(char *output, const char *patch, int plen)
 			}
 		}
 		fixed = 1;
-		i = last_tab_in_indent;
+		i = after_indent;
+		i -= consecutive_spaces;
+		plen += consecutive_spaces;
 	}
 	else
 		i = 1;
-- 
1.5.3.1.42.gfe5df

[PATCH 3/3] git-apply: add tests for stripping of leading and trailing whitespace

[J. Bruce Fields]

Add tests to make sure we strip leading and trailing whitespace correctly.

Of the four tests, the first two should always have passed, the third
requires the "fix whitespace stripping" patch, and the fourth requires
the "complain about >= 8 consecutive spaces in initial indent" patch.

Note that this patch itself adds leading and trailing whitespace.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 t/t4124-apply-whitespace-strip.sh |   43 +++++++++++++++++++++++++++++++++++++
 t/t4124/1-after                   |    3 ++
 t/t4124/1-before                  |    3 ++
 t/t4124/2-after                   |    3 ++
 t/t4124/2-before                  |    3 ++
 t/t4124/3-after                   |    1 +
 t/t4124/3-before                  |    1 +
 t/t4124/4-after                   |    5 ++++
 t/t4124/4-before                  |    5 ++++
 9 files changed, 67 insertions(+), 0 deletions(-)
 create mode 100644 t/t4124-apply-whitespace-strip.sh
 create mode 100644 t/t4124/1-after
 create mode 100644 t/t4124/1-before
 create mode 100644 t/t4124/2-after
 create mode 100644 t/t4124/2-before
 create mode 100644 t/t4124/3-after
 create mode 100644 t/t4124/3-before
 create mode 100644 t/t4124/4-after
 create mode 100644 t/t4124/4-before

diff --git a/t/t4124-apply-whitespace-strip.sh b/t/t4124-apply-whitespace-strip.sh
new file mode 100644
index 0000000..3b5f58b
--- /dev/null
+++ b/t/t4124-apply-whitespace-strip.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='handle space and tab combinations with --whitespace=strip'
+
+. ./test-lib.sh
+
+# The directory t4124/ contains pairs of files "n-before" and "n-after",
+# identicaly except leading and trailing whitespace are stripped from
+# the latter.
+#
+# Check that we strip whitespace correctly by checking that the diff
+# between the two files, applied to the first (with --whitespace=strip)
+# produces the second.
+
+mkpatch () {
+	cp "$1" foo
+	git diff /dev/null foo >patch
+	rm foo
+}
+
+checkstrip () {
+	mkpatch "../t4124/$1-before"
+	git apply --whitespace=strip patch
+	git diff foo "../t4124/$1-after"
+}
+
+test_expect_success \
+	'trailing tabs and spaces' \
+	'checkstrip 1'
+
+test_expect_success \
+	'spaces before tabs' \
+	'checkstrip 2' 
+
+test_expect_success \
+	'8 or more non-consecutive initial spaces' \
+	'checkstrip 3'
+
+test_expect_success \
+	'8 or more consecutive initial spaces' \
+	'checkstrip 4'
+
+test_done
diff --git a/t/t4124/1-after b/t/t4124/1-after
new file mode 100644
index 0000000..cf5dfce
--- /dev/null
+++ b/t/t4124/1-after
@@ -0,0 +1,3 @@
+trailing space
+trailing tab
+trailing spaces and tabs
diff --git a/t/t4124/1-before b/t/t4124/1-before
new file mode 100644
index 0000000..1f2505b
--- /dev/null
+++ b/t/t4124/1-before
@@ -0,0 +1,3 @@
+trailing space 
+trailing tab 
+trailing spaces and tabs 	 	 	
diff --git a/t/t4124/2-after b/t/t4124/2-after
new file mode 100644
index 0000000..f198144
--- /dev/null
+++ b/t/t4124/2-after
@@ -0,0 +1,3 @@
+	space tab
+	space space tab
+		tab space tab
diff --git a/t/t4124/2-before b/t/t4124/2-before
new file mode 100644
index 0000000..8fc35bb
--- /dev/null
+++ b/t/t4124/2-before
@@ -0,0 +1,3 @@
+ 	space tab
+  	space space tab
+	 	tab space tab
diff --git a/t/t4124/3-after b/t/t4124/3-after
new file mode 100644
index 0000000..4db0e80
--- /dev/null
+++ b/t/t4124/3-after
@@ -0,0 +1 @@
+		4 spaces, tab, 4 spaces, tab
diff --git a/t/t4124/3-before b/t/t4124/3-before
new file mode 100644
index 0000000..f0e2b9c
--- /dev/null
+++ b/t/t4124/3-before
@@ -0,0 +1 @@
+    	    	4 spaces, tab, 4 spaces, tab
diff --git a/t/t4124/4-after b/t/t4124/4-after
new file mode 100644
index 0000000..a9b8cf6
--- /dev/null
+++ b/t/t4124/4-after
@@ -0,0 +1,5 @@
+       7 spaces
+	8 spaces
+	 9 spaces
+		tab 8 spaces
+		 tab 9 spaces
diff --git a/t/t4124/4-before b/t/t4124/4-before
new file mode 100644
index 0000000..a35b624
--- /dev/null
+++ b/t/t4124/4-before
@@ -0,0 +1,5 @@
+       7 spaces
+        8 spaces
+         9 spaces
+	        tab 8 spaces
+	         tab 9 spaces
-- 
1.5.3.1.42.gfe5df

Re: [PATCH 3/3] git-apply: add tests for stripping of leading and trailing whitespace

[Krzysztof Halasa]

"J. Bruce Fields" <bfields@citi.umich.edu> writes:

> +test_expect_success \
> +	'8 or more consecutive initial spaces' \
> +	'checkstrip 4'

It may be valid, some projects use tabs for indentation and spaces
for alignment, e.g.:

	if (cond && (cond1 ||
	             cond2))
		...

The second line is actually:
	             cond2))
<TAB--->SSSSSSSSSSSSScond2))
where 'S' means space.

This is the only way to write code which display correctly with
different tab sizes.

With tab = 4 spaces it would be expanded to:
    if (cond && (cond1 ||
                 cond2))
        ...
I.e., it would be still fine.


Most of the formating tools probably can't do it automatically.
-- 
Krzysztof Halasa

Re: [PATCH 3/3] git-apply: add tests for stripping of leading and trailing whitespace

[J. Bruce Fields]

On Mon, Sep 17, 2007 at 04:16:07PM +0200, Krzysztof Halasa wrote:
> "J. Bruce Fields" <bfields@citi.umich.edu> writes:
> 
> > +test_expect_success \
> > +	'8 or more consecutive initial spaces' \
> > +	'checkstrip 4'
> 
> It may be valid, some projects use tabs for indentation and spaces
> for alignment, e.g.:

Yeah, I know.  I was hoping that the stripspace behavior was already
specific enough to the linux-kernel style that we could just assume that 
it was only used by developers on projects with the same style.  I agree
that I was wrong--apologies.

--b.

Re: [PATCH 3/3] git-apply: add tests for stripping of leading and trailing whitespace

[Krzysztof Halasa]

"J. Bruce Fields" <bfields@fieldses.org> writes:

>> It may be valid, some projects use tabs for indentation and spaces
>> for alignment, e.g.:
>
> Yeah, I know.  I was hoping that the stripspace behavior was already
> specific enough to the linux-kernel style that we could just assume that 
> it was only used by developers on projects with the same style.

Actually I would consider linux kernel an example of such project
with spaces for alignment. Except that current tools are not to
the task. Someday...

Obviously other cases are valid, especially 'spaces before tabs'
which IMHO includes '8 or more non-consecutive initial spaces'.
And all trailing whitespace of course.
-- 
Krzysztof Halasa

Re: [PATCH 3/3] git-apply: add tests for stripping of leading and trailing whitespace

[J. Bruce Fields]

On Tue, Sep 18, 2007 at 01:44:46AM +0200, Krzysztof Halasa wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> >> It may be valid, some projects use tabs for indentation and spaces
> >> for alignment, e.g.:
> >
> > Yeah, I know.  I was hoping that the stripspace behavior was already
> > specific enough to the linux-kernel style that we could just assume that 
> > it was only used by developers on projects with the same style.
> 
> Actually I would consider linux kernel an example of such project
> with spaces for alignment. Except that current tools are not to
> the task. Someday...

I guess I haven't followed previous arguments on the subject, but based
on checkpatch.pl I was assuming the tabs-whenever-possible policy had
won out.

Anyway, I can only care about whitespace for so long....

--b.

Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent

[Martin Langhoff]

On 9/17/07, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> Complain if we find 8 spaces or more in a row as part of the initial
> whitespace on a line, and (with --whitespace=stripspace) replace such by
> a tab.

I do quite a bit of hacking on "spaces-for-indentation" projects and
still use stripspace to cleanup my patches. So no, thanks.

Perhaps split it off to a separate option? I'm not opposed to the
functionality per-se, but don't put together with
trailing-space-trimming. It's a different beast. Everyone agrees
trimming trailing spaces as much as everyone disagrees on
tabs-vs-spaces.

cheers,



m

Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent

[J. Bruce Fields]

On Mon, Sep 17, 2007 at 11:24:12AM +1200, Martin Langhoff wrote:
> On 9/17/07, J. Bruce Fields <bfields@citi.umich.edu> wrote:
> > Complain if we find 8 spaces or more in a row as part of the initial
> > whitespace on a line, and (with --whitespace=stripspace) replace such by
> > a tab.
> 
> I do quite a bit of hacking on "spaces-for-indentation" projects and
> still use stripspace to cleanup my patches. So no, thanks.

OK, fair enough.

--b.

Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent

[Junio C Hamano]

"J. Bruce Fields" <bfields@citi.umich.edu> writes:

> Complain if we find 8 spaces or more in a row as part of the initial
> whitespace on a line, and (with --whitespace=stripspace) replace such by
> a tab.
>
> Well, linux's checkpatch.pl complains about this sort of thing.

Some people program in Python, so I am afraid that this needs to
be a separate option.

Maybe it is time to redo the --whitespace options as bitmasks so
that we can say --whitespace-fix=tab,tail,lines to pick and
choose which kinds of breakage to fix?

Re: [PATCH 2/3] git-apply: complain about >=8 consecutive spaces in initial indent

[J. Bruce Fields]

On Sun, Sep 16, 2007 at 05:24:31PM -0700, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@citi.umich.edu> writes:
> 
> > Complain if we find 8 spaces or more in a row as part of the initial
> > whitespace on a line, and (with --whitespace=stripspace) replace such by
> > a tab.
> >
> > Well, linux's checkpatch.pl complains about this sort of thing.
> 
> Some people program in Python, so I am afraid that this needs to
> be a separate option.

OK.

> Maybe it is time to redo the --whitespace options as bitmasks so
> that we can say --whitespace-fix=tab,tail,lines to pick and
> choose which kinds of breakage to fix?

OK.  Or maybe keep the current commandline options and have a
whitespace-style config option someplace?

I'm afraid I won't get to either anytime soon, though, so that project's
up for grabs....

--b.

[PATCH] git-diff: complain about >=8 consecutive spaces in initial indent

[Junio C Hamano]

This teaches coloring code in "diff" to detect indent of 8 or
more places using SP, which can and should (in some projects
including the kernel and git itself) use HT instead.

---

 * This is primarily meant as a "reminder" patch, and not for
   inclusion.  We earlier saw a patch to "git-apply" to rewrite
   them to HT but rejected it, because some projects use "no HT,
   all SP" policy (e.g. Python).

   We probably should resurrect the earlier "git-apply" patch,
   and teach it and this patch to selectively enable/disable
   detection of different kinds of whitespace breakages.

 diff.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 0ee9ea1..647377b 100644
--- a/diff.c
+++ b/diff.c
@@ -531,8 +531,10 @@ static void emit_line_with_ws(int nparents,
 	int i;
 	int tail = len;
 	int need_highlight_leading_space = 0;
-	/* The line is a newly added line.  Does it have funny leading
-	 * whitespaces?  In indent, SP should never precede a TAB.
+	/*
+	 * The line is a newly added line.  Does it have funny leading
+	 * whitespaces?  In indent, SP should never precede a TAB, and
+	 * there shouldn't be more than 8 consecutive spaces.
 	 */
 	for (i = col0; i < len; i++) {
 		if (line[i] == '\t') {
@@ -545,6 +547,11 @@ static void emit_line_with_ws(int nparents,
 		else
 			break;
 	}
+	if (0 <= last_space_in_indent && last_tab_in_indent < 0 &&
+	    8 <= (i - col0)) {
+		last_tab_in_indent = i;
+		need_highlight_leading_space = 1;
+	}
 	fputs(set, stdout);
 	fwrite(line, col0, 1, stdout);
 	fputs(reset, stdout);

Re: [PATCH 1/3] git-apply: fix whitespace stripping

[David Kastrup]

"J. Bruce Fields" <bfields@citi.umich.edu> writes:

> The algorithm isn't right here: it accumulates any set of 8 spaces into
> tabs even if they're separated by tabs, so
>
> 	<four spaces><tab><four spaces><tab>
>
> is converted to
>
> 	<tab><tab><tab>
>
> when it should be just
>
> 	<tab><tab>
>
> So teach git-apply that a tab hides any group of less than 8 previous
> spaces in a row.
>
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  builtin-apply.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 976ec77..70359c1 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -1642,15 +1642,22 @@ static int apply_line(char *output, const char *patch, int plen)
>  
>  	buf = output;
>  	if (need_fix_leading_space) {
> +		int consecutive_spaces = 0;
>  		/* between patch[1..last_tab_in_indent] strip the
>  		 * funny spaces, updating them to tab as needed.
>  		 */
>  		for (i = 1; i < last_tab_in_indent; i++, plen--) {
>  			char ch = patch[i];
> -			if (ch != ' ')
> +			if (ch != ' ') {
> +				consecutive_spaces = 0;
>  				*output++ = ch;
> -			else if ((i % 8) == 0)
> -				*output++ = '\t';
> +			} else {
> +				consecutive_spaces++;
> +				if (consecutive_spaces == 8) {
> +					*output++ = '\t';
> +					consecutive_spaces = 0;
> +				}
> +			}
>  		}
>  		fixed = 1;
>  		i = last_tab_in_indent;
> -- 
> 1.5.3.1.42.gfe5df

As far as I can see, this does not really work since it does not
maintain an idea of a current column.

If you have

abcd<four spaces><tab><four spaces><tab>

then indeed the resulting conversion needs to be <tab><tab><tab>
whereas with

abc<four spaces><tab><four spaces><tab>

the resulting conversion needs to be just <tab><tab>


-- 
David Kastrup

Re: [PATCH 1/3] git-apply: fix whitespace stripping

[J. Bruce Fields]

On Tue, Sep 18, 2007 at 10:55:25AM +0200, David Kastrup wrote:
> As far as I can see, this does not really work since it does not
> maintain an idea of a current column.
> 
> If you have
> 
> abcd<four spaces><tab><four spaces><tab>
> 
> then indeed the resulting conversion needs to be <tab><tab><tab>
> whereas with
> 
> abc<four spaces><tab><four spaces><tab>
> 
> the resulting conversion needs to be just <tab><tab>

Note that this code *only* handles whitespace in the initial indent;
processing stops as soon as it hits anything other than a tab or an
indent.

Given that, I believe the proposed patch is correct.  Am I missing
something else?

--b.

Re: [PATCH 1/3] git-apply: fix whitespace stripping

[David Kastrup]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Tue, Sep 18, 2007 at 10:55:25AM +0200, David Kastrup wrote:
>> As far as I can see, this does not really work since it does not
>> maintain an idea of a current column.
>> 
>> If you have
>> 
>> abcd<four spaces><tab><four spaces><tab>
>> 
>> then indeed the resulting conversion needs to be <tab><tab><tab>
>> whereas with
>> 
>> abc<four spaces><tab><four spaces><tab>
>> 
>> the resulting conversion needs to be just <tab><tab>
>
> Note that this code *only* handles whitespace in the initial indent;
> processing stops as soon as it hits anything other than a tab or an
> indent.
>
> Given that, I believe the proposed patch is correct.  Am I missing
> something else?

Don't think so.  Sorry for the noise.

-- 
David Kastrup