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

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

[Tassilo Horn]

I have `delete-by-moving-to-trash' enabled.  When I delete a directory
with `M-x delete-directory' or using `D' in dired on a directory and say
that I want to delete it recursively, the freedesktop trashcan contains
entries for each file deleted, and not only one entry for the whole
directory.

For example, I delete the directory ~/foo with these contents:

    ~/foo/
    ~/foo/bar/
    ~/foo/bar/test.txt

After that, the freedesktop trashcan contains these Entries:

--8<---------------cut here---------------start------------->8---
  /home/horn/.local/share/Trash/files:
  total used in directory 20K available 70801444
  drwx------ 4 horn horn 4.0K Jan 21 13:42 .
  drwxr-xr-x 4 horn horn 4.0K Jan 21 13:42 ..
  drwxr-xr-x 2 horn horn 4.0K Jan 21 13:42 bar                 ###
  drwxr-xr-x 2 horn horn 4.0K Jan 21 13:42 foo
  -rw-r--r-- 1 horn horn    1 Jan 21 13:42 test.txt            ###

  /home/horn/.local/share/Trash/info:
  total used in directory 20K available 70801444
  drwx------ 2 horn horn 4.0K Jan 21 13:42 .
  drwxr-xr-x 4 horn horn 4.0K Jan 21 13:42 ..
  -rw-r--r-- 1 horn horn   74 Jan 21 13:42 bar.trashinfo       ###
  -rw-r--r-- 1 horn horn   70 Jan 21 13:42 foo.trashinfo
  -rw-r--r-- 1 horn horn   83 Jan 21 13:42 test.txt.trashinfo  ###
--8<---------------cut here---------------end--------------->8---

The lines marked with ### are wrong.  `delete-directory' should keep the
structure of the deleted directory and move it as a whole to the trash.
Else, it's nearly impossible to restore the top level dir foo including
all contents again, and that's what the trashcan if for, right?


In GNU Emacs 23.1.91.1 (x86_64-pc-linux-gnu, GTK+ Version 2.18.6)
 of 2010-01-19 on thinkpad
Windowing system distributor `The X.Org Foundation', version 11.0.10704000
configured using `configure  '--prefix=/usr' '--build=x86_64-pc-linux-gnu' '--host=x86_64-pc-linux-gnu' '--mandir=/usr/share/man' '--infodir=/usr/share/info' '--datadir=/usr/share' '--sysconfdir=/etc' '--localstatedir=/var/lib' '--libdir=/usr/lib64' '--program-suffix=-emacs-23-vcs' '--infodir=/usr/share/info/emacs-23-vcs' '--with-sound' '--with-x' '--without-gconf' '--without-toolkit-scroll-bars' '--with-gif' '--with-jpeg' '--with-png' '--with-rsvg' '--with-tiff' '--with-xpm' '--with-xft' '--with-libotf' '--with-m17n-flt' '--with-x-toolkit=gtk' '--without-hesiod' '--without-kerberos' '--without-kerberos5' '--with-gpm' '--with-dbus' 'build_alias=x86_64-pc-linux-gnu' 'host_alias=x86_64-pc-linux-gnu' 'CFLAGS=-march=core2 -O2 -pipe' 'LDFLAGS=-Wl,-z,now''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Dired by name

Minor modes in effect:
  gnus-dired-mode: t
  auto-revert-mode: t
  diff-auto-refine-mode: t
  rcirc-track-minor-mode: t
  dired-omit-mode: t
  recentf-mode: t
  window-number-meta-mode: t
  window-number-mode: t
  exec-abbrev-cmd-mode: t
  global-undo-tree-mode: t
  undo-tree-mode: t
  global-subword-mode: t
  subword-mode: t
  global-hl-line-mode: t
  savehist-mode: t
  show-paren-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
<down> <down> <down> <down> <down> <up> <up> <up> <up> 
<up> <up> <down> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <C-f8> <C-f8> 
<C-f8> <C-f8> <C-f8> <C-f8> <C-f8> C-h f <return> M-. 
C-g M-2 <tab> <return> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <right> <right> 
<right> <right> <right> <right> <right> <right> <right> 
C-x k <return> C-x C-b <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <down> <up> <up> 
m m <down> m <up> <up> D y <down> <down> <return> <down> 
<down> <down> <down> <down> <down> <down> <down> <down> 
<down> <down> <down> <down> <down> <return> <return> 
+ t e s t <return> <return> C-x C-f f o o . t x t <return> 
C-x C-s C-x k <return> g <up> g g g ^ <return> C-x 
C-f f o o . t x t <return> C-x C-s <return> C-x C-s 
C-x k <return> ^ ^ C-x d <M-backspace> . l e <tab> 
<backspace> <backspace> <backspace> <backspace> <backspace> 
<backspace> o c <tab> s <tab> <return> <down> <down> 
<down> <down> <down> <down> <down> <down> <down> <up> 
<return> m m D y y y <up> C-x x d e l e <tab> d i r 
<tab> e <tab> <return> <return> y ^ <return> C-x b 
<return> D y y C-x b <return> <return> <return> ^ <down> 
<down> <return> ^ ^ <down> <return> M-x t r q <backspace> 
<backspace> <backspace> r e b <return>

Recent messages:
Omitting...
(Nothing to omit)
Omitting...
(Nothing to omit)
Omitting...
(Nothing to omit)
Omitting...
(Nothing to omit)
Omitting...
(Nothing to omit)

