Re: [PATCH] editor: only save (and restore) the terminal if using a tty
From: Junio C Hamano <hidden>
Date: 2021-11-22 23:03:38
Carlo Marcelo Arenas Belón [off-list ref] writes:
If the editor is invoked without a controlling terminal, then saving the state and restoring it later is not very useful and could generate signals that the invoking process wouldn't know how to handle. if git's standard output is not connected to a terminal, then presume there is no need to worry if the invoking terminal could garble it.
Shouldn't the logic apply equally to all callers of save_term()? In other words, why aren't we doing this check inside save_term() implementation? i.e. before opening /dev/tty, we can do isatty(1) and return -1 if it is false, or something? That way, when we gain the second caller to save/restore other than editor (prehaps the pager code path wants to do this? I dunno), we do not have to remember that isatty() check must be made before doing save_term(), no? In any case, I am quite tempted to just revert the offending topic for now, but later accept a resurrection patch with this isatty check rolled in (either at this caller, or inside save_term) when the dust settles. Thanks.
quoted hunk
Reported-by: Alexander Veit <redacted> Signed-off-by: Carlo Marcelo Arenas Belón <redacted> --- editor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)diff --git a/editor.c b/editor.c index 674309eed8..214e3834cb 100644 --- a/editor.c +++ b/editor.c@@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path, p.env = env; p.use_shell = 1; p.trace2_child_class = "editor"; - term_fail = save_term(1); + term_fail = isatty(1) ? save_term(1) : 1; if (start_command(&p) < 0) { if (!term_fail) restore_term();