{"thread":{"id":"28428","subject":"[PATCH] t4014: remove Message-Id/timestamp before comparing patches","startedAt":"2011-09-19T06:45:43Z","lastAt":"2011-09-20T08:43:41Z","messageCount":5,"participants":["Thomas Rast","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"175773","messageId":"6b2cb6ebec907342a02d56a36ddc58715efabc00.1316414731.git.trast@student.ethz.ch","threadId":"28428","inReplyTo":null,"subject":"[PATCH] t4014: remove Message-Id/timestamp before comparing patches","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-19T06:45:43Z","receivedAt":"2011-09-19T06:45:43Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The test introduced in 787570c (format-patch: ignore ui.color,\n2011-09-13) has a race condition: if the two format-patch invocations\ndo not ask for the current time in the same second, their Message-Id\nheaders will disagree.\n\nNormally this would be a pretty unlikely occurrence.  However, under\nvalgrind format-patch runs so slowly that the race triggers every\ntime, with a time difference of 2-3s on my hardware.\n\nTo avoid this problem, replace the contents of the Message-Id header\nwith a dummy before comparing.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n t/t4014-format-patch.sh |    7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7e405d7..8700089 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -886,11 +886,18 @@ test_expect_success 'empty subject prefix does not have extra space' '\n \ttest_cmp expect actual\n '\n \n+strip_msgid () {\n+\tsed 's#\\(Message-Id: *\\)<[^>]*>#\\1<MESSAGE@ID>#' \"$1\" >\"$1+\" &&\n+\tmv \"$1+\" \"$1\"\n+}\n+\n test_expect_success 'format patch ignores color.ui' '\n \ttest_unconfig color.ui &&\n \tgit format-patch --stdout -1 >expect &&\n+\tstrip_msgid expect &&\n \ttest_config color.ui always &&\n \tgit format-patch --stdout -1 >actual &&\n+\tstrip_msgid actual &&\n \ttest_cmp expect actual\n '\n \n-- \n1.7.7.rc1.366.ge210a6\n"},{"id":"175789","messageId":"7v4o08bguc.fsf@alter.siamese.dyndns.org","threadId":"28428","inReplyTo":"6b2cb6ebec907342a02d56a36ddc58715efabc00.1316414731.git.trast@student.ethz.ch","subject":"Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-19T17:08:11Z","receivedAt":"2011-09-19T17:08:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> +strip_msgid () {\n> +\tsed 's#\\(Message-Id: *\\)<[^>]*>#\\1<MESSAGE@ID>#' \"$1\" >\"$1+\" &&\n> +\tmv \"$1+\" \"$1\"\n> +}\n> +\n\nAs this is munging the contents of the line, if future versions of\nformat-patch start coloring the message ID values for some mistake, the\ntest using strip_msgid won't catch them anyway, so I'll amend the\nstripping to actually strip the message id, like this:\n\n\tsed '/^Message-Id: /d' <\"$1\" >\"$1+\"\n\nbefore applying.\n\nThanks.\n"},{"id":"175798","messageId":"20110919191545.GD26115@sigill.intra.peff.net","threadId":"28428","inReplyTo":"6b2cb6ebec907342a02d56a36ddc58715efabc00.1316414731.git.trast@student.ethz.ch","subject":"Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-19T19:15:45Z","receivedAt":"2011-09-19T19:15:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 19, 2011 at 08:45:43AM +0200, Thomas Rast wrote:\n\n> The test introduced in 787570c (format-patch: ignore ui.color,\n> 2011-09-13) has a race condition: if the two format-patch invocations\n> do not ask for the current time in the same second, their Message-Id\n> headers will disagree.\n> \n> Normally this would be a pretty unlikely occurrence.  However, under\n> valgrind format-patch runs so slowly that the race triggers every\n> time, with a time difference of 2-3s on my hardware.\n> \n> To avoid this problem, replace the contents of the Message-Id header\n> with a dummy before comparing.\n\nHmph. I was confused at first reading this, because git format-patch\ndoesn't produce message-ids. Usually.\n\nBut it does if you turn on threading, which obviously requires it. And\nearlier tests set format.thread without bothering to clean up after\nthemselves.\n\nSo perhaps this is a better solution:\n\n-- >8 --\nSubject: [PATCH] t4014: clean up format.thread config after each test\n\nThe threading tests turn on format.thread, but never clean\nup after themselves, meaning that later tests will also have\nformat.thread set.\n\nThis is more annoying than most leftover config, too,\nbecause not only does it impact the results of other tests,\nbut it does so non-deterministically. Threading requires the\ngeneration of message-ids, which incorporate the current\ntime, meaning a slow-running test script may generate\ndifferent results from run to run.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis uses the test_color helper I introduced recently in d960c47. This\nshould be OK, because ph/format-patch-no-color (which introduces the\nproblem) is built on top of jk/color-and-pager (which introduces\ntest_config).\n\nThere are several other places in the script that could use the same\ncleanup, but I didn't bother as they are more complex (e.g., using\n\"--add\" to build up multi-value header config) and don't actually cause\nproblems (because they're deterministic, and don't clutter the output in\na meaningful way)\n\n t/t4014-format-patch.sh |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7e405d7..6797512 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -457,22 +457,22 @@ test_expect_success 'thread deep cover-letter in-reply-to' '\n '\n \n test_expect_success 'thread via config' '\n-\tgit config format.thread true &&\n+\ttest_config format.thread true &&\n \tcheck_threading expect.thread master\n '\n \n test_expect_success 'thread deep via config' '\n-\tgit config format.thread deep &&\n+\ttest_config format.thread deep &&\n \tcheck_threading expect.deep master\n '\n \n test_expect_success 'thread config + override' '\n-\tgit config format.thread deep &&\n+\ttest_config format.thread deep &&\n \tcheck_threading expect.thread --thread master\n '\n \n test_expect_success 'thread config + --no-thread' '\n-\tgit config format.thread deep &&\n+\ttest_config format.thread deep &&\n \tcheck_threading expect.no-threading --no-thread master\n '\n \n-- \n1.7.7.rc1.3.gb95be\n"},{"id":"175805","messageId":"7vr53c9tlt.fsf@alter.siamese.dyndns.org","threadId":"28428","inReplyTo":"20110919191545.GD26115@sigill.intra.peff.net","subject":"Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-19T20:15:26Z","receivedAt":"2011-09-19T20:15:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, your fix makes a lot more sense.\n"},{"id":"175846","messageId":"201109201043.41267.trast@student.ethz.ch","threadId":"28428","inReplyTo":"7vr53c9tlt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t4014: remove Message-Id/timestamp before comparing patches","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-20T08:43:41Z","receivedAt":"2011-09-20T08:43:41Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> Thanks, your fix makes a lot more sense.\n\nAgreed.  Thanks!\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}