bug#60338: [PATCH] Add option to present server changes as diffs

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

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

X-CC-Debbugs: João Távora <joaotavora@gmail.com>

I'd like to propose adding an option that makes server modifications by
Eglot less invasive.  The current behaviour is to make the changes
directly in a buffer and open the remaining files to make the
modifications in those as well (?).  If `eglot-use-diffs' is enabled,
all confirmations are prepared as patches in a pop-up buffer that the
user can review and apply at will.  To my knowledge there is no general
`diff-apply-hunk' that will apply all the changes from a buffer, but
that is a separate issue that can be fixed in a separate patch.

(Note, I'm still testing emacs-29, so the patch was developed on that
branch.  But it should be applied to master)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Add option to present server changes as diffs --]
[-- Type: text/x-patch, Size: 4834 bytes --]

From 88ba620b47b7987d203fc5b42716fa08be3b1d8f Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Mon, 26 Dec 2022 11:06:09 +0100
Subject: [PATCH] Add option to present server changes as diffs

* lisp/progmodes/eglot.el (eglot-use-diffs): Add new user option.
(eglot--apply-workspace-edit): Respect 'eglot-use-diffs'.
* doc/misc/eglot.texi (Customizing Eglot): Document 'eglot-use-diffs'.
---
 doc/misc/eglot.texi     |  7 ++++++
 lisp/progmodes/eglot.el | 54 ++++++++++++++++++++++++++++++++---------
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/doc/misc/eglot.texi b/doc/misc/eglot.texi
index 2aff038b9a..9150e1a879 100644
--- a/doc/misc/eglot.texi
+++ b/doc/misc/eglot.texi
@@ -955,6 +955,13 @@ Customizing Eglot
 to use Eglot in your @code{eglot-managed-mode-hook} or via some other
 mechanism.
 
