bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Warren Lynn]

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

On Linux. When I call "find-name-dired" command, which in turn invokes
"find-dired-with-command" function, in a multi-hop tramp remote directory,
an error will occur in side the function, with backtrace (only shown lowest
level):

Debugger entered--Lisp error: (wrong-type-argument processp nil)
  process-mark(nil)
  (move-marker (process-mark proc) (point) (current-buffer))

The issue is around this part of the code:

    ;; Start the find process.
    (shell-command (concat command "&") (current-buffer))
    (let ((proc (get-buffer-process (current-buffer)))) ;; !!!!!!!! proc
could be nil here !!!!!!!!!!
      ;; Initialize the process marker; it is used by the filter.
      (move-marker (process-mark proc) (point) (current-buffer))

After changing the code to:

    ;; Start the find process.
    (let ((proc (shell-command (concat command "&") (current-buffer))))
      ;; Initialize the process marker; it is used by the filter.
      (move-marker (process-mark proc) (point) (current-buffer))

I do not have the error any more.

However, I observe that even with my fix above, the behavior is not
completely right. Sometimes, the (found) file entries appear before the
dired buffer header line. My impression of the code is it does not handle
asynchronous timing in a robust way, and that is more of a problem when the
command is invoked from a remote directory. Probably some serious revamp is
needed here, which is beyond my expertise. Maybe Emacs should include the
"aio" package and rely on that for asynchronous event handling.

[-- Attachment #2: Type: text/html, Size: 2025 bytes --]

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Warren Lynn <wrn.lynn@gmail.com> writes:

Hi Warren,

> On Linux. When I call "find-name-dired" command, which in turn invokes
> "find-dired-with-command" function, in a multi-hop tramp remote
> directory, an error will occur in side the function, with backtrace
> (only shown lowest level):
>
> Debugger entered--Lisp error: (wrong-type-argument processp nil)
>   process-mark(nil)
>   (move-marker (process-mark proc) (point) (current-buffer))

I confirm the bug (tested on Emacs 30.0.50). It happens even on a simple
remote connection; no multi-hop needed.

> The issue is around this part of the code:
>
>     ;; Start the find process.
>     (shell-command (concat command "&") (current-buffer))
>     (let ((proc (get-buffer-process (current-buffer)))) ;; !!!!!!!!
> proc could be nil here !!!!!!!!!!
>       ;; Initialize the process marker; it is used by the filter.
>       (move-marker (process-mark proc) (point) (current-buffer))
>
> After changing the code to:
>
>     ;; Start the find process.
>     (let ((proc (shell-command (concat command "&")
> (current-buffer))))
>       ;; Initialize the process marker; it is used by the filter.
>       (move-marker (process-mark proc) (point) (current-buffer))
>
> I do not have the error any more.

That patch isn't correct. It assumes, that an async shell-command returns
the process object. However, the documentation doesn't specify the
return value. And indeed, the type is different in the local and the
remote case:

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/"))
 (shell-command "ls &"))
=> #<window 21 on *Async Shell Command*>

(let ((default-directory "/ssh::"))
 (shell-command "ls &"))
=> #<process *Async Shell Command*>
--8<---------------cut here---------------end--------------->8---

It is sheer luck for you, that in the remote case the process object is
returned.

> However, I observe that even with my fix above, the behavior is not
> completely right. Sometimes, the (found) file entries appear before
> the dired buffer header line. My impression of the code is it does not
> handle asynchronous timing in a robust way, and that is more of a
> problem when the command is invoked from a remote directory. Probably
> some serious revamp is needed here, which is beyond my expertise.
> Maybe Emacs should include the "aio" package and rely on that for
> asynchronous event handling.

There seem to be timing problems, indeed. And I believe using
make-process instead of shell-command would give us better control over
the process.

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Version: 29.2

Warren Lynn <wrn.lynn@gmail.com> writes:

> Hi Michael:

Hi Warren,

> I tried out your patch and could no longer reproduce the problem, of
> either the nil proc or the timing. Thanks a lot for such quick work.

I've pushed the patch to the emacs-29 branch. Closing the bug.

> Warren

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Hello Michael and Warren,

Your change - this part in particular:

| @@ -244,8 +244,8 @@ find-dired-with-command
|      (erase-buffer)
|      (setq default-directory dir)
|      ;; Start the find process.
| -    (shell-command (concat command "&") (current-buffer))
| -    (let ((proc (get-buffer-process (current-buffer))))
| +    (let ((proc (start-file-process-shell-command
| +                 (buffer-name) (current-buffer) command)))
|        ;; Initialize the process marker; it is used by the filter.
|        (move-marker (process-mark proc) (point) (current-buffer))
|        (set-process-filter proc #'find-dired-filter)

causes the following for me:

I am using this setting:

#+begin_src emacs-lisp
(setq find-ls-option
      (setq find-ls-option-default-exec
            '("-exec ls -adhl {} +" . "-adhl")))
#+end_src

and the *Find* buffer now contains lots of lines like

'./Zacher/Mauricio Kagel - Fantasia for Organ.mp4'

or

"./Gideon Klein/Gideon Klein's 'Praeludium,' Played by Arturo Fernandez.mp4"

i.e. lines with file names quoted inside '...' or "...".  These lines
are not functional - dired can't handle them.  I can reproduce this
problem with emacs -Q.  The above setting worked ok before.

Is this expected and my fault or a regression?


TIA,

Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hello Michael and Warren,

Hi Michael,

> I am using this setting:
>
> #+begin_src emacs-lisp
> (setq find-ls-option
>       (setq find-ls-option-default-exec
>             '("-exec ls -adhl {} +" . "-adhl")))
> #+end_src
>
> and the *Find* buffer now contains lots of lines like
>
> './Zacher/Mauricio Kagel - Fantasia for Organ.mp4'
>
> or
>
> "./Gideon Klein/Gideon Klein's 'Praeludium,' Played by Arturo Fernandez.mp4"
>
> i.e. lines with file names quoted inside '...' or "...".  These lines
> are not functional - dired can't handle them.  I can reproduce this
> problem with emacs -Q.  The above setting worked ok before.
>
> Is this expected and my fault or a regression?

I don't know what's expected. But w/o your setting of find-ls-option,
Emacs sends the command

--8<---------------cut here---------------start------------->8---
find . \( -name \*\ \* \) -ls
--8<---------------cut here---------------end--------------->8---

which works also fine in a shell outside Emacs. With your setting, Emacs
sends

--8<---------------cut here---------------start------------->8---
find . \( -name \*\ \* \) -exec ls -adhl \{\} +
--8<---------------cut here---------------end--------------->8---

which returns indeed quoted file names as you have shown. What if you
set instead

(setq find-ls-option
      (setq find-ls-option-default-exec
            '("-exec ls -abdhl {} +" . "-abdhl")))

> TIA,
>
> Michael.

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Michael Albinus <michael.albinus@gmx.de> writes:

> find . \( -name \*\ \* \) -exec ls -adhl \{\} +
>
> which returns indeed quoted file names as you have shown. What if you
> set instead
>
> (setq find-ls-option
>       (setq find-ls-option-default-exec
>             '("-exec ls -abdhl {} +" . "-abdhl")))

Then the quotes are completely gone.

Obviously I don't want to need use -b however, since the escaped space
chars in the file names add a lot of noise.

I don't understand what is going on.  I tried to compare the situations
before and after the patch.  In both cases the `call-process' parameters
are more or less the same.  The process sentinels already seem to
receive different strings (one time without, the other time including
the quotes) - so seems it's not about sentinels.

Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I don't understand what is going on.  I tried to compare the situations
> before and after the patch.  In both cases the `call-process' parameters
                                                  ^
                                                  start-process

> are more or less the same.  The process sentinels already seem to
> receive different strings (one time without, the other time including
> the quotes) - so seems it's not about sentinels.

Also `process-environment' doesn't make a difference.  I'm out of
guesses.

Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Michael Albinus <michael.albinus@gmx.de> writes:

> What if you set instead
>
> (setq find-ls-option
>       (setq find-ls-option-default-exec
>             '("-exec ls -abdhl {} +" . "-abdhl")))

-N is the ls option I would want: it suppresses quoting entirely,
apparently without breaking anything.

But I still don't understand why that was not necessary before your
change.

Note that for some users the default value of `find-ls-option' can have
a form like above (AFAIU when the installed `find` executable does not
support the -ls option - see the definition).  These users will also
have this problem.  I tested this with emacs -Q: if the

#+begin_src emacs-lisp
(eq 0
  (ignore-errors
    (process-file find-program nil nil nil null-device "-ls")))
#+end_src

test fails, you get the problem without changing any variable (Bug).

Do you want to care about this, or should I open a separate bug report,
or reopen this one?


Regards,

Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Michael Heerdegen <michael_heerdegen@web.de> writes:

Hi Michael,

> Do you want to care about this, or should I open a separate bug report,
> or reopen this one?

I don't know what I could do for you. I've reverted
7bbd7cae0748958a623f23637b95a6fc9debb8b7 in the emacs-29 branch. Then
I've started in parallel Emacs from the emacs-29 branch with this
change, and Emacs from the master branch. Both with option -Q. I've evall'ed

(trace-function-foreground 'start-process)

in both cases, nothing else.

1. test: In my ~/tmp directory there are files with spaces in their
name. I've called "M-x find-name-dired RET ~/tmp RET * RET". In both
cases I see

--8<---------------cut here---------------start------------->8---
   11017308      4 -rw-r--r--   1 albinus  albinus         2 Aug 12 14:26 1\ 2\ 3
--8<---------------cut here---------------end--------------->8---

In *trace-output*, there is

--8<---------------cut here---------------start------------->8---
1 -> (start-process "Shell" #<buffer *Find*> "/usr/bin/tcsh" "-c" "find . \\( -name \\* \\) -ls")
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
1 -> (start-process "*Find*" #<buffer *Find*> "/usr/bin/tcsh" "-c" "find . \\( -name \\* \\) -ls")
--8<---------------cut here---------------end--------------->8---

Identical arguments, different buffer names. As expected.

2. test: I've evall'ed in both Emacs instances your customization

--8<---------------cut here---------------start------------->8---
(setq find-ls-option
      (setq find-ls-option-default-exec
            '("-exec ls -adhl {} +" . "-adhl")))
--8<---------------cut here---------------end--------------->8---

In both Emacs instances, I've called again "M-x find-name-dired RET ~/tmp RET * RET".
I see

--8<---------------cut here---------------start------------->8---
  -rw-r--r--.   1 albinus albinus    2 Aug 12 14:26 './1 2 3'
--8<---------------cut here---------------end--------------->8---

in both cases. That's bad, but there are no regressions due to the
patch! And indeed, in *trace-output*, there is

--8<---------------cut here---------------start------------->8---
1 -> (start-process "Shell" #<buffer *Find*> "/usr/bin/tcsh" "-c" "find . \\( -name \\* \\) -exec ls -adhl \\{\\} +")
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
1 -> (start-process "*Find*" #<buffer *Find*> "/usr/bin/tcsh" "-c" "find . \\( -name \\* \\) -exec ls -adhl \\{\\} +")
--8<---------------cut here---------------end--------------->8---

Identical arguments, different buffer names. As expected.

> Regards,
>
> Michael.

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Michael Albinus <michael.albinus@gmx.de> writes:

> I don't know what I could do for you. I've reverted
> 7bbd7cae0748958a623f23637b95a6fc9debb8b7 in the emacs-29 branch. Then
> I've started in parallel Emacs from the emacs-29 branch with this
> change, and Emacs from the master branch. Both with option -Q. I've evall'ed
>
> (trace-function-foreground 'start-process)
>
> in both cases, nothing else.

You are correct, it seems I wasted your time: your change didn't
introduce a regression.  I'm very sorry.

Now I can't make sense of the past days.  I particular I don't
understand why I did not see the quoted file names in find-dired buffers
until some days ago - although I used find-dired nearly daily, and
didn't change any related settings.  The problem appeared at the time of
your patch, but now I can reproduce the same issue with older Emacs
versions, too.  I must be missing something.

Anyway, what do you think about adding the -N switch to the `ls`
switches to prevent the broken file lines?  Do you know if it would
introduce any problems or is not available for all `ls` versions we have
to expect?

Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Michael Heerdegen <michael_heerdegen@web.de> writes:

Hi Michael,

> You are correct, it seems I wasted your time: your change didn't
> introduce a regression.  I'm very sorry.

No problem. My main job as Tramp maintainer is to explain other people
their configuration :-)

> Now I can't make sense of the past days.  I particular I don't
> understand why I did not see the quoted file names in find-dired buffers
> until some days ago - although I used find-dired nearly daily, and
> didn't change any related settings.  The problem appeared at the time of
> your patch, but now I can reproduce the same issue with older Emacs
> versions, too.  I must be missing something.

GNU ls has changed its default how to quote special characters in file
names. Likely, you've got a recent version of coreutils on your machine.

This change has raised controversioal discussions, but now it
exists. See <https://www.gnu.org/software/coreutils/quotes.html>.

> Anyway, what do you think about adding the -N switch to the `ls`
> switches to prevent the broken file lines?  Do you know if it would
> introduce any problems or is not available for all `ls` versions we have
> to expect?

"-N" isn't a default ls option. It doesn't work on my QNAP NAS, for
example (being busybox ls there). See
<https://pubs.opengroup.org/onlinepubs/009696899/utilities/ls.html> for
POSIX conform ls arguments.

> Michael.

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Eli Zaretskii]

> Cc: 64897@debbugs.gnu.org, Warren Lynn <wrn.lynn@gmail.com>
> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sun, 13 Aug 2023 09:42:50 +0200
> 
> GNU ls has changed its default how to quote special characters in file
> names. Likely, you've got a recent version of coreutils on your machine.
> 
> This change has raised controversioal discussions, but now it
> exists. See <https://www.gnu.org/software/coreutils/quotes.html>.

Maybe we should invoke 'ls' via a pipe, rather than PTY, so that this
change (which AFAIU we don't need in Dired) doesn't affect us?  Of
course, this will probably not help remote invocations.  But maybe we
should inject "QUOTING_STYLE=literal" into the environment in those
cases?

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> GNU ls has changed its default how to quote special characters in file
>> names. Likely, you've got a recent version of coreutils on your machine.
>>
>> This change has raised controversioal discussions, but now it
>> exists. See <https://www.gnu.org/software/coreutils/quotes.html>.
>
> Maybe we should invoke 'ls' via a pipe, rather than PTY, so that this
> change (which AFAIU we don't need in Dired) doesn't affect us?  Of
> course, this will probably not help remote invocations.  But maybe we
> should inject "QUOTING_STYLE=literal" into the environment in those
> cases?

I don't believe we must change anything. Calling find-name-dired from
emacs -Q, it ends up in "find . \( -name <pattern> \) -ls". This works
as expected, because the -ls argument of find is not affected by the
change in GNU ls.

The problem happens only, if people like Michael prefer to use the ls
command directly. In that case they are responsible themselves to use
proper ls arguments, matching the ls version on their system.

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: michael_heerdegen@web.de,  64897@debbugs.gnu.org,  wrn.lynn@gmail.com
> Date: Sun, 13 Aug 2023 11:33:09 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This change has raised controversioal discussions, but now it
> >> exists. See <https://www.gnu.org/software/coreutils/quotes.html>.
> >
> > Maybe we should invoke 'ls' via a pipe, rather than PTY, so that this
> > change (which AFAIU we don't need in Dired) doesn't affect us?  Of
> > course, this will probably not help remote invocations.  But maybe we
> > should inject "QUOTING_STYLE=literal" into the environment in those
> > cases?
> 
> I don't believe we must change anything. Calling find-name-dired from
> emacs -Q, it ends up in "find . \( -name <pattern> \) -ls". This works
> as expected, because the -ls argument of find is not affected by the
> change in GNU ls.
> 
> The problem happens only, if people like Michael prefer to use the ls
> command directly. In that case they are responsible themselves to use
> proper ls arguments, matching the ls version on their system.

I was thinking about Dired, not about find-name-dired.

Are you saying that Dired is not affected by this change in Coreutils'
behavior?

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> I was thinking about Dired, not about find-name-dired.
>
> Are you saying that Dired is not affected by this change in Coreutils'
> behavior?

Don't know. I haven't seen such bug reports. And the coreutils change
isn't brand new, I guess it has hit major Linux distros already.

--8<---------------cut here---------------start------------->8---
# grep PRETTY_NAME /etc/os-release
PRETTY_NAME="Fedora Linux 38 (Workstation Edition)"
# ls --version
ls (GNU coreutils) 9.1
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
# grep PRETTY_NAME /etc/os-release
PRETTY_NAME="Ubuntu 23.04"
# ls --version
ls (GNU coreutils) 9.1
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I was thinking about Dired, not about find-name-dired.
>
> Are you saying that Dired is not affected by this change in Coreutils'
> behavior?

If you look in `dired-insert-directory': this is already handled for
Dired in a careful way.  As requested in Bug#63142, fixed by a certain
Eli Z in May :-)

Should we do the same for find-dired?

Michael.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  64897@debbugs.gnu.org,
>   wrn.lynn@gmail.com
> Date: Mon, 14 Aug 2023 03:25:57 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I was thinking about Dired, not about find-name-dired.
> >
> > Are you saying that Dired is not affected by this change in Coreutils'
> > behavior?
> 
> If you look in `dired-insert-directory': this is already handled for
> Dired in a careful way.  As requested in Bug#63142, fixed by a certain
> Eli Z in May :-)

Great minds think alike!

> Should we do the same for find-dired?

Maybe, I didn't research the issue.  Feel free to suggest patches.

bug#64897: 29.0.91; Bug (and patch) in find-dired-with-command

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe, I didn't research the issue.  Feel free to suggest patches.

I did research now and understand now why Michael A. said that it would
not be necessary.  He is right.

The default behavior of find-dired is that all files have special
characters quoted.  Even if you insert subdirectories or refresh file
lines, -b is used for ls calls.  Unless we want to change that (I find
the -b output harder to read, but that is a different matter) the change
of the output of `ls` doesn't seem to make changes in find-dired.el
necessary.

Michael.