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]

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

[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

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