From: Carl Worth <cworth@cworth.org>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 25/25] Fix stdout stream grabbing in
	format_part_content_text
In-Reply-To: <87lixihahz.fsf@servo.factory.finestructure.net>
References: <1306619520-25730-1-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-2-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-3-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-4-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-5-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-6-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-7-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-8-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-9-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-10-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-11-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-12-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-13-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-14-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-15-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-16-git-se nd-email-jrollins@finestructure.net>
	<1306619520-25730-17-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-18-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-19-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-20-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-21-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-22-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-23-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-24-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-25-git-send-email-jrollins@finestructure.net>
	<1306619520-25730-26-git-send-email-jrollins@finestructure.net>
	<87r57almd9.fsf@yoom.home.cworth.org>
	<87lixihahz.fsf@servo.factory.finestructure.net>
User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1
	(i486-pc-linux-gnu)
Date: Fri, 03 Jun 2011 15:38:29 -0700
Message-ID: <87d3iulevu.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

On Fri, 03 Jun 2011 14:26:48 -0700, Jameson Graef Rollins <jrollins@finestr=
ucture.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?
>=20
> 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.

=2DCarl

commit d5b4d950245605b84c56ce991fa3c59a073a70e5
Author: Jameson Graef Rollins <jrollins@finestructure.net>
Date:   Sat May 28 14:52:00 2011 -0700

    show: Avoid inadvertently closing stdout
=20=20=20=20
    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.
=20=20=20=20
    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.
=20=20=20=20
    This led to the FILE* being closed early when notmuch show output was
    redirected to a file.
=20=20=20=20
    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).
=20=20=20=20
    Edited-by: Carl Worth <cworth@cworth.org> with a new commit message to
    explain the bug and fix.

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAk3pYmUACgkQ6JDdNq8qSWjz3QCfZ3bmMGBdHscBBSOKiUSRaxHo
7hUAnRFkfAXcy9bK6c/+KTcsj+SOsL8o
=r/3M
-----END PGP SIGNATURE-----
--=-=-=--