bug#1256: Race condition in vc-diff

bug#1256: Race condition in vc-diff

[Bob Rogers]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1188 bytes --]

   This problem seems to affect only the trunk, and not 22.3.  (I last
built emacs on 21-Oct.)

   To reproduce this, you need a working copy with a VCS that has a fast
backend "diff to base" command; something that doesn't have to contact a
server is probably a requirement (I used SVN with the WC on a local
disk).  You also need a version-controlled file with a trivial change
with respect to the base version; touching a single line is sufficient.

   If you visit this file and do "C-x v =" in a single-window frame that
is more than about 30 lines tall, and if the backend diff command is
fast enough, the diff window will remain half the size of the screen,
rather than being shrunk to fit the buffer.  This is because
vc-exec-after finds that the buffer process has already finished, and
runs vc-diff-finish before the diff buffer has been made visible.  In
that case, vc-diff-finish assumes that the user has already buried it.

   I am not a big fan of this shrink-wrapping, but I do think vc-diff
ought to behave consistently (and it has worked this way for quite a
while now).  The attached patch is sufficient to fix it.

					-- Bob Rogers
					   http://rgrjr.dyndns.org/


[-- Attachment #2: Type: text/plain, Size: 934 bytes --]

--- lisp/vc.el.~1.706.~	2008-10-06 21:02:25.000000000 -0400
+++ lisp/vc.el	2008-10-25 17:49:35.000000000 -0400
@@ -1507,10 +1507,12 @@
       ;; bindings are nicer for read only buffers. pcl-cvs does the
       ;; same thing.
       (setq buffer-read-only t)
-      (vc-exec-after `(vc-diff-finish ,(current-buffer) ',(when verbose
-                                                            messages)))
       ;; Display the buffer, but at the end because it can change point.
       (pop-to-buffer (current-buffer))
+      ;; Do this after pop-to-buffer, because it needs to see the proper window
+      ;; state if it runs immediately.  -- rgr, 25-Oct-08.
+      (vc-exec-after `(vc-diff-finish ,(current-buffer) ',(when verbose
+                                                            messages)))
       ;; In the async case, we return t even if there are no differences
       ;; because we don't know that yet.
       t)))

bug#1256: Race condition in vc-diff

[Stefan Monnier]

>    If you visit this file and do "C-x v =" in a single-window frame that
> is more than about 30 lines tall, and if the backend diff command is
> fast enough, the diff window will remain half the size of the screen,
> rather than being shrunk to fit the buffer.  This is because
> vc-exec-after finds that the buffer process has already finished, and
> runs vc-diff-finish before the diff buffer has been made visible.  In
> that case, vc-diff-finish assumes that the user has already buried it.

>    I am not a big fan of this shrink-wrapping, but I do think vc-diff
> ought to behave consistently (and it has worked this way for quite a
> while now).  The attached patch is sufficient to fix it.

As the comment indicates, this patch is not quite good enough because
pop-to-buffer may change point.  We probably will need to manipulate
point explicitly to work around the problem.  BTW you can make the
"race-condition" deterministic by making the diff command synchronous
(as is the case with the RCS backend, for example, where your problem
should just *always* happen).


        Stefan

bug#1256: Race condition in vc-diff

[Bob Rogers]

   From: Stefan Monnier <monnier@iro.umontreal.ca>
   Date: Sat, 25 Oct 2008 22:31:18 -0400

   >    If you visit this file and do "C-x v =" in a single-window frame that
   > is more than about 30 lines tall, and if the backend diff command is
   > fast enough, the diff window will remain half the size of the screen,
   > rather than being shrunk to fit the buffer.  This is because
   > vc-exec-after finds that the buffer process has already finished, and
   > runs vc-diff-finish before the diff buffer has been made visible.  In
   > that case, vc-diff-finish assumes that the user has already buried it.

   >    I am not a big fan of this shrink-wrapping, but I do think vc-diff
   > ought to behave consistently (and it has worked this way for quite a
   > while now).  The attached patch is sufficient to fix it.

   As the comment indicates, this patch is not quite good enough because
   pop-to-buffer may change point.  We probably will need to manipulate
   point explicitly to work around the problem.  

Problem?  vc-diff-finish sets point in the diff buffer, so if it runs
immediately, it seems to me that it ought to override what pop-to-buffer
does.  But perhaps I don't understand what point-changing behavior you
mean.

   BTW you can make the "race-condition" deterministic by making the
   diff command synchronous (as is the case with the RCS backend, for
   example, where your problem should just *always* happen).

	   Stefan

It never occurred to me to try that.  However, the same argument would
seem to apply for (setq vc-disable-async-diff t), but that only makes
the bug appear intermittently for me, even with the identical test case.

					-- Bob

bug#1256: Race condition in vc-diff

[Chong Yidong]

>    If you visit this file and do "C-x v =" in a single-window frame
> that is more than about 30 lines tall, and if the backend diff command
> is fast enough, the diff window will remain half the size of the
> screen, rather than being shrunk to fit the buffer.  This is because
> vc-exec-after finds that the buffer process has already finished, and
> runs vc-diff-finish before the diff buffer has been made visible.  In
> that case, vc-diff-finish assumes that the user has already buried it.
>
>    I am not a big fan of this shrink-wrapping, but I do think vc-diff
> ought to behave consistently (and it has worked this way for quite a
> while now).  The attached patch is sufficient to fix it.

I committed the patch to trunk.  Thanks.