bug#33319: Support revisions in diff-goto-source

bug#33319: Support revisions in diff-goto-source

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

Severity: wishlist
Tags: patch

Currently diff-goto-source with no prefix arg jumps to the new file,
and with a prefix arg jumps to the old file.

It does nothing special when the diff buffer is created by
a version control system, because the same current file is
visited by old and new.  This is a useful default behavior
to visit the current file and continue making changes
at an approximate location of the difference.

However, often this fails to find the change in old revisions.

So the proposed feature is backward-compatible: in the diff buffer
created by a version control system, with a prefix arg it will jump
to an old revision of the file.

It is very useful to look at the context of changed lines 
of the file as it was at the time of that old revision.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-goto-source.1.patch --]
[-- Type: text/x-diff, Size: 3093 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index 1020a2a0ea..36ad3d82df 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -353,6 +353,11 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
 *** File headers can be shortened, mimicking Magit's diff format.
 To enable it, set the new defcustom 'diff-font-lock-prettify to t.
 
+*** In the diff buffer created by a version control system, the prefix
+arg of diff-goto-source means it jumps to the old revision of the file
+if point is on an old changed line, or to the new revision of the file
+otherwise.
+
 ** Browse-url
 
 *** The function 'browse-url-emacs' can now visit a URL in selected window.
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index dcfbf26e86..6b7ca02440 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1728,6 +1729,7 @@ vc-diff-internal
     (set-buffer buffer)
     (diff-mode)
     (set (make-local-variable 'diff-vc-backend) (car vc-fileset))
+    (set (make-local-variable 'diff-vc-revisions) (list rev1 rev2))
     (set (make-local-variable 'revert-buffer-function)
 	 (lambda (_ignore-auto _noconfirm)
            (vc-diff-internal async vc-fileset rev1 rev2 verbose)))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index cf52368508..b41206296d 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -104,6 +105,9 @@ diff-font-lock-prettify
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
+(defvar diff-vc-revisions nil
+  "The VC revisions compared in the current Diff buffer, if any.")
+
 (defvar diff-outline-regexp
   "\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
 
@@ -1736,7 +1740,11 @@ diff-find-source-location
 		       (match-string 1)))))
 	   (file (or (diff-find-file-name other noprompt)
                      (error "Can't find the file")))
-	   (buf (find-file-noselect file)))
+	   (revision (and other diff-vc-backend
+                          (nth (if reverse 1 0) diff-vc-revisions)))
+	   (buf (if revision
+                    (vc-find-revision file revision diff-vc-backend)
+                  (find-file-noselect file))))
       ;; Update the user preference if he so wished.
       (when (> (prefix-numeric-value other-file) 8)
 	(setq diff-jump-to-old-file other))
@@ -1862,7 +1870,11 @@ diff-goto-source
 `diff-jump-to-old-file' (or its opposite if the OTHER-FILE prefix arg
 is given) determines whether to jump to the old or the new file.
 If the prefix arg is bigger than 8 (for example with \\[universal-argument] \\[universal-argument])
-then `diff-jump-to-old-file' is also set, for the next invocations."
+then `diff-jump-to-old-file' is also set, for the next invocations.
+
+In the diff buffer created by a version control system, the OTHER-FILE
+prefix arg means it jumps to the old revision of the file if point is
+on an old changed line, or to the new revision of the file otherwise."
   (interactive (list current-prefix-arg last-input-event))
   ;; When pointing at a removal line, we probably want to jump to
   ;; the old location, and else to the new (i.e. as if reverting).

bug#33319: Support revisions in diff-goto-source

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 08 Nov 2018 23:05:10 +0200
> 
> So the proposed feature is backward-compatible: in the diff buffer
> created by a version control system, with a prefix arg it will jump
> to an old revision of the file.
> 
> It is very useful to look at the context of changed lines 
> of the file as it was at the time of that old revision.

Thanks.

> diff --git a/etc/NEWS b/etc/NEWS
> index 1020a2a0ea..36ad3d82df 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -353,6 +353,11 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
>  *** File headers can be shortened, mimicking Magit's diff format.
>  To enable it, set the new defcustom 'diff-font-lock-prettify to t.
>  
> +*** In the diff buffer created by a version control system, the prefix
> +arg of diff-goto-source means it jumps to the old revision of the file
                           ^^^^^^^^^^^^^^^^^
"... means jump to ..." is a better wording, I think.

Also, it is best to provide a header for the description of the
change, so that it could be meaningfully folded by Outline mode.

And I think we want to update the manual as well.

> +(defvar diff-vc-revisions nil
> +  "The VC revisions compared in the current Diff buffer, if any.")
> +
>  (defvar diff-outline-regexp
>    "\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")

Which VCSes does this support?  I'm not sure all of them produce such
markers.  If this supports only some, we should document that and make
sure the code does something reasonable when the revision is not
found.

bug#33319: Support revisions in diff-goto-source

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

>> +*** In the diff buffer created by a version control system, the prefix
>> +arg of diff-goto-source means it jumps to the old revision of the file
>                            ^^^^^^^^^^^^^^^^^
> "... means jump to ..." is a better wording, I think.
>
> Also, it is best to provide a header for the description of the
> change, so that it could be meaningfully folded by Outline mode.

Fixed in a new patch.

> And I think we want to update the manual as well.

Updated.

>> +(defvar diff-vc-revisions nil
>> +  "The VC revisions compared in the current Diff buffer, if any.")
>> +
>>  (defvar diff-outline-regexp
>>    "\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
>
> Which VCSes does this support?  I'm not sure all of them produce such
> markers.  If this supports only some, we should document that and make
> sure the code does something reasonable when the revision is not
> found.

It should work in all VCSes that support the VC interface function
'find-revision' defined in the comments of vc.el:

;; * find-revision (file rev buffer)
;;
;;   Fetch revision REV of file FILE and put it into BUFFER.
;;   If REV is the empty string, fetch the head of the trunk.
;;   The implementation should pass the value of vc-checkout-switches
;;   to the backend command.

BTW, its implementation in vc-find-revision currently has
a major shortcoming: it always saves the retrieved revision
to the file.  I spend too much time cleaning after this command
all old files that it creates.  Thus I propose a new option
`vc-find-revision-no-save' to not save the buffer it creates
to the file.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-goto-source.2.patch --]
[-- Type: text/x-diff, Size: 6699 bytes --]

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 6c68075ae4..523734ec1f 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1507,7 +1507,11 @@ Diff Mode
 @item C-c C-c
 @findex diff-goto-source
 Go to the source file and line corresponding to this hunk
