bug#34915: 27.0.50; Wdired regression with ls -F

bug#34915: 27.0.50; Wdired regression with ls -F

[Basil L. Contovounesios]

X-Debbugs-Cc: Stephen Berman <stephen.berman@gmx.net>

The following behaviour is present in master but not emacs-26.
I haven't bisected the issue to Stephen's changes to Wdired, but I
checked that it wasn't caused by Stefan's recent changes to Dired.

0. emacs -Q
1. (progn (setq dired-listing-switches "-Fl")
          (dired (make-temp-file "my-wdired-" t)))
   C-j
2. M-x dired-create-empty-file RET exec RET
3. M +x RET
4. M-x wdired-change-to-wdired-mode RET
5. w C-c C-c
   1 rename actions failed--type ? for details
6. ?
   Rename ‘/tmp/my-wdired-tMSVuW/exec*’ to ‘/tmp/my-wdired-tMSVuW/wexec*’ failed:
   (file-missing Renaming No such file or directory
                 /tmp/my-wdired-tMSVuW/exec*
                 /tmp/my-wdired-tMSVuW/wexec*)
7. M-x dired-create-empty-file RET plain RET
8. M-x wdired-change-to-wdired-mode RET
9. w C-c C-c

This wdired-finish-edit succeeds, but instead of only renaming 'plain'
to 'wplain', it also renames 'exec' to 'exec*'.

It seems like Wdired thinks the indicators added by ls (one of */=>@|)
are part of the file name now.

-- 
Basil

In GNU Emacs 27.0.50 (build 4, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2019-03-19 built on thunk
Repository revision: c0936672876bccc15e7899e83d8ab99910f8feee
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Debian GNU/Linux buster/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O2 -march=native -pipe'
 --config-cache --prefix=/home/blc/.local --with-mailutils
 --with-x-toolkit=lucid --with-modules --with-file-notification=yes
 --with-x'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS
GLIB NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT
LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

bug#34915: 27.0.50; Wdired regression with ls -F

[Stephen Berman]

On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:

> X-Debbugs-Cc: Stephen Berman <stephen.berman@gmx.net>
>
> The following behaviour is present in master but not emacs-26.
> I haven't bisected the issue to Stephen's changes to Wdired, but I
> checked that it wasn't caused by Stefan's recent changes to Dired.
>
> 0. emacs -Q
> 1. (progn (setq dired-listing-switches "-Fl")
>           (dired (make-temp-file "my-wdired-" t)))
>    C-j
> 2. M-x dired-create-empty-file RET exec RET
> 3. M +x RET
> 4. M-x wdired-change-to-wdired-mode RET
> 5. w C-c C-c
>    1 rename actions failed--type ? for details
> 6. ?
>    Rename ‘/tmp/my-wdired-tMSVuW/exec*’ to ‘/tmp/my-wdired-tMSVuW/wexec*’ failed:
>    (file-missing Renaming No such file or directory
>                  /tmp/my-wdired-tMSVuW/exec*
>                  /tmp/my-wdired-tMSVuW/wexec*)
> 7. M-x dired-create-empty-file RET plain RET
> 8. M-x wdired-change-to-wdired-mode RET
> 9. w C-c C-c
>
> This wdired-finish-edit succeeds, but instead of only renaming 'plain'
> to 'wplain', it also renames 'exec' to 'exec*'.
>
> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
> are part of the file name now.

This is indeed due to my changes.  The patch below appears to fix the
problem, but I'm not sure how robust it is (I was also, and remain,
unsure about my handling of symlinks in the previous patch, but I
haven't found time to look at it more closely; at least I haven't seen
any bug reports about it so far).

Steve Berman

