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

> On 07/03/2022 11:49, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Mar 07 2022, Phillip Wood wrote:
>> 
>>> 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"
>> I think this is overed in "11.2.7 Optimization of the *gettext
>> functions", a pedantic reading might suggest not, but what's meant with
>> the combination of that API documentation & the description of how MO
>> files work is that you're just getting pointers into the already-loaded
>> translation catalog, so it's safe to hold onto the pointer and re-use it
>> later.
>
> Strictly that section only shows it is safe if there are no other
> calls to gettext() before the returned string is used. I agree the 
> implementation is likely to be just returning static strings but I
> can't find anywhere that says another implementation (e.g. on
> macos/*bsd) has to do that.

>> In any case, if we're going to be paranoid about gettext() it would make
>> sense to propose that as some general change to how we use it, we rely
>> on this assumption holding in a lot of our use of the API:
>>      git grep '= _\('
>> Rather than sneak that partcular new assumption in here in this
>> already
>> tricky code...
>
> The ones I looked at are mostly not calling gettext() again before
> using the translated string (there is one exception in
> builtin/remote.c).

>>>> 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.
>> Because translators translate "we failed" for most errors, not
>> "error:
>> we failed".
>> If and when we convert it from "error: we failed" to "we failed"
>> they'll
>> need to translate it again (although to be fair, the translation cache
>> will help).
>> And even then it'll be one of very few exceptions where the "error:
>> "
>> currently that *is* translated.
>> 
>>>> 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).
>> Makes sense.
>> FWIW I meant if you're going to xstrdup() or xstrfmt() it anyway you
>> could do:
>>      xstrfmt("error: %s\n", _("the error"))
>> And then do one call to write_in_full().
>> But I think just:
>>      msg = _("the error");
>> And then:
>> 	const char *const = pfx = "error: ";
>>          const size_t len = strlen(pfx);
>> 	write_in_full(2, pfx, len);
>>          write_in_full(2, msg, strlen(msg));
>> 	write_in_full(2, "\n", 1);
>> Makes more sense :)
>
> Agreed, I'll change that.
>
> Best Wishes
>
> Phillip