[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Unbalanced change hooks (part 1)
From: |
Richard Copley |
Subject: |
Re: Unbalanced change hooks (part 1) |
Date: |
Mon, 1 Aug 2016 21:13:01 +0100 |
On 31 July 2016 at 11:16, Alan Mackenzie <address@hidden> 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),
Richard Copley <=