bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Start emacs -Q.  Visit a CVS-controlled file.  Then:

C-x v l
M->
C-x v l

Notice the CVS log added once more after current end of log buffer.

I tracked this down to the following snippet from `vc-do-command´:

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

That is, `vc-setup-buffer´ (which erases the buffer) is *not* called if 
the buffer where the VC command is to be executed in equals the current 
buffer.  I guess that logic has a reason, so it probably should be some 
upper layer which should erase *vc-log-buffer*...

For git, for example, function `vc-git-print-log´ calls 
`vc-setup-buffer´.  Probably the other VC backends should follow suit?

If this is the way to go, I could provide patches for VC backends CVS 
and RCS, pls let me know.



In GNU Emacs 30.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version
  3.24.24, cairo version 1.16.0) of 2023-06-07 built on sappc2
Repository revision: a902156068ab071f93cc9bbd34cd320919b74064
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
   value of $LC_COLLATE: POSIX
   value of $LC_TIME: POSIX
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: Git-Log-View

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   buffer-read-only: t
   line-number-mode: t
   indent-tabs-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils parse-time iso8601
time-date subr-x sh-script rx smie treesit cl-seq executable misearch
multi-isearch vc-hg vc-bzr vc-src vc-sccs vc-svn vc-cvs vc-rcs add-log
log-view pcvs-util cl-extra help-mode vc vc-git diff-mode easy-mmode
vc-dispatcher cl-loaddefs cl-lib rmc iso-transl tooltip cconv eldoc
paren electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
emacs)

Memory information:
((conses 16 61575 13745)
  (symbols 48 7721 0)
  (strings 32 23157 2232)
  (string-bytes 1 634764)
  (vectors 16 14390)
  (vector-slots 8 207631 12167)
  (floats 8 35 32)
  (intervals 56 586 0)
  (buffers 976 13))

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Manuel Giraud via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

[...]

> For git, for example, function `vc-git-print-log´ calls
> `vc-setup-buffer´.  Probably the other VC backends should follow suit?
>
> If this is the way to go, I could provide patches for VC backends CVS
> and RCS, pls let me know.

FWIW, I tested with mercurial (vc-hg.el) and it works correctly (as with
git) but it have the same call to 'vc-setup-buffer' in
'vc-hg-print-log'.  So I guess it would be logical that others backend
"print-log" functions have this same call.

(note: I could not test it but SVN and Bazaar print-log functions also
have this call to 'vc-setup-buffer')
-- 
Manuel Giraud

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-08  15:12, Manuel Giraud wrote:

> FWIW, I tested with mercurial (vc-hg.el) and it works correctly (as with
> git) but it have the same call to 'vc-setup-buffer' in
> 'vc-hg-print-log'.  So I guess it would be logical that others backend
> "print-log" functions have this same call.
> 
> (note: I could not test it but SVN and Bazaar print-log functions also
> have this call to 'vc-setup-buffer')

Thanks for checking this out.  Will provide patches for CVS and RCS in 
that direction.  Probably I'll even do SCCS, which seems to lack the 
call to `vc-setup-buffer' as well.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Root cause of this issue being that `vc-deduce-fileset' does not change
buffer in Emacs 29 and newer, while it does in Emacs 27 and older.  (Too
lazy to check for Emacs 28.)

So in Emacs 27 and older, a call to `vc-print-log' started from
*vc-change-log* would change buffer to the vc parent buffer before
calling the backend function, and thus `vc-do-command' would call
`vc-setup-buffer' in the snippet mentioned above.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Caused by commit d494833d47968fcd97ba549654a259d6fb6c2eee.

There is another subtle change to (most likely) that, which seems to 
affect all backends: When calling `vc-print-log' from 
*vc-change-buffer*, it now modifies the VC parent buffer to itself, 
which is of course incorrect.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Cc: 63949@debbugs.gnu.org
> Date: Fri, 9 Jun 2023 00:10:57 +0200
> From:  Jens Schmidt via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Caused by commit d494833d47968fcd97ba549654a259d6fb6c2eee.

Which means this was a regression in Emacs 28, and we should try to
fix it in Emacs 29.

