On Fri, 13 May 2011 01:07:08 -0700, Jameson Graef Rollins 
<jroll...@finestructure.net> wrote:
> Hi, Carl.  I went through dme's multipart patch series and cleaned
> things up.
...
> The result is the new
> 
> release-candidate/0.6+mpmfix

Thanks so much! This looks much better than before.

I'm still hitting some snags quite early in the review process, (I'm
hoping that real soon we'll be able to just start merging like crazy).

Here at the things I've seen so far:

d3e7415d "add argument to part format function to indicate initial part"

        This one fails to build due to a simple missing argument, (easy
        mistake with the recent splitting of patches).

373f352b "Have output structure fully reflect message MIME structure"

        I'm not comfortable with this commit. Previously there was
        recursion in one function, (show_message_part), and afterwards
        there is duplicated code performing recursion in both
        format_part_text and format_part_json.

        Similarly, the code adds header formatting code that duplicates
        functionality in the existing format_headers_text and
        format_headers_json functions.

        Meanwhile, I still can't tell exactly what the behavioral change
        intended is. The commit message talks about "fully recursing"
        and "match[ing] the MIME structure of the message". Was it not
        fully recursing before? In what way did the output not match the
        MIME structure before?

        It would probably be easier to tell what was going on if the
        test suite were updated at the same time (or in a subsequent
        commit at least). As is, this commit introduces test suite
        failures, (re-ordering of MIME part ID numbers and then
        emacs-client confusion), which remain until commit 7ee6aaa1

        But what does the rest of the series really need from this
        commit? Is there some structural change to the json output aside
        from the part ID? If so, we're definitely missing a test for
        that directly, (other than the indirect testing we get with the
        emacs tests).

28ab74e0 "clean up expected emacs output to reflect cleaner from lines 
introduced in 78d6c196d90"

        This commit message refers to a stale (now non-existent) commit
        ID. I presume it's attempting to reference the commit with a
        message of "emacs: Show cleaner `From:' addresses in the summary
        line.".

        Granted, it's hard to keep commit IDs valid in a branch that's
        getting continually rebuilt. One fix is to not use the commit
        identifiers. Another help would be to have the test-suite
        cleanups occurring more closely after the commits that changed
        things.

I'm happy to help work on cleaning up some of these issues if that would
be useful. Let me know.

-Carl

Attachment: pgpT3nil5FBZU.pgp
Description: PGP signature

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

Reply via email to