bug#63744: 28.2; fix dired-guess-default

bug#63744: 28.2; fix dired-guess-default

[Leo Liu]

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

I have this customisation

(setq dired-guess-shell-alist-user '(("." EXP)))

where EXP evaluates to a list of strings. It has been working for ~10
years until Emacs 28.

After some digging it turns out there is a rewrite of
`dired-guess-default' which is not compatible. Previously returning a
list of strings from EXP accidentally worked.

  (eval (car cmds) `((file . ,file)))  ; single command

but if it evaluates to a list of strings it is perfectly fine as per the
documentation of dired-guess-default. I propose the following patch for
remedy.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-dired-guess-default.patch --]
[-- Type: text/x-patch, Size: 1714 bytes --]

From 16fbf6eb7e054d715326801647af579715f2911f Mon Sep 17 00:00:00 2001
From: Leo Liu <sdl.web@gmail.com>
Date: Sat, 27 May 2023 12:03:08 +0800
Subject: [PATCH] Fix dired-guess-default

* lisp/dired-x.el (dired-guess-shell-alist-user): Document COMMAND can
also return a list of strings.
(dired-guess-default): Handle list of strings from eval COMMAND.
---
 lisp/dired-x.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/dired-x.el b/lisp/dired-x.el
index bc85da2c..0420a86c 100644
--- a/lisp/dired-x.el
+++ b/lisp/dired-x.el
@@ -950,9 +950,9 @@ If several files are to be processed, REGEXP has to match all the
 files.
 
 Each COMMAND can either be a string or a Lisp expression that evaluates
-to a string.  If this expression needs to consult the name of the file for
-which the shell commands are being requested, it can access that file name
-as the variable `file'.
+to a string or list of strings.  If this expression needs to consult
+the name of the file for which the shell commands are being requested,
+it can access that file name as the variable `file'.
 
 If several COMMANDs are given, the first one will be the default
 and the rest will be added temporarily to the history and can be retrieved
