On Fri, 06 May 2011 12:46:34 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote: > Hi, folks. As some of you already know, I am trying to put together a > release candidate for a 0.6 release that we can present to cworth for > approval. Hi Jameson, This is really great! I appreciate you collecting useful patches for review like this. I hope this helps us get things moving once again. > So far, this release candidate includes a couple of patch series that > are not currently on cworth's master branch: > > json structure now replicates mime structure > * dme's json-fully-reflects-MIME-structure improvements I got stalled on the first commit on this branch: commit 5a5aae66bf41d3c621c412da711fec9b33f37dcc Author: David Edmondson <dme@dme.org> Date: Mon May 10 10:25:15 2010 +0100 Improved MIME support. Signed-off-by: Jameson Rollins <jrollins@finestructure.net> It's probably the same issue that stalled me when David first sent this patch series for review over a year ago[!]. I'm guessing I didn't do a good job of stating the issue then, so I'll try to do a better job here. This first patch appears to be doing multiple things: * It changes how the emacs code inserts MIME parts when constructing a mail view. * It changes something in the json formatting. (Just extra newlines?) * It changes something about the MIME part counting. * It appears to be doing new emission of multipart part when doing json output, (and for this has some hard-coded printf("]}\n") stuff rather than using the formatter). * It has new explicit support for an embedded Message as a MIME part, (inserting the headers of the enclosed message). So that's five seemingly independent changes. I'd really like to see those split up into separate commits as much as possible. Or, excepting that, there should be some explanation/justification in the commit message for why some must be combined. The worst part is that not a single change is actually described in the commit message. All we have is "Improved MIME support". Improved how? What changed? Why? What impact does it have? Does it fix bugs? Lay groundwork for future changes? What's really changing here and why? If the patch series were sufficiently-well described in the commit message, then my review process could be more or less: * Read the commit message. Does it describe a desirable change? * If so, read the patch content. Does it do what the commit message describes? Accurately? Without doing unrelated things? * If so, accept the patch. Does anyone want to attempt to fix up this first patch? (It doesn't necessarily have to be David). From a quick skim, many later patches in the series appear to be better in this regard. -Carl [!] Has it really been that long?