bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

X-debbugs-cc: monnier@iro.umontreal.ca, dgutov@yandex.ru, juri@linkov.net

Hello,

If you do C-c C-n in a diff-mode buffer with multiple hunks, a
subsequent C-x v v signals a user-error from diff-file-next.  I think
that the bug is in how diff-vc-deduce-fileset invokes diff-file-next.
I'm not sure exactly how to rework diff-vc-deduce-fileset.

It would be nice to fix this because then C-c C-n C-x v v would be a
convenient way to commit just a single hunk.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Fri 20 Sep 2024 at 05:08pm +01, Sean Whitton wrote:

> If you do C-c C-n in a diff-mode buffer with multiple hunks, a
> subsequent C-x v v signals a user-error from diff-file-next.  I think
> that the bug is in how diff-vc-deduce-fileset invokes diff-file-next.
> I'm not sure exactly how to rework diff-vc-deduce-fileset.
>
> It would be nice to fix this because then C-c C-n C-x v v would be a
> convenient way to commit just a single hunk.

I applied a brute force fix to diff-vc-deduce-fileset.
Attempting to commit a single hunk still fails because after C-c C-n the
file name header is not present, and 'git apply' can't handle a hunk
without a file name header.

We have diff-find-file-name to get the name; I wonder if we should try
to construct a fake file header?

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 22/09/2024 15:46, Sean Whitton wrote:
> Attempting to commit a single hunk still fails because after C-c C-n the
> file name header is not present, and 'git apply' can't handle a hunk
> without a file name header.
> 
> We have diff-find-file-name to get the name; I wonder if we should try
> to construct a fake file header?

Temporarily killing the "outside" hunks might be a little easier to 
implement. This was the narrowing could also span multiple files, for 
example.

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

Hi!

On 20/09/2024 19:08, Sean Whitton wrote:
> X-debbugs-cc: monnier@iro.umontreal.ca, dgutov@yandex.ru, juri@linkov.net
> 
> Hello,
> 
> If you do C-c C-n in a diff-mode buffer with multiple hunks, a
> subsequent C-x v v signals a user-error from diff-file-next.  I think
> that the bug is in how diff-vc-deduce-fileset invokes diff-file-next.
> I'm not sure exactly how to rework diff-vc-deduce-fileset.
> 
> It would be nice to fix this because then C-c C-n C-x v v would be a
> convenient way to commit just a single hunk.

