Subject: Re: bug#72341: VC: CVS template lines not stripped when committing

From: Christoph Badura <[email-masked]>
Date: Sat, 8 Mar 2025 15:45:11 +0100

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
>From d72d34273ed8714f52685d09c282808eed31ce26 Mon Sep 17 00:00:00 2001
From: Christoph Badura <[email-masked]>
Date: Sun, 26 Jan 2025 22:48:11 +0100
Subject: [PATCH v3] VC: add hook to strip CVS template lines when committing

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): 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): 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): New test cases.

* etc/NEWS:
Mention log-edit-done-strip-cvs-lines.
---
 etc/NEWS                       |  9 ++++++
 lisp/vc/log-edit.el            | 17 ++++++++++-
 test/lisp/vc/log-edit-tests.el | 53 ++++++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 258f0dcc4ba..607708b4680 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -458,8 +458,17 @@ based on the state of the buffer, such as for the different states of
 modal editing packages.
 
 
+
 * Changes in Specialized Modes and Packages in Emacs 31.1
 
+** 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 invoking "cvs commit [files...]" from the
+command line.
+
 ---
 ** Text mode
 
diff --git a/lisp/vc/log-edit.el b/lisp/vc/log-edit.el
index e23e7414a18..1899907a4e7 100644
--- a/lisp/vc/log-edit.el
+++ b/lisp/vc/log-edit.el
@@ -210,7 +210,8 @@ log-edit-done-hook
 can be obtained from `log-edit-files'."
   :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 +927,20 @@ log-edit-insert-cvs-template
       (goto-char (point-max))
       (insert-file-contents "CVS/Template"))))
 
+(defun log-edit-done-strip-cvs-lines (&optional interactive)
+  "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 "P")
+  (when (or interactive (eq log-edit-vc-backend 'CVS))
+    (let ((case-fold-search nil))
+      (goto-char (point-min))
+      ;; NB: While CVS defines CVSEDITPREFIX as "CVS: " it actually
+      ;; checks only the first four characters of af a line, i.e. "CVS:"
+      ;; do deal with editors that strip trailing whitespace.
+      ;; c.f. src/cvs.h and src/logmsg.c:do_editor()
+      (flush-lines "^CVS:"))))
+
 (defun log-edit-insert-cvs-rcstemplate ()
   "Insert the RCS commit log template from the CVS repository.
 This contacts the repository to get the rcstemplate file and
diff --git a/test/lisp/vc/log-edit-tests.el b/test/lisp/vc/log-edit-tests.el
index 005c336a3b6..6a312c6dac7 100644
--- a/test/lisp/vc/log-edit-tests.el
+++ b/test/lisp/vc/log-edit-tests.el
@@ -360,4 +360,57 @@ 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)
+  "Helper function for the log-edit-done-strip-cvs-lines tests.
+Tests that running log-edit-done-strip-cvs-lines as a log-edit-done-hook
+produces the WANTED string when run on INITIAL-TEXT with
+'log-edit-vc-backend' set to VC-BACKEND.\""
+  (with-temp-buffer
+      (let ((log-edit-done-hook 'log-edit-done-strip-cvs-lines)
+            (log-edit-vc-backend vc-backend))
+        (setq-local log-edit-callback #'(lambda () (interactive) nil))
+        (insert initial-text)
+        (log-edit-done)
+        (should (equal (buffer-string) wanted)))))
+
+(ert-deftest log-edit-done-strip-cvs-lines-cvs ()
+  "Strip lines beginning with \"CVS:\" when using CVS as VC backend."
+  (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
+")
+    (log-edit-done-strip-cvs-lines-helper string wanted 'CVS)))
+
+(ert-deftest log-edit-done-strip-cvs-lines-non-cvs ()
+  "Do not strip lines beginning with \"CVS:\" when not using CVS as VC backend."
+  (let (string)
+    (setq string "summary line
+first line
+CVS: Please evaluate your changes and consider the following.
+CVS: Abort checkin if you answer no.
+")
+  (log-edit-done-strip-cvs-lines-helper string string nil)))
+
+(ert-deftest log-edit-done-strip-cvs-lines-only-cvs-colon-blank ()
+  "Strip lines that contain solely \"CVS: \" when using CVS as VC backend."
+  (let (string wanted)
+    (setq string "CVS: \n"
+          wanted "")
+    (log-edit-done-strip-cvs-lines-helper string wanted 'CVS)))
+
+(ert-deftest log-edit-done-strip-cvs-lines-only-cvs-colon ()
+  "Strip lines that contain solely \"CVS:\" when using CVS as VC backend."
+  ;; This test verifies that lines consisting only of "CVS:" (no blank
+  ;; after the colon) are stripped from the commit message.
+  ;; CVS does this to accomodate editors that delete trailing whitespace.
+  (let (string wanted)
+    (setq string "CVS:\n"
+          wanted "")
+    (log-edit-done-strip-cvs-lines-helper string wanted 'CVS)))
+
 ;;; log-edit-tests.el ends here
-- 
2.48.1