/* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

/* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Emacs.

I've just had a bug report on bug-cc-mode from "Sun, Wei"
<waysun@amazon.com> (Subject: bug#38691: CC Mode 5.34 (C++//lhw);
`Invalid search bound` when undo).

Essentially, this bug is reproduced by inserting a single line comment
(complete with CR) into an otherwise empty C++ Mode buffer:

    // 2019-12-20 17:57

, then doing
  C-x C-p (mark-page),
  C-u M-| <CR> sort <CR> (shell-command-on-region), then
  C-_ (undo).

This throws an error.

The cause of the error is that the C function call_process in
src/callproc.c is calling before-change-functions, but fails to call
after-change-functions.  This is at callproc.c ~L790, where there is the
helpful comment:

    /* FIXME: Call signal_after_change!  */

(thanks, Stefan!).

Well, I've tried adding this call to signal_after_change thusly:



diff --git a/src/callproc.c b/src/callproc.c
index b51594c2d5..6d74b34068 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -785,9 +785,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	    {			/* We have to decode the input.  */
 	      Lisp_Object curbuf;
 	      ptrdiff_t count1 = SPECPDL_INDEX ();
+              ptrdiff_t beg;
 
 	      XSETBUFFER (curbuf, current_buffer);
 	      /* FIXME: Call signal_after_change!  */
+              beg = PT;
 	      prepare_to_modify_buffer (PT, PT, NULL);
 	      /* We cannot allow after-change-functions be run
 		 during decoding, because that might modify the
@@ -798,6 +800,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	      decode_coding_c_string (&process_coding,
 				      (unsigned char *) buf, nread, curbuf);
 	      unbind_to (count1, Qnil);
+              signal_after_change (beg, 0, PT - beg);
 	      if (display_on_the_fly
 		  && CODING_REQUIRE_DETECTION (&saved_coding)
 		  && ! CODING_REQUIRE_DETECTION (&process_coding))


, and this appears to solve the OP's problem.

However, a few lines further on, there's a del_range_2 call inside a condition
I don't understand (though might, with a great deal of study).  I suspect that
the call to signal_after_change ought to take this del_range_2 into account,
possibly coming after it.

Would somebody who's familiar with this bit of callproc.c please help me out
here, and explain what this call to del_range_2 is doing, and whether there's
anything basically wrong with my simple-minded addition of
signal_after_change.

Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Sat, 21 Dec 2019 17:23:24 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
>  	      /* FIXME: Call signal_after_change!  */
> +              beg = PT;

Can you tell why you needed this variable and the assignment?  AFAIK,
PT doesn't change when we call decode_coding_c_string.

>  	      decode_coding_c_string (&process_coding,
>  				      (unsigned char *) buf, nread, curbuf);
>  	      unbind_to (count1, Qnil);
> +              signal_after_change (beg, 0, PT - beg);
>  	      if (display_on_the_fly
>  		  && CODING_REQUIRE_DETECTION (&saved_coding)
>  		  && ! CODING_REQUIRE_DETECTION (&process_coding))
> 
> 
> , and this appears to solve the OP's problem.
> 
> However, a few lines further on, there's a del_range_2 call inside a condition
> I don't understand (though might, with a great deal of study).  I suspect that
> the call to signal_after_change ought to take this del_range_2 into account,
> possibly coming after it.
> 
> Would somebody who's familiar with this bit of callproc.c please help me out
> here, and explain what this call to del_range_2 is doing, and whether there's
> anything basically wrong with my simple-minded addition of
> signal_after_change.

I'm not sure what you want to hear.  The del_range_2 call deletes the
just-inserted text, because the condition means that text was inserted
using the wrong coding-system to decode the incoming bytes.  What does
that mean for the modification hooks, I don't know: the
before-change-functions were already called, but nothing was inserted
from the Lisp application's POV, so if you insist on having before and
after hooks to be called in pairs, you are in a conundrum.

It's possible that we should simplify all this by calling the before
hooks just once before the loop and the after hooks just once after
the loop, instead of calling them for each individual chunk inside the
loop, but again I don't know what that means for applications which
expects these hook calls to pair.

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

Sorry my last post was not thought through.  I hope this one is better.

On Sat, Dec 21, 2019 at 20:11:01 +0200, Eli Zaretskii wrote:
> > Date: Sat, 21 Dec 2019 17:23:24 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > 
> >  	      /* FIXME: Call signal_after_change!  */
> > +              beg = PT;

> Can you tell why you needed this variable and the assignment?  AFAIK,
> PT doesn't change when we call decode_coding_c_string.

I'd misunderstood decode_coding_c_string.  You're right, PT doesn't
change with this macro.  With this, the arguments I was getting to
after-change-functions were wrong.

However, I've kept BEG for the next version of the patch.

> >  	      decode_coding_c_string (&process_coding,
> >  				      (unsigned char *) buf, nread, curbuf);
> >  	      unbind_to (count1, Qnil);
> > +              signal_after_change (beg, 0, PT - beg);
> >  	      if (display_on_the_fly
> >  		  && CODING_REQUIRE_DETECTION (&saved_coding)
> >  		  && ! CODING_REQUIRE_DETECTION (&process_coding))
> > 
> > 
> > , and this appears to solve the OP's problem.
> > 
> > However, a few lines further on, there's a del_range_2 call inside a condition
> > I don't understand (though might, with a great deal of study).  I suspect that
> > the call to signal_after_change ought to take this del_range_2 into account,
> > possibly coming after it.
> > 
> > Would somebody who's familiar with this bit of callproc.c please help me out
> > here, and explain what this call to del_range_2 is doing, and whether there's
> > anything basically wrong with my simple-minded addition of
> > signal_after_change.

> I'm not sure what you want to hear.  The del_range_2 call deletes the
> just-inserted text, because the condition means that text was inserted
> using the wrong coding-system to decode the incoming bytes.  What does
> that mean for the modification hooks, I don't know: the
> before-change-functions were already called, but nothing was inserted
> from the Lisp application's POV, so if you insist on having before and
> after hooks to be called in pairs, you are in a conundrum.

I think I've solved this with the new variable prepared_position.  It
records the position of BEG for prepare_to_modify_buffer, and only calls
that function if it hasn't already done so for that BEG.  It's not an
elegant solution, but I think it will work.

> It's possible that we should simplify all this by calling the before
> hooks just once before the loop and the after hooks just once after
> the loop, instead of calling them for each individual chunk inside the
> loop, but again I don't know what that means for applications which
> expects these hook calls to pair.

I don't think this is necessary.  (If we positively want to do it,
that's a different matter.)

Here's the latest version of my patch.  It's only been slightly tested,
but it doesn't produce any unexpected "successes" from 'make check'.



diff --git a/src/callproc.c b/src/callproc.c
index b51594c2d5..34da7af863 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -746,6 +746,8 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
       int carryover = 0;
       bool display_on_the_fly = display_p;
       struct coding_system saved_coding = process_coding;
+      ptrdiff_t prepared_position = 0;
+      ptrdiff_t beg;
 
       while (1)
 	{
@@ -780,7 +782,13 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	    ;
 	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
 		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
-	    insert_1_both (buf, nread, nread, 0, 1, 0);
+            {
+              beg = PT;
+              insert_1_both (buf, nread, nread, 0, prepared_position < PT, 0);
+              if (prepared_position < PT)
+                prepared_position = PT;
+              signal_after_change (beg, 0, PT - beg);
+            }
 	  else
 	    {			/* We have to decode the input.  */
 	      Lisp_Object curbuf;
@@ -788,7 +796,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 
 	      XSETBUFFER (curbuf, current_buffer);
 	      /* FIXME: Call signal_after_change!  */
-	      prepare_to_modify_buffer (PT, PT, NULL);
+              if (prepared_position < PT)
+                {
+                  prepare_to_modify_buffer (PT, PT, NULL);
+                  prepared_position = PT;
+                }
 	      /* We cannot allow after-change-functions be run
 		 during decoding, because that might modify the
 		 buffer, while we rely on process_coding.produced to
@@ -822,6 +834,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 		  continue;
 		}
 
+              signal_after_change (PT, 0, process_coding.produced_char);
 	      TEMP_SET_PT_BOTH (PT + process_coding.produced_char,
 				PT_BYTE + process_coding.produced);
 	      carryover = process_coding.carryover_bytes;


-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Sat, 21 Dec 2019 21:47:52 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
>  	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
>  		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
> -	    insert_1_both (buf, nread, nread, 0, 1, 0);
> +            {
> +              beg = PT;
> +              insert_1_both (buf, nread, nread, 0, prepared_position < PT, 0);
> +              if (prepared_position < PT)
> +                prepared_position = PT;
> +              signal_after_change (beg, 0, PT - beg);
                                              ^^^^^^^^
'PT - beg' is just 'nread' in this case, since we are inserting
'nread' characters.  Right?

And I don't think you need the prepared_position stuff here, since PT
doesn't move in signal_after_change.

> @@ -788,7 +796,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
>  
>  	      XSETBUFFER (curbuf, current_buffer);
>  	      /* FIXME: Call signal_after_change!  */
> -	      prepare_to_modify_buffer (PT, PT, NULL);
> +              if (prepared_position < PT)
> +                {
> +                  prepare_to_modify_buffer (PT, PT, NULL);
> +                  prepared_position = PT;
> +                }
>  	      /* We cannot allow after-change-functions be run
>  		 during decoding, because that might modify the
>  		 buffer, while we rely on process_coding.produced to
> @@ -822,6 +834,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
>  		  continue;
>  		}
>  
> +              signal_after_change (PT, 0, process_coding.produced_char);
>  	      TEMP_SET_PT_BOTH (PT + process_coding.produced_char,
>  				PT_BYTE + process_coding.produced);
>  	      carryover = process_coding.carryover_bytes;

This really ugly, IMO.  And the code logic is very hard to follow and
verify its correctness, given the various use cases.

I think you should simply call signal_after_change after the call to
del_range_2 (telling the after-change hooks that actually nothing was
inserted or deleted).  Then you won't need the prepared_position
thingy.

Thanks.

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

On Sun, Dec 22, 2019 at 20:38:41 +0200, Eli Zaretskii wrote:
> > Date: Sat, 21 Dec 2019 21:47:52 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>
> > 
> >  	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
> >  		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
> > -	    insert_1_both (buf, nread, nread, 0, 1, 0);
> > +            {
> > +              beg = PT;
> > +              insert_1_both (buf, nread, nread, 0, prepared_position < PT, 0);
> > +              if (prepared_position < PT)
> > +                prepared_position = PT;
> > +              signal_after_change (beg, 0, PT - beg);
>                                               ^^^^^^^^
> 'PT - beg' is just 'nread' in this case, since we are inserting
> 'nread' characters.  Right?

Er, yes.  ;-).  So BEG is silly, and as you say, PT - beg should just be
nread.

> And I don't think you need the prepared_position stuff here, since PT
> doesn't move in signal_after_change.

The point is not to call prepare_to_modify_buffer twice at the same
position.  prepared_position records the most recent place p_t_m_b was
called, thus enabling us to avoid calling it there again, should we
remove the already inserted text, decode it, and insert it again.

> > @@ -788,7 +796,11 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
> >  
> >  	      XSETBUFFER (curbuf, current_buffer);
> >  	      /* FIXME: Call signal_after_change!  */
> > -	      prepare_to_modify_buffer (PT, PT, NULL);
> > +              if (prepared_position < PT)
> > +                {
> > +                  prepare_to_modify_buffer (PT, PT, NULL);
> > +                  prepared_position = PT;
> > +                }
> >  	      /* We cannot allow after-change-functions be run
> >  		 during decoding, because that might modify the
> >  		 buffer, while we rely on process_coding.produced to
> > @@ -822,6 +834,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
> >  		  continue;
> >  		}
> >  
> > +              signal_after_change (PT, 0, process_coding.produced_char);
> >  	      TEMP_SET_PT_BOTH (PT + process_coding.produced_char,
> >  				PT_BYTE + process_coding.produced);
> >  	      carryover = process_coding.carryover_bytes;

> This really ugly, IMO.  And the code logic is very hard to follow and
> verify its correctness, given the various use cases.

Well, call_process was already ugly, with its inserting text once,
sometimes deleting it and inserting a modified version.  But I see my
changes don't help its readability.

> I think you should simply call signal_after_change after the call to
> del_range_2 (telling the after-change hooks that actually nothing was
> inserted or deleted).  Then you won't need the prepared_position
> thingy.

After thinking it over a couple of days, I can't agree this is a good
idea.  Calling before/after-change-functions for a non-change would be
very unusual in Emacs - I don't know of anywhere where this is currently
done - and would surely cause problems somewhere, and would certainly
cause some inefficiency.  Also we would have to amend the Change Hooks
page in the Elisp manual to warn of this possibility.

And all that because a low level C function is a little tricky.

I think the basic idea of my change is sound, but it is suboptimally
coded.  My confusion, I think, arose from the use of the PREPARE
parameter in the call to insert_1_both, which creates several different
cases.  This is a bad idea.  If instead we put in a single call to
prepare_to_modify_buffer, this should be relatively easy to follow.  One
or two comments would also be helpful.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

On Tue, Dec 24, 2019 at 09:47:24 +0000, Alan Mackenzie wrote:

[ .... ]

> The point is not to call prepare_to_modify_buffer twice at the same
> position.  prepared_position records the most recent place p_t_m_b was
> called, thus enabling us to avoid calling it there again, should we
> remove the already inserted text, decode it, and insert it again.

[ .... ]

> > This really ugly, IMO.  And the code logic is very hard to follow and
> > verify its correctness, given the various use cases.

[ .... ]

> I think the basic idea of my change is sound, but it is suboptimally
> coded.  My confusion, I think, arose from the use of the PREPARE
> parameter in the call to insert_1_both, which creates several different
> cases.  This is a bad idea.  If instead we put in a single call to
> prepare_to_modify_buffer, this should be relatively easy to follow.  One
> or two comments would also be helpful.

I think the following patch is better.  What do you think?


diff --git a/src/callproc.c b/src/callproc.c
index b51594c2d5..0df0292633 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -746,6 +746,8 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
       int carryover = 0;
       bool display_on_the_fly = display_p;
       struct coding_system saved_coding = process_coding;
+      ptrdiff_t prepared_pos = 0; /* prepare_to_modify_buffer was last
+                                     called here.  */
 
       while (1)
 	{
@@ -774,21 +776,29 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 		break;
 	    }
 
+          /* Call `prepare_to_modify_buffer' exactly once for PT.  */
+          if ((prepared_pos < PT) && nread)
+            {
+              prepare_to_modify_buffer (PT, PT, NULL);
+              prepared_pos = PT;
+            }
+
 	  /* Now NREAD is the total amount of data in the buffer.  */
 
 	  if (!nread)
 	    ;
 	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
 		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
-	    insert_1_both (buf, nread, nread, 0, 1, 0);
+            {
+              insert_1_both (buf, nread, nread, 0, 0, 0);
+              signal_after_change (PT, 0, nread);
+            }
 	  else
 	    {			/* We have to decode the input.  */
 	      Lisp_Object curbuf;
 	      ptrdiff_t count1 = SPECPDL_INDEX ();
 
 	      XSETBUFFER (curbuf, current_buffer);
-	      /* FIXME: Call signal_after_change!  */
-	      prepare_to_modify_buffer (PT, PT, NULL);
 	      /* We cannot allow after-change-functions be run
 		 during decoding, because that might modify the
 		 buffer, while we rely on process_coding.produced to
@@ -822,6 +832,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 		  continue;
 		}
 
+              signal_after_change (PT, 0, process_coding.produced_char);
 	      TEMP_SET_PT_BOTH (PT + process_coding.produced_char,
 				PT_BYTE + process_coding.produced);
 	      carryover = process_coding.carryover_bytes;

> > Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Tue, 24 Dec 2019 12:51:11 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > This really ugly, IMO.  And the code logic is very hard to follow and
> > > verify its correctness, given the various use cases.
> 
> [ .... ]
> 
> > I think the basic idea of my change is sound, but it is suboptimally
> > coded.  My confusion, I think, arose from the use of the PREPARE
> > parameter in the call to insert_1_both, which creates several different
> > cases.  This is a bad idea.  If instead we put in a single call to
> > prepare_to_modify_buffer, this should be relatively easy to follow.  One
> > or two comments would also be helpful.
> 
> I think the following patch is better.  What do you think?

Frankly, I don't like this, for the reasons I explained in my other
message.  If you insist on jumping through these hoops just to avoid
an extra call to the modification hooks, please write a comment with
the detailed description of the logic of these calls and their
conditions, and how that ensures the paired calls with no extra calls.

Thanks.

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Tue, 24 Dec 2019 09:47:24 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> The point is not to call prepare_to_modify_buffer twice at the same
> position.

Why is that a problem?  Surely, something like that can happen in real
life, and any modification hook should be prepared to deal with that?

> > I think you should simply call signal_after_change after the call to
> > del_range_2 (telling the after-change hooks that actually nothing was
> > inserted or deleted).  Then you won't need the prepared_position
> > thingy.
> 
> After thinking it over a couple of days, I can't agree this is a good
> idea.  Calling before/after-change-functions for a non-change would be
> very unusual in Emacs - I don't know of anywhere where this is currently
> done - and would surely cause problems somewhere, and would certainly
> cause some inefficiency.  Also we would have to amend the Change Hooks
> page in the Elisp manual to warn of this possibility.

Again, I don't see why this could cause any trouble.  Inserting an
empty string is not an outlandish situation, and any modification hook
must be prepared to (trivially) deal with it.

IOW, jumping through hoops in order to avoid such calls is IMNSHO
unjustified.  It will definitely complicate code, and thus will run
higher risk of subtle bugs.  Why risk that?

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

I've combined your two recent posts into one, to make it easier to
answer them.

<Post 1>

On Tue, Dec 24, 2019 at 17:47:18 +0200, Eli Zaretskii wrote:
> > Date: Tue, 24 Dec 2019 09:47:24 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > The point is not to call prepare_to_modify_buffer twice at the same
> > position.

> Why is that a problem?  Surely, something like that can happen in real
> life, and any modification hook should be prepared to deal with that?

Well, the more we can issue balanced before- and after- calls, the
better.  It is true that the hook functions should be able to handle
unbalanced calls, but we don't know for sure that they can, in all
cases.  CC Mode might not be the only casualty (though I intend to fix
CC Mode, here).

Besides, the Elisp manual page "Change Hooks" only describes one
situation for unbalanced calls.  This is one large enclosing before-
followed by a sequence of smaller after-s.  The situation in callproc.c
is thus technically a bug.

> > > I think you should simply call signal_after_change after the call to
> > > del_range_2 (telling the after-change hooks that actually nothing was
> > > inserted or deleted).  Then you won't need the prepared_position
> > > thingy.

> > After thinking it over a couple of days, I can't agree this is a good
> > idea.  Calling before/after-change-functions for a non-change would be
> > very unusual in Emacs - I don't know of anywhere where this is currently
> > done - and would surely cause problems somewhere, and would certainly
> > cause some inefficiency.  Also we would have to amend the Change Hooks
> > page in the Elisp manual to warn of this possibility.

> Again, I don't see why this could cause any trouble.  Inserting an
> empty string is not an outlandish situation, and any modification hook
> must be prepared to (trivially) deal with it.

This may be true, but I wouldn't bet anything on it being true for all
existing hooks.  Doing this would necessitate amending the Elisp manual,
and (if we're going to be honest) adding a NEWS item in the incompatible
changes section.

> IOW, jumping through hoops in order to avoid such calls is IMNSHO
> unjustified.  It will definitely complicate code, and thus will run
> higher risk of subtle bugs.  Why risk that?

We have a real existing bug here, and any fix to it runs the risk of
further bugs.  I think introducing no-change change hooks is more likely
to cause trouble than the relatively straightforward patch I'm
proposing.  My proposed patch is not complicated.  Really, it isn't.
It's the least messy way of fixing the bug we have.

<Post 2>

> > I think the following patch is better.  What do you think?

> Frankly, I don't like this, for the reasons I explained in my other
> message.  If you insist on jumping through these hoops just to avoid
> an extra call to the modification hooks, please write a comment with
> the detailed description of the logic of these calls and their
> conditions, and how that ensures the paired calls with no extra calls.

OK, I've done this.  Writing this comment was actually more difficult
than amending the code.  :-)  The current state of my proposed patch,
unchanged except for the new comment, follows.  Comments?



diff --git a/src/callproc.c b/src/callproc.c
index b51594c2d5..66eebdebb4 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -746,6 +746,8 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
       int carryover = 0;
       bool display_on_the_fly = display_p;
       struct coding_system saved_coding = process_coding;
+      ptrdiff_t prepared_pos = 0; /* prepare_to_modify_buffer was last
+                                     called here.  */
 
       while (1)
 	{
@@ -773,6 +775,27 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	      if (display_on_the_fly)
 		break;
 	    }
+          /* CHANGE FUNCTIONS
+             For each typical iteration of the enclosing while (1)
+             loop which yields data (i.e. nread > 0), before- and
+             after-change-functions are invoked exactly once.  The
+             call to prepare_to_modify_buffer follows this comment,
+             and there is one call to signal_after_change in each of
+             the branches of the next `else if'.
+
+             Exceptionally, the insertion into the buffer is aborted
+             at the call to del_range_2 ~45 lines further down,
+             without signal_after_change being called.  The data are
+             then inserted finally at the next iteration of the
+             while (1) loop.  A second, unwanted, call to
+             prepare_to_modify_buffer is inhibited by the test
+             prepared_pos < PT, and signal_after_change is then
+             called normally after the insertion.  */
+          if ((prepared_pos < PT) && nread)
+            {
+              prepare_to_modify_buffer (PT, PT, NULL);
+              prepared_pos = PT;
+            }
 
 	  /* Now NREAD is the total amount of data in the buffer.  */
 
@@ -780,15 +803,16 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 	    ;
 	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
 		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
-	    insert_1_both (buf, nread, nread, 0, 1, 0);
+            {
+              insert_1_both (buf, nread, nread, 0, 0, 0);
+              signal_after_change (PT, 0, nread);
+            }
 	  else
 	    {			/* We have to decode the input.  */
 	      Lisp_Object curbuf;
 	      ptrdiff_t count1 = SPECPDL_INDEX ();
 
 	      XSETBUFFER (curbuf, current_buffer);
-	      /* FIXME: Call signal_after_change!  */
-	      prepare_to_modify_buffer (PT, PT, NULL);
 	      /* We cannot allow after-change-functions be run
 		 during decoding, because that might modify the
 		 buffer, while we rely on process_coding.produced to
@@ -822,6 +846,7 @@ call_process (ptrdiff_t nargs, Lisp_Object *args, int filefd,
 		  continue;
 		}
 
+              signal_after_change (PT, 0, process_coding.produced_char);
 	      TEMP_SET_PT_BOTH (PT + process_coding.produced_char,
 				PT_BYTE + process_coding.produced);
 	      carryover = process_coding.carryover_bytes;

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Stefan Monnier]

> Besides, the Elisp manual page "Change Hooks" only describes one
> situation for unbalanced calls.  This is one large enclosing before-
> followed by a sequence of smaller after-s.

That's right.  And the sequence can be empty.

>> Again, I don't see why this could cause any trouble.  Inserting an
>> empty string is not an outlandish situation, and any modification hook
>> must be prepared to (trivially) deal with it.
> This may be true, but I wouldn't bet anything on it being true for all
> existing hooks.

It's probably harmless to run the after change hook for such
a non-change, but it should never be necessary.


        Stefan

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Sun, 29 Dec 2019 13:34:36 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > The point is not to call prepare_to_modify_buffer twice at the same
> > > position.
> 
> > Why is that a problem?  Surely, something like that can happen in real
> > life, and any modification hook should be prepared to deal with that?
> 
> Well, the more we can issue balanced before- and after- calls, the
> better.

I wasn't suggesting to produce unbalanced calls, I was suggesting that
calling the hooks in a balanced way twice for the same position cannot
be a serious problem.

> > > After thinking it over a couple of days, I can't agree this is a good
> > > idea.  Calling before/after-change-functions for a non-change would be
> > > very unusual in Emacs - I don't know of anywhere where this is currently
> > > done - and would surely cause problems somewhere, and would certainly
> > > cause some inefficiency.  Also we would have to amend the Change Hooks
> > > page in the Elisp manual to warn of this possibility.
> 
> > Again, I don't see why this could cause any trouble.  Inserting an
> > empty string is not an outlandish situation, and any modification hook
> > must be prepared to (trivially) deal with it.
> 
> This may be true, but I wouldn't bet anything on it being true for all
> existing hooks.

Really?  I'd be surprised if such buggy hooks existed in any
production code.  How can a modification hook assume the insertion is
always non-empty?

> > IOW, jumping through hoops in order to avoid such calls is IMNSHO
> > unjustified.  It will definitely complicate code, and thus will run
> > higher risk of subtle bugs.  Why risk that?
> 
> We have a real existing bug here, and any fix to it runs the risk of
> further bugs.

Sure, but the more complex the fix, the higher the risk.  It's
uneconomical to make fixes that are more complex than strictly
necessary, I'm sure you agree.

> > > I think the following patch is better.  What do you think?
> 
> > Frankly, I don't like this, for the reasons I explained in my other
> > message.  If you insist on jumping through these hoops just to avoid
> > an extra call to the modification hooks, please write a comment with
> > the detailed description of the logic of these calls and their
> > conditions, and how that ensures the paired calls with no extra calls.
> 
> OK, I've done this.  Writing this comment was actually more difficult
> than amending the code.  :-)  The current state of my proposed patch,
> unchanged except for the new comment, follows.  Comments?

It falls short of what I'd like to see, because it doesn't cover the
situation where this test:

	      if (display_on_the_fly
		  && CODING_REQUIRE_DETECTION (&saved_coding)
		  && ! CODING_REQUIRE_DETECTION (&process_coding))

causes us to switch from using the 'else' branch to using the 'else if'
branch in the following snippet:

	  if (!nread)
	    ;
	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
	    insert_1_both (buf, nread, nread, 0, 1, 0);
	  else
	    {			/* We have to decode the input.  */

IOW, the commentary you wrote doesn't tell the reader what
insert_1_both, decode_coding_c_string, and del_range_2 do (or don't
do) with regards to the modification hooks, and without that the
comment is incomplete, and doesn't explain the logic of what the code
does.

Thanks (and apologies for a delay in responding).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

On Fri, Jan 03, 2020 at 10:45:53 +0200, Eli Zaretskii wrote:
> > Date: Sun, 29 Dec 2019 13:34:36 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Well, the more we can issue balanced before- and after- calls, the
> > better.

> I wasn't suggesting to produce unbalanced calls, I was suggesting that
> calling the hooks in a balanced way twice for the same position cannot
> be a serious problem.

It might, for example, cause some hook to invalidate some cache
unnecessarily.  But even if it caused no specific problem, it would cost
a needless runtime penalty.

[ .... ]

> > We have a real existing bug here, and any fix to it runs the risk of
> > further bugs.

> Sure, but the more complex the fix, the higher the risk.  It's
> uneconomical to make fixes that are more complex than strictly
> necessary, I'm sure you agree.

I think I disagree with you generally on this point - I think it is
uneconomical to install a simple workaround when a complete fix is only a
little more difficult.  This change of mine is NOT complicated.   

[ .... ]

> It falls short of what I'd like to see, because it doesn't cover the
> situation where this test:

> 	      if (display_on_the_fly
> 		  && CODING_REQUIRE_DETECTION (&saved_coding)
> 		  && ! CODING_REQUIRE_DETECTION (&process_coding))

> causes us to switch from using the 'else' branch to using the 'else if'
> branch in the following snippet:

> 	  if (!nread)
> 	    ;
> 	  else if (NILP (BVAR (current_buffer, enable_multibyte_characters))
> 		   && ! CODING_MAY_REQUIRE_DECODING (&process_coding))
> 	    insert_1_both (buf, nread, nread, 0, 1, 0);
> 	  else
> 	    {			/* We have to decode the input.  */

> IOW, the commentary you wrote doesn't tell the reader what
> insert_1_both, decode_coding_c_string, and del_range_2 do (or don't
> do) with regards to the modification hooks, and without that the
> comment is incomplete, and doesn't explain the logic of what the code
> does.

OK.  I have to say here, I really don't believe such an extensive
commentary is needed here.  The code is there, and anybody generally
familiar with our C code would understand it without a great deal of
difficulty, even the mechanism which prevents a spurious second call to
prepare_to_modify_buffer.  Surely?

Anyhow I've spent quite a bit of time trying to get the level of this
comment right, and my current version of it reads as follows:


          /* CHANGE FUNCTIONS
             For each iteration of the enclosing while (1) loop which
             yields data (i.e. nread > 0), before- and
             after-change-functions are each invoked exactly once.
             This is done directly from the current function only, by
             calling prepare_to_modify_buffer and signal_after_change.
             It is never done by directing another function such as
             insert_1_both to call them.  The call to
             prepare_to_modify_buffer follows this comment, and
             there is one call to signal_after_change in each of the
             branches of the next `else if'.

             Exceptionally, the insertion into the buffer is aborted
             at the call to del_range_2 ~45 lines further down, this
             function removing the newly inserted data.  At this stage
             prepare_to_modify_buffer has been called, but
             signal_after_change hasn't.  A continue statement
             restarts the enclosing while(1) loop.  A second,
             unwanted, call to `prepare_to_modify_buffer' is inhibited
             by the test prepared_pos < PT.  The data are inserted
             again, and this time signal_after_change gets called,
             balancing the previous call to prepare_to_modify_buffer.
          */

> Thanks (and apologies for a delay in responding).

That's OK, I haven't exactly been prompt myself in posting to this
thread.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Sat, 4 Jan 2020 22:47:30 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> OK.  I have to say here, I really don't believe such an extensive
> commentary is needed here.  The code is there, and anybody generally
> familiar with our C code would understand it without a great deal of
> difficulty, even the mechanism which prevents a spurious second call to
> prepare_to_modify_buffer.  Surely?

If you think this is a waste of effort, you can leave the commentary
to me.

>              For each iteration of the enclosing while (1) loop which
>              yields data (i.e. nread > 0), before- and
>              after-change-functions are each invoked exactly once.
>              This is done directly from the current function only, by
>              calling prepare_to_modify_buffer and signal_after_change.
>              It is never done by directing another function such as
>              insert_1_both to call them.

The last sentence above is inaccurate, since insert_1_both does call
prepare_to_modify_buffer.

Thanks.

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

On Sun, Jan 05, 2020 at 20:17:44 +0200, Eli Zaretskii wrote:
> > Date: Sat, 4 Jan 2020 22:47:30 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > OK.  I have to say here, I really don't believe such an extensive
> > commentary is needed here.  The code is there, and anybody generally
> > familiar with our C code would understand it without a great deal of
> > difficulty, even the mechanism which prevents a spurious second call to
> > prepare_to_modify_buffer.  Surely?

> If you think this is a waste of effort, you can leave the commentary
> to me.

It was more that that amount of commentary, 21 lines, could well get in
the way, rather than being a help.

> >              For each iteration of the enclosing while (1) loop which
> >              yields data (i.e. nread > 0), before- and
> >              after-change-functions are each invoked exactly once.
> >              This is done directly from the current function only, by
> >              calling prepare_to_modify_buffer and signal_after_change.
> >              It is never done by directing another function such as
> >              insert_1_both to call them.

> The last sentence above is inaccurate, since insert_1_both does call
> prepare_to_modify_buffer.

insert_1_both _can_ call prepare_to_modify_buffer, but only if it's
directed to do so by setting its PREPARE parameter to true.  Here
it is set to false, to make it easier to keep control of the various
prepare_to_modify_buffer's and signal_after_change's.  How about
changing the last sentence to:

    It is not done here by directing another function such as
    insert_1_both to call them.

?

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

Ping?

Could we move forward with this, please?

On Sun, Jan 05, 2020 at 18:48:44 +0000, Alan Mackenzie wrote:
> Hello, Eli.

> On Sun, Jan 05, 2020 at 20:17:44 +0200, Eli Zaretskii wrote:
> > > Date: Sat, 4 Jan 2020 22:47:30 +0000
> > > Cc: emacs-devel@gnu.org
> > > From: Alan Mackenzie <acm@muc.de>

> > > OK.  I have to say here, I really don't believe such an extensive
> > > commentary is needed here.  The code is there, and anybody
> > > generally familiar with our C code would understand it without a
> > > great deal of difficulty, even the mechanism which prevents a
> > > spurious second call to prepare_to_modify_buffer.  Surely?

> > If you think this is a waste of effort, you can leave the commentary
> > to me.

> It was more that that amount of commentary, 21 lines, could well get in
> the way, rather than being a help.

> > >              For each iteration of the enclosing while (1) loop which
> > >              yields data (i.e. nread > 0), before- and
> > >              after-change-functions are each invoked exactly once.
> > >              This is done directly from the current function only, by
> > >              calling prepare_to_modify_buffer and signal_after_change.
> > >              It is never done by directing another function such as
> > >              insert_1_both to call them.

> > The last sentence above is inaccurate, since insert_1_both does call
> > prepare_to_modify_buffer.

> insert_1_both _can_ call prepare_to_modify_buffer, but only if it's
> directed to do so by setting its PREPARE parameter to true.  Here it is
> set to false, to make it easier to keep control of the various
> prepare_to_modify_buffer's and signal_after_change's.  How about
> changing the last sentence to:

>     It is not done here by directing another function such as
>     insert_1_both to call them.

> ?

> > Thanks.

> -- 
> Alan Mackenzie (Nuremberg, Germany).

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Eli Zaretskii]

> Date: Tue, 21 Jan 2020 20:34:53 +0000
> Cc: emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Eli.
> 
> Ping?
> 
> Could we move forward with this, please?

What holds this?  I wasn't aware we had yet to discuss something or
decide.

Re: /* FIXME: Call signal_after_change! */ in callproc.c. Well, why not?

[Alan Mackenzie]

Hello, Eli.

On Wed, Jan 22, 2020 at 05:27:46 +0200, Eli Zaretskii wrote:
> > Date: Tue, 21 Jan 2020 20:34:53 +0000
> > Cc: emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Hello, Eli.

> > Ping?

> > Could we move forward with this, please?

> What holds this?  I wasn't aware we had yet to discuss something or
> decide.

Ah, sorry.  I thought we were still negotiating it.

I've committed the fix to the emacs-27 branch, and will now close bug
#38691.

-- 
Alan Mackenzie (Nuremberg, Germany).