On Fri, 13 May 2011 01:07:08 -0700, Jameson Graef Rollins <jrollins@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