+@vindex eglot-use-diffs
+@item eglot-use-diffs
+If this option is enabled, any server modifications (renames, code
+actions, refactoring, @dots{}) will be presented as diffs that the
+user can selectively apply.  The default is that all modifications are
+made in place.
+
 @vindex eglot-report-progress
 @cindex progress
 @item eglot-report-progress
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 15cb1b6fad..74472b25ce 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -111,6 +111,8 @@
 (require 'ert)
 (require 'array)
 (require 'external-completion)
+(require 'diff-mode)
+(require 'diff)
 
 ;; ElDoc is preloaded in Emacs, so `require'-ing won't guarantee we are
 ;; using the latest version from GNU Elpa when we load eglot.el.  Use an
@@ -3160,6 +3162,11 @@ eglot--apply-text-edits
       (undo-amalgamate-change-group change-group)
       (progress-reporter-done reporter))))
 
+(defcustom eglot-use-diffs nil
+  "Non-nil means that server changes are presented as diffs."
+  :type 'boolean
+  :version "30.1")
+
 (defun eglot--apply-workspace-edit (wedit &optional confirm)
   "Apply the workspace edit WEDIT.  If CONFIRM, ask user first."
   (eglot--dbind ((WorkspaceEdit) changes documentChanges) wedit
@@ -3175,18 +3182,41 @@ eglot--apply-workspace-edit
         ;; prefer documentChanges over changes.
         (cl-loop for (uri edits) on changes by #'cddr
                  do (push (list (eglot--uri-to-path uri) edits) prepared)))
-      (if (or confirm
-              (cl-notevery #'find-buffer-visiting
-                           (mapcar #'car prepared)))
-          (unless (y-or-n-p
-                   (format "[eglot] Server wants to edit:\n  %s\n Proceed? "
-                           (mapconcat #'identity (mapcar #'car prepared) "\n  ")))
-            (jsonrpc-error "User canceled server edit")))
-      (cl-loop for edit in prepared
-               for (path edits version) = edit
-               do (with-current-buffer (find-file-noselect path)
-                    (eglot--apply-text-edits edits version))
-               finally (eldoc) (eglot--message "Edit successful!")))))
+      (if eglot-use-diffs
+          (with-current-buffer (get-buffer-create " *Server Changes*")
+            (buffer-disable-undo (current-buffer))
+            (let ((buffer-read-only t))
+              (diff-mode))
+            (let ((inhibit-read-only t)
+                  (target (current-buffer)))
+              (erase-buffer)
+              (pcase-dolist (`(,path ,edits ,_) prepared)
+                (with-temp-buffer
+                  (let ((diff (current-buffer)))
+                    (with-temp-buffer
+                      (insert-file-contents path)
+                      (eglot--apply-text-edits edits)
+                      (diff-no-select path (current-buffer)
+                                      nil t diff))
+                    (with-current-buffer target
+                      (insert-buffer-substring diff))))))
+            (setq-local buffer-read-only t)
+            (buffer-enable-undo (current-buffer))
+            (goto-char (point-min))
+            (pop-to-buffer (current-buffer))
+            (font-lock-ensure))
+        (if (or confirm
+                (cl-notevery #'find-buffer-visiting
+                             (mapcar #'car prepared)))
+            (unless (y-or-n-p
+                     (format "[eglot] Server wants to edit:\n  %s\n Proceed? "
+                             (mapconcat #'identity (mapcar #'car prepared) "\n  ")))
+              (jsonrpc-error "User canceled server edit")))
+        (cl-loop for edit in prepared
+                 for (path edits version) = edit
+                 do (with-current-buffer (find-file-noselect path)
+                      (eglot--apply-text-edits edits version))
+                 finally (eldoc) (eglot--message "Edit successful!"))))))
 
 (defun eglot-rename (newname)
   "Rename the current symbol to NEWNAME."
-- 
2.35.1

bug#60338: [PATCH] Add option to present server changes as diffs

[Yuan Fu]

Philip Kaludercic <philipk@posteo.net> writes:

> X-CC-Debbugs: João Távora <joaotavora@gmail.com>
>
> I'd like to propose adding an option that makes server modifications by
> Eglot less invasive.  The current behaviour is to make the changes
> directly in a buffer and open the remaining files to make the
> modifications in those as well (?).  If `eglot-use-diffs' is enabled,
> all confirmations are prepared as patches in a pop-up buffer that the
> user can review and apply at will.  To my knowledge there is no general
> `diff-apply-hunk' that will apply all the changes from a buffer, but
> that is a separate issue that can be fixed in a separate patch.
>
> (Note, I'm still testing emacs-29, so the patch was developed on that
> branch.  But it should be applied to master)

This seems really nice :-)

Yuan

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

Yuan Fu <casouri@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> X-CC-Debbugs: 
>>
>> I'd like to propose adding an option that makes server modifications by
>> Eglot less invasive.  The current behaviour is to make the changes
>> directly in a buffer and open the remaining files to make the
>> modifications in those as well (?).  If `eglot-use-diffs' is enabled,
>> all confirmations are prepared as patches in a pop-up buffer that the
>> user can review and apply at will.  To my knowledge there is no general
>> `diff-apply-hunk' that will apply all the changes from a buffer, but
>> that is a separate issue that can be fixed in a separate patch.
>>
>> (Note, I'm still testing emacs-29, so the patch was developed on that
>> branch.  But it should be applied to master)
>
> This seems really nice :-)

Have you tried it out?  My worry is that there are some simple changes
that don't warrant a diff, but I don't know how these can be
distinguished.

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

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

The idea is good, but how does this play along with the existing
eglot-confirm-server-initiated-edits?

João

[-- Attachment #2: Type: text/html, Size: 182 bytes --]

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:

> The idea is good, but how does this play along with the existing
> eglot-confirm-server-initiated-edits?

It takes precedence, since the diff is regarded as a kind of prompt.  If
you want to, I can also update the patch to use that option instead of a
custom one (e.g. present a diff if
`eglot-confirm-server-initiated-edits' is set to `diff').

> João

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

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

On Thu, Dec 29, 2022 at 2:39 PM Philip Kaludercic <philipk@posteo.net>
wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > The idea is good, but how does this play along with the existing
> > eglot-confirm-server-initiated-edits?
>
> It takes precedence, since the diff is regarded as a kind of prompt.  If
> you want to, I can also update the patch to use that option instead of a
> custom one (e.g. present a diff if
> `eglot-confirm-server-initiated-edits' is set to `diff').


This is an improvement, but it's not enough, unfortunately.

The current semantics of eglot-confirm-server-initiated-edits must first be
investigated, then potentially expanded/consolidated, even before the
augmentation
with the new 'diff value.   Only then should 'diff be added.  If we do this
some other
way, we only increase inconsistency and confusion.

Here is some information to get started with the investigation.

The function responsible for applying edits, eglot--apply-workspace-edit is
called currently from 3 places:

1. eglot-rename (this ignores eglot-c-s-initiated-edits, but confirms with
a prefix
argument).  The reasoning is that this is such a common action that by
default
we shouldn't bother the user with confirmation.

2. When the user chooses a code action from a list of code actions presented
as a menu and that code action happens to contain an edit.  This also
ignores
eglot-c-s-initiated-edits.  The locus of the call is
eglot--read-execute-code-action.
The reasoning here is, I believe, that the user is already being presented
with
an interactive prompt, and presenting a second follow-up seemed too much.

3. When applying a code action that makes the server initiate a code
action.
The locus is eglot--handle-request (for workspace/applyEdit).  Here
eglot-confirm-server-initiated-edits is read honoured.  The reasoning here
is
that the user might not be aware of the breadth of the code action that the
server is about to propose.

Note that the differences between 2 and 3 are subtle, and perhaps
conceptually
non-existent, but technically they do exist.  In 2, the edit is immediately
available whereas in 3, a further server trip is required to get the edit
to apply.

Moreover, the current method of  "confirmation" is just a prompt that lists
the
files about to be changed.  This is what should change, perhaps even by
default,  to a presentation of a diff in a separate buffer.

Currently, the confirmation happens also (regardless of the value of
eglot-confirm-server-initiated-edits) if any of the files about to be
changed
is not yet visited in a buffer.

So to summarize, I would like to hear proposals on how to make
user confirmation of edits more consistent and predictable across the
various use cases.

Then, as I've already said, the "diff" method of confirmation sounds in
principle very robust  and in-line with Emacs' principles. It could
eventually
even be made the default.

João

[-- Attachment #2: Type: text/html, Size: 4092 bytes --]

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:

> On Thu, Dec 29, 2022 at 2:39 PM Philip Kaludercic <philipk@posteo.net>
> wrote:
>
>> João Távora <joaotavora@gmail.com> writes:
>>
>> > The idea is good, but how does this play along with the existing
>> > eglot-confirm-server-initiated-edits?
>>
>> It takes precedence, since the diff is regarded as a kind of prompt.  If
>> you want to, I can also update the patch to use that option instead of a
>> custom one (e.g. present a diff if
>> `eglot-confirm-server-initiated-edits' is set to `diff').
>
> This is an improvement, but it's not enough, unfortunately.
>
> The current semantics of eglot-confirm-server-initiated-edits must first be
> investigated, then potentially expanded/consolidated, even before the
> augmentation
> with the new 'diff value.   Only then should 'diff be added.  If we do this
> some other
> way, we only increase inconsistency and confusion.
>
> Here is some information to get started with the investigation.
>
> The function responsible for applying edits, eglot--apply-workspace-edit is
> called currently from 3 places:
>
> 1. eglot-rename (this ignores eglot-c-s-initiated-edits, but confirms with
> a prefix
> argument).  The reasoning is that this is such a common action that by
> default
> we shouldn't bother the user with confirmation.

This is also one of the situations where I believe that a diff is not
necessary, since the change is predictable.

> 2. When the user chooses a code action from a list of code actions presented
> as a menu and that code action happens to contain an edit.  This also
> ignores
> eglot-c-s-initiated-edits.  The locus of the call is
> eglot--read-execute-code-action.
> The reasoning here is, I believe, that the user is already being presented
> with
> an interactive prompt, and presenting a second follow-up seemed too much.
>
> 3. When applying a code action that makes the server initiate a code
> action.
> The locus is eglot--handle-request (for workspace/applyEdit).  Here
> eglot-confirm-server-initiated-edits is read honoured.  The reasoning here
> is
> that the user might not be aware of the breadth of the code action that the
> server is about to propose.
>
> Note that the differences between 2 and 3 are subtle, and perhaps
> conceptually
> non-existent, but technically they do exist.  In 2, the edit is immediately
> available whereas in 3, a further server trip is required to get the edit
> to apply.

I am afraid I can't conceptualise the difference between the two.  The
2. is probably what I am familiar with, but when does the server
initiate a code action.  Are we talking about an unprompted change?  Or
something like code formatting?

> Moreover, the current method of  "confirmation" is just a prompt that lists
> the
> files about to be changed.  This is what should change, perhaps even by
> default,  to a presentation of a diff in a separate buffer.

Would you say that this means we should always generate a diff and only
apply it when the user confirms the change, or is the cost of doubling
the communication acceptable?

> Currently, the confirmation happens also (regardless of the value of
> eglot-confirm-server-initiated-edits) if any of the files about to be
> changed
> is not yet visited in a buffer.
>
> So to summarize, I would like to hear proposals on how to make
> user confirmation of edits more consistent and predictable across the
> various use cases.
>
> Then, as I've already said, the "diff" method of confirmation sounds in
> principle very robust  and in-line with Emacs' principles. It could
> eventually
> even be made the default.

So in the sense that a diff is presented as a kind of confirmation,
along with the option to apply all the changes immediately.   All in
all, this makes me less certain that merging the change into
`eglot-confirm-server-initiated-edits' is the right approach.

> João

bug#60338: [PATCH] Add option to present server changes as diffs

[Felician Nemeth]

This is going to be a bit off-topic, but I'm guessing that users (after
a while) get used to what they can expect from a specific language
server when it comes to code-actions.  And with a well written server,
users should never want to partially apply a server initiated text-edit.

Therefore it might be a better UI to apply every text-edit without
questions, display a message when the changes are not visible ("Changed
100 lines in 10 files"), provide a command to view the last text-edit as
a diff, and allow the users to undo the change with a single undo
command.

This UI wouldn't slow down experienced users, and it would allow them to
quickly correct rare mistakes.  Eglot could also teach inexperienced
users with messages like "Changed 2 lines in 1 file.  `undo-view-last'
shows the change."

WDT?

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

(Sorry for the delay in answering, I forgot about this thread)

Felician Nemeth <felician.nemeth@gmail.com> writes:

> This is going to be a bit off-topic, but I'm guessing that users (after
> a while) get used to what they can expect from a specific language
> server when it comes to code-actions.  And with a well written server,
> users should never want to partially apply a server initiated text-edit.
>
> Therefore it might be a better UI to apply every text-edit without
> questions, display a message when the changes are not visible ("Changed
> 100 lines in 10 files"), provide a command to view the last text-edit as
> a diff, and allow the users to undo the change with a single undo
> command.
>
> This UI wouldn't slow down experienced users, and it would allow them to
> quickly correct rare mistakes.  Eglot could also teach inexperienced
> users with messages like "Changed 2 lines in 1 file.  `undo-view-last'
> shows the change."
>
> WDT?

In principle this should be ok, but there are users who for whatever
reason do not want the language server to modify the files (e.g.
because they don't like it in principle, or because the language is
/not/ well written).  I tend towards that camp, and which is why I wrote
the patch in the first place.

The other issue is that the patch would have to be generated before any
changes are applied, so that the user can inspect it after the server
made the changes, regardless of whether or not they will be interested
or not.  I am not sure how much waste this will generate, and if it is
really worth it, compared to having a diff as a "prompt".

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

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


The previous patch couldn't be applied onto master anymore, so I have
adjusted it and merged the user option into 'eglot-confirm-server-initiated-edits'.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-for-LSP-edits-to-be-generated-as-diffs.patch --]
[-- Type: text/x-diff, Size: 6524 bytes --]

From 7263d821c2d217c5748965aad57df8c0311f7e95 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 11 Jun 2023 23:18:12 +0200
Subject: [PATCH 1/2] Allow for LSP edits to be generated as diffs

* doc/misc/eglot.texi (Eglot Variables): Document the new behaviour.
* lisp/progmodes/eglot.el (eglot-confirm-server-initiated-edits): Add
new value 'diff'.
(eglot-execute): Check if 'eglot-confirm-server-initiated-edits' is
set to 'diff'.
(eglot--apply-workspace-edit): Respect the new 'diff' value.
(eglot-rename): Pass 'eglot-confirm-server-initiated-edits' instead of
just the value 'current-prefix-arg'.  (Bug#60338)
---
 doc/misc/eglot.texi     |  6 ++--
 lisp/progmodes/eglot.el | 62 +++++++++++++++++++++++++++++++----------
 2 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/doc/misc/eglot.texi b/doc/misc/eglot.texi
index 962e6c914ce..305feda43a6 100644
--- a/doc/misc/eglot.texi
+++ b/doc/misc/eglot.texi
@@ -833,9 +833,11 @@ Eglot Variables
 @item eglot-confirm-server-initiated-edits
 Various Eglot commands and code actions result in the language server
 sending editing commands to Emacs.  If this option's value is
-non-@code{nil} (the default), Eglot will ask for confirmation before
+@code{confirm} (the default), Eglot will ask for confirmation before
 performing edits initiated by the server or edits whose scope affects
-buffers other than the one where the user initiated the request.
+buffers other than the one where the user initiated the request.  If
+set to @code{diff}, Eglot will generate and display a diff to display
+the changes.
 
 @item eglot-ignored-server-capabilities
 This variable's value is a list of language server capabilities that
diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 7d5d786dea3..9c9c3152472 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -108,6 +108,7 @@
 (require 'filenotify)
 (require 'ert)
 (require 'text-property-search nil t)
+(require 'diff)
 
 ;; These dependencies are also GNU ELPA core packages.  Because of
 ;; bug#62576, since there is a risk that M-x package-install, despite
@@ -386,9 +387,14 @@ eglot-events-buffer-size
   :type '(choice (const :tag "No limit" nil)
                  (integer :tag "Number of characters")))
 
 (defcustom eglot-confirm-server-initiated-edits 'confirm
-  "Non-nil if server-initiated edits should be confirmed with user."
+  "Control how server edits are applied.
+If set to `confirm', a prompt is presented to confirm server
+changes.  If set to `diff', a buffer will pop up with changes
+that can be applied manually.  If set to nil, modifications are
+made automatically."
   :type '(choice (const :tag "Don't show confirmation prompt" nil)
+                 (const :tag "Present as diffs in a buffer" diff)
                  (const :tag "Show confirmation prompt" confirm)))
 
 (defcustom eglot-extend-to-xref nil
@@ -740,7 +746,8 @@ eglot-execute
    (eglot--dcase action
      (((Command)) (eglot--request server :workspace/executeCommand action))
      (((CodeAction) edit command)
-      (when edit (eglot--apply-workspace-edit edit))
+      (when edit
+        (eglot--apply-workspace-edit edit (eq eglot-confirm-server-initiated-edits t)))
       (when command (eglot--request server :workspace/executeCommand command))))))
 
 (cl-defgeneric eglot-initialization-options (server)
@@ -3408,18 +3415,43 @@ eglot--apply-workspace-edit
         ;; prefer documentChanges over changes.
         (cl-loop for (uri edits) on changes by #'cddr
                  do (push (list (eglot--uri-to-path uri) edits) prepared)))
-      (if (or confirm
-              (cl-notevery #'find-buffer-visiting
-                           (mapcar #'car prepared)))
-          (unless (y-or-n-p
+      (cond
+       ((eq confirm 'diff)
+        (with-current-buffer (get-buffer-create " *Changes*")
+          (buffer-disable-undo (current-buffer))
+          (let ((buffer-read-only t))
+            (diff-mode))
+          (let ((inhibit-read-only t)
+                (target (current-buffer)))
+            (erase-buffer)
+            (pcase-dolist (`(,path ,edits ,_) prepared)
+              (with-temp-buffer
+                (let ((diff (current-buffer)))
+                  (with-temp-buffer
+                    (insert-file-contents path)
+                    (eglot--apply-text-edits edits)
+                    (diff-no-select path (current-buffer)
+                                    nil t diff))
+                  (with-current-buffer target
+                    (insert-buffer-substring diff))))))
+          (setq-local buffer-read-only t)
+          (buffer-enable-undo (current-buffer))
+          (goto-char (point-min))
+          (pop-to-buffer (current-buffer))
+          (font-lock-ensure)))
+       ((and (or (eq confirm 'confirm)
+                 (cl-find-if-not #'find-buffer-visiting prepared
+                                 :key #'car))
+             ;; Should the list of buffers be popped up temporarily in
+             ;; a buffer instead of resizing the minibufffer?
+             (not (y-or-n-p
                    (format "[eglot] Server wants to edit:\n  %s\n Proceed? "
-                           (mapconcat #'identity (mapcar #'car prepared) "\n  ")))
-            (jsonrpc-error "User canceled server edit")))
-      (cl-loop for edit in prepared
-               for (path edits version) = edit
-               do (with-current-buffer (find-file-noselect path)
-                    (eglot--apply-text-edits edits version))
-               finally (eldoc) (eglot--message "Edit successful!")))))
+                           (mapconcat #'identity (mapcar #'car prepared) "\n  ")))))
+        (jsonrpc-error "User canceled server edit"))
+       ((cl-loop for (path edits version) in prepared
+                 do (with-current-buffer (find-file-noselect path)
+                      (eglot--apply-text-edits edits version))
+                 finally (eldoc) (eglot--message "Edit successful!")))))))
 
 (defun eglot-rename (newname)
   "Rename the current symbol to NEWNAME."
@@ -3434,7 +3466,7 @@ eglot-rename
    (eglot--request (eglot--current-server-or-lose)
                    :textDocument/rename `(,@(eglot--TextDocumentPositionParams)
                                           :newName ,newname))
-   current-prefix-arg))
+   (and current-prefix-arg eglot-confirm-server-initiated-edits)))
 
 (defun eglot--region-bounds ()
   "Region bounds if active, else bounds of things at point."
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 183 bytes --]


In addition to that, here is a cheap command that would allow for a
patch to be applied with a single command.  I've bound it to C-c C-x,
but perhaps there is a more intuitive key?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-Add-command-to-apply-an-entire-diff-at-once.patch --]
[-- Type: text/x-diff, Size: 1351 bytes --]

From 1cf6f09318860a8232f5850c0d1ca497d8c621f9 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 11 Jun 2023 23:30:03 +0200
Subject: [PATCH 2/2] Add command to apply an entire diff at once

* lisp/vc/diff-mode.el (diff-mode-map): Bind 'diff-apply-everything'.
(diff-apply-everything): Add new command.
---
 lisp/vc/diff-mode.el | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index d776375d681..d81f0af365e 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -216,6 +216,7 @@ diff-mode-map
   "C-x 4 A" #'diff-add-change-log-entries-other-window
   ;; Misc operations.
   "C-c C-a" #'diff-apply-hunk
+  "C-c C-x" #'diff-apply-everything
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
   "C-c C-s" #'diff-split-hunk
@@ -2033,6 +2034,16 @@ diff-apply-hunk
       (when diff-advance-after-apply-hunk
 	(diff-hunk-next))))))
 
+(defun diff-apply-everything ()
+  "Apply the entire diff."
+  (interactive)
+  (save-excursion
+    (goto-char (point-min))
+    (while (let ((inhibit-message t)
+                 (start (point)))
+             (diff-hunk-next)
+             (/= start (point)))
+      (diff-apply-hunk))))
 
 (defun diff-test-hunk (&optional reverse)
   "See whether it's possible to apply the current hunk.
-- 
2.39.2


[-- Attachment #5: Type: text/plain, Size: 774 bytes --]


Philip Kaludercic <philipk@posteo.net> writes:

> X-CC-Debbugs: 
>
> I'd like to propose adding an option that makes server modifications by
> Eglot less invasive.  The current behaviour is to make the changes
> directly in a buffer and open the remaining files to make the
> modifications in those as well (?).  If `eglot-use-diffs' is enabled,
> all confirmations are prepared as patches in a pop-up buffer that the
> user can review and apply at will.  To my knowledge there is no general
> `diff-apply-hunk' that will apply all the changes from a buffer, but
> that is a separate issue that can be fixed in a separate patch.
>
> (Note, I'm still testing emacs-29, so the patch was developed on that
> branch.  But it should be applied to master)

-- 
Philip Kaludercic

bug#60338: [PATCH] Add option to present server changes as diffs

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 11 Jun 2023 21:33:40 +0000
> 
> +       ((eq confirm 'diff)
> +        (with-current-buffer (get-buffer-create " *Changes*")
> +          (buffer-disable-undo (current-buffer))
> +          (let ((buffer-read-only t))
> +            (diff-mode))
> +          (let ((inhibit-read-only t)
> +                (target (current-buffer)))
> +            (erase-buffer)
> +            (pcase-dolist (`(,path ,edits ,_) prepared)
> +              (with-temp-buffer
> +                (let ((diff (current-buffer)))
> +                  (with-temp-buffer
> +                    (insert-file-contents path)
> +                    (eglot--apply-text-edits edits)
> +                    (diff-no-select path (current-buffer)
> +                                    nil t diff))

Isn't there a better way of presenting the edits in human-readable
form than apply them and then run Diff?  What format is used by the
server itself to send the edits?

Besides, this assumes the 'diff' command is available, which is not
portable -- one more reason not to go that way, or at leas provide an
alternative.

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sun, 11 Jun 2023 21:33:40 +0000
>> 
>> +       ((eq confirm 'diff)
>> +        (with-current-buffer (get-buffer-create " *Changes*")
>> +          (buffer-disable-undo (current-buffer))
>> +          (let ((buffer-read-only t))
>> +            (diff-mode))
>> +          (let ((inhibit-read-only t)
>> +                (target (current-buffer)))
>> +            (erase-buffer)
>> +            (pcase-dolist (`(,path ,edits ,_) prepared)
>> +              (with-temp-buffer
>> +                (let ((diff (current-buffer)))
>> +                  (with-temp-buffer
>> +                    (insert-file-contents path)
>> +                    (eglot--apply-text-edits edits)
>> +                    (diff-no-select path (current-buffer)
>> +                                    nil t diff))
>
> Isn't there a better way of presenting the edits in human-readable
> form than apply them and then run Diff?  What format is used by the
> server itself to send the edits?

The server sends a JSON message.  Here is an example from a little toy
project of mine in C, where I intentionally uncommented a #include
directive:

--8<---------------cut here---------------start------------->8---
(:id 51 :jsonrpc "2.0" :result
     [(:diagnostics
       [(:code "-Wimplicit-function-declaration" :message "Implicitly declaring library function 'strcmp' with type 'int (const char *, const char *)' (fix available)" :range
	       (:end
		(:character 21 :line 63)
		:start
		(:character 15 :line 63))
	       :severity 2 :source "clang")]
       :edit
       (:changes
	(:file:///home/philip/Source/sgo/sgo.c
	 [(:newText "#include <string.h>\n" :range
		    (:end
		     (:character 0 :line 25)
		     :start
		     (:character 0 :line 25)))]))
       :isPreferred t :kind "quickfix" :title "Include <string.h> for symbol strcmp")])
--8<---------------cut here---------------end--------------->8---

The server gives me a diagnostic and suggests a change.  The change is
just a replacement of a text range with a string:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEdit

> Besides, this assumes the 'diff' command is available, which is not
> portable -- one more reason not to go that way, or at leas provide an
> alternative.

I am personally fond of this approach, because I get a regular text
buffer that I can store or send someone.  The fact that diff is not
available everywhere (even though I would guess any system with a
language server /should/ have diff installed), is an argument against
enabling this behaviour by default, not against providing this interface.

-- 
Philip Kaludercic

bug#60338: [PATCH] Add option to present server changes as diffs

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 60338@debbugs.gnu.org,  joaotavora@gmail.com
> Date: Mon, 12 Jun 2023 12:35:09 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Isn't there a better way of presenting the edits in human-readable
> > form than apply them and then run Diff?  What format is used by the
> > server itself to send the edits?
> 
> The server sends a JSON message.  Here is an example from a little toy
> project of mine in C, where I intentionally uncommented a #include
> directive:

And we cannot generate the Diff format from this?

> > Besides, this assumes the 'diff' command is available, which is not
> > portable -- one more reason not to go that way, or at leas provide an
> > alternative.
> 
> I am personally fond of this approach, because I get a regular text
> buffer that I can store or send someone.  The fact that diff is not
> available everywhere (even though I would guess any system with a
> language server /should/ have diff installed), is an argument against
> enabling this behaviour by default, not against providing this interface.

What do we suggest to people who don't have Diff, though?  Nothing?

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 60338@debbugs.gnu.org,  joaotavora@gmail.com
>> Date: Mon, 12 Jun 2023 12:35:09 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Isn't there a better way of presenting the edits in human-readable
>> > form than apply them and then run Diff?  What format is used by the
>> > server itself to send the edits?
>> 
>> The server sends a JSON message.  Here is an example from a little toy
>> project of mine in C, where I intentionally uncommented a #include
>> directive:
>
> And we cannot generate the Diff format from this?

Certainly we /could/, but I don't think that would be worth the effort
to generate a proper diff manually.

>> > Besides, this assumes the 'diff' command is available, which is not
>> > portable -- one more reason not to go that way, or at leas provide an
>> > alternative.
>> 
>> I am personally fond of this approach, because I get a regular text
>> buffer that I can store or send someone.  The fact that diff is not
>> available everywhere (even though I would guess any system with a
>> language server /should/ have diff installed), is an argument against
>> enabling this behaviour by default, not against providing this interface.
>
> What do we suggest to people who don't have Diff, though?  Nothing?

They still have two different options for
`eglot-confirm-server-initiated-edits', 'confirm (the default) and nil.
This is the current state.

-- 
Philip Kaludercic

bug#60338: [PATCH] Add option to present server changes as diffs

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 60338@debbugs.gnu.org,  joaotavora@gmail.com
> Date: Mon, 12 Jun 2023 13:29:21 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Philip Kaludercic <philipk@posteo.net>
> >> Cc: 60338@debbugs.gnu.org,  joaotavora@gmail.com
> >> Date: Mon, 12 Jun 2023 12:35:09 +0000
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > Isn't there a better way of presenting the edits in human-readable
> >> > form than apply them and then run Diff?  What format is used by the
> >> > server itself to send the edits?
> >> 
> >> The server sends a JSON message.  Here is an example from a little toy
> >> project of mine in C, where I intentionally uncommented a #include
> >> directive:
> >
> > And we cannot generate the Diff format from this?
> 
> Certainly we /could/, but I don't think that would be worth the effort
> to generate a proper diff manually.

It doesn't necessarily have to be the full-fledged diffs, it could be
something approximate.  After all, this is for human consumption.

> > What do we suggest to people who don't have Diff, though?  Nothing?
> 
> They still have two different options for
> `eglot-confirm-server-initiated-edits', 'confirm (the default) and nil.
> This is the current state.

IOW, we offer them nothing for this feature.

Look, I cannot force you to do anything else, if you don't feel like
it, but I don't like this solution, and think we should try harder.  I
urge you to try to find some way of presenting the edits in
human-readable form that doesn't need running Diff.

Thanks in advance.

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 60338@debbugs.gnu.org,  joaotavora@gmail.com
>> Date: Mon, 12 Jun 2023 13:29:21 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Philip Kaludercic <philipk@posteo.net>
>> >> Cc: 60338@debbugs.gnu.org,  joaotavora@gmail.com
>> >> Date: Mon, 12 Jun 2023 12:35:09 +0000
>> >> 
>> >> Eli Zaretskii <eliz@gnu.org> writes:
>> >> 
>> >> > Isn't there a better way of presenting the edits in human-readable
>> >> > form than apply them and then run Diff?  What format is used by the
>> >> > server itself to send the edits?
>> >> 
>> >> The server sends a JSON message.  Here is an example from a little toy
>> >> project of mine in C, where I intentionally uncommented a #include
>> >> directive:
>> >
>> > And we cannot generate the Diff format from this?
>> 
>> Certainly we /could/, but I don't think that would be worth the effort
>> to generate a proper diff manually.
>
> It doesn't necessarily have to be the full-fledged diffs, it could be
> something approximate.  After all, this is for human consumption.

Not really, the point is that you can apply these diffs directly.  Also,
I would not say that diffs are not for human consumption.

>> > What do we suggest to people who don't have Diff, though?  Nothing?
>> 
>> They still have two different options for
>> `eglot-confirm-server-initiated-edits', 'confirm (the default) and nil.
>> This is the current state.
>
> IOW, we offer them nothing for this feature.

In the sense of a preview?  Yes, but I would still argue, that that
would be a different feature.  I can take a look at that as well, but I
know of at least a few people who would be explicitly interested in this
kind of a user interface.

> Look, I cannot force you to do anything else, if you don't feel like
> it, but I don't like this solution, and think we should try harder.  I
> urge you to try to find some way of presenting the edits in
> human-readable form that doesn't need running Diff.

I respect your perspective, so I don't want to insist on anything
against your or Joao's will.

> Thanks in advance.

bug#60338: [PATCH] Add option to present server changes as diffs

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

FWIW I'd definitely want to use this feature when it lands.

>> It doesn't necessarily have to be the full-fledged diffs, it could be
>> something approximate.  After all, this is for human consumption.
>
> Not really, the point is that you can apply these diffs directly.  Also,
> I would not say that diffs are not for human consumption.

I agree, it'd be nice to get a proper diff buffer with all the benefits
of `diff-mode`.

I also see how it would be useful for Emacs to be able to generate such
a diff buffer directly from the change description that the LSP server
provides, without applying the change and running the diff program, but
it doesn't seem quite trivial so I hope that can be implemented as an
enhancement down the line.  (I'd be happy to help with that, BTW.)


-- 
Best,

Eshel

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

Eshel Yaron <me@eshelyaron.com> writes:

> FWIW I'd definitely want to use this feature when it lands.
>
>>> It doesn't necessarily have to be the full-fledged diffs, it could be
>>> something approximate.  After all, this is for human consumption.
>>
>> Not really, the point is that you can apply these diffs directly.  Also,
>> I would not say that diffs are not for human consumption.
>
> I agree, it'd be nice to get a proper diff buffer with all the benefits
> of `diff-mode`.

Right.

> I also see how it would be useful for Emacs to be able to generate such
> a diff buffer directly from the change description that the LSP server
> provides, without applying the change and running the diff program, but
> it doesn't seem quite trivial so I hope that can be implemented as an
> enhancement down the line.  (I'd be happy to help with that, BTW.)

Yes, and we were to do this, I'd rather add it to a pure-elisp `diff' to
diff.el so that it can be transparently reused anywhere where the "diff"
program is missing.

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

João, do you have anything to add to this discussion?

Philip Kaludercic <philipk@posteo.net> writes:

> Eshel Yaron <me@eshelyaron.com> writes:
>
>> FWIW I'd definitely want to use this feature when it lands.
>>
>>>> It doesn't necessarily have to be the full-fledged diffs, it could be
>>>> something approximate.  After all, this is for human consumption.
>>>
>>> Not really, the point is that you can apply these diffs directly.  Also,
>>> I would not say that diffs are not for human consumption.
>>
>> I agree, it'd be nice to get a proper diff buffer with all the benefits
>> of `diff-mode`.
>
> Right.
>
>> I also see how it would be useful for Emacs to be able to generate such
>> a diff buffer directly from the change description that the LSP server
>> provides, without applying the change and running the diff program, but
>> it doesn't seem quite trivial so I hope that can be implemented as an
>> enhancement down the line.  (I'd be happy to help with that, BTW.)
>
> Yes, and we were to do this, I'd rather add it to a pure-elisp `diff' to
> diff.el so that it can be transparently reused anywhere where the "diff"
> program is missing.

-- 
Philip Kaludercic

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

Philip Kaludercic <philipk@posteo.net> writes:

> João, do you have anything to add to this discussion?

Yes, a bit.

To the "should it use diff" objection, I think it's OK to keep it as is.
Especially if we frame this new 'diff' value to Eglot's
eglot-confirm-server-initiated-edits as a "Present changes as 'diff',
and user takes it from there".  I'd say most programmers can read diffs
and apply them by hand if required.

Anyway this usage of the entirely separate 'diff-mode' is the strongest
point of this feature anyway, it keeps the concerns "propose changes"
and "apply changes" neatly separated.

I also think the second patch adding 'diff-apply-everything' is fine.

Now I've tried your patch very briefly and I like it, in general.  But I
have a few comments after my sig.

João

> @@ -108,6 +108,7 @@
>  (require 'filenotify)
>  (require 'ert)
>  (require 'text-property-search nil t)
> +(require 'diff)
>  
>  ;; These dependencies are also GNU ELPA core packages.  Because of
>  ;; bug#62576, since there is a risk that M-x package-install, despite
> @@ -387,8 +388,13 @@ eglot-events-buffer-size
>                   (integer :tag "Number of characters")))
>  
>  (defcustom eglot-confirm-server-initiated-edits 'confirm
> -  "Non-nil if server-initiated edits should be confirmed with user."
> +  "Control how server edits are applied.
> +If set to `confirm', a prompt is presented to confirm server
> +changes.  If set to `diff', a buffer will pop up with changes
> +that can be applied manually.  If set to nil, modifications are
> +made automatically."
>    :type '(choice (const :tag "Don't show confirmation prompt" nil)
> +                 (const :tag "Present as diffs in a buffer" diff)
>                   (const :tag "Show confirmation prompt" confirm)))

This is the most important comment.  Later on, you add a condition

   (eq eglot-confirm-server-initiated-edits t)

Which would seem to mean "confirm unconditionally".  But 't' is not
presented as an option in the defcustom.  What meaning should it have?
Should it use a prompt or a diff?

>  
>  (defcustom eglot-extend-to-xref nil
> @@ -740,7 +746,8 @@ eglot-execute
>     (eglot--dcase action
>       (((Command)) (eglot--request server :workspace/executeCommand action))
>       (((CodeAction) edit command)
> -      (when edit (eglot--apply-workspace-edit edit))
> +      (when edit
> +        (eglot--apply-workspace-edit edit (eq eglot-confirm-server-initiated-edits t)))
>        (when command (eglot--request server :workspace/executeCommand command))))))


This is where the 't' value is used.  These types of edits are usually
automatically confirmed.  Why?  Not easy to say, but normally when the
server suggests them they are more localized.  Also by this point the
client already knows what is about to change, whereas we don't know what
diff the server will provide with the ones of "Command"-type, which
require a further round trip.

Anyway it would be very helpful to describe this in the manual and in
the docstrings.  So that a user can say: "even in those simple edits I
want a diff-style presentation of what is about to happen".

This may require another custom option of more added syntax to the
existing eglot-confirm-server-initiated-edits option.

Though I would appreciate if you could take this opportunity to address
them, you can also choose to simply not answer these tough questions.
But in this case, this hunk should be reverted

>  (cl-defgeneric eglot-initialization-options (server)
> @@ -3408,18 +3415,43 @@ eglot--apply-workspace-edit
>          ;; prefer documentChanges over changes.
>          (cl-loop for (uri edits) on changes by #'cddr
>                   do (push (list (eglot--uri-to-path uri) edits) prepared)))
> -      (if (or confirm
> -              (cl-notevery #'find-buffer-visiting
> -                           (mapcar #'car prepared)))
> -          (unless (y-or-n-p
> +      (cond
> +       ((eq confirm 'diff)
> +        (with-current-buffer (get-buffer-create " *Changes*")
> +          (buffer-disable-undo (current-buffer))
> +          (let ((buffer-read-only t))
> +            (diff-mode))

Why bind buffer-read-only to t here?  Why not re-enter diff-mode at the end
of cosntruction?

> +          (let ((inhibit-read-only t)
> +                (target (current-buffer)))
> +            (erase-buffer)
> +            (pcase-dolist (`(,path ,edits ,_) prepared)
> +              (with-temp-buffer
> +                (let ((diff (current-buffer)))
> +                  (with-temp-buffer
> +                    (insert-file-contents path)
> +                    (eglot--apply-text-edits edits)
> +                    (diff-no-select path (current-buffer)
> +                                    nil t diff))
> +                  (with-current-buffer target
> +                    (insert-buffer-substring diff))))))
> +          (setq-local buffer-read-only t)
> +          (buffer-enable-undo (current-buffer))
> +          (goto-char (point-min))
> +          (pop-to-buffer (current-buffer))

pop-to-buffer is fine, I guess, though I wonder if some users wouldn't
prefer display-buffer.

> +          (font-lock-ensure)))

I think this logic should be in a new helper function.

And the generated buffer name should be more Eglotish, and not hidden.
It should read something like "*EGLOT (<project-name>/<modes>) proposed
changes*".  Ideally the logic for generating the usual prefix "*EGLOT
(<project-name>/<modes>)" should also be extracted, so that we can
switch to something less ugly in the fugure.  In a different commit or
patch, of course.  

> -      (cl-loop for edit in prepared
> -               for (path edits version) = edit
> -               do (with-current-buffer (find-file-noselect path)
> -                    (eglot--apply-text-edits edits version))
> -               finally (eldoc) (eglot--message "Edit successful!")))))
> +                           (mapconcat #'identity (mapcar #'car prepared) "\n  ")))))
> +        (jsonrpc-error "User canceled server edit"))
> +       ((cl-loop for (path edits version) in prepared
> +                 do (with-current-buffer (find-file-noselect path)
> +                      (eglot--apply-text-edits edits version))
> +                 finally (eldoc) (eglot--message "Edit successful!")))))))

This last bit of cl-loop change, though better, doesn't seem related to
the new feature being developed.  It's not very important, but it does
complicate reading the patch.  You can always provide such improvements 

>  (defun eglot-rename (newname)
>    "Rename the current symbol to NEWNAME."
> @@ -3434,7 +3466,7 @@ eglot-rename
>     (eglot--request (eglot--current-server-or-lose)
>                     :textDocument/rename `(,@(eglot--TextDocumentPositionParams)
>                                            :newName ,newname))
> -   current-prefix-arg))
> +   (and current-prefix-arg eglot-confirm-server-initiated-edits)))

I get the idea and I like it.  Now one can preview the renames.  But if
'eglot-confirm-server-initiated-edits' is nil, now I can't use C-u to
override this decision.  Not necessarily something we need to fix in
your patch, but it should be taken in consideration with the first
comment about documentating the semantics of this variable.
  
>  (defun eglot--region-bounds ()
>    "Region bounds if active, else bounds of things at point."

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

On Sun, Jun 18, 2023 at 4:15 PM João Távora <joaotavora@gmail.com> wrote:
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
> > João, do you have anything to add to this discussion?
>
> Yes, a bit.
>
> To the "should it use diff" objection, I think it's OK to keep it as is.
> Especially if we frame this new 'diff' value to Eglot's
> eglot-confirm-server-initiated-edits as a "Present changes as 'diff',
> and user takes it from there".  I'd say most programmers can read diffs
> and apply them by hand if required.

After thinking a bit, I realized I missed a point of Eli's objection
to the implementation of this.  It's that the 'diff' program is required
just to generate the presentation, not just to apply it.

But I don't see any other better ways to summarily present changes to
files.  Ediff maybe?  But that's not so good, and it requires splitting the
window and entering a transient mode.  And I'm not sure it doesn't rely
on the diff program itself (but haven't checked).

Anyway, who works with Emacs in a programming context -- which is
what LSP is mainly for -- that also doesn't have the diff program
for patches, vc-diff, etc?

João

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

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

João Távora <joaotavora@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> João, do you have anything to add to this discussion?
>
> Yes, a bit.
>
> To the "should it use diff" objection, I think it's OK to keep it as is.
> Especially if we frame this new 'diff' value to Eglot's
> eglot-confirm-server-initiated-edits as a "Present changes as 'diff',
> and user takes it from there".  I'd say most programmers can read diffs
> and apply them by hand if required.
>
> Anyway this usage of the entirely separate 'diff-mode' is the strongest
> point of this feature anyway, it keeps the concerns "propose changes"
> and "apply changes" neatly separated.
>
> I also think the second patch adding 'diff-apply-everything' is fine.
>
> Now I've tried your patch very briefly and I like it, in general.  But I
> have a few comments after my sig.
>
> João
>
>> @@ -108,6 +108,7 @@
>>  (require 'filenotify)
>>  (require 'ert)
>>  (require 'text-property-search nil t)
>> +(require 'diff)
>>  
>>  ;; These dependencies are also GNU ELPA core packages.  Because of
>>  ;; bug#62576, since there is a risk that M-x package-install, despite
>> @@ -387,8 +388,13 @@ eglot-events-buffer-size
>>                   (integer :tag "Number of characters")))
>>  
>>  (defcustom eglot-confirm-server-initiated-edits 'confirm
>> -  "Non-nil if server-initiated edits should be confirmed with user."
>> +  "Control how server edits are applied.
>> +If set to `confirm', a prompt is presented to confirm server
>> +changes.  If set to `diff', a buffer will pop up with changes
>> +that can be applied manually.  If set to nil, modifications are
>> +made automatically."
>>    :type '(choice (const :tag "Don't show confirmation prompt" nil)
>> +                 (const :tag "Present as diffs in a buffer" diff)
>>                   (const :tag "Show confirmation prompt" confirm)))
>
> This is the most important comment.  Later on, you add a condition
>
>    (eq eglot-confirm-server-initiated-edits t)
>
> Which would seem to mean "confirm unconditionally".  But 't' is not
> presented as an option in the defcustom.  What meaning should it have?
> Should it use a prompt or a diff?

No, that was a thinko, the check has to be replaced with something else.

>>  
>>  (defcustom eglot-extend-to-xref nil
>> @@ -740,7 +746,8 @@ eglot-execute
>>     (eglot--dcase action
>>       (((Command)) (eglot--request server :workspace/executeCommand action))
>>       (((CodeAction) edit command)
>> -      (when edit (eglot--apply-workspace-edit edit))
>> +      (when edit
>> +        (eglot--apply-workspace-edit edit (eq eglot-confirm-server-initiated-edits t)))
>>        (when command (eglot--request server :workspace/executeCommand command))))))
>
>
> This is where the 't' value is used.  These types of edits are usually
> automatically confirmed.  Why?  Not easy to say, but normally when the
> server suggests them they are more localized.  Also by this point the
> client already knows what is about to change, whereas we don't know what
> diff the server will provide with the ones of "Command"-type, which
> require a further round trip.
>
> Anyway it would be very helpful to describe this in the manual and in
> the docstrings.  So that a user can say: "even in those simple edits I
> want a diff-style presentation of what is about to happen".
>
> This may require another custom option of more added syntax to the
> existing eglot-confirm-server-initiated-edits option.

> Though I would appreciate if you could take this opportunity to address
> them, you can also choose to simply not answer these tough questions.
> But in this case, this hunk should be reverted

I've gone ahead and fixed the check, to pass 'diff if
`eglot-confirm-server-initiated-edits' was set to 'diff, otherwise nil.
That seems like the most sensible option to me.

>>  (cl-defgeneric eglot-initialization-options (server)
>> @@ -3408,18 +3415,43 @@ eglot--apply-workspace-edit
>>          ;; prefer documentChanges over changes.
>>          (cl-loop for (uri edits) on changes by #'cddr
>>                   do (push (list (eglot--uri-to-path uri) edits) prepared)))
>> -      (if (or confirm
>> -              (cl-notevery #'find-buffer-visiting
>> -                           (mapcar #'car prepared)))
>> -          (unless (y-or-n-p
>> +      (cond
>> +       ((eq confirm 'diff)
>> +        (with-current-buffer (get-buffer-create " *Changes*")
>> +          (buffer-disable-undo (current-buffer))
>> +          (let ((buffer-read-only t))
>> +            (diff-mode))
>
> Why bind buffer-read-only to t here?  Why not re-enter diff-mode at the end
> of cosntruction?

I am not sure what you mean by re-enter?  The point of binding
`buffer-read-only' is just to enable `diff-mode-read-only' on enabling
the mode, but we can also set if afterwards.  That would require adding
a defvar though.

>> +          (let ((inhibit-read-only t)
>> +                (target (current-buffer)))
>> +            (erase-buffer)
>> +            (pcase-dolist (`(,path ,edits ,_) prepared)
>> +              (with-temp-buffer
>> +                (let ((diff (current-buffer)))
>> +                  (with-temp-buffer
>> +                    (insert-file-contents path)
>> +                    (eglot--apply-text-edits edits)
>> +                    (diff-no-select path (current-buffer)
>> +                                    nil t diff))
>> +                  (with-current-buffer target
>> +                    (insert-buffer-substring diff))))))
>> +          (setq-local buffer-read-only t)
>> +          (buffer-enable-undo (current-buffer))
>> +          (goto-char (point-min))
>> +          (pop-to-buffer (current-buffer))
>
> pop-to-buffer is fine, I guess, though I wonder if some users wouldn't
> prefer display-buffer.

I guess that this is a persistent disagreement between users and their
expectations.  I don't expect it to be resolved here, not at least
because I cannot put my own preferences into words.

>> +          (font-lock-ensure)))
>
> I think this logic should be in a new helper function.
>
> And the generated buffer name should be more Eglotish, and not hidden.
> It should read something like "*EGLOT (<project-name>/<modes>) proposed
> changes*".  

Oh, I was under the impression that this was just the name for
"internal" or hidden buffers, that don't interest the user directory.

>             Ideally the logic for generating the usual prefix "*EGLOT
> (<project-name>/<modes>)" should also be extracted, so that we can
> switch to something less ugly in the fugure.  In a different commit or
> patch, of course.  

Should we then keep it the way this is for now, and when this function
exists make use of it?

>> -      (cl-loop for edit in prepared
>> -               for (path edits version) = edit
>> -               do (with-current-buffer (find-file-noselect path)
>> -                    (eglot--apply-text-edits edits version))
>> -               finally (eldoc) (eglot--message "Edit successful!")))))
>> +                           (mapconcat #'identity (mapcar #'car prepared) "\n  ")))))
>> +        (jsonrpc-error "User canceled server edit"))
>> +       ((cl-loop for (path edits version) in prepared
>> +                 do (with-current-buffer (find-file-noselect path)
>> +                      (eglot--apply-text-edits edits version))
>> +                 finally (eldoc) (eglot--message "Edit successful!")))))))
>
> This last bit of cl-loop change, though better, doesn't seem related to
> the new feature being developed.  It's not very important, but it does
> complicate reading the patch.  You can always provide such improvements 

Reverted.

>>  (defun eglot-rename (newname)
>>    "Rename the current symbol to NEWNAME."
>> @@ -3434,7 +3466,7 @@ eglot-rename
>>     (eglot--request (eglot--current-server-or-lose)
>>                     :textDocument/rename `(,@(eglot--TextDocumentPositionParams)
>>                                            :newName ,newname))
>> -   current-prefix-arg))
>> +   (and current-prefix-arg eglot-confirm-server-initiated-edits)))
>
> I get the idea and I like it.  Now one can preview the renames.  But if
> 'eglot-confirm-server-initiated-edits' is nil, now I can't use C-u to
> override this decision.  Not necessarily something we need to fix in
> your patch, but it should be taken in consideration with the first
> comment about documentating the semantics of this variable.

I've modified it, see below.

>>  (defun eglot--region-bounds ()
>>    "Region bounds if active, else bounds of things at point."


[-- Attachment #2: Type: text/plain, Size: 2313 bytes --]

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 9c9c3152472..9c76d4abdef 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -747,7 +747,8 @@ eglot-execute
      (((Command)) (eglot--request server :workspace/executeCommand action))
      (((CodeAction) edit command)
       (when edit
-        (eglot--apply-workspace-edit edit (eq eglot-confirm-server-initiated-edits t)))
+        (let ((confirm (and (eq eglot-confirm-server-initiated-edits 'diff) 'diff)))
+          (eglot--apply-workspace-edit edit confirm)))
       (when command (eglot--request server :workspace/executeCommand command))))))
 
 (cl-defgeneric eglot-initialization-options (server)
@@ -3401,7 +3402,9 @@ eglot--apply-text-edits
       (progress-reporter-done reporter))))
 
 (defun eglot--apply-workspace-edit (wedit &optional confirm)
-  "Apply the workspace edit WEDIT.  If CONFIRM, ask user first."
+  "Apply the workspace edit WEDIT.
+CONFIRM should have a value as specified in the user option
+`eglot-confirm-server-initiated-edits'."
   (eglot--dbind ((WorkspaceEdit) changes documentChanges) wedit
     (let ((prepared
            (mapcar (eglot--lambda ((TextDocumentEdit) textDocument edits)
@@ -3448,7 +3451,8 @@ eglot--apply-workspace-edit
                    (format "[eglot] Server wants to edit:\n  %s\n Proceed? "
                            (mapconcat #'identity (mapcar #'car prepared) "\n  ")))))
         (jsonrpc-error "User canceled server edit"))
-       ((cl-loop for (path edits version) in prepared
+       ((cl-loop for edit in prepared
+                 for (path edits version) = edit
                  do (with-current-buffer (find-file-noselect path)
                       (eglot--apply-text-edits edits version))
                  finally (eldoc) (eglot--message "Edit successful!")))))))
@@ -3466,7 +3470,7 @@ eglot-rename
    (eglot--request (eglot--current-server-or-lose)
                    :textDocument/rename `(,@(eglot--TextDocumentPositionParams)
                                           :newName ,newname))
-   (and current-prefix-arg eglot-confirm-server-initiated-edits)))
+   (and current-prefix-arg (or eglot-confirm-server-initiated-edits 'confirm))))
 
 (defun eglot--region-bounds ()
   "Region bounds if active, else bounds of things at point."

[-- Attachment #3: Type: text/plain, Size: 23 bytes --]


-- 
Philip Kaludercic

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

Hello,

I've finally pushed this to master, after working on mostly on the user
confirmation logic.  I deprecated `eglot-confirm-server-initiated-edits'
(had crap semantics anyway) and replaced it with a shiny new
'eglot-confirm-server-edits', with a simpler name but much more powerful
(see its docstring).

I left in Philip's new implementation of the diff confirmation but I
didn't make it the default (although it's my preferred one) because it
might appeal to everyone used to the "summary" prompt.

Please have a look, give it some testing, and tell me if I missed
anything.

Thanks to Philip for the original idea.

João

bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:

> Hello,
>
> I've finally pushed this to master, after working on mostly on the user
> confirmation logic.  I deprecated `eglot-confirm-server-initiated-edits'
> (had crap semantics anyway) and replaced it with a shiny new
> 'eglot-confirm-server-edits', with a simpler name but much more powerful
> (see its docstring).
>
> I left in Philip's new implementation of the diff confirmation but I
> didn't make it the default (although it's my preferred one) because it
> might appeal to everyone used to the "summary" prompt.
>
> Please have a look, give it some testing, and tell me if I missed
> anything.
>
> Thanks to Philip for the original idea.

Thank you very much for finishing up the patch to the end.

> João

bug#60338: [PATCH] Add option to present server changes as diffs

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

João Távora <joaotavora@gmail.com> writes:

> I've finally pushed this to master, after working on mostly on the user
> confirmation logic.  I deprecated `eglot-confirm-server-initiated-edits'
> (had crap semantics anyway) and replaced it with a shiny new
> 'eglot-confirm-server-edits', with a simpler name but much more powerful
> (see its docstring).
>
[...]
>
> Please have a look, give it some testing, and tell me if I missed
> anything.

Great stuff!  One issue I came across is with unsaved buffers.
Here's a recipe:

With emacs -Q, set `eglot-confirm-server-edits` to `diff`.  In a fresh
git repository, create a new file `foo.go` and insert the following:

--8<---------------cut here---------------start------------->8---
package baz

type Foobar interface {}
--8<---------------cut here---------------end--------------->8---

Now save the file, and do `M-x go-ts-mode` followed by `M-x eglot` to
connect to an LSP server (`gopls` in my case).  Without saving the
buffer, insert `type Baz interface {}` after the last line:

--8<---------------cut here---------------start------------->8---
package baz

type Foobar interface {}
type Baz interface {}
--8<---------------cut here---------------end--------------->8---

With point on `Baz`, do `M-x eglot-rename RET Spam RET`.  Emacs displays
the following diff in *EGLOT proposed server changes*:

--8<---------------cut here---------------start------------->8---
diff -u /Users/eshelyaron/tmp/bug/foo.go /var/folders/q1/h0vdt_hx00zgbv3bpyx2vdr00000gp/T/buffer-content-sgzkbC
--- /Users/eshelyaron/tmp/bug/foo.go	2023-09-01 20:55:02
+++ /var/folders/q1/h0vdt_hx00zgbv3bpyx2vdr00000gp/T/buffer-content-sgzkbC	2023-09-01 20:55:07
@@ -1,3 +1,4 @@
 package baz
 
 type Foobar interface {}
+Spam
\ No newline at end of file

Diff finished.  Fri Sep  1 20:55:08 2023
--8<---------------cut here---------------end--------------->8---

We clearly get an incorrect patch. It seems like the patch is produced
by comparing the modified version of the buffer to the visited file,
instead of comparing it to the current buffer contents.

Another, smaller issue is that while `eglot-rename` is creating the
patch it prints messages along the lines of:

    [eglot] applying 1 edits to `*temp*-207900'

Which feels less appropriate in these settings.

Lastly, I have an improvement suggestion for the `:type` of
`eglot-confirm-server-edits` to make it nicer to deal with via Custom:

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 8d95019c3ed..afb99e4b9ca 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -413,12 +413,17 @@ eglot-confirm-server-edits
 `eglot-code-actions', `eglot-code-action-quickfix', etc.  ACTION
 is one of the symbols described above.  The value `t' for COMMAND
 is accepted and its ACTION is the default value."
-  :type '(choice (const :tag "Use diff" diff)
+  :type (let ((basic-choices
+               '((const :tag "Use diff" diff)
                  (const :tag "Summarize and prompt" summary)
                  (const :tag "Maybe use diff" maybe-diff)
                  (const :tag "Maybe summarize and prompt" maybe-summary)
-                 (const :tag "Don't confirm" nil)
-                 (alist :tag "Per-command alist")))
+                 (const :tag "Don't confirm" nil))))
+          `(choice ,@basic-choices
+                   (alist :tag "Per-command alist"
+                          :key-type (choice (function :tag "Command")
+                                            (const :tag "Default" t))
+                          :value-type (choice . ,basic-choices)))))
 
 (defcustom eglot-extend-to-xref nil
   "If non-nil, activate Eglot in cross-referenced non-project files."

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

On Fri, Sep 1, 2023 at 10:13 PM Eshel Yaron <me@eshelyaron.com> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > I've finally pushed this to master, after working on mostly on the user
> > confirmation logic.  I deprecated `eglot-confirm-server-initiated-edits'
> > (had crap semantics anyway) and replaced it with a shiny new
> > 'eglot-confirm-server-edits', with a simpler name but much more powerful
> > (see its docstring).
> >
> [...]
> >
> > Please have a look, give it some testing, and tell me if I missed
> > anything.
>
> Great stuff!  One issue I came across is with unsaved buffers.

I'm fixing that as we speak.  I'll read your recipe later, but
I'm 90% sure it's the same stuff I've been trying to fix for
most of today.

> Lastly, I have an improvement suggestion for the `:type` of
> `eglot-confirm-server-edits` to make it nicer to deal with via Custom:
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 8d95019c3ed..afb99e4b9ca 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -413,12 +413,17 @@ eglot-confirm-server-edits
>  `eglot-code-actions', `eglot-code-action-quickfix', etc.  ACTION
>  is one of the symbols described above.  The value `t' for COMMAND
>  is accepted and its ACTION is the default value."
> -  :type '(choice (const :tag "Use diff" diff)
> +  :type (let ((basic-choices
> +               '((const :tag "Use diff" diff)
>                   (const :tag "Summarize and prompt" summary)
>                   (const :tag "Maybe use diff" maybe-diff)
>                   (const :tag "Maybe summarize and prompt" maybe-summary)
> -                 (const :tag "Don't confirm" nil)
> -                 (alist :tag "Per-command alist")))
> +                 (const :tag "Don't confirm" nil))))
> +          `(choice ,@basic-choices
> +                   (alist :tag "Per-command alist"
> +                          :key-type (choice (function :tag "Command")
> +                                            (const :tag "Default" t))
> +                          :value-type (choice . ,basic-choices)))))
>
>  (defcustom eglot-extend-to-xref nil
>    "If non-nil, activate Eglot in cross-referenced non-project files."

I'll look at this bit, too.   I looks sane, and I'm glad I found a custom.el
expert to call on :-)

João

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

OK, Eshel

I think I fixed all these issues in the latest
fdf6c164efd0bb467d0d46460161c146e955a48c which I just
pushed to master.  Please have a look.

Thanks,
João

On Fri, Sep 1, 2023 at 10:13 PM Eshel Yaron <me@eshelyaron.com> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > I've finally pushed this to master, after working on mostly on the user
> > confirmation logic.  I deprecated `eglot-confirm-server-initiated-edits'
> > (had crap semantics anyway) and replaced it with a shiny new
> > 'eglot-confirm-server-edits', with a simpler name but much more powerful
> > (see its docstring).
> >
> [...]
> >
> > Please have a look, give it some testing, and tell me if I missed
> > anything.
>
> Great stuff!  One issue I came across is with unsaved buffers.
> Here's a recipe:
>
> With emacs -Q, set `eglot-confirm-server-edits` to `diff`.  In a fresh
> git repository, create a new file `foo.go` and insert the following:
>
> --8<---------------cut here---------------start------------->8---
> package baz
>
> type Foobar interface {}
> --8<---------------cut here---------------end--------------->8---
>
> Now save the file, and do `M-x go-ts-mode` followed by `M-x eglot` to
> connect to an LSP server (`gopls` in my case).  Without saving the
> buffer, insert `type Baz interface {}` after the last line:
>
> --8<---------------cut here---------------start------------->8---
> package baz
>
> type Foobar interface {}
> type Baz interface {}
> --8<---------------cut here---------------end--------------->8---
>
> With point on `Baz`, do `M-x eglot-rename RET Spam RET`.  Emacs displays
> the following diff in *EGLOT proposed server changes*:
>
> --8<---------------cut here---------------start------------->8---
> diff -u /Users/eshelyaron/tmp/bug/foo.go /var/folders/q1/h0vdt_hx00zgbv3bpyx2vdr00000gp/T/buffer-content-sgzkbC
> --- /Users/eshelyaron/tmp/bug/foo.go    2023-09-01 20:55:02
> +++ /var/folders/q1/h0vdt_hx00zgbv3bpyx2vdr00000gp/T/buffer-content-sgzkbC      2023-09-01 20:55:07
> @@ -1,3 +1,4 @@
>  package baz
>
>  type Foobar interface {}
> +Spam
> \ No newline at end of file
>
> Diff finished.  Fri Sep  1 20:55:08 2023
> --8<---------------cut here---------------end--------------->8---
>
> We clearly get an incorrect patch. It seems like the patch is produced
> by comparing the modified version of the buffer to the visited file,
> instead of comparing it to the current buffer contents.
>
> Another, smaller issue is that while `eglot-rename` is creating the
> patch it prints messages along the lines of:
>
>     [eglot] applying 1 edits to `*temp*-207900'
>
> Which feels less appropriate in these settings.
>
> Lastly, I have an improvement suggestion for the `:type` of
> `eglot-confirm-server-edits` to make it nicer to deal with via Custom:
>
> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 8d95019c3ed..afb99e4b9ca 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -413,12 +413,17 @@ eglot-confirm-server-edits
>  `eglot-code-actions', `eglot-code-action-quickfix', etc.  ACTION
>  is one of the symbols described above.  The value `t' for COMMAND
>  is accepted and its ACTION is the default value."
> -  :type '(choice (const :tag "Use diff" diff)
> +  :type (let ((basic-choices
> +               '((const :tag "Use diff" diff)
>                   (const :tag "Summarize and prompt" summary)
>                   (const :tag "Maybe use diff" maybe-diff)
>                   (const :tag "Maybe summarize and prompt" maybe-summary)
> -                 (const :tag "Don't confirm" nil)
> -                 (alist :tag "Per-command alist")))
> +                 (const :tag "Don't confirm" nil))))
> +          `(choice ,@basic-choices
> +                   (alist :tag "Per-command alist"
> +                          :key-type (choice (function :tag "Command")
> +                                            (const :tag "Default" t))
> +                          :value-type (choice . ,basic-choices)))))
>
>  (defcustom eglot-extend-to-xref nil
>    "If non-nil, activate Eglot in cross-referenced non-project files."



-- 
João Távora

bug#60338: [PATCH] Add option to present server changes as diffs

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

João Távora <joaotavora@gmail.com> writes:

> OK, Eshel
>
> I think I fixed all these issues in the latest
> fdf6c164efd0bb467d0d46460161c146e955a48c which I just
> pushed to master.  Please have a look.

Looks good, and works well too, thank you!

bug#60338: [PATCH] Add option to present server changes as diffs

[João Távora]

Eshel Yaron <me@eshelyaron.com> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> OK, Eshel
>>
>> I think I fixed all these issues in the latest
>> fdf6c164efd0bb467d0d46460161c146e955a48c which I just
>> pushed to master.  Please have a look.
>
> Looks good, and works well too, thank you!

I'm glad to hear that.  I must say that altough I like the new
functionality myself -- both the new user option and the diff view --
the current implementation of the latter leaves much to be desired.

I've pushed a further commit to simplify it, but it's complicated and
brittle.  A little change to diff.el would simplify some of it, but then
you can't easily publish those changes to Emacs < 29.

Anyway I invite everyone to have a look and try to improve it, perhaps
moving it out of Eglot into the shiny new "refactoring interface" if
those ever become a thing.

In the meantime, I'm going to close this.  Thanks everybody.

João

bug#60338: [PATCH] Add option to present server changes as diffs

[Dmitry Gutov]

On 02/09/2023 12:55, João Távora wrote:
> Anyway I invite everyone to have a look and try to improve it, perhaps
> moving it out of Eglot into the shiny new "refactoring interface" if
> those ever become a thing.

Regarding the diff approach vs. "shiny new", I've run a little 
comparison with VS Code: doing a rename of 'glyph_row' across the Emacs 
codebase (a moderately sized project) takes about ~1.44s to produce the 
diff.

VS Code's "Refactor Preview" takes about 1/3rd of that time to show. I'm 
guessing that's because it just paints whatever the language server 
returns instead of doing a bunch of process calls. If that is the case, 
one of the approaches seems to have a fundamental performance advantage.

Not to belittle the new addition, though - it's a good step for Eglot.

Two other thoughts after trying it:

- I would probably want to bind the originally proposed 
'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"), 
but that's already taken in diff-mode.

- 'git diff' has a less-frequently used option called '--word-diff' 
which could reduce the length of the diff in the case I am testing (but 
I guess diff-mode would have to be updated to support that output). And 
another way in that direction would be to reduce the size of the diff 
context (e.g. to 0).

bug#60338: [PATCH] Add option to present server changes as diffs

[Juri Linkov]

> Two other thoughts after trying it:
>
> - I would probably want to bind the originally proposed
>   'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
>   but that's already taken in diff-mode.

If there are no more intuitive keys, maybe then use the proposed by Philip
'C-c C-x'?

> - 'git diff' has a less-frequently used option called '--word-diff' which
>   could reduce the length of the diff in the case I am testing (but I guess
>   diff-mode would have to be updated to support that output). And another
>   way in that direction would be to reduce the size of the diff context
>  (e.g. to 0).

Whereas the output of '--word-diff' is closer to VS Code, but it's hard to read
for anyone accustomed to the diff unified format ;-)

BTW, it looks '--word-diff' could help Samuel in bug#61396?

bug#60338: [PATCH] Add option to present server changes as diffs

[Dmitry Gutov]

On 07/09/2023 09:28, Juri Linkov wrote:
>> Two other thoughts after trying it:
>>
>> - I would probably want to bind the originally proposed
>>    'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
>>    but that's already taken in diff-mode.
> If there are no more intuitive keys, maybe then use the proposed by Philip
> 'C-c C-x'?

IDK, seems a little dangerous given to its proximity to 'C-x C-c'.

Personally, the existing 'C-c C-c' binding seems unnecessary given that 
this command has 6 other bindings already.

If we chose to replace it, we could just start the new command with a 
prompt (Apply all changes from diff?), so it doesn't surprise anyone.

bug#60338: [PATCH] Add option to present server changes as diffs

[Dmitry Gutov]

On 07/09/2023 09:28, Juri Linkov wrote:
> BTW, it looks '--word-diff' could help Samuel in bug#61396?

Quite possible, but it'd also require changes in diff-mode, and the 
currently discussed alternative (in refine logic, I think) might be more 
localized.

Also, 'git apply' doesn't work on word diffs :(