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

[David De La Harpe Golden]

Eli Zaretskii wrote:
> > Date: Sun, 24 Jan 2010 04:14:10 +0000
> > From: David De La Harpe Golden <david@harpegolden.net>
> > Cc: Tassilo Horn <tassilo@member.fsf.org>, 5436@debbugs.gnu.org
> >
> > 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: david@harpegolden.net-20100124201115-8gdf0v8qud9k6mbi
# target_branch: http://bzr.savannah.gnu.org/r/emacs/trunk/
# testament_sha1: 5549a39778e154134d8a775f7ecf6b930e588062
# timestamp: 2010-01-24 20:17:31 +0000
# base_revision_id: rgm@gnu.org-20100123232525-hl30xzix3wevh4bn
# 
# 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
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWXe//W0ACQ1/gFVf6Eh59///
/+f+gL////pgDsRfZQbeTu7uAoAAEwXSLu7ta5stgo6aWwNs0AwlEU9CMI0ZMSmxNNojTI1DTQ9Q
AeoA0BggaCSQAIBAiMSmnqfqmaIH6oAAAAAANPUHA0GmQ00aGEDIaGCNDTJo0AyDEAAaCQpJop5T
00npNoRpiPSYhtBoajIaANAAPUA0HA0GmQ00aGEDIaGCNDTJo0AyDEAAaBUkQTQI0aJghT9TE0zU
0mGKbUZMoDEGRhG0NFwoURA4N9a8uWuvSJbrOyyKKFhMyW2sPx8SONVU4OYUtyDmaWLHbhy6enp1
QlPNeGN7Bn2JLPQ2frVkvJBaImbjox5kPbf+UO6uloVM+95YYnv4NWDh7GdV5E+jdmQdJaHlJ5dq
XPfYQpsrMEWEvWUeVrJeDOa9XYrEe4sD/CkyU2g+1oBQOKhFAg6MhCRBIQMEUOq/+TEy6YU5zAVO
cfJgRk5VZLnUzn60gLQoScmpq449uO9t8rPF0nS88Yx3RSUqVRKVDz+gQc+fq8X+HFQ4CBTyJxiR
HET2w9kE9ntJiTQYTWHIZTknJ6vElK8R7K03BMk6DSe6zWetzIOk6nF9vR09R3PuPsaGfkO3TA+0
t2VX3vSmpTdqYe0z4BzTKjjyPbuP9ydyo59OKkD/vnz/Fi8iY2CAPkxf5/d0GIGjsD9LcnjPf3ZR
fEHCyH3Mw/3MKwCY5Zza09ys8WhA7zIh+obFHEDcy6Su0oT231aS6Is64EyPxE6qBG4xcJTiSH2Q
8iTNSd43oE2jWqBnZHqP+QiIxefxPLmXWjEMxl2JLmbXkyHFLu5Mxv5BqZzATJm0IrgtsezlyiHR
+o8QphDICRWtu3HjkiqkWqO1ddvdRYT7a9/2fCr/RQYP0e+hXY5KI5GG2GzIndjHo8F6o2jS1rJH
uNFiUlbjKmPRhRleySjUKU6Ju5BY1eFXatbUkag2ZhbLKbSxZ5sb7/l/rPkhsOZ7Pzd81pDNHhTz
jhujxc2q/CotyOUvgpKNjSYFi7wR70WnoESxBJfWizxwVLsMyuk76Qh/rtI0f2qYO2jxRxBqGGuK
5LdlRCohAjEEJ0scglYeb0Y0H79DGSSSQ1S7u6etvUvqrOuCTaRZZns1aohbFUVLKUzWkRQvlJPi
fSwvIj00NS9QjO6NflxmhHqzIyKERFpwANZuEehWNBNDBh6nYRggTAJDGkUpsKKDBAhPKVTczxM2
C4U9dAIq+RukGEymxF6clYi4qpJEBeARSEebamANcw+Fc0lIDVsJIy/W+gVDR+tByBGQbcPZ0jm2
Ol5BUhhklQd0zukNuhQFNmGf1DUNcrgRZjpqZko416J+nRPPmXFI2kfOekncHOwEkNyNnSkdw5o9
1dxt3Hs3KaLkcxssjua/TvEJOGtGnTiF0X0wZJeOyhjS3iuvOTKycu45TcrJ65nMziJA8lsZ2lq9
PLJkdrtEfYNQh1fEzJBtDV9G3JnaQlDfR5rt1cxS/Laz8iRwNKJIBq9936UcbMuii7NhySZKX5sP
XTGg0yNIwkd89qWWTuHDrVi8RSpOYgiqGbpuETMKBjZ/HR1biPlsI96zy00yWb8zXTJcDnUFvQ5L
AkqiMIXNhOj3vlMgkmxnUBypZ0Y5LXBeNSqSzTncxspGqcwiI0HXIVln2DLOkcuRbPclIDVBFXHE
/NcEJStJ/GDQ8RGoi+USRww2muIRGLgTUHu0hijoiJNQnGjlnHk0Q3CzAzAxioUO7jqZmbl0Ed07
PNNJIlc0M9B4jQq4uQMtikTuluPkZkuu+xCCcMO+2koqQ/xSVtLQvo+bhcozUiEwxSylXUrR8Ay0
HBrg74kywXGTxsMK81GcFyEP4CdGQMcbFm7/jA5CMt+XJxPJzbYEXhqFTBOUI7kJcHiz9sovKuTN
DtVPJEzgJIV/LuiQ2Ya526TnPiKA5ZlIDDs65mXKVhxYmbdpXs+MrJ1xRJCGHDVs+BpF5icJ0w8q
+QdQ2B1dwdT3iMZGZy2MpxiewCgHE992bDt7wEaJLeToZZ5pKvGpuV6RqxeafmQycx1NCp25Ojma
DixWLuYsyVg5mfDY4aYY3G9F0eDirTDZq2FpOJU1nOcgXwKmZYrItuLowLRKMytStBaErNoGGEuA
HGkHDw5OCmTI47DYxszg3EDFjDRRnLFpNtscKKJjS1w4kVhhLRj4lZEIxZSwW7oDENjYeDkKu4DD
MyleUqObm8Ny2WBNFuIPoXVDKhgVnzLx7+Xl5zNVr45hCQiFYrgHKcBqm1gGxjbTGMfUCFAojURx
rEmE/z+7FHuauU2l/c+b3PnaRL1Gpk93ufm0VbFF7H+V6ds48hJaNkMGWIcSkwaBEh4dpgJPL650
LR3vszkk7jBtKd6leI///ZhyZW8aNvVqSNR40Rbjm1qrPRC2vOjlo3K+nPE/+PGiLMX3/d94o5hh
8N6+IPG9uurKWY3hpic2SKN+jjfJxBrB8DrN6mFLwPtyIiVJh9RGiAOYmFFEUponGQV5yQopfloU
foqEKy3ITd69y9/O4v48N3Ys3CtE8EAKVOYEsOENln4R8FmSs2hSSSi5VpSu2nn7FdPrMebyc9J7
VceLieA8YFCpkI63zQo1Ihgd1AuAlALmodbN75gHoIZBxDIOIZEyfcLN9367GANxqjFqQqGkxtZr
ERCfL2wI46s6A6ADlrpDOOcUqVZLOCl5dyqlucyxnbUwe1Hcf7b5JzDCsTeIWANuB0w3goireN/H
/t/TK+60VFVq70JQoYzBp4a2GapG5VKpSmaxVN4qr4ReTSkLhXZ01atN01X+IzZnCFAhYqghlZE2
IaX2brYpHjWNMuRt98dMYcajxFSxaOmUwtGpJSLGDb2dSH67dY/pJCBfKyRgnRx9TZ4g5fIT9ORL
OHCS8BiIbOj1gGwRweDvPUuesQjxuHYFqlnXP25B6crtVgH5nqOcHAYkRDo24KzKFRwqgqPiIhUD
rt9sMyd9onzU0y8zTz+Qw/I23NPKYNzbuZVa62qWKP+7SWPDE6c7Hsd6Izkjd65y88GJOtxkxrCI
RIg+ZcA2igMTKmLGSAbPBr0m++Uv8WjgRj4XI93yFIEklna0C5LYmefx0foPzOFtXRBdJamL2bYb
Muw9X1xTk9RSjC9l1KlFo8I8cwjFLHwdkqVMmKi91KXnfUOYr+axYvE9Lzy0dbyikOBzMZ+HqqYI
jb7fVO2oUU+Wea90M0R6eWNLRw/pkYsHh+Ping+R3ElRsM452uvDh+W/f2GbE2nlSWRFjZx9Wekm
o3PTX6vzQglkVQUPp+VxNqCSOuQHFryOXrEtMGrj8LVUtKVN7WdCmz4zuNZ1yNI3VSGvniPhSSeS
jYO3iyg/0OWU9pkdTnR4j498PnR7525IwjZl7EdaO9ZFkd7a+gWPkerrEZMIlp48MrvM14R0KUUC
S9QAqNNMvoBi4ebckjM8lQh8Wu0kykjt7Xgtjj1LR5j9dzdwxNcuU7+uxXPyvUc3pRyb9ntFRJzU
5w3QSgHkXRHeIXZLxVQigkGqR8AjTDwhaoyU6lO9UjBiiLQ4KKdyegmG6KuU9iM9ZylSWSUaFdDU
q59aI/5/bVYhNIOA7vxjmGuQKrzEQPsUS1DsOEzbpLdbMgtgMFbkEsCgLMEZ+xgHoHPj7JU6oKWS
JapGd+aSc6L5VwfibkRf1pRoMmkWKVEo+U4u9kmZL2qcDekx7paS8Pr+W6RM9aNrR07rGxJ6eycZ
0HzKmJ8aLM8D0FoqJ3Moby1IjaePh5ccOl2azWZ31yTInk4yTYjijYj3jGZL1Ko6Lyt9k+5FYJPQ
eLqQ6Iwx+tWJjpBOcleTrc7BvSKdB6y0MJ6v0v6opOBhEAgYg7wYJMjv78CSKc0k7mIoFsLkGhVN
AikooQIQdxmKihU46inRQWtFjjNP77WbVZz8OTBJxvKqVVU4Rc7eOppBUMY5HZRVSqlVnyfj/fTS
H0SzoyRYxAyErjh2Z6tIToY7GxOBs4j0pzkiwzz3CG0SF2B9LDUYmEfEjRNiMMahgVJtYsnQUbeS
R1lJuayqbDmGrYa1cCt6NJtUc0mSTIMCkShUl8KpRVKaGGrczXUwtE1Gx9PJI+KSYzTr9o5Kf8K/
T3p6qODl6OKlSOX0LuhH5Rx2SSsimNKj47HMS7rcDKSRdKtB9V94+qU62JN8k1a0dEkowE3zZmOl
bOLXRaxmvL0S7qlk0lRdSXjFS0aIs3o9aOLXinjm4+p+yf7rWNw/4u5IpwoSDvf/raA=