Eli Zaretskii writes: >> From: Mauro Aranda >> Date: Tue, 31 Dec 2019 14:21:09 -0300 >> Cc: 38812@debbugs.gnu.org >> >> > And I have a question: isn't it better not to use setcar in >> > custom-push-theme instead? >> >> I thought of doing that, and use setf with alist-get to make the change >> instead. But I think we'll be better off if we avoid sharing the cons >> cell inadvertedly, since that is prone to have bugs like this one. > > And I actually think that a problem should be fixed where it is > caused. There's nothing wrong per se with sharing portions of Lisp > data structures. Of course. But I said "inadvertedly". Now we are aware. >> Alternatively, we could create the list in custom-theme-recalc-variable, >> to accomplish the same thing without changing the return value of >> custom-variable-theme-value. In that case, I think it would be >> convenient to change the doc string of custom-variable-theme-value, to >> say it returns some cdr. >> >> To me, either the patch I posted (with an additional explanatory >> comment, of course) or the latter option sound better, but I won't argue >> too much if you think otherwise. > > My alternative patch is below. WDYT? > > diff --git a/lisp/custom.el b/lisp/custom.el > index 26bdaae2c2..7ed85b22e8 100644 > --- a/lisp/custom.el > +++ b/lisp/custom.el > @@ -886,7 +886,10 @@ custom-push-theme > (put theme 'theme-settings > (cons (list prop symbol theme value) > (delq res theme-settings))) > - (setcar (cdr setting) value))) > + ;; It's tempting to use setcar here, but that could > + ;; inadvertently modify other properties in SYMBOL's proplist, > + ;; if those just happen to share elements with the value of PROP. > + (put symbol prop (cons (list theme value) (delq setting old))))) > ;; Add a new setting: > (t > (when (custom--should-apply-setting theme) Looks good, thank you.