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

[Stephen Berman]

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.

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