unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
       [not found] ` <20220425194030.030A8C01687@vcs2.savannah.gnu.org>
@ 2022-04-26  9:38   ` Lars Ingebrigtsen
  2022-04-26 10:20     ` Manuel Uberti
                       ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-26  9:38 UTC (permalink / raw)
  To: emacs-devel; +Cc: Paul Eggert

Paul Eggert <eggert@cs.ucla.edu> writes:

>     Use (TICKS . HZ) for current-time etc.
>
>     * src/timefns.c (CURRENT_TIME_LIST): Change default to false.

I'm afraid that this is going to break a lot of third party code out
there (that assumes that `current-time' returns a list).  Have you done
any kind of survey?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26  9:38   ` master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Lars Ingebrigtsen
@ 2022-04-26 10:20     ` Manuel Uberti
  2022-04-26 10:38       ` Po Lu
  2022-04-26 20:28     ` Paul Eggert
  2022-04-26 23:54     ` Michael Heerdegen
  2 siblings, 1 reply; 20+ messages in thread
From: Manuel Uberti @ 2022-04-26 10:20 UTC (permalink / raw)
  To: Lars Ingebrigtsen, emacs-devel; +Cc: Paul Eggert

On 26/04/22 11:38, Lars Ingebrigtsen wrote:
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
>>      Use (TICKS . HZ) for current-time etc.
>>
>>      * src/timefns.c (CURRENT_TIME_LIST): Change default to false.
> 
> I'm afraid that this is going to break a lot of third party code out
> there (that assumes that `current-time' returns a list).  Have you done
> any kind of survey?
> 

It sure breaks CIDER, for instance: 
https://github.com/clojure-emacs/cider/issues/3183


-- 
Manuel Uberti
https://manueluberti.eu



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26 10:20     ` Manuel Uberti
@ 2022-04-26 10:38       ` Po Lu
  2022-04-26 14:42         ` Bozhidar Batsov
  2022-04-26 20:29         ` Paul Eggert
  0 siblings, 2 replies; 20+ messages in thread
From: Po Lu @ 2022-04-26 10:38 UTC (permalink / raw)
  To: Manuel Uberti; +Cc: Lars Ingebrigtsen, emacs-devel, Paul Eggert

Manuel Uberti <manuel.uberti@inventati.org> writes:

> It sure breaks CIDER, for instance:
> https://github.com/clojure-emacs/cider/issues/3183

It also breaks something on my system, but I can't find what.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26 10:38       ` Po Lu
@ 2022-04-26 14:42         ` Bozhidar Batsov
  2022-04-26 20:29         ` Paul Eggert
  1 sibling, 0 replies; 20+ messages in thread
From: Bozhidar Batsov @ 2022-04-26 14:42 UTC (permalink / raw)
  To: Emacs Devel

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

As Manuel mentioned - this broke CIDER (a package that I maintain), and I'm pretty sure it will break other packages as well. I find it weird to be doing breaking changes to
such old functions instead of introducing newer functions to serve in their place. It'd be easy for me to update CIDER to work with the old and the new version, but anyone running an older CIDER on a newer Emacs will be affected by this. 

My suggestion would be to revert this until a less problematic solution is agreed upon. 

On Tue, Apr 26, 2022, at 1:38 PM, Po Lu wrote:
> Manuel Uberti <manuel.uberti@inventati.org> writes:
> 
> > It sure breaks CIDER, for instance:
> > https://github.com/clojure-emacs/cider/issues/3183
> 
> It also breaks something on my system, but I can't find what.
> 
> 

