From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aaron Ecay Subject: Re: Inheriting some local variables from source code block editing buffers Date: Mon, 21 May 2018 15:20:55 +0100 Message-ID: <87a7st5bug.fsf@gmail.com> References: <87bmdizb7k.fsf@nicolasgoaziou.fr> <87d0xyxjyf.fsf@nicolasgoaziou.fr> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:46931) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fKlgM-0002yC-J2 for emacs-orgmode@gnu.org; Mon, 21 May 2018 10:21:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fKlgJ-0000J0-EE for emacs-orgmode@gnu.org; Mon, 21 May 2018 10:21:02 -0400 Received: from mail-wr0-x236.google.com ([2a00:1450:400c:c0c::236]:35990) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fKlgJ-0000Ih-7R for emacs-orgmode@gnu.org; Mon, 21 May 2018 10:20:59 -0400 Received: by mail-wr0-x236.google.com with SMTP id k5-v6so4108800wrn.3 for ; Mon, 21 May 2018 07:20:59 -0700 (PDT) In-Reply-To: List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: "Emacs-orgmode" To: =?utf-8?B?R8O2a3R1xJ8=?= Kayaalp , Nicolas Goaziou Cc: emacs-orgmode@gnu.org Hi G=C3=B6ktu=C4=9F, A couple of comments: 2018ko maiatzak 18an, G=C3=B6ktu=C4=9F Kayaalp-ek idatzi zuen: [...] > +(defcustom org-src-apply-risky-edit-bindings 'ask > + "What to do if an edit binding is a risky local variable. > +If this is nil, bindings that satisfy =E2=80=98risky-local-variable-p=E2= =80=99 > +are skipped, with a warning message. Otherwise, its value should > +be a symbol telling how to thread them. Possible values of this > +setting are: > + > +nil Skip, warning the user via a message. > +skip-silent Skip risky local varibles silently. > +ask Prompt user for each variable. > +t Apply the variable but show a warning. > +apply-silent Apply risky local variables silently." It would be more consistent/less confusing to use skip-warn and apply-warn instead of nil and t. It would also lead to fewer bugs in the sense that: [...] > + (cond ((or (and (eq org-src-apply-risky-edit-bindings 'ask) > + (y-or-n-p (format prompt-apply name value))) > + (eq org-src-apply-risky-edit-bindings 'apply-silent)) > + (funcall apply-binding)) > + (org-src-apply-risky-edit-bindings > + (prog1 > + (funcall apply-binding) > + (message risky-message "Applied" name))) > + ((not org-src-apply-risky-edit-bindings) > + (message risky-message "Skipped" name)) > + ((eq org-src-apply-risky-edit-bindings 'skip-silent)) > + ('else > + (user-error > + "Unexpected value for =E2=80=98%S=E2=80=99, will not apply this or = any more bindings." > + 'org-src-apply-risky-edit-bindings)))) If I am reading the above cond correctly, then the (eq org-src-apply-risky-edit-bindings 'skip-silent) branch will never be reached, because it will be eaten by the second branch. (And so will the =E2=80=9Celse=E2=80=9D branch, for that matter.) IMO you should use cl-case instead of cond, which will be less conducive to subtle problems like this and make it clear that the possible values are exhaustively handled. (That approach will mean shuffling the y-or-n-p stuff around slightly). [...] > +(defun org-src--parse-edit-bindings (sexp-str pos-beg pos-end) > + ;; XXX: require cadr of the varlist items to be atoms, for security? > + ;; Or prompt users? Because otherwise there can be complete > + ;; programs embedded in there. Maybe instead of using risky-local-variable-p above, this feature should use safe-local-variable-p instead. Then we wouldn=CA=BCt have to worry abo= ut the security implications of allowing non-atom values. It seems like a version of this feature that only worked for atoms would be quite limited in functionality. In that case, we should probably call the defcustom above ...apply-unsafe-edit-bindings for the sake of accuracy. --=20 Aaron Ecay