Quotes in Dired listing switches

Quotes in Dired listing switches

[Juri Linkov]

To make file sizes in Dired more readable, `ls' provides the option
`--block-size'.  (info "(coreutils) What information is listed") says:

     `--block-size="'1"' prints a byte count with the thousands
     separator of the current locale.

However, this doesn't work in Dired when set by `dired-listing-switches'.
After setting

  (setq dired-listing-switches "-al --block-size=\"'1\"")

`C-x d' fails with the error message:

  insert-directory: Listing directory failed but `access-file' worked

because in `insert-directory', `call-process' called as

(apply 'call-process
       insert-directory-program nil t nil
       '("--dired" "-al" "--block-size=\"'1\"" "--" "/tmp/."))

creates a Dired buffer with the following line:

  /bin/ls: invalid --block-size argument `"'1"'

When using `--block-size' without double quotes

(setq dired-listing-switches "-al --block-size='1")

then Dired works correctly, but with e.g. `C-x d /sudo::/tmp'
Tramp hangs because the unmatched single quote causes the shell
to wait for the closing quote.

Fortunately, (info "(coreutils) Block size") mentions the
environment variable `LS_BLOCK_SIZE' that indeed works with:

  (setenv "LS_BLOCK_SIZE" "'1")

without using Dired switches.

But I still have a question: would it be possible to solve this problem
without resorting to an environment variable that may not exist
for other similar cases?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Quotes in Dired listing switches

[Stefan Monnier]

>   (setq dired-listing-switches "-al --block-size=\"'1\"")
[...]
> (apply 'call-process
>        insert-directory-program nil t nil
>        '("--dired" "-al" "--block-size=\"'1\"" "--" "/tmp/."))
[...]
> then Dired works correctly, but with e.g. `C-x d /sudo::/tmp'
> Tramp hangs because the unmatched single quote causes the shell
> to wait for the closing quote.

Obviously, there's a problem in the inconsistent parsing of
dired-listing-switches, where the basic code splits it at spaces without
doing any additional unquoting or analysis, whereas the Tramp code seems
to just pass it as-is to the shell (so if you set it to "-al $(rm -rf ~/.)"
it will try to do something funny).
I won't claim that the basic code's naive splitting is great, but
Tramp's similarly naive use is not great either.  So, I suggest to treat
it as a bug in Tramp which should split it like the basic code and then
shell-quote-argument the parts.


        Stefan

Re: Quotes in Dired listing switches

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Obviously, there's a problem in the inconsistent parsing of
> dired-listing-switches, where the basic code splits it at spaces without
> doing any additional unquoting or analysis, whereas the Tramp code seems
> to just pass it as-is to the shell (so if you set it to "-al $(rm -rf ~/.)"
> it will try to do something funny).
> I won't claim that the basic code's naive splitting is great, but
> Tramp's similarly naive use is not great either.  So, I suggest to treat
> it as a bug in Tramp which should split it like the basic code and then
> shell-quote-argument the parts.

In tramp.el, I've added quoting of the "'" character in SWITCHES, passed
by `insert-directory', which shall be sufficient.

However, remote directory listings won't show a "thousands separator of
the current locale", because Tramps sets LC_ALL to "C" on the remote
hosts. The C locale has no thousands separator. If you (Juri) want to
change this, you might customize `tramp-remote-process-environment',
setting LC_ALL to en_US.UTF-8 or so. But please be careful, because
Tramp reads output from some commands, and might expect English.

>         Stefan

Best regards, Michael.

Re: Quotes in Dired listing switches

[Juri Linkov]

> In tramp.el, I've added quoting of the "'" character in SWITCHES, passed
> by `insert-directory', which shall be sufficient.

Thanks, Tramp doesn't hang now.

> However, remote directory listings won't show a "thousands separator of
> the current locale", because Tramps sets LC_ALL to "C" on the remote
> hosts. The C locale has no thousands separator. If you (Juri) want to
> change this, you might customize `tramp-remote-process-environment',
> setting LC_ALL to en_US.UTF-8 or so. But please be careful, because
> Tramp reads output from some commands, and might expect English.

Yes, this helps to show a thousands separator, and also to display
non-ASCII characters in file names!  IMHO, it's a drawback that currently
Tramp doesn't support Unicode in Dired file names by default.

Is it possible to improve Tramp to set LC_ALL to "C" only for commands
where Tramp expects English?  Or just to set LC_MESSAGES to "C" if Tramp
expects only English messages?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Quotes in Dired listing switches

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

>> However, remote directory listings won't show a "thousands separator of
>> the current locale", because Tramps sets LC_ALL to "C" on the remote
>> hosts. The C locale has no thousands separator. If you (Juri) want to
>> change this, you might customize `tramp-remote-process-environment',
>> setting LC_ALL to en_US.UTF-8 or so. But please be careful, because
>> Tramp reads output from some commands, and might expect English.
>
> Yes, this helps to show a thousands separator, and also to display
> non-ASCII characters in file names!  IMHO, it's a drawback that currently
> Tramp doesn't support Unicode in Dired file names by default.
>
> Is it possible to improve Tramp to set LC_ALL to "C" only for commands
> where Tramp expects English?  Or just to set LC_MESSAGES to "C" if Tramp
> expects only English messages?