[-- Attachment #2: Type: text/html, Size: 1292 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26  9:38   ` master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Lars Ingebrigtsen
  2022-04-26 10:20     ` Manuel Uberti
@ 2022-04-26 20:28     ` Paul Eggert
  2022-04-27 12:17       ` Lars Ingebrigtsen
  2022-04-26 23:54     ` Michael Heerdegen
  2 siblings, 1 reply; 20+ messages in thread
From: Paul Eggert @ 2022-04-26 20:28 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

On 4/26/22 02:38, Lars Ingebrigtsen wrote:
> Have you done any kind of survey?

I asked here when this code was added years ago. I also surveyed code in 
ELPA at the time. As I recall, it appeared that it wouldn't be that much 
of a problem and that any fixes would be simple. (My memory is no doubt 
biased. :-)

The CIDER bug is now fixed 
<https://github.com/clojure-emacs/cider/commit/7506cc4613a5b71e5246fd83de347185b5d49c42>. 
FWIW the unfixed CIDER code didn't always work in Emacs 28 and earlier 
either, though its bugs there were intermittent.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26 10:38       ` Po Lu
  2022-04-26 14:42         ` Bozhidar Batsov
@ 2022-04-26 20:29         ` Paul Eggert
  2022-04-27  0:36           ` Po Lu
  1 sibling, 1 reply; 20+ messages in thread
From: Paul Eggert @ 2022-04-26 20:29 UTC (permalink / raw)
  To: Po Lu; +Cc: emacs-devel

On 4/26/22 03:38, Po Lu wrote:
> It also breaks something on my system, but I can't find what.

What are the symptoms?



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26  9:38   ` master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Lars Ingebrigtsen
  2022-04-26 10:20     ` Manuel Uberti
  2022-04-26 20:28     ` Paul Eggert
@ 2022-04-26 23:54     ` Michael Heerdegen
  2 siblings, 0 replies; 20+ messages in thread
From: Michael Heerdegen @ 2022-04-26 23:54 UTC (permalink / raw)
  To: emacs-devel

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I'm afraid that this is going to break a lot of third party code out
> there (that assumes that `current-time' returns a list).

It broke w3m, but they have installed a fix already.  It also introduced
an issue in el-search.

Michael.




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26 20:29         ` Paul Eggert
@ 2022-04-27  0:36           ` Po Lu
  2022-04-27  1:18             ` Paul Eggert
  0 siblings, 1 reply; 20+ messages in thread
From: Po Lu @ 2022-04-27  0:36 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-devel

Paul Eggert <eggert@cs.ucla.edu> writes:

> What are the symptoms?

My desktop file isn't loaded.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27  0:36           ` Po Lu
@ 2022-04-27  1:18             ` Paul Eggert
  2022-04-27  1:29               ` Po Lu
  0 siblings, 1 reply; 20+ messages in thread
From: Paul Eggert @ 2022-04-27  1:18 UTC (permalink / raw)
  To: Po Lu; +Cc: emacs-devel

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

On 4/26/22 17:36, Po Lu wrote:
> My desktop file isn't loaded.

Not a good sign. I installed the attached, which should fix a bug with a 
desktop saved with older Emacs and restored with newer. Does this patch 
fix things for you? If not, is ~/.emacs.d/.emacs.desktop something you 
can send me? And even if so, this suggests we need a better way to tell 
Emacs to use old-format timestamps for compatibility.

[-- Attachment #2: 0001-Be-more-compatible-with-older-desktops.patch --]
[-- Type: text/x-patch, Size: 979 bytes --]

From 9102c63d99b7dda5d49136c48826f87f1cc2bcea Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 26 Apr 2022 18:10:51 -0700
Subject: [PATCH] Be more compatible with older desktops
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/desktop.el (desktop-save): When comparing timestamps use
time-equal-p instead of ‘equal’.
---
 lisp/desktop.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/desktop.el b/lisp/desktop.el
index cd581e028b..baa3f32970 100644
--- a/lisp/desktop.el
+++ b/lisp/desktop.el
@@ -1121,7 +1121,7 @@ desktop-save
 			(file-attributes (desktop-full-file-name)))))
       (when
 	  (or (not new-modtime)		; nothing to overwrite
-	      (equal desktop-file-modtime new-modtime)
+	      (time-equal-p desktop-file-modtime new-modtime)
 	      (yes-or-no-p (if desktop-file-modtime
 			       (if (time-less-p desktop-file-modtime
 						new-modtime)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27  1:18             ` Paul Eggert
@ 2022-04-27  1:29               ` Po Lu
  2022-04-27  4:10                 ` Paul Eggert
  0 siblings, 1 reply; 20+ messages in thread
From: Po Lu @ 2022-04-27  1:29 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-devel

Paul Eggert <eggert@cs.ucla.edu> writes:

> Not a good sign. I installed the attached, which should fix a bug with
> a desktop saved with older Emacs and restored with newer. Does this
> patch fix things for you? If not, is ~/.emacs.d/.emacs.desktop
> something you can send me? And even if so, this suggests we need a
> better way to tell Emacs to use old-format timestamps for
> compatibility.

The change you installed fixes my problem (though I really don't see why
it did, since nothing is saved.)

Also, shouldn't the desktop file version be bumped when the format of
the time data changes?

Thanks.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27  1:29               ` Po Lu
@ 2022-04-27  4:10                 ` Paul Eggert
  0 siblings, 0 replies; 20+ messages in thread
From: Paul Eggert @ 2022-04-27  4:10 UTC (permalink / raw)
  To: Po Lu; +Cc: emacs-devel

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

On 4/26/22 18:29, Po Lu wrote:

> The change you installed fixes my problem (though I really don't see why
> it did, since nothing is saved.)

desktop-file-modtime's value is saved, and was compared for list 
equality not time equality.

> Also, shouldn't the desktop file version be bumped when the format of
> the time data changes?

Yes, or we could not change the desktop file format. I installed the 
attached to do the latter.

[-- Attachment #2: 0001-Avoid-change-to-desktop-file-format.patch --]
[-- Type: text/x-patch, Size: 2047 bytes --]

From 8c2ea3a7086353ab2e62e70f8fc7567d8cd75f7a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 26 Apr 2022 21:03:21 -0700
Subject: [PATCH] Avoid change to desktop file format

* lisp/desktop.el (desktop--get-file-modtime): New function.
(desktop-save, desktop-read): Use it.
---
 lisp/desktop.el | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lisp/desktop.el b/lisp/desktop.el
index baa3f32970..f41a41c3c3 100644
--- a/lisp/desktop.el
+++ b/lisp/desktop.el
@@ -645,6 +645,14 @@ desktop-file-modtime
   "When the desktop file was last modified to the knowledge of this Emacs.
 Used to detect desktop file conflicts.")
 
+(defun desktop--get-file-modtime ()
+  "Get desktop file modtime, in list form for desktop format version 208."
+  (setq desktop-file-modtime
+	(time-convert (file-attribute-modification-time
+		       (file-attributes
+			(desktop-full-file-name)))
+		      'list)))
+
 (defvar desktop-var-serdes-funs
   (list (list
 	 'mark-ring
@@ -1221,9 +1229,7 @@ desktop-save
 		(write-region (point-min) (point-max) (desktop-full-file-name) nil 'nomessage))
 	      (setq desktop-file-checksum checksum)
 	      ;; We remember when it was modified (which is presumably just now).
-	      (setq desktop-file-modtime (file-attribute-modification-time
-					  (file-attributes
-					   (desktop-full-file-name)))))))))))
+	      (desktop--get-file-modtime))))))))
 
 ;; ----------------------------------------------------------------------------
 ;;;###autoload
@@ -1332,9 +1338,7 @@ desktop-read
                           'window-configuration-change-hook)))
 	    (desktop-auto-save-disable)
 	    ;; Evaluate desktop buffer and remember when it was modified.
-	    (setq desktop-file-modtime (file-attribute-modification-time
-					(file-attributes
-					 (desktop-full-file-name))))
+	    (desktop--get-file-modtime)
 	    (load (desktop-full-file-name) t t t)
 	    ;; If it wasn't already, mark it as in-use, to bother other
 	    ;; desktop instances.
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
@ 2022-04-27  6:37 Mark Barton
  2022-04-27  7:20 ` Po Lu
  2022-04-27  7:39 ` Paul Eggert
  0 siblings, 2 replies; 20+ messages in thread
From: Mark Barton @ 2022-04-27  6:37 UTC (permalink / raw)
  To: eggert, emacs-devel, emacs-orgmode

The change also breaks org-file-newer-than-p function that triggered the debugger while loading my init that uses org babel. I was able to use the example of the patch that Paul Eggert provided earlier for the desktop-save to add the time-convert to “fix” org-file-newer-than-p as shown below. Not positive that I needed to change it in both places, but it works for me now on macOS Monterey.

modified   lisp/org/org-macs.el
@@ -264,8 +264,8 @@ org-file-newer-than-p
        ;; (e.g. HFS+) do not retain any finer granularity.  As
        ;; a consequence, make sure we return non-nil when the two
        ;; times are equal.
-       (not (time-less-p (cl-subseq (nth 5 (file-attributes file)) 0 2)
-			 (cl-subseq time 0 2)))))
+       (not (time-less-p (cl-subseq (time-convert (nth 5 (file-attributes file)) 'list) 0 2)
+			 (cl-subseq (time-convert time 'list) 0 2)))))




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27  6:37 Mark Barton
@ 2022-04-27  7:20 ` Po Lu
  2022-04-27  7:39 ` Paul Eggert
  1 sibling, 0 replies; 20+ messages in thread
From: Po Lu @ 2022-04-27  7:20 UTC (permalink / raw)
  To: Mark Barton; +Cc: eggert, emacs-devel, emacs-orgmode

Mark Barton <mbarton98@gmail.com> writes:

> The change also breaks org-file-newer-than-p function that triggered
> the debugger while loading my init that uses org babel. I was able to
> use the example of the patch that Paul Eggert provided earlier for the
> desktop-save to add the time-convert to “fix” org-file-newer-than-p as
> shown below. Not positive that I needed to change it in both places,
> but it works for me now on macOS Monterey.
>
> modified   lisp/org/org-macs.el
> @@ -264,8 +264,8 @@ org-file-newer-than-p
>         ;; (e.g. HFS+) do not retain any finer granularity.  As
>         ;; a consequence, make sure we return non-nil when the two
>         ;; times are equal.
> -       (not (time-less-p (cl-subseq (nth 5 (file-attributes file)) 0 2)
> -			 (cl-subseq time 0 2)))))
> +       (not (time-less-p (cl-subseq (time-convert (nth 5 (file-attributes file)) 'list) 0 2)
> +			 (cl-subseq (time-convert time 'list) 0 2)))))

Isn't the fact that changes are needed in multiple pieces of in-tree
code a wake-up call demonstrating it's too early to change the default
time format?



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27  6:37 Mark Barton
  2022-04-27  7:20 ` Po Lu
@ 2022-04-27  7:39 ` Paul Eggert
  2022-04-27 16:55   ` Stefan Monnier
  1 sibling, 1 reply; 20+ messages in thread
From: Paul Eggert @ 2022-04-27  7:39 UTC (permalink / raw)
  To: Mark Barton, emacs-devel, emacs-orgmode

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

Thanks for reporting that. Fixed in Emacs master via the attached.

For the more general issue I'm planning to add a builtin boolean 
variable current-time-list soon, that is t for (HIGH LOW MICROSEC 
PICOSEC) format, nil for (TICKS . HZ) format.

[-- Attachment #2: 0001-Use-org-time-convert-to-integer-instead-of-by-hand.patch --]
[-- Type: text/x-patch, Size: 1623 bytes --]

From 3abb3681b57d7c8ca7fa808addb0a10b6b109cab Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 27 Apr 2022 00:29:26 -0700
Subject: [PATCH] Use org-time-convert-to-integer instead of by hand
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/org/org-macs.el (org-file-newer-than-p):
Don’t assume list-format timestamps, by using
org-time-convert-to-integer instead of doing it by hand.
---
 lisp/org/org-macs.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
index b10725bd52..92591b5bb7 100644
--- a/lisp/org/org-macs.el
+++ b/lisp/org/org-macs.el
@@ -257,15 +257,16 @@ org-fit-window-to-buffer
 
 (defun org-file-newer-than-p (file time)
   "Non-nil if FILE is newer than TIME.
-FILE is a filename, as a string, TIME is a list of integers, as
+FILE is a filename, as a string, TIME is a Lisp time value, as
 returned by, e.g., `current-time'."
   (and (file-exists-p file)
        ;; Only compare times up to whole seconds as some file-systems
        ;; (e.g. HFS+) do not retain any finer granularity.  As
        ;; a consequence, make sure we return non-nil when the two
        ;; times are equal.
-       (not (time-less-p (cl-subseq (nth 5 (file-attributes file)) 0 2)
-			 (cl-subseq time 0 2)))))
+       (not (time-less-p (org-time-convert-to-integer
+			  (nth 5 (file-attributes file)))
+			 (org-time-convert-to-integer time)))))
 
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-26 20:28     ` Paul Eggert
@ 2022-04-27 12:17       ` Lars Ingebrigtsen
  0 siblings, 0 replies; 20+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-27 12:17 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-devel

Paul Eggert <eggert@cs.ucla.edu> writes:

> I asked here when this code was added years ago. I also surveyed code
> in ELPA at the time. As I recall, it appeared that it wouldn't be that
> much of a problem and that any fixes would be simple. (My memory is no
> doubt biased. :-)
>
> The CIDER bug is now fixed
> <https://github.com/clojure-emacs/cider/commit/7506cc4613a5b71e5246fd83de347185b5d49c42>. FWIW
> the unfixed CIDER code didn't always work in Emacs 28 and earlier
> either, though its bugs there were intermittent.

I think there's a high likelihood that users have many snippets of code
out there that are doing maths on the result from `current-time' (which
used a well-defined and documented format), and that code will break.
Packages like CIDER can be fixed, of course, but I think we'll be
putting our users through a lot of pain here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27  7:39 ` Paul Eggert
@ 2022-04-27 16:55   ` Stefan Monnier
  2022-04-28 22:27     ` Paul Eggert
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Monnier @ 2022-04-27 16:55 UTC (permalink / raw)
  To: Paul Eggert; +Cc: Mark Barton, emacs-devel, emacs-orgmode

> -       (not (time-less-p (cl-subseq (nth 5 (file-attributes file)) 0 2)
> -			 (cl-subseq time 0 2)))))
> +       (not (time-less-p (org-time-convert-to-integer
> +			  (nth 5 (file-attributes file)))
> +			 (org-time-convert-to-integer time)))))

Instead of rounding the times to whole seconds, wouldn't it make more
sense to check that the difference is larger than 1s?


        Stefan




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-27 16:55   ` Stefan Monnier
@ 2022-04-28 22:27     ` Paul Eggert
  2022-04-29 14:22       ` Max Nikulin
  0 siblings, 1 reply; 20+ messages in thread
From: Paul Eggert @ 2022-04-28 22:27 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Mark Barton, emacs-orgmode, emacs-devel

On 4/27/22 09:55, Stefan Monnier wrote:
> Instead of rounding the times to whole seconds, wouldn't it make more
> sense to check that the difference is larger than 1s?

org-file-newer-than-p is intended to work on filesystems like HFS+ that 
store just the seconds part of the last-modified time. Since these 
filesystems take the floor of the system time, taking the floor should 
be the most-accurate way to work around timestamp truncation issues, 
where comparing one timestamp that comes from an HFS+ filesystem to 
another timestamp coming from some other source (which is how 
org-file-newer-than-p is used).

The code won't work as desired on filesystems like FAT where the 
last-modified time has only 2-second resolution. Ideally Emacs Lisp code 
would have access to file timestamp resolution but that's not something 
it has now, so I merely preserved org-file-newer-than-p's assumption 
that taking the floor is good enough.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-28 22:27     ` Paul Eggert
@ 2022-04-29 14:22       ` Max Nikulin
  2022-04-29 18:10         ` Paul Eggert
  0 siblings, 1 reply; 20+ messages in thread
From: Max Nikulin @ 2022-04-29 14:22 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-orgmode, emacs-devel

On 29/04/2022 05:27, Paul Eggert wrote:
> On 4/27/22 09:55, Stefan Monnier wrote:
>> Instead of rounding the times to whole seconds, wouldn't it make more
>> sense to check that the difference is larger than 1s?
> 
> org-file-newer-than-p is intended to work on filesystems like HFS+ that 
> store just the seconds part of the last-modified time.

I have found just 2 calls of `org-file-newer-than-p' in the Org code and 
in both cases the intention is to check whether particular file has been 
updated. I have not checked Org extensions for usage of this function. I 
would rather assume that the code was written without any considerations 
concerning filesystem timestamps precision and its difference from 
`current-time' representation. It was still working in most real-life cases.

 From my point of view, it is better to rewrite `org-compile-time' to 
treat the case when there were no file prior to the call as that the 
file has been updated without comparison of timestamps, so 
`current-time' can be dropped to eliminate comparison of timestamp from 
different sources. With such modification it is better to compare file 
timestamps without truncation to whole seconds, however I have not tried 
to create an example where fractional seconds may change behavior.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-29 14:22       ` Max Nikulin
@ 2022-04-29 18:10         ` Paul Eggert
  2022-04-30 10:56           ` Max Nikulin
  0 siblings, 1 reply; 20+ messages in thread
From: Paul Eggert @ 2022-04-29 18:10 UTC (permalink / raw)
  To: Max Nikulin; +Cc: emacs-orgmode, emacs-devel

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

On 4/29/22 07:22, Max Nikulin wrote:

> It was still working in most real-life cases.

Yes, the current code breaks only in fine-grained cases. Most of the 
time it'll work fine since people rarely compile the same file twice in 
the same second.

>  From my point of view, it is better to rewrite `org-compile-time' to 
> treat the case when there were no file prior to the call as that the 
> file has been updated without comparison of timestamps

Yes, that sounds simpler and better. How about the attached patch?

[-- Attachment #2: 0001-Improve-org-compile-file-timestamp-handling.patch --]
[-- Type: text/x-patch, Size: 2512 bytes --]

From fbd6561952acf359236afcf7957a197376a18c66 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 Apr 2022 11:06:00 -0700
Subject: [PATCH] Improve org-compile-file timestamp handling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/org/org-macs.el (org-file-newer-than-p):
Don’t lose timestamp information in an attempt to work around
problems on filesystems with coarse-grained timestamps.
(org-compile-file): Use only filesystem timestamps;
don’t try to compare them to the current time, as
the filesystem clock may be different from our clock.
---
 lisp/org/org-macs.el | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
index bb0562dde0..907043580a 100644
--- a/lisp/org/org-macs.el
+++ b/lisp/org/org-macs.el
@@ -260,14 +260,8 @@ org-file-newer-than-p
   "Non-nil if FILE is newer than TIME.
 FILE is a filename, as a string, TIME is a Lisp time value, as
 returned by, e.g., `current-time'."
-  (and (file-exists-p file)
-       ;; Only compare times up to whole seconds as some file-systems
-       ;; (e.g. HFS+) do not retain any finer granularity.  As
-       ;; a consequence, make sure we return non-nil when the two
-       ;; times are equal.
-       (not (time-less-p (org-time-convert-to-integer
-			  (nth 5 (file-attributes file)))
-			 (org-time-convert-to-integer time)))))
+  (when-let ((mtime (file-attribute-modification-time (file-attributes file))))
+    (time-less-p time mtime)))
 
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
@@ -301,7 +295,7 @@ org-compile-file
 	 (full-name (file-truename source))
 	 (out-dir (or (file-name-directory source) "./"))
 	 (output (expand-file-name (concat base-name "." ext) out-dir))
-	 (time (current-time))
+	 (time (file-attribute-modification-time (file-attributes output)))
 	 (err-msg (if (stringp err-msg) (concat ".  " err-msg) "")))
     (save-window-excursion
       (pcase process
@@ -320,7 +314,7 @@ org-compile-file
 	(_ (error "No valid command to process %S%s" source err-msg))))
     ;; Check for process failure.  Output file is expected to be
     ;; located in the same directory as SOURCE.
-    (unless (org-file-newer-than-p output time)
+    (unless (or (not time) (org-file-newer-than-p output time))
       (error (format "File %S wasn't produced%s" output err-msg)))
     output))
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
  2022-04-29 18:10         ` Paul Eggert
@ 2022-04-30 10:56           ` Max Nikulin
  0 siblings, 0 replies; 20+ messages in thread
From: Max Nikulin @ 2022-04-30 10:56 UTC (permalink / raw)
  To: Paul Eggert; +Cc: emacs-orgmode, emacs-devel

On 30/04/2022 01:10, Paul Eggert wrote:
> On 4/29/22 07:22, Max Nikulin wrote:
> 
>>  From my point of view, it is better to rewrite `org-compile-time' to 
>> treat the case when there were no file prior to the call as that the 
>> file has been updated without comparison of timestamps
> 
> Yes, that sounds simpler and better. How about the attached patch?

Thank you, Paul. I am glad to see that you agree with my idea.

> From: Paul Eggert
> Date: Fri, 29 Apr 2022 11:06:00 -0700
> Subject: [PATCH] Improve org-compile-file timestamp handling

> diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
> index bb0562dde0..907043580a 100644
> --- a/lisp/org/org-macs.el
> +++ b/lisp/org/org-macs.el

It should be committed to the "main" branch of the Org repository as 
well. I am unsure if all Emacs developers have commit permission however.

> @@ -260,14 +260,8 @@ org-file-newer-than-p
>    "Non-nil if FILE is newer than TIME.
>  FILE is a filename, as a string, TIME is a Lisp time value, as
>  returned by, e.g., `current-time'."

It is not true any more, it is expected that TIME is obtained using 
`file-attributes'.

I would consider replacing docstring with something like:

Non-nil if FILE modification time is greater than or equal to TIME.
TIME should be obtained from the return value of the `file-attributes'
function.  If TIME is nil (file did not exist)
then any existing FILE is considered as a newer one.
This function may give false positive for file systems with coarse
timestamp resolution such as 1 second for HFS+ or 2 seconds for FAT.
File timestamp and system clock may have different resolution,
so attempts to compare them may give unexpected results.

> -  (and (file-exists-p file)
> -       ;; Only compare times up to whole seconds as some file-systems
> -       ;; (e.g. HFS+) do not retain any finer granularity.  As
> -       ;; a consequence, make sure we return non-nil when the two
> -       ;; times are equal.
> -       (not (time-less-p (org-time-convert-to-integer
> -			  (nth 5 (file-attributes file)))
> -			 (org-time-convert-to-integer time)))))
> +  (when-let ((mtime (file-attribute-modification-time (file-attributes file))))
> +    (time-less-p time mtime)))

