emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Max Nikulin <manikulin@gmail.com>
To: emacs-orgmode@gnu.org
Subject: Re: [PATCH v2] org.el: Preserve case for link subgroups from `org-file-apps'
Date: Thu, 8 Sep 2022 21:37:49 +0700	[thread overview]
Message-ID: <tfcunv$cd2$1@ciao.gmane.io> (raw)
In-Reply-To: <8735d4wvsy.fsf@localhost>

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

* 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:

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

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

* 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

      . "okular --find %2 -- %s%1"))

and the link
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
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))
 	  (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))
@@ -8191,7 +8191,7 @@ If the file does not exist, throw an error."
       (user-error "No such file: %s" file))
      ((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))
 	(message "Running %s...done" cmd)

[-- 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-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."
       (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
 		      (if match
 			  (progn (setq link-match-data (match-data))
 			(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

  reply	other threads:[~2022-09-08 14:54 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-05  7:15 [PATCH] org.el: Preserve case for link subgroups from `org-file-apps' Max Nikulin
2022-09-06 12:07 ` Ihor Radchenko
2022-09-08 14:37   ` Max Nikulin [this message]
2022-09-09 11:10     ` [PATCH v2] " Ihor Radchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='tfcunv$cd2$1@ciao.gmane.io' \
    --to=manikulin@gmail.com \
    --cc=emacs-orgmode@gnu.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).