bug#40940: 27.0.91; project-query-replace-regexp stops too early

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Simen Heggestøyl]

Create a directory with two files in it: test-2.txt and test.txt:

test-dir
├── test-2.txt
└── test.txt

Enter the text "Foo" into test.txt.
Enter the text "foo" into test-2.txt.

Visit the directory or any of the files (the result is the same either
way).

Type 'M-x project-query-replace-regexp', choosing to replace "Foo"
with "Bar".

Confirm test-dir as the project directory in the next prompt.

The replacement process stops with point before "foo" in test-2.txt
with a message "Replaced 0 occurrences", without replacing "Foo" in
test.txt as it should have.

The bug appears both in emacs-27 as of
16fed05ba85c3d92d3c913657dd50a648ad3884a, and on master as of
771a6b68165b986b6bf9249c57ca11d310b0f0e4.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 28.04.2020 17:35, Simen Heggestøyl wrote:
> 
> Create a directory with two files in it: test-2.txt and test.txt:
> 
> test-dir
> ├── test-2.txt
> └── test.txt
> 
> Enter the text "Foo" into test.txt.
> Enter the text "foo" into test-2.txt.
> 
> Visit the directory or any of the files (the result is the same either
> way).
> 
> Type 'M-x project-query-replace-regexp', choosing to replace "Foo"
> with "Bar".
> 
> Confirm test-dir as the project directory in the next prompt.
> 
> The replacement process stops with point before "foo" in test-2.txt
> with a message "Replaced 0 occurrences", without replacing "Foo" in
> test.txt as it should have.
> 
> The bug appears both in emacs-27 as of
> 16fed05ba85c3d92d3c913657dd50a648ad3884a, and on master as of
> 771a6b68165b986b6bf9249c57ca11d310b0f0e4.

I can confirm this. Tested with the current emacs-27.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 29 Apr 2020 06:54:22 +0300
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>
> 
> > The replacement process stops with point before "foo" in test-2.txt
> > with a message "Replaced 0 occurrences", without replacing "Foo" in
> > test.txt as it should have.
> > 
> > The bug appears both in emacs-27 as of
> > 16fed05ba85c3d92d3c913657dd50a648ad3884a, and on master as of
> > 771a6b68165b986b6bf9249c57ca11d310b0f0e4.
> 
> I can confirm this. Tested with the current emacs-27.

Do we need to fix this in emacs-27?  Is this a regression since Emacs 26?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Simen Heggestøyl]

Eli Zaretskii <eliz@gnu.org> writes:

> Is this a regression since Emacs 26?

project-query-replace-regexp is new in Emacs 27.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> From: Simen Heggestøyl <simenheg@runbox.com>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  40940@debbugs.gnu.org,
>   monnier@IRO.UMontreal.CA
> Date: Wed, 29 Apr 2020 10:29:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is this a regression since Emacs 26?
> 
> project-query-replace-regexp is new in Emacs 27.

OK, so we should fix this on the release branch.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Date: Wed, 29 Apr 2020 11:37:53 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org, dgutov@yandex.ru
> 
> > From: Simen Heggestøyl <simenheg@runbox.com>
> > Cc: Dmitry Gutov <dgutov@yandex.ru>,  40940@debbugs.gnu.org,
> >   monnier@IRO.UMontreal.CA
> > Date: Wed, 29 Apr 2020 10:29:18 +0200
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Is this a regression since Emacs 26?
> > 
> > project-query-replace-regexp is new in Emacs 27.
> 
> OK, so we should fix this on the release branch.

