bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

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

The ediff lefts temporary files "/tmp/fineDiff*" when kill emacs during an ediff comparing.

Reproducing steps:

1. echo 0 > /tmp/0; echo 1 > /tmp/1

2. emacs -nw -q -f ediff-files-command /tmp/0 /tmp/1 

on the ediff control buffer, press "n" to move to first difference; there should have two temp files like /tmp/fineDiffA* and /tmp/fineDiffB* were generated by ediff, stay on the ediff buffer (do not press "q" to quit the ediff session). then

3. Press "C-x C-c" or kill-emacs to quit the emacs directly.

Then two temp files /tmp/fineDiff* were left there.

This patch will make sure the temp files be removed when kill-emacs without quitting the ediff session.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-vc-ediff-util.el-Remove-temp-files-on-kill-emac.patch --]
[-- Type: text/x-patch; name="0001-lisp-vc-ediff-util.el-Remove-temp-files-on-kill-emac.patch", Size: 1195 bytes --]

From dee9750144df6cdd827397f3d97e74f56e25e1fa Mon Sep 17 00:00:00 2001
From: Lin Sun <sunlin7@hotmail.com>
Date: Sun, 15 Dec 2024 06:52:17 +0000
Subject: [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

---
 lisp/vc/ediff-util.el | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 6038f3eae30..fb47b3f7563 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -213,6 +213,12 @@ ediff-setup-keymap
   (fset 'ediff-mode-map ediff-mode-map)
   (run-hooks 'ediff-keymap-setup-hook))
 
+(defun ediff--delete-temp-files-on-kill-emacs ()
+  "Delete the temp-files associated with the ediff buffers."
+  (dolist (b (buffer-list))
+    (with-current-buffer b
+      (when (eq major-mode 'ediff-mode)
+        (ediff-delete-temp-files)))))
 
 ;;; Setup functions
 
@@ -488,6 +494,7 @@ ediff-setup
       (if (ediff-buffer-live-p ediff-meta-buffer)
 	  (ediff-update-meta-buffer
 	   ediff-meta-buffer nil ediff-meta-session-number))
+      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
       (run-hooks 'ediff-startup-hook)
       ) ; eval in control-buffer
     control-buffer))
-- 
2.34.1

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Eli Zaretskii]

> From: Lin Sun <sunlin7@hotmail.com>
> Date: Sun, 15 Dec 2024 05:59:40 +0000
> 
> @@ -488,6 +494,7 @@ ediff-setup
>        (if (ediff-buffer-live-p ediff-meta-buffer)
>  	  (ediff-update-meta-buffer
>  	   ediff-meta-buffer nil ediff-meta-session-number))
> +      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
>        (run-hooks 'ediff-startup-hook)
>        ) ; eval in control-buffer
>      control-buffer))

This should be carefully programmed to avoid preventing Emacs from
exiting due to some problem.  If ediff-delete-temp-files or one of the
functions it calls can signal an error, it should be wrapped by
condition-case, and if it or one of its callees can try interacting
with the user, we should use kill-emacs-query-functions instead.

Alternatively, we could end the Ediff session when Emacs is killed.

Bottom line: this is a minor cleanup feature, so we should be very
careful not to cause any regressions and problems just because we want
to exit more cleanly.  (On most systems, files in /tmp are routinely
deleted by system's cleanup processes anyway.)

Thanks.

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Sunday, December 15, 2024 12:04 AM
> > From: Lin Sun <sunlin7@hotmail.com>
> > Date: Sun, 15 Dec 2024 05:59:40 +0000
> >
> > @@ -488,6 +494,7 @@ ediff-setup
> >        (if (ediff-buffer-live-p ediff-meta-buffer)
> >          (ediff-update-meta-buffer
> >           ediff-meta-buffer nil ediff-meta-session-number))
> > +      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
> >        (run-hooks 'ediff-startup-hook)
> >        ) ; eval in control-buffer
> >      control-buffer))
>
> This should be carefully programmed to avoid preventing Emacs from
> exiting due to some problem.  If ediff-delete-temp-files or one of the
> functions it calls can signal an error, it should be wrapped by
> condition-case, and if it or one of its callees can try interacting
> with the user, we should use kill-emacs-query-functions instead.

The function ediff-delete-temp-files was called at the tail of Ediff  quit routine, 
will be also safe on kill-emacs routine, and can confirm it dose not interactive 
with the user. Actually it deletes the temp files created by Ediff-mode, should 
has full privilege to do its job.

> Alternatively, we could end the Ediff session when Emacs is killed.

