bug#66113: Apply the entire diff buffer

bug#66113: Apply the entire diff buffer

[Juri Linkov]

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

After discussing on emacs-devel, here is a complete patch for the feature
of applying the entire diff buffer:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-apply-buffer.patch --]
[-- Type: text/x-diff, Size: 2081 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index d776375d681..4633d5896a7 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-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
   "C-c C-s" #'diff-split-hunk
@@ -2054,6 +2055,40 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
+(defun diff-apply-buffer ()
+  "Apply the diff in the entire diff buffer.
+When applying all hunks was successful, then save the changed buffers."
+  (interactive)
+  (let ((buffers nil)
+        (failures 0)
+        (diff-refine nil))
+    (save-excursion
+      (goto-char (point-min))
+      (diff-beginning-of-hunk t)
+      (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
+                          (diff-find-source-location nil nil)))
+               (cond ((and line-offset (not switched))
+                      (with-current-buffer buf
+                        (goto-char (car pos))
+                        (delete-region (car pos) (cdr pos))
+                        (insert (car dst))
+                        (when buffer-file-name
+                          (push (current-buffer) buffers))))
+                     (t (setq failures (1+ failures))))
+               (not (or (eq (prog1 (point) (diff-hunk-next)) (point))
+                        (eobp))))))
+    (setq buffers (delete-dups buffers))
+    (cond ((zerop failures)
+           (dolist (buf (reverse buffers))
+             (with-current-buffer buf
+               (save-buffer)))
+           (message "Saved %d buffers" (length buffers)))
+          (t
+           (dolist (buf buffers)
+             (with-current-buffer buf
+               (display-buffer buf)))
+           (message "%d hunks failed; no buffers saved" failures)))))
+
 (defalias 'diff-mouse-goto-source #'diff-goto-source)
 
 (defun diff-goto-source (&optional other-file event)

bug#66113: Apply the entire diff buffer

[Dmitry Gutov]

Hi Juri,

On 20/09/2023 09:46, Juri Linkov wrote:
> +          (t
> +           (dolist (buf buffers)
> +             (with-current-buffer buf
> +               (display-buffer buf)))
> +           (message "%d hunks failed; no buffers saved" failures)))))

What happens next in this case? How do you undo in the buffers that had 
the patch hunks already applied?

Any change you wanted to work on the idea of the "atomic rollback" as well?

bug#66113: Apply the entire diff buffer

[Juri Linkov]

>> +          (t
>> +           (dolist (buf buffers)
>> +             (with-current-buffer buf
>> +               (display-buffer buf)))
>> +           (message "%d hunks failed; no buffers saved" failures)))))
>
> What happens next in this case? How do you undo in the buffers that had the
> patch hunks already applied?

Manually, like in case of ediff-patch-buffer.

> Any change you wanted to work on the idea of the "atomic rollback" as well?

This would be an unreliable feature: in case of diff-apply creates a mess,
such automatic undo can create more mess, because there are many different
strategies to undo the mess such as using undo-auto-amalgamate, or
applying the reverse diff partially, doing more damage in case when
buffers were already modified before diff-apply.

But fortunately the need to undo will be extremely rare, because
when patch hunks are already applied, it reports the failure,
but doesn't modify the source buffer.  Therefore there is
nothing to undo!

bug#66113: Apply the entire diff buffer

[Dmitry Gutov]

On 22/09/2023 09:45, Juri Linkov wrote:
>>> +          (t
>>> +           (dolist (buf buffers)
>>> +             (with-current-buffer buf
>>> +               (display-buffer buf)))
>>> +           (message "%d hunks failed; no buffers saved" failures)))))
>>
>> What happens next in this case? How do you undo in the buffers that had the
>> patch hunks already applied?
> 
> Manually, like in case of ediff-patch-buffer.

I've never used this, so I don't understand.

>> Any change you wanted to work on the idea of the "atomic rollback" as well?
> 
> This would be an unreliable feature: in case of diff-apply creates a mess,
> such automatic undo can create more mess, because there are many different
> strategies to undo the mess such as using undo-auto-amalgamate, or
> applying the reverse diff partially, doing more damage in case when
> buffers were already modified before diff-apply.

How about we save the tips of buffer-undo-list, then in case the buffer 
needs reverting, basically 'undo' each of the buffers until the saved 
"tip" is reached? I don't have a quick code anippet, but that seems doable.

> But fortunately the need to undo will be extremely rare, because
> when patch hunks are already applied, it reports the failure,
> but doesn't modify the source buffer.  Therefore there is
> nothing to undo!

The problem situations is, of course, when one of the hunks (somewhere 
in the middle or near the end) fails to apply cleanly or at all.

Another approach would be to first go through the patch and check that 
all hunks apply without problems, and then, on the second pass, actually 
apply them.

bug#66113: Apply the entire diff buffer

[Juri Linkov]

>>>> +          (t
>>>> +           (dolist (buf buffers)
>>>> +             (with-current-buffer buf
>>>> +               (display-buffer buf)))
>>>> +           (message "%d hunks failed; no buffers saved" failures)))))
>>>
>>> What happens next in this case? How do you undo in the buffers that had the
>>> patch hunks already applied?
>> Manually, like in case of ediff-patch-buffer.
>
> I've never used this, so I don't understand.

ediff-patch-buffer is an example when trying to roll back a mess
creates more mess with .orig/.rej files.

>>> Any change you wanted to work on the idea of the "atomic rollback" as well?
>> This would be an unreliable feature: in case of diff-apply creates
>> a mess,
>> such automatic undo can create more mess, because there are many different
>> strategies to undo the mess such as using undo-auto-amalgamate, or
>> applying the reverse diff partially, doing more damage in case when
>> buffers were already modified before diff-apply.
>
> How about we save the tips of buffer-undo-list, then in case the buffer
> needs reverting, basically 'undo' each of the buffers until the saved "tip"
> is reached? I don't have a quick code anippet, but that seems doable.

Maybe, but I definitely won't use such an option.

>> But fortunately the need to undo will be extremely rare, because
>> when patch hunks are already applied, it reports the failure,
>> but doesn't modify the source buffer.  Therefore there is
>> nothing to undo!
>
> The problem situations is, of course, when one of the hunks (somewhere in
> the middle or near the end) fails to apply cleanly or at all.

Indeed, this is a problematic case when one of the hunks fails
whereas all other hunks are applied cleanly.  This sometimes happens
when receiving an external patch, but the source already changed.

> Another approach would be to first go through the patch and check that all
> hunks apply without problems, and then, on the second pass, actually apply
> them.

This is a better option, thanks for the idea, will try to do this with
'diff-test-hunk' in a loop.

bug#66113: Apply the entire diff buffer

[Juri Linkov]

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

>> Another approach would be to first go through the patch and check that all
>> hunks apply without problems, and then, on the second pass, actually apply
>> them.
>
> This is a better option, thanks for the idea, will try to do this with
> 'diff-test-hunk' in a loop.

Indeed, this is much better:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-apply-buffer.patch --]
[-- Type: text/x-diff, Size: 2071 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index d776375d681..2b0daabb12c 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-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
   "C-c C-s" #'diff-split-hunk
@@ -2054,6 +2055,39 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
+(defun diff-apply-buffer ()
+  "Apply the diff in the entire diff buffer.
+When applying all hunks was successful, then save the changed buffers."
+  (interactive)
+  (let ((buffer-edits nil)
+        (failures 0)
+        (diff-refine nil))
+    (save-excursion
+      (goto-char (point-min))
+      (diff-beginning-of-hunk t)
+      (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
+                          (diff-find-source-location nil nil)))
+               (cond ((and line-offset (not switched))
+                      (push (cons pos dst)
+                            (alist-get buf buffer-edits)))
+                     (t (setq failures (1+ failures))))
+               (not (or (eq (prog1 (point) (diff-hunk-next)) (point))
+                        (eobp))))))
+    (cond ((zerop failures)
+           (dolist (buf-edits (reverse buffer-edits))
+             (with-current-buffer (car buf-edits)
+               (dolist (edit (cdr buf-edits))
+                 (let ((pos (car edit))
+                       (dst (cdr edit))
+                       (inhibit-read-only t))
+                   (goto-char (car pos))
+                   (delete-region (car pos) (cdr pos))
+                   (insert (car dst))))
+               (save-buffer)))
+           (message "Saved %d buffers" (length buffer-edits)))
+          (t
+           (message "%d hunks failed; no buffers changed" failures)))))
+
 (defalias 'diff-mouse-goto-source #'diff-goto-source)
 
 (defun diff-goto-source (&optional other-file event)

bug#66113: Apply the entire diff buffer

[Dmitry Gutov]

On 23/09/2023 20:52, Juri Linkov wrote:
> +(defun diff-apply-buffer ()
> +  "Apply the diff in the entire diff buffer.
> +When applying all hunks was successful, then save the changed buffers."
> +  (interactive)
> +  (let ((buffer-edits nil)
> +        (failures 0)
> +        (diff-refine nil))
> +    (save-excursion
> +      (goto-char (point-min))
> +      (diff-beginning-of-hunk t)
> +      (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
> +                          (diff-find-source-location nil nil)))
> +               (cond ((and line-offset (not switched))
> +                      (push (cons pos dst)
> +                            (alist-get buf buffer-edits)))
> +                     (t (setq failures (1+ failures))))
> +               (not (or (eq (prog1 (point) (diff-hunk-next)) (point))
> +                        (eobp))))))
> +    (cond ((zerop failures)
> +           (dolist (buf-edits (reverse buffer-edits))
> +             (with-current-buffer (car buf-edits)
> +               (dolist (edit (cdr buf-edits))
> +                 (let ((pos (car edit))
> +                       (dst (cdr edit))
> +                       (inhibit-read-only t))
> +                   (goto-char (car pos))
> +                   (delete-region (car pos) (cdr pos))
> +                   (insert (car dst))))
> +               (save-buffer)))
> +           (message "Saved %d buffers" (length buffer-edits)))
> +          (t
> +           (message "%d hunks failed; no buffers changed" failures)))))

