bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Chris Siebenmann]

If you attempt to forward an email message in MH-E with mh-forward
(bound to eg 'f'), then the resulting Subject: of the draft message will
be manged and come out as, for example:

	Subject: test forwarding subject (fwd) Chris Siebenmann: test forwarding subject

The original subject of the test message was 'test forwarding subject'.
The 'test forwarding subject (forw)' portion of the mh-forward Subject
comes from the default Ubuntu 22.04 NMH 1.7.1 /etc/nmh/forwcomps file,
used by 'forw' to create the NMH draft, which has:

	Subject:%<{nmh-subject} %(trim)%(putlit)%?{subject} %(trim)%(putlit) (fwd)%>

The portion after it comes from the fact that mh-forward creates its own
new Subject: for the forwarded message (using
mh-forwarded-letter-subject, which in turn formats with
mh-forward-subject-format), and then does:

	(mh-insert-fields "Subject:" forw-subject)

I suspect that the intention is to replace any existing Subject:, or
perhaps mh-forward assumes that the draft won't have one. Unfortunately
for mh-forward, mh-insert-fields is specifically documented to *append*
to the existing header field value, if there is one, not overwrite it,
so the resulting Subject: of the draft is as we see here: the original
subject created by 'forw', and then an appended Subject from mh-forward.
This result is not terribly useful.

Reproduction in emacs -Q provided you have NMH and MH-E set up:
* enter MH-E with 'M-x mh-rmail'
* arrange to have some message in your inbox that you can forward, for
  example by 'F r' to see all of your inbox.
* display your test message to forward, eg cursor to it and hit Return
* start to forward with 'f' or 'M-x mh-forward'.

NOTE: while I have reproduced this in a clean Emacs (and NMH)
environment with 'emacs -Q', this report is being filed from within my
regular customized Emacs because life is too short. You can skip over
the packages, shadowed load files, and so on.

In GNU Emacs 29.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.33,
 cairo version 1.16.0) of 2023-10-30 built on comps3
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Ubuntu 22.04.3 LTS

Configured using:
 'configure --prefix /u/cks/lib/x64-linux/emacs-29.1 --without-gsettings
 --with-x --with-x-toolkit=gtk3 --with-json --with-tree-sitter
 --with-native-compilation=aot'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LC_COLLATE: C
  value of $LC_CTYPE: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  smartparens-mode: t
  global-corfu-mode: t
  corfu-mode: t
  marginalia-mode: t
  vertico-mouse-mode: t
  vertico-multiform-mode: t
  vertico-mode: t
  which-key-mode: t
  global-flycheck-mode: t
  flycheck-mode: t
  backward-forward-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/u/cks/.emacs.d/elpa/transient-20231121.1154/transient hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/transient
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-core hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-core
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-diminish hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-diminish
/u/cks/.emacs.d/elpa/bind-key-20230203.2004/bind-key hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/bind-key
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-lint hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-lint
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-jump hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-jump
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-delight hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-delight
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-bind-key hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-bind-key
/u/cks/.emacs.d/elpa/use-package-20230426.2324/use-package-ensure hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/use-package/use-package-ensure
/u/cks/.emacs.d/elpa/project-0.10.0/project hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/progmodes/project
/u/cks/.emacs.d/elpa/seq-2.24/seq hides /u/cks/lib/x64-linux/emacs-29.1/share/emacs/29.1/lisp/emacs-lisp/seq

