bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer <mkupfer@alum.berkeley.edu>]

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

Gustavo Barros wrote:

> On Mon, 31 Oct 2022 at 09:49, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > What is the expected semantics of moving a symlink to trashcan?  Is it
> > supposed to move the symlink or its target?  (I'd think it's the
> > former, but maybe my instincts are wrong.)  If the expectations are
> > that the symlink is moved, then all we need to do is to treat symlinks
> > as regular files, by augmenting file-directory-p not to dupe us.
> 
> I'm not sure either, but my instincts are the same as yours. If that's
> any reference, I just tested here, and that's what "gio trash" does
> (moves the symlink, not the target).

Yes, I tried a few graphical file browsers (Thunar, Caja, and Dolphin),
and they all move the symlink, not the target, to Trash.

After getting my test setup straightened out, I think I have a fix for
the symlink issue and for the issue that Gustavo originally reported
(cross-filesystem trashing fails when there's already a directory with
the same name in Trash).

I've committed these fixes separately; see attached.  Gustavo, can you
try these out and make sure they handle your use case(s)?

> > I'm okay with filing another bug report about rename-file, and
> > discussing this there.  But that's a separate issue, and fix of this
> > bug should not depend on that.
> 
> Understood.

Okay.  I'm not planning to follow up on this, Gustavo, so if you'd like
to lobby for a change to rename-file, you'll need to open a bug for it
(if you haven't already).

cheers,
mike

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix duplicate trashing --]
[-- Type: text/x-diff, Size: 2280 bytes --]

From f749a1dd7875a83e6415d8c512c5659f0f23c834 Mon Sep 17 00:00:00 2001
From: Mike Kupfer <mkupfer@alum.berkeley.edu>
Date: Sun, 30 Oct 2022 10:31:11 -0700
Subject: [PATCH 1/2] Fix cross-filesystem directory trashing (Bug#58721)

* lisp/files.el (move-file-to-trash): When trashing a directory with
the same name as something that's already in the trash, copy it into
the trash folder and then delete it, rather than using rename-file.
---
 lisp/files.el | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index a2825322580..de4f68ebd51 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8596,10 +8596,27 @@ move-file-to-trash
 		    (setq files-base (substring (file-name-nondirectory info-fn)
                                                 0 (- (length ".trashinfo"))))
 		    (write-region nil nil info-fn nil 'quiet info-fn)))
-		 ;; Finally, try to move the file to the trashcan.
+		 ;; Finally, try to move the item to the trashcan.  If
+                 ;; it's a file, just move it.  Things are more
+                 ;; complicated for directories.  If the target
+                 ;; directory already exists (due to uniquification)
+                 ;; and the trash directory is in a different
+                 ;; filesystem, rename-file will error out, even when
+                 ;; 'overwrite' is non-nil.  Rather than worry about
+                 ;; whether we're crossing filesystems, just check if
+                 ;; we've moving a directory and the target directory
+                 ;; already exists.  That handles both the
+                 ;; same-filesystem and cross-filesystem cases.
 		 (let ((delete-by-moving-to-trash nil)
 		       (new-fn (file-name-concat trash-files-dir files-base)))
-		   (rename-file fn new-fn overwrite)))))))))
+                   (if (or (not is-directory)
+                           (not (file-exists-p new-fn)))
+                       (rename-file fn new-fn overwrite)
+                     (copy-directory fn
+                                     (file-name-as-directory new-fn)
+                                     t nil t)
+                     (delete-directory fn t))))))))))
+
 
 \f
 (defsubst file-attribute-type (attributes)
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: fix trashing of symlink --]
[-- Type: text/x-diff, Size: 1004 bytes --]

From 4aefb5b5964e1aaad0c74238e14ad0a1e3dc4e7a Mon Sep 17 00:00:00 2001
From: Mike Kupfer <mkupfer@alum.berkeley.edu>
Date: Sun, 20 Nov 2022 16:44:20 -0800
Subject: [PATCH 2/2] Fix trashing of symlink that points at a directory

* lisp/files.el (move-file-to-trash): Redefine is-directory so
that it is false for symlinks.
---
 lisp/files.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index de4f68ebd51..a78d1627689 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8566,7 +8566,8 @@ move-file-to-trash
 
 	       ;; Make a .trashinfo file.  Use O_EXCL, as per trash-spec 1.0.
 	       (let* ((files-base (file-name-nondirectory fn))
-                      (is-directory (file-directory-p fn))
+                      (is-directory (and (file-directory-p fn)
+					 (not (file-symlink-p fn))))
                       (overwrite nil)
                       info-fn)
                  ;; We're checking further down whether the info file
-- 
2.30.2