Sorry, was there supposed to be a call to diff-test-hunk here? Or I'm 
just not following the implementation.

I tried testing it out too. There is a patch where the third hunk 
doesn't apply (errors with "can't find the text to patch" in regular 
usage). This command ends with cryptic "No next hunk".

bug#66113: Apply the entire diff buffer

[Juri Linkov]

>> +(defun diff-apply-buffer ()
>> +  "Apply the diff in the entire diff buffer.
>> +When applying all hunks was successful, then save the changed buffers."
>> +  (interactive)
>> +  (let ((buffer-edits nil)
>> +        (failures 0)
>> +        (diff-refine nil))
>> +    (save-excursion
>> +      (goto-char (point-min))
>> +      (diff-beginning-of-hunk t)
>> +      (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
>> +                          (diff-find-source-location nil nil)))
>> +               (cond ((and line-offset (not switched))
>> +                      (push (cons pos dst)
>> +                            (alist-get buf buffer-edits)))
>> +                     (t (setq failures (1+ failures))))
>> +               (not (or (eq (prog1 (point) (diff-hunk-next)) (point))
>> +                        (eobp))))))
>> +    (cond ((zerop failures)
>> +           (dolist (buf-edits (reverse buffer-edits))
>> +             (with-current-buffer (car buf-edits)
>> +               (dolist (edit (cdr buf-edits))
>> +                 (let ((pos (car edit))
>> +                       (dst (cdr edit))
>> +                       (inhibit-read-only t))
>> +                   (goto-char (car pos))
>> +                   (delete-region (car pos) (cdr pos))
>> +                   (insert (car dst))))
>> +               (save-buffer)))
>> +           (message "Saved %d buffers" (length buffer-edits)))
>> +          (t
>> +           (message "%d hunks failed; no buffers changed" failures)))))
>
> Sorry, was there supposed to be a call to diff-test-hunk here? Or I'm just
> not following the implementation.

diff-test-hunk was just an example of implementation.  But the same way as
diff-apply-hunk is not used here, also only the logic of diff-test-hunk is used.

> I tried testing it out too. There is a patch where the third hunk doesn't
> apply (errors with "can't find the text to patch" in regular usage). This
> command ends with cryptic "No next hunk".

This is what I expected that diff-hunk-next might signal an error
that we should catch, but still couldn't find a patch that fails
with such error.  So I will try to construct such a patch manually.

bug#66113: Apply the entire diff buffer

[Dmitry Gutov]

On 24/09/2023 10:34, Juri Linkov wrote:
>> Sorry, was there supposed to be a call to diff-test-hunk here? Or I'm just
>> not following the implementation.
> diff-test-hunk was just an example of implementation.  But the same way as
> diff-apply-hunk is not used here, also only the logic of diff-test-hunk is used.

Fair enough.

>> I tried testing it out too. There is a patch where the third hunk doesn't
>> apply (errors with "can't find the text to patch" in regular usage). This
>> command ends with cryptic "No next hunk".
> This is what I expected that diff-hunk-next might signal an error
> that we should catch, but still couldn't find a patch that fails
> with such error.  So I will try to construct such a patch manually.

Try the patch attached to this message: 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63896#5

bug#66113: Apply the entire diff buffer

[Juri Linkov]

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

