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 restore_term() I'm checking if the messages are NULL to see if job
control is enabled, I could use a flag but I'm inclined to just keep
coping the strings.
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
