bug#73407: 31.0.50; Add diff-discard-hunk

bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

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

X-debbugs-cc: dgutov@yandex.ru, juri@linkov.net

These patches add a new command, diff-discard-hunk, inspired by some
functionality from Magit.  It nicely complements a workflow based around
committing with C-x v v from C-x v D.  I have been using it every day
for a year or so, and I think others will find it useful, too.

Posting for comments.  Thanks!

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-user-option-diff-display-after-apply-hunk.patch --]
[-- Type: text/x-patch, Size: 2108 bytes --]

From 8d4911ed2db8a1ace388616bff35857559920f84 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Sat, 21 Sep 2024 10:51:23 +0100
Subject: [PATCH 1/2] New user option 'diff-display-after-apply-hunk'

* lisp/vc/diff-mode.el (diff-display-after-apply-hunk): New option.
(diff-apply-hunk): Use it.
* etc/NEWS: Announce the new option.
---
 etc/NEWS             | 6 ++++++
 lisp/vc/diff-mode.el | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 8bd5c7c9515..65e3a484e7a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -304,6 +304,12 @@ according to diffs in the current buffer, but without applying the diffs
 to the original text.  If the selected range extends a hunk, the
 command attempts to look up and copy the text in-between the hunks.
 
+---
+*** New user option 'diff-display-after-apply-hunk'.
+This option can be customized to nil to inhibit how, by default,
+'diff-apply-hunk' displays the changed text after applying the hunk.
+It can also be let-bound by Lisp code wrapping 'diff-apply-hunk'.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4810b9ce01c..8af155c79e0 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -95,6 +95,10 @@ diff-advance-after-apply-hunk
   "Non-nil means `diff-apply-hunk' will move to the next hunk after applying."
   :type 'boolean)
 
+(defcustom diff-display-after-apply-hunk t
+  "Non-nil means `diff-apply-hunk' will display the buffer it just changed."
+  :type 'boolean)
+
 (defcustom diff-mode-hook nil
   "Run after setting up the `diff-mode' major mode."
   :type 'hook
@@ -2024,7 +2028,8 @@ diff-apply-hunk
 	(delete-region (car pos) (cdr pos))
 	(insert (car new)))
       ;; Display BUF in a window
-      (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
+      (when diff-display-after-apply-hunk
+        (set-window-point (display-buffer buf) (+ (car pos) (cdr new))))
       (diff-hunk-status-msg line-offset (xor switched reverse) nil)
       (when diff-advance-after-apply-hunk
 	(diff-hunk-next))))))
-- 
2.39.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-New-command-diff-discard-hunk.patch --]
[-- Type: text/x-patch, Size: 3973 bytes --]

From 076a74fd4798bd3908aa0cd4ed07ed85e03559c1 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Sat, 21 Sep 2024 11:16:18 +0100
Subject: [PATCH 2/2] New command 'diff-discard-hunk'

* lisp/vc/diff-mode.el (diff-discard-hunk): New command.
(diff-mode-map): Bind it to C-c C-k.
(diff-mode-menu): New entry "Discard hunk".
* doc/emacs/files.texi (Diff Mode):
* etc/NEWS: Document the new command.
---
 doc/emacs/files.texi | 11 +++++++++++
 etc/NEWS             |  5 +++++
 lisp/vc/diff-mode.el | 23 +++++++++++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 709cb0910e6..04ecfabc1ee 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1682,6 +1682,17 @@ Diff Mode
 version.  If @code{diff-jump-to-old-file} is non-@code{nil}, apply the
 hunk to the ``old'' version of the file instead.
 
+@findex diff-discard-hunk
+@item C-c C-k
+Reverse-apply this hunk to the target file, and then kill it
+(@code{diff-discard-hunk}).  Unless the buffer visiting the target file
+was already modified, save it.
+
+This command is useful in buffers generated by @w{@kbd{C-x v =}} and
+@w{@kbd{C-x v D}} (@pxref{Old Revisions}).  You can use this command to
+remove hunks you never intend to commit, such as temporary debug prints,
+and the like.
+
 @findex diff-apply-buffer
 @item C-c @key{RET} a
 Apply all the hunks in the buffer (@code{diff-apply-buffer}).  If the
diff --git a/etc/NEWS b/etc/NEWS
index 65e3a484e7a..8da68eded82 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -310,6 +310,11 @@ This option can be customized to nil to inhibit how, by default,
 'diff-apply-hunk' displays the changed text after applying the hunk.
 It can also be let-bound by Lisp code wrapping 'diff-apply-hunk'.
 
++++
+*** New command 'diff-discard-hunk' bound to C-c C-k.
+This command reverse-applies the hunk at point, and then kills it.
+This is useful in buffers generated C-x v = and C-x v D.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 8af155c79e0..3a6bc62b78a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -222,6 +222,7 @@ diff-mode-map
   "C-x 4 A" #'diff-add-change-log-entries-other-window
   ;; Misc operations.
   "C-c C-a" #'diff-apply-hunk
+  "C-c C-k" #'diff-discard-hunk
   "C-c C-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
@@ -246,6 +247,8 @@ diff-mode-menu
      :help "Apply the current hunk to the source file and go to the next"]
     ["Test applying hunk"	diff-test-hunk
      :help "See whether it's possible to apply the current hunk"]
