bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi All,

If one enables `delete-by-moving-to-trash' and tries to delete a
directory with the same name twice from dired, dired won't let one do
so, resulting in "file-already-exists: File exists: </path/>" error
instead.

Start with `emacs -Q'.  Set:

    (setq delete-by-moving-to-trash t)

Open dired at "/tmp/" (or some other place of your choosing).  Clone
the org repository there with `M-& git clone
https://git.savannah.gnu.org/git/emacs/org-mode.git RET' (a comment on
why this below), refresh with `g', navigate to the corresponding
directory, and trash it with `D', confirm.  The operation succeeds.

Now, repeat it. Clone the repository and delete the directory again. I
get here "file-already-exists: File exists:
/home/<username>/.local/share/Trash/files/org-moderiQJ08", and the
directory is not trashed.

Of course, this has nothing to do with Org.  But I've tried to create
a simple file with `touch' or an empty directory with `mkdir' and the
issue does not arise then.  And I haven't figured out what is the
difference in the `org-mode' directory which triggers the problem, it
just happens to be the last failing case of an issue which happens
occasionally here.

Best regards,
Gustavo.



In GNU Emacs 28.2 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.20,
cairo version 1.16.0)
 of 2022-09-12 built on gusbrs-laptop
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Linux Mint 20.3

Configured using:
 'configure --with-mailutils --with-xwidgets --with-native-compilation
 --without-compress-install'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: pt_BR.UTF-8
  value of $LC_NUMERIC: pt_BR.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Messages

Minor modes in effect:
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-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-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny rfc822 mml mml-sec epa
derived epg rfc6068 epg-config gnus-util rmail rmail-loaddefs
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map text-property-search seq byte-opt gv bytecomp
byte-compile cconv mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils shell pcomplete comint ansi-color ring
dired-aux dired dired-loaddefs time-date subr-x cl-loaddefs cl-lib
iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads
xwidget-internal dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
x multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 94422 7608)
 (symbols 48 7290 0)
 (strings 32 22714 1404)
 (string-bytes 1 765786)
 (vectors 16 15111)
 (vector-slots 8 314769 11896)
 (floats 8 27 42)
 (intervals 56 1346 103)
 (buffers 992 13))

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Gustavo Barros <gusbrs.2016@gmail.com>
> Date: Sat, 22 Oct 2022 15:23:15 -0300
> 
> If one enables `delete-by-moving-to-trash' and tries to delete a
> directory with the same name twice from dired, dired won't let one do
> so, resulting in "file-already-exists: File exists: </path/>" error
> instead.
> 
> Start with `emacs -Q'.  Set:
> 
>     (setq delete-by-moving-to-trash t)
> 
> Open dired at "/tmp/" (or some other place of your choosing).  Clone
> the org repository there with `M-& git clone
> https://git.savannah.gnu.org/git/emacs/org-mode.git RET' (a comment on
> why this below), refresh with `g', navigate to the corresponding
> directory, and trash it with `D', confirm.  The operation succeeds.
> 
> Now, repeat it. Clone the repository and delete the directory again. I
> get here "file-already-exists: File exists:
> /home/<username>/.local/share/Trash/files/org-moderiQJ08", and the
> directory is not trashed.
> 
> Of course, this has nothing to do with Org.  But I've tried to create
> a simple file with `touch' or an empty directory with `mkdir' and the
> issue does not arise then.  And I haven't figured out what is the
> difference in the `org-mode' directory which triggers the problem, it
> just happens to be the last failing case of an issue which happens
> occasionally here.

Could you please set debug-on-error t, repeat the experiment, and post
the Lisp backtrace from the error?

Thanks.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi Eli,

On Thu, 27 Oct 2022 at 13:09, Eli Zaretskii <eliz@gnu.org> wrote:

Thanks for looking into this.

> Could you please set debug-on-error t, repeat the experiment, and post
> the Lisp backtrace from the error?

I take this means you can't reproduce? Anyway, of course I can send
the backtrace.

For some reason I don't understand, even though I get the message as
described in the original recipe, it did not generate a backtrace
(some error control somewhere?).

I could generate a backtrace with:

    (setq debug-on-error t)
    (setq delete-by-moving-to-trash t)
    (move-file-to-trash "/path/to/org-mode")

And it is (slightly edited, since it contained more info than I'd like
to share):

Debugger entered--Lisp error: (file-already-exists "File exists"
"/home/<username>/.local/share/Trash/files/org-mode83g...")
  make-directory-internal("/home/<username>/.local/share/Trash/files/org-mode83g...")
  make-directory("/home/<username>/.local/share/Trash/files/org-mode83g..." nil)
  copy-directory("/home/<username>/path/to/file/org-mode"
"/home/<username>/.local/share/Trash/files/org-mode83g..." t nil)
  rename-file("/home/<username>/path/to/file/org-mode"
"/home/<username>/.local/share/Trash/files/org-mode83g..." t)
  move-file-to-trash("/home/<username>/path/to/file/org-mode")
  (progn (move-file-to-trash "/home/<username>/path/to/file/org-mode"))
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  command-execute(eval-last-sexp)

Best regards,
Gustavo.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi Eli,

On Thu, 27 Oct 2022 at 14:07, Gustavo Barros <gusbrs.2016@gmail.com> wrote:

> Could you please set debug-on-error t, repeat the experiment, and post
> the Lisp backtrace from the error?

One more thing I just observed and which might be interesting in
understanding what's going on.  From trying things out to report now,
my "~/.local/share/Trash/files" contains:

    org-mode
    org-mode83gQ8O
    org-modeAZxK5B
    org-modepKMpkM
    org-moderWcC2T

The first one of them contains the full expected contents. All the
other ones are empty. So it seems somehow somewhere we are trying to
mkdir twice.

Best regards,
Gustavo.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Gustavo Barros <gusbrs.2016@gmail.com>
> Date: Thu, 27 Oct 2022 14:07:21 -0300
> Cc: 58721@debbugs.gnu.org
> 
> Debugger entered--Lisp error: (file-already-exists "File exists"
> "/home/<username>/.local/share/Trash/files/org-mode83g...")
>   make-directory-internal("/home/<username>/.local/share/Trash/files/org-mode83g...")

And the directory by that name really already exists under Trash?
That would mean something is wrong with make-temp-file on your system,
because it's supposed to return a different file name.  You will see
that move-file-to-trash calls make-temp-file near its end.

(I cannot myself try reproducing this because I don't have access to a
system with freedesktop.org-style Trash.)

Thanks.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Thu, 27 Oct 2022 at 14:30, Eli Zaretskii <eliz@gnu.org> wrote:

> And the directory by that name really already exists under Trash?
> That would mean something is wrong with make-temp-file on your system,
> because it's supposed to return a different file name.  You will see
> that move-file-to-trash calls make-temp-file near its end.

Well, not like this. It existed because Emacs created it. Indeed, a
Trash should be able to receive multiple files of the same name, and
they must be uniquified somehow, and that's what Emacs is doing by
appending a suffix to the file name.

But, to be clear on the process. Before starting things, I cleared my
Trash, so that "~/.local/share/Trash/files" is empty.

I clone the repo, and move it to trash with (move-file-to-trash
"/path/to/org-mode").

Now "~/.local/share/Trash/files" contains "org-mode", with proper
contents, as expected.

I clone again and move to trash again with (move-file-to-trash
"/path/to/org-mode"). Now it fails with "(file-already-exists "File
exists" "/home/<username>/.local/share/Trash/files/org-mode7AV...")",
and "~/.local/share/Trash/files" contains:

    org-mode
    org-mode7AVOuq

Where "org-mode7AVOuq" is empty. And "org-mode", with contents, is
still in "/path/to/org-mode".

> (I cannot myself try reproducing this because I don't have access to a
> system with freedesktop.org-style Trash.)

Understood, I'm at your disposal to send any information and perform
any tests you need.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Gustavo Barros <gusbrs.2016@gmail.com>
> Date: Thu, 27 Oct 2022 14:51:39 -0300
> Cc: 58721@debbugs.gnu.org
> 
> I clone the repo, and move it to trash with (move-file-to-trash
> "/path/to/org-mode").
> 
> Now "~/.local/share/Trash/files" contains "org-mode", with proper
> contents, as expected.
> 
> I clone again and move to trash again with (move-file-to-trash
> "/path/to/org-mode"). Now it fails with "(file-already-exists "File
> exists" "/home/<username>/.local/share/Trash/files/org-mode7AV...")",
> and "~/.local/share/Trash/files" contains:
> 
>     org-mode
>     org-mode7AVOuq
> 
> Where "org-mode7AVOuq" is empty. And "org-mode", with contents, is
> still in "/path/to/org-mode".

We need to understand when was org-mode7AVOuq created, and why does
Emacs tries to create it again.  AFAIU from your description,
org-mode7AVOuq was not created by the first move to trash, so why does
only the second move to trash cause the error?

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Thu, 27 Oct 2022 at 15:20, Eli Zaretskii <eliz@gnu.org> wrote:

> We need to understand when was org-mode7AVOuq created, and why does
> Emacs tries to create it again.

Agreed.

> AFAIU from your description,
> org-mode7AVOuq was not created by the first move to trash [...]

That is correct. After the first move to trash, only "org-mode" exists
in "~/.local/share/Trash/files". Indeed, at this point there's no need
for the name to be uniquified, since there was no other directory with
the same name there before the first move to trash.

> [...] so why does
> only the second move to trash cause the error?

As far as I can tell, the existence of
"~/.local/share/Trash/files/org-mode" triggers it. I'd presume that
its existence takes the execution path to some code branch (the one
which tries to uniquify the file name/calls make-temp-file) which
tries to somehow create the directory twice.

But there's more to it. In the original report I explained why I ended
up "cloning the org repo" for this. Indeed, when creating the
reproduction recipe, I've tried first to create a simple empty file
with `touch' and a simple empty dir with `mkdir', but those did not
trigger the error. This is utterly mysterious to me. Perhaps something
like "size induced delay with some asynchronous process"? But that's
just a (very) wild guess. Truth is, I'm at a loss. And I did go
through the rabbit role to some extent, which resulted in that other
report about `move-file-to-trash', but I could not understand what's
going on here.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Gustavo Barros <gusbrs.2016@gmail.com>
> Date: Thu, 27 Oct 2022 15:41:43 -0300
> Cc: 58721@debbugs.gnu.org
> 
> > [...] so why does
> > only the second move to trash cause the error?
> 
> As far as I can tell, the existence of
> "~/.local/share/Trash/files/org-mode" triggers it. I'd presume that
> its existence takes the execution path to some code branch (the one
> which tries to uniquify the file name/calls make-temp-file) which
> tries to somehow create the directory twice.

Perhaps step in Edebug through copy-directory, and see what's going on
there?  AFAIU, the problem happens inside that function.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Thu, 27 Oct 2022 at 15:41, Gustavo Barros <gusbrs.2016@gmail.com> wrote:

> > [...] so why does
> > only the second move to trash cause the error?
>
> As far as I can tell, the existence of
> "~/.local/share/Trash/files/org-mode" triggers it. I'd presume that
> its existence takes the execution path to some code branch (the one
> which tries to uniquify the file name/calls make-temp-file) which
> tries to somehow create the directory twice.
>
> But there's more to it. In the original report I explained why I ended
> up "cloning the org repo" for this. Indeed, when creating the
> reproduction recipe, I've tried first to create a simple empty file
> with `touch' and a simple empty dir with `mkdir', but those did not
> trigger the error. This is utterly mysterious to me. Perhaps something
> like "size induced delay with some asynchronous process"? But that's
> just a (very) wild guess. Truth is, I'm at a loss. And I did go
> through the rabbit role to some extent, which resulted in that other
> report about `move-file-to-trash', but I could not understand what's
> going on here.

I just had an idea here to test the "delay" hypothesis, and it paid off.

All of my tests so far were being done from one of two places, the
"/tmp/" dir, or the place in my user files where I keep Emacs cloned
libraries. And they share a characteristic in my system, neither is in
the same filesystem as "~/.local/share/Trash/". "/tmp/" is a tmpfs
mount, and my personal files are in a separate encrypted partition.

Hence, what I did now, was to follow the recipe, but instead cloning
to "~", which is in the same partition as the Trash. And, guess what?
The error does not occur!

So it seems indeed a delay is at play. And that this one is related to
bug#58781, after all. A "fix" there would "fix" here too. But by luck,
because it would make the move "quick enough". However, the behavior
does suggest there's really some double attempt to create the
directory somewhere in the code.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Thu, 27 Oct 2022 at 16:04, Eli Zaretskii <eliz@gnu.org> wrote:

> Perhaps step in Edebug through copy-directory, and see what's going on
> there?  AFAIU, the problem happens inside that function.

We were both replying at the same time. If that's what you need after
seeing my message which came right after this one, I can try to do it.
(I may need some guidance, but let me try by myself first before
troubling you with that).

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi Eli,

On Thu, 27 Oct 2022 at 16:04, Eli Zaretskii <eliz@gnu.org> wrote:

> Perhaps step in Edebug through copy-directory, and see what's going on
> there?  AFAIU, the problem happens inside that function.

I think I was able to narrow it down a little.

The empty directory is indeed created, when the file already exists,
by the call to `make-temp-file' at:

    (when (file-exists-p
           (file-name-concat trash-files-dir files-base))
      (setq overwrite t
            files-base (file-name-nondirectory
                        (make-temp-file
                         (file-name-concat
                          trash-files-dir files-base)
                         is-directory))))

