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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 27968 in the body.
You can then email your comments to 27968 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.0.50;
 dired-revert: Saved positions might change if a dired header changes
Date: Sat, 05 Aug 2017 22:01:34 +0900

X-Debbugs-CC: Stephen Berman <stephen.berman <at> 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 <at> 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)

Message #8 received at 27968 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: stephen.berman <at> gmx.net, 27968 <at> debbugs.gnu.org
Subject: Re: bug#27968: 26.0.50;
 dired-revert: Saved positions might change if a dired header changes
Date: Sat, 05 Aug 2017 16:15:27 +0300

> From: Tino Calancha <tino.calancha <at> gmail.com>
> Date: Sat, 05 Aug 2017 22:01:34 +0900
> Cc: stephen berman <stephen.berman <at> 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?

Message #11 received at 27968 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 27968 <at> debbugs.gnu.org, stephen.berman <at> gmx.net,
 Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#27968: 26.0.50; dired-revert: Saved positions might change
 if a dired header changes
Date: Sun, 6 Aug 2017 00:45:02 +0900 (JST)

On Sat, 5 Aug 2017, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha <at> gmail.com>
>> Date: Sat, 05 Aug 2017 22:01:34 +0900
>> Cc: stephen berman <stephen.berman <at> 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.

Message #18 received at 27968-done <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 27968-done <at> debbugs.gnu.org
Subject: Re: bug#27968: 26.0.50;
 dired-revert: Saved positions might change if a dired header changes
Date: Sun, 06 Aug 2017 13:15:28 +0900

Tino Calancha <tino.calancha <at> 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)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.