-(@code{diff-goto-source}).
+(@code{diff-goto-source}).  With a prefix argument of @kbd{C-u},
+go to the old source file.  If the source file is under version
+control (@pxref{Version Control}), with a prefix argument of
+@kbd{C-u}, go to the old revision of the file (@pxref{Old Revisions})
+when point is on old line, or otherwise to the new revision.
 
 @item C-c C-e
 @findex diff-ediff-patch
diff --git a/etc/NEWS b/etc/NEWS
index 29bbde9395..cea600c430 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -334,6 +334,9 @@ still be used if it exists.)  Set the variable to nil to get the
 previous behavior of always creating a buffer that visits a ChangeLog
 file.
 
+*** New customizable variable 'vc-find-revision-no-save'.
+With non-nil, 'vc-find-revision' doesn't write the created buffer to file.
+
 *** New customizable variable 'vc-git-grep-template'.
 This new variable allows customizing the default arguments passed to
 git-grep when 'vc-git-grep' is used.
@@ -358,6 +361,10 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
 *** File headers can be shortened, mimicking Magit's diff format.
 To enable it, set the new defcustom 'diff-font-lock-prettify to t.
 
+*** Prefix arg of 'diff-goto-source' means jump to the old revision
+of the file under version control if point is on an old changed line,
+or to the new revision of the file otherwise.
+
 ** Browse-url
 
 *** The function 'browse-url-emacs' can now visit a URL in selected window.
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index cf52368508..aef16e2e67 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -104,6 +105,9 @@ diff-font-lock-prettify
 (defvar diff-vc-backend nil
   "The VC backend that created the current Diff buffer, if any.")
 
+(defvar diff-vc-revisions nil
+  "The VC revisions compared in the current Diff buffer, if any.")
+
 (defvar diff-outline-regexp
   "\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
 
@@ -1736,7 +1740,12 @@ diff-find-source-location
 		       (match-string 1)))))
 	   (file (or (diff-find-file-name other noprompt)
                      (error "Can't find the file")))
-	   (buf (find-file-noselect file)))
+	   (revision (and other diff-vc-backend
+                          (nth (if reverse 1 0) diff-vc-revisions)))
+	   (buf (if revision
+                    (let ((vc-find-revision-no-save t))
+                      (vc-find-revision file revision diff-vc-backend))
+                  (find-file-noselect file))))
       ;; Update the user preference if he so wished.
       (when (> (prefix-numeric-value other-file) 8)
 	(setq diff-jump-to-old-file other))
