bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Juri Linkov]

As reported in https://lists.gnu.org/archive/html/emacs-devel/2018-09/msg00372.html
Wdired breaks delete-selection-mode in the latest master 27.0.50:

1. create scratch directory with a file

   mkdir /tmp/test
   cd /tmp/test
   touch foo.c

2. start emacs

   LC_ALL=C emacs -Q -nw

3. activate delete-selection-mode

   M-x delete-selection-mode RET

4. go into the folder

   C-x C-f /tmp/test

5. enter wdired mode

   C-x C-q

6. replace 'foo' with 'test' by selecting the whole file name
   and typing a new one, e.g.

   M-2 C-M-SPC test

   or

   C-SPC C-e test

When typing the first letter, it removes delete-selection-pre-hook from
pre-command-hook, thus breaking delete-selection-mode, because of this
error seen in the *Messages* buffer:

  Error in pre-command-hook (delete-selection-pre-hook): (error "No file on this line")

The same error is reproducible when simply deleting an old file name
before typing a new one in the step 6, e.g.:

   M-d test

The raised error is:

  Debugger entered--Lisp error: (error "No file on this line")
    signal(error ("No file on this line"))
    error("%s" "No file on this line")
    dired-move-to-end-of-filename(nil)
    dired-get-filename()
    wdired--restore-dired-filename-prop(198 198 1)
    delete-char(1 nil)
    funcall-interactively(delete-char 1 nil)
    call-interactively(delete-char nil nil)
    command-execute(delete-char)

This is caused by changes in bug#32173.

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Stephen Berman]

On Sun, 09 Sep 2018 18:52:55 +0300 Juri Linkov <juri@linkov.net> wrote:

> As reported in https://lists.gnu.org/archive/html/emacs-devel/2018-09/msg00372.html
> Wdired breaks delete-selection-mode in the latest master 27.0.50:
>
> 1. create scratch directory with a file
>
>    mkdir /tmp/test
>    cd /tmp/test
>    touch foo.c
>
> 2. start emacs
>
>    LC_ALL=C emacs -Q -nw
>
> 3. activate delete-selection-mode
>
>    M-x delete-selection-mode RET
>
> 4. go into the folder
>
>    C-x C-f /tmp/test
>
> 5. enter wdired mode
>
>    C-x C-q
>
> 6. replace 'foo' with 'test' by selecting the whole file name
>    and typing a new one, e.g.
>
>    M-2 C-M-SPC test
>
>    or
>
>    C-SPC C-e test
>
> When typing the first letter, it removes delete-selection-pre-hook from
> pre-command-hook, thus breaking delete-selection-mode, because of this
> error seen in the *Messages* buffer:
>
>   Error in pre-command-hook (delete-selection-pre-hook): (error "No file on this line")
>
> The same error is reproducible when simply deleting an old file name
> before typing a new one in the step 6, e.g.:
>
>    M-d test
>
> The raised error is:
>
>   Debugger entered--Lisp error: (error "No file on this line")
>     signal(error ("No file on this line"))
>     error("%s" "No file on this line")
>     dired-move-to-end-of-filename(nil)
>     dired-get-filename()
>     wdired--restore-dired-filename-prop(198 198 1)
>     delete-char(1 nil)
>     funcall-interactively(delete-char 1 nil)
>     call-interactively(delete-char nil nil)
>     command-execute(delete-char)
>
> This is caused by changes in bug#32173.

