Patch to make VC annotate async

Patch to make VC annotate async

[Tom Tromey]

This patch changes the VC annotate command to allow back ends to run
annotation in the background.  It also changes the CVS back end to do
this.

I was initially inspired to write this patch because "svn annotate"
can be rather slow, and I didn't want to wait for it to complete.
However, it turns out that changing vc-svn.el to annotate in the
background does not work -- in my test case (a file in GCC), the
annotation is truncated before the end of the file.  I'm not sure what
is going on here.

A couple other back ends (hg and mcvs) could be changed to use code
similar to the CVS code below.  I didn't attempt this.

Tom


2007-06-24  Tom Tromey  <tromey@redhat.com>

	* vc.el (vc-annotate-display-select): Don't pop to buffer if one
	is specified.
	(vc-annotate): Run vc-annotate-display-select via vc-exec-after.
	* vc-cvs.el (vc-cvs-annotate-output): New variable.
	(vc-cvs-annotate-process-filter): New function.
	(vc-cvs-annotate-command): Run command async.  Use
	vc-cvs-annotate-process-filter.

Index: vc-cvs.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc-cvs.el,v
retrieving revision 1.80
diff -u -r1.80 vc-cvs.el
--- vc-cvs.el	21 Jan 2007 03:53:10 -0000	1.80
+++ vc-cvs.el	24 Jun 2007 02:23:00 -0000
@@ -153,6 +153,8 @@
 ;;; Internal variables
 ;;;
 
+;; Collects initial 'cvs annotate' output.
+(defvar vc-cvs-annotate-output nil)
 
 ;;;
 ;;; State-querying functions
@@ -588,14 +590,33 @@
                (and rev2 (concat "-r" rev2))
                (vc-switches 'CVS 'diff))))))
 
