bug#40967: 27.0.50; vc-diff in indirect buffers

bug#40967: 27.0.50; vc-diff in indirect buffers

[Nathan Moreau]

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

Calling vc-diff does not work in indirect buffers. If the base buffer
visits a file under version control, I expect it to behave as if
called from the base buffer.

The attached patch is a possible fix in that direction. I don't know
if other commands in vc should have the same fix, should that kind of
patch be applied.

Reproduction steps:
===============
run emacs -Q
open any file-under-version-control
M-x clone-indirect-buffer
In the new buffer, run vc-diff fails with `vc-deduce-fileset: Buffer
[...] is not associated with a file`


Details about the machine:
====================
Configured using:
 'configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install -C 'CFLAGS=-O2 -static -g3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG SOUND NOTIFY W32NOTIFY ACL GNUTLS LIBXML2
HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS MODULES THREADS PDUMPER LCMS2 GMP

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp65001

Major mode: Messages

Minor modes in effect:
  global-subword-mode: t
  subword-mode: t
  save-place-mode: t
  minibuffer-depth-indicate-mode: t
  global-undo-tree-mode: t
  recentf-mode: t
  show-paren-mode: t
  global-auto-revert-mode: t
  savehist-mode: t
  delete-selection-mode: t
  override-global-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  global-visual-line-mode: t
  visual-line-mode: t
  transient-mark-mode: t

Load-path shadows:
~/.emacs.d/swiper/elpa hides ~/.emacs.d/lispy/elpa

Features:
(shadow sort mail-extr emacsbug message rmc puny format-spec rfc822 mml
mml-sec epa epg epg-config gnus-util rmail rmail-loaddefs time-date
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils bookmark text-property-search pp smerge-mode tabify
multiple-cursors-core rect eieio-opt speedbar sb-image ezimage dframe
counsel xdg dired dired-loaddefs apropos go-mode url url-proxy
url-privacy url-expand url-methods url-history url-cookie url-domsuf
url-util mailcap find-file ffap compile cl-print debug cus-start
cus-load diff-hl face-remap vc-hg vc-dir vc-bzr vc-src vc-sccs vc-svn
vc-cvs vc-rcs ewoc vc vc-dispatcher vc-git diff-mode lispy swiper
lispy-inline noutline outline etags fileloop generator edebug backtrace
help-fns radix-tree lispy-tags mode-local find-func rainbow-delimiters
hl-todo pcase my-color-theme init my-init highlight-symbol thingatpt
cap-words superword subword saveplace ace-link mb-depth hl-line battery
undo-tree diff avy rx ivy-hydra ivy-xref ivy colir color ivy-overlay
xref project recentf tree-widget wid-edit hydra lv hippie-exp comint
ansi-color ring key-chord benchmark-init-modes paren elec-pair
autorevert filenotify savehist delsel dabbrev cl-extra help-mode edmacro
kmacro diminish cl use-package use-package-ensure use-package-delight
use-package-diminish use-package-bind-key bind-key easy-mmode
use-package-core derived benchmark-init advice info package easymenu
browse-url url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json subr-x map url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32
ls-lisp disp-table term/w32-win w32-win w32-vars term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode elisp-mode lisp-mode prog-mode register page
tab-bar menu-bar rfn-eshadow isearch timer select scroll-bar mouse
jit-lock font-lock syntax facemenu font-core term/tty-colors frame
minibuffer cl-generic cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite charscript charprop case-table epa-hook jka-cmpr-hook help
simple abbrev obarray cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
threads w32notify w32 lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 311030 114283)
 (symbols 48 21138 1)
 (strings 32 60774 2776)
 (string-bytes 1 1941284)
 (vectors 16 26613)
 (vector-slots 8 306017 10984)
 (floats 8 293 480)
 (intervals 56 3804 12)
 (buffers 1000 24))