org-macs.el does not contain (require 'subr-x) thus `when-let' will 
cause a warning on Emacs-26.

`file-attribute-modification-time' makes code clearer, but it causes 
some complications. Formally compatibility with Emacs-25 (e.g. 
ubuntu-18.04 LTS bionic) is not required for the "main" branch. Emacs 
sources have the "bugfix" Org branch of the stable release though. The 
latter still supports Emacs-25, so either the Emacs source tree and the 
Org bugfix branch will diverge at this point or it is safer to avoid 
`file-attribute-modification-time' till the next major Org release. 
Maybe Org maintainers and developers will correct me.

Likely you already guessed from the suggested docscring that I would prefer

     (and mtime (not (and time (time-less-p mtime time))))

to keep existing behavior on HFS+ and to allow nil for TIME.

>  (defun org-compile-file (source process ext &optional err-msg log-buf spec)
>    "Compile a SOURCE file using PROCESS.
> @@ -301,7 +295,7 @@ org-compile-file
>  	 (full-name (file-truename source))
>  	 (out-dir (or (file-name-directory source) "./"))
>  	 (output (expand-file-name (concat base-name "." ext) out-dir))
> -	 (time (current-time))
> +	 (time (file-attribute-modification-time (file-attributes output)))
>  	 (err-msg (if (stringp err-msg) (concat ".  " err-msg) "")))
>      (save-window-excursion
>        (pcase process
> @@ -320,7 +314,7 @@ org-compile-file
>  	(_ (error "No valid command to process %S%s" source err-msg))))
>      ;; Check for process failure.  Output file is expected to be
>      ;; located in the same directory as SOURCE.
> -    (unless (org-file-newer-than-p output time)
> +    (unless (or (not time) (org-file-newer-than-p output time))
>        (error (format "File %S wasn't produced%s" output err-msg)))
>      output))

