bug#28852: make revert-buffer ('g') in VC diff buffers keep point

bug#28852: make revert-buffer ('g') in VC diff buffers keep point

[Charles A. Roelli]

At the moment, when you hit 'g' in a *vc-diff* buffer, the cursor is
moved back to (point-min), which can be annoying.  Attached is an
attempt at fixing that (using 'replace-buffer-contents' to keep
markers/point in the same place).

<#part type="text/x-patch" filename="~/Datacar/Code/emacs-devel/0001-Make-revert-buffer-g-keep-point-in-VC-diff-buffers.patch" disposition=attachment>
<#/part>

bug#28852: make revert-buffer ('g') in VC diff buffers keep point

[Charles A. Roelli]

> Date: Sun, 15 Oct 2017 21:07:30 +0200
> From: charles@aurox.ch (Charles A. Roelli)
> 
> At the moment, when you hit 'g' in a *vc-diff* buffer, the cursor is
> moved back to (point-min), which can be annoying.  Attached is an
> attempt at fixing that (using 'replace-buffer-contents' to keep
> markers/point in the same place).

Hm, I'll try again:

From 48e3febfee28276b3eb7d9af58342c70d2d798f9 Mon Sep 17 00:00:00 2001
From: "Charles A. Roelli" <charles@aurox.ch>
Date: Sun, 15 Oct 2017 20:58:01 +0200
Subject: [PATCH] Make revert-buffer ('g') keep point in VC diff buffers
 (Bug#28852)

* lisp/vc/vc.el (vc-diff-restore-buffer): New function.
(vc-diff-finish): Update its calling convention to include an
optional 'oldbuf' parameter, and handle it.
(vc-diff-internal): Pass a clone of the incumbent vc-diff
buffer to 'vc-diff-finish'.
---
 lisp/vc/vc.el | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index b80f0e6..57d5a50 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1654,7 +1654,20 @@ vc-diff-switches-list
   (declare (obsolete vc-switches "22.1"))
   `(vc-switches ',backend 'diff))
 
-(defun vc-diff-finish (buffer messages)
+(defun vc-diff-restore-buffer (original new)
+  "Restore point in buffer NEW to where it was in ORIGINAL.
+
+This function works by updating buffer ORIGINAL with the contents
+of NEW (without destroying existing markers), swapping their text
+objects, and finally killing buffer ORIGINAL."
+  (with-current-buffer original
+    (let ((inhibit-read-only t))
+      (replace-buffer-contents new)))
+  (with-current-buffer new
+    (buffer-swap-text original))
+  (kill-buffer original))
+
+(defun vc-diff-finish (buffer messages &optional oldbuf)
   ;; The empty sync output case has already been handled, so the only
   ;; possibility of an empty output is for an async process.
   (when (buffer-live-p buffer)
@@ -1666,7 +1679,13 @@ vc-diff-finish
 	       (insert (cdr messages) ".\n")
 	       (message "%s" (cdr messages))))
 	(diff-setup-whitespace)
-	(goto-char (point-min))
+        ;; `oldbuf' is the buffer that used to show this diff.  Make
+        ;; sure that we restore point in it if it's given.
+	(if oldbuf
+            (progn
+              (vc-diff-restore-buffer oldbuf buffer)
+              (diff-mode))
+          (goto-char (point-min)))
 	(when window
 	  (shrink-window-if-larger-than-buffer window)))
       (when (and messages (not emptyp))
@@ -1692,7 +1711,12 @@ vc-diff-internal
 	 ;; but the only way to set it for each file included would
 	 ;; be to call the back end separately for each file.
 	 (coding-system-for-read
-	  (if files (vc-coding-system-for-diff (car files)) 'undecided)))
+	  (if files (vc-coding-system-for-diff (car files)) 'undecided))
+         (orig-diff-buffer-clone
+          (if (and (get-buffer buffer) revert-buffer-in-progress-p)
+              (with-current-buffer buffer
+                (clone-buffer
+                 (generate-new-buffer-name " *vc-diff-clone*") nil)))))
     ;; On MS-Windows and MS-DOS, Diff is likely to produce DOS-style
     ;; EOLs, which will look ugly if (car files) happens to have Unix
     ;; EOLs.
@@ -1752,7 +1776,8 @@ vc-diff-internal
       ;; after `pop-to-buffer'; the former assumes the diff buffer is
       ;; shown in some window.
       (let ((buf (current-buffer)))
-        (vc-run-delayed (vc-diff-finish buf (when verbose messages))))
+        (vc-run-delayed (vc-diff-finish buf (when verbose messages)
+                                        orig-diff-buffer-clone)))
       ;; In the async case, we return t even if there are no differences
       ;; because we don't know that yet.
       t)))
-- 
2.9.4

bug#28852: make revert-buffer ('g') in VC diff buffers keep point

[Lars Ingebrigtsen]

charles@aurox.ch (Charles A. Roelli) writes:

>> At the moment, when you hit 'g' in a *vc-diff* buffer, the cursor is
>> moved back to (point-min), which can be annoying.

Yes, I agree.

> +(defun vc-diff-restore-buffer (original new)
> +  "Restore point in buffer NEW to where it was in ORIGINAL.
> +
> +This function works by updating buffer ORIGINAL with the contents
> +of NEW (without destroying existing markers), swapping their text
> +objects, and finally killing buffer ORIGINAL."
> +  (with-current-buffer original
> +    (let ((inhibit-read-only t))
> +      (replace-buffer-contents new)))
> +  (with-current-buffer new
> +    (buffer-swap-text original))
> +  (kill-buffer original))

This will restore point to the same point it was before...  but not to
"where it was", necessarily, I think?  That is, if your point is at
character 105, then it'll be on character 105 after this, but since the
contents may (and will) change, what's on character 105 may be totally
different after running the diff -- new changes will appear and others
will disappear.

I remember discussing a "sloppy save excursion" on emacs-devel once:
Save as much data as possible about the old buffer contents, and then
try to move point to a similar context in the new buffer contents, even
if that means moving to a different line.

This is a general problem in many buffers that display generated content
and need to get back to "where it was" after a `g'.

But I can't remember what the conclusion was, and now I can't find the
discussion...  Does anybody remember?

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

bug#28852: make revert-buffer ('g') in VC diff buffers keep point

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> This will restore point to the same point it was before...  but not to
> "where it was", necessarily, I think? 

I was confused about how these functions worked.  I've respun the patch
for Emacs 29, and it seem to work very well, so I've now pushed it.

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

bug#28852: make revert-buffer ('g') in VC diff buffers keep point

[Juri Linkov]

>> This will restore point to the same point it was before...  but not to
>> "where it was", necessarily, I think?
>
> I was confused about how these functions worked.  I've respun the patch
> for Emacs 29, and it seem to work very well, so I've now pushed it.

This doesn't work with such customization:

  (add-hook 'diff-mode-hook 'rename-uniquely)

because this change assumes that the current buffer is always "*vc-diff*"
whereas the reverted buffer can be "*vc-diff*<2>".

Isn't the reverted buffer always current during revert-buffer command?
If this assumption is correct, how about his fix?

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 0096a5fcb3..1671f2f860 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1798,10 +1798,9 @@ vc-diff-internal
 	 (coding-system-for-read
 	  (if files (vc-coding-system-for-diff (car files)) 'undecided))
          (orig-diff-buffer-clone
-          (if (and (get-buffer buffer) revert-buffer-in-progress-p)
-              (with-current-buffer buffer
-                (clone-buffer
-                 (generate-new-buffer-name " *vc-diff-clone*") nil)))))
+          (if revert-buffer-in-progress-p
+              (clone-buffer
+               (generate-new-buffer-name " *vc-diff-clone*") nil))))
     ;; On MS-Windows and MS-DOS, Diff is likely to produce DOS-style
     ;; EOLs, which will look ugly if (car files) happens to have Unix
     ;; EOLs.
-- 

bug#28852: make revert-buffer ('g') in VC diff buffers keep point

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Isn't the reverted buffer always current during revert-buffer command?

Yes, I think so.

> If this assumption is correct, how about his fix?

Makes sense to me.

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