Load-path shadows:
~/repos/el/org-mode/lisp/org-bbdb hides /usr/share/emacs/23.1.91/lisp/org/org-bbdb
~/repos/el/org-mode/lisp/org-habit hides /usr/share/emacs/23.1.91/lisp/org/org-habit
~/repos/el/org-mode/lisp/org-colview hides /usr/share/emacs/23.1.91/lisp/org/org-colview
~/repos/el/org-mode/lisp/org-footnote hides /usr/share/emacs/23.1.91/lisp/org/org-footnote
~/repos/el/org-mode/lisp/org-freemind hides /usr/share/emacs/23.1.91/lisp/org/org-freemind
~/repos/el/org-mode/lisp/org-compat hides /usr/share/emacs/23.1.91/lisp/org/org-compat
~/repos/el/org-mode/lisp/org-icalendar hides /usr/share/emacs/23.1.91/lisp/org/org-icalendar
~/repos/el/org-mode/lisp/org-clock hides /usr/share/emacs/23.1.91/lisp/org/org-clock
~/repos/el/org-mode/lisp/org-bibtex hides /usr/share/emacs/23.1.91/lisp/org/org-bibtex
~/repos/el/org-mode/lisp/org-indent hides /usr/share/emacs/23.1.91/lisp/org/org-indent
~/repos/el/org-mode/lisp/org-faces hides /usr/share/emacs/23.1.91/lisp/org/org-faces
~/repos/el/org-mode/lisp/org-timer hides /usr/share/emacs/23.1.91/lisp/org/org-timer
~/repos/el/org-mode/lisp/org-vm hides /usr/share/emacs/23.1.91/lisp/org/org-vm
~/repos/el/org-mode/lisp/org-list hides /usr/share/emacs/23.1.91/lisp/org/org-list
~/repos/el/org-mode/lisp/org-gnus hides /usr/share/emacs/23.1.91/lisp/org/org-gnus
~/repos/el/org-mode/lisp/org-crypt hides /usr/share/emacs/23.1.91/lisp/org/org-crypt
~/repos/el/org-mode/lisp/org-exp hides /usr/share/emacs/23.1.91/lisp/org/org-exp
~/repos/el/org-mode/lisp/org-protocol hides /usr/share/emacs/23.1.91/lisp/org/org-protocol
~/repos/el/org-mode/lisp/org-inlinetask hides /usr/share/emacs/23.1.91/lisp/org/org-inlinetask
~/repos/el/org-mode/lisp/org-wl hides /usr/share/emacs/23.1.91/lisp/org/org-wl
~/repos/el/org-mode/lisp/org-plot hides /usr/share/emacs/23.1.91/lisp/org/org-plot
~/repos/el/org-mode/lisp/org-w3m hides /usr/share/emacs/23.1.91/lisp/org/org-w3m
~/repos/el/org-mode/lisp/org-agenda hides /usr/share/emacs/23.1.91/lisp/org/org-agenda
~/repos/el/org-mode/lisp/org-archive hides /usr/share/emacs/23.1.91/lisp/org/org-archive
~/repos/el/org-mode/lisp/org-attach hides /usr/share/emacs/23.1.91/lisp/org/org-attach
~/repos/el/org-mode/lisp/org-latex hides /usr/share/emacs/23.1.91/lisp/org/org-latex
~/repos/el/org-mode/lisp/org-mhe hides /usr/share/emacs/23.1.91/lisp/org/org-mhe
~/repos/el/org-mode/lisp/org-irc hides /usr/share/emacs/23.1.91/lisp/org/org-irc
~/repos/el/org-mode/lisp/org-table hides /usr/share/emacs/23.1.91/lisp/org/org-table
~/repos/el/org-mode/lisp/org-info hides /usr/share/emacs/23.1.91/lisp/org/org-info
~/repos/el/org-mode/lisp/org-docbook hides /usr/share/emacs/23.1.91/lisp/org/org-docbook
~/repos/el/org-mode/lisp/org-ascii hides /usr/share/emacs/23.1.91/lisp/org/org-ascii
~/repos/el/org-mode/lisp/org-jsinfo hides /usr/share/emacs/23.1.91/lisp/org/org-jsinfo
~/repos/el/org-mode/lisp/org-id hides /usr/share/emacs/23.1.91/lisp/org/org-id
~/repos/el/org-mode/lisp/org-feed hides /usr/share/emacs/23.1.91/lisp/org/org-feed
~/repos/el/org-mode/lisp/org-xoxo hides /usr/share/emacs/23.1.91/lisp/org/org-xoxo
~/repos/el/org-mode/lisp/org-publish hides /usr/share/emacs/23.1.91/lisp/org/org-publish
~/repos/el/org-mode/lisp/org-exp-blocks hides /usr/share/emacs/23.1.91/lisp/org/org-exp-blocks
~/repos/el/org-mode/lisp/org-mew hides /usr/share/emacs/23.1.91/lisp/org/org-mew
~/repos/el/org-mode/lisp/org-mobile hides /usr/share/emacs/23.1.91/lisp/org/org-mobile
~/repos/el/org-mode/lisp/org-datetree hides /usr/share/emacs/23.1.91/lisp/org/org-datetree
~/repos/el/org-mode/lisp/org-remember hides /usr/share/emacs/23.1.91/lisp/org/org-remember
~/repos/el/org-mode/lisp/org-macs hides /usr/share/emacs/23.1.91/lisp/org/org-macs
~/repos/el/org-mode/lisp/org-mouse hides /usr/share/emacs/23.1.91/lisp/org/org-mouse
~/repos/el/org-mode/lisp/org-html hides /usr/share/emacs/23.1.91/lisp/org/org-html
~/repos/el/org-mode/lisp/org-install hides /usr/share/emacs/23.1.91/lisp/org/org-install
~/repos/el/org-mode/lisp/org-src hides /usr/share/emacs/23.1.91/lisp/org/org-src
~/repos/el/org-mode/lisp/org hides /usr/share/emacs/23.1.91/lisp/org/org
~/repos/el/org-mode/lisp/org-rmail hides /usr/share/emacs/23.1.91/lisp/org/org-rmail
~/repos/el/org-mode/lisp/org-mac-message hides /usr/share/emacs/23.1.91/lisp/org/org-mac-message