@@ -1862,7 +1871,11 @@ diff-goto-source
 `diff-jump-to-old-file' (or its opposite if the OTHER-FILE prefix arg
 is given) determines whether to jump to the old or the new file.
 If the prefix arg is bigger than 8 (for example with \\[universal-argument] \\[universal-argument])
-then `diff-jump-to-old-file' is also set, for the next invocations."
+then `diff-jump-to-old-file' is also set, for the next invocations.
+
+Under version control, the OTHER-FILE prefix arg means jump to the old
+revision of the file if point is on an old changed line, or to the new
+revision of the file otherwise."
   (interactive (list current-prefix-arg last-input-event))
   ;; When pointing at a removal line, we probably want to jump to
   ;; the old location, and else to the new (i.e. as if reverting).
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index dcfbf26e86..7c4c288b66 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -871,6 +871,12 @@ vc-comment-alist
 		       (string :tag "Comment End")))
   :group 'vc)
 
+(defcustom vc-find-revision-no-save nil
+  "If non-nil, `vc-find-revision' doesn't write the created buffer to file."
+  :type 'boolean
+  :group 'vc
+  :version "27.1")
+
 \f
 ;; File property caching
 
@@ -1728,6 +1735,7 @@ vc-diff-internal
     (set-buffer buffer)
     (diff-mode)
     (set (make-local-variable 'diff-vc-backend) (car vc-fileset))
+    (set (make-local-variable 'diff-vc-revisions) (list rev1 rev2))
     (set (make-local-variable 'revert-buffer-function)
 	 (lambda (_ignore-auto _noconfirm)
            (vc-diff-internal async vc-fileset rev1 rev2 verbose)))
@@ -1951,6 +1959,8 @@ vc-revision-other-window
 (defun vc-find-revision (file revision &optional backend)
   "Read REVISION of FILE into a buffer and return the buffer.
 Use BACKEND as the VC backend if specified."
+  (if vc-find-revision-no-save
+      (vc-find-revision-no-save file revision backend)
   (let ((automatic-backup (vc-version-backup-file-name file revision))
 	(filebuf (or (get-file-buffer file) (current-buffer)))
         (filename (vc-version-backup-file-name file revision 'manual)))
@@ -1981,6 +1991,38 @@ vc-find-revision
 	;; Set the parent buffer so that things like
 	;; C-x v g, C-x v l, ... etc work.
 	(set (make-local-variable 'vc-parent-buffer) filebuf))
+      result-buf))))
+
+(defun vc-find-revision-no-save (file revision &optional backend)
+  "Read REVISION of FILE into a buffer and return the buffer.
+Unlike `vc-find-revision', doesn't save the created buffer to file."
+  (let ((filebuf (or (get-file-buffer file) (current-buffer)))
+        (filename (vc-version-backup-file-name file revision 'manual)))
+    (unless (or (get-file-buffer filename)
+                (file-exists-p filename))
+      (with-current-buffer filebuf
+	(let ((failed t))
+	  (unwind-protect
+	      (let ((coding-system-for-read 'no-conversion)
+		    (coding-system-for-write 'no-conversion))
+		(with-current-buffer (create-file-buffer filename)
+                  (setq buffer-file-name filename)
+		  (let ((outbuf (current-buffer)))
+		    (with-current-buffer filebuf
+		      (if backend
+			  (vc-call-backend backend 'find-revision file revision outbuf)
+			(vc-call find-revision file revision outbuf))))
+                  (goto-char (point-min))
+                  (normal-mode)
+	          (set-buffer-modified-p nil)
+                  (setq buffer-read-only t))
+		(setq failed nil))
+	    (when (and failed (get-file-buffer filename))
+	      (kill-buffer (get-file-buffer filename)))))))
+    (let ((result-buf (or (get-file-buffer filename)
+                          (find-file-noselect filename))))
+      (with-current-buffer result-buf
+	(set (make-local-variable 'vc-parent-buffer) filebuf))
       result-buf)))
 
 ;; Header-insertion code

bug#33319: Support revisions in diff-goto-source

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33319@debbugs.gnu.org
> Date: Sat, 10 Nov 2018 23:12:51 +0200
> 
> > And I think we want to update the manual as well.
> 
> Updated.

Thanks, but meanwhile the release branch changed the documentation of
that command, so please wait for the next merge to master, and update
the result, to avoid merge conflicts.

Thanks for the other updates.

bug#33319: Support revisions in diff-goto-source

[Juri Linkov]

>> > And I think we want to update the manual as well.
>> 
>> Updated.
>
> Thanks, but meanwhile the release branch changed the documentation of
> that command, so please wait for the next merge to master, and update
> the result, to avoid merge conflicts.
>
> Thanks for the other updates.

I updated the manual adding new text to your changes.