> There is another subtle change to (most likely) that, which seems to 
> affect all backends: When calling `vc-print-log' from 
> *vc-change-buffer*, it now modifies the VC parent buffer to itself, 
> which is of course incorrect.

What do you mean by "modifies the VC parent buffer to itself"?  Do you
mean that vc-print-log makes the *vc-change-buffer* be the VC parent
buffer?  If so, why is this incorrect?

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-09  08:09, Eli Zaretskii wrote:

> What do you mean by "modifies the VC parent buffer to itself"?  Do 
> you mean that vc-print-log makes the *vc-change-buffer* be the VC 
> parent buffer?  If so, why is this incorrect?

Hope below recipe explains it.  The general idea is to start a VC
command first from the buffer of a VC-controlled file, then from the VC
result buffer, again from the VC result buffer, etc.

emacs -Q (from emacs-29)
<visit git-controlled file, say README>

;; current buffer equals README
C-x v l
;; current buffer equals *vc-change-log*
C-h v vc-parent-buffer
=> #<buffer README>

;; current buffer equals *vc-change-log*
C-x v l
C-h v vc-parent-buffer
=> #<buffer *vc-change-log*>

;; current buffer equals *vc-change-log*
C-x v l
;; git log of whole repository is displayed, not of README

I backed out commit d494833d47968fcd97ba549654a259d6fb6c2eee from
emacs-29 - both issues (the initial CVS-related one and the above one)
are gone after having done so.

Not all VC commands expose this problem.  `vc-annotate' seems to work
and even the VC parent buffer stays unchanged when invoking
`vc-annotate' repeatedly.  `vc-diff' gets the parent buffer wrong but
seems to work when invoked from buffer *vc-diff* repeatedly even if
parent buffer is wrong.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Fri, 9 Jun 2023 22:27:37 +0200
> Cc: manuel@ledu-giraud.fr, 63949@debbugs.gnu.org
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> emacs -Q (from emacs-29)
> <visit git-controlled file, say README>
> 
> ;; current buffer equals README
> C-x v l
> ;; current buffer equals *vc-change-log*
> C-h v vc-parent-buffer
> => #<buffer README>
> 
> ;; current buffer equals *vc-change-log*
> C-x v l
> C-h v vc-parent-buffer
> => #<buffer *vc-change-log*>
> 
> ;; current buffer equals *vc-change-log*
> C-x v l
> ;; git log of whole repository is displayed, not of README

Thanks for the explanations.

> Not all VC commands expose this problem.

Why do you think it's a problem?  I can justify this behavior, at
least in some use cases.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-10  08:01, Eli Zaretskii wrote:

> Why do you think it's a problem?  I can justify this behavior, at 
> least in some use cases.

Let's distinguish "VC-controlled buffers" like the buffer of a
VC-controlled file or a *vc-dir* buffer or a dired buffer of
VC-controlled directory.  And "VC working buffers", like
*vc-change-log*, *vc-log*, *vc-diff*.

It is my understanding that `vc-parent-buffer' in a VC working buffer
points to the VC-controlled buffer from which it originates.  The
rationale of that variable is to allow VC operations from a VC working
buffer as if executed on the original VC-controlled buffer.  So I can do
C-x v l, pick a commit, do a C-x v ~ on that commit, then a C-x v =, and
all these operations would automagically relate to the original
VC-controlled buffer.  At least I use that concept frequently.

The documentation on `vc-parent-buffer', unfortunately, is out of date 
and does not necessarily support my understanding:

   ;; In a log entry buffer, this is a local variable
   ;; that points to the buffer for which it was made
   ;; (either a file, or a directory buffer).

However, this has been working as described by me up to and including
Emacs 27.  So at least we can say that the fix fur bug#40967 has changed
established behavior.

And also the following code snippet from function `vc-deduce-fileset-1'
seems to prove my point:

   ((and (buffer-live-p vc-parent-buffer)
         ;; FIXME: Why this test?  --Stef
         (or (buffer-file-name vc-parent-buffer)
             (with-current-buffer vc-parent-buffer
               (or (derived-mode-p 'vc-dir-mode)
                   (derived-mode-p 'dired-mode)
                   (derived-mode-p 'diff-mode)))))
    (progn          ;FIXME: Why not `with-current-buffer'? --Stef.
      (set-buffer vc-parent-buffer)
      (vc-deduce-fileset-1 not-state-changing
                           allow-unregistered
                           state-model-only-files)))

Meaning: If the current buffer has a live vc-parent-buffer, this
function switches to it and deduces the fileset from that.  Plus it
leaves the VC parent buffer current, which is important for follow-up
code to keep the VC parent buffer unchanged.

That logic used to work as intended (by me) up to Nathan's commit, which
put a `with-current-buffer' around the whole function and rendered the
`set-buffer' side effect pointless.

Let's put it that way: The pre-28 logic of handling the VC parent buffer
was not necessarily clean, as also pointed out by Stefan.  But I think
the concept of having a stable VC parent buffer across multiple VC
operations is nice, and changing it midway according to rather unclear
rules undesirable.

Ideally, we would have a fix that handled indirect buffers and VC parent
buffers (which are somewhat similar by concept) all consistently and
nicely and in a stable way, at the same time fixing both issues that I 
have.  I'll mediate about that...

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I'll mediate about that...

