GNU bug report logs - #20608
25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 20608 in the body.
You can then email your comments to 20608 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when
 interrupted
Date: Tue, 19 May 2015 14:43:31 +0300

Tags: patch

1. Visit lisp/vc/vc-dispatcher.el (the important part being that it's a
file with long history).

2. Press C-x v l, see the print-log buffer pop up, with [waiting ...].

3. Press g before the process finishes running.

See the window shrink and "Show 2X entries    Show unlimited entries"
appear at the top (it will also get added at the bottom when the process
finishes).

It seems like a general problem, to be fixed either in vc-do-command (by
unsetting the process sentinel before deleting the process), or in
vc--process-sentinel. Here's the patch for the second option:

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index a2c1cba..d6b50b7 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -183,35 +183,36 @@ Another is that undo information is not kept."
 (defvar vc-sentinel-movepoint)          ;Dynamically scoped.
 
 (defun vc--process-sentinel (p code)
-  (let ((buf (process-buffer p)))
+  (let ((buf (process-buffer p))
+        (status (process-status p)))
     ;; Impatient users sometime kill "slow" buffers; check liveness
     ;; to avoid "error in process sentinel: Selecting deleted buffer".
     (when (buffer-live-p buf)
       (with-current-buffer buf
         (setq mode-line-process
-              (let ((status (process-status p)))
-                ;; Leave mode-line uncluttered, normally.
-                (unless (eq 'exit status)
-                  (format " (%s)" status))))
-        (let (vc-sentinel-movepoint
-              (m (process-mark p)))
-          ;; Normally, we want async code such as sentinels to not move point.
-          (save-excursion
-            (goto-char m)
-            ;; Each sentinel may move point and the next one should be run
-            ;; at that new point.  We could get the same result by having
-            ;; each sentinel read&set process-mark, but since `cmd' needs
-            ;; to work both for async and sync processes, this would be
-            ;; difficult to achieve.
-            (vc-exec-after code)
-            (move-marker m (point)))
-          ;; But sometimes the sentinels really want to move point.
-          (when vc-sentinel-movepoint
-	    (let ((win (get-buffer-window (current-buffer) 0)))
-	      (if (not win)
-		  (goto-char vc-sentinel-movepoint)
-		(with-selected-window win
-		  (goto-char vc-sentinel-movepoint))))))))))
+              ;; Leave mode-line uncluttered, normally.
+              (unless (eq 'exit status)
+                (format " (%s)" status)))
+        (unless (eq 'signal status)
+          (let (vc-sentinel-movepoint
+                (m (process-mark p)))
+            ;; Normally, we want async code such as sentinels to not move point.
+            (save-excursion
+              (goto-char m)
+              ;; Each sentinel may move point and the next one should be run
+              ;; at that new point.  We could get the same result by having
+              ;; each sentinel read&set process-mark, but since `cmd' needs
+              ;; to work both for async and sync processes, this would be
+              ;; difficult to achieve.
+              (vc-exec-after code)
+              (move-marker m (point)))
+            ;; But sometimes the sentinels really want to move point.
+            (when vc-sentinel-movepoint
+              (let ((win (get-buffer-window (current-buffer) 0)))
+                (if (not win)
+                    (goto-char vc-sentinel-movepoint)
+                  (with-selected-window win
+                    (goto-char vc-sentinel-movepoint)))))))))))
 
 (defun vc-set-mode-line-busy-indicator ()
   (setq mode-line-process

Message #8 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Dmitry Gutov <dgutov <at> yandex.ru>, 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries"
 at bob when interrupted
Date: Tue, 19 May 2015 15:01:28 +0200

> +                (if (not win)
> +                    (goto-char vc-sentinel-movepoint)
> +                  (with-selected-window win
> +                    (goto-char vc-sentinel-movepoint)))))))))))

How about

                (if win
                    (set-window-point win vc-sentinel-movepoint)
		  (goto-char vc-sentinel-movepoint))

instead?

martin

Message #11 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: martin rudalics <rudalics <at> gmx.at>, 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X
 entries" at bob when interrupted
Date: Tue, 19 May 2015 16:03:45 +0300