First, this is broken if the shell doesn't expand ~/ or if the Emacs
notion of the home directory is different from that of the shell.
Here's the proposed patch (ignoring the whitespace changes); OK to
push to the emacs-27 branch?

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 1f4cbe9..dbc967b 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -185,13 +185,17 @@ project--files-in-directory
   (require 'find-dired)
   (require 'xref)
   (defvar find-name-arg)
-  (let ((default-directory dir)
+  (let* ((default-directory dir)
+         (dirname (file-remote-p dir 'localname))
+         (dirname (or dirname
+                      ;; Make sure ~/ etc. in local directory name is
+                      ;; expanded and not left for the shell command
+                      ;; to interpret.
+                      (expand-file-name dir)))
          (command (format "%s %s %s -type f %s -print0"
                           find-program
-                         (file-local-name dir)
-                         (xref--find-ignores-arguments
-                          ignores
-                          (expand-file-name dir))
+                          dirname
+                          (xref--find-ignores-arguments ignores dirname)
                           (if files
                               (concat (shell-quote-argument "(")
                                       " " find-name-arg " "

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Date: Wed, 29 Apr 2020 12:24:36 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> 
> First, this is broken if the shell doesn't expand ~/ or if the Emacs
> notion of the home directory is different from that of the shell.

The patch below seems to fix the rest.  Note that I've decided to make
the change in fileloop.el, since I think all the other callers need
the same fix.  Also, using downcase is not entirely correct, we should
use isearch-no-upper-case-p instead.

Comments?

diff --git a/lisp/fileloop.el b/lisp/fileloop.el
index 543963f..f7a199e 100644
--- a/lisp/fileloop.el
+++ b/lisp/fileloop.el
@@ -204,14 +204,24 @@ fileloop-initialize-replace
    files
    (lambda ()
      (let ((case-fold-search
-            (if (memql case-fold '(nil t)) case-fold case-fold-search)))
+            (if (memql case-fold '(nil t))
+                case-fold
+              (if (equal from (downcase from))
+                  case-fold-search
+                nil))))
        (if (re-search-forward from nil t)
 	   ;; When we find a match, move back
 	   ;; to the beginning of it so perform-replace
 	   ;; will see it.
 	   (goto-char (match-beginning 0)))))
    (lambda ()
-     (perform-replace from to t t delimited nil multi-query-replace-map))))
+     (let ((case-fold-search
+            (if (memql case-fold '(nil t))
+                case-fold
+              (if (equal from (downcase from))
+                  case-fold-search
+                nil))))
+       (perform-replace from to t t delimited nil multi-query-replace-map)))))
 
 (provide 'fileloop)
 ;;; fileloop.el ends here

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 12:24, Eli Zaretskii wrote:
> First, this is broken if the shell doesn't expand ~/ or if the Emacs
> notion of the home directory is different from that of the shell.

The patch seems okay, but could you describe a case when the shell fails 
to expand '~/'? Is that about cmd.exe? Cygwin?

AFAICT the patch doesn't fix the same problem for remote hosts, is that 
a problem?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 29 Apr 2020 17:50:53 +0300
> 
> On 29.04.2020 12:24, Eli Zaretskii wrote:
> > First, this is broken if the shell doesn't expand ~/ or if the Emacs
> > notion of the home directory is different from that of the shell.
> 
> The patch seems okay, but could you describe a case when the shell fails 
> to expand '~/'? Is that about cmd.exe? Cygwin?

I've bumped into it with cmd.exe on MS-Windows, yes.

> AFAICT the patch doesn't fix the same problem for remote hosts, is that 
> a problem?

No, I don't think so.  A remote host cannot be a non-Posix system,
AFAIK.  Michael, can you confirm?  (And in any case, this issue should
be handled in the file-remote-p handler.)

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 17:59, Eli Zaretskii wrote:
>> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Wed, 29 Apr 2020 17:50:53 +0300
>>
>> On 29.04.2020 12:24, Eli Zaretskii wrote:
>>> First, this is broken if the shell doesn't expand ~/ or if the Emacs
>>> notion of the home directory is different from that of the shell.
>>
>> The patch seems okay, but could you describe a case when the shell fails
>> to expand '~/'? Is that about cmd.exe? Cygwin?
> 
> I've bumped into it with cmd.exe on MS-Windows, yes.

I see. Then LGTM, thank you.

Could you clarify, though, does that mean that no project commands are 
currently working on Windows, without this patch? Or does that only 
affect the "transient" project type?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> AFAICT the patch doesn't fix the same problem for remote hosts, is that
>> a problem?
>
> No, I don't think so.  A remote host cannot be a non-Posix system,
> AFAIK.

A remote host can be (kind of) non-Posix, for example not expanding
"~/". But this is a general problem then accessing such a host, and one
shall avoid to use a remote file name containing the tilde.

> Michael, can you confirm?  (And in any case, this issue should
> be handled in the file-remote-p handler.)

Tramp checks for all remote hosts whether they can expand "~/" (or
"~user/" in general). If possible, it does.

Your patch uses `expand-file-name', and that's all what we have to say
to Tramp.

Best regards, Michael.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 18:49, Michael Albinus wrote:
> Your patch uses `expand-file-name', and that's all what we have to say
> to Tramp.

I think it uses it only for non-Tramp dirs, doesn't it?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 29 Apr 2020 18:42:41 +0300
> 
> >> The patch seems okay, but could you describe a case when the shell fails
> >> to expand '~/'? Is that about cmd.exe? Cygwin?
> > 
> > I've bumped into it with cmd.exe on MS-Windows, yes.
> 
> I see. Then LGTM, thank you.

Thanks, pushed to the emacs-27 branch.

> Could you clarify, though, does that mean that no project commands are 
> currently working on Windows, without this patch? Or does that only 
> affect the "transient" project type?

Only the "transient" projects, AFAICT.  At least I tried with a Git
repository under ~/, and got absolute file names in the list.  I hope
Mercurial does the same.  You can test it easily by running

   (project-files (project-current t))

in a suitable set up directory.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  simenheg@runbox.com,
>   monnier@IRO.UMontreal.CA,  40940@debbugs.gnu.org
> Date: Wed, 29 Apr 2020 17:49:46 +0200
> 
> > Michael, can you confirm?  (And in any case, this issue should
> > be handled in the file-remote-p handler.)
> 
> Tramp checks for all remote hosts whether they can expand "~/" (or
> "~user/" in general). If possible, it does.
> 
> Your patch uses `expand-file-name', and that's all what we have to say
> to Tramp.

My patch only calls expand-file-name on local file names.  For remote
file names, I've left the result of the call to file-remote-p
unaltered.  Would that be a problem if the file name, after stripping
the remote protocol parts, started with "~/" ?  And if so, what should
be done here about these cases?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 19:04, Eli Zaretskii wrote:
>> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Wed, 29 Apr 2020 18:42:41 +0300
>>
>>>> The patch seems okay, but could you describe a case when the shell fails
>>>> to expand '~/'? Is that about cmd.exe? Cygwin?
>>>
>>> I've bumped into it with cmd.exe on MS-Windows, yes.
>>
>> I see. Then LGTM, thank you.
> 
> Thanks, pushed to the emacs-27 branch.

Thanks.

>> Could you clarify, though, does that mean that no project commands are
>> currently working on Windows, without this patch? Or does that only
>> affect the "transient" project type?
> 
> Only the "transient" projects, AFAICT.  At least I tried with a Git
> repository under ~/, and got absolute file names in the list.  I hope
> Mercurial does the same.  You can test it easily by running
> 
>     (project-files (project-current t))
> 
> in a suitable set up directory.

It does. Very good.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Tramp checks for all remote hosts whether they can expand "~/" (or
>> "~user/" in general). If possible, it does.
>>
>> Your patch uses `expand-file-name', and that's all what we have to say
>> to Tramp.
>
> My patch only calls expand-file-name on local file names.  For remote
> file names, I've left the result of the call to file-remote-p
> unaltered.  Would that be a problem if the file name, after stripping
> the remote protocol parts, started with "~/" ?  And if so, what should
> be done here about these cases?

Call `expand-file-name' on the remote file name, like
'(expand-file-name "/ssh:user@host:~/")'. Tramp expands it to
"/ssh:user@host:/home/user/", in case the "/home/user" is the home
directory of "user" on "host".

Best regards, Michael.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: dgutov@yandex.ru,  simenheg@runbox.com,  monnier@IRO.UMontreal.CA,
>   40940@debbugs.gnu.org
> Date: Wed, 29 Apr 2020 18:22:06 +0200
> 
> > My patch only calls expand-file-name on local file names.  For remote
> > file names, I've left the result of the call to file-remote-p
> > unaltered.  Would that be a problem if the file name, after stripping
> > the remote protocol parts, started with "~/" ?  And if so, what should
> > be done here about these cases?
> 
> Call `expand-file-name' on the remote file name, like
> '(expand-file-name "/ssh:user@host:~/")'. Tramp expands it to
> "/ssh:user@host:/home/user/", in case the "/home/user" is the home
> directory of "user" on "host".

Thanks.

Dmitry, this means my change should be reworked to call
expand-file-name before file-local-name, right?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 19:44, Eli Zaretskii wrote:
> Dmitry, this means my change should be reworked to call
> expand-file-name before file-local-name, right?

Seems so.

Something like this?

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index dbc967b885..f80b4328bc 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -185,17 +185,16 @@ project--files-in-directory
    (require 'find-dired)
    (require 'xref)
    (defvar find-name-arg)
-  (let* ((default-directory dir)
-         (dirname (file-remote-p dir 'localname))
-         (dirname (or dirname
-                      ;; Make sure ~/ etc. in local directory name is
-                      ;; expanded and not left for the shell command
-                      ;; to interpret.
-                      (expand-file-name dir)))
+  (let* ((dir
+          ;; Make sure ~/ etc. in local directory name is
+          ;; expanded and not left for the shell command
+          ;; to interpret.
+          (expand-file-name dir))
+         (default-directory dir)
           (command (format "%s %s %s -type f %s -print0"
                            find-program
-                          dirname
-                          (xref--find-ignores-arguments ignores dirname)
+                          dir
+                          (xref--find-ignores-arguments ignores dir)
                            (if files
                                (concat (shell-quote-argument "(")
                                        " " find-name-arg " "

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 29.04.2020 19:44, Eli Zaretskii wrote:
>> Dmitry, this means my change should be reworked to call
>> expand-file-name before file-local-name, right?
>
> Seems so.
>
> Something like this?
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index dbc967b885..f80b4328bc 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -185,17 +185,16 @@ project--files-in-directory
>    (require 'find-dired)
>    (require 'xref)
>    (defvar find-name-arg)
> -  (let* ((default-directory dir)
> -         (dirname (file-remote-p dir 'localname))
> -         (dirname (or dirname
> -                      ;; Make sure ~/ etc. in local directory name is
> -                      ;; expanded and not left for the shell command
> -                      ;; to interpret.
> -                      (expand-file-name dir)))
> +  (let* ((dir
> +          ;; Make sure ~/ etc. in local directory name is
> +          ;; expanded and not left for the shell command
> +          ;; to interpret.
> +          (expand-file-name dir))
> +         (default-directory dir)
>           (command (format "%s %s %s -type f %s -print0"
>                            find-program
> -                          dirname
> -                          (xref--find-ignores-arguments ignores dirname)
> +                          dir
> +                          (xref--find-ignores-arguments ignores dir)
>                            (if files
>                                (concat (shell-quote-argument "(")
>                                        " " find-name-arg " "

No. dir is a remote file name. So you must still declare dirname as
(file-local-name dir).

Best regards, Michael.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 29 Apr 2020 21:20:06 +0300
> 
> On 29.04.2020 19:44, Eli Zaretskii wrote:
> > Dmitry, this means my change should be reworked to call
> > expand-file-name before file-local-name, right?
> 
> Seems so.
> 
> Something like this?

Yes, but the original code called file-local-name, whereas this one
doesn't.  We had the call to file-local-name for a reason, didn't we?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, but the original code called file-local-name, whereas this one
> doesn't.  We had the call to file-local-name for a reason, didn't we?

Yes. The process shall always use the local file name in its arguments.

Best regards, Michael.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 21:38, Michael Albinus wrote:
> No. dir is a remote file name. So you must still declare dirname as
> (file-local-name dir).

Right, thank you. I also see no reason not to pass the local name to 
xref--find-ignores-arguments (xref-matches-in-directory ends up doing 
that already).

So the patch will be:

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index dbc967b885..f5f4092bab 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -186,16 +186,14 @@ project--files-in-directory
    (require 'xref)
    (defvar find-name-arg)
    (let* ((default-directory dir)
-         (dirname (file-remote-p dir 'localname))
-         (dirname (or dirname
-                      ;; Make sure ~/ etc. in local directory name is
-                      ;; expanded and not left for the shell command
-                      ;; to interpret.
-                      (expand-file-name dir)))
+         ;; Make sure ~/ etc. in local directory name is
+         ;; expanded and not left for the shell command
+         ;; to interpret.
+         (localdir (file-local-name (expand-file-name dir)))
           (command (format "%s %s %s -type f %s -print0"
                            find-program
-                          dirname
-                          (xref--find-ignores-arguments ignores dirname)
+                          localdir
+                          (xref--find-ignores-arguments ignores localdir)
                            (if files
                                (concat (shell-quote-argument "(")
                                        " " find-name-arg " "

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index dbc967b885..f5f4092bab 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -186,16 +186,14 @@ project--files-in-directory
>    (require 'xref)
>    (defvar find-name-arg)
>    (let* ((default-directory dir)
> -         (dirname (file-remote-p dir 'localname))
> -         (dirname (or dirname
> -                      ;; Make sure ~/ etc. in local directory name is
> -                      ;; expanded and not left for the shell command
> -                      ;; to interpret.
> -                      (expand-file-name dir)))
> +         ;; Make sure ~/ etc. in local directory name is
> +         ;; expanded and not left for the shell command
> +         ;; to interpret.
> +         (localdir (file-local-name (expand-file-name dir)))
>           (command (format "%s %s %s -type f %s -print0"
>                            find-program
> -                          dirname
> -                          (xref--find-ignores-arguments ignores dirname)
> +                          localdir
> +                          (xref--find-ignores-arguments ignores localdir)
>                            (if files
>                                (concat (shell-quote-argument "(")
>                                        " " find-name-arg " "

LGTM (I haven't tested, 'tho).

Best regards, Michael.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 29 Apr 2020 22:09:12 +0300
> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> 
> On 29.04.2020 21:38, Michael Albinus wrote:
> > No. dir is a remote file name. So you must still declare dirname as
> > (file-local-name dir).
> 
> Right, thank you. I also see no reason not to pass the local name to 
> xref--find-ignores-arguments (xref-matches-in-directory ends up doing 
> that already).
> 
> So the patch will be:

Yes, I think so.

Thanks.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 29.04.2020 13:41, Eli Zaretskii wrote:
> The patch below seems to fix the rest.  Note that I've decided to make
> the change in fileloop.el, since I think all the other callers need
> the same fix.

Naturally.

> Also, using downcase is not entirely correct, we should
> use isearch-no-upper-case-p instead.

I usually like isearch-no-upper-case-p behavior, but here it doesn't fit 
the docstring, does it? There is no value of CASE-FOLD described that 
would indicate that the function will be too smart about it.

So we'd need to update the docstring that the CASE-FOLD value of t will 
obey the value of `search-upper-case'. Then we'll consult it as well, 
like isearch-search does.

Alternatively, we could for now use the below patch which changes the 
behavior less:

diff --git a/lisp/fileloop.el b/lisp/fileloop.el
index 543963feaf..c3c55badf5 100644
--- a/lisp/fileloop.el
+++ b/lisp/fileloop.el
@@ -175,14 +175,16 @@ fileloop-continue
              (funcall fileloop--operate-function)))
        (setq file-finished t))))

+(defun fileloop--case-fold (arg)
+  (if (memq arg '(t nil)) arg case-fold-search))
+
  ;;;###autoload
  (defun fileloop-initialize-search (regexp files case-fold)
    (let ((last-buffer (current-buffer)))
      (fileloop-initialize
       files
       (lambda ()
-       (let ((case-fold-search
-              (if (memq case-fold '(t nil)) case-fold case-fold-search)))
+       (let ((case-fold-search (fileloop--case-fold case-fold)))
           (re-search-forward regexp nil t)))
       (lambda ()
         (unless (eq last-buffer (current-buffer))
@@ -203,15 +205,16 @@ fileloop-initialize-replace
    (fileloop-initialize
     files
     (lambda ()
-     (let ((case-fold-search
-            (if (memql case-fold '(nil t)) case-fold case-fold-search)))
+     (let ((case-fold-search (fileloop--case-fold case-fold)))
         (if (re-search-forward from nil t)
  	   ;; When we find a match, move back
  	   ;; to the beginning of it so perform-replace
  	   ;; will see it.
  	   (goto-char (match-beginning 0)))))
     (lambda ()
-     (perform-replace from to t t delimited nil multi-query-replace-map))))
+     (let ((case-fold-search (fileloop--case-fold case-fold))
+           search-upper-case)
+       (perform-replace from to t t delimited nil 
multi-query-replace-map)))))

  (provide 'fileloop)
  ;;; fileloop.el ends here

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Juri Linkov]

>> Also, using downcase is not entirely correct, we should
>> use isearch-no-upper-case-p instead.
>
> I usually like isearch-no-upper-case-p behavior, but here it doesn't fit
> the docstring, does it? There is no value of CASE-FOLD described that
> would indicate that the function will be too smart about it.
>
> So we'd need to update the docstring that the CASE-FOLD value of t will
> obey the value of `search-upper-case'. Then we'll consult it as well,
> like isearch-search does.

Recently we changed hi-lock.el to follow the same logic as
isearch-no-upper-case-p, so this will make the behavior
consistent across different packages.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  simenheg@runbox.com,
>   monnier@IRO.UMontreal.CA,  40940@debbugs.gnu.org
> Date: Thu, 30 Apr 2020 23:16:04 +0300
> 
> >> Also, using downcase is not entirely correct, we should
> >> use isearch-no-upper-case-p instead.
> >
> > I usually like isearch-no-upper-case-p behavior, but here it doesn't fit
> > the docstring, does it? There is no value of CASE-FOLD described that
> > would indicate that the function will be too smart about it.
> >
> > So we'd need to update the docstring that the CASE-FOLD value of t will
> > obey the value of `search-upper-case'. Then we'll consult it as well,
> > like isearch-search does.
> 
> Recently we changed hi-lock.el to follow the same logic as
> isearch-no-upper-case-p, so this will make the behavior
> consistent across different packages.

Btw, looking at the other callers of fileloop-initialize-search, don't
get their logic backwards?  If the string typed by the user does NOT
include upper-case character, that means the search SHOULD obey the
default value of case-fold-search.  Or am I missing something?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 29 Apr 2020 23:53:46 +0300
> 
> > Also, using downcase is not entirely correct, we should
> > use isearch-no-upper-case-p instead.
> 
> I usually like isearch-no-upper-case-p behavior, but here it doesn't fit 
> the docstring, does it? There is no value of CASE-FOLD described that 
> would indicate that the function will be too smart about it.
> 
> So we'd need to update the docstring that the CASE-FOLD value of t will 
> obey the value of `search-upper-case'. Then we'll consult it as well, 
> like isearch-search does.

I don't think I follow.  isearch-no-upper-case-p is just supposed to
tell you whether a regexp includes upper-case characters that would
need the search to become case-sensitive.  All the rest of the
considerations, like the value of case-fold-search, are to be applied
by the caller.  I see no reference to either case-fold-search or
case-fold in isearch-no-upper-case-p.  So why would we need to update
its doc string?

> Alternatively, we could for now use the below patch which changes the 
> behavior less:
> 
> diff --git a/lisp/fileloop.el b/lisp/fileloop.el
> index 543963feaf..c3c55badf5 100644
> --- a/lisp/fileloop.el
> +++ b/lisp/fileloop.el
> @@ -175,14 +175,16 @@ fileloop-continue
>               (funcall fileloop--operate-function)))
>         (setq file-finished t))))
> 
> +(defun fileloop--case-fold (arg)
> +  (if (memq arg '(t nil)) arg case-fold-search))
> +
>   ;;;###autoload
>   (defun fileloop-initialize-search (regexp files case-fold)
>     (let ((last-buffer (current-buffer)))
>       (fileloop-initialize
>        files
>        (lambda ()
> -       (let ((case-fold-search
> -              (if (memq case-fold '(t nil)) case-fold case-fold-search)))
> +       (let ((case-fold-search (fileloop--case-fold case-fold)))
>            (re-search-forward regexp nil t)))
>        (lambda ()
>          (unless (eq last-buffer (current-buffer))
> @@ -203,15 +205,16 @@ fileloop-initialize-replace
>     (fileloop-initialize
>      files
>      (lambda ()
> -     (let ((case-fold-search
> -            (if (memql case-fold '(nil t)) case-fold case-fold-search)))
> +     (let ((case-fold-search (fileloop--case-fold case-fold)))
>          (if (re-search-forward from nil t)
>   	   ;; When we find a match, move back
>   	   ;; to the beginning of it so perform-replace
>   	   ;; will see it.
>   	   (goto-char (match-beginning 0)))))
>      (lambda ()
> -     (perform-replace from to t t delimited nil multi-query-replace-map))))
> +     (let ((case-fold-search (fileloop--case-fold case-fold))
> +           search-upper-case)
> +       (perform-replace from to t t delimited nil multi-query-replace-map)))))

Does that really work in the case in point?  Unless I'm missing
something, if case-fold-search is t by default, this patch would cause
the search to be case-insensitive even if the FROM regexp includes
upper-case characters.  But in that case, perform-replace will
internally decide to be case-sensitive, and we have the same failure
on our hands.  This is why the patch I proposed explicitly examined
the FROM regexp for upper-case characters.  Whereas yours doesn't.

Thanks.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 01.05.2020 09:57, Eli Zaretskii wrote:
> I see no reference to either case-fold-search or
> case-fold in isearch-no-upper-case-p.  So why would we need to update
> its doc string?

Sorry if that was unclear: I think we'd need to update the docstring of 
fileloop-initialize-replace. Which doesn't offer any hints that the 
logic of isearch-no-upper-case-p will be employed.

Also see the part about obeying the value of search-upper-case.

> Does that really work in the case in point?  Unless I'm missing
> something, if case-fold-search is t by default, this patch would cause
> the search to be case-insensitive even if the FROM regexp includes
> upper-case characters.  But in that case, perform-replace will
> internally decide to be case-sensitive, and we have the same failure
> on our hands.  This is why the patch I proposed explicitly examined
> the FROM regexp for upper-case characters.  Whereas yours doesn't.

Since we bind search-upper-case to nil in this patch, perform-replace 
won't try to alter the value of case-fold-search internally.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 1 May 2020 18:27:45 +0300
> 
> On 01.05.2020 09:57, Eli Zaretskii wrote:
> > I see no reference to either case-fold-search or
> > case-fold in isearch-no-upper-case-p.  So why would we need to update
> > its doc string?
> 
> Sorry if that was unclear: I think we'd need to update the docstring of 
> fileloop-initialize-replace. Which doesn't offer any hints that the 
> logic of isearch-no-upper-case-p will be employed.

Ah, okay.  Agreed.

> > Does that really work in the case in point?  Unless I'm missing
> > something, if case-fold-search is t by default, this patch would cause
> > the search to be case-insensitive even if the FROM regexp includes
> > upper-case characters.  But in that case, perform-replace will
> > internally decide to be case-sensitive, and we have the same failure
> > on our hands.  This is why the patch I proposed explicitly examined
> > the FROM regexp for upper-case characters.  Whereas yours doesn't.
> 
> Since we bind search-upper-case to nil in this patch, perform-replace 
> won't try to alter the value of case-fold-search internally.

But that's contrary to how query-replace works, isn't it?

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 01.05.2020 18:45, Eli Zaretskii wrote:
>> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 1 May 2020 18:27:45 +0300
>>
>> On 01.05.2020 09:57, Eli Zaretskii wrote:
>>> I see no reference to either case-fold-search or
>>> case-fold in isearch-no-upper-case-p.  So why would we need to update
>>> its doc string?
>>
>> Sorry if that was unclear: I think we'd need to update the docstring of
>> fileloop-initialize-replace. Which doesn't offer any hints that the
>> logic of isearch-no-upper-case-p will be employed.
> 
> Ah, okay.  Agreed.

Should I leave that to you? At least the "updated docstring" part.

>>> Does that really work in the case in point?  Unless I'm missing
>>> something, if case-fold-search is t by default, this patch would cause
>>> the search to be case-insensitive even if the FROM regexp includes
>>> upper-case characters.  But in that case, perform-replace will
>>> internally decide to be case-sensitive, and we have the same failure
>>> on our hands.  This is why the patch I proposed explicitly examined
>>> the FROM regexp for upper-case characters.  Whereas yours doesn't.
>>
>> Since we bind search-upper-case to nil in this patch, perform-replace
>> won't try to alter the value of case-fold-search internally.
> 
> But that's contrary to how query-replace works, isn't it?

I suppose. But query-replace documents that aspects of its behavior:

   Matching is independent of case if `case-fold-search' is non-nil and
   FROM-STRING has no uppercase letters.

Though it doesn't mention the search-upper-case variable.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 2 May 2020 02:21:57 +0300
> 
> >> Sorry if that was unclear: I think we'd need to update the docstring of
> >> fileloop-initialize-replace. Which doesn't offer any hints that the
> >> logic of isearch-no-upper-case-p will be employed.
> > 
> > Ah, okay.  Agreed.
> 
> Should I leave that to you? At least the "updated docstring" part.

I can update the doc string, yes.  But I don't think it's a good idea
to divide the coding part between us, though.  So please show the
code, and I will suggest the doc change.

> >> Since we bind search-upper-case to nil in this patch, perform-replace
> >> won't try to alter the value of case-fold-search internally.
> > 
> > But that's contrary to how query-replace works, isn't it?
> 
> I suppose. But query-replace documents that aspects of its behavior:
> 
>    Matching is independent of case if `case-fold-search' is non-nil and
>    FROM-STRING has no uppercase letters.

Sure.  Wouldn't users of project-query-replace-regexp expect the same?


> Though it doesn't mention the search-upper-case variable.

Ugh!  That should be fixed, of course.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Date: Sat, 02 May 2020 09:44:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> 
> > Though it doesn't mention the search-upper-case variable.
> 
> Ugh!  That should be fixed, of course.

Now done on the emacs-27 branch.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 02.05.2020 09:44, Eli Zaretskii wrote:
> I can update the doc string, yes.  But I don't think it's a good idea
> to divide the coding part between us, though.  So please show the
> code, and I will suggest the doc change.

Here's the comprehensive fix I'd like to see:

diff --git a/lisp/fileloop.el b/lisp/fileloop.el
index 543963feaf..69825fdcae 100644
--- a/lisp/fileloop.el
+++ b/lisp/fileloop.el
@@ -181,8 +181,7 @@ fileloop-initialize-search
      (fileloop-initialize
       files
       (lambda ()
-       (let ((case-fold-search
-              (if (memq case-fold '(t nil)) case-fold case-fold-search)))
+       (let ((case-fold-search (fileloop--case-fold regexp case-fold)))
           (re-search-forward regexp nil t)))
       (lambda ()
         (unless (eq last-buffer (current-buffer))
@@ -190,6 +189,15 @@ fileloop-initialize-search
           (message "Scanning file %s...found" buffer-file-name))
         nil))))

+(defun fileloop--case-fold (regexp case-fold)
+  (let ((value
+         (if (memql case-fold '(nil t))
+             case-fold
+           case-fold-search)))
+    (if (and value search-upper-case)
+        (isearch-no-upper-case-p regexp t)
+      value)))
+
  ;;;###autoload
  (defun fileloop-initialize-replace (from to files case-fold &optional 
delimited)
    "Initialize a new round of query&replace on several files.
@@ -203,15 +211,15 @@ fileloop-initialize-replace
    (fileloop-initialize
     files
     (lambda ()
-     (let ((case-fold-search
-            (if (memql case-fold '(nil t)) case-fold case-fold-search)))
+     (let ((case-fold-search (fileloop--case-fold from case-fold)))
         (if (re-search-forward from nil t)
  	   ;; When we find a match, move back
  	   ;; to the beginning of it so perform-replace
  	   ;; will see it.
  	   (goto-char (match-beginning 0)))))
     (lambda ()
-     (perform-replace from to t t delimited nil multi-query-replace-map))))
+     (let ((case-fold-search (fileloop--case-fold from case-fold)))
+       (perform-replace from to t t delimited nil 
multi-query-replace-map)))))

  (provide 'fileloop)
  ;;; fileloop.el ends here

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Simen Heggestøyl]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Here's the comprehensive fix I'd like to see:

This works as expected in my testing.

> +         (if (memql case-fold '(nil t))

AKA (booleanp case-fold) if you'd like.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Eli Zaretskii]

> Cc: simenheg@runbox.com, monnier@IRO.UMontreal.CA, 40940@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 3 May 2020 04:43:26 +0300
> 
> +(defun fileloop--case-fold (regexp case-fold)
> +  (let ((value
> +         (if (memql case-fold '(nil t))
> +             case-fold
> +           case-fold-search)))
> +    (if (and value search-upper-case)
> +        (isearch-no-upper-case-p regexp t)
> +      value)))

LGTM, thanks.  But don't you need to require isearch to get
isearch-no-upper-case-p? or to autoload it?

Here's the doc string I promised:

  (defun fileloop-initialize-replace (from to files case-fold &optional delimited)
    "Initialize a new round of query&replace on several files.
  FROM is a regexp and TO is the replacement to use.
  FILES describes the files, as in `fileloop-initialize'.
  CASE-FOLD can be t, nil, or `default':
    if it is nil, matching of FROM is case-sensitive.
    if it is t, matching of FROM is case-insensitive, except
       when `search-upper-case' is non-nil and FROM includes
       upper-case letters.
    if it is `default', the function uses the value of
       `case-fold-search' instead.
  DELIMITED if non-nil means replace only word-delimited matches."

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 03.05.2020 20:10, Eli Zaretskii wrote:
> LGTM, thanks.  But don't you need to require isearch to get
> isearch-no-upper-case-p? or to autoload it?

isearch is in loadup.el, isn't it?

 > Here's the doc string I promised:

Looks good, thanks.

bug#40940: 27.0.91; project-query-replace-regexp stops too early

[Dmitry Gutov]

On 03.05.2020 20:28, Dmitry Gutov wrote:
>  > Here's the doc string I promised:
> 
> Looks good, thanks.

Patch installed, thanks to all.