bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Tags: patch


This command solves a long-standing annoyance I have had when working
with diffs.  To my knowledge there is no existing way to achieve this
(rectangular selection might be possible, but that behaves differently
when the text is yanked).  The binding "w" seems intuitive when
contrasted with "M-w", but might be confused with "W" (widen).  Not sure
if that is an issue or not.

In GNU Emacs 30.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version
 3.24.37, cairo version 1.16.0) of 2023-08-18 built on quetzal
Repository revision: 2f74a459d2a82578fd9a92e9e2facdca90dad974
Repository branch: feature/a-package-for-elpa
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --with-pgtk --with-native-compilation --with-imagemagick
 --with-tree-sitter'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/patch, Size: 2884 bytes --]

From 9d755a12614fb9c1afe4dd88cecfbe16c3c009c4 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-kill-ring-save): Add new command
* etc/NEWS: Mention 'diff-kill-ring-save'.
---
 etc/NEWS             |  7 +++++++
 lisp/vc/diff-mode.el | 22 +++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 6588299c532..9ce510e0f81 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -276,6 +276,13 @@ This allows changing which type of whitespace changes are ignored when
 regenerating hunks with 'diff-ignore-whitespace-hunk'.  Defaults to
 the previously hard-coded "-b".
 
+---
+*** New command 'diff-kill-ring-save'.
+This command behaves like 'kill-ring-save', but removes change
+indicators ("+", "-") from the beginning of a line.  This is useful
+when you wish to copy part of the contents of a diff, but not the diff
+itself.
+
 ** Ediff
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index d776375d681..fce61837069 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -195,6 +195,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -207,7 +208,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -2079,6 +2080,25 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save ()
+  " "
+  (interactive)
+  (let ((at-bol (save-excursion
+                  (goto-char (region-beginning))
+                  (bolp)))
+        lines)
+    (save-restriction
+      (narrow-to-region (region-beginning) (region-end))
+      (goto-char (point-min))
+      (while (not (eobp))
+        (let ((line (thing-at-point 'line t)))
+          (if (and (null lines) (not at-bol))
+              (push line lines)
+            (push (substring line 1) lines)))
+        (forward-line)))
+    (let ((region-extract-function
+           (lambda (_) (apply #'concat (nreverse lines)))))
+      (copy-region-as-kill nil nil t))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.39.2

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Philip Kaludercic <philipk@posteo.net> writes:

> Tags: patch
>
>
> This command solves a long-standing annoyance I have had when working
> with diffs.  To my knowledge there is no existing way to achieve this
> (rectangular selection might be possible, but that behaves differently
> when the text is yanked).  The binding "w" seems intuitive when
> contrasted with "M-w", but might be confused with "W" (widen).  Not sure
> if that is an issue or not.
>
> In GNU Emacs 30.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version
>  3.24.37, cairo version 1.16.0) of 2023-08-18 built on quetzal
> Repository revision: 2f74a459d2a82578fd9a92e9e2facdca90dad974
> Repository branch: feature/a-package-for-elpa
> System Description: Debian GNU/Linux 12 (bookworm)
>
> Configured using:
>  'configure --with-pgtk --with-native-compilation --with-imagemagick
>  --with-tree-sitter'

It seems I prepared to patch too hastily, and forgot to add a docstring.
Here is the updated version, with a docstring:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-diff, Size: 3147 bytes --]

From fc39bd54ac93dbebec3b4c16aa1fe995cd421c92 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-kill-ring-save): Add new command
* etc/NEWS: Mention 'diff-kill-ring-save'.
---
 etc/NEWS             |  7 +++++++
 lisp/vc/diff-mode.el | 24 +++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 6588299c532..9ce510e0f81 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -276,6 +276,13 @@ This allows changing which type of whitespace changes are ignored when
 regenerating hunks with 'diff-ignore-whitespace-hunk'.  Defaults to
 the previously hard-coded "-b".
 
+---
+*** New command 'diff-kill-ring-save'.
+This command behaves like 'kill-ring-save', but removes change
+indicators ("+", "-") from the beginning of a line.  This is useful
+when you wish to copy part of the contents of a diff, but not the diff
+itself.
+
 ** Ediff
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index d776375d681..342d2d8ce90 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -195,6 +195,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -207,7 +208,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -2079,6 +2080,27 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end)
+  "Save contents of the region between BEG and END akin to `kill-ring-save'.
+The contents of a region will not include diff indicators at the
+beginning of each line."
+  (interactive (list (region-beginning) (region-end)))
+  (let ((at-bol (save-excursion (goto-char beg) (bolp)))
+        lines)
+    (save-restriction
+      (narrow-to-region beg end)
+      (goto-char (point-min))
+      (while (not (eobp))
+        (let ((line (thing-at-point 'line t)))
+          ;; In case the user has selected a region that begins
+          ;; mid-line, we should not chomp off the first character.
+          (if (and (null lines) (not at-bol))
+              (push line lines)
+            (push (substring line 1) lines)))
+        (forward-line)))
+    (let ((region-extract-function
+           (lambda (_) (apply #'concat (nreverse lines)))))
+      (kill-ring-save beg end t))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.39.2

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sat, 19 Aug 2023 09:53:58 +0000
> 
> This command solves a long-standing annoyance I have had when working
> with diffs.  To my knowledge there is no existing way to achieve this
> (rectangular selection might be possible, but that behaves differently
> when the text is yanked).  The binding "w" seems intuitive when
> contrasted with "M-w", but might be confused with "W" (widen).  Not sure
> if that is an issue or not.

Please tell more about the use cases where this would be useful.  I
couldn't understand that from the patch itself.

Thanks.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Sat, 19 Aug 2023 09:53:58 +0000
>> 
>> This command solves a long-standing annoyance I have had when working
>> with diffs.  To my knowledge there is no existing way to achieve this
>> (rectangular selection might be possible, but that behaves differently
>> when the text is yanked).  The binding "w" seems intuitive when
>> contrasted with "M-w", but might be confused with "W" (widen).  Not sure
>> if that is an issue or not.
>
> Please tell more about the use cases where this would be useful.  I
> couldn't understand that from the patch itself.

A simple example is where someone sends me a patch with a Elisp function
I'd like to evaluate.  If I can't or don't want to apply the patch right
now, I can use `diff-kill-ring-save' to copy the function without the
leading "+" into my scratch buffer and evaluate it there.

> Thanks.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 65380@debbugs.gnu.org
> Date: Sat, 19 Aug 2023 10:48:03 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please tell more about the use cases where this would be useful.  I
> > couldn't understand that from the patch itself.
> 
> A simple example is where someone sends me a patch with a Elisp function
> I'd like to evaluate.  If I can't or don't want to apply the patch right
> now, I can use `diff-kill-ring-save' to copy the function without the
> leading "+" into my scratch buffer and evaluate it there.

But diffs don't necessarily show the entire body/ies of function(s),
they show just part of them.  So this seems to be useful only in a
very small subset of cases?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 65380@debbugs.gnu.org
>> Date: Sat, 19 Aug 2023 10:48:03 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Please tell more about the use cases where this would be useful.  I
>> > couldn't understand that from the patch itself.
>> 
>> A simple example is where someone sends me a patch with a Elisp function
>> I'd like to evaluate.  If I can't or don't want to apply the patch right
>> now, I can use `diff-kill-ring-save' to copy the function without the
>> leading "+" into my scratch buffer and evaluate it there.
>
> But diffs don't necessarily show the entire body/ies of function(s),
> they show just part of them.  So this seems to be useful only in a
> very small subset of cases?

In theory yes, but as I mentioned in my first message, it comes up
surprisingly often, at least to the degree that I think it would be
something nice to have in general.  If you think the current
implementation is too primitive, one could extend it to check if the
region is a subregion of a hunk, and handle that appropriately.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: 65380@debbugs.gnu.org
> Date: Sat, 19 Aug 2023 15:45:13 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But diffs don't necessarily show the entire body/ies of function(s),
> > they show just part of them.  So this seems to be useful only in a
> > very small subset of cases?
> 
> In theory yes, but as I mentioned in my first message, it comes up
> surprisingly often, at least to the degree that I think it would be
> something nice to have in general.  If you think the current
> implementation is too primitive, one could extend it to check if the
> region is a subregion of a hunk, and handle that appropriately.

I'd like to hear from more people who will find this useful enough to
have in Emacs.  My first thought was that this is something you should
keep as your local extension, but maybe I'm mistaken.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 65380@debbugs.gnu.org
>> Date: Sat, 19 Aug 2023 15:45:13 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > But diffs don't necessarily show the entire body/ies of function(s),
>> > they show just part of them.  So this seems to be useful only in a
>> > very small subset of cases?
>> 
>> In theory yes, but as I mentioned in my first message, it comes up
>> surprisingly often, at least to the degree that I think it would be
>> something nice to have in general.  If you think the current
>> implementation is too primitive, one could extend it to check if the
>> region is a subregion of a hunk, and handle that appropriately.
>
> I'd like to hear from more people who will find this useful enough to
> have in Emacs.  My first thought was that this is something you should
> keep as your local extension, but maybe I'm mistaken.

I have no problem with that, and there is no urgency in applying this
patch.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Sean Whitton]

Hello,

On Sat 19 Aug 2023 at 10:09pm +03, Eli Zaretskii wrote:

> I'd like to hear from more people who will find this useful enough to
> have in Emacs.  My first thought was that this is something you should
> keep as your local extension, but maybe I'm mistaken.

This comes up for me in Debian packaging work, and I would like to have
this feature.  The diff contains the addition of a new test, say.  I
want to copy the test into a version of the package that's different
from the one the patch was originally prepared for.

-- 
Sean Whitton

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Rudolf Adamkovič via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd like to hear from more people who will find this useful enough to
> have in Emacs.  My first thought was that this is something you should
> keep as your local extension, but maybe I'm mistaken.

I, too, find myself removing those +/- signs quite often.

Rudy
-- 
"One can begin to reason only when a clear picture has been formed in
the imagination."
-- Walter Warwick Sawyer, Mathematician's Delight, 1943

Rudolf Adamkovič <salutis@me.com> [he/him]
Studenohorská 25
84103 Bratislava
Slovakia

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: 65380@debbugs.gnu.org
>> Date: Sat, 19 Aug 2023 15:45:13 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > But diffs don't necessarily show the entire body/ies of function(s),
>> > they show just part of them.  So this seems to be useful only in a
>> > very small subset of cases?
>> 
>> In theory yes, but as I mentioned in my first message, it comes up
>> surprisingly often, at least to the degree that I think it would be
>> something nice to have in general.  If you think the current
>> implementation is too primitive, one could extend it to check if the
>> region is a subregion of a hunk, and handle that appropriately.
>
> I'd like to hear from more people who will find this useful enough to
> have in Emacs.  My first thought was that this is something you should
> keep as your local extension, but maybe I'm mistaken.

I'd find this command useful.  When I copy parts of a diff for other
purposes, I usually have to remove the diff markers manually.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Philip Kaludercic <philipk@posteo.net> writes:

> +(defun diff-kill-ring-save (beg end)
> +  "Save contents of the region between BEG and END akin to `kill-ring-save'.
> +The contents of a region will not include diff indicators at the
> +beginning of each line."
> +  (interactive (list (region-beginning) (region-end)))
> +  (let ((at-bol (save-excursion (goto-char beg) (bolp)))
> +        lines)
> +    (save-restriction
> +      (narrow-to-region beg end)
> +      (goto-char (point-min))
> +      (while (not (eobp))
> +        (let ((line (thing-at-point 'line t)))
> +          ;; In case the user has selected a region that begins
> +          ;; mid-line, we should not chomp off the first character.
> +          (if (and (null lines) (not at-bol))
> +              (push line lines)
> +            (push (substring line 1) lines)))
> +        (forward-line)))
> +    (let ((region-extract-function
> +           (lambda (_) (apply #'concat (nreverse lines)))))
> +      (kill-ring-save beg end t))))
>

As an alternative implementation, to avoid creating lots of intermediate
substrings, we could check if we're inside a hunk and, if so, perform a
regular expression replace to remove the diff indicators.  If the region
covers text outside of a hunk, then we could copy the text normally, as
if the user pressed M-w.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Daniel Martín <mardani29@yahoo.es> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> +(defun diff-kill-ring-save (beg end)
>> +  "Save contents of the region between BEG and END akin to `kill-ring-save'.
>> +The contents of a region will not include diff indicators at the
>> +beginning of each line."
>> +  (interactive (list (region-beginning) (region-end)))
>> +  (let ((at-bol (save-excursion (goto-char beg) (bolp)))
>> +        lines)
>> +    (save-restriction
>> +      (narrow-to-region beg end)
>> +      (goto-char (point-min))
>> +      (while (not (eobp))
>> +        (let ((line (thing-at-point 'line t)))
>> +          ;; In case the user has selected a region that begins
>> +          ;; mid-line, we should not chomp off the first character.
>> +          (if (and (null lines) (not at-bol))
>> +              (push line lines)
>> +            (push (substring line 1) lines)))
>> +        (forward-line)))
>> +    (let ((region-extract-function
>> +           (lambda (_) (apply #'concat (nreverse lines)))))
>> +      (kill-ring-save beg end t))))
>>
>
> As an alternative implementation, to avoid creating lots of intermediate
> substrings, we could check if we're inside a hunk and, if so, perform a
> regular expression replace to remove the diff indicators.  If the region
> covers text outside of a hunk, then we could copy the text normally, as
> if the user pressed M-w.

What text would we want to copy outside of a hunk?  Something like a
patch commit message?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Juri Linkov]

>> > But diffs don't necessarily show the entire body/ies of function(s),
>> > they show just part of them.  So this seems to be useful only in a
>> > very small subset of cases?
>>
>> In theory yes, but as I mentioned in my first message, it comes up
>> surprisingly often, at least to the degree that I think it would be
>> something nice to have in general.  If you think the current
>> implementation is too primitive, one could extend it to check if the
>> region is a subregion of a hunk, and handle that appropriately.
>
> I'd like to hear from more people who will find this useful enough to
> have in Emacs.  My first thought was that this is something you should
> keep as your local extension, but maybe I'm mistaken.

Sometimes it's useful to copy the plain text from the diff
without applying the patch.  Regarding the implementation, there is
the function 'diff-hunk-text', but it's limited to one hunk only.
So another variant would be to extend 'diff--filter-substring'
that conditionally could modify the kill-ring-save filter
depending on a new customizable variable.
Although for some strange reason, the filter is not applied
when using the region.  I think this is a bug, or at least
a misfeature.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Philip Kaludercic <philipk@posteo.net>,  65380@debbugs.gnu.org
> Date: Sun, 20 Aug 2023 19:30:03 +0300
> 
> Regarding the implementation, there is the function
> 'diff-hunk-text', but it's limited to one hunk only.

So is the proposed new function, isn't it?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Juri Linkov <juri@linkov.net>
>> Cc: Philip Kaludercic <philipk@posteo.net>,  65380@debbugs.gnu.org
>> Date: Sun, 20 Aug 2023 19:30:03 +0300
>> 
>> Regarding the implementation, there is the function
>> 'diff-hunk-text', but it's limited to one hunk only.
>
> So is the proposed new function, isn't it?

No, the current proposal doesn't have any special handling for text
between hunks.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Juri Linkov <juri@linkov.net>,  65380@debbugs.gnu.org
> Date: Sun, 20 Aug 2023 18:24:53 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Juri Linkov <juri@linkov.net>
> >> Cc: Philip Kaludercic <philipk@posteo.net>,  65380@debbugs.gnu.org
> >> Date: Sun, 20 Aug 2023 19:30:03 +0300
> >> 
> >> Regarding the implementation, there is the function
> >> 'diff-hunk-text', but it's limited to one hunk only.
> >
> > So is the proposed new function, isn't it?
> 
> No, the current proposal doesn't have any special handling for text
> between hunks.

AFAIU, the function you proposed removes the first character from each
line in the region, so how will it handle multiple hunks?

Or maybe I misunderstood what you meant by "No"?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Jim Porter]

On 8/19/2023 12:09 PM, Eli Zaretskii wrote:
> I'd like to hear from more people who will find this useful enough to
> have in Emacs.  My first thought was that this is something you should
> keep as your local extension, but maybe I'm mistaken.

Based on my understanding of the current implementation, I would *not* 
find this useful, and instead I'd propose a couple of different ways to 
handle this.

First, the original message says, "rectangular selection might be 
possible, but that behaves differently when the text is yanked". I've 
been bitten by that a few times in the past, and I'd much rather a 
general solution to that problem instead. Some way of yanking a 
rectangular selection as though it were a normal region (or some way to 
put the rectangular selection on the normal kill ring) would be great, 
and would solve this in a more-general way. Then, for example, you could 
use the same command to copy the contents of a diff *or* to copy a 
commented-out block of code without the comment introducers. Something like:

   ;; (defun hello ()
   ;;   "Say hello."
   ;;   (message "Hello"))

If I could select a rectangle around the actual code to copy it, 
excluding the leading ";; ", that would be useful (occasionally, at least).

Second, for the diff case in particular, I'd rather have a command that 
copies the added or unchanged lines, and *skips* the removed lines. (As 
far as I can tell, the proposed implementation copies the removed lines 
as well.) Then I could actually yank this into my destination and it 
would work. Copying the removed lines would mean I need to go and remove 
them manually, only now I've lost the "-" indicator that tells me which 
lines are removed.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Gregory Heytings]

>
> Some way of yanking a rectangular selection as though it were a normal 
> region (or some way to put the rectangular selection on the normal kill 
> ring) would be great, and would solve this in a more-general way.
>

These commands exist: C-x r k to kill a rectangular selection, C-x r M-w 
to save it, and C-x r y to yank it.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Jim Porter]

On 8/20/2023 1:13 PM, Gregory Heytings wrote:
> 
>>
>> Some way of yanking a rectangular selection as though it were a normal 
>> region (or some way to put the rectangular selection on the normal 
>> kill ring) would be great, and would solve this in a more-general way.
>>
> 
> These commands exist: C-x r k to kill a rectangular selection, C-x r M-w 
> to save it, and C-x r y to yank it.

That's not quite what I mean. "C-x r y" ('yank-rectangle') yanks the 
rectangle *as a rectangle*. That is, if I just copied a rectangle with 5 
lines, and yank it with 'yank-rectangle', it will add it to the next 5 
existing lines. Instead, what I want is to insert 5 new lines.

For example, if my buffer is:

   Hello
   There
   World

And I copied this rectangle:

   1
   2

Then 'yank-rectangle' at the beginning of the buffer yields:

   1Hello
   2There
   World

But I want:

   1
   2
   Hello
   There
   World

The latter would be much more useful when trying to copy/yank a rect out 
of a diff buffer as in this report.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Gregory Heytings]

>
> That's not quite what I mean. "C-x r y" ('yank-rectangle') yanks the 
> rectangle *as a rectangle*. That is, if I just copied a rectangle with 5 
> lines, and yank it with 'yank-rectangle', it will add it to the next 5 
> existing lines. Instead, what I want is to insert 5 new lines.
>

Indeed, I see what you mean.  Perhaps a new C-x r command that would do 
that could be added to Emacs?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Jim Porter]

On 8/20/2023 2:29 PM, Gregory Heytings wrote:
>
>> That's not quite what I mean. "C-x r y" ('yank-rectangle') yanks the 
>> rectangle *as a rectangle*. That is, if I just copied a rectangle with 
>> 5 lines, and yank it with 'yank-rectangle', it will add it to the next 
>> 5 existing lines. Instead, what I want is to insert 5 new lines.
>>
> 
> Indeed, I see what you mean.  Perhaps a new C-x r command that would do 
> that could be added to Emacs?

Yeah, the question then is: should it be new kill/copy commands or a new 
yank command? The former would mean you could use all the existing yank 
functions to paste the text in, but the latter means you can defer your 
decision about how to yank the text (as a regular region or as a 
rectangle) until you're ready to actually yank.

I'd lean a bit towards the former, but that does mean (potentially) two 
new key bindings.



... hmm, or maybe you could make the existing rectangle kill/copy 
commands also add to the "regular" kill ring automatically? But then 
that might cause issues with 'rectangle-mark-mode', where 'C-y' performs 
'yank-rectangle'[1]: how would I use 'rectangle-mark-mode' to copy a 
rect and then paste it as a regular region?

[1] Well, technically 'rectangle--insert-for-yank', but they both call 
'insert-rectangle' in the end.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Gregory Heytings]

>
> Yeah, the question then is: should it be new kill/copy commands or a new 
> yank command?
>

I would do something else: a command to move the killed rectangle to the 
kill-ring.  And perhaps a command to do the opposite: transform the last 
item of the kill-ring into a killed rectangle.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Gregory Heytings]

>> Yeah, the question then is: should it be new kill/copy commands or a 
>> new yank command?
>
> I would do something else: a command to move the killed rectangle to the 
> kill-ring.  And perhaps a command to do the opposite: transform the last 
> item of the kill-ring into a killed rectangle.
>

And here are the two commands I had in mind:

(defun move-killed-rectangle-to-kill-ring ()
   "Move the killed rectangle to the `kill-ring'."
   (interactive)
   (if killed-rectangle
       (with-temp-buffer
 	(while killed-rectangle
 	  (insert-for-yank (car killed-rectangle))
 	  (insert "\n")
 	  (setq killed-rectangle (cdr killed-rectangle)))
 	(kill-ring-save (point-min) (point-max)))
     (user-error "No killed rectangle")))

(defun copy-last-kill-to-killed-rectangle ()
   "Transform the current item of `kill-ring' into a killed rectangle"
   (interactive)
   (with-temp-buffer
     (let ((max -1))
       (insert-for-yank (current-kill 0 t))
       (goto-char (point-min))
       (while (not (eobp))
 	(move-end-of-line nil)
 	(let ((col (current-column)))
 	  (when (> col max)
 	    (setq max col)))
 	(forward-line 1))
       (let ((col (current-column)))
 	(when (< col max)
 	  (insert (make-string (- max col) ? ))))
     (kill-rectangle (point-min) (point-max)))))

WDYT?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Jim Porter]

On 8/20/2023 4:39 PM, Gregory Heytings wrote:
>> I would do something else: a command to move the killed rectangle to 
>> the kill-ring.  And perhaps a command to do the opposite: transform 
>> the last item of the kill-ring into a killed rectangle.
>>
> 
> And here are the two commands I had in mind:
[snip]> WDYT?

Hm, it could work. I'm not sure we *need* to be able to go from the kill 
ring to 'killed-rectangle', but if people are happy with the 
implementation, I don't see a problem.

I wonder how this would interact with 'rectangle-mark-mode' though. That 
puts the killed rectangle on the "regular" kill ring, but applies a 
'yank-handler' text property to the killed rect. 'rectangle-mark-mode' 
might need its own implementation for this. (Either not setting 
'yank-handler' when killing or having a way of ignoring the 
'yank-handler' when yanking?)

(I also wonder if it would make sense for the "normal" rect commands 
like "C-x r k" / 'kill-rectangle' to integrate with the kill ring like 
'rectangle-mark-mode', but that's another can of worms...)

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Juri Linkov <juri@linkov.net>,  65380@debbugs.gnu.org
>> Date: Sun, 20 Aug 2023 18:24:53 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Juri Linkov <juri@linkov.net>
>> >> Cc: Philip Kaludercic <philipk@posteo.net>,  65380@debbugs.gnu.org
>> >> Date: Sun, 20 Aug 2023 19:30:03 +0300
>> >> 
>> >> Regarding the implementation, there is the function
>> >> 'diff-hunk-text', but it's limited to one hunk only.
>> >
>> > So is the proposed new function, isn't it?
>> 
>> No, the current proposal doesn't have any special handling for text
>> between hunks.
>
> AFAIU, the function you proposed removes the first character from each
> line in the region, so how will it handle multiple hunks?
>
> Or maybe I misunderstood what you meant by "No"?

The command does not error when the region selects more than just a
sub-region of a hunk, but it doesn't do anything useful either.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Tue, 22 Aug 2023 11:06:28 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Philip Kaludercic <philipk@posteo.net>
> >> Cc: Juri Linkov <juri@linkov.net>,  65380@debbugs.gnu.org
> >> Date: Sun, 20 Aug 2023 18:24:53 +0000
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> >> From: Juri Linkov <juri@linkov.net>
> >> >> Cc: Philip Kaludercic <philipk@posteo.net>,  65380@debbugs.gnu.org
> >> >> Date: Sun, 20 Aug 2023 19:30:03 +0300
> >> >> 
> >> >> Regarding the implementation, there is the function
> >> >> 'diff-hunk-text', but it's limited to one hunk only.
> >> >
> >> > So is the proposed new function, isn't it?
> >> 
> >> No, the current proposal doesn't have any special handling for text
> >> between hunks.
> >
> > AFAIU, the function you proposed removes the first character from each
> > line in the region, so how will it handle multiple hunks?
> >
> > Or maybe I misunderstood what you meant by "No"?
> 
> The command does not error when the region selects more than just a
> sub-region of a hunk, but it doesn't do anything useful either.

Then how about making a command that is basically a wrapper around
diff-hunk-text, and allows to copy bodies of N hunks, given an
argument of N?  Wouldn't that be more useful?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Tue, 22 Aug 2023 11:06:28 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Philip Kaludercic <philipk@posteo.net>
>> >> Cc: Juri Linkov <juri@linkov.net>,  65380@debbugs.gnu.org
>> >> Date: Sun, 20 Aug 2023 18:24:53 +0000
>> >> 
>> >> Eli Zaretskii <eliz@gnu.org> writes:
>> >> 
>> >> >> From: Juri Linkov <juri@linkov.net>
>> >> >> Cc: Philip Kaludercic <philipk@posteo.net>,  65380@debbugs.gnu.org
>> >> >> Date: Sun, 20 Aug 2023 19:30:03 +0300
>> >> >> 
>> >> >> Regarding the implementation, there is the function
>> >> >> 'diff-hunk-text', but it's limited to one hunk only.
>> >> >
>> >> > So is the proposed new function, isn't it?
>> >> 
>> >> No, the current proposal doesn't have any special handling for text
>> >> between hunks.
>> >
>> > AFAIU, the function you proposed removes the first character from each
>> > line in the region, so how will it handle multiple hunks?
>> >
>> > Or maybe I misunderstood what you meant by "No"?
>> 
>> The command does not error when the region selects more than just a
>> sub-region of a hunk, but it doesn't do anything useful either.
>
> Then how about making a command that is basically a wrapper around
> diff-hunk-text, and allows to copy bodies of N hunks, given an
> argument of N?  Wouldn't that be more useful?

Sorry for the unreasonably delay in responding to this message.  I've
finally come up with something acceptable:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-patch, Size: 3497 bytes --]

From 463e555d2bd4837f02163ef3e93df85c7dc092da Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-kill-ring-save): Add new command
* etc/NEWS: Mention 'diff-kill-ring-save'.
---
 lisp/vc/diff-mode.el | 46 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 81e8b23ee33..94e40f5900b 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -196,6 +196,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -208,7 +209,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -2108,6 +2109,49 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end &optional reverse)
+  "Save contents of the region between BEG and END akin to `kill-ring-save'.
+By default the command will copy the text that applying the diff would
+produce, along with the text between hunks.  If REVERSE is non-nil, or
+the command was invoked with a prefix argument, copy the deleted text."
+  (interactive (list (region-beginning) (region-end)
+                     current-prefix-arg))
+  (unless (derived-mode-p 'diff-mode)
+    (user-error "Command can only be invoked in a diff-buffer"))
+  (revert-buffer)                       ;refresh the diff
+  (let ((parts '()))
+    (save-excursion
+      (goto-char beg)
+      (catch 'break
+        (while t
+          (let ((hunk (diff-hunk-text
+                       (buffer-substring
+                        (save-excursion (diff-beginning-of-hunk))
+                        (save-excursion
+                          (min (diff-end-of-hunk) end)))
+                       (not reverse)
+                       (save-excursion
+                         (- (point) (diff-beginning-of-hunk))))))
+            (push (substring (car hunk) (cdr hunk))
+                  parts))
+          ;; check if we have copied everything
+          (diff-end-of-hunk)
+          (when (< end (point)) (throw 'break t))
+          ;; copy the text between hunks
+          (let (start)
+            (save-window-excursion
+              (save-excursion
+                (forward-line -1)
+                (diff-goto-source)
+                (forward-line +1)
+                (setq start (point))))
+            (save-window-excursion
+              (diff-goto-source)
+              (push (buffer-substring start (point))
+                    parts))))))
+    (kill-new (string-join (nreverse parts)))
+    (setq deactivate-mark t)
+    (message "Copied text")))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.46.0


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


This will also copy text between hunks.  I've tested it a bit, but I am
sure someone else here might find some edge-cases.

-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Philip Kaludercic <philipk@posteo.net> writes:

> +          ;; copy the text between hunks
> +          (let (start)
> +            (save-window-excursion
> +              (save-excursion
> +                (forward-line -1)
> +                (diff-goto-source)
> +                (forward-line +1)
> +                (setq start (point))))

One issue I still have here is that the `forwarad-line' might scroll the
window, which `save-window-excursion' doesn't restore.  To fix this I
have written a general macro for subr.el to restore the scroll position:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Add-new-macro-to-restore-the-scroll-position-of-a-wi.patch --]
[-- Type: text/x-patch, Size: 1167 bytes --]

From c289c114ac381105cb550b3805041ad5cf5e61a5 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 18 Aug 2024 17:26:01 +0200
Subject: [PATCH 2/2] Add new macro to restore the scroll position of a window

* lisp/subr.el (save-window-start): Add macro.
---
 lisp/subr.el | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9ea18ca5bff..003e27f2ad7 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4922,6 +4922,16 @@ save-window-excursion
        (unwind-protect (progn ,@body)
          (set-window-configuration ,c)))))
 
+(defmacro save-window-start (&rest body)
+  "Execute BODY, then restore the starting position of the selected window."
+  (declare (indent 0) (debug t))
+  (let ((window (make-symbol "window"))
+        (start (make-symbol "start")))
+    `(let* ((,window (selected-window))
+            (,start (window-start ,window)))
+       (unwind-protect ,(macroexp-progn body)
+         (set-window-start ,window ,start t)))))
+
 (defun internal-temp-output-buffer-show (buffer)
   "Internal function for `with-output-to-temp-buffer'."
   (with-current-buffer buffer
-- 
2.46.0


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


and updated the previous patch accordingly (+ some other minor
improvements):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-patch, Size: 4433 bytes --]

From 4565d12cdcbdb22c316f741e656d5388e880201c Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH 1/2] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-beginning-of-hunk-position, diff-end-of-hunk-position):
New utility functions.
(diff-kill-ring-save): Add the command.
* etc/NEWS: Mention 'diff-kill-ring-save'.  (Bug#65380)
---
 lisp/vc/diff-mode.el | 59 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 81e8b23ee33..2a41862e363 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -196,6 +196,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -208,7 +209,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -630,6 +631,22 @@ diff-end-of-hunk
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
+(defun diff-beginning-of-hunk-position (&optional try-harder)
+  "Call `diff-end-of-hunk' without moving.
+The optional argument TRY-HARDER is passed on to
+`diff-beginning-of-hunk'."
+  (save-excursion
+    (save-window-excursion
+      (diff-beginning-of-hunk try-harder))))
+
+(defun diff-end-of-hunk-position (&optional style donttrustheader)
+  "Call `diff-end-of-hunk' without moving.
+The optional arguments STYLE and DONTTRUSTHEADER are passed on to
+`diff-end-of-hunk'."
+  (save-excursion
+    (save-window-excursion
+      (diff-end-of-hunk style donttrustheader))))
+
 ;; "index ", "old mode", "new mode", "new file mode" and
 ;; "deleted file mode" are output by git-diff.
 (defconst diff-file-junk-re
@@ -2108,6 +2125,46 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end &optional reverse)
+  "Save contents of the region between BEG and END akin to `kill-ring-save'.
+By default the command will copy the text that applying the diff would
+produce, along with the text between hunks.  If REVERSE is non-nil, or
+the command was invoked with a prefix argument, copy the deleted text."
+  (interactive (list (region-beginning) (region-end) current-prefix-arg))
+  (unless (derived-mode-p 'diff-mode)
+    (user-error "Command can only be invoked in a diff-buffer"))
+  (revert-buffer t t t)                 ;refresh the diff
+  (let ((parts '()))
+    (save-excursion
+      (goto-char beg)
+      (catch 'break
+        (while t
+          (let ((hunk (diff-hunk-text
+                       (buffer-substring
+                        (diff-beginning-of-hunk-position)
+                        (min (diff-end-of-hunk-position) end))
+                       (not reverse)
+                       (- (point) (diff-beginning-of-hunk-position)))))
+            (push (substring (car hunk) (cdr hunk))
+                  parts))
+          ;; check if we have copied everything
+          (diff-end-of-hunk)
+          (when (<= end (point)) (throw 'break t))
+          ;; copy the text between hunks
+          (let ((inhibit-message t) start)
+            (save-window-excursion
+              (save-excursion
+                (forward-line -1)
+                (diff-goto-source t)
+                (forward-line +1)
+                (setq start (point))))
+            (save-window-excursion
+              (diff-goto-source t)
+              (push (buffer-substring start (point))
+                    parts))))))
+    (kill-new (string-join (nreverse parts)))
+    (setq deactivate-mark t)
+    (message (if reverse "Copied original text" "Copied modified text"))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.46.0


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


-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Sun, 18 Aug 2024 15:29:25 +0000
> 
> > +          ;; copy the text between hunks
> > +          (let (start)
> > +            (save-window-excursion
> > +              (save-excursion
> > +                (forward-line -1)
> > +                (diff-goto-source)
> > +                (forward-line +1)
> > +                (setq start (point))))
> 
> One issue I still have here is that the `forwarad-line' might scroll the
> window, which `save-window-excursion' doesn't restore.

I don't understand what you are saying here.  As long as a Lisp
program runs and doesn't force redisplay (via sit-for or explicit
calls to 'redisplay' or somesuch), forwarad-line cannot cause any
scroll of the window, because what scrolls the window is redisplay
when it kicks in and finds that point is outside of the window.  So if
the above code takes care to restore point before it finishes, the
window won't scroll.  Or what am I missing?

> To fix this I have written a general macro for subr.el to restore
> the scroll position:

Let's first make sure we understand what happens here before we add
such a macro.  (It also has some conceptual problems of its own, but
let's defer that until we actually sure it is needed.)

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Sun, 18 Aug 2024 15:29:25 +0000
>> 
>> > +          ;; copy the text between hunks
>> > +          (let (start)
>> > +            (save-window-excursion
>> > +              (save-excursion
>> > +                (forward-line -1)
>> > +                (diff-goto-source)
>> > +                (forward-line +1)
>> > +                (setq start (point))))
>> 
>> One issue I still have here is that the `forwarad-line' might scroll the
>> window, which `save-window-excursion' doesn't restore.
>
> I don't understand what you are saying here.  As long as a Lisp
> program runs and doesn't force redisplay (via sit-for or explicit
> calls to 'redisplay' or somesuch), forwarad-line cannot cause any
> scroll of the window, because what scrolls the window is redisplay
> when it kicks in and finds that point is outside of the window.  So if
> the above code takes care to restore point before it finishes, the
> window won't scroll.  Or what am I missing?

No, you were right, and I manged to locate what was causing the issue.
I had a (revert-buffer t t t) at the beginning of the function.
Wrapping `save-window-start' around just that expression also prevented
the displayed portion of the window from changing.

Thinking about the proposal again, it might seem better to avoid calling
`revert-buffer' in the first place.  The problem I was concerned with
was that if a .diff was outdated, jumping to the original file might not
do the right thing and copy the wrong inter-hunk text.  Now I realise
that `revert-buffer' doesn't always help resolve this either, e.g. if
copying text from an old .patch file.

The proper solution would be to try and detect these kinds of
inconsistencies and signal and error instead.

>> To fix this I have written a general macro for subr.el to restore
>> the scroll position:
>
> Let's first make sure we understand what happens here before we add
> such a macro.  (It also has some conceptual problems of its own, but
> let's defer that until we actually sure it is needed.)

It is not needed, but I would be interested in what the conceptual
errors are.

-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Sun, 18 Aug 2024 16:20:52 +0000
> 
> >> To fix this I have written a general macro for subr.el to restore
> >> the scroll position:
> >
> > Let's first make sure we understand what happens here before we add
> > such a macro.  (It also has some conceptual problems of its own, but
> > let's defer that until we actually sure it is needed.)
> 
> It is not needed, but I would be interested in what the conceptual
> errors are.

Forcing window-start has some side-effects that could look like bugs.
For example, the window's vscroll is reset, and if the original window
started its display with an overlay or display property,
set-window-start will not necessarily restore the original display,
due to boring technical issues.

So my recommendation is to avoid calling set-window-start as a means
to return to some previous display, because it is not guaranteed that
you can do it like that.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Sun, 18 Aug 2024 16:20:52 +0000
>> 
>> >> To fix this I have written a general macro for subr.el to restore
>> >> the scroll position:
>> >
>> > Let's first make sure we understand what happens here before we add
>> > such a macro.  (It also has some conceptual problems of its own, but
>> > let's defer that until we actually sure it is needed.)
>> 
>> It is not needed, but I would be interested in what the conceptual
>> errors are.
>
> Forcing window-start has some side-effects that could look like bugs.
> For example, the window's vscroll is reset, and if the original window
> started its display with an overlay or display property,
> set-window-start will not necessarily restore the original display,
> due to boring technical issues.
>
> So my recommendation is to avoid calling set-window-start as a means
> to return to some previous display, because it is not guaranteed that
> you can do it like that.

OK, I understood and will keep it in mind for the future.  Thank you for
the background!

I'm attaching the newest version of the patch here:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-patch, Size: 4988 bytes --]

From b87daa72aa4f833001cd60be0d1f5cfe4064f717 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-beginning-of-hunk-position, diff-end-of-hunk-position):
New utility functions.
(diff-kill-ring-save): Add the command.
* etc/NEWS: Mention 'diff-kill-ring-save'.  (Bug#65380)
---
 etc/NEWS             |  9 +++++++
 lisp/vc/diff-mode.el | 58 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index b89a80aa14d..e493c1bb035 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -210,6 +210,15 @@ The host name for Kubernetes connections can be of kind
 used.  This overrides the setiing in 'tramp-kubernetes-namespace', if
 any.
 
+** Diff
+
+---
+*** New command 'diff-kill-ring-save'
+This command copies out the modified contents out of a diff, without
+having to apply it first.  If the selected range extends a hunk, the
+commands attempts to look up and copy the text between from the
+referenced file.
+
 \f
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 81e8b23ee33..0f3b4c6b4d1 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -196,6 +196,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -208,7 +209,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -630,6 +631,22 @@ diff-end-of-hunk
     ;; The return value is used by easy-mmode-define-navigation.
     (goto-char (or end (point-max)))))
 
+(defun diff-beginning-of-hunk-position (&optional try-harder)
+  "Call `diff-end-of-hunk' without moving.
+The optional argument TRY-HARDER is passed on to
+`diff-beginning-of-hunk'."
+  (save-excursion
+    (save-window-excursion
+      (diff-beginning-of-hunk try-harder))))
+
+(defun diff-end-of-hunk-position (&optional style donttrustheader)
+  "Call `diff-end-of-hunk' without moving.
+The optional arguments STYLE and DONTTRUSTHEADER are passed on to
+`diff-end-of-hunk'."
+  (save-excursion
+    (save-window-excursion
+      (diff-end-of-hunk style donttrustheader))))
+
 ;; "index ", "old mode", "new mode", "new file mode" and
 ;; "deleted file mode" are output by git-diff.
 (defconst diff-file-junk-re
@@ -2108,6 +2125,45 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end &optional reverse)
+  "Save contents of the region between BEG and END akin to `kill-ring-save'.
+By default the command will copy the text that applying the diff would
+produce, along with the text between hunks.  If REVERSE is non-nil, or
+the command was invoked with a prefix argument, copy the deleted text."
+  (interactive (list (region-beginning) (region-end) current-prefix-arg))
+  (unless (derived-mode-p 'diff-mode)
+    (user-error "Command can only be invoked in a diff-buffer"))
+  (let ((parts '()))
+    (save-excursion
+      (goto-char beg)
+      (catch 'break
+        (while t
+          (let ((hunk (diff-hunk-text
+                       (buffer-substring
+                        (diff-beginning-of-hunk-position)
+                        (min (diff-end-of-hunk-position) end))
+                       (not reverse)
+                       (- (point) (diff-beginning-of-hunk-position)))))
+            (push (substring (car hunk) (cdr hunk))
+                  parts))
+          ;; check if we have copied everything
+          (diff-end-of-hunk)
+          (when (<= end (point)) (throw 'break t))
+          ;; copy the text between hunks
+          (let ((inhibit-message t) start)
+            (save-window-excursion
+              (save-excursion
+                (forward-line -1)
+                (diff-goto-source t)
+                (forward-line +1)
+                (setq start (point))))
+            (save-window-excursion
+              (diff-goto-source t)
+              (push (buffer-substring start (point))
+                    parts))))))
+    (kill-new (string-join (nreverse parts)))
+    (setq deactivate-mark t)
+    (message (if reverse "Copied original text" "Copied modified text"))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.46.0


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


-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Juri Linkov]

> I'm attaching the newest version of the patch here:

Thanks, I tried it out, and it works nicely for the single hunk case.

> +*** New command 'diff-kill-ring-save'
> +This command copies out the modified contents out of a diff, without
> +having to apply it first.  If the selected range extends a hunk, the
> +commands attempts to look up and copy the text between from the
> +referenced file.

I'm not sure about usefulness of the last part in multi-hunk case.
Does someone really need to copy a huge part of the source file
between hunks at the top and bottom?  I expected that multi-hunk case
would copy only concatenated text of hunks, not the source file.
But I have no strong opinion about this.  My main use case will be
to copy the text of the current hunk.  Could you please support
this case where typing 'w' would copy the current hunk when the
region is not activated.

> @@ -630,6 +631,22 @@ diff-end-of-hunk
> +(defun diff-beginning-of-hunk-position (&optional try-harder)
> +  "Call `diff-end-of-hunk' without moving.
> +The optional argument TRY-HARDER is passed on to
> +`diff-beginning-of-hunk'."
> +  (save-excursion
> +    (save-window-excursion
> +      (diff-beginning-of-hunk try-harder))))
> +
> +(defun diff-end-of-hunk-position (&optional style donttrustheader)
> +  "Call `diff-end-of-hunk' without moving.
> +The optional arguments STYLE and DONTTRUSTHEADER are passed on to
> +`diff-end-of-hunk'."
> +  (save-excursion
> +    (save-window-excursion
> +      (diff-end-of-hunk style donttrustheader))))

I don't understand why separate functions with 'save-window-excursion'
are needed here, since all other uses of 'diff-beginning-of-hunk'
just wrap the calls with 'save-excursion'.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Juri Linkov <juri@linkov.net> writes:

>> I'm attaching the newest version of the patch here:
>
> Thanks, I tried it out, and it works nicely for the single hunk case.

1+

>> +*** New command 'diff-kill-ring-save'
>> +This command copies out the modified contents out of a diff, without
>> +having to apply it first.  If the selected range extends a hunk, the
>> +commands attempts to look up and copy the text between from the
>> +referenced file.
>
> I'm not sure about usefulness of the last part in multi-hunk case.
> Does someone really need to copy a huge part of the source file
> between hunks at the top and bottom?  I expected that multi-hunk case
> would copy only concatenated text of hunks, not the source file.
> But I have no strong opinion about this.  

The main scenario I had in mind was when someone modifies the beginning
and the end of a function, and I want to copy all of it at once.  I
agree that there is usually not much of a use for copying huge parts of
a source file, but we get that for free when the beginning-to-end of a
function case is handled.

>                                           My main use case will be
> to copy the text of the current hunk.  Could you please support
> this case where typing 'w' would copy the current hunk when the
> region is not activated.

Done, see below.

>> @@ -630,6 +631,22 @@ diff-end-of-hunk
>> +(defun diff-beginning-of-hunk-position (&optional try-harder)
>> +  "Call `diff-end-of-hunk' without moving.
>> +The optional argument TRY-HARDER is passed on to
>> +`diff-beginning-of-hunk'."
>> +  (save-excursion
>> +    (save-window-excursion
>> +      (diff-beginning-of-hunk try-harder))))
>> +
>> +(defun diff-end-of-hunk-position (&optional style donttrustheader)
>> +  "Call `diff-end-of-hunk' without moving.
>> +The optional arguments STYLE and DONTTRUSTHEADER are passed on to
>> +`diff-end-of-hunk'."
>> +  (save-excursion
>> +    (save-window-excursion
>> +      (diff-end-of-hunk style donttrustheader))))
>
> I don't understand why separate functions with 'save-window-excursion'
> are needed here, since all other uses of 'diff-beginning-of-hunk'
> just wrap the calls with 'save-excursion'.

You are right, it isn't needed anymore, and I have removed it.

Here is the updated patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-patch, Size: 4393 bytes --]

From c19e18e3a2fd8df3547ddebce0f63c1620e7d2f6 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-kill-ring-save): Add the command.
* etc/NEWS: Mention 'diff-kill-ring-save'.  (Bug#65380)
---
 etc/NEWS             |  9 ++++++++
 lisp/vc/diff-mode.el | 51 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index b89a80aa14d..e493c1bb035 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -210,6 +210,15 @@ The host name for Kubernetes connections can be of kind
 used.  This overrides the setiing in 'tramp-kubernetes-namespace', if
 any.
 
+** Diff
+
+---
+*** New command 'diff-kill-ring-save'
+This command copies out the modified contents out of a diff, without
+having to apply it first.  If the selected range extends a hunk, the
+commands attempts to look up and copy the text between from the
+referenced file.
+
 \f
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 81e8b23ee33..5caf2cb5e5b 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -196,6 +196,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -208,7 +209,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -2108,6 +2109,54 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end &optional reverse)
+  "Save contents of the region between BEG and END akin to `kill-ring-save'.
+By default the command will copy the text that applying the diff would
+produce, along with the text between hunks.  If REVERSE is non-nil, or
+the command was invoked with a prefix argument, copy the deleted text."
+  (interactive
+   (append (if (use-region-p)
+               (list (region-beginning) (region-end))
+             (save-excursion
+               (list (diff-beginning-of-hunk)
+                     (diff-end-of-hunk))))
+           (list current-prefix-arg)))
+  (unless (derived-mode-p 'diff-mode)
+    (user-error "Command can only be invoked in a diff-buffer"))
+  (let ((parts '()))
+    (save-excursion
+      (goto-char beg)
+      (catch 'break
+        (while t
+          (let ((hunk (diff-hunk-text
+                       (buffer-substring
+                        (save-excursion (diff-beginning-of-hunk))
+                        (save-excursion (min (diff-end-of-hunk) end)))
+                       (not reverse)
+                       (save-excursion
+                         (- (point) (diff-beginning-of-hunk))))))
+            (push (substring (car hunk) (cdr hunk))
+                  parts))
+          ;; check if we have copied everything
+          (diff-end-of-hunk)
+          (when (<= end (point)) (throw 'break t))
+          ;; copy the text between hunks
+          (let ((inhibit-message t) start)
+            (save-window-excursion
+              (save-excursion
+                (forward-line -1)
+                ;; FIXME: Detect if the line we jump to doesn't match
+                ;; the line in the diff.
+                (diff-goto-source t)
+                (forward-line +1)
+                (setq start (point))))
+            (save-window-excursion
+              (diff-goto-source t)
+              (push (buffer-substring start (point))
+                    parts))))))
+    (kill-new (string-join (nreverse parts)))
+    (setq deactivate-mark t)
+    (message (if reverse "Copied original text" "Copied modified text"))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.46.0


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



-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Mon, 19 Aug 2024 19:34:48 +0000
> 
> +*** New command 'diff-kill-ring-save'
> +This command copies out the modified contents out of a diff, without
> +having to apply it first.

This could be reworded to make the effect of the command more clear.
For example:

  This command copies to the 'kill-ring' a region of text modified
  according to diffs in the current buffer, but without applying the
  diffs to the original text.

>                            If the selected range extends a hunk, the
> +commands attempts to look up and copy the text between from the
> +referenced file.                          ^^^^^^^^^^^^^^^^^

Something is missing (or redundant) in this sentence.

> +(defun diff-kill-ring-save (beg end &optional reverse)
> +  "Save contents of the region between BEG and END akin to `kill-ring-save'.

The first line should IMO say something to make it clear this command
uses diffs from the current buffer.  OTOH, the reference to
`kill-ring-save' is much less important.  So how about

  Save to `kill-ring' the result of applying diffs in region between BEG and END.

> +By default the command will copy the text that applying the diff would
> +produce, along with the text between hunks.  If REVERSE is non-nil, or
> +the command was invoked with a prefix argument, copy the deleted text."

The "deleted text" part here is unclear: who or what deletes text and
what text is deleted?

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Mon, 19 Aug 2024 19:34:48 +0000
>> 
>> +*** New command 'diff-kill-ring-save'
>> +This command copies out the modified contents out of a diff, without
>> +having to apply it first.
>
> This could be reworded to make the effect of the command more clear.
> For example:
>
>   This command copies to the 'kill-ring' a region of text modified
>   according to diffs in the current buffer, but without applying the
>   diffs to the original text.

Applied

>>                            If the selected range extends a hunk, the
>> +commands attempts to look up and copy the text between from the
>> +referenced file.                          ^^^^^^^^^^^^^^^^^
>
> Something is missing (or redundant) in this sentence.

How about "the text in between from".  Does that sound better?
                    ^^

>> +(defun diff-kill-ring-save (beg end &optional reverse)
>> +  "Save contents of the region between BEG and END akin to `kill-ring-save'.
>
> The first line should IMO say something to make it clear this command
> uses diffs from the current buffer.  OTOH, the reference to
> `kill-ring-save' is much less important.  So how about
>
>   Save to `kill-ring' the result of applying diffs in region between BEG and END.

I like it.

>> +By default the command will copy the text that applying the diff would
>> +produce, along with the text between hunks.  If REVERSE is non-nil, or
>> +the command was invoked with a prefix argument, copy the deleted text."
>
> The "deleted text" part here is unclear: who or what deletes text and
> what text is deleted?

I want to express that it copies the parts of the diff, that the
changeset removes (lines beginning with "-").  So perhaps "... copy the text
removed in the diff" would be better?

-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Tue, 20 Aug 2024 12:10:01 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>                            If the selected range extends a hunk, the
> >> +commands attempts to look up and copy the text between from the
> >> +referenced file.                          ^^^^^^^^^^^^^^^^^
> >
> > Something is missing (or redundant) in this sentence.
> 
> How about "the text in between from".  Does that sound better?
>                     ^^

If that's what you mean, I suggest "the text in-between the hunks".

> >> +By default the command will copy the text that applying the diff would
> >> +produce, along with the text between hunks.  If REVERSE is non-nil, or
> >> +the command was invoked with a prefix argument, copy the deleted text."
> >
> > The "deleted text" part here is unclear: who or what deletes text and
> > what text is deleted?
> 
> I want to express that it copies the parts of the diff, that the
> changeset removes (lines beginning with "-").  So perhaps "... copy the text
> removed in the diff" would be better?

I think "copy the text removed by the diffs", and perhaps say
explicitly that those are the lines in the hunks preceded with "-".

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Tue, 20 Aug 2024 12:10:01 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >>                            If the selected range extends a hunk, the
>> >> +commands attempts to look up and copy the text between from the
>> >> +referenced file.                          ^^^^^^^^^^^^^^^^^
>> >
>> > Something is missing (or redundant) in this sentence.
>> 
>> How about "the text in between from".  Does that sound better?
>>                     ^^
>
> If that's what you mean, I suggest "the text in-between the hunks".

Stole that idea :)

>> >> +By default the command will copy the text that applying the diff would
>> >> +produce, along with the text between hunks.  If REVERSE is non-nil, or
>> >> +the command was invoked with a prefix argument, copy the deleted text."
>> >
>> > The "deleted text" part here is unclear: who or what deletes text and
>> > what text is deleted?
>> 
>> I want to express that it copies the parts of the diff, that the
>> changeset removes (lines beginning with "-").  So perhaps "... copy the text
>> removed in the diff" would be better?
>
> I think "copy the text removed by the diffs", and perhaps say
> explicitly that those are the lines in the hunks preceded with "-".

According to `diff-font-lock-keywords' diff-mode also supports the "<"
syntax for removed parts of a hunk (I believe this comes from RCS?).
Then again, I would assume that the structure and contents of a diff
should be familiar to anyone using diff-mode seriously enough to be
interested in a command like this one?

-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Juri Linkov]

> The main scenario I had in mind was when someone modifies the beginning
> and the end of a function, and I want to copy all of it at once.  I
> agree that there is usually not much of a use for copying huge parts of
> a source file, but we get that for free when the beginning-to-end of a
> function case is handled.

I see, this is useful for the group of adjacent related hunks.

>>                                           My main use case will be
>> to copy the text of the current hunk.  Could you please support
>> this case where typing 'w' would copy the current hunk when the
>> region is not activated.
>
> Done, see below.

Thank you!

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Tue, 20 Aug 2024 16:23:09 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think "copy the text removed by the diffs", and perhaps say
> > explicitly that those are the lines in the hunks preceded with "-".
> 
> According to `diff-font-lock-keywords' diff-mode also supports the "<"
> syntax for removed parts of a hunk (I believe this comes from RCS?).

No, that's the original Diff format, before we had -c and -u.

By all means mention the "<" lines as well.

> Then again, I would assume that the structure and contents of a diff
> should be familiar to anyone using diff-mode seriously enough to be
> interested in a command like this one?

Yes, but the connection between that and what you mean by "deleted
text" might not be obvious to everyone.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Tue, 20 Aug 2024 16:23:09 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I think "copy the text removed by the diffs", and perhaps say
>> > explicitly that those are the lines in the hunks preceded with "-".
>> 
>> According to `diff-font-lock-keywords' diff-mode also supports the "<"
>> syntax for removed parts of a hunk (I believe this comes from RCS?).
>
> No, that's the original Diff format, before we had -c and -u.
>
> By all means mention the "<" lines as well.
>
>> Then again, I would assume that the structure and contents of a diff
>> should be familiar to anyone using diff-mode seriously enough to be
>> interested in a command like this one?
>
> Yes, but the connection between that and what you mean by "deleted
> text" might not be obvious to everyone.

OK on both counts, here is the updated version:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-patch, Size: 4452 bytes --]

From 56d4747ca1514fdac6abaf49b9742cdf114aa859 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-kill-ring-save): Add the command.
* etc/NEWS: Mention 'diff-kill-ring-save'.  (Bug#65380)
---
 etc/NEWS             |  9 ++++++++
 lisp/vc/diff-mode.el | 52 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index b89a80aa14d..e493c1bb035 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -210,6 +210,15 @@ The host name for Kubernetes connections can be of kind
 used.  This overrides the setiing in 'tramp-kubernetes-namespace', if
 any.
 
+** Diff
+
+---
+*** New command 'diff-kill-ring-save'
+This command copies out the modified contents out of a diff, without
+having to apply it first.  If the selected range extends a hunk, the
+commands attempts to look up and copy the text between from the
+referenced file.
+
 \f
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 81e8b23ee33..4810b9ce01c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -196,6 +196,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -208,7 +209,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -2108,6 +2109,55 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end &optional reverse)
+  "Save to `kill-ring' the result of applying diffs in region between BEG and END.
+By default the command will copy the text that applying the diff would
+produce, along with the text between hunks.  If REVERSE is non-nil, or
+the command was invoked with a prefix argument, copy the lines that the
+diff would remove (beginning with \"+\" or \"<\")."
+  (interactive
+   (append (if (use-region-p)
+               (list (region-beginning) (region-end))
+             (save-excursion
+               (list (diff-beginning-of-hunk)
+                     (diff-end-of-hunk))))
+           (list current-prefix-arg)))
+  (unless (derived-mode-p 'diff-mode)
+    (user-error "Command can only be invoked in a diff-buffer"))
+  (let ((parts '()))
+    (save-excursion
+      (goto-char beg)
+      (catch 'break
+        (while t
+          (let ((hunk (diff-hunk-text
+                       (buffer-substring
+                        (save-excursion (diff-beginning-of-hunk))
+                        (save-excursion (min (diff-end-of-hunk) end)))
+                       (not reverse)
+                       (save-excursion
+                         (- (point) (diff-beginning-of-hunk))))))
+            (push (substring (car hunk) (cdr hunk))
+                  parts))
+          ;; check if we have copied everything
+          (diff-end-of-hunk)
+          (when (<= end (point)) (throw 'break t))
+          ;; copy the text between hunks
+          (let ((inhibit-message t) start)
+            (save-window-excursion
+              (save-excursion
+                (forward-line -1)
+                ;; FIXME: Detect if the line we jump to doesn't match
+                ;; the line in the diff.
+                (diff-goto-source t)
+                (forward-line +1)
+                (setq start (point))))
+            (save-window-excursion
+              (diff-goto-source t)
+              (push (buffer-substring start (point))
+                    parts))))))
+    (kill-new (string-join (nreverse parts)))
+    (setq deactivate-mark t)
+    (message (if reverse "Copied original text" "Copied modified text"))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.46.0


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


-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Tue, 20 Aug 2024 21:35:06 +0000
> 
> > Yes, but the connection between that and what you mean by "deleted
> > text" might not be obvious to everyone.
> 
> OK on both counts, here is the updated version:

Thanks.

> +*** New command 'diff-kill-ring-save'
> +This command copies out the modified contents out of a diff, without
                       ^^^                       ^^^
One of these two "outs"s should be removed, I think.

> +having to apply it first.  If the selected range extends a hunk, the
> +commands attempts to look up and copy the text between from the
> +referenced file.

I think it is better to say "copies to the kill-ring the modified
contents..."

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Tue, 20 Aug 2024 21:35:06 +0000
>> 
>> > Yes, but the connection between that and what you mean by "deleted
>> > text" might not be obvious to everyone.
>> 
>> OK on both counts, here is the updated version:
>
> Thanks.
>
>> +*** New command 'diff-kill-ring-save'
>> +This command copies out the modified contents out of a diff, without
>                        ^^^                       ^^^
> One of these two "outs"s should be removed, I think.
>
>> +having to apply it first.  If the selected range extends a hunk, the
>> +commands attempts to look up and copy the text between from the
>> +referenced file.
>
> I think it is better to say "copies to the kill-ring the modified
> contents..."

Ugh, I forgot to amend by last patch, this is the current version:

  This command copies to the 'kill-ring' a region of text modified
  according to diffs in the current buffer, but without applying the
  diffs to the original text.  If the selected range extends a hunk, the
  commands attempts to look up and copy the text in-between the
  hunks.

I am thinking about splitting the first sentence up into

  This command copies text out of a diff to the 'kill-ring'.  By default
  it will use the text the diff would modify, without having to apply a
  hunk.  If the selected range extends a hunk, the commands attempts to
  look up and copy the text in-between the hunks.

WDYT?

Here is the patch as it is:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-command-to-copy-contents-in-a-diff-mode-buffer.patch --]
[-- Type: text/x-patch, Size: 4507 bytes --]

From 494b9ea69a501f3420c8eee1080d7d45637e39d5 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sat, 19 Aug 2023 11:47:54 +0200
Subject: [PATCH] Add command to copy contents in a diff-mode buffer

* lisp/vc/diff-mode.el (diff-mode-shared-map): Bind 'diff-kill-ring-save'.
(diff-mode-map): Ensure the "w" binding does not get prefixed.
(diff-kill-ring-save): Add the command.
* etc/NEWS: Mention 'diff-kill-ring-save'.  (Bug#65380)
---
 etc/NEWS             | 10 +++++++++
 lisp/vc/diff-mode.el | 52 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index b89a80aa14d..25ceb408e2c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -210,6 +210,16 @@ The host name for Kubernetes connections can be of kind
 used.  This overrides the setiing in 'tramp-kubernetes-namespace', if
 any.
 
+** Diff
+
+---
+*** New command 'diff-kill-ring-save'
+This command copies to the 'kill-ring' a region of text modified
+according to diffs in the current buffer, but without applying the
+diffs to the original text.  If the selected range extends a hunk, the
+commands attempts to look up and copy the text in-between the
+hunks.
+
 \f
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 81e8b23ee33..4810b9ce01c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -196,6 +196,7 @@ diff-mode-shared-map
   "RET" #'diff-goto-source
   "<mouse-2>" #'diff-goto-source
   "W" #'widen
+  "w" #'diff-kill-ring-save
   "o" #'diff-goto-source                ; other-window
   "A" #'diff-ediff-patch
   "r" #'diff-restrict-view
@@ -208,7 +209,7 @@ diff-mode-map
           ;; We want to inherit most bindings from
           ;; `diff-mode-shared-map', but not all since they may hide
           ;; useful `M-<foo>' global bindings when editing.
-          (dolist (key '("A" "r" "R" "g" "q" "W" "z"))
+          (dolist (key '("A" "r" "R" "g" "q" "W" "w" "z"))
             (keymap-set map key nil))
           map)
   ;; From compilation-minor-mode.
@@ -2108,6 +2109,55 @@ diff-goto-source
       (goto-char (+ (car pos) (cdr src)))
       (when buffer (next-error-found buffer (current-buffer))))))
 
+(defun diff-kill-ring-save (beg end &optional reverse)
+  "Save to `kill-ring' the result of applying diffs in region between BEG and END.
+By default the command will copy the text that applying the diff would
+produce, along with the text between hunks.  If REVERSE is non-nil, or
+the command was invoked with a prefix argument, copy the lines that the
+diff would remove (beginning with \"+\" or \"<\")."
+  (interactive
+   (append (if (use-region-p)
+               (list (region-beginning) (region-end))
+             (save-excursion
+               (list (diff-beginning-of-hunk)
+                     (diff-end-of-hunk))))
+           (list current-prefix-arg)))
+  (unless (derived-mode-p 'diff-mode)
+    (user-error "Command can only be invoked in a diff-buffer"))
+  (let ((parts '()))
+    (save-excursion
+      (goto-char beg)
+      (catch 'break
+        (while t
+          (let ((hunk (diff-hunk-text
+                       (buffer-substring
+                        (save-excursion (diff-beginning-of-hunk))
+                        (save-excursion (min (diff-end-of-hunk) end)))
+                       (not reverse)
+                       (save-excursion
+                         (- (point) (diff-beginning-of-hunk))))))
+            (push (substring (car hunk) (cdr hunk))
+                  parts))
+          ;; check if we have copied everything
+          (diff-end-of-hunk)
+          (when (<= end (point)) (throw 'break t))
+          ;; copy the text between hunks
+          (let ((inhibit-message t) start)
+            (save-window-excursion
+              (save-excursion
+                (forward-line -1)
+                ;; FIXME: Detect if the line we jump to doesn't match
+                ;; the line in the diff.
+                (diff-goto-source t)
+                (forward-line +1)
+                (setq start (point))))
+            (save-window-excursion
+              (diff-goto-source t)
+              (push (buffer-substring start (point))
+                    parts))))))
+    (kill-new (string-join (nreverse parts)))
+    (setq deactivate-mark t)
+    (message (if reverse "Copied original text" "Copied modified text"))))
 
 (defun diff-current-defun ()
   "Find the name of function at point.
-- 
2.46.0


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



-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Wed, 21 Aug 2024 19:40:01 +0000
> 
> Ugh, I forgot to amend by last patch, this is the current version:
> 
>   This command copies to the 'kill-ring' a region of text modified
>   according to diffs in the current buffer, but without applying the
>   diffs to the original text.  If the selected range extends a hunk, the
>   commands attempts to look up and copy the text in-between the
>   hunks.
> 
> I am thinking about splitting the first sentence up into
> 
>   This command copies text out of a diff to the 'kill-ring'.  By default
>   it will use the text the diff would modify, without having to apply a
>   hunk.  If the selected range extends a hunk, the commands attempts to
>   look up and copy the text in-between the hunks.
> 
> WDYT?

Two comments:

 . "copies text out of a diff" is hard to understand.  My
   understanding is that it takes the original text, modifies it using
   the diff hunks in the region, and copies the result to kill-ring.
   That's what I tried to describe in the first sentence that you now
   want to split.  If that interpretation is correct, then "copies
   text out of a diff" makes it much less clear.
 . The split version says "by default", but doesn't clearly say what
   will happen "not by default".  If the next sentence is that
   "non-default" case, then it is better to join these two sentences:
   "By default, ..., but if the selected text extends a hunk, ...".
   And in that latter case, I'm not sure I understand the significance
   of "by default", to tell the truth.

Or maybe I simply misunderstand what the patch does, as I'm not an
expert on diff-mode and don't know well enough what the various
functions you call do.  But then neither will the prospective reader
of this NEWS entry.

Thanks.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Wed, 21 Aug 2024 19:40:01 +0000
>> 
>> Ugh, I forgot to amend by last patch, this is the current version:
>> 
>>   This command copies to the 'kill-ring' a region of text modified
>>   according to diffs in the current buffer, but without applying the
>>   diffs to the original text.  If the selected range extends a hunk, the
>>   commands attempts to look up and copy the text in-between the
>>   hunks.
>> 
>> I am thinking about splitting the first sentence up into
>> 
>>   This command copies text out of a diff to the 'kill-ring'.  By default
>>   it will use the text the diff would modify, without having to apply a
>>   hunk.  If the selected range extends a hunk, the commands attempts to
>>   look up and copy the text in-between the hunks.
>> 
>> WDYT?
>
> Two comments:
>
>  . "copies text out of a diff" is hard to understand.  My
>    understanding is that it takes the original text, modifies it using
>    the diff hunks in the region, and copies the result to kill-ring.
>    That's what I tried to describe in the first sentence that you now
>    want to split.  If that interpretation is correct, then "copies
>    text out of a diff" makes it much less clear.
>  . The split version says "by default", but doesn't clearly say what
>    will happen "not by default".  If the next sentence is that
>    "non-default" case, then it is better to join these two sentences:
>    "By default, ..., but if the selected text extends a hunk, ...".
>    And in that latter case, I'm not sure I understand the significance
>    of "by default", to tell the truth.
>
> Or maybe I simply misunderstand what the patch does, as I'm not an
> expert on diff-mode and don't know well enough what the various
> functions you call do.  But then neither will the prospective reader
> of this NEWS entry.

No, your understanding was ring.  I'll go ahead and use your suggestion
then.

Is there anything else left to discuss?

> Thanks.

-- 
	Philip Kaludercic on peregrine

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  65380@debbugs.gnu.org
> Date: Thu, 22 Aug 2024 06:41:22 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Or maybe I simply misunderstand what the patch does, as I'm not an
> > expert on diff-mode and don't know well enough what the various
> > functions you call do.  But then neither will the prospective reader
> > of this NEWS entry.
> 
> No, your understanding was ring.  I'll go ahead and use your suggestion
> then.
> 
> Is there anything else left to discuss?

Not from me, no.

bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Thu, 22 Aug 2024 06:41:22 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Or maybe I simply misunderstand what the patch does, as I'm not an
>> > expert on diff-mode and don't know well enough what the various
>> > functions you call do.  But then neither will the prospective reader
>> > of this NEWS entry.
>> 
>> No, your understanding was ring.  I'll go ahead and use your suggestion
>> then.
>> 
>> Is there anything else left to discuss?
>
> Not from me, no.

OK, I have pushed the patch to master and am closing this report.

-- 
	Philip Kaludercic on peregrine