diff --git a/lisp/wdired.el b/lisp/wdired.el
index acc62e4e39..ca3e2f7845 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -612,14 +612,21 @@ wdired--restore-dired-filename-prop
         (when (re-search-forward
                directory-listing-before-filename-regexp lep t)
           (setq beg (point)
-                ;; If the file is a symlink, put the dired-filename
-                ;; property only on the link name.  (Using
-                ;; (file-symlink-p (dired-get-filename)) fails in
-                ;; wdired-mode, bug#32673.)
-                end (if (and (re-search-backward
-                              dired-permission-flags-regexp nil t)
-                             (looking-at "l")
-                             (search-forward " -> " lep t))
+                end (if (or
+                         ;; If the file is a symlink, put the
+                         ;; dired-filename property only on the link
+                         ;; name.  (Using (file-symlink-p
+                         ;; (dired-get-filename)) fails in
+                         ;; wdired-mode, bug#34915.)
+                         (and (re-search-backward
+                               dired-permission-flags-regexp nil t)
+                              (looking-at "l")
+                              (search-forward " -> " lep t))
+                         ;; When using the -F switch, don't treat file
+                         ;; type indicator characters as part of the
+                         ;; file name (bug#34915).
+                         (and (string-match "F" dired-actual-switches)
+                              (re-search-forward "[*/@|=>]$" lep t)))
                         (goto-char (match-beginning 0))
                       lep))
           (put-text-property beg end 'dired-filename t))))))

bug#34915: 27.0.50; Wdired regression with ls -F

[Basil L. Contovounesios]

Stephen Berman <stephen.berman@gmx.net> writes:

> On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:
>
>> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
>> are part of the file name now.
>
> This is indeed due to my changes.  The patch below appears to fix the
> problem, but I'm not sure how robust it is (I was also, and remain,
> unsure about my handling of symlinks in the previous patch, but I
> haven't found time to look at it more closely; at least I haven't seen
> any bug reports about it so far).

I can confirm your patch fixes the issue, thanks!

-- 
Basil

bug#34915: 27.0.50; Wdired regression with ls -F

[Stephen Berman]

On Fri, 12 Apr 2019 13:58:25 +0100 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios"
>> <contovob@tcd.ie> wrote:
>>
>>> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
>>> are part of the file name now.
>>
>> This is indeed due to my changes.  The patch below appears to fix the
>> problem, but I'm not sure how robust it is (I was also, and remain,
>> unsure about my handling of symlinks in the previous patch, but I
>> haven't found time to look at it more closely; at least I haven't seen
>> any bug reports about it so far).
>
> I can confirm your patch fixes the issue, thanks!

Thanks for testing.  If there are no objections within a few days, I'll
push it to master.

Steve Berman

bug#34915: 27.0.50; Wdired regression with ls -F

[Stephen Berman]

On Fri, 12 Apr 2019 15:06:18 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Fri, 12 Apr 2019 13:58:25 +0100 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:
>
>> Stephen Berman <stephen.berman@gmx.net> writes:
>>
>>> On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios"
>>> <contovob@tcd.ie> wrote:
>>>
>>>> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
>>>> are part of the file name now.
>>>
>>> This is indeed due to my changes.  The patch below appears to fix the
>>> problem, but I'm not sure how robust it is (I was also, and remain,
>>> unsure about my handling of symlinks in the previous patch, but I
>>> haven't found time to look at it more closely; at least I haven't seen
>>> any bug reports about it so far).
>>
>> I can confirm your patch fixes the issue, thanks!
>
> Thanks for testing.  If there are no objections within a few days, I'll
> push it to master.

I didn't expect "a few days" to become almost two weeks, but I've
finally pushed the fix to master as commit 6d8e0fc5aa.  I slightly
changed the patch to account for using either the short or long form of
the indicator switch, and I added a test.

Steve Berman

bug#34915: 27.0.50; Wdired regression with ls -F

[Basil L. Contovounesios]

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

Stephen Berman <stephen.berman@gmx.net> writes:

> On Fri, 12 Apr 2019 15:06:18 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:
>
>> On Fri, 12 Apr 2019 13:58:25 +0100 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:
>>
>>> Stephen Berman <stephen.berman@gmx.net> writes:
>>>
>>>> On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios"
>>>> <contovob@tcd.ie> wrote:
>>>>
>>>>> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
>>>>> are part of the file name now.
>>>>
>>>> This is indeed due to my changes.  The patch below appears to fix the
>>>> problem, but I'm not sure how robust it is (I was also, and remain,
>>>> unsure about my handling of symlinks in the previous patch, but I
>>>> haven't found time to look at it more closely; at least I haven't seen
>>>> any bug reports about it so far).
>>>
>>> I can confirm your patch fixes the issue, thanks!
>>
>> Thanks for testing.  If there are no objections within a few days, I'll
>> push it to master.
>
> I didn't expect "a few days" to become almost two weeks, but I've
> finally pushed the fix to master as commit 6d8e0fc5aa.  I slightly
> changed the patch to account for using either the short or long form of
> the indicator switch, and I added a test.

Thanks.  I noticed an opportunity for a tiny bit of reuse:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: classify.diff --]
[-- Type: text/x-diff, Size: 1842 bytes --]

diff --git a/lisp/dired.el b/lisp/dired.el
index 63082fe392..3050a4bd2d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1261,6 +1261,10 @@ dired-switches-recursive-p
   "Return non-nil if the string SWITCHES contains -R or --recursive."
   (dired-check-switches switches "R" "recursive"))
 
