bug#65380: [PATCH] Add command to copy contents in a diff-mode buffer

[Philip Kaludercic <philipk@posteo.net>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  65380@debbugs.gnu.org
>> Date: Sun, 18 Aug 2024 15:29:25 +0000
>> 
>> > +          ;; copy the text between hunks
>> > +          (let (start)
>> > +            (save-window-excursion
>> > +              (save-excursion
>> > +                (forward-line -1)
>> > +                (diff-goto-source)
>> > +                (forward-line +1)
>> > +                (setq start (point))))
>> 
>> One issue I still have here is that the `forwarad-line' might scroll the
>> window, which `save-window-excursion' doesn't restore.
>
> I don't understand what you are saying here.  As long as a Lisp
> program runs and doesn't force redisplay (via sit-for or explicit
> calls to 'redisplay' or somesuch), forwarad-line cannot cause any
> scroll of the window, because what scrolls the window is redisplay
> when it kicks in and finds that point is outside of the window.  So if
> the above code takes care to restore point before it finishes, the
> window won't scroll.  Or what am I missing?

No, you were right, and I manged to locate what was causing the issue.
I had a (revert-buffer t t t) at the beginning of the function.
Wrapping `save-window-start' around just that expression also prevented
the displayed portion of the window from changing.

Thinking about the proposal again, it might seem better to avoid calling
`revert-buffer' in the first place.  The problem I was concerned with
was that if a .diff was outdated, jumping to the original file might not
do the right thing and copy the wrong inter-hunk text.  Now I realise
that `revert-buffer' doesn't always help resolve this either, e.g. if
copying text from an old .patch file.

The proper solution would be to try and detect these kinds of
inconsistencies and signal and error instead.

>> To fix this I have written a general macro for subr.el to restore
>> the scroll position:
>
> Let's first make sure we understand what happens here before we add
> such a macro.  (It also has some conceptual problems of its own, but
> let's defer that until we actually sure it is needed.)

It is not needed, but I would be interested in what the conceptual
errors are.

-- 
	Philip Kaludercic on peregrine