Re: undo weirdness with insert-file-contents

[Stefan Monnier]

>> What does that mean?  Are you going to mark the buffer as modified?

> Where do I mark a buffer as modified?

I don't know.  I just got the impression that your code might cause this
to happen.  If it doesn't happen, that's great.

> What I propose is to insert a "first change" element in the undo list.
> Is it that what you mean?

I'm only asking about the impact of your change, not the way it works.

>> Normally record_first_change is called "automatically" elsewhere.
>> So why is it needed here?
> Because it's _not_ done elsewhere.

Currently it's done at 3 places: record_property_change, record_point
(both of them are in undo.c) and modify_region in insdel.c.
And record_point is called from record_insert.  So normally it all
happens "automatically".

>> 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.

When visiting a file, insert-file-contents is called from some other
piece of elisp which is the one that should take care of setting up the
undo-list and the unmodified status.  It shouldn't be
insert-file-contents's job.

> 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.

I understand this part, but it seems there's got to be a simpler way.

BTW, rather than use an undo-boundary, we could simply store the old
undo-list in some local var, then set undo-list to nil, then do the
dance, then nconc the new undo-list and the old undo-list.  Setting the
undo-list to nil temporarily hides the previous undo-list, thus
preventing merging entries.

>> 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.

Or rather create a new insert-file-contents-1 which doesn't take any
`visit' argument.


        Stefan