Re: undo weirdness with insert-file-contents

[martin rudalics]

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

Maybe it doesn't happen because the

  if (MODIFF <= SAVE_MODIFF)

check fails.  Anyway, the behavior reported by Glenn in an earlier
posting (and also observed by me) indicates it doesn't happen.

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

Currently `insert-file-contents' is the only routine knowing 'bout
whether it was able to do something with the REPLACE argument.  That's
also why I asked whether VISIT nil/REPLACE non-nil DTRT.  We always have
the dichotomy that from the user's point of view text is replaced from
BEG to END while in fact text was replaced only from REALBEG >= BEG to
REALEND =< REND.  IIUC, the REPLACE stuff was mainly written to make
reverting smoother.  Since buffer reverting throws the undo list away,
optimizing handling of the latter was hardly a concern.

Also note: We never care about text-properties here - hence strictly
spoken the REPLACE stuff _is_ incorrect.  Suppose I visit a file and
assign some text a face property - the buffer is modified.  Next I
revert the buffer and it appears unmodified although the text property
is still there.  In fact the only change caused by reverting was to make
the buffer appear unmodified and the undo list empty.  I know, with
font-locking there's hardly a way to tell whether a text property is
something that should be considered a "buffer change" or just a "hidden
buffer change" as Alan calls it.  Anyway: When you store text properties
on file the current behavior is inherently wrong :-(

I always wondered whether the REPLACE thingy could have been implemented
easier by comparing the region around `point' before and after the
replacement.  Many routines apply such heuristics (bookmarks, etags,
diff related programs).

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

... dancing with undo enabled?

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

But _all_ we really need are the beginning and end of the inserted text.
We can get them easily with undo recording turned off during decoding -
once the problems with VISIT and REPLACE have been resolved.

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

... but a REPLACE argument, I suppose.  See the example with BEG and
REALBEG above: Undo from BEG or REALBEG?  Don't change the modified
state when the region is (apparently) identic with that on the file?