bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

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


Goto the *scratch* buffer, and perform

M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)

Move the cursor into the string /tmp/, and perform

M-x describe-char

There is no text property 'dired-filename, as it should.

The following patch seems to cure the problem. Run the same test, you
will see the text property 'dired-filename.


[-- Attachment #2: Type: text/plain, Size: 504 bytes --]

diff --git a/lisp/ls-lisp.el b/lisp/ls-lisp.el
index caddc7f760..6765cc8dc9 100644
--- a/lisp/ls-lisp.el
+++ b/lisp/ls-lisp.el
@@ -841,9 +841,7 @@ ls-lisp-format
 	    " "
 	    (ls-lisp-format-time file-attr time-index)
 	    " "
-	    (if (not (memq ?F switches)) ; ls-lisp-classify already did that
-		(propertize file-name 'dired-filename t)
-	      file-name)
+	    (propertize file-name 'dired-filename t)
 	    (if (stringp file-type)	; is a symbolic link
 		(concat " -> " file-type))
 	    "\n"

[-- Attachment #3: Type: text/plain, Size: 7351 bytes --]


This is a minor problem only. But I've stumbled over it, working on new
Tramp tests. Any objection to install the patch?


In GNU Emacs 27.0.50 (build 14, x86_64-pc-linux-gnu, GTK+ Version 3.22.24)
 of 2017-11-23 built on detlef
Repository revision: 0092a856ff3900c3771408893fb7fd8d731de568
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description:	Ubuntu 17.10

Recent messages:
Buffer B: Processing difference region 0 of 2
Processing difference regions ... done
Refining difference region 1 ...
Saving file /usr/local/src/emacs/lisp/ls-lisp.el...
Wrote /usr/local/src/emacs/lisp/ls-lisp.el
ls-lisp-format
Continue...
nil
Type C-x 1 to delete the help window, C-M-v to scroll help.
Mark set

Configured using:
 'configure --with-file-notification=inotify'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 LCMS2

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8

Major mode: Lisp Interaction

Minor modes in effect:
  diff-auto-refine-mode: t
  erc-notify-mode: t
  erc-notifications-mode: t
  erc-list-mode: t
  erc-menu-mode: t
  erc-autojoin-mode: t
  erc-ring-mode: t
  erc-networks-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-match-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-netsplit-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  url-handler-mode: t
  display-time-mode: t
  shell-dirtrack-mode: t
  icomplete-mode: t
  show-paren-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/albinus/src/elpa/packages/debbugs/debbugs-org hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-org
/home/albinus/src/elpa/packages/debbugs/debbugs-gnu hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-gnu
/home/albinus/src/elpa/packages/debbugs/debbugs hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs
/home/albinus/src/elpa/packages/debbugs/debbugs-autoloads hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-autoloads
/home/albinus/src/elpa/packages/debbugs/debbugs-pkg hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-pkg
/home/albinus/src/elpa/packages/debbugs/debbugs-browse hides /home/albinus/.emacs.d/elpa/debbugs-0.14/debbugs-browse
/home/albinus/.emacs.d/elpa/counsel-20171101.1121/counsel hides /home/albinus/.emacs.d/elpa/ivy-0.9.1/counsel
/home/albinus/.emacs.d/elpa/swiper-20171105.42/swiper hides /home/albinus/.emacs.d/elpa/ivy-0.9.1/swiper
/home/albinus/src/elpa/packages/tramp-theme/tramp-theme hides /home/albinus/.emacs.d/elpa/tramp-theme-0.2/tramp-theme
/home/albinus/src/elpa/packages/tramp-theme/tramp-theme-autoloads hides /home/albinus/.emacs.d/elpa/tramp-theme-0.2/tramp-theme-autoloads
/home/albinus/src/elpa/packages/tramp-theme/tramp-theme-pkg hides /home/albinus/.emacs.d/elpa/tramp-theme-0.2/tramp-theme-pkg
/home/albinus/.emacs.d/elpa/telepathy-20131209.458/telepathy hides ~/lisp/telepathy
~/src/tramp/lisp/tramp-smb hides /usr/local/src/emacs/lisp/net/tramp-smb
~/src/tramp/lisp/tramp-uu hides /usr/local/src/emacs/lisp/net/tramp-uu
~/src/tramp/lisp/tramp-adb hides /usr/local/src/emacs/lisp/net/tramp-adb
~/src/tramp/lisp/tramp-cmds hides /usr/local/src/emacs/lisp/net/tramp-cmds
~/src/tramp/lisp/tramp-cache hides /usr/local/src/emacs/lisp/net/tramp-cache
~/src/tramp/lisp/trampver hides /usr/local/src/emacs/lisp/net/trampver
~/src/tramp/lisp/tramp-ftp hides /usr/local/src/emacs/lisp/net/tramp-ftp
~/src/tramp/lisp/tramp-sh hides /usr/local/src/emacs/lisp/net/tramp-sh
~/src/tramp/lisp/tramp hides /usr/local/src/emacs/lisp/net/tramp
~/src/tramp/lisp/tramp-loaddefs hides /usr/local/src/emacs/lisp/net/tramp-loaddefs
~/lisp/dbus hides /usr/local/src/emacs/lisp/net/dbus
~/src/tramp/lisp/tramp-gvfs hides /usr/local/src/emacs/lisp/net/tramp-gvfs
~/src/tramp/lisp/tramp-compat hides /usr/local/src/emacs/lisp/net/tramp-compat

Features:
(shadow sort mail-extr warnings emacsbug message rmc puny rfc822 mml
mml-sec epa derived epg gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
add-log log-view pcvs-util ediff-vers ediff-merg ediff-wind ediff-diff
ediff-mult ediff-help ediff-init ediff-util ediff erc-replace
magit-utils crm cus-edit descr-text time-stamp misearch multi-isearch
tramp-adb tramp-cmds tramp-ftp cl-print edebug eieio-opt speedbar
sb-image ezimage dframe help-fns radix-tree ls-lisp files-x cl-extra
help-mode tramp-archive-tests tramp-archive tramp-gvfs zeroconf url-util
ert find-func ewoc debug vc-hg vc-git diff-mode easy-mmode bug-reference
map cus-start cus-load elec-pair erc-notify erc-desktop-notifications
notifications dbus xml erc-list erc-menu erc-join erc-ring erc-networks
erc-pcomplete erc-track erc-match erc-button wid-edit erc-fill erc-stamp
erc-netsplit erc-goodies erc erc-backend erc-compat thingatpt pp
cperl-mode tramp-theme em-dirs esh-var esh-io esh-cmd esh-opt esh-ext
esh-proc esh-arg esh-groups eshell esh-module esh-mode esh-util
finder-inf rx docker-tramp tramp-cache slime-autoloads vagrant-tramp
dash term disp-table ehelp info package easymenu epg-config url-handlers
url-parse url-vars time tramp-sh tramp tramp-compat tramp-loaddefs
trampver ucs-normalize shell pcomplete comint ansi-color ring parse-time
format-spec advice auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache ido seq byte-opt gv bytecomp byte-compile
cconv jka-compr icomplete paren vc cl-loaddefs cl-lib vc-dispatcher
dired dired-loaddefs time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 591729 78592)
 (symbols 48 51583 5)
 (miscs 40 1187 1629)
 (strings 32 163059 9766)
 (string-bytes 1 4144809)
 (vectors 16 66157)
 (vector-slots 8 2194248 184812)
 (floats 8 143 706)
 (intervals 56 12315 1573)
 (buffers 992 44))

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Fri, 24 Nov 2017 13:45:45 +0100
> 
> Goto the *scratch* buffer, and perform
> 
> M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)
> 
> Move the cursor into the string /tmp/, and perform
> 
> M-x describe-char
> 
> There is no text property 'dired-filename, as it should.
> 
> The following patch seems to cure the problem. Run the same test, you
> will see the text property 'dired-filename.
> 
> 
> [2:text/plain Hide]
> 
> diff --git a/lisp/ls-lisp.el b/lisp/ls-lisp.el
> index caddc7f760..6765cc8dc9 100644
> --- a/lisp/ls-lisp.el
> +++ b/lisp/ls-lisp.el
> @@ -841,9 +841,7 @@ ls-lisp-format
>  	    " "
>  	    (ls-lisp-format-time file-attr time-index)
>  	    " "
> -	    (if (not (memq ?F switches)) ; ls-lisp-classify already did that
> -		(propertize file-name 'dired-filename t)
> -	      file-name)
> +	    (propertize file-name 'dired-filename t)
>  	    (if (stringp file-type)	; is a symbolic link
>  		(concat " -> " file-type))
>  	    "\n"

How come ls-lisp-classify doesn't propertize the file name in this
case?

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> How come ls-lisp-classify doesn't propertize the file name in this
> case?

Because it wasn't called. ls-lisp-classify-file was called only, if I'm
not mistaken.

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 29423@debbugs.gnu.org
> Date: Fri, 24 Nov 2017 14:41:33 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How come ls-lisp-classify doesn't propertize the file name in this
> > case?
> 
> Because it wasn't called.

Then we should at least delete the propertize call from
ls-lisp-classify, as part of your patch, no?

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Then we should at least delete the propertize call from
> ls-lisp-classify, as part of your patch, no?

Yes. At least my (new) Tramp tests still pass, after I've done
this. Don't know, whether there are other insert-directory tests in
Emacs' test subdirectory.

Shall I commit to master?

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 29423@debbugs.gnu.org
> Date: Fri, 24 Nov 2017 16:13:51 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Then we should at least delete the propertize call from
> > ls-lisp-classify, as part of your patch, no?
> 
> Yes. At least my (new) Tramp tests still pass, after I've done
> this. Don't know, whether there are other insert-directory tests in
> Emacs' test subdirectory.
> 
> Shall I commit to master?

Yes, please, and thanks.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Drew Adams]

> Goto the *scratch* buffer, and perform
> M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)
> Move the cursor into the string /tmp/, and perform
> M-x describe-char
> 
> There is no text property 'dired-filename, as it should.

I think you're talking (only) about the final / char.
That seems to be the only place where the property is not
present.

Is that / part of the (directory as) file name?  Dunno
whether that consideration helps here - probably not.
What to cover by the property really depends on what the
property is used for.

Unfortunately perhaps, unlike the case for functions and
variables, there is no doc string for text properties.
Unless something is called out for this in some doc string
or in code comments, only the current uses of the property
can guide what it should apply to.

I don't know whether the / should have that property.
I have checked and see that in Emacs 22 it has it, and
thereafter it does not.  Regression?  Intentional change?

The current uses of the property, as I quickly check them
don't suggest that it matters whether / has the property.

Do you have something (e.g. some use case) particular in
mind, where you think that the / should have the property?

Should we consider code that expects the result of checking
for that property to give the same position regardless of
whether switch `F' is used?  Should the file name be
considered to be the same, regardless of whether a / is
appended?

In sum, is this a bug to be fixed, a design question, or
design that was already changed intentionally for Emacs 23?

(I have no idea, and no code of mine depends on what is
decided, AFAIK.)

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Drew Adams <drew.adams@oracle.com> writes:

Hi Drew,

>> Goto the *scratch* buffer, and perform
>> M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)
>> Move the cursor into the string /tmp/, and perform
>> M-x describe-char
>>
>> There is no text property 'dired-filename, as it should.
>
> I think you're talking (only) about the final / char.
> That seems to be the only place where the property is not
> present.

Yes. Finally, it was triggered by Tramp using the F switch. This was
added by Tramp in order to get a trailing / for directory names in the
directory listing.

> Is that / part of the (directory as) file name?  Dunno
> whether that consideration helps here - probably not.
> What to cover by the property really depends on what the
> property is used for.

No, it is not part of the file name. Like the trailing " -> foo" for
symlinks.

My problem is, that the whole file name is missing the text property,
not only the trailing slash.

> Unfortunately perhaps, unlike the case for functions and
> variables, there is no doc string for text properties.
> Unless something is called out for this in some doc string
> or in code comments, only the current uses of the property
> can guide what it should apply to.

I agree. However, in the given case, the text property 'dired-filename
shall highlight exactly the file name. It is used later in dired (as the
name of the property says), but it is also used in Tramp, because for
some of the Tramp backends, ls-lisp-insert-directory is used internally,
and Tramp needs some massage on the result.

> I don't know whether the / should have that property.

No.

> I have checked and see that in Emacs 22 it has it, and
> thereafter it does not.  Regression?  Intentional change?

Likely intentionally.

> The current uses of the property, as I quickly check them
> don't suggest that it matters whether / has the property.
>
> Do you have something (e.g. some use case) particular in
> mind, where you think that the / should have the property?

Again, this is not my point. My point is, that the name itself does not
have the property.

> Should we consider code that expects the result of checking
> for that property to give the same position regardless of
> whether switch `F' is used?  Should the file name be
> considered to be the same, regardless of whether a / is
> appended?

Parsing the output of any insert-directory is a pain. Therefore, it is
helpful to know that the file name part of this output is marked with
'dired-filename.

Even in backends where Tramp does not use ls-lisp-insert-directory,
Tramp tries its best to set this text property for dired. See
tramp-sh-handle-insert-directory, for example.

> In sum, is this a bug to be fixed, a design question, or
> design that was already changed intentionally for Emacs 23?
>
> (I have no idea, and no code of mine depends on what is
> decided, AFAIK.)

I believe it is a bug.

I have just applied my patch locally, and I have rerun the whole Emacs
testsuite. Several errors did appear I haven't seen before. I'll check,
whether they are related to my change.

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> Date: Fri, 24 Nov 2017 08:30:47 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> 
> > Goto the *scratch* buffer, and perform
> > M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)
> > Move the cursor into the string /tmp/, and perform
> > M-x describe-char
> > 
> > There is no text property 'dired-filename, as it should.
> 
> I think you're talking (only) about the final / char.
> That seems to be the only place where the property is not
> present.
> 
> Is that / part of the (directory as) file name?

No, it isn't.  It's what -F append to the file name to indicate that
it's a directory.

So if that's the problem, the patch is not TRT, IMO.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Drew Adams]

> > I think you're talking (only) about the final / char.
> > That seems to be the only place where the property is not
> > present.
> 
> Yes. Finally, it was triggered by Tramp using the F switch. This was
> added by Tramp in order to get a trailing / for directory names in the
> directory listing.

You said "Yes", but below you seem to say "No".  Is the
/ the only place where the property is not present?

> > Is that / part of the (directory as) file name?  Dunno
> > whether that consideration helps here - probably not.
> > What to cover by the property really depends on what the
> > property is used for.
> 
> No, it is not part of the file name. Like the trailing " -> foo" for
> symlinks.
> 
> My problem is, that the whole file name is missing the text property,
> not only the trailing slash.

(See above for (my) confusion about this.)

That's not what I see, in any Emacs release or in the
26.1 prerelease (MS Windows binary).  Perhaps what you
see is a problem introduced after that prerelease or
is platform-dependent?

> > Unfortunately perhaps, unlike the case for functions and
> > variables, there is no doc string for text properties.
> > Unless something is called out for this in some doc string
> > or in code comments, only the current uses of the property
> > can guide what it should apply to.
> 
> I agree. However, in the given case, the text property 'dired-filename
> shall highlight exactly the file name. It is used later in dired (as the
> name of the property says), but it is also used in Tramp, because for
> some of the Tramp backends, ls-lisp-insert-directory is used internally,
> and Tramp needs some massage on the result.

I definitely agree that the name of the directory
(sans /) needs the property.  If that's missing
then there is likely a regression.

> Parsing the output of any insert-directory is a pain. Therefore, it is
> helpful to know that the file name part of this output is marked with
> 'dired-filename.

I agree 100% that the name needs the property.
As the only char I see missing the property is the
final /, I thought that's what you were asking about
and reporting as a problem.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Drew Adams <drew.adams@oracle.com> writes:

Hi Drew,

> That's not what I see, in any Emacs release or in the
> 26.1 prerelease (MS Windows binary).  Perhaps what you
> see is a problem introduced after that prerelease or
> is platform-dependent?

I've just checked again my recipe with the Emacs 26 branch, started as
"emacs -Q". Reproduced. And I could reproduce it also with Emacs 25, as
provided by Ubuntu 17.10.

Maybe the difference is that I haven't said explicitly that you need
(require 'ls-lisp) prior my recipe. I thought it was obvious, due to the
subject of the bug report. Sorry.

> As the only char I see missing the property is the
> final /, I thought that's what you were asking about
> and reporting as a problem.

Use ls-lisp.

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Is that / part of the (directory as) file name?
>
> No, it isn't.  It's what -F append to the file name to indicate that
> it's a directory.
>
> So if that's the problem, the patch is not TRT, IMO.

I'll check.

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Drew Adams]

> > That's not what I see, in any Emacs release or in the
> > 26.1 prerelease (MS Windows binary).  Perhaps what you
> > see is a problem introduced after that prerelease or
> > is platform-dependent?
> 
> I've just checked again my recipe with the Emacs 26 branch, started as
> "emacs -Q". Reproduced. And I could reproduce it also with Emacs 25, as
> provided by Ubuntu 17.10.
> 
> Maybe the difference is that I haven't said explicitly that you need
> (require 'ls-lisp) prior my recipe. I thought it was obvious, due to the
> subject of the bug report. Sorry.
> 
> > As the only char I see missing the property is the
> > final /, I thought that's what you were asking about
> > and reporting as a problem.
> 
> Use ls-lisp.

I did it again, from emacs -Q, with the Emacs 26.1 pretest.

I tried with M-x load-library ls-lisp.el, and
I tried with M-x load-library ls-lisp.elc.  And I
think that neither should be needed, since Emacs
on MS Windows (which I'm using) uses ls-lisp by
default.

I still see what I reported earlier: the property
is on the directory name (but not on the /).

I'm guessing that the Emacs 26 you're using is
something later than the pretest.  Or else the
difference has something to do with the platform.

HTH.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Noam Postavsky]

On Fri, Nov 24, 2017 at 2:28 PM, Drew Adams <drew.adams@oracle.com> wrote:

> I did it again, from emacs -Q, with the Emacs 26.1 pretest.
>
> I tried with M-x load-library ls-lisp.el, and
> I tried with M-x load-library ls-lisp.elc.  And I
> think that neither should be needed, since Emacs
> on MS Windows (which I'm using) uses ls-lisp by
> default.
>
> I still see what I reported earlier: the property
> is on the directory name (but not on the /).

I can reproduce according to Michael's instructions on MS-Windows, in
Emacs 24.5, 25.3, and an Emacs 26 pretest (I don't have exactly
26.0.90 handy though).

> I'm guessing that the Emacs 26 you're using is
> something later than the pretest.  Or else the
> difference has something to do with the platform.

Are you looking at a dired buffer? That's the only I can replicate
what you are reporting.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Drew Adams <drew.adams@oracle.com>,  29423@debbugs.gnu.org
> Date: Fri, 24 Nov 2017 19:49:56 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Is that / part of the (directory as) file name?
> >
> > No, it isn't.  It's what -F append to the file name to indicate that
> > it's a directory.
> >
> > So if that's the problem, the patch is not TRT, IMO.
> 
> I'll check.

The more I think about this the less I understand how your patch is
going to work.  How can ls-lisp-format know what was appended to a
file name by ls-lisp-classify-file?

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Drew Adams]

> > I did it again, from emacs -Q, with the Emacs 26.1 pretest.
> >
> > I tried with M-x load-library ls-lisp.el, and
> > I tried with M-x load-library ls-lisp.elc.  And I
> > think that neither should be needed, since Emacs
> > on MS Windows (which I'm using) uses ls-lisp by
> > default.
> >
> > I still see what I reported earlier: the property
> > is on the directory name (but not on the /).
> 
> I can reproduce according to Michael's instructions on MS-Windows, in
> Emacs 24.5, 25.3, and an Emacs 26 pretest (I don't have exactly
> 26.0.90 handy though).

OK.  If you can repro it then I'm probably not helping
here.

> > I'm guessing that the Emacs 26 you're using is
> > something later than the pretest.  Or else the
> > difference has something to do with the platform.
> 
> Are you looking at a dired buffer? That's the only I can replicate
> what you are reporting.

I can't parse your last sentence.  Yes, I was looking
at a dired buffer.

I tested with this pretest build:

In GNU Emacs 26.0.90 (build 3, x86_64-w64-mingw32)
 of 2017-10-13 built on LAPHROAIG
Repository revision: 906224eba147bdfc0514090064e8e8f53160f1d4
Windowing system distributor 'Microsoft Corp.', version 6.1.7601

I just repeated the test with this other pretest build,
and I see the same things as before.

In GNU Emacs 26.0.90 (build 3, x86_64-w64-mingw32)
 of 2017-10-13 built on LAPHROAIG
Repository revision: 906224eba147bdfc0514090064e8e8f53160f1d4
Windowing system distributor 'Microsoft Corp.', version 6.1.7601

And I just repeated the test with Emacs 25.3.1,
and I see the same things I reported earlier.

In GNU Emacs 25.3.1 (x86_64-w64-mingw32)
 of 2017-09-26 built on LAPHROAIG
Windowing system distributor 'Microsoft Corp.', version 6.1.7601
Configured using:
 'configure --without-dbus --without-compress-install 'CFLAGS=-O2
 -static -g3' PKG_CONFIG_PATH=/mingw64/lib/pkgconfig'

HTH.

(I'm still guessing that you guys are using a more
recent Emacs 26 than these pretests, and that something
broke this since these pretest builds.)

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Noam Postavsky]

On Fri, Nov 24, 2017 at 3:27 PM, Drew Adams <drew.adams@oracle.com> wrote:

>> Are you looking at a dired buffer? That's the only I can replicate
                                                     ^
                                                     way
>> what you are reporting.

>  Yes, I was looking
> at a dired buffer.

Then you didn't follow the instructions in the OP:

    Goto the *scratch* buffer, and perform

    M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)

No calls to dired. The ls-lisp-insert-directory function does not
create a dired buffer.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Drew Adams]

