bug#24340: insert-file-contents calls before-change-functions too late

[Eli Zaretskii <eliz@gnu.org>]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 24340@debbugs.gnu.org
> Date: Tue, 30 Aug 2016 17:21:38 -0400
> 
> > Thanks for the patch, but I don't want to make such pervasive changes
> > for this problem.
> 
> It was for another problem (the fact that b-c-f was not called at all in
> a corner case).
> 
> This is a problem (it breaks the promise that b-c-f covers all
> following changes) that shows up everytime insert-file-contents is
> called with a non-nil `replace' argument, which is a completely
> different situation.  It's a common case.

Then you misunderstood me.  My suggestion was meant for any use of
insert-file-contents with REPLACE non-nil.

> > I thought we agreed that a single call to before-change hooks for the
> > entire buffer would be an okay solution for this scenario.
> 
> Kind of, but I think it'd be a kludge compared to the patch I submitted.
> This said, if you insist on fixing it some other way, that's fine by me.

I'm sorry, but I must insist.  The change I proposed is the only one
in insert-file-contents for that use case that I'm prepared to
consider in the current situation.  (We could also leave this bug
open, until such time as a more thorough refactoring is done of the
related functionalities.)  Deeper changes are exactly the can of worms
that I don't want to open to fix just this one use case, especially
since Emacs 25.2 will almost certainly be branched from what now is
the master branch.  Such deeper changes were what Alan proposed in the
first place (with a similar patch), and I already said I didn't want
to do that.

> Its effect is to call prepare_to_modify_buffer one more time in
> a function which already calls it moments later.  Seems safer than about
> 90% of the changes I installed in the past.

I disagree about the safety.  The various things that
prepare_to_modify_buffer does is a hodge-podge of unrelated jobs that
were lumped there for "histerical raisins".  Just look at this list of
what it does:

 . bitches if the buffer is read-only
 . signals an undoable change
 . sets the buffer's redisplay flag
 . optionally preserves a buffer position by holding it in a pointer
 . locks the file
 . saves the text of selected region
 . calls before-change-functions and modification hooks stored in text
   properties
 . sets deactivate-mark non-nil

Doing this in the delete portions of insert-file-contents will send
ripples everywhere in Emacs, and I don't want to risk that without
being able to test that everything still works as before.  Each one of
these jobs should be carefully analyzed, decided whether we want to do
it for each chunk of text we change, and separate controls designed
and implemented for each of them we want to be able to control
independently of the others.  Anything else is just kludging patch
upon patch and hoping they will somehow DTRT.

> I agree that the let-binding is a bit ugly, but that's the kludge we use
> currently, so my patch doesn't make it much uglier in this respect.
> A cleaner way to handle this issue might be to introduce a new
> buffer-local variable (like inhibit-file-supersession-check) which we'd
> let-bind to t around the whole function

I think a cleaner way is to change the model of how we partition
piecemeal changes, when we signal the changes and when don't, when we
ask the user about supersession-threat, etc.  The current model is
fundamentally flawed, if we want to use the buffer-change hooks in the
ways that emerged from these discussions.  E.g., locking the file
should clearly be decoupled from signaling a change, because you
generally lock the file only once for a batch of changes, and the same
for the read-only checks.

Thanks.