*meditate*

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Sat, 10 Jun 2023 17:44:26 +0200
> Cc: manuel@ledu-giraud.fr, 63949@debbugs.gnu.org
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> On 2023-06-10  08:01, Eli Zaretskii wrote:
> 
> > Why do you think it's a problem?  I can justify this behavior, at 
> > least in some use cases.
> 
> Let's distinguish "VC-controlled buffers" like the buffer of a
> VC-controlled file or a *vc-dir* buffer or a dired buffer of
> VC-controlled directory.  And "VC working buffers", like
> *vc-change-log*, *vc-log*, *vc-diff*.

First, let me clarify what I alluded to as "a problem": I meant the
fact that "C-x v l" in a *vc-change-log* buffer shows the log for the
entire repository.  How this is accomplished by setting
vc-parent-buffer is an implementation detail.

> It is my understanding that `vc-parent-buffer' in a VC working buffer
> points to the VC-controlled buffer from which it originates.  The
> rationale of that variable is to allow VC operations from a VC working
> buffer as if executed on the original VC-controlled buffer.  So I can do
> C-x v l, pick a commit, do a C-x v ~ on that commit, then a C-x v =, and
> all these operations would automagically relate to the original
> VC-controlled buffer.  At least I use that concept frequently.

If the backend is a VCS that records changes per-file, what you want
will happen automatically, since "C-x v l" and other operations must
in general refer to a file with those VCSes.  For backends that record
changes per-repository, why does it make sense that typing "C-x v l"
from a buffer that already shows a log should produce the same log
again?

> The documentation on `vc-parent-buffer', unfortunately, is out of date 
> and does not necessarily support my understanding:
> 
>    ;; In a log entry buffer, this is a local variable
>    ;; that points to the buffer for which it was made
>    ;; (either a file, or a directory buffer).

We need to define what we want vc-parent-buffer to mean, and then we
should adjust the documentation and keep our promises about that
variable in the future.

> However, this has been working as described by me up to and including
> Emacs 27.  So at least we can say that the fix fur bug#40967 has changed
> established behavior.

Probably because no one realized that this must be the established
behavior.

> That logic used to work as intended (by me) up to Nathan's commit, which
> put a `with-current-buffer' around the whole function and rendered the
> `set-buffer' side effect pointless.
> 
> Let's put it that way: The pre-28 logic of handling the VC parent buffer
> was not necessarily clean, as also pointed out by Stefan.  But I think
> the concept of having a stable VC parent buffer across multiple VC
> operations is nice, and changing it midway according to rather unclear
> rules undesirable.
> 
> Ideally, we would have a fix that handled indirect buffers and VC parent
> buffers (which are somewhat similar by concept) all consistently and
> nicely and in a stable way, at the same time fixing both issues that I 
> have.  I'll mediate about that...

If you can suggest changes to make the treatment of vc-parent-buffer
more consistent, that would be good, I think.

I'd also like to hear Dmitry's views on these issues.  He was until
now silent in this discussion, AFAICT.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-10  19:23, Eli Zaretskii wrote:

> If the backend is a VCS that records changes per-file, what you want 
> will happen automatically, since "C-x v l" and other operations must 
> in general refer to a file with those VCSes.  For backends that
> record changes per-repository, why does it make sense that typing
> "C-x v l" from a buffer that already shows a log should produce the
> same log again?

To get it updated, for example because I perform some operation outside
of Emacs.  One of these habits ...

> If you can suggest changes to make the treatment of vc-parent-buffer 
> more consistent, that would be good, I think.

Here is another minor ugliness I forgot to mention so far which also
should be defined and fixed, then: Whenever the `vc-parent-buffer' goes
"out of sync" (as far as I am concerned) in some VC working buffer, its
mode line shows, e.g.

   *vc-change-log*   Top   1   (Git-Log-View from *vc-change-log*)

or

   *vc-diff [...] (Diff ws from *vc-diff*)

since mode line variable `vc-parent-buffer-name' goes likewise out of sync.

> I'd also like to hear Dmitry's views on these issues.  He was until 
> now silent in this discussion, AFAICT.

Agreed.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Sat, 10 Jun 2023 23:18:01 +0200
> Cc: manuel@ledu-giraud.fr, 63949@debbugs.gnu.org
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> On 2023-06-10  19:23, Eli Zaretskii wrote:
> 
> > If the backend is a VCS that records changes per-file, what you want 
> > will happen automatically, since "C-x v l" and other operations must 
> > in general refer to a file with those VCSes.  For backends that
> > record changes per-repository, why does it make sense that typing
> > "C-x v l" from a buffer that already shows a log should produce the
> > same log again?
> 
> To get it updated, for example because I perform some operation outside
> of Emacs.  One of these habits ...

