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

From: Carlo Arenas <carenas@gmail.com>
Date: Nov 22, 2021, 23:39 UTC
Message-ID: <CAPUEspgDafXHHPvzNijTsPsna76yE8W=JH-78LX3jyaieSmp0A@mail.gmail.com>
In-Reply-To: <xmqqa6hvbxob.fsf@gitster.g>

On Mon, Nov 22, 2021 at 3:03 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

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