bug#25365: 25.1; Coding system for bookmarks and desktop

[Eli Zaretskii]

> From: Dmitri Paduchikh <dpaduchikh@gmail.com>
> Date: Thu, 05 Jan 2017 17:37:30 +0500
> 
> It appears that all Cyrillic text in my bookmarks file has been
> corrupted. I wasn't able to reproduce such a corruption using emacs -Q,
> so probably this is due to interference with my settings which I will
> have to investigate. But in my opinion there is a problem with
> bookmark.el as well since it ignores completely coding system when
> saving bookmarks. Thus I have written the following two advices to fix
> its behavior. It seems that they work as needed.
> 
> (advice-add 'bookmark-write-file :around
>             #'(lambda (f &rest args)
>                 (let ((coding-system-for-write (or coding-system-for-write 
> 'utf-8-emacs)))
>                   (apply f args)))
>             '((name . "coding")))

Thanks, I used something similar in the patch below, except that I
think we should honor the existing coding cookie in the bookmark file,
instead of forcing the user to override the default each time the file
is saved.  So we should record the encoding when we load the file, and
apply it when saving.  This way, the user could specify the
non-default encoding only once, using "C-x RET c", and have that value
honored by Emacs thereafter.

> (advice-add 'bookmark-insert-file-format-version-stamp :before
>             #'(lambda (&rest args)
>                 (when coding-system-for-write
>                   (insert (format "\
> ;;;; Emacs Bookmark Format Version %d ;;;;
> ;;; -*- coding: %S -*-\n"
>                                   bookmark-file-format-version
>                                   coding-system-for-write))))
>             '((name . "coding")))

This is not quite right, as coding-system-for-write could be something
inappropriate, like undecided or prefer-utf-8.  And again, we should
honor the existing encoding.  So I modified this part to support that.

I installed the patch below on the master branch.  Please try it
(after removing your advices) and see if it gives good results.  If
you see problems, please reopen this bug with the details.

Thanks.

> Besides, although desktop.el specifies coding system in its file, it is
> old one - emacs-mule. Shouldn't this be utf-8-emacs these days instead?

I'm not sure how many users share the desktop files with older Emacs
versions that didn't support utf-8-emacs.  There's nothing wrong with
using emacs-mule in recent Emacs versions, for files that aren't
supposed to be read by programs that are not Emacs.

commit e272032769178768cf970839a9c22aba1f5b572e
Author:     Eli Zaretskii <eliz@gnu.org>
Date: Sat Jan 7 14:33:41 2017 +0200

    Specify encoding of the bookmark file
    
    * lisp/bookmark.el (bookmark-insert-file-format-version-stamp):
    Accept an argument CODING and include a 'coding:' cookie in the
    bookmark file preamble.
    (bookmark-upgrade-file-format-from-0): Call
    'bookmark-insert-file-format-version-stamp' with the file buffer's
    encoding, as detected when it was read.
    (bookmark-file-coding-system): New variable.
    (bookmark-load): Set bookmark-file-coding-system to the encoding
    of the loaded file.
    (bookmark-write-file): Bind coding-system-for-write to either the
    user setting via "C-x RET c" or to the existing file encoding,
    defaulting to 'utf-8-emacs'.  Update the value of
    bookmark-file-coding-system.  (Bug#25365)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 3440a52..e18362b 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -267,6 +267,8 @@ bookmark-alist
 (defvar bookmarks-already-loaded nil
   "Non-nil if and only if bookmarks have been loaded from 
`bookmark-default-file'.")
 
+(defvar bookmark-file-coding-system nil
+  "The coding-system of the last loaded or saved bookmark file.")
 
 ;; more stuff added by db.
 
@@ -689,7 +691,7 @@ bookmark-upgrade-file-format-from-0
   (let* ((old-list (bookmark-alist-from-buffer))
          (new-list (bookmark-upgrade-version-0-alist old-list)))
     (delete-region (point-min) (point-max))
-    (bookmark-insert-file-format-version-stamp)
+    (bookmark-insert-file-format-version-stamp buffer-file-coding-system)
     (pp new-list (current-buffer))
     (save-buffer))
   (goto-char (point-min))
@@ -726,11 +728,14 @@ bookmark-maybe-upgrade-file-format
       (error "Bookmark file format version strangeness")))))
 
 
-(defun bookmark-insert-file-format-version-stamp ()
-  "Insert text indicating current version of bookmark file format."
+(defun bookmark-insert-file-format-version-stamp (coding)
+  "Insert text indicating current version of bookmark file format.
+CODING is the symbol of the coding-system in which the file is encoded."
+  (if (memq (coding-system-base coding) '(undecided prefer-utf-8))
+      (setq coding 'utf-8-emacs))
   (insert
-   (format ";;;; Emacs Bookmark Format Version %d ;;;;\n"
-           bookmark-file-format-version))
+   (format ";;;; Emacs Bookmark Format Version %d ;;;; -*- coding: %S -*- \n"
+           bookmark-file-format-version (coding-system-base coding)))
   (insert ";;; This format is meant to be slightly human-readable;\n"
           ";;; nevertheless, you probably don't want to edit it.\n"
           ";;; "
@@ -1417,14 +1422,17 @@ bookmark-write-file
   (with-current-buffer (get-buffer-create " *Bookmarks*")
     (goto-char (point-min))
     (delete-region (point-min) (point-max))
-    (let ((print-length nil)
+    (let ((coding-system-for-write
+           (or coding-system-for-write
+               bookmark-file-coding-system 'utf-8-emacs))
+          (print-length nil)
           (print-level nil)
           ;; See bug #12503 for why we bind `print-circle'.  Users
           ;; can define their own bookmark types, which can result in
           ;; arbitrary Lisp objects being stored in bookmark records,
           ;; and some users create objects containing circularities.
           (print-circle t))
-      (bookmark-insert-file-format-version-stamp)
+      (bookmark-insert-file-format-version-stamp coding-system-for-write)
       (insert "(")
       ;; Rather than a single call to `pp' we make one per bookmark.
       ;; Apparently `pp' has a poor algorithmic complexity, so this
@@ -1440,6 +1448,7 @@ bookmark-write-file
         (condition-case nil
             (write-region (point-min) (point-max) file)
           (file-error (message "Can't write %s" file)))
+        (setq bookmark-file-coding-system coding-system-for-write)
         (kill-buffer (current-buffer))
         (bookmark-maybe-message
          "Saving bookmarks to file %s...done" file)))))
@@ -1521,7 +1530,8 @@ bookmark-load
                     (expand-file-name bookmark-default-file))
                    file)
                   (setq bookmarks-already-loaded t))
-              (bookmark-bmenu-surreptitiously-rebuild-list))
+              (bookmark-bmenu-surreptitiously-rebuild-list)
+              (setq bookmark-file-coding-system buffer-file-coding-system))
           (error "Invalid bookmark list in %s" file)))
       (kill-buffer (current-buffer)))
     (if (null no-msg)