Subject: bug#79367: 31.0.50; magit-commit sometimes doesn't work if diff-hl-update-async is t

From: Eli Zaretskii <[email-masked]>
Date: Tue, 02 Sep 2025 19:13:32 +0300

> From: Spencer Baugh <[email-masked]>
> Cc: [email-masked],  [email-masked],  [email-masked]
> Date: Tue, 02 Sep 2025 11:19:21 -0400
> 
> Eli Zaretskii <[email-masked]> writes:
> 
> >> > And why do you think this thread is a random one?  If everything works
> >> > as intended, it is the thread which created the existing process,
> >> > because the descriptor whose presence in Available triggers the call
> >> > to server_accept_connection is one of those on which pselect waited,
> >> > and should have belonged to the current thread.
> >> 
> >> Not if the existing process was unlocked.  Then this is a random thread.
> >
> > Then let's fix the case of an unlocked process (which AFAIU is not
> > what happened here).  But if the process was locked to a particular
> > thread, as that is the default, then do you agree that this code
> > should be run by that very thread, at least nominally?
> 
> Yes.
> 
> But we can't rely on that since of course the process could be unlocked.
> Our code needs to behave correctly in both cases.

If you mean the case where the original process was not locked to any
thread, I agree: the fix should include both cases.  It is easy to
distinguish between a process that wasn't locked (in which case the
new one shouldn't be locked, either), and the case where it was locked
to a thread.

If you mean something else, please elaborate.

> > This particular breakage is simply because my change was incomplete:
> > it left some processes not locked to their thread, and the solution
> > proposed by the OP, which fixed that blunder, is simpler and has fewer
> > downsides than backing out the process locking to threads.  This
> > happens all the time in development, and the conclusion is rarely that
> > the original changes should be backed out, certainly not if there are
> > simpler fixes.
> 
> That's true, but that doesn't change the fact that your change was
> broken and caused this bug.

This happens all the time in development, to all of us.  When it does,
we install follow-up changes to fix the regressions.  We don't usually
revert the original ones, unless we learn that their ideas are
unworkable, or cause much worse problems.  This is not that case, not
yet.

> And I am annoyed that I have to debug issues caused by your own
> broken changes while you refuse the simple solution of backing out
> the broken change.

It happens all the time in Emacs development that someone makes a
mistake and someone else needs to debug and fix this.  If you are
annoyed in this case, just leave the debugging to me.  Unlike some
other people who make changes and disappear, I'm here, day in and day
out, and when a bug due to my changes is reported, it is usually very
high priority to me.  (In this case, I responded within an hour of
reading the bug report.  Before you did, btw.)  But backing out
changes is not a "simple solution", because those changes were made
for a reason.

> Could we at least turn off your change by default, if we can't back it
> out?  I would be happy to add code to do that.  That will give us more
> time to iron out the issues with it.

If we turn this off by default, then any problems, like this one,
which need to be fixed as followup, will never materialize.  So I must
say no in this case.  (I also don't think this strategy is correct in
any software development in general.)

If your use cases suffer from this, you can use set-process-thread to
unlock your processes, as a stopgap.  Although I would encourage you
not to do that and report the problems instead, so that they could be
debugged and fixed ASAP.