It maybe heavy to end the Ediff session if the user choose 
`ediff-setup-windows-plain` as the `ediff-window-setup-function` for a graphic 
frame, then ending a Ediff session will trigger emacs graphic frames layout change 
(Ediff will restore frames layout to the one before its startup).  So here we just try 
clean up the temp files to avoid the heavy works.

> Bottom line: this is a minor cleanup feature, so we should be very
> careful not to cause any regressions and problems just because we want
> to exit more cleanly.  (On most systems, files in /tmp are routinely
> deleted by system's cleanup processes anyway.)

Agree and calling the ediff-delete-temp-files should only for the scenario 
that user kill emacs during an Ediff-session, otherwise it will do nothing. 

Thank you.

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

Hi Eli,

I checked the "ediff-delete-temp-files", it should be safe for executing.  Is it more safe to wrap it with `ignore-errors` ? Like: 

+(defun ediff--delete-temp-files-on-kill-emacs ()
+  "Delete the temp-files associated with the ediff buffers."
+  (dolist (b (buffer-list))
+    (with-current-buffer b
+      (when (eq major-mode 'ediff-mode)
+        (ignore-errors (ediff-delete-temp-files))))))

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Eli Zaretskii]

> From: Lin Sun <sunlin7@hotmail.com>
> CC: "74881@debbugs.gnu.org" <74881@debbugs.gnu.org>
> Date: Fri, 27 Dec 2024 05:38:57 +0000
> 
> Hi Eli,
> 
> I checked the "ediff-delete-temp-files", it should be safe for executing.  Is it more safe to wrap it with `ignore-errors` ? Like: 
> 
> +(defun ediff--delete-temp-files-on-kill-emacs ()
> +  "Delete the temp-files associated with the ediff buffers."
> +  (dolist (b (buffer-list))
> +    (with-current-buffer b
> +      (when (eq major-mode 'ediff-mode)
> +        (ignore-errors (ediff-delete-temp-files))))))
> 

Yes, probably.  For a good measure, I'd also bind inhibit-interaction
to a non-nil value, to make sure we never ever ask the user anything
inside ediff-delete-temp-files.

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

[-- Attachment #1.1: Type: text/plain, Size: 330 bytes --]

> From: Eli Zaretskii <eliz@gnu.org>
> Yes, probably.  For a good measure, I'd also bind inhibit-interaction
> to a non-nil value, to make sure we never ever ask the user anything
> inside ediff-delete-temp-files.

Sure, that's will be more reliable. I had attached the modified patch, please help review again. Thank you !

[-- Attachment #1.2: Type: text/html, Size: 1168 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-vc-ediff-util.el-Remove-temp-files-on-kill-emac.patch --]
[-- Type: text/x-patch; name="0001-lisp-vc-ediff-util.el-Remove-temp-files-on-kill-emac.patch", Size: 1271 bytes --]

From 39b23f1b3f3d58569b7fa4742e8fd24e2cc7071f Mon Sep 17 00:00:00 2001
From: Lin Sun <sunlin7@hotmail.com>
Date: Sun, 15 Dec 2024 06:52:17 +0000
Subject: [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

---
 lisp/vc/ediff-util.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 6038f3eae30..e46dc70218b 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -213,6 +213,14 @@ ediff-setup-keymap
   (fset 'ediff-mode-map ediff-mode-map)
   (run-hooks 'ediff-keymap-setup-hook))
 
+(defun ediff--delete-temp-files-on-kill-emacs ()
+  "Delete the temp-files associated with the ediff buffers."
+  (let ((inhibit-interaction nil))
+    (ignore-errors
+      (dolist (b (buffer-list))
+        (with-current-buffer b
+          (when (eq major-mode 'ediff-mode)
+            (ediff-delete-temp-files)))))))
 
 ;;; Setup functions
 
@@ -488,6 +496,7 @@ ediff-setup
       (if (ediff-buffer-live-p ediff-meta-buffer)
 	  (ediff-update-meta-buffer
 	   ediff-meta-buffer nil ediff-meta-session-number))
+      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
       (run-hooks 'ediff-startup-hook)
       ) ; eval in control-buffer
     control-buffer))
-- 
2.34.1

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Eli Zaretskii]

> From: Lin Sun <sunlin7@hotmail.com>
> CC: "74881@debbugs.gnu.org" <74881@debbugs.gnu.org>
> Date: Fri, 27 Dec 2024 17:34:31 +0000
> 
> > From: Eli Zaretskii <eliz@gnu.org>
> > Yes, probably.  For a good measure, I'd also bind inhibit-interaction
> > to a non-nil value, to make sure we never ever ask the user anything
> > inside ediff-delete-temp-files.
> 
> Sure, that's will be more reliable. I had attached the modified patch, please help review again. Thank you !
> 
> From 39b23f1b3f3d58569b7fa4742e8fd24e2cc7071f Mon Sep 17 00:00:00 2001
> From: Lin Sun <sunlin7@hotmail.com>
> Date: Sun, 15 Dec 2024 06:52:17 +0000
> Subject: [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs
> 
> ---
>  lisp/vc/ediff-util.el | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
> index 6038f3eae30..e46dc70218b 100644
> --- a/lisp/vc/ediff-util.el
> +++ b/lisp/vc/ediff-util.el
> @@ -213,6 +213,14 @@ ediff-setup-keymap
>    (fset 'ediff-mode-map ediff-mode-map)
>    (run-hooks 'ediff-keymap-setup-hook))
>  
> +(defun ediff--delete-temp-files-on-kill-emacs ()
> +  "Delete the temp-files associated with the ediff buffers."
> +  (let ((inhibit-interaction nil))
                                ^^^
This should be t, not nil.

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

[-- Attachment #1.1: Type: text/plain, Size: 371 bytes --]

> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Friday, December 27, 2024 11:38 PM
> > +  (let ((inhibit-interaction nil))
>                                 ^^^
> This should be t, not nil.

My apologies for posted the testing code (I toggled t/nil to test the behavior on my local).

And I attached a new version of the patch, please help review again. Thank you!

[-- Attachment #1.2: Type: text/html, Size: 1657 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-vc-ediff-util.el-Remove-temp-files-on-kill-emac.patch --]
[-- Type: text/x-patch; name="0001-lisp-vc-ediff-util.el-Remove-temp-files-on-kill-emac.patch", Size: 1269 bytes --]

From 254b06993363f20f82b5f2916e890e30a492ba45 Mon Sep 17 00:00:00 2001
From: Lin Sun <sunlin7@hotmail.com>
Date: Sun, 15 Dec 2024 06:52:17 +0000
Subject: [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

---
 lisp/vc/ediff-util.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 6038f3eae30..87d6f7c4aec 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -213,6 +213,14 @@ ediff-setup-keymap
   (fset 'ediff-mode-map ediff-mode-map)
   (run-hooks 'ediff-keymap-setup-hook))
 
+(defun ediff--delete-temp-files-on-kill-emacs ()
+  "Delete the temp-files associated with the ediff buffers."
+  (ignore-errors
+    (let ((inhibit-interaction t))
+      (dolist (b (buffer-list))
+        (with-current-buffer b
+          (when (eq major-mode 'ediff-mode)
+            (ediff-delete-temp-files)))))))
 
 ;;; Setup functions
 
@@ -488,6 +496,7 @@ ediff-setup
       (if (ediff-buffer-live-p ediff-meta-buffer)
 	  (ediff-update-meta-buffer
 	   ediff-meta-buffer nil ediff-meta-session-number))
+      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
       (run-hooks 'ediff-startup-hook)
       ) ; eval in control-buffer
     control-buffer))
-- 
2.34.1

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Eli Zaretskii]

> From: Lin Sun <sunlin7@hotmail.com>
> CC: "74881@debbugs.gnu.org" <74881@debbugs.gnu.org>
> Date: Sat, 28 Dec 2024 08:04:52 +0000
> 
> > From: Eli Zaretskii <eliz@gnu.org>
> > Sent: Friday, December 27, 2024 11:38 PM
> > > +  (let ((inhibit-interaction nil))
> >                                 ^^^
> > This should be t, not nil.
> 
> My apologies for posted the testing code (I toggled t/nil to test the behavior on my local).
> 
> And I attached a new version of the patch, please help review again. Thank you!

Thanks, installed on the master branch, and closing the bug.

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

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

> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Saturday, December 28, 2024 04:41 AM
>
> Thanks, installed on the master branch, and closing the bug.

Thank you !

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

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Michael Heerdegen via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

I'm a bit late, feel free to ignore may comments if you think it's not
worth the trouble.

Lin Sun <sunlin7@hotmail.com> writes:

> +(defun ediff--delete-temp-files-on-kill-emacs ()
> +  "Delete the temp-files associated with the ediff buffers."
> +  (ignore-errors
> +    (let ((inhibit-interaction t))
> +      (dolist (b (buffer-list))
> +        (with-current-buffer b
> +          (when (eq major-mode 'ediff-mode)
> +            (ediff-delete-temp-files)))))))


I think this is the same as mapping over `ediff-session-registry' which
should hold exactly the list of buffers we want.

Second: Can we move the `ignore-errors' inwards so that an error in one
case doesn't abort the complete loop?


>  ;;; Setup functions
>
> @@ -488,6 +496,7 @@ ediff-setup
>        (if (ediff-buffer-live-p ediff-meta-buffer)
>  	  (ediff-update-meta-buffer
>  	   ediff-meta-buffer nil ediff-meta-session-number))
> +      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)

Would it be ok to avoid the above loop completely by using the buffer
local version of `kill-emacs-hook' instead?


Michael.

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Lin Sun]

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

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Sent: Saturday, December 28, 2024 05:49 PM
> > +(defun ediff--delete-temp-files-on-kill-emacs ()
> > +  "Delete the temp-files associated with the ediff buffers."
> > +  (ignore-errors
> > +    (let ((inhibit-interaction t))
> > +      (dolist (b (buffer-list))
> > +        (with-current-buffer b
> > +          (when (eq major-mode 'ediff-mode)
> > +            (ediff-delete-temp-files)))))))
> I think this is the same as mapping over `ediff-session-registry' which
>  should hold exactly the list of buffers we want.
>  Second: Can we move the `ignore-errors' inwards so that an error in one
> case doesn't abort the complete loop?

Sure, I had attached the modified one towards your comments.

> >  ;;; Setup functions
> >
> > @@ -488,6 +496,7 @@ ediff-setup
> >        (if (ediff-buffer-live-p ediff-meta-buffer)
> >          (ediff-update-meta-buffer
> >           ediff-meta-buffer nil ediff-meta-session-number))
> > +      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
> Would it be ok to avoid the above loop completely by using the buffer
>  local version of `kill-emacs-hook' instead?

I didn't found a local version of `kill-emacs-hook` example in emacs git repo. 
Current version also work for the killing from command line, for example executing a "pkill emacs" during ediff session.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-vc-ediff-util.el-Loop-the-ediff-session-registr.patch --]
[-- Type: text/x-patch; name="0001-lisp-vc-ediff-util.el-Loop-the-ediff-session-registr.patch", Size: 1121 bytes --]

From 5d21b65eaa45c996182ce3181b1289ed03ff1c26 Mon Sep 17 00:00:00 2001
From: Lin Sun <sunlin7@hotmail.com>
Date: Sun, 29 Dec 2024 06:55:01 +0000
Subject: [PATCH] * lisp/vc/ediff-util.el: Loop the ediff-session-registry
 instead of buffers

---
 lisp/vc/ediff-util.el | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index 33666535843..d448973b345 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -219,12 +219,11 @@ ediff--delete-temp-files-on-kill-emacs
   ;; where this hook could prevent kill-emacs from shutting down Emacs,
   ;; because user interaction is not possible (e.g., in a daemon), or
   ;; if deleting these files signals an error.
-  (ignore-errors
-    (let ((inhibit-interaction t))
-      (dolist (b (buffer-list))
+  (let ((inhibit-interaction t))
+    (dolist (b ediff-session-registry)
+      (ignore-errors
         (with-current-buffer b
-          (when (eq major-mode 'ediff-mode)
-            (ediff-delete-temp-files)))))))
+          (ediff-delete-temp-files))))))
 
 ;;; Setup functions
 
-- 
2.34.1

bug#74881: 31.0.50; [PATCH] * lisp/vc/ediff-util.el: Remove temp files on kill-emacs

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  "74881@debbugs.gnu.org"
>  <74881@debbugs.gnu.org>
> Date: Sun, 29 Dec 2024 02:49:14 +0100
> 
> Second: Can we move the `ignore-errors' inwards so that an error in one
> case doesn't abort the complete loop?

Binding inhibit-interaction non-nil could cause the body signal an
error, so this should be taken into account if we move ignore-errors
inwards.  This code must NOT signal any errors, ever.  Which AFAIU
means that if we want ignore-errors not to abort the rest of the loop
(why not, btw? these are just temporary files, after all), we should
redesign this loop such that it catches the errors and continues with
the rest of the buffers.

> >  ;;; Setup functions
> >
> > @@ -488,6 +496,7 @@ ediff-setup
> >        (if (ediff-buffer-live-p ediff-meta-buffer)
> >  	  (ediff-update-meta-buffer
> >  	   ediff-meta-buffer nil ediff-meta-session-number))
> > +      (add-hook 'kill-emacs-hook #'ediff--delete-temp-files-on-kill-emacs)
> 
> Would it be ok to avoid the above loop completely by using the buffer
> local version of `kill-emacs-hook' instead?

You assume that the temporary Ediff files are always visited in some
buffer?  Is that assumption true?