[-- Attachment #2: vc-diff-indirect-buffers.diff --]
[-- Type: application/octet-stream, Size: 763 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 132278e823..d1c339c5b5 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1874,9 +1874,10 @@ saving the buffer."
   (interactive (list current-prefix-arg t))
   (if historic
       (call-interactively 'vc-version-diff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
-    (vc-diff-internal t (vc-deduce-fileset t) nil nil
-		      (called-interactively-p 'interactive))))
+    (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+      (when buffer-file-name (vc-buffer-sync not-urgent))
+      (vc-diff-internal t (vc-deduce-fileset t) nil nil
+                        (called-interactively-p 'interactive)))))
 
 ;;;###autoload
 (defun vc-diff-mergebase (_files rev1 rev2)

bug#40967: 27.0.50; vc-diff in indirect buffers

[Dmitry Gutov]

On 29.04.2020 21:01, Nathan Moreau wrote:
> Reproduction steps:
> ===============
> run emacs -Q
> open any file-under-version-control
> M-x clone-indirect-buffer
> In the new buffer, run vc-diff fails with `vc-deduce-fileset: Buffer
> [...] is not associated with a file`

I'm not sure we really want to support that workflow. Or else a lot of 
commands would have to be changed the same way, inside and outside of VC.

But opinions welcome.

bug#40967: 27.0.50; vc-diff in indirect buffers

[Phil Sainty]

On 30/04/20 6:13 am, Dmitry Gutov wrote:
> On 29.04.2020 21:01, Nathan Moreau wrote:
>> open any file-under-version-control
>> M-x clone-indirect-buffer
>> In the new buffer, run vc-diff fails with `vc-deduce-fileset:
>> Buffer [...] is not associated with a file`
>
> I'm not sure we really want to support that workflow. Or else
> a lot of commands would have to be changed the same way,
> inside and outside of VC.
>
> But opinions welcome.

FWIW, we did the same thing for `diff-buffer-with-file' quite
recently:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=1bcf5d02da96784a04034b4c0aba8fdfa1413c4e

Personally I'd be ok with seeing piecemeal changes to commands
like these to enable certain common workflows to DTRT with
indirect buffers.  I think there will likely be a subset of
commands which are of higher value than others, and so there
might not be such a vast number of things which are very useful
to change in practice.  (Or maybe it's a slippery slope.)

Something like the following might be convenient:

 (defun buffer-base-buffer-file-name (&optional buffer)
   "Like `buffer-file-name', but also supporting indirect buffers."
   (buffer-file-name
    (or (buffer-base-buffer buffer) buffer (current-buffer))))


-Phil

bug#40967: 27.0.50; vc-diff in indirect buffers

[Dmitry Gutov]

On 16.06.2020 15:08, Phil Sainty wrote:
> Personally I'd be ok with seeing piecemeal changes to commands
> like these to enable certain common workflows to DTRT with
> indirect buffers.

All right then.

> I think there will likely be a subset of
> commands which are of higher value than others, and so there
> might not be such a vast number of things which are very useful
> to change in practice.

IDK, seems like it wouldn't simplify the proposed patch. It's short 
already anywa.

bug#40967: 27.0.50; vc-diff in indirect buffers

[Nathan Moreau]

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

Here is a new version of the patch, with support for most of the
commands defined in vc.el.

The support is not exhaustive as I was not able to make all commands work
I did not succeed in testing all the commands: vc-delete-file and
vc-rename-file are probably candidates next.

Feedback is very welcome, I am not used to the contribution process on Emacs.
Nathan

On Wed, 17 Jun 2020 at 00:37, Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 16.06.2020 15:08, Phil Sainty wrote:
> > Personally I'd be ok with seeing piecemeal changes to commands
> > like these to enable certain common workflows to DTRT with
> > indirect buffers.
>
> All right then.
>
> > I think there will likely be a subset of
> > commands which are of higher value than others, and so there
> > might not be such a vast number of things which are very useful
> > to change in practice.
>
> IDK, seems like it wouldn't simplify the proposed patch. It's short
> already anywa.

[-- Attachment #2: vc-diff-indirect-buffers.diff --]
[-- Type: application/octet-stream, Size: 4667 bytes --]

commit 46841d828c6f1bc9504019266e78d7c565871700
Author: Nathan Moreau <nathan.moreau@m4x.org>
Date:   Wed Jul 1 02:02:25 2020 +0200

    Improve support for indirect buffers.
    
    * lisp/vc/vc.el (vc-deduce-fileset-1): new defun.
    (vc-deduce-fileset): adapt.
    (vc-maybe-buffer-sync): new defun.
    (vc-diff): adapt.
    (vc-ediff): adapt.
    (vc-root-diff): adapt.
    (vc-revision-other-window): adapt.

diff --git lisp/vc/vc.el lisp/vc/vc.el
index c640ba0420..291e10bc8a 100644
--- lisp/vc/vc.el
+++ lisp/vc/vc.el
@@ -1027,7 +1027,9 @@ vc-deduce-fileset
 list of marked files, or the current directory if no files are
 marked.
 Otherwise, if the current buffer is visiting a version-controlled
-file, FILESET is a single-file list containing that file's name.
+file or is an indirect buffer whose base buffer visits a
+version-controlled file, FILESET is a single-file list containing
+that file's name.
 Otherwise, if ALLOW-UNREGISTERED is non-nil and the visited file
 is unregistered, FILESET is a single-file list containing the
 name of the visited file.
@@ -1041,6 +1043,14 @@ vc-deduce-fileset
 the returned list.
 
 BEWARE: this function may change the current buffer."
+  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+    (vc-deduce-fileset-1 not-state-changing
+                         allow-unregistered
+                         state-model-only-files)))
+
+(defun vc-deduce-fileset-1 (not-state-changing
+                            allow-unregistered
+                            state-model-only-files)
   (let (backend)
     (cond
      ((derived-mode-p 'vc-dir-mode)
@@ -1062,7 +1072,7 @@ vc-deduce-fileset
 				      (derived-mode-p 'dired-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
-	(vc-deduce-fileset not-state-changing allow-unregistered state-model-only-files)))
+	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
      ((and (derived-mode-p 'log-view-mode)
 	   (setq backend (vc-responsible-backend default-directory)))
       (list backend nil))
@@ -1878,6 +1888,10 @@ vc-root-version-diff
        t (list backend (list rootdir)) rev1 rev2
        (called-interactively-p 'interactive)))))
 
+(defun vc-maybe-buffer-sync (not-urgent)
+  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+    (when buffer-file-name (vc-buffer-sync not-urgent))))
+
 ;;;###autoload
 (defun vc-diff (&optional historic not-urgent)
   "Display diffs between file revisions.
@@ -1890,7 +1904,7 @@ vc-diff
   (interactive (list current-prefix-arg t))
   (if historic
       (call-interactively 'vc-version-diff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
+    (vc-maybe-buffer-sync not-urgent)
     (vc-diff-internal t (vc-deduce-fileset t) nil nil
 		      (called-interactively-p 'interactive))))
 
@@ -1969,7 +1983,7 @@ vc-ediff
   (interactive (list current-prefix-arg t))
   (if historic
       (call-interactively 'vc-version-ediff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
+    (vc-maybe-buffer-sync not-urgent)
     (vc-version-ediff (cadr (vc-deduce-fileset t)) nil nil)))
 
 ;;;###autoload
@@ -1986,7 +2000,7 @@ vc-root-diff
   (if historic
       ;; We want the diff for the VC root dir.
       (call-interactively 'vc-root-version-diff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
+    (vc-maybe-buffer-sync not-urgent)
     (let ((backend (vc-deduce-backend))
 	  (default-directory default-directory)
 	  rootdir working-revision)
@@ -2026,17 +2040,18 @@ vc-revision-other-window
 If the current file is named `F', the revision is named `F.~REV~'.
 If `F.~REV~' already exists, use it instead of checking it out again."
   (interactive
-   (save-current-buffer
+   (with-current-buffer (or (buffer-base-buffer) (current-buffer))
      (vc-ensure-vc-buffer)
      (list
       (vc-read-revision "Revision to visit (default is working revision): "
                         (list buffer-file-name)))))
-  (vc-ensure-vc-buffer)
-  (let* ((file buffer-file-name)
-	 (revision (if (string-equal rev "")
-		      (vc-working-revision file)
-		    rev)))
-    (switch-to-buffer-other-window (vc-find-revision file revision))))
+  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+    (vc-ensure-vc-buffer)
+    (let* ((file buffer-file-name)
+	   (revision (if (string-equal rev "")
+		         (vc-working-revision file)
+		       rev)))
+      (switch-to-buffer-other-window (vc-find-revision file revision)))))
 
 (defun vc-find-revision (file revision &optional backend)
   "Read REVISION of FILE into a buffer and return the buffer.

bug#40967: 27.0.50; vc-diff in indirect buffers

[Nathan Moreau]

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

Hello,

Gentle ping. This can be rebased on whichever branch you like if it does
not apply cleanly (currently it is based on d6f6353cfdbbea for because it
happened to be checked-out locally).

Nathan

On Wed, 1 Jul 2020 at 02:19, Nathan Moreau <nathan.moreau@m4x.org> wrote:

> Here is a new version of the patch, with support for most of the
> commands defined in vc.el.
>
> The support is not exhaustive as I was not able to make all commands work
> I did not succeed in testing all the commands: vc-delete-file and
> vc-rename-file are probably candidates next.
>
> Feedback is very welcome, I am not used to the contribution process on
> Emacs.
> Nathan
>
> On Wed, 17 Jun 2020 at 00:37, Dmitry Gutov <dgutov@yandex.ru> wrote:
> >
> > On 16.06.2020 15:08, Phil Sainty wrote:
> > > Personally I'd be ok with seeing piecemeal changes to commands
> > > like these to enable certain common workflows to DTRT with
> > > indirect buffers.
> >
> > All right then.
> >
> > > I think there will likely be a subset of
> > > commands which are of higher value than others, and so there
> > > might not be such a vast number of things which are very useful
> > > to change in practice.
> >
> > IDK, seems like it wouldn't simplify the proposed patch. It's short
> > already anywa.
>

[-- Attachment #2: Type: text/html, Size: 1838 bytes --]

bug#40967: 27.0.50; vc-diff in indirect buffers

[Lars Ingebrigtsen]

Nathan Moreau <nathan.moreau@m4x.org> writes:

> Gentle ping. This can be rebased on whichever branch you like if it
> does not apply cleanly (currently it is based on d6f6353cfdbbea for
> because it happened to be checked-out locally).

The patch does not apply cleanly to Emacs 28, so I've respun it.  There
were some conflicts; please check that it looks correct.

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 3852a64550..46c44fa54b 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1038,7 +1038,9 @@ vc-deduce-fileset
 list of marked files, or the current directory if no files are
 marked.
 Otherwise, if the current buffer is visiting a version-controlled
-file, FILESET is a single-file list containing that file's name.
+file or is an indirect buffer whose base buffer visits a
+version-controlled file, FILESET is a single-file list containing
+that file's name.
 Otherwise, if ALLOW-UNREGISTERED is non-nil and the visited file
 is unregistered, FILESET is a single-file list containing the
 name of the visited file.
@@ -1052,6 +1054,14 @@ vc-deduce-fileset
 the returned list.
 
 BEWARE: this function may change the current buffer."
+  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+    (vc-deduce-fileset-1 not-state-changing
+                         allow-unregistered
+                         state-model-only-files)))
+
+(defun vc-deduce-fileset-1 (not-state-changing
+                            allow-unregistered
+                            state-model-only-files)
   (let (backend)
     (cond
      ((derived-mode-p 'vc-dir-mode)
@@ -1073,7 +1083,7 @@ vc-deduce-fileset
 				      (derived-mode-p 'dired-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
-	(vc-deduce-fileset not-state-changing allow-unregistered state-model-only-files)))
+	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
      ((and (not buffer-file-name)
 	   (setq backend (vc-responsible-backend default-directory)))
       (list backend nil))
@@ -1883,6 +1893,10 @@ vc-root-version-diff
        t (list backend (list rootdir)) rev1 rev2
        (called-interactively-p 'interactive)))))
 
+(defun vc-maybe-buffer-sync (not-urgent)
+  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+    (when buffer-file-name (vc-buffer-sync not-urgent))))
+
 ;;;###autoload
 (defun vc-diff (&optional historic not-urgent)
   "Display diffs between file revisions.
@@ -1895,6 +1909,7 @@ vc-diff
   (interactive (list current-prefix-arg t))
   (if historic
       (call-interactively 'vc-version-diff)
+    (vc-maybe-buffer-sync not-urgent)
     (let ((fileset (vc-deduce-fileset t)))
       (vc-buffer-sync-fileset fileset not-urgent)
       (vc-diff-internal t fileset nil nil
@@ -1981,7 +1996,7 @@ vc-ediff
   (interactive (list current-prefix-arg t))
   (if historic
       (call-interactively 'vc-version-ediff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
+    (vc-maybe-buffer-sync not-urgent)
     (vc-version-ediff (cadr (vc-deduce-fileset t)) nil nil)))
 
 ;;;###autoload
@@ -1998,7 +2013,7 @@ vc-root-diff
   (if historic
       ;; We want the diff for the VC root dir.
       (call-interactively 'vc-root-version-diff)
-    (when buffer-file-name (vc-buffer-sync not-urgent))
+    (vc-maybe-buffer-sync not-urgent)
     (let ((backend (vc-deduce-backend))
 	  (default-directory default-directory)
 	  rootdir working-revision)
@@ -2038,17 +2053,18 @@ vc-revision-other-window
 If the current file is named `F', the revision is named `F.~REV~'.
 If `F.~REV~' already exists, use it instead of checking it out again."
   (interactive
-   (save-current-buffer
+   (with-current-buffer (or (buffer-base-buffer) (current-buffer))
      (vc-ensure-vc-buffer)
      (list
       (vc-read-revision "Revision to visit (default is working revision): "
                         (list buffer-file-name)))))
-  (vc-ensure-vc-buffer)
-  (let* ((file buffer-file-name)
-	 (revision (if (string-equal rev "")
-		      (vc-working-revision file)
-		    rev)))
-    (switch-to-buffer-other-window (vc-find-revision file revision))))
+  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+    (vc-ensure-vc-buffer)
+    (let* ((file buffer-file-name)
+	   (revision (if (string-equal rev "")
+		         (vc-working-revision file)
+		       rev)))
+      (switch-to-buffer-other-window (vc-find-revision file revision)))))
 
 (defun vc-find-revision (file revision &optional backend)
   "Read REVISION of FILE into a buffer and return the buffer.


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

bug#40967: 27.0.50; vc-diff in indirect buffers

[Nathan Moreau]

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

Hi Lars,

Thanks for having a look. This looks good to me.

Best regards,
Nathan

On Thu, 1 Oct 2020 at 19:56, Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Nathan Moreau <nathan.moreau@m4x.org> writes:
>
> > Gentle ping. This can be rebased on whichever branch you like if it
> > does not apply cleanly (currently it is based on d6f6353cfdbbea for
> > because it happened to be checked-out locally).
>
> The patch does not apply cleanly to Emacs 28, so I've respun it.  There
> were some conflicts; please check that it looks correct.
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 3852a64550..46c44fa54b 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1038,7 +1038,9 @@ vc-deduce-fileset
>  list of marked files, or the current directory if no files are
>  marked.
>  Otherwise, if the current buffer is visiting a version-controlled
> -file, FILESET is a single-file list containing that file's name.
> +file or is an indirect buffer whose base buffer visits a
> +version-controlled file, FILESET is a single-file list containing
> +that file's name.
>  Otherwise, if ALLOW-UNREGISTERED is non-nil and the visited file
>  is unregistered, FILESET is a single-file list containing the
>  name of the visited file.
> @@ -1052,6 +1054,14 @@ vc-deduce-fileset
>  the returned list.
>
>  BEWARE: this function may change the current buffer."
> +  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
> +    (vc-deduce-fileset-1 not-state-changing
> +                         allow-unregistered
> +                         state-model-only-files)))
> +
> +(defun vc-deduce-fileset-1 (not-state-changing
> +                            allow-unregistered
> +                            state-model-only-files)
>    (let (backend)
>      (cond
>       ((derived-mode-p 'vc-dir-mode)
> @@ -1073,7 +1083,7 @@ vc-deduce-fileset
>                                       (derived-mode-p 'dired-mode)))))
>        (progn                  ;FIXME: Why not `with-current-buffer'?
> --Stef.
>         (set-buffer vc-parent-buffer)
> -       (vc-deduce-fileset not-state-changing allow-unregistered
> state-model-only-files)))
> +       (vc-deduce-fileset-1 not-state-changing allow-unregistered
> state-model-only-files)))
>       ((and (not buffer-file-name)
>            (setq backend (vc-responsible-backend default-directory)))
>        (list backend nil))
> @@ -1883,6 +1893,10 @@ vc-root-version-diff
>         t (list backend (list rootdir)) rev1 rev2
>         (called-interactively-p 'interactive)))))
>
> +(defun vc-maybe-buffer-sync (not-urgent)
> +  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
> +    (when buffer-file-name (vc-buffer-sync not-urgent))))
> +
>  ;;;###autoload
>  (defun vc-diff (&optional historic not-urgent)
>    "Display diffs between file revisions.
> @@ -1895,6 +1909,7 @@ vc-diff
>    (interactive (list current-prefix-arg t))
>    (if historic
>        (call-interactively 'vc-version-diff)
> +    (vc-maybe-buffer-sync not-urgent)
>      (let ((fileset (vc-deduce-fileset t)))
>        (vc-buffer-sync-fileset fileset not-urgent)
>        (vc-diff-internal t fileset nil nil
> @@ -1981,7 +1996,7 @@ vc-ediff
>    (interactive (list current-prefix-arg t))
>    (if historic
>        (call-interactively 'vc-version-ediff)
> -    (when buffer-file-name (vc-buffer-sync not-urgent))
> +    (vc-maybe-buffer-sync not-urgent)
>      (vc-version-ediff (cadr (vc-deduce-fileset t)) nil nil)))
>
>  ;;;###autoload
> @@ -1998,7 +2013,7 @@ vc-root-diff
>    (if historic
>        ;; We want the diff for the VC root dir.
>        (call-interactively 'vc-root-version-diff)
> -    (when buffer-file-name (vc-buffer-sync not-urgent))
> +    (vc-maybe-buffer-sync not-urgent)
>      (let ((backend (vc-deduce-backend))
>           (default-directory default-directory)
>           rootdir working-revision)
> @@ -2038,17 +2053,18 @@ vc-revision-other-window
>  If the current file is named `F', the revision is named `F.~REV~'.
>  If `F.~REV~' already exists, use it instead of checking it out again."
>    (interactive
> -   (save-current-buffer
> +   (with-current-buffer (or (buffer-base-buffer) (current-buffer))
>       (vc-ensure-vc-buffer)
>       (list
>        (vc-read-revision "Revision to visit (default is working revision):
> "
>                          (list buffer-file-name)))))
> -  (vc-ensure-vc-buffer)
> -  (let* ((file buffer-file-name)
> -        (revision (if (string-equal rev "")
> -                     (vc-working-revision file)
> -                   rev)))
> -    (switch-to-buffer-other-window (vc-find-revision file revision))))
> +  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
> +    (vc-ensure-vc-buffer)
> +    (let* ((file buffer-file-name)
> +          (revision (if (string-equal rev "")
> +                        (vc-working-revision file)
> +                      rev)))
> +      (switch-to-buffer-other-window (vc-find-revision file revision)))))
>
>  (defun vc-find-revision (file revision &optional backend)
>    "Read REVISION of FILE into a buffer and return the buffer.
>
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>

[-- Attachment #2: Type: text/html, Size: 6470 bytes --]

bug#40967: 27.0.50; vc-diff in indirect buffers

[Lars Ingebrigtsen]

Nathan Moreau <nathan.moreau@m4x.org> writes:

> Thanks for having a look. This looks good to me.

Dmitry, do you have any comments here?  The feature makes sense to me
conceptually, but I'm not a domain expert.

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

bug#40967: 27.0.50; vc-diff in indirect buffers

[Dmitry Gutov]

On 03.10.2020 20:33, Lars Ingebrigtsen wrote:
> Nathan Moreau <nathan.moreau@m4x.org> writes:
> 
>> Thanks for having a look. This looks good to me.
> 
> Dmitry, do you have any comments here?  The feature makes sense to me
> conceptually, but I'm not a domain expert.

No objections from me.

I'm not a fan of using indirect buffers like this, but if you like the 
feature, and the patch works well for you, please go ahead.

bug#40967: 27.0.50; vc-diff in indirect buffers

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> No objections from me.
>
> I'm not a fan of using indirect buffers like this, but if you like the
> feature, and the patch works well for you, please go ahead.

OK; pushed to Emacs 28 now.

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

bug#40967: 27.0.50; vc-diff in indirect buffers

[Richard Copley]

In 'vc-revision-other-window', the 'switch-to-buffer-other-window'
call is now wrapped in a 'with-current-buffer', and therefore
'vc-revision-other-window' no longer has the effect of switching
buffer.

This is unfortunate. See 'ediff-vc-internal':

    (vc-revision-other-window rev1)
    (setq rev1buf (current-buffer)
          file1 (buffer-file-name)))

At this point, 'file1' is intended to be the name of the versioned
backup file checked out by 'vc-revision-other-window'. With your
change, instead 'file1' is the name of the base file.

      [...]
      (push (lambda ()
          (ediff-delete-version-file file1)
          [...])
        startup-hooks))

The startup-hooks function deletes the base file containing the local
changes that the user wanted to examine, instead of the temporary
backup file.

Recipe, from emacs -Q:

* visit a file under vc that has local changes
* M-x ediff-revision RET RET RET RET

By the time the Ediff control panel appears, the file has been deleted.

That behaviour is, emphatically, NOT desirable. I'd even go as far as
to suggest it should be fixed as a matter of urgency, or reverted for
now. Just my opinion, of course.