Re: [PATCH 4/4] terminal: restore settings on SIGTSTP

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 10, 2022, 16:06 UTC
Message-ID: <7fff6841-968d-b741-2589-fa737499113d@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2203091310080.357@tvgsbejvaqbjf.bet>

Hi Dscho

On 09/03/2022 12:19, Johannes Schindelin wrote:

Show 21 quoted lines

> Hi Phillip,
> 
> On Fri, 4 Mar 2022, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the user suspends git while it is waiting for a keypress reset the
>> terminal before stopping and restore the settings when git resumes. If
>> the user tries to resume in the background print an error
>> message (taking care to use async safe functions) before stopping
>> again. Ideally we would reprint the prompt for the user when git
>> resumes but this patch just restarts the read().
>>
>> The signal handler is established with sigaction() rather than using
>> sigchain_push() as this allows us to control the signal mask when the
>> handler is invoked and ensure SA_RESTART is used to restart the
>> read() when resuming.
> 
> This description makes sense. From my understanding of signals, the code
> also does make sense, but it is unfortunate that it has to be so much code
> to implement something as straight-forward as suspend/resume.

Yes it is a lot of code. It would be a bit simpler if we omitted the warning about resuming in the background but I think that is worth having. There's also a lot of changing signal masks to avoid stopping twice if the user presses ^Z a second time while the signal handler is active.

> FWIW I tested the `add -p` command with these patches on Windows and it
> still works as well as when I had developed it.

Thanks for testing this on Windows, I don't think we have any meaningful test coverage for interactive.singlekey and it is probably tricky to add because it relies on having a tty.

Best Wishes

Phillip

> Thank you,
> Dscho