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 <address@hidden>
> > 
> >           /* 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).