From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Felix Dietrich Newsgroups: gmane.emacs.devel Subject: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding Date: Mon, 07 Mar 2022 17:35:23 +0100 Message-ID: <87tuc9pvp0.fsf@sperrhaken.name> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21419"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) To: emacs-devel Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Mar 08 04:21:00 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nRQPC-0005N1-Rr for ged-emacs-devel@m.gmane-mx.org; Tue, 08 Mar 2022 04:20:58 +0100 Original-Received: from localhost ([::1]:47824 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nRQPB-000535-OL for ged-emacs-devel@m.gmane-mx.org; Mon, 07 Mar 2022 22:20:57 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:58982) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nRGKh-0007Dh-TV for emacs-devel@gnu.org; Mon, 07 Mar 2022 11:35:39 -0500 Original-Received: from mout.kundenserver.de ([212.227.17.24]:35695) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nRGKf-0005s5-GM for emacs-devel@gnu.org; Mon, 07 Mar 2022 11:35:39 -0500 Original-Received: from localhost ([78.54.129.84]) by mrelayeu.kundenserver.de (mreue109 [212.227.15.183]) with ESMTPSA (Nemesis) id 1MbRXj-1o35Bo0jij-00boyE for ; Mon, 07 Mar 2022 17:35:29 +0100 X-Provags-ID: V03:K1:Re/8TOKCmi50sxUWnK1wSjmlftF52oC9ONnQ9vTfO7GzsYy0FBK SWky0NRMmxx0cEVfvXXzyXgVrZMdcfHZZWMWRHoE7+OWc5gaRgRaCq97l8ONo8Qt12nYLB3 30paksrgZDhAzSTuCP844MPxndOH//p5PQf2OvlAPjvWUjxUMEAeLTPJzOKy9/i/nQbDD0h CROg7NhVlbjzlO1LtAK6w== X-UI-Out-Filterresults: notjunk:1;V03:K0:m2pPDE7r+YA=:jj6Cd/7y6QQ+25Y0dwbwZ6 ccI4bR00+cSgilnO1UlSGAYOlxHgV1EUANtUa22Il2zbPidqqfD/ond6aJ4RqHcZNA+xbSJWP rjDm/eoe40hRl4z+hzthg3F+6SGHe9gAZ6ISM73WDG3hNHcxqhfxYqZkevDliU2SdNxjFuv3f O3nDw0W+YBx++0lDP1+h1VQLIgDRijirJvY9DmGA/CqadhqF/VGW6E51+087qfxqhlKCaHtcH nGVdVOcw2p3RpLez7G9f7WdkXXUVHihjmBO1UpCumoe49ODOInBYkeE25N2+VMv5PnAAbDkwW pAzkTrM1flqBXFm59m8AEy9GQUUr7TF3zNtLmasdtv0l5ZLVxof4jW+kj/JwHKAZTYitKMcxg lVW/TS534PXPmUXPDeRrzp2+HMvn3zAW9D472rKYAdXEksqccFYBzKTp1z7efdZmdWkjuV4Qj i7OExSCce7BCjfz3qjMOzCkTyfRJOThj0T6Oaw+8IKZtxvVednkyihcuN4eVqFewPdZY5FS+P k8RZL9hF+eovj1R+XatCNvt5IZ2ReKlaLxu+yGOm5AANFRwrpWbatSo0FF5w0PFzzqd1Cez8D rlanM/uqrJNEeG0c9qHbmYD71gHJaC29WbkBF+MBQR1+Shwsois+X8ZHAiIhjPoGk2CNRGG1I QeRxIZwilviOSz6fpi515j0PQ2kjeFWvekLETwE5gIlHUQycwUrIQPTH2Avw2igIYacmKhXzc /AvvhWxD7Q5aXEe4 Received-SPF: none client-ip=212.227.17.24; envelope-from=felix.dietrich@sperrhaken.name; helo=mout.kundenserver.de X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Mon, 07 Mar 2022 22:20:02 -0500 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:286911 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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=CA=BCs 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. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-mailcap-Remove-superfluous-setcdr.patch Content-Description: Patch to remove superfluous setcdr >From 318751a0794ba510c6f2ef7263c363b51ceca6b6 Mon Sep 17 00:00:00 2001 From: Felix Dietrich 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 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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=CA=BCs 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. --=-=-= Content-Type: text/x-diff; charset=utf-8 Content-Disposition: inline; filename=0001-Restructure-mailcap-add-mailcap-entry-to-improve-rea.patch Content-Transfer-Encoding: quoted-printable Content-Description: Patch to restructure mailcap-add-mailcap-entry >From b176e9ede469e7187417e63cf8ae56c0372a12cc Mon Sep 17 00:00:00 2001 From: Felix Dietrich Date: Sun, 6 Mar 2022 20:41:41 +0100 Subject: [PATCH] Restructure mailcap-add-mailcap-entry to improve readabili= ty --- 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)))) =20 (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 =E2=80=98mailcap-mime-data=E2=80=99. + +STORAGE should be a symbol refering to a variable. The value of +this variable should have the same format as =E2=80=98mailcap-mime-data=E2= =80=99. +STORAGE defaults to =E2=80=98mailcap--computed-mime-data=E2=80=99. + +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 =E2=80=98test=E2=80=99 associated in t= heir info + ;; alist and both use the same =E2=80=98viewer=E2=80=99 command. Th= is ignores + ;; other fields in the previous entry=CA=BCs 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)))))) =20 (defun mailcap-add (type viewer &optional test) "Add VIEWER as a handler for TYPE. --=20 2.35.1 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 =E2=80=98viewer=E2=80=99 and =E2=80=98test=E2=80=99 field= s 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 =E2=80=98print=E2=80=99 in gnus-mime-print-part, I believe this behaviour t= o 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 =E2=80=98print=E2=80=99 field: they are not actually identical. The = earlier entry gets overwritten; the extra =E2=80=98print=E2=80=99 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 =E2=80=98test=E2= =80=99 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 --=-=-= Content-Type: text/x-diff; charset=utf-8 Content-Disposition: inline; filename=0001-Simplify-mailcap-add-mailcap-entry.patch Content-Transfer-Encoding: quoted-printable Content-Description: Patch to not override a previous minor entry >From dc6c4697176135191ca805cab0b0fbb9491b174c Mon Sep 17 00:00:00 2001 From: Felix Dietrich 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)))) =20 (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 =E2=80=98mailcap-mime-data=E2=80=99. + +STORAGE should be a symbol refering to a variable. The value of +this variable should have the same format as =E2=80=98mailcap-mime-data=E2= =80=99. +STORAGE defaults to =E2=80=98mailcap--computed-mime-data=E2=80=99. + +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))))) =20 (defun mailcap-add (type viewer &optional test) "Add VIEWER as a handler for TYPE. --=20 2.35.1 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 =E2=80= =93 but this remains to be done. --=20 Felix Dietrich --=-=-=--