Subject: Re: bug#5436: 23.1.91; Deleting directories causes unusable file layout in freedesktop trashcan

From: David De La Harpe Golden <[email-masked]>
Date: Sun, 24 Jan 2010 20:19:32 +0000

Eli Zaretskii wrote:
>> Date: Sun, 24 Jan 2010 04:14:10 +0000
>> From: David De La Harpe Golden <[email-masked]>
>> Cc: Tassilo Horn <[email-masked]>, [email-masked]
>>
>> Please find attached small* patch that should address this (and Bug#3353).
> 
> Thanks.
> 
>> -	   ;; We do not want to delete "." and "..".
> 
> This comment was not added to the new code.

The comment was not appropriate - Beware the same test is for a 
different reason in the new bit of code.  Anyway, I've tried to rephrase 
(and, yes, fill) the new comment explaining that better in the attached.

>> +      (if delete-by-moving-to-trash
>> +          (if (and (not recursive)
>> +                   (directory-files
>> +                    directory 'full directory-files-no-dot-files-regexp))
>> +              (error "Directory is not empty, not moving to trash.")
>> +            (move-file-to-trash directory))
 >
> Why error out here? 

Again, specifically because the non- delete-by-moving-to-trash case does 
  - delete-directory only deletes non-empty directories if you ask for a 
recursive delete, otherwise it will error out if the directory is 
non-empty.  That may not be completely obvious from the lisp code since 
it happens when delete-directory-internal calls rename() in the 
non-trash case.

I wanted to avoid inconsistent behaviour between the two cases, which is 
what the comment was documenting in fact, though it obviously wasn't
clear enough the first time around.

I for one _really_ don't like the idea of delete-directory acting 
differently in the trash and non-trash cases, although i suppose it is 
slightly "safer" in the trash case. Still, I would strongly recommend 
consistency between the two cases.

Going the other way to achieve consistency, "fixing" the 
non-delete-by-moving-to-trash case to successfully recursively delete 
even when a recursive delete was not requested at all seems ...not a 
good idea...

> No need for braces if there's only one line in the clause.

Yeah, something in C that I tend to dislike.  Nonetheless, you're of 
course correct for emacs source code conventions, fixed.

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: [email-masked]
# target_branch: http://bzr.savannah.gnu.org/r/emacs/trunk/
# testament_sha1: 5549a39778e154134d8a775f7ecf6b930e588062
# timestamp: 2010-01-24 20:17:31 +0000
# base_revision_id: [email-masked]
# 
# Begin patch
=== modified file 'lisp/files.el'
--- lisp/files.el	2010-01-17 02:25:53 +0000
+++ lisp/files.el	2010-01-24 20:11:15 +0000
@@ -4667,19 +4667,35 @@
   (let ((handler (find-file-name-handler directory 'delete-directory)))
     (if handler
 	(funcall handler 'delete-directory directory recursive)
-      (if (and recursive (not (file-symlink-p directory)))
-	  (mapc
-	   (lambda (file)
-	     ;; This test is equivalent to
-	     ;; (and (file-directory-p fn) (not (file-symlink-p fn)))
-	     ;; but more efficient
-	     (if (eq t (car (file-attributes file)))
-		 (delete-directory file recursive)
-	       (delete-file file)))
-	   ;; We do not want to delete "." and "..".
-	   (directory-files
-	    directory 'full directory-files-no-dot-files-regexp)))
-      (delete-directory-internal directory))))
+      (if delete-by-moving-to-trash
+          ;; To mimic the non- delete-by-moving-to-trash case, which
+          ;; will (sensibly) fail (in delete-directory-internal) if
+          ;; the directory is not empty and recursion wasn't
+          ;; requested, only move non-empty directories to trash if
+          ;; recursive deletion was requested.  As move-file-to-trash
+          ;; moves directories, empty or otherwise, into the trash as
+          ;; a unit, we do not need to recurse ourselves here.
+          (if (and (not recursive)
+                   ;; Check if directory is empty apart from "." and "..".
+                   (directory-files
+                    directory 'full directory-files-no-dot-files-regexp))
+              (error "Directory is not empty, not moving to trash.")
+            (move-file-to-trash directory))
+        ;; Recurse ourselves since we're not moving to trash.
+        (progn
+          (if (and recursive (not (file-symlink-p directory)))
+              (mapc
+               (lambda (file)
+                 ;; This test is equivalent to
+                 ;; (and (file-directory-p fn) (not (file-symlink-p fn)))
+                 ;; but more efficient
+                 (if (eq t (car (file-attributes file)))
+                     (delete-directory file recursive)
+                   (delete-file file)))
+               ;; We do not want to delete "." and "..".
+               (directory-files
+                directory 'full directory-files-no-dot-files-regexp)))
+          (delete-directory-internal directory))))))
 
 (defun copy-directory (directory newname &optional keep-time parents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
@@ -6170,7 +6186,8 @@
   "Move the file (or directory) named FILENAME to the trash.
 When `delete-by-moving-to-trash' is non-nil, this function is
 called by `delete-file' and `delete-directory' instead of
-deleting files outright.
+deleting files outright.  If FILENAME is a directory, it may
+be empty or non-empty.
 
 If the function `system-move-file-to-trash' is defined, call it
  with FILENAME as an argument.

=== modified file 'src/fileio.c'
--- src/fileio.c	2010-01-13 04:33:42 +0000
+++ src/fileio.c	2010-01-24 20:11:15 +0000
@@ -215,6 +215,12 @@
 /* Lisp function for moving files to trash.  */
 Lisp_Object Qmove_file_to_trash;
 
+/* Lisp function for recursively copying directories.  */
+Lisp_Object Qcopy_directory;
+
+/* Lisp function for recursively deleting directories.  */
+Lisp_Object Qdelete_directory;
+
 extern Lisp_Object Vuser_login_name;
 
 #ifdef WINDOWSNT
@@ -2241,7 +2247,11 @@
       && (NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname))))
 #endif
       )
-    newname = Fexpand_file_name (Ffile_name_nondirectory (file), newname);
+
+    if (!NILP (Ffile_directory_p (file)))
+      newname = Fexpand_file_name (Ffile_name_nondirectory (Fdirectory_file_name (file)), newname);
+    else
+      newname = Fexpand_file_name (Ffile_name_nondirectory (file), newname);
   else
     newname = Fexpand_file_name (newname, Qnil);
 
@@ -2279,15 +2289,23 @@
                                  NILP (ok_if_already_exists) ? Qnil : Qt);
           else
 #endif
-	    Fcopy_file (file, newname,
-			/* We have already prompted if it was an integer,
-			   so don't have copy-file prompt again.  */
-			NILP (ok_if_already_exists) ? Qnil : Qt,
-			Qt, Qt);
+            {
+              if ( Ffile_directory_p (file) )
+                call4 (Qcopy_directory, file, newname, Qt, Qnil);
+              else
+                Fcopy_file (file, newname,
+                            /* We have already prompted if it was an integer,
+                               so don't have copy-file prompt again.  */
+                            NILP (ok_if_already_exists) ? Qnil : Qt,
+                            Qt, Qt);
+            }
 
 	  count = SPECPDL_INDEX ();
 	  specbind (Qdelete_by_moving_to_trash, Qnil);
-	  Fdelete_file (file);
+          if ( Ffile_directory_p (file) )
+            call2 (Qdelete_directory, file, Qt);
+          else
+            Fdelete_file (file);
 	  unbind_to (count, Qnil);
 	}
       else
@@ -5727,6 +5745,10 @@
   Qdelete_by_moving_to_trash = intern_c_string ("delete-by-moving-to-trash");
   Qmove_file_to_trash = intern_c_string ("move-file-to-trash");
   staticpro (&Qmove_file_to_trash);
+  Qcopy_directory = intern_c_string ("copy-directory");
+  staticpro (&Qcopy_directory);
+  Qdelete_directory = intern_c_string ("delete-directory");
+  staticpro (&Qdelete_directory);
 
   defsubr (&Sfind_file_name_handler);
   defsubr (&Sfile_name_directory);

# Begin bundle