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