bug#4623: 23.1.50; rmail changes encoding of characters on save

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

With rmail I observed random encoding changes in mbox files:  some
non-ascii characters are changed depending on which message you look
at while saving.

Providing a test case is a bit tiresome, so I omit one for now.

The reason for the problem is that rmail-expunge-and-save uses the
coding system of the current message when saving the file.  This is
certainly wrong:  the coding system for writing the mbox file must not
depend on the currently selected message.

Since rmail reads the mbox file with coding-system 'no-conversion, it
should save it with coding-system 'no-conversion.

Perhaps the problem sits in rmail-swap-buffers:  Shouldn't
rmail-swap-buffers exchange the value of buffer-file-coding-system in
both ways?  Or perhaps it should ensure that the buffer which contains
the full mbox file has always the same buffer-file-coding-system,
namely 'no-conversion.

Anyway, the following ad hoc patch fixes the problem for me.

*** rmail.el.~1.552.~	2009-09-26 21:21:07.000000000 +0200
--- rmail.el	2009-10-02 16:14:30.000000000 +0200
***************
*** 1458,1464 ****
    (rmail-expunge)
    ;; No need to swap buffers: rmail-write-region-annotate takes care of it.
    ;; (rmail-swap-buffers-maybe)
!   (save-buffer)
    (if (rmail-summary-exists)
        (rmail-select-summary (set-buffer-modified-p nil))))
  
--- 1458,1465 ----
    (rmail-expunge)
    ;; No need to swap buffers: rmail-write-region-annotate takes care of it.
    ;; (rmail-swap-buffers-maybe)
!   (let ((coding-system-for-write 'no-conversion))
!     (save-buffer))
    (if (rmail-summary-exists)
        (rmail-select-summary (set-buffer-modified-p nil))))


In GNU Emacs 23.1.50.1 (i686-pc-linux-gnu, GTK+ Version 2.16.1)
 of 2009-10-02 on laptop
Windowing system distributor `The X.Org Foundation', version 11.0.10600000
configured using `configure  '--prefix=/home/rost/local/cvs''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: C
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Glenn Morris]

Thanks for the test case. I know very little about coding issues, but
if setting coding-system-for-write is the correct fix, I think it
should be done in a manner similar to the following, since saving an
rmail file with just "C-x C-s" should work.

*** rmail.el   6 Oct 2009 06:40:40 -0000  1.554
--- rmail.el   13 Oct 2009 01:17:20 -0000
***************
*** 4176,4181 ****
--- 4176,4182 ----
  (defun rmail-write-region-annotate (start end)
    (when (and (null start) (rmail-buffers-swapped-p))
      (set-buffer rmail-view-buffer)
+     (set (make-local-variable 'coding-system-for-write) 'no-conversion)
      (widen)
      nil))
  

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

The crucial thing is that the mbox file after reading and saving in
rmail differs from the original file only by some headers, usually
X-RMAIL-ATTRIBUTES, X-RMAIL-KEYWORDS.

Your patch fixes the problem.

> I think it
> should be done in a manner similar to the following, since saving an
> rmail file with just "C-x C-s" should work.

Yes, your patch is better than my attempts, as in

<URL:http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-10/msg00052.html>

and at the end of

<URL:http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-10/msg00182.html>

However all 3 patches leave the same problem, namely that the rmail
buffer gets the buffer-file-coding-system 'no-conversion.  This can be
easily seen from the mode line indicator ('=' instead of say '1' or
'u').  See the detailed discussion in the second part of

<URL:http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-10/msg00182.html>

Now this problem is of minor importance, at least I don't know of any
serious effects, and refreshing the rmail buffer by hitting "Nj"
(where N is the message number you are looking at) resets the
buffer-file-coding-system (and the mode line indicator).

One could work around this by calling

(rmail-show-message rmail-current-message)

after the save, although this isn't satisfactory.

Anyway, I think that your patch should be installed.

bug#4623: marked as done (23.1.50; rmail changes encoding of characters on save)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

Your message dated Fri, 16 Oct 2009 23:12:23 -0400
with message-id <22r5t2emso.fsf@fencepost.gnu.org>
and subject line Re: bug#4623: 23.1.50; rmail changes encoding of characters on save
has caused the Emacs bug report #4623,
regarding 23.1.50; rmail changes encoding of characters on save
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
4623: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4623
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 4255 bytes --]

From: Markus Rost <rost@math.uni-bielefeld.de>
To: emacs-pretest-bug@gnu.org
Subject: 23.1.50; rmail changes encoding of characters on save
Date: Sat,  3 Oct 2009 16:54:35 +0200 (CEST)
Message-ID: <20091003145435.75D5D288A0AB@xen12.math.uni-bielefeld.de>


