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

[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

[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

[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

[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

[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...)