bug#52302: 28.0.50; [PATCH] Overlay strings should not increment vpos

[Eli Zaretskii]

> From: dick.r.chiang@gmail.com
> Date: Sun, 05 Dec 2021 12:37:35 -0500
> 
> >From 010b26de2993754db6bb42243b5c6c89fc5e8a50 Mon Sep 17 00:00:00 2001
> From: dickmao <dick.r.chiang@gmail.com>
> Date: Sun, 5 Dec 2021 12:25:32 -0500
> Subject: [PATCH] Overlay strings at `to_charpos` should not increment vpos
> 
> Previously, two calls to `move_it_vertically_backward (it, 0)` were
> required to get IT back to line start.  It should only ever
> take one call.

Please tell more about the motivation.  In which use cases this change
behaves better, and why?  This is a delicate code, used in many
places, so we need a very good understanding of what gets fixed.

> +           else if (skip == MOVE_LINE_CONTINUED
> +                    && it->method == GET_FROM_STRING
> +                    && IT_CHARPOS (*it) == to_charpos)
> +             /* TO_CHARPOS reached, now consuming overlay string. */

it->method == GET_FROM_STRING doesn't necessarily mean we are
it->consuming an overlay string.  It could be a string from display
it->property, for example.

> -      ++it->vpos;
> +      if (! reached_continued)
> +     ++it->vpos;

I don't think I see the connection between the above condition and the
need to increment (or not increment) VPOS.  Can you elaborate on that?

> @@ -10490,11 +10497,11 @@ move_it_vertically_backward (struct it *it, int dy)
>          || (it2.method == GET_FROM_STRING
>              && IT_CHARPOS (it2) == start_pos
>              && SREF (it2.string, IT_STRING_BYTEPOS (it2) - 1) == '\n')));
> -  eassert (IT_CHARPOS (*it) >= BEGV);
> +  eassert (IT_CHARPOS (it2) >= BEGV);
>    SAVE_IT (it3, it2, it3data);
>  
>    move_it_to (&it2, start_pos, -1, -1, -1, MOVE_TO_POS);
> -  eassert (IT_CHARPOS (*it) >= BEGV);
> +  eassert (IT_CHARPOS (it2) >= BEGV);

Why are you replacing the assertions here?

> --- a/test/src/xdisp-tests.el
> +++ b/test/src/xdisp-tests.el

What exactly is changed in this test?  It looks like purely stylistic
changes to me (which for some reason also lots the comments).  Did I
miss something?