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

[Alan Mackenzie <acm@muc.de>]

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).