+    ["Discard hunk"             diff-discard-hunk
+     :help "Reverse-apply and then kill the current hunk."]
     ["Apply all hunks"		diff-apply-buffer
      :help "Apply all hunks in the current diff buffer"]
     ["Apply diff with Ediff"	diff-ediff-patch
@@ -2055,6 +2058,26 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
+(defun diff-discard-hunk ()
+  "Reverse-apply and then kill the hunk at point.
+Unless the buffer was already modified, save the changed buffer.
+
+This command is useful in buffers generated by \\[vc-diff] and \\[vc-root-diff].
+You can use `diff-hunk-kill' to remove hunks you intend to commit later.
+You can use this command to remove hunks you never intend to commit,
+such as temporary debug prints, and the like."
+  (interactive)
+  (let* ((diff-advance-after-apply-hunk nil)
+         (diff-display-after-apply-hunk nil)
+         (buffer (car (diff-find-source-location)))
+         (to-save (and (not (buffer-modified-p buffer))
+                       buffer)))
+    (diff-apply-hunk t)
+    (diff-hunk-kill)
+    (when to-save
+      (with-current-buffer to-save
+        (save-buffer)))))
+
 (defun diff-apply-buffer ()
   "Apply the diff in the entire diff buffer.
 When applying all hunks was successful, then save the changed buffers."
-- 
2.39.5

bug#73407: 31.0.50; Add diff-discard-hunk

[Dmitry Gutov]

Hi!

On 21/09/2024 13:19, Sean Whitton wrote:
> X-debbugs-cc: dgutov@yandex.ru, juri@linkov.net
> 
> These patches add a new command, diff-discard-hunk, inspired by some
> functionality from Magit.  It nicely complements a workflow based around
> committing with C-x v v from C-x v D.  I have been using it every day
> for a year or so, and I think others will find it useful, too.

I'd like to clarify one thing first: for 'C-x v v' to work correctly in 
a diff buffer (such as one produced by 'C-x v D') you don't have to 
synchronize any changes in that buffer back to disk before making the 
commit. In fact, we went to some effort to make this a non-requirement.

So when you just want to edit the patch before the commit, you can do so 
already, for example using 'M-k' (diff-hunk-kill) - or just 'k' if the 
buffer is read-only.

But of course if the idea is to really "discard" some changes, that works.

bug#73407: 31.0.50; Add diff-discard-hunk

[Juri Linkov]

> These patches add a new command, diff-discard-hunk, inspired by some
> functionality from Magit.  It nicely complements a workflow based around
> committing with C-x v v from C-x v D.  I have been using it every day
> for a year or so, and I think others will find it useful, too.
>
> Posting for comments.  Thanks!

It was hard to understand how it's related to C-x v v.
But then I realized it saves time to do what currently is done with:
'C-c C-r' (diff-reverse-direction)
'C-c C-a' (diff-apply-hunk)
'k' (diff-hunk-kill).

> +(defcustom diff-display-after-apply-hunk t
> +  "Non-nil means `diff-apply-hunk' will display the buffer it just changed."
> +  :type 'boolean)
> +
> @@ -2024,7 +2028,8 @@ diff-apply-hunk
> -      (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
> +      (when diff-display-after-apply-hunk
> +        (set-window-point (display-buffer buf) (+ (car pos) (cdr new))))

Probably this change is not needed if 'diff-discard-hunk'
could avoid using 'diff-apply-hunk', and instead rely
on the algorithm from 'diff-apply-buffer'.  Also optional
region arguments 'beg' and 'end' could be added to
'diff-apply-buffer'.

bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

Hello,

On Tue 24 Sep 2024 at 01:52am +03, Dmitry Gutov wrote:

> Hi!
>
> On 21/09/2024 13:19, Sean Whitton wrote:
>> X-debbugs-cc: dgutov@yandex.ru, juri@linkov.net
>> These patches add a new command, diff-discard-hunk, inspired by some
>> functionality from Magit.  It nicely complements a workflow based around
>> committing with C-x v v from C-x v D.  I have been using it every day
>> for a year or so, and I think others will find it useful, too.
>
> I'd like to clarify one thing first: for 'C-x v v' to work correctly in a diff
> buffer (such as one produced by 'C-x v D') you don't have to synchronize any
> changes in that buffer back to disk before making the commit. In fact, we went
> to some effort to make this a non-requirement.

Right.  I believe I was responsible for some of that effort!

> So when you just want to edit the patch before the commit, you can do so
> already, for example using 'M-k' (diff-hunk-kill) - or just 'k' if the buffer
> is read-only.
>
> But of course if the idea is to really "discard" some changes, that works.

Yeah, this is about really discarding the changes.

It's like using C-x v D as a kind of review view.  You use it for
committing what you want to keep and getting rid of temporary changes
you made just for development purposes, such as additional debug prints.

When I used Magit I used its magit-discard for this a lot (the docstring
for that function talks about conflicts but it's also useful without those).

One question I have is whether this should offer a y/n prompt to confirm
discarding the change.  Do you have an opinion on that?

-- 
Sean Whitton

bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

Hello,

On Tue 24 Sep 2024 at 09:36am +03, Juri Linkov wrote:

>> These patches add a new command, diff-discard-hunk, inspired by some
>> functionality from Magit.  It nicely complements a workflow based around
>> committing with C-x v v from C-x v D.  I have been using it every day
>> for a year or so, and I think others will find it useful, too.
>>
>> Posting for comments.  Thanks!
>
> It was hard to understand how it's related to C-x v v.
> But then I realized it saves time to do what currently is done with:
> 'C-c C-r' (diff-reverse-direction)
> 'C-c C-a' (diff-apply-hunk)
> 'k' (diff-hunk-kill).

Yeah, exactly.

>> +(defcustom diff-display-after-apply-hunk t
>> +  "Non-nil means `diff-apply-hunk' will display the buffer it just changed."
>> +  :type 'boolean)
>> +
>> @@ -2024,7 +2028,8 @@ diff-apply-hunk
>> -      (set-window-point (display-buffer buf) (+ (car pos) (cdr new)))
>> +      (when diff-display-after-apply-hunk
>> +        (set-window-point (display-buffer buf) (+ (car pos) (cdr new))))
>
> Probably this change is not needed if 'diff-discard-hunk'
> could avoid using 'diff-apply-hunk', and instead rely
> on the algorithm from 'diff-apply-buffer'.  Also optional
> region arguments 'beg' and 'end' could be added to
> 'diff-apply-buffer'.

I'll look into that, thanks.

-- 
Sean Whitton

bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

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

Hello,

Thank you both for looking.  Here is a revised patch based on Juri's
ideas about implementation, and also adding a y/n confirmation, since
this operation is destructive.

Let me know what you think!

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-New-command-diff-discard-hunk.patch --]
[-- Type: text/x-patch, Size: 5926 bytes --]

From 2eccebb6159e6c384e1cb7a543f259d2146d8b81 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Tue, 24 Sep 2024 09:38:43 +0100
Subject: [PATCH v2] New command diff-discard-hunk

* lisp/vc/diff-mode.el (diff-discard-hunk): New command (bug#73407).
(diff-apply-buffer): New optional BEG and END arguments.  Return
nil if buffers were saved, or the number of failed applications.
(diff-mode-map): Bind diff-discard-hunk to C-c C-k.
(diff-mode-menu): New entry "Discard hunk".
* doc/emacs/files.texi (Diff Mode):
* etc/NEWS: Document the new command.
---
 doc/emacs/files.texi | 11 +++++++++++
 etc/NEWS             |  5 +++++
 lisp/vc/diff-mode.el | 40 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 709cb0910e6..04ecfabc1ee 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1682,6 +1682,17 @@ Diff Mode
 version.  If @code{diff-jump-to-old-file} is non-@code{nil}, apply the
 hunk to the ``old'' version of the file instead.
 
+@findex diff-discard-hunk
+@item C-c C-k
+Reverse-apply this hunk to the target file, and then kill it
+(@code{diff-discard-hunk}).  Unless the buffer visiting the target file
+was already modified, save it.
+
+This command is useful in buffers generated by @w{@kbd{C-x v =}} and
+@w{@kbd{C-x v D}} (@pxref{Old Revisions}).  You can use this command to
+remove hunks you never intend to commit, such as temporary debug prints,
+and the like.
+
 @findex diff-apply-buffer
 @item C-c @key{RET} a
 Apply all the hunks in the buffer (@code{diff-apply-buffer}).  If the
diff --git a/etc/NEWS b/etc/NEWS
index c1a0524c8ba..3edd827d15c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -341,6 +341,11 @@ according to diffs in the current buffer, but without applying the diffs
 to the original text.  If the selected range extends a hunk, the
 command attempts to look up and copy the text in-between the hunks.
 
++++
+*** New command 'diff-discard-hunk' bound to C-c C-k.
+This command reverse-applies the hunk at point, and then kills it.
+This is useful in buffers generated C-x v = and C-x v D.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4810b9ce01c..a48ba8e717a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -218,6 +218,7 @@ diff-mode-map
   "C-x 4 A" #'diff-add-change-log-entries-other-window
   ;; Misc operations.
   "C-c C-a" #'diff-apply-hunk
+  "C-c C-k" #'diff-discard-hunk
   "C-c C-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
@@ -242,6 +243,8 @@ diff-mode-menu
      :help "Apply the current hunk to the source file and go to the next"]
     ["Test applying hunk"	diff-test-hunk
      :help "See whether it's possible to apply the current hunk"]
+    ["Discard hunk"             diff-discard-hunk
+     :help "Reverse-apply and then kill the current hunk."]
     ["Apply all hunks"		diff-apply-buffer
      :help "Apply all hunks in the current diff buffer"]
     ["Apply diff with Ediff"	diff-ediff-patch
@@ -2050,15 +2053,39 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
-(defun diff-apply-buffer ()
+(defun diff-discard-hunk ()
+  "Reverse-apply and then kill the hunk at point.  Save changed buffer.
+
+This command is useful in buffers generated by \\[vc-diff] and \\[vc-root-diff].
+You can use `diff-hunk-kill' to remove hunks you intend to commit later.
+You can use this command to remove hunks you never intend to commit,
+such as temporary debug prints, and the like."
+  (interactive)
+  (when (yes-or-no-p "Discard this hunk?")
+    (cl-destructuring-bind (beg end) (diff-bounds-of-hunk)
+      (diff-reverse-direction beg end)
+      (condition-case ret
+          (diff-apply-buffer beg end)
+        ;; Reversing the hunk is an implementation detail, so ensure the
+        ;; user never sees it.
+        (error (diff-reverse-direction beg end)
+               (signal (car ret) (cdr ret)))
+        (:success (if ret
+                      (diff-reverse-direction beg end)
+                    (diff-hunk-kill)))))))
+
+(defun diff-apply-buffer (&optional beg end)
   "Apply the diff in the entire diff buffer.
-When applying all hunks was successful, then save the changed buffers."
+When applying all hunks was successful, then save the changed buffers.
+When called from Lisp with optional arguments, restrict the application
+to hunks lying between BEG and END.  Returns nil if buffers were saved,
+or the number of failed applications."
   (interactive)
   (let ((buffer-edits nil)
         (failures 0)
         (diff-refine nil))
     (save-excursion
-      (goto-char (point-min))
+      (goto-char (or beg (point-min)))
       (diff-beginning-of-hunk t)
       (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
                           (diff-find-source-location nil nil)))
@@ -2068,6 +2095,7 @@ diff-apply-buffer
                      (t (setq failures (1+ failures))))
                (and (not (eq (prog1 (point) (ignore-errors (diff-hunk-next)))
                              (point)))
+                    (or (not end) (< (point) end))
                     (looking-at-p diff-hunk-header-re)))))
     (cond ((zerop failures)
            (dolist (buf-edits (reverse buffer-edits))
@@ -2080,9 +2108,11 @@ diff-apply-buffer
                    (delete-region (car pos) (cdr pos))
                    (insert (car dst))))
                (save-buffer)))
-           (message "Saved %d buffers" (length buffer-edits)))
+           (message "Saved %d buffers" (length buffer-edits))
+           nil)
           (t
-           (message "%d hunks failed; no buffers changed" failures)))))
+           (message "%d hunks failed; no buffers changed" failures)
+           failures))))
 
 (defalias 'diff-mouse-goto-source #'diff-goto-source)
 
-- 
2.39.5

bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

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

Hello,

On Tue 24 Sep 2024 at 09:40am +01, Sean Whitton wrote:

> Hello,
>
> Thank you both for looking.  Here is a revised patch based on Juri's
> ideas about implementation, and also adding a y/n confirmation, since
> this operation is destructive.
>
> Let me know what you think!

I missed updating something in the info; here is v3.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-New-command-diff-discard-hunk.patch --]
[-- Type: text/x-patch, Size: 5892 bytes --]

From a384ea810d388dfb685a3694e851457bb5e668dd Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Tue, 24 Sep 2024 09:38:43 +0100
Subject: [PATCH v3] New command diff-discard-hunk

* lisp/vc/diff-mode.el (diff-discard-hunk): New command (bug#73407).
(diff-apply-buffer): New optional BEG and END arguments.  Return
nil if buffers were saved, or the number of failed applications.
(diff-mode-map): Bind diff-discard-hunk to C-c C-k.
(diff-mode-menu): New entry "Discard hunk".
* doc/emacs/files.texi (Diff Mode):
* etc/NEWS: Document the new command.
---
 doc/emacs/files.texi | 10 ++++++++++
 etc/NEWS             |  5 +++++
 lisp/vc/diff-mode.el | 40 +++++++++++++++++++++++++++++++++++-----
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 709cb0910e6..78d90a914aa 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1682,6 +1682,16 @@ Diff Mode
 version.  If @code{diff-jump-to-old-file} is non-@code{nil}, apply the
 hunk to the ``old'' version of the file instead.
 
+@findex diff-discard-hunk
+@item C-c C-k
+Reverse-apply this hunk to the target file, and then kill it
+(@code{diff-discard-hunk}).  Save the buffer visiting the target file.
+
+This command is useful in buffers generated by @w{@kbd{C-x v =}} and
+@w{@kbd{C-x v D}} (@pxref{Old Revisions}).  You can use this command to
+remove hunks you never intend to commit, such as temporary debug prints,
+and the like.
+
 @findex diff-apply-buffer
 @item C-c @key{RET} a
 Apply all the hunks in the buffer (@code{diff-apply-buffer}).  If the
diff --git a/etc/NEWS b/etc/NEWS
index c1a0524c8ba..3edd827d15c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -341,6 +341,11 @@ according to diffs in the current buffer, but without applying the diffs
 to the original text.  If the selected range extends a hunk, the
 command attempts to look up and copy the text in-between the hunks.
 
++++
+*** New command 'diff-discard-hunk' bound to C-c C-k.
+This command reverse-applies the hunk at point, and then kills it.
+This is useful in buffers generated C-x v = and C-x v D.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4810b9ce01c..a48ba8e717a 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -218,6 +218,7 @@ diff-mode-map
   "C-x 4 A" #'diff-add-change-log-entries-other-window
   ;; Misc operations.
   "C-c C-a" #'diff-apply-hunk
+  "C-c C-k" #'diff-discard-hunk
   "C-c C-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
@@ -242,6 +243,8 @@ diff-mode-menu
      :help "Apply the current hunk to the source file and go to the next"]
     ["Test applying hunk"	diff-test-hunk
      :help "See whether it's possible to apply the current hunk"]
+    ["Discard hunk"             diff-discard-hunk
+     :help "Reverse-apply and then kill the current hunk."]
     ["Apply all hunks"		diff-apply-buffer
      :help "Apply all hunks in the current diff buffer"]
     ["Apply diff with Ediff"	diff-ediff-patch
@@ -2050,15 +2053,39 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
-(defun diff-apply-buffer ()
+(defun diff-discard-hunk ()
+  "Reverse-apply and then kill the hunk at point.  Save changed buffer.
+
+This command is useful in buffers generated by \\[vc-diff] and \\[vc-root-diff].
+You can use `diff-hunk-kill' to remove hunks you intend to commit later.
+You can use this command to remove hunks you never intend to commit,
+such as temporary debug prints, and the like."
+  (interactive)
+  (when (yes-or-no-p "Discard this hunk?")
+    (cl-destructuring-bind (beg end) (diff-bounds-of-hunk)
+      (diff-reverse-direction beg end)
+      (condition-case ret
+          (diff-apply-buffer beg end)
+        ;; Reversing the hunk is an implementation detail, so ensure the
+        ;; user never sees it.
+        (error (diff-reverse-direction beg end)
+               (signal (car ret) (cdr ret)))
+        (:success (if ret
+                      (diff-reverse-direction beg end)
+                    (diff-hunk-kill)))))))
+
+(defun diff-apply-buffer (&optional beg end)
   "Apply the diff in the entire diff buffer.
-When applying all hunks was successful, then save the changed buffers."
+When applying all hunks was successful, then save the changed buffers.
+When called from Lisp with optional arguments, restrict the application
+to hunks lying between BEG and END.  Returns nil if buffers were saved,
+or the number of failed applications."
   (interactive)
   (let ((buffer-edits nil)
         (failures 0)
         (diff-refine nil))
     (save-excursion
-      (goto-char (point-min))
+      (goto-char (or beg (point-min)))
       (diff-beginning-of-hunk t)
       (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
                           (diff-find-source-location nil nil)))
@@ -2068,6 +2095,7 @@ diff-apply-buffer
                      (t (setq failures (1+ failures))))
                (and (not (eq (prog1 (point) (ignore-errors (diff-hunk-next)))
                              (point)))
+                    (or (not end) (< (point) end))
                     (looking-at-p diff-hunk-header-re)))))
     (cond ((zerop failures)
            (dolist (buf-edits (reverse buffer-edits))
@@ -2080,9 +2108,11 @@ diff-apply-buffer
                    (delete-region (car pos) (cdr pos))
                    (insert (car dst))))
                (save-buffer)))
-           (message "Saved %d buffers" (length buffer-edits)))
+           (message "Saved %d buffers" (length buffer-edits))
+           nil)
           (t
-           (message "%d hunks failed; no buffers changed" failures)))))
+           (message "%d hunks failed; no buffers changed" failures)
+           failures))))
 
 (defalias 'diff-mouse-goto-source #'diff-goto-source)
 
