bug#48805: 27.2; dired-mode moves point to wrong positions while deleting non-empty directories

bug#48805: 27.2; dired-mode moves point to wrong positions while deleting non-empty directories

[ynyaaa]

The form below creates a working directory and generate many non-empty
directories under the working directory, then displaye the working
directory in a dired-mode buffer.
  (let ((dir (make-temp-file "dir" t)))
    (dotimes (i 100)
      (let ((d (expand-file-name (format "d%03d" i) dir)))
        (make-directory d)
        (write-region "" nil (expand-file-name "file" d))
        ))
    (dired dir))
Mark all subdirectories to be deleted with typing 'C-u 100 d'.
Tell emacs to delete all marked directories with typing 'x'.
Emacs asks 'Delete D [100 files] (yes or no) ', and answer yes.
Then emacs asks like 'Recursively delete d000? (yes, no, all, quit, help) '
for each directory, and answer yes for each confirmation.
While these confirmations, emacs tries to move point to the 'D' marker of
the line of the asked directory.
But the real position of the point is different from the line.
Perhaps because the goal point value is changed with the deletion of the
lines of the directories which has been deleted.

Also, I think the point should be moved to the directory name, not marker.
Directory names are much more important than marker types and there is a
long distance between the marker and the name.


In GNU Emacs 27.2 (build 1, x86_64-w64-mingw32)
 of 2021-03-26 built on CIRROCUMULUS
Repository revision: deef5efafb70f4b171265b896505b92b6eef24e6
Repository branch: HEAD
Windowing system distributor 'Microsoft Corp.', version 10.0.19043
System Description: Microsoft Windows 10 Pro (v10.0.2009.19043.1023)

Recent messages:

Configured using:
 'configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY W32NOTIFY ACL GNUTLS LIBXML2
HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS JSON PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: JPN
  locale-coding-system: cp932

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-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
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr term/bobcat emacsbug message rmc puny format-spec
rfc822 mml mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils dired dired-loaddefs
cl-print debug backtrace find-func mule-util info apropos goto-addr
thingatpt noutline outline easy-mmode view misearch multi-isearch
cl-extra seq byte-opt gv bytecomp byte-compile cconv help-fns radix-tree
help-mode easymenu time-date subr-x cl-loaddefs cl-lib japan-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel dos-w32 ls-lisp disp-table term/w32-win w32-win w32-vars
term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu 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 charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads w32notify w32 lcms2 multi-tty make-network-process
emacs)

Memory information:
((conses 16 73546 10832)
 (symbols 48 8298 1)
 (strings 32 24697 1901)
 (string-bytes 1 692843)
 (vectors 16 13822)
 (vector-slots 8 265412 15684)
 (floats 8 37 270)
 (intervals 56 1015 643)
 (buffers 1000 17))

bug#48805: 27.2; dired-mode moves point to wrong positions while deleting non-empty directories

[Stephen Berman]

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

On Thu, 03 Jun 2021 16:20:24 +0900 ynyaaa@gmail.com wrote:

> The form below creates a working directory and generate many non-empty
> directories under the working directory, then displaye the working
> directory in a dired-mode buffer.
>   (let ((dir (make-temp-file "dir" t)))
>     (dotimes (i 100)
>       (let ((d (expand-file-name (format "d%03d" i) dir)))
>         (make-directory d)
>         (write-region "" nil (expand-file-name "file" d))
>         ))
>     (dired dir))
> Mark all subdirectories to be deleted with typing 'C-u 100 d'.
> Tell emacs to delete all marked directories with typing 'x'.
> Emacs asks 'Delete D [100 files] (yes or no) ', and answer yes.
> Then emacs asks like 'Recursively delete d000? (yes, no, all, quit, help) '
> for each directory, and answer yes for each confirmation.
> While these confirmations, emacs tries to move point to the 'D' marker of
> the line of the asked directory.
> But the real position of the point is different from the line.
> Perhaps because the goal point value is changed with the deletion of the
> lines of the directories which has been deleted.

This bug was introduced by this commit:

commit 9ecbdeeaa845a75c63210057a8a554eedc9387bf
Author:     Tino Calancha <tino.calancha@gmail.com>
Commit:     Tino Calancha <tino.calancha@gmail.com>
CommitDate: Wed Aug 9 14:37:21 2017 +0900

    Ask files for deletion in buffer order: top first, botton later

    * lisp/dired.el (dired-do-flagged-delete, dired-do-delete):
    Call `nreverse' t invert the output of `dired-map-over-marks'.

In effect, this countermanded the requirement stated by this comment in
dired-internal-do-deletions:

  ;; L is an alist of files to delete, with their buffer positions.
  [...]
  ;; (car L) *must* be the *last* (bottommost) file in the dired buffer.
  ;; That way as changes are made in the buffer they do not shift the
  ;; lines still to be changed, so the (point) values in L stay valid.
  ;; Also, for subdirs in natural order, a subdir's files are deleted
  ;; before the subdir itself - the other way around would not work.

However, the last sentence of this comment was made obsolete by commit
f06280268, which allows deleting non-empty directories.  And since the
motivation for commit 9ecbdeeaa seems reasonable, it seems best not to
rely on buffer positions but instead to use markers.  The attached patch
does this, and that fixes the bug reported above AFAICT.  (If an
accumulation of markers is not a concern here, the patch could be
simplified.)  (Commit a84c3810b, which fixed another regression due to
commit 9ecbdeeaa but did not address the problem reported in this bug,
is left intact by the patch.)

> Also, I think the point should be moved to the directory name, not marker.
> Directory names are much more important than marker types and there is a
> long distance between the marker and the name.

That seems like a reasonable request, and the attached patch implements
it too.  (A further development of this could be to highlight the file
name when prompting for whether to delete it, making it even more
obvious which file is meant.  But maybe that's overengineering.)


2021-06-03  Stephen Berman  <stephen.berman@gmx.net>

	Fix placement of point in Dired deletion operations (bug#48805)

	* lisp/dired.el (dired-do-flagged-delete, dired-do-delete): Use
	point-marker instead of point to record each file name position.
	Clean up the markers before returning.
	(dired-internal-do-deletions): Move to the file name marker, and
	then move point to the file name to visually emphasize which file
	is being operated on.


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

diff --git a/lisp/dired.el b/lisp/dired.el
index 8527634760..165484302a 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -3280,15 +3280,19 @@ dired-do-flagged-delete
   (interactive)
   (let* ((dired-marker-char dired-del-marker)
 	 (regexp (dired-marker-regexp))
-	 case-fold-search)
+	 case-fold-search markers)
     (if (save-excursion (goto-char (point-min))
 			(re-search-forward regexp nil t))
 	(dired-internal-do-deletions
          (nreverse
 	  ;; this can't move point since ARG is nil
-	  (dired-map-over-marks (cons (dired-get-filename) (point))
+	  (dired-map-over-marks (cons (dired-get-filename)
+                                      (let ((m (point-marker)))
+                                        (push m markers)
+                                        m))
 			        nil))
 	 nil t)
+      (dolist (m markers) (set-marker m nil))
       (or nomessage
 	  (message "(No deletions requested)")))))

@@ -3299,12 +3303,17 @@ dired-do-delete
   ;; This is more consistent with the file marking feature than
   ;; dired-do-flagged-delete.
   (interactive "P")
-  (dired-internal-do-deletions
-   (nreverse
-    ;; this may move point if ARG is an integer
-    (dired-map-over-marks (cons (dired-get-filename) (point))
-			  arg))
-   arg t))
+  (let (markers)
+    (dired-internal-do-deletions
+     (nreverse
+      ;; this may move point if ARG is an integer
+      (dired-map-over-marks (cons (dired-get-filename)
+                                  (let ((m (point-marker)))
+                                    (push m markers)
+                                    m))
+                            arg))
+     arg t)
+    (dolist (m markers) (set-marker m nil))))

 (defvar dired-deletion-confirmer 'yes-or-no-p) ; or y-or-n-p?

@@ -3312,11 +3321,6 @@ dired-internal-do-deletions
   ;; L is an alist of files to delete, with their buffer positions.
   ;; ARG is the prefix arg.
   ;; Filenames are absolute.
-  ;; (car L) *must* be the *last* (bottommost) file in the dired buffer.
-  ;; That way as changes are made in the buffer they do not shift the
-  ;; lines still to be changed, so the (point) values in L stay valid.
-  ;; Also, for subdirs in natural order, a subdir's files are deleted
-  ;; before the subdir itself - the other way around would not work.
   (let* ((files (mapcar #'car l))
 	 (count (length l))
 	 (succ 0)
@@ -3337,9 +3341,10 @@ dired-internal-do-deletions
 		 (make-progress-reporter
 		  (if trashing "Trashing..." "Deleting...")
 		  succ count))
-		failures) ;; files better be in reverse order for this loop!
+		failures)
 	    (while l
-	      (goto-char (cdr (car l)))
+	      (goto-char (marker-position (cdr (car l))))
+              (dired-move-to-filename)
 	      (let ((inhibit-read-only t))
 		(condition-case err
 		    (let ((fn (car (car l))))

bug#48805: 27.2; dired-mode moves point to wrong positions while deleting non-empty directories

[Lars Ingebrigtsen]

Stephen Berman <stephen.berman@gmx.net> writes:

> 	* lisp/dired.el (dired-do-flagged-delete, dired-do-delete): Use
> 	point-marker instead of point to record each file name position.
> 	Clean up the markers before returning.
> 	(dired-internal-do-deletions): Move to the file name marker, and
> 	then move point to the file name to visually emphasize which file
> 	is being operated on.

Looks good to me, so I've applied it to Emacs 28.

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