But, at the same time, the `overwrite' flag is set to t, in this case.
I'm not sure why the file is actually created, I suppose that it is to
"reserve" that name and ensure nothing else takes it in the meantime.

At the end of the function, the call is done to:

    (rename-file fn new-fn overwrite)

But, when the operation is crossing filesystems and the file is large
enough, the `rename-file' will fail with "file exists", despite the
`OK-IF-ALREADY-EXISTS' argument being `t'.

You can try that with:

    (make-directory "~/.local/share/Trash/files/org-mode-foo-bar")
    (rename-file "/tmp/org-mode"
"~/.local/share/Trash/files/org-mode-foo-bar" t)

Provided "crossing filesystems" and "large enough" we get (I do, at
least) "(file-already-exists "File exists" ..."

WDYT? Can you reproduce this?

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Gustavo Barros <gusbrs.2016@gmail.com>
> Date: Thu, 27 Oct 2022 19:01:27 -0300
> Cc: 58721@debbugs.gnu.org
> 
> At the end of the function, the call is done to:
> 
>     (rename-file fn new-fn overwrite)
> 
> But, when the operation is crossing filesystems and the file is large
> enough, the `rename-file' will fail with "file exists", despite the
> `OK-IF-ALREADY-EXISTS' argument being `t'.

This sounds very strange.  Why would the failure depend on the size of
the file/directory and on whether it does or doesn't cross
filesystems?  I see nothing in the code involved in this that could
cause that.  Perhaps on your system something happens in the
background, due to one of the filesystems being encrypted or
something?

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Fri, 28 Oct 2022 at 04:47, Eli Zaretskii <eliz@gnu.org> wrote:

> This sounds very strange.

It baffles me too. Yet it still fails like clockwork here.

>  Why would the failure depend on the size of
> the file/directory and on whether it does or doesn't cross
> filesystems?  I see nothing in the code involved in this that could
> cause that.

Well, "crossing filesystems" and "large enough" are admittedly a
working hypothesis. I don't really know that's what makes it fail. I
just know that I could create other cases which do not fail, out of
these conditions.

I take this means you still cannot reproduce... I'm sorry, I don't
know what else I can try, I'm out of my depth here. Do you have
anything you'd like me to try? Can anyone else reproduce this, or is
it really just me?

>  Perhaps on your system something happens in the
> background, due to one of the filesystems being encrypted or
> something?

The encrypted partition is open and mounted, as far as the system can
see, it is just another ext4 partition. More clearly, it cannot be
encryption per se, since the "/tmp/" folder is not encrypted at all,
it is just a tmpfs mount. It's fstab entry is:

    tmpfs /tmp tmpfs nodev,nosuid,size=6G 0 0

Of course, it might still be "or something". But I don't do anything
out of the ordinary here that I'm aware of.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Gustavo Barros <gusbrs.2016@gmail.com>
> Date: Fri, 28 Oct 2022 07:43:08 -0300
> Cc: 58721@debbugs.gnu.org
> 
> I take this means you still cannot reproduce...

I don't have the necessary configuration here to try reproducing.  You
said it doesn't happen when moving to trash just a small directory, so
it's quite clear that something specific to your system is at work here.

> I'm sorry, I don't know what else I can try, I'm out of my depth
> here. Do you have anything you'd like me to try?

Try figuring out why the make-directory call fails.  What is the name
of the directory that fails the call, and why?

> Can anyone else reproduce this, or is it really just me?

Yes, if someone else could reproduce, it could help.

> >  Perhaps on your system something happens in the
> > background, due to one of the filesystems being encrypted or
> > something?
> 
> The encrypted partition is open and mounted, as far as the system can
> see, it is just another ext4 partition. More clearly, it cannot be
> encryption per se, since the "/tmp/" folder is not encrypted at all,
> it is just a tmpfs mount. It's fstab entry is:
> 
>     tmpfs /tmp tmpfs nodev,nosuid,size=6G 0 0
> 
> Of course, it might still be "or something". But I don't do anything
> out of the ordinary here that I'm aware of.

Here are some questions for you to try to answer:

  . Does this happen only with moves across filesystems?
  . Would any target filesystem do, or just some special kind(s)/
  . What is the difference between the first move and the subsequent
    moves that triggers the error?

Thanks.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Fri, 28 Oct 2022 at 08:44, Eli Zaretskii <eliz@gnu.org> wrote:

> I don't have the necessary configuration here to try reproducing.  You
> said it doesn't happen when moving to trash just a small directory, so
> it's quite clear that something specific to your system is at work here.

This is no longer about `move-file-to-trash', but about `rename-file'
third argument. So you don't need a freedesktop.org Trash to
reproduce. Of course, it doesn't mean you can reproduce it, since
there might still be something else which is system specific.

I've tried to isolate things from the Trash issue, and I could
reproduce the problem with the following steps.

I grabbed two USB sticks, formatted both of them with EXT4 (I used
Mint's "USB Stick formatter" tool, I suppose it is pretty standard in
what it does), labeled each "orig" and "dest". Mounted both.

In the terminal, I did:

$ cd /path/to/orig
$ mkdir barbaz
$ cd barbaz
$ dd if=/dev/zero of=zero.file bs=1024 count=204800
$ cd /path/to/dest
$ mkdir barbaz-foobar

I started "emacs -Q", and issued:

(rename-file "/path/to/orig/barbaz" "/path/to/dest/barbaz-foobar" t)

And got "(file-already-exists "File exists" "/path/to/dest/barbaz-foobar")"

> Try figuring out why the make-directory call fails.  What is the name
> of the directory that fails the call, and why?

I can try further, but I did try quite hard already.
And I don't think this has anything to do with particular names, I
used foo bar stuff above on purpose, and they are our quintessential
"arbitrary".

> Here are some questions for you to try to answer:
>
>   . Does this happen only with moves across filesystems?

Formally, what I tested is that when I tried the same recipe of the
original report, but with the "org-mode" directory I was trying to
delete/trash located in the same partition as the Trash, the error did
not occur.

So, the answer is that, as far as I can tell "yes", but the reasoning
is based on small sample inference. "Educated" perhaps, but I can't
make a rule out of it.

>   . Would any target filesystem do, or just some special kind(s)/

In the example above I used standard EXT4 ones. I'd say this should
work on them, but I haven't tried other ones. Would you like me to
test anything specific in this regard?

>   . What is the difference between the first move and the subsequent
>     moves that triggers the error?

I think this question only applies to `move-file-to-trash', we don't
need "two moves" anymore with the example boiled down to
`rename-file'. We know `move-file-to-trash' does create an empty dir
there, and this only happens when a file of the same name exists
(which required the "first move"), but it also sets the `overwrite'
flag, the problem is that the flag fails.

Thank *you*.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Stefan Kangas]

Gustavo Barros <gusbrs.2016@gmail.com> writes:

> I've tried to isolate things from the Trash issue, and I could
> reproduce the problem with the following steps.
>
> I grabbed two USB sticks, formatted both of them with EXT4 (I used
> Mint's "USB Stick formatter" tool, I suppose it is pretty standard in
> what it does), labeled each "orig" and "dest". Mounted both.
>
> In the terminal, I did:
>
> $ cd /path/to/orig
> $ mkdir barbaz
> $ cd barbaz
> $ dd if=/dev/zero of=zero.file bs=1024 count=204800
> $ cd /path/to/dest
> $ mkdir barbaz-foobar
>
> I started "emacs -Q", and issued:
>
> (rename-file "/path/to/orig/barbaz" "/path/to/dest/barbaz-foobar" t)
>
> And got "(file-already-exists "File exists" "/path/to/dest/barbaz-foobar")"

I get the same result.  But I think this is expected, as the
`rename-file' docstring says:

    For NEWNAME to be recognized as a directory name, it should
    end in a slash.

So this function seems to be working as documented.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi Stefan,

On Fri, 28 Oct 2022 at 12:26, Stefan Kangas <stefankangas@gmail.com> wrote:

> I get the same result.  But I think this is expected, as the
> `rename-file' docstring says:
>
>     For NEWNAME to be recognized as a directory name, it should
>     end in a slash.
>
> So this function seems to be working as documented.

Thanks for trying! I'm glad I'm not mad. :-)

I had missed this bit of the docstring, and you're right, of course.
So, it seems we are back to `move-file-to-trash', and how it calls
`rename-file'.

I've redefined `move-file-to-trash' to message `new-fn` right before
`rename-file' is being called. And tested both:

    (move-file-to-trash "/path/to/orig/barbaz")
    (move-file-to-trash "/path/to/orig/barbaz/")

And both cases output things like:

Back to top level
/home/<username>/.local/share/Trash/files/barbazOfwLav
Entering debugger...
Back to top level
/home/<username>/.local/share/Trash/files/barbazVICScf
Entering debugger...
Back to top level
/home/<username>/.local/share/Trash/files/barbazVd4241
Entering debugger...
Back to top level
/home/<username>/.local/share/Trash/files/barbaz04EjUm
Entering debugger...
Back to top level

That is, no end slash appended to the directory at
`move-file-to-trash'. So, it appears we have a good suspect. Why it
works when the filesystem is the same still beats me.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Fri, 28 Oct 2022 at 13:06, Gustavo Barros <gusbrs.2016@gmail.com> wrote:

> > I get the same result.  But I think this is expected, as the
> > `rename-file' docstring says:
> >
> >     For NEWNAME to be recognized as a directory name, it should
> >     end in a slash.
> >
> > So this function seems to be working as documented.

> That is, no end slash appended to the directory at
> `move-file-to-trash'. So, it appears we have a good suspect. Why it
> works when the filesystem is the same still beats me.

Uhm, it is still not this. The previous sentence in the docstring you
(Stefan) mentioned is:

    If NEWNAME is a directory name, rename FILE to a like-named file under
    NEWNAME.  For NEWNAME to be recognized as a directory name, it should
    end in a slash.

And, indeed, if we do:

    (rename-file "/path/to/orig/barbaz" "/path/to/dest/barbaz-foobar/" t)

the operation succeeds, but the result is:

    /path/to/dest/barbaz-foobar/barbaz/zero.file

And that's not what we want, or not what `move-file-to-trash' is
trying to do. And that is precisely why it is passing the
`ok-if-already-exists' as `t' there in the first place.

In other words, as far as I can see, the sentence:

    For NEWNAME to be recognized as a directory name, it should
    end in a slash.

should not imply that if `ok-if-already-exists' is `t' `rename-file'
should fail to overwrite, whether the `newname' is a directory or not.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

Eli Zaretskii wrote:

> > From: Gustavo Barros <gusbrs.2016@gmail.com>
[...]
> > Can anyone else reproduce this, or is it really just me?
> 
> Yes, if someone else could reproduce, it could help.

I can reproduce it (Emacs 28.2, Debian 11).  I copied my ~/src/emacs-git
tree to various directories and then tried trashing it.

The failure only happened when trashing a tree that was on a separate
filesystem from $HOME, and then it happened reliably.  The type of
filesystem that contained the copy didn't seem to matter--I saw the
problem with both ext4 and vfat.  My $HOME is ext4.

When trashing a directory tree on the same filesystem, it's sufficient
to rename the top-level directory into the trash hierarchy.  But Unix
doesn't support that across filesystems, so the directory tree needs to
be copied into the trash hierarchy and then deleted at the original
location.  Since we see an empty directory after the error, I assume
that something is going wrong with the copy phase.

I set a breakpoint in #'copy-directory and got this stack:

* copy-directory("/tmp/emacs-git" "/home/kupfer/.local/share/Trash/files/emacs-gitH0l..." t nil)
  rename-file("/tmp/emacs-git" "/home/kupfer/.local/share/Trash/files/emacs-gitH0l..." t)
  move-file-to-trash("/tmp/emacs-git")
  delete-directory("/tmp/emacs-git" always t)
  dired-delete-file("/tmp/emacs-git" top t)
  dired-internal-do-deletions((("/tmp/emacs-git" . #<marker at 233 in tmp<>>)) nil t)
  dired-do-flagged-delete()
  funcall-interactively(dired-do-flagged-delete)
  call-interactively(dired-do-flagged-delete nil nil)
  command-execute(dired-do-flagged-delete)

The target directory (emacs-gitH0lx1e) already exists.

In fact, it looks like the target directory already exists when
#'rename-file is called.  But I think that's expected.

I clicked on the "..." in the backtrace to see whether the new name ends
with a "/", and it does not.

Hrm.  Shouldn't the call to copy-directory look like

  copy-directory("/tmp/emacs-git" "/home/kupfer/.local/share/Trash/files/emacs-gitH0l..." t nil t)
            ^^^

?

mike

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi Mike,

On Sat, 29 Oct 2022 at 02:25, Mike Kupfer <mkupfer@alum.berkeley.edu> wrote:

> I can reproduce it (Emacs 28.2, Debian 11).  I copied my ~/src/emacs-git
> tree to various directories and then tried trashing it.

Thank you for testing!

Gustavo.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

Mike Kupfer wrote:

> * copy-directory("/tmp/emacs-git" "/home/kupfer/.local/share/Trash/files/emacs-gitH0l..." t nil)
>   rename-file("/tmp/emacs-git" "/home/kupfer/.local/share/Trash/files/emacs-gitH0l..." t)
>   move-file-to-trash("/tmp/emacs-git")
>   delete-directory("/tmp/emacs-git" always t)
>   dired-delete-file("/tmp/emacs-git" top t)
>   dired-internal-do-deletions((("/tmp/emacs-git" . #<marker at 233 in tmp<>>)) nil t)
>   dired-do-flagged-delete()
>   funcall-interactively(dired-do-flagged-delete)
>   call-interactively(dired-do-flagged-delete nil nil)
>   command-execute(dired-do-flagged-delete)
[...]
> Hrm.  Shouldn't the call to copy-directory look like
> 
>   copy-directory("/tmp/emacs-git" "/home/kupfer/.local/share/Trash/files/emacs-gitH0l..." t nil t)
>                ^^^

I played with this some more this morning, this time on Emacs 29.  It
looks like rename-file should have called copy-directory as

copy-directory("/tmp/emacs-git"
  "/home/kupfer/.local/share/Trash/files/emacs-gitH0lx1e/" t nil t)

That is, 2 changes from current behavior are needed: append a "/" to the
target name, and specify copy-contents as t.

The test case that I used to emulate the current behavior is

1. mkdir a; touch a/b

2. mkdir /tmp/newa
(This assumes that /tmp is a different filesystem than the filesystem
where "a" was created.)

3. M-: (copy-directory "a" "/tmp/newa" t nil) RET

That gives the already-exists error.

With (copy-directory "a" "/tmp/newa/" t nil), I get /tmp/newa/a/b, which
isn't what's wanted for the Trash directory.

With (copy-directory "a" "/tmp/newa/" t nil t), I get /tmp/newa/b, which
is what's desired.

mike

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> Date: Sat, 29 Oct 2022 08:24:53 -0700
> 
> I played with this some more this morning, this time on Emacs 29.  It
> looks like rename-file should have called copy-directory as
> 
> copy-directory("/tmp/emacs-git"
>   "/home/kupfer/.local/share/Trash/files/emacs-gitH0lx1e/" t nil t)

I don't think we can change how rename-file behaves when the argument
is a directory.  Any changes we need to make must be in
move-file-to-trash, not in primitives it calls.

Thanks.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

Eli Zaretskii wrote:

> > From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> > Date: Sat, 29 Oct 2022 08:24:53 -0700
> > 
> > I played with this some more this morning, this time on Emacs 29.  It
> > looks like rename-file should have called copy-directory as
> > 
> > copy-directory("/tmp/emacs-git"
> >   "/home/kupfer/.local/share/Trash/files/emacs-gitH0lx1e/" t nil t)
> 
> I don't think we can change how rename-file behaves when the argument
> is a directory.  Any changes we need to make must be in
> move-file-to-trash, not in primitives it calls.

Yes, that makes sense.

So... (rename-file "a" "/tmp/newa" t) gives the original error.

(rename-file "a" "/tmp/newa/" t) gives us /tmp/newa/a/b.

What we want is /tmp/newa/b.

I can think of 2 ways forward.  One is to add an optional argument to
rename-file to get the desired behavior, like the copy-contents argument
to copy-directory.

The other is for move-file-to-trash to call rename-file on the top-level
contents of the directory that is being trashed ("a/b" in my simple test
case), rather than on the directory itself.

I think the first approach is preferable, in that it parallels the
definition of copy-directory.  But either should work.

mike

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> cc: 58721@debbugs.gnu.org, gusbrs.2016@gmail.com
> Date: Sat, 29 Oct 2022 09:32:52 -0700
> 
> So... (rename-file "a" "/tmp/newa" t) gives the original error.
> 
> (rename-file "a" "/tmp/newa/" t) gives us /tmp/newa/a/b.
> 
> What we want is /tmp/newa/b.
> 
> I can think of 2 ways forward.  One is to add an optional argument to
> rename-file to get the desired behavior, like the copy-contents argument
> to copy-directory.
> 
> The other is for move-file-to-trash to call rename-file on the top-level
> contents of the directory that is being trashed ("a/b" in my simple test
> case), rather than on the directory itself.
> 
> I think the first approach is preferable, in that it parallels the
> definition of copy-directory.  But either should work.

Yet another possibility is to refrain from calling rename-file when
the moved file is a directory, and instead to do what rename-file
does, with a twist, "by hand".  That is what I actually prefer, as
nothing is really wrong with rename-file.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

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

Eli Zaretskii wrote:

> Yet another possibility is to refrain from calling rename-file when
> the moved file is a directory, and instead to do what rename-file
> does, with a twist, "by hand".  That is what I actually prefer, as
> nothing is really wrong with rename-file.

I'm afraid I don't understand your suggestion.

I've attached the proof-of-concept patch that I came up with, which just
modifies move-file-to-trash.  I've tested it with files and directories,
both same-filesystem and crossing filesystems.  Did you have in mind
something similar?

cheers,
mike

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: proof-of-concept for fix --]
[-- Type: text/x-diff, Size: 1619 bytes --]

diff --git a/lisp/files.el b/lisp/files.el
index 1e1ec6127d..63786ec103 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8565,10 +8565,29 @@ move-file-to-trash
 		    (setq files-base (substring (file-name-nondirectory info-fn)
                                                 0 (- (length ".trashinfo"))))
 		    (write-region nil nil info-fn nil 'quiet info-fn)))
-		 ;; Finally, try to move the file to the trashcan.
+		 ;; Finally, try to move the item to the trashcan.  If
+                 ;; it's a file, just move it.  If it's a directory,
+                 ;; there's no way to invoke rename-file to replace
+                 ;; new-fn with fn, so move everything in fn and then
+                 ;; delete it.
 		 (let ((delete-by-moving-to-trash nil)
 		       (new-fn (file-name-concat trash-files-dir files-base)))
-		   (rename-file fn new-fn overwrite)))))))))
+                   (if (not (file-directory-p fn))
+                       (rename-file fn new-fn overwrite)
+                     (make-directory new-fn)
+                     (mapc  
+                      (lambda (f1)
+                        (let ((src (file-name-concat fn f1))
+	                      (targ (file-name-concat new-fn f1)))
+                          (cond
+                           ((or (string= f1 ".")
+	                        (string= f1 ".."))
+                            t)
+                           (t
+                            (rename-file src targ)))))
+                      (directory-files fn))
+                     (delete-directory fn))))))))))
+
 
 \f
 (defsubst file-attribute-type (attributes)

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> cc: 58721@debbugs.gnu.org, gusbrs.2016@gmail.com
> Date: Sat, 29 Oct 2022 17:09:06 -0700
> 
> Eli Zaretskii wrote:
> 
> > Yet another possibility is to refrain from calling rename-file when
> > the moved file is a directory, and instead to do what rename-file
> > does, with a twist, "by hand".  That is what I actually prefer, as
> > nothing is really wrong with rename-file.
> 
> I'm afraid I don't understand your suggestion.
> 
> I've attached the proof-of-concept patch that I came up with, which just
> modifies move-file-to-trash.  I've tested it with files and directories,
> both same-filesystem and crossing filesystems.  Did you have in mind
> something similar?

Yes, thanks.  I just thought you'd be able to call copy-directory,
similarly (but differently) to what rename-file does, instead of using
your lambda-function.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

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

Eli Zaretskii wrote:

> I just thought you'd be able to call copy-directory, similarly (but
> differently) to what rename-file does, instead of using your
> lambda-function.

Ah, of course.  Thanks, yes, that's cleaner.  How does the attached
patch look?

mike


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: proposed patch --]
[-- Type: text/x-diff, Size: 1734 bytes --]

From e8fdbeb44dbea9ebf5679c23bf3ad5c5a61141ec Mon Sep 17 00:00:00 2001
From: Mike Kupfer <mkupfer@alum.berkeley.edu>
Date: Sun, 30 Oct 2022 10:31:11 -0700
Subject: [PATCH] Fix cross-filesystem directory trashing (Bug#58721)

* lisp/files.el (move-file-to-trash): When trashing a directory,
copy it into the trash folder and then delete it, rather than
using rename-file.
---
 lisp/files.el | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 1e1ec6127d..b347815314 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8565,10 +8565,20 @@ move-file-to-trash
 		    (setq files-base (substring (file-name-nondirectory info-fn)
                                                 0 (- (length ".trashinfo"))))
 		    (write-region nil nil info-fn nil 'quiet info-fn)))
-		 ;; Finally, try to move the file to the trashcan.
+		 ;; Finally, try to move the item to the trashcan.  If
+                 ;; it's a file, just move it.  If it's a directory,
+                 ;; rename-file will not work across filesystems, so
+                 ;; copy the directory tree and then delete it.
 		 (let ((delete-by-moving-to-trash nil)
 		       (new-fn (file-name-concat trash-files-dir files-base)))
-		   (rename-file fn new-fn overwrite)))))))))
+                   (if (not (file-directory-p fn))
+                       (rename-file fn new-fn overwrite)
+                     (make-directory new-fn)
+                     (copy-directory fn
+                                     (file-name-as-directory new-fn)
+                                     t nil t)
+                     (delete-directory fn t))))))))))
+
 
 \f
 (defsubst file-attribute-type (attributes)
-- 
2.30.2

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> cc: 58721@debbugs.gnu.org, gusbrs.2016@gmail.com
> Date: Sun, 30 Oct 2022 10:40:24 -0700
> 
> Eli Zaretskii wrote:
> 
> > I just thought you'd be able to call copy-directory, similarly (but
> > differently) to what rename-file does, instead of using your
> > lambda-function.
> 
> Ah, of course.  Thanks, yes, that's cleaner.  How does the attached
> patch look?

Looks good, but maybe add to the comment the reason _why_ we don't use
rename-file for directories.  You explained _what_ we do, but not
_why_.

Thanks.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

Eli Zaretskii wrote:

> Looks good, but maybe add to the comment the reason _why_ we don't use
> rename-file for directories.  You explained _what_ we do, but not
> _why_.

Do you mean something like

-----8<-----8<-----
Fix cross-filesystem directory trashing (Bug#58721)

* lisp/files.el (move-file-to-trash): When trashing a directory,
copy it into the trash folder and then delete it, rather than
using rename-file.  rename-file can only be used when the
to-be-trashed directory and the trashcan live in the same filesystem;
this approach handles both that case and the different-filesystem case.
----->8----->8-----

?

mike

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> cc: 58721@debbugs.gnu.org, gusbrs.2016@gmail.com
> Date: Sun, 30 Oct 2022 11:18:29 -0700
> 
> Eli Zaretskii wrote:
> 
> > Looks good, but maybe add to the comment the reason _why_ we don't use
> > rename-file for directories.  You explained _what_ we do, but not
> > _why_.
> 
> Do you mean something like
> 
> -----8<-----8<-----
> Fix cross-filesystem directory trashing (Bug#58721)
> 
> * lisp/files.el (move-file-to-trash): When trashing a directory,
> copy it into the trash folder and then delete it, rather than
> using rename-file.  rename-file can only be used when the
> to-be-trashed directory and the trashcan live in the same filesystem;
> this approach handles both that case and the different-filesystem case.
> ----->8----->8-----

I meant in the code, not the log message, and I meant to tell more
detail.

But never mind, I can always add it later myself.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

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

Eli Zaretskii wrote:

> I meant in the code, not the log message, and I meant to tell more
> detail.
> 
> But never mind, I can always add it later myself.

Well, it turns out I hadn't tested my previous patch enough.  It handled
the first deletion of directory foo, but not subsequent deletions (which
was the entire point of the fix, sigh).  I've attached another patch.
I took a stab at improving the commenting in the code while I was
there.

mike

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2 fix --]
[-- Type: text/x-diff, Size: 2274 bytes --]

From 402c7c819dbe2a1cd96462be699ae2af1f5c2440 Mon Sep 17 00:00:00 2001
From: Mike Kupfer <mkupfer@alum.berkeley.edu>
Date: Sun, 30 Oct 2022 10:31:11 -0700
Subject: [PATCH] Fix cross-filesystem directory trashing (Bug#58721)

* lisp/files.el (move-file-to-trash): When trashing a directory with
the same name as something that's already in the trash, copy it into
the trash folder and then delete it, rather than using rename-file.
---
 lisp/files.el | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 1e1ec6127d..6bfb3aa738 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8565,10 +8565,27 @@ move-file-to-trash
 		    (setq files-base (substring (file-name-nondirectory info-fn)
                                                 0 (- (length ".trashinfo"))))
 		    (write-region nil nil info-fn nil 'quiet info-fn)))
-		 ;; Finally, try to move the file to the trashcan.
+		 ;; Finally, try to move the item to the trashcan.  If
+                 ;; it's a file, just move it.  Things are more
+                 ;; complicated for directories.  If the target
+                 ;; directory already exists (due to uniquification)
+                 ;; and the trash directory is in a different
+                 ;; filesystem, rename-file will error out, even when
+                 ;; 'overwrite' is non-nil.  Rather than worry about
+                 ;; whether we're crossing filesystems, just check if
+                 ;; we've moving a directory and the target directory
+                 ;; already exists.  That handles both the
+                 ;; same-filesystem and cross-filesystem cases.
 		 (let ((delete-by-moving-to-trash nil)
 		       (new-fn (file-name-concat trash-files-dir files-base)))
-		   (rename-file fn new-fn overwrite)))))))))
+                   (if (or (not is-directory)
+                           (not (file-exists-p new-fn)))
+                       (rename-file fn new-fn overwrite)
+                     (copy-directory fn
+                                     (file-name-as-directory new-fn)
+                                     t nil t)
+                     (delete-directory fn t))))))))))
+
 
 \f
 (defsubst file-attribute-type (attributes)
-- 
2.30.2

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Sun, 30 Oct 2022 at 19:20, Mike Kupfer <mkupfer@alum.berkeley.edu> wrote:

> Well, it turns out I hadn't tested my previous patch enough.  It handled
> the first deletion of directory foo, but not subsequent deletions (which
> was the entire point of the fix, sigh).  I've attached another patch.
> I took a stab at improving the commenting in the code while I was
> there.

I may be wrong but, as far as my reading goes, I think this might
misbehave if the "directory" is a symlink. `is-directory' is built as
`(file-directory-p fn)' which returns t even if it is a symlink to a
directory. Would things such as `copy-directory` (with `copy-contents`
arg t) and `delete-directory' work just as well in such a case?

Besides that, in general, imho I cannot think of this issue as
something else other than a misbehavior of `rename-file', so that the
patch in these terms feels like a workaround. I understand Eli
disagrees. True, most likely this is just a muggle (me) being naive,
so I'll try to watch and learn.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

Gustavo Barros wrote:

> I may be wrong but, as far as my reading goes, I think this might
> misbehave if the "directory" is a symlink. `is-directory' is built as
> `(file-directory-p fn)' which returns t even if it is a symlink to a
> directory. 

Good point, thanks.  I think this points out a pre-existing issue with
move-file-to-trash, in that the code to create a unique trash name will
create a directory, rather than a file.

> Besides that, in general, imho I cannot think of this issue as
> something else other than a misbehavior of `rename-file', so that the
> patch in these terms feels like a workaround.

I agree that it's an inconsistency in the behavior of rename-file.  If
the thing being renamed is a file, the target gets replaced, whether or
not we're crossing filesystems.  But if it's a directory, the target
gets replaced if it's in the same filesystem, but it's an error if the
target is in a different filesystem.  If I understand Eli's concern,
it's a question of whether making the behavior more consistent is worth
the risk that it would break existing code--code that could assume the
current behavior.

I'd like to resolve the symlink behavior before pushing any fix.  But
I've been feeling increasingly unwell as the day has progressed, so I
think I'll stop work on this bug until I'm feeling better.