-- 
2.39.5

bug#73407: 31.0.50; Add diff-discard-hunk

[Dmitry Gutov]

On 24/09/2024 09:36, Juri Linkov wrote:
> 'C-c C-r' (diff-reverse-direction)
> 'C-c C-a' (diff-apply-hunk)

Also known as 'C-u C-c C-a'.

bug#73407: 31.0.50; Add diff-discard-hunk

[Eli Zaretskii]

> Cc: 73407@debbugs.gnu.org
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Tue, 24 Sep 2024 09:40:30 +0100
> 
> +@findex diff-discard-hunk
> +@item C-c C-k
> +Reverse-apply this hunk to the target file, and then kill it
> +(@code{diff-discard-hunk}).  Unless the buffer visiting the target file
> +was already modified, save it.

We should come up with a better name for this command.
diff-discard-hunk tells me the command will discard the hunk, but says
nothing about applying it, let alone reverse-applying it.  How about
diff-revert-hunk, or maybe diff-revert-and-discard?

> +This command is useful in buffers generated by @w{@kbd{C-x v =}} and
> +@w{@kbd{C-x v D}} (@pxref{Old Revisions}).  You can use this command to
> +remove hunks you never intend to commit, such as temporary debug prints,
> +and the like.

This text could also use some clarification: "remove hunks you never
intended to commit" only hints on what it actually does.  A better
description would be something like "undo some of the changes you made
that you didn't intend".  I wouldn't mention "commit" at all, since
AFAIU this command is not specific to VC and doesn't require a VCS.

