bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Wolfgang Jenkner]

Call vc-dir for some repo under hg version control and then type l in
the resulting *vc-dir* buffer.

As expected, this produces a *vc-change-log* buffer with the most recent
change at the beginning.  However, point is at the end of the buffer.
I would expect point to be put next to the most recent commit, as it is
the case for, e.g., git.  Note that explicitly selecting a revision
(typing C-u l) does work.

The different behaviour is due to the hg log subprocess being run
synchronously while the git subprocess is run asynchronously.

For an async process the following happens:

vc-process-filter inserts each output chunk at the process mark and
updates the latter; vc-run-delayed has advised the sentinel to go to the
process mark and insert additional stuff (buttons) when the process is
finished.  All those insertions happen inside save-excursion forms.
However, the sentinel sets point to the value of vc-sentinel-movepoint
(if non-nil).

For a sync process the following happens:

The output is inserted and additional stuff set up with vc-run-delayed
is inserted at point.  Those insertions don't happen inside
save-excursion forms and vc-sentinel-movepoint is ignored.

Now, while it is true that the name of vc-sentinel-movepoint doesn't
sound like it was meant for synchronous processes anyway, it is also
true that generic functions like vc-print-log and callees don't know
whether the subprocess will be run synchronously or asynchronously.

So I wonder whether it wouldn't be worthwile to make their job easier
(and thereby avoid bugs like the one described above) by handling async
and sync processes alike in this respect.  The condition for this to
work is that backend functions don't expect point to be preserved,
except, of course, if it is actually their purpose to compute point
(like for vc-git-show-log-entry).

Here's a patch.

-- >8 --
Subject: [PATCH] Handle point for sync vc processes like for async ones

* lisp/vc/vc-dispatcher.el (vc--process-sentinel): Cut some code.
(vc-exec-after): Move it here.  Remove comment which has not been
matching the code for a while.
(vc-do-command): Use save-excursion around the sync process call.
---
 lisp/vc/vc-dispatcher.el | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index ec55867..4f44d35 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -199,25 +199,7 @@ Another is that undo information is not kept."
                 ;; 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))))))))))
