Unbalanced change hooks (part 1)

Unbalanced change hooks (part 1)

[Alan Mackenzie]

Hello, Emacs.

In certain buffer modifications, after-change-hooks is being called, yet
before-change-hooks is not being called.  This is a Bad Thing, and is at
the root of bug #24074/#24094.  The documentation (page "Change Hooks"
in the Elisp manual) is quite clear, if a little implicit, that both
hooks, or neither (when inhibit_modification_hooks is non-nil) get
called on a buffer modification.

The first of these problems is in Finsert_file_contents, where
before-change-hooks is invoked by a call to prepare_to_modify_buffer
(which calls signal_before_change), and after-change-hooks is invoked by
a call to signal_after_change.

Both of these invocations are conditional (which is correct), but
different conditions are applied to the before-... and after-...
invocations (which is not correct).  The after-... condition tests both
parameters `visit' and `replace', but the before-... condition tests
only `visit'.  It seems likely that the test on `replace' was added at a
later date, and it was mistakenly missed out of the before-...
condition.

I propose to amend Finsert_file_contents so that the same condition is
tested for the invocation of both hooks, and to enforce this by
recording the state in a bool variable.  Comments on this proposed
change are requested:



diff --git a/src/fileio.c b/src/fileio.c
index b1f9d3c..0431cbc 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -3440,6 +3440,7 @@ by calling `format-decode', which see.  */)
   /* SAME_AT_END_CHARPOS counts characters, because
      restore_window_points needs the old character count.  */
   ptrdiff_t same_at_end_charpos = ZV;
+  bool run_change_hooks;
 
   if (current_buffer->base_buffer && ! NILP (visit))
     error ("Cannot do file visiting in an indirect buffer");
@@ -4077,7 +4078,9 @@ by calling `format-decode', which see.  */)
     /* For a special file, all we can do is guess.  */
     total = READ_BUF_SIZE;
 
