bug#51776: master 9741924: In insert_file_contents, always set windows' point markers.

bug#51776: master 9741924: In insert_file_contents, always set windows' point markers.

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi Alan,

Alan Mackenzie [2021-11-12 13:44:52] wrote:
>     In insert_file_contents, always set windows' point markers.
>     This fixes bug #51776.
>
>     * src/fileio.c (restore_window_points): Restore a w->mpoint even
>     when that marker originally pointed into the unchanged area near
>     BOB or EOB.  This prevents that window's point being moved a long
>     way from its starting place due to the removal of the central part
>     of the buffer by insert_file_contents.

Hmm... my understanding of the code says that if `oldppos <= same_at_beg` then
your change is harmless but unnecessary because the marker
is still at `oldpos` anyway.

But when `oldppos > same_at_beg` your change is harmful because the
marker has properly preserved its exact position in the text whereas
your code will arbitrarily move it to `oldpos` (which is even
potentially past Z if the revert shortened the buffer).

So maybe the better fix is to just change

    XFIXNUM (oldpos) < same_at_end

into

    XFIXNUM (oldpos) <= same_at_end

Or am I missing something?

[ Not sure what we should do when `oldpos == same_at_beg == same_at_end`, OTOH.
  I suspect staying at `same_at_beg` might be the better choice in that
  case.  ]


        Stefan


>  src/fileio.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/fileio.c b/src/fileio.c
> index 3c13d3f..a7b1649 100644
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -3827,6 +3827,7 @@ restore_window_points (Lisp_Object window_markers, ptrdiff_t inserted,
>  	Lisp_Object car = XCAR (window_markers);
>  	Lisp_Object marker = XCAR (car);
>  	Lisp_Object oldpos = XCDR (car);
> +	ptrdiff_t newpos;
>  	if (MARKERP (marker) && FIXNUMP (oldpos)
>  	    && XFIXNUM (oldpos) > same_at_start
>  	    && XFIXNUM (oldpos) < same_at_end)
> @@ -3834,10 +3835,12 @@ restore_window_points (Lisp_Object window_markers, ptrdiff_t inserted,
>  	    ptrdiff_t oldsize = same_at_end - same_at_start;
>  	    ptrdiff_t newsize = inserted;
>  	    double growth = newsize / (double)oldsize;
> -	    ptrdiff_t newpos
> -	      = same_at_start + growth * (XFIXNUM (oldpos) - same_at_start);
> -	    Fset_marker (marker, make_fixnum (newpos), Qnil);
> +	    newpos = same_at_start
> +	      + growth * (XFIXNUM (oldpos) - same_at_start);
>  	  }
> +	else
> +	  newpos = XFIXNUM (oldpos);
> +	Fset_marker (marker, make_fixnum (newpos), Qnil);
>        }
>  }
>  

bug#51776: master 9741924: In insert_file_contents, always set windows' point markers.

[Alan Mackenzie]

On Fri, Nov 12, 2021 at 19:27:29 -0500, Stefan Monnier wrote:
> Hi Alan,

> Alan Mackenzie [2021-11-12 13:44:52] wrote:
> >     In insert_file_contents, always set windows' point markers.
> >     This fixes bug #51776.

> >     * src/fileio.c (restore_window_points): Restore a w->mpoint even
> >     when that marker originally pointed into the unchanged area near
> >     BOB or EOB.  This prevents that window's point being moved a long
> >     way from its starting place due to the removal of the central part
> >     of the buffer by insert_file_contents.

> Hmm... my understanding of the code says that if `oldppos <= same_at_beg` then
> your change is harmless but unnecessary because the marker
> is still at `oldpos` anyway.

> But when `oldppos > same_at_beg` your change is harmful because the
> marker has properly preserved its exact position in the text whereas
> your code will arbitrarily move it to `oldpos` (which is even
> potentially past Z if the revert shortened the buffer).

Er, yes, you're right.  Thanks!

> So maybe the better fix is to just change

>     XFIXNUM (oldpos) < same_at_end

> into

>     XFIXNUM (oldpos) <= same_at_end

Indeed.

> Or am I missing something?

> [ Not sure what we should do when `oldpos == same_at_beg == same_at_end`, OTOH.
>   I suspect staying at `same_at_beg` might be the better choice in that
>   case.  ]

That case is harmless, because it just means that after the reversion,
point stays where it was (before the re-inserted middle) rather than
where it was (after the re-inserted middle).

I'll correct this patch.

>         Stefan


> >  src/fileio.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/fileio.c b/src/fileio.c
> > index 3c13d3f..a7b1649 100644
> > --- a/src/fileio.c
> > +++ b/src/fileio.c
> > @@ -3827,6 +3827,7 @@ restore_window_points (Lisp_Object window_markers, ptrdiff_t inserted,
> >  	Lisp_Object car = XCAR (window_markers);
> >  	Lisp_Object marker = XCAR (car);
> >  	Lisp_Object oldpos = XCDR (car);
> > +	ptrdiff_t newpos;
> >  	if (MARKERP (marker) && FIXNUMP (oldpos)
> >  	    && XFIXNUM (oldpos) > same_at_start
> >  	    && XFIXNUM (oldpos) < same_at_end)
> > @@ -3834,10 +3835,12 @@ restore_window_points (Lisp_Object window_markers, ptrdiff_t inserted,
> >  	    ptrdiff_t oldsize = same_at_end - same_at_start;
> >  	    ptrdiff_t newsize = inserted;
> >  	    double growth = newsize / (double)oldsize;
> > -	    ptrdiff_t newpos
> > -	      = same_at_start + growth * (XFIXNUM (oldpos) - same_at_start);
> > -	    Fset_marker (marker, make_fixnum (newpos), Qnil);
> > +	    newpos = same_at_start
> > +	      + growth * (XFIXNUM (oldpos) - same_at_start);
> >  	  }
> > +	else
> > +	  newpos = XFIXNUM (oldpos);
> > +	Fset_marker (marker, make_fixnum (newpos), Qnil);
> >        }
> >  }
> >  

-- 
Alan Mackenzie (Nuremberg, Germany).