> -           (message "%d hunks failed; no buffers changed" failures)))))
> +           (message "%d hunks failed; no buffers changed" failures)
> +           failures))))

This comes from existing text, but still: what does the above say when
there's only 1 failure? does it say "1 hunks failed"?  If so, can we
improve the handling of singular/plural here?

Thanks.

bug#73407: 31.0.50; Add diff-discard-hunk

[Dmitry Gutov]

On 24/09/2024 11:42, Sean Whitton wrote:
> +*** New command 'diff-discard-hunk' bound to C-c C-k.

I'm a little concerned about the binding.

We have 'diff-hunk-kill' on M-k (and also 'k'), and this new addition is 
actually more destructive. I'm expecting users will trip over the 
difference and call one of them when they wanted to call the other.

bug#73407: 31.0.50; Add diff-discard-hunk

[Dmitry Gutov]

On 24/09/2024 15:21, Eli Zaretskii wrote:

> We should come up with a better name for this command.
> diff-discard-hunk tells me the command will discard the hunk, but says
> nothing about applying it, let alone reverse-applying it.  How about
> diff-revert-hunk, or maybe diff-revert-and-discard?

diff-revert-and-kill, maybe.

bug#73407: 31.0.50; Add diff-discard-hunk

[Dmitry Gutov]

