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 #32 received at 79367 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: i <at> fuzy.me, dmitry <at> gutov.dev, 79367 <at> debbugs.gnu.org
Subject: Re: bug#79367: 31.0.50; magit-commit sometimes doesn't work if
 diff-hl-update-async is t
Date: Tue, 02 Sep 2025 11:19:21 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: i <at> fuzy.me,  dmitry <at> gutov.dev,  79367 <at> debbugs.gnu.org
>> Date: Tue, 02 Sep 2025 10:33:12 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> > But make_process, called by server_accept_connection, already did
>> > this:
>> >
>> >   pset_thread (p, Fcurrent_thread ());
>> >
>> > So if we want to lock this new process to some other thread, we should
>> > fix that part as well to be consistent.
>> 
>> True, that's a bug.
>> 
>> > 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 that doesn't fix it (or you already had that commit), can you try
>> >> reverting commits 034d755f2f21088b97fdb0a34d846c39fcdbf46d then
>> >> c93be71e45d4cebeb017c813426228e579e9381d and test again?
>> >
>> > What will that prove?  Those changes are not going away, so trying
>> > that is just wasting everyone's time and energy.  I thought we were
>> > past that...
>> 
>> No, we're not past that, I still think those changes are wrong.
>> Especially because this bug report shows that those changes are indeed
>> breaking existing code.
>> 
>> I repeatedly said that those changes would cause new issues to be
>> reported, and now it is happening.  Maybe it doesn't convince you that
>> the changes need to be reverted, but you should at least be giving it a
>> little more credence now.
>
> 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.  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.

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.

Previous Next

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