* Re: [PATCH v2] org.el: Preserve case for link subgroups from `org-file-apps'
2022-09-06 12:07 ` Ihor Radchenko
@ 2022-09-08 14:37 ` Max Nikulin
2022-09-09 11:10 ` Ihor Radchenko
0 siblings, 1 reply; 4+ messages in thread
From: Max Nikulin @ 2022-09-08 14:37 UTC (permalink / raw)
To: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]
On 06/09/2022 19:07, Ihor Radchenko wrote:
> Max Nikulin writes:
>
>> Debugging `org-file-apps' and `org-open-file' regexp subgroups I noticed
>> an issue: the code distorts case of the link components making them
>> invalid. The patch to fix the bug is attached, the commit message
>> contains an example of the problem.
>>
>> Maybe I break some use case, but I am unaware when namely downcased link
>> must be used.
>
> I tracked this dlink business down to
> 75563bf71e6df356a5ae77a93152fcf913378107.
> The relevant ML discussion is in
> https://orgmode.org/list/4B51D104.9090502@jboecker.de
It is the thread I mentioned in the commit message.
> So, it is probably safe to drop it, especially if tests are passing.
There is no tests for `org-open-file'. Checking of file existence,
running an asynchronous process make it harder to create such tests.
>> ;; First, try matching against apps-dlink if we
>> ;; get a match here, store the match data for
>> ;; later.
>> - (let ((match (assoc-default dlink apps-dlink
>> - 'string-match)))
>> + (let* ((case-fold-search t)
>> + (match (assoc-default link apps-dlink
>> + 'string-match)))
>
> With this patch, `apps-dlink' name becomes completely confusing.
> Is there any way to get rid of it as well? (or maybe rename to something
> more reasonable).
Evey attempt to read this function gives a new surprise. I agree that
dlink has no sense any more, see the new patches.
[-- Attachment #2: v2-0001-org.el-Pass-link-match-data-to-org-file-apps-func.patch --]
[-- Type: text/x-patch, Size: 1898 bytes --]
From 283b2721857350e9eba4b8056b433764bf121222 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Thu, 8 Sep 2022 19:02:37 +0700
Subject: [PATCH v2 1/3] org.el: Pass link match data to `org-file-apps'
functions
* lisp/org.el (org--file-apps-entry-dlink-p): Fix it to pass match data
to handler functions from `org-open-file' alist when pattern field of
`org-file-apps' contains regexp subgroups.
Update `org--file-apps-entry-dlink-p' to use current convention for action
field of `org-file-apps' structure. Currently it may be a function while
earlier s-expression was allowed. Obsolete test wrongly separated actions
able to handle regexp subgroups matched in the link. An example when
match data were not passed to the handler function:
(add-to-list
'org-file-apps
'("\\.pdf\\(?:\\.gz\\|\\.bz2\\|\\.xz\\)?\\(?:::.*\\)?\\'"
. my-open-pdf-locator))
Notice that lambda functions passed `consp' test, so namely
`defun' is required to reproduce the issue.
This change was missed in the commit:
c8a3ab1e4 2016-02-03 18:30:17 +0100
Nicolas Goaziou: `org-file-apps' accept functions instead of sexp
For discussion of the issue with evaluation of arbitrary expression see
https://list.orgmode.org/CALn3zoh+ACSU09eRurfwKjmCnw7i-_0KX7tA2jWqtu=vvQepLQ@mail.gmail.com/T/#u
Michael Brand. org-player and switch to lexical binding in org.el.
Sun, 17 Jan 2016 19:58:38 +0100
---
lisp/org.el | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lisp/org.el b/lisp/org.el
index 34560c83f..3d2fbd2cb 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -8002,7 +8002,7 @@ a parameter."
(> (regexp-opt-depth selector) 0)
(or (and (stringp action)
(string-match "%[0-9]" action))
- (consp action))))
+ (functionp action))))
(_ nil)))
(defun org--file-apps-regexp-alist (list &optional add-auto-mode)
--
2.25.1
[-- Attachment #3: v2-0002-org.el-Preserve-case-for-link-subgroups-from-org-.patch --]
[-- Type: text/x-patch, Size: 2565 bytes --]
From 6800981712855f76c88703b0ca9905ec0af557cd Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Mon, 5 Sep 2022 13:54:25 +0700
Subject: [PATCH v2 2/3] org.el: Preserve case for link subgroups from
`org-file-apps'
* lisp/org.el (org-open-file): Avoid matching of `org-file-apps' records
against the link converted to downcase since it caused incorrect
substitutions to the command.
Consider the following entry
(add-to-list
'org-file-apps
'("\\.PDF\\(?:\\.[gx]z\\|\\.bz2\\)?::\\(#[^:]*\\)::\\(.+\\)\\'"
. "okular --find %2 -- %s%1"))
and the link
<file:///usr/share/doc/bash/bashref.pdf::#Redirections::before>
Without the patch
okular --find before -- /usr/share/doc/bash/bashref.pdf\#redirections
command is executed and the application can not resolve internal
cross-reference anchor.
In https://list.orgmode.org/4B51D104.9090502@jboecker.de/T/#u
https://list.orgmode.org/k2jfb2eb6811004041733zf176e0aq8367924746db81f5@mail.gmail.com/T/#u
the purpose of `dlink' is not clarified, so I assume that the only
purpose is to allow matching file suffixes, e.g. ".pdf" vs ".PDF".
---
lisp/org.el | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lisp/org.el b/lisp/org.el
index 3d2fbd2cb..236969649 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -8134,7 +8134,6 @@ If the file does not exist, throw an error."
(link (cond (line (concat file "::" (number-to-string line)))
(search (concat file "::" search))
(t file)))
- (dlink (downcase link))
(ext
(and (string-match "\\`.*?\\.\\([a-zA-Z0-9]+\\(\\.gz\\)?\\)\\'" dfile)
(match-string 1 dfile)))
@@ -8159,8 +8158,9 @@ If the file does not exist, throw an error."
;; First, try matching against apps-dlink if we
;; get a match here, store the match data for
;; later.
- (let ((match (assoc-default dlink apps-dlink
- 'string-match)))
+ (let* ((case-fold-search t)
+ (match (assoc-default link apps-dlink
+ 'string-match)))
(if match
(progn (setq link-match-data (match-data))
match)
@@ -8191,7 +8191,7 @@ If the file does not exist, throw an error."
(user-error "No such file: %s" file))
(cond
((org-string-nw-p cmd)
- (setq cmd (org--open-file-format-command cmd file dlink link-match-data))
+ (setq cmd (org--open-file-format-command cmd file link link-match-data))
(save-window-excursion
(message "Running %s...done" cmd)
--
2.25.1
[-- Attachment #4: v2-0003-org.el-Avoid-dlink-identifiers-in-org-open-file.patch --]
[-- Type: text/x-patch, Size: 2803 bytes --]
From 28b37409cbbce8a2eae2d79cbf807ce8f3c7deb2 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Thu, 8 Sep 2022 19:13:35 +0700
Subject: [PATCH v2 3/3] org.el: Avoid dlink identifiers in `org-open-file'
* lisp/org.el (org--file-apps-entry-locator-p): Rename from
`org--file-apps-entry-dlink-p'.
(org-open-file): Avoid confusing "dlink" part of some identifiers.
Earlier `dlink' local variable was removed to prevent an issue with
distorted case of link components.
---
lisp/org.el | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/lisp/org.el b/lisp/org.el
index 236969649..6412d3090 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -7988,7 +7988,7 @@ This is saved in case the need arises to restore it.")
(`windows-nt org-file-apps-windowsnt)
(_ org-file-apps-gnu)))
-(defun org--file-apps-entry-dlink-p (entry)
+(defun org--file-apps-entry-locator-p (entry)
"Non-nil if ENTRY should be matched against the link by `org-open-file'.
It assumes that is the case when the entry uses a regular
@@ -8119,9 +8119,9 @@ If the file does not exist, throw an error."
(let* ((file (if (equal path "") buffer-file-name
(substitute-in-file-name (expand-file-name path))))
(file-apps (append org-file-apps (org--file-default-apps)))
- (apps (cl-remove-if #'org--file-apps-entry-dlink-p file-apps))
- (apps-dlink (cl-remove-if-not #'org--file-apps-entry-dlink-p
- file-apps))
+ (apps (cl-remove-if #'org--file-apps-entry-locator-p file-apps))
+ (apps-locator (cl-remove-if-not #'org--file-apps-entry-locator-p
+ file-apps))
(remp (and (assq 'remote apps) (file-remote-p file)))
(dirp (unless remp (file-directory-p file)))
(file (if (and dirp org-open-directory-means-index-dot-org)
@@ -8155,17 +8155,17 @@ If the file does not exist, throw an error."
(t
(setq cmd (or (and remp (cdr (assq 'remote apps)))
(and dirp (cdr (assq 'directory apps)))
- ;; First, try matching against apps-dlink if we
+ ;; First, try matching against apps-locator if we
;; get a match here, store the match data for
;; later.
(let* ((case-fold-search t)
- (match (assoc-default link apps-dlink
+ (match (assoc-default link apps-locator
'string-match)))
(if match
(progn (setq link-match-data (match-data))
match)
(progn (setq in-emacs (or in-emacs line search))
- nil))) ; if we have no match in apps-dlink,
+ nil))) ; if we have no match in apps-locator,
; always open the file in emacs if line or search
; is given (for backwards compatibility)
(assoc-default dfile
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread