bug#11605: 24.1.50; vc-ediff revert annoyance

bug#11605: 24.1.50; vc-ediff revert annoyance

[William Xu]

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

Steps to reproduce:

  1. emacs -Q
  2. visit a version controlled file, say `foo'.
  3. M-x vc-ediff, run it couple of times.

You will see this annoying message many times:

  File foo.~master~ changed on disk.  Reread from disk? (yes or no)

The reason is the ~master~ file is checked out everytime we run
vc-ediff, and after ediff session, the ~master~ buffer is always left
there, hence, when running vc-ediff multiple times it constantly asks
you whether you want to revert.  Why not let it revert automatically?

How about this patch?

-- 
William

http://xwl.appspot.com

[-- Attachment #2: 0001-vc-find-revision-Auto-revert-file-revision-buffer-qu.patch --]
[-- Type: application/octet-stream, Size: 1097 bytes --]

From d2af0f4dcc81b39d9360135244d6870388a264a3 Mon Sep 17 00:00:00 2001
From: William Xu <william.xwl@gmail.com>
Date: Sat, 2 Jun 2012 12:16:45 +0800
Subject: [PATCH] (vc-find-revision): Auto revert file revision buffer
 quietly.

---
 lisp/vc/vc.el |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 87e4e1c..bd337d0 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1815,11 +1815,13 @@ Use BACKEND as the VC backend if specified."
 		(delete-file filename))))
 	  (vc-mode-line file))
 	(message "Checking out %s...done" filename)))
-    (let ((result-buf (find-file-noselect filename)))
+    (let ((result-buf (or (get-file-buffer filename)
+                          (find-file-noselect filename))))
       (with-current-buffer result-buf
 	;; 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))
+	(set (make-local-variable 'vc-parent-buffer) filebuf)
+        (revert-buffer nil t))
       result-buf)))
 
 ;; Header-insertion code
-- 
1.7.8.2

bug#11605: 24.1.50; vc-ediff revert annoyance

[Lars Ingebrigtsen]

William Xu <william.xwl@gmail.com> writes:

>   1. emacs -Q
>   2. visit a version controlled file, say `foo'.
>   3. M-x vc-ediff, run it couple of times.
>
> You will see this annoying message many times:
>
>   File foo.~master~ changed on disk.  Reread from disk? (yes or no)

I can confirm that this bug is still present in the Emacs trunk.

> The reason is the ~master~ file is checked out everytime we run
> vc-ediff, and after ediff session, the ~master~ buffer is always left
> there, hence, when running vc-ediff multiple times it constantly asks
> you whether you want to revert.  Why not let it revert automatically?
>
> How about this patch?

[...]

I think the patch sounds reasonable, but the vc machinery is something
with which I'm not overly familiar.

Could somebody take a peek at this patch?

> From d2af0f4dcc81b39d9360135244d6870388a264a3 Mon Sep 17 00:00:00 2001
> From: William Xu <william.xwl@gmail.com>
> Date: Sat, 2 Jun 2012 12:16:45 +0800
> Subject: [PATCH] (vc-find-revision): Auto revert file revision buffer
>  quietly.
>
> ---
>  lisp/vc/vc.el |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 87e4e1c..bd337d0 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1815,11 +1815,13 @@ Use BACKEND as the VC backend if specified."
>  		(delete-file filename))))
>  	  (vc-mode-line file))
>  	(message "Checking out %s...done" filename)))
> -    (let ((result-buf (find-file-noselect filename)))
> +    (let ((result-buf (or (get-file-buffer filename)
> +                          (find-file-noselect filename))))
>        (with-current-buffer result-buf
>  	;; 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))
> +	(set (make-local-variable 'vc-parent-buffer) filebuf)
> +        (revert-buffer nil t))
>        result-buf)))
>  
>  ;; Header-insertion code

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

bug#11605: 24.1.50; vc-ediff revert annoyance

[Dmitry Gutov]

On 02/24/2016 08:33 AM, Lars Ingebrigtsen wrote:

>> @@ -1815,11 +1815,13 @@ Use BACKEND as the VC backend if specified."
>>  		(delete-file filename))))
>>  	  (vc-mode-line file))
>>  	(message "Checking out %s...done" filename)))
>> -    (let ((result-buf (find-file-noselect filename)))
>> +    (let ((result-buf (or (get-file-buffer filename)
>> +                          (find-file-noselect filename))))

Doesn't find-file-noselect call get-file-buffer anyway?

>>        (with-current-buffer result-buf
>>  	;; 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))
>> +	(set (make-local-variable 'vc-parent-buffer) filebuf)
>> +        (revert-buffer nil t))

It seems like this might conflict with other uses of vc-find-revision, 
like vc-revision-other-window. Where the user is allowed to change the 
contents of the returned buffer, and might've done so before we do this 
automatic silent revert.

Maybe do it on ediff's side instead?

bug#11605: 24.1.50; vc-ediff revert annoyance

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 02/24/2016 08:33 AM, Lars Ingebrigtsen wrote:
>
>>> @@ -1815,11 +1815,13 @@ Use BACKEND as the VC backend if specified."
>>>  		(delete-file filename))))
>>>  	  (vc-mode-line file))
>>>  	(message "Checking out %s...done" filename)))
>>> -    (let ((result-buf (find-file-noselect filename)))
>>> +    (let ((result-buf (or (get-file-buffer filename)
>>> +                          (find-file-noselect filename))))
>
> Doesn't find-file-noselect call get-file-buffer anyway?

It does, but then it goes into all the "File %s changed on disk.  Reread
from disk?" stuff, which is what the bug is about.  But, I mean, the
file may have changed, so...

>>>        (with-current-buffer result-buf
>>>  	;; 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))
>>> +	(set (make-local-variable 'vc-parent-buffer) filebuf)
>>> +        (revert-buffer nil t))
>
> It seems like this might conflict with other uses of vc-find-revision,
> like vc-revision-other-window. Where the user is allowed to change the
> contents of the returned buffer, and might've done so before we do
> this automatic silent revert.
>
> Maybe do it on ediff's side instead?

If the buffer with the comparison file had been killed before all this
had happened, we wouldn't have gotten the "file changed on disk" thing,
yes...  That may be a better fix.

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

bug#11605: 24.1.50; vc-ediff revert annoyance

[Dmitry Gutov]

On 02/26/2016 08:01 AM, Lars Ingebrigtsen wrote:

> If the buffer with the comparison file had been killed before all this
> had happened, we wouldn't have gotten the "file changed on disk" thing,
> yes...  That may be a better fix.

Maybe so. But I'm not sure it's easy to compute the exact buffer name, 
to kill it.

Or disable the automatic prompting somehow (add a NOWARN argument to 
vc-find-revision?), and call revert-buffer from ediff after.

bug#11605: 24.1.50; vc-ediff revert annoyance

[William Xu]

Dmitry Gutov <dgutov@yandex.ru> writes:

> It seems like this might conflict with other uses of vc-find-revision,
> like vc-revision-other-window. Where the user is allowed to change the
> contents of the returned buffer, and might've done so before we do
> this automatic silent revert.

It sounds strange that one would try to modify it.  I thought the
checkout version is only for comparison.  The file name XX.~yy~ looks
also very similar to automatic backup files, e.g., "!tmp!foo.cpp.~1~".

-- 
William

My idea of roughing it turning the air conditioner too low.

bug#11605: 24.1.50; vc-ediff revert annoyance

[Dmitry Gutov]

On 02/26/2016 11:38 AM, William Xu wrote:

> It sounds strange that one would try to modify it.  I thought the
> checkout version is only for comparison.  The file name XX.~yy~ looks
> also very similar to automatic backup files, e.g., "!tmp!foo.cpp.~1~".

I agree it is strange, but it's not out of the question. Maybe the user 
wants to restore an old version of a function, and on a whim, started 
retouching it in the "old version" buffer?

bug#11605: 24.1.50; vc-ediff revert annoyance

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>>        (with-current-buffer result-buf
>>>  	;; 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))
>>> +	(set (make-local-variable 'vc-parent-buffer) filebuf)
>>> +        (revert-buffer nil t))
>
> It seems like this might conflict with other uses of vc-find-revision,
> like vc-revision-other-window. Where the user is allowed to change the
> contents of the returned buffer, and might've done so before we do
> this automatic silent revert.
>
> Maybe do it on ediff's side instead?

Yeah, after poking at this a bit more, I agree -- ediff should set up
this better, and just calling revert in the vc functions does not seem
to be the logical choice.

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