From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#24340: insert-file-contents calls before-change-functions too late Date: Tue, 30 Aug 2016 17:21:38 -0400 Message-ID: References: <8360qim5sv.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1472591961 31341 195.159.176.226 (30 Aug 2016 21:19:21 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 30 Aug 2016 21:19:21 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) Cc: 24340@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Aug 30 23:19:17 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 1beqR9-0007bc-Ol for geb-bug-gnu-emacs@m.gmane.org; Tue, 30 Aug 2016 23:19:16 +0200 Original-Received: from localhost ([::1]:51327 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1beqR7-000329-Ar for geb-bug-gnu-emacs@m.gmane.org; Tue, 30 Aug 2016 17:19:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:54496) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1beqR0-000307-Id for bug-gnu-emacs@gnu.org; Tue, 30 Aug 2016 17:19:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1beqQw-000817-4n for bug-gnu-emacs@gnu.org; Tue, 30 Aug 2016 17:19:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:47034) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1beqQw-000813-1B for bug-gnu-emacs@gnu.org; Tue, 30 Aug 2016 17:19:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1beqQv-00034N-Si for bug-gnu-emacs@gnu.org; Tue, 30 Aug 2016 17:19:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 30 Aug 2016 21:19:01 +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.147259192211772 (code B ref 24340); Tue, 30 Aug 2016 21:19:01 +0000 Original-Received: (at 24340) by debbugs.gnu.org; 30 Aug 2016 21:18:42 +0000 Original-Received: from localhost ([127.0.0.1]:44746 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1beqQc-00033n-91 for submit@debbugs.gnu.org; Tue, 30 Aug 2016 17:18:42 -0400 Original-Received: from alt13.smtp-out.videotron.ca ([135.19.0.26]:47014 helo=alt12.smtp-out.videotron.ca) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1beqQa-00033b-AW for 24340@debbugs.gnu.org; Tue, 30 Aug 2016 17:18:40 -0400 Original-Received: from ceviche.home ([184.161.231.20]) by Videotron with SMTP id eqQTbtvQeHh2deqQUbYwRx; Tue, 30 Aug 2016 17:18:34 -0400 X-Authority-Analysis: v=2.1 cv=Lv0ysipc c=1 sm=1 tr=0 a=RXLRw6V2y9MFlfnpr5gyhA==:117 a=RXLRw6V2y9MFlfnpr5gyhA==:17 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=7z1cN_iqozsA:10 a=HPZwGtT0M34nKraRG8AA:9 a=MNxCqmdJpjbzcpao:21 a=uyArZf1Cez8_ZioL:21 Original-Received: by ceviche.home (Postfix, from userid 20848) id 8099A66274; Tue, 30 Aug 2016 17:21:38 -0400 (EDT) In-Reply-To: <8360qim5sv.fsf@gnu.org> (Eli Zaretskii's message of "Tue, 30 Aug 2016 22:10:24 +0300") X-CMAE-Envelope: MS4wfMSkJh2LRKiwAn+7d0CPKUUFzbW+sjp5cf0vDtpuiGrf8n5gbUijERTVRBSszlvxoEOguNVfBKoAEpzZRP7Jir7m4tyGGtHpYiMdnRp6ItXyauiBeMJQ KhA6H00HOflMtVbSVR+q8Yv578wDxU/HerRefkrcbh+x/tes5xGvbktUQh/xxTkc8iDUmRoO2vvaI74OBBzdQxvoCJnEtwsA5KG1D8g7XXcyQ2mZfoSuX1UZ fZ8VTrV0Go5lGpus+h2o4g== 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:122783 Archived-At: > 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. > 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. It's just that this problem is a lot more common than the one we knew about, so I think it makes it more important to fix it sooner rather than later. > What you suggest is exactly the slippery path which I was afraid of: > a lot of tricky/kludgey changes whose effect we don't really > understand, because we have no tests for the affected functionality. 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. >> + /* FIXME: Shouldn't invalidate_buffer_caches be called by >> + del_range_byte or even directly by prepare_to_modify_buffer? */ > prepare_to_modify_buffer already calls invalidate_buffer_caches. > The direct call here is because del_range_byte is called with its last > argument zero. OMG, indeed, so my patch makes even more sense and is even cleaner. See new patch below, which actually simplifies the code. 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 (either inside insert-file-contents when `replace' is non-nil, or around revert-buffer's call to insert-file-contents). But for now, I can live with the current kludge. Stefan diff --git a/src/fileio.c b/src/fileio.c index bf6bf62..a63deee 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -3839,6 +3839,7 @@ by calling `format-decode', which see. */) if (! giveup_match_end) { ptrdiff_t temp; + ptrdiff_t this_count = SPECPDL_INDEX (); /* We win! We can handle REPLACE the optimized way. */ @@ -3868,13 +3869,19 @@ by calling `format-decode', which see. */) beg_offset += same_at_start - BEGV_BYTE; end_offset -= ZV_BYTE - same_at_end; - invalidate_buffer_caches (current_buffer, - BYTE_TO_CHAR (same_at_start), - same_at_end_charpos); - del_range_byte (same_at_start, same_at_end, 0); + /* This binding is to avoid ask-user-about-supersession-threat + being called in insert_from_buffer or del_range_bytes (via + prepare_to_modify_buffer). + AFAICT we could avoid ask-user-about-supersession-threat by setting + current_buffer->modtime earlier, but we could still end up calling + ask-user-about-supersession-threat if the file is modified while + we read it, so we bind buffer-file-name instead. */ + specbind (intern ("buffer-file-name"), Qnil); + del_range_byte (same_at_start, same_at_end); /* Insert from the file at the proper position. */ temp = BYTE_TO_CHAR (same_at_start); SET_PT_BOTH (temp, same_at_start); + unbind_to (this_count, Qnil); /* If display currently starts at beginning of line, keep it that way. */ @@ -3979,10 +3986,9 @@ by calling `format-decode', which see. */) /* Truncate the buffer to the size of the file. */ if (same_at_start != same_at_end) { - invalidate_buffer_caches (current_buffer, - BYTE_TO_CHAR (same_at_start), - BYTE_TO_CHAR (same_at_end)); - del_range_byte (same_at_start, same_at_end, 0); + /* See previous specbind for the reason behind this. */ + specbind (intern ("buffer-file-name"), Qnil); + del_range_byte (same_at_start, same_at_end); } inserted = 0; @@ -4030,12 +4036,11 @@ by calling `format-decode', which see. */) we are taking from the decoded string. */ inserted -= (ZV_BYTE - same_at_end) + (same_at_start - BEGV_BYTE); + /* See previous specbind for the reason behind this. */ + specbind (intern ("buffer-file-name"), Qnil); if (same_at_end != same_at_start) { - invalidate_buffer_caches (current_buffer, - BYTE_TO_CHAR (same_at_start), - same_at_end_charpos); - del_range_byte (same_at_start, same_at_end, 0); + del_range_byte (same_at_start, same_at_end); temp = GPT; eassert (same_at_start == GPT_BYTE); same_at_start = GPT_BYTE; @@ -4056,10 +4061,6 @@ by calling `format-decode', which see. */) same_at_start + inserted - BEGV_BYTE + BUF_BEG_BYTE (XBUFFER (conversion_buffer))) - same_at_start_charpos); - /* This binding is to avoid ask-user-about-supersession-threat - being called in insert_from_buffer (via in - prepare_to_modify_buffer). */ - specbind (intern ("buffer-file-name"), Qnil); insert_from_buffer (XBUFFER (conversion_buffer), same_at_start_charpos, inserted_chars, 0); /* Set `inserted' to the number of inserted characters. */ diff --git a/src/fns.c b/src/fns.c index 31f0fd2..d3f9a6b 100644 --- a/src/fns.c +++ b/src/fns.c @@ -3192,7 +3192,7 @@ into shorter lines. */) SET_PT_BOTH (XFASTINT (beg), ibeg); insert (encoded, encoded_length); SAFE_FREE (); - del_range_byte (ibeg + encoded_length, iend + encoded_length, 1); + del_range_byte (ibeg + encoded_length, iend + encoded_length); /* If point was outside of the region, restore it exactly; else just move to the beginning of the region. */ diff --git a/src/insdel.c b/src/insdel.c index 5d3884b..ed914ec 100644 --- a/src/insdel.c +++ b/src/insdel.c @@ -1690,7 +1690,7 @@ del_range_1 (ptrdiff_t from, ptrdiff_t to, bool prepare, bool ret_string) /* Like del_range_1 but args are byte positions, not char positions. */ void -del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte, bool prepare) +del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte) { ptrdiff_t from, to; @@ -1706,23 +1706,22 @@ del_range_byte (ptrdiff_t from_byte, ptrdiff_t to_byte, bool prepare) from = BYTE_TO_CHAR (from_byte); to = BYTE_TO_CHAR (to_byte); - if (prepare) - { - ptrdiff_t old_from = from, old_to = Z - to; - ptrdiff_t range_length = to - from; - prepare_to_modify_buffer (from, to, &from); - to = from + range_length; - - if (old_from != from) - from_byte = CHAR_TO_BYTE (from); - if (to > ZV) - { - to = ZV; - to_byte = ZV_BYTE; - } - else if (old_to == Z - to) - to_byte = CHAR_TO_BYTE (to); - } + { + ptrdiff_t old_from = from, old_to = Z - to; + ptrdiff_t range_length = to - from; + prepare_to_modify_buffer (from, to, &from); + to = from + range_length; + + if (old_from != from) + from_byte = CHAR_TO_BYTE (from); + if (to > ZV) + { + to = ZV; + to_byte = ZV_BYTE; + } + else if (old_to == Z - to) + to_byte = CHAR_TO_BYTE (to); + } del_range_2 (from, from_byte, to, to_byte, 0); signal_after_change (from, to - from, 0); diff --git a/src/lisp.h b/src/lisp.h index 97c8d9f..b757e7b 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -3514,7 +3514,7 @@ extern void insert_from_string_before_markers (Lisp_Object, ptrdiff_t, ptrdiff_t, bool); extern void del_range (ptrdiff_t, ptrdiff_t); extern Lisp_Object del_range_1 (ptrdiff_t, ptrdiff_t, bool, bool); -extern void del_range_byte (ptrdiff_t, ptrdiff_t, bool); +extern void del_range_byte (ptrdiff_t, ptrdiff_t); extern void del_range_both (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t, bool); extern Lisp_Object del_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t, bool);