@@ -975,9 +975,9 @@ See `dired-guess-shell-alist-user'."
   (let* ((case-fold-search dired-guess-shell-case-fold-search)
          (programs
           (delete-dups
-           (mapcar
+           (mapcan
             (lambda (command)
-              (eval command `((file . ,(car files)))))
+              (ensure-list (eval command `((file . ,(car files))))))
             (seq-reduce
              #'append
              (mapcar #'cdr
-- 
2.32.0 (Apple Git-132)

bug#63744: 28.2; fix dired-guess-default

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Sat, 27 May 2023 12:10:15 +0800
> 
> I have this customisation
> 
> (setq dired-guess-shell-alist-user '(("." EXP)))
> 
> where EXP evaluates to a list of strings. It has been working for ~10
> years until Emacs 28.

What is the semantics of a list of strings in this case?
shell-command takes a single string as its argument COMMAND, it
doesn't take a list of strings.

> After some digging it turns out there is a rewrite of
> `dired-guess-default' which is not compatible. Previously returning a
> list of strings from EXP accidentally worked.
> 
>   (eval (car cmds) `((file . ,file)))  ; single command
> 
> but if it evaluates to a list of strings it is perfectly fine as per the
> documentation of dired-guess-default. I propose the following patch for
> remedy.

I'd rather not proliferate an undocumented "feature" that is merely a
side effect of the particular implementation we had at some point,
without understanding what it gives us.  The alternative is for you to
change your customization so that EXP returns the car of the list it
returned before, and that should be both easy and backward-compatible,
AFAIU.

bug#63744: 28.2; fix dired-guess-default

[Leo Liu]

On 2023-05-27 09:49 +0300, Eli Zaretskii wrote:
[snipped 10 lines]
> What is the semantics of a list of strings in this case?
> shell-command takes a single string as its argument COMMAND, it
> doesn't take a list of strings.

See the documentation of dired-guess-default.

,----
| Return a shell command, or a list of commands, appropriate for FILES.
`----

[snipped 10 lines]
> I'd rather not proliferate an undocumented "feature" that is merely a
> side effect of the particular implementation we had at some point,
> without understanding what it gives us. 

I agree in principle.

> The alternative is for you to change your customization so that EXP
> returns the car of the list it returned before, and that should be
> both easy and backward-compatible, AFAIU.

Unfortunately many (or most) things in Emacs grow from a 50% solution.
There is no alternative at the moment other than redefining
dired-guess-default.

If you have a static list it can be expressed as an element in
dired-guess-shell-alist-user as such:

   (RE "STR1" "STR2" ...)

If you have a dynamic list (for example a list from querying the OS) you
are stuck. The patch makes this possible and in my view makes
dired-guess-shell-alist-user more coherent. (RE "STR1" "STR2" ...) can
be regarded as another syntax for

   (RE ("STR1" "STR2" ...))

WDYT?

bug#63744: 28.2; fix dired-guess-default

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Sun, 28 May 2023 10:40:16 +0800
> 
> On 2023-05-27 09:49 +0300, Eli Zaretskii wrote:
> [snipped 10 lines]
> > What is the semantics of a list of strings in this case?
> > shell-command takes a single string as its argument COMMAND, it
> > doesn't take a list of strings.
> 
> See the documentation of dired-guess-default.
> 
> ,----
> | Return a shell command, or a list of commands, appropriate for FILES.
> `----

That's not the same, but I get the intent.

> > I'd rather not proliferate an undocumented "feature" that is merely a
> > side effect of the particular implementation we had at some point,
> > without understanding what it gives us. 
> 
> I agree in principle.
> 
> > The alternative is for you to change your customization so that EXP
> > returns the car of the list it returned before, and that should be
> > both easy and backward-compatible, AFAIU.
> 
> Unfortunately many (or most) things in Emacs grow from a 50% solution.

Where are you when I'm trying to convince people on emacs-devel not to
install such 50% solutions?  If every marginally-useful change that
can "potentially" help someone is press-pushed into the code base,
with the main reason being "why not?", the result is what you observe.
But almost no one tries to see this in advance when half-baked changes
are brought up, so stuff gets in.  Help in reviewing extensions and
noticing such marginal solutions will be greatly appreciated.

But I digress.

> There is no alternative at the moment other than redefining
> dired-guess-default.

But your suggestion is also a change in that function, so what's the
difference?

> If you have a static list it can be expressed as an element in
> dired-guess-shell-alist-user as such:
> 
>    (RE "STR1" "STR2" ...)
> 
> If you have a dynamic list (for example a list from querying the OS) you
> are stuck. The patch makes this possible and in my view makes
> dired-guess-shell-alist-user more coherent. (RE "STR1" "STR2" ...) can
> be regarded as another syntax for
> 
>    (RE ("STR1" "STR2" ...))

Can't you generate the entire value of dired-guess-shell-alist-user
dynamically, including the RE part?

Anyway, you are requesting a new feature: that the elements of the
alist could be of the form (REGEXP (COMMAND1 COMMAND2 ...)).  I don't
see how this makes dired-guess-shell-alist-user more coherent, but I
have nothing against such an extension.  However, it's too late for
such extensions on the emacs-29 release branch, so it could only go to
master, for Emacs 30.  And it should be properly documented, of
course.

bug#63744: 28.2; fix dired-guess-default

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > There is no alternative at the moment other than redefining
> > dired-guess-default.
>
> But your suggestion is also a change in that function, so what's the
> difference?

I think he means: the user should not have to do this.

> > If you have a static list it can be expressed as an element in
> > dired-guess-shell-alist-user as such:
> >
> >    (RE "STR1" "STR2" ...)
> >
> > If you have a dynamic list (for example a list from querying the OS) you
> > are stuck. The patch makes this possible and in my view makes
> > dired-guess-shell-alist-user more coherent. (RE "STR1" "STR2" ...) can
> > be regarded as another syntax for
> >
> >    (RE ("STR1" "STR2" ...))
>
> Can't you generate the entire value of dired-guess-shell-alist-user
> dynamically, including the RE part?

It's a real difference, actually a real win.  Please don't only have
constant lists in mind.

"Dynamically" means that dired can inspect the matched file (the file
name is provided via the variable 'file' bound when evaluating the
expression).  This means the EXPR can look at the properties of the
file, it's type and modes etc, which allows a more fine grained user
customization.  With such a change one can have entries looking like

  (RE (if COND (GET-DEFAULTS-FOR-COND) (GET-OTHER-DEFAULTS)))

which would currently be have to be specified as several entries like

  (RE (when COND DEFAULT-FOR-COND-1))
  (RE (when COND DEFAULT-FOR-COND-2))
    ...
  (RE (when COND DEFAULT-FOR-COND-n))
  (RE (when (not COND) OTHER-DEFAULT-1))
  ...
  (RE (when (not COND) OTHER-DEFAULT-m))

and, much worse, testing the CONDITION would have to be performed many
times.

Other things are currently impossible, for example, taking the current
screen setup into account (which can change within an Emacs session).

@Leo:

> However, it's too late for such extensions on the emacs-29 release
> branch, so it could only go to master, for Emacs 30.  And it should be
> properly documented, of course.

`dired-guess-default' lives in "dired-aux" now.

>  If several COMMANDs are given, the first one will be the default
>  and the rest will be added temporarily to the history and can be retrieved
> @@ -975,9 +975,9 @@ See `dired-guess-shell-alist-user'."
>    (let* ((case-fold-search dired-guess-shell-case-fold-search)
>           (programs
>            (delete-dups
> -           (mapcar
> +           (mapcan
>              (lambda (command)
> -              (eval command `((file . ,(car files)))))
> +              (ensure-list (eval command `((file . ,(car files))))))

I think we should avoid `nconc'ing: if a user does specify literal lists.
the config will be destructively altered (right?).  I think we better
use (apply #'append (mapcar ...)) or something like that.


Michael.

bug#63744: 28.2; fix dired-guess-default

[Leo Liu]

On 2023-05-29 03:23 +0200, Michael Heerdegen wrote:
> I think we should avoid `nconc'ing: if a user does specify literal lists.
> the config will be destructively altered (right?).  I think we better
> use (apply #'append (mapcar ...)) or something like that.

Good point. Will update the patch.

If we put all these details behind and look from the users' perspective,
I wonder if there is a chance for Emacs 28.

Basically this feature has been there and working since at least Emacs
24 (or even earlier). It may be an accident which I only discovered last
week though the bug has been annoying me for a few months since I
upgraded from Emacs 27. For users this is a regression and it's bad.

Comments?

bug#63744: 28.2; fix dired-guess-default

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Leo Liu <sdl.web@gmail.com>,  63744@debbugs.gnu.org
> Date: Mon, 29 May 2023 03:23:46 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > There is no alternative at the moment other than redefining
> > > dired-guess-default.
> >
> > But your suggestion is also a change in that function, so what's the
> > difference?
> 
> I think he means: the user should not have to do this.

If there's only one user who wants that, there's no difference...

> > >    (RE ("STR1" "STR2" ...))
> >
> > Can't you generate the entire value of dired-guess-shell-alist-user
> > dynamically, including the RE part?
> 
> It's a real difference, actually a real win.  Please don't only have
> constant lists in mind.

Does this answer my question above?  If so, I don't think I understand
the answer, please elaborate.  I asked why not generate the whole
value, including the REGEXP part, dynamically.

> "Dynamically" means that dired can inspect the matched file (the file
> name is provided via the variable 'file' bound when evaluating the
> expression).  This means the EXPR can look at the properties of the
> file, it's type and modes etc, which allows a more fine grained user
> customization.  With such a change one can have entries looking like
> 
>   (RE (if COND (GET-DEFAULTS-FOR-COND) (GET-OTHER-DEFAULTS)))
> 
> which would currently be have to be specified as several entries like
> 
>   (RE (when COND DEFAULT-FOR-COND-1))
>   (RE (when COND DEFAULT-FOR-COND-2))
>     ...
>   (RE (when COND DEFAULT-FOR-COND-n))
>   (RE (when (not COND) OTHER-DEFAULT-1))
>   ...
>   (RE (when (not COND) OTHER-DEFAULT-m))
> 
> and, much worse, testing the CONDITION would have to be performed many
> times.

Sorry, I don't see why it has to be so complicated.  RE can be
generated just once, and then consed into a list with whatever should
follow it, and those other elements could be generated dynamically.

Anyway, I don't see why we should keep arguing.  I already said that I
won't object to such an extension of the forms in
dired-guess-shell-alist-user, I just think we should document them,
and I'm quite sure such a change in the code is not for the released
branch.

bug#63744: 28.2; fix dired-guess-default

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Mon, 29 May 2023 11:49:27 +0800
> 
> If we put all these details behind and look from the users' perspective,
> I wonder if there is a chance for Emacs 28.

It doesn't look like there will be another Emacs 28.x release.

And, given the change to the code, I don't think this is appropriate
for Emacs 29.1, either.  It's too late for such changes on the release
branch.

> Basically this feature has been there and working since at least Emacs
> 24 (or even earlier). It may be an accident which I only discovered last
> week though the bug has been annoying me for a few months since I
> upgraded from Emacs 27. For users this is a regression and it's bad.

If it's a bad regression, why didn't we hear about it until now?
Emacs 28.1 was released more than a year ago, and was in pretest for
several months before that.  My guess would be that this accidental
"feature" is not used frequently enough to be a bad regression.

And I don't object to extending Dired in this way, I just don't think
we should do that in Emacs 29.  There are enough real bad problems and
regressions that block its release, and I'm not thrilled adding
another non-trivial change so late.  Sorry.

bug#63744: 28.2; fix dired-guess-default

[Leo Liu]

On 2023-05-29 15:07 +0300, Eli Zaretskii wrote:
> Does this answer my question above?  If so, I don't think I understand
> the answer, please elaborate.  I asked why not generate the whole
> value, including the REGEXP part, dynamically.

I thought you would have grasped the problem by now ;)

"I asked why not generate the whole value, including the REGEXP part,
dynamically."

This is a static list.

The problem we are trying to solve here is we don't know the list until
we know the FILE. Even when given the FILE we may still have imperfect
knowledge about which tools can handle it.

For example, On my Mac an html file can be handled by 10 apps at the
moment including all browsers and Emacs and more. It may change next
minute when some apps are installed or uninstalled or file association
changed.

bug#63744: 28.2; fix dired-guess-default

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Tue, 30 May 2023 02:20:28 +0800
> 
> On 2023-05-29 15:07 +0300, Eli Zaretskii wrote:
> > Does this answer my question above?  If so, I don't think I understand
> > the answer, please elaborate.  I asked why not generate the whole
> > value, including the REGEXP part, dynamically.
> 
> I thought you would have grasped the problem by now ;)
> 
> "I asked why not generate the whole value, including the REGEXP part,
> dynamically."
> 
> This is a static list.
> 
> The problem we are trying to solve here is we don't know the list until
> we know the FILE. Even when given the FILE we may still have imperfect
> knowledge about which tools can handle it.
> 
> For example, On my Mac an html file can be handled by 10 apps at the
> moment including all browsers and Emacs and more. It may change next
> minute when some apps are installed or uninstalled or file association
> changed.

I understand all that.  But you are evidently still not understanding
what I'm asking.  Never mind, I give up.  Some kind of failure to
explain myself.

Bottom line: we can extend the feature the way you'd like, but it will
need be installed on master, and will have to wait for Emacs 30.

bug#63744: 28.2; fix dired-guess-default

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> If it's a bad regression, why didn't we hear about it until now?

This (gone) feature had not been documented.  I fixed this regression in
my local config and added a note "make a bug report".  I'm sorry that I
didn't do this.

In my estimation, the dired-mode's way to configure the "open with"
behavior is too inconvenient for the majority of people and they just
don't use it, so they don't care.  The rest has a bunch of advices of
dired functions in their init file or uses self written stuff.

This patch is a step to improve this situation.

Michael.

bug#63744: 28.2; fix dired-guess-default

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I understand all that.  But you are evidently still not understanding
> what I'm asking.

If interested - else feel free to ignore.

If this didn't answer your question:

> Can't you generate the entire value of dired-guess-shell-alist-user
> dynamically, including the RE part?

then you meant to recompute the variable's value every time the user wants
to open a file with dired?

It's a defcustom so Emacs should not change the value at all.
And recomputing the whole value every time is much slower than computing
a list of defaults for one class of files matched by the regexp.  The
conditions that need to be checked are very different for different file
types in a typical scenario, and the suggested design is more efficient:
first look at the file type by matching the RE part, then decide, at the
moment of user request, what applications are appropriate for this type
of file (and only for this type) in this moment.

Michael.

bug#63744: 28.2; fix dired-guess-default

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Leo Liu <sdl.web@gmail.com>,  63744@debbugs.gnu.org
> Date: Tue, 30 May 2023 02:24:25 +0200
> 
> then you meant to recompute the variable's value every time the user wants
> to open a file with dired?

No, every time the situation changes in a way that requires its
recomputation.  I don't have a clear idea when that happens, but if
the REGEXP part is ".", it is clear to me it happens quite often
already, since the list of commands for a given file depends on the
file, and "." does not.

> It's a defcustom so Emacs should not change the value at all.

We are talking about user customizations.  A user can make his/her
customizations change the value as often as needed.

> And recomputing the whole value every time is much slower than computing
> a list of defaults for one class of files matched by the regexp.

That is not Leo's use case, AFAICT: the list of commands is
dynamically recomputed when needed anyway.

> The conditions that need to be checked are very different for
> different file types in a typical scenario, and the suggested design
> is more efficient: first look at the file type by matching the RE
> part, then decide, at the moment of user request, what applications
> are appropriate for this type of file (and only for this type) in
> this moment.

Like I said: I'm not against extending the value to support this, I
just don't see how this could be considered an urgent bugfix that must
be done in Emacs 29, that's all.

bug#63744: 28.2; fix dired-guess-default

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> [...] but I have nothing against such an extension.  However, it's too
> late for such extensions on the emacs-29 release branch, so it could
> only go to master, for Emacs 30.  And it should be properly
> documented, of course.

So, Leo, is it ok for you to do that?

Michael.

bug#63744: 28.2; fix dired-guess-default

[Leo Liu]

On 2023-06-02 02:14 +0200, Michael Heerdegen wrote:
> So, Leo, is it ok for you to do that?
>
> Michael.

Yes, that's no problem at all. But I was wondering if we could do a
little more for Emacs 30. For example should the CDR of each element
just be a function? And do you have other suggestions? Thanks.