From: Aaron Ecay <aaronecay@gmail.com>
To: "Göktuğ Kayaalp" <self@gkayaalp.com>,
"Nicolas Goaziou" <mail@nicolasgoaziou.fr>
Cc: emacs-orgmode@gnu.org
Subject: Re: Inheriting some local variables from source code block editing buffers
Date: Mon, 21 May 2018 15:20:55 +0100 [thread overview]
Message-ID: <87a7st5bug.fsf@gmail.com> (raw)
In-Reply-To: <ygmwow0ocsa.fsf@gkayaalp.com>
Hi Göktuğ,
A couple of comments:
2018ko maiatzak 18an, Göktuğ 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 ‘risky-local-variable-p’
> +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 ‘%S’, 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 “else” 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ʼt have to worry about
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.
--
Aaron Ecay
next prev parent reply other threads:[~2018-05-21 14:21 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-29 17:19 Inheriting some local variables from source code block editing buffers Göktuğ Kayaalp
2018-04-29 22:09 ` Bastien
2018-05-01 3:30 ` Göktuğ Kayaalp
2018-05-01 8:43 ` Nicolas Goaziou
2018-05-01 8:45 ` Nicolas Goaziou
2018-05-01 11:41 ` Göktuğ Kayaalp
2018-05-01 11:55 ` Nicolas Goaziou
[not found] ` <ygmvac7lcl1.fsf@gkayaalp.com>
2018-05-01 14:00 ` Nicolas Goaziou
2018-05-01 16:37 ` Aaron Ecay
[not found] ` <ygmin87kwua.fsf@gkayaalp.com>
2018-05-01 19:35 ` Aaron Ecay
2018-05-01 20:04 ` Göktuğ Kayaalp
2018-05-01 20:53 ` Aaron Ecay
2018-05-01 22:12 ` Göktuğ Kayaalp
2018-05-01 22:19 ` Aaron Ecay
2018-05-01 22:26 ` Göktuğ Kayaalp
2018-05-02 10:16 ` Nicolas Goaziou
2018-05-02 19:52 ` Göktuğ Kayaalp
2018-05-01 11:45 ` Göktuğ Kayaalp
2018-05-14 5:44 ` Göktuğ Kayaalp
2018-05-14 12:13 ` Nicolas Goaziou
2018-05-14 16:34 ` Göktuğ Kayaalp
2018-05-14 16:47 ` Nicolas Goaziou
2018-05-15 18:36 ` Göktuğ Kayaalp
2018-05-18 21:48 ` Göktuğ Kayaalp
2018-05-19 12:26 ` Nicolas Goaziou
2018-05-21 14:20 ` Aaron Ecay [this message]
2018-05-14 13:33 ` Aaron Ecay
2018-05-14 16:46 ` Göktuğ Kayaalp
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87a7st5bug.fsf@gmail.com \
--to=aaronecay@gmail.com \
--cc=emacs-orgmode@gnu.org \
--cc=mail@nicolasgoaziou.fr \
--cc=self@gkayaalp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).