GNU bug report logs - #63204
29.0.90; vc-annotate does not properly test whether a face exists

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 63204 in the body.
You can then email your comments to 63204 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: Tobias Bading <tbading <at> web.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.90; vc-annotate does not properly test whether a face exits
Date: Mon, 1 May 2023 15:15:44 +0200

1. emacs -Q

2. Imagine that in a previous Emacs session you have copied a string with
   text properties from a buffer in vc-annotate-mode, maybe also searched
   for it somewhere. IOW, your ~/.emacs.d/.emacs.desktop file may contain
   something like this:

   (setq some-search-ring-or-whatever
         '(#("heisenbug" 0 9 (face vc-annotate-face-CCCCFF))))

   Evaluate this expression to simulate a desktop-read of such a previous
   session.

3. Open lisp/vc/vc-annotate.el from the Git branch emacs-29.

4. C-x v g (vc-annotate)

5. There are lines with white background caused by the use of undefined face
   vc-annotate-face-CCCCFF. C-h e should contain:

   Invalid face reference: vc-annotate-face-CCCCFF

   If for whatever reason you don’t see these bad lines, use C-u C-x = to
   see what face a line uses and repeat these steps with this face instead
   of vc-annotate-face-CCCCFF.


This can be fixed with:

diff --git a/lisp/vc/vc-annotate.el b/lisp/vc/vc-annotate.el
index 70057a6aac..ae8db021ad 100644
--- a/lisp/vc/vc-annotate.el
+++ b/lisp/vc/vc-annotate.el
@@ -724,7 +724,9 @@ vc-annotate-lines
                                       (substring (cdr color) 1)
                                     (cdr color))))
                ;; Make the face if not done.
