bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Boruch Baum]

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

The docstring for dired-guess-default claims it should handle multiple
instances of COMMAND each of which could be a STRING or a FUNCTION which
evaluates to a STRING ...

The attached patch does that. It also weeds out strings that don't represent
valid programs on the host machine.

It turned out that function dired-guess-shell-command wasn't ready for
the multiple responses, so the patch corrects that as well.

My package diredc has an additional feature of an option for a universal
fall-back. My memory is that the emacs developers rejected the idea when
I originally proposed it, so it's not in this patch.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[-- Attachment #2: dired-guess-default.patch --]
[-- Type: text/x-diff, Size: 3800 bytes --]

diff --git a/dired-x.el b/dired-x.el
index 80a266f..d122027 100644
--- a/dired-x.el
+++ b/dired-x.el
@@ -1039,64 +1039,50 @@ REGEXP is matched case-sensitively."
 (defun dired-guess-default (files)
   "Return a shell command, or a list of commands, appropriate for FILES.
 See `dired-guess-shell-alist-user'."
-
   (let* ((case-fold-search dired-guess-shell-case-fold-search)
          ;; Prepend the user's alist to the default alist.
          (alist (append dired-guess-shell-alist-user
                         dired-guess-shell-alist-default))
-         (file (car files))
-         (flist (cdr files))
          elt regexp cmds)
-
-    ;; Find the first match in the alist for first file in FILES.
-    (while alist
-      (setq elt (car alist)
-            regexp (car elt)
-            alist (cdr alist))
-      (if (string-match-p regexp file)
-          (setq cmds (cdr elt)
-                alist nil)))
-
-    ;; If more than one file, see if all of FILES match regular expression.
-    (while (and flist
-                (string-match-p regexp (car flist)))
-      (setq flist (cdr flist)))
-
-    ;; If flist is still non-nil, then do not guess since this means that not
-    ;; all the files in FILES were matched by the regexp.
-    (setq cmds (and (not flist) cmds))
-
-    ;; Return commands or nil if flist is still non-nil.
-    ;; Evaluate the commands in order that any logical testing will be done.
-    (if (cdr cmds)
-	(delete-dups (mapcar (lambda (cmd) (eval cmd `((file . ,file)))) cmds))
-      (eval (car cmds) `((file . ,file))))))		; single command
+    (cl-loop
+      for elt in alist
+      do (setq regexp (car elt))
+         (cl-loop
+           for file in files
+           always (string-match-p regexp file)
+           finally
+             (cl-loop
+               for cmd in (cdr elt) do
+               (unless (stringp cmd)
+                 (setq cmd (condition-case nil
+                             (funcall cmd `((file . ,file)))
+                             (error nil))))
+               (and (stringp cmd)
+                    (executable-find cmd)
+                    (push cmd cmds)))))
+    (nreverse (delete-dups cmds))))

 (defun dired-guess-shell-command (prompt files)
   "Ask user with PROMPT for a shell command, guessing a default from FILES."
   (let ((default (dired-guess-default files))
-        default-list val)
+        val)
     (if (null default)
         ;; Nothing to guess
         (read-shell-command prompt nil 'dired-shell-command-history)
       (setq prompt (replace-regexp-in-string ": $" " " prompt))
-      (if (listp default)
+      (if (cdr default)
           ;; More than one guess
-          (setq default-list default
-                default (car default)
-                prompt (concat
+          (setq prompt (concat
                         prompt
-                        (format "{%d guesses} " (length default-list))))
-        ;; Just one guess
-        (setq default-list (list default)))
+                        (format "{%d guesses} " (length default-list)))))
       ;; Put the first guess in the prompt but not in the initial value.
-      (setq prompt (concat prompt (format "[%s]: " default)))
+      (setq prompt (concat prompt (format "[%s]: " (car default))))
       ;; All guesses can be retrieved with M-n
       (setq val (read-shell-command prompt nil
                                     'dired-shell-command-history
-                                    default-list))
+                                    default))
       ;; If we got a return, then return default.
-      (if (equal val "") default val))))
+      (if (equal val "") (car default) val))))

 \f
 ;;; RELATIVE SYMBOLIC LINKS.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Lars Ingebrigtsen]

Boruch Baum <boruch_baum@gmx.com> writes:

> The docstring for dired-guess-default claims it should handle multiple
> instances of COMMAND each of which could be a STRING or a FUNCTION which
> evaluates to a STRING ...
>
> The attached patch does that. It also weeds out strings that don't represent
> valid programs on the host machine.

