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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73387 in the body.
You can then email your comments to 73387 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n
Date: Fri, 20 Sep 2024 17:08:37 +0100

X-debbugs-cc: monnier <at> iro.umontreal.ca, dgutov <at> yandex.ru, juri <at> 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

Message #8 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: 73387 <at> debbugs.gnu.org
Cc: juri <at> linkov.net, monnier <at> iro.umontreal.ca, dgutov <at> yandex.ru
Subject: Re: 30.0.90; C-x v v in diff-mode doesn't work after C-c C-n
Date: Sun, 22 Sep 2024 13:46:44 +0100

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

Message #11 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>, 73387 <at> debbugs.gnu.org
Cc: monnier <at> iro.umontreal.ca, juri <at> linkov.net
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 24 Sep 2024 01:41:00 +0300

Hi!

On 20/09/2024 19:08, Sean Whitton wrote:
> X-debbugs-cc: monnier <at> iro.umontreal.ca, dgutov <at> yandex.ru, juri <at> 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.

Message #14 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>, 73387 <at> debbugs.gnu.org
Cc: monnier <at> iro.umontreal.ca, juri <at> linkov.net
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 24 Sep 2024 01:41:50 +0300

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.

Message #17 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 24 Sep 2024 09:32:24 +0300

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

Message #20 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Juri Linkov <juri <at> linkov.net>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 24 Sep 2024 16:54:02 +0100

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

Message #23 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>, Juri Linkov <juri <at> linkov.net>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 24 Sep 2024 20:36:32 +0300

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

Message #26 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Wed, 25 Sep 2024 07:34:51 +0100

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

Message #29 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 26 Sep 2024 02:46:08 +0300

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?

Message #32 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Fri, 27 Sep 2024 12:55:28 +0100

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

Message #35 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Fri, 27 Sep 2024 22:13:56 +0300

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

Message #38 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Mon, 30 Sep 2024 07:46:46 +0800

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

Message #41 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Mon, 30 Sep 2024 03:27:56 +0300

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?

Message #44 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Mon, 30 Sep 2024 17:38:39 +0800

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

Message #47 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Mon, 30 Sep 2024 13:11:18 +0300

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

Message #50 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Mon, 30 Sep 2024 21:10:04 +0800

[Message part 1 (text/plain, inline)]

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

[0001-New-command-diff-delete-other-hunks.patch (text/x-diff, attachment)]

Message #53 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Mon, 30 Sep 2024 21:25:26 +0800

[Message part 1 (text/plain, inline)]

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

[v2-0001-New-command-diff-delete-other-hunks.patch (text/x-diff, attachment)]

Message #56 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90;
 C-x v v in diff-mode doesn't work after C-c C-n
Date: Mon, 30 Sep 2024 17:15:37 +0300

> Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
>  Juri Linkov <juri <at> linkov.net>
> From: Sean Whitton <spwhitton <at> 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.

Message #59 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 1 Oct 2024 03:27:38 +0300

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?

Message #62 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 1 Oct 2024 03:39:13 +0300

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.

Message #65 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 08:50:43 +0800

Hello,

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

>> Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
>>  Juri Linkov <juri <at> linkov.net>
>> From: Sean Whitton <spwhitton <at> 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

Message #68 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 08:57:45 +0800

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

Message #71 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 09:01:19 +0800

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

Message #74 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 1 Oct 2024 04:15:19 +0300

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

Message #77 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 09:40:34 +0800

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

Message #80 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 1 Oct 2024 04:57:19 +0300

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.

Message #83 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, Juri Linkov <juri <at> linkov.net>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 09:55:22 -0400

> 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

Message #86 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 10:41:31 +0800

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

Message #89 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 01 Oct 2024 18:51:58 +0300

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
>   juri <at> 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.

Message #92 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, juri <at> linkov.net
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Tue, 1 Oct 2024 22:13:03 +0300

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.

Message #97 received at 73387-done <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Dmitry Gutov <dgutov <at> yandex.ru>, 73387-done <at> debbugs.gnu.org
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Wed, 02 Oct 2024 09:24:21 +0800

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

Message #100 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Wed, 02 Oct 2024 09:26:42 +0800

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

Message #103 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 spwhitton <at> spwhitton.name
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Wed, 02 Oct 2024 09:17:45 +0300

> Date: Tue, 1 Oct 2024 22:13:03 +0300
> Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca
> From: Dmitry Gutov <dgutov <at> 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."

Message #106 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Wed, 02 Oct 2024 10:15:14 +0300

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
>   juri <at> 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.

Message #109 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 03 Oct 2024 08:50:37 +0800

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

Message #112 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 03 Oct 2024 09:33:23 +0300

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
>   juri <at> 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.

Message #115 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
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 <at> spwhitton.name>
>> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
>>   juri <at> 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

Message #118 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 03 Oct 2024 14:07:36 +0300

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
>   juri <at> 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 <at> spwhitton.name>
> >> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
> >>   juri <at> 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.

Message #121 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca, juri <at> linkov.net
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 3 Oct 2024 14:15:12 +0300

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.

Message #124 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 03 Oct 2024 19:36:42 +0800

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

Message #127 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Thu, 03 Oct 2024 15:15:53 +0300

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: dgutov <at> yandex.ru,  73387 <at> debbugs.gnu.org,  monnier <at> iro.umontreal.ca,
>   juri <at> 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."

Message #130 received at 73387 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 73387 <at> debbugs.gnu.org, juri <at> linkov.net, monnier <at> iro.umontreal.ca,
 dgutov <at> yandex.ru
Subject: Re: bug#73387: 30.0.90; C-x v v in diff-mode doesn't work after C-c
 C-n
Date: Fri, 04 Oct 2024 09:41:16 +0800

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.