Bug 271190 - mail/mutt: bug in quote patch
Summary: mail/mutt: bug in quote patch
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Fernando Apesteguía
Depends on:
Reported: 2023-05-02 01:36 UTC by Kurt Hackenberg
Modified: 2023-05-13 20:34 UTC (History)
2 users (show)

See Also:
dereks: maintainer-feedback+

Remove quote and future patch changes (14.23 KB, patch)
2023-05-05 23:51 UTC, Derek Schrock
dereks: maintainer-approval+
Details | Diff
Diff between current and fixed (2.50 KB, text/plain)
2023-05-13 18:38 UTC, Kurt Hackenberg
no flags Details
Makefile.radio (7.28 KB, text/plain)
2023-05-13 18:39 UTC, Kurt Hackenberg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kurt Hackenberg 2023-05-02 01:36:25 UTC
The quote patch includes this change to mutt_body_handler(), in handler.c:

-      else if (option(OPTREFLOWTEXT) && ascii_strcasecmp ("flowed", mutt_get_parameter ("format", b->parameter)) == 0)
+      else if (option(OPTREFLOWTEXT) && ascii_strcasecmp ("flowed", mutt_get_parameter ("format", b->parameter)) == 0 && !s->prefix)

That's wrong. That line should not be modified; the extra condition "&& !s->prefix" should not be added. It's clearly wrong just from reading code, and it causes two symptoms that I know of.

Excising that wrongly added condition fixes the two symptoms.

