From: David De La Harpe Golden <david@harpegolden.net>
To: Eli Zaretskii <eliz@gnu.org>
Cc: cyd@stupidchicken.com, tassilo@member.fsf.org, 5436@debbugs.gnu.org
Subject: bug#5436: 23.1.91; Deleting directories causes unusable file layout in freedesktop trashcan
Date: Sun, 24 Jan 2010 20:19:32 +0000 [thread overview]
Message-ID: <4B5CAB54.2060607@harpegolden.net> (raw)
In-Reply-To: <83y6jn5pju.fsf@gnu.org>
[-- 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=
next prev parent reply other threads:[~2010-01-24 20:19 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-21 12:50 bug#5436: 23.1.91; Deleting directories causes unusable file layout in freedesktop trashcan Tassilo Horn
2010-01-22 16:20 ` Chong Yidong
2010-01-22 22:58 ` David De La Harpe Golden
2010-01-24 4:14 ` David De La Harpe Golden
2010-01-24 9:46 ` Tassilo Horn
2010-01-24 18:24 ` Eli Zaretskii
2010-01-24 20:19 ` David De La Harpe Golden [this message]
2010-01-24 20:25 ` David De La Harpe Golden
2010-01-25 19:00 ` Chong Yidong
2010-01-25 23:33 ` David De La Harpe Golden
2010-01-26 16:04 ` Chong Yidong
2010-01-27 0:06 ` David De La Harpe Golden
2010-01-27 4:10 ` Chong Yidong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B5CAB54.2060607@harpegolden.net \
--to=david@harpegolden.net \
--cc=5436@debbugs.gnu.org \
--cc=cyd@stupidchicken.com \
--cc=eliz@gnu.org \
--cc=tassilo@member.fsf.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).