[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: greedy substitution in org-open-file

From: Maxim Nikulin
Subject: Re: greedy substitution in org-open-file
Date: Tue, 16 Feb 2021 00:04:57 +0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

On 13/02/2021 11:38, Kyle Meyer wrote:

All right, here's
a format-spec-inspired fix.  At the very least it needs doc updates and
a comment or two.

Thank you. I am hardly familiar with elisp so it would be difficult for me to express the same. My comments are mostly a matter of taste.

Sorry, I have not tried to run the code yet.

diff --git a/lisp/org.el b/lisp/org.el
index 5b1443c4e..e8f60fd83 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -8644,6 +8644,23 @@ (defun org--file-apps-regexp-alist (list &optional 
     (when add-auto-mode
       (mapcar (lambda (x) (cons (car x) 'emacs)) auto-mode-alist))))
+(defun org--open-file-format-spec (format specification)
+  (with-temp-buffer
+    (insert format)
+    (goto-char (point-min))
+    (while (search-forward "%" nil t)
+      (cond ((eq (char-after) ?%)
+             (delete-char 1))
+            ((looking-at "[s0-9]")

Personally I do not see a reason to limit substitutions just to just "%s". I would consider "[a-zA-Z0-9]".

+             (replace-match
+              (or (cdr (assoc (match-string 0) specification))
+                  (error "Invalid format string"))

I think, substitution character in the error message could be useful during debugging, something like (format "Invalid format character %%%s" (match-string 0)).

+              'fixed-case 'literal)
+             (delete-region (1- (match-beginning 0)) (match-beginning 0)))
+            (t
+             (error "Invalid format string"))))
+    (buffer-string)))
  (defun org-open-file (path &optional in-emacs line search)
    "Open the file at PATH.
@@ -8745,24 +8762,20 @@ (defun org-open-file (path &optional in-emacs line 
        ;; Remove quotes around the file name - we'll use shell-quote-argument.
        (while (string-match "['\"]%s['\"]" cmd)
        (setq cmd (replace-match "%s" t t cmd)))
-      (setq cmd (replace-regexp-in-string
-                "%s"
-                (shell-quote-argument (convert-standard-filename file))
-                cmd
-                nil t))
-      ;; Replace "%1", "%2" etc. in command with group matches from regex
-      (save-match-data
-       (let ((match-index 1)
-             (number-of-groups (- (/ (length link-match-data) 2) 1)))
-         (set-match-data link-match-data)
-         (while (<= match-index number-of-groups)
-           (let ((regex (concat "%" (number-to-string match-index)))
-                 (replace-with (match-string match-index dlink)))
-             (while (string-match regex cmd)
-               (setq cmd (replace-match replace-with t t cmd))))
-           (setq match-index (+ match-index 1)))))
+      (setq cmd
+            (org--open-file-format-spec
+             cmd
+             (cons
+              (cons "s" (shell-quote-argument
+                         (convert-standard-filename file)))
+              (let ((ngroups (- (/ (length link-match-data) 2) 1)))
+                (and (> ngroups 0)
+                     (progn
+                       (set-match-data link-match-data)
+                       (mapcar (lambda (n)
+                                 (cons (number-to-string n)
+                                       (match-string-no-properties n dlink)))

Should not be numbered substitutions passed through shell-quote-argument as well? Besides just page numbers the link could be named internal anchor where more characters are allowed. It is the reason why in general I prefer bare exec to shell commands.

I am unsure concerning optional matches as


Maybe they should not be used at all in favor of separate entries with and without page number. Maybe nil should coalesce to empty string "". Certainly I am against "nil" string for a missed group. By the way, in the original format-spec, cdr is applied after the check whether assoc value is nil.

+                               (number-sequence 1 ngroups))))))))
        (message "Running %s...done" cmd)
        (start-process-shell-command cmd nil cmd)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]