Re: [PATCH] t/.gitattributes: only ignore whitespace errors in test files

9 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] t/.gitattributes: only ignore whitespace errors in test files

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:44

Jeff King [off-list ref] writes:
On Fri, Jun 13, 2008 at 12:35:59AM +0200, Lea Wiemann wrote:
quoted
Only ignore whitespace errors in t/tNNNN-*.sh and the t/tNNNN
subdirectories.  Other files (like test libraries) should still be
checked.
Why?

What is the difference between test-lib.sh and tNNNN-*.sh that makes one
subject to whitespace checking and the other not?
Eventually we would want to make all of the t/*.sh not exempt from the
whitespace rules.  Some currently do have trailing whitespaces as part of
their embedded test vectors, but there are many that are more carefully
written to avoid trailing whitespaces, by marking the EOL explicitly with
a non whitespace characters in the source, and running sed to produce the
actual vector that is used in the test.  That style is vastly preferrable
than having actual lines that end with trailing whitespaces, because it
makes it much clearer what is being fed to the scripts and what are
expected output when reading the source.  You do not have to "cat -e" to
see what they exactly do.

So I think this is one step in the right direction.  I do not want to keep
tNNNN-*.sh exemption forever.

Re: [PATCH] t/.gitattributes: only ignore whitespace errors in test files

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

On Fri, Jun 13, 2008 at 03:00:35AM -0700, Junio C Hamano wrote:
Eventually we would want to make all of the t/*.sh not exempt from the
whitespace rules.  Some currently do have trailing whitespaces as part of
This turned out to be a fairly easy change, as most of the places had
been caught already.

Four part patch series follows. The only one potentially not
maint-worthy is 3/4, because it actually changes git's output slightly.

  1/4: fix whitespace violations in test scripts
  2/4: mask necessary whitespace policy violations in test scripts
  3/4: avoid trailing whitespace in zero-change diffstat lines
  4/4: enable whitespace checking of test scripts

-Peff

[PATCH 1/4] fix whitespace violations in test scripts

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

These violations are simply wrong, but were never caught
because whitespace policy checking is turned off in the test
scripts.

Signed-off-by: Jeff King <redacted>
---
 t/t1502-rev-parse-parseopt.sh |    2 +-
 t/t3800-mktag.sh              |    2 +-
 t/t3903-stash.sh              |    2 +-
 t/t4014-format-patch.sh       |    6 +++---
 t/t4150-am.sh                 |    2 +-
 t/t5540-http-push.sh          |    2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index d24a47d..7cdd70a 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -5,7 +5,7 @@ test_description='test git rev-parse --parseopt'
 
 cat > expect.err <<EOF
 usage: some-command [options] <args>...
-    
+
     some-command does foo and bar!
 
     -h, --help            show the help
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index df1fd6f..3907e67 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -245,7 +245,7 @@ cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
-tagger T A Gger <tagger@example.com>  
+tagger T A Gger <tagger@example.com>
 
 EOF
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2d3ee3b..54d99ed 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -41,7 +41,7 @@ test_expect_success 'apply needs clean working directory' '
 	echo 4 > other-file &&
 	git add other-file &&
 	echo 5 > other-file &&
- 	test_must_fail git stash apply
+	test_must_fail git stash apply
 '
 
 test_expect_success 'apply stashed changes' '
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 3583e68..7fe853c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -98,7 +98,7 @@ test_expect_success 'extra headers' '
 	sed -e "/^$/q" patch2 > hdrs2 &&
 	grep "^To: R. E. Cipient <rcipient@example.com>$" hdrs2 &&
 	grep "^Cc: S. E. Cipient <scipient@example.com>$" hdrs2
-	
+
 '
 
 test_expect_success 'extra headers without newlines' '
@@ -109,7 +109,7 @@ test_expect_success 'extra headers without newlines' '
 	sed -e "/^$/q" patch3 > hdrs3 &&
 	grep "^To: R. E. Cipient <rcipient@example.com>$" hdrs3 &&
 	grep "^Cc: S. E. Cipient <scipient@example.com>$" hdrs3
-	
+
 '
 
 test_expect_success 'extra headers with multiple To:s' '
@@ -170,7 +170,7 @@ test_expect_success 'thread cover-letter' '
 	git checkout side &&
 	git format-patch --cover-letter --thread -o patches/ master &&
 	FIRST_MID=$(grep "Message-Id:" patches/0000-* | sed "s/^[^<]*\(<[^>]*>\).*$/\1/") &&
-	for i in patches/0001-* patches/0002-* patches/0003-* 
+	for i in patches/0001-* patches/0002-* patches/0003-*
 	do
 	  grep "References: $FIRST_MID" $i &&
 	  grep "In-Reply-To: $FIRST_MID" $i || break
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 722ae96..bc98260 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -110,7 +110,7 @@ test_expect_success 'am applies patch correctly' '
 
 GIT_AUTHOR_NAME="Another Thor"
 GIT_AUTHOR_EMAIL="a.thor@example.com"
-GIT_COMMITTER_NAME="Co M Miter" 
+GIT_COMMITTER_NAME="Co M Miter"
 GIT_COMMITTER_EMAIL="c.miter@example.com"
 export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL
 
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 7372439..f15dd03 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -38,7 +38,7 @@ test_expect_success 'setup remote repository' '
 	cd - &&
 	mv test_repo.git $HTTPD_DOCUMENT_ROOT_PATH
 '
-	
+
 test_expect_success 'clone remote repository' '
 	cd "$ROOT_PATH" &&
 	git clone $HTTPD_URL/test_repo.git test_repo_clone
-- 
1.5.6.rc2.183.g04614

[PATCH 2/4] mask necessary whitespace policy violations in test scripts

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

All of these violations are necessary parts of the tests
(which are generally checking the behavior of trailing
whitespace, or contain diff fragments with empty lines).

Our solution is two-fold:

  1. Process input with whitespace problems using tr. This
     has the added bonus that it becomes very obvious where
     the bogus whitespace is intended to go.

  2. Move large diff fragments into their own supplemental
     files. This gets rid of the whitespace problem, since
     supplemental files are not checked, and it also makes
     the test script a bit easier to read.

Signed-off-by: Jeff King <redacted>
---
Actually, the diff fragments could simply use empty lines to indicate an
empty context line, instead of a single space. This matches the GNU diff
format which Linus added support for a while back. However, I think
this change actually makes the test script more readable, and there is
something wrong to me about taking advantage of a diff convention that
we complained so much about at the time. ;)

 t/t4015-diff-whitespace.sh |    8 +-
 t/t4109-apply-multifrag.sh |  132 +------------------------------------------
 t/t4109/patch1.patch       |   28 +++++++++
 t/t4109/patch2.patch       |   30 ++++++++++
 t/t4109/patch3.patch       |   31 ++++++++++
 t/t4109/patch4.patch       |   30 ++++++++++
 t/t4119-apply-config.sh    |    4 +-
 7 files changed, 129 insertions(+), 134 deletions(-)
 create mode 100644 t/t4109/patch1.patch
 create mode 100644 t/t4109/patch2.patch
 create mode 100644 t/t4109/patch3.patch
 create mode 100644 t/t4109/patch4.patch
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index ca0302f..b7cc6b2 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -62,16 +62,16 @@ EOF
 
 git update-index x
 
-cat << EOF > x
+tr '_' ' ' << EOF > x
 	whitespace at beginning
 whitespace 	 change
 white space in the middle
-whitespace at end  
+whitespace at end__
 unchanged line
 CR at end
 EOF
 
-tr 'Q' '\015' << EOF > expect
+tr 'Q_' '\015 ' << EOF > expect
 diff --git a/x b/x
 index d99af23..8b32fb5 100644
 --- a/x
@@ -84,7 +84,7 @@ index d99af23..8b32fb5 100644
 +	whitespace at beginning
 +whitespace 	 change
 +white space in the middle
-+whitespace at end  
++whitespace at end__
  unchanged line
 -CR at endQ
 +CR at end
diff --git a/t/t4109-apply-multifrag.sh b/t/t4109-apply-multifrag.sh
index bd40a21..ff5fdf3 100755
--- a/t/t4109-apply-multifrag.sh
+++ b/t/t4109-apply-multifrag.sh
@@ -9,134 +9,10 @@ test_description='git apply test patches with multiple fragments.
 '
 . ./test-lib.sh
 
-# setup
-
-cat > patch1.patch <<\EOF
-diff --git a/main.c b/main.c
-new file mode 100644
---- /dev/null
-+++ b/main.c
-@@ -0,0 +1,23 @@
-+#include <stdio.h>
-+
-+int func(int num);
-+void print_int(int num);
-+
-+int main() {
-+	int i;
-+
-+	for (i = 0; i < 10; i++) {
-+		print_int(func(i));
-+	}
-+
-+	return 0;
-+}
-+
-+int func(int num) {
-+	return num * num;
-+}
-+
-+void print_int(int num) {
-+	printf("%d", num);
-+}
-+
-EOF
-cat > patch2.patch <<\EOF
-diff --git a/main.c b/main.c
---- a/main.c
-+++ b/main.c
-@@ -1,7 +1,9 @@
-+#include <stdlib.h>
- #include <stdio.h>
- 
- int func(int num);
- void print_int(int num);
-+void print_ln();
- 
- int main() {
- 	int i;
-@@ -10,6 +12,8 @@
- 		print_int(func(i));
- 	}
- 
-+	print_ln();
-+
- 	return 0;
- }
- 
-@@ -21,3 +25,7 @@
- 	printf("%d", num);
- }
- 
-+void print_ln() {
-+	printf("\n");
-+}
-+
-EOF
-cat > patch3.patch <<\EOF
-diff --git a/main.c b/main.c
---- a/main.c
-+++ b/main.c
-@@ -1,9 +1,7 @@
--#include <stdlib.h>
- #include <stdio.h>
- 
- int func(int num);
- void print_int(int num);
--void print_ln();
- 
- int main() {
- 	int i;
-@@ -12,8 +10,6 @@
- 		print_int(func(i));
- 	}
- 
--	print_ln();
--
- 	return 0;
- }
- 
-@@ -25,7 +21,3 @@
- 	printf("%d", num);
- }
- 
--void print_ln() {
--	printf("\n");
--}
--
-EOF
-cat > patch4.patch <<\EOF
-diff --git a/main.c b/main.c
---- a/main.c
-+++ b/main.c
-@@ -1,13 +1,14 @@
- #include <stdio.h>
- 
- int func(int num);
--void print_int(int num);
-+int func2(int num);
- 
- int main() {
- 	int i;
- 
- 	for (i = 0; i < 10; i++) {
--		print_int(func(i));
-+		printf("%d", func(i));
-+		printf("%d", func3(i));
- 	}
- 
- 	return 0;
-@@ -17,7 +18,7 @@
- 	return num * num;
- }
- 
--void print_int(int num) {
--	printf("%d", num);
-+int func2(int num) {
-+	return num * num * num;
- }
- 
-EOF
+cp ../t4109/patch1.patch .
+cp ../t4109/patch2.patch .
+cp ../t4109/patch3.patch .
+cp ../t4109/patch4.patch .
 
 test_expect_success "S = git apply (1)" \
     'git apply patch1.patch patch2.patch'
diff --git a/t/t4109/patch1.patch b/t/t4109/patch1.patch
new file mode 100644
index 0000000..1d411fc
--- /dev/null
+++ b/t/t4109/patch1.patch
@@ -0,0 +1,28 @@
+diff --git a/main.c b/main.c
+new file mode 100644
+--- /dev/null
++++ b/main.c
+@@ -0,0 +1,23 @@
++#include <stdio.h>
++
++int func(int num);
++void print_int(int num);
++
++int main() {
++	int i;
++
++	for (i = 0; i < 10; i++) {
++		print_int(func(i));
++	}
++
++	return 0;
++}
++
++int func(int num) {
++	return num * num;
++}
++
++void print_int(int num) {
++	printf("%d", num);
++}
++
diff --git a/t/t4109/patch2.patch b/t/t4109/patch2.patch
new file mode 100644
index 0000000..8c6b06d
--- /dev/null
+++ b/t/t4109/patch2.patch
@@ -0,0 +1,30 @@
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -1,7 +1,9 @@
++#include <stdlib.h>
+ #include <stdio.h>
+ 
+ int func(int num);
+ void print_int(int num);
++void print_ln();
+ 
+ int main() {
+ 	int i;
+@@ -10,6 +12,8 @@
+ 		print_int(func(i));
+ 	}
+ 
++	print_ln();
++
+ 	return 0;
+ }
+ 
+@@ -21,3 +25,7 @@
+ 	printf("%d", num);
+ }
+ 
++void print_ln() {
++	printf("\n");
++}
++
diff --git a/t/t4109/patch3.patch b/t/t4109/patch3.patch
new file mode 100644
index 0000000..d696c55
--- /dev/null
+++ b/t/t4109/patch3.patch
@@ -0,0 +1,31 @@
+cat > patch3.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -1,9 +1,7 @@
+-#include <stdlib.h>
+ #include <stdio.h>
+ 
+ int func(int num);
+ void print_int(int num);
+-void print_ln();
+ 
+ int main() {
+ 	int i;
+@@ -12,8 +10,6 @@
+ 		print_int(func(i));
+ 	}
+ 
+-	print_ln();
+-
+ 	return 0;
+ }
+ 
+@@ -25,7 +21,3 @@
+ 	printf("%d", num);
+ }
+ 
+-void print_ln() {
+-	printf("\n");
+-}
+-
diff --git a/t/t4109/patch4.patch b/t/t4109/patch4.patch
new file mode 100644
index 0000000..4b08590
--- /dev/null
+++ b/t/t4109/patch4.patch
@@ -0,0 +1,30 @@
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -1,13 +1,14 @@
+ #include <stdio.h>
+ 
+ int func(int num);
+-void print_int(int num);
++int func2(int num);
+ 
+ int main() {
+ 	int i;
+ 
+ 	for (i = 0; i < 10; i++) {
+-		print_int(func(i));
++		printf("%d", func(i));
++		printf("%d", func3(i));
+ 	}
+ 
+ 	return 0;
+@@ -17,7 +18,7 @@
+ 	return num * num;
+ }
+ 
+-void print_int(int num) {
+-	printf("%d", num);
++int func2(int num) {
++	return num * num * num;
+ }
+ 
diff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh
index b540f72..3c73a78 100755
--- a/t/t4119-apply-config.sh
+++ b/t/t4119-apply-config.sh
@@ -19,12 +19,12 @@ test_expect_success setup '
 '
 
 # Also handcraft GNU diff output; note this has trailing whitespace.
-cat >gpatch.file <<\EOF &&
+tr '_' ' ' >gpatch.file <<\EOF &&
 --- file1	2007-02-21 01:04:24.000000000 -0800
 +++ file1+	2007-02-21 01:07:44.000000000 -0800
 @@ -1 +1 @@
 -A
-+B 
++B_
 EOF
 
 sed -e 's|file1|sub/&|' gpatch.file >gpatch-sub.file &&
-- 
1.5.6.rc2.183.g04614

[PATCH 3/4] avoid trailing whitespace in zero-change diffstat lines

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

In some cases, we produce a diffstat line even though no
lines have changed (e.g., because of an exact rename). In
this case, there is no +/- "graph" after the number of
changed lines. However, we output the space separator
unconditionally, meaning that these lines contained a
trailing space character.

This isn't a huge problem, but in cleaning up the output we
are able to eliminate some trailing whitespace from a test
vector.

Signed-off-by: Jeff King <redacted>
---
I can't imagine any programs actually depend on the trailing space, but
the output change does make this the only contentious patch.

 diff.c                |    3 ++-
 t/t4016-diff-quote.sh |   14 +++++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 62fdc54..f77f9e9 100644
--- a/diff.c
+++ b/diff.c
@@ -922,7 +922,8 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 			total = add + del;
 		}
 		show_name(options->file, prefix, name, len, reset, set);
-		fprintf(options->file, "%5d ", added + deleted);
+		fprintf(options->file, "%5d%s", added + deleted,
+				added + deleted ? " " : "");
 		show_graph(options->file, '+', add, add_c, reset);
 		show_graph(options->file, '-', del, del_c, reset);
 		fprintf(options->file, "\n");
diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh
index 0950250..f07035a 100755
--- a/t/t4016-diff-quote.sh
+++ b/t/t4016-diff-quote.sh
@@ -53,13 +53,13 @@ test_expect_success 'git diff --summary -M HEAD' '
 '
 
 cat >expect <<\EOF
- pathname.1 => "Rpathname\twith HT.0"            |    0 
- pathname.3 => "Rpathname\nwith LF.0"            |    0 
- "pathname\twith HT.3" => "Rpathname\nwith LF.1" |    0 
- pathname.2 => Rpathname with SP.0               |    0 
- "pathname\twith HT.2" => Rpathname with SP.1    |    0 
- pathname.0 => Rpathname.0                       |    0 
- "pathname\twith HT.0" => Rpathname.1            |    0 
+ pathname.1 => "Rpathname\twith HT.0"            |    0
+ pathname.3 => "Rpathname\nwith LF.0"            |    0
+ "pathname\twith HT.3" => "Rpathname\nwith LF.1" |    0
+ pathname.2 => Rpathname with SP.0               |    0
+ "pathname\twith HT.2" => Rpathname with SP.1    |    0
+ pathname.0 => Rpathname.0                       |    0
+ "pathname\twith HT.0" => Rpathname.1            |    0
  7 files changed, 0 insertions(+), 0 deletions(-)
 EOF
 test_expect_success 'git diff --stat -M HEAD' '
-- 
1.5.6.rc2.183.g04614

[PATCH 4/4] enable whitespace checking of test scripts

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

Now that all of the policy violations have been cleaned up,
we can turn this on and start checking incoming patches.

Signed-off-by: Jeff King <redacted>
---
Check and mate, whitespace.

 t/.gitattributes |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/t/.gitattributes b/t/.gitattributes
index ab6edbf..1b97c54 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -1,2 +1 @@
-t[0-9][0-9][0-9][0-9]-*.sh -whitespace
 t[0-9][0-9][0-9][0-9]/* -whitespace
-- 
1.5.6.rc2.183.g04614

Re: [PATCH 1/4] fix whitespace violations in test scripts

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

On Sat, Jun 14, 2008 at 02:51:19AM -0400, Jeff King wrote:
quoted hunk
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -5,7 +5,7 @@ test_description='test git rev-parse --parseopt'
 
 cat > expect.err <<EOF
 usage: some-command [options] <args>...
-    
+
     some-command does foo and bar!
 
     -h, --help            show the help
<sigh> I thought I had run all of the tests after this, but obviously I
screwed up. This is not a correct change.

I will send out a respun patch in a second.

-Peff

Re: [PATCH 3/4] avoid trailing whitespace in zero-change diffstat lines

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:44

Jeff King [off-list ref] writes:
In some cases, we produce a diffstat line even though no lines have
changed (e.g., because of an exact rename). In this case, there is no
+/- "graph" after the number of changed lines. However, we output the
space separator unconditionally, meaning that these lines contained a
trailing space character.

This isn't a huge problem, but in cleaning up the output we are able to
eliminate some trailing whitespace from a test vector.
This is why I love your patches.  Not merely fixing superficial issues but
doing so with _real thinking_ ;-)

Re: [PATCH 3/4] avoid trailing whitespace in zero-change diffstat lines

From: Jeff King <hidden>
Date: 2016-06-15 22:44:44

On Sat, Jun 14, 2008 at 12:22:38AM -0700, Junio C Hamano wrote:
quoted
This isn't a huge problem, but in cleaning up the output we are able to
eliminate some trailing whitespace from a test vector.
This is why I love your patches.  Not merely fixing superficial issues but
doing so with _real thinking_ ;-)
Heh. I am still trying to remove the brown paper bag from sending
patches to the test scripts that don't even pass!

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help