bug#65224: mailcap-viewer-passes-test’s caching interferes with precomputed tests

bug#65224: mailcap-viewer-passes-test’s caching interferes with precomputed tests

[Felix Dietrich]

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

When called with a VIEWER-INFO that does not contain a test,
‘mailcap-viewer-passes-test’ erroneously returns nil if it had
previously been called with a VIEWER-INFO whose test was explicitly set
to nil.  As a consequence ‘mailcap-mime-info’ (used e.g. by
‘gnus-mime-view-part-externally’) may return the wrong viewer.

The reason is that ‘mailcap-viewer-passes-test’ adds tests that are
explicitly nil (i.e. (test . nil)) to the cache as (nil nil) (first the
test, second the result) and checks for the non-existence of a test only
after it has tried to find the test result in the cache.  The test
variable for a VIEWER-INFO without a test is nil, and, therefore, the
assoc lookup for nil done on the cache yields (nil nil) and, hence, nil
(the cadr) as the test result.

Moving the check for the non-existence of a test before the cache lookup
fixes the issue; this is what the attached patch does.  In the log
message the bug number (Bug#) needs to be filled in.


[-- Attachment #2: Patch with fix and test --]
[-- Type: text/x-diff, Size: 2866 bytes --]

From 811ef5f697e165c58c54b3839672c920336da947 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Sun, 6 Aug 2023 06:01:24 +0200
Subject: [PATCH] =?UTF-8?q?Make=20=E2=80=98mailcap-viewer-passes-test?=
 =?UTF-8?q?=E2=80=99=20return=20t=20for=20viewers=20without=20tests?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/net/mailcap.el (mailcap-viewer-passes-test):
Make ‘mailcap-viewer-passes-test’ follow its docstring and return t
for viewers without a test.  (Bug#)
* test/lisp/net/mailcap-tests.el
(mailcap-viewer-passes-test-w/o-test-returns-t): New test.
---
 lisp/net/mailcap.el            |  2 +-
 test/lisp/net/mailcap-tests.el | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 4d01737e3e6..81cc51f1bf0 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -689,9 +689,9 @@ to supply to the test."
 	 status cache result)
     (cond ((not (or (stringp viewer) (fboundp viewer)))
 	   nil)				; Non-existent Lisp function
+	  ((null test-info) t)		; No test clause
 	  ((setq cache (assoc test mailcap-viewer-test-cache))
 	   (cadr cache))
-	  ((not test-info) t)		; No test clause
 	  (t
 	   (setq
 	    result
diff --git a/test/lisp/net/mailcap-tests.el b/test/lisp/net/mailcap-tests.el
index e47ead98f42..fa72d1bd285 100644
--- a/test/lisp/net/mailcap-tests.el
+++ b/test/lisp/net/mailcap-tests.el
@@ -537,5 +537,30 @@ help to verify the correct addition and merging of an entry."
                           ("minor" . ((viewer . "viewer")
                                       (edit . "edit")))))))))
 
+\f
+
+(ert-deftest mailcap-viewer-passes-test-w/o-test-returns-t ()
+  "A VIEWER-INFO without a test should return t with a valid viewer."
+
+  (should (equal t
+                 (let ((mailcap-viewer-test-cache)
+                       (viewer-info
+                        (list (cons 'viewer "viewer-w/o-test"))))
+                   (mailcap-viewer-passes-test viewer-info nil))))
+
+  (should (equal '(t t nil t)
+                 (let ((mailcap-viewer-test-cache)
+                       (viewer-infos
+                        (list
+                         (list (cons 'viewer "viewer-w/o-test"))
+                         (list (cons 'viewer "viewer-w/o-test"))
+                         (list (cons 'viewer "viewer-w/nil-test")
+                               (cons 'test    nil))
+                         (list (cons 'viewer "viewer-w/o-test"))
+                         )))
+                   (mapcar (lambda (vi)
+                             (mailcap-viewer-passes-test vi nil))
+                           viewer-infos)))))
+
 
 ;;; mailcap-tests.el ends here
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 1813 bytes --]



The following example calls ‘mailcap-viewer-passes-test’ four times.
For the first, second and third call the VIEWER-INFO does not contain a
test; for the third the test is nil.  The results show that the first
and second call correctly return t; the third with (test . nil)
correctly returns nil and adds (nil nil) to the cache; the fourth, then,
incorrectly returns nil.

     #+HEADER: :results table
     #+begin_src emacs-lisp
       (let (mailcap-viewer-test-cache
             (viewer-infos (list (list (cons 'viewer "viewer-w/o-test"))
                                 (list (cons 'viewer "viewer-w/o-test"))
                                 (list (cons 'viewer "viewer-w/-test-nil")
                                       (cons 'test nil))
                                 (list (cons 'viewer "viewer-w/o-test")))))
         (cl-list* (list "Index" "Viewer" "Passes Test" "Cache after Call")
                   'hline
                   (cl-loop for i = 1 then (1+ i)
                            for vi in viewer-infos
                            collect (list (format "%i." i)
                                          (alist-get 'viewer vi)
                                          (mailcap-viewer-passes-test vi nil)
                                          mailcap-viewer-test-cache))))
     #+end_src

     #+RESULTS:
     | Index | Viewer             | Passes Test | Cache after Call |
     |-------+--------------------+-------------+------------------|
     |    1. | viewer-w/o-test    | t           | nil              |
     |    2. | viewer-w/o-test    | t           | nil              |
     |    3. | viewer-w/-test-nil | nil         | ((nil nil))      |
     |    4. | viewer-w/o-test    | nil         | ((nil nil))      |


-- 
Felix Dietrich

bug#65224: mailcap-viewer-passes-test’s caching interferes with precomputed tests

[Stefan Kangas]

Version: 30.1

Felix Dietrich <felix.dietrich@sperrhaken.name> writes:

> When called with a VIEWER-INFO that does not contain a test,
> ‘mailcap-viewer-passes-test’ erroneously returns nil if it had
> previously been called with a VIEWER-INFO whose test was explicitly set
> to nil.  As a consequence ‘mailcap-mime-info’ (used e.g. by
> ‘gnus-mime-view-part-externally’) may return the wrong viewer.
>
> The reason is that ‘mailcap-viewer-passes-test’ adds tests that are
> explicitly nil (i.e. (test . nil)) to the cache as (nil nil) (first the
> test, second the result) and checks for the non-existence of a test only
> after it has tried to find the test result in the cache.  The test
> variable for a VIEWER-INFO without a test is nil, and, therefore, the
> assoc lookup for nil done on the cache yields (nil nil) and, hence, nil
> (the cadr) as the test result.
>
> Moving the check for the non-existence of a test before the cache lookup
> fixes the issue; this is what the attached patch does.  In the log
> message the bug number (Bug#) needs to be filled in.

LGTM, pushed to master as commit 6d8458571f7.