Re: master da4e5f6: Simplify use of timestamps

Re: master da4e5f6: Simplify use of timestamps

[Glenn Morris]

Paul Eggert wrote:

> branch: master
> commit da4e5f668582e1f047b6bd5259a1a4f92b5461b6
[...]
> --- a/lisp/calendar/icalendar.el
> +++ b/lisp/calendar/icalendar.el
> @@ -1016,9 +1016,7 @@ current iCalendar object, as a string.  Increase
>        (setq icalendar--uid-count (1+ icalendar--uid-count))
>        (setq uid (replace-regexp-in-string
>                   "%t"
> -                 (format "%d%d%d" (car (current-time))
> -                         (cadr (current-time))
> -                         (car (cddr (current-time))))
> +                 (format-time-string "%s%N")
>                   uid t t))

This causes test icalendar--create-uid to fail.
(BTW, make -j8 check takes 22 seconds here, so not a big burden to run
after making a change.)

> --- a/lisp/gnus/nnmaildir.el
> +++ b/lisp/gnus/nnmaildir.el
> @@ -764,7 +764,7 @@ This variable is set by `nnmaildir-request-article'.")
>  
>  (defun nnmaildir--scan (gname scan-msgs groups _method srv-dir srv-ls)
>    (catch 'return
> -    (let ((36h-ago (- (car (current-time)) 2))
> +    (let ((36h-ago (- (float-time) 129600))
>  	  absdir nndir tdir ndir cdir nattr cattr isnew pgname read-only ls
>  	  files num dir flist group x)
>        (setq absdir (nnmaildir--srvgrp-dir srv-dir gname)
> @@ -801,7 +801,7 @@ This variable is set by `nnmaildir-request-article'.")
>  	  (throw 'return nil))
>  	(dolist (file (funcall ls tdir 'full "\\`[^.]" 'nosort))
>  	  (setq x (file-attributes file))
> -	  (if (or (> (cadr x) 1) (< (car (nth 4 x)) 36h-ago))
> +	  (if (or (> (cadr x) 1) (time-less-p (nth 4 x) 36h-ago))
>  	      (delete-file file))))
>        (or scan-msgs
>  	  isnew
> @@ -1463,9 +1463,7 @@ This variable is set by `nnmaildir-request-article'.")
>        (unless (string-equal nnmaildir--delivery-time file)
>  	(setq nnmaildir--delivery-time file
>  	      nnmaildir--delivery-count 0))
> -      (when (and (consp (cdr time))
> -		 (consp (cddr time)))
> -	(setq file (concat file "M" (number-to-string (caddr time)))))
> +      (setq file (concat file (format-time-string "M%6N" time)))
>        (setq file (concat file nnmaildir--delivery-pid)
>  	    file (concat file "Q" (number-to-string nnmaildir--delivery-count))
>  	    file (concat file "." (nnmaildir--system-name))

I haven't looked at this in any detail, so this comment could be
rubbish, but if this causes a change in the on-disk file names Gnus uses
for maildir, that could be bad, no? (Eg running a Gnus-with-new-maildir
on a pre-existing maildir store might not find the same files as
Gnus-with-old-maildir.)

Re: master da4e5f6: Simplify use of timestamps

[Paul Eggert]

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

Glenn Morris wrote:

> This causes test icalendar--create-uid to fail.

Argh, that test is fooling around with internal implementation details that it 
has no business looking into. I fixed it to stop doing that by installing the 
first attached patch.

> (BTW, make -j8 check takes 22 seconds here, so not a big burden to run
> after making a change.)

Yes, I normally run those tests, though they take 36 seconds on my older 
machine. Don't know why I forgot to do it this time; sorry about that.

By the way, 'make check' routinely fails on my platform (Fedora 28 x86-64) on 
master, and this is a bit discouraging. Does it fail for you? 
lisp/progmodes/flymake-tests.log and lisp/epg-tests.log are common failures, but 
there are others on occasion. It's getting so that I sometimes don't notice real 
failures.

> I haven't looked at this in any detail, so this comment could be
> rubbish, but if this causes a change in the on-disk file names Gnus uses
> for maildir, that could be bad, no?

The timestamp is used as part of a nonce, as 'time' was set to (current-time) a 
few lines earlier. So although the patch might change the on-disk file name (by 
padding a microseconds count less than 100,000 with leading "0"s) I don't see 
how that would cause trouble; anybody later needs to grab the file by its name 
anyway, and will get that name either from this code or by looking at the 
containing directory.

That being said, you're right that this is a change. And it's one that I 
overlooked, because I didn't notice that the old code didn't pad with leading 
"0"s. Since it's only a trivial simplification I undid it by installing the 
second attached patch. Although this doesn't entirely revert the change, since 
the old code had some unnecessary stuff about checking that (current-time) 
returns a list of four items, it does make the file names byte-for-byte 
equivalent to the old code.

[-- Attachment #2: 0001-Fix-icalendar-tests-to-match-new-behavior.patch --]
[-- Type: text/x-patch, Size: 1955 bytes --]

From 3937b5b52af3eb78101dc385ac8dfe7a36e2e624 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 15 Sep 2018 14:10:49 -0700
Subject: [PATCH 1/2] Fix icalendar tests to match new behavior

* test/lisp/calendar/icalendar-tests.el (icalendar--create-uid):
Do not intrude into or rely upon undocumented internal
implementation details of icalendar--create-uid.
Problem reported by Glenn Morris in:
https://lists.gnu.org/r/emacs-devel/2018-09/msg00660.html
---
 test/lisp/calendar/icalendar-tests.el | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/test/lisp/calendar/icalendar-tests.el b/test/lisp/calendar/icalendar-tests.el
index 2fecabcd75..617e886989 100644
--- a/test/lisp/calendar/icalendar-tests.el
+++ b/test/lisp/calendar/icalendar-tests.el
@@ -57,17 +57,16 @@ icalendar-tests--trim
 
 (ert-deftest icalendar--create-uid ()
   "Test for `icalendar--create-uid'."
-  (let* ((icalendar-uid-format "xxx-%t-%c-%h-%u-%s")
+  (let* ((icalendar-uid-format "xxx-%c-%h-%u-%s")
          (icalendar--uid-count 77)
          (entry-full "30.06.1964 07:01 blahblah")
          (hash (format "%d" (abs (sxhash entry-full))))
          (contents "DTSTART:19640630T070100\nblahblah")
          (username (or user-login-name "UNKNOWN_USER")))
-    (cl-letf (((symbol-function 'current-time) (lambda () '(1 2 3))))
-      (should (= 77 icalendar--uid-count))
-      (should (string=  (concat "xxx-123-77-" hash "-" username "-19640630")
-                        (icalendar--create-uid entry-full contents)))
-      (should (= 78 icalendar--uid-count)))
+    (should (= 77 icalendar--uid-count))
+    (should (string= (concat "xxx-77-" hash "-" username "-19640630")
+                     (icalendar--create-uid entry-full contents)))
+    (should (= 78 icalendar--uid-count))
     (setq contents "blahblah")
     (setq icalendar-uid-format "yyy%syyy")
     (should (string=  (concat "yyyDTSTARTyyy")
-- 
2.17.1


[-- Attachment #3: 0002-Go-back-to-old-method-for-nnmaildir-names.patch --]
[-- Type: text/x-patch, Size: 1176 bytes --]

From d2048949bcd5735c17b4a194fc75d16ba25907ca Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 15 Sep 2018 15:00:54 -0700
Subject: [PATCH 2/2] Go back to old method for nnmaildir names

* lisp/gnus/nnmaildir.el (nnmaildir-request-accept-article):
Omit leading 0s after "M" in file name.
Problem reported by Glenn Morris in:
        https://lists.gnu.org/r/emacs-devel/2018-09/msg00660.html
---
 lisp/gnus/nnmaildir.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/gnus/nnmaildir.el b/lisp/gnus/nnmaildir.el
index 48a470c746..fbabf573c4 100644
--- a/lisp/gnus/nnmaildir.el
+++ b/lisp/gnus/nnmaildir.el
@@ -1463,7 +1463,7 @@ nnmaildir-request-accept-article
       (unless (string-equal nnmaildir--delivery-time file)
 	(setq nnmaildir--delivery-time file
 	      nnmaildir--delivery-count 0))
-      (setq file (concat file (format-time-string "M%6N" time)))
+      (setq file (concat file "M" (number-to-string (caddr time))))
       (setq file (concat file nnmaildir--delivery-pid)
 	    file (concat file "Q" (number-to-string nnmaildir--delivery-count))
 	    file (concat file "." (nnmaildir--system-name))
-- 
2.17.1