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

From: Carlo Arenas <carenas@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, phillip.wood123@gmail.com,
	thomas.wolf@paranor.ch, Alexander Veit <alexander.veit@gmx.net>
Subject: Re: [PATCH] editor: only save (and restore) the terminal if using a tty
Date: Mon, 22 Nov 2021 15:39:12 -0800	[thread overview]
Message-ID: <CAPUEspgDafXHHPvzNijTsPsna76yE8W=JH-78LX3jyaieSmp0A@mail.gmail.com> (raw)
In-Reply-To: <xmqqa6hvbxob.fsf@gitster.g>

On Mon, Nov 22, 2021 at 3:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> 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?

yes, my plan was to minimize the impact of this bugfix by doing this
as narrow as possible, but you are correct that if we consider that
the only caller for  save_term() is in editor.c then it would had make
more sense to put it there to begin with and with supportability in
mind for the future; but save_term() is also called internally, and so
the logic would also propagate to other places that use
compat/terminal.c (like our fallback version of getpass() or `git add
-p`).

I should have mentioned though that a better fix was forthcoming, just
not with so little time before 2.34.1 gets released.

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

I indeed suggested[1] a revert but I wouldn't have proposed this
alternative if it wouldn't be done safely enough, agree though that
without a successful report that it fixes the problem by the reporter
(who has since moved[2] on to a different hack which was proposed by
Peff), a revert is always safer.

Carlo

[1] https://lore.kernel.org/git/xmqqilwjbyj4.fsf@gitster.g/T/#mad3fd4d0015ec939c1e0001444d5affd720d56b2
[2] https://git.eclipse.org/r/c/jgit/jgit/+/187938