On 24/09/2024 10:53, Sean Whitton wrote:

>> I'd like to clarify one thing first: for 'C-x v v' to work correctly in a diff
>> buffer (such as one produced by 'C-x v D') you don't have to synchronize any
>> changes in that buffer back to disk before making the commit. In fact, we went
>> to some effort to make this a non-requirement.
> 
> Right.  I believe I was responsible for some of that effort!

Oh, right. Sorry.

>> So when you just want to edit the patch before the commit, you can do so
>> already, for example using 'M-k' (diff-hunk-kill) - or just 'k' if the buffer
>> is read-only.
>>
>> But of course if the idea is to really "discard" some changes, that works.
> 
> Yeah, this is about really discarding the changes.
> 
> It's like using C-x v D as a kind of review view.  You use it for
> committing what you want to keep and getting rid of temporary changes
> you made just for development purposes, such as additional debug prints.
> 
> When I used Magit I used its magit-discard for this a lot (the docstring
> for that function talks about conflicts but it's also useful without those).
> 
> One question I have is whether this should offer a y/n prompt to confirm
> discarding the change.  Do you have an opinion on that?

I guess we should? 'diff-hl-revert-hunk' also prompts, but it has a user 
option to disable that.

bug#73407: 31.0.50; Add diff-discard-hunk

[Óscar Fuentes]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 24/09/2024 15:21, Eli Zaretskii wrote:
>
>> We should come up with a better name for this command.
>> diff-discard-hunk tells me the command will discard the hunk, but says
>> nothing about applying it, let alone reverse-applying it.  How about
>> diff-revert-hunk, or maybe diff-revert-and-discard?
>
> diff-revert-and-kill, maybe.

If this command works on hunks, please say it on the name:

diff-revert-and-kill-hunk

It makes the intent clear and greatly helps discoverability when using
certain tools that navigate the command names.

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

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

Hello,

On Tue 24 Sep 2024 at 03:21pm +03, Eli Zaretskii wrote:

> We should come up with a better name for this command.
> diff-discard-hunk tells me the command will discard the hunk, but says
> nothing about applying it, let alone reverse-applying it.  How about
> diff-revert-hunk, or maybe diff-revert-and-discard?

Oh, nice.  I was just following Magit, but including "revert" is better,
indeed.  I think I prefer diff-revert-and-kill-hunk.

>> +This command is useful in buffers generated by @w{@kbd{C-x v =}} and
>> +@w{@kbd{C-x v D}} (@pxref{Old Revisions}).  You can use this command to
>> +remove hunks you never intend to commit, such as temporary debug prints,
>> +and the like.
>
> This text could also use some clarification: "remove hunks you never
> intended to commit" only hints on what it actually does.  A better
> description would be something like "undo some of the changes you made
> that you didn't intend".  I wouldn't mention "commit" at all, since
> AFAIU this command is not specific to VC and doesn't require a VCS.

Yes, this is the diff-mode manual, so that makes sense.
I've made a similar change in the docstring but kept a reference to
committing, there.

>> -           (message "%d hunks failed; no buffers changed" failures)))))
>> +           (message "%d hunks failed; no buffers changed" failures)
>> +           failures))))
>
> This comes from existing text, but still: what does the above say when
> there's only 1 failure? does it say "1 hunks failed"?  If so, can we
> improve the handling of singular/plural here?

