On Fri, 03 Jun 2011 14:26:48 -0700, Jameson Graef Rollins <jrollins at finestructure.net> wrote: > Well actually it's only meant to sound like the committer doesn't > understand the problem!
Heh, OK. > > I'd like to investigate this more. Perhaps with a test case? > > The current tests are how I found the problem! Without this patch at > least the multipart tests will fail. I don't see how another test will > add anything. Ah, in my review I'd managed to get this commit detached from the original previous commit that introduced the test failures. It's funny that while you were replying I was reviewing *that* commit and thinking, "why are all these tests failing now just because they changed to test_expect_equal_file"? > Carl, if you (or anyone else) understands what the issue is, then please > go ahead and modify the commit message. Done. > I don't understand things > enough myself to do any better. Clearly there is some strange > interaction with things that try to use stdout after > g_mime_stream_file_new() has already grabbed it. g_mime_stream_file_new is a bad citizen, API-wise. It closes files that it didn't open, (by default). > I really wouldn't block on this, though, since the patch does fix an > actual bug. Not blocked. All pushed. New commit message below for reference. -Carl commit d5b4d950245605b84c56ce991fa3c59a073a70e5 Author: Jameson Graef Rollins <jrollins at finestructure.net> Date: Sat May 28 14:52:00 2011 -0700 show: Avoid inadvertently closing stdout GMime has a nasty habit of taking ownership by default of any FILE* handed to it va g_mime_stream_file_new. Specifically it will close the FILE* when the stream is destroyed---even though GMime didn't open the file itself. To avoid this bad behavior, we have to carefully set_owner(FALSE) after calling g_mime_stream_file_new. In the format_part_content_text function, since commit d92146d3a6809f8ad940302af49cd99a0820665e we've been calling g_mime_stream_file_new unconditionally, but only calling g_mime_stream_file_set_owner(FALSE) conditionally. This led to the FILE* being closed early when notmuch show output was redirected to a file. Fixing this fixes the test-suite cases that broke with the previous commit, (which added redirected "notmuch show" calls to the test suite to expose this bug). Edited-by: Carl Worth <cworth at cworth.org> with a new commit message to explain the bug and fix. -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 197 bytes Desc: not available URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20110603/e6e00e66/attachment.pgp>