That's something I have thought about already. But it requires a very
precise review of Tramp's communication, in order to identify those
places in the code. And also LC_NUMERIC might be involved; I fear that
Tramp might be confused by reading numbers with a thousand separator.
(Hopefully, `tramp-send-command-and-read' is the only place, but I do
not know whether we have been careful enough writing tramp.el)

Furthermore, Tramp cannot expect Unicode on the remote machine. There
might be anything encoded ...

I put it on the todo list. I won't change it before Emacs 23.2 is out,
due to stability reasons.

Best regards, Michael.

Re: Quotes in Dired listing switches

[Juri Linkov]

>> Is it possible to improve Tramp to set LC_ALL to "C" only for commands
>> where Tramp expects English?  Or just to set LC_MESSAGES to "C" if Tramp
>> expects only English messages?
>
> That's something I have thought about already. But it requires a very
> precise review of Tramp's communication, in order to identify those
> places in the code. And also LC_NUMERIC might be involved; I fear that
> Tramp might be confused by reading numbers with a thousand separator.
> (Hopefully, `tramp-send-command-and-read' is the only place, but I do
> not know whether we have been careful enough writing tramp.el)
>
> Furthermore, Tramp cannot expect Unicode on the remote machine. There
> might be anything encoded ...

There is another related problem: Tramp doesn't display the Dired buffer
when an older version of `ls' on the remote machine doesn't support
a new switch supported by the local newer version of `ls'.

Something like below could handle this situation to remove unsupported
switches before running `ls' on the remote machine:

(add-hook 'dired-before-readin-hook
          (lambda ()
            (when (file-remote-p default-directory)
              (setq dired-actual-switches
                    (replace-regexp-in-string "--block-size='1" ""
                                              dired-actual-switches)))))

but this is an ugly hack.  Do you have an idea how to handle this problem?

> I put it on the todo list. I won't change it before Emacs 23.2 is out,
> due to stability reasons.

Yes, this is not an urgent problem, but rather a need for improvement.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Quotes in Dired listing switches

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

> There is another related problem: Tramp doesn't display the Dired buffer
> when an older version of `ls' on the remote machine doesn't support
> a new switch supported by the local newer version of `ls'.
>
> Something like below could handle this situation to remove unsupported
> switches before running `ls' on the remote machine:
>
> (add-hook 'dired-before-readin-hook
>           (lambda ()
>             (when (file-remote-p default-directory)
>               (setq dired-actual-switches
>                     (replace-regexp-in-string "--block-size='1" ""
>                                               dired-actual-switches)))))
>
> but this is an ugly hack.  Do you have an idea how to handle this problem?

There is an idea to introduce server-local environment variables. Then
you could keep different settings for `dired-actual-switches', depending
on the host Tramp is accessing.

See the todo on the end of tramp.el. I haven't started an implementation yet.

Best regards, Michael.

Re: Quotes in Dired listing switches

[Juri Linkov]

> There is an idea to introduce server-local environment variables. Then
> you could keep different settings for `dired-actual-switches', depending
> on the host Tramp is accessing.

It seems there are already some server-local variables in
~/.emacs.d/tramp.  At least, I see two relevant variables:
"ls" that has the value "/bin/ls" and "ls-dired" with the
value `t'.  Maybe a new variable would help to keep settings
for `dired-actual-switches'.

BTW, there is one problem with ~/.emacs.d/tramp.
After upgrading the kernel, Tramp fails with the message

  Tramp: Connection reset, because remote host changed from
  `Linux 2.6.24-23-generic' to `Linux 2.6.24-24-generic'.

because ~/.emacs.d/tramp has the old value of "uname".

Could you suggest what to do in this situation?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Quotes in Dired listing switches

[Michael Albinus]

Juri Linkov <juri@jurta.org> writes:

>> There is an idea to introduce server-local environment variables. Then
>> you could keep different settings for `dired-actual-switches', depending
>> on the host Tramp is accessing.
>
> It seems there are already some server-local variables in
> ~/.emacs.d/tramp.  At least, I see two relevant variables:
> "ls" that has the value "/bin/ls" and "ls-dired" with the
> value `t'.  Maybe a new variable would help to keep settings
> for `dired-actual-switches'.

Yes, but this are Tramp specific server local variables. The idea is to
behave like buffer-local or frame-local variables do, i.e. introduce a
mechanism to declare an existing global variable as "server local".

> BTW, there is one problem with ~/.emacs.d/tramp.
> After upgrading the kernel, Tramp fails with the message
>
>   Tramp: Connection reset, because remote host changed from
>   `Linux 2.6.24-23-generic' to `Linux 2.6.24-24-generic'.
>
> because ~/.emacs.d/tramp has the old value of "uname".

