Re: master 19a3b499f84: ; * lisp/loadup.el: Don't prohibit advice when ls-lisp is loaded.

[Stefan Monnier]

I pushed a cleaner version of the code, split into more understandable
individual patches into the branch `scratch/no-ls-lisp-advice`.

> - The docstring of `ls-lisp--insert-directory' still makes references
>   to its previous advice nature, this probably should be ironed out.

Fixed.

> - The docstring of `insert-directory-program' does not very explicitly
>   mention that its value could be nil.  (Nothing introduced by your
>   patch, just noticed it.)

Still left as is.  Maybe we should document more prominently that nil is
a valid value now, but Eli seemed to prefer not to go there.

> - And the custom spec of `insert-directory-program' isn't prepared for
>   nil as well, but that is probably OK since it should be used for a
>   generated default value only?

If we do want to support nil more actively we should definitely include
it in the `:type`.

> - AFAICT you call `files--insert-directory-program' as a predicate
>   only.  Probably we should rename it to `...-p' or
>   `files--use-insert-directory-program'?

Fixed.

> - And here is a feeble attempt to provide a docstring:
>
>   "Return wether `insert-directory' should use an external program.
> Take into consideration `ls-lisp-use-insert-directory-program' if that is
> available."

Done (well, I ended up changing it afterwards).

> - Not sure whether that makes a difference, but: Without your patch the
>   advice was conditional on whether ls-lisp was loaded, now the usage of
>   `ls-lisp--insert-directory' is conditional on whether
>   `ls-lisp-use-insert-directory-program' is bound.  What if a user has
>   a .emacs with a customization or `setq' of `ls-l-u-i-d-p' to nil, which
>   she uses both on Windows and Unix?

As mentioned, I don't think this is really a new bug.
It may appear more often, but I don't think there's much we can do about
it, and I'd argue that the new behavior is better because it doesn't
magically change when some Lisp file is loaded.

> - Your fix in `file-expand-wildcards'
>
>           (if (equal "" nondir)
>               (push (or dir nondir) contents)
>
>
>   is hard to digest at 11PM, so a comment won't hurt here, I guess.

Added.

>   Plus it seems to violate the order promise done in the docstring of
>   the function ("... sorted in the `string<' order").

Fixed.

> - Now the logic gets harder and harder, so I may be wrong here.  The
>   docstring of `dired-insert-directory' seems to handle FILE-LIST
>   exclusive to WILDCARD, as if WILDCARDS do not get expanded if
>   FILE-LIST is non-nil:
>
>     If FILE-LIST is non-nil, list only those files.
>     Otherwise, if WILDCARD is non-nil, expand wildcards;
>
>   but your patch changes that, no?


This is now factored into its own commit (included in the branch):

    commit f7cf85c3879c6857e8478bef41cce25a94759fb8
    Author: Stefan Monnier <monnier@iro.umontreal.ca>
    Date:   Sat Dec 9 22:55:32 2023 -0500

    (dired-insert-directory): Obey `file-list` and `wildcard`
    
    Commit 6f6639d6ed6c's support for wildcards in directories failed
    to obey `file-list` and `wildcard` arguments.  Fix it.
    
    * lisp/dired.el (dired-insert-directory): Expand directory wildcards
    only if `file-list` is nil and `wildcard` is non-nil.
    Also, refer back to `dir-wildcard` instead of recomputing it.
    (dired-readin-insert): Pass a non-nil `wildcard` when wildcard
    expansion might be needed to preserve former behavior.

The branch currently doesn't include any doc nor etc/NEWS changes.
I'm not sure what to say there, because this is an internal change.
Let me know if you think something's needed in this respect, and if
so what.


        Stefan