GNU bug report logs - #72341
VC: CVS template lines not stripped when committing

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 72341 in the body.
You can then email your comments to 72341 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: Christoph Badura <bad <at> bsd.de>
To: bug-gnu-emacs <at> gnu.org
Subject: VC: CVS template lines not stripped when committing
Date: Sun, 28 Jul 2024 14:32:57 +0200

CVS strips all lines beginning with "CVS: " after editing the commit
message.  This is not done when using VC.

Specifically log-edit-hook's default includes log-edit-insert-cvs-template
causing the CVS/Template file to be inserted.  However, the lines starting
with "CVS: " aren't stripped out when log-edit-done is called.  The change
is then committed with "cvs ci -m" which does not strip these line, as the
template file is only loaded when cvs invokes an editor to edit the commit
message.

This behaviour is very annoying when working in a project that makes use of
CVS templates (e.g. NetBSD).  It would be nice, if VC behaved by default
like CVS does.

I guess on could work around this be defining a log-editdone-hook like
this:

(defun my/log-edit-done-strip-cvs-lines ()
  "Strip lines beginning with \"CVS: \" from commit log message."
  (let ((search-upper-case nil))
    (goto-char (point-min))
    (flush-lines "^CVS: ")))

Not sure that's right, though.

I've looked at the latest version of log-mode.el in emacs git and the
behaviour doesn't seem to have changed.

In GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38, cairo
version 1.16.0)
 of 2024-06-25, modified by Debian built on x86-conova-01 Windowing system
distributor 'The X.Org Foundation', version 11.0.12101006 System
Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/libexec localstatedir=/var/lib
 ----infodir=/usr/share/info mandir=/usr/share/man --with-libsystemd
 ----with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/28.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/28.2/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils with-native-compilation
 ----build x86_64-linux-gnu --prefix=/usr sharedstatedir=/var/lib
 ----libexecdir=/usr/libexec localstatedir=/var/lib
 ----infodir=/usr/share/info mandir=/usr/share/man --with-libsystemd
 ----with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/28.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/28.2/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils with-native-compilation
 ----with-cairo --with-x=yes with-x-toolkit=gtk3 --with-toolkit-scroll-bars
 --'CFLAGS=-g -O2
 -ffile-prefix-map=/build/reproducible-path/emacs-28.2+1=. -fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

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

Important settings:
  value of $LC_CTYPE: C.utf8 value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: ELisp/d

Minor modes in effect:
  paredit-mode: t global-company-mode: t company-mode: t persp-mode: t
  editorconfig-mode: t direnv-mode: t savehist-mode: t save-place-mode: t
  ido-everywhere: t override-global-mode: t tooltip-mode: t
  global-eldoc-mode: t eldoc-mode: t show-paren-mode: t
  electric-indent-mode: t mouse-wheel-mode: t file-name-shadow-mode: t
  global-font-lock-mode: t font-lock-mode: t blink-cursor-mode: t
  auto-composition-mode: t auto-encryption-mode: t auto-compression-mode: t
  line-number-mode: t indent-tabs-mode: t transient-mark-mode: t

Load-path shadows: /usr/share/emacs/site-lisp/elpa/htmlize-1.56/htmlize-pkg
hides /usr/share/emacs/site-lisp/elpa-src/htmlize-1.56/htmlize-pkg
/usr/share/emacs/site-lisp/elpa/htmlize-1.56/htmlize-autoloads hides
/usr/share/emacs/site-lisp/elpa-src/htmlize-1.56/htmlize-autoloads
/usr/share/emacs/site-lisp/elpa/htmlize-1.56/htmlize hides
/usr/share/emacs/site-lisp/elpa-src/htmlize-1.56/htmlize
/home/bad/.emacs.d/elpa/transient-20240311.1638/transient hides
/usr/share/emacs/28.2/lisp/transient /home/bad/.emacs.d/elpa/seq-2.24/seq
hides /usr/share/emacs/28.2/lisp/emacs-lisp/seq

Features: (shadow sort mail-extr emacsbug message rmc puny rfc822 mml
mml-sec epa derived epg rfc6068 epg-config gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
macrostep-c cmacexp macrostep NetBSD cc-mode cc-fonts cc-guess cc-menus
cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs dired-aux dired
dired-loaddefs vc-git paredit cus-start company-oddmuse company-keywords
company-etags etags fileloop generator xref company-gtags
company-dabbrev-code company-dabbrev company-files company-clang
company-capf company-cmake company-semantic company-template company-bbdb
company server dim init-tex init-markdown persp-mode finder-inf init-python
init-scheme init-erc erc-goodies erc erc-backend iso8601 time-date thingatpt
erc-loaddefs init-jabber init-circe init-yaml init-jsonnet init-json
init-golang reformatter project comp comp-cstr warnings editorconfig
editorconfig-core editorconfig-core-handle editorconfig-fnmatch rg files-x
vc vc-dispatcher rg-info-hack rg-menu transient seq seq-25 loadhist
format-spec compat compat-29 rg-ibuffer rg-result wgrep-rg wgrep rg-history
rg-header ibuf-ext ibuffer ibuffer-loaddefs grep compile
text-property-search comint ansi-color ring cus-edit pp wid-edit
init-flyspell advice direnv diff-mode dash better-defaults savehist
saveplace ido init-company diminish cl-extra help-mode use-package
use-package-ensure use-package-delight use-package-diminish
use-package-bind-key bind-key easy-mmode use-package-core desktop frameset
cus-load tex-site rx edmacro kmacro pcase slime-autoloads info package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie url-domsuf url-util mailcap url-handlers url-parse auth-source
cl-seq eieio eieio-core cl-macs eieio-loaddefs password-cache json subr-x
map url-vars byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib
iso-transl tooltip 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 cl-generic 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 simple abbrev obarray cl-preloaded
nadvice button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo move-toolbar
gtk x-toolkit x multi-tty make-network-process native-compile emacs)

Memory information: ((conses 16 555286 88165)
 (symbols 48 28058 4) (strings 32 169614 14216) (string-bytes 1 4737597)
 (vectors 16 48599) (vector-slots 8 844310 114128) (floats 8 120 281)
 (intervals 56 726 187) (buffers 992 16))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sun, 28 Jul 2024 20:49:15 +0300

> Date: Sun, 28 Jul 2024 14:32:57 +0200
> From: Christoph Badura <bad <at> bsd.de>
> 
> CVS strips all lines beginning with "CVS: " after editing the commit
> message.  This is not done when using VC.
> 
> Specifically log-edit-hook's default includes log-edit-insert-cvs-template
> causing the CVS/Template file to be inserted.  However, the lines starting
> with "CVS: " aren't stripped out when log-edit-done is called.  The change
> is then committed with "cvs ci -m" which does not strip these line, as the
> template file is only loaded when cvs invokes an editor to edit the commit
> message.
> 
> This behaviour is very annoying when working in a project that makes use of
> CVS templates (e.g. NetBSD).  It would be nice, if VC behaved by default
> like CVS does.

I'm confused by your description.  At the beginning you say:

  CVS strips all lines beginning with "CVS: " after editing the commit
  message.

But later you say:

  However, the lines starting with "CVS: " aren't stripped out when
  log-edit-done is called.  The change is then committed with
  "cvs ci -m" which does not strip these line, as the template file is
  only loaded when cvs invokes an editor to edit the commit message.

If "cvs ci -m" doesn't strip the "CVS: " lines, then when and how does
the stripping you describe at the beginning happens?  And what does
the last part of the last sentence above, about the template file
being loaded when CVS invokes an editor, has to do with this issue?

Thanks.

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sun, 28 Jul 2024 21:48:30 +0200

Resending the message, because debbugs.gnu.org wasn't cc'ed on the
original reply.

On Sun, Jul 28, 2024 at 08:49:15PM +0300, Eli Zaretskii wrote:
> I'm confused by your description.  At the beginning you say:
> 
>   CVS strips all lines beginning with "CVS: " after editing the commit
>   message.
> 
> But later you say:
> 
>   However, the lines starting with "CVS: " aren't stripped out when
>   log-edit-done is called.  The change is then committed with
>   "cvs ci -m" which does not strip these line, as the template file is
>   only loaded when cvs invokes an editor to edit the commit message.

CVS only adds the template file and later strips the "CVS: " lines if
you actually edit the commit message in an editor (which is invoked by
cvs).  I.e. if you do not pass a commit message via "cvs ci -m'message'"
or "cvs ci -F messagefile".

> If "cvs ci -m" doesn't strip the "CVS: " lines, then when and how does
> the stripping you describe at the beginning happens?  And what does
> the last part of the last sentence above, about the template file
> being loaded when CVS invokes an editor, has to do with this issue?

When invoked as "cvs ci -m" (or "cvs ci -F") cvs uses the commit message
*as is*.  With either option, cvs itself doesn't add the template file to
the commit message and hence doesn't have to do any stripping.

log-edit-insert-cvs-template adds the CVS template file to the commit
message outside of cvs.  Therefore log-edit has to strip the "CVS: " lines
outside of cvs too.

Is that clearer?

--chris

--chris

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 29 Jul 2024 05:26:50 +0300

> Date: Sun, 28 Jul 2024 21:21:15 +0200
> From: Christoph Badura <bad <at> bsd.de>
> 
> CVS only adds the template file and later strips the "CVS: " lines if
> you actually edit the commit message in an editor (which is invoked by
> cvs).  I.e. if you do not pass a commit message via "cvs ci -m'message'"
> or "cvs ci -F messagefile".
> 
> > If "cvs ci -m" doesn't strip the "CVS: " lines, then when and how does
> > the stripping you describe at the beginning happens?  And what does
> > the last part of the last sentence above, about the template file
> > being loaded when CVS invokes an editor, has to do with this issue?
> 
> When invoked as "cvs ci -m" (or "cvs ci -F") cvs uses the commit message
> *as is*.  With either option, cvs itself doesn't add the template file to
> the commit message and hence doesn't have to do any stripping.
> 
> log-edit-insert-cvs-template adds the CVS template file to the commit
> message outside of cvs.  Therefore log-edit has to strip the "CVS: " lines
> outside of cvs too.
> 
> Is that clearer?

Somewhat clearer, thanks.  I don't see this stripping feature
documented in the CVS manual; did I miss something?

And one more questions: where do those "CVS:" lines come from when you
use the template file?

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

From: Christoph Badura <bad <at> bsd.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 29 Jul 2024 11:53:13 +0200

On Mon, Jul 29, 2024 at 05:26:50AM +0300, Eli Zaretskii wrote:
> Somewhat clearer, thanks.  I don't see this stripping feature
> documented in the CVS manual; did I miss something?

I didn't see it documented either.  The closest thing that comes to
documentation of that feature is the documentation for rcsinfo:
https://www.gnu.org/software/trans-coord/manual/cvs/html_node/rcsinfo.html#rcsinfo