-  if (NILP (visit) && total > 0)
+  run_change_hooks = ((NILP (visit) || !NILP (replace))
+                      && total > 0);
+  if (run_change_hooks)
     {
       if (!NILP (BVAR (current_buffer, file_truename))
 	  /* Make binding buffer-file-name to nil effective.  */
@@ -4313,8 +4316,7 @@ by calling `format-decode', which see.  */)
   /* Call after-change hooks for the inserted text, aside from the case
      of normal visiting (not with REPLACE), which is done in a new buffer
      "before" the buffer is changed.  */
-  if (inserted > 0 && total > 0
-      && (NILP (visit) || !NILP (replace)))
+  if (run_change_hooks)
     {
       signal_after_change (PT, 0, inserted);
       update_compositions (PT, PT, CHECK_BORDER);


Unfortunately, this amendment, by itself, doesn't fix #240[79]4, since
there are other causes for the change hooks being improperly invoked.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Unbalanced change hooks (part 1)

[Richard Copley]

On 31 July 2016 at 11:16, Alan Mackenzie <acm@muc.de> wrote:
> Hello, Emacs.
>
> In certain buffer modifications, after-change-hooks is being called, yet
> before-change-hooks is not being called.  This is a Bad Thing, and is at
> the root of bug #24074/#24094.  The documentation (page "Change Hooks"
> in the Elisp manual) is quite clear, if a little implicit, that both
> hooks, or neither (when inhibit_modification_hooks is non-nil) get
> called on a buffer modification.
>
> The first of these problems is in Finsert_file_contents, where
> before-change-hooks is invoked by a call to prepare_to_modify_buffer
> (which calls signal_before_change), and after-change-hooks is invoked by
> a call to signal_after_change.
>
> Both of these invocations are conditional (which is correct), but
> different conditions are applied to the before-... and after-...
> invocations (which is not correct).  The after-... condition tests both
> parameters `visit' and `replace', but the before-... condition tests
> only `visit'.  It seems likely that the test on `replace' was added at a
> later date, and it was mistakenly missed out of the before-...
> condition.
>
> I propose to amend Finsert_file_contents so that the same condition is
> tested for the invocation of both hooks, and to enforce this by
> recording the state in a bool variable.  Comments on this proposed
> change are requested:
>
>
>
> diff --git a/src/fileio.c b/src/fileio.c
> index b1f9d3c..0431cbc 100644
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -3440,6 +3440,7 @@ by calling `format-decode', which see.  */)
>    /* SAME_AT_END_CHARPOS counts characters, because
>       restore_window_points needs the old character count.  */
>    ptrdiff_t same_at_end_charpos = ZV;
> +  bool run_change_hooks;
>
>    if (current_buffer->base_buffer && ! NILP (visit))
>      error ("Cannot do file visiting in an indirect buffer");
> @@ -4077,7 +4078,9 @@ by calling `format-decode', which see.  */)
>      /* For a special file, all we can do is guess.  */
>      total = READ_BUF_SIZE;
>
> -  if (NILP (visit) && total > 0)
> +  run_change_hooks = ((NILP (visit) || !NILP (replace))
> +                      && total > 0);
> +  if (run_change_hooks)
>      {
>        if (!NILP (BVAR (current_buffer, file_truename))
>           /* Make binding buffer-file-name to nil effective.  */
> @@ -4313,8 +4316,7 @@ by calling `format-decode', which see.  */)
>    /* Call after-change hooks for the inserted text, aside from the case
>       of normal visiting (not with REPLACE), which is done in a new buffer
>       "before" the buffer is changed.  */
> -  if (inserted > 0 && total > 0
> -      && (NILP (visit) || !NILP (replace)))
> +  if (run_change_hooks)
>      {
>        signal_after_change (PT, 0, inserted);
>        update_compositions (PT, PT, CHECK_BORDER);
>
>
> Unfortunately, this amendment, by itself, doesn't fix #240[79]4, since
> there are other causes for the change hooks being improperly invoked.
>
> --
> Alan Mackenzie (Nuremberg, Germany).

LGTM. It's hard to imagine anyone relying on the before-change hooks
_not_ being run, so it should be safe, at least, to make this change.

Re: Unbalanced change hooks (part 1)

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Mon, 1 Aug 2016 21:13:01 +0100
> Cc: Óscar Fuentes <ofv@wanadoo.es>,
> 	Emacs Development <emacs-devel@gnu.org>
> 
> > diff --git a/src/fileio.c b/src/fileio.c
> > index b1f9d3c..0431cbc 100644
> > --- a/src/fileio.c
> > +++ b/src/fileio.c
> > @@ -3440,6 +3440,7 @@ by calling `format-decode', which see.  */)
> >    /* SAME_AT_END_CHARPOS counts characters, because
> >       restore_window_points needs the old character count.  */
> >    ptrdiff_t same_at_end_charpos = ZV;
> > +  bool run_change_hooks;
> >
> >    if (current_buffer->base_buffer && ! NILP (visit))
> >      error ("Cannot do file visiting in an indirect buffer");
> > @@ -4077,7 +4078,9 @@ by calling `format-decode', which see.  */)
> >      /* For a special file, all we can do is guess.  */
> >      total = READ_BUF_SIZE;
> >
> > -  if (NILP (visit) && total > 0)
> > +  run_change_hooks = ((NILP (visit) || !NILP (replace))
> > +                      && total > 0);
> > +  if (run_change_hooks)
> >      {
> >        if (!NILP (BVAR (current_buffer, file_truename))
> >           /* Make binding buffer-file-name to nil effective.  */
> > @@ -4313,8 +4316,7 @@ by calling `format-decode', which see.  */)
> >    /* Call after-change hooks for the inserted text, aside from the case
> >       of normal visiting (not with REPLACE), which is done in a new buffer
> >       "before" the buffer is changed.  */
> > -  if (inserted > 0 && total > 0
> > -      && (NILP (visit) || !NILP (replace)))
> > +  if (run_change_hooks)
> >      {
> >        signal_after_change (PT, 0, inserted);
> >        update_compositions (PT, PT, CHECK_BORDER);
> >
> >
> > Unfortunately, this amendment, by itself, doesn't fix #240[79]4, since
> > there are other causes for the change hooks being improperly invoked.
> >
> > --
> > Alan Mackenzie (Nuremberg, Germany).
> 
> LGTM. It's hard to imagine anyone relying on the before-change hooks
> _not_ being run, so it should be safe, at least, to make this change.

That code is almost never run when REPLACE is non-nil, so doing that
won't help.  See an earlier message that explained why.

Re: Unbalanced change hooks (part 1)

[Richard Copley]

On 2 August 2016 at 03:34, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Richard Copley <rcopley@gmail.com>
>> Date: Mon, 1 Aug 2016 21:13:01 +0100
>> Cc: Óscar Fuentes <ofv@wanadoo.es>,
>>       Emacs Development <emacs-devel@gnu.org>
>>
>> > diff --git a/src/fileio.c b/src/fileio.c
>> > index b1f9d3c..0431cbc 100644
>> > --- a/src/fileio.c
>> > +++ b/src/fileio.c
>> > @@ -3440,6 +3440,7 @@ by calling `format-decode', which see.  */)
>> >    /* SAME_AT_END_CHARPOS counts characters, because
>> >       restore_window_points needs the old character count.  */
>> >    ptrdiff_t same_at_end_charpos = ZV;
>> > +  bool run_change_hooks;
>> >
>> >    if (current_buffer->base_buffer && ! NILP (visit))
>> >      error ("Cannot do file visiting in an indirect buffer");
>> > @@ -4077,7 +4078,9 @@ by calling `format-decode', which see.  */)
>> >      /* For a special file, all we can do is guess.  */
>> >      total = READ_BUF_SIZE;
>> >
>> > -  if (NILP (visit) && total > 0)
>> > +  run_change_hooks = ((NILP (visit) || !NILP (replace))
>> > +                      && total > 0);
>> > +  if (run_change_hooks)
>> >      {
>> >        if (!NILP (BVAR (current_buffer, file_truename))
>> >           /* Make binding buffer-file-name to nil effective.  */
>> > @@ -4313,8 +4316,7 @@ by calling `format-decode', which see.  */)
>> >    /* Call after-change hooks for the inserted text, aside from the case
>> >       of normal visiting (not with REPLACE), which is done in a new buffer
>> >       "before" the buffer is changed.  */
>> > -  if (inserted > 0 && total > 0
>> > -      && (NILP (visit) || !NILP (replace)))
>> > +  if (run_change_hooks)
>> >      {
>> >        signal_after_change (PT, 0, inserted);
>> >        update_compositions (PT, PT, CHECK_BORDER);
>> >
>> >
>> > Unfortunately, this amendment, by itself, doesn't fix #240[79]4, since
>> > there are other causes for the change hooks being improperly invoked.
>> >
>> > --
>> > Alan Mackenzie (Nuremberg, Germany).
>>
>> LGTM. It's hard to imagine anyone relying on the before-change hooks
>> _not_ being run, so it should be safe, at least, to make this change.
>
> That code is almost never run when REPLACE is non-nil, so doing that
> won't help.  See an earlier message that explained why.

Thanks Eli, I saw it. It's not the sort of discussion I care to get involved in.