unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories?
@ 2021-04-22 20:48 Protesilaos Stavrou
  2021-04-22 21:11 ` Gregory Heytings
  0 siblings, 1 reply; 6+ messages in thread
From: Protesilaos Stavrou @ 2021-04-22 20:48 UTC (permalink / raw)
  To: 47960

On trunk the (setq delete-by-moving-to-trash t) no longer works in Dired
and Eshell when trying to delete a directory.

Steps to reproduce with emacs -Q:

+ Evaluate (setq delete-by-moving-to-trash t).
+ Use 'C-x C-j' to jump to a dired buffer.
+ Then type '+' and create a new directory named "test" or whatever.
+ With point over the newly created directory ,type "D", and confirm.

You get an error: "file-error: Renaming: Not a directory [...]"

Same if you try to delete the directory with the 'rm -r' command from
inside eshell (though 'rmdir' works in this case).

For completeness, M-x shell works with either 'rm -r' and 'rmdir'.

-- 
Protesilaos Stavrou
https://protesilaos.com





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories?
  2021-04-22 20:48 bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories? Protesilaos Stavrou
@ 2021-04-22 21:11 ` Gregory Heytings
  2021-04-22 21:42   ` Protesilaos Stavrou
  0 siblings, 1 reply; 6+ messages in thread
From: Gregory Heytings @ 2021-04-22 21:11 UTC (permalink / raw)
  To: Protesilaos Stavrou; +Cc: 47960


>
> On trunk the (setq delete-by-moving-to-trash t) no longer works in Dired 
> and Eshell when trying to delete a directory.
>
> Steps to reproduce with emacs -Q:
>
> + Evaluate (setq delete-by-moving-to-trash t).
> + Use 'C-x C-j' to jump to a dired buffer.
> + Then type '+' and create a new directory named "test" or whatever.
> + With point over the newly created directory ,type "D", and confirm.
>
> You get an error: "file-error: Renaming: Not a directory [...]"
>
> Same if you try to delete the directory with the 'rm -r' command from 
> inside eshell (though 'rmdir' works in this case).
>
> For completeness, M-x shell works with either 'rm -r' and 'rmdir'.
>

Are you sure this bug is not due to something going wrong on your machine? 
I'm unable to reproduce this bug here, both Dired and Eshell seem to work 
as expected.

Did you by any chance define a 'system-move-file-to-trash' function?

Does it work when you (setq trash-directory <something>)?





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories?
  2021-04-22 21:11 ` Gregory Heytings
@ 2021-04-22 21:42   ` Protesilaos Stavrou
  2021-04-23 20:39     ` Gregory Heytings
  0 siblings, 1 reply; 6+ messages in thread
From: Protesilaos Stavrou @ 2021-04-22 21:42 UTC (permalink / raw)
  To: Gregory Heytings; +Cc: 47960

On 2021-04-22, 21:11 +0000, Gregory Heytings <gregory@heytings.org> wrote:

>>
>> On trunk the (setq delete-by-moving-to-trash t) no longer works in
>> Dired and Eshell when trying to delete a directory.
>>
>> Steps to reproduce with emacs -Q:
>>
>> + Evaluate (setq delete-by-moving-to-trash t).
>> + Use 'C-x C-j' to jump to a dired buffer.
>> + Then type '+' and create a new directory named "test" or whatever.
>> + With point over the newly created directory ,type "D", and confirm.
>>
>> You get an error: "file-error: Renaming: Not a directory [...]"
>>
>> Same if you try to delete the directory with the 'rm -r' command from
>> inside eshell (though 'rmdir' works in this case).
>>
>> For completeness, M-x shell works with either 'rm -r' and 'rmdir'.
>>
>
> Are you sure this bug is not due to something going wrong on your
> machine? I'm unable to reproduce this bug here, both Dired and Eshell
> seem to work as expected.

It could be, though I have only updated Emacs in the last ~10 days.

What seems to be the issue is when a deleted entry of the same name
already exists.  I emptied my trash and tried the above recipe: the
problem would not occur.  But upon immediate retry the problem appeared,
presumably because "test" already existed in the trash.

> Did you by any chance define a 'system-move-file-to-trash' function?

No

> Does it work when you (setq trash-directory <something>)?