I've installed a fix for that.

On Tue 24 Sep 2024 at 03:22pm +03, Dmitry Gutov wrote:

> I'm a little concerned about the binding.
>
> We have 'diff-hunk-kill' on M-k (and also 'k'), and this new addition is
> actually more destructive. I'm expecting users will trip over the difference
> and call one of them when they wanted to call the other.

Yeah, and thinking about it more, it also doesn't really match what C-c
C-k does elsewhere in Emacs.  I've made it C-c M-r in the attached v4.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v4-0001-New-command-diff-revert-and-kill-hunk.patch --]
[-- Type: text/x-patch, Size: 6207 bytes --]

From fc5e3a5be18f9b1532490c4c6b81b90b83c0acf6 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Tue, 24 Sep 2024 09:38:43 +0100
Subject: [PATCH v4] New command diff-revert-and-kill-hunk

* lisp/vc/diff-mode.el (diff-revert-and-kill-hunk): New
command (bug#73407).
(diff-apply-buffer): New optional BEG and END arguments.  Return
nil if buffers were saved, or the number of failed applications.
(diff-mode-map): Bind diff-revert-and-kill-hunk to C-c M-r.
(diff-mode-menu): New entry "Revert and kill hunk".
* doc/emacs/files.texi (Diff Mode):
* etc/NEWS: Document the new command.
---
 doc/emacs/files.texi | 11 +++++++++++
 etc/NEWS             |  5 +++++
 lisp/vc/diff-mode.el | 41 ++++++++++++++++++++++++++++++++++++-----
 3 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 709cb0910e6..434fb30484a 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1682,6 +1682,17 @@ Diff Mode
 version.  If @code{diff-jump-to-old-file} is non-@code{nil}, apply the
 hunk to the ``old'' version of the file instead.
 
+@findex diff-revert-and-kill-hunk
+@item C-c M-r
+Reverse-apply this hunk to the target file, and then kill it
+(@code{diff-revert-and-kill-hunk}).  Save the buffer visiting the target
+file.
+
+This command is useful in buffers generated by @w{@kbd{C-x v =}} and
+@w{@kbd{C-x v D}} (@pxref{Old Revisions}).  These buffers present you
+with a view of the changes you've made, and then you can use this
+command to drop changes you didn't intend, or no longer want.
+
 @findex diff-apply-buffer
 @item C-c @key{RET} a
 Apply all the hunks in the buffer (@code{diff-apply-buffer}).  If the
diff --git a/etc/NEWS b/etc/NEWS
index c1a0524c8ba..7b85b9cbc04 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -341,6 +341,11 @@ according to diffs in the current buffer, but without applying the diffs
 to the original text.  If the selected range extends a hunk, the
 command attempts to look up and copy the text in-between the hunks.
 
++++
+*** New command 'diff-revert-and-kill-hunk' bound to C-c M-r.
+This command reverse-applies the hunk at point, and then kills it.
+This is useful in buffers generated by C-x v = and C-x v D.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index be3d94db011..9e39e58e69b 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -218,6 +218,7 @@ diff-mode-map
   "C-x 4 A" #'diff-add-change-log-entries-other-window
   ;; Misc operations.
   "C-c C-a" #'diff-apply-hunk
+  "C-c M-r" #'diff-revert-and-kill-hunk
   "C-c C-m a" #'diff-apply-buffer
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
@@ -242,6 +243,8 @@ diff-mode-menu
      :help "Apply the current hunk to the source file and go to the next"]
     ["Test applying hunk"	diff-test-hunk
      :help "See whether it's possible to apply the current hunk"]