It is implemented in src/logmsg.c:do_editor().

> And one more questions: where do those "CVS:" lines come from when you
> use the template file?

They have to be be in the template file.

I figure the idea is that the template files can contain "mandatory" text
that will be part of the actuall log message and "explanatory" text,
prefixed with "CVS: ", that will be stripped after the commit message has
been edited.

As an example, here's the default template for the NetBSD repositories:
----------------8<------------------8<------------------8<-----------------
CVS: ----------------------------------------------------------------------
CVS: CVSROOT  cvs.NetBSD.org:/cvsroot
CVS: please use "PR category/123" to have the commitmsg appended to PR 123
----------------8<------------------8<------------------8<-----------------

I'm only aware of the NetBSD and pkgsrc repositories that make use of CVS
templates.  And their template files contain only lines prefixed with
"CVS: ".

Off topic and speaking of documentation.
I noticed that
https://www.gnu.org/software/emacs/manual/html_node/emacs/Log-Buffer.html
doesn't document C-c C-k and doesn't explain how the region is set up so
that an initial C-w will kill from point to the end of the buffer.  log-edit's
documentation is also not correct with regard to this (i.e. the entire log
buffer isn't emptied as point is after the Summary: header).

Is that worth reporting separately?

--chris

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>, Dmitry Gutov <dmitry <at> gutov.dev>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 29 Jul 2024 15:34:32 +0300

> Date: Mon, 29 Jul 2024 11:53:13 +0200
> From: Christoph Badura <bad <at> bsd.de>
> Cc: 72341 <at> debbugs.gnu.org
> 
> On Mon, Jul 29, 2024 at 05:26:50AM +0300, Eli Zaretskii wrote:
> > Somewhat clearer, thanks.  I don't see this stripping feature
> > documented in the CVS manual; did I miss something?
> 
> I didn't see it documented either.  The closest thing that comes to
> documentation of that feature is the documentation for rcsinfo:
> https://www.gnu.org/software/trans-coord/manual/cvs/html_node/rcsinfo.html#rcsinfo

This says nothing about the "CVS: " prefix, AFAICT.

> It is implemented in src/logmsg.c:do_editor().
> 
> > And one more questions: where do those "CVS:" lines come from when you
> > use the template file?
> 
> They have to be be in the template file.
> 
> I figure the idea is that the template files can contain "mandatory" text
> that will be part of the actuall log message and "explanatory" text,
> prefixed with "CVS: ", that will be stripped after the commit message has
> been edited.
> 
> As an example, here's the default template for the NetBSD repositories:
> ----------------8<------------------8<------------------8<-----------------
> CVS: ----------------------------------------------------------------------
> CVS: CVSROOT  cvs.NetBSD.org:/cvsroot
> CVS: please use "PR category/123" to have the commitmsg appended to PR 123
> ----------------8<------------------8<------------------8<-----------------
> 
> I'm only aware of the NetBSD and pkgsrc repositories that make use of CVS
> templates.  And their template files contain only lines prefixed with
> "CVS: ".

I think I see what's happening.  This is basically an undocumented
feature.  The removal of "CVS: " lines is there for when CVS itself
invokes the editor: in that case, it injects instructions into the
temporary file that it submits to the editor, and those instructions
all start with "CVS: ", so that they could be later removed.  IOW,
this prefix is basically an agreement between CVS and itself.

Now, nothing in the documentation of CVS/Template file says anything
about "CVS: ", but the code processes the temporary file, which
includes the template, the same as it processes the lines injected by
CVS, and thus every line that begins with "CVS: " in the template will
be removed.

IOW, NetBSD piggy-backs this feature to include their own instructions
in the template.

I'm unsure how to proceed with this.  My bother is that this is a
definite change in behavior wrt what VC did until now.  Users of VC
might be unaware of this removal, and could start some log lines with
the prefix, which will mysteriously disappear from the log message.

Dmitry, WDYT?  Maybe we should add this removal guarded by a user
option, by default off?  Or maybe we can identify where the template
starts and ends, and only remove in that region?

> Off topic and speaking of documentation.
> I noticed that
> https://www.gnu.org/software/emacs/manual/html_node/emacs/Log-Buffer.html
> doesn't document C-c C-k and doesn't explain how the region is set up so
> that an initial C-w will kill from point to the end of the buffer.  log-edit's
> documentation is also not correct with regard to this (i.e. the entire log
> buffer isn't emptied as point is after the Summary: header).
> 
> Is that worth reporting separately?

Yes, probably.  IMO, documenting "C-c C-k" should be accompanied with
the description of log-edit-comment-ring and its usage, otherwise the
command will not make sense to the users.

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

From: Christoph Badura <bad <at> bsd.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 29 Jul 2024 17:02:48 +0200

On Mon, Jul 29, 2024 at 03:34:32PM +0300, Eli Zaretskii wrote:
> > Date: Mon, 29 Jul 2024 11:53:13 +0200
> > From: Christoph Badura <bad <at> bsd.de>
> > Cc: 72341 <at> debbugs.gnu.org
> > 
> > On Mon, Jul 29, 2024 at 05:26:50AM +0300, Eli Zaretskii wrote:
> > > Somewhat clearer, thanks.  I don't see this stripping feature
> > > documented in the CVS manual; did I miss something?
> > 
> > I didn't see it documented either.  The closest thing that comes to
> > documentation of that feature is the documentation for rcsinfo:
> > https://www.gnu.org/software/trans-coord/manual/cvs/html_node/rcsinfo.html#rcsinfo
> 
> This says nothing about the "CVS: " prefix, AFAICT.
> 
> > It is implemented in src/logmsg.c:do_editor().
> > 
> > > And one more questions: where do those "CVS:" lines come from when you
> > > use the template file?
> > 
> > They have to be be in the template file.
> > 
> > I figure the idea is that the template files can contain "mandatory" text
> > that will be part of the actuall log message and "explanatory" text,
> > prefixed with "CVS: ", that will be stripped after the commit message has
> > been edited.
> > 
> > As an example, here's the default template for the NetBSD repositories:
> > ----------------8<------------------8<------------------8<-----------------
> > CVS: ----------------------------------------------------------------------
> > CVS: CVSROOT  cvs.NetBSD.org:/cvsroot
> > CVS: please use "PR category/123" to have the commitmsg appended to PR 123
> > ----------------8<------------------8<------------------8<-----------------
> > 
> > I'm only aware of the NetBSD and pkgsrc repositories that make use of CVS
> > templates.  And their template files contain only lines prefixed with
> > "CVS: ".
> 
> I think I see what's happening.  This is basically an undocumented
> feature.  The removal of "CVS: " lines is there for when CVS itself
> invokes the editor: in that case, it injects instructions into the
> temporary file that it submits to the editor, and those instructions
> all start with "CVS: ", so that they could be later removed.  IOW,
> this prefix is basically an agreement between CVS and itself.
> 
> Now, nothing in the documentation of CVS/Template file says anything
> about "CVS: ", but the code processes the temporary file, which
> includes the template, the same as it processes the lines injected by
> CVS, and thus every line that begins with "CVS: " in the template will
> be removed.
> 
> IOW, NetBSD piggy-backs this feature to include their own instructions
> in the template.

That pretty much sums it up.

However, while this behaviour isn't mentioned in the documentation, it is
mentioned in every commit message file that cvs creates when it invokes an
editor.  E.g.:

CVS: ----------------------------------------------------------------------
CVS: Enter Log.  Lines beginning with `CVS:' are removed automatically
CVS: 
CVS: Committing in .
CVS: 
CVS: Modified Files:
CVS: 	test.txt 
CVS: ----------------------------------------------------------------------

I have to admit, that after 30+ years of using cvs outside of emacs, my
brain filters out that bit of the commit message.  Anyway, it's clearly
advertised even if it isn't documented in the manual.

Note though, that when cvs invokes an editor itself to enter the commit
message, it is impossible to disable this behaviour.  That is, lines
starting with "CVS: " are always removed.

> I'm unsure how to proceed with this.  My bother is that this is a
> definite change in behavior wrt what VC did until now.  Users of VC
> might be unaware of this removal, and could start some log lines with
> the prefix, which will mysteriously disappear from the log message.

I've thought about that.  Clearly it is a change in behaviour.  The
interesting question is, will users notice?

My experience with this is that when I took up using emacs again after a
very long hiatus this behaviour of VC tripped me up on my first commit.
That was very annyoing indeed.  A friend suggested to M-x erase-buffer on
every commit.  Obviously he wasn't aware of that C-w would have the
desired effect.  Neither did I.  And that is not documented in the manual.

This was enough of a bother to not use VC again until this weekend and
then dig into why that is happending and not when I use "cvs ci file"
outside emacs (and have it invoke emacsclient).

If you work in an environment where other uses are using cvs outside of
emacs VC, the only way to have lines starting with "CVS: " in the stored
commit message is to require that the other users use "cvs ci -m" or
prepare a commit message separately in a file and use "cvs ci -F".
Frankly, a policy that requires stored commit messages containing lines
starting with "CVS: " is not practical.

Would VC users notice the change in behaviour?  I'd say they are
already using erase-buffer or C-w to clear the cvs template lines.  Or
they dug into the code and created a log-edit-done-hook similar to what
I proposed.  And they can continue to do so with no change to observed
behaviour.

And how many users that would be affected are there anyway?  I think it is
likely that there are very few or you would have got complaints in the
last decades.

Anyway, IMHO it is fine to add such a hook function by default to
log-edit-done-hook.  People could customize it away if it really
interferes with their work.

It's also not a big deal to fix up the commit messages with "cvs admin".
I'd imagine people would notice quickly.

--chris

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

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>, Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Tue, 30 Jul 2024 16:35:29 +0300

On 29/07/2024 15:34, Eli Zaretskii wrote:
> I'm unsure how to proceed with this.  My bother is that this is a
> definite change in behavior wrt what VC did until now.  Users of VC
> might be unaware of this removal, and could start some log lines with
> the prefix, which will mysteriously disappear from the log message.
> 
> Dmitry, WDYT?  Maybe we should add this removal guarded by a user
> option, by default off?  Or maybe we can identify where the template
> starts and ends, and only remove in that region?

If we do this, we'd only remove the lines starting with "CVS:". We could 
also add special syntax highlighting for them. That would probably be 
enough.

Then vc-cvs-checkin would additionally process the commit message string 
returned by log-edit-extract-headers.

Note that it seems we only handle "templates" for CVS and RCS, so there 
is no prior art for implementing this feature.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: bad <at> bsd.de, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 02 Aug 2024 10:19:30 +0300

> Date: Tue, 30 Jul 2024 16:35:29 +0300
> Cc: 72341 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> On 29/07/2024 15:34, Eli Zaretskii wrote:
> > I'm unsure how to proceed with this.  My bother is that this is a
> > definite change in behavior wrt what VC did until now.  Users of VC
> > might be unaware of this removal, and could start some log lines with
> > the prefix, which will mysteriously disappear from the log message.
> > 
> > Dmitry, WDYT?  Maybe we should add this removal guarded by a user
> > option, by default off?  Or maybe we can identify where the template
> > starts and ends, and only remove in that region?
> 
> If we do this, we'd only remove the lines starting with "CVS:". We could 
> also add special syntax highlighting for them. That would probably be 
> enough.

Agreed.

> Then vc-cvs-checkin would additionally process the commit message string 
> returned by log-edit-extract-headers.
> 
> Note that it seems we only handle "templates" for CVS and RCS, so there 
> is no prior art for implementing this feature.

Right.  Patches welcome to implement this.

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

From: Christoph Badura <bad <at> bsd.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 9 Aug 2024 17:11:51 +0200

Sorry for the delay.

On Fri, Aug 02, 2024 at 10:19:30AM +0300, Eli Zaretskii wrote:
> > Date: Tue, 30 Jul 2024 16:35:29 +0300
> > Cc: 72341 <at> debbugs.gnu.org
> > From: Dmitry Gutov <dmitry <at> gutov.dev>
> > 
> > On 29/07/2024 15:34, Eli Zaretskii wrote:
> > > I'm unsure how to proceed with this.  My bother is that this is a
> > > definite change in behavior wrt what VC did until now.  Users of VC
> > > might be unaware of this removal, and could start some log lines with
> > > the prefix, which will mysteriously disappear from the log message.
> > > 
> > > Dmitry, WDYT?  Maybe we should add this removal guarded by a user
> > > option, by default off?  Or maybe we can identify where the template
> > > starts and ends, and only remove in that region?
> > 
> > If we do this, we'd only remove the lines starting with "CVS:". We could 
> > also add special syntax highlighting for them. That would probably be 
> > enough.
> Agreed.

I'm afraid my emacs-fu isn't strong enough for that.

> > Then vc-cvs-checkin would additionally process the commit message string 
> > returned by log-edit-extract-headers.
> > 
> > Note that it seems we only handle "templates" for CVS and RCS, so there 
> > is no prior art for implementing this feature.
> 
> Right.  Patches welcome to implement this.

Cool.  I'll get back to this.  But probably not this month.  I'm extremly
busy right now.

--chris

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 00:08:26 +0100

[Message part 1 (text/plain, inline)]

Sorry for the long delay.

I guess I better start with a short summary to spare you the trouble of
reading through the previous messages relating to this bug.

When "cvs commit [file...]" is invoked from the command line, cvs prepares
a temp file with a template of a commit message and invokes an editor on
it. The template commit message, by default, is like this:

CVS: ----------------------------------------------------------------------
CVS: Enter Log.  Lines beginning with `CVS:' are removed automatically
CVS: 
CVS: Committing in .
CVS: 
CVS: Modified Files:
CVS: 	test.txt 
CVS: ----------------------------------------------------------------------

After saving the edited commit message and exiting the editor, cvs strips
lines beginning with 'CVS:' from the temp file.  NB. this is the only
documentation for this feature.  The CVS manual doesn't mention this.

When committing with "cvs commit -m 'Commit message.' [files...]" this
stripping is *not* performed.

The commit message template in the temp file is augmented with the
contents of the file 'CVS/Template', if it exists or a template received
from the CVS server..

I work on NetBSD where we make use of such a template.

'vc-cvs-checkin' emulates the first part of this (inserting the template
from the server/'CVS/Template file.  However, it does not strip the lines
beginning with "CVS:" and uses the "cvs commit -m 'Commit message.'" method
to do the actual commit.  That causes the "CVS:"-lines to end up in the
actual commit.

I have drafted a change that adds a function to the 'log-edit-done-hook'
that strips lines beginning with "CVS:" from the commit message if the
'log-edit-vc-backend' is "'CVS".

Because this hook function does its work only when the
'log-edit-vc-backend' is CVS and since this just makes 'vc-cvs-checkin'
behave like invking "cvs commit [files...]" from the command line I
consider this a change low risk and don't think that it would surprise
existing users.  Therefore I think it is appropriate to enable this hook
by default.

It would be nice if this would end up in emacs-30.1.

I'd appreciate any review and guidance what is missing to make this
committable.

I guess I need to add an entry to the NEWS file.

--chris

[0001-VC-strip-CVS-template-lines-when-committing.patch (text/plain, attachment)]

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 10:54:36 +0000

Hello,

On Mon 27 Jan 2025 at 12:08am +01, Christoph Badura wrote:

> Because this hook function does its work only when the
> 'log-edit-vc-backend' is CVS and since this just makes 'vc-cvs-checkin'
> behave like invking "cvs commit [files...]" from the command line I
> consider this a change low risk and don't think that it would surprise
> existing users.  Therefore I think it is appropriate to enable this hook
> by default.

Cool.

> It would be nice if this would end up in emacs-30.1.

Unfortunately, I believe it's too late for that.

> I'd appreciate any review and guidance what is missing to make this
> committable.
>
> I guess I need to add an entry to the NEWS file.

This is more of a bugfix, so you can add one if you would like, but I'm
not sure it's necessary.

> +  :version "30.1"
>    :group 'log-edit
>    :type '(hook :options (log-edit-set-common-indentation
> -			 log-edit-add-to-changelog)))
> +			 log-edit-add-to-changelog
> +                         log-edit-done-strip-cvs-lines)))
>
>  (defcustom log-edit-strip-single-file-name nil
>    "If non-nil, remove file name from single-file log entries."
> @@ -926,6 +928,16 @@ log-edit-insert-cvs-template
>        (goto-char (point-max))
>        (insert-file-contents "CVS/Template"))))
>
> +(defun log-edit-done-strip-cvs-lines ()
> +  "Strip lines starting with \"CVS:\" from commit log message.
> +When not interactively do this only when the VC backend is CVS."

I think it would be better to use a wrapper function that looks at
log-edit-vc-backend and have the command do it unconditionally.
called-interactively-p should be avoided if it can be avoided.

> +    (let ((search-upper-case nil))

I think it's more usual to bind case-fold-search not search-upper-case.

> diff --git a/test/lisp/vc/log-edit-tests.el b/test/lisp/vc/log-edit-tests.el
> index 005c336a3b6..e0e8d3e677a 100644
> --- a/test/lisp/vc/log-edit-tests.el
> +++ b/test/lisp/vc/log-edit-tests.el
> @@ -360,4 +360,41 @@ log-edit-fill-entry-no-defun-list-wrapping
>        (let ((fill-column 64)) (log-edit-fill-entry))
>        (should (equal (buffer-string) wanted)))))
>
> +(ert-deftest log-edit-done-strip-cvs-lines-cvs ()
> +  ;; This test verifies that lines beginning with "CVS: " are stripped
> +  ;; from the commit message when log-edit-vc-backend is CVS.
> +  (let (string wanted)
> +    (setq string "summary line
> +first line
> +CVS: Please evaluate your changes and consider the following.
> +CVS: Abort checkin if you answer no.
> +"
> +          wanted "summary line
> +first line
> +")
> +    (with-temp-buffer
> +      (let ((log-edit-done-hook 'log-edit-done-strip-cvs-lines)
> +            (log-edit-vc-backend 'CVS))

These tests are fairly contrived because you bind log-edit-vc-backend
yourself instead of using an actual CVS repository.  (Not a blocker.)

-- 
Sean Whitton

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>, Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 05:51:06 -0600

Sean Whitton <spwhitton <at> spwhitton.name> writes:

> On Mon 27 Jan 2025 at 12:08am +01, Christoph Badura wrote:
>
>> It would be nice if this would end up in emacs-30.1.
>
> Unfortunately, I believe it's too late for that.

It's definitely too late for Emacs 30.1, but if it's a bug fix, we might
be able to consider it for Emacs 30.2.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 14:21:39 +0200

> Date: Mon, 27 Jan 2025 00:08:26 +0100
> From: Christoph Badura <bad <at> bsd.de>
> 
> It would be nice if this would end up in emacs-30.1.

As Sean says, it's too late for that.  But we could perhaps consider
it for backporting into Emacs 30.2, provided that it will cause no
complaints while on master.

> I guess I need to add an entry to the NEWS file.

Yes, because the user option changed.

> -(defcustom log-edit-done-hook nil
> +(defcustom log-edit-done-hook '(log-edit-done-strip-cvs-lines)
>    "Hook run before doing the actual commit.
>  This hook can be used to cleanup the message, enforce various
>  conventions, or to allow recording the message in some other database,
>  such as a bug-tracking system.  The list of files about to be committed
>  can be obtained from `log-edit-files'."
> +  :version "30.1"

The :version should change to 31.1.

Also, I'm not sure we can turn this on by default, as I explained in
the previous discussions.

> +(defun log-edit-done-strip-cvs-lines ()
> +  "Strip lines starting with \"CVS:\" from commit log message.
> +When not interactively do this only when the VC backend is CVS."

Please add here a short description of the rationale for this
functionality.  CVS being used as infrequently nowadays as it is, I'm
fairly sure most people won't understand the purpose of this without
some help.

Last, but not least: you don't seem to have a copyright assignment on
file, without which we cannot accept a contribution of this size.
Would you like to start your copyright assignment paperwork at this
time?  If you agree, I will send you the form to fill with the
instructions.

Thanks for working on this.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: bad <at> bsd.de, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 15:29:20 +0200

> Cc: 72341 <at> debbugs.gnu.org
> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Mon, 27 Jan 2025 10:54:36 +0000
> 
> > I guess I need to add an entry to the NEWS file.
> 
> This is more of a bugfix, so you can add one if you would like, but I'm
> not sure it's necessary.

From where I stand, it introduces a new feature (albeit a minor one):
users can now request removal of these prefixes.  And if we agree to
change the default value of the user option, then we definitely
should call it out.

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

From: Christoph Badura <bad <at> bsd.de>
To: Eli Zaretskii <eliz <at> gnu.org>, Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 20:13:31 +0100

I'll reply to the 3 relevant mails in one message.  Is that generally
acceptable on this list?

On Mon, Jan 27, 2025 at 02:21:39PM +0200, Eli Zaretskii wrote:
> > Date: Mon, 27 Jan 2025 00:08:26 +0100
> > From: Christoph Badura <bad <at> bsd.de>
> > 
> > It would be nice if this would end up in emacs-30.1.
> 
> As Sean says, it's too late for that.  But we could perhaps consider
> it for backporting into Emacs 30.2, provided that it will cause no
> complaints while on master.

Sure. I'd be very happy with that too.  I'm not going to press the issue.

I was thinking it would be convenient for me to have this show up in a
release soon.  But I guess I will have to drag around my local
implementation in the init file for a couple of years until the default
emacs on the various machines will have caught up.

> > -(defcustom log-edit-done-hook nil
> > +(defcustom log-edit-done-hook '(log-edit-done-strip-cvs-lines)
> > [...]
> > +  :version "30.1"
> The :version should change to 31.1.

OK.  But now that the function isn't going to be enabled by default,
the :version doesn't need to change (or be added).

> Also, I'm not sure we can turn this on by default, as I explained in
> the previous discussions.

OK.  I do understand that you guys are not comfortable with turning this
on immediately.  That begs the question what the criteria for turning this
on by default are.  But let's discuss that separately.

> > +(defun log-edit-done-strip-cvs-lines ()
> > +  "Strip lines starting with \"CVS:\" from commit log message.
> > +When not interactively do this only when the VC backend is CVS."
> 
> Please add here a short description of the rationale for this
> functionality.  CVS being used as infrequently nowadays as it is, I'm
> fairly sure most people won't understand the purpose of this without
> some help.

How about:

  "Strip lines starting with \"CVS:\" from commit log message.
When not interactively do this only when the VC backend is CVS.
This mimicks what CVS does when invoked 'cvs commit [files...]'"

> Last, but not least: you don't seem to have a copyright assignment on
> file, without which we cannot accept a contribution of this size.

I've already started the process.  I've turned in the paperwork on Jan
6th but haven't heard back yet.

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 20:24:47 +0100

On Mon, Jan 27, 2025 at 02:21:39PM +0200, Eli Zaretskii wrote:
> > Date: Mon, 27 Jan 2025 00:08:26 +0100
> > From: Christoph Badura <bad <at> bsd.de>
> > 
> > It would be nice if this would end up in emacs-30.1.
> 
> As Sean says, it's too late for that.  But we could perhaps consider
> it for backporting into Emacs 30.2, provided that it will cause no
> complaints while on master.

No worries.  I just thought it would be nice to have this show up in a
release soonich.  But I guess I will be dragging along my local
implementation of the hook for years in my init.el anyway until all the
emacsen installed by default on the machines I use will have caughtup.

> > I guess I need to add an entry to the NEWS file.
> Yes, because the user option changed.

How about:

  ---
  ** VC mode
  *** New function 'log-edit-done-strip-cvs-lines'.
  This function strips all lines beginning with "CVS:" from the buffer.
  It is intended to be added to the 'log-edit-done-hook' so that
  'vc-cvs-checkin' behaves like "cvs commit" from the command line.

Or perhaps:

  It is intended to be added to the 'log-edit-done-hook' so that checking
  in a commit under CVS behaves like "cvs commit" from the command line.

> > -(defcustom log-edit-done-hook nil
> > +(defcustom log-edit-done-hook '(log-edit-done-strip-cvs-lines)
> >    "Hook run before doing the actual commit.
> >  This hook can be used to cleanup the message, enforce various
> >  conventions, or to allow recording the message in some other database,
> >  such as a bug-tracking system.  The list of files about to be committed
> >  can be obtained from `log-edit-files'."
> > +  :version "30.1"
> 
> The :version should change to 31.1.

OK.  But as the default value now isn't going to change that's not needed
anymore.

> Also, I'm not sure we can turn this on by default, as I explained in
> the previous discussions.

OK.  That begs the question what the criteria for enabling this by
default are.  But let's discuss that separately.

> > +(defun log-edit-done-strip-cvs-lines ()
> > +  "Strip lines starting with \"CVS:\" from commit log message.
> > +When not interactively do this only when the VC backend is CVS."
> 
> Please add here a short description of the rationale for this
> functionality.  CVS being used as infrequently nowadays as it is, I'm
> fairly sure most people won't understand the purpose of this without
> some help.

How about:

  "Strip lines starting with \"CVS:\" from commit log message.
  When not interactively do this only when the VC backend is CVS.
  This mimicks what CVS does when invoked 'cvs commit [files...]'"

> Last, but not least: you don't seem to have a copyright assignment on
> file, without which we cannot accept a contribution of this size.

I've already started the paperwork.  I've turned in the signed assignement
on Jan 6th but haven't heard back yet.  I did reply to the ticket
yesterday so see what is up.

--chris

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org, spwhitton <at> spwhitton.name
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 21:49:09 +0200

> Date: Mon, 27 Jan 2025 20:13:31 +0100
> From: Christoph Badura <bad <at> bsd.de>
> Cc: 72341 <at> debbugs.gnu.org
> 
> I'll reply to the 3 relevant mails in one message.  Is that generally
> acceptable on this list?

Sure, no problem with that.

> > > -(defcustom log-edit-done-hook nil
> > > +(defcustom log-edit-done-hook '(log-edit-done-strip-cvs-lines)
> > > [...]
> > > +  :version "30.1"
> > The :version should change to 31.1.
> 
> OK.  But now that the function isn't going to be enabled by default,
> the :version doesn't need to change (or be added).

Yes.  But (a) let's hear what others think about making this the
default, and (b) I think we should add to the doc string of this
option the reference to this function, whose main purpose is to be
used in this hook.

> > Also, I'm not sure we can turn this on by default, as I explained in
> > the previous discussions.
> 
> OK.  I do understand that you guys are not comfortable with turning this
> on immediately.  That begs the question what the criteria for turning this
> on by default are.  But let's discuss that separately.

Right.

> > > +(defun log-edit-done-strip-cvs-lines ()
> > > +  "Strip lines starting with \"CVS:\" from commit log message.
> > > +When not interactively do this only when the VC backend is CVS."
> > 
> > Please add here a short description of the rationale for this
> > functionality.  CVS being used as infrequently nowadays as it is, I'm
> > fairly sure most people won't understand the purpose of this without
> > some help.
> 
> How about:
> 
>   "Strip lines starting with \"CVS:\" from commit log message.
> When not interactively do this only when the VC backend is CVS.
> This mimicks what CVS does when invoked 'cvs commit [files...]'"

SGTM, thanks.

> > Last, but not least: you don't seem to have a copyright assignment on
> > file, without which we cannot accept a contribution of this size.
> 
> I've already started the process.  I've turned in the paperwork on Jan
> 6th but haven't heard back yet.

If they don't reply a week from now, please ping them and CC me.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 21:53:21 +0200

> Date: Mon, 27 Jan 2025 20:24:47 +0100
> From: Christoph Badura <bad <at> bsd.de>
> 
> On Mon, Jan 27, 2025 at 02:21:39PM +0200, Eli Zaretskii wrote:
> > > Date: Mon, 27 Jan 2025 00:08:26 +0100
> > > From: Christoph Badura <bad <at> bsd.de>
> > > 
> > > It would be nice if this would end up in emacs-30.1.
> > 
> > As Sean says, it's too late for that.  But we could perhaps consider
> > it for backporting into Emacs 30.2, provided that it will cause no
> > complaints while on master.
> 
> No worries.  I just thought it would be nice to have this show up in a
> release soonich.  But I guess I will be dragging along my local
> implementation of the hook for years in my init.el anyway until all the
> emacsen installed by default on the machines I use will have caughtup.
> 
> > > I guess I need to add an entry to the NEWS file.
> > Yes, because the user option changed.
> 
> How about:
> 
>   ---
>   ** VC mode
>   *** New function 'log-edit-done-strip-cvs-lines'.
>   This function strips all lines beginning with "CVS:" from the buffer.
>   It is intended to be added to the 'log-edit-done-hook' so that
>   'vc-cvs-checkin' behaves like "cvs commit" from the command line.
> 
> Or perhaps:
> 
>   It is intended to be added to the 'log-edit-done-hook' so that checking
>   in a commit under CVS behaves like "cvs commit" from the command line.

Both are okay, thanks.

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 21:17:36 +0100

Thanks to for the review by both you and Eli.

On Mon, Jan 27, 2025 at 10:54:36AM +0000, Sean Whitton wrote:
> On Mon 27 Jan 2025 at 12:08am +01, Christoph Badura wrote:
> > +(defun log-edit-done-strip-cvs-lines ()
> > +  "Strip lines starting with \"CVS:\" from commit log message.
> > +When not interactively do this only when the VC backend is CVS."
> 
> I think it would be better to use a wrapper function that looks at
> log-edit-vc-backend and have the command do it unconditionally.

I don't understand what you mean.  Wrapper function around what?  And what
command should do what exactly unconditionally?

> called-interactively-p should be avoided if it can be avoided.

I was going by what almost all of the other log-edit-*-hook function in
log-edit.el do.  Also I thought it would be convenient to be able to M-X
this function.  I did cal called-interactively-p incorrectly, though.

> > +    (let ((search-upper-case nil))
> I think it's more usual to bind case-fold-search not search-upper-case.

Changed.  I did use search-upper-case because the docstring for
flush-lines mentions that and the regexp does contain upper case
characters.  So all the conditions are true and I did not look further.

Do you think the docstring for flush-lines should mention case-fold-search
too?

> > diff --git a/test/lisp/vc/log-edit-tests.el b/test/lisp/vc/log-edit-tests.el
> > index 005c336a3b6..e0e8d3e677a 100644
> > --- a/test/lisp/vc/log-edit-tests.el
> > +++ b/test/lisp/vc/log-edit-tests.el
> > @@ -360,4 +360,41 @@ log-edit-fill-entry-no-defun-list-wrapping
> >        (let ((fill-column 64)) (log-edit-fill-entry))
> >        (should (equal (buffer-string) wanted)))))
> >
> > +(ert-deftest log-edit-done-strip-cvs-lines-cvs ()
> > +  ;; This test verifies that lines beginning with "CVS: " are stripped
> > +  ;; from the commit message when log-edit-vc-backend is CVS.
> > +  (let (string wanted)
> > +    (setq string "summary line
> > +first line
> > +CVS: Please evaluate your changes and consider the following.
> > +CVS: Abort checkin if you answer no.
> > +"
> > +          wanted "summary line
> > +first line
> > +")
> > +    (with-temp-buffer
> > +      (let ((log-edit-done-hook 'log-edit-done-strip-cvs-lines)
> > +            (log-edit-vc-backend 'CVS))
> 
> These tests are fairly contrived because you bind log-edit-vc-backend
> yourself instead of using an actual CVS repository.  (Not a blocker.)

Binding log-edit-vc-backend myself is deliberate.  I'm testing behavior
that is internal to log-edit and independent from an actual respository
(CVS or other).  This is about what happens when log-edit-done cleans up
the buffer with the commit message before handing it is handed over to
the backend for the actual committing.  (Setting log-edit-vc-backend is
part of the contract of calling log-edit from vc-log-edit.)

I did notice just now that a) the docstring for log-edit-vc-backend is
wrong, and b) the call to vc-diff in log-edit-diff-fileset no longer
matches the signature of vc-diff.

--chris

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Mon, 27 Jan 2025 22:58:57 +0100

On Mon, Jan 27, 2025 at 09:49:09PM +0200, Eli Zaretskii wrote:
> > Date: Mon, 27 Jan 2025 20:13:31 +0100
> > From: Christoph Badura <bad <at> bsd.de>
> > > > -(defcustom log-edit-done-hook nil
> > > > +(defcustom log-edit-done-hook '(log-edit-done-strip-cvs-lines)
> > > > [...]
> > > > +  :version "30.1"
> > > The :version should change to 31.1.
> > 
> > OK.  But now that the function isn't going to be enabled by default,
> > the :version doesn't need to change (or be added).
> 
> Yes.  But (a) let's hear what others think about making this the
> default, and (b) I think we should add to the doc string of this
> option the reference to this function, whose main purpose is to be
> used in this hook.

Do you mean something like the following?

diff --git a/lisp/vc/log-edit.el b/lisp/vc/log-edit.el
index e5f195c01dd..aff62e8bb3c 100644
--- a/lisp/vc/log-edit.el
+++ b/lisp/vc/log-edit.el
@@ -207,7 +207,8 @@ log-edit-done-hook
 This hook can be used to cleanup the message, enforce various
 conventions, or to allow recording the message in some other database,
 such as a bug-tracking system.  The list of files about to be committed
-can be obtained from `log-edit-files'."
+can be obtained from `log-edit-files'.
+Add `log-edit-done-strip-cvs-lines' to be fully compatible with \"cvs commit\"."
   :group 'log-edit
   :type '(hook :options (log-edit-set-common-indentation
 			 log-edit-add-to-changelog

I'm somewhat surprised.  The other two hook functions aren't mentioned in
the docstring.  And I thought the customize UI would be "obvious" enough.

--chris

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Tue, 28 Jan 2025 14:23:39 +0200

> Date: Mon, 27 Jan 2025 22:58:57 +0100
> From: Christoph Badura <bad <at> bsd.de>
> 
> On Mon, Jan 27, 2025 at 09:49:09PM +0200, Eli Zaretskii wrote:
> > Yes.  But (a) let's hear what others think about making this the
> > default, and (b) I think we should add to the doc string of this
> > option the reference to this function, whose main purpose is to be
> > used in this hook.
> 
> Do you mean something like the following?

That's one possibility, yes.  But maybe we can do better, see below.

> I'm somewhat surprised.  The other two hook functions aren't mentioned in
> the docstring.

We could add them as well.

> And I thought the customize UI would be "obvious" enough.

The Customize UI will be more helpful if we explicitly mention the
recommended values together with descriptive tags.

But I realize that this is a separate issue, so maybe it should be the
subject of a separate patch.  So maybe just mentioning the 3
recommended values in the doc string will be enough for fixing this
issue.

Thanks.

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Tue, 28 Jan 2025 23:47:56 +0100

On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
> The Customize UI will be more helpful if we explicitly mention the
> recommended values together with descriptive tags.

So, I've just tested this:
  emacs -nw --eval "(customize-group 'log-edit)"

Search for "done hook".
Be sure to toggle the "More" button and than the "Show value" button on
the line for "Log Edit Done Hook".
With this patch applied there's even a "More" button at the end of the
one-line description of the log-edit-done-strip-cvs-lines option.

Do you really think that UX needs improving?

The only thing that annoys me is that when I search for "More", the point
ends up just behind the button and RET doesn't toggle it.  Whereas it does
when it is right before a button.  Fixing that would be a huge improvement
for me.  But this is a separate issue.

> But I realize that this is a separate issue, so maybe it should be the
> subject of a separate patch.  So maybe just mentioning the 3
> recommended values in the doc string will be enough for fixing this
> issue.

I've made a note in any case.

I've way overrun my "emacs hacking" time budget for this week.  I'll get
back to this bug at the end of next week after returning from travel.

--chris

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>, Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 15 Feb 2025 12:19:22 +0200

> Date: Tue, 28 Jan 2025 23:47:56 +0100
> From: Christoph Badura <bad <at> bsd.de>
> 
> On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
> > The Customize UI will be more helpful if we explicitly mention the
> > recommended values together with descriptive tags.
> 
> So, I've just tested this:
>   emacs -nw --eval "(customize-group 'log-edit)"
> 
> Search for "done hook".
> Be sure to toggle the "More" button and than the "Show value" button on
> the line for "Log Edit Done Hook".
> With this patch applied there's even a "More" button at the end of the
> one-line description of the log-edit-done-strip-cvs-lines option.
> 
> Do you really think that UX needs improving?
> 
> The only thing that annoys me is that when I search for "More", the point
> ends up just behind the button and RET doesn't toggle it.  Whereas it does
> when it is right before a button.  Fixing that would be a huge improvement
> for me.  But this is a separate issue.
> 
> > But I realize that this is a separate issue, so maybe it should be the
> > subject of a separate patch.  So maybe just mentioning the 3
> > recommended values in the doc string will be enough for fixing this
> > issue.
> 
> I've made a note in any case.
> 
> I've way overrun my "emacs hacking" time budget for this week.  I'll get
> back to this bug at the end of next week after returning from travel.

Mauro, WDYT about this?

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 15 Feb 2025 07:44:02 -0300

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Tue, 28 Jan 2025 23:47:56 +0100
>> From: Christoph Badura <bad <at> bsd.de>
>>
>> On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
>> > The Customize UI will be more helpful if we explicitly mention the
>> > recommended values together with descriptive tags.
>>
>> So, I've just tested this:
>>   emacs -nw --eval "(customize-group 'log-edit)"
>>
>> Search for "done hook".
>> Be sure to toggle the "More" button and than the "Show value" button on
>> the line for "Log Edit Done Hook".
>> With this patch applied there's even a "More" button at the end of the
>> one-line description of the log-edit-done-strip-cvs-lines option.
>>
>> Do you really think that UX needs improving?
>>
>> The only thing that annoys me is that when I search for "More", the 
point
>> ends up just behind the button and RET doesn't toggle it.  Whereas 
it does
>> when it is right before a button.  Fixing that would be a huge 
improvement
>> for me.  But this is a separate issue.
>>
>> > But I realize that this is a separate issue, so maybe it should be the
>> > subject of a separate patch.  So maybe just mentioning the 3
>> > recommended values in the doc string will be enough for fixing this
>> > issue.
>>
>> I've made a note in any case.
>>
>> I've way overrun my "emacs hacking" time budget for this week.  I'll get
>> back to this bug at the end of next week after returning from travel.
>
> Mauro, WDYT about this?

I guess you're asking me about actioning the "More" button when point is
past it, to show the rest of the documentation.

There's already code that handles custom group links that way, in
Custom-newline (RET is bound to that command).  For example:
emacs -Q
M-x customize-group RET vc
C-s Log-Edit RET
RET

Emacs follows the link even though point is past the link.

It should be easy to extend it to other links/buttons.

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 15 Feb 2025 09:47:51 -0300

[Message part 1 (text/plain, inline)]

Mauro Aranda <maurooaranda <at> gmail.com> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> Date: Tue, 28 Jan 2025 23:47:56 +0100
>>> From: Christoph Badura <bad <at> bsd.de>
>>>
>>> On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
>>> > The Customize UI will be more helpful if we explicitly mention the
>>> > recommended values together with descriptive tags.
>>>
>>> So, I've just tested this:
>>>   emacs -nw --eval "(customize-group 'log-edit)"
>>>
>>> Search for "done hook".
>>> Be sure to toggle the "More" button and than the "Show value" button on
>>> the line for "Log Edit Done Hook".
>>> With this patch applied there's even a "More" button at the end of the
>>> one-line description of the log-edit-done-strip-cvs-lines option.
>>>
>>> Do you really think that UX needs improving?
>>>
>>> The only thing that annoys me is that when I search for "More", the
>    point
>>> ends up just behind the button and RET doesn't toggle it.  Whereas
>    it does
>>> when it is right before a button.  Fixing that would be a huge
>    improvement
>>> for me.  But this is a separate issue.
>>>
>>> > But I realize that this is a separate issue, so maybe it should 
be the
>>> > subject of a separate patch.  So maybe just mentioning the 3
>>> > recommended values in the doc string will be enough for fixing this
>>> > issue.
>>>
>>> I've made a note in any case.
>>>
>>> I've way overrun my "emacs hacking" time budget for this week.  
I'll get
>>> back to this bug at the end of next week after returning from travel.
>>
>> Mauro, WDYT about this?
>
> I guess you're asking me about actioning the "More" button when point is
> past it, to show the rest of the documentation.
>
> There's already code that handles custom group links that way, in
> Custom-newline (RET is bound to that command).  For example:
> emacs -Q
> M-x customize-group RET vc
> C-s Log-Edit RET
> RET
>
> Emacs follows the link even though point is past the link.
>
> It should be easy to extend it to other links/buttons.

Like in the attached patch, lightly tested.  I can post it in a
different bug report if desired.

[0001-Action-button-when-point-is-just-after-it.patch (text/x-patch, attachment)]

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Tue, 18 Feb 2025 00:54:26 +0100

[Message part 1 (text/plain, inline)]

Here is the updated patch.  I incorporated all the feedback and requested
changes.  I've also found some other nits on my own.

I also looked more carefully at the CVS code and discovered that it only
looks for lines beginning with "CVS:", i.e., it doesn't care about a
following blank.  It does this in order to deal with editor that strip
trailing whitespace.

Changes since the last patch:
- don't activate log-edit-done-strip-cvs-lines by default just yet
- supply missing arg 'interactive to called-interactively-p
- use (case-fold-search nil) instead of (search-upper-case nil)
- mention log-edit-done-strip-cvs-lines in etc/NEWS
- explain why log-edit-done-strip-cvs-lines only looks for "^CVS:"
- doc strings for the log-edit-done-strip-cvs-lines tests
- factor out common code in log-edit-done-strip-cvs-lines tests
- 2 more tests for exactly the same behaviour as CVS

I've just noticed that, e.g. the lines following 
the "hook :options" in the log-edit-done-hook
defcustom, use TAB for indentation while indent-tabs-mode is turned off in
the top-level .dir-locals.el.  Do you care for consistency with the old
code?  This affects only a very small number of lines.

Sorry for this all taking so long.  And thanks for the feedback and the
patience.

--chris

[0001-VC-add-hook-to-strip-CVS-template-lines-when-committ.patch (text/plain, attachment)]

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 21 Feb 2025 11:54:16 +0800

Hello,

On Tue 18 Feb 2025 at 12:54am +01, Christoph Badura wrote:

> Sorry for this all taking so long.  And thanks for the feedback and the
> patience.

I'll make a note to review this soon.

In the future, please CC everyone involved in the discussion who might
want to re-review.

-- 
Sean Whitton

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: bad <at> bsd.de, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 01 Mar 2025 14:16:04 +0200

> Cc: 72341 <at> debbugs.gnu.org
> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Fri, 21 Feb 2025 11:54:16 +0800
> 
> Hello,
> 
> On Tue 18 Feb 2025 at 12:54am +01, Christoph Badura wrote:
> 
> > Sorry for this all taking so long.  And thanks for the feedback and the
> > patience.
> 
> I'll make a note to review this soon.

Thanks, did you have time to review it?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 01 Mar 2025 14:17:45 +0200

> Date: Tue, 18 Feb 2025 00:54:26 +0100
> From: Christoph Badura <bad <at> bsd.de>
> 
> +(defun log-edit-done-strip-cvs-lines-helper (initial-text wanted vc-backend)
> +  "Test that running log-edit-done-strip-cvs-lines as a
> +log-edit-done-hook produces the WANTED
> +string when run on INITIAL-TEXT."

The first line of a doc string should be a single complete sentence.

Thanks.

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: bad <at> bsd.de, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 01 Mar 2025 20:27:37 +0800

Hello,

On Sat 01 Mar 2025 at 02:16pm +02, Eli Zaretskii wrote:

>> Cc: 72341 <at> debbugs.gnu.org
>> From: Sean Whitton <spwhitton <at> spwhitton.name>
>> Date: Fri, 21 Feb 2025 11:54:16 +0800
>>
>> Hello,
>>
>> On Tue 18 Feb 2025 at 12:54am +01, Christoph Badura wrote:
>>
>> > Sorry for this all taking so long.  And thanks for the feedback and the
>> > patience.
>>
>> I'll make a note to review this soon.
>
> Thanks, did you have time to review it?

I had a busy week backporting the CVE fixes to Emacs 28.2, 27.1, 26.1,
25.1 and am still doing 24.5 and 24.4 as distributed by Debian and
Freexian, so I'm afraid not, but I expect to have upstream Emacs time
this upcoming week.

-- 
Sean Whitton

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Wed, 05 Mar 2025 19:44:33 +0800

Hello Christoph,

Thanks.  Let me first ask you, how is the copyright paperwork going?

On Tue 18 Feb 2025 at 12:54am +01, Christoph Badura wrote:

> I've just noticed that, e.g. the lines following
> the "hook :options" in the log-edit-done-hook
> defcustom, use TAB for indentation while indent-tabs-mode is turned off in
> the top-level .dir-locals.el.  Do you care for consistency with the old
> code?  This affects only a very small number of lines.

No, it's fine to convert them to being indented with only spaces if
you're editing them.  (We just don't do mass conversions.)

> Add a hook function to strip all lines beginning with "CVS:" from the
> commit message as CVS does.  Do this only if 'log-edit-vc-backend' is
> 'CVS'.  (Bug#72341)
> * lisp/vc/log-edit.el (log-edit-done-strip-cvs-lines)
> Add a hook function to strip the lines starting with "CVS:".

In the GNU changelog style, please use

* lisp/vc/log-edit.el
(log-edit-done-strip-cve-lines): New command.
(log-edit-done-hook): Add it as an option.

> * test/lisp/vc/log-edit-tests.el (log-edit-done-strip-cvs-lines-helper,

Similarly, just "New function."

> log-edit-done-strip-cvs-lines-cvs, log-edit-done-strip-cvs-lines-non-cvs,
> log-edit-done-strip-cvs-lines-only-cvs-colon-blank,
> log-edit-done-strip-cvs-lines-only-cvs-colon):
> Add test cases for 'log-edit-done-strip-cvs-lines'.

Similarly, just "New test cases."

> +(defun log-edit-done-strip-cvs-lines ()
> +  "Strip lines starting with \"CVS:\" from commit log message.
> +When not called interactively do this only when the VC backend is CVS.
> +This mimicks what CVS does when invoked as 'cvs commit [files...]'"
> +  (interactive)
> +  (when (or (called-interactively-p 'interactive)
> +            (eq log-edit-vc-backend 'CVS))

Take a look at the docstring for called-interactively-p.
It explains that it is better to do something like this:

    (defun log-edit-done-strip-cvs-lines (&optional interactive)
      (interactive "P")
      (when (or interactive (eq log-edit-vc-backend)) ...

> --- a/test/lisp/vc/log-edit-tests.el
> +++ b/test/lisp/vc/log-edit-tests.el
> @@ -360,4 +360,62 @@ log-edit-fill-entry-no-defun-list-wrapping
>        (let ((fill-column 64)) (log-edit-fill-entry))
>        (should (equal (buffer-string) wanted)))))
>
> +(defun log-edit-done-strip-cvs-lines-helper (initial-text wanted vc-backend)
> +  "Test that running log-edit-done-strip-cvs-lines as a
> +log-edit-done-hook produces the WANTED
> +string when run on INITIAL-TEXT."

It's not such an issue for test helper functions, but it would be better
to follow our convention of the first line of the docstring being a
complete sentence.

> +(ert-deftest log-edit-done-strip-cvs-lines-cvs ()
> +  "Strip lines beginning with \"CVS:\" when using CVS as VC backend."
> +  ;; This test verifies that lines beginning with "CVS:" are stripped
> +  ;; from the commit message when log-edit-vc-backend is CVS.

I would suggest merging the comment into the docstring, or just deleting
the docstring and just having a comment.
Less for someone to have to read.

Otherwise, all looks good to me.  Looking forward to applying it.

-- 
Sean Whitton

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 72341 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Wed, 05 Mar 2025 19:53:34 +0800

Hello,

Christoph, would you mind testing Mauro's patch here?

I'm keen not to let your valuable feedback on the customize UX, and
Mauro's fix, get lost in this VC bug.

Eli, did you have any feedback on Mauro's patch?

On Sat 15 Feb 2025 at 09:47am -03, Mauro Aranda wrote:

> Mauro Aranda <maurooaranda <at> gmail.com> writes:
>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>>>> Date: Tue, 28 Jan 2025 23:47:56 +0100
>>>> From: Christoph Badura <bad <at> bsd.de>
>>>>
>>>> On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
>>>> > The Customize UI will be more helpful if we explicitly mention the
>>>> > recommended values together with descriptive tags.
>>>>
>>>> So, I've just tested this:
>>>>   emacs -nw --eval "(customize-group 'log-edit)"
>>>>
>>>> Search for "done hook".
>>>> Be sure to toggle the "More" button and than the "Show value" button on
>>>> the line for "Log Edit Done Hook".
>>>> With this patch applied there's even a "More" button at the end of the
>>>> one-line description of the log-edit-done-strip-cvs-lines option.
>>>>
>>>> Do you really think that UX needs improving?
>>>>
>>>> The only thing that annoys me is that when I search for "More", the
>>    point
>>>> ends up just behind the button and RET doesn't toggle it.  Whereas
>>    it does
>>>> when it is right before a button.  Fixing that would be a huge
>>    improvement
>>>> for me.  But this is a separate issue.
>>>>
>>>> > But I realize that this is a separate issue, so maybe it should be the
>>>> > subject of a separate patch.  So maybe just mentioning the 3
>>>> > recommended values in the doc string will be enough for fixing this
>>>> > issue.
>>>>
>>>> I've made a note in any case.
>>>>
>>>> I've way overrun my "emacs hacking" time budget for this week.  I'll get
>>>> back to this bug at the end of next week after returning from travel.
>>>
>>> Mauro, WDYT about this?
>>
>> I guess you're asking me about actioning the "More" button when point is
>> past it, to show the rest of the documentation.
>>
>> There's already code that handles custom group links that way, in
>> Custom-newline (RET is bound to that command).  For example:
>> emacs -Q
>> M-x customize-group RET vc
>> C-s Log-Edit RET
>> RET
>>
>> Emacs follows the link even though point is past the link.
>>
>> It should be easy to extend it to other links/buttons.
>
> Like in the attached patch, lightly tested.  I can post it in a
> different bug report if desired.
>
> From e83562f8050118319f701fa45970a721afc44b36 Mon Sep 17 00:00:00 2001
> From: Mauro Aranda <maurooaranda <at> gmail.com>
> Date: Sat, 15 Feb 2025 09:43:58 -0300
> Subject: [PATCH] Action button when point is just after it
>
> * lisp/cus-edit.el (Custom-newline): If no button at point,
> check for a button before point.  (Bug#72341)
> ---
>  lisp/cus-edit.el | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
> index febbc8d1b8b..1fd87c4e9f3 100644
> --- a/lisp/cus-edit.el
> +++ b/lisp/cus-edit.el
> @@ -5344,10 +5344,13 @@ Custom-newline
>  To see what function the widget will call, use the
>  `widget-describe' command."
>    (interactive "@d")
> -  (let ((button (get-char-property pos 'button)))
> -    ;; If there is no button at point, then use the one at the start
> -    ;; of the line, if it is a custom-group-link (bug#2298).
> +  (let ((button (or (get-char-property pos 'button)
> +                    ;; Maybe we are just past a button, and it's quite handy
> +                    ;; to action it as well.  (Bug#72341)
> +                    (get-char-property (1- pos) 'button))))
>      (or button
> +        ;; If there is no button at point, then use the one at the start
> +        ;; of the line, if it is a custom-group-link (bug#2298).
>  	(if (setq button (get-char-property (line-beginning-position) 'button))
>  	    (or (eq (widget-type button) 'custom-group-link)
>  		(setq button nil))))

-- 
Sean Whitton

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: bad <at> bsd.de, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Wed, 05 Mar 2025 15:52:44 +0200

> Cc: 72341 <at> debbugs.gnu.org
> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Wed, 05 Mar 2025 19:44:33 +0800
> 
> Hello Christoph,
> 
> Thanks.  Let me first ask you, how is the copyright paperwork going?

It's done, the assignment is on file.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: bad <at> bsd.de, 72341 <at> debbugs.gnu.org, maurooaranda <at> gmail.com
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Wed, 05 Mar 2025 15:55:31 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  72341 <at> debbugs.gnu.org
> Date: Wed, 05 Mar 2025 19:53:34 +0800
> 
> Hello,
> 
> Christoph, would you mind testing Mauro's patch here?
> 
> I'm keen not to let your valuable feedback on the customize UX, and
> Mauro's fix, get lost in this VC bug.
> 
> Eli, did you have any feedback on Mauro's patch?

No comments here.

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Thu, 6 Mar 2025 11:49:25 +0100

Hi.

cc'ing Eli, too.

I stripped the Cc:s because I was hoping that debbugs would cc: all
parties involved in the bug report automatically.  I couldn't find
anything about the developer's preferences in the documentation
(src/CONTRIBUTE, https://debbugs.gnu.org/ and the manual "(Emacs) Bugs")

Perhaps the preferred way could be documented?

On Wed, Mar 05, 2025 at 07:44:33PM +0800, Sean Whitton wrote:
> Thanks.  Let me first ask you, how is the copyright paperwork going?

Yeah, it's done.

I'll try hard to prepare a new patch over the weekend.

I've deleted the parts of your message that don't need
explanation/disscussion from my point of view.

> On Tue 18 Feb 2025 at 12:54am +01, Christoph Badura wrote:
> 
> > +(defun log-edit-done-strip-cvs-lines ()
> > +  "Strip lines starting with \"CVS:\" from commit log message.
> > +When not called interactively do this only when the VC backend is CVS.
> > +This mimicks what CVS does when invoked as 'cvs commit [files...]'"
> > +  (interactive)
> > +  (when (or (called-interactively-p 'interactive)
> > +            (eq log-edit-vc-backend 'CVS))
> 
> Take a look at the docstring for called-interactively-p.
> It explains that it is better to do something like this:
> 
>     (defun log-edit-done-strip-cvs-lines (&optional interactive)
>       (interactive "P")
>       (when (or interactive (eq log-edit-vc-backend)) ...

Sure, I'm happy to do that.

Frankly, I just copyied the code from the other log-edit-* hooks in
log-edit.el without giving it further thought.

That begs the question whether you want the other places in that file
adjusted to this pattern too?  Presumably as a separate patch.

> > --- a/test/lisp/vc/log-edit-tests.el
> > +++ b/test/lisp/vc/log-edit-tests.el
> > @@ -360,4 +360,62 @@ log-edit-fill-entry-no-defun-list-wrapping
> >        (let ((fill-column 64)) (log-edit-fill-entry))
> >        (should (equal (buffer-string) wanted)))))
> >
> > +(defun log-edit-done-strip-cvs-lines-helper (initial-text wanted vc-backend)
> > +  "Test that running log-edit-done-strip-cvs-lines as a
> > +log-edit-done-hook produces the WANTED
> > +string when run on INITIAL-TEXT."
> 
> It's not such an issue for test helper functions, but it would be better
> to follow our convention of the first line of the docstring being a
> complete sentence.

Perhaps:

"Helper function for the log-edit-done-strip-cvs-lines tests.

Test that running log-edit-done-strip-cvs-lines as a
log-edit-done-hook produces the WANTED string when run on INITIAL-TEXT."

> > +(ert-deftest log-edit-done-strip-cvs-lines-cvs ()
> > +  "Strip lines beginning with \"CVS:\" when using CVS as VC backend."
> > +  ;; This test verifies that lines beginning with "CVS:" are stripped
> > +  ;; from the commit message when log-edit-vc-backend is CVS.
> 
> I would suggest merging the comment into the docstring, or just deleting
> the docstring and just having a comment.
> Less for someone to have to read.

I think I'll delete the comment.  On second reading it is just saying what
the docstring says except in more words.

> Otherwise, all looks good to me.  Looking forward to applying it.

--chris

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Mauro Aranda <maurooaranda <at> gmail.com>,
 Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Thu, 6 Mar 2025 13:54:05 +0100

Hi,

Sure, I'll test it.

I wanted to check this more carefully.  I see the same behaviour for links
in help-mode buffers as created by e.g. describe-variable, and
describe-function.  But I can't rember if I checked that on a build from
the lastest git sources.

Maybe this should be moved to a separate bug report?  I would prefer that.

--chris

On Wed, Mar 05, 2025 at 07:53:34PM +0800, Sean Whitton wrote:
> Hello,
> 
> Christoph, would you mind testing Mauro's patch here?
> 
> I'm keen not to let your valuable feedback on the customize UX, and
> Mauro's fix, get lost in this VC bug.
> 
> Eli, did you have any feedback on Mauro's patch?
> 
> On Sat 15 Feb 2025 at 09:47am -03, Mauro Aranda wrote:
> 
> > Mauro Aranda <maurooaranda <at> gmail.com> writes:
> >
> >> Eli Zaretskii <eliz <at> gnu.org> writes:
> >>
> >>>> Date: Tue, 28 Jan 2025 23:47:56 +0100
> >>>> From: Christoph Badura <bad <at> bsd.de>
> >>>>
> >>>> On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
> >>>> > The Customize UI will be more helpful if we explicitly mention the
> >>>> > recommended values together with descriptive tags.
> >>>>
> >>>> So, I've just tested this:
> >>>>   emacs -nw --eval "(customize-group 'log-edit)"
> >>>>
> >>>> Search for "done hook".
> >>>> Be sure to toggle the "More" button and than the "Show value" button on
> >>>> the line for "Log Edit Done Hook".
> >>>> With this patch applied there's even a "More" button at the end of the
> >>>> one-line description of the log-edit-done-strip-cvs-lines option.
> >>>>
> >>>> Do you really think that UX needs improving?
> >>>>
> >>>> The only thing that annoys me is that when I search for "More", the
> >>    point
> >>>> ends up just behind the button and RET doesn't toggle it.  Whereas
> >>    it does
> >>>> when it is right before a button.  Fixing that would be a huge
> >>    improvement
> >>>> for me.  But this is a separate issue.
> >>>>
> >>>> > But I realize that this is a separate issue, so maybe it should be the
> >>>> > subject of a separate patch.  So maybe just mentioning the 3
> >>>> > recommended values in the doc string will be enough for fixing this
> >>>> > issue.
> >>>>
> >>>> I've made a note in any case.
> >>>>
> >>>> I've way overrun my "emacs hacking" time budget for this week.  I'll get
> >>>> back to this bug at the end of next week after returning from travel.
> >>>
> >>> Mauro, WDYT about this?
> >>
> >> I guess you're asking me about actioning the "More" button when point is
> >> past it, to show the rest of the documentation.
> >>
> >> There's already code that handles custom group links that way, in
> >> Custom-newline (RET is bound to that command).  For example:
> >> emacs -Q
> >> M-x customize-group RET vc
> >> C-s Log-Edit RET
> >> RET
> >>
> >> Emacs follows the link even though point is past the link.
> >>
> >> It should be easy to extend it to other links/buttons.
> >
> > Like in the attached patch, lightly tested.  I can post it in a
> > different bug report if desired.
> >
> > From e83562f8050118319f701fa45970a721afc44b36 Mon Sep 17 00:00:00 2001
> > From: Mauro Aranda <maurooaranda <at> gmail.com>
> > Date: Sat, 15 Feb 2025 09:43:58 -0300
> > Subject: [PATCH] Action button when point is just after it
> >
> > * lisp/cus-edit.el (Custom-newline): If no button at point,
> > check for a button before point.  (Bug#72341)
> > ---
> >  lisp/cus-edit.el | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
> > index febbc8d1b8b..1fd87c4e9f3 100644
> > --- a/lisp/cus-edit.el
> > +++ b/lisp/cus-edit.el
> > @@ -5344,10 +5344,13 @@ Custom-newline
> >  To see what function the widget will call, use the
> >  `widget-describe' command."
> >    (interactive "@d")
> > -  (let ((button (get-char-property pos 'button)))
> > -    ;; If there is no button at point, then use the one at the start
> > -    ;; of the line, if it is a custom-group-link (bug#2298).
> > +  (let ((button (or (get-char-property pos 'button)
> > +                    ;; Maybe we are just past a button, and it's quite handy
> > +                    ;; to action it as well.  (Bug#72341)
> > +                    (get-char-property (1- pos) 'button))))
> >      (or button
> > +        ;; If there is no button at point, then use the one at the start
> > +        ;; of the line, if it is a custom-group-link (bug#2298).
> >  	(if (setq button (get-char-property (line-beginning-position) 'button))
> >  	    (or (eq (widget-type button) 'custom-group-link)
> >  		(setq button nil))))
> 
> -- 
> Sean Whitton

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Christoph Badura <bad <at> bsd.de>
Cc: 72341 <at> debbugs.gnu.org, spwhitton <at> spwhitton.name
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Thu, 06 Mar 2025 16:23:36 +0200

> Date: Thu, 6 Mar 2025 11:49:25 +0100
> From: Christoph Badura <bad <at> bsd.de>
> Cc: Sean Whitton <spwhitton <at> spwhitton.name>, Eli Zaretskii <eliz <at> gnu.org>
> 
> I stripped the Cc:s because I was hoping that debbugs would cc: all
> parties involved in the bug report automatically.

No, it won't.  It is best to Reply All.  (Some people subscribe to the
bug list, so they get everything posted, but it is best not to rely on
that.)

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72341 <at> debbugs.gnu.org,
 Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 07 Mar 2025 10:49:00 +0800

Hello,

On Thu 06 Mar 2025 at 01:54pm +01, Christoph Badura wrote:

> Hi,
>
> Sure, I'll test it.
>
> I wanted to check this more carefully.  I see the same behaviour for links
> in help-mode buffers as created by e.g. describe-variable, and
> describe-function.  But I can't rember if I checked that on a build from
> the lastest git sources.
>
> Maybe this should be moved to a separate bug report?  I would prefer that.

Yes, fine, want to do it, or shall I?

-- 
Sean Whitton

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Mauro Aranda <maurooaranda <at> gmail.com>,
 Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 7 Mar 2025 10:13:23 +0100

On Fri, Mar 07, 2025 at 10:49:00AM +0800, Sean Whitton wrote:
> On Thu 06 Mar 2025 at 01:54pm +01, Christoph Badura wrote:
> > Maybe this should be moved to a separate bug report?  I would prefer that.
> Yes, fine, want to do it, or shall I?

I can do it later today.

I guess I'll just submit a new bug report and refer to the specific
subthread in this discussion.  I'll do that unless you tell me to do
something different.

--chris

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72341 <at> debbugs.gnu.org,
 Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 07 Mar 2025 19:52:56 +0800

Hello,

On Fri 07 Mar 2025 at 10:13am +01, Christoph Badura wrote:

> On Fri, Mar 07, 2025 at 10:49:00AM +0800, Sean Whitton wrote:
>> On Thu 06 Mar 2025 at 01:54pm +01, Christoph Badura wrote:
>> > Maybe this should be moved to a separate bug report?  I would prefer that.
>> Yes, fine, want to do it, or shall I?
>
> I can do it later today.
>
> I guess I'll just submit a new bug report and refer to the specific
> subthread in this discussion.  I'll do that unless you tell me to do
> something different.

Might be a good idea to include a copy of Mauro's patch in the new bug
report, but yeah, what you say is fine too.

-- 
Sean Whitton

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>, Christoph Badura <bad <at> bsd.de>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72341 <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 7 Mar 2025 08:59:24 -0300

On 7/3/25 08:52, Sean Whitton wrote:
> Hello,
>
> On Fri 07 Mar 2025 at 10:13am +01, Christoph Badura wrote:
>
>> On Fri, Mar 07, 2025 at 10:49:00AM +0800, Sean Whitton wrote:
>>> On Thu 06 Mar 2025 at 01:54pm +01, Christoph Badura wrote:
>>>> Maybe this should be moved to a separate bug report?  I would 
prefer that.
>>> Yes, fine, want to do it, or shall I?
>>
>> I can do it later today.
>>
>> I guess I'll just submit a new bug report and refer to the specific
>> subthread in this discussion.  I'll do that unless you tell me to do
>> something different.
>
> Might be a good idea to include a copy of Mauro's patch in the new bug
> report, but yeah, what you say is fine too.

I can re-post it once I see the bug report, no problem.

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Mauro Aranda <maurooaranda <at> gmail.com>,
 Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Fri, 7 Mar 2025 21:48:20 +0100

On Fri, Mar 07, 2025 at 10:13:23AM +0100, Christoph Badura wrote:
> I can do it later today.

Sorry, I ran into other bugs not directly related to emacs that prevented
me from testing and I ran out of time to day.  I'll take care of it
tomorrow.

--chris

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Mauro Aranda <maurooaranda <at> gmail.com>,
 Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 8 Mar 2025 15:06:50 +0100

I've tested Mauro's patch and it works fine for me.

I've also checked out the behaviour in the help-mode buffers and that
seems more difficult to address.  I'll handle that separately.

I think at this point it is not really worthwhile to split Mauro's patch
out into a separate bug report, unless you guys give a good reason to do
so.

--chris

On Wed, Mar 05, 2025 at 07:53:34PM +0800, Sean Whitton wrote:
> Hello,
> 
> Christoph, would you mind testing Mauro's patch here?
> 
> I'm keen not to let your valuable feedback on the customize UX, and
> Mauro's fix, get lost in this VC bug.
> 
> Eli, did you have any feedback on Mauro's patch?
> 
> On Sat 15 Feb 2025 at 09:47am -03, Mauro Aranda wrote:
> 
> > Mauro Aranda <maurooaranda <at> gmail.com> writes:
> >
> >> Eli Zaretskii <eliz <at> gnu.org> writes:
> >>
> >>>> Date: Tue, 28 Jan 2025 23:47:56 +0100
> >>>> From: Christoph Badura <bad <at> bsd.de>
> >>>>
> >>>> On Tue, Jan 28, 2025 at 02:23:39PM +0200, Eli Zaretskii wrote:
> >>>> > The Customize UI will be more helpful if we explicitly mention the
> >>>> > recommended values together with descriptive tags.
> >>>>
> >>>> So, I've just tested this:
> >>>>   emacs -nw --eval "(customize-group 'log-edit)"
> >>>>
> >>>> Search for "done hook".
> >>>> Be sure to toggle the "More" button and than the "Show value" button on
> >>>> the line for "Log Edit Done Hook".
> >>>> With this patch applied there's even a "More" button at the end of the
> >>>> one-line description of the log-edit-done-strip-cvs-lines option.
> >>>>
> >>>> Do you really think that UX needs improving?
> >>>>
> >>>> The only thing that annoys me is that when I search for "More", the
> >>    point
> >>>> ends up just behind the button and RET doesn't toggle it.  Whereas
> >>    it does
> >>>> when it is right before a button.  Fixing that would be a huge
> >>    improvement
> >>>> for me.  But this is a separate issue.
> >>>>
> >>>> > But I realize that this is a separate issue, so maybe it should be the
> >>>> > subject of a separate patch.  So maybe just mentioning the 3
> >>>> > recommended values in the doc string will be enough for fixing this
> >>>> > issue.
> >>>>
> >>>> I've made a note in any case.
> >>>>
> >>>> I've way overrun my "emacs hacking" time budget for this week.  I'll get
> >>>> back to this bug at the end of next week after returning from travel.
> >>>
> >>> Mauro, WDYT about this?
> >>
> >> I guess you're asking me about actioning the "More" button when point is
> >> past it, to show the rest of the documentation.
> >>
> >> There's already code that handles custom group links that way, in
> >> Custom-newline (RET is bound to that command).  For example:
> >> emacs -Q
> >> M-x customize-group RET vc
> >> C-s Log-Edit RET
> >> RET
> >>
> >> Emacs follows the link even though point is past the link.
> >>
> >> It should be easy to extend it to other links/buttons.
> >
> > Like in the attached patch, lightly tested.  I can post it in a
> > different bug report if desired.
> >
> > From e83562f8050118319f701fa45970a721afc44b36 Mon Sep 17 00:00:00 2001
> > From: Mauro Aranda <maurooaranda <at> gmail.com>
> > Date: Sat, 15 Feb 2025 09:43:58 -0300
> > Subject: [PATCH] Action button when point is just after it
> >
> > * lisp/cus-edit.el (Custom-newline): If no button at point,
> > check for a button before point.  (Bug#72341)
> > ---
> >  lisp/cus-edit.el | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
> > index febbc8d1b8b..1fd87c4e9f3 100644
> > --- a/lisp/cus-edit.el
> > +++ b/lisp/cus-edit.el
> > @@ -5344,10 +5344,13 @@ Custom-newline
> >  To see what function the widget will call, use the
> >  `widget-describe' command."
> >    (interactive "@d")
> > -  (let ((button (get-char-property pos 'button)))
> > -    ;; If there is no button at point, then use the one at the start
> > -    ;; of the line, if it is a custom-group-link (bug#2298).
> > +  (let ((button (or (get-char-property pos 'button)
> > +                    ;; Maybe we are just past a button, and it's quite handy
> > +                    ;; to action it as well.  (Bug#72341)
> > +                    (get-char-property (1- pos) 'button))))
> >      (or button
> > +        ;; If there is no button at point, then use the one at the start
> > +        ;; of the line, if it is a custom-group-link (bug#2298).
> >  	(if (setq button (get-char-property (line-beginning-position) 'button))
> >  	    (or (eq (widget-type button) 'custom-group-link)
> >  		(setq button nil))))
> 
> -- 
> Sean Whitton

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

From: Christoph Badura <bad <at> bsd.de>
To: 72341 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sat, 8 Mar 2025 15:45:11 +0100

[Message part 1 (text/plain, inline)]

Hi,

attached is v3 of the patch with all the feedback incorporated.  And two
more nits fixed:

- use TAB for indent in the options list for the log-edit-hook defcustom.
- update the docstring of log-edit-done-strip-cvs-lines-helper to describe
  all arguments.

--chris

On Wed, Mar 05, 2025 at 07:44:33PM +0800, Sean Whitton wrote:
> Hello Christoph,
> 
> Thanks.  Let me first ask you, how is the copyright paperwork going?
> 
> On Tue 18 Feb 2025 at 12:54am +01, Christoph Badura wrote:
> 
> > I've just noticed that, e.g. the lines following
> > the "hook :options" in the log-edit-done-hook
> > defcustom, use TAB for indentation while indent-tabs-mode is turned off in
> > the top-level .dir-locals.el.  Do you care for consistency with the old
> > code?  This affects only a very small number of lines.
> 
> No, it's fine to convert them to being indented with only spaces if
> you're editing them.  (We just don't do mass conversions.)
> 
> > Add a hook function to strip all lines beginning with "CVS:" from the
> > commit message as CVS does.  Do this only if 'log-edit-vc-backend' is
> > 'CVS'.  (Bug#72341)
> > * lisp/vc/log-edit.el (log-edit-done-strip-cvs-lines)
> > Add a hook function to strip the lines starting with "CVS:".
> 
> In the GNU changelog style, please use
> 
> * lisp/vc/log-edit.el
> (log-edit-done-strip-cve-lines): New command.
> (log-edit-done-hook): Add it as an option.
> 
> > * test/lisp/vc/log-edit-tests.el (log-edit-done-strip-cvs-lines-helper,
> 
> Similarly, just "New function."
> 
> > log-edit-done-strip-cvs-lines-cvs, log-edit-done-strip-cvs-lines-non-cvs,
> > log-edit-done-strip-cvs-lines-only-cvs-colon-blank,
> > log-edit-done-strip-cvs-lines-only-cvs-colon):
> > Add test cases for 'log-edit-done-strip-cvs-lines'.
> 
> Similarly, just "New test cases."
> 
> > +(defun log-edit-done-strip-cvs-lines ()
> > +  "Strip lines starting with \"CVS:\" from commit log message.
> > +When not called interactively do this only when the VC backend is CVS.
> > +This mimicks what CVS does when invoked as 'cvs commit [files...]'"
> > +  (interactive)
> > +  (when (or (called-interactively-p 'interactive)
> > +            (eq log-edit-vc-backend 'CVS))
> 
> Take a look at the docstring for called-interactively-p.
> It explains that it is better to do something like this:
> 
>     (defun log-edit-done-strip-cvs-lines (&optional interactive)
>       (interactive "P")
>       (when (or interactive (eq log-edit-vc-backend)) ...
> 
> > --- a/test/lisp/vc/log-edit-tests.el
> > +++ b/test/lisp/vc/log-edit-tests.el
> > @@ -360,4 +360,62 @@ log-edit-fill-entry-no-defun-list-wrapping
> >        (let ((fill-column 64)) (log-edit-fill-entry))
> >        (should (equal (buffer-string) wanted)))))
> >
> > +(defun log-edit-done-strip-cvs-lines-helper (initial-text wanted vc-backend)
> > +  "Test that running log-edit-done-strip-cvs-lines as a
> > +log-edit-done-hook produces the WANTED
> > +string when run on INITIAL-TEXT."
> 
> It's not such an issue for test helper functions, but it would be better
> to follow our convention of the first line of the docstring being a
> complete sentence.
> 
> > +(ert-deftest log-edit-done-strip-cvs-lines-cvs ()
> > +  "Strip lines beginning with \"CVS:\" when using CVS as VC backend."
> > +  ;; This test verifies that lines beginning with "CVS:" are stripped
> > +  ;; from the commit message when log-edit-vc-backend is CVS.
> 
> I would suggest merging the comment into the docstring, or just deleting
> the docstring and just having a comment.
> Less for someone to have to read.
> 
> Otherwise, all looks good to me.  Looking forward to applying it.
> 
> -- 
> Sean Whitton

[0001-VC-add-hook-to-strip-CVS-template-lines-when-committ-v3.patch (text/plain, attachment)]

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Christoph Badura <bad <at> bsd.de>, Mauro Aranda <maurooaranda <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72341-done <at> debbugs.gnu.org
Subject: Re: bug#72341: VC: CVS template lines not stripped when committing
Date: Sun, 09 Mar 2025 14:28:42 +0800

Version: 31.1

Hello,

On Thu 06 Mar 2025 at 11:49am +01, Christoph Badura wrote:

>> Take a look at the docstring for called-interactively-p.
>> It explains that it is better to do something like this:
>>
>>     (defun log-edit-done-strip-cvs-lines (&optional interactive)
>>       (interactive "P")
>>       (when (or interactive (eq log-edit-vc-backend)) ...
>
> Sure, I'm happy to do that.
>
> Frankly, I just copyied the code from the other log-edit-* hooks in
> log-edit.el without giving it further thought.

I slightly misinformed you here.  It's (interactive "p") that's needed.
I've fixed it in your patch as installed.

I also moved the NEWS entry to the already-existent VC section, fixed a
typo and a tiny compiler error.

> That begs the question whether you want the other places in that file
> adjusted to this pattern too?  Presumably as a separate patch.

It'd be welcome, if you'd like to work on that.

Now installed both patches, and closing the bug.  Thanks all.

-- 
Sean Whitton

Previous Next

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