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 #23 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 10:33:12 -0400

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

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: Zhengyi Fu <i <at> fuzy.me>,  Dmitry Gutov <dmitry <at> gutov.dev>,
>>    79367 <at> debbugs.gnu.org
>> Date: Tue, 02 Sep 2025 09:08:14 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> --- a/src/process.c
>> >> +++ b/src/process.c
>> >> @@ -5115,6 +5115,8 @@ server_accept_connection (Lisp_Object server, int channel)
>> >>    /* Client processes for accepted connections are not stopped initially.  */
>> >>    if (!EQ (p->filter, Qt))
>> >>      add_process_read_fd (s);
>> >> +  set_proc_thread (p, current_thread);
>> >> +  
>> >>    if (s > max_desc)
>> >>      max_desc = s;
>> >
>> > Thanks.
>> >
>> > I believe you are right, since server_accept_connection calls
>> > make_process to create a new process object, which should by default
>> > be locked to the thread which created it.
>> >
>> > Spencer and Dmitry, do you agree?
>> 
>> No.  At worst, the locking of the new connection should match that of
>> the existing process.  The new connection should not randomly be locked
>> to the thread which happened to run wait_reading_process_output.
>
> 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.

>> >> Further debugging through gdb revealed more details: the `thread'
>> >> member of the fd_callback_info entry for the server connection
>> >> references the diff-hl-async thread, which is already exited.
>> 
>> This aspect sounds similar to bug#79201.
>> 
>> Zhengyi, I assume you're testing with an Emacs built from source.  Can
>> you check whether the Emacs you're building includes commit
>> 37325ed5a9c7f62c35b368d9530496ed31404940, which was pushed just a couple
>> weeks ago?  If not, can you pull and test again?
>> 
>> 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.

Previous Next

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