+    ["Revert and kill hunk"     diff-revert-and-kill-hunk
+     :help "Reverse-apply and then kill the current hunk."]
     ["Apply all hunks"		diff-apply-buffer
      :help "Apply all hunks in the current diff buffer"]
     ["Apply diff with Ediff"	diff-ediff-patch
@@ -2050,15 +2053,40 @@ diff-kill-applied-hunks
           (diff-hunk-kill)
         (diff-hunk-next)))))
 
-(defun diff-apply-buffer ()
+(defun diff-revert-and-kill-hunk ()
+  "Reverse-apply and then kill the hunk at point.  Save changed buffer.
+
+This command is useful in buffers generated by \\[vc-diff] and \\[vc-root-diff],
+especially when preparing to commit the patch with \\[vc-next-action].
+You can use \\<diff-mode-map>\\[diff-hunk-kill] to temporarily remove changes that you intend to
+include in a separate commit or commits, and you can use this command
+to permanently drop changes you didn't intend, or no longer want."
+  (interactive)
+  (when (yes-or-no-p "Really reverse-apply and kill this hunk?")
+    (cl-destructuring-bind (beg end) (diff-bounds-of-hunk)
+      (diff-reverse-direction beg end)
+      (condition-case ret
+          (diff-apply-buffer beg end)
+        ;; Reversing the hunk is an implementation detail, so ensure the
+        ;; user never sees it.
+        (error (diff-reverse-direction beg end)
+               (signal (car ret) (cdr ret)))
+        (:success (if ret
+                      (diff-reverse-direction beg end)
+                    (diff-hunk-kill)))))))
+
+(defun diff-apply-buffer (&optional beg end)
   "Apply the diff in the entire diff buffer.
-When applying all hunks was successful, then save the changed buffers."
+When applying all hunks was successful, then save the changed buffers.
+When called from Lisp with optional arguments, restrict the application
+to hunks lying between BEG and END.  Returns nil if buffers were saved,
+or the number of failed applications."
   (interactive)
   (let ((buffer-edits nil)
         (failures 0)
         (diff-refine nil))
     (save-excursion
-      (goto-char (point-min))
+      (goto-char (or beg (point-min)))
       (diff-beginning-of-hunk t)
       (while (pcase-let ((`(,buf ,line-offset ,pos ,_src ,dst ,switched)
                           (diff-find-source-location nil nil)))
@@ -2068,6 +2096,7 @@ diff-apply-buffer
                      (t (setq failures (1+ failures))))
                (and (not (eq (prog1 (point) (ignore-errors (diff-hunk-next)))
                              (point)))
+                    (or (not end) (< (point) end))
                     (looking-at-p diff-hunk-header-re)))))
     (cond ((zerop failures)
            (dolist (buf-edits (reverse buffer-edits))
@@ -2080,10 +2109,12 @@ diff-apply-buffer
                    (delete-region (car pos) (cdr pos))
                    (insert (car dst))))
                (save-buffer)))
-           (message "Saved %d buffers" (length buffer-edits)))
+           (message "Saved %d buffers" (length buffer-edits))
+           nil)
           (t
            (message "%d %s failed; no buffers changed"
-                    failures (if (> failures 1) "hunks" "hunk"))))))
+                    failures (if (> failures 1) "hunks" "hunk"))
+           failures))))
 
 (defalias 'diff-mouse-goto-source #'diff-goto-source)
 
-- 
2.39.5

bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

Hello,

On Tue 24 Sep 2024 at 03:27pm +03, Dmitry Gutov wrote:

> Oh, right. Sorry.

Not at all!

>  'diff-hl-revert-hunk' also prompts, but it has a user option to
> disable that.

Hmm.  I've added prompting.  Let's try using it for a while and see if
we feel like such a user option is needed for this one too.

-- 
Sean Whitton

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Juri Linkov]

>>> -           (message "%d hunks failed; no buffers changed" failures)))))
>>> +           (message "%d hunks failed; no buffers changed" failures)
>>> +           failures))))
>>
>> This comes from existing text, but still: what does the above say when
>> there's only 1 failure? does it say "1 hunks failed"?  If so, can we
>> improve the handling of singular/plural here?
>
> I've installed a fix for that.

Another variant would be to use 'ngettext'.

>> I'm a little concerned about the binding.
>>
>> We have 'diff-hunk-kill' on M-k (and also 'k'), and this new addition is
>> actually more destructive. I'm expecting users will trip over the difference
>> and call one of them when they wanted to call the other.
>
> Yeah, and thinking about it more, it also doesn't really match what C-c
> C-k does elsewhere in Emacs.  I've made it C-c M-r in the attached v4.

There is the existing 'C-c C-m' submap that could be used with e.g.
'C-c RET C-k'.


> +      (diff-reverse-direction beg end)
> +      (condition-case ret
> +          (diff-apply-buffer beg end)
> +        ;; Reversing the hunk is an implementation detail, so ensure the
> +        ;; user never sees it.
> +        (error (diff-reverse-direction beg end)
> +               (signal (car ret) (cdr ret)))
> +        (:success (if ret
> +                      (diff-reverse-direction beg end)

Instead of reversing the direction back and forth,
it would be simpler to add another optional argument
to 'diff-apply-buffer' to delegate it to the
REVERSE argument of 'diff-find-source-location'.

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

Hello,

On Tue 24 Sep 2024 at 07:59pm +03, Juri Linkov wrote:

>>>> -           (message "%d hunks failed; no buffers changed" failures)))))
>>>> +           (message "%d hunks failed; no buffers changed" failures)
>>>> +           failures))))
>>>
>>> This comes from existing text, but still: what does the above say when
>>> there's only 1 failure? does it say "1 hunks failed"?  If so, can we
>>> improve the handling of singular/plural here?
>>
>> I've installed a fix for that.
>
> Another variant would be to use 'ngettext'.

TIL!

>>> I'm a little concerned about the binding.
>>>
>>> We have 'diff-hunk-kill' on M-k (and also 'k'), and this new addition is
>>> actually more destructive. I'm expecting users will trip over the difference
>>> and call one of them when they wanted to call the other.
>>
>> Yeah, and thinking about it more, it also doesn't really match what C-c
>> C-k does elsewhere in Emacs.  I've made it C-c M-r in the attached v4.
>
> There is the existing 'C-c C-m' submap that could be used with e.g.
> 'C-c RET C-k'.

I considered that, but I think I'd prefer that it's shorter sequence if
possible, because you might want to use it several times.  By contrast,
C-c C-m a is a command you probably just want once at a time.

>> +      (diff-reverse-direction beg end)
>> +      (condition-case ret
>> +          (diff-apply-buffer beg end)
>> +        ;; Reversing the hunk is an implementation detail, so ensure the
>> +        ;; user never sees it.
>> +        (error (diff-reverse-direction beg end)
>> +               (signal (car ret) (cdr ret)))
>> +        (:success (if ret
>> +                      (diff-reverse-direction beg end)
>
> Instead of reversing the direction back and forth,
> it would be simpler to add another optional argument
> to 'diff-apply-buffer' to delegate it to the
> REVERSE argument of 'diff-find-source-location'.

Thanks, I'll try that.

-- 
Sean Whitton

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  dgutov@yandex.ru,  73407@debbugs.gnu.org
> Date: Tue, 24 Sep 2024 19:59:04 +0300
> 
> >>> -           (message "%d hunks failed; no buffers changed" failures)))))
> >>> +           (message "%d hunks failed; no buffers changed" failures)
> >>> +           failures))))
> >>
> >> This comes from existing text, but still: what does the above say when
> >> there's only 1 failure? does it say "1 hunks failed"?  If so, can we
> >> improve the handling of singular/plural here?
> >
> > I've installed a fix for that.
> 
> Another variant would be to use 'ngettext'.

That's what I had in mind...

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

Hello,

On Tue 24 Sep 2024 at 09:07pm +03, Eli Zaretskii wrote:

>> From: Juri Linkov <juri@linkov.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  dgutov@yandex.ru,  73407@debbugs.gnu.org
>> Date: Tue, 24 Sep 2024 19:59:04 +0300
>>
>> >>> -           (message "%d hunks failed; no buffers changed" failures)))))
>> >>> +           (message "%d hunks failed; no buffers changed" failures)
>> >>> +           failures))))
>> >>
>> >> This comes from existing text, but still: what does the above say when
>> >> there's only 1 failure? does it say "1 hunks failed"?  If so, can we
>> >> improve the handling of singular/plural here?
>> >
>> > I've installed a fix for that.
>>
>> Another variant would be to use 'ngettext'.
>
> That's what I had in mind...

Do you mean like this?

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index be3d94db011..9677fbb9175 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -2083,7 +2083,7 @@ diff-apply-buffer
            (message "Saved %d buffers" (length buffer-edits)))
           (t
            (message "%d %s failed; no buffers changed"
-                    failures (if (> failures 1) "hunks" "hunk"))))))
+                    failures (ngettext "hunk" "hunks" failures))))))

 (defalias 'diff-mouse-goto-source #'diff-goto-source)

-- 
Sean Whitton

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: Juri Linkov <juri@linkov.net>,  dgutov@yandex.ru,  73407@debbugs.gnu.org
> Date: Wed, 25 Sep 2024 07:36:42 +0100
> 
> >> Another variant would be to use 'ngettext'.
> >
> > That's what I had in mind...
> 
> Do you mean like this?

Yes.  But it is considered a better style not to break the message
into separate words, so the below would be even better:

  (message (ngettext "%d hunk failed; no buffers changed"
                     "%d hunks failed; no buffers changed"
                     failures))

bug#73407: [PATCH v4] bug#73407: 31.0.50; Add diff-discard-hunk

[Sean Whitton]

Hello,

On Wed 25 Sep 2024 at 03:17pm +03, Eli Zaretskii wrote:

> Yes.  But it is considered a better style not to break the message
> into separate words, so the below would be even better:
>
>   (message (ngettext "%d hunk failed; no buffers changed"
>                      "%d hunks failed; no buffers changed"
>                      failures))

Thanks -- installed.

-- 
Sean Whitton

bug#73407: 31.0.50; Add diff-revert-and-kill-hunk

[Sean Whitton]

Hello,

I believe all outstanding issues are resolved, so I've installed this, and am
closing the bug.  Thank you for the input, which improved my work, as ever.

-- 
Sean Whitton