Re: [PATCH 5/5] show: Convert raw format to the new self-recursive style

[Jameson Graef Rollins <jrollins@finestructure.net>]

[-- Attachment #1: Type: text/plain, Size: 3667 bytes --]

Hey, Austin.  As always, thank you so much for your hard work on this
rewrite.  It looks like things are definitely moving the right
direction.

I haven't done a full review of this patch set, and I've been pretty out
of the loop on this stuff recently, but I do notice that there are some
changes to the tests that don't look right to me.

On Sat,  3 Mar 2012 00:20:25 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> This is fully compatible for root and leaf parts, but drops support
> for interior parts.  Showing interior parts in raw has always been
> braindead broken, so I don't think anyone will miss this.  Tests have
> been updated to reflect this.

I think I'm confused about this "drop support for interior parts".  What
constitutes an "interior part"?  Aren't all parts interior?  It looks
From the patch that maybe you're referring specifically to rfc822 parts?

I can understand not supporting output of multipart parts in raw; it's
unclear what a "raw" multipart even is.  But I think we do need to
support common-sense handling of rfc822 parts, even if it requires
special casing.  These following two test modifications illustrate the
issue:

>  test_begin_subtest "--format=raw --part=3, rfc822 part"
> -test_subtest_known_broken
> -
> -notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
> -test_expect_equal_file OUTPUT embedded_message
> +notmuch show --format=raw --part=3 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
> +cat <<EOF >EXPECTED
> +Error: Raw only supports root and leaf parts
> +EOF
> +test_expect_equal_file OUTPUT EXPECTED

I pretty strongly think that this test needs to remain how it was.  If
someone forwards me a message as a rfc822 part I should be able to
retrieve the full forwarded message directly, by e.g. redirecting it to
a file and recreating the original message exactly intact.  That's why I
constructed this test the way I did originally, and left it
known_broken.  If we can't support this now that's fine, but I still
think this test describes an important needed functionality that we
should strive to support at some point.  Maybe it needs an entirely new
output formatter, or some special casing, but I still think it's
reasonable to expect that we should support this.

>  test_begin_subtest "--format=raw --part=4, rfc822's html part"
> -notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >OUTPUT
> +notmuch show --format=raw --part=4 'id:87liy5ap00.fsf@yoom.home.cworth.org' >&OUTPUT
>  cat <<EOF >EXPECTED
> -<p>This is an embedded message, with a multipart/alternative part.</p>
> -This is an embedded message, with a multipart/alternative part.
> +Error: Raw only supports root and leaf parts
>  EOF
>  test_expect_equal_file OUTPUT EXPECTED

Maybe this is ultimately a limitation of what we can expect the raw
formatter to do, but isn't this a leaf part?  As with whole rfc822
parts, I also expect that we should be able to retrieve interior leaf
parts of rfc822 message parts.  So I think I would prefer that this test
just move to test_subtest_known_broken until a better way to handle this
is figured out.

Without looking into the details of your whole show rewrite, I'm
guessing that we just can't reasonably expect the raw formatter to
handle rfc822 parts the way we need.  Is that correct?  Is there some
other formatter that might be better suited for this?  Like I say, I
think it's ok if we have to have some special casing for this particular
type of part.  But either way I think we should try to support handling
rfc822 parts in a useful way.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]