From: Carl Worth <cworth@cworth.org>
To: David Edmondson <dme@dme.org>, notmuch <notmuch@notmuchmail.org>
Subject: Re: pull request
In-Reply-To: <87tyr4j7l3.fsf@ut.hh.sledj.net>
References: <87sk722sfq.fsf@ut.hh.sledj.net> <87eiibq22s.fsf@ut.hh.sledj.net>
	<87iq7kjz7c.fsf@yoom.home.cworth.org>
	<87tyr4j7l3.fsf@ut.hh.sledj.net>
Date: Fri, 23 Apr 2010 12:44:00 -0700
Message-ID: <871ve6gdj3.fsf@yoom.home.cworth.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=

On Thu, 22 Apr 2010 07:59:36 +0100, David Edmondson <dme@dme.org> wrote:
> Your concerns appear to be about `notmuch-wash-wrap-long-lines'. I agree
> that it can generate unfortunate results in the "deep thread, narrow
> terminal" case. Are the other two functions okay?

If they had come in as separate patches, I would have reviewed them
separately and would have applied any I was OK with.

I'm not sure about the other two, as I really haven't played with
them. I don't think I'd get a lot of benefit from them as I don't recall
seeing a lot of accidental duplicate lines anywhere.

I still do have some concerns about munging the original message,
though. I can easily imagine that suppressing duplicate blank lines
could destroy some ASCII (or UTF-8!) art for example.

So I think things that do this kind of munging really should be on an
opt-in basis.

> One of the reasons that I chose a hook-based approach is to allow the
> user to decide which body cleaning should be applied. If you don't want
> the line-wrapping to be enabled by default we can leave it out but
> include the definition so that a user can enable it. The inline-patch
> formatting is similar (I'll say more about that in response to your
> comments on it).

I do appreciate the way the hook works such that users can select which
ones they want. Let's make all of these available but any that could
possibly "corrupt" a message be off by default. And let's be sure to
find a way to easily advertise the list of available washing functions
to the user, (perhaps in the documentation of the customize option for
this).

> Aside from the need to test, this particular patch will benefit only
> minimally from testing - there will always be false positive cases
> however good the analysis used. Are you saying that we can not tolerate
> any false-positives, or that we just need to demonstrate that it is down
> to some acceptable level? (Though I've no idea how I would achieve the
> latter.)

I may have overreacted to a misreading of the commit message. I read it
almost like "Here's some code that is known to be buggy" which is
something I don't ever want to accept. If the meaning is instead, "some
users might find this handy while others find it unacceptable", then
that would be more acceptable.

I think it would be useful to define exactly what the implemented
conditions are so that users can easily decide whether they trust the
heuristic.

(Personally, even when correctly identified, the coloring of patch
output is distracting to me.)

The remainder of the discussion about future improvements to mime part
handling and, multipart support, and HTML rendering is all very
encouraging. I look forward to the future!

-Carl

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iD8DBQFL0fiA6JDdNq8qSWgRAt8MAKCcQYagEWBtOTzfHCtnxPxJ68YlpgCgnQj+
n22t8vFI1BnMghTqFpZE/gw=
=Eyr9
-----END PGP SIGNATURE-----
--=-=-=--