[patch] message-fetch-field, message-remove-header called improperly

[patch] message-fetch-field, message-remove-header called improperly

[Benjamin Rutt]

I believe that `message-fetch-field' and `message-remove-header'
should only be called when the message buffer has been narrowed to the
headers.  I have found that this is not always the case in message.el.
As a result, there are a few flaws in some callers of the above said
functions, and I've attached a patch below.

In some cases (e.g. if a header-like line is present in the body,
which it is sometimes when people are discussing mail and newsreaders
like on the gnus lists), calling `message-remove-header' un-narrowed
could destroy some part of the body of the message, which is clearly
wrong.  And similarly, calling `message-fetch-field' un-narrowed means
that some body lines which look like headers might be mis-interpreted
as header lines (e.g. see `message-change-subject').

The following functions have been fixed:

    `message-change-subject'
    `message-reduce-to-to-cc'
    `message-generate-unsubscribed-mail-followup-to'
    `message-insert-importance-low'
    `message-insert-importance-high'
    `message-insert-or-toggle-importance'
    `message-insert-disposition-notification-to'

And finally, the docstring was enhanced in

    `message-fetch-field'

to remind the elisp programmer that it must be called from a buffer
narrowed to the headers.  The fact that un-narrowed
`message-fetch-field' calls keep showing up in gnus code on newsgroups
and in message.el means that the users of that method are not aware
enough of the narrowing issue.  I think the doc patch helps.

Here is the patch against current CVS head message.el:

Index: message.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/message.el,v
retrieving revision 6.345
diff -c -r6.345 message.el
*** message.el	28 May 2003 21:35:28 -0000	6.345
--- message.el	29 May 2003 18:53:59 -0000
***************
*** 1533,1539 ****
        (looking-at message-unix-mail-delimiter))))
  
  (defun message-fetch-field (header &optional not-all)
!   "The same as `mail-fetch-field', only remove all newlines."
    (let* ((inhibit-point-motion-hooks t)
  	 (case-fold-search t)
  	 (value (mail-fetch-field header nil (not not-all))))
--- 1533,1540 ----
        (looking-at message-unix-mail-delimiter))))
  
  (defun message-fetch-field (header &optional not-all)
!   "The same as `mail-fetch-field', only remove all newlines.
! The buffer is expected to be narrowed to just the header of the message."
    (let* ((inhibit-point-motion-hooks t)
  	 (case-fold-search t)
  	 (value (mail-fetch-field header nil (not not-all))))
***************
*** 1664,1670 ****
  		       (zerop (string-width new-subject))
  		       (string-match "^[ \t]*$" new-subject))))
  	 (save-excursion
! 	   (let ((old-subject (message-fetch-field "Subject")))
  	     (cond ((not old-subject)
  		    (error "No current subject"))
  		   ((not (string-match
--- 1665,1674 ----
  		       (zerop (string-width new-subject))
  		       (string-match "^[ \t]*$" new-subject))))
  	 (save-excursion
! 	   (let ((old-subject
! 		  (save-restriction
! 		    (message-narrow-to-headers)
! 		    (message-fetch-field "Subject"))))
  	     (cond ((not old-subject)
  		    (error "No current subject"))
  		   ((not (string-match
***************
*** 1850,1868 ****
  (defun message-reduce-to-to-cc ()
   "Replace contents of To: header with contents of Cc: or Bcc: header."
   (interactive)
!  (let ((cc-content (message-fetch-field "cc"))
         (bcc nil))
     (if (and (not cc-content)
! 	    (setq cc-content (message-fetch-field "bcc")))
         (setq bcc t))
     (cond (cc-content
  	  (save-excursion
  	    (message-goto-to)
  	    (message-delete-line)
  	    (insert (concat "To: " cc-content "\n"))
! 	    (message-remove-header (if bcc
! 				       "bcc"
! 				     "cc")))))))
  
  ;;; End of functions adopted from `message-utils.el'.
  
--- 1854,1879 ----
  (defun message-reduce-to-to-cc ()
   "Replace contents of To: header with contents of Cc: or Bcc: header."
   (interactive)
!  (let ((cc-content
! 	(save-restriction (message-narrow-to-headers)
! 			  (message-fetch-field "cc")))
         (bcc nil))
     (if (and (not cc-content)
! 	    (setq cc-content
! 		  (save-restriction
! 		    (message-narrow-to-headers)
! 		    (message-fetch-field "bcc"))))
         (setq bcc t))
     (cond (cc-content
  	  (save-excursion
  	    (message-goto-to)
  	    (message-delete-line)
  	    (insert (concat "To: " cc-content "\n"))
! 	    (save-restriction
! 	      (message-narrow-to-headers)
! 	      (message-remove-header (if bcc
! 					 "bcc"
! 				       "cc"))))))))
  
  ;;; End of functions adopted from `message-utils.el'.
  
***************
*** 2512,2522 ****
  Cc: header are also put into the MFT."
  
    (interactive "P")
!   (message-remove-header "Mail-Followup-To")
!   (let* ((cc (and include-cc (message-fetch-field "Cc")))
! 	 (tos (if cc
! 		  (concat (message-fetch-field "To") "," cc)
! 		(message-fetch-field "To"))))
      (message-goto-mail-followup-to)
      (insert (concat tos ", " user-mail-address))))
  
--- 2523,2536 ----
  Cc: header are also put into the MFT."
  
    (interactive "P")
!   (let* (cc tos)
!     (save-restriction
!       (message-narrow-to-headers)
!       (message-remove-header "Mail-Followup-To")
!       (setq cc (and include-cc (message-fetch-field "Cc")))
!       (setq tos (if cc
! 		    (concat (message-fetch-field "To") "," cc)
! 		  (message-fetch-field "To"))))
      (message-goto-mail-followup-to)
      (insert (concat tos ", " user-mail-address))))
  
***************
*** 2779,2785 ****
    "Insert header to mark message as important."
    (interactive)
    (save-excursion
!     (message-remove-header "Importance")
      (message-goto-eoh)
      (insert "Importance: high\n")))
  
--- 2793,2801 ----
    "Insert header to mark message as important."
    (interactive)
    (save-excursion
!     (save-restriction
!       (message-narrow-to-headers)
!       (message-remove-header "Importance"))
      (message-goto-eoh)
      (insert "Importance: high\n")))
  
***************
*** 2787,2793 ****
    "Insert header to mark message as unimportant."
    (interactive)
    (save-excursion
!     (message-remove-header "Importance")
      (message-goto-eoh)
      (insert "Importance: low\n")))
  
--- 2803,2811 ----
    "Insert header to mark message as unimportant."
    (interactive)
    (save-excursion
!     (save-restriction
!       (message-narrow-to-headers)
!       (message-remove-header "Importance"))
      (message-goto-eoh)
      (insert "Importance: low\n")))
  
***************
*** 2800,2813 ****
      (let ((valid '("high" "normal" "low"))
  	  (new "high")
  	  cur)
!       (when (setq cur (message-fetch-field "Importance"))
! 	(message-remove-header "Importance")
! 	(setq new (cond ((string= cur "high")
! 			 "low")
! 			((string= cur "low")
! 			 "normal")
! 			(t
! 			 "high"))))
        (message-goto-eoh)
        (insert (format "Importance: %s\n" new)))))
  
--- 2818,2833 ----
      (let ((valid '("high" "normal" "low"))
  	  (new "high")
  	  cur)
!       (save-restriction
! 	(message-narrow-to-headers)
! 	(when (setq cur (message-fetch-field "Importance"))
! 	  (message-remove-header "Importance")
! 	  (setq new (cond ((string= cur "high")
! 			   "low")
! 			  ((string= cur "low")
! 			   "normal")
! 			  (t
! 			   "high")))))
        (message-goto-eoh)
        (insert (format "Importance: %s\n" new)))))
  
***************
*** 2816,2825 ****
  Note that this should not be used in newsgroups."
    (interactive)
    (save-excursion
!     (message-remove-header "Disposition-Notification-To")
      (message-goto-eoh)
      (insert (format "Disposition-Notification-To: %s\n"
! 		    (or (message-fetch-field "From") (message-make-from))))))
  
  (defun message-elide-region (b e)
    "Elide the text in the region.
--- 2836,2851 ----
  Note that this should not be used in newsgroups."
    (interactive)
    (save-excursion
!     (save-restriction
!       (message-narrow-to-headers)
!       (message-remove-header "Disposition-Notification-To"))
      (message-goto-eoh)
      (insert (format "Disposition-Notification-To: %s\n"
! 		    (or (save-excursion
! 			  (save-restriction
! 			    (message-narrow-to-headers)
! 			    (message-fetch-field "From")))
! 			(message-make-from))))))
  
  (defun message-elide-region (b e)
    "Elide the text in the region.

Re: [patch] message-fetch-field, message-remove-header called

[Kai Großjohann]

Benjamin Rutt <rutt+news@cis.ohio-state.edu> writes:

> I believe that `message-fetch-field' and `message-remove-header'
> should only be called when the message buffer has been narrowed to the
> headers.  I have found that this is not always the case in message.el.
> As a result, there are a few flaws in some callers of the above said
> functions, and I've attached a patch below.

Thank you very much.  I very slightly extended your docstring change
and committed this.

Gnah.  I wish I had fencepost access so I didn't have to ask for the
copyright situation every time.  Or maybe a less bad memory would be
sufficient.  This time, I'm too ashamed to ask...

I've tried various addresses to reactivate my fencepost account; does
anyone here know which one works?  (I believe system-hackers sent me
a nice autoreply, but nothing happened.)
-- 
This line is not blank.

Re: [patch] message-fetch-field, message-remove-header called

[Simon Josefsson]

kai.grossjohann@gmx.net (Kai Großjohann) writes:

> Gnah.  I wish I had fencepost access so I didn't have to ask for the
> copyright situation every time.  Or maybe a less bad memory would be
> sufficient.  This time, I'm too ashamed to ask...

EMACS GNUS       Benjamin Rutt
Assigns past and future changes to gnus. (message.el)

> I've tried various addresses to reactivate my fencepost account; does
> anyone here know which one works?  (I believe system-hackers sent me
> a nice autoreply, but nothing happened.)

Perhaps accounts at gnu.org works?

Re: [patch] message-fetch-field, message-remove-header called

[Benjamin Rutt]

kai.grossjohann@gmx.net (Kai Großjohann) writes:

> Thank you very much.  I very slightly extended your docstring change
> and committed this.

Your welcome, glad to have helped.  I think the docstring change is
useful and will prevent the type of the bugs I fixed in the patch from
re-occuring in the future.

> Gnah.  I wish I had fencepost access so I didn't have to ask for the
> copyright situation every time.  Or maybe a less bad memory would be
> sufficient.  This time, I'm too ashamed to ask...

Well, I only submit a patch about every 6 months so I wouldn't expect
anyone's memory to recall that I'm a legal contributor. :)
-- 
Benjamin