With rmail I observed random encoding changes in mbox files:  some
non-ascii characters are changed depending on which message you look
at while saving.

Providing a test case is a bit tiresome, so I omit one for now.

The reason for the problem is that rmail-expunge-and-save uses the
coding system of the current message when saving the file.  This is
certainly wrong:  the coding system for writing the mbox file must not
depend on the currently selected message.

Since rmail reads the mbox file with coding-system 'no-conversion, it
should save it with coding-system 'no-conversion.

Perhaps the problem sits in rmail-swap-buffers:  Shouldn't
rmail-swap-buffers exchange the value of buffer-file-coding-system in
both ways?  Or perhaps it should ensure that the buffer which contains
the full mbox file has always the same buffer-file-coding-system,
namely 'no-conversion.

Anyway, the following ad hoc patch fixes the problem for me.

*** rmail.el.~1.552.~	2009-09-26 21:21:07.000000000 +0200
--- rmail.el	2009-10-02 16:14:30.000000000 +0200
***************
*** 1458,1464 ****
    (rmail-expunge)
    ;; No need to swap buffers: rmail-write-region-annotate takes care of it.
    ;; (rmail-swap-buffers-maybe)
!   (save-buffer)
    (if (rmail-summary-exists)
        (rmail-select-summary (set-buffer-modified-p nil))))
  
--- 1458,1465 ----
    (rmail-expunge)
    ;; No need to swap buffers: rmail-write-region-annotate takes care of it.
    ;; (rmail-swap-buffers-maybe)
