bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Hello,

the recipe is quite trivial:

- start emacs -Q

- dired a directory with some files in it

- let c be a character that appears in a file line before the filename,
  e.g. the character "r" in this line:

  drwxr-xr-x   3 micha micha    4096 Mar  3 21:51 bin
   ^
- M-x dired-isearch-filenames-regexp RET

- Enter the character c (i.e. hit r in my example)

  Only matches inside file names are found

- Now hit .*

  Now also matches starting before filenames are found.  This behavior
  continues if you enter even more characters.


This happens in all Emacs versions I tested: 23.4, 24.2, and trunk.


Thanks,

Michael.






In GNU Emacs 24.3.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.4.2)
 of 2013-03-17 on dex, modified by Debian
 (emacs-snapshot package, version 2:20130317-1)
Windowing system distributor `The X.Org Foundation', version 11.0.10707000
System Description:	Debian GNU/Linux 7.0 (wheezy)

Configured using:
 `configure --build x86_64-linux-gnu --host x86_64-linux-gnu
 --prefix=/usr --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var --infodir=/usr/share/info --mandir=/usr/share/man
 --with-pop=yes
 --enable-locallisppath=/etc/emacs-snapshot:/etc/emacs:/usr/local/share/emacs/24.3.50/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.3.50/site-lisp:/usr/share/emacs/site-lisp
 --without-compress-info --with-crt-dir=/usr/lib/x86_64-linux-gnu/
 --with-x=yes --with-x-toolkit=gtk3 --with-imagemagick=yes
 CFLAGS='-DDEBIAN -DSITELOAD_PURESIZE_EXTRA=5000 -g -O2'
 CPPFLAGS='-D_FORTIFY_SOURCE=2' LDFLAGS='-g -Wl,--as-needed
 -znocombreloc''

Important settings:
  value of $LC_ALL: de_DE.utf8
  value of $LC_TIME: C
  value of $LANG: de_DE.utf8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> - M-x dired-isearch-filenames-regexp RET
>
> - Enter the character c (i.e. hit r in my example)
>
>   Only matches inside file names are found
>
> - Now hit .*
>
>   Now also matches starting before filenames are found.  This behavior
>   continues if you enter even more characters.

