[PATCH v3] Re: Bug in 9.5.3 org--file-default-apps

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> However, in my opinion, it does not address original Craig's issue. The 
> patch improves handling of *non-text* files requiring *external* 
> viewers, while Craig complained that behavior for shell script is 
> incorrect and his problem is tightly bound to erased `mailcap-mime-data'.

Ideally, we need a feedback from him.
For emacs-27 specifically, we might need to work around the bug.
However, I am not sure what would be the best way to do it. The easiest
can be changing the default value of org-file-apps-gnu on Emacs 27
specifically to not use mailcap at all. But I am pretty sure that we can
do better.

> I can not reproduce behavior he observed exactly, Org does not opens 
> shell scripts by less, but it tries and silently (it is expected) fails.
>
> My results (emacs-27):
> 1. If there is no mailcap files at all, the script is opened
>     in the same emacs session that is correct from my point of view.
> 2. If I add ~/.mailcap
>         text/plain; less '%s'; needsterminal
>     then I get silent failures
>         Running less /etc/profile...done
> 3. With your patch and the following additional entry in ~/.mailcap
>     (notice "text" instead of "application" and just "emacs")
>         text/x-shellscript; emacs %s; test=test -n "$DISPLAY"
>     new Emacs session is started. It is a problem but partially
>     it is caused by incorrect mailcap configuration.
>     Unlike "text/plain" that would be handled by view-mode
>     unless `mailcap-mime-data' were erased in Emacs-27,
>     "text/x-shellscript" is handled by less on my main system
>     due to mailcap while I would expect same Emacs session.

I am confused here. org-file-apps-gnu says that we rely on mailcap:

((remote . emacs)
 (system . mailcap)
 (t . mailcap))

So, is (3) following what you would expect from mailcap (regardless
whether it is incorrectly configured or not)? Wrong configuration of
mailcap is none of Org business - we need not to be "smart" and fix user
"errors". They may be deliberate.

> I read implementation of `org-open-file' once more and now I see that 
> currently remote files can not be processed by mailcap code path even 
> with custom `org-file-apps', so thank you for explanation. In some cases 
> it may be handy to launch remote viewer from a host accessed through 
> ssh, but let's leave it aside.

This is exactly why you can always customize org-file-apps. 

>> -      (let* ((mime-type (mailcap-extension-to-mime (or ext "")))
>> +      (let* ((mime-type (if (executable-find "file")
>
> I would consider (and ... (not remp)) despite currently it is redundant. 
> Just to mitigate consequences if other parts of this complicated 
> function will be modified. On the other hand 
> `start-process-shell-command' is not ready for remote files anyway, so 
> major cleanup would be required.

I would be in favor of a cleanup (by someone™), but I am against
redundancy. Such redundancy may mask bugs making them difficult to
debug. Not to mention code readability.

>> +                            (shell-command-to-string
>> +                             (format "%s --brief --mime-type %s"
>> +                                     (executable-find "file")
>> +                                     (shell-quote-argument 
>> (convert-standard-filename file))))
>
> It is not enough to cure my paranoia. However the following case is 
> rather pathological
>
> mkdir 'Program Files'
> ln -s /usr/bin/file 'Program Files'/
> PATH="$HOME/Program Files:$PATH" \
>     emacs -Q -L ~/src/org-mode/lisp/ ~/examples/org/open-script.org &
>
> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>    string-match("/" nil 0)
>    split-string(nil "/")
>    mailcap-mime-info("/bin/bash: line 1: /home/ubuntu/Program: No such 
> f...")
>    (let* ((mime-type (if (executable-find "file") 
> (shell-command-to-string (format "%s --brief --mime-type %s" 
> (executable-find "file") (shell-quote-argument 
> (convert-standard-filename file)))) (mailcap-extension-to-mime (or ext 
> "")))) (command (mailcap-mime-info mime-type))) (if (stringp command) 
> (setq cmd command) (setq cmd 'emacs)))

Well. If we want to be this paranoid, could you write a generic safe
shell-command wrapper that takes care of various edge cases? Then, we
can add that wrapper to org-macs and reuse it in various places where we
need to run external command.

> Another corner case:
>
>     file --brief --mime-type tstorg-sh-symlink
>     inode/symlink
>     file --brief --mime-type --dereference tstorg-sh-symlink
>     text/x-shellscript

I added the extra argument as you suggested. See the new version of the
patch. Though my man tells me that --dereference is the default. Not on
your system apparently.

>> +                                     (executable-find "file")
>> +                                     (shell-quote-argument 
>> (convert-standard-filename file))))
>> +                          (mailcap-extension-to-mime (or ext ""))))
>
> Actually MIME type for shell scripts varies a lot
>
> (mailcap-extension-to-mime "sh") => "text/x-sh"
>
> run-mailcap --norun examples/org/script/tstorg.sh
> Error: no "view" mailcap rules found for type "application/x-sh"
>
> And "text/x-shellscript" as above.

This should not matter for us. As long as mailcap-mime-info returns
something meaningful, we should be good to go. Unless mailcap-mime-info
itself is buggy.

Best,
Ihor

>From 3dac7d62b7c85bde31d27ef48569df9d07ae6eec Mon Sep 17 00:00:00 2001
Message-Id: 
<3dac7d62b7c85bde31d27ef48569df9d07ae6eec.1653918246.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Sun, 22 May 2022 12:04:35 +0800
Subject: [PATCH v3] org-open-file: Use file command to determine mime type,
 when available

* lisp/org.el (org-open-file): Prefer file command to determine file
type instead of relying on `mailcap-extension-to-mime'.  Only fallback
to the latter when file command is not available on the system.

Fixes 
https://list.orgmode.org/874k1n2hpv.fsf@localhost/T/#mcc10df4ea7937360671e6dea8061153dee518807
---
 lisp/org.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/org.el b/lisp/org.el
index 95dff27ad..b9a7b4b2e 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -7981,7 +7981,12 @@ (defun org-open-file (path &optional in-emacs line 
search)
     (when (eq cmd 'mailcap)
       (require 'mailcap)
       (mailcap-parse-mailcaps)
-      (let* ((mime-type (mailcap-extension-to-mime (or ext "")))
+      (let* ((mime-type (if (executable-find "file")
+                            (shell-command-to-string
+                             (format "%s --brief --mime-type --dereference %s"
+                                     (executable-find "file")
+                                     (shell-quote-argument 
(convert-standard-filename file))))
+                          (mailcap-extension-to-mime (or ext ""))))
             (command (mailcap-mime-info mime-type)))
        (if (stringp command)
            (setq cmd command)
-- 
2.35.1