If there's more than one natural meaning of a command, the usual way
we solve the dilemma is by using a prefix argument.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-10  19:23, Eli Zaretskii wrote:

> I'd also like to hear Dmitry's views on these issues.  He was until 
> now silent in this discussion, AFAICT.

@Dmitry: What's your view on these issues?  Just making the handling of
VC parent buffer more documented and "more consistent", whatever that
means, or do you prefer some particular direction?

Thanks

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

On 16/06/2023 22:33, Jens Schmidt wrote:
> On 2023-06-10  19:23, Eli Zaretskii wrote:
> 
>> I'd also like to hear Dmitry's views on these issues.  He was until 
>> now silent in this discussion, AFAICT.
> 
> @Dmitry: What's your view on these issues?  Just making the handling of
> VC parent buffer more documented and "more consistent", whatever that
> means, or do you prefer some particular direction?

Sorry, been busy. I'll make sure to read the thread and comment on the 
issue tomorrow.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

On 16/06/2023 22:33, Jens Schmidt wrote:
> On 2023-06-10  19:23, Eli Zaretskii wrote:
> 
>> I'd also like to hear Dmitry's views on these issues.  He was until 
>> now silent in this discussion, AFAICT.
> 
> @Dmitry: What's your view on these issues?  Just making the handling of
> VC parent buffer more documented and "more consistent", whatever that
> means, or do you prefer some particular direction?

