GNU bug report logs - #79367
31.0.50; magit-commit sometimes doesn't work if diff-hl-update-async is t

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: i <at> fuzy.me, 79367 <at> debbugs.gnu.org, dmitry <at> gutov.dev
Subject: bug#79367: 31.0.50; magit-commit sometimes doesn't work if diff-hl-update-async is t
Date: Thu, 04 Sep 2025 07:48:05 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: i <at> fuzy.me,  79367 <at> debbugs.gnu.org,  dmitry <at> gutov.dev
> Date: Wed, 03 Sep 2025 13:07:08 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > +  /* make_process calls pset_thread, but if the server process is not
> > +     locked to any thread, we need to undo what make_process did.  */
> > +  if (NILP (ps->thread))
> > +    pset_thread (p, Qnil);
> 
> We should probably just unconditionally do:
>   pset_thread (p, ps->thread)
> here.

That's a bit dangerous, since the I/O descriptors of the new process
are not yet set, so the call to pset_thread there, in case ps->thread
is non-nil, would do a partial job.  We could instead move the
pset_thread call to later, where set_proc_thread is called, but I
preferred to undo what make_process did ASAP.

> 
> >    /* Build new contact information for this setup.  */
> >    contact = Fcopy_sequence (ps->childp);
> > @@ -5117,6 +5121,19 @@ server_accept_connection (Lisp_Object server, int channel)
> >      add_process_read_fd (s);
> >    if (s > max_desc)
> >      max_desc = s;
> > +  /* If the server process is locked to this thread, lock the client
> > +     process to the same thread, otherwise clear the thread of its I/O
> > +     descriptors.  */
> > +  if (NILP (ps->thread))
> > +    {
> > +      eassert (!fd_callback_info[p->infd].thread);
> > +      set_proc_thread (p, NULL);
> 
> Yes, this is the right assertion.  But it should be outside the if/else
> because it's true even if ps->thread isn't nil.

I don't mind moving it out.

> This whole if/else conditional should probably be just:
> 
>   eassert (!fd_callback_info[p->infd].thread);
>   if (!NILP (p->thread))
>     {
>       eassert (XTHREAD (ps->thread) == current_thread);
>       set_proc_thread (p, XTHREAD (p->thread));
>     }
> 
> It's not necessary to call set_proc_thread(p, NULL) in the other case
> since it will just set .thread to NULL, which we're already asserting
> anyway.

The assertions compile to nothing in a production build.  Also, I
don't like the caller to assume too much about what set_proc_thread
does, it's not future-proof.  So I'd prefer to leave that call in
place.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.