GNU bug report logs - #44905
27.1; Packages that customize xref-show-xrefs-function can break Dired's dired-do-find-regexp-and-replace

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44905 in the body.
You can then email your comments to 44905 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Martín <mardani29 <at> yahoo.es>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.1; Packages that customize xref-show-xrefs-function can break
 Dired's dired-do-find-regexp-and-replace
Date: Fri, 27 Nov 2020 21:14:39 +0100

Scenario: A package customizes xref-show-refs-function to use a
different UI to show xref results.  If that function does not return a
valid xref buffer, then Dired's dired-do-find-regexp-and-replace (bound
to Q by default) won't work anymore because it calls
xref-query-replace-in-results on the buffer returned by
xref--show-xrefs.

A workaround is that the package explicitly calls
xref--show-xref-buffer, which is done for example at
https://github.com/alexmurray/ivy-xref/commit/aa97103ea8ce6ab8891e34deff7d43aa83fe36dd,
but it doesn't feel like an ideal solution because of the duplicated
work.

Is there a way dired-do-find-regexp-and-replace could not depend on an
actual xref buffer to perform the replacement?

Message #8 received at 44905 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Daniel Martín <mardani29 <at> yahoo.es>, 44905 <at> debbugs.gnu.org
Subject: Re: bug#44905: 27.1; Packages that customize xref-show-xrefs-function
 can break Dired's dired-do-find-regexp-and-replace
Date: Sat, 28 Nov 2020 21:04:53 +0200

On 27.11.2020 22:14, Daniel Martín via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> 
> Scenario: A package customizes xref-show-refs-function to use a
> different UI to show xref results.  If that function does not return a
> valid xref buffer, then Dired's dired-do-find-regexp-and-replace (bound
> to Q by default) won't work anymore because it calls
> xref-query-replace-in-results on the buffer returned by
> xref--show-xrefs.

Oh, indeed. Thanks for the report.

> A workaround is that the package explicitly calls
> xref--show-xref-buffer, which is done for example at
> https://github.com/alexmurray/ivy-xref/commit/aa97103ea8ce6ab8891e34deff7d43aa83fe36dd,
> but it doesn't feel like an ideal solution because of the duplicated
> work.

It's too bad this wasn't reported a year ago.

> Is there a way dired-do-find-regexp-and-replace could not depend on an
> actual xref buffer to perform the replacement?

Well, there are options like:

 - Mandate that any xref-show-refs-function alternative has a 
corresponding feature that performs replacements in the matches.
 - Reimplement dired-do-find-regexp-and-replace entirely in terms of a 
different UI (e.g. create a faster dired-do-query-replace-regexp).

But in this particular scenario we can just override 
xref-show-xrefs-function to use the default behavior, see below.

While this change in the Right Thing(tm), I have to question the wisdom 
of setting xref-show-xrefs-function to an Ivy or Helm-based function, 
though. Those UIs serve to help you choose one item, whereas commands 
like dired-do-find-regexp and project-find-regexp show the user a list 
of matches, to interact with (usually) several of them.

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 94a2bbf1f3..4caafc8df6 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3140,7 +3140,10 @@ dired-do-find-regexp-and-replace
           (query-replace-read-args
            "Query replace regexp in marked files" t t)))
      (list (nth 0 common) (nth 1 common))))
-  (with-current-buffer (dired-do-find-regexp from)
+  (defvar xref-show-xrefs-function)
+  (with-current-buffer
+      (let ((xref-show-xrefs-function 'xref--show-xref-buffer))
+        (dired-do-find-regexp from))
     (xref-query-replace-in-results from to)))

 (defun dired-nondirectory-p (file)

Message #11 received at 44905 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 44905 <at> debbugs.gnu.org
Subject: Re: bug#44905: 27.1; Packages that customize
 xref-show-xrefs-function can break Dired's
 dired-do-find-regexp-and-replace
Date: Sun, 29 Nov 2020 13:57:14 +0100

Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
> While this change in the Right Thing(tm), I have to question the
> wisdom of setting xref-show-xrefs-function to an Ivy or Helm-based
> function, though. Those UIs serve to help you choose one item, whereas
> commands like dired-do-find-regexp and project-find-regexp show the
> user a list of matches, to interact with (usually) several of them.

I agree with you, Ivy/Helm may not be the best UX for Xref, but this
particular problem can still happen even if the user returns a buffer
that represents the Xref data in a different way.  I think that
xref-query-replace-in-results assumes certain invariants from the
original *xref* buffer that are not documented.

>
> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
> index 94a2bbf1f3..4caafc8df6 100644
> --- a/lisp/dired-aux.el
> +++ b/lisp/dired-aux.el
> @@ -3140,7 +3140,10 @@ dired-do-find-regexp-and-replace
>            (query-replace-read-args
>             "Query replace regexp in marked files" t t)))
>       (list (nth 0 common) (nth 1 common))))
> -  (with-current-buffer (dired-do-find-regexp from)
> +  (defvar xref-show-xrefs-function)
> +  (with-current-buffer
> +      (let ((xref-show-xrefs-function 'xref--show-xref-buffer))
> +        (dired-do-find-regexp from))
>      (xref-query-replace-in-results from to)))
>
>  (defun dired-nondirectory-p (file)

LGTM, thanks.

Message #14 received at 44905 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 44905 <at> debbugs.gnu.org
Subject: Re: bug#44905: 27.1; Packages that customize xref-show-xrefs-function
 can break Dired's dired-do-find-regexp-and-replace
Date: Mon, 30 Nov 2020 03:00:17 +0200

On 29.11.2020 14:57, Daniel Martín via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>>
>> While this change in the Right Thing(tm), I have to question the
>> wisdom of setting xref-show-xrefs-function to an Ivy or Helm-based
>> function, though. Those UIs serve to help you choose one item, whereas
>> commands like dired-do-find-regexp and project-find-regexp show the
>> user a list of matches, to interact with (usually) several of them.
> 
> I agree with you, Ivy/Helm may not be the best UX for Xref, but this
> particular problem can still happen even if the user returns a buffer
> that represents the Xref data in a different way.  I think that
> xref-query-replace-in-results assumes certain invariants from the
> original *xref* buffer that are not documented.

True.

>>
>> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
>> index 94a2bbf1f3..4caafc8df6 100644
>> --- a/lisp/dired-aux.el
>> +++ b/lisp/dired-aux.el
>> @@ -3140,7 +3140,10 @@ dired-do-find-regexp-and-replace
>>             (query-replace-read-args
>>              "Query replace regexp in marked files" t t)))
>>        (list (nth 0 common) (nth 1 common))))
>> -  (with-current-buffer (dired-do-find-regexp from)
>> +  (defvar xref-show-xrefs-function)
>> +  (with-current-buffer
>> +      (let ((xref-show-xrefs-function 'xref--show-xref-buffer))
>> +        (dired-do-find-regexp from))
>>       (xref-query-replace-in-results from to)))
>>
>>   (defun dired-nondirectory-p (file)
> 
> LGTM, thanks.

Eli, is this OK for Emacs 27.2?

Here's also a slightly more future-proofed version that avoids 
referencing the function we might want to rename/change later:

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 94a2bbf1f3..26155190d4 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3140,7 +3140,13 @@ dired-do-find-regexp-and-replace
           (query-replace-read-args
            "Query replace regexp in marked files" t t)))
      (list (nth 0 common) (nth 1 common))))
