Dired delete(d) the wrong file!

Dired delete(d) the wrong file!

[Dave Goel]

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

Dired deleted the wrong file for me, IRL!

But, I guess I don't know if this qualifies as a bug.

Maybe the emacs user is supposed to be forewarned -  "Buyer beware, dired
uses regexps, duh! heuristics always have fail cases"?

On the other hand, if it's a "buyer beware", maybe we need to communicate
this better? I have been an emacs "power?" user for upwards of two decades,
and I never knew or thought about it till I encountered this real-life
corruption.


TIL after lots of debugging - dired uses heuristics, and this means you can
delete/rename/edit the wrong file.

Example - touch two files in tmp/
"09 aa 3333 kk.mmn" and "kk.mmn".

Now, do some operation on the first file using any of dired (or wdired)
operations, example delete it d and x.

Guess which file you actually deleted?

And you can see why it happens - the regexes it uses in dired-get-filename
(and other places).

I reproduced these on my emacs 28.2 on debian stable, and reproduced on
27.1 ubunutu.

Mekeor (irc) couldn't reproduce this on 30.0.50 build.

Have the regexps changed?  I see from github that the structure and
mechanism is still very much the same  in the bleeding edge -

dired-get-filename
directory-listing-before-filename-regexp

.. to get the file name.

When you have regexps, the problem always remains? And, it did bit me IRL.


Lots of fun curiosities as you play around with the filenames  - it stops
working if you make the first number too large, say 43.  but, i guess 32,
still looks like a date to the regexp. The regexp probably likes to 0-3 to
filter for dates, etc.

(If you are curious - I had a file that started with 07 ... had the date in
it, and a version of it without the date in it. dired deleted the wrong
version for me!)


Is there a way for dired to dtrt? Is regexp the only way?

[-- Attachment #2: Type: text/html, Size: 2228 bytes --]

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

I should have reproduced it using -Q. Here are the exact instructions -

Here are the exact steps to reproduce it using -Q on 28.2 debian -

(0) touch the two files in tmp/
cd ~/tmp/
touch "07 nov 2395 zz.kk"
touch "zz.kk"

(1) Launch emacs -Q and eval this -
(progn
  (setq dired-ls-F-marks-symlinks t)
  (setq dired-listing-switches "-alF")
  (setq delete-by-moving-to-trash t)

  )

(2) Run dired ONLY after Eval'ing this

(3) Press d on the file labeled "07 aa 2395 zz.kk". Then press x.

(4) Dired will now show that the correct(!) file is deleted.

(5) Refresh the buffer by pressing "g" to see that actulaly, the wrong file
was deleted!





>

[-- Attachment #2: Type: text/html, Size: 1224 bytes --]

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

Can anyone test if using these exact instructions, the problem still exists
in the bleeding edge? -


On Thu, Jul 4, 2024 at 7:35 PM Dave Goel <deego3@gmail.com> wrote:

> I should have reproduced it using -Q. Here are the exact instructions -
>
> Here are the exact steps to reproduce it using -Q on 28.2 debian -
>
> (0) touch the two files in tmp/
> cd ~/tmp/
> touch "07 nov 2395 zz.kk"
> touch "zz.kk"
>
> (1) Launch emacs -Q and eval this -
> (progn
>   (setq dired-ls-F-marks-symlinks t)
>   (setq dired-listing-switches "-alF")
>   (setq delete-by-moving-to-trash t)
>
>   )
>
> (2) Run dired ONLY after Eval'ing this
>
> (3) Press d on the file labeled "07 aa 2395 zz.kk". Then press x.
>
> (4) Dired will now show that the correct(!) file is deleted.
>
> (5) Refresh the buffer by pressing "g" to see that actulaly, the wrong
> file was deleted!
>
>
>
>
>
>>

[-- Attachment #2: Type: text/html, Size: 1860 bytes --]

Re: Dired delete(d) the wrong file!

[Tassilo Horn]

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

Dave Goel <deego3@gmail.com> writes:

Hi Dave,

> Can anyone test if using these exact instructions, the problem still
> exists in the bleeding edge?

I've tried and it still exists.  However, after marking the file for
deletion with `d`, the highlighting already shows it uses the wrong
file.  Ditto for the query after `x`.


[-- Attachment #2: Screenshot-2024-07-05_095949.png --]
[-- Type: image/png, Size: 37410 bytes --]

[-- Attachment #3: Type: text/plain, Size: 16 bytes --]


Bye,
  Tassilo

Re: Dired delete(d) the wrong file!

[Po Lu]

Dave Goel <deego3@gmail.com> writes:

> Is there a way for dired to dtrt? Is regexp the only way?

Dired should use the `--dired' option in GNU ls to extract the true
positions of printed file names.

Re: Dired delete(d) the wrong file!

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 05 Jul 2024 09:24:40 +0800
> 
> Dave Goel <deego3@gmail.com> writes:
> 
> > Is there a way for dired to dtrt? Is regexp the only way?
> 
> Dired should use the `--dired' option in GNU ls to extract the true
> positions of printed file names.

And it does, by default at least.  Doesn't it?

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

I just tested, and the MRE I provided continues to hold if I modify the
instructions to -
  (setq dired-listing-switches "-alFD")

>
>

[-- Attachment #2: Type: text/html, Size: 577 bytes --]

Re: Dired delete(d) the wrong file!

[Eli Zaretskii]

> From: Dave Goel <deego3@gmail.com>
> Date: Fri, 5 Jul 2024 01:50:16 -0400
> Cc: Po Lu <luangruo@yahoo.com>, emacs-devel@gnu.org
> 
> I just tested, and the MRE I provided continues to hold if I modify the instructions to - 
>   (setq dired-listing-switches "-alFD")

Adding 'D' is pointless, since Emacs adds --dired anyway.

The problem is with 'F': it triggers a (very old) bug whereby we
mistakenly don't trust the --dired information.  Then the problem is
that the offending file name "07 nov 2395 zz.kk" looks like a date
followed by a file "zz.kk".

Please try the patch below (you will need to rebuild Emacs for it to
take effect, or extract insert-directory-clean from files.el, make the
change as below, and then evaluate the changed function):

diff --git a/lisp/files.el b/lisp/files.el
index 042b8e2..c518273 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -8087,8 +8087,8 @@ insert-directory-clean
 		  (end (insert-directory-adj-pos
 			(+ beg (read (current-buffer)))
 			error-lines)))
-	      (if (memq (char-after end) '(?\n ?\s))
-		  ;; End is followed by \n or by " -> ".
+	      (if (memq (char-after end) '(?\n ?\s ?/ ?* ?@ ?% ?= ?|))
+		  ;; End is followed by \n or by output of -F.
 		  (put-text-property start end 'dired-filename t)
 		;; It seems that we can't trust ls's output as to
 		;; byte positions of filenames.

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

(a) I just build emacs from git.
Emacs version - "31.0.50" (on GNU - debian - on gce.)


(b) There, in the git version, I was reproduced the MRE I had presented,
with and without -D.


(c) I applied Eli's fix, and re-eval'd the affected,
function insert-directory-clean,  per Eli's suggestion. Didn't re-build
emacs. Instead, eval'd the new function. (Had killed the dired buffer
before that, of course). Then, followed the MRE again. The bug still seems
to persist(??)





>
>

[-- Attachment #2: Type: text/html, Size: 1193 bytes --]

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

Just to make sure, I re-built it as well.
The problem is not fixed with the patch. The MRE reproduces.

Maybe this points to something? --

During build -

files.el:8091:20: Warning: ‘memq’ called with literal list that may never
match (element 2 of arg 2).

[-- Attachment #2: Type: text/html, Size: 664 bytes --]

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

Yes, it pointed to something - It pointed to me making typos.
After fixing my typos, Eli's patch works!

Thanks!!!!!!

[-- Attachment #2: Type: text/html, Size: 824 bytes --]

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

On Fri, Jul 5, 2024 at 4:26 AM Dave Goel <deego3@gmail.com> wrote:

> > Eli's patch works!
>


I still don't understand one thing -

The problem is now fixed, right?

It appears that it's still using a very hacky regexp - see (defvar
directory-listing-before-filename-regexp...) during file deletion.
which does all kinds of hacky checks, guessing where the timestring ends
and where the filename begins.

(A) So, is this still a hack?
(B) Is the problem fixed for good, or can one still concoct edge cases?
(C) If it is fixed, then why does that regex need this massive hack?

[-- Attachment #2: Type: text/html, Size: 2012 bytes --]

Re: Dired delete(d) the wrong file!

[Eli Zaretskii]

> From: Dave Goel <deego3@gmail.com>
> Date: Fri, 5 Jul 2024 04:39:29 -0400
> Cc: luangruo@yahoo.com, emacs-devel@gnu.org
> 
>  > Eli's patch works!
> 
> I still don't understand one thing -
> 
> The problem is now fixed, right? 

Yes, I think so.

> It appears that it's still using a very hacky regexp - see (defvar directory-listing-before-filename-regexp...)
> during file deletion.
> which does all kinds of hacky checks, guessing where the timestring ends and where the filename begins.
> 
> (A) So, is this still a hack? 
> (B) Is the problem fixed for good, or can one still concoct edge cases?
> (C) If it is fixed, then why does that regex need this massive hack?

That regexp is the best heuristic we could come up with that caters to
the various formats of date and time that the various versions of 'ls'
out there produce.  It is true that the regexp can be tricked into
mistakes, but we could not find a better solution that supports all
the valid use cases.  We use the --dired option of 'ls' when available
precisely for this reason: to allow Dired to determine the file names
reliably.  If --dired is not supported, then there will be problems
with certain file names, yes.  That is a known limitation, so users
are encouraged to install GNU Coreutils (which is where GNU 'ls' comes
from) to make Dired immune to those problems.

Re: Dired delete(d) the wrong file!

[Dave Goel]

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

On Fri, Jul 5, 2024 at 7:05 AM Eli Zaretskii <eliz@gnu.org> wrote:

>
> That regexp is the best heuristic we could come up with that caters to
> the various formats of date and time that the various versions of 'ls'
> out there produce.  It is true that the regexp can be tricked into
> mistakes, but we could not find a better solution that supports all
> the valid use cases.  We use the --dired option of 'ls' when available
> precisely for this reason: to allow Dired to determine the file names
> reliably.  If --dired is not supported, then there will be problems
> with certain file names, yes.  That is a known limitation, so users
> are encouraged to install GNU Coreutils (which is where GNU 'ls' comes
> from) to make Dired immune to those problems.
>

Thank you.

[-- Attachment #2: Type: text/html, Size: 1475 bytes --]

Re: Dired delete(d) the wrong file!

[Eli Zaretskii]

> From: Dave Goel <deego3@gmail.com>
> Date: Fri, 5 Jul 2024 04:26:27 -0400
> Cc: luangruo@yahoo.com, emacs-devel@gnu.org
> 
> Yes, it pointed to something - It pointed to me making typos.
> After fixing my typos, Eli's patch works!

Thanks for testing, the patch is now installed on the emacs-30 release
branch.

Re: Dired delete(d) the wrong file!

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> diff --git a/lisp/files.el b/lisp/files.el
> index 042b8e2..c518273 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -8087,8 +8087,8 @@ insert-directory-clean
>  		  (end (insert-directory-adj-pos
>  			(+ beg (read (current-buffer)))
>  			error-lines)))
> -	      (if (memq (char-after end) '(?\n ?\s))
> -		  ;; End is followed by \n or by " -> ".
> +	      (if (memq (char-after end) '(?\n ?\s ?/ ?* ?@ ?% ?= ?|))
> +		  ;; End is followed by \n or by output of -F.
>  		  (put-text-property start end 'dired-filename t)
>  		;; It seems that we can't trust ls's output as to
>  		;; byte positions of filenames.
>

It would be useful to have a test for this, BTW.

Re: Dired delete(d) the wrong file!

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 5 Jul 2024 09:53:05 +0000
> Cc: luangruo@yahoo.com, emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > diff --git a/lisp/files.el b/lisp/files.el
> > index 042b8e2..c518273 100644
> > --- a/lisp/files.el
> > +++ b/lisp/files.el
> > @@ -8087,8 +8087,8 @@ insert-directory-clean
> >  		  (end (insert-directory-adj-pos
> >  			(+ beg (read (current-buffer)))
> >  			error-lines)))
> > -	      (if (memq (char-after end) '(?\n ?\s))
> > -		  ;; End is followed by \n or by " -> ".
> > +	      (if (memq (char-after end) '(?\n ?\s ?/ ?* ?@ ?% ?= ?|))
> > +		  ;; End is followed by \n or by output of -F.
> >  		  (put-text-property start end 'dired-filename t)
> >  		;; It seems that we can't trust ls's output as to
> >  		;; byte positions of filenames.
> >
> 
> It would be useful to have a test for this, BTW.

I agree.  Patches welcome.

Note that the problem does not exist if ls-lisp is used, so the test
should specifically test the configuration where that is not so.

Re: Dired delete(d) the wrong file!

[Michael Heerdegen via Emacs development discussions.]

Eli Zaretskii <eliz@gnu.org> writes:

> diff --git a/lisp/files.el b/lisp/files.el
> index 042b8e2..c518273 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -8087,8 +8087,8 @@ insert-directory-clean
>  		  (end (insert-directory-adj-pos
>  			(+ beg (read (current-buffer)))
>  			error-lines)))
> -	      (if (memq (char-after end) '(?\n ?\s))
> -		  ;; End is followed by \n or by " -> ".
> +	      (if (memq (char-after end) '(?\n ?\s ?/ ?* ?@ ?% ?= ?|))
> +		  ;; End is followed by \n or by output of -F.

How about emitting a warning message when this condition fails?  It
could help us to detect other problematic cases and warn users that
Dired needs to fall back to a heuristical parsing of ls output.


Michael.

Re: Dired delete(d) the wrong file!

[Eli Zaretskii]

> Date: Sat, 06 Jul 2024 01:28:37 +0200
> From:  Michael Heerdegen via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > diff --git a/lisp/files.el b/lisp/files.el
> > index 042b8e2..c518273 100644
> > --- a/lisp/files.el
> > +++ b/lisp/files.el
> > @@ -8087,8 +8087,8 @@ insert-directory-clean
> >  		  (end (insert-directory-adj-pos
> >  			(+ beg (read (current-buffer)))
> >  			error-lines)))
> > -	      (if (memq (char-after end) '(?\n ?\s))
> > -		  ;; End is followed by \n or by " -> ".
> > +	      (if (memq (char-after end) '(?\n ?\s ?/ ?* ?@ ?% ?= ?|))
> > +		  ;; End is followed by \n or by output of -F.
> 
> How about emitting a warning message when this condition fails?  It
> could help us to detect other problematic cases and warn users that
> Dired needs to fall back to a heuristical parsing of ls output.

I think warning is too much.  After all, in the vast majority of cases
the problem will be resolved by the fallback code.  This discussion
needed a specially-concocted file name to demonstrate a failure.  But
it might be good to show an echo-area message, yes.  The challenge is
to come up with a message text that is both specific, accurate, and
clear, when the issue is subtle and potentially unknown to many users
(who might well be ignorant about the --dired switch and its effects
on 'ls').

Re: Dired delete(d) the wrong file!

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> And it does, by default at least.  Doesn't it?

Yes, that was my point.  Something on Dave's system is otherwise than it
is on the system where Dired was reported to identify the correct file.

Re: Dired delete(d) the wrong file!

[Michael Heerdegen via Emacs development discussions.]

Po Lu <luangruo@yahoo.com> writes:

> Yes, that was my point.  Something on Dave's system is otherwise than it
> is on the system where Dired was reported to identify the correct file.

Edebugging `insert-directory-clean' reveals that here:

#+begin_src emacs-lisp
	  (while (< (point) end)
	    (let ((start (insert-directory-adj-pos
			  (+ beg (read (current-buffer)))
			  error-lines))
		  (end (insert-directory-adj-pos
			(+ beg (read (current-buffer)))
			error-lines)))
	      (if (memq (char-after end) '(?\n ?\s))
		  ;; End is followed by \n or by " -> ".
		  (put-text-property start end 'dired-filename t)
		;; It seems that we can't trust ls's output as to
		;; byte positions of filenames.
		(put-text-property beg (point) 'dired-filename nil)
		(end-of-line))))
#+end_src

the loop gets aborted because the (memq (char-after end) '(?\n ?\s)) call,
when using Dave's ls options, returns the slash character that comes after
".":

| bash-5.2:micha:tmp$ ls -alF --dired
|   total 72
|   drwxr-xr-x   3 micha micha  4096 Jul  5 07:26  ./
|   drwxr-xr-x 213 micha micha 61440 Jul  5 07:07  ../
|   -rw-rw-r--   1 micha micha     0 Jul  5 07:25 '07 nov 2395 zz.kk'
|   drwxr-xr-x   3 micha micha  4096 Jun 23  2014  src/
| //DIRED// 60 61 112 114 164 183 233 236
| //DIRED-OPTIONS// --quoting-style=shell-escape

and so we throw away that information.  60 and 61 are the positions
before and after the "." directory.


Michael.