mike

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Sun, 30 Oct 2022 at 21:01, Mike Kupfer <mkupfer@alum.berkeley.edu> wrote:

> Good point, thanks.  I think this points out a pre-existing issue with
> move-file-to-trash, in that the code to create a unique trash name will
> create a directory, rather than a file.

I agree, I also think that there was a pre-existing inconsistency
there, but `rename-file` would cover it up later on. But trying to do
it manually, complicates things in this regard as well.

> I agree that it's an inconsistency in the behavior of rename-file.  If
> the thing being renamed is a file, the target gets replaced, whether or
> not we're crossing filesystems.  But if it's a directory, the target
> gets replaced if it's in the same filesystem, but it's an error if the
> target is in a different filesystem.  If I understand Eli's concern,
> it's a question of whether making the behavior more consistent is worth
> the risk that it would break existing code--code that could assume the
> current behavior.

I understand that concern too. But we don't even understand well (as
far as I can see) why it fails when crossing filesystems, and are
already rushing to a workaround. I just offered some respectful
resistance. And if, after all, the decision is not to touch
`rename-file' because it's risky, so that a workaround is really what
is intended, I'll get that.

> I'd like to resolve the symlink behavior before pushing any fix.  But
> I've been feeling increasingly unwell as the day has progressed, so I
> think I'll stop work on this bug until I'm feeling better.

Get well soon! :-)

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> cc: Eli Zaretskii <eliz@gnu.org>, 58721@debbugs.gnu.org
> Date: Sun, 30 Oct 2022 17:01:41 -0700
> 
> Gustavo Barros wrote:
> 
> > I may be wrong but, as far as my reading goes, I think this might
> > misbehave if the "directory" is a symlink. `is-directory' is built as
> > `(file-directory-p fn)' which returns t even if it is a symlink to a
> > directory. 
> 
> Good point, thanks.  I think this points out a pre-existing issue with
> move-file-to-trash, in that the code to create a unique trash name will
> create a directory, rather than a file.

What is the expected semantics of moving a symlink to trashcan?  Is it
supposed to move the symlink or its target?  (I'd think it's the
former, but maybe my instincts are wrong.)  If the expectations are
that the symlink is moved, then all we need to do is to treat symlinks
as regular files, by augmenting file-directory-p not to dupe us.

> > Besides that, in general, imho I cannot think of this issue as
> > something else other than a misbehavior of `rename-file', so that the
> > patch in these terms feels like a workaround.
> 
> I agree that it's an inconsistency in the behavior of rename-file.  If
> the thing being renamed is a file, the target gets replaced, whether or
> not we're crossing filesystems.  But if it's a directory, the target
> gets replaced if it's in the same filesystem, but it's an error if the
> target is in a different filesystem.  If I understand Eli's concern,
> it's a question of whether making the behavior more consistent is worth
> the risk that it would break existing code--code that could assume the
> current behavior.

I'm okay with filing another bug report about rename-file, and
discussing this there.  But that's a separate issue, and fix of this
bug should not depend on that.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Mon, 31 Oct 2022 at 09:49, Eli Zaretskii <eliz@gnu.org> wrote:

> What is the expected semantics of moving a symlink to trashcan?  Is it
> supposed to move the symlink or its target?  (I'd think it's the
> former, but maybe my instincts are wrong.)  If the expectations are
> that the symlink is moved, then all we need to do is to treat symlinks
> as regular files, by augmenting file-directory-p not to dupe us.

I'm not sure either, but my instincts are the same as yours. If that's
any reference, I just tested here, and that's what "gio trash" does
(moves the symlink, not the target).

> I'm okay with filing another bug report about rename-file, and
> discussing this there.  But that's a separate issue, and fix of this
> bug should not depend on that.

Understood.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

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

Gustavo Barros wrote:

> On Mon, 31 Oct 2022 at 09:49, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > What is the expected semantics of moving a symlink to trashcan?  Is it
> > supposed to move the symlink or its target?  (I'd think it's the
> > former, but maybe my instincts are wrong.)  If the expectations are
> > that the symlink is moved, then all we need to do is to treat symlinks
> > as regular files, by augmenting file-directory-p not to dupe us.
> 
> I'm not sure either, but my instincts are the same as yours. If that's
> any reference, I just tested here, and that's what "gio trash" does
> (moves the symlink, not the target).