-  (with-current-buffer (dired-do-find-regexp from)
+  (require 'xref)
+  (defvar xref-show-xrefs-function)
+  (with-current-buffer
+      (let ((xref-show-xrefs-function
+             ;; Some future-proofing (bug#44905).
+             (eval (car (get 'xref-show-xrefs-function 'standard-value)))))
+        (dired-do-find-regexp from))
     (xref-query-replace-in-results from to)))

 (defun dired-nondirectory-p (file)

Message #17 received at 44905 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 44905 <at> debbugs.gnu.org, mardani29 <at> yahoo.es
Subject: Re: bug#44905: 27.1; Packages that customize xref-show-xrefs-function
 can break Dired's dired-do-find-regexp-and-replace
Date: Mon, 30 Nov 2020 17:26:57 +0200

> Cc: 44905 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Mon, 30 Nov 2020 03:00:17 +0200
> 
> >> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
> >> index 94a2bbf1f3..4caafc8df6 100644
> >> --- a/lisp/dired-aux.el
> >> +++ b/lisp/dired-aux.el
> >> @@ -3140,7 +3140,10 @@ dired-do-find-regexp-and-replace
> >>             (query-replace-read-args
> >>              "Query replace regexp in marked files" t t)))
> >>        (list (nth 0 common) (nth 1 common))))
> >> -  (with-current-buffer (dired-do-find-regexp from)
> >> +  (defvar xref-show-xrefs-function)
> >> +  (with-current-buffer
> >> +      (let ((xref-show-xrefs-function 'xref--show-xref-buffer))
> >> +        (dired-do-find-regexp from))
> >>       (xref-query-replace-in-results from to)))
> >>
> >>   (defun dired-nondirectory-p (file)
> > 
> > LGTM, thanks.
> 
> Eli, is this OK for Emacs 27.2?

Yes, thanks.

> Here's also a slightly more future-proofed version that avoids 
> referencing the function we might want to rename/change later:

That, too.

Message #22 received at 44905-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 44905-done <at> debbugs.gnu.org, mardani29 <at> yahoo.es
Subject: Re: bug#44905: 27.1; Packages that customize xref-show-xrefs-function
 can break Dired's dired-do-find-regexp-and-replace
Date: Tue, 1 Dec 2020 03:50:17 +0200

Version: 27.2

On 30.11.2020 17:26, Eli Zaretskii wrote:
>> Cc: 44905 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
>> From: Dmitry Gutov <dgutov <at> yandex.ru>
>> Date: Mon, 30 Nov 2020 03:00:17 +0200
>>
>>>> diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
>>>> index 94a2bbf1f3..4caafc8df6 100644
>>>> --- a/lisp/dired-aux.el
>>>> +++ b/lisp/dired-aux.el
>>>> @@ -3140,7 +3140,10 @@ dired-do-find-regexp-and-replace
>>>>              (query-replace-read-args
>>>>               "Query replace regexp in marked files" t t)))
>>>>         (list (nth 0 common) (nth 1 common))))
>>>> -  (with-current-buffer (dired-do-find-regexp from)
>>>> +  (defvar xref-show-xrefs-function)
>>>> +  (with-current-buffer
>>>> +      (let ((xref-show-xrefs-function 'xref--show-xref-buffer))
>>>> +        (dired-do-find-regexp from))
>>>>        (xref-query-replace-in-results from to)))
>>>>
>>>>    (defun dired-nondirectory-p (file)
>>>
>>> LGTM, thanks.
>>
>> Eli, is this OK for Emacs 27.2?
> 
> Yes, thanks.
> 
>> Here's also a slightly more future-proofed version that avoids
>> referencing the function we might want to rename/change later:
> 
> That, too.

Thanks.

Pushed to the release branch (will get merged to master in due time), 
commit 749e4b7e0b.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.