I don't think we want to do the latter because of Tramp considerations.

I've now changed dired-guess-default in a somewhat different way than
your patch suggested, but it's functionally similar.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've now changed dired-guess-default in a somewhat different way than
> your patch suggested, but it's functionally similar.

What happened to the expression case?  Have you maybe removed it by
accident (at least it's broken for me now)?  See the `eval' calls in the
original code.

These were valid `eval' calls, they allowed to construct commands on the
fly at run time, even specific to the currently given FILE, by
specifying an expression that calculated the command string.

TIA,

Michael.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> What happened to the expression case?  Have you maybe removed it by
> accident (at least it's broken for me now)?  See the `eval' calls in the
> original code.

Other things changed, too: AFAIU now only one file has to match,
originally all had to - correct?  That means if I mark 10 files and one
of them is an image, I will get (all) image viewers as guesses.

If this is intended: Should we not at least provide an option to get the
old behavior or sort those matches that match all files first?

TIA,

Michael.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Other things changed, too: AFAIU now only one file has to match,
> originally all had to - correct?  That means if I mark 10 files and one
> of them is an image, I will get (all) image viewers as guesses.

Fixed now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> What happened to the expression case?  Have you maybe removed it by
> accident (at least it's broken for me now)?  See the `eval' calls in the
> original code.

Yup; removed by accident.  Put back in now, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yup; removed by accident.  Put back in now, I think.

Yes, looks good, thanks -

Michael.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
> > Other things changed, too: AFAIU now only one file has to match,
> > originally all had to - correct?
>
> Fixed now.

Yes - thanks.

So the (only) remaining user visible change is that now all matching
entries are considered instead of only the first one, which is actually
a fix: the docstring of `dired-guess-shell-alist-user' describes it
should work like that.

Ok, I hope we are done then ;-) and: closing again.

Thanks,

Michael.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Ok, I hope we are done then ;-) and: closing again.

BTW, one more thought: quite a few people might want to configure this
beyond the "adding entries to dired-guess-shell-alist-user" level -
the suggested filtering of not installed executables mentioned in the
original report is one example.

If we bound #'dired-guess-default to a new variable
`dired-guess-default-function' and used the binding of that variable in
the code, it would be easier to do such things, and it would be a tiny
change.

What do you think?

Michael.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> So the (only) remaining user visible change is that now all matching
> entries are considered instead of only the first one, which is actually
> a fix: the docstring of `dired-guess-shell-alist-user' describes it
> should work like that.

Well...  does it?  It's pretty vague on the issue.  And the comments
seem to contradict that:

;; * Parse `dired-guess-shell-alist-user' and
;;   `dired-guess-shell-alist-default' (in that order) for the first REGEXP
;;   that matches the first file in the file list.
;;
;; * If the REGEXP matches all the entries of the file list then evaluate
;;   COMMAND, which is either a string or a Lisp expression returning a
;;   string.  COMMAND may be a list of commands.
;;
;; * Return this command to `dired-guess-shell-command' which prompts user
;;   with it.  The list of commands is put into the list of default values.
;;   If a command is used successfully then it is stored permanently in
;;   `dired-shell-command-history'.

But this is pretty nonsensical -- it describes how the function what
implemented, but not really what to do when the rest of the files don't
match, or when more of them match "the first file".

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> If we bound #'dired-guess-default to a new variable
> `dired-guess-default-function' and used the binding of that variable in
> the code, it would be easier to do such things, and it would be a tiny
> change.
>
> What do you think?

Sure, if you want to...  but do people really want to customise the
defaults in Dired to this extent?  I haven't really seen much clamouring
for this stuff.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > What do you think?
>
> Sure, if you want to...  but do people really want to customise the
> defaults in Dired to this extent?  I haven't really seen much
> clamouring for this stuff.

I think this is partly because doing this is such an unpleasant and
byzantine thing to do.  But I also have package developers in mind.

Michael.

bug#48071: 28.0.50: dired-guess-default: comply with docstring options [PATCH]

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> Sure, if you want to...  but do people really want to customise the
>> defaults in Dired to this extent?  I haven't really seen much
>> clamouring for this stuff.
>
> I think this is partly because doing this is such an unpleasant and
> byzantine thing to do.  But I also have package developers in mind.

Sure; I'm not against adding it if you feel it would be useful.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no