On 05/19/2015 04:01 PM, martin rudalics wrote:
>  > +                (if (not win)
>  > +                    (goto-char vc-sentinel-movepoint)
>  > +                  (with-selected-window win
>  > +                    (goto-char vc-sentinel-movepoint)))))))))))
>
> How about
>
>                  (if win
>                      (set-window-point win vc-sentinel-movepoint)
>            (goto-char vc-sentinel-movepoint))
>
> instead?

Looks OK to me. But it's not relevant to this bug or the proposed patch.

Message #14 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50;
 vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
Date: Tue, 19 May 2015 13:40:03 -0400

> It seems like a general problem, to be fixed either in vc-do-command (by
> unsetting the process sentinel before deleting the process), or in
> vc--process-sentinel. Here's the patch for the second option:

Indeed, it's a general problem.  Maybe handling it in vc-do-command
would be a good idea, but unsetting the process sentinel altogether
sounds a bit dangerous (the sentinel might also be used to clear the
":running" annotation in the modeline and other such things).


        Stefan


> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> index a2c1cba..d6b50b7 100644
> --- a/lisp/vc/vc-dispatcher.el
> +++ b/lisp/vc/vc-dispatcher.el
> @@ -183,35 +183,36 @@ Another is that undo information is not kept."
>  (defvar vc-sentinel-movepoint)          ;Dynamically scoped.
 
