Re: insert-file-contents and format-decode

[martin rudalics <rudalics@gmx.at>]

 >     ! 	  /* Suppose replace is non-nil and we succeeded in not replacing the
 >     ! 	  beginning or end of the buffer text with the file's contents.  In this
 >     ! 	  case we neverthelss have to call format-decode with `point' positioned
 >     ! 	  at the beginning of the buffer
 >
 > I think that's a bug.  Point has to be at the beginning of the
 > inserted text.  If you put it at the beginning of the buffer,
 > the decoding functions have no way to determine what text
 > was just inserted, so they can't possibly do the job.

I don't understand: Fixing this was my primary intention (where
"beginning of the buffer" should read "beginning of the accessible
portion of the buffer" though in practice this is needed only for
`revert-buffer').  That's what the "replace" stuff is all about.  In the
particular case the decoding functions should be fooled into believing
that more text was inserted from the file.

 > The same goes for the Vafter_insert_file_functions.
 > It is the only way to make that case work.

These are done for the replace non-nil case only.  Both `format-decode'
and `after-insert-file-functions' were inherently broken in that case.

 > If that conflicts with documentation, then the documentation
 > has to be corrected.

There's no specific documentation on that.  `format-decode' and
`after-insert-file-functions' simply ceased to work correctly after the
`insert-file-contents' optimizations for the replace non-nil case were
installed.

 >
 >     !       else
 >     ! 	/* In the visiting case restore the previous value. */
 >     ! 	current_buffer->undo_list = old_undo;
 >
 > When visiting a file, you should set undo_list to t
 > if it was t before, otherwise to nil.
 >

I believed an earlier comment here that read as

       /* If we're anyway going to discard undo information, don't
	 record it in the first place.  The buffer's undo list at this
	 point is either nil or t when visiting a file.  */

Hence my

 >     ! 	current_buffer->undo_list = old_undo;

should have done that automatically.  Is it better to rewrite as:

       if (NILP (visit))
	{
	  ...
	}
       else if (old_undo == Qt)
	/* If undo_list was Qt before, keep it that way. */
	current_buffer->undo_list = Qt;
       else
	/* Otherwise start with an empty undo_list. */
	current_buffer->undo_list = Qnil;