[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Jameson Graef Rollins 
On Fri, 03 Jun 2011 15:38:29 -0700, Carl Worth  wrote:
> commit d5b4d950245605b84c56ce991fa3c59a073a70e5
> Author: Jameson Graef Rollins 
> 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  with a new commit message to
> explain the bug and fix.

Now I sound like I know what I'm talking about!  Thanks, Carl.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL:

[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Carl Worth 
On Fri, 03 Jun 2011 14:26:48 -0700, Jameson Graef Rollins  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 
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  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:

[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Jameson Graef Rollins 
On Fri, 03 Jun 2011 12:56:50 -0700, Carl Worth  wrote:
> Otherwise, the patches up to this point in the thread have all either
> been pushed or I've asked for some additional information (perhaps
> that's just this patch and the "old style fcc dirs" patch?).

Great!  That's really great news, Carl.  Thank you very much.  I think
at this point the only thing we need for 0.6 are:

* Austin's atomicity patches
* maybe fix the frc822 handling issue

Can we set a target date for 0.6 release?  So we'll all start feeling
really bad if we miss it?

> I'm actually a bit surprised to see myself preferring patches in
> email. When Linus first wrote git, I couldn't understand why the Linux
> community kept to such a consistent culture of sending patches via
> email. It seemed so backwards to do these awkward machinations (git
> format-patch, git send-email, SMTP, MUA, git am), and risk all the
> problems of email clients corrupting patches, etc.?especially when git
> has such clean mechanisms for reliably moving patches around (git push,
> git pull).

Yeah, I'm with you that I'm surprised by liking this method as well.  I
think it only really works once most of the bulk of things are already
working, but for bug fixes and feature additions it really works well.
It's nice to have comments for patches on list.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL:

[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Jameson Graef Rollins 
On Fri, 03 Jun 2011 12:56:50 -0700, Carl Worth  wrote:
> On Sat, 28 May 2011 14:52:00 -0700, Jameson Graef Rollins  finestructure.net> wrote:
> > The declaration of the GMimeStream pointer to stdout in
> > format_part_content_text was somehow preventing subsequent printf
> > calls from outputting to stdout if the output was redirected to a
> > file.  Scoping the declaration to the actual use of the stream pointer
> > works around this problem.
> 
> This commit message sounds like we don't actually understand the problem
> being fixed here.

Well actually it's only meant to sound like the committer doesn't
understand the problem!

> 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.

Carl, if you (or anyone else) understands what the issue is, then please
go ahead and modify the commit message.  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.

I really wouldn't block on this, though, since the patch does fix an
actual bug.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL:

[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Carl Worth 
On Sat, 28 May 2011 14:52:00 -0700, Jameson Graef Rollins  wrote:
> The declaration of the GMimeStream pointer to stdout in
> format_part_content_text was somehow preventing subsequent printf
> calls from outputting to stdout if the output was redirected to a
> file.  Scoping the declaration to the actual use of the stream pointer
> works around this problem.

This commit message sounds like we don't actually understand the problem
being fixed here.

I'd like to investigate this more. Perhaps with a test case?

Otherwise, the patches up to this point in the thread have all either
been pushed or I've asked for some additional information (perhaps
that's just this patch and the "old style fcc dirs" patch?).

I'll continue to work through my patch queue as contained in email
messages. I'm finding it easier to do that (and just piping the patches
I like to "git am") as opposed to working through a release-candidate
branch in git, (and having to keep rebasing/postponing it after I reject
some patch in the series).

I'm actually a bit surprised to see myself preferring patches in
email. When Linus first wrote git, I couldn't understand why the Linux
community kept to such a consistent culture of sending patches via
email. It seemed so backwards to do these awkward machinations (git
format-patch, git send-email, SMTP, MUA, git am), and risk all the
problems of email clients corrupting patches, etc.?especially when git
has such clean mechanisms for reliably moving patches around (git push,
git pull).

Call me converted. Email really is the right way to do collaborative
code development.

-Carl
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL:

Re: [PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Carl Worth 
On Sat, 28 May 2011 14:52:00 -0700, Jameson Graef Rollins 
jroll...@finestructure.net wrote:
 The declaration of the GMimeStream pointer to stdout in
 format_part_content_text was somehow preventing subsequent printf
 calls from outputting to stdout if the output was redirected to a
 file.  Scoping the declaration to the actual use of the stream pointer
 works around this problem.

This commit message sounds like we don't actually understand the problem
being fixed here.

I'd like to investigate this more. Perhaps with a test case?

Otherwise, the patches up to this point in the thread have all either
been pushed or I've asked for some additional information (perhaps
that's just this patch and the old style fcc dirs patch?).

I'll continue to work through my patch queue as contained in email
messages. I'm finding it easier to do that (and just piping the patches
I like to git am) as opposed to working through a release-candidate
branch in git, (and having to keep rebasing/postponing it after I reject
some patch in the series).

I'm actually a bit surprised to see myself preferring patches in
email. When Linus first wrote git, I couldn't understand why the Linux
community kept to such a consistent culture of sending patches via
email. It seemed so backwards to do these awkward machinations (git
format-patch, git send-email, SMTP, MUA, git am), and risk all the
problems of email clients corrupting patches, etc.—especially when git
has such clean mechanisms for reliably moving patches around (git push,
git pull).

Call me converted. Email really is the right way to do collaborative
code development.

-Carl


pgpizI5odauir.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Jameson Graef Rollins 
On Fri, 03 Jun 2011 12:56:50 -0700, Carl Worth cwo...@cworth.org wrote:
 On Sat, 28 May 2011 14:52:00 -0700, Jameson Graef Rollins 
 jroll...@finestructure.net wrote:
  The declaration of the GMimeStream pointer to stdout in
  format_part_content_text was somehow preventing subsequent printf
  calls from outputting to stdout if the output was redirected to a
  file.  Scoping the declaration to the actual use of the stream pointer
  works around this problem.
 
 This commit message sounds like we don't actually understand the problem
 being fixed here.

Well actually it's only meant to sound like the committer doesn't
understand the problem!

 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.

Carl, if you (or anyone else) understands what the issue is, then please
go ahead and modify the commit message.  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.

I really wouldn't block on this, though, since the patch does fix an
actual bug.

jamie.


pgpDZan2dZhJr.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Jameson Graef Rollins 
On Fri, 03 Jun 2011 12:56:50 -0700, Carl Worth cwo...@cworth.org wrote:
 Otherwise, the patches up to this point in the thread have all either
 been pushed or I've asked for some additional information (perhaps
 that's just this patch and the old style fcc dirs patch?).

Great!  That's really great news, Carl.  Thank you very much.  I think
at this point the only thing we need for 0.6 are:

* Austin's atomicity patches
* maybe fix the frc822 handling issue

Can we set a target date for 0.6 release?  So we'll all start feeling
really bad if we miss it?

 I'm actually a bit surprised to see myself preferring patches in
 email. When Linus first wrote git, I couldn't understand why the Linux
 community kept to such a consistent culture of sending patches via
 email. It seemed so backwards to do these awkward machinations (git
 format-patch, git send-email, SMTP, MUA, git am), and risk all the
 problems of email clients corrupting patches, etc.—especially when git
 has such clean mechanisms for reliably moving patches around (git push,
 git pull).

Yeah, I'm with you that I'm surprised by liking this method as well.  I
think it only really works once most of the bulk of things are already
working, but for bug fixes and feature additions it really works well.
It's nice to have comments for patches on list.

jamie.


pgp5zz3wPc2IA.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

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


pgpLSzt2ZnU4W.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-06-03 Thread Jameson Graef Rollins 
On Fri, 03 Jun 2011 15:38:29 -0700, Carl Worth cwo...@cworth.org wrote:
 commit d5b4d950245605b84c56ce991fa3c59a073a70e5
 Author: Jameson Graef Rollins jroll...@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 cwo...@cworth.org with a new commit message to
 explain the bug and fix.

Now I sound like I know what I'm talking about!  Thanks, Carl.

jamie.


pgptPc1mWLDnO.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-05-28 Thread Jameson Graef Rollins 
The declaration of the GMimeStream pointer to stdout in
format_part_content_text was somehow preventing subsequent printf
calls from outputting to stdout if the output was redirected to a
file.  Scoping the declaration to the actual use of the stream pointer
works around this problem.
---
 notmuch-show.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index e90f07e..61c6f49 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -447,7 +447,6 @@ format_part_content_text (GMimeObject *part)
 {
 GMimeContentDisposition *disposition = 
g_mime_object_get_content_disposition (part);
 GMimeContentType *content_type = g_mime_object_get_content_type 
(GMIME_OBJECT (part));
-GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);

 printf (", Content-type: %s\n", g_mime_content_type_to_string 
(content_type));

@@ -462,6 +461,7 @@ format_part_content_text (GMimeObject *part)
 if (g_mime_content_type_is_type (content_type, "text", "*") &&
!g_mime_content_type_is_type (content_type, "text", "html"))
 {
+   GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);
g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
show_part_content (part, stream_stdout);
g_object_unref(stream_stdout);
-- 
1.7.4.4

[PATCH 25/25] Fix stdout stream grabbing in format_part_content_text

2011-05-28 Thread Jameson Graef Rollins 
The declaration of the GMimeStream pointer to stdout in
format_part_content_text was somehow preventing subsequent printf
calls from outputting to stdout if the output was redirected to a
file.  Scoping the declaration to the actual use of the stream pointer
works around this problem.
---
 notmuch-show.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index e90f07e..61c6f49 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -447,7 +447,6 @@ format_part_content_text (GMimeObject *part)
 {
 GMimeContentDisposition *disposition = 
g_mime_object_get_content_disposition (part);
 GMimeContentType *content_type = g_mime_object_get_content_type 
(GMIME_OBJECT (part));
-GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);
 
 printf (, Content-type: %s\n, g_mime_content_type_to_string 
(content_type));
 
@@ -462,6 +461,7 @@ format_part_content_text (GMimeObject *part)
 if (g_mime_content_type_is_type (content_type, text, *) 
!g_mime_content_type_is_type (content_type, text, html))
 {
+   GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);
g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
show_part_content (part, stream_stdout);
g_object_unref(stream_stdout);
-- 
1.7.4.4

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch