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