[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v3] Re: Bug in 9.5.3 org--file-default-apps
From: |
Ihor Radchenko |
Subject: |
[PATCH v3] Re: Bug in 9.5.3 org--file-default-apps |
Date: |
Mon, 30 May 2022 22:00:27 +0800 |
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
- Re: Bug in 9.5.3 org--file-default-apps, (continued)
- Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/19
- Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/19
- Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/20
- Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/20
- Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/20
- Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/21
- [PATCH] Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/22
- Re: [PATCH] Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/22
- [PATCH v2] Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/26
- Re: [PATCH v2] Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/29
- [PATCH v3] Re: Bug in 9.5.3 org--file-default-apps,
Ihor Radchenko <=
- Re: [PATCH v3] Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/30
- Re: [PATCH v3] Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/31
- Re: [PATCH v2] Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/29
- Re: Bug in 9.5.3 org--file-default-apps, Craig STCR, 2022/05/23
- Re: Bug in 9.5.3 org--file-default-apps, Craig STCR, 2022/05/23
- Re: Bug in 9.5.3 org--file-default-apps, Craig STCR, 2022/05/23
- Re: Bug in 9.5.3 org--file-default-apps, Craig STCR, 2022/05/23
- Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/25
- Re: Bug in 9.5.3 org--file-default-apps, Max Nikulin, 2022/05/23
- Re: Bug in 9.5.3 org--file-default-apps, Ihor Radchenko, 2022/05/25