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

Previous Next

Full log

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

Previous Next

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