That's intended. Tramp expects that all cached values are invalid once
the operating system has changed. Just connect, again; Tramp has flushed
its cache. (There is the request that Tramp reconnects automatically,
but this is not easy to implement. See the todo ...)

Best regars, Michael.

Re: Quotes in Dired listing switches

[Stefan Monnier]

> However, remote directory listings won't show a "thousands separator of
> the current locale", because Tramps sets LC_ALL to "C" on the remote
> hosts. The C locale has no thousands separator. If you (Juri) want to
> change this, you might customize `tramp-remote-process-environment',
> setting LC_ALL to en_US.UTF-8 or so. But please be careful, because
> Tramp reads output from some commands, and might expect English.

That's a shortcoming of Tramp which should only use LC_ALL=C for
commands whose output is parsed by Tramp but not for those whose output
is passed to the caller.  Then again, we may have the problem of
choosing between the caller's locale and the remote host's default
locale, I don't know.  In any case it's probably not a terribly
important problem for now, but should be kept somewhere as a thing that
would be nice to fix.


        Stefan

Re: Quotes in Dired listing switches

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> That's a shortcoming of Tramp which should only use LC_ALL=C for
> commands whose output is parsed by Tramp but not for those whose output
> is passed to the caller.  Then again, we may have the problem of
> choosing between the caller's locale and the remote host's default
> locale, I don't know.  In any case it's probably not a terribly
> important problem for now, but should be kept somewhere as a thing that
> would be nice to fix.

Already noted in the Todo list of tramp.el (in my local branch). I'll
commit any change for this only after-the-release.

One idea is to check the $LANG and/or $LC_ALL settings on the remote
host, once connected, and to DTRT then. Whatever this is.

>         Stefan

Best regards, Michael.

Re: Quotes in Dired listing switches

[Juri Linkov]

>>   (setq dired-listing-switches "-al --block-size=\"'1\"")
> [...]
>> (apply 'call-process
>>        insert-directory-program nil t nil
>>        '("--dired" "-al" "--block-size=\"'1\"" "--" "/tmp/."))
> [...]
>> then Dired works correctly, but with e.g. `C-x d /sudo::/tmp'
>> Tramp hangs because the unmatched single quote causes the shell
>> to wait for the closing quote.
>
> Obviously, there's a problem in the inconsistent parsing of
> dired-listing-switches, where the basic code splits it at spaces without
> doing any additional unquoting or analysis, whereas the Tramp code seems
> to just pass it as-is to the shell (so if you set it to "-al $(rm -rf ~/.)"
> it will try to do something funny).
> I won't claim that the basic code's naive splitting is great, but
> Tramp's similarly naive use is not great either.  So, I suggest to treat
> it as a bug in Tramp which should split it like the basic code and then
> shell-quote-argument the parts.

There is still something wrong in `insert-directory'.  Currently it's
impossible to use both `C-x d dir-name RET' and `C-x d file-name RET'
(where the latter specifies a filename wildcard for dired).

With (setq dired-listing-switches "-al --block-size='1")
`insert-directory' succeeds for `C-x d /tmp/ RET', but fails
for `C-x d /tmp/file RET'.

With (setq dired-listing-switches "-al --block-size=\"'1\"")
`insert-directory' fails for `C-x d /tmp/ RET', and succeeds
for `C-x d /tmp/file RET'.

The difference in `insert-directory' is in the `if wildcard' condition:
the first branch calls `insert-directory-program' using `shell-file-name',
the second branch calls `insert-directory-program' directly.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Quotes in Dired listing switches

[Juri Linkov]

I discovered another case where unquoted file names cause a bug.

When a zip archive contains a file with square brackets in its name
(e.g. "file[name].txt") then visiting this file displays an error:

  caution: filename not matched:  file[name].txt

That's because `archive-extract-by-stdout' runs `call-process'
where the filename is unquoted:

  unzip -qq -c archive.zip file[name].txt

There are more calls of `call-process' in arc-mode.el, so perhaps
this is a more general problem.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Quotes in Dired listing switches

[Eli Zaretskii]

> From: Juri Linkov <juri@jurta.org>
> Date: Mon, 25 Jan 2010 11:29:54 +0200
> 
> When a zip archive contains a file with square brackets in its name
> (e.g. "file[name].txt") then visiting this file displays an error:
> 
>   caution: filename not matched:  file[name].txt
> 
> That's because `archive-extract-by-stdout' runs `call-process'
> where the filename is unquoted:
> 
>   unzip -qq -c archive.zip file[name].txt

No, that's because `unzip' expands wildcards in its command-line
arguments.  Since `call-process' doesn't go through the shell, the
arguments generally don't need to be quoted (except with `unzip',
argh).

I suggest that you file a bug report, because the fix is not trivial,
if we want to keep the code in arc-mode.el independent of the
customizable settings of the archive-*-extract etc. options.