bug#57293: 29.0.50; query-replace with Wdired skips symlink target

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Mike Kupfer]

From "emacs -Q":

1. Create a file foo, and create a symlink bar that points to foo.

S lrwxrwxrwx  1 kupfer kupfer    3 Aug 18 19:48 bar -> foo
  -rw-r--r--  1 kupfer kupfer    0 Aug 18 19:46 foo

2. Visit the directory with dired and type C-x C-q.

3. Move point to the start of the buffer.

4. M-% foo RET baz RET

I expect query-replace to ask me about both the file foo and the symlink
target foo.  That's what Emacs 28.1 does, and it's quite handy.
Instead, query-replace asks me only about the file foo, ignoring the
symlink target.

S lrwxrwxrwx  1 kupfer kupfer    3 Aug 18 19:48 bar -> foo
  -rw-r--r--  1 kupfer kupfer    0 Aug 18 19:46 baz

I can still manually fix the symlink (replace "foo" with "baz") while in
Wdired, but I think query-replace should be able to do this for me.

The build is Emacs master 7dab52dfbc plus a couple local fixes to MH-E,
so I expect it's reproducible with just 7dab52dfbc.

In GNU Emacs 29.0.50 (build 14, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw scroll bars)
 of 2022-08-18 built on alto
Repository revision: d3fd48a98606cd8d8de577af114ce0eb046ff210
Repository branch: master-mdk
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --prefix=/usr/new'

Configured features:
CAIRO FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG LIBSELINUX
LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM LUCID ZLIB

Important settings:
  value of $LC_TIME: C
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Shell

Minor modes in effect:
  shell-dirtrack-mode: t
  delete-selection-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  column-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow mh-identity emacsbug misearch multi-isearch wdired dired-aux
dired-x shr-color color textsec uni-scripts idna-mapping ucs-normalize
uni-confusable textsec-check qp mm-archive mail-extr mh-mime pp
mule-util mh-search mh-letter mh-show goto-addr thingatpt gnus-cite
mh-thread mh-inc hl-line mh-tool-bar mh-acros mh-seq mh-xface mh-utils
mh-folder which-func imenu files-x mdk-mail gnus-mh gnus-msg mh-comp
mh-scan mh-gnus gnus-dup nnmh gnus-score score-mode gnus-art mm-uu
mml2015 mm-view mml-smime smime gnutls dig gnus-sum shr pixel-fill
kinsoku url-file svg dom browse-url url url-proxy url-privacy url-expand
url-methods url-history url-cookie generate-lisp-file url-domsuf
url-util url-parse auth-source cl-seq eieio eieio-core cl-macs json map
gv url-vars gnus-group gnus-undo gnus-start gnus-dbus dbus xml
gnus-cloud nnimap nnmail mail-source utf7 nnoo parse-time iso8601
gnus-spec gnus-int gnus-range gnus-win gnus nnheader range wid-edit mh-e
mh-buffers mh-loaddefs message sendmail mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util ietf-drums
mail-prsvr mailabbrev mail-utils gmm-utils mailheader server noutline
outline easy-mmode icons cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs byte-opt bytecomp
byte-compile cconv shell subr-x pcomplete comint ansi-color ring xcscope
advice delsel vc vc-dispatcher timeclock cl-loaddefs cl-lib mdk-hacks
rmc iso-transl tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode 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 lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine 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 emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads inotify dynamic-setting
system-font-setting font-render-setting cairo x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 341028 35766)
 (symbols 48 19344 2)
 (strings 32 82149 1520)
 (string-bytes 1 2322871)
 (vectors 16 64652)
 (vector-slots 8 1498444 216905)
 (floats 8 324 210)
 (intervals 56 3883 77)
 (buffers 992 24))

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Michael Heerdegen]

Mike Kupfer <mkupfer@alum.berkeley.edu> writes:

> I expect query-replace to ask me about both the file foo and the symlink
> target foo.  That's what Emacs 28.1 does, and it's quite handy.
> Instead, query-replace asks me only about the file foo, ignoring the
> symlink target.

Thanks for the report, Mike.  See the new option
`wdired-search-replace-filenames' to turn off the new behavior.

Maybe turning this new behavior on by default was too much (Juri)?

BTW, shouldn't the "\M-sf" key prefix work in wdired?  And when
starting a search in a wdired mode buffer, a subsequent M-% doesn't
inherit the we-are-searching-filenames state from the search, only the
binding of `wdired-search-replace-filenames' counts.  If that worked, it
would allow to get the other query-replace behavior wrt filename
replacing without the need to touch the option.

Michael.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Stefan Kangas]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Mike Kupfer <mkupfer@alum.berkeley.edu> writes:
>
>> I expect query-replace to ask me about both the file foo and the symlink
>> target foo.  That's what Emacs 28.1 does, and it's quite handy.
>> Instead, query-replace asks me only about the file foo, ignoring the
>> symlink target.
>
> Thanks for the report, Mike.  See the new option
> `wdired-search-replace-filenames' to turn off the new behavior.
>
> Maybe turning this new behavior on by default was too much (Juri)?

Shouldn't we get the Emacs 28 behavior whether or not that option is
enabled though?  IOW, isn't this just a bug?

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Michael Heerdegen]

Stefan Kangas <stefankangas@gmail.com> writes:

> Shouldn't we get the Emacs 28 behavior whether or not that option is
> enabled though?  IOW, isn't this just a bug?

In the sense of user-visible (default) behavior change that is not
fixing a bug - yes, I think so.

Michael.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Mike Kupfer]

Stefan Kangas wrote:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
> 
> > Mike Kupfer <mkupfer@alum.berkeley.edu> writes:
> >
> >> I expect query-replace to ask me about both the file foo and the symlink
> >> target foo.  That's what Emacs 28.1 does, and it's quite handy.
> >> Instead, query-replace asks me only about the file foo, ignoring the
> >> symlink target.
> >
> > Thanks for the report, Mike.  See the new option
> > `wdired-search-replace-filenames' to turn off the new behavior.
> >
> > Maybe turning this new behavior on by default was too much (Juri)?
> 
> Shouldn't we get the Emacs 28 behavior whether or not that option is
> enabled though?  IOW, isn't this just a bug?

The docstring says

  Non-nil to search and replace in file names only.

so it does look like the relevant control.  (But I wouldn't be surprised
if I were missing something.)

I have a slight preference for getting the Emacs 28 behavior by default,
but I'm okay either way.  If there had been a NEWS entry about the new
option (hint hint :-)) I wouldn't have filed a bug.

I agree with Michael's point about "\M-sf".  Right now, you get the
filename-only behavior for isearch via a specific functions and
keybindings, but for query-replace, the behavior is controlled by a user
option.  More consistency between isearch and query-replace seems like
it would be a good thing.

I did notice this NEWS entry:

  *** Search and replace in Dired/Wdired supports more regexps.
  For example, the regexp ".*" will match only characters that are part
  of the file name.  Also "^.*$" can be used to match at the beginning
  of the file name and at the end of the file name.

Does that functionality require wdired-search-replace-filenames to be
non-nil?

cheers,
mike

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Juri Linkov]

>> > Maybe turning this new behavior on by default was too much (Juri)?
>>
>> Shouldn't we get the Emacs 28 behavior whether or not that option is
>> enabled though?  IOW, isn't this just a bug?
>
> The docstring says
>
>   Non-nil to search and replace in file names only.
>
> so it does look like the relevant control.  (But I wouldn't be surprised
> if I were missing something.)

The main question is what constitutes a file name?  The old definition
was that replaceable file names are everywhere where read-only is nil.
The new definition is that file names are everywhere where the
text property 'dired-filename' is non-nil.

So if "foo" in "bar -> foo" had the text property 'dired-filename',
query-replace would visit it as well.

But wouldn't such change break some other commands?

> I have a slight preference for getting the Emacs 28 behavior by default,
> but I'm okay either way.  If there had been a NEWS entry about the new
> option (hint hint :-)) I wouldn't have filed a bug.

A NEWS entry could be added too unless something like below will make it
obsolete.

> I agree with Michael's point about "\M-sf".  Right now, you get the
> filename-only behavior for isearch via a specific functions and
> keybindings, but for query-replace, the behavior is controlled by a user
> option.  More consistency between isearch and query-replace seems like
> it would be a good thing.

Please note that unlike the corresponding option dired-isearch-filenames
that is nil, it makes much more sense to enable wdired-search-replace-filenames
by default because renaming file names is the primary use of WDired.

> I did notice this NEWS entry:
>
>   *** Search and replace in Dired/Wdired supports more regexps.
>   For example, the regexp ".*" will match only characters that are part
>   of the file name.  Also "^.*$" can be used to match at the beginning
>   of the file name and at the end of the file name.
>
> Does that functionality require wdired-search-replace-filenames to be
> non-nil?

Indeed, when wdired-search-replace-filenames is nil, ".*" matches whole lines.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> So if "foo" in "bar -> foo" had the text property 'dired-filename',
> query-replace would visit it as well.
>
> But wouldn't such change break some other commands?

That property plays a central role in dired buffers, I would be
surprised if nothing would break.  Even third party code might rely on
the current placement.

I would rather try to generalize the query-replace semantics further,
even when that might be a harder task.

Michael.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Juri Linkov]

>> So if "foo" in "bar -> foo" had the text property 'dired-filename',
>> query-replace would visit it as well.
>>
>> But wouldn't such change break some other commands?
>
> That property plays a central role in dired buffers, I would be
> surprised if nothing would break.  Even third party code might rely on
> the current placement.
>
> I would rather try to generalize the query-replace semantics further,
> even when that might be a harder task.

Maybe then it's possible to add a new text property on symlink file names?
Then query-replace could check for two text properties:
'dired-filename' and 'dired-symlink-filename'.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Maybe then it's possible to add a new text property on symlink file names?
> Then query-replace could check for two text properties:
> 'dired-filename' and 'dired-symlink-filename'.

Yes, I think we could do that.

Note that `dired-insert-set-properties' already sets the invisible text
property to 'dired-hide-details-link'.

Michael.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Juri Linkov]

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

>> Maybe then it's possible to add a new text property on symlink file names?
>> Then query-replace could check for two text properties:
>> 'dired-filename' and 'dired-symlink-filename'.
>
> Yes, I think we could do that.
>
> Note that `dired-insert-set-properties' already sets the invisible text
> property to 'dired-hide-details-link'.

Ok, here is the fix to support search and query-replace of e.g. "fo* → baz":


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired-symlink-filename.patch --]
[-- Type: text/x-diff, Size: 4690 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index 83fa28b325..596e52b4a2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2002,7 +2002,9 @@ the buffer will take you to that directory.
 *** Search and replace in Dired/Wdired supports more regexps.
 For example, the regexp ".*" will match only characters that are part
 of the file name.  Also "^.*$" can be used to match at the beginning
-of the file name and at the end of the file name.
+of the file name and at the end of the file name.  In Wdired this can
+be used when the new user option 'wdired-search-replace-filenames' is
+non-nil (which is the default).
 
 ** Bookmarks
 
diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 94b2baf72d..06f0b86fc4 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3544,7 +3544,8 @@ dired-isearch-search-filenames
 The returned function narrows the search to match the search string
 only as part of a file name enclosed by the text property `dired-filename'.
 It's intended to override the default search function."
-  (isearch-search-fun-in-text-property (funcall orig-fun) 'dired-filename))
+  (isearch-search-fun-in-text-property
+   (funcall orig-fun) '(dired-filename dired-symlink-filename)))
 
 ;;;###autoload
 (defun dired-isearch-filenames ()
diff --git a/lisp/dired.el b/lisp/dired.el
index f45d215ed6..fa06c8fd44 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -786,7 +786,7 @@ dired-font-lock-keywords
                nil
                '(1 'dired-broken-symlink)
                '(2 dired-symlink-face)
-               '(3 'dired-broken-symlink)))
+               '(3 '(face dired-broken-symlink dired-symlink-filename t))))
    ;;
    ;; Symbolic link to a directory.
    (list dired-re-sym
@@ -798,7 +798,7 @@ dired-font-lock-keywords
                '(dired-move-to-filename)
                nil
                '(1 dired-symlink-face)
-               '(2 dired-directory-face)))
+               '(2 '(face dired-directory-face dired-symlink-filename t))))
    ;;
    ;; Symbolic link to a non-directory.
    (list dired-re-sym
@@ -812,7 +812,7 @@ dired-font-lock-keywords
                '(dired-move-to-filename)
                nil
                '(1 dired-symlink-face)
-               '(2 'default)))
+               '(2 '(face default dired-symlink-filename t))))
    ;;
    ;; Sockets, pipes, block devices, char devices.
    (list dired-re-special
diff --git a/lisp/isearch.el b/lisp/isearch.el
index 31fcf01949..7fdae06c96 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -4512,21 +4522,34 @@ isearch-search-fun-in-noncontiguous-region
            (setq bounds (cdr bounds))))
        found))))
 
-(defun isearch-search-fun-in-text-property (search-fun property)
-  "Return the function to search inside text that has the specified PROPERTY.
+(defun isearch-search-fun-in-text-property (search-fun properties)
+  "Return the function to search inside text that has the specified PROPERTIES.
 The function will limit the search for matches only inside text which has
-this property in the current buffer.
+these list of PROPERTIES in the current buffer.
 The argument SEARCH-FUN provides the function to search text, and
 defaults to the value of `isearch-search-fun-default' when nil."
   (apply-partially
    #'search-within-boundaries
    search-fun
-   (lambda (pos) (get-text-property (if isearch-forward pos
-                                      (max (1- pos) (point-min)))
-                                    property))
-   (lambda (pos) (if isearch-forward
-                     (next-single-property-change pos property)
-                   (previous-single-property-change pos property)))))
+   (lambda (pos)
+     (let ((pos (if isearch-forward pos (max (1- pos) (point-min)))))
+       (seq-some (lambda (property)
+                   (get-text-property pos property))
+                 (ensure-list properties))))
+   (lambda (pos)
+     (let ((pos-list (if isearch-forward
+                         (mapcar (lambda (property)
+                                   (next-single-property-change
+                                    pos property))
+                                 (ensure-list properties))
+                       (mapcar (lambda (property)
+                                 (previous-single-property-change
+                                  pos property))
+                               (ensure-list properties)))))
+       (setq pos-list (delq nil pos-list))
+       (when pos-list (if isearch-forward
+                          (seq-min pos-list)
+                        (seq-max pos-list)))))))
 
 (defun search-within-boundaries ( search-fun get-fun next-fun
                                   string &optional bound noerror count)

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Michael Heerdegen]