+        (vc-exec-after code)))))
 
 (defun vc-set-mode-line-busy-indicator ()
   (setq mode-line-process
@@ -241,7 +223,20 @@ CODE should be a function of no arguments."
      ((or (null proc) (eq (process-status proc) 'exit))
       ;; Make sure we've read the process's output before going further.
       (when proc (accept-process-output proc))
-      (if (functionp code) (funcall code) (eval code)))
+      (let (vc-sentinel-movepoint)
+        ;; Normally, we want code to not move point.
+        (save-excursion
+          (let ((m (when proc (process-mark proc))))
+            (goto-char (or m (point-max)))
+            (if (functionp code) (funcall code) (eval code))
+            (when m (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)))))))
      ;; If a process is running, add CODE to the sentinel
      ((eq (process-status proc) 'run)
       (vc-set-mode-line-busy-indicator)
@@ -337,7 +332,9 @@ case, and the process object in the asynchronous case."
 	    (when vc-command-messages
 	      (message "Running %s in foreground..." full-command))
 	    (let ((buffer-undo-list t))
-	      (setq status (apply 'process-file command nil t nil squeezed)))
+              ;; Use `save-excursion' like `vc-process-filter' does.
+	      (setq status (save-excursion
+                             (apply 'process-file command nil t nil squeezed))))
 	    (when (and (not (eq t okstatus))
 		       (or (not (integerp status))
 			   (and okstatus (< okstatus status))))
-- 
2.4.5

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Dmitry Gutov]

On 07/15/2015 02:09 PM, Wolfgang Jenkner wrote:

> So I wonder whether it wouldn't be worthwile to make their job easier
> (and thereby avoid bugs like the one described above) by handling async
> and sync processes alike in this respect.  The condition for this to
> work is that backend functions don't expect point to be preserved,
> except, of course, if it is actually their purpose to compute point
> (like for vc-git-show-log-entry).

I like the idea, but FWIW the patch breaks diff-hl-revert-hunk (diff-hl 
is in GNU ELPA). Haven't investigated it further yet.

And the particular bug you've described, naturally, can also be fixed by 
making vc-hg-print-log asynchronous:

diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index 556174a..f634e2e 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -272,7 +272,7 @@ If LIMIT is non-nil, show no more than this many 
entries."
    (let ((inhibit-read-only t))
      (with-current-buffer
  	buffer
-      (apply 'vc-hg-command buffer 0 files "log"
+      (apply 'vc-hg-command buffer 'async files "log"
  	     (nconc
  	      (when start-revision (list (format "-r%s:0" start-revision)))
  	      (when limit (list "-l" (format "%s" limit)))

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Wolfgang Jenkner]

On Sun, Jul 19 2015, Dmitry Gutov wrote:

> I like the idea, but FWIW the patch breaks diff-hl-revert-hunk
> (diff-hl is in GNU ELPA). Haven't investigated it further yet.

If I read diff-hl-revert-hunk correctly the code in vc-exec-after
expects point where (the synchronous call to) vc-diff-internal left it,
which should not be at eob, which is exactly where my patch would put
it :-(

It seems some code (not so far) out there uses synchronous calls for
a reason and also does much more weird things in vc-exec-after than I'd
have thought.

So, I'd rather just accept that the synchronous and asynchronous case
behave in different ways and scrap my proposed patch.

Thanks for pointing out this example.

> And the particular bug you've described, naturally, can also be fixed
> by making vc-hg-print-log asynchronous:

If all -print-log backend functions can be made asynchronous there's
nothing left to fix in the frontend I guess...

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Dmitry Gutov]

On 07/19/2015 04:36 AM, Wolfgang Jenkner wrote:

> If I read diff-hl-revert-hunk correctly the code in vc-exec-after
> expects point where (the synchronous call to) vc-diff-internal left it,
> which should not be at eob, which is exactly where my patch would put
> it :-(

It's not hard to fix, though: (goto-char (point-min)) at the beginning 
of vc-exec-after block there should do it. That would be 
backward-compatible, so I'm perfectly willing to make that change.

> It seems some code (not so far) out there uses synchronous calls for
> a reason and also does much more weird things in vc-exec-after than I'd
> have thought.

There are not too many third-party packages that integrate with VC, so 
one could go over them and check for similar assumptions. Your patch 
does make a certain amount of sense.

> If all -print-log backend functions can be made asynchronous there's
> nothing left to fix in the frontend I guess...

They can, but a similar problem could conceivably arise with something 
other than print-log.

On the other hand, the long-term target is to make all VC calls 
asynchronous. So maybe we can take the easy way out in fixing the 
present bug.

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Dmitry Gutov]

On 07/19/2015 04:36 AM, Wolfgang Jenkner wrote:

> If all -print-log backend functions can be made asynchronous there's
> nothing left to fix in the frontend I guess...

I've applied the patch to vc-hg-print-log, but a lot of older backends 
(mtn, sccs, and also src) still use the synchronous calls there.

Some even seem to do that on purpose (cvs and mcvs), choosing between 
async and sync at runtime. I don't know why, so I'm hesitant to change 
all of them.

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Dmitry Gutov]

On 07/19/2015 04:36 AM, Wolfgang Jenkner wrote:

> If all -print-log backend functions can be made asynchronous there's
> nothing left to fix in the frontend I guess...

Actually, it's still a problem if the repository history is small.

It seems that if the log can be delivered in one chunk, then point still 
ends at the end of the buffer (if someone can't reproduce it 
immediately, try a new repo with a few commits, and if that doesn't 
reproduce, press `g' a few times).

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Dmitry Gutov]

I was looking into applying your patch (except for the very last bit), 
because the idea of treating the asynchronous and synchronous delayed 
code basically seems right, but the current solution (using 
process-mark) will probably be inadequate in the synchronous case.

The semantics we seem to want to guarantee is that every delayed bit of 
code runs at the position the previous left off, but

(goto-char (or m (point-max)))

won't be sufficient, because that position is not necessarily point-max.

But we could try to simplify this semantics ("always runs at point-max" 
is actually easier to reason about than "runs where the previous code 
left off"). That kind of change seems too late for 25.1, though, and 
will require a review of the current vc-exec-after uses.

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Lars Ingebrigtsen]

Dmitry Gutov <dgutov@yandex.ru> writes:

> But we could try to simplify this semantics ("always runs at
> point-max" is actually easier to reason about than "runs where the
> previous code left off"). That kind of change seems too late for 25.1,
> though, and will require a review of the current vc-exec-after uses.

This was five years ago -- skimming this thread, I'm not quite sure what
the conclusion here was.  Is this something that should still be worked
on, or has this problem gone away in the meantime?

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

bug#21067: 25.0.50; [PATCH] With mercurial, vc-print-log puts point at eob

[Dmitry Gutov]

Version: 26.2

On 10.05.2021 14:28, Lars Ingebrigtsen wrote:
> This was five years ago -- skimming this thread, I'm not quite sure what
> the conclusion here was.  Is this something that should still be worked
> on, or has this problem gone away in the meantime?

Thanks for the reminder.

Looks like we've fixed this in Emacs 26.2. Commit fcd66d059, bug#31764.

Closing, reopen if you still can reproduce.