-               (face (or (intern-soft face-name)
+               (face (or (let ((sym (intern-soft face-name)))
+                           (if (facep sym)
+                               sym))
                          (let ((tmp-face (make-face (intern face-name))))
                            (set-face-extend tmp-face t)
                            (cond

My Emacs Lisp isn’t that great so please feel free to improve this code
if needed. The important part is that (intern-soft face-name) is not
sufficient to check whether a face exists.

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Tobias Bading <tbading <at> web.de>
Cc: 63204 <at> debbugs.gnu.org
Subject: Re: bug#63204: 29.0.90; vc-annotate does not properly test whether
 a face exits
Date: Mon, 01 May 2023 18:32:49 +0000

Tobias Bading <tbading <at> web.de> writes:

> 1. emacs -Q
>
> 2. Imagine that in a previous Emacs session you have copied a string with
>    text properties from a buffer in vc-annotate-mode, maybe also searched
>    for it somewhere. IOW, your ~/.emacs.d/.emacs.desktop file may contain
>    something like this:
>
>    (setq some-search-ring-or-whatever
>          '(#("heisenbug" 0 9 (face vc-annotate-face-CCCCFF))))
>
>    Evaluate this expression to simulate a desktop-read of such a previous
>    session.
>
> 3. Open lisp/vc/vc-annotate.el from the Git branch emacs-29.
>
> 4. C-x v g (vc-annotate)
>
> 5. There are lines with white background caused by the use of undefined face
>    vc-annotate-face-CCCCFF. C-h e should contain:
>
>    Invalid face reference: vc-annotate-face-CCCCFF
>
>    If for whatever reason you don’t see these bad lines, use C-u C-x = to
>    see what face a line uses and repeat these steps with this face instead
>    of vc-annotate-face-CCCCFF.
>
>
> This can be fixed with:
>
> diff --git a/lisp/vc/vc-annotate.el b/lisp/vc/vc-annotate.el
> index 70057a6aac..ae8db021ad 100644
> --- a/lisp/vc/vc-annotate.el
> +++ b/lisp/vc/vc-annotate.el
> @@ -724,7 +724,9 @@ vc-annotate-lines
>                                        (substring (cdr color) 1)
>                                      (cdr color))))
>                 ;; Make the face if not done.
> -               (face (or (intern-soft face-name)
> +               (face (or (let ((sym (intern-soft face-name)))
> +                           (if (facep sym)
> +                               sym))

I'd write this as (and (facep sym) sym).

>                           (let ((tmp-face (make-face (intern face-name))))
>                             (set-face-extend tmp-face t)
>                             (cond
>
> My Emacs Lisp isn’t that great so please feel free to improve this code
> if needed. The important part is that (intern-soft face-name) is not
> sufficient to check whether a face exists.

I think a better solution would be to revise general approach
vc-annotate takes, and instead of hard-coding faces/colours to use (that
break when switching between light and dark themes), to have a list of
faces that could be used independently of vc-annotate.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: tbading <at> web.de, 63204 <at> debbugs.gnu.org
Subject: Re: bug#63204: 29.0.90;
 vc-annotate does not properly test whether a face exits
Date: Mon, 01 May 2023 21:50:02 +0300

> Cc: 63204 <at> debbugs.gnu.org
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Mon, 01 May 2023 18:32:49 +0000
> 
> Tobias Bading <tbading <at> web.de> writes:
> 
> > 1. emacs -Q
> >
> > 2. Imagine that in a previous Emacs session you have copied a string with
> >    text properties from a buffer in vc-annotate-mode, maybe also searched
> >    for it somewhere. IOW, your ~/.emacs.d/.emacs.desktop file may contain
> >    something like this:
> >
> >    (setq some-search-ring-or-whatever
> >          '(#("heisenbug" 0 9 (face vc-annotate-face-CCCCFF))))
> >
> >    Evaluate this expression to simulate a desktop-read of such a previous
> >    session.
> >
> > 3. Open lisp/vc/vc-annotate.el from the Git branch emacs-29.
> >
> > 4. C-x v g (vc-annotate)
> >
> > 5. There are lines with white background caused by the use of undefined face
> >    vc-annotate-face-CCCCFF. C-h e should contain:
> >
> >    Invalid face reference: vc-annotate-face-CCCCFF
> >
> >    If for whatever reason you don’t see these bad lines, use C-u C-x = to
> >    see what face a line uses and repeat these steps with this face instead
> >    of vc-annotate-face-CCCCFF.
> >
> >
> > This can be fixed with:
> >
> > diff --git a/lisp/vc/vc-annotate.el b/lisp/vc/vc-annotate.el
> > index 70057a6aac..ae8db021ad 100644
> > --- a/lisp/vc/vc-annotate.el
> > +++ b/lisp/vc/vc-annotate.el
> > @@ -724,7 +724,9 @@ vc-annotate-lines
> >                                        (substring (cdr color) 1)
> >                                      (cdr color))))
> >                 ;; Make the face if not done.
> > -               (face (or (intern-soft face-name)
> > +               (face (or (let ((sym (intern-soft face-name)))
> > +                           (if (facep sym)
> > +                               sym))
> 
> I'd write this as (and (facep sym) sym).
> 
> >                           (let ((tmp-face (make-face (intern face-name))))
> >                             (set-face-extend tmp-face t)
> >                             (cond
> >
> > My Emacs Lisp isn’t that great so please feel free to improve this code
> > if needed. The important part is that (intern-soft face-name) is not
> > sufficient to check whether a face exists.
> 
> I think a better solution would be to revise general approach
> vc-annotate takes, and instead of hard-coding faces/colours to use (that
> break when switching between light and dark themes), to have a list of
> faces that could be used independently of vc-annotate.

We should probably do that on master, but for emacs-29 this is too
much of a churn, I think.  So for emacs-29, I think we should install
something like the proposed patch, just a bit cleaned-up.

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: tbading <at> web.de, 63204 <at> debbugs.gnu.org
Subject: Re: bug#63204: 29.0.90; vc-annotate does not properly test whether
 a face exits
Date: Mon, 01 May 2023 19:12:42 +0000

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

>> Cc: 63204 <at> debbugs.gnu.org
>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Date: Mon, 01 May 2023 18:32:49 +0000
>> 
>> Tobias Bading <tbading <at> web.de> writes:
>> 
>> > 1. emacs -Q
>> >
>> > 2. Imagine that in a previous Emacs session you have copied a string with
>> >    text properties from a buffer in vc-annotate-mode, maybe also searched
>> >    for it somewhere. IOW, your ~/.emacs.d/.emacs.desktop file may contain
>> >    something like this:
>> >
>> >    (setq some-search-ring-or-whatever
>> >          '(#("heisenbug" 0 9 (face vc-annotate-face-CCCCFF))))
>> >
>> >    Evaluate this expression to simulate a desktop-read of such a previous
>> >    session.
>> >
>> > 3. Open lisp/vc/vc-annotate.el from the Git branch emacs-29.
>> >
>> > 4. C-x v g (vc-annotate)
>> >
>> > 5. There are lines with white background caused by the use of undefined face
>> >    vc-annotate-face-CCCCFF. C-h e should contain:
>> >
>> >    Invalid face reference: vc-annotate-face-CCCCFF
>> >
>> >    If for whatever reason you don’t see these bad lines, use C-u C-x = to
>> >    see what face a line uses and repeat these steps with this face instead
>> >    of vc-annotate-face-CCCCFF.
>> >
>> >
>> > This can be fixed with:
>> >
>> > diff --git a/lisp/vc/vc-annotate.el b/lisp/vc/vc-annotate.el
>> > index 70057a6aac..ae8db021ad 100644
>> > --- a/lisp/vc/vc-annotate.el
>> > +++ b/lisp/vc/vc-annotate.el
>> > @@ -724,7 +724,9 @@ vc-annotate-lines
>> >                                        (substring (cdr color) 1)
>> >                                      (cdr color))))
>> >                 ;; Make the face if not done.
>> > -               (face (or (intern-soft face-name)
>> > +               (face (or (let ((sym (intern-soft face-name)))
>> > +                           (if (facep sym)
>> > +                               sym))
>> 
>> I'd write this as (and (facep sym) sym).
>> 
>> >                           (let ((tmp-face (make-face (intern face-name))))
>> >                             (set-face-extend tmp-face t)
>> >                             (cond
>> >
>> > My Emacs Lisp isn’t that great so please feel free to improve this code
>> > if needed. The important part is that (intern-soft face-name) is not
>> > sufficient to check whether a face exists.
>> 
>> I think a better solution would be to revise general approach
>> vc-annotate takes, and instead of hard-coding faces/colours to use (that
>> break when switching between light and dark themes), to have a list of
>> faces that could be used independently of vc-annotate.
>
> We should probably do that on master, but for emacs-29 this is too
> much of a churn, I think.  So for emacs-29, I think we should install
> something like the proposed patch, just a bit cleaned-up.

I agree, the change that Tobias proposed looks like the right approach
for the immediate issue, I just wanted to mention the idea, since I have
a patch or a stash somewhere I wanted to suggest adding a while back.

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: 63204-done <at> debbugs.gnu.org
Subject: Re: bug#63204: 29.0.90; vc-annotate does not properly test whether
 a face exits
Date: Fri, 14 Mar 2025 12:05:30 +0800

Version 30.1

This was fixed in a different way by Mattias in commit d082f46c8cac.

-- 
Sean Whitton

Previous Next

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