Re: insert-file-contents and format-decode

[martin rudalics]

>     !        /* 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;