>  (defun vc--process-sentinel (p code)
> -  (let ((buf (process-buffer p)))
> +  (let ((buf (process-buffer p))
> +        (status (process-status p)))
>      ;; Impatient users sometime kill "slow" buffers; check liveness
>      ;; to avoid "error in process sentinel: Selecting deleted buffer".
>      (when (buffer-live-p buf)
>        (with-current-buffer buf
>          (setq mode-line-process
> -              (let ((status (process-status p)))
> -                ;; Leave mode-line uncluttered, normally.
> -                (unless (eq 'exit status)
> -                  (format " (%s)" status))))
> -        (let (vc-sentinel-movepoint
> -              (m (process-mark p)))
> -          ;; Normally, we want async code such as sentinels to not move point.
> -          (save-excursion
> -            (goto-char m)
> -            ;; Each sentinel may move point and the next one should be run
> -            ;; at that new point.  We could get the same result by having
> -            ;; each sentinel read&set process-mark, but since `cmd' needs
> -            ;; to work both for async and sync processes, this would be
> -            ;; difficult to achieve.
> -            (vc-exec-after code)
> -            (move-marker m (point)))
> -          ;; But sometimes the sentinels really want to move point.
> -          (when vc-sentinel-movepoint
> -	    (let ((win (get-buffer-window (current-buffer) 0)))
> -	      (if (not win)
> -		  (goto-char vc-sentinel-movepoint)
> -		(with-selected-window win
> -		  (goto-char vc-sentinel-movepoint))))))))))
> +              ;; Leave mode-line uncluttered, normally.
> +              (unless (eq 'exit status)
> +                (format " (%s)" status)))
> +        (unless (eq 'signal status)
> +          (let (vc-sentinel-movepoint
> +                (m (process-mark p)))
> +            ;; Normally, we want async code such as sentinels to not move point.
> +            (save-excursion
> +              (goto-char m)
> +              ;; Each sentinel may move point and the next one should be run
> +              ;; at that new point.  We could get the same result by having
> +              ;; each sentinel read&set process-mark, but since `cmd' needs
> +              ;; to work both for async and sync processes, this would be
> +              ;; difficult to achieve.
> +              (vc-exec-after code)
> +              (move-marker m (point)))
> +            ;; But sometimes the sentinels really want to move point.
> +            (when vc-sentinel-movepoint
> +              (let ((win (get-buffer-window (current-buffer) 0)))
> +                (if (not win)
> +                    (goto-char vc-sentinel-movepoint)
> +                  (with-selected-window win
> +                    (goto-char vc-sentinel-movepoint)))))))))))
 
>  (defun vc-set-mode-line-busy-indicator ()
>    (setq mode-line-process

Message #17 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X
 entries" at bob when interrupted
Date: Mon, 25 May 2015 00:42:33 +0300

On 05/19/2015 08:40 PM, Stefan Monnier wrote:
>> It seems like a general problem, to be fixed either in vc-do-command (by
>> unsetting the process sentinel before deleting the process), or in
>> vc--process-sentinel. Here's the patch for the second option:
>
> Indeed, it's a general problem.  Maybe handling it in vc-do-command
> would be a good idea, but unsetting the process sentinel altogether
> sounds a bit dangerous (the sentinel might also be used to clear the
> ":running" annotation in the modeline and other such things).

I think it's only dangerous if the sentinel is used to clean up some 
other buffer than the one where the process is running (vc-do-command 
will take care about the latter, and anyway it's launching a new 
process, so mode-line-process will be set either way). Do we know of the 
instances of the former?

Doing it in vc--process-sentinel indeed seems more dangerous, because 
it'll preclude doing cleanup even when no new process is being launched.

As far as the current (Show 2X entries) problem goes, we should've been 
able to use a more direct approach, and make the choice inside the 
delayed code, but unfortunately the buffer process is already nil in there.

So, this doesn't work:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 1bd04e1..88bd335 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2263,13 +2263,15 @@ earlier revisions.  Show up to LIMIT entries 
(non-nil means unlimited)."
     ;; the major-mode.
     (pop-to-buffer buffer-name)
     (vc-run-delayed
-     (let ((inhibit-read-only t))
-       (funcall setup-buttons-func backend files retval)
-       (shrink-window-if-larger-than-buffer)
-       (when goto-location-func
-         (funcall goto-location-func backend)
-         (setq vc-sentinel-movepoint (point)))
-       (set-buffer-modified-p nil)))))
+      (let ((inhibit-read-only t)
+            (proc (get-buffer-process buffer-name)))
+        (when (or (null proc) (eq (process-status proc) 'exit))
+          (funcall setup-buttons-func backend files retval)
+          (shrink-window-if-larger-than-buffer)
+          (when goto-location-func
+            (funcall goto-location-func backend)
+            (setq vc-sentinel-movepoint (point)))
+          (set-buffer-modified-p nil))))))

 (defun vc-incoming-outgoing-internal (backend remote-location 
buffer-name type)
   (vc-log-internal-common

Message #20 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50;
 vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
Date: Sun, 24 May 2015 22:57:22 -0400

> See the window shrink and "Show 2X entries    Show unlimited entries"
> appear at the top (it will also get added at the bottom when the process
> finishes).

> It seems like a general problem, to be fixed either in vc-do-command (by
> unsetting the process sentinel before deleting the process), or in
> vc--process-sentinel. Here's the patch for the second option:

Could it be that the problem is that in vc-do-command we first do

      (unless (or (eq buffer t)
		  (and (stringp buffer)
		       (string= (buffer-name) buffer))
		  (eq buffer (current-buffer)))
	(vc-setup-buffer buffer))

and only afterwards do we do

      (let ((oldproc (get-buffer-process (current-buffer))))
        ;; If we wanted to wait for oldproc to finish before doing
        ;; something, we'd have used vc-eval-after.
        ;; Use `delete-process' rather than `kill-process' because we don't
        ;; want any of its output to appear from now on.
        (when oldproc (delete-process oldproc)))

?


-- Stefan

Message #23 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X
 entries" at bob when interrupted
Date: Tue, 26 May 2015 02:41:13 +0300

On 05/25/2015 05:57 AM, Stefan Monnier wrote:

> Could it be that the problem is that in vc-do-command we first do
> ...
> and only afterwards do we do
> ...
> ?

Swapping them makes no difference. It doesn't help with the viability of 
my latest patch (for vc-log-internal-common) either.

Message #26 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50;
 vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
Date: Tue, 26 May 2015 16:53:12 -0400

>> Could it be that the problem is that in vc-do-command we first do
>> ...
>> and only afterwards do we do
>> ...
>> ?
> Swapping them makes no difference.

Hmm... so I guess in the case at hand, the issue is that the buffer is
setup (e.g. erased) before calling vc-do-command (unless the issue is
that the sentinel is not run during delete-process but only later, but
my reading of process.c makes me think this should not be the case).
But I think the real fix is in this direction: kill the process (and run
its sentinel) earlier, i.e. before setting up the buffer.

The reason why I think this is the better way forward, is that it lets
us run the sentinels in the normal way, so we don't need to worry about
what the sentinels might or might not do.


        Stefan

Message #29 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X
 entries" at bob when interrupted
Date: Wed, 27 May 2015 17:14:29 +0300

On 05/26/2015 11:53 PM, Stefan Monnier wrote:

> Hmm... so I guess in the case at hand, the issue is that the buffer is
> setup (e.g. erased) before calling vc-do-command

Indeed, vc-git-print-log calls vc-setup-buffer.

> But I think the real fix is in this direction: kill the process (and run
> its sentinel) earlier, i.e. before setting up the buffer.
>
> The reason why I think this is the better way forward, is that it lets
> us run the sentinels in the normal way, so we don't need to worry about
> what the sentinels might or might not do.

Good point, then the delayed code being unable to read the process 
status is irrelevant.

Any reservations about this patch?

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index a2c1cba..ec55867 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -171,6 +171,12 @@ Another is that undo information is not kept."
   (let ((camefrom (current-buffer))
 	(olddir default-directory))
     (set-buffer (get-buffer-create buf))
+    (let ((oldproc (get-buffer-process (current-buffer))))
+      ;; If we wanted to wait for oldproc to finish before doing
+      ;; something, we'd have used vc-eval-after.
+      ;; Use `delete-process' rather than `kill-process' because we don't
+      ;; want any of its output to appear from now on.
+      (when oldproc (delete-process oldproc)))
     (kill-all-local-variables)
     (set (make-local-variable 'vc-parent-buffer) camefrom)
     (set (make-local-variable 'vc-parent-buffer-name)
@@ -302,12 +308,6 @@ case, and the process object in the asynchronous case."
 		  (eq buffer (current-buffer)))
 	(vc-setup-buffer buffer))
       ;; If there's some previous async process still running, just 
kill it.
-      (let ((oldproc (get-buffer-process (current-buffer))))
-        ;; If we wanted to wait for oldproc to finish before doing
-        ;; something, we'd have used vc-eval-after.
-        ;; Use `delete-process' rather than `kill-process' because we don't
-        ;; want any of its output to appear from now on.
-        (when oldproc (delete-process oldproc)))
       (let ((squeezed (remq nil flags))
 	    (inhibit-read-only t)
 	    (status 0))

Message #32 received at 20608 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 20608 <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50;
 vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
Date: Wed, 27 May 2015 12:26:48 -0400

> Any reservations about this patch?

Assuming it works, LGTM,


        Stefan


> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> index a2c1cba..ec55867 100644
> --- a/lisp/vc/vc-dispatcher.el
> +++ b/lisp/vc/vc-dispatcher.el
> @@ -171,6 +171,12 @@ Another is that undo information is not kept."
>    (let ((camefrom (current-buffer))
>  	(olddir default-directory))
>      (set-buffer (get-buffer-create buf))
> +    (let ((oldproc (get-buffer-process (current-buffer))))
> +      ;; If we wanted to wait for oldproc to finish before doing
> +      ;; something, we'd have used vc-eval-after.
> +      ;; Use `delete-process' rather than `kill-process' because we don't
> +      ;; want any of its output to appear from now on.
> +      (when oldproc (delete-process oldproc)))
>      (kill-all-local-variables)
>      (set (make-local-variable 'vc-parent-buffer) camefrom)
>      (set (make-local-variable 'vc-parent-buffer-name)
> @@ -302,12 +308,6 @@ case, and the process object in the asynchronous case."
>  		  (eq buffer (current-buffer)))
>  	(vc-setup-buffer buffer))
>        ;; If there's some previous async process still running, just
> kill it.
> -      (let ((oldproc (get-buffer-process (current-buffer))))
> -        ;; If we wanted to wait for oldproc to finish before doing
> -        ;; something, we'd have used vc-eval-after.
> -        ;; Use `delete-process' rather than `kill-process' because we don't
> -        ;; want any of its output to appear from now on.
> -        (when oldproc (delete-process oldproc)))
>        (let ((squeezed (remq nil flags))
>  	    (inhibit-read-only t)
>  	    (status 0))

Message #37 received at 20608-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 20608-done <at> debbugs.gnu.org
Subject: Re: bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X
 entries" at bob when interrupted
Date: Thu, 28 May 2015 02:29:19 +0300

On 05/27/2015 07:26 PM, Stefan Monnier wrote:

> Assuming it works, LGTM,

It does solve the described problem.

Thanks for taking a look, installed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.