From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#24340: insert-file-contents calls before-change-functions too late Date: Wed, 31 Aug 2016 17:22:24 +0300 Message-ID: <83vayhkogv.fsf@gnu.org> References: <8360qim5sv.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1472653424 12107 195.159.176.226 (31 Aug 2016 14:23:44 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 31 Aug 2016 14:23:44 +0000 (UTC) Cc: 24340@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Wed Aug 31 16:23:31 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bf6QM-0001tQ-BF for geb-bug-gnu-emacs@m.gmane.org; Wed, 31 Aug 2016 16:23:30 +0200 Original-Received: from localhost ([::1]:54406 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6QN-0003Z0-2e for geb-bug-gnu-emacs@m.gmane.org; Wed, 31 Aug 2016 10:23:31 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:49323) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6Q5-0003OI-Be for bug-gnu-emacs@gnu.org; Wed, 31 Aug 2016 10:23:24 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bf6Pu-0007Tr-Cj for bug-gnu-emacs@gnu.org; Wed, 31 Aug 2016 10:23:12 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:47825) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6Pu-0007Tm-AV for bug-gnu-emacs@gnu.org; Wed, 31 Aug 2016 10:23:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bf6Pu-0008D1-7T for bug-gnu-emacs@gnu.org; Wed, 31 Aug 2016 10:23:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 31 Aug 2016 14:23:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24340 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 24340-submit@debbugs.gnu.org id=B24340.147265337231526 (code B ref 24340); Wed, 31 Aug 2016 14:23:02 +0000 Original-Received: (at 24340) by debbugs.gnu.org; 31 Aug 2016 14:22:52 +0000 Original-Received: from localhost ([127.0.0.1]:45537 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bf6Pg-0008CM-Ev for submit@debbugs.gnu.org; Wed, 31 Aug 2016 10:22:52 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:57819) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bf6Pb-0008C6-02 for 24340@debbugs.gnu.org; Wed, 31 Aug 2016 10:22:46 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bf6PR-0007Gm-9T for 24340@debbugs.gnu.org; Wed, 31 Aug 2016 10:22:37 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:44106) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6PR-0007Ga-5w; Wed, 31 Aug 2016 10:22:33 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:2862 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bf6PN-0002og-M9; Wed, 31 Aug 2016 10:22:31 -0400 In-reply-to: (message from Stefan Monnier on Tue, 30 Aug 2016 17:21:38 -0400) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:122797 Archived-At: > From: Stefan Monnier > 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.