>>> I tried testing it out too. There is a patch where the third hunk doesn't
>>> apply (errors with "can't find the text to patch" in regular usage). This
>>> command ends with cryptic "No next hunk".
>> This is what I expected that diff-hunk-next might signal an error
>> that we should catch, but still couldn't find a patch that fails
>> with such error.  So I will try to construct such a patch manually.
>
> Try the patch attached to this message:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63896#5

Thanks, I forgot about signatures in git patches.
diff-mode already handles them in diff-fixup-modifs:

                        ;; In git format-patch "^-- $" signifies
                        ;; the end of the patch.
			(and (eq diff-buffer-type 'git)
			     (looking-at "^-- $"))

So I just copied this code here:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff-apply-buffer.patch --]
[-- Type: text/x-diff, Size: 2298 bytes --]

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index d776375d681..8f70731c7dc 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-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
   "C-c C-s" #'diff-split-hunk
@@ -2054,6 +2055,43 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
+(defun diff-apply-buffer ()
+  "Apply the diff in the entire diff buffer.
+When applying all hunks was successful, then save the changed buffers."
+  (interactive)
+  (let ((buffer-edits nil)
+        (failures 0)
+        (diff-refine nil))
+    (save-excursion
+      (goto-char (point-min))
+      (diff-beginning-of-hunk t)
+      (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
+                          (diff-find-source-location nil nil)))
+               (cond ((and line-offset (not switched))
+                      (push (cons pos dst)
+                            (alist-get buf buffer-edits)))
+                     (t (setq failures (1+ failures))))
+               (not (or (eq (prog1 (point) (diff-hunk-next)) (point))
+                        (eobp)
+                        ;; In git format-patch "^-- $" signifies
+                        ;; the end of the patch.
+                        (and (eq diff-buffer-type 'git)
+                             (looking-at-p "^-- $")))))))
+    (cond ((zerop failures)
+           (dolist (buf-edits (reverse buffer-edits))
+             (with-current-buffer (car buf-edits)
+               (dolist (edit (cdr buf-edits))
+                 (let ((pos (car edit))
+                       (dst (cdr edit))
+                       (inhibit-read-only t))
+                   (goto-char (car pos))
+                   (delete-region (car pos) (cdr pos))
+                   (insert (car dst))))
+               (save-buffer)))
+           (message "Saved %d buffers" (length buffer-edits)))
+          (t
+           (message "%d hunks failed; no buffers changed" failures)))))
+
 (defalias 'diff-mouse-goto-source #'diff-goto-source)
 
 (defun diff-goto-source (&optional other-file event)

bug#66113: Apply the entire diff buffer

[Dmitry Gutov]

On 25/09/2023 20:49, Juri Linkov wrote:
>>>> I tried testing it out too. There is a patch where the third hunk doesn't
>>>> apply (errors with "can't find the text to patch" in regular usage). This
>>>> command ends with cryptic "No next hunk".
>>> This is what I expected that diff-hunk-next might signal an error
>>> that we should catch, but still couldn't find a patch that fails
>>> with such error.  So I will try to construct such a patch manually.
>> Try the patch attached to this message:
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63896#5
> Thanks, I forgot about signatures in git patches.
> diff-mode already handles them in diff-fixup-modifs:
> 
>                          ;; In git format-patch "^-- $" signifies
>                          ;; the end of the patch.
> 			(and (eq diff-buffer-type 'git)
> 			     (looking-at "^-- $"))
> 
> So I just copied this code here:

That works. Thanks!

Ideally, I think point would move to the first failing hunk. But that's 
not urgent, could be a TODO for later.

Aside from that, I'm still not crazy about the binding. But that's not a 
technical issue.

bug#66113: Apply the entire diff buffer

[Juri Linkov]

close 66113 30.0.50
thanks

>>                          ;; In git format-patch "^-- $" signifies
>>                          ;; the end of the patch.
>> 			(and (eq diff-buffer-type 'git)
>> 			     (looking-at "^-- $"))
>> So I just copied this code here:
>
> That works. Thanks!

Actually this doesn't work for bzr that has a bug that adds
an extra line at the end.  So I pushed a better fix.

> Ideally, I think point would move to the first failing hunk. But that's not
> urgent, could be a TODO for later.

There are many variants what would be better to do here.
Maybe also to show all failing hunks.  And leaving point
at the beginning also makes sense, because then the user
could start applying hunks one by one with 'C-c C-a'
from the beginning until encountering the failing hunk
and handle it manually.

> Aside from that, I'm still not crazy about the binding. But that's not
> a technical issue.

Agreed, the key binding is the best among bad variants.