bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes

bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes

[Tino Calancha]

X-Debbugs-CC: Stephen Berman <stephen.berman@gmx.net>
Tag: patch

Dired headers also changes; if you add/deleted some files,
the used space shown changes: that will mess up the saved
buffer positions if the length of the header is different.

This problem causes dired-test-bug27243-01 to fail in hydra.

One possible solution is the patch below.  It saves line numbers
instead of positions.  Line numbers won't change if a dired header
changes its length.

--8<-----------------------------cut here---------------start------------->8---
commit a6799f27a9d18618f3046a1db7ffebc480310e8f
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Sat Aug 5 21:59:06 2017 +0900

    dired-revert: save line numbers instead of positions
    
    Positions might change if the length of one dired header line
    changes; this happen, for instance, if we add new files.
    https://lists.gnu.org/archive/html/emacs-devel/2017-07/msg01092.html
    * lisp/dired.el (dired-save-positions): Save the line numbers at point.
    (dired-restore-positions): Use forward-line to restore the original
    position (Bug#27968).
    * test/lisp/dired-tests.el (dired-test-bug27968): Add test.

diff --git a/lisp/dired.el b/lisp/dired.el
index 24759c6c9b..d04bd6fe03 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1444,18 +1444,22 @@ dired-save-positions
 The positions have the form (BUFFER-POSITION WINDOW-POSITIONS).
 
 BUFFER-POSITION is the point position in the current Dired buffer.
-It has the form (BUFFER DIRED-FILENAME BUFFER-POINT).
+It has the form (BUFFER DIRED-FILENAME BUFFER-LINE-NUMBER).
 
 WINDOW-POSITIONS are current positions in all windows displaying
 this dired buffer.  The window positions have the form (WINDOW
-DIRED-FILENAME WINDOW-POINT)."
+DIRED-FILENAME WINDOW-LINE-NUMBER).
+
+We store line numbers instead of point positions because the header
+lines might change as well: when this happen the line number doesn't
+change; the point does."
   (list
-   (list (current-buffer) (dired-get-filename nil t) (point))
+   (list (current-buffer) (dired-get-filename nil t) (line-number-at-pos))
    (mapcar (lambda (w)
-	     (list w
-		   (with-selected-window w
-		     (dired-get-filename nil t))
-		   (window-point w)))
+	     (with-selected-window w
+               (list w
+		     (dired-get-filename nil t)
+                     (line-number-at-pos (window-point w)))))
 	   (get-buffer-window-list nil 0 t))))
 
 (defun dired-restore-positions (positions)
@@ -1464,7 +1468,8 @@ dired-restore-positions
 	 (buffer (nth 0 buf-file-pos)))
     (unless (and (nth 1 buf-file-pos)
 		 (dired-goto-file (nth 1 buf-file-pos)))
-      (goto-char (nth 2 buf-file-pos))
+      (goto-char (point-min))
+      (forward-line (1- (nth 2 buf-file-pos)))
       (dired-move-to-filename))
     (dolist (win-file-pos (nth 1 positions))
       ;; Ensure that window still displays the original buffer.
@@ -1472,7 +1477,8 @@ dired-restore-positions
 	(with-selected-window (nth 0 win-file-pos)
 	  (unless (and (nth 1 win-file-pos)
 		       (dired-goto-file (nth 1 win-file-pos)))
-	    (goto-char (nth 2 win-file-pos))
+            (goto-char (point-min))
+	    (forward-line (1- (nth 2 win-file-pos)))
 	    (dired-move-to-filename)))))))
 
 (defun dired-remember-marks (beg end)
diff --git a/test/lisp/dired-tests.el b/test/lisp/dired-tests.el
index b14bbc6360..4bbf72ab74 100644
--- a/test/lisp/dired-tests.el
+++ b/test/lisp/dired-tests.el
@@ -308,5 +308,44 @@ dired-dwim-target
           (should (eq 2 (current-column))))
       (dired-hide-details-mode orig))))
 
