Max Nikulin <manikulin@gmail.com> writes:

> On 22/08/2023 16:46, Ihor Radchenko wrote:
>> See the updated version of the patches attached.
>
> Thank you, I do not see apparent issues with code any more. Commit 
> message needs an update, apostrophes in the doc string should be 
> escaped. Feel free to ignore other comments since there are other issues 
> and investing excessive time into polishing of this one is not reasonable.

Thanks for the feedback!
I have updated the patch, except for the comments I reply to below.

>> +  `(org-make-shell-command \"command\" \"-l\"
>> +      \"value with spaces\"
>> +      (,org-shell-arg-tag-unescaped \"$HOME\")
>> +      (mapcar #'identity files)))
>
> Is `mapcar' necessary here? Anyway `delq' is called on another result of 
> `mapcar', so the function should not do any destructive list modification.

The idea was to highlight that `files' is a list.
I now changed this to

 files ; list variable

> An idea that may be ignored: make the constant internal and add
> (defsubst org-make-shell-command-unescaped (arg)
>   (list org--shell-arg-tag-unescaped arg))
>
> to avoid `, noise in `(,org-shell-arg-tag-unescaped STRING).

Good idea. I also converted `org-make-shell-command' into defsubst that
cannot be reliably adviced. To reduce attack vectors further.

>> +will shell-escape \"-l\", \"value with spaces\", and each non-nil member of
>
> There is nothing to escape in "-l".

I deliberately list all the arguments, detailing which are escaped and
which are not.

> Perhaps it deserves a mention that COMMAND is passed unquoted to be 
> suitable for commands with arguments as defcustom user option values. To 
> escape it pass nil as fist argument and add COMMAND before ARGS.

>> -	(org-fill-template
>
> Should an explicit warning be added to `org-fill-template' that enough 
> care is required to escape values if it is used to build a shell command?

I don't think so. `org-fill-template' is usually not used to build shell
command. ob-sqlite is the only instance of such use in Org code. Other
backends use different Elisp means to build shell command strings. So,
adding warning to `org-fill-template' docstring will not achieve much.

The new version of the org-macs patch attached.