> > Yes, I was looking at a dired buffer.
> 
> Then you didn't follow the instructions in the OP:
>     Goto the *scratch* buffer, and perform
>     M-: (ls-lisp-insert-directory "/tmp/" '(?F) nil nil nil)
> 
> No calls to dired. The ls-lisp-insert-directory function does not
> create a dired buffer.

I see, and I see what you see if I follow the recipe.
I missed that.  Sorry for the noise.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> The more I think about this the less I understand how your patch is
> going to work.

Me too.

> How can ls-lisp-format know what was appended to a file name by
> ls-lisp-classify-file?

Give me more time to check, please.

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 29423@debbugs.gnu.org
> Date: Fri, 24 Nov 2017 14:41:33 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How come ls-lisp-classify doesn't propertize the file name in this
> > case?
> 
> Because it wasn't called. ls-lisp-classify-file was called only, if I'm
> not mistaken.

Right, and so the correct fix is below, I think.  Do you agree?

We can install this on the release branch, unless the original problem
with Tramp exists only on master.  Let me know.

Thanks.

diff --git a/lisp/ls-lisp.el b/lisp/ls-lisp.el
index caddc7f..cf3bff5 100644
--- a/lisp/ls-lisp.el
+++ b/lisp/ls-lisp.el
@@ -713,23 +713,26 @@ ls-lisp-handle-switches
 (defun ls-lisp-classify-file (filename fattr)
   "Append a character to FILENAME indicating the file type.
 
+This function puts the `dired-filename' property on FILENAME, but
+not on the character indicator it appends.
 FATTR is the file attributes returned by `file-attributes' for the file.
 The file type indicators are `/' for directories, `@' for symbolic
 links, `|' for FIFOs, `=' for sockets, `*' for regular files that
 are executable, and nothing for other types of files."
   (let* ((type (car fattr))
 	 (modestr (nth 8 fattr))
-	 (typestr (substring modestr 0 1)))
+	 (typestr (substring modestr 0 1))
+         (file-name (propertize filename 'dired-filename t)))
     (cond
      (type
-      (concat filename (if (eq type t) "/" "@")))
+      (concat file-name (if (eq type t) "/" "@")))
      ((string-match "x" modestr)
-      (concat filename "*"))
+      (concat file-name "*"))
      ((string= "p" typestr)
-      (concat filename "|"))
+      (concat file-name "|"))
      ((string= "s" typestr)
-      (concat filename "="))
-     (t filename))))
+      (concat file-name "="))
+     (t file-name))))
 
 (defun ls-lisp-classify (filedata)
   "Append a character to file name in FILEDATA indicating the file type.
@@ -742,7 +745,6 @@ ls-lisp-classify
 are executable, and nothing for other types of files."
   (let ((file-name (car filedata))
         (fattr (cdr filedata)))
-    (setq file-name (propertize file-name 'dired-filename t))
     (cons (ls-lisp-classify-file file-name fattr) fattr)))
 
 (defun ls-lisp-extension (filename)

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Because it wasn't called. ls-lisp-classify-file was called only, if I'm
>> not mistaken.
>
> Right, and so the correct fix is below, I think.  Do you agree?

Yes. I've applied your patch instead of mine, and the corresponding test
still passes.

> We can install this on the release branch, unless the original problem
> with Tramp exists only on master.  Let me know.

Yes, please. The problem does not happen in existing Tramp, but in the
not published yet tramp-archive.el I'm working on. It shall support as
many older Emacsen as the existing Tramp; if it would be fixed in Emacs
26 it would be great.

If needed, I will add some compat code for older Emacsen (24, 25), as it
is tradition in Tramp.

> Thanks.

Best regards, Michael.

bug#29423: 27.0.50; ls-lisp does not handle -F switch properly

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: 29423@debbugs.gnu.org
> Date: Sat, 25 Nov 2017 09:59:52 +0100
> 
> > We can install this on the release branch, unless the original problem
> > with Tramp exists only on master.  Let me know.
> 
> Yes, please. The problem does not happen in existing Tramp, but in the
> not published yet tramp-archive.el I'm working on. It shall support as
> many older Emacsen as the existing Tramp; if it would be fixed in Emacs
> 26 it would be great.

Done.