Ideally, the author of that patch would fix it. Otherwise, the FreeBSD port could patch the patch or not use the patch.
Comment 1 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-02 10:21:36 UTC
(In reply to Kurt Hackenberg from comment #0)
Hi Kurt,

Can you provide the exact name of the patch you are referring to?

Comment 2 Kurt Hackenberg 2023-05-02 18:15:31 UTC
The patch file is mutt-2.2.10.vvv.quote. The port configuration has an option "quote patch", which includes that patch file and another, patch-1.13.0.vvv.initials.

Symptoms of this bug:

1. Even when the patch file's two features are not enabled, when Mutt quotes a flowed text message into a flowed text reply, it removes trailing spaces from the quoted text, and so converts it to fixed text.

2. When $quote_quoted is yes, lines that start with any character in $quote_regexp are quoted with another copy of that character. That's wrong for flowed text, which RFC 3676 says must be quoted with '>'.
Comment 3 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-02 18:30:02 UTC
(In reply to Kurt Hackenberg from comment #2)
I can't find that file anywhere in the ports collection. Where's that patch exactly?
Comment 4 Kurt Hackenberg 2023-05-02 18:38:38 UTC
It's downloaded from http://www.mutt.org.ua/. See /usr/ports/mail/mutt/Makefile, variables

Comment 5 Derek Schrock 2023-05-02 20:49:25 UTC
I'd rather just drop the patch all together.  The less patches the better.
Comment 6 Kurt Hackenberg 2023-05-02 21:08:37 UTC
I wouldn't mind if that patch were dropped. I'm just one user though. Do you know whether it's popular? Seems unlikely...
Comment 7 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-03 13:28:30 UTC
(In reply to Kurt Hackenberg from comment #6)
I don't know how popular it is. I don't use mutt. However, being this an optional patch, isn't it OK for you to opt out in make configure?

Comment 8 Derek Schrock 2023-05-03 14:58:26 UTC
It is an option to disable it but it's on by default.  However, given that it might be breaking something it could be a good idea to at the very least make it non-default.  I haven't checked to make sure it wasn't breaking something I'm just taking Kurt's word for it.

Given that the patch isn't maintained even though :va is distributing it vs :vvv that has apeared to have abandoned it 3 years ago an even better reason to drop it.

The less patches the better for the port for maintainability.  It's only there for legacy reasons.
Comment 9 Kurt Hackenberg 2023-05-03 15:35:24 UTC
Optional or not, the bug I reported is unacceptable. It uses text_plain_handler(), the function for format=fixed, to handle quoting of messages that are text/plain format=flowed. That's a crude hack, blatantly wrong, destructive. It may cause more problems than the two I've noticed.

That hack was probably done to apply the patch's quoting features to flowed text as well as fixed, and that idea is wrong. For flowed text, the quoting mechanism is defined by RFC 3676, and may not be changed. As it is now, the patch could generated quoted text that will not be understood by other mail readers.

The patch should be either repaired or dropped.

The patch adds two optional small features that modify the string of '>' and space used to quote a message in a reply. It does not add those features to the Mutt documentation, so they're probably not widely known. I use Mutt, and didn't know those features existed. I found all this by debugging a symptom.

Also, I agree with Derek that it's best to hold down the number of patches.

I say, drop the patch.
Comment 10 Derek Schrock 2023-05-04 22:08:10 UTC
While this might be extreme what about dropping all the feature patches.  While you are one user... Do you care about any of the other feature patches?
Comment 11 Kurt Hackenberg 2023-05-05 04:06:13 UTC
I didn't know any of them existed until the other day, when I tracked down this bug. I've looked briefly at some of them, but not all; haven't tried any but the quote stuff.

Note that the quote patch and the initials patch are packaged together in the single configuration option "quote patch".

Opinions so far, based on not much:

quote, initials, greeting: these strike me as minor and only marginally useful. I don't plan to use them, and wouldn't miss them.

smart date, reverse reply: probably the same, but I'd want to look at them more.

ifdef: looks handy, if it is what I think it is. Needs more attention.

maildir mtime: doesn't look like a feature, don't know what it's about. I wouldn't change anything without knowing more.
Comment 12 Derek Schrock 2023-05-05 16:50:39 UTC
Created the following


Hoping to get some feedback.

I think the current plan is the remove quote patch, keep initial patch either way a new option or just keep the QUOTE_PATCH option.

Maybe use UPDATING to solicit information for more feedback.
Comment 13 Kurt Hackenberg 2023-05-05 17:54:36 UTC
If you drop the quote patch and keep the initials patch, you should rename the port configuration option, to "initials". Calling it "quote" when it has nothing to do with quoting would be confusing.
Comment 14 Derek Schrock 2023-05-05 23:49:56 UTC
Created https://reviews.freebsd.org/D39985  for review.
Comment 15 Derek Schrock 2023-05-05 23:51:27 UTC
Created attachment 242007 [details]
Remove quote and future patch changes

Patch from https://reviews.freebsd.org/D39985
Comment 16 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-06 18:33:06 UTC
LGTM except that we need to bump PORTREVISION since we are changing options.
No need to update the patch, I can do it at commit time.

Please set the maintainer-approval attachment flag (to +) on patches for ports you maintain to signify approval.

Then, I can proceed with the commit :-)
Comment 17 Derek Schrock 2023-05-06 18:59:10 UTC
(In reply to Fernando Apesteguía from comment #16)
doh always forget about rev.  Thanks for the feedback.

I plan on waiting till Mon/Tues to move it to +.
Comment 18 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-06 19:53:55 UTC
(In reply to Derek Schrock from comment #17)
Ok, have a good weekend!
Comment 19 commit-hook freebsd_committer 2023-05-09 09:44:08 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=efe2cf37e62dedefdf9996990f197af13262bb2f

commit efe2cf37e62dedefdf9996990f197af13262bb2f
Author:     Derek Schrock <dereks@lifeofadishwasher.com>
AuthorDate: 2023-05-06 18:14:03 +0000
Commit:     Fernando Apesteguía <fernape@FreeBSD.org>
CommitDate: 2023-05-09 09:38:23 +0000

    mail/mutt: Remove QUOTE_PATCH option and future patch removal

     * Removing QUOTE_PATCH due to logic issues and RFC3676 quoting.
     * Keep vvv initials patch from QUOTE_PATCH adding INITIALS_PATCH
     * Add depercation of other patches to be removed in future updates via

    UPDATING and pkg-message. See PR 271190 [1] comment 12 [2].
     * SASL_NONE will now add --with-sasl=no (making portlint happy)
     * Update context on patches

    [1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=271190
    [2] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=271190#c12

    Context and discussion:

    PR:                     271190
    Reported by:            kh@panix.com
    Differential Revision:  https://reviews.freebsd.org/D39985

 mail/mutt/Makefile                        | 23 ++++++++++++-----------
 mail/mutt/distinfo                        |  4 +---
 mail/mutt/files/extra-patch-ifdef         | 10 +++++-----
 mail/mutt/files/extra-patch-maildir-mtime |  4 ++--
 mail/mutt/files/extra-patch-reverse_reply | 18 +++++++++---------
 mail/mutt/files/extra-patch-smartdate     |  4 ++--
 mail/mutt/files/extra-smime-sender        | 10 +++++-----
 mail/mutt/files/patch-date-conditional    |  6 +++---
 mail/mutt/files/patch-dgc-deepif          |  2 +-
 mail/mutt/pkg-message (new)               | 18 ++++++++++++++++++
 10 files changed, 58 insertions(+), 41 deletions(-)
Comment 20 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-09 09:44:20 UTC

Comment 21 Derek Schrock 2023-05-09 18:58:18 UTC
Thanks however any reason UPDATING was excluded?
Comment 22 commit-hook freebsd_committer 2023-05-10 06:37:27 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=429dc21f6687578c0ead0d11e5a87784224f14ae

commit 429dc21f6687578c0ead0d11e5a87784224f14ae
Author:     Fernando Apesteguía <fernape@FreeBSD.org>
AuthorDate: 2023-05-10 06:30:00 +0000
Commit:     Fernando Apesteguía <fernape@FreeBSD.org>
CommitDate: 2023-05-10 06:30:00 +0000

    UPDATING: Add entry for mail/mutt

    Present in the OP's patch, but that I somehow managed to miss.

    PR:             271190
    Fixes:          efe2cf37e62d

 UPDATING | 9 +++++++++
 1 file changed, 9 insertions(+)
Comment 23 Fernando Apesteguía freebsd_committer freebsd_triage 2023-05-10 06:38:21 UTC
(In reply to Derek Schrock from comment #21)
Should be fixed now. I don't know what happened sorry :S

I didn't even have UPDATING as modified, so it was not that I forgot to add it before committing...

Thanks for the heads up!
Comment 24 Kurt Hackenberg 2023-05-13 18:38:29 UTC
Created attachment 242143 [details]
Diff between current and fixed
Comment 25 Kurt Hackenberg 2023-05-13 18:39:59 UTC
Created attachment 242144 [details]
Comment 26 Kurt Hackenberg 2023-05-13 18:47:46 UTC
I built Mutt from the new port, and found a bug: it fails to send through the SMTP server of one mail provider, with these messages:

[2023-05-12 22:23:16] SMTP authentication requires SASL
[2023-05-12 22:23:17] No authenticators available

I found and fixed a bug in the Makefile. Attachment shows diff between current and fixed, for Makefile, output of "make showconfig", and output of "mutt -v". Note that the bug also messes with the options reverse reply and smart date.

I also did an experiment: converted OPTIONS_SINGLE to OPTIONS_RADIO, for GSSAPI, HCACHE, and SASL, since they follow that pattern. Removed all explicit references to their NONE options. That seems to work, but needs more testing. Attached that Makefile.

I don't know much about port Makefiles; please check what I've done. The port needs to be fixed, though.
Comment 27 Derek Schrock 2023-05-13 19:59:27 UTC
Dang.  Lets try to address these one at a time...

Ok I understand the SASL issue.  I think having the --with-sasl might be an issue in cases where it's looking for something outside of LOCALBASE?  Having the just do nothing should be fine?

+SASL_NONE_CONFIGURE_ON=                --with-sasl=no

Building with each option mutt -v shows + for the given feature and - for all when disabled.

I'm not following the issue with the reverse reply and smarte date options.

I like the idea of the radio.  However, going to leave it out until the port is stable.  However, the idea is you unselect the option to make it "none"?
Comment 28 Derek Schrock 2023-05-13 20:17:16 UTC
Created https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=271395
Comment 29 Kurt Hackenberg 2023-05-13 20:34:29 UTC
The --with-sasl stuff -- I wrote those two lines imitating other NONE_CONFIGURE_{ON,OFF} in the Makefile. Depends on exactly how the configure script interprets those options, which I don't really know.

Reverse reply and smart date -- I don't know why that happens, but we see that it does. I notice those options surround SASL_CYRUS in OPTIONS_DEFAULT; maybe there's some text processing problem. We see that it's fixed with the fix to SASL_NONE_CONFIGURE.

Radio, unselect for none -- I guessed that, and it seems to work for GSSAPI. Needs more testing.