GNU bug report logs - #75789
[PATCH Debbugs] Factor cache accesses into dedicated functions

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 75789 in the body.
You can then email your comments to 75789 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <Morgan.J.Smith <at> outlook.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Subject: [PATCH Debbugs] Factor cache accesses into dedicated functions
Date: Thu, 23 Jan 2025 13:22:18 -0500

* debbugs.el (debbugs-get-cache, debbugs-put-cache): New functions.
(debbugs-newest-bugs, debbugs-get-status): Use new functions.

* test/debbugs-tests.el: Use advice to set the `cache_time' around the new
functions.
(debbugs-test-newest-bug-cached): New test.
(debbugs-test-get-status): Add test for caching behavior.
---

Hello!

Just a little bit of code cleanup.  I hope this is helpful.

You'll notice I removed a bit in `debbugs-newest-bugs' that was supposed to
temporarily generate a null value.  I'm not entirely certain but I don't
believe that actually does anything right?  Immediately after that we make a
synchronous soap call anyways and then return that value.

 debbugs.el            | 80 +++++++++++++++++++------------------------
 test/debbugs-tests.el | 50 +++++++++++++++++++++++----
 2 files changed, 79 insertions(+), 51 deletions(-)

diff --git a/debbugs.el b/debbugs.el
index 5ab1dfc4ba..36fdca8ff0 100644
--- a/debbugs.el
+++ b/debbugs.el
@@ -118,6 +118,28 @@ t or 0 disables caching, nil disables expiring."
 		 (const :tag "Forever" nil)
 		 (integer :tag "Seconds")))
 