+(defun dired-switches-classify-p (switches)
+  "Return non-nil if the string SWITCHES contains -F or --classify."
+  (dired-check-switches switches "F" "classify"))
+
 (defun dired-insert-directory (dir switches &optional file-list wildcard hdr)
   "Insert a directory listing of DIR, Dired style.
 Use SWITCHES to make the listings.
@@ -2588,7 +2592,7 @@ dired-move-to-end-of-filename
   (if (get-text-property (point) 'dired-filename)
       (goto-char (next-single-property-change (point) 'dired-filename))
     (let ((opoint (point))
-          (used-F (dired-check-switches dired-actual-switches "F" "classify"))
+          (used-F (dired-switches-classify-p dired-actual-switches))
           (eol (line-end-position))
           (hidden (dired--hidden-p))
           file-type executable symlink)
diff --git a/lisp/wdired.el b/lisp/wdired.el
index d2a298bd25..1e9c7f6c5a 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -626,8 +626,7 @@ wdired--restore-dired-filename-prop
                          ;; or "classify", don't treat appended
                          ;; indicator characters as part of the file
                          ;; name (bug#34915).
-                         (and (dired-check-switches dired-actual-switches
-                                                    "F" "classify")
+                         (and (dired-switches-classify-p dired-actual-switches)
                               (re-search-forward "[*/@|=>]$" lep t)))
                         (goto-char (match-beginning 0))
                       lep))

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


Which makes me wonder: is there no Dired function that
wdired--restore-dired-filename-prop can reuse for finding the boundaries
of a file name?  Is dired-move-to-end-of-filename not suitable?  It
seems to perform similar checks for symlinks and --classify.

-- 
Basil

bug#34915: 27.0.50; Wdired regression with ls -F

[Basil L. Contovounesios]

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

Stephen Berman <stephen.berman@gmx.net> writes:

> On Fri, 12 Apr 2019 15:06:18 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:
>
>> On Fri, 12 Apr 2019 13:58:25 +0100 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:
>>
>>> Stephen Berman <stephen.berman@gmx.net> writes:
>>>
>>>> On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios"
>>>> <contovob@tcd.ie> wrote:
>>>>
>>>>> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
>>>>> are part of the file name now.
>>>>
>>>> This is indeed due to my changes.  The patch below appears to fix the
>>>> problem, but I'm not sure how robust it is (I was also, and remain,
>>>> unsure about my handling of symlinks in the previous patch, but I
>>>> haven't found time to look at it more closely; at least I haven't seen
>>>> any bug reports about it so far).
>>>
>>> I can confirm your patch fixes the issue, thanks!
>>
>> Thanks for testing.  If there are no objections within a few days, I'll
>> push it to master.
>
> I didn't expect "a few days" to become almost two weeks, but I've
> finally pushed the fix to master as commit 6d8e0fc5aa.  I slightly
> changed the patch to account for using either the short or long form of
> the indicator switch, and I added a test.

Thanks.  I noticed an opportunity for a tiny bit of reuse:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: classify.diff --]
[-- Type: text/x-diff, Size: 1842 bytes --]

diff --git a/lisp/dired.el b/lisp/dired.el
index 63082fe392..3050a4bd2d 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1261,6 +1261,10 @@ dired-switches-recursive-p
   "Return non-nil if the string SWITCHES contains -R or --recursive."
   (dired-check-switches switches "R" "recursive"))
 
+(defun dired-switches-classify-p (switches)
+  "Return non-nil if the string SWITCHES contains -F or --classify."
+  (dired-check-switches switches "F" "classify"))
+
 (defun dired-insert-directory (dir switches &optional file-list wildcard hdr)
   "Insert a directory listing of DIR, Dired style.
 Use SWITCHES to make the listings.
@@ -2588,7 +2592,7 @@ dired-move-to-end-of-filename
   (if (get-text-property (point) 'dired-filename)
       (goto-char (next-single-property-change (point) 'dired-filename))
     (let ((opoint (point))
-          (used-F (dired-check-switches dired-actual-switches "F" "classify"))
+          (used-F (dired-switches-classify-p dired-actual-switches))
           (eol (line-end-position))
           (hidden (dired--hidden-p))
           file-type executable symlink)
diff --git a/lisp/wdired.el b/lisp/wdired.el
index d2a298bd25..1e9c7f6c5a 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -626,8 +626,7 @@ wdired--restore-dired-filename-prop
                          ;; or "classify", don't treat appended
                          ;; indicator characters as part of the file
                          ;; name (bug#34915).
-                         (and (dired-check-switches dired-actual-switches
-                                                    "F" "classify")
+                         (and (dired-switches-classify-p dired-actual-switches)
                               (re-search-forward "[*/@|=>]$" lep t)))
                         (goto-char (match-beginning 0))
                       lep))

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


Which makes me wonder: is there no Dired function that
wdired--restore-dired-filename-prop can reuse for finding the boundaries
of a file name?  Is dired-move-to-end-of-filename not suitable?  It
seems to perform similar checks for symlinks and --classify.

-- 
Basil

bug#34915: 27.0.50; Wdired regression with ls -F

[Stephen Berman]

On Fri, 26 Apr 2019 14:56:57 +0100 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> On Fri, 12 Apr 2019 15:06:18 +0200 Stephen Berman <stephen.berman@gmx.net> wrote:
>>
>>> On Fri, 12 Apr 2019 13:58:25 +0100 "Basil L. Contovounesios"
>>> <contovob@tcd.ie> wrote:
>>>
>>>> Stephen Berman <stephen.berman@gmx.net> writes:
>>>>
>>>>> On Tue, 19 Mar 2019 13:43:16 +0000 "Basil L. Contovounesios"
>>>>> <contovob@tcd.ie> wrote:
>>>>>
>>>>>> It seems like Wdired thinks the indicators added by ls (one of */=>@|)
>>>>>> are part of the file name now.
>>>>>
>>>>> This is indeed due to my changes.  The patch below appears to fix the
>>>>> problem, but I'm not sure how robust it is (I was also, and remain,
>>>>> unsure about my handling of symlinks in the previous patch, but I
>>>>> haven't found time to look at it more closely; at least I haven't seen
>>>>> any bug reports about it so far).
>>>>
>>>> I can confirm your patch fixes the issue, thanks!
>>>
>>> Thanks for testing.  If there are no objections within a few days, I'll
>>> push it to master.
>>
>> I didn't expect "a few days" to become almost two weeks, but I've
>> finally pushed the fix to master as commit 6d8e0fc5aa.  I slightly
>> changed the patch to account for using either the short or long form of
>> the indicator switch, and I added a test.
>
> Thanks.  I noticed an opportunity for a tiny bit of reuse:
[...]

I'm not sure the two uses justify a new function, but I don't oppose it.

> Which makes me wonder: is there no Dired function that
> wdired--restore-dired-filename-prop can reuse for finding the boundaries
> of a file name?  Is dired-move-to-end-of-filename not suitable?  It
> seems to perform similar checks for symlinks and --classify.

dired-move-to-end-of-filename doesn't work in wdired-mode because the
dired-filename text property it uses was removed to fix bug#32173, and
wdired--restore-dired-filename-prop was added to compensate.  I couldn't
come up with a more elegant solution.

Steve Berman

bug#34915: 27.0.50; Wdired regression with ls -F

[Basil L. Contovounesios]

tags 34915 fixed
close 34915
quit

Stephen Berman <stephen.berman@gmx.net> writes:

> On Fri, 26 Apr 2019 14:56:57 +0100 "Basil L. Contovounesios" <contovob@tcd.ie> wrote:
>
>> Thanks.  I noticed an opportunity for a tiny bit of reuse:
> [...]
>
> I'm not sure the two uses justify a new function, but I don't oppose
> it.

I'll leave it for now.

>> Which makes me wonder: is there no Dired function that
>> wdired--restore-dired-filename-prop can reuse for finding the boundaries
>> of a file name?  Is dired-move-to-end-of-filename not suitable?  It
>> seems to perform similar checks for symlinks and --classify.
>
> dired-move-to-end-of-filename doesn't work in wdired-mode because the
> dired-filename text property it uses was removed to fix bug#32173, and
> wdired--restore-dired-filename-prop was added to compensate.  I couldn't
> come up with a more elegant solution.

Thanks for explaining and for working on this.  Since your patch fixed
the bug, I'm closing this report.

-- 
Basil

bug#34915: 27.0.50; Wdired regression with ls -F

[Ken Brown]

The fix for this bug causes wdired-tests to hang on my system (Cygwin):

$ make -C test wdired-tests
make: Entering directory '/home/kbrown/src/emacs/i686/test'
make[1]: Entering directory '/home/kbrown/src/emacs/i686/test'
   GEN      lisp/wdired-tests.log
Running 5 tests (2019-05-30 15:39:07-0400, selector `(not (tag :unstable))')
Press C-c C-c when finished or C-c ESC to abort changes
Move: 1 of 1
Move: 1 file done
    passed  1/5  wdired-test-bug32173-01 (0.189231 sec)
Press C-c C-c when finished or C-c ESC to abort changes
    passed  2/5  wdired-test-bug32173-02 (0.140659 sec)
(Shell command succeeded with no output)
Connection file "/tmp/emacs197609/server" deleted
Press C-c C-c when finished or C-c ESC to abort changes
Connection file "/tmp/emacs197609/server" deleted

[At this point I kill it with C-c.]

I don't use wdired, so I haven't made a serious effort to track down the cause. 
I thought at first that it might have something to do with the following alias 
that I use:

alias ls='ls -F --color=tty'

But the problem persisted after I moved my .bashrc out of the way.

Ken

bug#34915: 27.0.50; Wdired regression with ls -F

[Ken Brown]

On 5/30/2019 3:51 PM, Ken Brown wrote:
> The fix for this bug causes wdired-tests to hang on my system (Cygwin):

This turns out to be purely a Cygwin issue (involving FIFOs), having nothing to 
do with wdired.  Sorry for the noise.

Ken

bug#34915: 27.0.50; Wdired regression with ls -F

[Stephen Berman]

On Wed, 26 Jun 2019 16:28:26 +0000 Ken Brown <kbrown@cornell.edu> wrote:

> On 5/30/2019 3:51 PM, Ken Brown wrote:
>> The fix for this bug causes wdired-tests to hang on my system (Cygwin):
>
> This turns out to be purely a Cygwin issue (involving FIFOs), having nothing to
> do with wdired.  Sorry for the noise.

No problem, on the contrary, I'm relieved, because I know next to
nothing about Cygwin and have no ready access to it (and I'm already in
arrears on dealing with another wdired issue).  So thanks for the good
news ;-)

Steve Berman

bug#34915: 27.0.50; Wdired regression with ls -F

[Mattias Engdegård]

Sorry about reopening, but wdired-test-bug34915 fails on macOS (and probably BSD in general).
It appears that GNU ls formats symlinks with ls -lF as

.... LINK -> TARGET

but macOS/BSD ls uses

.... LINK@ -> TARGET

which the new code in wdired--restore-dired-filename-prop fails to account for.

This was found on macOS 10.14.

bug#34915: 27.0.50; Wdired regression with ls -F

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Sorry about reopening, but wdired-test-bug34915 fails on macOS (and
> probably BSD in general).
> It appears that GNU ls formats symlinks with ls -lF as
>
> .... LINK -> TARGET
>
> but macOS/BSD ls uses
>
> .... LINK@ -> TARGET
>
> which the new code in wdired--restore-dired-filename-prop fails to account for.
>
> This was found on macOS 10.14.

I tries "make wdired-tests" in Mojave, and it doesn't report any
problems.  I seem to remember working on ls -lF-related bits for Macos a
while back, so I'm guessing that fixed this problem, too, and I'm
closing this bug report again.

If the problem persists, send an email to the debbugs address and we'll
reopen.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#34915: 27.0.50; Wdired regression with ls -F

[Mattias Engdegård]

20 sep. 2020 kl. 20.14 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I tries "make wdired-tests" in Mojave, and it doesn't report any
> problems.  I seem to remember working on ls -lF-related bits for Macos a
> while back, so I'm guessing that fixed this problem, too, and I'm
> closing this bug report again.

Thank you for re-testing! It indeed seems to be fixed on master.