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

[GNU bug Tracking System]

Your message dated Fri, 8 Sep 2023 10:20:52 -0700
with message-id 
<CADwFkmmou96RJSU9t9K4KW_A81uWiZvYjAPZZy8WBBzKsESSow@mail.gmail.com>
and subject line Re: bug#65224: mailcap-viewer-passes-test’s caching interferes 
with precomputed tests
has caused the debbugs.gnu.org bug report #65224,
regarding mailcap-viewer-passes-test’s caching interferes with precomputed tests
to be marked as done.

(If you believe you have received this mail in error, please contact
help-debbugs@gnu.org.)


-- 
65224: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65224
GNU Bug Tracking System
Contact help-debbugs@gnu.org with problems

--- Begin Message --- Subject: mailcap-viewer-passes-test’s caching interferes with precomputed tests Date: Fri, 11 Aug 2023 12:48:49 +0200 User-agent: Gnus/5.13 (Gnus v5.13)

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.

0001-Make-mailcap-viewer-passes-test-return-t-for-viewers.patch
Description: Patch with fix and test

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

--- End Message ---

> --- Begin Message ---
>
>
>
> Subject: 
>
> Re: bug#65224: mailcap-viewer-passes-test’s caching interferes with precomputed tests
>
>
>
>
> Date: 
>
> Fri, 8 Sep 2023 10:20:52 -0700
>
>
>
>
> 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.
>
>
> --- End Message ---