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

Previous Next

Full log

View this message in rfc822 format

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: 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

Previous Next

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