Features:
(shadow emacsbug ibuf-ext ibuffer gnus-dired autorevert cc-mode cc-fonts
cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs mule-util
cal-move find-func diff-mode diff org-colview mailalias arc-mode
archive-mode etags url-cache newst-plainview newst-ticker newst-reader
newst-backend gnus-fun rs-info bbdb-gui gnus-cite ansi-color flow-fill
gnus-async gnus-bcklg sort gnus-ml vc-dispatcher vc-svn newcomment
help-mode hippie-exp multi-isearch gnus-topic parse-time pop3 nnml utf-7
utf7 auth-source nnimap imap trace nndraft nnmh nnnil gnus-agent
gnus-srvr gnus-score score-mode nnvirtual nntp gnus-cache notmuch
bbdb-hooks bbdb-gnus bbdb-snarf mail-extr spam spam-stat bbdb-com
gnus-uu yenc gnus-msg smiley gnus-art mm-uu mml2015 epg-config mm-view
smime dig gnus-sum nnoo gnus-group gnus-undo nnmail mail-source
format-spec gnus-start gnus-spec gnus-int gnus-range gnus-win view solar
cal-dst holidays hol-loaddefs cal-iso garak elim warnings lui tracking
flyspell ispell incomplete rcirc-late-fix rcirc greqlscript-mode
greql-mode tg-mode generic th-latex swank-clojure clojure-mode
slime-repl slime apropos hideshow paredit wtf cus-edit cus-start
cus-load rdictcc appt diary-lib diary-loaddefs vc-git org-w3m org-irc
org-jsinfo org-infojs org-html org-exp org-exp-blocks org-info org-gnus
org-docview org-bibtex org-bbdb org-protocol org-attach org-id
org-agenda remember org-remember org-datetree org org-footnote org-src
org-list org-faces org-compat org-macs org-install cal-menu calendar
cal-loaddefs dired-x dired-aux pcomplete em-term term ehelp electric
esh-var esh-io esh-cmd esh-ext esh-proc esh-arg eldoc esh-groups eshell
esh-util esh-module esh-mode lisppaste xml-rpc xml th-boxquote boxquote
rect highlight-symbol hi-lock footnote smtpmail message idna sendmail
ecomplete rfc822 mml mml-sec password-cache mm-decode mm-bodies
mm-encode mailabbrev gmm-utils mailheader canlock sha1 hex-util hashcash
info server yasnippet dropdown-list noutline outline
highlight-parentheses browse-kill-ring derived filesets recentf
tree-widget sr-speedbar speedbar sb-image ezimage dframe ido
anything-config compile comint ring semantic/util-modes semantic/util
semantic semantic/tag semantic/lex semantic/fw eieio byte-opt bytecomp
byte-compile mode-local cedet imenu w3m-bookmark w3m browse-url doc-view
jka-compr image-mode w3m-hist w3m-fb w3m-ems w3m-ccl ccl w3m-favicon
w3m-image w3m-proc w3m-util bookmark pp ffap dired rx thingatpt anything
woman easymenu man assoc window-number uniquify exec-abbrev-cmd
undo-tree easy-mmode cl cl-19 subword hl-line saveplace savehist paren
th-private edmacro kmacro th-common mm-url gnus gnus-ems nnheader
gnus-util netrc mail-utils wid-edit url-http tls url url-proxy
url-privacy url-expand url-methods url-history mailcap url-auth
mail-parse rfc2231 rfc2047 rfc2045 qp ietf-drums time-date url-cookie
url-util url-parse url-gw url-vars mm-util mail-prsvr windmove
disp-table swank-clojure-autoloads advice help-fns advice-preload
clojure-mode-autoloads slime-repl-autoloads slime-autoloads package
reporter site-gentoo w3m-load bbdb-autoloads bbdb regexp-opt timezone
tex-site auto-loads tooltip ediff-hook vc-hooks lisp-float-type mwheel
x-win x-dnd font-setting tool-bar dnd fontset image fringe lisp-mode
register page menu-bar rfn-eshadow timer select scroll-bar mldrag mouse
jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
loaddefs button minibuffer faces cus-face files text-properties overlay
md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
font-render-setting gtk x-toolkit x multi-tty emacs)

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

[Chong Yidong]

Hi David,

Could you take a look at Bug#5436?  Thanks.

> I have `delete-by-moving-to-trash' enabled.  When I delete a directory
> with `M-x delete-directory' or using `D' in dired on a directory and say
> that I want to delete it recursively, the freedesktop trashcan contains
> entries for each file deleted, and not only one entry for the whole
> directory.
>
> For example, I delete the directory ~/foo with these contents:
>
>     ~/foo/
>     ~/foo/bar/
>     ~/foo/bar/test.txt
>
> After that, the freedesktop trashcan contains these Entries:
>
> --8<---------------cut here---------------start------------->8---
>   /home/horn/.local/share/Trash/files:
>   total used in directory 20K available 70801444
>   drwx------ 4 horn horn 4.0K Jan 21 13:42 .
>   drwxr-xr-x 4 horn horn 4.0K Jan 21 13:42 ..
>   drwxr-xr-x 2 horn horn 4.0K Jan 21 13:42 bar                 ###
>   drwxr-xr-x 2 horn horn 4.0K Jan 21 13:42 foo
>   -rw-r--r-- 1 horn horn    1 Jan 21 13:42 test.txt            ###
>
>   /home/horn/.local/share/Trash/info:
>   total used in directory 20K available 70801444
>   drwx------ 2 horn horn 4.0K Jan 21 13:42 .
>   drwxr-xr-x 4 horn horn 4.0K Jan 21 13:42 ..
>   -rw-r--r-- 1 horn horn   74 Jan 21 13:42 bar.trashinfo       ###
>   -rw-r--r-- 1 horn horn   70 Jan 21 13:42 foo.trashinfo
>   -rw-r--r-- 1 horn horn   83 Jan 21 13:42 test.txt.trashinfo  ###
> --8<---------------cut here---------------end--------------->8---
>
> The lines marked with ### are wrong.  `delete-directory' should keep the
> structure of the deleted directory and move it as a whole to the trash.
> Else, it's nearly impossible to restore the top level dir foo including
> all contents again, and that's what the trashcan if for, right?

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

[David De La Harpe Golden]

Chong Yidong wrote:
> Hi David,
> 
> Could you take a look at Bug#5436?  Thanks.
> 

Ah yes, noted as part of #1440
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=1440#31
(best keep it a separate bug at this stage...)

#3353 is also a related issue that needs investigation
alongside this.
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=3353

So I'll have a look at the area again this weekend I guess.

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

[David De La Harpe Golden]

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

