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