The immediate cause of this is that wdired--restore-dired-filename-prop,
which is on after-change-functions, calls dired-get-filename without
making sure that the file name is not the empty string.  But it turns
out that using dired-get-filename in that function also doesn't work
with symlinks as I had thought (and tested, but evidently not enough):
after enabling wdired-mode but before making a change, (file-symlink-p
(dired-get-filename)) on a symlink returns the name of the target, but
as soon as a change is made, that sexp returns nil, which causes a
text-read-only signal.  The patch below replaces this sexp and according
to my tests avoids the error which breaks delete-selection-mode and also
most text-read-only signals (one remains: when deleting the whole link
name; I haven't been able to figure out how to avoid that).  The patch
should get more testing (in particular, is looking for the `l' flag a
reliable way to identify a symlink?), and an ERT test would also be
good, but I probably can't do either for the next few weeks, because
I'll be travelling.  I can install the patch before I leave, since it
seems at least better than the status quo, or maybe someone else can
take a look and either confirm that it is good enough or else come up
with a better fix.

Steve Berman

diff --git a/lisp/wdired.el b/lisp/wdired.el
index be0bde290a..3157e887d7 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -607,15 +607,22 @@ wdired-check-kill-buffer
 (defun wdired--restore-dired-filename-prop (beg end _len)
   (save-match-data
     (save-excursion
-      (beginning-of-line)
-      (when (re-search-forward directory-listing-before-filename-regexp
-                               (line-end-position) t)
-        (setq beg (point)
-              end (if (and (file-symlink-p (dired-get-filename))
-                           (search-forward " -> " (line-end-position) t))
-                      (goto-char (match-beginning 0))
-                    (line-end-position)))
-        (put-text-property beg end 'dired-filename t)))))
+      (let ((lep (line-end-position)))
+        (beginning-of-line)
+        (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))
+                        (goto-char (match-beginning 0))
+                      lep))
+          (put-text-property beg end 'dired-filename t))))))
 
 (defun wdired-next-line (arg)
   "Move down lines then position at filename or the current column.

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Juri Linkov]

> The immediate cause of this is that wdired--restore-dired-filename-prop,
> which is on after-change-functions, calls dired-get-filename without
> making sure that the file name is not the empty string.  But it turns
> out that using dired-get-filename in that function also doesn't work
> with symlinks as I had thought (and tested, but evidently not enough):
> after enabling wdired-mode but before making a change, (file-symlink-p
> (dired-get-filename)) on a symlink returns the name of the target, but
> as soon as a change is made, that sexp returns nil, which causes a
> text-read-only signal.  The patch below replaces this sexp and according
> to my tests avoids the error which breaks delete-selection-mode and also
> most text-read-only signals (one remains: when deleting the whole link
> name; I haven't been able to figure out how to avoid that).  The patch
> should get more testing (in particular, is looking for the `l' flag a
> reliable way to identify a symlink?), and an ERT test would also be
> good, but I probably can't do either for the next few weeks, because
> I'll be travelling.  I can install the patch before I leave, since it
> seems at least better than the status quo, or maybe someone else can
> take a look and either confirm that it is good enough or else come up
> with a better fix.

Thanks, since your patch fixes the reported problem, there is no reason
to postpone installing it.  Regarding improving the handling of symlinks,
maybe you can use some code from dired-move-to-end-of-filename,
especially handling dired-ls-F-marks-symlinks.

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Stephen Berman]

On Wed, 12 Sep 2018 02:32:09 +0300 Juri Linkov <juri@linkov.net> wrote:

>> The immediate cause of this is that wdired--restore-dired-filename-prop,
>> which is on after-change-functions, calls dired-get-filename without
>> making sure that the file name is not the empty string.  But it turns
>> out that using dired-get-filename in that function also doesn't work
>> with symlinks as I had thought (and tested, but evidently not enough):
>> after enabling wdired-mode but before making a change, (file-symlink-p
>> (dired-get-filename)) on a symlink returns the name of the target, but
>> as soon as a change is made, that sexp returns nil, which causes a
>> text-read-only signal.  The patch below replaces this sexp and according
>> to my tests avoids the error which breaks delete-selection-mode and also
>> most text-read-only signals (one remains: when deleting the whole link
>> name; I haven't been able to figure out how to avoid that).  The patch
>> should get more testing (in particular, is looking for the `l' flag a
>> reliable way to identify a symlink?), and an ERT test would also be
>> good, but I probably can't do either for the next few weeks, because
>> I'll be travelling.  I can install the patch before I leave, since it
>> seems at least better than the status quo, or maybe someone else can
>> take a look and either confirm that it is good enough or else come up
>> with a better fix.
>
> Thanks, since your patch fixes the reported problem, there is no reason
> to postpone installing it.  

Ok, but I'll wait another day or two, to give others a chance to object.

>                             Regarding improving the handling of symlinks,
> maybe you can use some code from dired-move-to-end-of-filename,
> especially handling dired-ls-F-marks-symlinks.

Thanks for the pointer.  I don't think I'll have time to look into that
before leaving, but maybe I can do so while I'm away, otherwise after
returning.

Steve Berman

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 12 Sep 2018 02:32:09 +0300
> Cc: 32673@debbugs.gnu.org
> 
> Thanks, since your patch fixes the reported problem, there is no reason
> to postpone installing it.

Agreed.

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Stephen Berman]

On Wed, 12 Sep 2018 16:57:09 +0300 Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Juri Linkov <juri@linkov.net>
>> Date: Wed, 12 Sep 2018 02:32:09 +0300
>> Cc: 32673@debbugs.gnu.org
>> 
>> Thanks, since your patch fixes the reported problem, there is no reason
>> to postpone installing it.
>
> Agreed.

Ok, pushed to master as commit 755fa34.

Steve Berman

bug#32673: 27.0.50; wdired: broken 'wdired--restore-dired-filename-prop'

[Juri Linkov]

tags 32673 fixed
close 32673 27.1
quit

> Ok, pushed to master as commit 755fa34.

Thanks, closed.