bug#39585: after-change-functions called with invalid positions in call-process

bug#39585: after-change-functions called with invalid positions in call-process

[Clément Pit-Claudel]

Hi all,

Recent changes in call-process have introduced surprising behavior in after-change-functions (and caused bugs in flycheck, e.g. https://github.com/flycheck/flycheck/issues/1677).  From the documentation, I expected that functions added to the after-change-functions hook would be called only with valid positions.  However, the following snippets shows that it's not the case:

    (defun ~/after-change (beg end len)
      (message "(after-change %S %S %S); (buffer %S %S %S)"
               beg end len (point-min) (point-max) (buffer-size)))

    (with-current-buffer (get-buffer-create "*tmp*")
      (make-variable-buffer-local 'after-change-functions)
      (add-hook 'after-change-functions #'~/after-change)
      (call-process "echo" nil t t "Hello"))

Running it repeatedly, this is what I observe:

    (after-change 7 13 0); (buffer 1 7 6)
    (after-change 13 19 0); (buffer 1 13 12)
    (after-change 19 25 0); (buffer 1 19 18)
    (after-change 25 31 0); (buffer 1 25 24)

Note how each time the after-change-functions hook is called with a region past the end of the buffer.  It's as if after-change-functions was in fact call right before the insertion, instead of after.

Previous versions of Emacs didn't call after-change-functions in this case; it seems that the new behavior was introduced by this commit:

    commit 224e8d146485ce178086549d41fa8359dcc0e03e
    Author: Alan Mackenzie <acm@muc.de>
    Date:   Wed Jan 22 19:50:30 2020 +0000

        Make call_process call signal_after_change.  This fixes bug #38691.

        Now, functions such as call-proess-region invoke after-change-functions
        correctly.

        * src/callproc.c (call_process): Call prepare_to_modify_buffer in a single
        place, no longer delegating the task to insert_1_both, etc.  Call
        signal_after_change in each of two code branches, such that
        before-change-functions and after-change-functions are always called in
        balanced pairs.

Alan, is this behavior expected?

Thanks,
Clément.

bug#39585: after-change-functions called with invalid positions in call-process

[Alan Mackenzie]

Hello, Clément.

On Wed, Feb 12, 2020 at 22:56:49 -0500, Clément Pit-Claudel wrote:
> Hi all,

> Recent changes in call-process have introduced surprising behavior in
> after-change-functions (and caused bugs in flycheck, e.g.
> https://github.com/flycheck/flycheck/issues/1677).  From the
> documentation, I expected that functions added to the
> after-change-functions hook would be called only with valid positions.
> However, the following snippets shows that it's not the case:

>     (defun ~/after-change (beg end len)
>       (message "(after-change %S %S %S); (buffer %S %S %S)"
>                beg end len (point-min) (point-max) (buffer-size)))

>     (with-current-buffer (get-buffer-create "*tmp*")
>       (make-variable-buffer-local 'after-change-functions)
>       (add-hook 'after-change-functions #'~/after-change)
>       (call-process "echo" nil t t "Hello"))

> Running it repeatedly, this is what I observe:

>     (after-change 7 13 0); (buffer 1 7 6)
>     (after-change 13 19 0); (buffer 1 13 12)
>     (after-change 19 25 0); (buffer 1 19 18)
>     (after-change 25 31 0); (buffer 1 25 24)

> Note how each time the after-change-functions hook is called with a
> region past the end of the buffer.  It's as if after-change-functions
> was in fact call right before the insertion, instead of after.

Well, something like that.  Sorry about it, and thanks for drawing my
attention to it.

> Previous versions of Emacs didn't call after-change-functions in this
> case; it seems that the new behavior was introduced by this commit:

>     commit 224e8d146485ce178086549d41fa8359dcc0e03e
>     Author: Alan Mackenzie <acm@muc.de>
>     Date:   Wed Jan 22 19:50:30 2020 +0000

>         Make call_process call signal_after_change.  This fixes bug #38691.

>         Now, functions such as call-proess-region invoke after-change-functions
>         correctly.

>         * src/callproc.c (call_process): Call prepare_to_modify_buffer in a single
>         place, no longer delegating the task to insert_1_both, etc.  Call
>         signal_after_change in each of two code branches, such that
>         before-change-functions and after-change-functions are always called in
>         balanced pairs.

> Alan, is this behavior expected?

No, it is not.  I've applied this corrective patch to the emacs-27
branch:



commit d1e8ce8bb6fadf3d034ae437ff1c1b81be7d5209
Author: Alan Mackenzie <acm@muc.de>
Date:   Thu Feb 13 19:00:36 2020 +0000

    Make after-change-functions called from call-process get the correct BEG
    
    This fixes bug #39585.
    
    * src/callproc.c (call_process): Supply the correct CHARPOS to
    signal_after_change (twice).

diff --git a/src/callproc.c b/src/callproc.c
index 07dcc4c3ae..8883415f3f 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -811,7 +811,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
             {
               insert_1_both (buf, nread, nread, 0, 0, 0);
-              signal_after_change (PT, 0, nread);
+              signal_after_change (PT - nread, 0, nread);
             }
 	  else
 	    {			/* We have to decode the input.  */
@@ -854,7 +854,8 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
 	      TEMP_SET_PT_BOTH (PT + process_coding.produced_char,
 				PT_BYTE + process_coding.produced);
-              signal_after_change (PT, 0, process_coding.produced_char);
+              signal_after_change (PT - process_coding.produced_char,
+                                   0, process_coding.produced_char);
 	      carryover = process_coding.carryover_bytes;
 	      if (carryover > 0)
 		memcpy (buf, process_coding.carryover,



.  It should find its way into the master branch in the usual way.
Please let me know whether you agree with me that this bug can now be
closed.  Thanks!

> Thanks,
> Clément.

-- 
Alan Mackenzie (Nuremberg, Germany).