From: Carl Worth <cworth@cworth.org>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: release-candidate/0.6
In-Reply-To: <8762pn7gth.fsf@servo.factory.finestructure.net>
References: <8762pn7gth.fsf@servo.factory.finestructure.net>
User-Agent: Notmuch/0.5 (http://notmuchmail.org) Emacs/23.3.1
	(i486-pc-linux-gnu)
Date: Thu, 12 May 2011 15:36:43 -0700
Message-ID: <874o4zczr8.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Cc: David Edmondson <dme@dme.org>
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

On Fri, 06 May 2011 12:46:34 -0700, Jameson Graef Rollins <jrollins@finestr=
ucture.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:
>=20
> 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.
=20=20=20=20
	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=20a quick skim, many later patches in the series appear to be better
in this regard.

=2DCarl

[!] Has it really been that long?

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAk3MYPsACgkQ6JDdNq8qSWir9QCgnK926GAwX/KVMgLX2CDYBPhS
eAgAn1VqV/IFi7REPZDD6RGX28j3Xvh1
=BM8E
-----END PGP SIGNATURE-----
--=-=-=--