bug#56393: Actually fix the long lines display bug

[Eli Zaretskii]

> Date: Mon, 18 Jul 2022 12:58:59 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Gerd Möllmann <gerd.moellmann@gmail.com>, larsi@gnus.org, 
>     56393@debbugs.gnu.org
> 
> > This is isearch.el trying to establish whether the position of the match 
> > is visible in the window.  To do that, it starts from the current 
> > window-start position (which happens to be 1), and simulates display of 
> > the buffer text portion up until point or until the end of window, 
> > whichever comes first, to determine whether point is beyond the end of 
> > the window.  Your change makes the search start from some much later 
> > position instead, which could very well produce incorrect results: 
> > pos-visible-in-window-p could decide that point _is_ visible, when it 
> > really isn't.  (It doesn't happen in this particular case because the 
> > newline is far away -- at the very end of the buffer -- so it isn't 
> > visible in both cases.  But that's sheer luck.)
> 
> Well, this happens only in buffers with long lines, and only when we are 
> inside a long line

Is the last part really guaranteed?  AFAIU, the detection of long
lines scans the entire buffer, so if there's a long line _anywhere_ in
the buffer, the narrowing is applied, even if point is in no-so-long
lines.  Or am I missing something?

> so from my point of view it works as expected

"As expected" in what sense?  Suppose we really are in a long line,
the Isearch match is really outside the window, but if we use
point-10000 as BEGV point seems to be _inside_ the window -- in this
case the feature implemented in isearch-update for slow terminals will
not do its thing, right?

> and moreover the risk is small.

Not sure this should pacify the fears.

> Would the following be better from your point of view?
> 
> diff --git a/src/xdisp.c b/src/xdisp.c
> index d69d7440bc..572ad2b854 100644
> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -8668,18 +8668,11 @@ get_visually_first_element (struct it *it)
>     bool string_p = STRINGP (it->string) || it->s;
>     ptrdiff_t eob = (string_p ? it->bidi_it.string.schars : ZV);
>     ptrdiff_t bob;
> +  ptrdiff_t obegv = BEGV;
> 
> -  SET_WITH_NARROWED_BEGV (it, bob, string_p ? 0 : BEGV);
> -
> -  /* Reseat again when, as a consequence of the SET_WITH_NARROWED_BEGV
> -     above, the iterator is before bob.  */
> -  if (!string_p && IT_CHARPOS (*it) < bob)
> -    {
> -      struct text_pos pos;
> -      pos.charpos = bob;
> -      pos.bytepos = CHAR_TO_BYTE (bob);
> -      reseat (it, pos, true);
> -    }
> +  SET_WITH_NARROWED_BEGV (it, bob,
> +                         string_p ? 0 :
> +                         IT_BYTEPOS (*it) < BEGV ? obegv : BEGV);

I guess it's better, as it reduces the number of cases where the
problem could happen, at the price of making those cases slower.

> > More generally, if you look at redisplay_window, you will see that about 
> > 2/3 of its code tries very hard to reuse the previous window-start 
> > position, before it gives up and looks for a new starting position.  So 
> > in any situation where the previous window-start is far enough before 
> > point, all that code will basically work with a buffer position that is 
> > at risk of being before the "narrowed" BEGV.  Thus, any code there which 
> > tries stuff like start_display+move_it_to will risk hitting this kind of 
> > problems -- either FETCH_BYTE will crash, or we risk producing the wrong 
> > result because we force the code to jump to the "narrowed" BEGV before 
> > doing anything, while its caller expects results relative to a different 
> > position.
> 
> I understand.  But note that temporarily narrowing the buffer happens only 
> at a few well-chosen places, which are situated rather low in the 
> abstraction layers, so the effect on other parts of the code is nil.

I think I agree with everything except the "nil" part ;-)

> > I think this is because the display engine assumes that BEGV stays put 
> > during the entire redisplay_window lifetime, i.e. that all of the 
> > subroutines it calls see the same value of BEGV.  This is no longer so 
> > on the branch, and I wonder whether and how we should handle this new 
> > situation to keep the display code stable and reliable.
> >
> 
> I don't know.

Neither do I.  Still thinking about it.  I'd be interested to hear
Gerd's thoughts on this.