[PATCH] org.el: Preserve case for link subgroups from `org-file-apps'

[PATCH] org.el: Preserve case for link subgroups from `org-file-apps'

[Max Nikulin]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

Hi,

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.

[-- Attachment #2: 0001-org.el-Preserve-case-for-link-subgroups-from-org-fil.patch --]
[-- Type: text/x-patch, Size: 2551 bytes --]

From c7c514d62a8b2150cfbc1bfa927422790bb617cf Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Mon, 5 Sep 2022 13:54:25 +0700
Subject: [PATCH] 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 f317053c4..67721cc8d 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

Re: [PATCH] org.el: Preserve case for link subgroups from `org-file-apps'

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> 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

Carsten replied (in
https://list.orgmode.org/orgmode/9771A876-82D8-4755-9EC4-F951EA9FE6E6@gmail.com/):

>> Hi Jan,
>> 
>> I have now applied this patch.
>> 
>> Hi everyone,
>> 
>> I am not sure if I completely understood every part of it,  so if
>> anyone finds strange behavior of links, make sure to report it so
>> that we (Jan, that is :-) gets a chance to fix it.

I do not see much of discussion relevant to downcasing the links.
I also do not see a clear reason why downcasing is necessary.
So, it is probably safe to drop it, especially if tests are passing.

>  		    ;; 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).

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92

Re: [PATCH v2] org.el: Preserve case for link subgroups from `org-file-apps'

[Max Nikulin]

[-- 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

Re: [PATCH v2] org.el: Preserve case for link subgroups from `org-file-apps'

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> Evey attempt to read this function gives a new surprise. I agree that 
> dlink has no sense any more, see the new patches.

Thanks!
Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=33686b9955c011486a72ec548475651dcadb0339
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=6e9ea3a0766de9df6992bfb0359a5826ed1951b5
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=f25b308af671766bb01e507c6db57bf3e83d8d05

>> 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.

We need mocking, I guess (aka cl-letf).

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92