bug#63627: Improve plstore.el and fix various issues of it

[Jens Schmidt via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

Tag: patch

Here come the first two patches:


0001-Add-internal-documentation-on-plstore.el.patch

Few but large changes, mostly in the commentary section, to describe 
internal structures and nail down terminology.


0002-Brush-up-doc-strings-and-terminology-in-plstore.el.patch

Many but small changes, building on the work from the first patch.


All of these changes so far are doc fixes or message changes, none of 
them touches any code.  Not sure whether they qualify for emacs-29, but 
I guess it would be easier to have them there to have the diff context 
ready for any following bug fixes.

The further plan is: Provide bug fixes, ERT tests for existing code, new 
features, ERT tests for new features (roughly in that order, new 
features last).  So please don't close this bug yet.

Comments welcome.

[-- Attachment #2: 0001-Add-internal-documentation-on-plstore.el.patch --]
[-- Type: text/x-patch, Size: 7865 bytes --]

From 6b0fc4bc93717e6ff34bfb05bf66a79d9370ab5d Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Sun, 21 May 2023 21:37:35 +0200
Subject: [PATCH 1/2] Add internal documentation on plstore.el

* lisp/plstore.el: Add internal documentation and some words of
warning in the user documentation.  (Bug#63627)
---
 lisp/plstore.el | 133 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 131 insertions(+), 2 deletions(-)

diff --git a/lisp/plstore.el b/lisp/plstore.el
index 0276a752a0f..1a0dacffb01 100644
--- a/lisp/plstore.el
+++ b/lisp/plstore.el
@@ -24,6 +24,14 @@
 
 ;; Plist based data store providing search and partial encryption.
 ;;
+;; By default, this package uses symmetric encryption, which means
+;; that you have to enter the password protecting your store more
+;; often than you probably expect to.  To use public key encryption
+;; with this package, create a GnuPG key and customize user option
+;; `plstore-encrypt-to' to use it.  You can then configure the GnuPG
+;; agent to adjust caching and expiration of the passphrase for your
+;; store.
+;;
 ;; Creating:
 ;;
 ;; ;; Open a new store associated with ~/.emacs.d/auth.plist.
@@ -43,12 +51,16 @@
 ;; ;; Kill the buffer visiting ~/.emacs.d/auth.plist.
 ;; (plstore-close store)
 ;;
+;; Avoid marking one property both as public *and* secret, as the
+;; behavior of this package with respect to such duplicate properties
+;; is not (yet) defined.
+;;
 ;; Searching:
 ;;
 ;; (setq store (plstore-open (expand-file-name "~/.emacs.d/auth.plist")))
 ;;
 ;; ;; As the entry "foo" associated with "foo.example.org" has no
-;; ;; secret properties, no need to decryption.
+;; ;; secret properties, no need for decryption.
 ;; (plstore-find store '(:host ("foo.example.org")))
 ;;
 ;; ;; As the entry "bar" associated with "bar.example.org" has a
@@ -73,10 +85,112 @@
 ;;
 ;; where the prefixing `:secret-' means the property (without
 ;; `:secret-' prefix) is marked as secret.  Thus, when you save the
-;; buffer, the `:secret-user' property is encrypted as `:user'.
+;; buffer, the `:secret-user' property is encrypted as `:user'.  Do
+;; not use a property consisting solely of the prefix, as the behavior
+;; of this package with respect to such properties is not (yet)
+;; defined.
 ;;
 ;; You can toggle the view between encrypted form and the decrypted
 ;; form with C-c C-c.
+;;
+;; If you have opened a plstore with `plstore-open' you should not
+;; edit its underlying buffer in `plstore-mode' or in any other way at
+;; the same time, since your manual changes will be overwritten when
+;; `plstore-save' is called on that plstore.
+;;
+;; Internals:
+;;
+;; This is information on the internal data structure and functions of
+;; this package.  None of it should be necessary to actually use it.
+;; For easier reading, we usually do not distinguish in this internal
+;; documentation between a Lisp object and its printed representation.
+;;
+;; A plstore corresponds to an alist mapping strings to property
+;; lists.  Internally, that alist is organized as two alists, one
+;; mapping to the non-secret properties and placeholders for the
+;; secret properties (called "template alist" with identifier ALIST)
+;; and one mapping to the secret properties ("secret alist",
+;; SECRET-ALIST).  The secret alist is read from and written to file
+;; as pgp-encrypted printed representation of the alist ("encrypted
+;; data", ENCRYPTED-DATA).
+;;
+;; During the lifetime of a plstore, a third type of alist may pop up,
+;; which maps to the merged non-secret properties and plain-text
+;; secret properties ("merged alist", MERGED-ALIST).
+;;
+;; After executing the "foo", "bar", "baz" example from above the
+;; alists described above look like the following:
+;;
+;;   Template Alist:
+;;
+;;     (("foo" :host "foo.example.org" :port 80)
+;;      ("bar" :secret-user t :host "bar.example.org")
+;;      ("baz" :secret-password t :host "baz.example.org"))
+;;
+;;   Secret Alist:
+;;
+;;     (("bar" :user "test")
+;;      ("baz" :password "test"))
+;;
+;;   Merged Alist:
+;;
+;;     (("foo" :host "foo.example.org" :port 80)
+;;      ("bar" :user "test" :host "bar.example.org")
+;;      ("baz" :password "test" :host "baz.example.org"))
+;;
+;; Finally, a plstore requires a buffer ("plstore buffer", BUFFER) for
+;; conversion between its Lisp objects and its file representation.
+;; It is important to note that this buffer is *not* continuously
+;; synchronized as the plstore changes.  During the lifetime of a
+;; plstore, its buffer is read from in function `plstore-open' and
+;; (destructively) written to in `plstore-save', but not touched
+;; otherwise.  We call the file visited by the plstore buffer the
+;; associated file of the plstore.
+;;
+;; With the identifiers defined above a plstore is a vector with the
+;; following elements and accessor functions:
+;;
+;;   [
+;;     BUFFER           ; plstore--get/set-buffer
+;;     ALIST            ; plstore--get/set-alist
+;;     ENCRYPTED-DATA   ; plstore--get/set-encrypted-data
+;;     SECRET-ALIST     ; plstore--get/set-secret-alist
+;;     MERGED-ALIST     ; plstore--get/set-merged-alist
+;;   ]
+;;
+;; When a plstore is created through `plstore-open', its ALIST and
+;; ENCRYPTED-DATA are initialized from the contents of BUFFER without
+;; any decryption taking place, and MERGED-ALIST is initialized as a
+;; copy of ALIST.  (Which means that at that stage the merged alist
+;; still contains the secret property placeholders!)
+;;
+;; During on-demand decryption of a plstore through function
+;; `plstore--decrypt', SECRET-ALIST is populated from ENCRYPTED-DATA,
+;; which is in turn replaced by value nil.  (Which further serves as
+;; an indicator that the plstore has been decrypted already.)  In
+;; addition, MERGED-ALIST is recomputed by function
+;; `plstore--merge-secret' to replace the secret property placeholders
+;; by their plain-text secret property equivalents.
+;;
+;; The file representation of a plstore consists of two Lisp forms plus
+;; markers to introduce them:
+;;
+;;   ;;; public entries
+;;   ALIST
+;;   ;;; secret entries
+;;   ENCRYPTED-DATA
+;;
+;; Both of these are optional, but the first section must be present
+;; if the second one is.  If both sections are missing, the plstore is
+;; empty.  If the second section is missing, it contains only
+;; non-secret data.  If present, the printed representation of the
+;; encrypted data includes the delimiting double quotes.
+;;
+;; The plstore API (`plstore-open', `plstore-put', etc.) and the
+;; plstore mode implemented by `plstore-mode' are orthogonal to each
+;; other and should not be mixed up.  In particular, encoding and
+;; decoding a plstore mode buffer with `plstore-mode-toggle-display'
+;; is not related in any way to the state of the plstore buffer.
 
 ;;; Code:
 
@@ -457,6 +571,21 @@ plstore-save
     (plstore--insert-buffer plstore)
     (save-buffer)))
 
+;; The functions related to plstore mode unfortunately introduce yet
+;; another alist format ("decoded alist").  After executing the "foo",
+;; "bar", "baz" example from above the decoded alist of the plstore
+;; would look like the following:
+;;
+;;   (("foo" :host "foo.example.org" :port 80)
+;;    ("bar" :secret-user "test" :host "bar.example.org")
+;;    ("baz" :secret-password "test" :host "baz.example.org"))
+;;
+;; Even more unfortunately, variable and function names of the
+;; following are a bit mixed up IMHO: With the current names, the
+;; result of function `plstore--encode' is used to create what is
+;; presented as "decoded form of a plstore" to the user.  And variable
+;; `plstore-encoded' is non-nil if a buffer shows the decoded form.
+
 (defun plstore--encode (plstore)
   (plstore--decrypt plstore)
   (let ((merged-alist (plstore--get-merged-alist plstore)))
-- 
2.30.2


[-- Attachment #3: 0002-Brush-up-doc-strings-and-terminology-in-plstore.el.patch --]
[-- Type: text/x-patch, Size: 10289 bytes --]

From 4568056f2154794c03109f3a78534fe63c6537cb Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Mon, 22 May 2023 21:47:13 +0200
Subject: [PATCH 2/2] Brush up doc strings and terminology in plstore.el

* lisp/plstore.el (plstore-encoded, plstore-passphrase-callback-function)
(plstore--init-from-buffer, plstore-revert, plstore-close)
(plstore--merge-secret, plstore--decrypt, plstore--match, plstore-find)
(plstore-get, plstore-put, plstore-delete, plstore--insert-buffer)
(plstore-save, plstore--encode, plstore--decode)
(plstore--write-contents-functions, plstore-mode-decoded)
(plstore-mode): Brush up doc strings and documentation in general.
Fix terminology, in particular spurious occurences of all uppercase
"PLSTORE".  (Bug#63627)
---
 lisp/plstore.el | 70 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 18 deletions(-)

diff --git a/lisp/plstore.el b/lisp/plstore.el
index 1a0dacffb01..d18d461d7d1 100644
--- a/lisp/plstore.el
+++ b/lisp/plstore.el
@@ -78,7 +78,7 @@
 ;; Editing:
 ;;
 ;; This file also provides `plstore-mode', a major mode for editing
-;; the PLSTORE format file.  Visit a non-existing file and put the
+;; the plstore format file.  Visit a non-existing file and put the
 ;; following line:
 ;;
 ;; (("foo" :host "foo.example.org" :secret-user "user"))
@@ -235,10 +235,13 @@ plstore-encrypt-to
 
 (put 'plstore-encrypt-to 'permanent-local t)
 
-(defvar plstore-encoded nil)
+(defvar plstore-encoded nil
+  "Non-nil if the current buffer shows the decoded alist.") ; [sic!]
 
 (put 'plstore-encoded 'permanent-local t)
 
+;;; EasyPG callback functions.
+
 (defvar plstore-cache-passphrase-for-symmetric-encryption nil)
 (defvar plstore-passphrase-alist nil)
 
@@ -255,11 +258,11 @@ plstore-passphrase-callback-function
 		      (cons entry
 			    plstore-passphrase-alist)))
 	      (setq passphrase
-		    (read-passwd (format "Passphrase for PLSTORE %s: "
+		    (read-passwd (format "Passphrase for plstore %s: "
 					 (plstore--get-buffer plstore))))
 	      (setcdr entry (copy-sequence passphrase))
 	      passphrase)))
-    (read-passwd (format "Passphrase for PLSTORE %s: "
+    (read-passwd (format "Passphrase for plstore %s: "
 			 (plstore--get-buffer plstore)))))
 
 (defun plstore-progress-callback-function (_context _what _char current total
@@ -269,6 +272,8 @@ plstore-progress-callback-function
     (message "%s...%d%%" handback
 	     (if (> total 0) (floor (* (/ current (float total)) 100)) 0))))
 
+;;; Core functions.
+
 (defun plstore--get-buffer (arg)
   (aref arg 0))
 
@@ -307,6 +312,7 @@ plstore--make
   (vector buffer alist encrypted-data secret-alist merged-alist))
 
 (defun plstore--init-from-buffer (plstore)
+  "Parse current buffer and initialize PLSTORE from it."
   (goto-char (point-min))
   (when (looking-at ";;; public entries")
     (forward-line)
@@ -337,16 +343,20 @@ plstore-open
       store)))
 
 (defun plstore-revert (plstore)
-  "Replace current data in PLSTORE with the file on disk."
+  "Replace current data in PLSTORE from its associated file."
   (with-current-buffer (plstore--get-buffer plstore)
     (revert-buffer t t)
     (plstore--init-from-buffer plstore)))
 
 (defun plstore-close (plstore)
-  "Destroy a plstore instance PLSTORE."
+  "Destroy plstore instance PLSTORE."
   (kill-buffer (plstore--get-buffer plstore)))
 
 (defun plstore--merge-secret (plstore)
+  "Determine the merged alist of PLSTORE.
+Create the merged alist as a copy of the template alist with all
+placeholder properties that have corresponding properties in the
+secret alist replaced by their plain-text secret properties."
   (let ((alist (plstore--get-secret-alist plstore))
 	modified-alist
 	modified-plist
@@ -365,19 +375,26 @@ plstore--merge-secret
 	    modified-entry (assoc (car entry) modified-alist)
 	    modified-plist (cdr modified-entry))
       (while plist
+        ;; Search for a placeholder property in the merged alist
+        ;; corresponding to the current secret property.
 	(setq placeholder
 	      (plist-member
 	       modified-plist
 	       (intern (concat ":secret-"
 			       (substring (symbol-name (car plist)) 1)))))
+        ;; Replace its name with the real, secret property name.
 	(if placeholder
 	    (setcar placeholder (car plist)))
+        ;; Update its value to the plain-text secret property value.
 	(setq modified-plist
 	      (plist-put modified-plist (car plist) (car (cdr plist))))
 	(setq plist (nthcdr 2 plist)))
       (setcdr modified-entry modified-plist))))
 
 (defun plstore--decrypt (plstore)
+  "Decrypt the encrypted data of PLSTORE.
+Update its internal alists and other data structures
+accordingly."
   (if (plstore--get-encrypted-data plstore)
       (let ((context (epg-make-context 'OpenPGP))
 	    plain)
@@ -404,6 +421,11 @@ plstore--decrypt
 	(plstore--set-encrypted-data plstore nil))))
 
 (defun plstore--match (entry keys skip-if-secret-found)
+  "Return whether plist KEYS matches ENTRY.
+ENTRY should be a key of the merged alist of a PLSTORE.  This
+function returns nil if KEYS do not match ENTRY, t if they match,
+and symbol `secret' if the secret alist needs to be consulted to
+perform a match."
   (let ((result t) key-name key-value prop-value secret-name)
     (while keys
       (setq key-name (car keys)
@@ -425,11 +447,10 @@ plstore--match
     result))
 
 (defun plstore-find (plstore keys)
-  "Perform search on PLSTORE with KEYS.
-KEYS is a plist."
+  "Return all PLSTORE entries matching plist KEYS."
   (let (entries alist entry match decrypt plist)
-    ;; First, go through the merged plist alist and collect entries
-    ;; matched with keys.
+    ;; First, go through the merged alist and collect entries matched
+    ;; by the keys.
     (setq alist (plstore--get-merged-alist plstore))
     (while alist
       (setq entry (car alist)
@@ -445,7 +466,7 @@ plstore-find
 		      plist nil))
 	    (setq plist (nthcdr 2 plist)))
 	  (setq entries (cons entry entries)))))
-    ;; Second, decrypt the encrypted plist and try again.
+    ;; Second, decrypt the plstore and try again.
     (when decrypt
       (setq entries nil)
       (plstore--decrypt plstore)
@@ -459,7 +480,8 @@ plstore-find
     (nreverse entries)))
 
 (defun plstore-get (plstore name)
-  "Get an entry with NAME in PLSTORE."
+  "Return the entry named NAME in PLSTORE.
+Return nil if there is none."
   (let ((entry (assoc name (plstore--get-merged-alist plstore)))
 	plist)
     (setq plist (cdr entry))
@@ -473,7 +495,7 @@ plstore-get
     entry))
 
 (defun plstore-put (plstore name keys secret-keys)
-  "Put an entry with NAME in PLSTORE.
+  "Put an entry named NAME in PLSTORE.
 KEYS is a plist containing non-secret data.
 SECRET-KEYS is a plist containing secret data."
   (let (entry
@@ -512,7 +534,7 @@ plstore-put
     (plstore--merge-secret plstore)))
 
 (defun plstore-delete (plstore name)
-  "Delete an entry with NAME from PLSTORE."
+  "Delete the first entry named NAME from PLSTORE."
   (let ((entry (assoc name (plstore--get-alist plstore))))
     (if entry
 	(plstore--set-alist
@@ -531,6 +553,8 @@ plstore-delete
 
 (defvar pp-escape-newlines)
 (defun plstore--insert-buffer (plstore)
+  "Insert the file representation of PLSTORE at point.
+Assumes that PLSTORE has been decrypted."
   (insert ";;; public entries -*- mode: plstore -*- \n"
 	  (pp-to-string (plstore--get-alist plstore)))
   (if (plstore--get-secret-alist plstore)
@@ -565,12 +589,14 @@ plstore--insert-buffer
 	(insert ";;; secret entries\n" (pp-to-string cipher)))))
 
 (defun plstore-save (plstore)
-  "Save the contents of PLSTORE associated with a FILE."
+  "Save PLSTORE to its associated file."
   (with-current-buffer (plstore--get-buffer plstore)
     (erase-buffer)
     (plstore--insert-buffer plstore)
     (save-buffer)))
 
+;;; plstore mode.
+
 ;; The functions related to plstore mode unfortunately introduce yet
 ;; another alist format ("decoded alist").  After executing the "foo",
 ;; "bar", "baz" example from above the decoded alist of the plstore
@@ -587,6 +613,7 @@ plstore-save
 ;; `plstore-encoded' is non-nil if a buffer shows the decoded form.
 
 (defun plstore--encode (plstore)
+  "Return the printed representation of the decoded alist of PLSTORE."
   (plstore--decrypt plstore)
   (let ((merged-alist (plstore--get-merged-alist plstore)))
     (concat "("
@@ -611,6 +638,9 @@ plstore--encode
 	    ")")))
 
 (defun plstore--decode (string)
+  "Create a plstore instance from STRING.
+STRING should be the printed representation of a decoded alist of
+some plstore."
   (let* ((alist (car (read-from-string string)))
 	 (pointer alist)
 	 secret-alist
@@ -618,7 +648,7 @@ plstore--decode
 	 entry)
     (while pointer
       (unless (stringp (car (car pointer)))
-	(error "Invalid PLSTORE format %s" string))
+	(error "Invalid plstore format %s" string))
       (setq plist (cdr (car pointer)))
       (while plist
 	(when (string-match "\\`:secret-" (symbol-name (car plist)))
@@ -638,6 +668,10 @@ plstore--decode
     (plstore--make nil alist nil secret-alist)))
 
 (defun plstore--write-contents-functions ()
+  "Convert the decoded form of a plstore in the current buffer.
+Convert it to the regular file representation of a plstore if
+needed.  This function is used on hook `write-contents-functions'
+in plstore mode buffers."
   (when plstore-encoded
     (let ((store (plstore--decode (buffer-string)))
 	  (file (buffer-file-name)))
@@ -675,7 +709,7 @@ plstore-mode-decoded
       (erase-buffer)
       (insert
        (substitute-command-keys "\
-;;; You are looking at the decoded form of the PLSTORE file.\n\
+;;; You are looking at the decoded form of the plstore file.\n\
 ;;; To see the original form content, do \\[plstore-mode-toggle-display]\n\n"))
       (insert (plstore--encode store))
       (set-buffer-modified-p nil)
@@ -690,7 +724,7 @@ plstore-mode-toggle-display
 
 ;;;###autoload
 (define-derived-mode plstore-mode emacs-lisp-mode "PLSTORE"
-  "Major mode for editing PLSTORE files."
+  "Major mode for editing plstore files."
   (make-local-variable 'plstore-encoded)
   (add-hook 'write-contents-functions #'plstore--write-contents-functions)
   (define-key plstore-mode-map "\C-c\C-c" #'plstore-mode-toggle-display)
-- 
2.30.2