Juri Linkov <juri@linkov.net> writes:

> Ok, here is the fix to support search and query-replace of e.g. "fo* →
> baz":

Great - thanks.  Mike, can you maybe give it a try?

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -2002,7 +2002,9 @@ the buffer will take you to that directory.
>  *** Search and replace in Dired/Wdired supports more regexps.
>  For example, the regexp ".*" will match only characters that are part
>  of the file name.  Also "^.*$" can be used to match at the beginning
> -of the file name and at the end of the file name.
> +of the file name and at the end of the file name.  In Wdired this can
> +be used when the new user option 'wdired-search-replace-filenames' is
> +non-nil (which is the default).

Can we say that the behavior in dired is optional, too, and maybe just
list the options (and commands) at the end?

> -(defun isearch-search-fun-in-text-property (search-fun property)
> -  "Return the function to search inside text that has the specified PROPERTY.
> +(defun isearch-search-fun-in-text-property (search-fun properties)
> +  "Return the function to search inside text that has the specified PROPERTIES.
>  The function will limit the search for matches only inside text which has
> -this property in the current buffer.
> +these list of PROPERTIES in the current buffer.

"which has at least one of the text PROPERTIES" may be better (I also
added "text" so that the docstring at least once tells that this is only
about text props).

The rest of the patch looks good to me, but I didn't test.

Thanks,

Michael.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Mike Kupfer]

Michael Heerdegen wrote:

> Juri Linkov <juri@linkov.net> writes:
> 
> > Ok, here is the fix to support search and query-replace of e.g. "fo* →
> > baz":
> 
> Great - thanks.  Mike, can you maybe give it a try?

Will do, though it may be a few days before I can get to it.

cheers,
mike

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Mike Kupfer]

Michael Heerdegen wrote:

> Juri Linkov <juri@linkov.net> writes:
> 
> > Ok, here is the fix to support search and query-replace of e.g. "fo* →
> > baz":
> 
> Great - thanks.  Mike, can you maybe give it a try?

It works great.  Thanks, Juri!

mike

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Juri Linkov]

close 57293 29.0.50
thanks

>> > Ok, here is the fix to support search and query-replace of e.g. "fo* →
>> > baz":
>> 
>> Great - thanks.  Mike, can you maybe give it a try?
>
> It works great.  Thanks, Juri!

Thanks for confirming.  Now pushed.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Juri Linkov]

>>  *** Search and replace in Dired/Wdired supports more regexps.
>>  For example, the regexp ".*" will match only characters that are part
>>  of the file name.  Also "^.*$" can be used to match at the beginning
>> -of the file name and at the end of the file name.
>> +of the file name and at the end of the file name.  In Wdired this can
>> +be used when the new user option 'wdired-search-replace-filenames' is
>> +non-nil (which is the default).
>
> Can we say that the behavior in dired is optional, too, and maybe just
> list the options (and commands) at the end?

I tried to improve this, please check.

>> -(defun isearch-search-fun-in-text-property (search-fun property)
>> -  "Return the function to search inside text that has the specified PROPERTY.
>> +(defun isearch-search-fun-in-text-property (search-fun properties)
>> +  "Return the function to search inside text that has the specified PROPERTIES.
>>  The function will limit the search for matches only inside text which has
>> -this property in the current buffer.
>> +these list of PROPERTIES in the current buffer.
>
> "which has at least one of the text PROPERTIES" may be better (I also
> added "text" so that the docstring at least once tells that this is only
> about text props).

Fixed this as well.

bug#57293: 29.0.50; query-replace with Wdired skips symlink target

[Michael Heerdegen]

Juri,

looks all good to me - thanks.

Michael.