+(ert-deftest dired-test-bug27968 ()
+  (let* ((top-dir (make-temp-file "top-dir" t))
+         (subdir (expand-file-name "subdir" top-dir))
+         (header-len-fn (lambda ()
+                          (save-excursion
+                            (goto-char 1)
+                            (forward-line 1)
+                            (- (point-at-eol) (point)))))
+         orig-len len diff pos)
+    (make-directory subdir 'parents)
+    (unwind-protect
+        (with-current-buffer (dired-noselect subdir)
+          (setq orig-len (funcall header-len-fn)
+                pos (point))
+          ;; Bug arises when the header line changes its length; this may
+          ;; happen if the used space has changed: for instance, with the
+          ;; creation of additional files.
+          (make-directory "subdir" t)
+          (dired-revert)
+          ;; Change the header line.
+          (save-excursion
+            (goto-char 1)
+            (forward-line 1)
+            (let ((inhibit-read-only t))
+              (delete-region (point) (point-at-eol))
+              (insert "  test-bug27968")))
+          (setq len (funcall header-len-fn)
+                diff (- len orig-len))
+          (should-not (zerop diff)) ; Header length has changed.
+          ;; If diff > 0, then the point moves back.
+          ;; If diff < 0, then the point moves forward.
+          ;; If diff = 0, then the point doesn't move.
+          ;; We correct `point' with diff to obtain the right line number.
+          (should (equal (line-number-at-pos (+ (point) diff))
+                         (line-number-at-pos pos)))
+          ;; After revert, the point must be in 'subdir' line.
+          (should (equal "subdir" (dired-get-filename nil t))))
+      (delete-directory top-dir t))))
+
 (provide 'dired-tests)
 ;; dired-tests.el ends here
--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 26.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-08-05 built on calancha-pc
Repository revision: 1d51b265075ea9852dab678071b0e400a2c36ae7
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description:	Debian GNU/Linux 9.1 (stretch)

bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Sat, 05 Aug 2017 22:01:34 +0900
> Cc: stephen berman <stephen.berman@gmx.net>
> 
> Dired headers also changes; if you add/deleted some files,
> the used space shown changes: that will mess up the saved
> buffer positions if the length of the header is different.
> 
> This problem causes dired-test-bug27243-01 to fail in hydra.
> 
> One possible solution is the patch below.  It saves line numbers
> instead of positions.  Line numbers won't change if a dired header
> changes its length.

Won't using markers be a better solution?

bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes

[Tino Calancha]

On Sat, 5 Aug 2017, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Sat, 05 Aug 2017 22:01:34 +0900
>> Cc: stephen berman <stephen.berman@gmx.net>
>>
>> Dired headers also changes; if you add/deleted some files,
>> the used space shown changes: that will mess up the saved
>> buffer positions if the length of the header is different.
>>
>> This problem causes dired-test-bug27243-01 to fail in hydra.
>>
>> One possible solution is the patch below.  It saves line numbers
>> instead of positions.  Line numbers won't change if a dired header
>> changes its length.
>
> Won't using markers be a better solution?
Yes, but `dired-revert' erases the buffer when calls
`dired-readin', so that the markers will 'gone baby gone': neither
the very same Casey Affleck could recover those markers.

bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

> On Sat, 5 Aug 2017, Eli Zaretskii wrote:
>
>>> One possible solution is the patch below.  It saves line numbers
>>> instead of positions.  Line numbers won't change if a dired header
>>> changes its length.
>>
>> Won't using markers be a better solution?
> Yes, but `dired-revert' erases the buffer when calls
> `dired-readin', so that the markers will be gone,
*) I really want to have buildboot clean again
   (http://emacs.bioswarm.net:8010/#/), in order to this tool
   be effective to catch new problems.

*) so i pushed a fix in master branch as commit
7c3593f81724d0c7a2ee2f90797db0e705adc859
(dired-revert: save line numbers instead of positions)