whitespace-stripping

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

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

is converted to

	<tab><tab><tab>

when it should be just

	<tab><tab>

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

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

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

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

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

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

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

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

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

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

--b.

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

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

-- 
Krzysztof Halasa

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

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

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

--b.

Martin Langhoff· Sep 16, 2007, 23:24 UTC · re: J. Bruce Fields · lore

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

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.

cheers,

m

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

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.

Junio C Hamano· Sep 17, 2007, 00:24 UTC · re: J. Bruce Fields · lore

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

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

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

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?

--b.

Junio C Hamano· Oct 3, 2007, 01:00 UTC · re: J. Bruce Fields · lore

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

---

 * 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);

David Kastrup· Sep 18, 2007, 08:55 UTC · re: J. Bruce Fields · lore

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

"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

If you have

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

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

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>

--b.

David Kastrup· Sep 18, 2007, 14:18 UTC · re: J. Bruce Fields · lore

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

"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