On Tue, Aug 4, 2009 at 9:25 AM, Junio C Hamano[off-list ref] wrote:
Giuseppe Bilotta [off-list ref] writes:
quoted
diff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh
new file mode 100755
index 0000000..8e3fce3
--- /dev/null
+++ b/t/t4107-apply-ignore-whitespace.sh
...
+cat > patch3.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -10,1 +10,1 @@
+ print_int(func(i));
+EOF
This part is triply troublesome in that:
(1) the payload ends with trailing whitespace which can be eaten by MUAs
(and I almost always use "git am" with --whitespace=fix);
This is also a problem with some other parts of that patch. I'll apply
the suggestion you mention below to all of them.
(2) even if we apply your patch correctly, it is very unobvious that you
are testing the case where the line has an unwanted trailing
whitespace; and
I'll add a comment for that that.
(3) a hunk without any added/deleted lines is an obviously artificial
test input that would not appear in real life, something one would
never think of doing unless one knows how "git apply" internally
works. It makes the test too knowledgable about the implementation.
Right. I'll make it do something else too.
You can fix the first two issues by doing:
sed -e 's/Z/ /g' >patch3.patch <<\EOF
...
+Z print_int(func(i));Z
EOF
to make invisible SP stand out more for the benefit of people reading the
test script (I know you did not have leading SP before HT in yours, but
the above illustrates the visibility issues). For other tests with test
vector patches, visibility of whitespace is not much an issue, but this
script is _all about_ whitespace, so anything that clarifies what is going
on better would help.
Indeed.
quoted
+test_expect_success "S = patch1" \
+ 'git-apply patch1.patch'
+
+test_expect_failure "F = patch2" \
+ 'git-apply patch2.patch'
Please say
test_expect_success "whitespace corrupted patch does not apply" '
test_must_fail git apply patch2.patch
'
instead.
"test_expect_failure" is a declaration that the command being tested (in
this case "git apply") is faulty. It also is a request for somebody
interested to later fix it (again, in this case "git apply") to make this
test pass.
But you do _not_ want this to pass without an explicit ignore option, so
test_expect_failure is inappropriate here.
Ah, interesting. This was the first test I wrote (mostly copied from
the original patch) and I didn't have a clear idea of the difference
between the expected failures (although the comment about 'fixing'
that the test printed when some failing case didn't should have put me
on the right track).
I'll resend with a fixed test.
--
Giuseppe "Oblomov" Bilotta