+(defun vc-cvs-annotate-process-filter (process string)
+  (setq vc-cvs-annotate-output (concat vc-cvs-annotate-output string))
+  (let ((list (split-string vc-cvs-annotate-output "\n")))
+    (if (equal (substring vc-cvs-annotate-output -1) "\n")
+	(setq vc-cvs-annotate-output "")
+      ;; If input doesn't end with \n, don't use the last list
+      ;; element.
+      (setq vc-cvs-annotate-output (car (last list)))
+      (setq list (nreverse (cdr (nreverse list)))))
+    (while (and list
+		(not (string-match "^[0-9]" (car list))))
+      (setq list (cdr list)))
+    ;; If there is a list element remaining, it means we saw the first
+    ;; line we want to insert.  Reset the process filter and continue.
+    (when list
+      (set-process-filter process 'vc-process-filter)
+      (mapc (lambda (line) (vc-process-filter process (concat line "\n"))) list)
+      (vc-process-filter process vc-cvs-annotate-output))))
+
 (defun vc-cvs-annotate-command (file buffer &optional version)
   "Execute \"cvs annotate\" on FILE, inserting the contents in BUFFER.
 Optional arg VERSION is a version to annotate from."
-  (vc-cvs-command buffer 0 file "annotate" (if version (concat "-r" version)))
-  (with-current-buffer buffer
-    (goto-char (point-min))
-    (re-search-forward "^[0-9]")
-    (delete-region (point-min) (1- (point)))))
+  (vc-cvs-command buffer 'async file "annotate"
+		  (if version (concat "-r" version)))
+  (set (make-local-variable 'vc-cvs-annotate-output) "")
+  (set-process-filter (get-buffer-process buffer)
+		      'vc-cvs-annotate-process-filter))
 
 (defun vc-cvs-annotate-current-time ()
   "Return the current time, based at midnight of the current day, and
Index: vc.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/vc.el,v
retrieving revision 1.428
diff -u -r1.428 vc.el
--- vc.el	20 Jun 2007 16:32:09 -0000	1.428
+++ vc.el	24 Jun 2007 02:23:04 -0000
@@ -3099,7 +3099,8 @@
 use; you may override this using the second optional arg MODE."
   (interactive)
   (if mode (setq vc-annotate-display-mode mode))
-  (pop-to-buffer (or buffer (current-buffer)))
+  ;; If BUFFER is set then we've already popped to it.
+  (unless buffer (pop-to-buffer (current-buffer)))
   (cond ((null vc-annotate-display-mode)
          ;; The ratio is global, thus relative to the global color-map.
          (kill-local-variable 'vc-annotate-color-map)
@@ -3159,7 +3160,6 @@
   (vc-ensure-vc-buffer)
   (setq vc-annotate-display-mode display-mode) ;Not sure why.  --Stef
   (let* ((temp-buffer-name (format "*Annotate %s (rev %s)*" (buffer-name) rev))
-         (temp-buffer-show-function 'vc-annotate-display-select)
          ;; If BUF is specified, we presume the caller maintains current line,
          ;; so we don't need to do it here.  This implementation may give
          ;; strange results occasionally in the case of REV != WORKFILE-REV.
@@ -3184,10 +3184,15 @@
         (set (make-local-variable 'vc-annotate-parent-file) file)
         (set (make-local-variable 'vc-annotate-parent-rev) rev)
         (set (make-local-variable 'vc-annotate-parent-display-mode)
-             display-mode)))
-    (when current-line
-      (goto-line current-line temp-buffer-name))
-    (message "Annotating... done")))
+             display-mode)
+
+	(vc-exec-after
+	 `(progn
+	    (vc-annotate-display-select ,(current-buffer))
+	    (when ,current-line
+	      (goto-line ,current-line ,temp-buffer-name))
+	    (unless (active-minibuffer-window)
+	      (message "Annotating... done"))))))))
 
 (defun vc-annotate-prev-version (prefix)
   "Visit the annotation of the version previous to this one.

Re: Patch to make VC annotate async

[Richard Stallman]

    However, it turns out that changing vc-svn.el to annotate in the
    background does not work -- in my test case (a file in GCC), the
    annotation is truncated before the end of the file.  I'm not sure what
    is going on here.

This reminds me of other bugs we have seen, and struggled with, over
the years.  Thus, investigating this is very important.  Can you post
precisel directions so that others can try to reproduce it?

Re: Patch to make VC annotate async

[Tom Tromey]

>>>>> "rms" == Richard Stallman <rms@gnu.org> writes:

>     However, it turns out that changing vc-svn.el to annotate in the
>     background does not work -- in my test case (a file in GCC), the
>     annotation is truncated before the end of the file.  I'm not sure what
>     is going on here.

rms> This reminds me of other bugs we have seen, and struggled with, over
rms> the years.  Thus, investigating this is very important.  Can you post
rms> precisel directions so that others can try to reproduce it?

Sure.  My current test case is somewhat complicated, unfortunately:

* Apply the patch I sent.

* Build and install Emacs.

* Check out GCC svn trunk.  You probably don't need all of it; the
  test file I use is gcc/c-parser.c.

* Run the new Emacs with -q

* Visit the test file and "C-x v g" as a test, to make sure it is
  working correctly.

* Eval this redefinition:

(defun vc-svn-annotate-command (file buf &optional rev)
  (vc-svn-command buf 'async file "annotate" (if rev (concat "-r" rev))))

  This changes a 0 to 'async.

* "C-x v g" again on the test file.  For me this reliably cuts off the
  output at the same place, before the file's end.

Tom

Re: Patch to make VC annotate async

[Stefan Monnier]

> This patch changes the VC annotate command to allow back ends to run
> annotation in the background.  It also changes the CVS back end to
> do this.

I've been using a local patch to do the same thing for a long time now.
The main problem has been that the UI is somewhat incompatible with an async
behavior: the coloring of lines may depend on the overall youngest and
oldest lines, so the color of even the first line may depend on the content
of the last line.
IIRC that was the main reason not to accept my changes at first.
But IIRC I've fixed this since so that the colors are updated as the content
comes along.  I'll try and extract a new patch for it.

> 	* vc.el (vc-annotate-display-select): Don't pop to buffer if one
> 	is specified.
> 	(vc-annotate): Run vc-annotate-display-select via vc-exec-after.

Oh, so you don't even add the coloring until after the whole data
was received.  Hmmm...

> 	* vc-cvs.el (vc-cvs-annotate-output): New variable.
> 	(vc-cvs-annotate-process-filter): New function.
> 	(vc-cvs-annotate-command): Run command async.  Use
> 	vc-cvs-annotate-process-filter.

I don't understand the prupose of vc-cvs-annotate-process-filter.  In my
implementation I haven't needed any change to vc-cvs.el other than passing
the `async' flag to vc-do-command.


        Stefan

Re: Patch to make VC annotate async

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> * vc.el (vc-annotate-display-select): Don't pop to buffer if one
>> is specified.
>> (vc-annotate): Run vc-annotate-display-select via vc-exec-after.

Stefan> Oh, so you don't even add the coloring until after the whole data
Stefan> was received.  Hmmm...

Yes.  This seemed like the simplest change, and after trying it a bit
I felt that it didn't make the user interface unacceptably odd.  The
weirdest bit is when point moves, but that is nice once you adjust to
it.

I also experimented a bit with how the buffer is displayed.  In
particular I tried making it so that the buffer is displayed only
after the contents are fontified.  I didn't like this since it means
the buffer is hidden while I do other tasks; popping it immediately is
less intrusive -- when the buffer is initially hidden I found that I
could sometimes context switch and forget that I had asked for the
annotation.

Also, in my various tests, I find that the buffer contents actually
show up reasonably quickly.  The slowdown with svn seems to come while
computing the text to send; once sent it seems to arrive quickly
enough that I don't notice the contents not being fontified.

>> * vc-cvs.el (vc-cvs-annotate-output): New variable.
>> (vc-cvs-annotate-process-filter): New function.
>> (vc-cvs-annotate-command): Run command async.  Use
>> vc-cvs-annotate-process-filter.

Stefan> I don't understand the prupose of
Stefan> vc-cvs-annotate-process-filter.  In my implementation I
Stefan> haven't needed any change to vc-cvs.el other than passing the
Stefan> `async' flag to vc-do-command.

The CVS annotator strips some text from the front of the buffer.  I
chose this approach since it means the user will not see the text
being stripped.  But, since the text does show up so quickly, and
since the code would be simpler, I could change this to a simple
vc-exec-after if you like.

Tom

Re: Patch to make VC annotate async

[Richard Stallman]

Can anyone else reproduce this problem?  I would like people to try.
We may be able to fix a lingering lossage.

(Tom, please don't let this be forgotten!)

Re: Patch to make VC annotate async

[Stefan Monnier]

>>> * vc.el (vc-annotate-display-select): Don't pop to buffer if one
>>> is specified.
>>> (vc-annotate): Run vc-annotate-display-select via vc-exec-after.

Stefan> Oh, so you don't even add the coloring until after the whole data
Stefan> was received.  Hmmm...

> Yes.  This seemed like the simplest change, and after trying it a bit
> I felt that it didn't make the user interface unacceptably odd.  The
> weirdest bit is when point moves, but that is nice once you adjust to
> it.

> I also experimented a bit with how the buffer is displayed.  In
> particular I tried making it so that the buffer is displayed only
> after the contents are fontified.  I didn't like this since it means
> the buffer is hidden while I do other tasks; popping it immediately is
> less intrusive -- when the buffer is initially hidden I found that I
> could sometimes context switch and forget that I had asked for the
> annotation.

So it looks like my patch is orthogonal: what it does is move the
highlighting to font-lock so that it can be done on-the-fly, and tweak the
oldest/youngest calculation so that it is also done on the fly (from the
process-filter).
So adding it to yours would simply cause the unfinished buffer to already
be fontified.

> Also, in my various tests, I find that the buffer contents actually
> show up reasonably quickly.

I introduced the async behaviuor specifically because the buffer content
didn't show up quickly in my use case (which was mostly interacting with
the Emacs CVS repository).

> The slowdown with svn seems to come while computing the text to send; once
> sent it seems to arrive quickly enough that I don't notice the contents
> not being fontified.

In the case of CVS, the annotated text is sent directly from the server, so
the rhythm at which it arrives depends on the speed of your connection.
Maybe SVN computes the annotated text locally, in which case the network
connection would only be involved before the result starts showing up.
But then I wonder: why did you bother to introduce this async patch (since
you seem to find the "context switch" to something else to be a problem
rather than a feature)?

> The CVS annotator strips some text from the front of the buffer.  I
> chose this approach since it means the user will not see the text
> being stripped.  But, since the text does show up so quickly, and
> since the code would be simpler, I could change this to a simple
> vc-exec-after if you like.

Oh, I see, I had completely forgotten about it, but yes I have it
written as:

  (with-current-buffer buffer
    ;; FIXME: it'd be even better to do it in the process-filter.
    (vc-exec-after
     '(let ((inhibit-read-only t))
        (save-excursion
          (goto-char (point-min))
          (re-search-forward "^[0-9]")
          (delete-region (point-min) (1- (point)))))))

so at least at that point I thought it would be better to do it like
you did.


        Stefan

Re: Patch to make VC annotate async

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The slowdown with svn seems to come while computing the text to send; once
>> sent it seems to arrive quickly enough that I don't notice the contents
>> not being fontified.

Stefan> In the case of CVS, the annotated text is sent directly from
Stefan> the server, so the rhythm at which it arrives depends on the
Stefan> speed of your connection.  Maybe SVN computes the annotated
Stefan> text locally, in which case the network connection would only
Stefan> be involved before the result starts showing up.

Or maybe my network connection is faster.  I didn't look too deeply
into this.

Stefan> But then I wonder: why did you bother to introduce this async
Stefan> patch (since you seem to find the "context switch" to
Stefan> something else to be a problem rather than a feature)?

For me the question here is when the buffer should be displayed.  It
could be displayed immediately, or it could be displayed after the
contents are all available.  I tried both.  Delaying the display of
the buffer felt a bit unfriendly because the creation of a new window
will rearrange the display enough that I lose track of point.  On the
other hand, immediately displaying the buffer gives me time to adjust
and continue working.  I hope this explains things a bit better; this
area is somewhat subjective and I notice that at least one other
package has gone the maximally configurable approach here -- see
Man-notify-method.

Stefan> Oh, I see, I had completely forgotten about it, but yes I have it
Stefan> written as:

Stefan>   (with-current-buffer buffer
Stefan>     ;; FIXME: it'd be even better to do it in the process-filter.

This suggests that you preferred the approach I took ;-)
But, I don't really care.  I can rewrite it to use vc-exec-after --
if that is what you want, let me know, and I will send a new patch.

Tom

Re: Patch to make VC annotate async

[Stefan Monnier]

> For me the question here is when the buffer should be displayed.  It
> could be displayed immediately, or it could be displayed after the
> contents are all available.  I tried both.  Delaying the display of
> the buffer felt a bit unfriendly because the creation of a new window
> will rearrange the display enough that I lose track of point.  On the
> other hand, immediately displaying the buffer gives me time to adjust
> and continue working.  I hope this explains things a bit better; this
> area is somewhat subjective and I notice that at least one other
> package has gone the maximally configurable approach here -- see
> Man-notify-method.

Oh, I see.  So we're in full agreement.

Stefan> (with-current-buffer buffer
Stefan> ;; FIXME: it'd be even better to do it in the process-filter.

> This suggests that you preferred the approach I took ;-)
> But, I don't really care.  I can rewrite it to use vc-exec-after --
> if that is what you want, let me know, and I will send a new patch.

Yes, that's what I wrote as well, isn't it?


        Stefan

Re: Patch to make VC annotate async

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

Stefan> Oh, I see.  So we're in full agreement.

Awesome.

>> This suggests that you preferred the approach I took ;-)
>> But, I don't really care.  I can rewrite it to use vc-exec-after --
>> if that is what you want, let me know, and I will send a new patch.

Stefan> Yes, that's what I wrote as well, isn't it?

Sorry about that, I misread what you wrote.

Tom

Re: Patch to make VC annotate async

[Stefan Monnier]

Hmm... so looking into it some more I see that I already installed some part
of my hack: the part that made vc-annotate use font-lock (this made
a significant difference on long files thanks to jit-lock).

But the part that made the async update work with the autoscale feature is
nowhere to be found.  Maybe I never finished it and just threw it out.

In any case it's orthogonal to Tom's change, and I completely support Tom's
change, so unless there's some objection I suggest we install it.


        Stefan

Re: Patch to make VC annotate async

[Stefan Monnier]

For the vc-cvs.el part of the patch , can you try the patch below?
Also I noticed that my own version used vc-exec-after because that makes it
work both for async and sync and I select between the two with

  (if (and (vc-stay-local-p file) (fboundp 'start-process)) 'async 0)

Not sure if it matters.


        Stefan


--- vc-cvs.el	26 jan 2007 23:51:27 -0500	1.80
+++ vc-cvs.el	07 jui 2007 17:00:40 -0400	
@@ -588,14 +591,22 @@
                (and rev2 (concat "-r" rev2))
                (vc-switches 'CVS 'diff))))))
 
+(defun vc-cvs-annotate-process-filter (process string)
+  (setq string (concat (process-get process 'output) string))
+  (if (not (string-match "^[0-9]" string))
+      ;; Still waiting for the first real line.
+      (process-put process 'output string)
+    ;; FIXME: we shouldn't hardcode vc-process-filter here.
+    (set-process-filter process 'vc-process-filter)
+    (vc-process-filter process (substring string (match-beginning 0)))))
+
 (defun vc-cvs-annotate-command (file buffer &optional version)
   "Execute \"cvs annotate\" on FILE, inserting the contents in BUFFER.
 Optional arg VERSION is a version to annotate from."
-  (vc-cvs-command buffer 0 file "annotate" (if version (concat "-r" version)))
-  (with-current-buffer buffer
-    (goto-char (point-min))
-    (re-search-forward "^[0-9]")
-    (delete-region (point-min) (1- (point)))))
+  (vc-cvs-command buffer 'async file "annotate"
+                  (if version (concat "-r" version)))
+  (set-process-filter (get-buffer-process buffer)
+		      'vc-cvs-annotate-process-filter))
 
 (defun vc-cvs-annotate-current-time ()
   "Return the current time, based at midnight of the current day, and

Re: Patch to make VC annotate async

[Tom Tromey]

>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:

Stefan> For the vc-cvs.el part of the patch , can you try the patch below?

I haven't tried it yet, but I do have a question.

Stefan> +(defun vc-cvs-annotate-process-filter (process string)
Stefan> +  (setq string (concat (process-get process 'output) string))
Stefan> +  (if (not (string-match "^[0-9]" string))
Stefan> +      ;; Still waiting for the first real line.
Stefan> +      (process-put process 'output string)
Stefan> +    ;; FIXME: we shouldn't hardcode vc-process-filter here.
Stefan> +    (set-process-filter process 'vc-process-filter)
Stefan> +    (vc-process-filter process (substring string (match-beginning 0)))))

I had written a much more complicated filter because my reading of the
Elisp manual is that the output from the process won't necessarily
appear as full lines.  Is that not so?

Tom
o

Re: Patch to make VC annotate async

[David Kastrup]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Stefan" == Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
> Stefan> For the vc-cvs.el part of the patch , can you try the patch below?
>
> I haven't tried it yet, but I do have a question.
>
> Stefan> +(defun vc-cvs-annotate-process-filter (process string)
> Stefan> +  (setq string (concat (process-get process 'output) string))
> Stefan> +  (if (not (string-match "^[0-9]" string))
> Stefan> +      ;; Still waiting for the first real line.
> Stefan> +      (process-put process 'output string)
> Stefan> +    ;; FIXME: we shouldn't hardcode vc-process-filter here.
> Stefan> +    (set-process-filter process 'vc-process-filter)
> Stefan> +    (vc-process-filter process (substring string (match-beginning 0)))))
>
> I had written a much more complicated filter because my reading of the
> Elisp manual is that the output from the process won't necessarily
> appear as full lines.  Is that not so?

Correct.  But the above does wait for a complete line, followed by a
newline followed by [0-9], then calls the filter function on
everything up to there.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Patch to make VC annotate async

[Tom Tromey]

>>>>> "David" == David Kastrup <dak@gnu.org> writes:

>> I had written a much more complicated filter because my reading of the
>> Elisp manual is that the output from the process won't necessarily
>> appear as full lines.  Is that not so?

David> Correct.  But the above does wait for a complete line, followed by a
David> newline followed by [0-9], then calls the filter function on
David> everything up to there.

I drastically misread this patch, how embarrassing.  Thanks for your
explanation.

I tried the patch and it works for me.  Thanks Stefan.

Tom

Re: Patch to make VC annotate async

[Stefan Monnier]

> I drastically misread this patch, how embarrassing.  Thanks for your
> explanation.

> I tried the patch and it works for me.  Thanks Stefan.

Thank you, installed,


        Stefan