Chong Yidong wrote:
> Hi David,
> 
> Could you take a look at Bug#5436?  Thanks.
> 
>> I have `delete-by-moving-to-trash' enabled.  When I delete a directory
>> with `M-x delete-directory' or using `D' in dired on a directory and say
>> that I want to delete it recursively, the freedesktop trashcan contains
>> entries for each file deleted, and not only one entry for the whole
>> directory.
>>

Please find attached small* patch that should address this (and Bug#3353).

* Given the recursively data devouring area it's in, please examine 
extra-carefully all the same...

delete-directory:

  is adjusted to call move-file-to-trash with the expectation 
move-file-to-trash can move non-empty directories to trash - which was 
already true, mostly, since it just called rename-file on them, and 
/that/ already silently worked ...so long as the source and destination 
were on the same device.

I believe, but haven't actually tested, that w32's override 
system-move-file-to-trash already works for non-empty directories too.


rename-file:

is adjusted to call copy-directory then delete-directory to emulate 
cross-device directory renames as proposed under Bug#3353

Note that rename-file's existing behaviour when FILE and NEWNAME (from 
and to) were both directories seemed problematic to me, so I adjusted it:

. existing behaviour:

Rather than moving directory FILE within directory NEWNAME, as happens
with the existing code if FILE is a regular file and NEWNAME a directory 
- if directory NEWNAME is empty, directory FILE becomes a directory 
NEWNAME, and if  NEWNAME isn't empty, the operation fails.

That means emacs without this patch acts more like raw C rename() for 
directory-to-directory, yet like shell mv (except "mv -T") for 
regular-file-to-directory.

. new behaviour:

I thought that was plain inconsistent, so made it move directory FILE 
into directory NEWNAME if NEWNAME exists and is a directory in the 
intra-device case.  That also means less messing about in the 
inter-device case to make it match the intra-device case, since 
copy-directory has 
copy-within-destination-if-destination-is-an-existing-directory
semantics. However, it is possible (though IMO unlikely) that may break 
something somewhere, so I note doing it the other way and making the 
inter-device behaviour emulate the unpatched intra-device 
directory-directory C-rename()-like behaviour would be possible - seems 
less friendly to me though.


























[-- Attachment #2: dirtrash_r1.diff --]
[-- Type: text/x-patch, Size: 9201 bytes --]

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: david@harpegolden.net-20100124040857-v8aqjr4myiydqj55
# target_branch: http://bzr.savannah.gnu.org/r/emacs/trunk/
# testament_sha1: 8222b7fc8a776796db7afbef784a6dbfc2c98464
# timestamp: 2010-01-24 04:09:50 +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 02:01:39 +0000
@@ -4667,19 +4667,32 @@
   (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))))
