* bug#60146: file-exists-in-trash-p needs better name or semantics @ 2022-12-17 5:17 Paul Eggert 2022-12-17 8:47 ` Eli Zaretskii 2022-12-17 10:01 ` Michael Albinus 0 siblings, 2 replies; 8+ messages in thread From: Paul Eggert @ 2022-12-17 5:17 UTC (permalink / raw) To: 60146 The recently-added function (file-exists-in-trash-p FILE) is poorly named since it's not really related to trash - it's simply checking for the existence of a directory entry named FILE. How about extending file-exists-p instead? (file-exists-p FILE t) would be like (file-exists-p FILE) except it would not follow symlinks. This extension can be implemented via a single system call on POSIX systems, and this would be more efficient and would avoid a race in the current implementation of file-exists-in-trash-p. (Though of course pretty much any use of this new function makes one vulnerable to races....) If extending file-exists-p is too much, at least please rename file-exists-in-trash-p to something like files--exists-nofollow-p, to indicate that it's private to files.el and to say better what it means. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 5:17 bug#60146: file-exists-in-trash-p needs better name or semantics Paul Eggert @ 2022-12-17 8:47 ` Eli Zaretskii 2022-12-17 15:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-12-17 19:57 ` Paul Eggert 2022-12-17 10:01 ` Michael Albinus 1 sibling, 2 replies; 8+ messages in thread From: Eli Zaretskii @ 2022-12-17 8:47 UTC (permalink / raw) To: Paul Eggert, Lars Ingebrigtsen, Stefan Monnier; +Cc: 60146 > Date: Fri, 16 Dec 2022 21:17:13 -0800 > From: Paul Eggert <eggert@cs.ucla.edu> > > The recently-added function (file-exists-in-trash-p FILE) is poorly > named since it's not really related to trash - it's simply checking for > the existence of a directory entry named FILE. > > How about extending file-exists-p instead? (file-exists-p FILE t) would > be like (file-exists-p FILE) except it would not follow symlinks. This > extension can be implemented via a single system call on POSIX systems, > and this would be more efficient and would avoid a race in the current > implementation of file-exists-in-trash-p. (Though of course pretty much > any use of this new function makes one vulnerable to races....) > > If extending file-exists-p is too much, at least please rename > file-exists-in-trash-p to something like files--exists-nofollow-p, to > indicate that it's private to files.el and to say better what it means. I don't really mind renaming that function, but the reason I called it like I did was that apparently no one needed such a functionality in Emacs until now, except in this obscure use case of moving to trash. That seems to tell me that extending file-exists-p would be a solution waiting for the problem, something that we don't like doing. In any case, if we do decide to extend file-exists-p, it would need to be done on master, as that is not a trivial change, which has to be done in C. Lars, Stefan, WDYT? ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 8:47 ` Eli Zaretskii @ 2022-12-17 15:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-12-17 17:45 ` Eli Zaretskii 2022-12-17 19:57 ` Paul Eggert 1 sibling, 1 reply; 8+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-12-17 15:51 UTC (permalink / raw) To: Eli Zaretskii; +Cc: Lars Ingebrigtsen, Paul Eggert, 60146 > I don't really mind renaming that function, but the reason I called it > like I did was that apparently no one needed such a functionality in > Emacs until now, except in this obscure use case of moving to trash. > That seems to tell me that extending file-exists-p would be a solution > waiting for the problem, something that we don't like doing. Hmm... arguing about names, eh? Let me throw that hat into the ring: IIUC this is a variant of `file-exists-p` (currently) used only when dealing the trash, so I'd call it `trash--file-exists-p`. > In any case, if we do decide to extend file-exists-p, it would need to > be done on master, as that is not a trivial change, which has to be > done in C. Indeed, I see no need for that on emacs-29. Stefan ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 15:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-12-17 17:45 ` Eli Zaretskii 0 siblings, 0 replies; 8+ messages in thread From: Eli Zaretskii @ 2022-12-17 17:45 UTC (permalink / raw) To: Stefan Monnier; +Cc: larsi, eggert, 60146 > From: Stefan Monnier <monnier@iro.umontreal.ca> > Cc: Paul Eggert <eggert@cs.ucla.edu>, Lars Ingebrigtsen <larsi@gnus.org>, > 60146@debbugs.gnu.org > Date: Sat, 17 Dec 2022 10:51:38 -0500 > > Let me throw that hat into the ring: IIUC this is a variant of > `file-exists-p` (currently) used only when dealing the trash, so I'd > call it `trash--file-exists-p`. Well, the function is defined in files.el, so starting the name with "trash" would be frowned upon... ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 8:47 ` Eli Zaretskii 2022-12-17 15:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-12-17 19:57 ` Paul Eggert 2022-12-17 20:09 ` Eli Zaretskii 1 sibling, 1 reply; 8+ messages in thread From: Paul Eggert @ 2022-12-17 19:57 UTC (permalink / raw) To: Eli Zaretskii, Lars Ingebrigtsen, Stefan Monnier; +Cc: 60146 [-- Attachment #1: Type: text/plain, Size: 817 bytes --] On 12/17/22 00:47, Eli Zaretskii wrote: > I don't really mind renaming that function, but the reason I called it > like I did was that apparently no one needed such a functionality in > Emacs until now, I have a bit of a different take. First, there's adequate functionality in Emacs now if we don't mind inefficiency, and it's already used; just call file-attributes. (Gnus does this, in nnmaildir-unlink at least, and I expect there are other uses.) Second, I wouldn't be surprised if other uses of file-exists-p have problems similar to the one you found in move-file-to-trash. Not that any of us have time to go scout for them right now. It's a minor point. Still, the name file-exists-in-trash-p really needs to go somehow as it's not a name that should be user-visible. How about the attached patch? [-- Attachment #2: 0001-Remove-file-exists-in-trash-p.patch --] [-- Type: text/x-patch, Size: 2525 bytes --] From adcf3112e72431e4c4a2610941afcc238e7dd243 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sat, 17 Dec 2022 11:54:46 -0800 Subject: [PATCH] Remove file-exists-in-trash-p * lisp/files.el (file-exists-in-trash-p): Remove, as this name is not suitable for users. All uses replaced by file-attributes, which is good enough here. --- lisp/files.el | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/lisp/files.el b/lisp/files.el index c74e7e808e4..149480a6f10 100644 --- a/lisp/files.el +++ b/lisp/files.el @@ -8467,14 +8467,6 @@ trash--hexify-table (declare-function system-move-file-to-trash "w32fns.c" (filename)) -(defun file-exists-in-trash-p (filename) - "Return non-nil if FILENAME exists in the trash. - -This is like `file-exists-p', but it also returns non-nil -if FILENAME is a dangling symlink, to allow trashing such files." - (or (file-exists-p filename) - (file-symlink-p filename))) - (defun move-file-to-trash (filename) "Move the file (or directory) named FILENAME to the trash. When `delete-by-moving-to-trash' is non-nil, this function is @@ -8505,7 +8497,7 @@ move-file-to-trash (unless (file-directory-p trash-dir) (make-directory trash-dir t)) ;; Ensure that the trashed file-name is unique. - (if (file-exists-in-trash-p new-fn) + (if (file-attributes new-fn) (let ((version-control t) (backup-directory-alist nil)) (setq new-fn (car (find-backup-file-name new-fn))))) @@ -8582,7 +8574,7 @@ move-file-to-trash ;; We're checking further down whether the info file ;; exists, but the file name may exist in the trash ;; directory even if there is no info file for it. - (when (file-exists-in-trash-p + (when (file-attributes (file-name-concat trash-files-dir files-base)) (setq overwrite t files-base (file-name-nondirectory @@ -8620,7 +8612,7 @@ move-file-to-trash (let ((delete-by-moving-to-trash nil) (new-fn (file-name-concat trash-files-dir files-base))) (if (or (not is-directory) - (not (file-exists-in-trash-p new-fn))) + (not (file-attributes new-fn))) (rename-file fn new-fn overwrite) (copy-directory fn (file-name-as-directory new-fn) -- 2.38.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 19:57 ` Paul Eggert @ 2022-12-17 20:09 ` Eli Zaretskii 2022-12-17 23:55 ` Paul Eggert 0 siblings, 1 reply; 8+ messages in thread From: Eli Zaretskii @ 2022-12-17 20:09 UTC (permalink / raw) To: Paul Eggert; +Cc: larsi, monnier, 60146 > Date: Sat, 17 Dec 2022 11:57:39 -0800 > Cc: 60146@debbugs.gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > > I have a bit of a different take. First, there's adequate functionality > in Emacs now if we don't mind inefficiency, and it's already used; just > call file-attributes. (Gnus does this, in nnmaildir-unlink at least, and > I expect there are other uses.) Second, I wouldn't be surprised if other > uses of file-exists-p have problems similar to the one you found in > move-file-to-trash. Not that any of us have time to go scout for them > right now. > > It's a minor point. Still, the name file-exists-in-trash-p really needs > to go somehow as it's not a name that should be user-visible. How about > the attached patch? If you can verify that it works in the scenario of bug#59986, I'm okay with this change. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 20:09 ` Eli Zaretskii @ 2022-12-17 23:55 ` Paul Eggert 0 siblings, 0 replies; 8+ messages in thread From: Paul Eggert @ 2022-12-17 23:55 UTC (permalink / raw) To: Eli Zaretskii; +Cc: larsi, monnier, 60146-done On 12/17/22 12:09, Eli Zaretskii wrote: > If you can verify that it works in the scenario of bug#59986, I'm okay > with this change. Thanks, I checked that the patch works in that scenario (dangling symlink in the trash directory), and installed it. Closing the bug report. ^ permalink raw reply [flat|nested] 8+ messages in thread
* bug#60146: file-exists-in-trash-p needs better name or semantics 2022-12-17 5:17 bug#60146: file-exists-in-trash-p needs better name or semantics Paul Eggert 2022-12-17 8:47 ` Eli Zaretskii @ 2022-12-17 10:01 ` Michael Albinus 1 sibling, 0 replies; 8+ messages in thread From: Michael Albinus @ 2022-12-17 10:01 UTC (permalink / raw) To: Paul Eggert; +Cc: 60146 Paul Eggert <eggert@cs.ucla.edu> writes: Hi Paul, > How about extending file-exists-p instead? (file-exists-p FILE t) > would be like (file-exists-p FILE) except it would not follow > symlinks. This extension can be implemented via a single system call > on POSIX systems, and this would be more efficient and would avoid a > race in the current implementation of file-exists-in-trash-p. (Though > of course pretty much any use of this new function makes one > vulnerable to races....) Pls don't do it in the emacs-29 branch. file-exists-p checks for file name handlers, with all consequences for a changed signature. Best regards, Michael. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-12-17 23:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-17 5:17 bug#60146: file-exists-in-trash-p needs better name or semantics Paul Eggert 2022-12-17 8:47 ` Eli Zaretskii 2022-12-17 15:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-12-17 17:45 ` Eli Zaretskii 2022-12-17 19:57 ` Paul Eggert 2022-12-17 20:09 ` Eli Zaretskii 2022-12-17 23:55 ` Paul Eggert 2022-12-17 10:01 ` Michael Albinus
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).