whitespace-stripping

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.

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

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

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

On 9/17/07, J. Bruce Fields [off-list ref] 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

On Mon, Sep 17, 2007 at 11:24:12AM +1200, Martin Langhoff wrote:

On 9/17/07, J. Bruce Fields [off-list ref] wrote:

quoted

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.

"J. Bruce Fields" [off-list ref] writes:

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

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.

"J. Bruce Fields" [off-list ref] writes:

On Tue, Sep 18, 2007 at 10:55:25AM +0200, David Kastrup wrote:

quoted

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