.* matches a whole line.  We can't hide from Isearch the fact
that lines in Dired contain more details besides filenames.
However, you can hide these details and leave only filenames
by using `dired-hide-details-mode' or typing `('.  Then .*
will match only visible filenames.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> > - M-x dired-isearch-filenames-regexp RET
> >
> > - Enter the character c (i.e. hit r in my example)
> >
> >   Only matches inside file names are found
> >
> > - Now hit .*
> >
> >   Now also matches starting before filenames are found.  This behavior
> >   continues if you enter even more characters.
>
> .* matches a whole line.  We can't hide from Isearch the fact
> that lines in Dired contain more details besides filenames.

What about doing this (unrelated parts stripped, just as an example):

(defun dired-isearch-filter-filenames (beg end)
  (let ((beg (min beg end))
        (end (max beg end)))
   (and (get-text-property beg 'dired-filename)
        (or (eq (char-after end) ?\n)
            (get-text-property end 'dired-filename)))))

This assumes that file names are convex and followed by a newline.

> However, you can hide these details and leave only filenames
> by using `dired-hide-details-mode' or typing `('.  Then .*
> will match only visible filenames.

Surprisingly that doesn't work.  Even with search-invisible nil I get
exactly the same matches, i.e., also the invisible text is being
matched.  What did I miss?


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> .* matches a whole line.  We can't hide from Isearch the fact
>> that lines in Dired contain more details besides filenames.
>
> What about doing this (unrelated parts stripped, just as an example):
>
> (defun dired-isearch-filter-filenames (beg end)
>   (let ((beg (min beg end))
>         (end (max beg end)))
>    (and (get-text-property beg 'dired-filename)
>         (or (eq (char-after end) ?\n)
>             (get-text-property end 'dired-filename)))))

This doesn't work when searching for the regexp .+
.+ matches nothing but should match the same text as .*

>> However, you can hide these details and leave only filenames
>> by using `dired-hide-details-mode' or typing `('.  Then .*
>> will match only visible filenames.
>
> Surprisingly that doesn't work.  Even with search-invisible nil I get
> exactly the same matches, i.e., also the invisible text is being
> matched.  What did I miss?

I meant that with `dired-hide-details-mode' at least you don't see
invisible text that Isearch matches outside filenames.  You see
only filenames being matched :)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> >> .* matches a whole line.  We can't hide from Isearch the fact
> >> that lines in Dired contain more details besides filenames.
> >
> > What about doing this (unrelated parts stripped, just as an example):
> >
> > (defun dired-isearch-filter-filenames (beg end)
> >   (let ((beg (min beg end))
> >         (end (max beg end)))
> >    (and (get-text-property beg 'dired-filename)
> >         (or (eq (char-after end) ?\n)
> >             (get-text-property end 'dired-filename)))))
>
> This doesn't work when searching for the regexp .+
> .+ matches nothing but should match the same text as .*

Right, thanks.  Now I'm beginning to understand how this code works.

But the current situation is IMHO a bit unsatisfying - don't you think
that being able to use the "wildcard" ".*" is a common, when not the
most important reason why a user would want to use regexp filename
searching in dired?

Would it be an appropriate approach to use a more sophisticated value
for `isearch-search-fun-function' for that case?  This function could
e.g. jump to the next filename before starting searching.


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> Would it be an appropriate approach to use a more sophisticated value
> for `isearch-search-fun-function' for that case?  This function could
> e.g. jump to the next filename before starting searching.

Instead of duplicating the complex logic of `isearch-search-fun-function'
in a new specialized function it would be better to implement this
using hooks.

We have already a post-processing hook `isearch-update-post-hook'
invoked after isearch has found matches.  So we need a similar hook
invoked before isearch starts searching matches, with a name like
`isearch-search-fun-pre-hook'.

It fixes your test case of searching for ".*":

=== modified file 'lisp/isearch.el'
--- lisp/isearch.el	2013-02-25 21:10:59 +0000
+++ lisp/isearch.el	2013-03-21 22:53:41 +0000
@@ -163,6 +163,9 @@ (defcustom isearch-resume-in-command-his
 (defvar isearch-mode-hook nil
   "Function(s) to call after starting up an incremental search.")
 
+(defvar isearch-search-fun-pre-hook nil
+  "Function(s) to call before isearch starts searching matches in the buffer.")
+
 (defvar isearch-update-post-hook nil
   "Function(s) to call after isearch has found matches in the buffer.")
 
@@ -2651,7 +2654,9 @@ (defun isearch-search-string (string bou
 If found, move point to the end of the occurrence,
 update the match data, and return point."
   (let* ((func (isearch-search-fun))
-         (pos1 (save-excursion (funcall func string bound noerror)))
+         (pos1 (save-excursion
+		 (run-hooks 'isearch-search-fun-pre-hook)
+		 (funcall func string bound noerror)))
          pos2)
     (when (and
 	   ;; Avoid "obsolete" warnings for translation-table-for-input.

=== modified file 'lisp/dired-aux.el'
--- lisp/dired-aux.el	2013-02-28 21:51:11 +0000
+++ lisp/dired-aux.el	2013-03-21 22:54:14 +0000
@@ -2506,6 +2506,12 @@ (defun dired-isearch-filenames-toggle ()
   (setq isearch-success t isearch-adjusted t)
   (isearch-update))
 
+(defun dired-isearch-filenames-pre-hook ()
+  (unless (get-text-property (point) 'dired-filename)
+    (if isearch-forward
+	(goto-char (or (next-single-property-change (point) 'dired-filename) (point-max)))
+      (goto-char (or (previous-single-property-change (point) 'dired-filename) (point-min))))))
+
 ;;;###autoload
 (defun dired-isearch-filenames-setup ()
   "Set up isearch to search in Dired file names.
@@ -2518,14 +2524,16 @@ (defun dired-isearch-filenames-setup ()
     (setq dired-isearch-filter-predicate-orig
 	  (default-value 'isearch-filter-predicate))
     (setq-default isearch-filter-predicate 'dired-isearch-filter-filenames)
-    (add-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end nil t)))
+    (add-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end nil t)
+    (add-hook 'isearch-search-fun-pre-hook 'dired-isearch-filenames-pre-hook nil t)))
 
 (defun dired-isearch-filenames-end ()
   "Clean up the Dired file name search after terminating isearch."
   (setq isearch-message-prefix-add nil)
   (define-key isearch-mode-map "\M-sf" nil)
   (setq-default isearch-filter-predicate dired-isearch-filter-predicate-orig)
-  (remove-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end t))
+  (remove-hook 'isearch-mode-end-hook 'dired-isearch-filenames-end t)
+  (remove-hook 'isearch-search-fun-pre-hook 'dired-isearch-filenames-pre-hook t))
 
 (defun dired-isearch-filter-filenames (beg end)
   "Test whether the current search hit is a visible file name.

=== modified file 'lisp/wdired.el'
--- lisp/wdired.el	2013-03-05 17:13:01 +0000
+++ lisp/wdired.el	2013-03-21 22:55:11 +0000
@@ -241,6 +241,7 @@ (defun wdired-change-to-wdired-mode ()
   (set (make-local-variable 'query-replace-skip-read-only) t)
   (set (make-local-variable 'isearch-filter-predicate)
        'wdired-isearch-filter-read-only)
+  (add-hook 'isearch-search-fun-pre-hook 'dired-isearch-filenames-pre-hook nil t)
   (use-local-map wdired-mode-map)
   (force-mode-line-update)
   (setq buffer-read-only nil)

Also (run-hooks 'isearch-search-fun-pre-hook) should be added to replace.el,
but first I have to install the pending patches in bug#11378 and bug#11746.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Hi Juri,

thanks for working on this!

> > Would it be an appropriate approach to use a more sophisticated value
> > for `isearch-search-fun-function' for that case?  This function could
> > e.g. jump to the next filename before starting searching.
>
> Instead of duplicating the complex logic of `isearch-search-fun-function'
> in a new specialized function it would be better to implement this
> using hooks.

Ok.  In the meanwhile, I wrote a proof of concept, which seems to work
well (just for the record):

(defun dired-search-forward-filename-regexp (&rest args)
  (catch 'result
    (while t
      (let* ((result (apply #'search-forward-regexp args))
             (beg (and result (match-beginning 0)))
             (end (and result (match-end 0))))
        (if (or (not result)
                (and (get-text-property beg 'dired-filename)
                     (or (get-text-property end 'dired-filename)
                         (eq (char-after end) ?\n))))
            (throw 'result result)
          (if (eobp)
              (throw 'result nil)
            (goto-char (1+ beg))))))))

To test shortly, just eval

 (defun isearch-search-fun () 'dired-search-forward-filename-regexp)

Works well for me.  But I think that your approach is more appropriate
here.

> We have already a post-processing hook `isearch-update-post-hook'
> invoked after isearch has found matches.  So we need a similar hook
> invoked before isearch starts searching matches, with a name like
> `isearch-search-fun-pre-hook'.
>
> It fixes your test case of searching for ".*":

I quickly tested it.  But it doesn't yet do the right thing.  For the
first search hit, it does - but if you repeat searching (by repeatedly
hitting C-M-s), the behavior is like it is now - i.e., with repeated
searching, there is no progress.  Do you need to run
`isearch-search-fun-pre-hook' at other places as well?


Thanks,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> I quickly tested it.  But it doesn't yet do the right thing.  For the
> first search hit, it does - but if you repeat searching (by repeatedly
> hitting C-M-s), the behavior is like it is now - i.e., with repeated
> searching, there is no progress.

It doesn't work for `C-M-s' because by default the search in Dired
is not restricted to filenames (the default value of `dired-isearch-filenames'
is "No restrictions").  You have to start Isearch either by running
`M-x dired-isearch-filenames-regexp' or by customizing `dired-isearch-filenames'
to "Always search in file names" and then starting Isearch with `C-M-s'.

> Do you need to run `isearch-search-fun-pre-hook' at other places as well?

The patch is complete and it's working in my tests for ".*" and ".+".

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> > I quickly tested it.  But it doesn't yet do the right thing.  For the
> > first search hit, it does - but if you repeat searching (by repeatedly
> > hitting C-M-s), the behavior is like it is now - i.e., with repeated
> > searching, there is no progress.
>
> It doesn't work for `C-M-s' because by default the search in Dired
> is not restricted to filenames (the default value of
> dired-isearch-filenames'
> is "No restrictions").

No no, of course I have `dired-isearch-filenames' configured.  I already
double-checked everything - although it's already late here, I don't
think I made an error while testing.

> > Do you need to run `isearch-search-fun-pre-hook' at other places as well?
>
> The patch is complete and it's working in my tests for ".*" and ".+".

Just ".*" also works for me as it should (as supposed to before, which
shows that I indeed use your new code).  But if I try "a.*", the
following matches begin before the file names (my user name ends with
the letter a).


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Stefan Monnier]

> -         (pos1 (save-excursion (funcall func string bound noerror)))
> +         (pos1 (save-excursion
> +		 (run-hooks 'isearch-search-fun-pre-hook)
> +		 (funcall func string bound noerror)))

Doesn't sound good.

> +    (add-hook 'isearch-search-fun-pre-hook 'dired-isearch-filenames-pre-hook nil t)))

Why not (add-function :around (local isearch-search-fun-function)
                      #'dired--isearch-filenames)
and then

   (defun dired--isearch-filenames (iiff &rest args)
     (let ((fun (apply iiff args)))
       (lambda (&rest args)
         (unless (get-text-property (point) 'dired-filename)
           (if isearch-forward
   	       (goto-char (or (next-single-property-change
                               (point) 'dired-filename)
                              (point-max)))
             (goto-char (or (previous-single-property-change
                            (point) 'dired-filename)
                        (point-min)))))
         (apply fun args))))


-- Stefan

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> -         (pos1 (save-excursion (funcall func string bound noerror)))
>> +         (pos1 (save-excursion
>> +		 (run-hooks 'isearch-search-fun-pre-hook)
>> +		 (funcall func string bound noerror)))
>
> Doesn't sound good.
>
>> +    (add-hook 'isearch-search-fun-pre-hook 'dired-isearch-filenames-pre-hook nil t)))
>
> Why not (add-function :around (local isearch-search-fun-function)
>                       #'dired--isearch-filenames)

This is even better, so Michael could easily try different ideas
without applying patches.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> The patch is complete and it's working in my tests for ".*" and ".+".
>
> Just ".*" also works for me as it should (as supposed to before, which
> shows that I indeed use your new code).  But if I try "a.*", the
> following matches begin before the file names (my user name ends with
> the letter a).

When Isearch doesn't find "a.*" in the current filename it continues
searching from the beginning of the next Dired line and finds a match
in the details area that contains user names and permissions.
It seems the search should be limited to the end of the filename
on the current line.  But setting the BOUND arg of `search-forward-regexp'
to the end of the filename will cause Isearch to fail for the whole buffer.

BTW, I looked at your previous proof of concept and noticed that even though
it works correctly, its (goto-char (1+ beg)) is very inefficient especially
in the case of ".*" where is advances slowly by 1 character offset
for every search function call in the details area outside of filenames.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> > Why not (add-function :around (local isearch-search-fun-function)
> >                       #'dired--isearch-filenames)
>
> This is even better, so Michael could easily try different ideas
> without applying patches.

I must say I got a bit lost.  Still learning how isearch internally
works.  Juri, can you help me with a new patch?

I must also admit that I got used to use `dired-hide-details-mode' in
combination with isearch now and am very happy.  (BTW, I have
search-invisible -> open, but invisible fields are not opened - is this
normal?).


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Stefan Monnier]

> (BTW, I have search-invisible -> open, but invisible fields are not
> opened - is this normal?).

I think it's "normal" in the sense that isearch only handles `invisible'
when added by overlays rather than by text-properties.  IOW, it's
a known problem.  Aka "patch welcome".


        Stefan

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> I must say I got a bit lost.  Still learning how isearch internally
> works.  Juri, can you help me with a new patch?

Sorry, I still have not figured out how to limit the search for ".*"
only to file names.  Maybe there is no good solution.  For example,
to limit operations to file names only, Wdired uses such kludge
as `wdired-downcase-word' and `wdired-upcase-word' to skip non-file
parts of Dired buffers.

For a regexp ".*" to match file names only this might require a new
regexp feature for specifying text-properties in regexps similar to
`\sCODE' for syntax classes, or `\cC' for character categories,
or character classes like `[:ascii:]'.  So for example, a regexp like
"^[#dired-filename#]*$" would match only the text with text-properties
`dired-filename'.

> I must also admit that I got used to use `dired-hide-details-mode' in
> combination with isearch now and am very happy.  (BTW, I have
> search-invisible -> open, but invisible fields are not opened - is this
> normal?).

I wonder why do you want isearch to open hidden dired details when you use
`dired-hide-details-mode' with the intention to hide them from isearch?

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> > I must also admit that I got used to use `dired-hide-details-mode' in
> > combination with isearch now and am very happy.  (BTW, I have
> > search-invisible -> open, but invisible fields are not opened - is this
> > normal?).
>
> I wonder why do you want isearch to open hidden dired details when you use
> `dired-hide-details-mode' with the intention to hide them from isearch?

You misunderstood.  I don't want isearch to open dired details.  Just
in one moment, I wondered why it actually didn't, although
search-invisible was bound to open (because I generally want isearch to
open hidden stuff).


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> > I must also admit that I got used to use `dired-hide-details-mode' in
>> > combination with isearch now and am very happy.  (BTW, I have
>> > search-invisible -> open, but invisible fields are not opened - is this
>> > normal?).
>>
>> I wonder why do you want isearch to open hidden dired details when you use
>> `dired-hide-details-mode' with the intention to hide them from isearch?
>
> You misunderstood.  I don't want isearch to open dired details.  Just
> in one moment, I wondered why it actually didn't, although
> search-invisible was bound to open (because I generally want isearch to
> open hidden stuff).

Good.  Then let's leave dired details always hidden from isearch.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Right, thanks.  Now I'm beginning to understand how this code works.

BTW, one (related) thing that I found problematic over the time: I like
to use query-replace on file names in WDired.  There is no way to match
the beginning of the file name - right?  That's sometimes a problem.  In
this situation it is even better to use dired-isearch-filenames -> nil
because then I can match the space character before the beginning of the
file name.


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> BTW, one (related) thing that I found problematic over the time: I like
> to use query-replace on file names in WDired.  There is no way to match
> the beginning of the file name - right?  That's sometimes a problem.  In
> this situation it is even better to use dired-isearch-filenames -> nil
> because then I can match the space character before the beginning of the
> file name.

This is a valid workaround.  Maybe it could be mentioned in the documentation?

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Hello Juri,

Do you ask me?

I had hoped we somehow find a better solution for the whole complex (of
searching and replacing file names in Dired/WDired).  I don't object to
give such a hint, but I don't find the approach very... aesthetic.

Regards,

Michael.


Juri Linkov <juri@jurta.org> writes:

> > BTW, one (related) thing that I found problematic over the time: I like
> > to use query-replace on file names in WDired.  There is no way to match
> > the beginning of the file name - right?  That's sometimes a problem.  In
> > this situation it is even better to use dired-isearch-filenames -> nil
> > because then I can match the space character before the beginning of the
> > file name.
>
> This is a valid workaround.  Maybe it could be mentioned in the
> documentation?

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

Hi Michael,

> Do you ask me?

Yes.

> I had hoped we somehow find a better solution for the whole complex (of
> searching and replacing file names in Dired/WDired).  I don't object to
> give such a hint, but I don't find the approach very... aesthetic.

I intended to continue searching for a better solution,
but when I tried to perform query-replace in Wdired,
it fails with the error:

Debugger entered--Lisp error: (error "Match data clobbered by buffer modification hooks")
  replace-match("to" nil t)
  replace-match-maybe-edit("to" nil t nil (883 890 #<buffer test>) nil)
  perform-replace(#("from" 0 7 (isearch-case-fold-search t isearch-regexp-function nil)) "to" t nil nil nil nil nil nil nil nil)
  query-replace(#("from" 0 7 (isearch-case-fold-search t isearch-regexp-function nil)) "to" nil nil nil nil nil)
  funcall-interactively(query-replace #("from" 0 7 (isearch-case-fold-search t isearch-regexp-function nil)) "to" nil nil nil nil nil)
  call-interactively(query-replace nil nil)
  command-execute(query-replace)

This is something new, maybe caused by some recent changes.
But I can't find anything suspicious in the commit history of replace.el.

Do you see the same error?

bug#14013: [External] : bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Drew Adams]

> when I tried to perform query-replace in Wdired,
> it fails with the error:
> 
> Debugger entered--Lisp error: (error "Match data clobbered by buffer
> modification hooks")
>   replace-match("to" nil t)...
> 
> This is something new, maybe caused by some recent changes.
> But I can't find anything suspicious in the commit history of
> replace.el.
> 
> Do you see the same error?

I think that's maybe something new.  I don't see that,
with, e.g., Emacs 26.3.

I do see the problem of not being able to use a regexp
that matches beginning of file name, for query replace.

That problem is from isearch filtering.  I see that
same problem when, say, limiting query replacing to a
rectangle that contains just file names (or file names
plus spaces before them).  That's entirely understandable. 

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> This is something new, maybe caused by some recent changes.  But I
> can't find anything suspicious in the commit history of replace.el.

Yes, I see it too indeed.

I guess

  4dbc44550d * lisp/wdired.el: Apply text properties lazily

could be related, especially because the error seems to happen only once
per file line.


Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I guess
>
>   4dbc44550d * lisp/wdired.el: Apply text properties lazily
>
> could be related, especially because the error seems to happen only
> once per file line.

Wrapping the body of `wdired--before-change-fn' into `save-match-data'
seems to fix that problem.  Would that be an appropriate fix?

Regards,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>>   4dbc44550d * lisp/wdired.el: Apply text properties lazily
>>
>> could be related, especially because the error seems to happen only
>> once per file line.
>
> Wrapping the body of `wdired--before-change-fn' into `save-match-data'
> seems to fix that problem.  Would that be an appropriate fix?

Thanks for the fix.  Now installed to master.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

forcemerge 14013 29215
thanks

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> Right, thanks.  Now I'm beginning to understand how this code works.
>
> BTW, one (related) thing that I found problematic over the time: I like
> to use query-replace on file names in WDired.  There is no way to match
> the beginning of the file name - right?  That's sometimes a problem.  In
> this situation it is even better to use dired-isearch-filenames -> nil
> because then I can match the space character before the beginning of the
> file name.

Like was discussed in bug#53758, xref uses ".*" to replace in results
where search/replace should be performed only within boundaries of matches.
xref uses own search function:

         (replace-re-search-function
          (lambda (from &optional _bound noerror)
            (let (found pair)
              (while (and (not found) pairs)
                (setq pair (pop pairs)
                      current-beg (car pair)
                      current-end (cdr pair))
                (goto-char current-beg)
                (when (re-search-forward from current-end noerror)
                  (setq found t)))
              found)))

So maybe Dired/WDired could use a similar function.

But in any case it seems matching at the beginning of the file name with "^"
as also requested by Drew in bug#29215 (merged) is still impossible.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

> Why not (add-function :around (local isearch-search-fun-function)
>                       #'dired--isearch-filenames)
> and then
>
>    (defun dired--isearch-filenames (iiff &rest args)
>      (let ((fun (apply iiff args)))
>        (lambda (&rest args)
>          (unless (get-text-property (point) 'dired-filename)
>            (if isearch-forward
>    	       (goto-char (or (next-single-property-change
>                                (point) 'dired-filename)
>                               (point-max)))
>              (goto-char (or (previous-single-property-change
>                             (point) 'dired-filename)
>                         (point-min)))))
>          (apply fun args))))

After a short delay, this has been implemented now:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired-isearch-search-filenames.patch --]
[-- Type: text/x-diff, Size: 4282 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 41c45b4e51..f1d0f9e225 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3140,11 +3140,11 @@ dired-isearch-filenames-mode
 When off, it uses the original predicate."
   :lighter nil
   (if dired-isearch-filenames-mode
-      (add-function :before-while (local 'isearch-filter-predicate)
-                    #'dired-isearch-filter-filenames
+      (add-function :around (local 'isearch-search-fun-function)
+                    #'dired-isearch-search-filenames
                     '((isearch-message-prefix . "filename ")))
-    (remove-function (local 'isearch-filter-predicate)
-                     #'dired-isearch-filter-filenames))
+    (remove-function (local 'isearch-search-fun-function)
+                     #'dired-isearch-search-filenames))
   (when isearch-mode
     (setq isearch-success t isearch-adjusted t)
     (isearch-update)))
@@ -3168,12 +3168,34 @@ dired-isearch-filenames-end
   (unless isearch-suspended
     (kill-local-variable 'dired-isearch-filenames)))
 
-(defun dired-isearch-filter-filenames (beg end)
+(defun dired-isearch-search-filenames (orig-fun)
   "Test whether some part of the current search match is inside a file name.
 This function returns non-nil if some part of the text between BEG and END
 is part of a file name (i.e., has the text property `dired-filename')."
-  (text-property-not-all (min beg end) (max beg end)
-			 'dired-filename nil))
+  (let ((search-fun (funcall orig-fun)))
+    (lambda (string &optional bound noerror count)
+      (let ((beg (and (get-text-property (point) 'dired-filename) (point)))
+            end found)
+        (unless beg
+          (setq beg (if isearch-forward
+                        (next-single-property-change (point) 'dired-filename)
+                      (previous-single-property-change (point) 'dired-filename)))
+          (when beg (goto-char beg)))
+        (while (and beg (not found))
+          (setq end (if isearch-forward
+                        (next-single-property-change beg 'dired-filename)
+                      (previous-single-property-change beg 'dired-filename)))
+          (setq found (funcall search-fun string
+                               (if isearch-forward
+                                   (min (or bound (point-max)) end)
+                                 (max (or bound (point-max)) end))
+                               noerror count))
+          (unless found
+            (setq beg (if isearch-forward
+                          (next-single-property-change end 'dired-filename)
+                        (previous-single-property-change end 'dired-filename)))
+            (when beg (goto-char beg))))
+        found))))
 
 ;;;###autoload
 (defun dired-isearch-filenames ()
diff --git a/lisp/wdired.el b/lisp/wdired.el
index ab3b91bbe5..573e125bc8 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -217,6 +217,7 @@ wdired-mode
   (error "This mode can be enabled only by `wdired-change-to-wdired-mode'"))
 (put 'wdired-mode 'mode-class 'special)
 
+(autoload 'dired-isearch-search-filenames "dired-aux")
 
 ;;;###autoload
 (defun wdired-change-to-wdired-mode ()
@@ -238,8 +239,9 @@ wdired-change-to-wdired-mode
   (setq-local wdired--old-point (point))
   (wdired--set-permission-bounds)
   (setq-local query-replace-skip-read-only t)
-  (add-function :after-while (local 'isearch-filter-predicate)
-                #'wdired-isearch-filter-read-only)
+  (add-function :around (local 'isearch-search-fun-function)
+                #'dired-isearch-search-filenames)
+  (setq-local replace-re-search-function #'dired-isearch-search-filenames)
   (use-local-map wdired-mode-map)
   (force-mode-line-update)
   (setq buffer-read-only nil)
@@ -438,8 +440,9 @@ wdired-change-to-dired-mode
     (remove-text-properties
      (point-min) (point-max)
      '(front-sticky nil rear-nonsticky nil read-only nil keymap nil)))
-  (remove-function (local 'isearch-filter-predicate)
-                   #'wdired-isearch-filter-read-only)
+  (remove-function (local 'isearch-search-fun-function)
+                   #'dired-isearch-search-filenames)
+  (kill-local-variable 'replace-re-search-function)
   (use-local-map dired-mode-map)
   (force-mode-line-update)
   (setq buffer-read-only t)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> After a short delay, this has been implemented now:

Thanks.  I quickly tested your patch, and it seems to work well so far
(except for the ^ thing fo course).  Also highlighting worked.  Anything
particular I should keep my eye on?

I only noticed that if we keep the patch, the docstrings of the touched
functions seem to need and update.

>  (defun wdired-change-to-wdired-mode ()
> @@ -238,8 +239,9 @@ wdired-change-to-wdired-mode
>    (setq-local wdired--old-point (point))
>    (wdired--set-permission-bounds)
>    (setq-local query-replace-skip-read-only t)
> -  (add-function :after-while (local 'isearch-filter-predicate)
> -                #'wdired-isearch-filter-read-only)
> +  (add-function :around (local 'isearch-search-fun-function)
> +                #'dired-isearch-search-filenames)
> +  (setq-local replace-re-search-function #'dired-isearch-search-filenames)

And: Is it intended that this is unconditional (I would expect a
`dired-isearch-filenames-mode' test)?

Thanks,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> Thanks.  I quickly tested your patch, and it seems to work well so far

Thanks for testing, this confirms that the proof of concept is viable,
so now the docstrings could be updated as well.

> (except for the ^ thing fo course).  Also highlighting worked.

To implement ^, I see no way other than special handling:
when a regexp begins with ^, then after removing ^ from the regexp,
call 'looking-at' at the beginning of the file name.

> Anything particular I should keep my eye on?

Please help to decide how search/replace should be implemented in WDired.
The current implementation filters out read-only parts of the Dired buffer.
Should the new search function also skip read-only parts or should it
match only on file names in WDired the same way as in Dired?

>>  (defun wdired-change-to-wdired-mode ()
>> @@ -238,8 +239,9 @@ wdired-change-to-wdired-mode
>>    (setq-local wdired--old-point (point))
>>    (wdired--set-permission-bounds)
>>    (setq-local query-replace-skip-read-only t)
>> -  (add-function :after-while (local 'isearch-filter-predicate)
>> -                #'wdired-isearch-filter-read-only)
>> +  (add-function :around (local 'isearch-search-fun-function)
>> +                #'dired-isearch-search-filenames)
>> +  (setq-local replace-re-search-function #'dired-isearch-search-filenames)
>
> And: Is it intended that this is unconditional (I would expect a
> `dired-isearch-filenames-mode' test)?

Currently it is unconditional when it's using isearch-filter-predicate.
So maybe it should be kept this way since it's what was used for many years?
Or is there a reason to change the current behavior?

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> To implement ^, I see no way other than special handling:
> when a regexp begins with ^, then after removing ^ from the regexp,
> call 'looking-at' at the beginning of the file name.

I don't know if we want to handle ^ specially, but if we do we can use
something like:

    (while (string-match "\\^" RE i)
      (setq i (match-end 0))
      (if (not (subregexp-context-p RE (match-beginning 0)))
          ;; The ^ is inside a char-range or escaped or something.
          nil
        (setq RE (replace-match (concat "\\(?:"
                                        directory-listing-before-filename-regexp
                                        "\\)")
                                t t RE))))


-- Stefan

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> To implement ^, I see no way other than special handling:
> when a regexp begins with ^, then after removing ^ from the regexp,
> call 'looking-at' at the beginning of the file name.

Just an idea: Seems that currently \= matches the position at the
beginning of the file names!  Could we maybe just guarantee and document
that?

If \= works reliably, we may also transform a leading \` in the regexp
into \=.  ^ is not so important in my eyes, it's easier to type than \`,
but the semantics of "beginning of line" is not as clear in this context
as "beginning of the object searched". If \= and \` would work, I would
be very happy.

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> Just an idea: Seems that currently \= matches the position at the
> beginning of the file names!  Could we maybe just guarantee and document
> that?

Nice finding.  This works reliably only because the search function puts point
at the beginning of every file name during search.

> If \= works reliably, we may also transform a leading \` in the regexp
> into \=.  ^ is not so important in my eyes, it's easier to type than \`,
> but the semantics of "beginning of line" is not as clear in this context
> as "beginning of the object searched". If \= and \` would work, I would
> be very happy.

Since ^ is easier to type, maybe both ^ and \` should be transformed into \=.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> >>  (defun wdired-change-to-wdired-mode ()
> >> @@ -238,8 +239,9 @@ wdired-change-to-wdired-mode
> >>    (setq-local wdired--old-point (point))
> >>    (wdired--set-permission-bounds)
> >>    (setq-local query-replace-skip-read-only t)
> >> -  (add-function :after-while (local 'isearch-filter-predicate)
> >> -                #'wdired-isearch-filter-read-only)
> >> +  (add-function :around (local 'isearch-search-fun-function)
> >> +                #'dired-isearch-search-filenames)
> >> +  (setq-local replace-re-search-function #'dired-isearch-search-filenames)
> >
> > And: Is it intended that this is unconditional (I would expect a
> > `dired-isearch-filenames-mode' test)?
>
> Currently it is unconditional when it's using isearch-filter-predicate.
> So maybe it should be kept this way since it's what was used for many years?
> Or is there a reason to change the current behavior?

Ok, then better let's keep it.

A different thing: I found that

| + (setq-local replace-re-search-function #'dired-isearch-search-filenames)

is not correct - of course, `dired-isearch-search-filenames' is a higher
order function (used for the around advice), not something suitable for
searching.  With the patch installed query-replace in wdired errors.

What's the correct value - the current binding of
`isearch-search-fun-function'?


Thanks,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> I don't know if we want to handle ^ specially, but if we do we can use
> something like:
>
>     (while (string-match "\\^" RE i)
>       (setq i (match-end 0))
>       (if (not (subregexp-context-p RE (match-beginning 0)))
>           ;; The ^ is inside a char-range or escaped or something.
>           nil
>         (setq RE (replace-match (concat "\\(?:"
>                                         directory-listing-before-filename-regexp
>                                         "\\)")
>                                 t t RE))))

Will matches then not be discarded by `dired-isearch-filenames-mode'
because they start before the file name?

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> This works reliably only because the search function puts point at the
> beginning of every file name during search.

I'm not about non-interactive searches, seldom use them - can something
like `dired-isearch-filenames-regexp' be done non-interactively?

Anyway, my complete idea was: whenever we provide a special search
command that limits isearch to certain entities, like file names in a
dired buffer, we always guarantee that point can be matched at the
beginning of each entity with \= (and maybe also \` and ^).  In an
interactive search \= is quite useless (right?), so we can use it for
that and avoid hacks that would probably be worse.

We would still not have a solution to match the end of the entities,
however.  Fortunately for files in dired $ works (by coincidence).

> Since ^ is easier to type, maybe both ^ and \` should be transformed
> into \=.

Why not.

Michael.

bug#14013: [External] : bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Drew Adams]

BTW, I just noticed that a couple of functions (commands) in dired-aux.el bind _user option_ `dired-isearch-filenames'.

Personally, I'm in favor of allowing code to do this, as long as its doc makes clear to users that this is happening.

But I thought this was a strict no-no - anathema for Emacs development.  Was this - as opposed to jumping through a hoop adding an additional variable - OK'd by the priests? ;-)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> I don't know if we want to handle ^ specially, but if we do we can use
>> something like:
>>
>>     (while (string-match "\\^" RE i)
>>       (setq i (match-end 0))
>>       (if (not (subregexp-context-p RE (match-beginning 0)))
>>           ;; The ^ is inside a char-range or escaped or something.
>>           nil
>>         (setq RE (replace-match (concat "\\(?:"
>>                                         directory-listing-before-filename-regexp
>>                                         "\\)")
>>                                 t t RE))))
>
> Will matches then not be discarded by `dired-isearch-filenames-mode'
> because they start before the file name?

It all depends on the replacement regexp the code uses.  I only used
`directory-listing-before-filename-regexp` as an example.


        Stefan

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> This works reliably only because the search function puts point at the
>> beginning of every file name during search.
>
> I'm not about non-interactive searches, seldom use them - can something
> like `dired-isearch-filenames-regexp' be done non-interactively?

It should work non-interactively as well.

> Anyway, my complete idea was: whenever we provide a special search
> command that limits isearch to certain entities, like file names in a
> dired buffer, we always guarantee that point can be matched at the
> beginning of each entity with \= (and maybe also \` and ^).  In an
> interactive search \= is quite useless (right?), so we can use it for
> that and avoid hacks that would probably be worse.

This is more complicated for backward search where \= should match
at the end of the file name, so \' and $ should be replaced with \=.

> We would still not have a solution to match the end of the entities,
> however.  Fortunately for files in dired $ works (by coincidence).

This is still not reliable since the dired line doesn't always end
with the end of the file name such as "filename -> symlink" format.

But with the previous patch this works because of the BOUND argument
of the search function set to the end of the file name.

For the backward search, the BOUND argument corresponds to $.
This makes the search function more complicated, but
this is doable, just needs more special-casing.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> For the backward search, the BOUND argument corresponds to $.
> This makes the search function more complicated, but
> this is doable, just needs more special-casing.

Ok, cool, then I think this is something I really would like to see (and
test).

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> With the patch installed query-replace in wdired errors.
>
> What's the correct value - the current binding of
> `isearch-search-fun-function'?

While trying to find the correct value in WDired, I noticed
a regression: in Emacs 27 search/replace skips read-only parts,
but in Emacs 28 there are no more read-only properties in
WDired buffers, so search/replace matches everything.
Looks like quite a recent change.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> While trying to find the correct value in WDired, I noticed
> a regression: in Emacs 27 search/replace skips read-only parts,
> but in Emacs 28 there are no more read-only properties in
> WDired buffers, so search/replace matches everything.
> Looks like quite a recent change.

There had been a change that made property changes be applied delayed on
the fly for faster startup of wdired.  This now happens only for lines
that the user edits.  Could that be related?

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> There had been a change that made property changes be applied delayed on
> the fly for faster startup of wdired.  This now happens only for lines
> that the user edits.  Could that be related?

But I remember that you were involved in fixing fallout of that change
for the isearch side.  So maybe a different change is the culprit.

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> There had been a change that made property changes be applied delayed on
>> the fly for faster startup of wdired.  This now happens only for lines
>> that the user edits.  Could that be related?
>
> But I remember that you were involved in fixing fallout of that change
> for the isearch side.  So maybe a different change is the culprit.

We fixed only the error raised in query-replace by adding save-match-data.
And now we are fixing the matching only within filenames for Emacs 29.
This means that Emacs 28 will be released with this regression.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

>> >> +  (add-function :around (local 'isearch-search-fun-function)
>> >> +                #'dired-isearch-search-filenames)
>> >
>> > And: Is it intended that this is unconditional (I would expect a
>> > `dired-isearch-filenames-mode' test)?
>>
>> Currently it is unconditional when it's using isearch-filter-predicate.
>> So maybe it should be kept this way since it's what was used for many years?
>> Or is there a reason to change the current behavior?
>
> Ok, then better let's keep it.

For more customizability I added a new user option
'wdired-search-replace-filenames' enabled by default.

> A different thing: I found that
>
> | + (setq-local replace-re-search-function #'dired-isearch-search-filenames)
>
> is not correct - of course, `dired-isearch-search-filenames' is a higher
> order function (used for the around advice), not something suitable for
> searching.  With the patch installed query-replace in wdired errors.
>
> What's the correct value - the current binding of
> `isearch-search-fun-function'?

You are right, the correct value is the current binding of
`isearch-search-fun-function'.  Everything is fixed now
in this patch, i.e. search/replace works ok in dired/wdired buffers
(except ^ at the beginning of filenames that is a separate feature):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired-isearch-search-filenames.patch --]
[-- Type: text/x-diff, Size: 5840 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 56897826cb..92f2848334 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3147,11 +3147,11 @@ dired-isearch-filenames-mode
 When off, it uses the original predicate."
   :lighter nil
   (if dired-isearch-filenames-mode
-      (add-function :before-while (local 'isearch-filter-predicate)
-                    #'dired-isearch-filter-filenames
+      (add-function :around (local 'isearch-search-fun-function)
+                    #'dired-isearch-search-filenames
                     '((isearch-message-prefix . "filename ")))
-    (remove-function (local 'isearch-filter-predicate)
-                     #'dired-isearch-filter-filenames))
+    (remove-function (local 'isearch-search-fun-function)
+                     #'dired-isearch-search-filenames))
   (when isearch-mode
     (setq isearch-success t isearch-adjusted t)
     (isearch-update)))
@@ -3175,12 +3175,42 @@ dired-isearch-filenames-end
   (unless isearch-suspended
     (kill-local-variable 'dired-isearch-filenames)))
 
-(defun dired-isearch-filter-filenames (beg end)
+(defun dired-isearch-search-filenames (orig-fun)
   "Test whether some part of the current search match is inside a file name.
 This function returns non-nil if some part of the text between BEG and END
 is part of a file name (i.e., has the text property `dired-filename')."
-  (text-property-not-all (min beg end) (max beg end)
-			 'dired-filename nil))
+  (let ((search-fun (funcall orig-fun)))
+    (lambda (string &optional bound noerror count)
+      (let ((old-pos (point))
+            (beg (when (get-text-property
+                        (if isearch-forward (point) (max (1- (point)) (point-min)))
+                        'dired-filename)
+                   (point)))
+            end found)
+        (unless beg
+          (setq beg (if isearch-forward
+                        (next-single-property-change (point) 'dired-filename)
+                      (previous-single-property-change (point) 'dired-filename)))
+          (when beg (goto-char beg)))
+        (while (and beg (not found))
+          (setq end (if isearch-forward
+                        (next-single-property-change beg 'dired-filename)
+                      (previous-single-property-change beg 'dired-filename)))
+          (if (not end)
+              (setq beg nil)
+            (setq found (funcall
+                         search-fun string (if bound (if isearch-forward
+                                                         (min bound end)
+                                                       (max bound end))
+                                             end)
+                         noerror count))
+            (unless found
+              (setq beg (if isearch-forward
+                            (next-single-property-change end 'dired-filename)
+                          (previous-single-property-change end 'dired-filename)))
+              (when beg (goto-char beg)))))
+        (unless found (goto-char old-pos))
+        found))))
 
 ;;;###autoload
 (defun dired-isearch-filenames ()
diff --git a/lisp/wdired.el b/lisp/wdired.el
index ab3b91bbe5..229a266d33 100644
--- a/lisp/wdired.el
+++ b/lisp/wdired.el
@@ -155,6 +155,11 @@ wdired-create-parent-directories
   :version "26.1"
   :type 'boolean)
 
+(defcustom wdired-search-replace-filenames t
+  "Non-nil to search and replace in file names only."
+  :version "29.1"
+  :type 'boolean)
+
 (defvar-keymap wdired-mode-map
   :doc "Keymap used in `wdired-mode'."
   "C-x C-s" #'wdired-finish-edit
@@ -217,6 +222,7 @@ wdired-mode
   (error "This mode can be enabled only by `wdired-change-to-wdired-mode'"))
 (put 'wdired-mode 'mode-class 'special)
 
+(declare-function dired-isearch-search-filenames "dired-aux")
 
 ;;;###autoload
 (defun wdired-change-to-wdired-mode ()
@@ -237,9 +243,12 @@ wdired-change-to-wdired-mode
               (dired-remember-marks (point-min) (point-max)))
   (setq-local wdired--old-point (point))
   (wdired--set-permission-bounds)
-  (setq-local query-replace-skip-read-only t)
-  (add-function :after-while (local 'isearch-filter-predicate)
-                #'wdired-isearch-filter-read-only)
+  (when wdired-search-replace-filenames
+    (add-function :around (local 'isearch-search-fun-function)
+                  #'dired-isearch-search-filenames
+                  '((isearch-message-prefix . "filename ")))
+    (setq-local replace-search-function
+    (setq-local replace-re-search-function (funcall isearch-search-fun-function))))
   (use-local-map wdired-mode-map)
   (force-mode-line-update)
   (setq buffer-read-only nil)
@@ -319,11 +328,6 @@ wdired--before-change-fn
             ;; Is this good enough? Assumes no extra white lines from dired.
             (put-text-property (1- (point-max)) (point-max) 'read-only t)))))))
 
-(defun wdired-isearch-filter-read-only (beg end)
-  "Skip matches that have a read-only property."
-  (not (text-property-not-all (min beg end) (max beg end)
-			      'read-only nil)))
-
 ;; Protect the buffer so only the filenames can be changed, and put
 ;; properties so filenames (old and new) can be easily found.
 (defun wdired--preprocess-files ()
@@ -438,8 +442,11 @@ wdired-change-to-dired-mode
     (remove-text-properties
      (point-min) (point-max)
      '(front-sticky nil rear-nonsticky nil read-only nil keymap nil)))
-  (remove-function (local 'isearch-filter-predicate)
-                   #'wdired-isearch-filter-read-only)
+  (when wdired-search-replace-filenames
+    (remove-function (local 'isearch-search-fun-function)
+                     #'dired-isearch-search-filenames)
+    (kill-local-variable 'replace-search-function)
+    (kill-local-variable 'replace-re-search-function))
   (use-local-map dired-mode-map)
   (force-mode-line-update)
   (setq buffer-read-only t)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> Everything is fixed now in this patch, i.e. search/replace works ok in
> dired/wdired buffers (except ^ at the beginning of filenames that is a
> separate feature):

Thanks - let me try it out!

> +    (setq-local replace-search-function
> +    (setq-local replace-re-search-function (funcall isearch-search-fun-function))))

Something's strange here.  The indentation level of the second line
suggests that you didn't intend this to be a nested `setq-local' call -
or did you?

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Thanks - let me try it out!
>
> > +    (setq-local replace-search-function
> > + (setq-local replace-re-search-function (funcall
> > isearch-search-fun-function))))
>
> Something's strange here.  The indentation level of the second line
> suggests that you didn't intend this to be a nested `setq-local' call
> - or did you?

Yes, seems to work well.

Only my \= trick doesn't work at all :-(

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> > +    (setq-local replace-search-function
>> > + (setq-local replace-re-search-function (funcall
>> > isearch-search-fun-function))))
>>
>> Something's strange here.  The indentation level of the second line
>> suggests that you didn't intend this to be a nested `setq-local' call
>> - or did you?
>
> Yes, seems to work well.

Sorry for code obfuscation.  It's really a nested `setq-local'.

> Only my \= trick doesn't work at all :-(

Alas, \= doesn't work, especially in backward search.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> Thanks - let me try it out!
> ...
> Yes, seems to work well.

Thanks for trying out.  Now I pushed the patch to master
that will clean up the platz for more features such as ^.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> Thanks for trying out.  Now I pushed the patch to master
> that will clean up the platz for more features such as ^.

I wonder if it is realistic to use a temporary helper buffer to
implement ^.

I would like to have a kind of filter that would allow to restrict
isearch and query replace to arbitrary parts of the buffer, defined e.g.
by the presence of some text property, anything you can define.

If we would use a helper buffer that contains only these chunks, ^ and
such would work out of the box for all of these cases.  I guess that
such a helper buffer would have to be filled on the fly, successively,
and the hard part is the details of handling (updating, killing, etc)
these buffers.

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> I wonder if it is realistic to use a temporary helper buffer to
> implement ^.

Good idea.  Maybe even not a buffer, but just a string.
Are there any differences between buffer matching and string matching?

Then first we could remove ^ from the search regexp, and when it finds
something, then get the found buffer-substring using text properties
and match it with the original regexp that contains ^.

> I would like to have a kind of filter that would allow to restrict
> isearch and query replace to arbitrary parts of the buffer, defined e.g.
> by the presence of some text property, anything you can define.

This has been asked before for search/replace in a rectangular region.
So since we have pairs of beg/end bounds for parts of the rectangle,
it shouldn't be different from search/replace in Dired that uses
text properties.

> If we would use a helper buffer that contains only these chunks, ^ and
> such would work out of the box for all of these cases.  I guess that
> such a helper buffer would have to be filled on the fly, successively,
> and the hard part is the details of handling (updating, killing, etc)
> these buffers.

It seems you intended to use such buffers for more complex feature than handling ^.
Because when you copy such a rectangular region to a separate buffer:

     +---+
  123|456|789
  abc|def|ghi
     +---+

then do you want to match the copied parts as contiguous text?
So the above rectangle when copied to a separate buffer:

      456
      def

do you expect it should match the regexp "456.def"?

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

>> I wonder if it is realistic to use a temporary helper buffer to
>> implement ^.
>
> Good idea.  Maybe even not a buffer, but just a string.
> Are there any differences between buffer matching and string matching?
>
> Then first we could remove ^ from the search regexp, and when it finds
> something, then get the found buffer-substring using text properties
> and match it with the original regexp that contains ^.

This works surprisingly well.  Maybe there are more corner cases,
but something already works with quick tests:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired-isearch-search-filenames.patch --]
[-- Type: text/x-diff, Size: 1513 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index c49e4e91d8..0832ea1ddb 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3202,7 +3202,16 @@ dired-isearch-search-filenames
                          (if isearch-forward old (max (1- old) (point-min)))
                          property)
                     old))
-             end found)
+             end found regexp (i 0))
+        (when isearch-regexp
+          (setq regexp string)
+          (while (string-match "\\^\\|\\$\\|\\\\`\\|\\\\'" string i)
+            (setq i (- (match-end 0) (length (match-string 0 string))))
+            (if (save-match-data (not (subregexp-context-p
+                                       string (match-beginning 0))))
+                ;; The ^/$ is inside a char-range or escaped or something.
+                nil
+              (setq string (replace-match "" t t string)))))
         ;; Otherwise, try to search for the next property.
         (unless beg
           (setq beg (if isearch-forward
@@ -3221,6 +3230,9 @@ dired-isearch-search-filenames
                                                      (max bound end))
                                            end)
                        noerror count))
+          (when (and regexp (not (string-match-p
+                                  regexp (buffer-substring beg end))))
+            (setq found nil))
           (unless found
             (setq beg (if isearch-forward
                           (next-single-property-change end property)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> This works surprisingly well.  Maybe there are more corner cases,
> but something already works with quick tests:

But there is a long-standing bug in query-replace.
The optimization that uses `match-again` and `looking-at`
ignores search-related variables such as
isearch-search-fun-function, replace-re-search-function, etc.

Here's is a test case that demonstrates the problem in current master
with the recent addition of dired-isearch-search-filenames:

1. cd /tmp; touch file1; ln -s file1 file2
2. enter Wdired and move point to the beginning of file2
3. C-M-% .* RET foo RET
4. answer `n` when asked to replace `file2`

After that the remaining part of the same line is highlighted,
i.e. the part after "file2" (that is a symbolic link) in:

  file2 -> file1

This is because the `match-again` optimization uses `(looking-at ".*")`
after the previous replacement "file2" to ask about the next replacement
of " -> file1" that ignores isearch-search-fun-function.

I believe it would be safe to just remove this `match-again` optimization
from `perform-replace`.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> So the above rectangle when copied to a separate buffer:
>
>       456
>       def
>
> do you expect it should match the regexp "456.def"?

I would not want that this matches.

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> This works surprisingly well.  Maybe there are more corner cases,
> but something already works with quick tests:
>
> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el

Yes, not bad indeed.  Did you find any corner cases?

Thanks,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

>> This works surprisingly well.  Maybe there are more corner cases,
>> but something already works with quick tests:
>
> Yes, not bad indeed.  Did you find any corner cases?

Below is the patch that works quite well for the most use cases.
But unfortunately, there are still some unsolvable corner cases:
when a file name is e.g. "aaxbbx" and the search regexp is "x$",
then after removing "$", searching for "x" will find the first
occurrence of "x", and will set match-data to it.  Later code
that uses string-match can adjust the found position to the
last occurrence of "x".  But it can't change match-data
used by isearch.  And using a temporary buffer won't help either
to set the real buffer positions in match-data to the last "x".
IOW, no post-processing can help the main search function
to set match-data to the correct place that matches "x$"
in the dired buffer (and file name doesn't always end at eol).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: dired-isearch-search-filenames.patch --]
[-- Type: text/x-diff, Size: 2627 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 4faf9431aa..bb17760598 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3217,7 +3217,17 @@ dired-isearch-search-filenames
                          (if isearch-forward old (max (1- old) (point-min)))
                          property)
                     old))
-             end found)
+             end found regexp regexp-^ regexp-$ (i 0))
+        (when isearch-regexp
+          (setq regexp string)
+          (while (string-match "\\(\\^\\|\\\\`\\)\\|\\$\\|\\\\'" string i)
+            (setq i (match-beginning 0))
+            (if (save-match-data (not (subregexp-context-p
+                                       string (match-beginning 0))))
+                ;; The ^/$ is inside a char-range or escaped or something.
+                (setq i (1+ i))
+              (setq string (replace-match "" t t string))
+              (if (match-beginning 1) (setq regexp-^ t) (setq regexp-$ t)))))
         ;; Otherwise, try to search for the next property.
         (unless beg
           (setq beg (if isearch-forward
@@ -3236,6 +3246,29 @@ dired-isearch-search-filenames
                                                      (max bound end))
                                            end)
                        noerror count))
+          ;; Handle ^/$ specially
+          (when (and regexp found)
+            ;; Apply ^/$ regexp on the whole filename field
+            (save-match-data
+              (if (string-match regexp (buffer-substring beg end))
+                  ;; FIXME: better to modify previous match-data
+                  (setq found (if isearch-forward
+                                  (+ beg (match-end 0))
+                                (- beg (match-end 0))))
+                (setq found nil)))
+            ;; Check ^/$ matches at filename field boundaries
+            (when found
+              (goto-char found)
+              (unless (and (or (not regexp-^)
+                               (eq (if isearch-forward beg end) (point-min))
+                               (null (get-text-property
+                                      (1- (if isearch-forward beg end)) property)))
+                           (or (not regexp-$)
+                               (eq (point) (point-max))
+                               (null (get-text-property
+                                      (point) property))))
+                (setq found nil))))
+          ;; Get the next filename field
           (unless found
             (setq beg (if isearch-forward
                           (next-single-property-change end property)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> So the above rectangle when copied to a separate buffer:
>>
>>       456
>>       def
>>
>> do you expect it should match the regexp "456.def"?
>
> I would not want that this matches.

BTW, do you think that such regexps as ".*" and "^.*$"
should be supported not only in Dired buffers, but also
when making replacements in rectangular regions?
E.g. C-x SPC ... C-M-% .* RET RET should effectively
clear the region, etc.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> So the above rectangle when copied to a separate buffer:
>>
>>       456
>>       def
>>
>> do you expect it should match the regexp "456.def"?
>
> I would not want that this matches.

I generalized dired-isearch-search-filenames to a new function
that can search in any text property, so e.g. after evaluating

  (setq-local isearch-search-fun-function
  (lambda () (isearch-search-fun-in-text-property 'face)))

isearch will match .* in all fontified chunks, etc.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

>> This works surprisingly well.  Maybe there are more corner cases,
>> but something already works with quick tests:
>>
>> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
>
> Yes, not bad indeed.  Did you find any corner cases?

Using a temporary buffer like you proposed works fine,
and handles all possible regexps including corner cases.
However, wouldn't this make the search too inefficient?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: isearch-search-fun-in-text-property.patch --]
[-- Type: text/x-diff, Size: 1961 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 5fbfb724a3..8470170f24 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -4482,12 +4545,25 @@ isearch-search-fun-in-text-property
         (setq end (if isearch-forward
                       (next-single-property-change beg property)
                     (previous-single-property-change beg property)))
-        (setq found (funcall (or search-fun (isearch-search-fun-default))
-                             string (if bound (if isearch-forward
-                                                  (min bound end)
-                                                (max bound end))
-                                      end)
-                             noerror count))
+        (if (and isearch-regexp (string-match-p "\\(\\^\\)\\|\\$" string))
+            (let ((substring (buffer-substring beg end))
+                  match-data)
+              (with-temp-buffer
+                (insert substring)
+                (goto-char (point-min))
+                (setq found (funcall (or search-fun (isearch-search-fun-default))
+                                     string (if bound (- bound beg) (1+ (- end beg)))
+                                     noerror count))
+                (when found
+                  (setq found (+ found beg)
+                        match-data (mapcar (lambda (m) (1- (+ m beg))) (match-data)))))
+              (when match-data (set-match-data match-data)))
+          (setq found (funcall (or search-fun (isearch-search-fun-default))
+                               string (if bound (if isearch-forward
+                                                    (min bound end)
+                                                  (max bound end))
+                                        end)
+                               noerror count)))
         (unless found
           (setq beg (if isearch-forward
                         (next-single-property-change end property)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

close 14013 29.0.50
thanks

>>> This works surprisingly well.  Maybe there are more corner cases,
>>> but something already works with quick tests:
>>
>> Yes, not bad indeed.  Did you find any corner cases?
>
> Using a temporary buffer like you proposed works fine,
> and handles all possible regexps including corner cases.
> However, wouldn't this make the search too inefficient?

Now the implementation that supports all cases is pushed.
Thanks for all suggestions that helped to achieve this.
Any ideas for more improvements are welcome.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

> I generalized dired-isearch-search-filenames to a new function
> that can search in any text property, so e.g. after evaluating
>
>   (setq-local isearch-search-fun-function
>   (lambda () (isearch-search-fun-in-text-property 'face)))
>
> isearch will match .* in all fontified chunks, etc.

I'm sure later someone might want to add more args such as e.g.
text-property-search-forward has PROPERTY &optional VALUE PREDICATE ...
So better to change the signature in anticipation of adding more
args after PROPERTY:

  diff --git a/lisp/isearch.el b/lisp/isearch.el
  -(defun isearch-search-fun-in-text-property (property &optional search-fun)
  +(defun isearch-search-fun-in-text-property (search-fun property)

  diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
  @@ -3208,7 +3208,7 @@ dired-isearch-search-filenames
  -  (isearch-search-fun-in-text-property 'dired-filename (funcall orig-fun)))
  +  (isearch-search-fun-in-text-property (funcall orig-fun) 'dired-filename))

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

> BTW, do you think that such regexps as ".*" and "^.*$"
> should be supported not only in Dired buffers, but also
> when making replacements in rectangular regions?
> E.g. C-x SPC ... C-M-% .* RET RET should effectively
> clear the region, etc.

After a small refactoring, searching in rectangular regions
is implemented as well:


[-- Attachment #2: isearch-search-fun-in-noncontiguous-region.patch --]
[-- Type: text/x-diff, Size: 4712 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 7650ebcfce..2fd172e75c 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -4455,18 +4518,45 @@ minibuffer-lazy-highlight-setup
         (funcall after-change nil nil nil)))))
 
 \f
+(defun isearch-search-fun-in-noncontiguous-region (search-fun region-bounds)
+  "Return the function that searches inside noncontiguous regions."
+  (apply-partially
+   #'search-within-boundaries
+   search-fun
+   (lambda (pos)
+     (seq-some (lambda (b) (and (>= pos (car b)) (<= pos (cdr b))))
+               region-bounds))
+   (lambda (pos)
+     (let* ((bounds (flatten-list region-bounds))
+            found)
+       (unless isearch-forward
+         (setq bounds (nreverse bounds)))
+       (while (and bounds (not found))
+         (if (if isearch-forward (< pos (car bounds)) (> pos (car bounds)))
+             (setq found (car bounds))
+           (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.
 The function will limit the search for matches only inside text which has
 this property 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."
-  (lambda (string &optional bound noerror count)
+  (apply-partially
+   #'search-within-boundaries
+   search-fun
+   (lambda (pos) (get-text-property pos property))
+   (lambda (pos) (if isearch-forward
+                     (next-single-property-change pos property)
+                   (previous-single-property-change pos property)))))
+
+(defun search-within-boundaries ( search-fun get-fun next-fun
+                                  string &optional bound noerror count)
   (let* ((old (point))
          ;; Check if point is already on the property.
-           (beg (when (get-text-property
-                       (if isearch-forward old (max (1- old) (point-min)))
-                       property)
+         (beg (when (funcall get-fun (if isearch-forward old
+                                       (max (1- old) (point-min))))
                 old))
          end found (i 0)
          (subregexp
@@ -4480,30 +4570,23 @@ isearch-search-fun-in-text-property
                        (throw 'subregexp t))))))))
     ;; Otherwise, try to search for the next property.
     (unless beg
-        (setq beg (if isearch-forward
-                      (next-single-property-change old property)
-                    (previous-single-property-change old property)))
+      (setq beg (funcall next-fun old))
       (when beg (goto-char beg)))
     ;; Non-nil `beg' means there are more properties.
     (while (and beg (not found))
       ;; Search for the end of the current property.
-        (setq end (if isearch-forward
-                      (next-single-property-change beg property)
-                    (previous-single-property-change beg property)))
+      (setq end (funcall next-fun beg))
       ;; Handle ^/$ specially by matching in a temporary buffer.
       (if subregexp
           (let* ((prop-beg
                   (if (or (if isearch-forward (bobp) (eobp))
-                            (null (get-text-property
-                                   (+ (point) (if isearch-forward -1 0))
-                                   property)))
+                          (null (funcall get-fun (+ (point)
+                                                    (if isearch-forward -1 0)))))
                       ;; Already at the beginning of the field.
                       beg
                     ;; Get the real beginning of the field when
                     ;; the search was started in the middle.
-                      (if isearch-forward
-                          (previous-single-property-change beg property)
-                        (next-single-property-change beg property))))
+                    (funcall next-fun beg)))
                  (substring (buffer-substring prop-beg end))
                  (offset (if isearch-forward prop-beg end))
                  match-data)
@@ -4532,12 +4615,10 @@ isearch-search-fun-in-text-property
                      noerror count)))
       ;; Get the next text property.
       (unless found
-          (setq beg (if isearch-forward
-                        (next-single-property-change end property)
-                      (previous-single-property-change end property)))
+        (setq beg (funcall next-fun end))
         (when beg (goto-char beg))))
     (unless found (goto-char old))
-      found)))
+    found))
 
 \f
 (defun isearch-resume (string regexp word forward message case-fold)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

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

> BTW, do you think that such regexps as ".*" and "^.*$"
> should be supported not only in Dired buffers, but also
> when making replacements in rectangular regions?
> E.g. C-x SPC ... C-M-% .* RET RET should effectively
> clear the region, etc.

Here is a complete patch with tests:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: search-noncontiguous-region.patch --]
[-- Type: text/x-diff, Size: 24365 bytes --]

diff --git a/lisp/isearch.el b/lisp/isearch.el
index 34c3665bd8..14e5bfb93f 100644
--- a/lisp/isearch.el
+++ b/lisp/isearch.el
@@ -2805,12 +2806,10 @@ isearch-search-and-update
 		   (if (and (eq case-fold-search t) search-upper-case)
 		       (setq case-fold-search
 			     (isearch-no-upper-case-p isearch-string isearch-regexp)))
-		   (looking-at (cond
-				((functionp isearch-regexp-function)
-				 (funcall isearch-regexp-function isearch-string t))
-				(isearch-regexp-function (word-search-regexp isearch-string t))
-				(isearch-regexp isearch-string)
-				(t (regexp-quote isearch-string)))))
+		   ;; Like `looking-at' but uses search functions:
+		   (let ((isearch-forward t))
+		     (isearch-search-string
+		      (concat "\\=\\(?:" isearch-string "\\)") nil t)))
 	       (error nil))
 	     (or isearch-yank-flag
 		 (<= (match-end 0)
@@ -4381,6 +4401,7 @@ minibuffer-lazy-highlight-setup
           (cleanup lazy-highlight-cleanup)
           (transform #'identity)
           (filter nil)
+          (search-fun nil)
           (regexp isearch-regexp)
           (regexp-function isearch-regexp-function)
           (case-fold isearch-case-fold-search)
@@ -4400,6 +4421,7 @@ minibuffer-lazy-highlight-setup
 TRANSFORM: A function taking one argument, the minibuffer contents,
 and returning the `isearch-string' to use for lazy highlighting.
 FILTER: A function to add to `isearch-filter-predicate'.
+SEARCH-FUN: A function to add to `isearch-search-fun-function'.
 REGEXP: The value of `isearch-regexp' to use for lazy highlighting.
 REGEXP-FUNCTION: The value of `isearch-regexp-function' to use for
 lazy highlighting.
@@ -4419,6 +4441,9 @@ minibuffer-lazy-highlight-setup
               (when filter
                 (with-current-buffer buffer
                   (remove-function (local 'isearch-filter-predicate) filter)))
+              (when search-fun
+                (with-current-buffer buffer
+                  (remove-function (local 'isearch-search-fun-function) search-fun)))
               (remove-hook 'lazy-count-update-hook display-count)
               (when overlay (delete-overlay overlay))
               (remove-hook 'after-change-functions after-change t)
@@ -4454,92 +4479,123 @@ minibuffer-lazy-highlight-setup
         (when filter
           (with-current-buffer buffer
             (add-function :after-while (local 'isearch-filter-predicate) filter)))
+        (when search-fun
+          (with-current-buffer buffer
+            (add-function :around (local 'isearch-search-fun-function) search-fun)))
         (funcall after-change nil nil nil)))))
 
 \f
+(defun isearch-search-fun-in-noncontiguous-region (search-fun bounds)
+  "Return the function that searches inside noncontiguous regions.
+A noncontiguous regions is defined by the argument BOUNDS that
+is a list of cons cells of the form (START . END)."
+  (apply-partially
+   #'search-within-boundaries
+   search-fun
+   (lambda (pos)
+     (seq-some (lambda (b) (if isearch-forward
+                               (and (>= pos (car b)) (< pos (cdr b)))
+                             (and (> pos (car b)) (<= pos (cdr b)))))
+               bounds))
+   (lambda (pos)
+     (let ((bounds (flatten-list bounds))
+           found)
+       (unless isearch-forward
+         (setq bounds (nreverse bounds)))
+       (while (and bounds (not found))
+         (if (if isearch-forward (< pos (car bounds)) (> pos (car bounds)))
+             (setq found (car bounds))
+           (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.
 The function will limit the search for matches only inside text which has
 this property 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."
-  (lambda (string &optional bound noerror count)
-    (let* ((old (point))
-           ;; Check if point is already on the property.
-           (beg (when (get-text-property
-                       (if isearch-forward old (max (1- old) (point-min)))
-                       property)
-                  old))
-           end found (i 0)
-           (subregexp
-            (and isearch-regexp
-                 (save-match-data
-                   (catch 'subregexp
-                     (while (string-match "\\^\\|\\$" string i)
-                       (setq i (match-end 0))
-                       (when (subregexp-context-p string (match-beginning 0))
-                         ;; The ^/$ is not inside a char-range or escaped.
-                         (throw 'subregexp t))))))))
-      ;; Otherwise, try to search for the next property.
-      (unless beg
-        (setq beg (if isearch-forward
-                      (next-single-property-change old property)
-                    (previous-single-property-change old property)))
-        (when beg (goto-char beg)))
-      ;; Non-nil `beg' means there are more properties.
-      (while (and beg (not found))
-        ;; Search for the end of the current property.
-        (setq end (if isearch-forward
-                      (next-single-property-change beg property)
-                    (previous-single-property-change beg property)))
-        ;; Handle ^/$ specially by matching in a temporary buffer.
-        (if subregexp
-            (let* ((prop-beg
-                    (if (or (if isearch-forward (bobp) (eobp))
-                            (null (get-text-property
-                                   (+ (point) (if isearch-forward -1 0))
-                                   property)))
-                        ;; Already at the beginning of the field.
-                        beg
-                      ;; Get the real beginning of the field when
-                      ;; the search was started in the middle.
-                      (if isearch-forward
-                          (previous-single-property-change beg property)
-                        (next-single-property-change beg property))))
-                   (substring (buffer-substring prop-beg end))
-                   (offset (if isearch-forward prop-beg end))
-                   match-data)
-              (with-temp-buffer
-                (insert substring)
-                (goto-char (- beg offset -1))
-                ;; Apply ^/$ regexp on the whole extracted substring.
-                (setq found (funcall
-                             (or search-fun (isearch-search-fun-default))
-                             string (and bound (max (point-min)
-                                                    (min (point-max)
-                                                         (- bound offset -1))))
-                             noerror count))
-                ;; Adjust match data as if it's matched in original buffer.
-                (when found
-                  (setq found (+ found offset -1)
-                        match-data (mapcar (lambda (m) (+ m offset -1))
-                                           (match-data)))))
-              (when match-data (set-match-data match-data)))
-          (setq found (funcall
-                       (or search-fun (isearch-search-fun-default))
-                       string (if bound (if isearch-forward
-                                            (min bound end)
-                                          (max bound end))
-                                end)
-                       noerror count)))
-        ;; Get the next text property.
-        (unless found
-          (setq beg (if isearch-forward
-                        (next-single-property-change end property)
-                      (previous-single-property-change end property)))
-          (when beg (goto-char beg))))
-      (unless found (goto-char old))
-      found)))
+  (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)))))
+
+(defun search-within-boundaries ( search-fun get-fun next-fun
+                                  string &optional bound noerror count)
+  (let* ((old (point))
+         ;; Check if point is already on the property.
+         (beg (when (funcall get-fun old) old))
+         end found (i 0)
+         (subregexp
+          (and isearch-regexp
+               (save-match-data
+                 (catch 'subregexp
+                   (while (string-match "\\^\\|\\$" string i)
+                     (setq i (match-end 0))
+                     (when (subregexp-context-p string (match-beginning 0))
+                       ;; The ^/$ is not inside a char-range or escaped.
+                       (throw 'subregexp t))))))))
+    ;; Otherwise, try to search for the next property.
+    (unless beg
+      (setq beg (funcall next-fun old))
+      (when beg (goto-char beg)))
+    ;; Non-nil `beg' means there are more properties.
+    (while (and beg (not found))
+      ;; Search for the end of the current property.
+      (setq end (funcall next-fun beg))
+      ;; Handle ^/$ specially by matching in a temporary buffer.
+      (if subregexp
+          (let* ((prop-beg
+                  (if (or (if isearch-forward (bobp) (eobp))
+                          (null (funcall get-fun
+                                         (+ (point)
+                                            (if isearch-forward -1 1)))))
+                      ;; Already at the beginning of the field.
+                      beg
+                    ;; Get the real beginning of the field when
+                    ;; the search was started in the middle.
+                    (let ((isearch-forward (not isearch-forward)))
+                      ;; Search in the reverse direction.
+                      (funcall next-fun beg))))
+                 (substring (buffer-substring prop-beg end))
+                 (offset (if isearch-forward prop-beg end))
+                 match-data)
+            (with-temp-buffer
+              (insert substring)
+              (goto-char (- beg offset -1))
+              ;; Apply ^/$ regexp on the whole extracted substring.
+              (setq found (funcall
+                           (or search-fun (isearch-search-fun-default))
+                           string (and bound (max (point-min)
+                                                  (min (point-max)
+                                                       (- bound offset -1))))
+                           noerror count))
+              ;; Adjust match data as if it's matched in original buffer.
+              (when found
+                (setq found (+ found offset -1)
+                      match-data (mapcar (lambda (m) (+ m offset -1))
+                                         (match-data)))))
+            (when found (goto-char found))
+            (when match-data (set-match-data
+                              (mapcar (lambda (m) (copy-marker m))
+                                      match-data))))
+        (setq found (funcall
+                     (or search-fun (isearch-search-fun-default))
+                     string (if bound (if isearch-forward
+                                          (min bound end)
+                                        (max bound end))
+                              end)
+                     noerror count)))
+      ;; Get the next text property.
+      (unless found
+        (setq beg (funcall next-fun end))
+        (when beg (goto-char beg))))
+    (unless found (goto-char old))
+    found))
 
 \f
 (defun isearch-resume (string regexp word forward message case-fold)
diff --git a/lisp/replace.el b/lisp/replace.el
index 34c3d5299e..ca2edbf8ea 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -371,9 +371,6 @@ query-replace-read-args
            (from (minibuffer-with-setup-hook
                      (minibuffer-lazy-highlight-setup
                       :case-fold case-fold-search
-                      :filter (when (use-region-p)
-                                (replace--region-filter
-                                 (funcall region-extract-function 'bounds)))
                       :highlight query-replace-lazy-highlight
                       :regexp regexp-flag
                       :regexp-function (or replace-regexp-function
@@ -381,6 +378,15 @@ query-replace-read-args
                                            (and replace-char-fold
 	                                        (not regexp-flag)
 	                                        #'char-fold-to-regexp))
+                      :search-fun (when (use-region-p)
+                                    (let ((bounds
+                                           (mapcar (lambda (p)
+                                                     (cons (copy-marker (car p))
+                                                           (copy-marker (cdr p))))
+                                                   (funcall region-extract-function 'bounds))))
+                                      (lambda (orig-fun)
+                                        (isearch-search-fun-in-noncontiguous-region
+                                         (funcall orig-fun) bounds))))
                       :transform (lambda (string)
                                    (let* ((split (query-replace--split-string string))
                                           (from-string (if (consp split) (car split) split)))
@@ -2850,26 +2856,6 @@ replace--push-stack
 	       ,search-str ,next-replace)
          ,stack))
 
-(defun replace--region-filter (bounds)
-  "Return a function that decides if a region is inside BOUNDS.
-BOUNDS is a list of cons cells of the form (START . END).  The
-returned function takes as argument two buffer positions, START
-and END."
-  (let ((region-bounds
-         (mapcar (lambda (position)
-                   (cons (copy-marker (car position))
-                         (copy-marker (cdr position))))
-                 bounds)))
-    (lambda (start end)
-      (delq nil (mapcar
-                 (lambda (bounds)
-                   (and
-                    (>= start (car bounds))
-                    (<= start (cdr bounds))
-                    (>= end   (car bounds))
-                    (<= end   (cdr bounds))))
-                 region-bounds)))))
-
 (defun perform-replace (from-string replacements
 		        query-flag regexp-flag delimited-flag
 			&optional repeat-count map start end backward region-noncontiguous-p)
@@ -2930,7 +2916,28 @@ perform-replace
 
          ;; If non-nil, it is marker saying where in the buffer to stop.
          (limit nil)
-         (region-filter nil)
+
+         ;; Unless a single contiguous chunk is selected, operate on multiple chunks.
+         (noncontiguous-region-bounds
+          (when region-noncontiguous-p
+            (mapcar (lambda (p)
+                      (cons (copy-marker (car p))
+                            (copy-marker (cdr p))))
+                    (funcall region-extract-function 'bounds))))
+         (noncontiguous-search-fun
+          (when noncontiguous-region-bounds
+            (isearch-search-fun-in-noncontiguous-region
+             nil noncontiguous-region-bounds)))
+         (replace-search-function
+          (or replace-search-function noncontiguous-search-fun))
+         (replace-re-search-function
+          ;; To be able to use "^.*$" on rectangles
+          (or replace-re-search-function noncontiguous-search-fun))
+         (replace-search-fun-function
+          (when noncontiguous-region-bounds
+            (lambda (orig-fun)
+              (isearch-search-fun-in-noncontiguous-region
+               (funcall orig-fun) noncontiguous-region-bounds))))
 
          ;; Data for the next match.  If a cons, it has the same format as
          ;; (match-data); otherwise it is t if a match is possible at point.
@@ -2952,11 +2959,10 @@ perform-replace
                               "(\\<query-replace-map>\\[help] for help) "))
                      minibuffer-prompt-properties))))
 
-    ;; Unless a single contiguous chunk is selected, operate on multiple chunks.
-    (when region-noncontiguous-p
-      (setq region-filter (replace--region-filter
-                           (funcall region-extract-function 'bounds)))
-      (add-function :after-while isearch-filter-predicate region-filter))
+    ;; For lazy-highlight during replacement
+    (when replace-search-fun-function
+      (add-function :around isearch-search-fun-function
+                    replace-search-fun-function))
 
     ;; If region is active, in Transient Mark mode, operate on region.
     (if backward
@@ -3052,9 +3058,11 @@ perform-replace
 	  (setq match-again
 		(and nonempty-match
 		     (or (not regexp-flag)
-			 (and (if backward
-				  (looking-back search-string nil)
-				(looking-at search-string))
+			 ;; Like `looking-at' but uses search functions:
+			 (and (replace-search
+			       (concat "\\=\\(?:" search-string "\\)")
+			       limit regexp-flag delimited-flag
+			       case-fold-search backward)
 			      (let ((match (match-data)))
 				(and (/= (nth 0 match) (nth 1 match))
 				     match))))))
@@ -3323,7 +3331,8 @@ perform-replace
 			       ;; when perform-replace was started from
 			       ;; `xref--query-replace-1' that let-binds
 			       ;; `isearch-filter-predicate' (bug#53758).
-			       (isearch-filter-predicate #'isearch-filter-visible))
+			       (isearch-filter-predicate #'isearch-filter-visible)
+			       (isearch-search-fun-function #'isearch-search-fun-default))
 			   (setq real-match-data (replace-match-data
 						  nil real-match-data
 						  real-match-data))
@@ -3337,8 +3346,13 @@ perform-replace
 			 ;; decide whether the search string
 			 ;; can match again just after this match.
 			 (if (and regexp-flag nonempty-match)
-			     (setq match-again (and (looking-at search-string)
-						    (match-data)))))
+			     (setq match-again
+				   (and (save-window-excursion
+					  (replace-search
+					   (concat "\\=\\(?:" search-string "\\)")
+					   limit regexp-flag delimited-flag
+					   case-fold-search backward))
+					(match-data)))))
 			;; Edit replacement.
 			((or (eq def 'edit-replacement)
                              (eq def 'edit-replacement-exact-case))
@@ -3406,8 +3420,9 @@ perform-replace
                       search-string-replaced    nil
                       last-was-act-and-show     nil))))))
       (replace-dehighlight)
-      (when region-filter
-        (remove-function isearch-filter-predicate region-filter)))
+      (when replace-search-fun-function
+        (remove-function isearch-search-fun-function
+                         replace-search-fun-function)))
     (or unread-command-events
 	(message (ngettext "Replaced %d occurrence%s"
 			   "Replaced %d occurrences%s"
diff --git a/test/lisp/isearch-tests.el b/test/lisp/isearch-tests.el
index 4600757d94..82f9fb0fe9 100644
--- a/test/lisp/isearch-tests.el
+++ b/test/lisp/isearch-tests.el
@@ -38,5 +38,85 @@ isearch--test-done
   ;; Bug #21091: let `isearch-done' work without `isearch-update'.
   (isearch-done))
 
+\f
+;; Search functions.
+
+(defun isearch--test-search-within-boundaries (pairs)
+  (goto-char (point-min))
+  (let ((isearch-forward t)
+        (isearch-regexp nil))
+    (dolist (pos (append pairs nil))
+      (should (eq (cdr pos) (isearch-search-string "foo" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (car pos) (should (eq (car pos) (match-beginning 0))))))
+
+  (goto-char (point-max))
+  (let ((isearch-forward nil)
+        (isearch-regexp nil))
+    (dolist (pos (append (reverse pairs) nil))
+      (should (eq (car pos) (isearch-search-string "foo" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (cdr pos) (should (eq (cdr pos) (match-end 0))))))
+
+  (goto-char (point-min))
+  (let ((isearch-forward t)
+        (isearch-regexp t))
+    (dolist (pos (append pairs nil))
+      (should (eq (cdr pos) (isearch-search-string ".*" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (car pos) (should (eq (car pos) (match-beginning 0))))))
+
+  (goto-char (point-min))
+  (let ((isearch-forward t)
+        (isearch-regexp t))
+    (dolist (pos (append pairs nil))
+      (should (eq (cdr pos) (isearch-search-string "^.*" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (car pos) (should (eq (car pos) (match-beginning 0))))))
+
+  (goto-char (point-min))
+  (let ((isearch-forward t)
+        (isearch-regexp t))
+    (dolist (pos (append pairs nil))
+      (should (eq (cdr pos) (isearch-search-string ".*$" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (car pos) (should (eq (car pos) (match-beginning 0))))))
+
+  (goto-char (point-max))
+  (let ((isearch-forward nil)
+        (isearch-regexp t))
+    (dolist (pos (append (reverse pairs) nil))
+      (should (eq (car pos) (isearch-search-string "^.*" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (cdr pos) (should (eq (cdr pos) (match-end 0))))))
+
+  (goto-char (point-max))
+  (let ((isearch-forward nil)
+        (isearch-regexp t))
+    (dolist (pos (append (reverse pairs) nil))
+      (should (eq (car pos) (isearch-search-string "foo$" nil t)))
+      (should (equal (match-string 0) "foo"))
+      (when (cdr pos) (should (eq (cdr pos) (match-end 0)))))))
+
+(ert-deftest isearch--test-search-fun-in-text-property ()
+  (let* ((pairs '((4 . 7) (11 . 14) (21 . 24)))
+         (isearch-search-fun-function
+          (lambda () (isearch-search-fun-in-text-property nil 'dired-filename))))
+    (with-temp-buffer
+      (insert "foo" (propertize "foo" 'dired-filename t) "foo\n")
+      (insert (propertize "foo" 'dired-filename t) "foo\n")
+      (insert "foo" (propertize "foo" 'dired-filename t) "\n")
+      (isearch--test-search-within-boundaries pairs))))
+
+(ert-deftest isearch--test-search-fun-in-noncontiguous-region ()
+  (let* ((pairs '((4 . 7) (11 . 14) (21 . 24)))
+         (isearch-search-fun-function
+          (lambda () (isearch-search-fun-in-noncontiguous-region nil pairs))))
+    (with-temp-buffer
+      (insert "foofoofoo\n")
+      (insert "foofoo\n")
+      (insert "foofoo\n")
+      (isearch--test-search-within-boundaries pairs))))
+
 (provide 'isearch-tests)
 ;;; isearch-tests.el ends here
diff --git a/test/lisp/replace-tests.el b/test/lisp/replace-tests.el
index ef1e5c3eaf..e59883b5b6 100644
--- a/test/lisp/replace-tests.el
+++ b/test/lisp/replace-tests.el
@@ -460,20 +460,12 @@ query-replace-search-function-tests
   (let* ((replace-re-search-function #'re-search-forward))
     (query-replace--run-tests query-replace-tests))
 
-  (let* ((pairs '((1 . 2) (3 . 4)))
+  (let* ((bounds '((1 . 2) (3 . 4)))
          (replace-re-search-function
-          (lambda (string &optional _bound noerror count)
-            (let (found)
-              (while (and (not found) pairs)
-                (goto-char (caar pairs))
-                (when (re-search-forward string (cdar pairs) noerror count)
-                  (setq found t))
-                (pop pairs))
-              found)))
+          (isearch-search-fun-in-noncontiguous-region nil bounds))
          (tests
           '(
-            ;; FIXME: this test should pass after fixing bug#54733:
-            ;; ("aaaa" "C-M-% .* RET 1 RET !" "1a1a")
+            ("aaaa" "C-M-% .* RET 1 RET !" "1a1a")
             )))
     (query-replace--run-tests tests)))
 
@@ -485,8 +477,9 @@ perform-replace-tests
     ;; Test case from commit 5632eb272c7
     ("a a a " "\\ba " "c" nil t nil nil nil nil nil nil nil "ccc") ; not "ca c"
     ;; The same with region inside the second match
-    ;; FIXME: this test should pass after fixing bug#54733:
-    ;; ("a a a " "\\ba " "c" nil t nil nil nil 1 4 nil nil "ca a ")
+    ("a a a " "\\ba " "c" nil t nil nil nil 1 3 nil nil "ca a ")
+    ("a a a " "\\ba " "c" nil t nil nil nil 1 4 nil nil "ca a ")
+    ("a a a " "\\ba " "c" nil t nil nil nil 1 5 nil nil "cca ")
     ))
 
 (defun perform-replace--run-tests (tests)

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> -		   (looking-at (cond
> -				((functionp isearch-regexp-function)
> -				 (funcall isearch-regexp-function isearch-string t))
> -				(isearch-regexp-function (word-search-regexp isearch-string t))
> -				(isearch-regexp isearch-string)
> -				(t (regexp-quote isearch-string)))))
> +		   ;; Like `looking-at' but uses search functions:
> +		   (let ((isearch-forward t))
> +		     (isearch-search-string
> +		      (concat "\\=\\(?:" isearch-string "\\)") nil t)))
>  	       (error nil))

These lines seem to break backwards char-fold isearch for me.  Test
case:

Insert "creme" in an empty buffer (yes, all just ascii latin chars), go
to eob, and C-r M-s ' creme ==> fails.

Works with forward search and non-char-folding backward search.

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> -		   (looking-at (cond
>> -				((functionp isearch-regexp-function)
>> -				 (funcall isearch-regexp-function isearch-string t))
>> -				(isearch-regexp-function (word-search-regexp isearch-string t))
>> -				(isearch-regexp isearch-string)
>> -				(t (regexp-quote isearch-string)))))
>> +		   ;; Like `looking-at' but uses search functions:
>> +		   (let ((isearch-forward t))
>> +		     (isearch-search-string
>> +		      (concat "\\=\\(?:" isearch-string "\\)") nil t)))
>>  	       (error nil))
>
> These lines seem to break backwards char-fold isearch for me.  Test
> case:
>
> Insert "creme" in an empty buffer (yes, all just ascii latin chars), go
> to eob, and C-r M-s ' creme ==> fails.
>
> Works with forward search and non-char-folding backward search.

Thanks for testing.  So this is a new reason why this hunk should be left out.
Another reason is that some search functions are unsuitable to be called
instead of looking-at, such as when the search function in the minibuffer
starts to visit history items here.

I hope that ‘(replace-search (concat "\\=\\(?:"’ in perform-replace
has no such problems.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Hello again,

only two more small things:

> +(defun isearch-search-fun-in-noncontiguous-region (search-fun bounds)
> +  "Return the function that searches inside noncontiguous regions.
> +A noncontiguous regions is defined by the argument BOUNDS that
                         ^
Typo.

> +(defun search-within-boundaries ( search-fun get-fun next-fun
> +                                  string &optional bound noerror count)
                                    ^
Is this space intended (looks a bit strange)?

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> +(defun search-within-boundaries ( search-fun get-fun next-fun
>> +                                  string &optional bound noerror count)
>                                     ^
> Is this space intended (looks a bit strange)?

Yes, it tells emacs-lisp-mode that this list should be indented like you
see instead of aligning `string` under `get-fun`.


        Stefan

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> +(defun isearch-search-fun-in-noncontiguous-region (search-fun bounds)
>> +  "Return the function that searches inside noncontiguous regions.
>> +A noncontiguous regions is defined by the argument BOUNDS that
>                          ^
> Typo.

Thanks for noticing, now pushed the fixed version.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> -  (remove-function (local 'isearch-filter-predicate)
> -                   #'wdired-isearch-filter-read-only)
> +  (when wdired-search-replace-filenames
> +    (remove-function (local 'isearch-search-fun-function)
> +                     #'dired-isearch-search-filenames)
> +    (kill-local-variable 'replace-search-function)
> +    (kill-local-variable 'replace-re-search-function))

Juri, when a user disables `wdired-search-replace-filenames' while still
in wdired-mode, won't we fail to undo these settings when the user
then returns to normal dired? - should we not better undo these things
unconditionally?

Second question: could we advice (local 'replace-search-function) and
(local 'replace-re-search-function) instead of replacing the value (it
might be nicer to users of other packages)?

TIA,

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> -  (remove-function (local 'isearch-filter-predicate)
>> -                   #'wdired-isearch-filter-read-only)
>> +  (when wdired-search-replace-filenames
>> +    (remove-function (local 'isearch-search-fun-function)
>> +                     #'dired-isearch-search-filenames)
>> +    (kill-local-variable 'replace-search-function)
>> +    (kill-local-variable 'replace-re-search-function))
>
> Juri, when a user disables `wdired-search-replace-filenames' while still
> in wdired-mode, won't we fail to undo these settings when the user
> then returns to normal dired? - should we not better undo these things
> unconditionally?

If these calls are idempotent, we could remove the condition.
Could you please confirm there is no adverse effect after removing this.

Also there is another call at the end that can't be removed:

  (add-hook 'isearch-mode-hook #'dired-isearch-filenames-setup nil t)

> Second question: could we advice (local 'replace-search-function) and
> (local 'replace-re-search-function) instead of replacing the value (it
> might be nicer to users of other packages)?

This looks nicer in theory.  But in practice I expect to see a lot of
conflicts.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> >> -  (remove-function (local 'isearch-filter-predicate)
> >> -                   #'wdired-isearch-filter-read-only)
> >> +  (when wdired-search-replace-filenames
> >> +    (remove-function (local 'isearch-search-fun-function)
> >> +                     #'dired-isearch-search-filenames)
> >> +    (kill-local-variable 'replace-search-function)
> >> +    (kill-local-variable 'replace-re-search-function))
> >
> > Juri, when a user disables `wdired-search-replace-filenames' while still
> > in wdired-mode, won't we fail to undo these settings when the user
> > then returns to normal dired? - should we not better undo these things
> > unconditionally?
>
> If these calls are idempotent, we could remove the condition.
> Could you please confirm there is no adverse effect after removing this.

Both `remove-function' and `k-l-variable' are documented to do nothing
when the function to be removed is not present/ there is no local
variable binding.

> Also there is another call at the end that can't be removed:
>
>   (add-hook 'isearch-mode-hook #'dired-isearch-filenames-setup nil t)

I think we should remove the condition when entering wdired here (for
the same reason).

> > Second question: could we advice (local 'replace-search-function) and
> > (local 'replace-re-search-function) instead of replacing the value (it
> > might be nicer to users of other packages)?
>
> This looks nicer in theory.  But in practice I expect to see a lot of
> conflicts.

What conflicts?

Michael.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Juri Linkov]

>> >> -  (remove-function (local 'isearch-filter-predicate)
>> >> -                   #'wdired-isearch-filter-read-only)
>> >> +  (when wdired-search-replace-filenames
>> >> +    (remove-function (local 'isearch-search-fun-function)
>> >> +                     #'dired-isearch-search-filenames)
>> >> +    (kill-local-variable 'replace-search-function)
>> >> +    (kill-local-variable 'replace-re-search-function))
>> >
>> > Juri, when a user disables `wdired-search-replace-filenames' while still
>> > in wdired-mode, won't we fail to undo these settings when the user
>> > then returns to normal dired? - should we not better undo these things
>> > unconditionally?
>>
>> If these calls are idempotent, we could remove the condition.
>> Could you please confirm there is no adverse effect after removing this.
>
> Both `remove-function' and `k-l-variable' are documented to do nothing
> when the function to be removed is not present/ there is no local
> variable binding.
>
>> Also there is another call at the end that can't be removed:
>>
>>   (add-hook 'isearch-mode-hook #'dired-isearch-filenames-setup nil t)
>
> I think we should remove the condition when entering wdired here (for
> the same reason).

If you don't see a problem, then let's change this in master.

>> > Second question: could we advice (local 'replace-search-function) and
>> > (local 'replace-re-search-function) instead of replacing the value (it
>> > might be nicer to users of other packages)?
>>
>> This looks nicer in theory.  But in practice I expect to see a lot of
>> conflicts.
>
> What conflicts?

Usual conflicts when two pieces of code are working simultaneously
on the same thing.  So we need concrete examples to verify if
everything is correct.

bug#14013: 24.3.50; dired-isearch-filenames-regexp is matching text outside filenames

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> >> > Second question: could we advice (local 'replace-search-function) and
> >> > (local 'replace-re-search-function) instead of replacing the value (it
> >> > might be nicer to users of other packages)?
> >>
> >> This looks nicer in theory.  But in practice I expect to see a lot of
> >> conflicts.
> >
> > What conflicts?
>
> Usual conflicts when two pieces of code are working simultaneously
> on the same thing.  So we need concrete examples to verify if
> everything is correct.

Not sure I understand.  We would do this to get any control over
conflicts at all (by using the different types of advices and the depth
property).

How would such examples behave currently, and why do you think changing
the code to use advices would be more problematic?  Currently we `setq'
and so the last one completely overwrites everything conflicting.  Isn't
that a problem per se (at least it's the thing I want to mitigate).


Michael.