Features:
(shadow mh-identity mh-letter emacsbug mule-util project consult
bookmark mail-extr mh-seq mh-inc hl-line mh-tool-bar flyspell ispell
mh-xface mh-cks mh-cks-speed mh-speed speedbar ezimage dframe
gnus-icalendar org-capture org-refile org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src ob-comint org-pcomplete pcomplete
comint ansi-osc ring org-list org-footnote org-faces org-entities
noutline outline ob-emacs-lisp ob-core ob-eval org-cycle org-table ol
org-fold org-fold-core org-keys oc org-loaddefs org-version org-compat
org-macs format-spec icalendar diary-lib diary-loaddefs cal-menu
calendar cal-loaddefs mh-utils mh-mime mh-acros mh-show gnus-cite
gnus-art mm-uu mml2015 gnus-sum shr pixel-fill kinsoku url-file svg dom
gnus-group gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 nnoo parse-time iso8601 gnus-spec gnus-int
message yank-media dired dired-loaddefs rfc822 mailabbrev gmm-utils
mailheader gnus-win gnus-range gnus nnheader range wid-edit mh-comp
sendmail mail-utils mh-gnus mml mm-view mml-smime mml-sec epa derived
epg rfc6068 epg-config smime gnutls puny dig mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mh-folder which-func imenu gnus-util text-property-search
time-date mh-scan mh-e mh-buffers mh-loaddefs goto-addr comp comp-cstr
warnings icons smartparens advice loadhist thingatpt .emacs corfu
marginalia vertico-mouse vertico-multiform vertico compat which-key
flycheck ansi-color find-func rx dash backward-forward edmacro kmacro
cl-extra help-mode pp diminish use-package use-package-ensure
use-package-delight use-package-diminish use-package-bind-key bind-key
easy-mmode use-package-core finder-inf company-autoloads
consult-flycheck-autoloads consult-flyspell-autoloads
consult-lsp-autoloads corfu-autoloads embark-consult-autoloads
consult-autoloads embark-autoloads evil-autoloads
expand-region-autoloads flycheck-golangci-lint-autoloads
flycheck-autoloads fold-this-autoloads go-mode-autoloads
goto-chg-autoloads lsp-ui-autoloads lsp-mode-autoloads magit-autoloads
pcase git-commit-autoloads magit-section-autoloads marginalia-autoloads
orderless-autoloads project-autoloads markdown-mode-autoloads
f-autoloads rust-mode-autoloads smartparens-autoloads
transient-autoloads try-autoloads vertico-autoloads vundo-autoloads
which-key-autoloads with-editor-autoloads info compat-autoloads
seq-autoloads yaml-mode-autoloads package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting font-render-setting cairo
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 545270 248252)
 (symbols 48 33083 301)
 (strings 32 163048 44156)
 (string-bytes 1 5324563)
 (vectors 16 58484)
 (vector-slots 8 1033146 512517)
 (floats 8 416 1220)
 (intervals 56 937 397)
 (buffers 984 16))

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Eli Zaretskii]

> Cc: cks.emacsbugs-01@cs.toronto.edu
> From: Chris Siebenmann <cks.emacsbugs-01@cs.toronto.edu>
> Date: Tue, 21 Nov 2023 19:49:24 -0500
> 
> If you attempt to forward an email message in MH-E with mh-forward
> (bound to eg 'f'), then the resulting Subject: of the draft message will
> be manged and come out as, for example:
> 
> 	Subject: test forwarding subject (fwd) Chris Siebenmann: test forwarding subject
> 
> The original subject of the test message was 'test forwarding subject'.
> The 'test forwarding subject (forw)' portion of the mh-forward Subject
> comes from the default Ubuntu 22.04 NMH 1.7.1 /etc/nmh/forwcomps file,
> used by 'forw' to create the NMH draft, which has:
> 
> 	Subject:%<{nmh-subject} %(trim)%(putlit)%?{subject} %(trim)%(putlit) (fwd)%>
> 
> The portion after it comes from the fact that mh-forward creates its own
> new Subject: for the forwarded message (using
> mh-forwarded-letter-subject, which in turn formats with
> mh-forward-subject-format), and then does:
> 
> 	(mh-insert-fields "Subject:" forw-subject)
> 
> I suspect that the intention is to replace any existing Subject:, or
> perhaps mh-forward assumes that the draft won't have one. Unfortunately
> for mh-forward, mh-insert-fields is specifically documented to *append*
> to the existing header field value, if there is one, not overwrite it,
> so the resulting Subject: of the draft is as we see here: the original
> subject created by 'forw', and then an appended Subject from mh-forward.
> This result is not terribly useful.
> 
> Reproduction in emacs -Q provided you have NMH and MH-E set up:
> * enter MH-E with 'M-x mh-rmail'
> * arrange to have some message in your inbox that you can forward, for
>   example by 'F r' to see all of your inbox.
> * display your test message to forward, eg cursor to it and hit Return
> * start to forward with 'f' or 'M-x mh-forward'.
> 
> NOTE: while I have reproduced this in a clean Emacs (and NMH)
> environment with 'emacs -Q', this report is being filed from within my
> regular customized Emacs because life is too short. You can skip over
> the packages, shadowed load files, and so on.

Adding Bill.

I'm not sure if it is better to report MH-E-related bugs here or
submit the reports to https://sourceforge.net/p/mh-e/bugs/.

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Mike Kupfer]

Eli Zaretskii wrote:

> > From: Chris Siebenmann <cks.emacsbugs-01@cs.toronto.edu>
> > Date: Tue, 21 Nov 2023 19:49:24 -0500
> > 
> > If you attempt to forward an email message in MH-E with mh-forward
> > (bound to eg 'f'), then the resulting Subject: of the draft message will
> > be manged and come out as, for example:
> > 
> > 	Subject: test forwarding subject (fwd) Chris Siebenmann: test forwarding subject
[...]
> > I suspect that the intention is to replace any existing Subject:, or
> > perhaps mh-forward assumes that the draft won't have one. Unfortunately
> > for mh-forward, mh-insert-fields is specifically documented to *append*
> > to the existing header field value, if there is one, not overwrite it,
> > so the resulting Subject: of the draft is as we see here: the original
> > subject created by 'forw', and then an appended Subject from mh-forward.
> > This result is not terribly useful.

