* [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
@ 2022-03-07 16:35 Felix Dietrich
2022-03-09 14:03 ` Lars Ingebrigtsen
0 siblings, 1 reply; 8+ messages in thread
From: Felix Dietrich @ 2022-03-07 16:35 UTC (permalink / raw)
To: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 978 bytes --]
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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch to remove superfluous setcdr --]
[-- Type: text/x-diff, Size: 862 bytes --]
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
[-- Attachment #3: Type: text/plain, Size: 493 bytes --]
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.
[-- Attachment #4: Patch to restructure mailcap-add-mailcap-entry --]
[-- Type: text/x-diff, Size: 3303 bytes --]
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
[-- Attachment #5: Type: text/plain, Size: 4049 bytes --]
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
[-- Attachment #6: Patch to not override a previous minor entry --]
[-- Type: text/x-diff, Size: 2585 bytes --]
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
[-- Attachment #7: Type: text/plain, Size: 273 bytes --]
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-07 16:35 [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding Felix Dietrich
@ 2022-03-09 14:03 ` Lars Ingebrigtsen
2022-03-10 5:45 ` Felix Dietrich
2022-03-19 14:12 ` Felix Dietrich
0 siblings, 2 replies; 8+ messages in thread
From: Lars Ingebrigtsen @ 2022-03-09 14:03 UTC (permalink / raw)
To: Felix Dietrich; +Cc: emacs-devel
Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
> 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.
Yup.
> 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.
Yes, the code is pretty confusing... so reading the patches, I'm not
quite sure whether the new one is equivalent or not. So I think we'd
want to have a number of tests in mailcap-tests.el to test that the
results really are equivalent before and after the change, too.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-09 14:03 ` Lars Ingebrigtsen
@ 2022-03-10 5:45 ` Felix Dietrich
2022-03-12 17:26 ` Lars Ingebrigtsen
2022-03-19 14:12 ` Felix Dietrich
1 sibling, 1 reply; 8+ messages in thread
From: Felix Dietrich @ 2022-03-10 5:45 UTC (permalink / raw)
To: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]
Lars Ingebrigtsen <larsi@gnus.org> writes:
> Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
>
>> 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.
>
> Yes, the code is pretty confusing... so reading the patches, I'm not
> quite sure whether the new one is equivalent or not. So I think we'd
> want to have a number of tests in mailcap-tests.el to test that the
> results really are equivalent before and after the change, too.
I agree that adding some automated tests is a good idea, but, since I am
not familiar with writing these in Emacs Lisp, I will first have to
spent a bit of time with the ERT manual and the existing tests, before I
cans start writing the tests. Meanwhile, to shorten the wait ;), here
is a series of patches that go through the transformation steps of
‘mailcap-add-mailcap-entry’ and demonstrate that it is still the same
function (unless I misstepped). (I donʼt know if this will end up all
that readable and useful; it seemed like a good idea when I started and
might end up as time better spent learning ERT.)
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Merge-the-inner-let-with-the-outer.patch --]
[-- Type: text/x-diff, Size: 2555 bytes --]
From 938e2bb38ea43a2cb87c8442c7c3b78a2e587f78 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 03:29:47 +0100
Subject: [PATCH 01/10] =?UTF-8?q?Merge=20the=20inner=20=E2=80=98let?=
=?UTF-8?q?=E2=80=99=20with=20the=20outer?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Note that it does not matter should there be no old-major:
(assoc minor nil) is valid and will simply yield nil.
---
lisp/net/mailcap.el | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index b65f7c25b8..633d43320e 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -719,26 +719,26 @@ to supply to the test."
(defun mailcap-add-mailcap-entry (major minor info &optional storage)
(let* ((storage (or storage 'mailcap--computed-mime-data))
- (old-major (assoc major (symbol-value storage))))
+ (old-major (assoc major (symbol-value storage)))
+ (cur-minor (assoc minor old-major)))
(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))))))))))
+ (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)))))))))
(defun mailcap-add (type viewer &optional test)
"Add VIEWER as a handler for TYPE.
--
2.35.1
[-- Attachment #3: 0002-Move-the-if-form-into-the-cond.patch --]
[-- Type: text/x-diff, Size: 2659 bytes --]
From f9a68bd1696d0cbf9bb07d9178045640cfa02dfc Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 03:30:59 +0100
Subject: [PATCH 02/10] =?UTF-8?q?Move=20the=20=E2=80=98if=E2=80=99=20form?=
=?UTF-8?q?=20into=20the=20=E2=80=98cond=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Its condition and then-form become the first alternative of the
‘cond’. The rest of the ‘cond’ remains, in a way, the previous
else-form of the ‘if’.
---
lisp/net/mailcap.el | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 633d43320e..c30b9e096d 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -721,24 +721,24 @@ to supply to the test."
(let* ((storage (or storage 'mailcap--computed-mime-data))
(old-major (assoc major (symbol-value storage)))
(cur-minor (assoc minor old-major)))
- (if (null old-major) ; New major area
- (set storage
- (cons (cons major (list (cons minor info)))
- (symbol-value storage)))
- (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)))))))))
+ (cond
+ ((null old-major) ; New major area
+ (set storage
+ (cons (cons major (list (cons minor info)))
+ (symbol-value storage))))
+ ((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))))))))
(defun mailcap-add (type viewer &optional test)
"Add VIEWER as a handler for TYPE.
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Remove-the-superfluous-setcdr-call.patch --]
[-- Type: text/x-diff, Size: 1068 bytes --]
From aa5f2640223db603ae158d2177d1bfc73aa0f2f9 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 03:36:17 +0100
Subject: [PATCH 03/10] =?UTF-8?q?Remove=20the=20superfluous=20=E2=80=98set?=
=?UTF-8?q?cdr=E2=80=99=20call?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Note the duplication of code in the second and last cond alternatives:
both setcdr calls are identical.
---
lisp/net/mailcap.el | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index c30b9e096d..bb34fb86c0 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -737,8 +737,7 @@ to supply to the test."
(setcdr cur-minor info))
(t
(setcdr old-major
- (setcdr old-major
- (cons (cons minor info) (cdr 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
[-- Attachment #5: 0004-Add-the-negated-condition-of-the-second-to-the-third.patch --]
[-- Type: text/x-diff, Size: 1295 bytes --]
From 4ddb4dbaced88a36ef2a0735c1b911cb60f4f597 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 03:45:09 +0100
Subject: [PATCH 04/10] Add the negated condition of the second to the third
alternative
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The negation of the absence of a cur-minor entry (null cur-minor) is
simply cur-minor. The negation of (assq ’test info) is already
there: (not (assq ’test cur-minor)).
Also: ¬(a ∨ b) = ¬a ∧ ¬b
---
lisp/net/mailcap.el | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index bb34fb86c0..48701ddee8 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -730,7 +730,8 @@ to supply to the test."
(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
+ ((and cur-minor
+ (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)))
--
2.35.1
[-- Attachment #6: 0005-Remove-the-second-condition.patch --]
[-- Type: text/x-diff, Size: 1538 bytes --]
From 2cfb0760c28e2afbb59ad01996b611d28cff47f9 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 03:51:08 +0100
Subject: [PATCH 05/10] Remove the second condition
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
With the conjunction of the negation of the second alternativeʼs
condition with the third alternativeʼs condition in the previous
patch, the second alternative has become redundant, because the
condition of the third alternative now ensure that it will not run in
cases that were previously handled by the second alternative. Since
the second alternative had the same body-form as the last, that is the
default alternative, it can be correctly handled by the default
alternative as well.
---
lisp/net/mailcap.el | 4 ----
1 file changed, 4 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 48701ddee8..165945040d 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -726,10 +726,6 @@ to supply to the test."
(set storage
(cons (cons major (list (cons minor info)))
(symbol-value storage))))
- ((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 cur-minor
(not (assq 'test info)) ; No test info, replace completely
(not (assq 'test cur-minor))
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: 0006-Remove-unnecessary-cons-move-list-one-element-to-the.patch --]
[-- Type: text/x-diff, Size: 1112 bytes --]
From eb3c3d46e06647976bfebe43dfb18f729cda0d26 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 04:02:58 +0100
Subject: [PATCH 06/10] =?UTF-8?q?Remove=20unnecessary=20=E2=80=98cons?=
=?UTF-8?q?=E2=80=99=20(move=20=E2=80=98list=E2=80=99=20one=20element=20to?=
=?UTF-8?q?=20the=20left)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
(cons something (list something-else)) is the same as
(list something something-else).
---
lisp/net/mailcap.el | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 165945040d..2d93720a56 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -724,7 +724,7 @@ to supply to the test."
(cond
((null old-major) ; New major area
(set storage
- (cons (cons major (list (cons minor info)))
+ (cons (list major (cons minor info))
(symbol-value storage))))
((and cur-minor
(not (assq 'test info)) ; No test info, replace completely
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #8: 0007-Add-extra-variables-to-make-things-clearer.patch --]
[-- Type: text/x-diff, Size: 1593 bytes --]
From befca96cd7ea332c9bec4136d23dfbc58d0ba805 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 04:20:27 +0100
Subject: [PATCH 07/10] Add extra variables to make things clearer
---
lisp/net/mailcap.el | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 2d93720a56..e7575e8097 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -720,12 +720,14 @@ to supply to the test."
(defun mailcap-add-mailcap-entry (major minor info &optional storage)
(let* ((storage (or storage 'mailcap--computed-mime-data))
(old-major (assoc major (symbol-value storage)))
- (cur-minor (assoc minor old-major)))
+ (cur-minor (assoc minor old-major))
+ (new-minor-entry (cons minor info))
+ new-major)
(cond
((null old-major) ; New major area
+ (setq new-major (list major new-minor-entry))
(set storage
- (cons (list major (cons minor info))
- (symbol-value storage))))
+ (cons new-major (symbol-value storage))))
((and cur-minor
(not (assq 'test info)) ; No test info, replace completely
(not (assq 'test cur-minor))
@@ -734,7 +736,7 @@ to supply to the test."
(setcdr cur-minor info))
(t
(setcdr old-major
- (cons (cons minor info) (cdr old-major)))))))
+ (cons new-minor-entry (cdr old-major)))))))
(defun mailcap-add (type viewer &optional test)
"Add VIEWER as a handler for TYPE.
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #9: 0008-Use-macros-with-Generalized-Variables.patch --]
[-- Type: text/x-diff, Size: 1672 bytes --]
From 293a324649e23a93cfaf6f277fd835340ebdefa9 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 04:36:52 +0100
Subject: [PATCH 08/10] =?UTF-8?q?Use=20macros=20with=20=E2=80=9CGeneralize?=
=?UTF-8?q?d=20Variables=E2=80=9D?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
For the last one (push new-minor-entry (cdr old-major)) it helped me
to remember that old-major has the form:
("major" ("minor" . info) ("minor" . info))
So, to push a new entry to the front of the list of minor entries, you
push it after "major", that is at the cdr.
---
lisp/net/mailcap.el | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index e7575e8097..72d292c3e4 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -726,17 +726,15 @@ to supply to the test."
(cond
((null old-major) ; New major area
(setq new-major (list major new-minor-entry))
- (set storage
- (cons new-major (symbol-value storage))))
+ (push new-major (symbol-value storage)))
((and cur-minor
(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))
+ (setf (cdr cur-minor) info))
(t
- (setcdr old-major
- (cons new-minor-entry (cdr old-major)))))))
+ (push new-minor-entry (cdr old-major))))))
(defun mailcap-add (type viewer &optional test)
"Add VIEWER as a handler for TYPE.
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #10: 0009-Clean-up-rename-variables-remove-comments.patch --]
[-- Type: text/x-diff, Size: 1923 bytes --]
From abc8b4410adf05d185c5f9293150d25980567181 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 04:43:45 +0100
Subject: [PATCH 09/10] Clean-up, rename variables, remove comments
---
lisp/net/mailcap.el | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 72d292c3e4..5d60e7ece4 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -719,22 +719,21 @@ to supply to the test."
(defun mailcap-add-mailcap-entry (major minor info &optional storage)
(let* ((storage (or storage 'mailcap--computed-mime-data))
- (old-major (assoc major (symbol-value storage)))
- (cur-minor (assoc minor old-major))
- (new-minor-entry (cons minor info))
- new-major)
+ (major-entry (assoc major (symbol-value storage)))
+ (new-minor-entry (cons minor info))
+ minor-entry)
(cond
- ((null old-major) ; New major area
- (setq new-major (list major new-minor-entry))
- (push new-major (symbol-value storage)))
- ((and cur-minor
- (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)))
- (setf (cdr cur-minor) info))
+ ((null major-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)))
+ (setf (cdr minor-entry) info))
(t
- (push new-minor-entry (cdr old-major))))))
+ (push new-minor-entry (cdr major-entry))))))
(defun mailcap-add (type viewer &optional test)
"Add VIEWER as a handler for TYPE.
--
2.35.1
[-- Attachment #11: 0010-Add-docstring-and-comments-this-is-the-version-I-sen.patch --]
[-- Type: text/x-diff, Size: 2367 bytes --]
From 2e2d02c5ff7bfe939a96a40fcf8fac7e6e255315 Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Thu, 10 Mar 2022 04:44:30 +0100
Subject: [PATCH 10/10] Add docstring and comments (this is the version I sent)
---
lisp/net/mailcap.el | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 5d60e7ece4..efcf9d7134 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -718,12 +718,26 @@ 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))
(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))
@@ -731,8 +745,15 @@ to supply to the test."
(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)
--
2.35.1
[-- Attachment #12: Type: text/plain, Size: 21 bytes --]
--
Felix Dietrich
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-10 5:45 ` Felix Dietrich
@ 2022-03-12 17:26 ` Lars Ingebrigtsen
0 siblings, 0 replies; 8+ messages in thread
From: Lars Ingebrigtsen @ 2022-03-12 17:26 UTC (permalink / raw)
To: Felix Dietrich; +Cc: emacs-devel
Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
> I agree that adding some automated tests is a good idea, but, since I am
> not familiar with writing these in Emacs Lisp, I will first have to
> spent a bit of time with the ERT manual and the existing tests, before I
> cans start writing the tests.
I don't think you need to read the ERT manual, really -- just have a
look at any file in test/lisp/*-tests.el and it should be pretty much
self-evident.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-09 14:03 ` Lars Ingebrigtsen
2022-03-10 5:45 ` Felix Dietrich
@ 2022-03-19 14:12 ` Felix Dietrich
2022-03-20 15:21 ` Lars Ingebrigtsen
1 sibling, 1 reply; 8+ messages in thread
From: Felix Dietrich @ 2022-03-19 14:12 UTC (permalink / raw)
To: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 775 bytes --]
Lars Ingebrigtsen <larsi@gnus.org> writes:
> Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
>
>> 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.
>
> Yes, the code is pretty confusing... so reading the patches, I'm not
> quite sure whether the new one is equivalent or not. So I think we'd
> want to have a number of tests in mailcap-tests.el to test that the
> results really are equivalent before and after the change, too.
I have written a couple of test cases. Let me know if this is what you
had in mind.
[-- Attachment #2: Patch that adds tests for mailcap-add-mailcap-entry --]
[-- Type: text/x-diff, Size: 19620 bytes --]
From da64940d6a980e5a8702b19a6796412717a098af Mon Sep 17 00:00:00 2001
From: Felix Dietrich <felix.dietrich@sperrhaken.name>
Date: Mon, 14 Mar 2022 14:01:26 +0100
Subject: [PATCH] Add tests for mailcap-add-mailcap-entry
---
test/lisp/net/mailcap-tests.el | 405 +++++++++++++++++++++++++++++++++
1 file changed, 405 insertions(+)
diff --git a/test/lisp/net/mailcap-tests.el b/test/lisp/net/mailcap-tests.el
index b439c08c79..be32c320e5 100644
--- a/test/lisp/net/mailcap-tests.el
+++ b/test/lisp/net/mailcap-tests.el
@@ -133,4 +133,409 @@
(mailcap-view-file (ert-resource-file "test.test")))
(should mailcap--test-result))))
+\f
+
+(ert-deftest mailcap-add-mailcap-entry-new-major ()
+ "Add a major entry not yet in ‘mailcap-mime-data’."
+ (let ((mailcap-mime-data))
+
+ ;; Add a new major entry to a empty ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major1" "minor1"
+ (list (cons 'viewer "viewer1"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major1"
+ ("minor1" . ((viewer . "viewer1")))))))
+
+ ;; Add a new major entry to a non-empty ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major2" "minor2"
+ (list (cons 'viewer "viewer2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major2"
+ ("minor2" . ((viewer . "viewer2"))))
+ ("major1"
+ ("minor1" . ((viewer . "viewer1"))))))))
+
+ ;; Same spiel but with extra entries in INFO.
+ (let ((mailcap-mime-data))
+ ;; Add a new major entry to an empty ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major1" "minor1"
+ (list (cons 'viewer "viewer1")
+ (cons 'print "print1"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major1"
+ ("minor1" . ((viewer . "viewer1")
+ (print . "print1")))))))
+
+ ;; Add a new major entry to a non-empty ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major2" "minor2"
+ (list (cons 'viewer "viewer2")
+ (cons 'print "print2")
+ (cons 'compose "compose2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major2"
+ ("minor2" . ((viewer . "viewer2")
+ (print . "print2")
+ (compose . "compose2"))))
+ ("major1"
+ ("minor1" . ((viewer . "viewer1")
+ (print . "print1")))))))))
+
+
+(ert-deftest mailcap-add-mailcap-entry-new-minor-to-empty-major ()
+ "Add a minor entry to a an empty major entry."
+ (let ((mailcap-mime-data (list (list "major"))))
+ (mailcap-add-mailcap-entry "major" "minor1"
+ (list (cons 'viewer "viewer1")
+ (cons 'print "print1"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor1" . ((viewer . "viewer1")
+ (print . "print1")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-new-minor-to-non-empty-major ()
+ "Add a minor to a major entry containing already minor entries."
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor1"
+ (cons 'viewer "viewer1")
+ (cons 'test "test1")
+ (cons 'print "print1"))))))
+
+ (mailcap-add-mailcap-entry "major" "minor2"
+ (list (cons 'viewer "viewer2")
+ (cons 'test "test2")
+ (cons 'print "print2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor2" . ((viewer . "viewer2")
+ (test . "test2")
+ (print . "print2")))
+ ("minor1" . ((viewer . "viewer1")
+ (test . "test1")
+ (print . "print1")))))))
+
+ (mailcap-add-mailcap-entry "major" "minor3"
+ (list (cons 'viewer "viewer3")
+ (cons 'test "test3")
+ (cons 'compose "compose3"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor3" . ((viewer . "viewer3")
+ (test . "test3")
+ (compose . "compose3")))
+ ("minor2" . ((viewer . "viewer2")
+ (test . "test2")
+ (print . "print2")))
+ ("minor1" . ((viewer . "viewer1")
+ (test . "test1")
+ (print . "print1")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-new-minor-to-various-major-positions ()
+ "Add a new minor entry to major entries at various postions
+in ‘mailcap-mime-data’."
+ (let ((mailcap-mime-data
+ (list
+ (list "major1"
+ (list "minor1.1"
+ (cons 'viewer "viewer1.1")
+ (cons 'print "print1.1")))
+ (list "major2"
+ (list "minor2.1"
+ (cons 'viewer "viewer2.1")
+ (cons 'print "print2.1")
+ (cons 'compose "compose2.1")))
+ (list "major3"
+ (list "minor3.1"
+ (cons 'viewer "viewer3.1")
+ (cons 'compose "compose3.1")))
+ (list "major4"
+ (list "minor4.1"
+ (cons 'viewer "viewer4.1")
+ (cons 'edit "edit4.1"))))))
+
+ ;; Add a minor entry to a major mode at the front of
+ ;; ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major1" "minor1.2"
+ (list (cons 'viewer "viewer1.2")
+ (cons 'test "test1.2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major1"
+ ("minor1.2" . ((viewer . "viewer1.2")
+ (test . "test1.2")))
+ ("minor1.1" . ((viewer . "viewer1.1")
+ (print . "print1.1"))))
+ ("major2"
+ ("minor2.1" . ((viewer . "viewer2.1")
+ (print . "print2.1")
+ (compose . "compose2.1"))))
+ ("major3"
+ ("minor3.1" . ((viewer . "viewer3.1")
+ (compose . "compose3.1"))))
+ ("major4"
+ ("minor4.1" . ((viewer . "viewer4.1")
+ (edit . "edit4.1")))))))
+
+ ;; Add a minor entry to a major mode in the middle of
+ ;; ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major3" "minor3.2"
+ (list (cons 'viewer "viewer3.2")
+ (cons 'test "test3.2")
+ (cons 'compose "compose3.2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major1"
+ ("minor1.2" . ((viewer . "viewer1.2")
+ (test . "test1.2")))
+ ("minor1.1" . ((viewer . "viewer1.1")
+ (print . "print1.1"))))
+ ("major2"
+ ("minor2.1" . ((viewer . "viewer2.1")
+ (print . "print2.1")
+ (compose . "compose2.1"))))
+ ("major3"
+ ("minor3.2" . ((viewer . "viewer3.2")
+ (test . "test3.2")
+ (compose . "compose3.2")))
+ ("minor3.1" . ((viewer . "viewer3.1")
+ (compose . "compose3.1"))))
+ ("major4"
+ ("minor4.1" . ((viewer . "viewer4.1")
+ (edit . "edit4.1")))))))
+
+ ;; Add a minor entry to a major mode at the end of
+ ;; ‘mailcap-mime-data’.
+ (mailcap-add-mailcap-entry "major4" "minor4.2"
+ (list (cons 'viewer "viewer4.2")
+ (cons 'test "test4.2")
+ (cons 'print "print4.2")
+ (cons 'compose "compose4.2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major1"
+ ("minor1.2" . ((viewer . "viewer1.2")
+ (test . "test1.2")))
+ ("minor1.1" . ((viewer . "viewer1.1")
+ (print . "print1.1"))))
+ ("major2"
+ ("minor2.1" . ((viewer . "viewer2.1")
+ (print . "print2.1")
+ (compose . "compose2.1"))))
+ ("major3"
+ ("minor3.2" . ((viewer . "viewer3.2")
+ (test . "test3.2")
+ (compose . "compose3.2")))
+ ("minor3.1" . ((viewer . "viewer3.1")
+ (compose . "compose3.1"))))
+ ("major4"
+ ("minor4.2" . ((viewer . "viewer4.2")
+ (test . "test4.2")
+ (print . "print4.2")
+ (compose . "compose4.2")))
+ ("minor4.1" . ((viewer . "viewer4.1")
+ (edit . "edit4.1")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-existing-with-test-differing-viewer ()
+ "Add a new entry for an already existing major/minor entry."
+
+ ;; The new and the existing entry have each a test info field.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer1")
+ (cons 'test "test1")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer2")
+ (cons 'test "test2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer2")
+ (test . "test2")))
+ ("minor" . ((viewer . "viewer1")
+ (test . "test1")
+ (print . "print1"))))))))
+
+ ;; Only the new entry has a test info field.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer1")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer2")
+ (cons 'test "test2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer2")
+ (test . "test2")))
+ ("minor" . ((viewer . "viewer1")
+ (print . "print1"))))))))
+
+ ;; Only the existing entry has a test info field.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer1")
+ (cons 'test "test1")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer2")))
+ ("minor" . ((viewer . "viewer1")
+ (test . "test1")
+ (print . "print1")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-existing-with-test-same-viewer ()
+ "Add a new entry for an already existing major/minor entry."
+ ;; Both the new and the existing entry have each a test info field.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer")
+ (cons 'test "test1")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer")
+ (cons 'test "test2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer")
+ (test . "test2")))
+ ("minor" . ((viewer . "viewer")
+ (test . "test1")
+ (print . "print1"))))))))
+
+ ;; Only the new entry has a test field.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer")
+ (cons 'test "test2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer")
+ (test . "test2")))
+ ("minor" . ((viewer . "viewer")
+ (print . "print1"))))))))
+
+ ;; Only the existing entry has a test info field.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer")
+ (cons 'test "test1")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer")))
+ ("minor" . ((viewer . "viewer")
+ (test . "test1")
+ (print . "print1")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-existing-without-test-differing-viewer ()
+ "Add a new entry for an already existing major/minor entry."
+ ;; Both entries do not have test fields.
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer1")
+ (cons 'print "print1"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer2")
+ (cons 'compose "print2"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer2")
+ (compose . "print2")))
+ ("minor" . ((viewer . "viewer1")
+ (print . "print1")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-simple-merge ()
+ "Merge entries without tests (no extra info fields in the existing entry)."
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer"))
+ 'mailcap-mime-data)
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer"))))))))
+
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer")
+ (cons 'print "print"))
+ 'mailcap-mime-data)
+
+ (should (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer")
+ (print . "print")))))))))
+
+(ert-deftest mailcap-add-mailcap-entry-erroneous-merge ()
+ "Merge entries without tests (extra info fields in existing entry).
+
+In its current implementation ‘mailcap-add-mailcap-entry’ loses
+extra fields of an entry already existing in ‘mailcap-mime-data’.
+This test does not actually verify a correct result; it merely
+checks whether ‘mailcap-add-mailcap-entry’ behaviour is still the
+incorrect one. As such, it can be satisfied by any other result
+than the expected and known wrong one, and its success does not
+help to verify the correct addition and merging of an entry."
+ :expected-result :failed
+
+ (let ((mailcap-mime-data
+ (list
+ (list "major"
+ (list "minor"
+ (cons 'viewer "viewer")
+ (cons 'print "print"))))))
+ (mailcap-add-mailcap-entry "major" "minor"
+ (list (cons 'viewer "viewer")
+ (cons 'edit "edit"))
+ 'mailcap-mime-data)
+ ;; Has the print field been lost?
+ (should-not (equal mailcap-mime-data
+ '(("major"
+ ("minor" . ((viewer . "viewer")
+ (edit . "edit")))))))))
+
+
;;; mailcap-tests.el ends here
--
2.35.1
[-- Attachment #3: Type: text/plain, Size: 21 bytes --]
--
Felix Dietrich
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-19 14:12 ` Felix Dietrich
@ 2022-03-20 15:21 ` Lars Ingebrigtsen
2022-03-20 23:52 ` Felix Dietrich
0 siblings, 1 reply; 8+ messages in thread
From: Lars Ingebrigtsen @ 2022-03-20 15:21 UTC (permalink / raw)
To: Felix Dietrich; +Cc: emacs-devel
Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
> I have written a couple of test cases. Let me know if this is what you
> had in mind.
Yes, great!
But I forget -- are your Emacs copyright assignment papers in the works?
(This is too big to apply without such paperwork.)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-20 15:21 ` Lars Ingebrigtsen
@ 2022-03-20 23:52 ` Felix Dietrich
2022-03-21 14:55 ` Lars Ingebrigtsen
0 siblings, 1 reply; 8+ messages in thread
From: Felix Dietrich @ 2022-03-20 23:52 UTC (permalink / raw)
To: emacs-devel
Lars Ingebrigtsen <larsi@gnus.org> writes:
> But I forget -- are your Emacs copyright assignment papers in the works?
No, could you send me the papers or point me to whom to ask for them?
--
Felix Dietrich
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding
2022-03-20 23:52 ` Felix Dietrich
@ 2022-03-21 14:55 ` Lars Ingebrigtsen
0 siblings, 0 replies; 8+ messages in thread
From: Lars Ingebrigtsen @ 2022-03-21 14:55 UTC (permalink / raw)
To: Felix Dietrich; +Cc: emacs-devel
Felix Dietrich <felix.dietrich@sperrhaken.name> writes:
> No, could you send me the papers or point me to whom to ask for them?
Below is the form to get started.
To avoid your patch getting lost or forgotten, please re-submit it with
`M-x submit-emacs-patch' so that it lands in the bug tracker.
Please email the following information to assign@gnu.org, and we
will send you the assignment form for your past and future changes.
Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES
[What is the name of the program or package you're contributing to?]
Emacs
[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]
[Do you have an employer who might have a basis to claim to own
your changes? Do you attend a school which might make such a claim?]
[For the copyright registration, what country are you a citizen of?]
[What year were you born?]
[Please write your email address here.]
[Please write your postal address here.]
[Which files have you changed so far, and which new files have you written
so far?]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-21 14:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-07 16:35 [PATCH] mailcap-add-mailcap-entry: Superfluous setcdr, readability, entry overriding Felix Dietrich
2022-03-09 14:03 ` Lars Ingebrigtsen
2022-03-10 5:45 ` Felix Dietrich
2022-03-12 17:26 ` Lars Ingebrigtsen
2022-03-19 14:12 ` Felix Dietrich
2022-03-20 15:21 ` Lars Ingebrigtsen
2022-03-20 23:52 ` Felix Dietrich
2022-03-21 14:55 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).