[PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding

[Felix Dietrich]

Hello,

I found some oversights in mailcap-add-mailcap-entry for which I am
proposing three different solutions in the form of attached patches.
The later patches actually supersede the earlier ones, but I am leaving
them in for the case that my understanding of the functionʼs operation
is wrong.  I have to admit, though, that the mail has gotten somewhat
long for such a relatively minor matter, and I am unsure about the
format and formatting, so let me know if there are any issues with it.

mailcap-add-mailcap-entry calls at its end setcdr twice, one call nested
into the other:

  #+begin_src emacs-lisp
  (setcdr old-major
          (setcdr old-major
                  (cons (cons minor info) (cdr old-major))))
  #+end_src

Given that setcdr returns its second argument, i.e. the NEWCDR, I
believe that setcdr here is essentially called twice with the same
arguments.  Therefore, one of the setcdr calls is superfluous and should
be removed.

>From 318751a0794ba510c6f2ef7263c363b51ceca6b6 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Sun, 6 Mar 2022 20:26:51 +0100
Subject: [PATCH] mailcap: Remove superfluous setcdr

---
 lisp/net/mailcap.el | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index b65f7c25b8..8a0860ddb4 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -736,9 +736,7 @@ to supply to the test."
                      (assq 'viewer cur-minor)))
          (setcdr cur-minor info))
         (t
-         (setcdr old-major
-                  (setcdr old-major
-                          (cons (cons minor info) (cdr old-major))))))))))
+         (setcdr old-major (cons (cons minor info) (cdr old-major)))))))))
 
 (defun mailcap-add (type viewer &optional test)
   "Add VIEWER as a handler for TYPE.
-- 
2.35.1


There are, though, two more related issues I noticed with this function:
the first concerns the readability; the second may be a bug in how a
minor entry under certain conditions overrides a previous one.

The first, relating to readability: I found it quit difficult to follow
the flow of the functionʼs code due to its nesting and idiosyncratic
order of condition checking.  I want to, therefore, propose a
restructured version, which I believe to be functionally equivalent.

>From b176e9ede469e7187417e63cf8ae56c0372a12cc Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Sun, 6 Mar 2022 20:41:41 +0100
Subject: [PATCH] Restructure mailcap-add-mailcap-entry to improve readability

---
 lisp/net/mailcap.el | 56 +++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index b65f7c25b8..efcf9d7134 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -718,27 +718,43 @@ to supply to the test."
           result))))
 
 (defun mailcap-add-mailcap-entry (major minor info &optional storage)
+  "Add handler INFO for mime type MAJOR/MINOR to STORAGE.
+
+MAJOR and MINOR should be strings.  MINOR is treated as a regexp
+in later lookups, and, therefore, you may need to escape it
+appropriately.
+
+The format of INFO is described in ‘mailcap-mime-data’.
+
+STORAGE should be a symbol refering to a variable.  The value of
+this variable should have the same format as ‘mailcap-mime-data’.
+STORAGE defaults to ‘mailcap--computed-mime-data’.
+
+None of this is enforced."
   (let* ((storage (or storage 'mailcap--computed-mime-data))
-         (old-major (assoc major (symbol-value storage))))
-    (if (null old-major)               ; New major area
-        (set storage
-             (cons (cons major (list (cons minor info)))
-                   (symbol-value storage)))
-      (let ((cur-minor (assoc minor old-major)))
-       (cond
-        ((or (null cur-minor)          ; New minor area, or
-             (assq 'test info))        ; Has a test, insert at beginning
-         (setcdr old-major
-                  (cons (cons minor info) (cdr old-major))))
-        ((and (not (assq 'test info))  ; No test info, replace completely
-              (not (assq 'test cur-minor))
-              (equal (assq 'viewer info)  ; Keep alternative viewer
-                     (assq 'viewer cur-minor)))
-         (setcdr cur-minor info))
-        (t
-         (setcdr old-major
-                  (setcdr old-major
-                          (cons (cons minor info) (cdr old-major))))))))))
+        (major-entry (assoc major (symbol-value storage)))
+        (new-minor-entry (cons minor info))
+        minor-entry)
+    (cond
+     ((null major-entry)
+      ;; Add a new major entry containing the new minor entry.
+      (setf major-entry (list major new-minor-entry))
+      (push major-entry (symbol-value storage)))
+     ((and (setf minor-entry (assoc minor major-entry))
+          (not (assq 'test info))
+          (not (assq 'test minor-entry))
+          (equal (assq 'viewer info)
+                 (assq 'viewer minor-entry)))
+      ;; Replace a previous MINOR entry if it and the entry to be
+      ;; added both do *not* have a ‘test’ associated in their info
+      ;; alist and both use the same ‘viewer’ command.  This ignores
+      ;; other fields in the previous entryʼs info alist: they will be
+      ;; lost when the info alist in the cdr of the previous entry is
+      ;; replaced with the new INFO alist.
+      (setf (cdr minor-entry) info))
+     (t
+      ;; Add the new minor entry to the existing major entry.
+      (push new-minor-entry (cdr major-entry))))))
 
 (defun mailcap-add (type viewer &optional test)
   "Add VIEWER as a handler for TYPE.
-- 
2.35.1


The second, concerning a potential bug: I believe that the overriding of
a previous minor-type entry in certain cases is not done correctly: it
only looks at the ‘viewer’ and ‘test’ fields in the INFO alist when
deciding whether to override an entry, which means that other fields of
a previous entry will be dropped and lost.  While these other fields are
not used much within Emacs, on a cursory grep, I only found a use of
‘print’ in gnus-mime-print-part, I believe this behaviour to still be
unexpected and wrong.  Let me illustrate the issue with some examples.
The 1. as well as 2.1 illustrate basic behaviour.  2.2 shows the
erroneous behaviour.  The rest serve to demonstrate that this feature is
not easily corrected and correctly implemented, and that it will be
easiest to just drop it, which the last proposed patch at the end does.


1. Two differing viewers are added, one after the other.  The result is
as expected: both can be found in the resulting mailcap-mime-data structure:

  #+begin_src emacs-lisp :results value pp :wrap
    (let (mailcap--computed-mime-data)
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")))
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "bar")))
      mailcap--computed-mime-data)
  #+end_src

  #+RESULTS:
  #+begin_results
  (("major"
    ("minor"
     (viewer . "bar"))
    ("minor"
     (viewer . "foo"))))
  #+end_results


2.1 Trying to add the same viewer twice in a row; it is only added once.
This result seems reasonable: there is no need to add the same viewer
twice:

  #+begin_src emacs-lisp :results value pp :wrap
    (let (mailcap--computed-mime-data)
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")))
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")))
      mailcap--computed-mime-data)
  #+end_src

  #+RESULTS:
  #+begin_results
  (("major"
    ("minor"
     (viewer . "foo"))))
  #+end_results


2.2 The same viewer is added twice, but the earlier entry contains an
extra ‘print’ field: they are not actually identical.  The earlier entry
gets overwritten; the extra ‘print’ field is lost:

  #+begin_src emacs-lisp :results value pp :wrap
        (let (mailcap--computed-mime-data)
          (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")
                                                                                
                   (print . "fooprint")))
          (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")))
          mailcap--computed-mime-data)
  #+end_src

  #+RESULTS:
  #+begin_results
  (("major"
        ("minor"
         (viewer . "foo"))))
  #+end_results


2.3 The same viewer is added twice, but with a precomputed ‘test’ with
the value t, which is, in essence, equivalent to not having a test.
Here the duplicate is not discarded:

  #+begin_src emacs-lisp :results value pp :wrap
    (let (mailcap--computed-mime-data)
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")
                                                   (test . t)))
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")
                                                   (test . t)))
      mailcap--computed-mime-data)
  #+end_src

  #+RESULTS:
  #+begin_results
  (("major"
        ("minor"
         (viewer . "foo")
         (test . t))
        ("minor"
         (viewer . "foo")
         (test . t))))
  #+end_results


2.4 The same viewer is added twice, but not in a row, another viewer was
added in-between.  Here the duplicate is also not discarded:

  #+begin_src emacs-lisp :results value pp :wrap
    (let (mailcap--computed-mime-data)
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")))
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "bar")))
      (mailcap-add-mailcap-entry "major" "minor" '((viewer . "foo")))
      mailcap--computed-mime-data)
  #+end_src

  #+RESULTS:
  #+begin_results
  (("major"
    ("minor"
     (viewer . "foo"))
    ("minor"
     (viewer . "bar"))
    ("minor"
     (viewer . "foo"))))
  #+end_results


>From dc6c4697176135191ca805cab0b0fbb9491b174c Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Sun, 6 Mar 2022 20:47:29 +0100
Subject: [PATCH] Simplify mailcap-add-mailcap-entry

---
 lisp/net/mailcap.el | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index b65f7c25b8..d49874c45c 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -718,27 +718,28 @@ to supply to the test."
           result))))
 
 (defun mailcap-add-mailcap-entry (major minor info &optional storage)
+  "Add handler INFO for mime type MAJOR/MINOR to STORAGE.
+
+MAJOR and MINOR should be strings.  MINOR is treated as a regexp
+in later lookups, and, therefore, you may need to escape it
+appropriately.
+
+The format of INFO is described in ‘mailcap-mime-data’.
+
+STORAGE should be a symbol refering to a variable.  The value of
+this variable should have the same format as ‘mailcap-mime-data’.
+STORAGE defaults to ‘mailcap--computed-mime-data’.
+
+None of this is enforced."
   (let* ((storage (or storage 'mailcap--computed-mime-data))
-         (old-major (assoc major (symbol-value storage))))
-    (if (null old-major)               ; New major area
-        (set storage
-             (cons (cons major (list (cons minor info)))
-                   (symbol-value storage)))
-      (let ((cur-minor (assoc minor old-major)))
-       (cond
-        ((or (null cur-minor)          ; New minor area, or
-             (assq 'test info))        ; Has a test, insert at beginning
-         (setcdr old-major
-                  (cons (cons minor info) (cdr old-major))))
-        ((and (not (assq 'test info))  ; No test info, replace completely
-              (not (assq 'test cur-minor))
-              (equal (assq 'viewer info)  ; Keep alternative viewer
-                     (assq 'viewer cur-minor)))
-         (setcdr cur-minor info))
-        (t
-         (setcdr old-major
-                  (setcdr old-major
-                          (cons (cons minor info) (cdr old-major))))))))))
+        (major-entry (assoc major (symbol-value storage)))
+        (new-minor-entry (cons minor info)))
+    (if major-entry
+        ;; Add the new minor entry to the existing major entry.
+       (push new-minor-entry (cdr major-entry))
+      ;; Add a new major entry containing the new minor entry.
+      (setq major-entry (list major new-minor-entry))
+      (push major-entry (symbol-value storage)))))
 
 (defun mailcap-add (type viewer &optional test)
   "Add VIEWER as a handler for TYPE.
-- 
2.35.1


Additionally, one should probably add some sanity checks to ensure that
a valid and correct mailcap-mime-data structure does not end up in an
invalid and broken state after a call to mailcap-add-mailcap-entry – but
this remains to be done.

-- 
Felix Dietrich