Indeed.

Yeah, mh-insert-fields seems like the wrong choice for Subject.  It's
more suited for headers like To or Cc, when you might want to add to the
existing list of recipients.  Replacing mh-insert-fields with
mh-modify-header-field works for me.

diff --git a/lisp/mh-e/mh-comp.el b/lisp/mh-e/mh-comp.el
index 92d31bf1826..0d1dcdf626a 100644
--- a/lisp/mh-e/mh-comp.el
+++ b/lisp/mh-e/mh-comp.el
@@ -574,7 +574,7 @@ mh-forward
         (setq orig-subject (mh-get-header-field "Subject:")))
       (let ((forw-subject
              (mh-forwarded-letter-subject orig-from orig-subject)))
-        (mh-insert-fields "Subject:" forw-subject)
+        (mh-modify-header-field "Subject" forw-subject t)
         (goto-char (point-min))
         ;; Set the local value of mh-mail-header-separator according to what is
         ;; present in the buffer...

> > Reproduction in emacs -Q provided you have NMH and MH-E set up:
> > * enter MH-E with 'M-x mh-rmail'
> > * arrange to have some message in your inbox that you can forward, for
> >   example by 'F r' to see all of your inbox.

Hide your personal forwcomps if you have one.

> > * display your test message to forward, eg cursor to it and hit Return
> > * start to forward with 'f' or 'M-x mh-forward'.

mike

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Bill Wohler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks, Mike! My forwcomps only injects one header field so I never saw
this before. Uncommenting the Subject field made it possible to
reproduce this issue.

Yes, I agree that the code clearly wants to replace the Subject field,
not append to it, and this patch works for me as well. Please feel free
to commit the patch and resolve the issue. Thanks again!

Mike Kupfer <kupfer@rawbw.com> wrote:

> Eli Zaretskii wrote:
> 
> > > From: Chris Siebenmann <cks.emacsbugs-01@cs.toronto.edu>
> > > Date: Tue, 21 Nov 2023 19:49:24 -0500
> > > 
> > > If you attempt to forward an email message in MH-E with mh-forward
> > > (bound to eg 'f'), then the resulting Subject: of the draft message will
> > > be manged and come out as, for example:
> > > 
> > > 	Subject: test forwarding subject (fwd) Chris Siebenmann: test forwarding subject
> [...]
> > > I suspect that the intention is to replace any existing Subject:, or
> > > perhaps mh-forward assumes that the draft won't have one. Unfortunately
> > > for mh-forward, mh-insert-fields is specifically documented to *append*
> > > to the existing header field value, if there is one, not overwrite it,
> > > so the resulting Subject: of the draft is as we see here: the original
> > > subject created by 'forw', and then an appended Subject from mh-forward.
> > > This result is not terribly useful.
> 
> Indeed.
> 
> Yeah, mh-insert-fields seems like the wrong choice for Subject.  It's
> more suited for headers like To or Cc, when you might want to add to the
> existing list of recipients.  Replacing mh-insert-fields with
> mh-modify-header-field works for me.
> 
> diff --git a/lisp/mh-e/mh-comp.el b/lisp/mh-e/mh-comp.el
> index 92d31bf1826..0d1dcdf626a 100644
> --- a/lisp/mh-e/mh-comp.el
> +++ b/lisp/mh-e/mh-comp.el
> @@ -574,7 +574,7 @@ mh-forward
>          (setq orig-subject (mh-get-header-field "Subject:")))
>        (let ((forw-subject
>               (mh-forwarded-letter-subject orig-from orig-subject)))
> -        (mh-insert-fields "Subject:" forw-subject)
> +        (mh-modify-header-field "Subject" forw-subject t)
>          (goto-char (point-min))
>          ;; Set the local value of mh-mail-header-separator according to what is
>          ;; present in the buffer...
> 
> > > Reproduction in emacs -Q provided you have NMH and MH-E set up:
> > > * enter MH-E with 'M-x mh-rmail'
> > > * arrange to have some message in your inbox that you can forward, for
> > >   example by 'F r' to see all of your inbox.
> 
> Hide your personal forwcomps if you have one.
> 
> > > * display your test message to forward, eg cursor to it and hit Return
> > > * start to forward with 'f' or 'M-x mh-forward'.
> 
> mike
> 

-- 
Bill Wohler <wohler@newt.com> aka <Bill.Wohler@nasa.gov>
http://www.newt.com/wohler/, GnuPG ID:610BD9AD

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Bill Wohler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli,

Would you prefer that I apply Mike's patch to emacs-29 or master?

> diff --git a/lisp/mh-e/mh-comp.el b/lisp/mh-e/mh-comp.el
> index 92d31bf1826..0d1dcdf626a 100644
> --- a/lisp/mh-e/mh-comp.el
> +++ b/lisp/mh-e/mh-comp.el
> @@ -574,7 +574,7 @@ mh-forward
>          (setq orig-subject (mh-get-header-field "Subject:")))
>        (let ((forw-subject
>               (mh-forwarded-letter-subject orig-from orig-subject)))
> -        (mh-insert-fields "Subject:" forw-subject)
> +        (mh-modify-header-field "Subject" forw-subject t)
>          (goto-char (point-min))
>          ;; Set the local value of mh-mail-header-separator according to what is
>          ;; present in the buffer...
-- 
Bill Wohler <wohler@newt.com> aka <Bill.Wohler@nasa.gov>
http://www.newt.com/wohler/, GnuPG ID:610BD9AD

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Eli Zaretskii]