+      ;; move-file-to-trash moves directories, empty or
+      ;; otherwise, into the trash as a unit. So if the directory is
+      ;; non-empty, only call move-file-to-trash here if recursive is non-nil,
+      ;; so that having delete-by-moving-trash non-nil doesn't cause "greedier"
+      ;; (pseudo-)deletion behaviour.
+      (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))
+        (progn
+          ;; do recursion if necessary if we're not moving to trash
+          (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 +6183,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 04:08:57 +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,15 @@
       && (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 +2293,31 @@
                                  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 +5757,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
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWRyGQXcABef/gFVf6AB59///
9+fugL////pgCq7vdry+oPVzQFBuHtnvbVrutrOsVqtVldjhIoKZJ4ip5ptRqn+lT9NT9Unk9Jqe
0Tak8npGoA0AyP1TQPUEkgExATSbU1Q00GQ0AAAAAAAGQZTQUepqeo02iNN6o/UjI0MgAYjQADIA
ACKn6FG9UGQABoAAGgAAAAAAARUTETEBPSGmmponqP1NNR6ntU9QZBoaM1Mm0g8oHqYSQiR6Iwqn
vRPU1MjaaBNTZNQ/VPUNMgAAAMgxGViFzcr06dPOFnioFVGn3e05qhnKV1j1Trs3tGrdu82XY4A3
4B3h1TWQkKyCA4EBxU3ITIyQeH5wIBBSqJHM0JdEUqdUGLoNR6p0gngAIoEVmrDnPUGuKQoCEEe9
5sdGJmKOmACBIBYwDDs1x7zwP7Go4Zz64l+HHhMe6Hu+Cp3mYsdJklmlir0EjLlPKpK5MG20Nq/+
AZWGU1GV4sY1sUDVEZylBjigWMyy1YW9K7EeghKbTjOHv41n9htKFPQs0V3l2Zvw5lS6NEXReUYY
cj5zgyUwnN3/d7l34Rq8cV7/G7ELuq/Zsa2e6A4LVMj+pp5GhjxOFpwhwWh9WFHw1y3lLTXKBuqY
BHz5VMI6NcnGeCnOj4t6VVBgdAsNhmP7rmKup7xlIVu2RO6zkvM8C+3UX9JNVxVb19NeQ2s5EFKU
lfMTMJ2WJBBZ4x0g9D2be1x1sFI9WyOCfdEO5rpDTCCdkGO3rxyL5nG/O0SeHmzsS4XIGW08kaD5
t686uVWfPzs7NbWO2gcc6fYdAqGHV7OVaECzC62t0Bjdmi1ge01ESZDBWblJOo6xG9A551MrJIPc
m0DqhYwWIf3xSt8XDZQtJaex/NXdfaBkVhYCWKl3LcerkVXBqd3d35DLi5O28wrEqmHYTQh2ElUK
ONB3EOJLUOwLOpCuUwTTKhUPJOQl8BJhVHlI31jaUE54QQk48i4p4KpcVFnoXirkw6IYqLkjSGxT
FeXMXD1Y3aYIV3IdcaNbkqywBbnNoqhGZpg9blrMgbnvgIbVBBSZhn9iyWX+QsW5jX5ndkszKPmv
QbHLAUk2o049o5ocUy8nmTx673v2tXqkgwQY6xDiK5AxsZQz6nNmZyVizqDIhAtRO8voyaiY4w0H
4LIj0fE3JGcM301OIiIa8nmmvRzFdKOtA2GiKM+uGM+xPlZyByUfoq852fN7+uW9al3auHPZBYhA
FBRN3ScgunluH8ux1RH+0EeFV0xsXua45HKZLdaoFQVkHNk6PV+49A2OKlWELLSphYogyTncxsom
jmiMzpwy4Vs5x3L7DxZCzqOJd5ciEpbSfY7iDIWD91dnMZJmu4RXKMVeO7UDRasDHLA7ehri5dgj
GmMO21RWJ3MYD8x3PstJvBBJXkeNBSykq3vCJjzL50U6aTNC0eIc1aDZUYFmxjd65CHbKV2FPrqg
w0o0xSdWEHPfkeQ2r5W4mVmYqKCvc6RIcMNz4lKW8UjpkY00L7RYnnrWnZHBVURMRpd+cHXtS0ib
4rwLVOpsnU8IrYmRveUIG22mtqOQYoNIQrayClzMk82LjrnaWONIXGMCkthMONDcxo+4hyDwHJpw
1tVOFsmMFyWQkmVbFhRjjalkxQg1tGNBsErsYPKXCeYRs2PY/TEN9dAmHUrlvro4m24jGw0rsWBd
BT1dioBLo3T3rENAxnr2mpTWLdg3S9uGmINNL31ENyaJnvs/Tb/xWUftXAhCCU2jaa7jy8A222m2
+IhFRVy2QQEl4d8xdZTjVZHVzlCCDUTXrOwLPGrXOZcwcZ43nWaAYMikdEEaXLVku8kxfG7OBArF
khVE1B8duWlNY379itjrBd6u2GIG2cHNBxnRfM7nWCxKev1bTPsyWiJmnknxPTMiOqyXDeZEEIH9
HkpWNTt5EHEZ/A6QBlwwZblyEKKy+WLIzJagvK6jfxei+OTNva71N6r+RZjUyt4l4aPWHg9Q1hQZ
IQyhOPh1K7ivl5efDjtsvRoXSpdBWxBWHUQgeU48b3jkNuDDhwQyA4fBY6MeRfHS1aFsuD6elJLJ
pqqtvFLCGzdacxOYwyZwe8YgguZPiqpm1iR7wti+e1Bgcq87fQspzwOFa1Elr6vHpun2tSlRXCRT
19kcA0Fg23Q4WiZokEILzdny9vvvOzzG6EHAIjXVpalHGbfWw/g9tap+jI2TkwbJoOUxDIcYc+xc
Y7/SQSEVz7rtIK/ymYvQZDGpXPbCuUqDbbaGsEsqLFBuC/Fuud2fNU6JYdrIeoA7vy4PAb7zFFXf
Dx4RGiBoOmiemjyXKWf5Yqn5FKsvswQjSwYwa1RWWcFoBaAF21yZwZjwHyaDoLkLnrMo/jAYuvjO
c5IOC/q5tyN250dJmgmg+DPBWTtqOzQ73HfV1izoKiZSKmfEcvanzcoMZKMItjCAeZXKSmoHcdwx
iomwRDNBysLBf2MnIA3m5OGsSvWEl6+tlIFu6uYDUw53kqqBrBduVxRM02VXfg83CjYzDM44a8GR
JsdSEQWLl8NLGt6bt2wMyX4hs6LQ65gAXFR/QRehp7kxwwxUotWy7q6Kq3AwXRG3Oj9UEHVBuTdu
oD+BmIHtaHBvCfM8uQelDepihVz8oshHcOJ0G2wZdPVwiN/MI2d+16ua4docYh7YBS2JsATKAW3S
klmONkgXLW6RUAcXEeV58Iy2r6oWnCZRQvJlBd1hh0JaKJBgsBZ3YYmWXOWwTjMTMaGNOQcl9TIK
b8dpBZzBXFeMm8S3ClpTQMdgitXjBQQxQbjM7QvAEvzIBJC2XT5HM1YFD2RED36BmdByZtUGpowq
0Mgrd0GpOgtEY8+obX/vMPCeodGYCqMEGyB8m08KuCXnBAwCWA5MMgY9BcvJJImBDteWiJ7E6hLr
cQiqiVhnx0PWg3ZLgWJ2DE3yoRK5HwMWCHFQcjGALe68+yctJqtLWuq1aAceKCxBiKwUpjszGMDa
XR6RNIRtWvhBYhKf0TJ4gQ0LDj4TQSMiuDtahvam/TdxTYsFSUjRJ1NgyysSSzYpYoHVFW7CJsAE
QmhOcwwzVdyYQ5wOWyp4pZY4INnjMmbNPWcgTCernYyZkzX8W0xekONYgOIFhGA51zq+MzHEuyk0
seCSoNToRagUEl2zzqakvGKiNSCU2SgmReW6ZXzS6JljgmYqthVLjamQihZjZEhExEkwQxMUZDQ6
VKyygi5QQUqr5saNCCaplb9Z3+jnR0MYX6sUr9xGoXBWgaYykG5zBBFwTABwZwXbGhdq4Zi0INla
DFAykAeZVCsqTpazM4JzIdFBghkQEmcKCe0R1DgWzDY43sO1/MjEhCJ7xdyRThQkByGQXcA=

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

[Tassilo Horn]

David De La Harpe Golden <david@harpegolden.net> writes:

Hi David,

> Please find attached small* patch that should address this (and
> Bug#3353).

I've applied it and rebuilt emacs, and now deletion moves the whole
directory to the trash, including all its contents.

Thanks a lot,
Tassilo

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

[Eli Zaretskii]

> 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.  I think it's useful, even
though the name directory-files-no-dot-files-regexp hints about the
intent.

> +      ;; move-file-to-trash moves directories, empty or
> +      ;; otherwise, into the trash as a unit. So if the directory is
> +      ;; non-empty, only call move-file-to-trash here if recursive is non-nil,
> +      ;; so that having delete-by-moving-trash non-nil doesn't cause "greedier"
> +      ;; (pseudo-)deletion behaviour.

Looks like this comment was not filled (via M-q).  Also, please always
leave two blanks after the period that ends a sentence.

> +      (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?  Isn't it better to display a message and continue
(with other potential moves)?  I'm frequently annoyed by a tool's
tendency to give up early leaving me in the middle of a partly done
job.

> +          ;; do recursion if necessary if we're not moving to trash

Full sentences beginning with a capital letter, please.

> +deleting files outright. If FILENAME is a directory, it
                          ^^
One more space.

> +/* Lisp function for recursively copying directories. */
                                                        ^
Two spaces here.

> +    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);
> +      }

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

Thanks again for working on this.

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

[David De La Harpe Golden]

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

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.


[-- Attachment #2: dirtrash_r2.diff --]
[-- Type: text/x-patch, Size: 10191 bytes --]

# 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=

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

[David De La Harpe Golden]

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

#$%? missed a "not" in one of the comments. Only noticed just after 
send.  Further slight comments improvement attached. Sorry.





[-- Attachment #2: dirtrash_r3.diff --]
[-- Type: text/x-patch, Size: 10678 bytes --]

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: david@harpegolden.net-20100124202459-k128rxdh6lf7m2jh
# target_branch: http://bzr.savannah.gnu.org/r/emacs/trunk/
# testament_sha1: 148945194f5afb56b9c652a4a14e1a222aabe40b
# timestamp: 2010-01-24 20:25:06 +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:24:59 +0000
@@ -4667,19 +4667,37 @@
   (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 not empty apart from "." and "..".
+                   (directory-files
+                    directory 'full directory-files-no-dot-files-regexp))
+              (error "Directory is not empty, not moving to trash.")
+            ;; Either recursive, or not recursive but directory is
+            ;; empty, so we can move directory 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 +6188,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
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWbRXlCwACo7/gFVf6Eh59///
/+f+gL////pgD+X193OoN8cK4oUAAAJlN3Kpu93vWKLb0DoGgAuyugJJIKeQYk2gyp6maZpqZJsk
ekyZABoA0BiaBoJJACYgQIhGoG1NAA9QDQAAAAD0hwNBpkNNGhhAyGhgjQ0yaNAMgxAAGgkKQmpG
00j0amaI0AZBiNqfqj0mahoAADQACSpqm1NND0gxNG9UNDIwEBhNGTJkGIGQaYIwiiIAiYCNTTCG
mUzQRp6gymgAyAA0NNNExgYBGfdarLlqqO4BLUdkYwwPrMiV5x9/gW7bMxn5xud5nTSkLjfczpJr
VW/1dWmyUfOcc92DrhLH0vXutnXOGnNNDEPTPI8yPbn+UPFbWhU183lhiZz6mrBw9jNV1F3LHGFP
FbyHVxuq5zhmfhki3wmUBZCBwL6F9ZB6mc16u8YtIg5aIfkrQlNoP00giiOlURBIIrgJJD4SQME4
H6Rfpb7IRvL7GkZ109GA8RCmMm/LujrURYGCSWIingxQSFrGbEBfC+VCgFiEwEEMQDBDEAdnnRN+
/n6adG3BNqVb+susrYwy2PXXrq0ikWwWxWBjElIbsbmjVytLrrrCkNmfNwpqTotF7rNZ64MI7B4O
h8XXseEN6T8IffKou6U58Kp+MJ8UR+Y7hvIM74KRceUmLjAk6ns2NPnJ3SVHuhTl5A/98HX8V9JE
xrkAfabmY09nl2GIGrsD6Wbo8Z0oeWJPkDksDzQyH+5osQCY5Zm1pnBWeLQgeZggfqGvRygcGO0t
GtONOONmmuwtHXAmR+EnUIEbxZxomZ84kh9hPIkzYtTST6Im0a1SGdbJ6D/kZAsXp8Ly5nvRstHU
YPBNcj9rydfa2ce5VmN3In0+qVeWbQmIESuhxVlyi1o/UclIy2hbiSq5HJv37NO8CbQoxa5QoYDG
EAXC7uDK6dtgFR4LqFle/RG+w7IbMid+MervYKjaMfQvEe20VykscZUx6kKMs2iUahSmeblwu4sA
rLbdqSNQbNJiEkGYSEnZM2Tp9X3voQyHY+z6PI4gDcD6oewHXoTl2X01Qk8hqClSBgMjBqkpQ9Qn
zgEvclhiVEPCyKEpgZQRDks+0SnjMS05Kc2Bmc3JQgsb/Uy+3PUa9mtTiZA6ER3tflHsDN4tgV7F
zJJJJISTYL+5r8/dpqYVXpZ6AbBWsxM6qyAdtZaXATcSiMAUtEPKe4rREe6AVB7JApvEW9+M0ken
UwWoKjFiYjSroJBvVwj6xpKgmhAsYgp1EYIFEgkMYigmyUUi5AhCeHVOTOJnCNEYzb1WBTWJanAk
ZBlSC2gShmWSZNqlR6UsRHtjYaBNmpPid4hIFu1xJY/W+iKo1ftQckCwjjr7BQc+Mzu3bLXmiRDD
CCg7tqaJDcgEAJswz/JG6N8aAot4HHiGdxic8T38KG3hPPlSQSOJHwPEDsnPQ73EUE0Dfd9JnA5p
eekxu/R58noqtuGy7DZyvNs+XnVAkHASBw5bFColMKlo0TkgCyCe7x616TeLfI5UctFQgm6lDprM
AUCkhcGt57vTy6ZHjDETzhdHAo+HyNyWgy5dFLmFW6FDykgIa9ZVhE3565+aKDy+X89WtDZTNRpR
JANp0w/WjjLLuouww4QMlL8zi7LvC+o0yVJzi49E96VDnL6nbrAgXvR4jS5NppIitBjZp8PEUNVE
xy/Xu62BH/bCPctc7bYWX5N9cB1HOonMjpQ7IuJLewjABHo4B7/Q+dCAgbGthA4c1N7vJR1TyNzS
VaswgynJO6jYkbpzJKI1ovuRtPPmjNaRx1LZ5JSEbJEVoOJ+tckJStIfzc1PfEbCNMRJDcuG12vD
Ek4uCoovdrDFXRESZxVTjRy6Wm5ohwFmBmBmBi9RUPPnwYMuXYR5Ts8112midzY12HiNizi5AzuN
TsKR4ngfJamtisivG5OIcDxw/ufcEE1oQ98QZ41IY4fNwsRmEjV0aBmuijalCtYRQ4fncgLh1Ch5
yK1ODRTKuuMpjGGEYko2grjdhEOqKVZIY0qbkNB7+ngXyAXURHjr0cUy5umRF4bh3sZKTjLqQl2P
Uz+MxeVuPTmjeD7JgmdqnVFADHr84kOGGwU7znOXMhC6PyViMOzMtkxrOI40JrfylrrbDryoVknY
QRJiGHDaF3wMxeZnCdMvKvkjujdDq8IdTB7hGMjzpyYZtKSkesFUFIXWnHDNh3ClAREWwg4JujjX
Igtzuble0asaTB9yGHMdzUqarx0oQnqbDloaRk/qIeaUHSNCu8jesCLwVhKCeo3EYVyvySUK7kkM
U2pyIlKpDekpFyToSgNUkAhLktZtUwQLr0hOwyAMokZMlxkyVXXOOqyMY2aSN1IYsiNUWkG5jbbY
24UUTTQdRFwrEdMtMm6ryELb4ysx0CQjEEQn1fAqeoYTT9pb9pV+v6+XLEsGExlR8y+ocaGgsPkY
D3Zs39NFW/DQISEQq7IBmN42DZxDYxtpjGPGEKIoLnCykSSqXFwVgvaU2RcH+0+l7T6hUAewqEj2
+0+1UaOo4b43xJ+115+8RxIksjxytFltkBKXCdGhAWt7mKSXyeq8iwbh9VwAeNKlmaS+QgjmD/X7
anJbOwS/mvUb06ERk3XGJEXYAk43QmqE0Ee66wP8p0IjJYfm83nMZDlfMj1iEx8l+gRERZGfILBQ
7HsDxtwWIIJrUj+HVEcBKSaLrwf9nm45uhMCnlEPE9P6DkASyXpYFiS/WXx6OvESlNFL5FdpYipt
xiMxM8WL3fn4uN/dYtYMfVj7438YeG++9IONhKbM+hPfJ9ye8ktIuMAhQCEokSwRzwebnI5fSlmr
Z07cL9nDKh0yX6UsqXmBkFfvfUAlYkiWB/cFcSAiF/ePHqr948DeSGQcQyDiGRMiGQc+yNDOwvDo
23tQNejGONQ00CIeDOYQlAkc4J566JspI0YAFPjaEoddTBDEjJsIKNDWQzuS2x6YanzCdSfs0iG0
K1qZtQsAaXBOjWh5cLKDhNbz/Zr7ZscjCfH4bcbnh5FQK/RsQmMkYGYZhhiY4HucZAZoQQ9KNhTG
aO7XRq01TmrDwFcDQCMIjTB2o7V10kjPyD5hdQ8ldOqp7ShbrNH0DzDXhC8UhlJTmYKyN6BAkhUz
8fiSmrZx0lp9hciP3wjoL8JPdfRE3ewu8DcW6a7RDqILE1zxt5rE2JboOqe7WZnQiPXZRQCuCJY5
JZJEbc+zQz6Lsc+xg6EQiNpJHT66ajKHO+mEiILhF5ws3c1FLlfISAeyDDlu8xj2daVsM6Bh1pU0
GegtiaE3shAfISJKd9icLpT1nlRG5U0el1bUSwHiew3HtDNAzT+BqA5hkDq2My28ZKC5fBwRb5ni
I6I9OjubyId5yILOlQjVkPEHvC2veRIwO5KHs+gUfuP2Oq5rskaCChYZm3Qpndzh6PpGDV6AggK0
koQQwEp3j0tQqxQMprJpqwrGFKJpqgdFiMKGfgiIFEe487KbDrBgU1hsLH7vRDVEc/DwO98cISSk
s+t7fKNlgpgiPo2JlKbf+FqZVtIPX+HF9Z6TpACEzS4d5lHr2/HTzeNLixM08iBKIymSb+F2C3ps
Pl1eHYFQNBgpeezryGNQgPLoE245Wm7oQOTMv3e+Yhlgh0mIbyDLuToTFOKmCaIgUx2qnvhV6oTI
Tn3FqJ+kNQQcQQhhPbOIwozypGsRyLEQiHxWoeARSIyHENBKCeZ0kj4A0TvPVzifT50Dp+bzzbqg
7ZJijSjIzOqgJU6WDaiQlATz6BFuDrgFDsxkQtVPnHp6D2s22+IlO1PsomnZYGTRIOznkI3a36B9
Ybu8HVyZ7QYFdpBuTapbVPIZrZtsTKF6nBLFLQ1o9gnJnRKzCWsHEg8xANSxEZU1kJB0D8QNdAxQ
IPkEuxTUDAiAYZwPEWjojfQhez3t1gXkgwox8RMEZFTB7hKnwvXUHMSBEbBDY64UxAhXGZEODKuk
Tf3cCOU/D5u0rFZWJuKLepdTYIbRKWxrPwTQiNDvPphJDEC4xSUggAgPam88patwNJgNgciBZ0Mr
RT6vZQFbsQczA5dEhkr6Od3vAPcQFidwkl1Q7UkYAOhtU0pMIjmnTr67K8DnxDFLqYoFoPVuEMhN
wmQnwBsbSkBEyG+WNMr8BIqgdqcvMCb0rZ+IiwLMAR2ix1cTaVNKjBvTvSVM2jlwy57FkIBqFTOs
9BAWwPi8WYi370CTeJRMUyk4DItkCpWYqwSEkkZkHtoElYmKkHCASZGU3uP/tBcXybtmqqunRNpt
sbOWKiNbTtM5CGkViwGNg2m02maevu+OpUR9dOOsgThiBhAaDh2T06wnQv5G5dusdhQt4YW2jiRQ
7xI5ABoVTwK3pYlR8oOC5CVshCqQuZYWm9IDPkR4pA5aDJGYujgRW4aDbg1BFVcZHCUkBIAghkWB
IWlYggIghwSt+guKMFZAL0yPDkQsgggUdHGClY+I3oyJaGC4TXpxhkKatPF4R8YO7IQi0iCLIiGE
7pDYrQ4msLVFoMSieFNKHgwcSwHSIX4ibxCGwYLFHS5VA4M3JNEZkLigUgGjzMjgwNCFolhBKYCS
aRPSJuMbF6HQngfufrJkiJP/i7kinChIWivKFgA=

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

[Chong Yidong]

I don't think it's wise to make this drastic change to rename-file so
late in the release process.  Is it necessary for fixing this particular
issue (Bug#5436)?  If not, we should only apply the delete-directory
change.

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

[David De La Harpe Golden]

Chong Yidong wrote:
> I don't think it's wise to make this drastic change to rename-file so
> late in the release process.  Is it necessary for fixing this particular
> issue (Bug#5436)?  If not, we should only apply the delete-directory
> change.

Hmm. I suppose strictly it is not necessary to fix the precise issue - 
partial or total failure to delete isn't the same as successfully 
deleting but with a bad layout in the trash.

Just the delete-directory change _is_ a marked improvement, since it 
does mean that the subsequent failure mode when you try to delete from a 
different filesystem to your /home is less obnoxious:

- Without the delete-directory change, the xdev behaviour is very bad 
indeed:

Say you have /usr/local and /home on different filesystems, as is quite 
common, and you delete-directory a tree in /usr/local with 
delete-by-moving-to-trash on - first, a few leaf regular files from a 
dir of the tree may be moved to trash (flattened), and only a bit later 
the operation will fail, when the first directory rename is attempted. 
Leaving behind a (maybe large) tree that might look untouched to casual 
inspection following an apparently-failed deletion operation, but 
actually with a few files missing, moved to the trash.  Eek.

- With the delete-directory change but without the rename-file change 
(or something else*), deleting from /usr/local will still fail, just 
pretty much immediately instead of making such a mess.


* There are also alternatives to expanding rename-file to handle xdev 
directory renames:

1. move-file-to-trash could just use a copy-directory and  then a 
delete-directory itself in the case it is passed a directory, which 
would be obviously less efficient in the same-filesystem case, but 
wouldn't fail nearly as easily as the untouched situation.
If it's easy to tell if directories are on the same filesystem from 
within elisp (which I haven't looked into yet), it could use rename-file 
where possible.

+++ That approach might squeeze in? copy-directory and delete-directory 
in themselves aren't as new.


2. could introduce a separate function capable of renaming directories 
xdev?

After all, copy-file and copy-directory and delete-file and 
delete-directory are separated.   move-file-to-trash could easily call, 
say, a "rename-directory" for directories and rename-file for files.
Of course rename-file does work _sometimes_ on directories, just not 
xdev (someone apparently hacked it to work xdev for files at some 
stage...) - I suppose a note could be added to the rename-file docstring 
saying that if you want directory renames that work xdev you should 
really use rename-directory.

A separate rename-directory function might facilitate dealing with 
complex cases.

(And/or there could be separate move-file-to-trash and 
move-directory-to-trash functions, or for naming regularity, 
move-file-to-trash could still handle both but be renamed move-to-trash...)

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

[Chong Yidong]

David De La Harpe Golden <david@harpegolden.net> writes:

> Just the delete-directory change _is_ a marked improvement, since it
> does mean that the subsequent failure mode when you try to delete from
> a different filesystem to your /home is less obnoxious:
>
> - Without the delete-directory change, the xdev behaviour is very bad
> indeed:
>
> Say you have /usr/local and /home on different filesystems, as is
> quite common, and you delete-directory a tree in /usr/local with
> delete-by-moving-to-trash on - first, a few leaf regular files from a
> dir of the tree may be moved to trash (flattened), and only a bit
> later the operation will fail, when the first directory rename is
> attempted. Leaving behind a (maybe large) tree that might look
> untouched to casual inspection following an apparently-failed deletion
> operation, but actually with a few files missing, moved to the trash.
> Eek.

I understand what you are saying.  How about conditioning the
delete-directory change on delete-by-moving-to-trash?

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

[David De La Harpe Golden]

Chong Yidong wrote:

> I understand what you are saying.  How about conditioning the
> delete-directory change on delete-by-moving-to-trash?

Unless I'm misunderstanding you (or did you mean the rename-file change?*):

delete-directory with the delete-directory part of the patch already 
becomes conditionalised on delete-by-moving-to-trash. See the patch
introducing "(if delete-by-moving-to-trash ...[a]... ...[b]...)"

[a] delete-by-moving-to-trash non-nil, delete-directory called with a 
directory arg and arg recursive non-nil:

a single call to move-file-to-trash is made in delete-directory, so that 
move-file-to-trash can handle the directory tree as a unit**.

[b] delete-by-moving-to-trash nil, delete-directory called with a
directory arg and arg recursive non-nil:

the original self-recursive code path in delete-directory is used - it's 
really deleting, so delete-directory just walks the tree and deletes 
everything, calling itself recursively - there's no need for renames and 
copies in this case, so what happens cross-device doesn't matter***

* If so, I think that would be a tad confusing, anyway. Probably 
possible, but would be getting a bit difficult to follow IMO (remember 
even an unpatched rename-file already wraps delete-by-moving-to-trash to 
nil around a call to delete-file.)

** With the other part of the patch (the more controversial change to 
rename-file), when move-file-to-trash then calls the extended 
rename-file to actually do the move,  the extended rename-file may call 
delete-directory again, but with with delete-by-moving-to-trash 
dynamically bound to nil around it to really delete after 
copy-directory, meaning path [a] does ultimately internally sometimes 
use path [b]. That is highly similar to the way the unpatched 
rename-file calls delete-file in the xdev with delete-by-moving-to-trash 
bound to nil around it to really delete files after copy-file, emulating 
a rename cross-device.


*** come to think of it, except for mountpoints that exist below
the directory being deleted ...wonder what happens then...

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

[Chong Yidong]

David De La Harpe Golden <david@harpegolden.net> writes:

> Chong Yidong wrote:
>
>> I understand what you are saying.  How about conditioning the
>> delete-directory change on delete-by-moving-to-trash?
>
> Unless I'm misunderstanding you (or did you mean the rename-file
> change?*):

Sorry, I meant the rename-file change.

However, after looking carefully at the change, I think it is OK.  I've
commited it to the respository.

Thank you for your work on this.