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

From: Sean Whitton <[email-masked]>
Date: Tue, 24 Sep 2024 18:56:07 +0100

Hello,

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

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

TIL!

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

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

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

Thanks, I'll try that.

-- 
Sean Whitton