bug#65854: Multi-file replacement diff

bug#65854: Multi-file replacement diff

[Juri Linkov]

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

Tags: patch

As discussed on emacs-devel, here is the patch that implements
a standalone command that reads a list of files and replacement strings,
then shows a diff to review before applying replacements.
Also provided the Dired integration to show the replacement diff
on marked files.  Later the same function could be used
to show replacement diffs from the xref buffer and maybe
from other packages as well.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multi-file-replace-as-diff.patch --]
[-- Type: text/x-diff, Size: 6672 bytes --]

diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 28513a2c61a..f27abc645bc 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -3655,6 +3655,20 @@ dired-do-query-replace-regexp
    delimited)
   (fileloop-continue))
 
+;;;###autoload
+(defun dired-do-replace-regexp-as-diff (from to &optional delimited)
+  "Do `replace-regexp' of FROM with TO as diff, on all marked files.
+Third arg DELIMITED (prefix arg) means replace only word-delimited matches."
+  (interactive
+   (let ((common
+	  (query-replace-read-args
+	   "Replace regexp as diff in marked files" t t)))
+     (list (nth 0 common) (nth 1 common) (nth 2 common))))
+  (dired-post-do-command)
+  (multi-file-replace-regexp-as-diff
+   (dired-get-marked-files nil nil #'dired-nondirectory-p)
+   from to delimited))
+
 (declare-function xref-query-replace-in-results "xref")
 (declare-function project--files-in-directory "project")
 
diff --git a/lisp/misearch.el b/lisp/misearch.el
index 9ac28c26c48..da70d708a9e 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -387,6 +387,120 @@ multi-isearch-files-regexp
     (goto-char (if isearch-forward (point-min) (point-max)))
     (isearch-forward-regexp nil t)))
 