!   (let ((coding-system-for-write 'no-conversion))
!     (save-buffer))
    (if (rmail-summary-exists)
        (rmail-select-summary (set-buffer-modified-p nil))))


In GNU Emacs 23.1.50.1 (i686-pc-linux-gnu, GTK+ Version 2.16.1)
 of 2009-10-02 on laptop
Windowing system distributor `The X.Org Foundation', version 11.0.10600000
configured using `configure  '--prefix=/home/rost/local/cvs''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: C
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t


[-- Attachment #3: Type: message/rfc822, Size: 1697 bytes --]

From: Glenn Morris <rgm@gnu.org>
To: 4623-done@emacsbugs.donarmstrong.com
Subject: Re: bug#4623: 23.1.50; rmail changes encoding of characters on save
Date: Fri, 16 Oct 2009 23:12:23 -0400
Message-ID: <22r5t2emso.fsf@fencepost.gnu.org>

Markus Rost wrote:

> Anyway, I think that your patch should be installed.

In the absence of any other comments on this, I did so.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

The cure for this bug, viz.

  *** rmail.el   6 Oct 2009 06:40:40 -0000  1.554
  --- rmail.el   13 Oct 2009 01:17:20 -0000
  ***************
  *** 4176,4181 ****
  --- 4176,4182 ----
    (defun rmail-write-region-annotate (start end)
      (when (and (null start) (rmail-buffers-swapped-p))
        (set-buffer rmail-view-buffer)
  +     (set (make-local-variable 'coding-system-for-write) 'no-conversion)
        (widen)
        nil))

is IMO not much better than the disease: now C-x C-s _always_ leaves
the RMAIL buffer in no-conversion state, until you display a message,
e.g., with "M-n".

I think this happens because the above sets the encoding of
rmail-view-buffer, where we display the messages, to no-conversion.

I'm not sure how to solve this.  What is needed is bind
coding-system-for-write to no-conversion just for the duration of
save-buffer or write-region that it calls eventually.  But because we
use annotations to switch the buffer behind save-buffer's back,
there's no easy way of doing that.

Perhaps we should special-case RMAIL in save-buffer, or maybe do
something in buffer-swap-text.

Any ideas are welcome.  If no better idea pops up very soon, I suggest
the following band-aid for Emacs 23.2:

  . don't set coding-system-for-write in rmail-write-region-annotate

  . bind C-x C-s in Rmail mode to a special command, like rmail-save,
    which will bind coding-system-for-write to no-conversion

  . wrap all instances of save-buffer in Rmail with a form that
    let-binds coding-system-for-write to no-conversion

This will at least plumb the problem in 99% of use cases, I think.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Glenn Morris]

Eli Zaretskii wrote:

> Any ideas are welcome.  If no better idea pops up very soon, I suggest
> the following band-aid for Emacs 23.2:

I was hoping at the time someone would comment, but no-one did. Please
do whatever you think is best.

>   . bind C-x C-s in Rmail mode to a special command, like rmail-save,
>     which will bind coding-system-for-write to no-conversion

What if someone does M-x save-buffer?

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Stefan Monnier]

> The cure for this bug, viz.
>   *** rmail.el   6 Oct 2009 06:40:40 -0000  1.554
>   --- rmail.el   13 Oct 2009 01:17:20 -0000
>   ***************
>   *** 4176,4181 ****
>   --- 4176,4182 ----
>     (defun rmail-write-region-annotate (start end)
>       (when (and (null start) (rmail-buffers-swapped-p))
>         (set-buffer rmail-view-buffer)
>   +     (set (make-local-variable 'coding-system-for-write) 'no-conversion)
>         (widen)
>         nil))

> is IMO not much better than the disease: now C-x C-s _always_ leaves
> the RMAIL buffer in no-conversion state, until you display a message,
> e.g., with "M-n".

> I think this happens because the above sets the encoding of
> rmail-view-buffer, where we display the messages, to no-conversion.

In the above code, rmail-view-buffer is (confusingly enough) the
raw-data buffer (which is why we switch to it to save the raw data), so
setting coding-system-for-write in that buffer is right.

The problem is that this setting is then somehow propagated to the other
buffer (the one we show to the user).

> Perhaps we should special-case RMAIL in save-buffer, or maybe do
> something in buffer-swap-text.

Most likely we need to do something in the rmail code that calls
buffer-swap-text.  Some debugging `message's in there showing the
coding-systems in use in each buffer before and after the swap should
make things more clear.


        Stefan

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 4623@emacsbugs.donarmstrong.com
> Date: Fri, 04 Dec 2009 13:42:31 -0500
> 
> Most likely we need to do something in the rmail code that calls
> buffer-swap-text.

Not sure what you had in mind.  buffer-swap-text is not called
(AFAICS) between "C-x C-s" and the return to top-level.  So how can
anything we do in rmail-swap-buffers help here?

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Stefan Monnier]

>> Most likely we need to do something in the rmail code that calls
>> buffer-swap-text.

> Not sure what you had in mind.  buffer-swap-text is not called
> (AFAICS) between "C-x C-s" and the return to top-level.  So how can
> anything we do in rmail-swap-buffers help here?

Oh, I see, it's because setting coding-system-for-write affect the
buffer-file-coding-system choice which is later saved into the
"original" buffer, which in this case is the displayed buffer.

Hmm...

Let's take a step back: when/why does buffer-file-coding-system matter
in the RMAIL display buffer?


        Stefan

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 4623@emacsbugs.donarmstrong.com
> Date: Fri, 04 Dec 2009 14:48:41 -0500
> 
> Oh, I see, it's because setting coding-system-for-write affect the
> buffer-file-coding-system choice which is later saved into the
> "original" buffer, which in this case is the displayed buffer.

Yes.

> Let's take a step back: when/why does buffer-file-coding-system matter
> in the RMAIL display buffer?

Several reasons off the top of my head:

  . It gives you an easily visible indication of how the message was
    encoded.  (We hide the relevant mail headers by default, so they
    are not visible unless you invoke rmail-toggle-header.)

  . It is useful when saving the current message to a file.

  . When replying to a message, rmail-reply uses it to set the
    encoding of the *mail* buffer where you compose the reply, which
    takes care of 99.99% of situations where the response needs to
    have some non-trivial encoding that is not your system's native
    one.

The last one of these is by far the most important reason, at least in
my use pattern.

Maybe we need to set up an after-save-hook to restore the original
encoding after saving the message collection?

Btw, I find rmail-swap-buffers incomplete in its handling of encoding
and the modified flag.  It looks like it works by sheer luck, unless
I'm missing something.  I suggest the following more thorough version.
If nothing else, it eliminates the need for the kludge in
rmail-write-region-annotate that forces no-conversion at save-buffer
time (because the encoding of the message collection buffer is now
kept at no-conversion at all times):

  (defun rmail-swap-buffers ()
    "Swap text between current buffer and `rmail-view-buffer'.
  This function preserves the buffers' modified flags, and also
  swaps the current buffer's `buffer-file-coding-system' with that
  of `rmail-view-buffer'."
    (let ((modp-this (buffer-modified-p))
	  (modp-that
	   (with-current-buffer rmail-view-buffer (buffer-modified-p)))
	  (coding-this buffer-file-coding-system)
	  (coding-that
	   (with-current-buffer rmail-view-buffer
	     buffer-file-coding-system)))
      (buffer-swap-text rmail-view-buffer)
      (setq buffer-file-coding-system coding-that)
      (with-current-buffer rmail-view-buffer
	(setq buffer-file-coding-system coding-this)
	(restore-buffer-modified-p modp-that))
      (restore-buffer-modified-p modp-this)))

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

> Btw, I find rmail-swap-buffers incomplete in its handling of encoding
> and the modified flag.  It looks like it works by sheer luck, unless
> I'm missing something.  I suggest the following more thorough version.

I feel like running in circles.  Eli's rmail-swap-buffers function
(except for the modp-that) is in my previous post

<URL:http://lists.gnu.org/archive/html/bug-gnu-emacs/2009-10/msg00182.html>

which itself mentions Eli's Oct 2008 message:

<URL:http://lists.gnu.org/archive/html/emacs-devel/2008-10/msg00468.html>

See also the other messages in

<URL:http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4623>
<URL:http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4655>

> Maybe we need to set up an after-save-hook to restore the original
> encoding after saving the message collection?

Agreed.  I don't see any other way without changing files.el and
fileio.c.  The real problem is the buffer change in
write-region-annotate-functions and this part of basic-save-buffer

		(setq setmodes (basic-save-buffer-1)))
	    ;; Now we have saved the current buffer.  Let's make sure
	    ;; that buffer-file-coding-system is fixed to what
	    ;; actually used for saving by binding it locally.
	    (if save-buffer-coding-system
		(setq save-buffer-coding-system last-coding-system-used)
	      (setq buffer-file-coding-system last-coding-system-used))

which just assumes that last-coding-system-used was used in the
current buffer.  This part of basic-save-buffer practically forbids a
buffer change in write-region-annotate-functions.

Anyway, I think that the logical way for saving in Rmail is to swap
buffers back and forth and not change buffers.

What about the patch below?

And:  Could someone change the name of the variable rmail-view-buffer?
A good name is perhaps 'rmail-hold-buffer'.

Also: The doc string of rmail-buffer-swapped is not helpful.  What about

(defvar rmail-buffer-swapped nil
  "If nil, `rmail-buffer' contains the actual mbox message collection")

or

(defvar rmail-buffer-swapped nil
  "If non-nil, the actual mbox message collection is in `rmail-hold-buffer'.")


=== Buffer *vc-diff* =======================================
*** rmail.el	05 Dec 2009 20:13:54 +0100	1.562
--- rmail.el	05 Dec 2009 20:38:06 +0100	
***************
*** 1279,1284 ****
--- 1279,1286 ----
    (rmail-perm-variables)
    (rmail-variables))
  
+ (defvar rmail-swapped-before-save nil)
+ 
  (defun rmail-mode-1 ()
    (setq major-mode 'rmail-mode)
    (setq mode-name "RMAIL")
***************
*** 1293,1303 ****
    (set-syntax-table text-mode-syntax-table)
    (setq local-abbrev-table text-mode-abbrev-table)
    ;; Functions to support buffer swapping:
!   (add-hook 'write-region-annotate-functions
! 	    'rmail-write-region-annotate nil t)
    (add-hook 'kill-buffer-hook 'rmail-mode-kill-buffer-hook nil t)
    (add-hook 'change-major-mode-hook 'rmail-change-major-mode-hook nil t))
  
  (defun rmail-generate-viewer-buffer ()
    "Return a reusable buffer suitable for viewing messages.
  Create the buffer if necessary."
--- 1295,1322 ----
    (set-syntax-table text-mode-syntax-table)
    (setq local-abbrev-table text-mode-abbrev-table)
    ;; Functions to support buffer swapping:
!   (make-local-variable 'rmail-swapped-before-save)
!   (add-hook 'before-save-hook 'rmail-swap-before-save nil t)
!   (add-hook 'after-save-hook 'rmail-swap-after-save t t)
    (add-hook 'kill-buffer-hook 'rmail-mode-kill-buffer-hook nil t)
    (add-hook 'change-major-mode-hook 'rmail-change-major-mode-hook nil t))
  
+ (defun rmail-swap-before-save ()
+   (setq rmail-swapped-before-save nil)
+   (when (rmail-buffers-swapped-p)
+     (rmail-swap-buffers)
+     ;; This is probably not necssary, but let's be on the safe side.
+     (set (make-local-variable 'coding-system-for-write) 'no-conversion)
+     (setq rmail-buffer-swapped nil)
+     (setq rmail-swapped-before-save t)
+     (widen)))
+ 
+ (defun rmail-swap-after-save ()
+   (when rmail-swapped-before-save
+     (rmail-swap-buffers)
+     (setq rmail-buffer-swapped t)
+     (setq rmail-swapped-before-save nil)))
+ 
  (defun rmail-generate-viewer-buffer ()
    "Return a reusable buffer suitable for viewing messages.
  Create the buffer if necessary."
***************
*** 1313,1328 ****
  
  (defun rmail-swap-buffers ()
    "Swap text between current buffer and `rmail-view-buffer'.
! This function preserves the current buffer's modified flag, and also
! sets the current buffer's `buffer-file-coding-system' to that of
! `rmail-view-buffer'."
!   (let ((modp (buffer-modified-p))
! 	(coding
  	 (with-current-buffer rmail-view-buffer
  	   buffer-file-coding-system)))
      (buffer-swap-text rmail-view-buffer)
!     (setq buffer-file-coding-system coding)
!     (restore-buffer-modified-p modp)))
  
  (defun rmail-buffers-swapped-p ()
    "Return non-nil if the message collection is in `rmail-view-buffer'."
--- 1332,1353 ----
  
  (defun rmail-swap-buffers ()
    "Swap text between current buffer and `rmail-view-buffer'.
!   This function preserves the buffers' modified flags, and also
!   swaps the current buffer's `buffer-file-coding-system' with that
!   of `rmail-view-buffer'."
!   (let ((modp-this (buffer-modified-p))
! 	(modp-that
! 	 (with-current-buffer rmail-view-buffer (buffer-modified-p)))
! 	(coding-this buffer-file-coding-system)
! 	(coding-that
  	 (with-current-buffer rmail-view-buffer
  	   buffer-file-coding-system)))
      (buffer-swap-text rmail-view-buffer)
!     (setq buffer-file-coding-system coding-that)
!     (with-current-buffer rmail-view-buffer
!       (setq buffer-file-coding-system coding-this)
!       (restore-buffer-modified-p modp-that))
!     (restore-buffer-modified-p modp-this)))
  
  (defun rmail-buffers-swapped-p ()
    "Return non-nil if the message collection is in `rmail-view-buffer'."
***************
*** 1470,1477 ****
    (interactive)
    (set-buffer rmail-buffer)
    (rmail-expunge)
-   ;; No need to swap buffers: rmail-write-region-annotate takes care of it.
-   ;; (rmail-swap-buffers-maybe)
    (save-buffer)
    (if (rmail-summary-exists)
        (rmail-select-summary (set-buffer-modified-p nil))))
--- 1495,1500 ----
***************
*** 4179,4194 ****
  (add-to-list 'desktop-buffer-mode-handlers
  	     '(rmail-mode . rmail-restore-desktop-buffer))
  
- ;; Used in `write-region-annotate-functions' to write rmail files.
- (defun rmail-write-region-annotate (start end)
-   (when (and (null start) (rmail-buffers-swapped-p))
-     (set-buffer rmail-view-buffer)
-     ;; Prevent viewing different messages from messing up the coding. (Bug#4623)
-     ;; FIXME is there a better solution?
-     (set (make-local-variable 'coding-system-for-write) 'no-conversion)
-     (widen)
-     nil))
- 
  \f
  ;;; Start of automatically extracted autoloads.
  \f
--- 4202,4207 ----
============================================================

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Markus Rost <rost@math.uni-bielefeld.de>
> CC: monnier@iro.umontreal.ca, 4623@emacsbugs.donarmstrong.com
> Date: Sat,  5 Dec 2009 21:10:56 +0100 (CET)
> 
> > Btw, I find rmail-swap-buffers incomplete in its handling of encoding
> > and the modified flag.  It looks like it works by sheer luck, unless
> > I'm missing something.  I suggest the following more thorough version.
> 
> I feel like running in circles.

Nah, it just means good ideas never die, they just get reborn ;-)

> What about the patch below?

I think it goes a tad too far.  What I had in mind was way simpler:
record the encoding of the current message inside rmail-show-message,
then restore it in after-save-hook.

Stefan, I surely would like to hear from you on this one, the 23.2
release branch being so close.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

> I think it goes a tad too far.  What I had in mind was way simpler:

I tried a bit by just adding something to after-save-hook.  But I ran
into problems with C-u g (rmail-get-new-mail from some file).

> record the encoding of the current message inside rmail-show-message,
> then restore it in after-save-hook.

I didn't try that.  Maybe that could work.

Actually, in the end my patch was meant to be simple and safe.  Its
changes can't be wrong (in the sense:  I proved the code is correct,
but didn't try it..., or just a few times).  It implements your
version of rmail-swap-buffers, which can't be wrong.  It removes
switching buffers in write-region-annotate-functions, which is wrong.
The buffer swapping with before/after-save-hook was at first meant as
a brutal but safe kludge, but on second thought it looks very natural
to me.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Markus Rost <rost@math.uni-bielefeld.de>
> CC: monnier@iro.umontreal.ca, 4623@emacsbugs.donarmstrong.com
> Date: Sat,  5 Dec 2009 23:37:27 +0100 (CET)
> 
> Actually, in the end my patch was meant to be simple and safe.

I didn't say it wasn't.  I just think that 2 days before the feature
freeze, we need to find a solution that modifies the current code as
little as possible.  The current code has survived since the pretest
of v23.1, and changing it significantly would be unsafe and could be
justifiably voted down by Stefan and Yidong.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

>  I just think that 2 days before the feature
> freeze,

I didn't know it is so close.

> changing it significantly would be unsafe

Indeed:  rmail-write-region-annotate is also in rmail-edit-mode in
rmailedit.el.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> Date: Sat, 05 Dec 2009 23:52:54 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 4623@emacsbugs.donarmstrong.com
> 
> I think it goes a tad too far.  What I had in mind was way simpler:
> record the encoding of the current message inside rmail-show-message,
> then restore it in after-save-hook.

Here's what I came up with.  This is relative to current CVS.  It
seems to work, but I'd appreciate more testing.  Markus, could you
please see if it works for you?  TIA

Index: lisp/mail/rmail.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/mail/rmail.el,v
retrieving revision 1.562
diff -u -r1.562 rmail.el
--- lisp/mail/rmail.el	4 Dec 2009 05:00:47 -0000	1.562
+++ lisp/mail/rmail.el	6 Dec 2009 19:16:05 -0000
@@ -642,6 +642,9 @@
 (defvar rmail-enable-mime-composing nil
   "*If non-nil, RMAIL uses `rmail-insert-mime-forwarded-message-function' to forward.")
 
+;; We use this to record the encoding of the last displayed message.
+(defvar rmail-message-encoding nil)
+
 ;; FIXME unused.
 (defvar rmail-show-mime-function nil
   "Function to show MIME decoded message of RMAIL file.
@@ -1316,13 +1319,19 @@
 This function preserves the current buffer's modified flag, and also
 sets the current buffer's `buffer-file-coding-system' to that of
 `rmail-view-buffer'."
-  (let ((modp (buffer-modified-p))
-	(coding
+  (let ((modp-this (buffer-modified-p))
+	(modp-that
+	 (with-current-buffer rmail-view-buffer (buffer-modified-p)))
+	(coding-this buffer-file-coding-system)
+	(coding-that
 	 (with-current-buffer rmail-view-buffer
 	   buffer-file-coding-system)))
     (buffer-swap-text rmail-view-buffer)
-    (setq buffer-file-coding-system coding)
-    (restore-buffer-modified-p modp)))
+    (setq buffer-file-coding-system coding-that)
+    (with-current-buffer rmail-view-buffer
+      (setq buffer-file-coding-system coding-this)
+      (restore-buffer-modified-p modp-that))
+    (restore-buffer-modified-p modp-this)))
 
 (defun rmail-buffers-swapped-p ()
   "Return non-nil if the message collection is in `rmail-view-buffer'."
@@ -2748,6 +2757,7 @@
 	  (rmail-highlight-headers)
 					;(rmail-activate-urls)
 					;(rmail-process-quoted-material)
+	  (setq rmail-message-encoding buffer-file-coding-system)
 	  )
 	;; Update the mode-line with message status information and swap
 	;; the view buffer/mail buffer contents.
@@ -4185,10 +4195,20 @@
     (set-buffer rmail-view-buffer)
     ;; Prevent viewing different messages from messing up the coding. (Bug#4623)
     ;; FIXME is there a better solution?
-    (set (make-local-variable 'coding-system-for-write) 'no-conversion)
+    ;(set (make-local-variable 'coding-system-for-write) 'no-conversion)
     (widen)
     nil))
 
+;; Used to restore the encoding of the buffer where we show the
+;; current message.
+(defun rmail-after-save-hook ()
+  (if (or (eq rmail-view-buffer (current-buffer))
+	  (eq rmail-buffer (current-buffer)))
+      (with-current-buffer
+	  (if (rmail-buffers-swapped-p) rmail-buffer rmail-view-buffer)
+	(setq buffer-file-coding-system rmail-message-encoding))))
+(add-hook 'after-save-hook 'rmail-after-save-hook)
+
 \f
 ;;; Start of automatically extracted autoloads.
 \f

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

> Here's what I came up with.

You are a Master of the Universe.  Once rmail-swap-buffers is set up
correctly, things get much simpler.

I tested:  The old bug is still gone, and problems which I encountered
during my own hacking don't show up.

One minor thing:  Since rmail-message-encoding is a global variable,
several Rmail buffers may interfere.  If you have displayed a message
in buffer A with encoding A (so rmail-message-encoding is set to A)
and you switch to a modified Rmail buffer B and save that buffer, then
the message in buffer B gets encoding A.  Only temporarily of course,
as before.  I don't suggest to fix that now.  Perhaps one could put a
note in the code.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Markus Rost <rost@math.uni-bielefeld.de>
> CC: 4623@emacsbugs.donarmstrong.com
> Date: Sun,  6 Dec 2009 21:23:48 +0100 (CET)
> 
> One minor thing:  Since rmail-message-encoding is a global variable,
> several Rmail buffers may interfere.

Right, I didn't think abut that possibility.  I will try to think
about some simple solution.

Thanks for testing.  Unless someone objects, I will commit these
changes (suitably cleaned up) by today's evening.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Stefan Monnier]

> Several reasons off the top of my head:
>   . It gives you an easily visible indication of how the message was
>     encoded.  (We hide the relevant mail headers by default, so they
>     are not visible unless you invoke rmail-toggle-header.)

So it's abusing buffer-file-coding-system for "message-coding-system"
(which of course is only meaningful is the message is not made up of
various parts using different coding-systems).

>   . It is useful when saving the current message to a file.

Good point.

>   . When replying to a message, rmail-reply uses it to set the
>     encoding of the *mail* buffer where you compose the reply, which
>     takes care of 99.99% of situations where the response needs to
>     have some non-trivial encoding that is not your system's native
>     one.

rmail-replay doesn't need to use buffer-file-coding-system for that, but
could just as well use some other variable for it, right?

> The last one of these is by far the most important reason, at least in
> my use pattern.

Then it's easy to fix.

> Maybe we need to set up an after-save-hook to restore the original
> encoding after saving the message collection?

That would be fine, yes.  I'd suggest to use an rmail-coding-system
variable as the canonical place to store the coding system used for the
currently shown message, use it in rmail-reply in preference to
buffer-file-coding-system, and simply copy rmail-coding-system to
buffer-file-coding-system whenever necessary, such as in
after-save-hook.

> Btw, I find rmail-swap-buffers incomplete in its handling of encoding
> and the modified flag.  It looks like it works by sheer luck, unless
> I'm missing something.

Could be.  I really think the first thing that needs to be done is to
get rid of Rmail's constant swapping of those buffers.  There should be
one swap when entering rmail-mode and one more when(if) leaving it,
no more.  That will get us rid of most of that insanity.


        Stefan

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 4623@emacsbugs.donarmstrong.com
> Date: Sun, 06 Dec 2009 23:34:55 -0500
> 
> >   . When replying to a message, rmail-reply uses it to set the
> >     encoding of the *mail* buffer where you compose the reply, which
> >     takes care of 99.99% of situations where the response needs to
> >     have some non-trivial encoding that is not your system's native
> >     one.
> 
> rmail-replay doesn't need to use buffer-file-coding-system for that, but
> could just as well use some other variable for it, right?

If that variable is set correctly, sure.  However,
buffer-file-coding-system has the advantage that it is already
prominently shown in the mode line.

> > Maybe we need to set up an after-save-hook to restore the original
> > encoding after saving the message collection?
> 
> That would be fine, yes.  I'd suggest to use an rmail-coding-system
> variable as the canonical place to store the coding system used for the
> currently shown message, use it in rmail-reply in preference to
> buffer-file-coding-system, and simply copy rmail-coding-system to
> buffer-file-coding-system whenever necessary, such as in
> after-save-hook.

Well, I did precisely that, as you could see, but then Markus pointed
out that a single global variable will not work when there are more
than one Rmail buffer.

I will post a slightly improved patch soon that hopefully will take
care of that as well.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> Date: Mon, 07 Dec 2009 06:04:34 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 4623@emacsbugs.donarmstrong.com
> 
> > From: Markus Rost <rost@math.uni-bielefeld.de>
> > CC: 4623@emacsbugs.donarmstrong.com
> > Date: Sun,  6 Dec 2009 21:23:48 +0100 (CET)
> > 
> > One minor thing:  Since rmail-message-encoding is a global variable,
> > several Rmail buffers may interfere.
> 
> Right, I didn't think abut that possibility.  I will try to think
> about some simple solution.

I think I found it.

If we have to live with rmail-write-region-annotate, we might as well
have it do something useful:

Index: lisp/mail/rmail.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/mail/rmail.el,v
retrieving revision 1.562
diff -u -r1.562 rmail.el
--- lisp/mail/rmail.el	4 Dec 2009 05:00:47 -0000	1.562
+++ lisp/mail/rmail.el	7 Dec 2009 19:14:23 -0000
@@ -1316,13 +1316,19 @@
 This function preserves the current buffer's modified flag, and also
 sets the current buffer's `buffer-file-coding-system' to that of
 `rmail-view-buffer'."
-  (let ((modp (buffer-modified-p))
-	(coding
+  (let ((modp-this (buffer-modified-p))
+	(modp-that
+	 (with-current-buffer rmail-view-buffer (buffer-modified-p)))
+	(coding-this buffer-file-coding-system)
+	(coding-that
 	 (with-current-buffer rmail-view-buffer
 	   buffer-file-coding-system)))
     (buffer-swap-text rmail-view-buffer)
-    (setq buffer-file-coding-system coding)
-    (restore-buffer-modified-p modp)))
+    (setq buffer-file-coding-system coding-that)
+    (with-current-buffer rmail-view-buffer
+      (setq buffer-file-coding-system coding-this)
+      (restore-buffer-modified-p modp-that))
+    (restore-buffer-modified-p modp-this)))
 
 (defun rmail-buffers-swapped-p ()
   "Return non-nil if the message collection is in `rmail-view-buffer'."
@@ -4179,16 +4185,31 @@
 (add-to-list 'desktop-buffer-mode-handlers
 	     '(rmail-mode . rmail-restore-desktop-buffer))
 
+;; We use this to record the encoding of the current message before
+;; saving the message collection.
+(defvar rmail-message-encoding nil)
+
 ;; Used in `write-region-annotate-functions' to write rmail files.
 (defun rmail-write-region-annotate (start end)
   (when (and (null start) (rmail-buffers-swapped-p))
+    (setq rmail-message-encoding buffer-file-coding-system)
     (set-buffer rmail-view-buffer)
-    ;; Prevent viewing different messages from messing up the coding. (Bug#4623)
-    ;; FIXME is there a better solution?
-    (set (make-local-variable 'coding-system-for-write) 'no-conversion)
     (widen)
     nil))
 
+;; Used to restore the encoding of the buffer where we show the
+;; current message, after we save the message collection.  This is
+;; needed because rmail-write-region-annotate switches buffers behind
+;; save-file's back, with the side effect that last-coding-system-used
+;; is assigned to buffer-file-coding-system of the wrong buffer.
+(defun rmail-after-save-hook ()
+  (if (or (eq rmail-view-buffer (current-buffer))
+	  (eq rmail-buffer (current-buffer)))
+      (with-current-buffer
+	  (if (rmail-buffers-swapped-p) rmail-buffer rmail-view-buffer)
+	(setq buffer-file-coding-system rmail-message-encoding))))
+(add-hook 'after-save-hook 'rmail-after-save-hook)
+
 \f
 ;;; Start of automatically extracted autoloads.
 \f

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Stefan Monnier]

>> That would be fine, yes.  I'd suggest to use an rmail-coding-system
>> variable as the canonical place to store the coding system used for the
>> currently shown message, use it in rmail-reply in preference to
>> buffer-file-coding-system, and simply copy rmail-coding-system to
>> buffer-file-coding-system whenever necessary, such as in
>> after-save-hook.

> Well, I did precisely that, as you could see, but then Markus pointed
> out that a single global variable will not work when there are more
> than one Rmail buffer.

Of course, just like buffer-file-coding-system, it should be buffer-local.


        Stefan

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Markus Rost]

> I think I found it.

Yes.  With the new rmail-swap-buffers one can rely on the correct
values of buffer-file-coding-system in the rmail and rmail-view
buffers at all times -- and use them.

I tested the new patch.  It works for me.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Markus Rost <rost@math.uni-bielefeld.de>
> CC: 4623@emacsbugs.donarmstrong.com
> Date: Mon,  7 Dec 2009 20:50:35 +0100 (CET)
> 
> I tested the new patch.  It works for me.

Thanks, committed.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 4623@emacsbugs.donarmstrong.com
> Date: Mon, 07 Dec 2009 14:25:37 -0500
> 
> > Well, I did precisely that, as you could see, but then Markus pointed
> > out that a single global variable will not work when there are more
> > than one Rmail buffer.
> 
> Of course, just like buffer-file-coding-system, it should be buffer-local.

Yes, I thought about that at first.  But this solution complicates
things because of the swapping.  So I went for a slightly different
one, which stashes the encoding immediately prior to switching to the
``other'' buffer.  This makes sure we have always the correct
encoding.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Stefan Monnier]

> Yes, I thought about that at first.  But this solution complicates
> things because of the swapping.

As I said: we should first fix the swapping madness.


        Stefan

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 4623@emacsbugs.donarmstrong.com
> Date: Mon, 07 Dec 2009 16:44:24 -0500
> 
> > Yes, I thought about that at first.  But this solution complicates
> > things because of the swapping.
> 
> As I said: we should first fix the swapping madness.

Not sure how you propose to do that.  All users of rmail-swap-buffers
do that because they need to do something with the entire message
collection.

bug#4623: 23.1.50; rmail changes encoding of characters on save

[Stefan Monnier]

>> > Yes, I thought about that at first.  But this solution complicates
>> > things because of the swapping.
>> As I said: we should first fix the swapping madness.
> Not sure how you propose to do that.  All users of rmail-swap-buffers
> do that because they need to do something with the entire message
> collection.

There's no need to swap buffers for that, there's with-current-buffer.
I know it's not as simple as that because they may also need to access
buffer-local variables.


        Stefan