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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
>   juri@linkov.net
> Date: Tue, 01 Oct 2024 08:50:43 +0800
> 
> Hello,
> 
> On Mon 30 Sep 2024 at 05:15pm +03, Eli Zaretskii wrote:
> 
> >> +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.

I don't see why.

I can explain the rationale for doing the opposite: when describing
the operation with active region, we usually start with "If the region
is active..." or with some other similar conditional language.  This
then makes it natural to explain the behavior in the other cases,
because the active-region one is clearly conditional, and thus not the
general case.

By contrast, your text starts with the description of behavior that
has no conditions, and thus is perceived as the complete and
exhaustive.  Then the next sentence is "out of the blue" because the
reader does not expect any such additions, being just told what the
command does, period.

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 18:51, Eli Zaretskii wrote:
> I can explain the rationale for doing the opposite: when describing
> the operation with active region, we usually start with "If the region
> is active..." or with some other similar conditional language.  This
> then makes it natural to explain the behavior in the other cases,
> because the active-region one is clearly conditional, and thus not the
> general case.

When using commands like kill-region, or kill-ring-save, etc, having an 
active region is the "usual" case (among other things because without it 
the user doesn't see the bounds that would be acted on).

With the command in question, not having an active region will likely be 
the more frequent case, and the boundaries of the text acted upon are 
already visible in the buffer.

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

[Eli Zaretskii]

> Date: Tue, 1 Oct 2024 22:13:03 +0300
> Cc: 73387@debbugs.gnu.org, juri@linkov.net, monnier@iro.umontreal.ca
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 01/10/2024 18:51, Eli Zaretskii wrote:
> > I can explain the rationale for doing the opposite: when describing
> > the operation with active region, we usually start with "If the region
> > is active..." or with some other similar conditional language.  This
> > then makes it natural to explain the behavior in the other cases,
> > because the active-region one is clearly conditional, and thus not the
> > general case.
> 
> When using commands like kill-region, or kill-ring-save, etc, having an 
> active region is the "usual" case (among other things because without it 
> the user doesn't see the bounds that would be acted on).
> 
> With the command in question, not having an active region will likely be 
> the more frequent case, and the boundaries of the text acted upon are 
> already visible in the buffer.

Thanks for providing the rationale.

However, IME, clarity of documentation is so important that it trumps
many other considerations of the order of describing things, so I'm
still not happy with your proposed order.  If you search diff-mode.el
for "region", you will see that this very file uses the conventions I
explained above.  Here's one example:

  (defun diff-context->unified (start end &optional to-context)
    "Convert context diffs to unified diffs.
  START and END are either taken from the region
  \(when it is highlighted) or else cover the whole buffer.
  With a prefix argument, convert unified format to context format."

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 06:51pm +03, Eli Zaretskii wrote:

> I can explain the rationale for doing the opposite: when describing
> the operation with active region, we usually start with "If the region
> is active..." or with some other similar conditional language.  This
> then makes it natural to explain the behavior in the other cases,
> because the active-region one is clearly conditional, and thus not the
> general case.
>
> By contrast, your text starts with the description of behavior that
> has no conditions, and thus is perceived as the complete and
> exhaustive.  Then the next sentence is "out of the blue" because the
> reader does not expect any such additions, being just told what the
> command does, period.

Yes, I know what you mean about that sentence coming out of the blue.
But as Dmitry says, the behaviour when the region is active is a bonus
feature, not really what the command is about.

Do you think maybe this is better:

    Delete all hunks other than the current hunk.
    As a special case, when called interactively with an active region ...

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
>   juri@linkov.net
> Date: Wed, 02 Oct 2024 09:26:42 +0800
> 
> On Tue 01 Oct 2024 at 06:51pm +03, Eli Zaretskii wrote:
> 
> > I can explain the rationale for doing the opposite: when describing
> > the operation with active region, we usually start with "If the region
> > is active..." or with some other similar conditional language.  This
> > then makes it natural to explain the behavior in the other cases,
> > because the active-region one is clearly conditional, and thus not the
> > general case.
> >
> > By contrast, your text starts with the description of behavior that
> > has no conditions, and thus is perceived as the complete and
> > exhaustive.  Then the next sentence is "out of the blue" because the
> > reader does not expect any such additions, being just told what the
> > command does, period.
> 
> Yes, I know what you mean about that sentence coming out of the blue.
> But as Dmitry says, the behaviour when the region is active is a bonus
> feature, not really what the command is about.
> 
> Do you think maybe this is better:
> 
>     Delete all hunks other than the current hunk.
>     As a special case, when called interactively with an active region ...

This doesn't really address the issue, IMO.

The main problem I have here is with the "all" part of the first
sentence, which is then contradicted by the second.  Also, in other
places, you describe the behavior when region is active, but not when
it isn't.

Here's what I would suggest to say in the manual:

  @item C-c @key{RET} k
  Delete diff hunks other than the current one.  If the region is
  active, this command deletes the hunks overlapped by the region;
  otherwise it deletes all the hunks other than the current hunk.  Do
  @emph{not} invoke this command in a narrowed buffer, as that could
  produce unexpected results.

And this is for the command's doc string:

  (defun diff-delete-other-hunks (&optional beg end)
    "Delete diff hunks other than the current one.
  Interactively, if the region is active, delete all the hunks which
  the region overlaps; otherwise delete all the hunks except the
  current one.
  When calling from Lisp, pass BEG and END as the bounds of region in
  which to delete diff hunks; BEG and END omitted or nil means to
  delete all the hunks but the one which contains point.

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

[Sean Whitton]

Hello,

On Wed 02 Oct 2024 at 10:15am +03, Eli Zaretskii wrote:

> This doesn't really address the issue, IMO.
>
> The main problem I have here is with the "all" part of the first
> sentence, which is then contradicted by the second.  Also, in other
> places, you describe the behavior when region is active, but not when
> it isn't.
>
> Here's what I would suggest to say in the manual:
>
>   @item C-c @key{RET} k
>   Delete diff hunks other than the current one.  If the region is
>   active, this command deletes the hunks overlapped by the region;
>   otherwise it deletes all the hunks other than the current hunk.  Do
>   @emph{not} invoke this command in a narrowed buffer, as that could
>   produce unexpected results.

Okay, I've rewritten based on this, though I haven't included the
imprecation, because invoking it in a narrowed buffer could never
produce unexpected results -- the function will always do nothing.

> And this is for the command's doc string:
>
>   (defun diff-delete-other-hunks (&optional beg end)
>     "Delete diff hunks other than the current one.
>   Interactively, if the region is active, delete all the hunks which
>   the region overlaps; otherwise delete all the hunks except the
>   current one.
>   When calling from Lisp, pass BEG and END as the bounds of region in
>   which to delete diff hunks; BEG and END omitted or nil means to
>   delete all the hunks but the one which contains point.

Done, thanks.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
>   juri@linkov.net
> Date: Thu, 03 Oct 2024 08:50:37 +0800
> 
> >   @item C-c @key{RET} k
> >   Delete diff hunks other than the current one.  If the region is
> >   active, this command deletes the hunks overlapped by the region;
> >   otherwise it deletes all the hunks other than the current hunk.  Do
> >   @emph{not} invoke this command in a narrowed buffer, as that could
> >   produce unexpected results.
> 
> Okay, I've rewritten based on this, though I haven't included the
> imprecation, because invoking it in a narrowed buffer could never
> produce unexpected results -- the function will always do nothing.

Then why do we signal user-error in that case?  I think this should be
explained in the manual.

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 03 Oct 2024 at 09:33am +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
>>   juri@linkov.net
>> Date: Thu, 03 Oct 2024 08:50:37 +0800
>>
>> >   @item C-c @key{RET} k
>> >   Delete diff hunks other than the current one.  If the region is
>> >   active, this command deletes the hunks overlapped by the region;
>> >   otherwise it deletes all the hunks other than the current hunk.  Do
>> >   @emph{not} invoke this command in a narrowed buffer, as that could
>> >   produce unexpected results.
>>
>> Okay, I've rewritten based on this, though I haven't included the
>> imprecation, because invoking it in a narrowed buffer could never
>> produce unexpected results -- the function will always do nothing.
>
> Then why do we signal user-error in that case?  I think this should be
> explained in the manual.

I think I am using user-error wrongly.

Shall I just switch it to 'message', and rearrange the control flow so
nothing else happens?

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
>   juri@linkov.net
> Date: Thu, 03 Oct 2024 15:06:58 +0800
> 
> Hello,
> 
> On Thu 03 Oct 2024 at 09:33am +03, Eli Zaretskii wrote:
> 
> >> From: Sean Whitton <spwhitton@spwhitton.name>
> >> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
> >>   juri@linkov.net
> >> Date: Thu, 03 Oct 2024 08:50:37 +0800
> >>
> >> >   @item C-c @key{RET} k
> >> >   Delete diff hunks other than the current one.  If the region is
> >> >   active, this command deletes the hunks overlapped by the region;
> >> >   otherwise it deletes all the hunks other than the current hunk.  Do
> >> >   @emph{not} invoke this command in a narrowed buffer, as that could
> >> >   produce unexpected results.
> >>
> >> Okay, I've rewritten based on this, though I haven't included the
> >> imprecation, because invoking it in a narrowed buffer could never
> >> produce unexpected results -- the function will always do nothing.
> >
> > Then why do we signal user-error in that case?  I think this should be
> > explained in the manual.
> 
> I think I am using user-error wrongly.
> 
> Shall I just switch it to 'message', and rearrange the control flow so
> nothing else happens?

Maybe so, but I'd still would like to hear the rationale for telling
the user "Cannot commit patch when narrowed; consider 'widen'".  If
indeed we "cannot commit" in this case, signaling an error is TRT, and
replacing that with a message is not, because it will do what you
claim in the message you cannot.

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

[Dmitry Gutov]

On 03/10/2024 14:07, Eli Zaretskii wrote:
> Maybe so, but I'd still would like to hear the rationale for telling
> the user "Cannot commit patch when narrowed; consider 'widen'".  If
> indeed we "cannot commit" in this case,

Indeed we can't, it's not supported.

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 03 Oct 2024 at 02:07pm +03, Eli Zaretskii wrote:

> Maybe so, but I'd still would like to hear the rationale for telling
> the user "Cannot commit patch when narrowed; consider 'widen'".  If
> indeed we "cannot commit" in this case, signaling an error is TRT, and
> replacing that with a message is not, because it will do what you
> claim in the message you cannot.

Okay.  I do agree with you that the manual should state this limitation.
This is what it says right now:

    This command does not work in a narrowed buffer because deleting
    hunks safely requires access to the file headers.

So, the only difference from what you wrote is that it doesn't tell the
user they *mustn't*, only that they *can't*.

I think this addresses your concern, but please let me know if not.

-- 
Sean Whitton

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

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru,  73387@debbugs.gnu.org,  monnier@iro.umontreal.ca,
>   juri@linkov.net
> Date: Thu, 03 Oct 2024 19:36:42 +0800
> 
> > Maybe so, but I'd still would like to hear the rationale for telling
> > the user "Cannot commit patch when narrowed; consider 'widen'".  If
> > indeed we "cannot commit" in this case, signaling an error is TRT, and
> > replacing that with a message is not, because it will do what you
> > claim in the message you cannot.
> 
> Okay.  I do agree with you that the manual should state this limitation.
> This is what it says right now:
> 
>     This command does not work in a narrowed buffer because deleting
>     hunks safely requires access to the file headers.
> 
> So, the only difference from what you wrote is that it doesn't tell the
> user they *mustn't*, only that they *can't*.
> 
> I think this addresses your concern, but please let me know if not.

That's fine, but I would suggest to clarify what you mean by "file
headers".  Something like "...because deleting hunks requires access
to the part of the diffs where file names are specified."

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 03 Oct 2024 at 03:15pm +03, Eli Zaretskii wrote:

> That's fine, but I would suggest to clarify what you mean by "file
> headers".  Something like "...because deleting hunks requires access
> to the part of the diffs where file names are specified."

Done, thanks.

-- 
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

[Sean Whitton]

Hello,

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

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

What do you think about checking (eq this-command 'vc-next-action)?
All we want to do is provide a pointer for the user to find C-c C-m n.

-- 
Sean Whitton

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

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:57am +03, Dmitry Gutov wrote:

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

Okay, done that now, and closing the bug.  Thanks!

-- 
Sean Whitton