Subject: bug#27968: closed (Re: bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes)

From: [email-masked] (GNU bug Tracking System)
Date: Sun, 06 Aug 2017 04:16:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
27968: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=27968
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#27968: 26.0.50; dired-revert: Saved positions might change if a dired header changes
From: Tino Calancha <[email-masked]>
To: [email-masked]
Date: Sun, 06 Aug 2017 13:15:28 +0900
List-Id: <debbugs-submit.debbugs.gnu.org>

Tino Calancha <[email-masked]> 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)

Subject: 26.0.50; dired-revert: Saved positions might change if a dired header changes
From: Tino Calancha <[email-masked]>
To: [email-masked]
Date: Sat, 05 Aug 2017 22:01:34 +0900
List-Id: <debbugs-submit.debbugs.gnu.org>

X-Debbugs-CC: Stephen Berman <[email-masked]>
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 <[email-masked]>
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)