+\f
+;;; Global multi-file and multi-buffer replacements as diff
+
+(defun multi-file-diff-no-select (old new &optional switches buf label-old label-new)
+  ;; Based on `diff-no-select' tailored to multi-file diffs.
+  "Compare the OLD and NEW file/buffer.
+If the optional SWITCHES is nil, the switches specified in the
+variable `diff-switches' are passed to the diff command,
+otherwise SWITCHES is used.  SWITCHES can be a string or a list
+of strings.  BUF should be non-nil.  LABEL-OLD and LABEL-NEW
+specify labels to use for file names."
+  (unless (bufferp new) (setq new (expand-file-name new)))
+  (unless (bufferp old) (setq old (expand-file-name old)))
+  (or switches (setq switches diff-switches)) ; If not specified, use default.
+  (setq switches (ensure-list switches))
+  (diff-check-labels)
+  (let* ((old-alt (diff-file-local-copy old))
+         (new-alt (diff-file-local-copy new))
+         (command
+          (mapconcat #'identity
+                     `(,diff-command
+                       ;; Use explicitly specified switches
+                       ,@switches
+                       ,@(mapcar #'shell-quote-argument
+                                 (nconc
+                                  (and (or old-alt new-alt)
+                                       (eq diff-use-labels t)
+                                       (list "--label"
+                                             (cond ((stringp label-old) label-old)
+                                                   ((stringp old) old)
+                                                   ((prin1-to-string old)))
+                                             "--label"
+                                             (cond ((stringp label-new) label-new)
+                                                   ((stringp new) new)
+                                                   ((prin1-to-string new)))))
+                                  (list (or old-alt old)
+                                        (or new-alt new)))))
+                     " ")))
+    (with-current-buffer buf
+      (insert command "\n")
+      (call-process shell-file-name nil buf nil
+                    shell-command-switch command)
+      (if old-alt (delete-file old-alt))
+      (if new-alt (delete-file new-alt)))))
+
+(defun multi-file-replace-as-diff (files-or-buffers from-string replacements regexp-flag delimited-flag)
+  (require 'diff)
+  (let ((inhibit-message t)
+        (diff-buffer (get-buffer-create "*replace-diff*")))
+    (with-current-buffer diff-buffer
+      (buffer-disable-undo (current-buffer))
+      (let ((inhibit-read-only t))
+        (erase-buffer))
+      (diff-mode))
+    (dolist (file-or-buffer files-or-buffers)
+      (let ((file-name (if (bufferp file-or-buffer) buffer-file-name file-or-buffer)))
+        (when file-name
+          (with-temp-buffer
+            (if (bufferp file-or-buffer)
+                (insert-buffer-substring file-or-buffer)
+              (insert-file-contents file-or-buffer))
+            (goto-char (point-min))
+            (perform-replace from-string replacements nil regexp-flag delimited-flag)
+            (multi-file-diff-no-select file-or-buffer (current-buffer) nil diff-buffer
+                                       (concat file-name "~") file-name)))))
+    (with-current-buffer diff-buffer
+      (setq-local buffer-read-only t)
+      (setq-local revert-buffer-function
+                  (lambda (_ignore-auto _noconfirm)
+                    (multi-file-replace-as-diff
+                     files-or-buffers from-string replacements regexp-flag delimited-flag)))
+      (diff-setup-whitespace)
+      (font-lock-ensure)
+      (buffer-enable-undo (current-buffer))
+      (goto-char (point-min)))
+    (pop-to-buffer diff-buffer)))
+
+;;;###autoload
+(defun multi-file-replace-regexp-as-diff (files regexp to-string &optional delimited)
+  "Show replacements in FILES matching REGEXP with TO-STRING as diff.
+With a prefix argument, ask for a wildcard, and replace in files
+whose file names match the specified wildcard."
+  (interactive
+   (let ((files (if current-prefix-arg
+                    (multi-isearch-read-matching-files)
+                  (multi-isearch-read-files)))
+         (common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in files")
+           t t)))
+     (list files (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff files regexp to-string t delimited))
+
+;;;###autoload
+(defun multi-buffer-replace-regexp-as-diff (buffers regexp to-string &optional delimited)
+  "Show replacements in file BUFFERS matching REGEXP with TO-STRING as diff.
+With a prefix argument, ask for a regexp, and replace in file buffers
+whose names match the specified regexp."
+  (interactive
+   (let ((buffers (if current-prefix-arg
+                      (multi-isearch-read-matching-buffers)
+                    (multi-isearch-read-buffers)))
+         (common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in buffers")
+           t t)))
+     (list buffers (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff buffers regexp to-string t delimited))
+
+\f
 (defvar unload-function-defs-list)
 
 (defun multi-isearch-unload-function ()

bug#65854: Multi-file replacement diff

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

Hi there,

Juri Linkov <juri@linkov.net> writes:

> +(defun multi-file-replace-as-diff (files-or-buffers from-string replacements regexp-flag delimited-flag)
> +  (require 'diff)
> +  (let ((inhibit-message t)
> +        (diff-buffer (get-buffer-create "*replace-diff*")))
> +    (with-current-buffer diff-buffer
> +      (buffer-disable-undo (current-buffer))
> +      (let ((inhibit-read-only t))
> +        (erase-buffer))
> +      (diff-mode))
> +    (dolist (file-or-buffer files-or-buffers)
> +      (let ((file-name (if (bufferp file-or-buffer) buffer-file-name file-or-buffer)))
> +        (when file-name
> +          (with-temp-buffer
> +            (if (bufferp file-or-buffer)
> +                (insert-buffer-substring file-or-buffer)
> +              (insert-file-contents file-or-buffer))

I wonder what happens if I call `multi-file-replace-regexp-as-diff` and
select a file `foo.txt`, that I already have open and modified in a
buffer.  IIUC, this will generate the diff based on the contents of the
file on disk, not the buffer, so it might not match when I subsequently
try to apply the diff to the buffer.  WDYT?


Eshel

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> +(defun multi-file-replace-as-diff (files-or-buffers from-string replacements regexp-flag delimited-flag)
>> +  (require 'diff)
>> +  (let ((inhibit-message t)
>> +        (diff-buffer (get-buffer-create "*replace-diff*")))
>> +    (with-current-buffer diff-buffer
>> +      (buffer-disable-undo (current-buffer))
>> +      (let ((inhibit-read-only t))
>> +        (erase-buffer))
>> +      (diff-mode))
>> +    (dolist (file-or-buffer files-or-buffers)
>> +      (let ((file-name (if (bufferp file-or-buffer) buffer-file-name file-or-buffer)))
>> +        (when file-name
>> +          (with-temp-buffer
>> +            (if (bufferp file-or-buffer)
>> +                (insert-buffer-substring file-or-buffer)
>> +              (insert-file-contents file-or-buffer))
>
> I wonder what happens if I call `multi-file-replace-regexp-as-diff` and
> select a file `foo.txt`, that I already have open and modified in a
> buffer.  IIUC, this will generate the diff based on the contents of the
> file on disk, not the buffer, so it might not match when I subsequently
> try to apply the diff to the buffer.  WDYT?

For such cases you can use multi-buffer-replace-regexp-as-diff
from this patch instead of multi-file-replace-regexp-as-diff.

The former generates the diff based on the contents of the
file in the buffer, the latter uses the contents on disk.

bug#65854: Multi-file replacement diff

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

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

>>> +(defun multi-file-replace-as-diff (files-or-buffers from-string replacements regexp-flag delimited-flag)
>>> +  (require 'diff)
>>> +  (let ((inhibit-message t)
>>> +        (diff-buffer (get-buffer-create "*replace-diff*")))
>>> +    (with-current-buffer diff-buffer
>>> +      (buffer-disable-undo (current-buffer))
>>> +      (let ((inhibit-read-only t))
>>> +        (erase-buffer))
>>> +      (diff-mode))
>>> +    (dolist (file-or-buffer files-or-buffers)
>>> +      (let ((file-name (if (bufferp file-or-buffer) buffer-file-name file-or-buffer)))
>>> +        (when file-name
>>> +          (with-temp-buffer
>>> +            (if (bufferp file-or-buffer)
>>> +                (insert-buffer-substring file-or-buffer)
>>> +              (insert-file-contents file-or-buffer))
>>
>> I wonder what happens if I call `multi-file-replace-regexp-as-diff` and
>> select a file `foo.txt`, that I already have open and modified in a
>> buffer.  IIUC, this will generate the diff based on the contents of the
>> file on disk, not the buffer, so it might not match when I subsequently
>> try to apply the diff to the buffer.  WDYT?
>
> For such cases you can use multi-buffer-replace-regexp-as-diff
> from this patch instead of multi-file-replace-regexp-as-diff.

Well, in the simple example of one file, yes that possible, but the
point is that you don't always know (or worry about) whether there's an
overlap between the files you have open and modified and the files your
regexp/wildcard matches.  Let's say I'm editing an HTML file, and find
something that I'd like to change.  So I do it.  Than I think "actually,
let's change that across all my HTML files in this directory".  IMO It
would be great if I could use this new command,
`multi-file-replace-regexp-as-diff`, to get a diff showing how that'd
look.  But in the proposed implementation, that won't work if one of
those HTML files is open and modified--without any warning, Emacs would
create a diff that doesn't apply.

> The former generates the diff based on the contents of the
> file in the buffer, the latter uses the contents on disk.

Yes, but what is the use case for generating the diff based on the
contents on disk when the file is modified in Emacs?  Basically, my
suggestion is to check in `multi-file-replace-regexp-as-diff` if any of
the matching files are visited by some buffer, and if so simply pass the
buffer instead of the file name for that file to
`multi-file-replace-as-diff`.  That way you always get an up-to-date
diff, and you don't need to manually check that you don't have any of
the matching files open by any chance.  Does that make sense?

bug#65854: Multi-file replacement diff

[Juri Linkov]

> Yes, but what is the use case for generating the diff based on the
> contents on disk when the file is modified in Emacs?  Basically, my
> suggestion is to check in `multi-file-replace-regexp-as-diff` if any of
> the matching files are visited by some buffer, and if so simply pass the
> buffer instead of the file name for that file to
> `multi-file-replace-as-diff`.  That way you always get an up-to-date
> diff, and you don't need to manually check that you don't have any of
> the matching files open by any chance.  Does that make sense?

Thanks for the idea, this makes sense and will help to
reduce the number of commands from 2 to 1 by merging
multi-buffer-replace-regexp-as-diff with
multi-file-replace-regexp-as-diff.

bug#65854: Multi-file replacement diff

[Eli Zaretskii]

> Cc: 65854@debbugs.gnu.org
> Date: Mon, 11 Sep 2023 09:23:37 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >> I wonder what happens if I call `multi-file-replace-regexp-as-diff` and
> >> select a file `foo.txt`, that I already have open and modified in a
> >> buffer.  IIUC, this will generate the diff based on the contents of the
> >> file on disk, not the buffer, so it might not match when I subsequently
> >> try to apply the diff to the buffer.  WDYT?
> >
> > For such cases you can use multi-buffer-replace-regexp-as-diff
> > from this patch instead of multi-file-replace-regexp-as-diff.
> 
> Well, in the simple example of one file, yes that possible, but the
> point is that you don't always know (or worry about) whether there's an
> overlap between the files you have open and modified and the files your
> regexp/wildcard matches.  Let's say I'm editing an HTML file, and find
> something that I'd like to change.  So I do it.  Than I think "actually,
> let's change that across all my HTML files in this directory".  IMO It
> would be great if I could use this new command,
> `multi-file-replace-regexp-as-diff`, to get a diff showing how that'd
> look.  But in the proposed implementation, that won't work if one of
> those HTML files is open and modified--without any warning, Emacs would
> create a diff that doesn't apply.

Our usual paradigm for these commands is to offer saving any buffers
with unsaved edits, before running the main part of the command.

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> Yes, but what is the use case for generating the diff based on the
>> contents on disk when the file is modified in Emacs?  Basically, my
>> suggestion is to check in `multi-file-replace-regexp-as-diff` if any of
>> the matching files are visited by some buffer, and if so simply pass the
>> buffer instead of the file name for that file to
>> `multi-file-replace-as-diff`.  That way you always get an up-to-date
>> diff, and you don't need to manually check that you don't have any of
>> the matching files open by any chance.  Does that make sense?
>
> Thanks for the idea, this makes sense and will help to
> reduce the number of commands from 2 to 1 by merging
> multi-buffer-replace-regexp-as-diff with
> multi-file-replace-regexp-as-diff.

Actually, separate commands are still needed when we will add
a command to show replacement diffs on the buffers marked on
the buffer list from M-x list-buffers.

But it seems we can't avoid the limitation that such buffers
should be file-visiting.  I see no way to generate a diff
for non-file buffers because 'C-c C-a' from diff-mode needs
a file name to apply the hunk to the file buffer.

Also another useful command that probably will be used the
most often is to show a replacement diff for the current buffer
as a counterpart of query-replace:

```
diff --git a/lisp/misearch.el b/lisp/misearch.el
index da70d708a9e..9aa639af5fe 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -500,6 +501,22 @@ multi-buffer-replace-regexp-as-diff
      (list buffers (nth 0 common) (nth 1 common) (nth 2 common))))
   (multi-file-replace-as-diff buffers regexp to-string t delimited))
 
+;;;###autoload
+(defun replace-regexp-as-diff (regexp to-string &optional delimited)
+  "Show replacements in the current file buffer matching REGEXP with TO-STRING as diff.
+With a prefix argument, ask for a regexp, and replace in file buffers
+whose names match the specified regexp."
+  (interactive
+   (let ((common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in buffers")
+           t t)))
+     (list (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff
+   (list (current-buffer)) regexp to-string t delimited))
+
 \f
 (defvar unload-function-defs-list)
 
```

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> Well, in the simple example of one file, yes that possible, but the
>> point is that you don't always know (or worry about) whether there's an
>> overlap between the files you have open and modified and the files your
>> regexp/wildcard matches.  Let's say I'm editing an HTML file, and find
>> something that I'd like to change.  So I do it.  Than I think "actually,
>> let's change that across all my HTML files in this directory".  IMO It
>> would be great if I could use this new command,
>> `multi-file-replace-regexp-as-diff`, to get a diff showing how that'd
>> look.  But in the proposed implementation, that won't work if one of
>> those HTML files is open and modified--without any warning, Emacs would
>> create a diff that doesn't apply.
>
> Our usual paradigm for these commands is to offer saving any buffers
> with unsaved edits, before running the main part of the command.

I don't know if users will find this too annoying, maybe not,
but probably this is what should be done to solve such dilemma that
the same diff can be used in two ways: by applying the diff to the
buffers with 'C-c C-a', or by applying to the files with 'git apply'.
Without saving the buffers before running the command, either
will fail: 'C-c C-a' will fail on changes produced from files,
'git apply' will fail on changes produced from unsaved buffers.

bug#65854: Multi-file replacement diff

[Juri Linkov]

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

> I don't know if users will find this too annoying, maybe not,
> but probably this is what should be done to solve such dilemma that
> the same diff can be used in two ways: by applying the diff to the
> buffers with 'C-c C-a', or by applying to the files with 'git apply'.
> Without saving the buffers before running the command, either
> will fail: 'C-c C-a' will fail on changes produced from files,
> 'git apply' will fail on changes produced from unsaved buffers.

Here is a new customizable option 'multi-file-diff-unsaved'
that defines what to do with unsaved changes.  When it is
'use-buffer' then it handles the case Eshel demonstrated
where changes are applied over unsaved buffers.  But when
the value is 'save-buffers', then 'save-some-buffers' is
called before producing the diff.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multi-file-diff-unsaved.patch --]
[-- Type: text/x-diff, Size: 7717 bytes --]

diff --git a/lisp/misearch.el b/lisp/misearch.el
index 9ac28c26c48..23872335c2d 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -387,6 +387,160 @@ multi-isearch-files-regexp
     (goto-char (if isearch-forward (point-min) (point-max)))
     (isearch-forward-regexp nil t)))
 
+\f
+;;; Global multi-file and multi-buffer replacements as diff
+
+(defcustom multi-file-diff-unsaved 'use-file
+  "A choice defining what to do with unsaved changes.
+If the value is `use-file', use text from the file.
+If the value is `use-buffer', use text from the file-visiting buffer
+to be able to use unsaved changes.  However, when the file is
+not visited in a buffer, read contents from the file.
+If the value is `save-buffers', save unsaved buffers before creating diff."
+  :type '(choice
+          (const :tag "Use file" use-file)
+          (const :tag "Use buffer" use-buffer)
+          (const :tag "Save buffers" save-buffers))
+  :version "30.1")
+
+(defun multi-file-replace-as-diff (files-or-buffers from-string replacements regexp-flag delimited-flag)
+  (require 'diff)
+  (let ((inhibit-message t)
+        (diff-buffer (get-buffer-create "*replace-diff*")))
+    (when (eq multi-file-diff-unsaved 'save-buffers)
+      (save-some-buffers t (lambda ()
+                             (seq-some (lambda (f-or-b)
+                                         (if (bufferp f-or-b)
+                                             (eq f-or-b (current-buffer))
+                                           (equal f-or-b (buffer-file-name))))
+                                       files-or-buffers))))
+    (with-current-buffer diff-buffer
+      (setq-local buffer-read-only t)
+      (erase-buffer)
+      (diff-mode)
+      (setq-local buffer-read-only nil)
+      (buffer-disable-undo (current-buffer)))
+    (dolist (file-or-buffer files-or-buffers)
+      (let ((file-name (if (bufferp file-or-buffer) buffer-file-name file-or-buffer)))
+        (when file-name
+          (with-temp-buffer
+            (if (bufferp file-or-buffer)
+                (insert-buffer-substring file-or-buffer)
+              (if (or (eq multi-file-diff-unsaved 'use-file)
+                      (not (find-buffer-visiting file-or-buffer)))
+                  (insert-file-contents file-or-buffer)
+                (insert-buffer-substring (find-buffer-visiting file-or-buffer))))
+            (goto-char (point-min))
+            (perform-replace from-string replacements nil regexp-flag delimited-flag)
+            (multi-file-diff-no-select file-or-buffer (current-buffer) nil diff-buffer
+                                       (concat file-name "~") file-name)))))
+    (with-current-buffer diff-buffer
+      (diff-setup-whitespace)
+      (font-lock-ensure)
+      (buffer-enable-undo (current-buffer))
+      (setq-local buffer-read-only t)
+      (setq-local revert-buffer-function
+                  (lambda (_ignore-auto _noconfirm)
+                    (multi-file-replace-as-diff
+                     files-or-buffers from-string replacements regexp-flag delimited-flag)))
+      (goto-char (point-min)))
+    (pop-to-buffer diff-buffer)))
+
+(defun multi-file-diff-no-select (old new &optional switches buf label-old label-new)
+  ;; Based on `diff-no-select' tailored to multi-file diffs.
+  "Compare the OLD and NEW file/buffer.
+If the optional SWITCHES is nil, the switches specified in the
+variable `diff-switches' are passed to the diff command,
+otherwise SWITCHES is used.  SWITCHES can be a string or a list
+of strings.  BUF should be non-nil.  LABEL-OLD and LABEL-NEW
+specify labels to use for file names."
+  (unless (bufferp new) (setq new (expand-file-name new)))
+  (unless (bufferp old) (setq old (expand-file-name old)))
+  (or switches (setq switches diff-switches)) ; If not specified, use default.
+  (setq switches (ensure-list switches))
+  (diff-check-labels)
+  (let* ((old-alt (diff-file-local-copy old))
+         (new-alt (diff-file-local-copy new))
+         (command
+          (mapconcat #'identity
+                     `(,diff-command
+                       ;; Use explicitly specified switches
+                       ,@switches
+                       ,@(mapcar #'shell-quote-argument
+                                 (nconc
+                                  (and (or old-alt new-alt)
+                                       (eq diff-use-labels t)
+                                       (list "--label"
+                                             (cond ((stringp label-old) label-old)
+                                                   ((stringp old) old)
+                                                   ((prin1-to-string old)))
+                                             "--label"
+                                             (cond ((stringp label-new) label-new)
+                                                   ((stringp new) new)
+                                                   ((prin1-to-string new)))))
+                                  (list (or old-alt old)
+                                        (or new-alt new)))))
+                     " ")))
+    (with-current-buffer buf
+      (insert command "\n")
+      (call-process shell-file-name nil buf nil
+                    shell-command-switch command)
+      (if old-alt (delete-file old-alt))
+      (if new-alt (delete-file new-alt)))))
+
+;;;###autoload
+(defun multi-file-replace-regexp-as-diff (files regexp to-string &optional delimited)
+  "Show replacements in FILES matching REGEXP with TO-STRING as diff.
+With a prefix argument, ask for a wildcard, and replace in files
+whose file names match the specified wildcard."
+  (interactive
+   (let ((files (if current-prefix-arg
+                    (multi-isearch-read-matching-files)
+                  (multi-isearch-read-files)))
+         (common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in files")
+           t t)))
+     (list files (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff files regexp to-string t delimited))
+
+;;;###autoload
+(defun multi-buffer-replace-regexp-as-diff (buffers regexp to-string &optional delimited)
+  "Show replacements in file BUFFERS matching REGEXP with TO-STRING as diff.
+With a prefix argument, ask for a regexp, and replace in file buffers
+whose names match the specified regexp."
+  (interactive
+   (let ((buffers (if current-prefix-arg
+                      (multi-isearch-read-matching-buffers)
+                    (multi-isearch-read-buffers)))
+         (common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in buffers")
+           t t)))
+     (list buffers (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff buffers regexp to-string t delimited))
+
+;;;###autoload
+(defun replace-regexp-as-diff (regexp to-string &optional delimited)
+  "Show replacements in the current file buffer matching REGEXP with TO-STRING as diff.
+With a prefix argument, ask for a regexp, and replace in file buffers
+whose names match the specified regexp."
+  (interactive
+   (let ((common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in buffers")
+           t t)))
+     (list (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff
+   (list (current-buffer)) regexp to-string t delimited))
+
+\f
 (defvar unload-function-defs-list)
 
 (defun multi-isearch-unload-function ()

bug#65854: Multi-file replacement diff

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

Juri Linkov <juri@linkov.net> writes:

>
> Here is a new customizable option 'multi-file-diff-unsaved'
> that defines what to do with unsaved changes.  When it is
> 'use-buffer' then it handles the case Eshel demonstrated
> where changes are applied over unsaved buffers.  But when
> the value is 'save-buffers', then 'save-some-buffers' is
> called before producing the diff.
>

Nice!

One thought about the default choice:

> +(defcustom multi-file-diff-unsaved 'use-file

I wonder if it wouldn't be better to use save-buffers by default, ISTM
that it's the "least surprising" choice.


Cheers,

Eshel

bug#65854: Multi-file replacement diff

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eshel Yaron <me@eshelyaron.com>,  65854@debbugs.gnu.org
> Date: Fri, 15 Sep 2023 09:40:21 +0300
> 
> +(defcustom multi-file-diff-unsaved 'use-file
> +  "A choice defining what to do with unsaved changes.

This first sentence is too general.  I suggest

  What to do with unsaved edits when showing multi-file replacements as diffs.

> +(defun multi-file-replace-regexp-as-diff (files regexp to-string &optional delimited)
> +  "Show replacements in FILES matching REGEXP with TO-STRING as diff.

  Show as diffs replacements of REGEXP with TO-STRING in FILES.

> +With a prefix argument, ask for a wildcard, and replace in files

"replace"?  Does this command perform replacements or just shows them
as diffs?  The first sentence seems to convey the latter.

> +(defun multi-buffer-replace-regexp-as-diff (buffers regexp to-string &optional delimited)
> +  "Show replacements in file BUFFERS matching REGEXP with TO-STRING as diff.
> +With a prefix argument, ask for a regexp, and replace in file buffers
> +whose names match the specified regexp."

Same comments here.

> +(defun replace-regexp-as-diff (regexp to-string &optional delimited)
> +  "Show replacements in the current file buffer matching REGEXP with TO-STRING as diff.
> +With a prefix argument, ask for a regexp, and replace in file buffers
> +whose names match the specified regexp."

And here.

Thanks.

bug#65854: Multi-file replacement diff

[Juri Linkov]

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

>> +(defcustom multi-file-diff-unsaved 'use-file
>> +  "A choice defining what to do with unsaved changes.
>
> This first sentence is too general.  I suggest
>
>   What to do with unsaved edits when showing multi-file replacements as diffs.

Fixed, and changed the default to 'save-buffers' like Eshel suggested.

>> +(defun multi-file-replace-regexp-as-diff (files regexp to-string &optional delimited)
>> +  "Show replacements in FILES matching REGEXP with TO-STRING as diff.
>
>   Show as diffs replacements of REGEXP with TO-STRING in FILES.

Fixed with more small changes:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multi-file-diff-unsaved.patch --]
[-- Type: text/x-diff, Size: 7299 bytes --]

diff --git a/lisp/misearch.el b/lisp/misearch.el
index 9ac28c26c48..44023d029d9 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -387,6 +387,148 @@ multi-isearch-files-regexp
     (goto-char (if isearch-forward (point-min) (point-max)))
     (isearch-forward-regexp nil t)))
 
+\f
+;;; Global multi-file and multi-buffer replacements as diff
+
+(defcustom multi-file-diff-unsaved 'save-buffers
+  "What to do with unsaved edits when showing multi-file replacements as diffs.
+If the value is `save-buffers', save unsaved buffers before creating diff.
+If the value is `use-file', use text from the file even when the visiting
+file buffer is modified.
+If the value is `use-modified-buffer', use text from the file-visiting
+modified buffer to be able to use unsaved changes.  However, when the file
+is not visited in a buffer, or the buffer is not modified, still read
+contents from the file."
+  :type '(choice
+          (const :tag "Save buffers" save-buffers)
+          (const :tag "Use file" use-file)
+          (const :tag "Use modified buffer" use-modified-buffer))
+  :version "30.1")
+
+(defun multi-file-replace-as-diff (files-or-buffers from-string replacements regexp-flag delimited-flag)
+  "Show as diffs replacements of FROM-STRING with REPLACEMENTS.
+FILES-OR-BUFFERS is a list of either file names or buffers.
+REGEXP-FLAG and DELIMITED-FLAG have the same meaning as in `perform-replace'."
+  (require 'diff)
+  (let ((inhibit-message t)
+        (diff-buffer (get-buffer-create "*replace-diff*")))
+    (when (eq multi-file-diff-unsaved 'save-buffers)
+      (save-some-buffers t (lambda ()
+                             (seq-some (lambda (f-or-b)
+                                         (if (bufferp f-or-b)
+                                             (eq f-or-b (current-buffer))
+                                           (equal f-or-b (buffer-file-name))))
+                                       files-or-buffers))))
+    (with-current-buffer diff-buffer
+      (setq-local buffer-read-only t)
+      (erase-buffer)
+      (diff-mode)
+      (setq-local buffer-read-only nil)
+      (buffer-disable-undo (current-buffer)))
+    (dolist (file-or-buffer files-or-buffers)
+      (let ((file-name (if (bufferp file-or-buffer) buffer-file-name file-or-buffer))
+            (file-buffer (when (eq multi-file-diff-unsaved 'use-modified-buffer)
+                           (find-buffer-visiting file-or-buffer))))
+        (when file-name
+          (with-temp-buffer
+            (if (bufferp file-or-buffer)
+                (insert-buffer-substring file-or-buffer)
+              (if (and file-buffer (buffer-modified-p file-buffer))
+                  (insert-buffer-substring file-buffer)
+                (insert-file-contents file-or-buffer)))
+            (goto-char (point-min))
+            (perform-replace from-string replacements nil regexp-flag delimited-flag)
+            (multi-file-diff-no-select file-or-buffer (current-buffer) nil diff-buffer
+                                       (concat file-name "~") file-name)))))
+    (with-current-buffer diff-buffer
+      (diff-setup-whitespace)
+      (font-lock-ensure)
+      (buffer-enable-undo (current-buffer))
+      (setq-local buffer-read-only t)
+      (setq-local revert-buffer-function
+                  (lambda (_ignore-auto _noconfirm)
+                    (multi-file-replace-as-diff
+                     files-or-buffers from-string replacements regexp-flag delimited-flag)))
+      (goto-char (point-min)))
+    (pop-to-buffer diff-buffer)))
+
+;;;###autoload
+(defun multi-file-replace-regexp-as-diff (files regexp to-string &optional delimited)
+  "Show as diffs replacements of REGEXP with TO-STRING in FILES.
+DELIMITED has the same meaning as in `replace-regexp'.
+With a prefix argument, ask for a wildcard, and show diffs for files
+whose file names match the specified wildcard."
+  (interactive
+   (let ((files (if current-prefix-arg
+                    (multi-isearch-read-matching-files)
+                  (multi-isearch-read-files)))
+         (common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in files")
+           t t)))
+     (list files (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff files regexp to-string t delimited))
+
+;;;###autoload
+(defun replace-regexp-as-diff (regexp to-string &optional delimited)
+  "Show as diffs replacements of REGEXP with TO-STRING in the current buffer.
+DELIMITED has the same meaning as in `replace-regexp'."
+  (interactive
+   (let ((common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff")
+           t t)))
+     (list (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff
+   (list (current-buffer)) regexp to-string t delimited))
+
+(defun multi-file-diff-no-select (old new &optional switches buf label-old label-new)
+  ;; Based on `diff-no-select' tailored to multi-file diffs.
+  "Compare the OLD and NEW file/buffer.
+If the optional SWITCHES is nil, the switches specified in the
+variable `diff-switches' are passed to the diff command,
+otherwise SWITCHES is used.  SWITCHES can be a string or a list
+of strings.  BUF should be non-nil.  LABEL-OLD and LABEL-NEW
+specify labels to use for file names."
+  (unless (bufferp new) (setq new (expand-file-name new)))
+  (unless (bufferp old) (setq old (expand-file-name old)))
+  (or switches (setq switches diff-switches)) ; If not specified, use default.
+  (setq switches (ensure-list switches))
+  (diff-check-labels)
+  (let* ((old-alt (diff-file-local-copy old))
+         (new-alt (diff-file-local-copy new))
+         (command
+          (mapconcat #'identity
+                     `(,diff-command
+                       ;; Use explicitly specified switches
+                       ,@switches
+                       ,@(mapcar #'shell-quote-argument
+                                 (nconc
+                                  (and (or old-alt new-alt)
+                                       (eq diff-use-labels t)
+                                       (list "--label"
+                                             (cond ((stringp label-old) label-old)
+                                                   ((stringp old) old)
+                                                   ((prin1-to-string old)))
+                                             "--label"
+                                             (cond ((stringp label-new) label-new)
+                                                   ((stringp new) new)
+                                                   ((prin1-to-string new)))))
+                                  (list (or old-alt old)
+                                        (or new-alt new)))))
+                     " ")))
+    (with-current-buffer buf
+      (insert command "\n")
+      (call-process shell-file-name nil buf nil
+                    shell-command-switch command)
+      (if old-alt (delete-file old-alt))
+      (if new-alt (delete-file new-alt)))))
+
+\f
 (defvar unload-function-defs-list)
 
 (defun multi-isearch-unload-function ()

bug#65854: Multi-file replacement diff

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: me@eshelyaron.com,  65854@debbugs.gnu.org
> Date: Fri, 22 Sep 2023 09:55:40 +0300
> 
> +(defcustom multi-file-diff-unsaved 'save-buffers
> +  "What to do with unsaved edits when showing multi-file replacements as diffs.
> +If the value is `save-buffers', save unsaved buffers before creating diff.
> +If the value is `use-file', use text from the file even when the visiting
> +file buffer is modified.
> +If the value is `use-modified-buffer', use text from the file-visiting
> +modified buffer to be able to use unsaved changes.  However, when the file
> +is not visited in a buffer, or the buffer is not modified, still read
> +contents from the file."

Please use consistent wording to describe the same entities.  If you
use "file-visiting buffer", use it everywhere, when you sometimes use
that and sometimes "visiting file buffer", you are already half way to
confusing the reader.

Also, this part:

> +                                                    However, when the file
> +is not visited in a buffer, or the buffer is not modified, still read
> +contents from the file."

Seems to describe an implementation detail, and I don't think it
should be there.  E.g., what if the file visited by the buffer no
longer exists?

Thanks.

bug#65854: Multi-file replacement diff

[Juri Linkov]

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

>> +(defcustom multi-file-diff-unsaved 'save-buffers
>> +  "What to do with unsaved edits when showing multi-file replacements as diffs.
>> +If the value is `save-buffers', save unsaved buffers before creating diff.
>> +If the value is `use-file', use text from the file even when the visiting
>> +file buffer is modified.
>> +If the value is `use-modified-buffer', use text from the file-visiting
>> +modified buffer to be able to use unsaved changes.  However, when the file
>> +is not visited in a buffer, or the buffer is not modified, still read
>> +contents from the file."
>
> Please use consistent wording to describe the same entities.  If you
> use "file-visiting buffer", use it everywhere, when you sometimes use
> that and sometimes "visiting file buffer", you are already half way to
> confusing the reader.

Ok, fixed below.

> Also, this part:
>
>> +                                                    However, when the file
>> +is not visited in a buffer, or the buffer is not modified, still read
>> +contents from the file."
>
> Seems to describe an implementation detail, and I don't think it
> should be there.  E.g., what if the file visited by the buffer no
> longer exists?

If the file visited by the buffer no longer exists, then
the standard error is signaled.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multi-file-diff-unsaved.patch --]
[-- Type: text/x-diff, Size: 7139 bytes --]

diff --git a/lisp/misearch.el b/lisp/misearch.el
index 9ac28c26c48..25d1b115af8 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -387,6 +387,150 @@ multi-isearch-files-regexp
     (goto-char (if isearch-forward (point-min) (point-max)))
     (isearch-forward-regexp nil t)))
 
+\f
+;;; Global multi-file and multi-buffer replacements as diff
+
+(defcustom multi-file-diff-unsaved 'save-buffers
+  "What to do with unsaved edits when showing multi-file replacements as diffs.
+If the value is `save-buffers', save unsaved buffers before creating diff.
+If the value is `use-file', use text from the file even when the
+file-visiting buffer is modified.
+If the value is `use-modified-buffer', use text from the file-visiting
+modified buffer to be able to use unsaved changes."
+  :type '(choice
+          (const :tag "Save buffers" save-buffers)
+          (const :tag "Use file" use-file)
+          (const :tag "Use modified buffer" use-modified-buffer))
+  :version "30.1")
+
+(declare-function diff-setup-whitespace "diff-mode" ())
+(declare-function diff-setup-buffer-type "diff-mode" ())
+
+(defun multi-file-replace-as-diff (files from-string replacements regexp-flag delimited-flag)
+  "Show as diffs replacements of FROM-STRING with REPLACEMENTS.
+FILES is a list of file names.  REGEXP-FLAG and DELIMITED-FLAG have
+the same meaning as in `perform-replace'."
+  (require 'diff)
+  (let ((inhibit-message t)
+        (diff-buffer (get-buffer-create "*replace-diff*")))
+    (when (eq multi-file-diff-unsaved 'save-buffers)
+      (save-some-buffers t (lambda ()
+                             (seq-some (lambda (f-or-b)
+                                         (equal f-or-b buffer-file-name))
+                                       files))))
+    (with-current-buffer diff-buffer
+      (buffer-disable-undo (current-buffer))
+      (let ((inhibit-read-only t))
+        (erase-buffer))
+      ;; Make the *vc-diff* buffer read only, the diff-mode key
+      ;; bindings are nicer for read only buffers.
+      (setq buffer-read-only t)
+      (diff-mode))
+    (dolist (file-name files)
+      (let ((file-buffer (when (eq multi-file-diff-unsaved 'use-modified-buffer)
+                           (find-buffer-visiting file-name))))
+        (when file-name
+          (with-temp-buffer
+            (if (and file-buffer (buffer-modified-p file-buffer))
+                (insert-buffer-substring file-buffer)
+              (insert-file-contents file-name))
+            (goto-char (point-min))
+            (perform-replace from-string replacements nil regexp-flag delimited-flag)
+            (multi-file-diff-no-select file-name (current-buffer) nil diff-buffer
+                                       (concat file-name "~") file-name)))))
+    (with-current-buffer diff-buffer
+      (diff-setup-whitespace)
+      (diff-setup-buffer-type)
+      (buffer-enable-undo (current-buffer))
+      (setq-local revert-buffer-function
+                  (lambda (_ignore-auto _noconfirm)
+                    (multi-file-replace-as-diff
+                     files from-string replacements regexp-flag delimited-flag)))
+      (goto-char (point-min)))
+    (pop-to-buffer diff-buffer)))
+
+;;;###autoload
+(defun multi-file-replace-regexp-as-diff (files regexp to-string &optional delimited)
+  "Show as diffs replacements of REGEXP with TO-STRING in FILES.
+DELIMITED has the same meaning as in `replace-regexp'.
+With a prefix argument, ask for a wildcard, and show diffs for files
+whose file names match the specified wildcard."
+  (interactive
+   (let ((files (if current-prefix-arg
+                    (multi-isearch-read-matching-files)
+                  (multi-isearch-read-files)))
+         (common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff in files")
+           t t)))
+     (list files (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff files regexp to-string t delimited))
+
+;;;###autoload
+(defun replace-regexp-as-diff (regexp to-string &optional delimited)
+  "Show as diffs replacements of REGEXP with TO-STRING in the current buffer.
+DELIMITED has the same meaning as in `replace-regexp'."
+  (interactive
+   (let ((common
+          (query-replace-read-args
+           (concat "Replace"
+                   (if current-prefix-arg " word" "")
+                   " regexp as diff")
+           t t)))
+     (list (nth 0 common) (nth 1 common) (nth 2 common))))
+  (multi-file-replace-as-diff
+   (list buffer-file-name) regexp to-string t delimited))
+
+(defvar diff-use-labels)
+(declare-function diff-check-labels "diff" (&optional force))
+(declare-function diff-file-local-copy "diff" (file-or-buf))
+
+(defun multi-file-diff-no-select (old new &optional switches buf label-old label-new)
+  ;; Based on `diff-no-select' tailored to multi-file diffs.
+  "Compare the OLD and NEW file/buffer.
+If the optional SWITCHES is nil, the switches specified in the
+variable `diff-switches' are passed to the diff command,
+otherwise SWITCHES is used.  SWITCHES can be a string or a list
+of strings.  BUF should be non-nil.  LABEL-OLD and LABEL-NEW
+specify labels to use for file names."
+  (unless (bufferp new) (setq new (expand-file-name new)))
+  (unless (bufferp old) (setq old (expand-file-name old)))
+  (or switches (setq switches diff-switches)) ; If not specified, use default.
+  (setq switches (ensure-list switches))
+  (diff-check-labels)
+  (let* ((old-alt (diff-file-local-copy old))
+         (new-alt (diff-file-local-copy new))
+         (command
+          (mapconcat #'identity
+                     `(,diff-command
+                       ;; Use explicitly specified switches
+                       ,@switches
+                       ,@(mapcar #'shell-quote-argument
+                                 (nconc
+                                  (and (or old-alt new-alt)
+                                       (eq diff-use-labels t)
+                                       (list "--label"
+                                             (cond ((stringp label-old) label-old)
+                                                   ((stringp old) old)
+                                                   ((prin1-to-string old)))
+                                             "--label"
+                                             (cond ((stringp label-new) label-new)
+                                                   ((stringp new) new)
+                                                   ((prin1-to-string new)))))
+                                  (list (or old-alt old)
+                                        (or new-alt new)))))
+                     " ")))
+    (with-current-buffer buf
+      (let ((inhibit-read-only t))
+        (insert command "\n")
+        (call-process shell-file-name nil buf nil
+                      shell-command-switch command))
+      (if old-alt (delete-file old-alt))
+      (if new-alt (delete-file new-alt)))))
+
+\f
 (defvar unload-function-defs-list)
 
 (defun multi-isearch-unload-function ()

bug#65854: Multi-file replacement diff

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: me@eshelyaron.com,  65854@debbugs.gnu.org
> Date: Fri, 22 Sep 2023 19:02:33 +0300
> 
> >> +                                                    However, when the file
> >> +is not visited in a buffer, or the buffer is not modified, still read
> >> +contents from the file."
> >
> > Seems to describe an implementation detail, and I don't think it
> > should be there.  E.g., what if the file visited by the buffer no
> > longer exists?
> 
> If the file visited by the buffer no longer exists, then
> the standard error is signaled.

Which means in that case it is better to use the buffer text, no?

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> >> +                                                    However, when the file
>> >> +is not visited in a buffer, or the buffer is not modified, still read
>> >> +contents from the file."
>> >
>> > Seems to describe an implementation detail, and I don't think it
>> > should be there.  E.g., what if the file visited by the buffer no
>> > longer exists?
>>
>> If the file visited by the buffer no longer exists, then
>> the standard error is signaled.
>
> Which means in that case it is better to use the buffer text, no?

Since replacement diffs are not supported in non-file buffers,
better to signal an error for heads up.

bug#65854: Multi-file replacement diff

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: me@eshelyaron.com,  65854@debbugs.gnu.org
> Date: Sat, 23 Sep 2023 20:36:02 +0300
> 
> >> >> +                                                    However, when the file
> >> >> +is not visited in a buffer, or the buffer is not modified, still read
> >> >> +contents from the file."
> >> >
> >> > Seems to describe an implementation detail, and I don't think it
> >> > should be there.  E.g., what if the file visited by the buffer no
> >> > longer exists?
> >>
> >> If the file visited by the buffer no longer exists, then
> >> the standard error is signaled.
> >
> > Which means in that case it is better to use the buffer text, no?
> 
> Since replacement diffs are not supported in non-file buffers,
> better to signal an error for heads up.

But it _is_ a file-visiting buffer.  It's just that its file was
deleted meanwhile.

bug#65854: Multi-file replacement diff

[Dmitry Gutov]

Hi!

On 10/09/2023 20:18, Juri Linkov wrote:
> As discussed on emacs-devel, here is the patch that implements
> a standalone command that reads a list of files and replacement strings,
> then shows a diff to review before applying replacements.
> Also provided the Dired integration to show the replacement diff
> on marked files.  Later the same function could be used
> to show replacement diffs from the xref buffer and maybe
> from other packages as well.

Here's a counter-proposal: we were talking about a "refactoring" 
packages on emacs-devel, maybe a week ago. And I suggested a function 
that would take a list of changes (as some data) and present them using 
some customizable logic: the current Eglot's solution uses a diff, and 
I'll add an implementation that shows a tree-like buffer with 
checkmarks, probably.

I'll be starting on this any day now ;-(

So... provided this won't take too long, I would suggest your code here 
just focuses on creating a list of changes (those shouldn't require 
buffers to visit files), and then you'd be able to pass them on to 
'refact-show-changes' (name under construction), which would then use 
the interface that the user prefers.

This was we'll also consolidate the diff-generating code for features of 
this sort.

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> As discussed on emacs-devel, here is the patch that implements
>> a standalone command that reads a list of files and replacement strings,
>> then shows a diff to review before applying replacements.
>> Also provided the Dired integration to show the replacement diff
>> on marked files.  Later the same function could be used
>> to show replacement diffs from the xref buffer and maybe
>> from other packages as well.
>
> Here's a counter-proposal: we were talking about a "refactoring" packages
> on emacs-devel, maybe a week ago. And I suggested a function that would
> take a list of changes (as some data) and present them using some
> customizable logic: the current Eglot's solution uses a diff, and I'll add
> an implementation that shows a tree-like buffer with checkmarks, probably.
>
> I'll be starting on this any day now ;-(
>
> So... provided this won't take too long, I would suggest your code here
> just focuses on creating a list of changes (those shouldn't require buffers
> to visit files), and then you'd be able to pass them on to
> 'refact-show-changes' (name under construction), which would then use the
> interface that the user prefers.
>
> This was we'll also consolidate the diff-generating code for features of
> this sort.

I'm not sure this complication is necessary.  The proposed patch
does its job already.  So more generalizations could be added later.

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> >> >> +                                                    However, when the file
>> >> >> +is not visited in a buffer, or the buffer is not modified, still read
>> >> >> +contents from the file."
>> >> >
>> >> > Seems to describe an implementation detail, and I don't think it
>> >> > should be there.  E.g., what if the file visited by the buffer no
>> >> > longer exists?
>> >>
>> >> If the file visited by the buffer no longer exists, then
>> >> the standard error is signaled.
>> >
>> > Which means in that case it is better to use the buffer text, no?
>>
>> Since replacement diffs are not supported in non-file buffers,
>> better to signal an error for heads up.
>
> But it _is_ a file-visiting buffer.  It's just that its file was
> deleted meanwhile.

The generated diff could not be applied to the deleted file.
So generating a diff to the deleted file makes no sense anyway.

bug#65854: Multi-file replacement diff

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: me@eshelyaron.com,  65854@debbugs.gnu.org
> Date: Sun, 24 Sep 2023 10:37:54 +0300
> 
> >> >> >> +                                                    However, when the file
> >> >> >> +is not visited in a buffer, or the buffer is not modified, still read
> >> >> >> +contents from the file."
> >> >> >
> >> >> > Seems to describe an implementation detail, and I don't think it
> >> >> > should be there.  E.g., what if the file visited by the buffer no
> >> >> > longer exists?
> >> >>
> >> >> If the file visited by the buffer no longer exists, then
> >> >> the standard error is signaled.
> >> >
> >> > Which means in that case it is better to use the buffer text, no?
> >>
> >> Since replacement diffs are not supported in non-file buffers,
> >> better to signal an error for heads up.
> >
> > But it _is_ a file-visiting buffer.  It's just that its file was
> > deleted meanwhile.
> 
> The generated diff could not be applied to the deleted file.
> So generating a diff to the deleted file makes no sense anyway.

I suggest not to second-guess what the user wants to do with the
generated diffs.  What if they just want to email it or something?

The basic rule of the least surprise is pertinent here: we have the
data, so why not generate the diffs when we can?

But if you feel strongly about signaling an error in that case, I
won't object.

bug#65854: Multi-file replacement diff

[Dmitry Gutov]

On 24/09/2023 10:36, Juri Linkov wrote:
>>> As discussed on emacs-devel, here is the patch that implements
>>> a standalone command that reads a list of files and replacement strings,
>>> then shows a diff to review before applying replacements.
>>> Also provided the Dired integration to show the replacement diff
>>> on marked files.  Later the same function could be used
>>> to show replacement diffs from the xref buffer and maybe
>>> from other packages as well.
>> Here's a counter-proposal: we were talking about a "refactoring" packages
>> on emacs-devel, maybe a week ago. And I suggested a function that would
>> take a list of changes (as some data) and present them using some
>> customizable logic: the current Eglot's solution uses a diff, and I'll add
>> an implementation that shows a tree-like buffer with checkmarks, probably.
>>
>> I'll be starting on this any day now ;-(
>>
>> So... provided this won't take too long, I would suggest your code here
>> just focuses on creating a list of changes (those shouldn't require buffers
>> to visit files), and then you'd be able to pass them on to
>> 'refact-show-changes' (name under construction), which would then use the
>> interface that the user prefers.
>>
>> This was we'll also consolidate the diff-generating code for features of
>> this sort.
> I'm not sure this complication is necessary.  The proposed patch
> does its job already.  So more generalizations could be added later.

If you are sure.

I just wouldn't want to keep unnecessary defcustoms around.

bug#65854: Multi-file replacement diff

[Juri Linkov]

> On 24/09/2023 10:36, Juri Linkov wrote:
>>>> As discussed on emacs-devel, here is the patch that implements
>>>> a standalone command that reads a list of files and replacement strings,
>>>> then shows a diff to review before applying replacements.
>>>> Also provided the Dired integration to show the replacement diff
>>>> on marked files.  Later the same function could be used
>>>> to show replacement diffs from the xref buffer and maybe
>>>> from other packages as well.
>>> Here's a counter-proposal: we were talking about a "refactoring" packages
>>> on emacs-devel, maybe a week ago. And I suggested a function that would
>>> take a list of changes (as some data) and present them using some
>>> customizable logic: the current Eglot's solution uses a diff, and I'll add
>>> an implementation that shows a tree-like buffer with checkmarks, probably.
>>>
>>> I'll be starting on this any day now ;-(
>>>
>>> So... provided this won't take too long, I would suggest your code here
>>> just focuses on creating a list of changes (those shouldn't require buffers
>>> to visit files), and then you'd be able to pass them on to
>>> 'refact-show-changes' (name under construction), which would then use the
>>> interface that the user prefers.
>>>
>>> This was we'll also consolidate the diff-generating code for features of
>>> this sort.
>> I'm not sure this complication is necessary.  The proposed patch
>> does its job already.  So more generalizations could be added later.
>
> If you are sure.
>
> I just wouldn't want to keep unnecessary defcustoms around.

Actually my point was that there is already eglot--propose-changes-as-diff.
And now with addition of multi-file-replace-as-diff you will have two cases
to generalize that would be simpler to do than with only one case.

bug#65854: Multi-file replacement diff

[Juri Linkov]

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

>> >> >> >> +                                                    However, when the file
>> >> >> >> +is not visited in a buffer, or the buffer is not modified, still read
>> >> >> >> +contents from the file."
>> >> >> >
>> >> >> > Seems to describe an implementation detail, and I don't think it
>> >> >> > should be there.  E.g., what if the file visited by the buffer no
>> >> >> > longer exists?
>> >> >>
>> >> >> If the file visited by the buffer no longer exists, then
>> >> >> the standard error is signaled.
>> >> >
>> >> > Which means in that case it is better to use the buffer text, no?
>> >>
>> >> Since replacement diffs are not supported in non-file buffers,
>> >> better to signal an error for heads up.
>> >
>> > But it _is_ a file-visiting buffer.  It's just that its file was
>> > deleted meanwhile.
>>
>> The generated diff could not be applied to the deleted file.
>> So generating a diff to the deleted file makes no sense anyway.
>
> I suggest not to second-guess what the user wants to do with the
> generated diffs.  What if they just want to email it or something?
>
> The basic rule of the least surprise is pertinent here: we have the
> data, so why not generate the diffs when we can?
>
> But if you feel strongly about signaling an error in that case, I
> won't object.

I don't disagree.  My only concern was extra complexity and
performance of file-exists-p for such rare case.  But if this
is not a problem, here is a patch over the previous patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: multi-file-replace-as-diff.patch --]
[-- Type: text/x-diff, Size: 1606 bytes --]

diff --git a/lisp/misearch.el b/lisp/misearch.el
index 25d1b115af8..06bb6f57777 100644
--- a/lisp/misearch.el
+++ b/lisp/misearch.el
@@ -427,17 +427,24 @@ multi-file-replace-as-diff
       (setq buffer-read-only t)
       (diff-mode))
     (dolist (file-name files)
-      (let ((file-buffer (when (eq multi-file-diff-unsaved 'use-modified-buffer)
-                           (find-buffer-visiting file-name))))
+      (let* ((file-exists (file-exists-p file-name))
+             (file-buffer
+              (when (or (not file-exists)
+                        (eq multi-file-diff-unsaved 'use-modified-buffer))
+                (find-buffer-visiting file-name))))
         (when file-name
           (with-temp-buffer
-            (if (and file-buffer (buffer-modified-p file-buffer))
+            (if (and file-buffer
+                     (or (not file-exists)
+                         (buffer-modified-p file-buffer)))
                 (insert-buffer-substring file-buffer)
               (insert-file-contents file-name))
             (goto-char (point-min))
             (perform-replace from-string replacements nil regexp-flag delimited-flag)
-            (multi-file-diff-no-select file-name (current-buffer) nil diff-buffer
-                                       (concat file-name "~") file-name)))))
+            (multi-file-diff-no-select
+             (if file-exists file-name file-buffer)
+             (current-buffer) nil diff-buffer
+             (concat file-name "~") file-name)))))
     (with-current-buffer diff-buffer
       (diff-setup-whitespace)
       (diff-setup-buffer-type)

bug#65854: Multi-file replacement diff

[Dmitry Gutov]

On 25/09/2023 20:58, Juri Linkov wrote:
>> On 24/09/2023 10:36, Juri Linkov wrote:
>>>>> As discussed on emacs-devel, here is the patch that implements
>>>>> a standalone command that reads a list of files and replacement strings,
>>>>> then shows a diff to review before applying replacements.
>>>>> Also provided the Dired integration to show the replacement diff
>>>>> on marked files.  Later the same function could be used
>>>>> to show replacement diffs from the xref buffer and maybe
>>>>> from other packages as well.
>>>> Here's a counter-proposal: we were talking about a "refactoring" packages
>>>> on emacs-devel, maybe a week ago. And I suggested a function that would
>>>> take a list of changes (as some data) and present them using some
>>>> customizable logic: the current Eglot's solution uses a diff, and I'll add
>>>> an implementation that shows a tree-like buffer with checkmarks, probably.
>>>>
>>>> I'll be starting on this any day now ;-(
>>>>
>>>> So... provided this won't take too long, I would suggest your code here
>>>> just focuses on creating a list of changes (those shouldn't require buffers
>>>> to visit files), and then you'd be able to pass them on to
>>>> 'refact-show-changes' (name under construction), which would then use the
>>>> interface that the user prefers.
>>>>
>>>> This was we'll also consolidate the diff-generating code for features of
>>>> this sort.
>>> I'm not sure this complication is necessary.  The proposed patch
>>> does its job already.  So more generalizations could be added later.
>> If you are sure.
>>
>> I just wouldn't want to keep unnecessary defcustoms around.
> Actually my point was that there is already eglot--propose-changes-as-diff.
> And now with addition of multi-file-replace-as-diff you will have two cases
> to generalize that would be simpler to do than with only one case.

Yes, that should help. Even having your patch in the bug tracker to 
refer to already helps (as well as the discussion around it).

I'm just saying that if Eglot has its own existing custom vars, and 
misearch.el has its own, it will take extra effort to unify them (or 
keep extra options inside said packages, I guess, increasing unavoidable 
duplication).

bug#65854: Multi-file replacement diff

[Juri Linkov]

close 65854 30.0.50
thanks

> I'm just saying that if Eglot has its own existing custom vars, and
> misearch.el has its own, it will take extra effort to unify them (or keep
> extra options inside said packages, I guess, increasing unavoidable
> duplication).

It's not a problem to unify custom vars until the next release.

So the current version is pushed to master.

bug#65854: Multi-file replacement diff

[Juri Linkov]

>> Actually my point was that there is already eglot--propose-changes-as-diff.
>> And now with addition of multi-file-replace-as-diff you will have two cases
>> to generalize that would be simpler to do than with only one case.
>
> Yes, that should help. Even having your patch in the bug tracker to refer
> to already helps (as well as the discussion around it).

And adding a third implementation could help more ;-)
What is still missing is doing replacements as diffs
from xref buffers - I discovered this is much needed
as poor-man's renamings in projects without eglot support.