> cc: Mike Kupfer <kupfer@rawbw.com>,
>     Chris Siebenmann <cks.emacsbugs-01@cs.toronto.edu>,
>     67360@debbugs.gnu.org
> From: Bill Wohler <wohler@newt.com>
> Comments: In-reply-to Bill Wohler <wohler@newt.com>
>    message dated "Sat, 30 Dec 2023 14:05:29 -0800."
> Date: Sun, 31 Dec 2023 08:44:40 -0800
> 
> Eli,
> 
> Would you prefer that I apply Mike's patch to emacs-29 or master?
> 
> > diff --git a/lisp/mh-e/mh-comp.el b/lisp/mh-e/mh-comp.el
> > index 92d31bf1826..0d1dcdf626a 100644
> > --- a/lisp/mh-e/mh-comp.el
> > +++ b/lisp/mh-e/mh-comp.el
> > @@ -574,7 +574,7 @@ mh-forward
> >          (setq orig-subject (mh-get-header-field "Subject:")))
> >        (let ((forw-subject
> >               (mh-forwarded-letter-subject orig-from orig-subject)))
> > -        (mh-insert-fields "Subject:" forw-subject)
> > +        (mh-modify-header-field "Subject" forw-subject t)
> >          (goto-char (point-min))
> >          ;; Set the local value of mh-mail-header-separator according to what is
> >          ;; present in the buffer...

If you think it's safe enough for the release branch, emacs-29 is fine
with me.

Thanks.

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Bill Wohler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

tags 67360 fixed 29.2
thanks

* lisp/mh-e/mh-comp.el (mh-forward): Overwrite subject when forwarding.

-- 
Bill Wohler <wohler@newt.com> aka <Bill.Wohler@nasa.gov>
http://www.newt.com/wohler/, GnuPG ID:610BD9AD

bug#67360: 29.1; MH-E forwarded email has a mangled default Subject header

[Bill Wohler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> wrote:

> > cc: Mike Kupfer <kupfer@rawbw.com>,
> >     Chris Siebenmann <cks.emacsbugs-01@cs.toronto.edu>,
> >     67360@debbugs.gnu.org
> > From: Bill Wohler <wohler@newt.com>
> > Comments: In-reply-to Bill Wohler <wohler@newt.com>
> >    message dated "Sat, 30 Dec 2023 14:05:29 -0800."
> > Date: Sun, 31 Dec 2023 08:44:40 -0800
> > 
> > Eli,
> > 
> > Would you prefer that I apply Mike's patch to emacs-29 or master?
> > 
> > > diff --git a/lisp/mh-e/mh-comp.el b/lisp/mh-e/mh-comp.el
> > > index 92d31bf1826..0d1dcdf626a 100644
> > > --- a/lisp/mh-e/mh-comp.el
> > > +++ b/lisp/mh-e/mh-comp.el
> > > @@ -574,7 +574,7 @@ mh-forward
> > >          (setq orig-subject (mh-get-header-field "Subject:")))
> > >        (let ((forw-subject
> > >               (mh-forwarded-letter-subject orig-from orig-subject)))
> > > -        (mh-insert-fields "Subject:" forw-subject)
> > > +        (mh-modify-header-field "Subject" forw-subject t)
> > >          (goto-char (point-min))
> > >          ;; Set the local value of mh-mail-header-separator according to what is
> > >          ;; present in the buffer...
> 
> If you think it's safe enough for the release branch, emacs-29 is fine
> with me.

I think so, yes. I've pushed the fix and sent what may be my first
message to a *-done email address. It's never too late to start.

-- 
Bill Wohler <wohler@newt.com> aka <Bill.Wohler@nasa.gov>
http://www.newt.com/wohler/, GnuPG ID:610BD9AD