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

Message #110 received at 79367 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#79367: 31.0.50; magit-commit sometimes doesn't work if
 diff-hl-update-async is t
Date: Thu, 04 Sep 2025 22:22:29 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: i <at> fuzy.me,  79367 <at> debbugs.gnu.org,  dmitry <at> gutov.dev
> Date: Thu, 04 Sep 2025 12:41:04 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> 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.
> 
> I'm confused.
> 
> - It's just duplicating what make_process did, so I expect it's
>   essentially a no-op, just simpler code.
> 
> - Why would it be dangerous even if that wasn't the case?  We're still
>   setting up this process, it's not visible to any Lisp code, so what
>   bad event could even happen?

Having some part of server_accept_connection, even a small part, run
with the new process locked to a thread introduces a window during
which the process is in an incorrect state, and I would like to avoid
that if possible.  Admittedly, in this case the window is very small,
but it is still there.  Some future changes could actually trip on
that.

> >> 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.
> 
> Then maybe we should turn this assertion on by default in production
> builds, since threads are still encountering problems, and it would help
> catch problems.

Production builds are not for catching problems, at least not in my
book.  A production build should try to avoid aborts if possible,
because an abort might mean loss of edits.  That's why we have eassert
to begin with: we use the period of time that code is on the
development branch to catch errors and fix them.

> Zhengyi says he saw in a debugger this field was set to
> non-NULL at this point, which indicates a bug, so it would help tracking
> that down.

Building with --enable-checking will allow us to do that.

Previous Next

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