bug#16804: 24.3.50; [PATCH] fix with-silent-modifications

[Stefan Monnier]

>> That doesn't change anything to the fact that calling something like
>> sit-for from within a with-silent-modifications is a problem, regardless
>> of buffer-file-name.  E.g. it will cause the process filter not to mark
>> the new text for highlighting, so it can screw up *compilation*.
> I see.  Could you help document the delicacy of
> with-silent-modifications, or people will not correctly use it?

Actually, the issue is not with with-silent-modifications but with
sit-for and other yielding functions: you should be careful to call them
from within the "plainest" context possible, since any dynamic-binding
(or other temporary global change, such as moving point) currently active
will then leak to any timer/filter/sentinel/redisplay that might be run
at that point.

>>>> Again, a backtrace would be much more useful.
>>> There is no backtrace.
>> Of course there is.  I know there is no error signaled, but you can
>> still get a backtrace by calling (backtrace), or (debug).
> Since input-pending-p is changed in 24.4 not to run timers by default,
> the backtrace is obtained in 24.3 and as expected:

Ah, right, input-pending-p shouldn't be a yield point, which we recently
fixed, so it should be OK to call input-pending-p from "anywhere".

Can you reproduce the problem in 24.4 (and show the backtrace)?

> nxml has both input-pending-p and sit-for, see
> rng-validate-while-idle-continue-p in rng-valid.el

Indeed, I see
                   (with-silent-modifications
                     (rng-do-some-validation-1 continue-p-function))))

where continue-p-function can be rng-validate-while-idle-continue-p
which calls sit-for (to cause a redisplay).  So we have a similar bug there.

> Is there something better than let-binding buffer-file-name and
> buffer-file-truename?

I have the patch below for 24.5 which makes it unnecessary for
with-silent-modifications to bind buffer-file-name and
buffer-file-truename (by strengthening the meaning of
inhibit-modification-hooks), but that doesn't magically fix the above
problems :-(


        Stefan


=== modified file 'lisp/subr.el'
--- lisp/subr.el        2014-02-12 19:40:35 +0000
+++ lisp/subr.el        2014-02-19 15:04:50 +0000
@@ -3176,12 +3176,7 @@
     `(let* ((,modified (buffer-modified-p))
             (buffer-undo-list t)
             (inhibit-read-only t)
-            (inhibit-modification-hooks t)
-            deactivate-mark
-            ;; Avoid setting and removing file locks and checking
-            ;; buffer's uptodate-ness w.r.t the underlying file.
-            buffer-file-name
-            buffer-file-truename)
+            (inhibit-modification-hooks t))
        (unwind-protect
            (progn
              ,@body)

=== modified file 'src/insdel.c'
--- src/insdel.c        2014-01-01 17:44:48 +0000
+++ src/insdel.c        2014-02-19 15:03:17 +0000
@@ -1829,6 +1829,9 @@
   else
     base_buffer = current_buffer;
 
+  if (inhibit_modification_hooks)
+    return;
+
 #ifdef CLASH_DETECTION
   if (!NILP (BVAR (base_buffer, file_truename))
       /* Make binding buffer-file-name to nil effective.  */
@@ -1848,7 +1851,6 @@
   /* If `select-active-regions' is non-nil, save the region text.  */
   /* FIXME: Move this to Elisp (via before-change-functions).  */
   if (!NILP (BVAR (current_buffer, mark_active))
-      && !inhibit_modification_hooks
       && XMARKER (BVAR (current_buffer, mark))->buffer
       && NILP (Vsaved_region_selection)
       && (EQ (Vselect_active_regions, Qonly)
@@ -1959,9 +1961,6 @@
   ptrdiff_t count = SPECPDL_INDEX ();
   struct rvoe_arg rvoe_arg;
 
-  if (inhibit_modification_hooks)
-    return;
-
   start = make_number (start_int);
   end = make_number (end_int);
   preserve_marker = Qnil;