This seems to work:

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4810b9ce01c..dc59621200c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -3133,11 +3133,16 @@ diff-syntax-fontify-props
  ;;;###autoload
  (defun diff-vc-deduce-fileset ()
    (let ((backend (vc-responsible-backend default-directory))
+        (start (point-min))
+        (end (point-max))
          files)
      (save-excursion
-      (goto-char (point-min))
-      (while (progn (diff-file-next) (not (eobp)))
-        (push (diff-find-file-name nil t) files)))
+      (save-restriction
+        (widen)
+        (goto-char start)
+        (diff-beginning-of-file-and-junk)
+        (while (progn (diff-file-next) (<= (point) end))
+          (push (diff-find-file-name nil t) files))))
      (list backend (delete nil (nreverse files)) nil nil 'patch)))

  (defun diff--filter-substring (str)


But to really commit the narrowed diff I think you'll need to do 
something about this line in vc-next-action

      ((eq model 'patch)
       (vc-checkin files backend nil nil nil (buffer-string)))

...to specify altered buffer contents as the diff to use.

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Juri Linkov]

>   (defun diff-vc-deduce-fileset ()
>     (let ((backend (vc-responsible-backend default-directory))
> +        (start (point-min))
> +        (end (point-max))
>           files)
>       (save-excursion
> -      (goto-char (point-min))
> -      (while (progn (diff-file-next) (not (eobp)))
> -        (push (diff-find-file-name nil t) files)))
> +      (save-restriction
> +        (widen)
> +        (goto-char start)
> +        (diff-beginning-of-file-and-junk)
> +        (while (progn (diff-file-next) (<= (point) end))
> +          (push (diff-find-file-name nil t) files))))
>       (list backend (delete nil (nreverse files)) nil nil 'patch)))

LGTM.

> But to really commit the narrowed diff I think you'll need to do
> something about this line in vc-next-action
>
>       ((eq model 'patch)
>        (vc-checkin files backend nil nil nil (buffer-string)))
>
> ...to specify altered buffer contents as the diff to use.

What is altered buffer contents?  Maybe widening is needed here as well?

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

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

>> But to really commit the narrowed diff I think you'll need to do
>> something about this line in vc-next-action
>>
>>       ((eq model 'patch)
>>        (vc-checkin files backend nil nil nil (buffer-string)))
>>
>> ...to specify altered buffer contents as the diff to use.
>
> What is altered buffer contents?  Maybe widening is needed here as well?

Yeah, could you say more, please, Dmitry?

What we basically want is a non-contiguous region, including the hunk
and the relevant file header.  Are you thinking something like two
(BEG . END) pairs specifying that region?

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 24/09/2024 18:54, Sean Whitton wrote:
> On Tue 24 Sep 2024 at 09:32am +03, Juri Linkov wrote:
> 
>>> But to really commit the narrowed diff I think you'll need to do
>>> something about this line in vc-next-action
>>>
>>>        ((eq model 'patch)
>>>         (vc-checkin files backend nil nil nil (buffer-string)))
>>>
>>> ...to specify altered buffer contents as the diff to use.
>> What is altered buffer contents?  Maybe widening is needed here as well?
> Yeah, could you say more, please, Dmitry?
> 
> What we basically want is a non-contiguous region, including the hunk
> and the relevant file header.  Are you thinking something like two
> (BEG . END) pairs specifying that region?

That's the question - what to do there (if be can), instead of passing 
the whole buffer string.

Maybe it should call again some new function inside diff-mode package 
which would return an altered patch based on the current restrictions 
(but with file headers added).

Simply calling 'widen' could counteract what seems like your intent.

OTOH, maybe what you want to do here could be reached some other way - 
e.g. instead of 'C-x n n' we would have a command which edits the diff 
buffer to leave in only the hunks intersecting the current region. When 
the subsequent (buffer-string) would do the right thing.

The latter might also be a better fit for the overall workflow we were 
thinking about (create a diff -> alter it as necessary -> commit).

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

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

> OTOH, maybe what you want to do here could be reached some other way -
> e.g. instead of 'C-x n n' we would have a command which edits the diff buffer
> to leave in only the hunks intersecting the current region. When the
> subsequent (buffer-string) would do the right thing.
>
> The latter might also be a better fit for the overall workflow we were
> thinking about (create a diff -> alter it as necessary -> commit).

Yeah, I already wrote something like that for my init.el.  I would like
to find some way to combine it with the existing C-c C-n, if we can.

Maybe:
if (equal (diff-bounds-of-hunk) (list (point-min) (point-max)),
then C-x v v prompts, "Kill all hunks but this one and commit? (y/n)" ?

Else if the buffer is narrowed, C-x v v signals a user-error that it
can't handle arbitrary narrowings.

Then it's just C-c C-n C-x v v but you're asked to confirm before all
the killing occurs, just in case.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 25/09/2024 09:34, Sean Whitton wrote:
> On Tue 24 Sep 2024 at 08:36pm +03, Dmitry Gutov wrote:
> 
>> OTOH, maybe what you want to do here could be reached some other way -
>> e.g. instead of 'C-x n n' we would have a command which edits the diff buffer
>> to leave in only the hunks intersecting the current region. When the
>> subsequent (buffer-string) would do the right thing.
>>
>> The latter might also be a better fit for the overall workflow we were
>> thinking about (create a diff -> alter it as necessary -> commit).
> Yeah, I already wrote something like that for my init.el.  I would like
> to find some way to combine it with the existing C-c C-n, if we can.
> 
> Maybe:
> if (equal (diff-bounds-of-hunk) (list (point-min) (point-max)),
> then C-x v v prompts, "Kill all hunks but this one and commit? (y/n)" ?
> 
> Else if the buffer is narrowed, C-x v v signals a user-error that it
> can't handle arbitrary narrowings.
> 
> Then it's just C-c C-n C-x v v but you're asked to confirm before all
> the killing occurs, just in case.

Suppose C-c C-n (or probably a different but similar binding) edited the 
diff instead of applying the narrowing, in a way that retained the file 
header(s), but keeping only the hunks intersecting the region or just 
the current one.

Would that work for you just as well, or do you prefer to use narrowing 
anyway, for some other reasons?

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Thu 26 Sep 2024 at 02:46am +03, Dmitry Gutov wrote:

> Suppose C-c C-n (or probably a different but similar binding) edited the diff
> instead of applying the narrowing, in a way that retained the file header(s),
> but keeping only the hunks intersecting the region or just the current one.
>
> Would that work for you just as well, or do you prefer to use narrowing
> anyway, for some other reasons?

It would work for me, and has a few advantages:

- it means you just hit 'g' afterwards, not C-x n w and then 'g'

- it fits better with our general paradigm of killing what you don't
  want to include and then committing.

On the other hand, it doesn't seem ideal that after C-c C-n you can, for
example, use C-c C-a or C-c M-k, but not C-x v v.  That could break you
out of your mental flow.

What do you think about this:

- add a command which does the kill-all-but-this-hunk (or hunks in
  region if mark active) thing -- it's generally useful.

- make C-x v v on a narrowed buffer, by default, issue a message saying
  "Cannot commit patch when narrowed, consider <binding of new command>"

- add a user option that when non-nil means C-x v v on a narrowed buffer
  automatically widens, invokes the new command, and then commits.

My thinking is that the latter behaviour is complex and so shouldn't be
the default, but once you understand what's going on then there is a
good chance you want to enable it.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 27/09/2024 14:55, Sean Whitton wrote:
> Hello,
> 
> On Thu 26 Sep 2024 at 02:46am +03, Dmitry Gutov wrote:
> 
>> Suppose C-c C-n (or probably a different but similar binding) edited the diff
>> instead of applying the narrowing, in a way that retained the file header(s),
>> but keeping only the hunks intersecting the region or just the current one.
>>
>> Would that work for you just as well, or do you prefer to use narrowing
>> anyway, for some other reasons?
> 
> It would work for me, and has a few advantages:
> 
> - it means you just hit 'g' afterwards, not C-x n w and then 'g'

Yep.

Or you could use 'undo' to get back to a previous state of the buffer.

> - it fits better with our general paradigm of killing what you don't
>    want to include and then committing.
> 
> On the other hand, it doesn't seem ideal that after C-c C-n you can, for
> example, use C-c C-a or C-c M-k, but not C-x v v.  That could break you
> out of your mental flow.

Fair point, and maybe we could support both.

> What do you think about this:
> 
> - add a command which does the kill-all-but-this-hunk (or hunks in
>    region if mark active) thing -- it's generally useful.
> 
> - make C-x v v on a narrowed buffer, by default, issue a message saying
>    "Cannot commit patch when narrowed, consider <binding of new command>"

Or it would implement that previous alternative - using the modified 
buffer string that's limited to the current narrowing.

I'm somewhat concerned about supporting both approaches (how different 
are the code paths going to be?), but if that's needed for usability, 
perhaps it's okay.

> - add a user option that when non-nil means C-x v v on a narrowed buffer
>    automatically widens, invokes the new command, and then commits.

And/or Emacs could by default prompt whether it should do that. And that 
prompting behavior could indeed be decided by a user option.

> My thinking is that the latter behaviour is complex and so shouldn't be
> the default, but once you understand what's going on then there is a
> good chance you want to enable it.

Maybe if the prompt is easy enough to understand, that will be fine...

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Fri 27 Sep 2024 at 10:13pm +03, Dmitry Gutov wrote:

>> What do you think about this:
>> - add a command which does the kill-all-but-this-hunk (or hunks in
>>    region if mark active) thing -- it's generally useful.
>> - make C-x v v on a narrowed buffer, by default, issue a message saying
>>    "Cannot commit patch when narrowed, consider <binding of new command>"
>
> Or it would implement that previous alternative - using the modified buffer
> string that's limited to the current narrowing.
>
> I'm somewhat concerned about supporting both approaches (how different are the
> code paths going to be?), but if that's needed for usability, perhaps it's
> okay.

Hmm, I thought that we thought the modified buffer string approach was
too messy.  Would you mind outlining your proposal as a whole and how it
differs from my most recent one?

>> - add a user option that when non-nil means C-x v v on a narrowed buffer
>>    automatically widens, invokes the new command, and then commits.
>
> And/or Emacs could by default prompt whether it should do that. And that
> prompting behavior could indeed be decided by a user option.
>
>> My thinking is that the latter behaviour is complex and so shouldn't be
>> the default, but once you understand what's going on then there is a
>> good chance you want to enable it.
>
> Maybe if the prompt is easy enough to understand, that will be fine...

Yeah.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 30/09/2024 02:46, Sean Whitton wrote:
> Hello,
> 
> On Fri 27 Sep 2024 at 10:13pm +03, Dmitry Gutov wrote:
> 
>>> What do you think about this:
>>> - add a command which does the kill-all-but-this-hunk (or hunks in
>>>     region if mark active) thing -- it's generally useful.
>>> - make C-x v v on a narrowed buffer, by default, issue a message saying
>>>     "Cannot commit patch when narrowed, consider <binding of new command>"
>>
>> Or it would implement that previous alternative - using the modified buffer
>> string that's limited to the current narrowing.
>>
>> I'm somewhat concerned about supporting both approaches (how different are the
>> code paths going to be?), but if that's needed for usability, perhaps it's
>> okay.
> 
> Hmm, I thought that we thought the modified buffer string approach was
> too messy.  Would you mind outlining your proposal as a whole and how it
> differs from my most recent one?

Actually, how about we start with your suggested steps, sans for the 
last one,  for now. Meaning, just aborting with a message when the 
buffer is narrowed, without the user option.

We would not be removing any existing functionality this way (this 
scenario didn't work before, after all), and we could add it later.

Would that work for your habits/scenarios?

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Mon 30 Sep 2024 at 03:27am +03, Dmitry Gutov wrote:

> On 30/09/2024 02:46, Sean Whitton wrote:
>> Hello,
>> On Fri 27 Sep 2024 at 10:13pm +03, Dmitry Gutov wrote:
>>
>>>> What do you think about this:
>>>> - add a command which does the kill-all-but-this-hunk (or hunks in
>>>>     region if mark active) thing -- it's generally useful.
>>>> - make C-x v v on a narrowed buffer, by default, issue a message saying
>>>>     "Cannot commit patch when narrowed, consider <binding of new command>"
>>>
>>> Or it would implement that previous alternative - using the modified buffer
>>> string that's limited to the current narrowing.
>>>
>>> I'm somewhat concerned about supporting both approaches (how different are the
>>> code paths going to be?), but if that's needed for usability, perhaps it's
>>> okay.
>> Hmm, I thought that we thought the modified buffer string approach was
>> too messy.  Would you mind outlining your proposal as a whole and how it
>> differs from my most recent one?
>
> Actually, how about we start with your suggested steps, sans for the last one,
> for now. Meaning, just aborting with a message when the buffer is narrowed,
> without the user option.
>
> We would not be removing any existing functionality this way (this scenario
> didn't work before, after all), and we could add it later.
>
> Would that work for your habits/scenarios?

You mean, just adding the command which kills hunks?

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 30/09/2024 12:38, Sean Whitton wrote:
> Hello,
> 
> On Mon 30 Sep 2024 at 03:27am +03, Dmitry Gutov wrote:
> 
>> On 30/09/2024 02:46, Sean Whitton wrote:
>>> Hello,
>>> On Fri 27 Sep 2024 at 10:13pm +03, Dmitry Gutov wrote:
>>>
>>>>> What do you think about this:
>>>>> - add a command which does the kill-all-but-this-hunk (or hunks in
>>>>>      region if mark active) thing -- it's generally useful.
>>>>> - make C-x v v on a narrowed buffer, by default, issue a message saying
>>>>>      "Cannot commit patch when narrowed, consider <binding of new command>"
>>>> Or it would implement that previous alternative - using the modified buffer
>>>> string that's limited to the current narrowing.
>>>>
>>>> I'm somewhat concerned about supporting both approaches (how different are the
>>>> code paths going to be?), but if that's needed for usability, perhaps it's
>>>> okay.
>>> Hmm, I thought that we thought the modified buffer string approach was
>>> too messy.  Would you mind outlining your proposal as a whole and how it
>>> differs from my most recent one?
>> Actually, how about we start with your suggested steps, sans for the last one,
>> for now. Meaning, just aborting with a message when the buffer is narrowed,
>> without the user option.
>>
>> We would not be removing any existing functionality this way (this scenario
>> didn't work before, after all), and we could add it later.
>>
>> Would that work for your habits/scenarios?
> You mean, just adding the command which kills hunks?

Just these two points:

- add a command which does the kill-all-but-this-hunk (or hunks in
   region if mark active) thing -- it's generally useful.

- make C-x v v on a narrowed buffer, by default, issue a message saying
   "Cannot commit patch when narrowed, consider <binding of new command>"

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

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

Hello,

On Mon 30 Sep 2024 at 01:11pm +03, Dmitry Gutov wrote:

> Just these two points:
>
> - add a command which does the kill-all-but-this-hunk (or hunks in
>   region if mark active) thing -- it's generally useful.
>
> - make C-x v v on a narrowed buffer, by default, issue a message saying
>   "Cannot commit patch when narrowed, consider <binding of new command>"

Okay, what do you think to the attached?

I tested the (apply #'user-error ...) by applying your patch from
up-thread, though I think there may be a bug with that patch because
after applying it I was not able to use C-x v v to commit.

(FTAOD I think the attached is valid independently of your patch.)

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-command-diff-delete-other-hunks.patch --]
[-- Type: text/x-diff, Size: 5374 bytes --]

From 34f5c95c6d04cd5d07d11214a72516b0c9ff10e4 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Mon, 30 Sep 2024 21:08:38 +0800
Subject: [PATCH] New command diff-delete-other-hunks

* lisp/vc/diff-mode.el (diff-delete-other-hunks): New
command (bug#73387).
(diff-mode-map): Bind the new command to C-c RET k.
(diff-mode-menu): New entry for the new command.
(vc-next-action): Stop, and warn, if the user attempts to commit
a patch from a narrowed buffer.
* doc/emacs/files.texi (Diff Mode):
* etc/NEWS: Document the new command.
---
 doc/emacs/files.texi |  5 +++++
 etc/NEWS             |  5 +++++
 lisp/vc/diff-mode.el | 35 +++++++++++++++++++++++++++++++++++
 lisp/vc/vc.el        | 10 ++++++++++
 4 files changed, 55 insertions(+)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index a3a8c854aa6..a32689552b1 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1703,6 +1703,11 @@ Diff Mode
 Apply all the hunks in the buffer (@code{diff-apply-buffer}).  If the
 diffs were applied successfully, save the changed buffers.
 
+@findex diff-delete-other-hunks
+@item C-c @key{RET} k
+Delete all hunks other than the current hunk.  If the region is active,
+then delete all hunks other than those the region overlaps.
+
 @findex diff-refine-hunk
 @item C-c C-b
 Highlight the changes of the hunk at point with a finer granularity
diff --git a/etc/NEWS b/etc/NEWS
index cdc7f47b7a9..a459ec18495 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -369,6 +369,11 @@ This command reverts the hunk at point (i.e., applies the reverse of the
 hunk), and then removes the hunk from the diffs.  This is useful to undo
 commits when you are in buffers generated by 'C-x v =' and 'C-x v D'.
 
++++
+*** New command 'diff-delete-other-hunks' bound to C-c RET k.
+This command deletes hunks other than the current hunk.
+It is useful to prepare a *vc-diff* buffer for committing a single hunk.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 25c6238765d..ff141e19c50 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -220,6 +220,7 @@ diff-mode-map
   "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-m k" #'diff-delete-other-hunks
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
   "C-c C-s" #'diff-split-hunk
@@ -278,6 +279,8 @@ diff-mode-menu
      :help "Kill current hunk"]
     ["Kill current file's hunks" diff-file-kill
      :help "Kill all current file's hunks"]
+    ["Delete other hunks"       diff-delete-other-hunks
+     :help "Delete hunks other than the current hunk"]
     "-----"
     ["Previous Hunk"		diff-hunk-prev
      :help "Go to the previous count'th hunk"]
@@ -814,6 +817,38 @@ diff-hunk-kill
       (goto-char (car bounds))
       (ignore-errors (diff-beginning-of-hunk t)))))
 
+;; This is not `diff-kill-other-hunks' because we might need to make
+;; copies of file headers in order to ensure the new kill ring entry
+;; would be a patch with the same meaning.  That is not implemented
+;; because it does not seem like it would be useful to anyone.
+(defun diff-delete-other-hunks (&optional beg end)
+  "Delete hunks other than this one.
+Interactively, if the region is active, then delete all hunks that the
+region does not overlap.
+When called from Lisp, the region to act upon is specified by optional
+arguments BEG and END."
+  (interactive (list (use-region-beginning) (use-region-end)))
+  (when (buffer-narrowed-p)
+    (user-error "Command is not safe in a narrowed buffer"))
+  (let ((inhibit-read-only t))
+    (save-excursion
+      (cond ((xor beg end)
+             (error "Require exactly zero or two arguments"))
+            (beg
+             (goto-char beg)
+             (setq beg (car (diff-bounds-of-hunk)))
+             (goto-char end)
+             (setq end (cadr (diff-bounds-of-hunk))))
+            (t
+             (pcase-setq `(,beg ,end) (diff-bounds-of-hunk))))
+      (delete-region end (point-max))
+      (goto-char beg)
+      (diff-beginning-of-file)
+      (diff-hunk-next)
+      (delete-region (point) beg)
+      (diff-beginning-of-file-and-junk)
+      (delete-region (point-min) (point)))))
+
 (defun diff-beginning-of-file-and-junk ()
   "Go to the beginning of file-related diff-info.
 This is like `diff-beginning-of-file' except it tries to skip back over leading
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 597a1622f5a..39e5ca424d4 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1302,6 +1302,16 @@ vc-next-action
      ;; Fileset comes from a diff-mode buffer, see
      ;; 'diff-vc-deduce-fileset', and the buffer is the patch to apply.
      ((eq model 'patch)
+      (when (buffer-narrowed-p)
+        ;; If we used `diff-restrict-view' then we may not have the file
+        ;; header and the commit will not succeed (bug#73387).
+        (apply #'user-error
+               "Cannot commit patch when narrowed; consider %s %s %s"
+               (mapcar (lambda (c)
+                         (key-description (where-is-internal c nil t)))
+                       '(widen
+                         diff-delete-other-hunks
+                         vc-next-action))))
       (vc-checkin files backend nil nil nil (buffer-string)))
      ((or (null state) (eq state 'unregistered))
       (cond (verbose
-- 
2.45.2

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

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

Hello,

On Mon 30 Sep 2024 at 09:10pm +08, Sean Whitton wrote:

> Hello,
>
> On Mon 30 Sep 2024 at 01:11pm +03, Dmitry Gutov wrote:
>
>> Just these two points:
>>
>> - add a command which does the kill-all-but-this-hunk (or hunks in
>>   region if mark active) thing -- it's generally useful.
>>
>> - make C-x v v on a narrowed buffer, by default, issue a message saying
>>   "Cannot commit patch when narrowed, consider <binding of new command>"
>
> Okay, what do you think to the attached?

Slightly simplified v2 attached -- uses mapconcat instead of (apply
#'user-error ...).

-- 
Sean Whitton

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

From 61342a10072ddfb0e8f89fbfa5a86cd4284c3142 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Mon, 30 Sep 2024 21:08:38 +0800
Subject: [PATCH v2] New command diff-delete-other-hunks

* lisp/vc/diff-mode.el (diff-delete-other-hunks): New
command (bug#73387).
(diff-mode-map): Bind the new command to C-c RET k.
(diff-mode-menu): New entry for the new command.
(vc-next-action): Stop, and warn, if the user attempts to commit
a patch from a narrowed buffer (bug#73387).
* doc/emacs/files.texi (Diff Mode):
* etc/NEWS: Document the new command.
---
 doc/emacs/files.texi |  5 +++++
 etc/NEWS             |  5 +++++
 lisp/vc/diff-mode.el | 35 +++++++++++++++++++++++++++++++++++
 lisp/vc/vc.el        | 11 +++++++++++
 4 files changed, 56 insertions(+)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index a3a8c854aa6..a32689552b1 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1703,6 +1703,11 @@ Diff Mode
 Apply all the hunks in the buffer (@code{diff-apply-buffer}).  If the
 diffs were applied successfully, save the changed buffers.
 
+@findex diff-delete-other-hunks
+@item C-c @key{RET} k
+Delete all hunks other than the current hunk.  If the region is active,
+then delete all hunks other than those the region overlaps.
+
 @findex diff-refine-hunk
 @item C-c C-b
 Highlight the changes of the hunk at point with a finer granularity
diff --git a/etc/NEWS b/etc/NEWS
index cdc7f47b7a9..90213cf342a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -369,6 +369,11 @@ This command reverts the hunk at point (i.e., applies the reverse of the
 hunk), and then removes the hunk from the diffs.  This is useful to undo
 commits when you are in buffers generated by 'C-x v =' and 'C-x v D'.
 
++++
+*** New command 'diff-delete-other-hunks' bound to C-c RET k.
+This command deletes all hunks other than the current hunk.
+It is useful to prepare a *vc-diff* buffer for committing a single hunk.
+
 ** php-ts-mode
 
 ---
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 25c6238765d..ff141e19c50 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -220,6 +220,7 @@ diff-mode-map
   "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-m k" #'diff-delete-other-hunks
   "C-c C-e" #'diff-ediff-patch
   "C-c C-n" #'diff-restrict-view
   "C-c C-s" #'diff-split-hunk
@@ -278,6 +279,8 @@ diff-mode-menu
      :help "Kill current hunk"]
     ["Kill current file's hunks" diff-file-kill
      :help "Kill all current file's hunks"]
+    ["Delete other hunks"       diff-delete-other-hunks
+     :help "Delete hunks other than the current hunk"]
     "-----"
     ["Previous Hunk"		diff-hunk-prev
      :help "Go to the previous count'th hunk"]
@@ -814,6 +817,38 @@ diff-hunk-kill
       (goto-char (car bounds))
       (ignore-errors (diff-beginning-of-hunk t)))))
 
+;; This is not `diff-kill-other-hunks' because we might need to make
+;; copies of file headers in order to ensure the new kill ring entry
+;; would be a patch with the same meaning.  That is not implemented
+;; because it does not seem like it would be useful to anyone.
+(defun diff-delete-other-hunks (&optional beg end)
+  "Delete hunks other than this one.
+Interactively, if the region is active, then delete all hunks that the
+region does not overlap.
+When called from Lisp, the region to act upon is specified by optional
+arguments BEG and END."
+  (interactive (list (use-region-beginning) (use-region-end)))
+  (when (buffer-narrowed-p)
+    (user-error "Command is not safe in a narrowed buffer"))
+  (let ((inhibit-read-only t))
+    (save-excursion
+      (cond ((xor beg end)
+             (error "Require exactly zero or two arguments"))
+            (beg
+             (goto-char beg)
+             (setq beg (car (diff-bounds-of-hunk)))
+             (goto-char end)
+             (setq end (cadr (diff-bounds-of-hunk))))
+            (t
+             (pcase-setq `(,beg ,end) (diff-bounds-of-hunk))))
+      (delete-region end (point-max))
+      (goto-char beg)
+      (diff-beginning-of-file)
+      (diff-hunk-next)
+      (delete-region (point) beg)
+      (diff-beginning-of-file-and-junk)
+      (delete-region (point-min) (point)))))
+
 (defun diff-beginning-of-file-and-junk ()
   "Go to the beginning of file-related diff-info.
 This is like `diff-beginning-of-file' except it tries to skip back over leading
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 597a1622f5a..6ebfedc9555 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1302,6 +1302,17 @@ vc-next-action
      ;; Fileset comes from a diff-mode buffer, see
      ;; 'diff-vc-deduce-fileset', and the buffer is the patch to apply.
      ((eq model 'patch)
+      (when (buffer-narrowed-p)
+        ;; If user used `diff-restrict-view' then we may not have the
+        ;; file header and the commit will not succeed (bug#73387).
+        (user-error "Cannot commit patch when narrowed; consider %s"
+                    (mapconcat (lambda (c)
+                                 (key-description
+                                  (where-is-internal c nil t)))
+                               '(widen
+                                 diff-delete-other-hunks
+                                 vc-next-action)
+                               " ")))
       (vc-checkin files backend nil nil nil (buffer-string)))
      ((or (null state) (eq state 'unregistered))
       (cond (verbose
-- 
2.45.2

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Eli Zaretskii]

> Cc: 73387@debbugs.gnu.org, monnier@iro.umontreal.ca,
>  Juri Linkov <juri@linkov.net>
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Mon, 30 Sep 2024 21:10:04 +0800
> 
> +Delete all hunks other than the current hunk.  If the region is active,
> +then delete all hunks other than those the region overlaps.

We usually describe what happens when the region is active, then what
happens "otherwise".

> +*** New command 'diff-delete-other-hunks' bound to C-c RET k.
> +This command deletes hunks other than the current hunk.
> +It is useful to prepare a *vc-diff* buffer for committing a single hunk.

This doesn't mention the active-region case.

> +(defun diff-delete-other-hunks (&optional beg end)
> +  "Delete hunks other than this one.

Which "this hunk"?  Did you mean "current hunk"?

> +When called from Lisp, the region to act upon is specified by optional
> +arguments BEG and END."                       ^^^^^^^^^^^^^^^

Passive tense alert!

> +        ;; If we used `diff-restrict-view' then we may not have the file
> +        ;; header and the commit will not succeed (bug#73387).
                    ^
Comma missing there.

Thanks.

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Mon 30 Sep 2024 at 05:15pm +03, Eli Zaretskii wrote:

>> Cc: 73387@debbugs.gnu.org, monnier@iro.umontreal.ca,
>>  Juri Linkov <juri@linkov.net>
>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Date: Mon, 30 Sep 2024 21:10:04 +0800
>>
>> +Delete all hunks other than the current hunk.  If the region is active,
>> +then delete all hunks other than those the region overlaps.
>
> We usually describe what happens when the region is active, then what
> happens "otherwise".

I think in the case of this command it's better to describe the
region-inactive case first.

>> +*** New command 'diff-delete-other-hunks' bound to C-c RET k.
>> +This command deletes hunks other than the current hunk.
>> +It is useful to prepare a *vc-diff* buffer for committing a single hunk.
>
> This doesn't mention the active-region case.

Added that.

>> +(defun diff-delete-other-hunks (&optional beg end)
>> +  "Delete hunks other than this one.
>
> Which "this hunk"?  Did you mean "current hunk"?

Indeed I did, thank you.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 30/09/2024 16:10, Sean Whitton wrote:
> +  "C-c C-m k" #'diff-delete-other-hunks

I'd like to suggest using a different binding: perhaps 'C-c C-m n'?

'k' seems to imply killing the current thing (something near point, or 
contained inside the region), whereas we want to retain the current 
hunk(s) instead. So maybe the character for (n)arrowing would be better 
- even if we use a different mechanism.

Or maybe we have some other existing term for that?

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Tue 01 Oct 2024 at 03:27am +03, Dmitry Gutov wrote:

> On 30/09/2024 16:10, Sean Whitton wrote:
>> +  "C-c C-m k" #'diff-delete-other-hunks
>
> I'd like to suggest using a different binding: perhaps 'C-c C-m n'?
>
> 'k' seems to imply killing the current thing (something near point, or
> contained inside the region), whereas we want to retain the current hunk(s)
> instead. So maybe the character for (n)arrowing would be better - even if we
> use a different mechanism.
>
> Or maybe we have some other existing term for that?

Indeed, C-c RET n is a better fit, given the existing C-c C-n.
Thank you.

I've installed this patch.  We still need to apply your fix to
diff-vc-deduce-fileset before we can close this bug.

I think we are agreed that anything cleverer should wait until we have a
clearer idea for how it could work.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 30/09/2024 16:10, Sean Whitton wrote:
> I tested the (apply #'user-error ...) by applying your patch from
> up-thread, though I think there may be a bug with that patch because
> after applying it I was not able to use C-x v v to commit.
> 
> (FTAOD I think the attached is valid independently of your patch.)

That patch was an illustration to answer the direct question (how to 
make diff-file-next work in narrowed buffer), but indeed - like 
mentioned in the same message - it didn't help on the next step.

So I'm not proposing it for inclusion, your patch should very well work 
on its own.

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Tue 01 Oct 2024 at 03:39am +03, Dmitry Gutov wrote:

> On 30/09/2024 16:10, Sean Whitton wrote:
>> I tested the (apply #'user-error ...) by applying your patch from
>> up-thread, though I think there may be a bug with that patch because
>> after applying it I was not able to use C-x v v to commit.
>> (FTAOD I think the attached is valid independently of your patch.)
>
> That patch was an illustration to answer the direct question (how to make
> diff-file-next work in narrowed buffer), but indeed - like mentioned in the
> same message - it didn't help on the next step.
>
> So I'm not proposing it for inclusion, your patch should very well work on its
> own.

I don't think that's quite right.

If you use C-c C-n in a diff-mode buffer with current master, then
C-x v v doesn't get far enough to call the user-error I added.
"No next file" is not the error message that you should get.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 01/10/2024 04:01, Sean Whitton wrote:
> I don't think that's quite right.
 >
> If you use C-c C-n in a diff-mode buffer with current master, then
> C-x v v doesn't get far enough to call the user-error I added.
> "No next file" is not the error message that you should get.

Ah, okay.

Building the list and then not using it feels kind of wasteful, though.

Should we move the check earlier?

I.e. do this (and probably remove it from vc-next-action):

diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 25c6238765d..33bd8b607f7 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -3168,6 +3168,17 @@ diff-syntax-fontify-props

  ;;;###autoload
  (defun diff-vc-deduce-fileset ()
+  (when (buffer-narrowed-p)
+    ;; If user used `diff-restrict-view' then we may not have the
+    ;; file header and the commit will not succeed (bug#73387).
+    (user-error "Cannot commit patch when narrowed; consider %s"
+                (mapconcat (lambda (c)
+                             (key-description
+                              (where-is-internal c nil t)))
+                           '(widen
+                             diff-delete-other-hunks
+                             vc-next-action)
+                           " ")))
    (let ((backend (vc-responsible-backend default-directory))
          files)
      (save-excursion

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Sean Whitton]

Hello,

On Tue 01 Oct 2024 at 04:15am +03, Dmitry Gutov wrote:

> On 01/10/2024 04:01, Sean Whitton wrote:
>> I don't think that's quite right.
>>
>> If you use C-c C-n in a diff-mode buffer with current master, then
>> C-x v v doesn't get far enough to call the user-error I added.
>> "No next file" is not the error message that you should get.
>
> Ah, okay.
>
> Building the list and then not using it feels kind of wasteful, though.
>
> Should we move the check earlier?
>
> I.e. do this (and probably remove it from vc-next-action):

Is it right that diff-vc-deduce-fileset will only ever be called in the
context of using C-x v v to commit a patch?  The name of the function
suggests that it might be used for something else, if not now then in
the future.

-- 
Sean Whitton

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

[Dmitry Gutov]

On 01/10/2024 04:40, Sean Whitton wrote:
> Is it right that diff-vc-deduce-fileset will only ever be called in the
> context of using C-x v v to commit a patch?  The name of the function
> suggests that it might be used for something else, if not now then in
> the future.

It's not a 100% guarantee but for now that's how things stand, and it 
doesn't work in a narrowed buffer. So for simplicity's sake we can make 
it show an error with such instructions, I think.

I don't mind doing it the another way too, but if its return value is 
not used, that's not a very intuitive design.

bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n

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

> What do you think about checking (eq this-command 'vc-next-action)?

Testing `this-command` is brittle, so it's a kind of "last recourse"
like advice.


        Stefan