I think the appropriate thing here is to back out of the change that 
caused the regression (d494833d47968fcd97ba549654a259d6fb6c2eee, as 
we've found out) and then try to re-fix it some other way (in master?).

Or maybe adjust the current code such as when vc-deduce-fileset-1 does 
change the current buffer, vc-deduce-fileset retains that change.

For example, using this ugly-ish (100% untested) patch:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 91d3f6f70d3..91aae40a677 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1121,10 +1121,13 @@ vc-deduce-fileset
  the returned list.

  BEWARE: this function may change the current buffer."
-  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
-    (vc-deduce-fileset-1 not-state-changing
-                         allow-unregistered
-                         state-model-only-files)))
+  (let (new-buf)
+    (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+      (vc-deduce-fileset-1 not-state-changing
+                           allow-unregistered
+                           state-model-only-files)
+      (setq new-buf (current-buffer)))
+    (set-buffer new-buf)))

  (defun vc-deduce-fileset-1 (not-state-changing
                              allow-unregistered

The fact that some backends do call vc-setup-buffer inside 
vc-xx-print-log and some dont', also bears investigation. But the 
question I would like to have answered is, can we drop this call from 
all of them? Rather than trying to add it everywhere.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Sun, 18 Jun 2023 05:42:44 +0300
> Cc: 63949@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 16/06/2023 22:33, Jens Schmidt wrote:
> > On 2023-06-10  19:23, Eli Zaretskii wrote:
> > 
> >> I'd also like to hear Dmitry's views on these issues.  He was until 
> >> now silent in this discussion, AFAICT.
> > 
> > @Dmitry: What's your view on these issues?  Just making the handling of
> > VC parent buffer more documented and "more consistent", whatever that
> > means, or do you prefer some particular direction?
> 
> I think the appropriate thing here is to back out of the change that 
> caused the regression (d494833d47968fcd97ba549654a259d6fb6c2eee, as 
> we've found out) and then try to re-fix it some other way (in master?).
> 
> Or maybe adjust the current code such as when vc-deduce-fileset-1 does 
> change the current buffer, vc-deduce-fileset retains that change.
> 
> For example, using this ugly-ish (100% untested) patch:

Whatever you decide, please do it soon, as I'd like to release another
pretest of Emacs 29.

Thanks.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-18  07:35, Eli Zaretskii wrote:

> Whatever you decide, please do it soon, as I'd like to release 
> another pretest of Emacs 29.

No need to change anything for Emacs 29, I think.  The issues that I
encountered are not that serious, and whatever gets implemented to
rectify them and commit d494833d47968fcd97ba549654a259d6fb6c2eee in a
consistent way would be too much for Emacs 29, anyway.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

On 18/06/2023 12:11, Jens Schmidt wrote:
> On 2023-06-18  07:35, Eli Zaretskii wrote:
> 
>> Whatever you decide, please do it soon, as I'd like to release another 
>> pretest of Emacs 29.
> 
> No need to change anything for Emacs 29, I think.  The issues that I
> encountered are not that serious, and whatever gets implemented to
> rectify them and commit d494833d47968fcd97ba549654a259d6fb6c2eee in a
> consistent way would be too much for Emacs 29, anyway.

Could you test the patch I showed?

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

(Please keep the bug address in Cc).

On 18/06/2023 16:21, Jens Schmidt wrote:
> On 2023-06-18  14:00, Dmitry Gutov wrote:
> 
>> Could you test the patch I showed?
> 
> Will do, pls just give me some time here...

Thanks. If we can verify that the part broken by the bisected revision, 
is fixed here, we could push that to Emacs 29 and then clean up for 
Emacs 30 more thoroughly.

In the meantime, here's the updated patch. The previous one discarded 
the return value :(

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 91d3f6f70d3..c8b2b3ac11d 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1121,10 +1121,15 @@ vc-deduce-fileset
  the returned list.

  BEWARE: this function may change the current buffer."
-  (with-current-buffer (or (buffer-base-buffer) (current-buffer))
-    (vc-deduce-fileset-1 not-state-changing
-                         allow-unregistered
-                         state-model-only-files)))
+  (let (new-buf res)
+    (with-current-buffer (or (buffer-base-buffer) (current-buffer))
+      (setq res
+            (vc-deduce-fileset-1 not-state-changing
+                                 allow-unregistered
+                                 state-model-only-files))
+      (setq new-buf (current-buffer)))
+    (set-buffer new-buf)
+    res))

  (defun vc-deduce-fileset-1 (not-state-changing
                              allow-unregistered

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-20  04:41, Dmitry Gutov wrote:

> (Please keep the bug address in Cc).

Will do.

> Thanks. If we can verify that the part broken by the bisected 
> revision, is fixed here, we could push that to Emacs 29 and then 
> clean up for Emacs 30 more thoroughly.

Both of my issues (described on "Wed, 7 Jun 2023 23:04:50 +0200" and on
"Fri, 9 Jun 2023 22:27:37 +0200") are fixed by your latest, second patch.

Tested on emacs-29.

Thanks!

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

On 21/06/2023 16:03, Jens Schmidt wrote:
> On 2023-06-20  04:41, Dmitry Gutov wrote:
> 
>> (Please keep the bug address in Cc).
> 
> Will do.
> 
>> Thanks. If we can verify that the part broken by the bisected 
>> revision, is fixed here, we could push that to Emacs 29 and then clean 
>> up for Emacs 30 more thoroughly.
> 
> Both of my issues (described on "Wed, 7 Jun 2023 23:04:50 +0200" and on
> "Fri, 9 Jun 2023 22:27:37 +0200") are fixed by your latest, second patch.
> 
> Tested on emacs-29.

Excellent, thank you.

Eli, I suggest we push it to emacs-29 now.

There could be ways to make it prettier, but probably not by much -- at 
least I don't see any obvious ones if we're keeping the indirect 
buffer-related functionality. Anyway, we can look into that later in 
Emacs 30.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Wed, 21 Jun 2023 17:37:55 +0300
> Cc: 63949@debbugs.gnu.org, Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 21/06/2023 16:03, Jens Schmidt wrote:
> > Tested on emacs-29.
> 
> Excellent, thank you.
> 
> Eli, I suggest we push it to emacs-29 now.

Fine by me, thanks.

> There could be ways to make it prettier, but probably not by much -- at 
> least I don't see any obvious ones if we're keeping the indirect 
> buffer-related functionality. Anyway, we can look into that later in 
> Emacs 30.

Right.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

On 21/06/2023 18:30, Eli Zaretskii wrote:
>> Date: Wed, 21 Jun 2023 17:37:55 +0300
>> Cc:63949@debbugs.gnu.org, Jens Schmidt<jschmidt4gnu@vodafonemail.de>
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> On 21/06/2023 16:03, Jens Schmidt wrote:
>>> Tested on emacs-29.
>> Excellent, thank you.
>>
>> Eli, I suggest we push it to emacs-29 now.
> Fine by me, thanks.

And done.

>> There could be ways to make it prettier, but probably not by much -- at
>> least I don't see any obvious ones if we're keeping the indirect
>> buffer-related functionality. Anyway, we can look into that later in
>> Emacs 30.
> Right.

I'm attaching a patch which should work fine, though could be a tad 
risky for 29.

Jens (and possibly others), could you try it out?

[-- Attachment #2: vc-deduce-fileset.diff --]
[-- Type: text/x-patch, Size: 1369 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 410fe5c01e1..24e786eb218 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1121,19 +1121,8 @@ vc-deduce-fileset
 the returned list.
 
 BEWARE: this function may change the current buffer."
-  (let (new-buf res)
-    (with-current-buffer (or (buffer-base-buffer) (current-buffer))
-      (setq res
-            (vc-deduce-fileset-1 not-state-changing
-                                 allow-unregistered
-                                 state-model-only-files))
-      (setq new-buf (current-buffer)))
-    (set-buffer new-buf)
-    res))
-
-(defun vc-deduce-fileset-1 (not-state-changing
-                            allow-unregistered
-                            state-model-only-files)
+  (when (buffer-base-buffer)
+    (set-buffer (buffer-base-buffer)))
   (let (backend)
     (cond
      ((derived-mode-p 'vc-dir-mode)
@@ -1158,7 +1147,7 @@ vc-deduce-fileset-1
 				      (derived-mode-p 'diff-mode)))))
       (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
 	(set-buffer vc-parent-buffer)
-	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
+	(vc-deduce-fileset not-state-changing allow-unregistered state-model-only-files)))
      ((and (not buffer-file-name)
 	   (setq backend (vc-responsible-backend default-directory)))
       (list backend nil))

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-24  05:08, Dmitry Gutov wrote:

> Jens (and possibly others), could you try it out?

Thanks, will do, might take some time again ...

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-24  05:08, Dmitry Gutov wrote:

> And done.

Thanks.

> I'm attaching a patch which should work fine, though could be a tad 
> risky for 29.

I like the patch - I think it is more in line with what the function
does/seems to do originally.

> Jens (and possibly others), could you try it out?

And it works, too.  It covers my test cases.  I also tried to run some
tests with indirect buffers, and they came out as I would have expected
them to come out.

It would have been nice to have the opinion of the original author of
commit d494833d47968fcd97ba549654a259d6fb6c2eee on this, but he doesn't
seem to be active any longer.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Thu, 29 Jun 2023 23:37:58 +0200
> Cc: 63949@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> 
> It would have been nice to have the opinion of the original author of
> commit d494833d47968fcd97ba549654a259d6fb6c2eee on this, but he doesn't
> seem to be active any longer.

We never CC'ed him.  Let's try harder: Nathan, any comments on this
discussion and the proposed changes?

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2023-06-30  07:57, Eli Zaretskii wrote:

> We never CC'ed him.

Actually, I contacted him off-list some weeks already before your
mail, without getting any reply, either.

To summarize:

- Nathan's changes related to indirect buffers regressed some less-used
   scenarios in vc.

- Dmitry provided a first fix for that which landed in emacs-29 and
   master already.

- Plus a second, IMO more elegant and in Dmitry's opinion slightly more
   risky patch that also fixes the regression while keeping (AFAICT)
   Nathan's intended behavior:
   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63949#86

Probably we could have that second patch at least in master?

Thanks

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Dmitry Gutov]

On 17/07/2023 22:53, Jens Schmidt wrote:
> Probably we could have that second patch at least in master?

Yes, of course.

Now pushed to master. Thanks for the report, testing and the reminders. :-)

Closing!

Now if emacs-29 pretest ends up going on for several months more, we 
could consider backporting the second patch. Or maybe do that after, for 
29.2...

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Tue, 18 Jul 2023 03:55:59 +0300
> Cc: 63949-done@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> Now if emacs-29 pretest ends up going on for several months more

I could do without salt on my wounds, thank you.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

Here come five patches for this issue, based on emacs-29:

0001-Always-erase-log-buffer-before-calling-vc-print-log.patch
0002-Always-erase-log-buffer-before-calling-vc-print-log.patch
0003-Always-erase-log-buffer-before-calling-vc-print-log.patch

   Name should be self-explaining, patches uncritical.

0004-Fix-documentation-bug-and-remove-obsolete-fixmes.patch

   `vc-deduce-fileset' uses `with-current-buffer' to protect the current
   buffer, which has not been reflected in comments.

0005-Avoid-setting-circular-vc-parent-buffer.patch

   This one fixes the issue related to VC parent buffer described in the
   previous update.  The "local" change seems to be logical (a buffer
   should not be the VC parent buffer of itself), but I'm not quite sure
   about any adverse "global" consequences.

[-- Attachment #2: 0001-Always-erase-log-buffer-before-calling-vc-print-log.patch --]
[-- Type: text/x-patch, Size: 1094 bytes --]

From 9e3cb8038ea5bea3e2f129f0271a0cf3ec948159 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 8 Jun 2023 23:36:46 +0200
Subject: [PATCH 1/5] Always erase log buffer before calling `vc-*-print-log'

When calling `vc-print-log' from buffer *vc-change-log* itself for
backends SCCS, RCS, and CVS, the log information is inserted or
appended to the *vc-change-log* buffer again without previously
erasing it.
* lisp/vc/vc-sccs.el (vc-sccs-print-log): Add call to
`vc-setup-buffer'. (Bug#63949)
---
 lisp/vc/vc-sccs.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/vc/vc-sccs.el b/lisp/vc/vc-sccs.el
index 03e9d12b76f..e5430753a64 100644
--- a/lisp/vc/vc-sccs.el
+++ b/lisp/vc/vc-sccs.el
@@ -310,6 +310,8 @@ vc-sccs-print-log
   "Print commit log associated with FILES into specified BUFFER.
 Remaining arguments are ignored."
   (setq files (vc-expand-dirs files 'SCCS))
+  (save-current-buffer
+    (vc-setup-buffer buffer))
   (vc-sccs-do-command buffer 0 "prs" (mapcar #'vc-master-name files))
   (when limit 'limit-unsupported))
 
-- 
2.30.2


[-- Attachment #3: 0002-Always-erase-log-buffer-before-calling-vc-print-log.patch --]
[-- Type: text/x-patch, Size: 1111 bytes --]

From 4cb5eaa2166103d645fa77972b456dd6a4fad282 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 8 Jun 2023 23:37:45 +0200
Subject: [PATCH 2/5] Always erase log buffer before calling `vc-*-print-log'

When calling `vc-print-log' from buffer *vc-change-log* itself for
backends SCCS, RCS, and CVS, the log information is inserted or
appended to the *vc-change-log* buffer again without previously
erasing it.
* lisp/vc/vc-cvs.el (vc-cvs-print-log): Add call to
`vc-setup-buffer'. (Bug#63949)
---
 lisp/vc/vc-cvs.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/vc/vc-cvs.el b/lisp/vc/vc-cvs.el
index 6e0246ea762..cf74ac31154 100644
--- a/lisp/vc/vc-cvs.el
+++ b/lisp/vc/vc-cvs.el
@@ -550,6 +550,8 @@ vc-cvs-modify-change-comment
 (defun vc-cvs-print-log (files buffer &optional _shortlog _start-revision limit)
   "Print commit log associated with FILES into specified BUFFER.
 Remaining arguments are ignored."
+  (save-current-buffer
+    (vc-setup-buffer buffer))
   ;; It's just the catenation of the individual logs.
   (vc-cvs-command
    buffer
-- 
2.30.2


[-- Attachment #4: 0003-Always-erase-log-buffer-before-calling-vc-print-log.patch --]
[-- Type: text/x-patch, Size: 1114 bytes --]

From 4eb1a951229d96026ca58f965c652ab8e8392812 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 8 Jun 2023 23:38:45 +0200
Subject: [PATCH 3/5] Always erase log buffer before calling `vc-*-print-log'

When calling `vc-print-log' from buffer *vc-change-log* itself for
backends SCCS, RCS, and CVS, the log information is inserted or
appended to the *vc-change-log* buffer again without previously
erasing it.
* lisp/vc/vc-rcs.el (vc-rcs-print-log): Add call to
`vc-setup-buffer'. (Bug#63949)
---
 lisp/vc/vc-rcs.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/vc/vc-rcs.el b/lisp/vc/vc-rcs.el
index c2112b76ad3..05846a7de35 100644
--- a/lisp/vc/vc-rcs.el
+++ b/lisp/vc/vc-rcs.el
@@ -548,6 +548,8 @@ vc-rcs-print-log
 Remaining arguments are ignored.
 If FILE is a directory the operation is applied to all registered
 files beneath it."
+  (save-current-buffer
+    (vc-setup-buffer buffer))
   (vc-do-command (or buffer "*vc*") 0 "rlog"
                  (mapcar #'vc-master-name (vc-expand-dirs files 'RCS)))
   (with-current-buffer (or buffer "*vc*")
-- 
2.30.2


[-- Attachment #5: 0004-Fix-documentation-bug-and-remove-obsolete-fixmes.patch --]
[-- Type: text/x-patch, Size: 1513 bytes --]

From 8f62cd7bbe336ba0668afb72cbbbd8c46c15376a Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Thu, 8 Jun 2023 23:46:33 +0200
Subject: [PATCH 4/5] Fix documentation bug and remove obsolete fixmes

Function `vc-deduce-fileset' preserves current buffer since commit
d494833d47968fcd97ba549654a259d6fb6c2eee.  Change documentation
accordingly.
* lisp/vc/vc.el (vc-deduce-fileset): Change doc string.
(vc-deduce-fileset-1): Remove now-obsolete fixme comment.  (Bug#63949)
---
 lisp/vc/vc.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 1144a23f317..6b99eba9d3b 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1120,7 +1120,7 @@ vc-deduce-fileset
 `vc-checkout-model'.  Otherwise, these 3 members may be omitted from
 the returned list.
 
-BEWARE: this function may change the current buffer."
+This function preserves the current buffer."
   (with-current-buffer (or (buffer-base-buffer) (current-buffer))
     (vc-deduce-fileset-1 not-state-changing
                          allow-unregistered
@@ -1151,7 +1151,7 @@ vc-deduce-fileset-1
 				  (or (derived-mode-p 'vc-dir-mode)
 				      (derived-mode-p 'dired-mode)
 				      (derived-mode-p 'diff-mode)))))
-      (progn                  ;FIXME: Why not `with-current-buffer'? --Stef.
+      (progn
 	(set-buffer vc-parent-buffer)
 	(vc-deduce-fileset-1 not-state-changing allow-unregistered state-model-only-files)))
      ((and (not buffer-file-name)
-- 
2.30.2


[-- Attachment #6: 0005-Avoid-setting-circular-vc-parent-buffer.patch --]
[-- Type: text/x-patch, Size: 1476 bytes --]

From 9f3fc16ac6eb2fbf6c73aef5e47569dba2cb5d80 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Fri, 9 Jun 2023 00:24:28 +0200
Subject: [PATCH 5/5] Avoid setting circular `vc-parent-buffer'

Otherwise, calling e.g. `vc-print-log' from buffer *vc-change-log*
sets `vc-parent-buffer' to itself.
* lisp/vc/vc-dispatcher.el (vc-setup-buffer): Avoid setting circular
`vc-parent-buffer'.  (Bug#63949)
---
 lisp/vc/vc-dispatcher.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index fd5f655a0f6..ac4d3a78afd 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -186,10 +186,13 @@ vc-setup-buffer
       ;; want any of its output to appear from now on.
       (when oldproc (delete-process oldproc)))
     (kill-all-local-variables)
-    (setq-local vc-parent-buffer camefrom)
-    (setq-local vc-parent-buffer-name
-                (concat " from " (buffer-name camefrom)))
-    (setq default-directory olddir)
+    ;; Do not set the VC parent buffer and related variables to
+    ;; ourselves.  (Bug#63949)
+    (unless (equal (current-buffer) camefrom)
+      (setq-local vc-parent-buffer camefrom)
+      (setq-local vc-parent-buffer-name
+                  (concat " from " (buffer-name camefrom)))
+      (setq default-directory olddir))
     (let ((buffer-undo-list t)
           (inhibit-read-only t))
       (erase-buffer))))
-- 
2.30.2

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Eli Zaretskii]

> Date: Fri, 9 Jun 2023 00:44:39 +0200
> From:  Jens Schmidt via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Here come five patches for this issue, based on emacs-29:
> 
> 0001-Always-erase-log-buffer-before-calling-vc-print-log.patch
> 0002-Always-erase-log-buffer-before-calling-vc-print-log.patch
> 0003-Always-erase-log-buffer-before-calling-vc-print-log.patch

Please add comments there explaining the significance of the call to
vc-setup-buffer and its effect, depending on what is the current
buffer.

> 0005-Avoid-setting-circular-vc-parent-buffer.patch
> 
>    This one fixes the issue related to VC parent buffer described in the
>    previous update.  The "local" change seems to be logical (a buffer
>    should not be the VC parent buffer of itself), but I'm not quite sure
>    about any adverse "global" consequences.

This code is very old, so at the very least we need to track its
origin and understand why it was added, before discussing whether it
should be removed.  AFAICT, this code was introduced in commit
1a2f456b73bab4a711a51c8f84abf1d9f63a3b90, 30 years ago, and there's
some explanation of the rationale in the log message.

Thanks.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> This code is very old, so at the very least we need to track its 
> origin and understand why it was added, before discussing whether it 
> should be removed. [...]

Totally agreed.  Pls ignore my patches for the time being, I just hacked
them together before really thinking and getting to the root cause of my
issues.  I'm trying to get into contact with Nathan, the author of the
enhancement that caused the regression, to see whether he can provide
more information than what is contained in his bug#40967.

bug#63949: 30.0.50; `vc-print-log´ does not erase buffer when called from *vc-change-log* buffer, at least for CVS logs

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Not sure about my previous patches 0004 and 0005.  IMHO that commit 
d494833d47968fcd97ba549654a259d6fb6c2eee which introduced all these 
issues looks a bit flaky.  (It does not even have a NEWS entry!)

I think I understand what it's doing and the intention seems noble, even 
if it covers only a subset of VC operations.  But that 
`with-current-buffer' around `vc-deduce-fileset', where that function 
explicitly declares that it changes buffer, seems to ask for more trouble.

Probably somebody with deeper VC insight should review both commit 
d494833d47968fcd97ba549654a259d6fb6c2eee and my patches.