Yes, it does.  Both for my original recipe and with the other case I
mentioned above.

I have always set this to nil, which makes it use the trash of
freedesktop.org.

-- 
Protesilaos Stavrou
https://protesilaos.com





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories?
  2021-04-22 21:42   ` Protesilaos Stavrou
@ 2021-04-23 20:39     ` Gregory Heytings
  2021-05-07 20:41       ` Gregory Heytings
  0 siblings, 1 reply; 6+ messages in thread
From: Gregory Heytings @ 2021-04-23 20:39 UTC (permalink / raw)
  To: Protesilaos Stavrou; +Cc: Lars Ingebrigtsen, 47960

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


>
> What seems to be the issue is when a deleted entry of the same name 
> already exists.  I emptied my trash and tried the above recipe: the 
> problem would not occur.  But upon immediate retry the problem appeared, 
> presumably because "test" already existed in the trash.
>

Indeed, this path of the code was recently changed (commit a5197e2240), 
and indeed, when a directory with the same name is trashed twice, it 
fails.  Patch attached.

(It can't be applied yet, I'm still waiting for the end of my paperwork.)

[-- Attachment #2: Type: text/x-diff, Size: 1498 bytes --]

From 68dece8c81045aad8c589777a7dc66a7e38e6385 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Fri, 23 Apr 2021 20:32:02 +0000
Subject: [PATCH] Fix bug when moving directories to trash

* lisp/files.el (move-file-to-trash): Pass the correct dir-flag to
make-temp-file so that a directory is created when a directory is
being trashed (Bug#47960).
---
 lisp/files.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 7440c11a21..27282f09fc 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -7878,6 +7878,7 @@ move-file-to-trash
 
 	       ;; Make a .trashinfo file.  Use O_EXCL, as per trash-spec 1.0.
 	       (let* ((files-base (file-name-nondirectory fn))
+                      (is-directory (file-directory-p fn))
                       (overwrite nil)
                       info-fn)
                  ;; We're checking further down whether the info file
@@ -7889,7 +7890,8 @@ move-file-to-trash
                          files-base (file-name-nondirectory
                                      (make-temp-file
                                       (expand-file-name
-                                       files-base trash-files-dir)))))
+                                       files-base trash-files-dir)
+                                      is-directory))))
 		 (setq info-fn (expand-file-name
 				(concat files-base ".trashinfo")
 				trash-info-dir))
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories?
  2021-04-23 20:39     ` Gregory Heytings
@ 2021-05-07 20:41       ` Gregory Heytings
  2021-05-24 22:26         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 6+ messages in thread
From: Gregory Heytings @ 2021-05-07 20:41 UTC (permalink / raw)
  To: Protesilaos Stavrou; +Cc: Lars Ingebrigtsen, 47960


A shorter (and therefore better) patch to solve this issue has been 
proposed in bug#48280.  It should be applied instead of this one.





^ permalink raw reply	[flat|nested] 6+ messages in thread

* bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories?
  2021-05-07 20:41       ` Gregory Heytings
@ 2021-05-24 22:26         ` Lars Ingebrigtsen
  0 siblings, 0 replies; 6+ messages in thread
From: Lars Ingebrigtsen @ 2021-05-24 22:26 UTC (permalink / raw)
  To: Gregory Heytings; +Cc: Protesilaos Stavrou, 47960

Gregory Heytings <gregory@heytings.org> writes:

> A shorter (and therefore better) patch to solve this issue has been
> proposed in bug#48280.  It should be applied instead of this one.

Your version of the patch looked more obviously (well, somewhat) correct
to me -- since it didn't change when the directory was created, if I
read the two patches correctly.  (Which may be important if there's a
race condition.)

And since the copyright clerk just said that your copyright assignment
is complete, I've now applied and pushed your patch to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-24 22:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 20:48 bug#47960: 28.0.50; delete-by-moving-to-trash fails on directories? Protesilaos Stavrou
2021-04-22 21:11 ` Gregory Heytings
2021-04-22 21:42   ` Protesilaos Stavrou
2021-04-23 20:39     ` Gregory Heytings
2021-05-07 20:41       ` Gregory Heytings
2021-05-24 22:26         ` Lars Ingebrigtsen

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).