Re: Inheriting some local variables from source code block editing buffers

[Aaron Ecay <aaronecay@gmail.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