* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
@ 2015-05-19 11:43 Dmitry Gutov
2015-05-19 13:01 ` martin rudalics
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Dmitry Gutov @ 2015-05-19 11:43 UTC (permalink / raw)
To: 20608
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-19 11:43 bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted Dmitry Gutov
@ 2015-05-19 13:01 ` martin rudalics
2015-05-19 13:03 ` Dmitry Gutov
2015-05-19 17:40 ` Stefan Monnier
2015-05-25 2:57 ` Stefan Monnier
2 siblings, 1 reply; 11+ messages in thread
From: martin rudalics @ 2015-05-19 13:01 UTC (permalink / raw)
To: Dmitry Gutov, 20608
> + (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
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-19 13:01 ` martin rudalics
@ 2015-05-19 13:03 ` Dmitry Gutov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Gutov @ 2015-05-19 13:03 UTC (permalink / raw)
To: martin rudalics, 20608
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-19 11:43 bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted Dmitry Gutov
2015-05-19 13:01 ` martin rudalics
@ 2015-05-19 17:40 ` Stefan Monnier
2015-05-24 21:42 ` Dmitry Gutov
2015-05-25 2:57 ` Stefan Monnier
2 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2015-05-19 17:40 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 20608
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-19 17:40 ` Stefan Monnier
@ 2015-05-24 21:42 ` Dmitry Gutov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Gutov @ 2015-05-24 21:42 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 20608
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-19 11:43 bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted Dmitry Gutov
2015-05-19 13:01 ` martin rudalics
2015-05-19 17:40 ` Stefan Monnier
@ 2015-05-25 2:57 ` Stefan Monnier
2015-05-25 23:41 ` Dmitry Gutov
2 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2015-05-25 2:57 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 20608
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-25 2:57 ` Stefan Monnier
@ 2015-05-25 23:41 ` Dmitry Gutov
2015-05-26 20:53 ` Stefan Monnier
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Gutov @ 2015-05-25 23:41 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 20608
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-25 23:41 ` Dmitry Gutov
@ 2015-05-26 20:53 ` Stefan Monnier
2015-05-27 14:14 ` Dmitry Gutov
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2015-05-26 20:53 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 20608
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-26 20:53 ` Stefan Monnier
@ 2015-05-27 14:14 ` Dmitry Gutov
2015-05-27 16:26 ` Stefan Monnier
0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Gutov @ 2015-05-27 14:14 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 20608
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))
^ permalink raw reply related [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-27 14:14 ` Dmitry Gutov
@ 2015-05-27 16:26 ` Stefan Monnier
2015-05-27 23:29 ` Dmitry Gutov
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2015-05-27 16:26 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 20608
> 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))
^ permalink raw reply [flat|nested] 11+ messages in thread
* bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted
2015-05-27 16:26 ` Stefan Monnier
@ 2015-05-27 23:29 ` Dmitry Gutov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Gutov @ 2015-05-27 23:29 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 20608-done
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-05-27 23:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-19 11:43 bug#20608: 25.0.50; vc-git-log-view-mode inserts "Show 2X entries" at bob when interrupted Dmitry Gutov
2015-05-19 13:01 ` martin rudalics
2015-05-19 13:03 ` Dmitry Gutov
2015-05-19 17:40 ` Stefan Monnier
2015-05-24 21:42 ` Dmitry Gutov
2015-05-25 2:57 ` Stefan Monnier
2015-05-25 23:41 ` Dmitry Gutov
2015-05-26 20:53 ` Stefan Monnier
2015-05-27 14:14 ` Dmitry Gutov
2015-05-27 16:26 ` Stefan Monnier
2015-05-27 23:29 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).