+(defun debbugs-get-cache (bug-number)
+  "Return the cached status entry for the bug identified by BUG-NUMBER."
+  (let ((status (gethash bug-number debbugs-cache-data)))
+    (when (and status
+               (or (null debbugs-cache-expiry)
+                   (and
+                    (natnump debbugs-cache-expiry)
+                    (> (alist-get 'cache_time status)
+                       (- (float-time) debbugs-cache-expiry)))))
+      status)))
+
+(defun debbugs-put-cache (bug-number status)
+  "Put the STATUS entry for the bug BUG-NUMBER in the cache.
+Return STATUS."
+  (if (or (null debbugs-cache-expiry)
+          (and (natnump debbugs-cache-expiry)
+               (not (zerop debbugs-cache-expiry))))
+      (puthash bug-number
+               (cons (cons 'cache_time (float-time)) status)
+               debbugs-cache-data)
+    status))
+
 (defun debbugs-soap-invoke (operation-name &rest parameters)
   "Invoke the SOAP connection.
 OPERATION-NAME and PARAMETERS are as described in `soap-invoke'."
@@ -325,38 +347,20 @@ patch:
 (defun debbugs-newest-bugs (amount)
   "Return the list of bug numbers, according to AMOUNT (a number) latest bugs."
   (if (= amount 1)
-      ;; We cache it as bug "0" in `debbugs-cache-data'.
-      (let ((status (gethash 0 debbugs-cache-data)))
-	(unless (and
-		 status
-		 (or
-		  (null debbugs-cache-expiry)
-		  (and
-		   (natnump debbugs-cache-expiry)
-		   (> (alist-get 'cache_time status)
-		      (- (float-time) debbugs-cache-expiry)))))
-	  ;; Due to `debbugs-gnu-completion-table', this function
-	  ;; could be called in rapid sequence.  We cache temporarily
-	  ;; the value nil, therefore.
-	  (when (natnump debbugs-cache-expiry)
-	    (puthash
-	     0
-	     (list (cons 'cache_time (1+ (- (float-time) debbugs-cache-expiry)))
-		   (list 'newest_bug))
-	     debbugs-cache-data))
+      ;; We cache it as bug "0"
+      (let ((status (debbugs-get-cache 0)))
+	(unless status
 	  ;; Compute the value.
 	  (setq
 	   status
-	   (list
-	    (cons 'cache_time (float-time))
-	    (cons 'newest_bug
-		  (caar
-		   (debbugs-soap-invoke
-		    debbugs-wsdl debbugs-port "newest_bugs" amount)))))
+       (list
+        (cons 'newest_bug
+		      (caar
+		       (debbugs-soap-invoke
+		        debbugs-wsdl debbugs-port "newest_bugs" amount)))))
 
 	  ;; Cache it.
-	  (when (or (null debbugs-cache-expiry) (natnump debbugs-cache-expiry))
-	    (puthash 0 status debbugs-cache-data)))
+      (debbugs-put-cache 0 status))
 
 	;; Return the value, as list.
 	(list (alist-get 'newest_bug status)))
@@ -477,15 +481,8 @@ Example:
 	  (delq nil
 	   (mapcar
 	    (lambda (bug)
-	      (let ((status (gethash bug debbugs-cache-data)))
-		(if (and
-		     status
-		     (or
-		      (null debbugs-cache-expiry)
-		      (and
-		       (natnump debbugs-cache-expiry)
-		       (> (alist-get 'cache_time status)
-			  (- (float-time) debbugs-cache-expiry)))))
+	      (let ((status (debbugs-get-cache bug)))
+		(if status
 		    (progn
 		      (setq cached-bugs (append cached-bugs (list status)))
 		      nil)
@@ -582,14 +579,9 @@ Example:
 	    (when (stringp (cdr y))
 	      (setcdr y (split-string (cdr y) ",\\| " t))))
 	  ;; Cache the result, and return.
-	  (if (or (null debbugs-cache-expiry) (natnump debbugs-cache-expiry))
-	      (puthash
-	       (alist-get 'key x)
-	       ;; Put also a time stamp.
-	       (cons (cons 'cache_time (float-time)) (alist-get 'value x))
-	       debbugs-cache-data)
-	    ;; Don't cache.
-	    (alist-get 'value x))))
+      (debbugs-put-cache
+       (alist-get 'key x)
+       (alist-get 'value x))))
       debbugs-soap-invoke-async-object))))
 
 (defun debbugs-get-usertag (&rest query)
diff --git a/test/debbugs-tests.el b/test/debbugs-tests.el
index ce6489ca39..d0aa9e87b4 100644
--- a/test/debbugs-tests.el
+++ b/test/debbugs-tests.el
@@ -28,6 +28,10 @@
 
 (require 'debbugs)
 
+;; TODO: This shouldn't be necessary but I get the error
+;; "(void-variable debbugs-gnu-use-threads)" without this
+(require 'debbugs-gnu)
+
 ;;; Helper Data:
 
 ;; Generated using this:
@@ -93,6 +97,20 @@
  :override (symbol-function #'soap-invoke-internal)
  #'debbugs-test--soap-invoke-internal)
 
+(defun debbugs-test--override-float-time (func &rest rest)
+  "Override `float-time' for FUNC with args REST."
+  (cl-letf (((symbol-function #'float-time)
+             (lambda (&optional _specified-time) 5000)))
+    (apply func rest)))
+
+(add-function
+ :around (symbol-function #'debbugs-get-cache)
+ #'debbugs-test--override-float-time)
+
+(add-function
+ :around (symbol-function #'debbugs-put-cache)
+ #'debbugs-test--override-float-time)
+
 ;;; Tests:
 
 (ert-deftest debbugs-test-get-bugs ()
@@ -115,16 +133,34 @@
     (should (string-equal debbugs-test--soap-operation-name "newest_bugs"))
     (should (equal debbugs-test--soap-parameters '(4)))))
 
+(ert-deftest debbugs-test-newest-bug-cached ()
+  "Test getting the newest bug from the cache."
+  (let (debbugs-test--soap-operation-name debbugs-test--soap-parameters)
+    ;; First time we get it from the server.
+    (should (equal (debbugs-newest-bugs 1) '(0)))
+    (should (equal debbugs-test--soap-operation-name "newest_bugs"))
+    (should (equal debbugs-test--soap-parameters '(1)))
+    (setq debbugs-test--soap-operation-name nil)
+    (setq debbugs-test--soap-parameters nil)
+    ;; Now it's cached
+    (should (equal (debbugs-newest-bugs 1) '(0)))
+    (should (equal debbugs-test--soap-operation-name nil))
+    (should (equal debbugs-test--soap-parameters nil))))
+
 (ert-deftest debbugs-test-get-status ()
   "Test \"get_status\"."
   (let (debbugs-test--soap-operation-name debbugs-test--soap-parameters)
-    (cl-letf (((symbol-function #'float-time)
-               (lambda (&optional _specified-time) 5000)))
-      (should (= (float-time) 5000))
-      (should (equal (sort (car (debbugs-get-status 64064)))
-                     (sort (car debbugs-test--bug-status))))
-      (should (string-equal debbugs-test--soap-operation-name "get_status"))
-      (should (equal debbugs-test--soap-parameters '([64064]))))))
+    (should (equal (sort (car (debbugs-get-status 64064)))
+                   (sort (car debbugs-test--bug-status))))
+    (should (string-equal debbugs-test--soap-operation-name "get_status"))
+    (should (equal debbugs-test--soap-parameters '([64064])))
+    (setq debbugs-test--soap-operation-name nil)
+    (setq debbugs-test--soap-parameters nil)
+    ;; cached
+    (should (equal (sort (car (debbugs-get-status 64064)))
+                   (sort (car debbugs-test--bug-status))))
+    (should (equal debbugs-test--soap-operation-name nil))
+    (should (equal debbugs-test--soap-parameters nil))))
 
 (ert-deftest debbugs-test-get-usertag ()
   "Test \"get_usertag\"."
-- 
2.47.1

Message #10 received at 75789 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 75789 <at> debbugs.gnu.org
Subject: Re: bug#75789: [PATCH Debbugs] Factor cache accesses into dedicated
 functions
Date: Sun, 26 Jan 2025 11:45:29 +0100

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

> Hello!

Hi Morgan,

> Just a little bit of code cleanup.  I hope this is helpful.
>
> You'll notice I removed a bit in `debbugs-newest-bugs' that was supposed to
> temporarily generate a null value.  I'm not entirely certain but I don't
> believe that actually does anything right?  Immediately after that we make a
> synchronous soap call anyways and then return that value.

I don't know. See the comment you've removed as well:

--8<---------------cut here---------------start------------->8---
> -	  ;; Due to `debbugs-gnu-completion-table', this function
> -	  ;; could be called in rapid sequence.  We cache temporarily
> -	  ;; the value nil, therefore.
--8<---------------cut here---------------end--------------->8---

Have you checked that this doesn't happen without the temporary setting?

Some few comments:

> index ce6489ca39..d0aa9e87b4 100644
> --- a/test/debbugs-tests.el
> +++ b/test/debbugs-tests.el
> @@ -28,6 +28,10 @@
>
>  (require 'debbugs)
>
> +;; TODO: This shouldn't be necessary but I get the error
> +;; "(void-variable debbugs-gnu-use-threads)" without this
> +(require 'debbugs-gnu)

Oops. This is an existing bug. I will try to fix it later.

> +(add-function
> + :around (symbol-function #'debbugs-get-cache)
> + #'debbugs-test--override-float-time)
> +
> +(add-function
> + :around (symbol-function #'debbugs-put-cache)
> + #'debbugs-test--override-float-time)

This is wrong as-it-is. The extended definitions of debbugs-*-cache will
persist after running the tests. A strict rule is, that all changes must
be reverted after running the tests.

I don't believe you need this. Keep the (cl-letf (((symbol-function
#'float-time) ...))) clause in debbugs-test-get-status, and do the same
in debbugs-test-newest-bug-cached.

Otherwise, it looks good to me. Please push to the debbugs repo; I'll
play with it for a while before making a new release.

Thanks, and best regards, Michael.

Message #13 received at 75789 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 75789 <at> debbugs.gnu.org
Subject: Re: bug#75789: [PATCH Debbugs] Factor cache accesses into dedicated
 functions
Date: Mon, 27 Jan 2025 18:13:01 +0100

Michael Albinus <michael.albinus <at> gmx.de> writes:

Hi Morgan,

>> +;; TODO: This shouldn't be necessary but I get the error
>> +;; "(void-variable debbugs-gnu-use-threads)" without this
>> +(require 'debbugs-gnu)
>
> Oops. This is an existing bug. I will try to fix it later.

This is fixed now in the git repo. You don't need to require debbugs-gnu anymore.

Best regards, Michael.

Message #16 received at 75789 <at> debbugs.gnu.org (full text, mbox):

From: Morgan Smith <morgan.j.smith <at> outlook.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 75789 <at> debbugs.gnu.org
Subject: Re: bug#75789: [PATCH Debbugs] Factor cache accesses into dedicated
 functions
Date: Sun, 09 Feb 2025 21:01:30 -0500

[Message part 1 (text/plain, inline)]

Michael Albinus <michael.albinus <at> gmx.de> writes:

> Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:
>
>> Hello!
>
> Hi Morgan,
>
Hi Michael,

Apologies for the delay.  I started a new job the day after I sent this
email!

>> Just a little bit of code cleanup.  I hope this is helpful.
>>
>> You'll notice I removed a bit in `debbugs-newest-bugs' that was supposed to
>> temporarily generate a null value.  I'm not entirely certain but I don't
>> believe that actually does anything right?  Immediately after that we make a
>> synchronous soap call anyways and then return that value.
>
> I don't know. See the comment you've removed as well:
>
> --8<---------------cut here---------------start------------->8---
>> -	  ;; Due to `debbugs-gnu-completion-table', this function
>> -	  ;; could be called in rapid sequence.  We cache temporarily
>> -	  ;; the value nil, therefore.
> --8<---------------cut here---------------end--------------->8---
>
> Have you checked that this doesn't happen without the temporary
> setting?

I'm honestly still not sure.  Maybe it's a simple test but I'm still
scared of completion tables from a bad experience a few years back.
I've just made some modifications so the same logic is in place.

>
> Some few comments:
>
>> index ce6489ca39..d0aa9e87b4 100644
>> --- a/test/debbugs-tests.el
>> +++ b/test/debbugs-tests.el
>> @@ -28,6 +28,10 @@
>>
>>  (require 'debbugs)
>>
>> +;; TODO: This shouldn't be necessary but I get the error
>> +;; "(void-variable debbugs-gnu-use-threads)" without this
>> +(require 'debbugs-gnu)
>
> Oops. This is an existing bug. I will try to fix it later.
>

Thanks for fixing that!

>> +(add-function
>> + :around (symbol-function #'debbugs-get-cache)
>> + #'debbugs-test--override-float-time)
>> +
>> +(add-function
>> + :around (symbol-function #'debbugs-put-cache)
>> + #'debbugs-test--override-float-time)
>
> This is wrong as-it-is. The extended definitions of debbugs-*-cache will
> persist after running the tests. A strict rule is, that all changes must
> be reverted after running the tests.

This is being violated in the current tests by the stubbing out of
`soap-invoke-internal'.  Please see the first attached patch where I fix
this.

> I don't believe you need this. Keep the (cl-letf (((symbol-function
> #'float-time) ...))) clause in debbugs-test-get-status, and do the same
> in debbugs-test-newest-bug-cached.

I wanted a clean wrapper around the cache functions to make it easy for
future testing.  Specifically, I want to add a feature so the cache
survives an emacs restart.

> Otherwise, it looks good to me. Please push to the debbugs repo; I'll
> play with it for a while before making a new release.

I don't believe I have push access to the repository.

> Thanks, and best regards, Michael.

Thanks for reviewing my patches!

Morgan

[0001-tests-Call-new-setup-and-teardown-functions-in-tests.patch (text/x-patch, attachment)]

[0002-Factor-cache-accesses-into-dedicated-functions.patch (text/x-patch, attachment)]

Message #19 received at 75789 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Morgan Smith <morgan.j.smith <at> outlook.com>
Cc: 75789 <at> debbugs.gnu.org
Subject: Re: bug#75789: [PATCH Debbugs] Factor cache accesses into dedicated
 functions
Date: Tue, 11 Feb 2025 12:04:16 +0100

Morgan Smith <morgan.j.smith <at> outlook.com> writes:

> Hi Michael,

Hi Morgan,

> Apologies for the delay.  I started a new job the day after I sent this
> email!

Congrat! I wish you will fun with the new job!

>> Otherwise, it looks good to me. Please push to the debbugs repo; I'll
>> play with it for a while before making a new release.
>
> I don't believe I have push access to the repository.

I've pushed it to the git repo.

> Thanks for reviewing my patches!
>
> Morgan

Best regards, Michael.

Message #24 received at 75789-done <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Morgan Smith <Morgan.J.Smith <at> outlook.com>
Cc: 75789-done <at> debbugs.gnu.org
Subject: Re: bug#75143: debbugs info manual name
Date: Sat, 15 Feb 2025 18:38:05 +0100

Morgan Smith <Morgan.J.Smith <at> outlook.com> writes:

> Hi Michael,

Hi Morgan,

> However, I think I did come up with a way to simply change the user
> facing name of the manual without breaking links.  I'm not super well
> versed in texinfo but after applying the attached changes and installing
> them on my system, I have been able to access the manuals using new
> names yet the links within the manual that link to each other seem to
> remain functional.
>
> Let me know what you think!

LGTM, thanks! I've pushed both patches to the ELPA git repo. In a couple
of days I'll release a new debbugs version.

Closing the bug.

> Thanks,
>
> Morgan

Best regards, Michael.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.