Yes, I tried a few graphical file browsers (Thunar, Caja, and Dolphin),
and they all move the symlink, not the target, to Trash.

After getting my test setup straightened out, I think I have a fix for
the symlink issue and for the issue that Gustavo originally reported
(cross-filesystem trashing fails when there's already a directory with
the same name in Trash).

I've committed these fixes separately; see attached.  Gustavo, can you
try these out and make sure they handle your use case(s)?

> > I'm okay with filing another bug report about rename-file, and
> > discussing this there.  But that's a separate issue, and fix of this
> > bug should not depend on that.
> 
> Understood.

Okay.  I'm not planning to follow up on this, Gustavo, so if you'd like
to lobby for a change to rename-file, you'll need to open a bug for it
(if you haven't already).

cheers,
mike

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix duplicate trashing --]
[-- Type: text/x-diff, Size: 2280 bytes --]

From f749a1dd7875a83e6415d8c512c5659f0f23c834 Mon Sep 17 00:00:00 2001
From: Mike Kupfer <mkupfer@alum.berkeley.edu>
Date: Sun, 30 Oct 2022 10:31:11 -0700
Subject: [PATCH 1/2] Fix cross-filesystem directory trashing (Bug#58721)

* lisp/files.el (move-file-to-trash): When trashing a directory with
the same name as something that's already in the trash, copy it into
the trash folder and then delete it, rather than using rename-file.
---
 lisp/files.el | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index a2825322580..de4f68ebd51 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8596,10 +8596,27 @@ move-file-to-trash
 		    (setq files-base (substring (file-name-nondirectory info-fn)
                                                 0 (- (length ".trashinfo"))))
 		    (write-region nil nil info-fn nil 'quiet info-fn)))
-		 ;; Finally, try to move the file to the trashcan.
+		 ;; Finally, try to move the item to the trashcan.  If
+                 ;; it's a file, just move it.  Things are more
+                 ;; complicated for directories.  If the target
+                 ;; directory already exists (due to uniquification)
+                 ;; and the trash directory is in a different
+                 ;; filesystem, rename-file will error out, even when
+                 ;; 'overwrite' is non-nil.  Rather than worry about
+                 ;; whether we're crossing filesystems, just check if
+                 ;; we've moving a directory and the target directory
+                 ;; already exists.  That handles both the
+                 ;; same-filesystem and cross-filesystem cases.
 		 (let ((delete-by-moving-to-trash nil)
 		       (new-fn (file-name-concat trash-files-dir files-base)))
-		   (rename-file fn new-fn overwrite)))))))))
+                   (if (or (not is-directory)
+                           (not (file-exists-p new-fn)))
+                       (rename-file fn new-fn overwrite)
+                     (copy-directory fn
+                                     (file-name-as-directory new-fn)
+                                     t nil t)
+                     (delete-directory fn t))))))))))
+
 
 \f
 (defsubst file-attribute-type (attributes)
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: fix trashing of symlink --]
[-- Type: text/x-diff, Size: 1004 bytes --]

From 4aefb5b5964e1aaad0c74238e14ad0a1e3dc4e7a Mon Sep 17 00:00:00 2001
From: Mike Kupfer <mkupfer@alum.berkeley.edu>
Date: Sun, 20 Nov 2022 16:44:20 -0800
Subject: [PATCH 2/2] Fix trashing of symlink that points at a directory

* lisp/files.el (move-file-to-trash): Redefine is-directory so
that it is false for symlinks.
---
 lisp/files.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index de4f68ebd51..a78d1627689 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8566,7 +8566,8 @@ 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))
+                      (is-directory (and (file-directory-p fn)
+					 (not (file-symlink-p fn))))
                       (overwrite nil)
                       info-fn)
                  ;; We're checking further down whether the info file
-- 
2.30.2

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

Hi Mike,

On Sun, 20 Nov 2022 at 22:08, Mike Kupfer <mkupfer@alum.berkeley.edu> wrote:

> Yes, I tried a few graphical file browsers (Thunar, Caja, and Dolphin),
> and they all move the symlink, not the target, to Trash.
>
> After getting my test setup straightened out, I think I have a fix for
> the symlink issue and for the issue that Gustavo originally reported
> (cross-filesystem trashing fails when there's already a directory with
> the same name in Trash).
>
> I've committed these fixes separately; see attached.  Gustavo, can you
> try these out and make sure they handle your use case(s)?

Thank you for seeing to this.

I've tested the patches, and I'm glad to report `move-file-to-trash`
works as expected across file systems. I've also tested symlinks,
which work as expected as well.
LGTM.

(Tests made with 28.2, which is what I have here, and also the version
from which the bug was initially filed).

Cheers!
Gustavo.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Mike Kupfer]

Gustavo Barros wrote:

> I've tested the patches, and I'm glad to report `move-file-to-trash`
> works as expected across file systems. I've also tested symlinks,
> which work as expected as well.
> LGTM.

Great, thanks for testing.

mike

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Eli Zaretskii]

> From: Mike Kupfer <mkupfer@alum.berkeley.edu>
> cc: 58721@debbugs.gnu.org
> Date: Sun, 20 Nov 2022 17:08:18 -0800
> 
> > I'm not sure either, but my instincts are the same as yours. If that's
> > any reference, I just tested here, and that's what "gio trash" does
> > (moves the symlink, not the target).
> 
> Yes, I tried a few graphical file browsers (Thunar, Caja, and Dolphin),
> and they all move the symlink, not the target, to Trash.
> 
> After getting my test setup straightened out, I think I have a fix for
> the symlink issue and for the issue that Gustavo originally reported
> (cross-filesystem trashing fails when there's already a directory with
> the same name in Trash).
> 
> I've committed these fixes separately; see attached.  Gustavo, can you
> try these out and make sure they handle your use case(s)?
> 
> > > I'm okay with filing another bug report about rename-file, and
> > > discussing this there.  But that's a separate issue, and fix of this
> > > bug should not depend on that.
> > 
> > Understood.
> 
> Okay.  I'm not planning to follow up on this, Gustavo, so if you'd like
> to lobby for a change to rename-file, you'll need to open a bug for it
> (if you haven't already).

No further comments, so I've now installed these changes, and I'm closing
the bug.

Thanks.

bug#58721: 28.2; dired with delete-by-moving-to-trash can't trash directory twice

[Gustavo Barros]

On Thu, 24 Nov 2022 at 08:19, Eli Zaretskii <eliz@gnu.org> wrote:

> No further comments, so I've now installed these changes, and I'm closing
> the bug.

Thank you all very much.

Gustavo.