login
Search
Only ignore whitespace errors in t/tNNNN-*.sh and the t/tNNNN
subdirectories. Other files (like test libraries) should still be
checked.
Also fix a whitespace error in t/test-lib.sh.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
t/.gitattributes | 3 ++-
t/test-lib.sh | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/.gitattributes b/t/.gitattributes
index 562b12e..ab6edbf 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -1 +1,2 @@
-* -whitespace
+t[0-9][0-9][0-9][0-9]-*.sh -whitespace
+t[0-9][0-9][0-9][0-9]/* -whitespace
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 2a08cdc..73079d8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -168,7 +168,7 @@ trap 'die' exit
# environment variables to work around this.
#
# In particular, quoting isn't enough, as the path may contain the same quote
-# that we're using.
+# that we're using.
test_set_editor () {
FAKE_EDITOR="$1"
export FAKE_EDITOR
--
1.5.6.rc2.23.gfef6b.dirty
--
Why? What is the difference between test-lib.sh and tNNNN-*.sh that makes one subject to whitespace checking and the other not? (I suspect the answer is "shoving all the code in tNNNN-*.sh into eval'd strings screws up the whitespace checking", but my point is that I shouldn't have to guess; the justification should go in the commit message). -Peff --
Only ignore whitespace errors in t/tNNNN-*.sh and the t/tNNNN
subdirectories (since they can contain test-relevant trailing
whitespace). Other files (like test libraries) should still be
checked.
Also fix a whitespace error in t/test-lib.sh.
Signed-off-by: Lea Wiemann <LeWiemann@gmail.com>
---
I thought that was obvious since they had been not been checked before
either (see the diff). :) Anyways, added explanation in parens in the
commit message; nothing else has changed since v1.
t/.gitattributes | 3 ++-
t/test-lib.sh | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/.gitattributes b/t/.gitattributes
index 562b12e..ab6edbf 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -1 +1,2 @@
-* -whitespace
+t[0-9][0-9][0-9][0-9]-*.sh -whitespace
+t[0-9][0-9][0-9][0-9]/* -whitespace
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7a8bd27..e9c9081 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -168,7 +168,7 @@ trap 'die' exit
# environment variables to work around this.
#
# In particular, quoting isn't enough, as the path may contain the same quote
-# that we're using.
+# that we're using.
test_set_editor () {
FAKE_EDITOR="$1"
export FAKE_EDITOR
--
1.5.6.rc2.33.g0b5e3.dirty
--
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. --
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 --
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 <peff@peff.net>
---
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 ...<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 --
This part unfortunately falls into the same category as your [3/4].
---
parse-options.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index acf3fe3..5e56bb5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -312,8 +312,12 @@ void usage_with_options_internal(const char * const *usagestr,
fprintf(stderr, "usage: %s\n", *usagestr++);
while (*usagestr && **usagestr)
fprintf(stderr, " or: %s\n", *usagestr++);
- while (*usagestr)
- fprintf(stderr, " %s\n", *usagestr++);
+ while (*usagestr) {
+ if (**usagestr)
+ fprintf(stderr, " %s", *usagestr);
+ putc('\n', stderr);
+ usagestr++;
+ }
if (opts->type != OPTION_GROUP)
fputc('\n', stderr);
--
I was just about to send the same patch. Hold on, there is one other bug, and I am about to send the respun series. -Peff --
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 <peff@peff.net> --- The original was total crap. t1502 didn't pass (which is now fixed in 3/4), and t3800 didn't pass (which is fixed correctly and lumped into 2/4 now). t/t3903-stash.sh | 2 +- t/t4014-format-patch.sh | 6 +++--- t/t4150-am.sh | 2 +- t/t5540-http-push.sh | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) 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-* ...
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 <peff@peff.net>
---
This now has a fix for t3800 which was incorrectly fixed in the original
1/4.
t/t3800-mktag.sh | 4 +-
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 +-
8 files changed, 131 insertions(+), 136 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/t3800-mktag.sh b/t/t3800-mktag.sh
index df1fd6f..c851db8 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -241,11 +241,11 @@ check_verify_failure 'disallow spaces in tag email' \
############################################################
# 17. disallow missing tag timestamp
-cat >tag.sig <<EOF
+tr '_' ' ' >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/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 ...When outputting a usage message with a blank line in the
header, we would output a line with four spaces. Make this
truly a blank line.
This helps us remove trailing whitespace from a test vector.
Signed-off-by: Jeff King <peff@peff.net>
---
This is equivalent to the fix you just posted.
parse-options.c | 8 ++++++--
t/t1502-rev-parse-parseopt.sh | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index acf3fe3..8071711 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -312,8 +312,12 @@ void usage_with_options_internal(const char * const *usagestr,
fprintf(stderr, "usage: %s\n", *usagestr++);
while (*usagestr && **usagestr)
fprintf(stderr, " or: %s\n", *usagestr++);
- while (*usagestr)
- fprintf(stderr, " %s\n", *usagestr++);
+ while (*usagestr) {
+ fprintf(stderr, "%s%s\n",
+ **usagestr ? " " : "",
+ *usagestr);
+ usagestr++;
+ }
if (opts->type != OPTION_GROUP)
fputc('\n', stderr);
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
--
1.5.6.rc2.183.g04614
--
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 <peff@peff.net> --- This is identical to the original 3/4. 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" => ...
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 <peff@peff.net> --- This is identical to the original 4/4. 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 --
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 <peff@peff.net>
---
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
...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 <peff@peff.net> --- 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" | ...
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 <peff@peff.net> --- 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 --
