[PATCH] t4014: remove Message-Id/timestamp before comparing patches

[PATCH] t4014: remove Message-Id/timestamp before comparing patches

[Thomas Rast]

The test introduced in 787570c (format-patch: ignore ui.color,
2011-09-13) has a race condition: if the two format-patch invocations
do not ask for the current time in the same second, their Message-Id
headers will disagree.

Normally this would be a pretty unlikely occurrence.  However, under
valgrind format-patch runs so slowly that the race triggers every
time, with a time difference of 2-3s on my hardware.

To avoid this problem, replace the contents of the Message-Id header
with a dummy before comparing.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t4014-format-patch.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7e405d7..8700089 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -886,11 +886,18 @@ test_expect_success 'empty subject prefix does not have extra space' '
 	test_cmp expect actual
 '
 
+strip_msgid () {
+	sed 's#\(Message-Id: *\)<[^>]*>#\1<MESSAGE@ID>#' "$1" >"$1+" &&
+	mv "$1+" "$1"
+}
+
 test_expect_success 'format patch ignores color.ui' '
 	test_unconfig color.ui &&
 	git format-patch --stdout -1 >expect &&
+	strip_msgid expect &&
 	test_config color.ui always &&
 	git format-patch --stdout -1 >actual &&
+	strip_msgid actual &&
 	test_cmp expect actual
 '
 
-- 
1.7.7.rc1.366.ge210a6

Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> +strip_msgid () {
> +	sed 's#\(Message-Id: *\)<[^>]*>#\1<MESSAGE@ID>#' "$1" >"$1+" &&
> +	mv "$1+" "$1"
> +}
> +

As this is munging the contents of the line, if future versions of
format-patch start coloring the message ID values for some mistake, the
test using strip_msgid won't catch them anyway, so I'll amend the
stripping to actually strip the message id, like this:

	sed '/^Message-Id: /d' <"$1" >"$1+"

before applying.

Thanks.

Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches

[Jeff King]

On Mon, Sep 19, 2011 at 08:45:43AM +0200, Thomas Rast wrote:

> The test introduced in 787570c (format-patch: ignore ui.color,
> 2011-09-13) has a race condition: if the two format-patch invocations
> do not ask for the current time in the same second, their Message-Id
> headers will disagree.
> 
> Normally this would be a pretty unlikely occurrence.  However, under
> valgrind format-patch runs so slowly that the race triggers every
> time, with a time difference of 2-3s on my hardware.
> 
> To avoid this problem, replace the contents of the Message-Id header
> with a dummy before comparing.

Hmph. I was confused at first reading this, because git format-patch
doesn't produce message-ids. Usually.

But it does if you turn on threading, which obviously requires it. And
earlier tests set format.thread without bothering to clean up after
themselves.

So perhaps this is a better solution:

-- >8 --
Subject: [PATCH] t4014: clean up format.thread config after each test

The threading tests turn on format.thread, but never clean
up after themselves, meaning that later tests will also have
format.thread set.

This is more annoying than most leftover config, too,
because not only does it impact the results of other tests,
but it does so non-deterministically. Threading requires the
generation of message-ids, which incorporate the current
time, meaning a slow-running test script may generate
different results from run to run.

Signed-off-by: Jeff King <peff@peff.net>
---
This uses the test_color helper I introduced recently in d960c47. This
should be OK, because ph/format-patch-no-color (which introduces the
problem) is built on top of jk/color-and-pager (which introduces
test_config).

There are several other places in the script that could use the same
cleanup, but I didn't bother as they are more complex (e.g., using
"--add" to build up multi-value header config) and don't actually cause
problems (because they're deterministic, and don't clutter the output in
a meaningful way)

 t/t4014-format-patch.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7e405d7..6797512 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -457,22 +457,22 @@ test_expect_success 'thread deep cover-letter in-reply-to' '
 '
 
 test_expect_success 'thread via config' '
-	git config format.thread true &&
+	test_config format.thread true &&
 	check_threading expect.thread master
 '
 
 test_expect_success 'thread deep via config' '
-	git config format.thread deep &&
+	test_config format.thread deep &&
 	check_threading expect.deep master
 '
 
 test_expect_success 'thread config + override' '
-	git config format.thread deep &&
+	test_config format.thread deep &&
 	check_threading expect.thread --thread master
 '
 
 test_expect_success 'thread config + --no-thread' '
-	git config format.thread deep &&
+	test_config format.thread deep &&
 	check_threading expect.no-threading --no-thread master
 '
 
-- 
1.7.7.rc1.3.gb95be

Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches

[Junio C Hamano]

Thanks, your fix makes a lot more sense.

Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches

[Thomas Rast]

Junio C Hamano wrote:
> Thanks, your fix makes a lot more sense.

Agreed.  Thanks!

-- 
Thomas Rast
trast@{inf,student}.ethz.ch