I am considering handling of (not time) case inside 
`org-file-newer-than-p` as a more robust approach.



^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-04-30 10:56 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <165091562900.12167.2356992007535632125@vcs2.savannah.gnu.org>
     [not found] ` <20220425194030.030A8C01687@vcs2.savannah.gnu.org>
2022-04-26  9:38   ` master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Lars Ingebrigtsen
2022-04-26 10:20     ` Manuel Uberti
2022-04-26 10:38       ` Po Lu
2022-04-26 14:42         ` Bozhidar Batsov
2022-04-26 20:29         ` Paul Eggert
2022-04-27  0:36           ` Po Lu
2022-04-27  1:18             ` Paul Eggert
2022-04-27  1:29               ` Po Lu
2022-04-27  4:10                 ` Paul Eggert
2022-04-26 20:28     ` Paul Eggert
2022-04-27 12:17       ` Lars Ingebrigtsen
2022-04-26 23:54     ` Michael Heerdegen
2022-04-27  6:37 Mark Barton
2022-04-27  7:20 ` Po Lu
2022-04-27  7:39 ` Paul Eggert
2022-04-27 16:55   ` Stefan Monnier
2022-04-28 22:27     ` Paul Eggert
2022-04-29 14:22       ` Max Nikulin
2022-04-29 18:10         ` Paul Eggert
2022-04-30 10:56           ` Max Nikulin

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).