Re: [PATCH] editor: only save (and restore) the terminal if using a tty

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2021, 23:03 UTC
Message-ID: <xmqqa6hvbxob.fsf@gitster.g>
In-Reply-To: <20211122222850.674-1-carenas@gmail.com>

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

Show 8 quoted lines

> 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.

Show 19 quoted lines

> Reported-by: Alexander Veit <alexander.veit@gmx.net>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  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();