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

From: Phillip Wood <phillip.wood123@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Cc: Git Mailing List <git@vger.kernel.org>,
	carenas@gmail.com,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 4/4] terminal: restore settings on SIGTSTP
Date: Mon, 7 Mar 2022 10:53:34 +0000	[thread overview]
Message-ID: <8aa11144-c9ce-46aa-2edd-15e8fa1298dc@gmail.com> (raw)
In-Reply-To: <220305.86bkyk4hwc.gmgdl@evledraar.gmail.com>

Hi Ævar

On 05/03/2022 13:59, Ævar Arnfjörð Bjarmason wrote:
> [...] 
>>   int save_term(unsigned flags)
>>   {
>> +	struct sigaction sa;
>> +
>>   	if (term_fd < 0)
>>   		term_fd = (flags & SAVE_TERM_STDIN) ? 0
>>   						    : open("/dev/tty", O_RDWR);
>> @@ -44,6 +136,26 @@ int save_term(unsigned flags)
>>   	if (tcgetattr(term_fd, &old_term) < 0)
>>   		return -1;
>>   	sigchain_push_common(restore_term_on_signal);
>> +	/*
>> +	 * If job control is disabled then the shell will have set the
>> +	 * disposition of SIGTSTP to SIG_IGN.
>> +	 */
>> +	sigaction(SIGTSTP, NULL, &sa);
>> +	if (sa.sa_handler == SIG_IGN)
>> +		return 0;
>> +
>> +	/* avoid calling gettext() from signal handler */
>> +	background_resume_msg = xstrdup(_("error: cannot resume in the background"));
>> +	restore_error_msg = xstrdup(_("error: cannot restore terminal settings"));
> 
> You don't need to xstrdup() the return values of gettext() (here _()),
> you'll get a pointer to static storage that's safe to hold on to for the
> duration of the program.

I had a look at the documentation and could not see anything about the 
lifetime of the returned string, all it says is "don't alter it"

> In this case I think it would make sense to skip "error: " from the
> message itself.
> 
> Eventually we'll get to making usage.c have that prefix translated, and
> can have some utility function exposed there (I have WIP patches for
> this already since a while ago).
> 
> To translators it'll look like the same thing, and avoid churn when we
> make the "error: " prefix translatable.

Unless we add a function that returns a string rather than printing the 
message I don't see how it avoids churn in the future. Having the whole 
string with the "error: " prefix translated here does not add any extra 
burden to translators - it is still the same number of strings to translate.

> Aside: If you do keep the xstrdup() (perhaps an xstrfmt() with the above
> advice...) doesn't it make sense to add the "\n" here, so you'll have
> one write_in_full() above?

I decided to keep the translated string simpler by omitting the newline, 
calling write_in_full() twice isn't a bit deal (I don't think the output 
can be split by a write from another thread or signal handler in between).

Best Wishes

Phillip