Re: undo weirdness with insert-file-contents

[martin rudalics]

>>the empty_undo_list check at all.  BTW, what is the semantics of
>>`insert-file-contents' with VISIT nil and REPLACE non-nil?
>
> It seems obvious to me that it's similar to "delete + insert".
> So I guess I don't understand the question quite right.  I use this kind
> of call to insert-file-contents in smerge-resolve, if you want to see
> an example.

I asked because from reading the code I sometimes got the impression
that REPLACE => VISIT.

>>>Also if the file is empty, is this going to mark the buffer as modified
>>>even though nothing was changed?
>
>>I'm afraid I don't understand the question - do you mean the case where
>>inserted equals zero?
>
> Yes.
>
>>I don't handle that.
>
> What does that mean?  Are you going to mark the buffer as modified?

Where do I mark a buffer as modified?  What I propose is to insert a
"first change" element in the undo list.  Is it that what you mean?

> Normally record_first_change is called "automatically" elsewhere.
> So why is it needed here?

Because it's _not_ done elsewhere.

> The handling of the undo-list and modified-p status in
> insert-file-contents is a real mess.

The main reason for the "mess" is that `insert-file-contents' has to
reconcile visiting a file and inserting the contents of some file into
an existing buffer.  In the first case the buffer should appear
unmodified and the undo-list empty after the insertion.  In the second
case the buffer should appear modified and the change recorded in the
undo list.

A second reason stems from the various decoding steps.  Ideally,
decoding should run transparently and only the "final" insertion get
recorded.  For this purpose you have to temporarily switch off undo
recording in the non-visiting case.  Otherwise, undoing changes could
reveal changes done by the decoding routine as can be observed with
Emacs 22.  Moreover, recording changes during decoding might be
expensive.

Finally, functions called by `insert-file-contents', for example,
`decode_coding' and `adjust_after_insert', may modify `buffer-undo-list'
in some way or the other.

> It'd be good to take a step back
> and think about how it *should* work.  E.g. I don't think
> insert-file-contents should mess with modified-p: it should behave just
> like a normal `insert' in this respect.

Its doc-string says "If second argument VISIT is non-nil, the buffer's
visited filename and last save file modtime are set, and it is marked
unmodified." hence we would have to change that first.

Maybe we could resolve these issues by renaming `insert-file-contents'
to `insert-file-contents-1' and having a new `insert-file-contents'
handle the undo list and the buffer-modified state while calling
`insert-file-contents-1' with undo disabled.