On Fri, 03 Jun 2011 14:26:48 -0700, Jameson Graef Rollins <jrollins@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@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@cworth.org> with a new commit message to
    explain the bug and fix.